From: arend.vanspriel@broadcom•com (Arend Van Spriel)
To: linux-arm-kernel@lists•infradead.org
Subject: [linux-sunxi] Re: [PATCH 1/4] brcmfmac: Add brcm, nvram_file_name dt property
Date: Wed, 6 Jul 2016 21:19:41 +0200 [thread overview]
Message-ID: <bb1beb8d-652d-aa97-e24b-e3e27397dd2e@broadcom.com> (raw)
In-Reply-To: <3354017.XSBSmHHtrQ@wuerfel>
On 6-7-2016 15:42, Arnd Bergmann wrote:
> On Wednesday, July 6, 2016 10:08:55 AM CEST Arend Van Spriel wrote:
>> On Tue, Jul 5, 2016 at 3:43 PM, Arnd Bergmann <arnd@arndb•de> wrote:
>>> On Monday, July 4, 2016 8:36:05 PM CEST Arend van Spriel wrote:
>>>> On 04-07-16 16:54, Arnd Bergmann wrote:
>>>>> On Monday, July 4, 2016 11:08:38 AM CEST Arend Van Spriel wrote:
>>>>>
>>>>> In drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c I already see
>>>>> over a dozen different chips being supported, bcm4329 is only one of
>>>>> them. In particular, there seem to be some that have various modules:
>>>>>
>>>>> BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43241_CHIP_ID, 0x0000001F, 43241B0),
>>>>> BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43241_CHIP_ID, 0x00000020, 43241B4),
>>>>> BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43241_CHIP_ID, 0xFFFFFFC0, 43241B5),
>>>>>
>>>>> So if you have a bcm43241, that compatible string probably should
>>>>> include both brcm,bcm43241-b4-fmac and brcm,bcm43241-fmac, possibly also
>>>>> brcm,bcm4329-fmac, to show that it is a superset of the programming
>>>>> interface of that one.
>>>>
>>>> Hi Arnd,
>>>>
>>>> I have to disagree here. The compatible string "brcm,bcm4329-fmac" is
>>>> chosen as the bcm4329 chip was the first supported and we never added
>>>> others as there is no other programming required. For all supported
>>>> devices the same device tree properties apply and are handled the same.
>>>> As such there is no need to come up with a new compatible string.
>>>
>>> Generally speaking, the way that the compatible strings work is that you
>>> add a new one whenever you get a new piece of hardware, and you can leave
>>> the first one as a fallback so you don't have to change the driver.
>>>
>>> Adding the new string for the actual device is important though,
>>> since you might only discover later that they are not 100% compatible
>>> and that you in fact need to know the difference.
>>>
>>> For discoverable buses like sdio or usb, it may actually be better
>>> to not identify the device through the compatible property at all,
>>> and instead use a string that is generated from the actual identifier
>>> as the primary key, as e.g. documented in
>>> Documentation/devicetree/bindings/usb/usb-device.txt
>>
>> Well, that is my point. We do not need to identify the device here. We
>> can obtain that info, ie. chip id and revision, from the device itself
>> as our wifi chips have a discoverable AXI backplane. What is missing
>> is that we have no way to tell what board/module this device is
>> integrated on, which makes it impossible to select the correct nvram
>> file. The model property can fill that gap just nicely.
>
> We have multiple inconsistencies here, and it's not this driver that is
> particularly messed up, but I think using the model property here
> would make it worse:
>
> All existing uses of the model property in arch/arm/boot/dts and most of
> the ones in arch/powerpc/boot/dts are against the intended usage in
> one way or another, but adding different kind of incorrect usage won't
> improve that.
>
> The only way I can see the model property being used correctly would
> be to have it match the first entry in the compatible property, but
> that is completely redundant, so we tend to omit it, except for the
> root node in which it is required. For the root node however, the
> historic practice that has crept in on ARM is to put something completely
> different in there, which is a human-readable description of the
> machine rather than something we can use as a unique indentifier.
>
> I'd just consider the "model" property burned, and not use it for anything
> that doesn't already use it, just like we handle "device_type": a few
> things require it, nothing else should use it.
If that is the agreed approach in devicetree arena I am fine with it. I
have been unaware of this and just looked at the suggestion from Jonas
seeing a solution to the problem at hand.
> I agree with your point that the "compatible" property in case of brcmfmac
> is really odd because it is not required to identify the hardware when
> the SDIO device identification is sufficient, and we just need it to guard
> the definition of the other properties. However it seems that now we
> actually do need to identify the hardware because we cannot read the
> board ID and revision that is supposed to come from nvram but also needed
> to read the nvram contents from a file.
The board ID and rev in the nvram may not be used if the device has
these stored in OTP. However, the problem is that the device need
firmware+nvram loaded into it before we can start the firmware on the
device. Now if we were to add boardtype and boardrev properties in the
binding to identify the hardware, I suppose a new compatible value would
be required.
Regards,
Arend
next prev parent reply other threads:[~2016-07-06 19:19 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-29 14:04 [PATCH 1/4] brcmfmac: Add brcm,nvram_file_name dt property Hans de Goede
2016-06-29 14:04 ` [PATCH 2/4] ARM: dts: sun7i-a20-cubietruck: Set brcm,nvram_file_name Hans de Goede
2016-06-29 17:01 ` [PATCH 2/4] ARM: dts: sun7i-a20-cubietruck: Set brcm, nvram_file_name Kalle Valo
2016-06-29 18:01 ` [linux-sunxi] Re: [PATCH 2/4] ARM: dts: sun7i-a20-cubietruck: Set brcm,nvram_file_name Hans de Goede
2016-06-29 14:04 ` [PATCH 3/4] ARM: dts: sun7i-a20-wits-pro-a20-dkt: Set brcm, nvram_file_name Hans de Goede
2016-06-29 14:04 ` [PATCH 4/4] ARM: dts: sun5i-a10s-auxtek-t004: " Hans de Goede
2016-06-29 14:42 ` [PATCH 1/4] brcmfmac: Add brcm,nvram_file_name dt property Jonas Gorski
2016-06-29 15:16 ` Hans de Goede
2016-06-29 17:00 ` Kalle Valo
2016-06-29 18:01 ` [linux-sunxi] Re: [PATCH 1/4] brcmfmac: Add brcm, nvram_file_name " Hans de Goede
2016-06-29 18:51 ` Arend Van Spriel
2016-06-29 18:57 ` Arend Van Spriel
2016-06-30 8:50 ` Kalle Valo
2016-06-29 19:33 ` Arnd Bergmann
2016-06-29 19:54 ` [linux-sunxi] Re: [PATCH 1/4] brcmfmac: Add brcm,nvram_file_name " Priit Laes
2016-06-29 20:07 ` [linux-sunxi] Re: [PATCH 1/4] brcmfmac: Add brcm, nvram_file_name " Arnd Bergmann
2016-06-30 9:02 ` Kalle Valo
2016-06-30 9:50 ` Hans de Goede
2016-06-30 9:58 ` Kalle Valo
2016-06-30 10:04 ` Hans de Goede
2016-06-30 10:18 ` Jonas Gorski
2016-06-30 10:25 ` Hans de Goede
2016-06-30 11:31 ` Arnd Bergmann
2016-06-30 19:23 ` Arend Van Spriel
2016-07-01 8:51 ` Arnd Bergmann
2016-07-01 8:58 ` Jonas Gorski
2016-07-02 6:59 ` Kalle Valo
2016-07-02 18:20 ` Arend Van Spriel
2016-07-02 21:30 ` Arnd Bergmann
2016-07-04 8:41 ` Arend Van Spriel
2016-07-04 8:55 ` Arnd Bergmann
2016-07-04 9:08 ` Arend Van Spriel
2016-07-04 14:54 ` Arnd Bergmann
2016-07-04 18:36 ` Arend van Spriel
2016-07-05 13:43 ` Arnd Bergmann
2016-07-06 8:08 ` Arend Van Spriel
2016-07-06 13:42 ` Arnd Bergmann
2016-07-06 19:19 ` Arend Van Spriel [this message]
2016-07-07 8:46 ` Arnd Bergmann
2016-07-07 9:16 ` Arend Van Spriel
2016-07-07 9:24 ` Arnd Bergmann
2016-07-17 21:45 ` Rob Herring
2016-07-18 7:51 ` Arend Van Spriel
2016-06-30 8:46 ` Kalle Valo
2016-06-30 9:49 ` Hans de Goede
2016-06-30 9:53 ` Hans de Goede
2016-07-01 2:08 ` [PATCH 1/4] brcmfmac: Add brcm,nvram_file_name " Rob Herring
2016-07-01 8:17 ` Arend Van Spriel
2016-07-01 9:20 ` Arnd Bergmann
2016-07-04 16:12 ` Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bb1beb8d-652d-aa97-e24b-e3e27397dd2e@broadcom.com \
--to=arend.vanspriel@broadcom$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox