Add a brcm,nvram_file_name dt property to allow overruling the default
nvram filename for sdio devices. The idea is that we can specify a
board specific nvram file, e.g. brcmfmac43362-ap6210.txt for boards
with an ap6210 wifi sdio module and ship this in linux-firmware, so
that wifi will work out of the box, without requiring users to find
and then manually install the right nvram file for their board.
Signed-off-by: Hans de Goede <[email protected]>
---
.../devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 2 ++
drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c | 2 ++
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 6 ++++++
include/linux/platform_data/brcmfmac.h | 2 ++
4 files changed, 12 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
index 5dbf169..2ba13a6 100644
--- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
+++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
@@ -11,6 +11,7 @@ Required properties:
Optional properties:
- brcm,drive-strength : drive strength used for SDIO pins on device in mA
(default = 6).
+ - brcm,nvram_file_name : name of the nvram file to load
- interrupt-parent : the phandle for the interrupt controller to which the
device interrupts are connected.
- interrupts : specifies attributes for the out-of-band interrupt (host-wake).
@@ -34,6 +35,7 @@ mmc3: mmc@01c12000 {
brcmf: bcrmf@1 {
reg = <1>;
compatible = "brcm,bcm4329-fmac";
+ brcm,nvram_file_name = "brcm/brcmfmac43362-ap6210.txt";
interrupt-parent = <&pio>;
interrupts = <10 8>; /* PH10 / EINT10 */
interrupt-names = "host-wake";
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
index 425c41d..a054122 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
@@ -36,6 +36,8 @@ void brcmf_of_probe(struct device *dev, struct brcmfmac_sdio_pd *sdio)
if (of_property_read_u32(np, "brcm,drive-strength", &val) == 0)
sdio->drive_strength = val;
+ of_property_read_string(np, "brcm,nvram_file_name", &sdio->nvram_name);
+
/* make sure there are interrupts defined in the node */
if (!of_find_property(np, "interrupts", NULL))
return;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 67e69bf..2655409 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -4201,6 +4201,12 @@ struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
if (ret)
goto fail;
+ if (sdiodev->settings->bus.sdio.nvram_name) {
+ strlcpy(sdiodev->nvram_name,
+ sdiodev->settings->bus.sdio.nvram_name,
+ BRCMF_FW_NAME_LEN);
+ }
+
ret = brcmf_fw_get_firmwares(sdiodev->dev, BRCMF_FW_REQUEST_NVRAM,
sdiodev->fw_name, sdiodev->nvram_name,
brcmf_sdio_firmware_callback);
diff --git a/include/linux/platform_data/brcmfmac.h b/include/linux/platform_data/brcmfmac.h
index 1d30bf2..a5515dd 100644
--- a/include/linux/platform_data/brcmfmac.h
+++ b/include/linux/platform_data/brcmfmac.h
@@ -65,6 +65,7 @@ enum brcmf_bus_type {
* the target drive strength, the exact drive strength
* which will be used depends on the capabilities of the
* device.
+ * @nvram_name: name of nvram file to load.
* @oob_irq_supported: does the board have support for OOB interrupts. SDIO
* in-band interrupts are relatively slow and for having
* less overhead on interrupt processing an out of band
@@ -91,6 +92,7 @@ enum brcmf_bus_type {
struct brcmfmac_sdio_pd {
int txglomsz;
unsigned int drive_strength;
+ const char *nvram_name;
bool oob_irq_supported;
unsigned int oob_irq_nr;
unsigned long oob_irq_flags;
--
2.7.4
Hans de Goede <[email protected]> writes:
> The cubietruck uses an ap6210 sdio wifi module, set brcm,nvram_file_name
> to brcmfmac43362-ap6210.txt so that we can ship the ap6210 specific
> nvram file in linunx-firmware to get the wifi to work out of the box
> without users needing to download and install the nvram file themselves.
>
> Note a downside of this patch is that users who have already manually
> installed the nvram file will need to rename it.
>
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 1 +
> 1 file changed, 1 insertion(+)
I can't apply .dts changes so better to send them in a separate
patchset.
--
Kalle Valo
Hi,
On 29-06-16 16:42, Jonas Gorski wrote:
> Hi,
>
> On 29 June 2016 at 16:04, Hans de Goede <[email protected]> wrote:
>> Add a brcm,nvram_file_name dt property to allow overruling the default
>> nvram filename for sdio devices. The idea is that we can specify a
>> board specific nvram file, e.g. brcmfmac43362-ap6210.txt for boards
>> with an ap6210 wifi sdio module and ship this in linux-firmware, so
>> that wifi will work out of the box, without requiring users to find
>> and then manually install the right nvram file for their board.
>
> Directly defining a filename doesn't seem like a good OS-agnostic
> approach. Maybe an alternative would be to add a model-property to the
> nodes (this is allowed) and make brcmfmac to request
> "FWFILENAME-<model>" as firmware if set? That would leave it to the OS
> on how the filename is set.
It only defines the base-filename, not the entire path, how / where
this file is searched for / loaded-from is then left up to the os
Regards,
Hans
Hi,
On 30 June 2016 at 12:04, Hans de Goede <[email protected]> wrote:
> Hi,
>
>
> On 30-06-16 11:58, Kalle Valo wrote:
>>
>> Hans de Goede <[email protected]> writes:
>>
>>> Hi,
>>>
>>> On 30-06-16 11:02, Kalle Valo wrote:
>>>>
>>>> Priit Laes <[email protected]> writes:
>>>>
>>>>>> What is the size of this nvram file? As it's board specific, I wonder
>>>>>> if we can simply include it inside of the DT verbatim. I remember
>>>>>> doing that (in the pre-dtb days, on real open firmware) for the
>>>>>> "spidernet"
>>>>>> ethernet driver.
>>>>>
>>>>>
>>>>> It contains a bit too much info:
>>>>>
>>>>> This is what CubieTruck requires:
>>>>>
>>>>>
>>>>> http://dl.cubieboard.org/public/Cubieboard/benn/firmware/ap6210/nvram_ap6210.txt
>>>>
>>>>
>>>> In the nvram file I see lots of identifiers:
>>>>
>>>> manfid=0x2d0
>>>> prodid=0x492
>>>> vendid=0x14e4
>>>> devid=0x4343
>>>> boardtype=0x0598
>>>> boardrev=0x1307
>>>> boardnum=777
>>>>
>>>> Are any of these (or a combination of them) unique for each board type?
>>>> What if one of these is provided through DT and then the driver could
>>>> choose the nvram file based on the board id? Just another idea...
>>>
>>>
>>> That would require updating a table in the driver every time a new
>>> board comes out, I do not believe that that is a good solution.
>>
>>
>> You don't necessarily need to have a table in the driver if you embed
>> the id to the filename, for example something like this:
>>
>> nvram-0598-1307.txt
>> nvram-<boardtype>-<boardrev>.txt
>
>
> Downside of this is, that as Arend already said, the nvram files are not
> readily available.
>
> Typically the boards we are talking about are shipped with android,
> and the mainline kernel bringup is done by a user, not the manufacturer.
>
> So the nvram file needs to be extracted from e.g. an android image,
> having ap6210 in the filename allows the user to see that his module
> (which is clearly labelled ap6210 is already supported, where as the
> boardtype/boardrev magic numbers are a big unknown.
>
> So I believe that using a human readable string is better here.
So then how about making use of a more specific compatible string?
e.g.
brcmf {
compatible = "foo,ap6210", "brcm,bcm4329-fmac";
...
};
and if the compatible has more than one element you request
FW_NAME_<compatible>.txt as the nvram file. Or try each comptible (and
lastly no suffix) until you get a match. (AFAICT, this is what the
"model" property was originally intended for anyway, but almost nobody
did it right, and everyone put a user readable string into "model" for
boards instead of the ePAPR defined compatible string).
Regards
Jonas
HI,
On 29-06-16 19:01, Kalle Valo wrote:
> Hans de Goede <[email protected]> writes:
>
>> The cubietruck uses an ap6210 sdio wifi module, set brcm,nvram_file_name
>> to brcmfmac43362-ap6210.txt so that we can ship the ap6210 specific
>> nvram file in linunx-firmware to get the wifi to work out of the box
>> without users needing to download and install the nvram file themselves.
>>
>> Note a downside of this patch is that users who have already manually
>> installed the nvram file will need to rename it.
>>
>> Signed-off-by: Hans de Goede <[email protected]>
>> ---
>> arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 1 +
>> 1 file changed, 1 insertion(+)
>
> I can't apply .dts changes so better to send them in a separate
> patchset.
Maxime Ripard who is in the Cc maintains the dts files
in question, I've send these as one set because I find
that sending out actual dts usage examples helps.
Regards,
Hans
Hi,
On 29 June 2016 at 16:04, Hans de Goede <[email protected]> wrote:
> Add a brcm,nvram_file_name dt property to allow overruling the default
> nvram filename for sdio devices. The idea is that we can specify a
> board specific nvram file, e.g. brcmfmac43362-ap6210.txt for boards
> with an ap6210 wifi sdio module and ship this in linux-firmware, so
> that wifi will work out of the box, without requiring users to find
> and then manually install the right nvram file for their board.
Directly defining a filename doesn't seem like a good OS-agnostic
approach. Maybe an alternative would be to add a model-property to the
nodes (this is allowed) and make brcmfmac to request
"FWFILENAME-<model>" as firmware if set? That would leave it to the OS
on how the filename is set.
Regards
Jonas
The cubietruck uses an ap6210 sdio wifi module, set brcm,nvram_file_name
to brcmfmac43362-ap6210.txt so that we can ship the ap6210 specific
nvram file in linunx-firmware to get the wifi to work out of the box
without users needing to download and install the nvram file themselves.
Note a downside of this patch is that users who have already manually
installed the nvram file will need to rename it.
Signed-off-by: Hans de Goede <[email protected]>
---
arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index 83f39b0..fda1da4 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -199,6 +199,7 @@
brcmf: bcrmf@1 {
reg = <1>;
compatible = "brcm,bcm4329-fmac";
+ brcm,nvram_file_name = "brcm/brcmfmac43362-ap6210.txt";
interrupt-parent = <&pio>;
interrupts = <7 10 IRQ_TYPE_LEVEL_LOW>; /* PH10 / EINT10 */
interrupt-names = "host-wake";
--
2.7.4
Priit Laes <[email protected]> writes:
>> What is the size of this nvram file? As it's board specific, I wonder
>> if we can simply include it inside of the DT verbatim. I remember
>> doing that (in the pre-dtb days, on real open firmware) for the
>> "spidernet"
>> ethernet driver.
>
> It contains a bit too much info:
>
> This is what CubieTruck requires:
>
> http://dl.cubieboard.org/public/Cubieboard/benn/firmware/ap6210/nvram_ap6210.txt
In the nvram file I see lots of identifiers:
manfid=0x2d0
prodid=0x492
vendid=0x14e4
devid=0x4343
boardtype=0x0598
boardrev=0x1307
boardnum=777
Are any of these (or a combination of them) unique for each board type?
What if one of these is provided through DT and then the driver could
choose the nvram file based on the board id? Just another idea...
--
Kalle Valo
The wits pro uses an ap6476 sdio wifi module, we do not have a nvram file
for this, so set brcm,nvram_file_name to brcmfmac43362-ap6210.txt which
works fine for this wifi module.
Setting brcm,nvram_file_name allows us to ship the ap6210 specific nvram
file in linux-firmware to get the wifi to work out of the box without
users needing to download and install the nvram file themselves.
Signed-off-by: Hans de Goede <[email protected]>
---
arch/arm/boot/dts/sun7i-a20-wits-pro-a20-dkt.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/sun7i-a20-wits-pro-a20-dkt.dts b/arch/arm/boot/dts/sun7i-a20-wits-pro-a20-dkt.dts
index dc31d47..3c5455a9 100644
--- a/arch/arm/boot/dts/sun7i-a20-wits-pro-a20-dkt.dts
+++ b/arch/arm/boot/dts/sun7i-a20-wits-pro-a20-dkt.dts
@@ -140,6 +140,7 @@
brcmf: bcrmf@1 {
reg = <1>;
compatible = "brcm,bcm4329-fmac";
+ brcm,nvram_file_name = "brcm/brcmfmac43362-ap6210.txt";
interrupt-parent = <&pio>;
interrupts = <7 10 IRQ_TYPE_LEVEL_LOW>; /* PH10 / EINT10 */
interrupt-names = "host-wake";
--
2.7.4
Hi,
On 30-06-16 10:46, Kalle Valo wrote:
> Arend Van Spriel <[email protected]> writes:
>
>>> Since we are dealing with a per-board config-file here, which is
>>> loaded from the os filesystem we really need to specify a basename
>>> here as the list of possible boards is endless, so we cannot
>>> have a lookup table in the driver.
>>
>> As Jonas mentioned the general principle of device tree is to be
>> agnostic with regards to OS and/or driver as you undoubtedly know. His
>> proposal seems like a usable solution for your problem while complying
>> to the device tree principle. So instead of overriding the default
>> brcmfmac should modify it when dt specifies the "module" property, ie:
>>
>> no "module" in DT: nvram filename = brcm/brcmfmac43362-sdio.txt
>> "module=ap6210" in DT: nvram filename = brcm/brcmfmac43362-ap6210.txt
>
> Just out of curiosity, what does "ap6210" exactly mean? I get that 43362
> is the chip id, but not ap6210. Is it just an arbitrary name?
It is more or less an arbitrary name, typically these sdio wifi chips
are used as a pre-assembled module (a tiny pcb) with some things like
the necessary crystal / resonator and various capacitors and resistors
on there. The brcm based sdio wifi modules typically come with a metal
shield cap which has a module type stamped on it, e.g. ap6210,
ap6476, toc9002.
Regards,
Hans
On Wed, 2016-06-29 at 21:33 +0200, Arnd Bergmann wrote:
> On Wednesday, June 29, 2016 8:51:44 PM CEST Arend Van Spriel wrote:
> > > Typical wifi devices will have some sort of non volatile storage
> > > on board to not only store the ethernet(mac) address, but also
> > > to contain e.g. info about the antenna gain so that the firmware
> > > and/or the driver can take the antenna gain into account and
> > > ensure
> > > that they never exceed the maximum allowed broadcast strength.
> > >
> > > However on some embedded devices there is no non-volatile storage
> > > for the wifi (for cost reasons) and instead this configuration
> > > info
> > > (which is board / pcb specific) is loaded in the form of a
> > > file which contains the contents which would normally be in the
> > > non-volatile storage.
> > >
> > > Since we are dealing with a per-board config-file here, which is
> > > loaded from the os filesystem we really need to specify a
> > > basename
> > > here as the list of possible boards is endless, so we cannot
> > > have a lookup table in the driver.
> >
> > As Jonas mentioned the general principle of device tree is to be
> > agnostic with regards to OS and/or driver as you undoubtedly know.
> > His
> > proposal seems like a usable solution for your problem while
> > complying
> > to the device tree principle. So instead of overriding the default
> > brcmfmac should modify it when dt specifies the "module" property,
> > ie:
> >
> > no "module" in DT: nvram filename = brcm/brcmfmac43362-
> > sdio.txt
> > "module=ap6210" in DT: nvram filename = brcm/brcmfmac43362-
> > ap6210.txt
> >
> > By the way, the example in the bindings file does not seem to
> > specify a
> > basename, but path+basename+fileext.
>
> What is the size of this nvram file? As it's board specific, I wonder
> if we can simply include it inside of the DT verbatim. I remember
> doing that (in the pre-dtb days, on real open firmware) for the
> "spidernet"
> ethernet driver.
It contains a bit too much info:
This is what CubieTruck requires:
http://dl.cubieboard.org/public/Cubieboard/benn/firmware/ap6210/nvram_a
p6210.txt
The auxtek t004 uses a toc9002 sdio wifi module, the android nvram file
for this is for an older firmware which does not work with the mainline
kernel driver.
So we set brcm,nvram_file_name to brcmfmac43362-ap6210.txt which
works fine for this wifi module.
Setting brcm,nvram_file_name allows us to ship the ap6210 specific nvram
file in linux-firmware to get the wifi to work out of the box without
users needing to download and install the nvram file themselves.
Signed-off-by: Hans de Goede <[email protected]>
---
arch/arm/boot/dts/sun5i-a10s-auxtek-t004.dts | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/sun5i-a10s-auxtek-t004.dts b/arch/arm/boot/dts/sun5i-a10s-auxtek-t004.dts
index a790ec8..e15d1ef 100644
--- a/arch/arm/boot/dts/sun5i-a10s-auxtek-t004.dts
+++ b/arch/arm/boot/dts/sun5i-a10s-auxtek-t004.dts
@@ -118,6 +118,12 @@
non-removable;
cap-sdio-irq;
status = "okay";
+
+ brcmf: bcrmf@1 {
+ reg = <1>;
+ compatible = "brcm,bcm4329-fmac";
+ brcm,nvram_file_name = "brcm/brcmfmac43362-ap6210.txt";
+ };
};
&ohci0 {
--
2.7.4
Hans de Goede <[email protected]> writes:
> Hi,
>
> On 30-06-16 11:02, Kalle Valo wrote:
>> Priit Laes <[email protected]> writes:
>>
>>>> What is the size of this nvram file? As it's board specific, I wonder
>>>> if we can simply include it inside of the DT verbatim. I remember
>>>> doing that (in the pre-dtb days, on real open firmware) for the
>>>> "spidernet"
>>>> ethernet driver.
>>>
>>> It contains a bit too much info:
>>>
>>> This is what CubieTruck requires:
>>>
>>> http://dl.cubieboard.org/public/Cubieboard/benn/firmware/ap6210/nvram_ap6210.txt
>>
>> In the nvram file I see lots of identifiers:
>>
>> manfid=0x2d0
>> prodid=0x492
>> vendid=0x14e4
>> devid=0x4343
>> boardtype=0x0598
>> boardrev=0x1307
>> boardnum=777
>>
>> Are any of these (or a combination of them) unique for each board type?
>> What if one of these is provided through DT and then the driver could
>> choose the nvram file based on the board id? Just another idea...
>
> That would require updating a table in the driver every time a new
> board comes out, I do not believe that that is a good solution.
You don't necessarily need to have a table in the driver if you embed
the id to the filename, for example something like this:
nvram-0598-1307.txt
nvram-<boardtype>-<boardrev>.txt
--
Kalle Valo
On Wednesday, June 29, 2016 8:51:44 PM CEST Arend Van Spriel wrote:
> > Typical wifi devices will have some sort of non volatile storage
> > on board to not only store the ethernet(mac) address, but also
> > to contain e.g. info about the antenna gain so that the firmware
> > and/or the driver can take the antenna gain into account and ensure
> > that they never exceed the maximum allowed broadcast strength.
> >
> > However on some embedded devices there is no non-volatile storage
> > for the wifi (for cost reasons) and instead this configuration info
> > (which is board / pcb specific) is loaded in the form of a
> > file which contains the contents which would normally be in the
> > non-volatile storage.
> >
> > Since we are dealing with a per-board config-file here, which is
> > loaded from the os filesystem we really need to specify a basename
> > here as the list of possible boards is endless, so we cannot
> > have a lookup table in the driver.
>
> As Jonas mentioned the general principle of device tree is to be
> agnostic with regards to OS and/or driver as you undoubtedly know. His
> proposal seems like a usable solution for your problem while complying
> to the device tree principle. So instead of overriding the default
> brcmfmac should modify it when dt specifies the "module" property, ie:
>
> no "module" in DT: nvram filename = brcm/brcmfmac43362-sdio.txt
> "module=ap6210" in DT: nvram filename = brcm/brcmfmac43362-ap6210.txt
>
> By the way, the example in the bindings file does not seem to specify a
> basename, but path+basename+fileext.
What is the size of this nvram file? As it's board specific, I wonder
if we can simply include it inside of the DT verbatim. I remember
doing that (in the pre-dtb days, on real open firmware) for the "spidernet"
ethernet driver.
Arnd
Hi,
On 30-06-16 11:58, Kalle Valo wrote:
> Hans de Goede <[email protected]> writes:
>
>> Hi,
>>
>> On 30-06-16 11:02, Kalle Valo wrote:
>>> Priit Laes <[email protected]> writes:
>>>
>>>>> What is the size of this nvram file? As it's board specific, I wonder
>>>>> if we can simply include it inside of the DT verbatim. I remember
>>>>> doing that (in the pre-dtb days, on real open firmware) for the
>>>>> "spidernet"
>>>>> ethernet driver.
>>>>
>>>> It contains a bit too much info:
>>>>
>>>> This is what CubieTruck requires:
>>>>
>>>> http://dl.cubieboard.org/public/Cubieboard/benn/firmware/ap6210/nvram_ap6210.txt
>>>
>>> In the nvram file I see lots of identifiers:
>>>
>>> manfid=0x2d0
>>> prodid=0x492
>>> vendid=0x14e4
>>> devid=0x4343
>>> boardtype=0x0598
>>> boardrev=0x1307
>>> boardnum=777
>>>
>>> Are any of these (or a combination of them) unique for each board type?
>>> What if one of these is provided through DT and then the driver could
>>> choose the nvram file based on the board id? Just another idea...
>>
>> That would require updating a table in the driver every time a new
>> board comes out, I do not believe that that is a good solution.
>
> You don't necessarily need to have a table in the driver if you embed
> the id to the filename, for example something like this:
>
> nvram-0598-1307.txt
> nvram-<boardtype>-<boardrev>.txt
Downside of this is, that as Arend already said, the nvram files are not
readily available.
Typically the boards we are talking about are shipped with android,
and the mainline kernel bringup is done by a user, not the manufacturer.
So the nvram file needs to be extracted from e.g. an android image,
having ap6210 in the filename allows the user to see that his module
(which is clearly labelled ap6210 is already supported, where as the
boardtype/boardrev magic numbers are a big unknown.
So I believe that using a human readable string is better here.
Regards,
Hans
On 29-6-2016 20:51, Arend Van Spriel wrote:
> On 29-6-2016 20:01, Hans de Goede wrote:
>> Hi,
>>
>> On 29-06-16 19:00, Kalle Valo wrote:
>>> Hans de Goede <[email protected]> writes:
>>>
>>>> Hi,
>>>>
>>>> On 29-06-16 16:42, Jonas Gorski wrote:
>>>>> Hi,
>>>>>
>>>>> On 29 June 2016 at 16:04, Hans de Goede <[email protected]> wrote:
>>>>>> Add a brcm,nvram_file_name dt property to allow overruling the default
>>>>>> nvram filename for sdio devices. The idea is that we can specify a
>>>>>> board specific nvram file, e.g. brcmfmac43362-ap6210.txt for boards
>>>>>> with an ap6210 wifi sdio module and ship this in linux-firmware, so
>>>>>> that wifi will work out of the box, without requiring users to find
>>>>>> and then manually install the right nvram file for their board.
>>>>>
>>>>> Directly defining a filename doesn't seem like a good OS-agnostic
>>>>> approach. Maybe an alternative would be to add a model-property to the
>>>>> nodes (this is allowed) and make brcmfmac to request
>>>>> "FWFILENAME-<model>" as firmware if set? That would leave it to the OS
>>>>> on how the filename is set.
>>>>
>>>> It only defines the base-filename, not the entire path, how / where
>>>> this file is searched for / loaded-from is then left up to the os
>>>
>>> It's still a bad idea. The filename, including the path, should be
>>> created in the driver. Can't you provide chipname (or similar) via
>>> device tree and then the driver can choose what image to use?
>>
>> No, the driver already does that, but this is not ...
>>
>>> Can you tell more about the naming the firmware image, how does it work
>>> exactly?
>>
>> About firmware, this is about the nvram file which is board specific,
>> rather then chip specific.
>
> The nvram filename is determined pretty much the same as the firmware
> filename, but indeed the nvram data is board specific. This is main
> reason why we do not submit nvram files to linux-firmware, because we
> simply do not have those files.
>
>> Typical wifi devices will have some sort of non volatile storage
>> on board to not only store the ethernet(mac) address, but also
>> to contain e.g. info about the antenna gain so that the firmware
>> and/or the driver can take the antenna gain into account and ensure
>> that they never exceed the maximum allowed broadcast strength.
>>
>> However on some embedded devices there is no non-volatile storage
>> for the wifi (for cost reasons) and instead this configuration info
>> (which is board / pcb specific) is loaded in the form of a
>> file which contains the contents which would normally be in the
>> non-volatile storage.
>>
>> Since we are dealing with a per-board config-file here, which is
>> loaded from the os filesystem we really need to specify a basename
>> here as the list of possible boards is endless, so we cannot
>> have a lookup table in the driver.
As a note: For BT Marcel was playing with the idea of having a lookup
table on the file system which would be loaded by the driver.
> As Jonas mentioned the general principle of device tree is to be
> agnostic with regards to OS and/or driver as you undoubtedly know. His
> proposal seems like a usable solution for your problem while complying
> to the device tree principle. So instead of overriding the default
> brcmfmac should modify it when dt specifies the "module" property, ie:
>
> no "module" in DT: nvram filename = brcm/brcmfmac43362-sdio.txt
> "module=ap6210" in DT: nvram filename = brcm/brcmfmac43362-ap6210.txt
>
> By the way, the example in the bindings file does not seem to specify a
> basename, but path+basename+fileext.
Hans,
Another btw: Kalle has taken over maintainer job from John.
Regards,
Arend
On 30-6-2016 13:31, Arnd Bergmann wrote:
> On Thursday, June 30, 2016 12:25:15 PM CEST Hans de Goede wrote:
>>> So then how about making use of a more specific compatible string?
>>>
>>> e.g.
>>>
>>> brcmf {
>>> compatible = "foo,ap6210", "brcm,bcm4329-fmac";
>>> ...
>>> };
>>>
>>> and if the compatible has more than one element you request
>>> FW_NAME_<compatible>.txt as the nvram file. Or try each comptible (and
>>> lastly no suffix) until you get a match. (AFAICT, this is what the
>>> "model" property was originally intended for anyway, but almost nobody
>>> did it right, and everyone put a user readable string into "model" for
>>> boards instead of the ePAPR defined compatible string).
>>
>> Hmm, interesting idea. Not sure how easy / hard it will be to implement
>> this, but from a dt binding point of view it seems elegant.
>>
>> Kalle, Arend, what do you think of this ?
At first glance I like the suggestion, but this would mean updating the
bindings document for each new wifi module that we want to add. Not a
big problem, but it makes that I have a slight preference to using a
property for it, eg. brcm,module = "ap6210";
> I think that's reasonable. Also, we have precedent for using things like
> the boardid as part of the compatible string, which would help do what
> Kalle suggested earlier with "nvram-<boardtype>-<boardrev>.txt",
> as we get that for free by trying out all the compatible strings.
The suggestion from Kalle was to use the field in the nvram file, but
things are a bit more complicated. The fields are only used if it is not
stored on the device in OTP or SROM. The 43362 does not have a SROM, but
it still has a small OTP storage if I am not mistaken. The brcmfmac
exposes a revinfo file in debugfs containing boardtype and boardrev, but
that is after starting the firmware. Still worth to check if those
values match the values in the ap6210 nvram file.
Regards,
Arend
Hi,
On 30-06-16 12:18, Jonas Gorski wrote:
> Hi,
>
> On 30 June 2016 at 12:04, Hans de Goede <[email protected]> wrote:
>> Hi,
>>
>>
>> On 30-06-16 11:58, Kalle Valo wrote:
>>>
>>> Hans de Goede <[email protected]> writes:
>>>
>>>> Hi,
>>>>
>>>> On 30-06-16 11:02, Kalle Valo wrote:
>>>>>
>>>>> Priit Laes <[email protected]> writes:
>>>>>
>>>>>>> What is the size of this nvram file? As it's board specific, I wonder
>>>>>>> if we can simply include it inside of the DT verbatim. I remember
>>>>>>> doing that (in the pre-dtb days, on real open firmware) for the
>>>>>>> "spidernet"
>>>>>>> ethernet driver.
>>>>>>
>>>>>>
>>>>>> It contains a bit too much info:
>>>>>>
>>>>>> This is what CubieTruck requires:
>>>>>>
>>>>>>
>>>>>> http://dl.cubieboard.org/public/Cubieboard/benn/firmware/ap6210/nvram_ap6210.txt
>>>>>
>>>>>
>>>>> In the nvram file I see lots of identifiers:
>>>>>
>>>>> manfid=0x2d0
>>>>> prodid=0x492
>>>>> vendid=0x14e4
>>>>> devid=0x4343
>>>>> boardtype=0x0598
>>>>> boardrev=0x1307
>>>>> boardnum=777
>>>>>
>>>>> Are any of these (or a combination of them) unique for each board type?
>>>>> What if one of these is provided through DT and then the driver could
>>>>> choose the nvram file based on the board id? Just another idea...
>>>>
>>>>
>>>> That would require updating a table in the driver every time a new
>>>> board comes out, I do not believe that that is a good solution.
>>>
>>>
>>> You don't necessarily need to have a table in the driver if you embed
>>> the id to the filename, for example something like this:
>>>
>>> nvram-0598-1307.txt
>>> nvram-<boardtype>-<boardrev>.txt
>>
>>
>> Downside of this is, that as Arend already said, the nvram files are not
>> readily available.
>>
>> Typically the boards we are talking about are shipped with android,
>> and the mainline kernel bringup is done by a user, not the manufacturer.
>>
>> So the nvram file needs to be extracted from e.g. an android image,
>> having ap6210 in the filename allows the user to see that his module
>> (which is clearly labelled ap6210 is already supported, where as the
>> boardtype/boardrev magic numbers are a big unknown.
>>
>> So I believe that using a human readable string is better here.
>
> So then how about making use of a more specific compatible string?
>
> e.g.
>
> brcmf {
> compatible = "foo,ap6210", "brcm,bcm4329-fmac";
> ...
> };
>
> and if the compatible has more than one element you request
> FW_NAME_<compatible>.txt as the nvram file. Or try each comptible (and
> lastly no suffix) until you get a match. (AFAICT, this is what the
> "model" property was originally intended for anyway, but almost nobody
> did it right, and everyone put a user readable string into "model" for
> boards instead of the ePAPR defined compatible string).
Hmm, interesting idea. Not sure how easy / hard it will be to implement
this, but from a dt binding point of view it seems elegant.
Kalle, Arend, what do you think of this ?
Regards,
Hans
Hans de Goede <[email protected]> writes:
> Hi,
>
> On 29-06-16 16:42, Jonas Gorski wrote:
>> Hi,
>>
>> On 29 June 2016 at 16:04, Hans de Goede <[email protected]> wrote:
>>> Add a brcm,nvram_file_name dt property to allow overruling the default
>>> nvram filename for sdio devices. The idea is that we can specify a
>>> board specific nvram file, e.g. brcmfmac43362-ap6210.txt for boards
>>> with an ap6210 wifi sdio module and ship this in linux-firmware, so
>>> that wifi will work out of the box, without requiring users to find
>>> and then manually install the right nvram file for their board.
>>
>> Directly defining a filename doesn't seem like a good OS-agnostic
>> approach. Maybe an alternative would be to add a model-property to the
>> nodes (this is allowed) and make brcmfmac to request
>> "FWFILENAME-<model>" as firmware if set? That would leave it to the OS
>> on how the filename is set.
>
> It only defines the base-filename, not the entire path, how / where
> this file is searched for / loaded-from is then left up to the os
It's still a bad idea. The filename, including the path, should be
created in the driver. Can't you provide chipname (or similar) via
device tree and then the driver can choose what image to use?
Can you tell more about the naming the firmware image, how does it work
exactly?
--
Kalle Valo
Arend Van Spriel <[email protected]> writes:
>> Since we are dealing with a per-board config-file here, which is
>> loaded from the os filesystem we really need to specify a basename
>> here as the list of possible boards is endless, so we cannot
>> have a lookup table in the driver.
>
> As Jonas mentioned the general principle of device tree is to be
> agnostic with regards to OS and/or driver as you undoubtedly know. His
> proposal seems like a usable solution for your problem while complying
> to the device tree principle. So instead of overriding the default
> brcmfmac should modify it when dt specifies the "module" property, ie:
>
> no "module" in DT: nvram filename = brcm/brcmfmac43362-sdio.txt
> "module=ap6210" in DT: nvram filename = brcm/brcmfmac43362-ap6210.txt
Just out of curiosity, what does "ap6210" exactly mean? I get that 43362
is the chip id, but not ap6210. Is it just an arbitrary name?
--
Kalle Valo
On 29-6-2016 20:01, Hans de Goede wrote:
> Hi,
>
> On 29-06-16 19:00, Kalle Valo wrote:
>> Hans de Goede <[email protected]> writes:
>>
>>> Hi,
>>>
>>> On 29-06-16 16:42, Jonas Gorski wrote:
>>>> Hi,
>>>>
>>>> On 29 June 2016 at 16:04, Hans de Goede <[email protected]> wrote:
>>>>> Add a brcm,nvram_file_name dt property to allow overruling the default
>>>>> nvram filename for sdio devices. The idea is that we can specify a
>>>>> board specific nvram file, e.g. brcmfmac43362-ap6210.txt for boards
>>>>> with an ap6210 wifi sdio module and ship this in linux-firmware, so
>>>>> that wifi will work out of the box, without requiring users to find
>>>>> and then manually install the right nvram file for their board.
>>>>
>>>> Directly defining a filename doesn't seem like a good OS-agnostic
>>>> approach. Maybe an alternative would be to add a model-property to the
>>>> nodes (this is allowed) and make brcmfmac to request
>>>> "FWFILENAME-<model>" as firmware if set? That would leave it to the OS
>>>> on how the filename is set.
>>>
>>> It only defines the base-filename, not the entire path, how / where
>>> this file is searched for / loaded-from is then left up to the os
>>
>> It's still a bad idea. The filename, including the path, should be
>> created in the driver. Can't you provide chipname (or similar) via
>> device tree and then the driver can choose what image to use?
>
> No, the driver already does that, but this is not ...
>
>> Can you tell more about the naming the firmware image, how does it work
>> exactly?
>
> About firmware, this is about the nvram file which is board specific,
> rather then chip specific.
The nvram filename is determined pretty much the same as the firmware
filename, but indeed the nvram data is board specific. This is main
reason why we do not submit nvram files to linux-firmware, because we
simply do not have those files.
> Typical wifi devices will have some sort of non volatile storage
> on board to not only store the ethernet(mac) address, but also
> to contain e.g. info about the antenna gain so that the firmware
> and/or the driver can take the antenna gain into account and ensure
> that they never exceed the maximum allowed broadcast strength.
>
> However on some embedded devices there is no non-volatile storage
> for the wifi (for cost reasons) and instead this configuration info
> (which is board / pcb specific) is loaded in the form of a
> file which contains the contents which would normally be in the
> non-volatile storage.
>
> Since we are dealing with a per-board config-file here, which is
> loaded from the os filesystem we really need to specify a basename
> here as the list of possible boards is endless, so we cannot
> have a lookup table in the driver.
As Jonas mentioned the general principle of device tree is to be
agnostic with regards to OS and/or driver as you undoubtedly know. His
proposal seems like a usable solution for your problem while complying
to the device tree principle. So instead of overriding the default
brcmfmac should modify it when dt specifies the "module" property, ie:
no "module" in DT: nvram filename = brcm/brcmfmac43362-sdio.txt
"module=ap6210" in DT: nvram filename = brcm/brcmfmac43362-ap6210.txt
By the way, the example in the bindings file does not seem to specify a
basename, but path+basename+fileext.
Regards,
Arend
On Wednesday, June 29, 2016 10:54:38 PM CEST Priit Laes wrote:
> On Wed, 2016-06-29 at 21:33 +0200, Arnd Bergmann wrote:
> > What is the size of this nvram file? As it's board specific, I wonder
> > if we can simply include it inside of the DT verbatim. I remember
> > doing that (in the pre-dtb days, on real open firmware) for the
> > "spidernet"
> > ethernet driver.
>
> It contains a bit too much info:
>
> This is what CubieTruck requires:
>
> http://dl.cubieboard.org/public/Cubieboard/benn/firmware/ap6210/nvram_a
> p6210.txt
Ah, I had not realized that this is a text based interface rather than
a small binary blob with fixed offsets.
On the other hand, each line in there could be translated easily into
a separate DT property, and some of them (manfid/prodid, macaddr, ...)
look like they directly correspond to properties we already have.
As Arend said there is also the option of having a chip specific
nvram file (brcmfmac43362-sdio.txt) as a fallback when there is no
more specific module. How many of the lines in your example would
actually differ between the two? Does this affect all of them, or
just a subset that could be turned into a smaller set of DT
properties?
Arnd
Hi,
On 29-06-16 19:00, Kalle Valo wrote:
> Hans de Goede <[email protected]> writes:
>
>> Hi,
>>
>> On 29-06-16 16:42, Jonas Gorski wrote:
>>> Hi,
>>>
>>> On 29 June 2016 at 16:04, Hans de Goede <[email protected]> wrote:
>>>> Add a brcm,nvram_file_name dt property to allow overruling the default
>>>> nvram filename for sdio devices. The idea is that we can specify a
>>>> board specific nvram file, e.g. brcmfmac43362-ap6210.txt for boards
>>>> with an ap6210 wifi sdio module and ship this in linux-firmware, so
>>>> that wifi will work out of the box, without requiring users to find
>>>> and then manually install the right nvram file for their board.
>>>
>>> Directly defining a filename doesn't seem like a good OS-agnostic
>>> approach. Maybe an alternative would be to add a model-property to the
>>> nodes (this is allowed) and make brcmfmac to request
>>> "FWFILENAME-<model>" as firmware if set? That would leave it to the OS
>>> on how the filename is set.
>>
>> It only defines the base-filename, not the entire path, how / where
>> this file is searched for / loaded-from is then left up to the os
>
> It's still a bad idea. The filename, including the path, should be
> created in the driver. Can't you provide chipname (or similar) via
> device tree and then the driver can choose what image to use?
No, the driver already does that, but this is not ...
> Can you tell more about the naming the firmware image, how does it work
> exactly?
About firmware, this is about the nvram file which is board specific,
rather then chip specific.
Typical wifi devices will have some sort of non volatile storage
on board to not only store the ethernet(mac) address, but also
to contain e.g. info about the antenna gain so that the firmware
and/or the driver can take the antenna gain into account and ensure
that they never exceed the maximum allowed broadcast strength.
However on some embedded devices there is no non-volatile storage
for the wifi (for cost reasons) and instead this configuration info
(which is board / pcb specific) is loaded in the form of a
file which contains the contents which would normally be in the
non-volatile storage.
Since we are dealing with a per-board config-file here, which is
loaded from the os filesystem we really need to specify a basename
here as the list of possible boards is endless, so we cannot
have a lookup table in the driver.
Regards,
Hans
Hi,
On 30-06-16 11:02, Kalle Valo wrote:
> Priit Laes <[email protected]> writes:
>
>>> What is the size of this nvram file? As it's board specific, I wonder
>>> if we can simply include it inside of the DT verbatim. I remember
>>> doing that (in the pre-dtb days, on real open firmware) for the
>>> "spidernet"
>>> ethernet driver.
>>
>> It contains a bit too much info:
>>
>> This is what CubieTruck requires:
>>
>> http://dl.cubieboard.org/public/Cubieboard/benn/firmware/ap6210/nvram_ap6210.txt
>
> In the nvram file I see lots of identifiers:
>
> manfid=0x2d0
> prodid=0x492
> vendid=0x14e4
> devid=0x4343
> boardtype=0x0598
> boardrev=0x1307
> boardnum=777
>
> Are any of these (or a combination of them) unique for each board type?
> What if one of these is provided through DT and then the driver could
> choose the nvram file based on the board id? Just another idea...
That would require updating a table in the driver every time a new
board comes out, I do not believe that that is a good solution.
Regards,
Hans
Arend Van Spriel <[email protected]> writes:
>>> Typical wifi devices will have some sort of non volatile storage
>>> on board to not only store the ethernet(mac) address, but also
>>> to contain e.g. info about the antenna gain so that the firmware
>>> and/or the driver can take the antenna gain into account and ensure
>>> that they never exceed the maximum allowed broadcast strength.
>>>
>>> However on some embedded devices there is no non-volatile storage
>>> for the wifi (for cost reasons) and instead this configuration info
>>> (which is board / pcb specific) is loaded in the form of a
>>> file which contains the contents which would normally be in the
>>> non-volatile storage.
>>>
>>> Since we are dealing with a per-board config-file here, which is
>>> loaded from the os filesystem we really need to specify a basename
>>> here as the list of possible boards is endless, so we cannot
>>> have a lookup table in the driver.
>
> As a note: For BT Marcel was playing with the idea of having a lookup
> table on the file system which would be loaded by the driver.
In ath10k we have a similar problem but in our case we can use the
subsystem id to detect what board file to use, so it's not exactly same
as yours. In our board-2.bin we have identifiers like this from which
ath10k selects the correct board file:
bus=pci,vendor=168c,device=003e,subsystem-vendor=168c,subsystem-device=334e
bus=pci,vendor=168c,device=003e,subsystem-vendor=168c,subsystem-device=3360
bus=pci,vendor=168c,device=003e,subsystem-vendor=168c,subsystem-device=3361
--
Kalle Valo
Hi,
On 29-06-16 20:51, 'Arend Van Spriel' via linux-sunxi wrote:
> On 29-6-2016 20:01, Hans de Goede wrote:
>> Hi,
>>
>> On 29-06-16 19:00, Kalle Valo wrote:
>>> Hans de Goede <[email protected]> writes:
>>>
>>>> Hi,
>>>>
>>>> On 29-06-16 16:42, Jonas Gorski wrote:
>>>>> Hi,
>>>>>
>>>>> On 29 June 2016 at 16:04, Hans de Goede <[email protected]> wrote:
>>>>>> Add a brcm,nvram_file_name dt property to allow overruling the default
>>>>>> nvram filename for sdio devices. The idea is that we can specify a
>>>>>> board specific nvram file, e.g. brcmfmac43362-ap6210.txt for boards
>>>>>> with an ap6210 wifi sdio module and ship this in linux-firmware, so
>>>>>> that wifi will work out of the box, without requiring users to find
>>>>>> and then manually install the right nvram file for their board.
>>>>>
>>>>> Directly defining a filename doesn't seem like a good OS-agnostic
>>>>> approach. Maybe an alternative would be to add a model-property to the
>>>>> nodes (this is allowed) and make brcmfmac to request
>>>>> "FWFILENAME-<model>" as firmware if set? That would leave it to the OS
>>>>> on how the filename is set.
>>>>
>>>> It only defines the base-filename, not the entire path, how / where
>>>> this file is searched for / loaded-from is then left up to the os
>>>
>>> It's still a bad idea. The filename, including the path, should be
>>> created in the driver. Can't you provide chipname (or similar) via
>>> device tree and then the driver can choose what image to use?
>>
>> No, the driver already does that, but this is not ...
>>
>>> Can you tell more about the naming the firmware image, how does it work
>>> exactly?
>>
>> About firmware, this is about the nvram file which is board specific,
>> rather then chip specific.
>
> The nvram filename is determined pretty much the same as the firmware
> filename, but indeed the nvram data is board specific. This is main
> reason why we do not submit nvram files to linux-firmware, because we
> simply do not have those files.
>
>> Typical wifi devices will have some sort of non volatile storage
>> on board to not only store the ethernet(mac) address, but also
>> to contain e.g. info about the antenna gain so that the firmware
>> and/or the driver can take the antenna gain into account and ensure
>> that they never exceed the maximum allowed broadcast strength.
>>
>> However on some embedded devices there is no non-volatile storage
>> for the wifi (for cost reasons) and instead this configuration info
>> (which is board / pcb specific) is loaded in the form of a
>> file which contains the contents which would normally be in the
>> non-volatile storage.
>>
>> Since we are dealing with a per-board config-file here, which is
>> loaded from the os filesystem we really need to specify a basename
>> here as the list of possible boards is endless, so we cannot
>> have a lookup table in the driver.
>
> As Jonas mentioned the general principle of device tree is to be
> agnostic with regards to OS and/or driver as you undoubtedly know. His
> proposal seems like a usable solution for your problem while complying
> to the device tree principle. So instead of overriding the default
> brcmfmac should modify it when dt specifies the "module" property, ie:
>
> no "module" in DT: nvram filename = brcm/brcmfmac43362-sdio.txt
> "module=ap6210" in DT: nvram filename = brcm/brcmfmac43362-ap6210.txt
Yes that seems like a good solution, I will send a v2 implementing this.
> By the way, the example in the bindings file does not seem to specify a
> basename, but path+basename+fileext.
fileext is always part of a file basename, but you're right that it
does include a relative path because of the way the existing firmware
files are organized under /lib/firmware under Linux and yes I'll admit
that that is a bit os specific :)
Regards,
Hans
On Thursday, June 30, 2016 12:25:15 PM CEST Hans de Goede wrote:
> > So then how about making use of a more specific compatible string?
> >
> > e.g.
> >
> > brcmf {
> > compatible = "foo,ap6210", "brcm,bcm4329-fmac";
> > ...
> > };
> >
> > and if the compatible has more than one element you request
> > FW_NAME_<compatible>.txt as the nvram file. Or try each comptible (and
> > lastly no suffix) until you get a match. (AFAICT, this is what the
> > "model" property was originally intended for anyway, but almost nobody
> > did it right, and everyone put a user readable string into "model" for
> > boards instead of the ePAPR defined compatible string).
>
> Hmm, interesting idea. Not sure how easy / hard it will be to implement
> this, but from a dt binding point of view it seems elegant.
>
> Kalle, Arend, what do you think of this ?
I think that's reasonable. Also, we have precedent for using things like
the boardid as part of the compatible string, which would help do what
Kalle suggested earlier with "nvram-<boardtype>-<boardrev>.txt",
as we get that for free by trying out all the compatible strings.
Arnd
On Wed, Jun 29, 2016 at 04:04:31PM +0200, Hans de Goede wrote:
> Add a brcm,nvram_file_name dt property to allow overruling the default
> nvram filename for sdio devices. The idea is that we can specify a
> board specific nvram file, e.g. brcmfmac43362-ap6210.txt for boards
> with an ap6210 wifi sdio module and ship this in linux-firmware, so
> that wifi will work out of the box, without requiring users to find
> and then manually install the right nvram file for their board.
What about putting its contents directly into DT? It's just text
key/value pairs so it would match up well.
Also, I have to wonder how all the non-SDIO based cards don't need this
file.
> Signed-off-by: Hans de Goede <[email protected]>
> ---
> .../devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 2 ++
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c | 2 ++
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 6 ++++++
> include/linux/platform_data/brcmfmac.h | 2 ++
> 4 files changed, 12 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> index 5dbf169..2ba13a6 100644
> --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> @@ -11,6 +11,7 @@ Required properties:
> Optional properties:
> - brcm,drive-strength : drive strength used for SDIO pins on device in mA
> (default = 6).
> + - brcm,nvram_file_name : name of the nvram file to load
The need for firmware file names has come up several times though
nothing merged to yet. There has been at least some level of agreement
to use "firmware-name" here.
> - interrupt-parent : the phandle for the interrupt controller to which the
> device interrupts are connected.
> - interrupts : specifies attributes for the out-of-band interrupt (host-wake).
> @@ -34,6 +35,7 @@ mmc3: mmc@01c12000 {
> brcmf: bcrmf@1 {
> reg = <1>;
> compatible = "brcm,bcm4329-fmac";
> + brcm,nvram_file_name = "brcm/brcmfmac43362-ap6210.txt";
> interrupt-parent = <&pio>;
> interrupts = <10 8>; /* PH10 / EINT10 */
> interrupt-names = "host-wake";
On Monday, July 4, 2016 11:08:38 AM CEST Arend Van Spriel wrote:
> On 4-7-2016 10:55, Arnd Bergmann wrote:
> > On Monday, July 4, 2016 10:41:20 AM CEST Arend Van Spriel wrote:
> >> On 2-7-2016 23:30, Arnd Bergmann wrote:
> >>> On Saturday, July 2, 2016 8:20:35 PM CEST Arend Van Spriel wrote:
> >>>>> If you want a separate property, then I repeat my very first
> >>>>> suggestion, the well defined model property.
> >>>>> e.g.
> >>>>>
> >>>>> brcmf@0 {
> >>>>> model = "ampak,ap6210";
> >>>>> compatible = "brcm,bcm4329-fmac";
> >>>>> ...
> >>>>> };
> >>>>>
> >>>>> All device nodes may have a model property, not just the top "machine" one.
> >>>>
> >>>> I heard you the first time I just was not sure what the implications
> >>>> would be to use it. Hence I suggested a vendor specific property.
> >>>> However, looking up and reading the definition in ePAPRv1.1 I suppose it
> >>>> is fine to use the model property:
> >>>>
> >>>> Property: model
> >>>> Value type: <string>
> >>>> Description:
> >>>> The model property value is a <string> that specifies the manufacturer’s
> >>>> model number of the device.
> >>>>
> >>>> The recommended format is: “manufacturer,model”, where manufacturer is a
> >>>> string describing the name of the manufacturer (such as a stock ticker
> >>>> symbol), and model specifies the model number.
> >>>
> >>> The model property is very similar to compatible, except that there is
> >>> only one entry rather than a list of entries from most specific to
> >>> most generic.
> >>
> >> They seem very similar, but I think there is a conceptual difference.
> >> The compatible property is mainly used to select the appropriate driver
> >> and as such the property is typically ignored by device drivers.
> >> Probably there are exceptions to be found.
> >>
> >>> I think by writing the above example as
> >>>
> >>> compatible = "ampak,ap6210", "brcm,bcm4329-fmac";
> >>>
> >>> we can provide the same functionality in a slightly simpler way, the driver
> >>> then just goes on to look for the nvram file for each entry in sequence until
> >>> it finds one.
> >>
> >> Not sure why this would be simpler. Why would traversing the compatible
> >> string be simpler than handling the model property if present and
> >> otherwise fallback to the default nvram naming.
> >
> > Because you have to walk the list anyway to find the other firmware files:
> > when you have a specialization of a device that requires listing both values
> > as compatible, the driver has no idea which of the entries to use, unless
> > you add a lookup table that adds more complexity.
>
> Currently, the brcmfmac bindings describe a single compatible string,
> ie. "brcm,bcm4329-fmac", which selects the driver/programming model. If
> that programming model supports "use model property if present,
> otherwise use default" there is nothing to traverse. The default way in
> the driver to determine firmware and nvram filename already has a lookup
> table which uses the chip id and chip revision as key, which are read
> from the device.
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.
Arnd
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
The mmc binding is less clear about that, and we may want to correct
that. In fact, the example in
Documentation/devicetree/bindings/mmc/mmc.txt even lists an invalid
compatible string, so that is even worse.
Arnd
On Monday, July 4, 2016 10:41:20 AM CEST Arend Van Spriel wrote:
> On 2-7-2016 23:30, Arnd Bergmann wrote:
> > On Saturday, July 2, 2016 8:20:35 PM CEST Arend Van Spriel wrote:
> >>> If you want a separate property, then I repeat my very first
> >>> suggestion, the well defined model property.
> >>> e.g.
> >>>
> >>> brcmf@0 {
> >>> model = "ampak,ap6210";
> >>> compatible = "brcm,bcm4329-fmac";
> >>> ...
> >>> };
> >>>
> >>> All device nodes may have a model property, not just the top "machine" one.
> >>
> >> I heard you the first time I just was not sure what the implications
> >> would be to use it. Hence I suggested a vendor specific property.
> >> However, looking up and reading the definition in ePAPRv1.1 I suppose it
> >> is fine to use the model property:
> >>
> >> Property: model
> >> Value type: <string>
> >> Description:
> >> The model property value is a <string> that specifies the manufacturer’s
> >> model number of the device.
> >>
> >> The recommended format is: “manufacturer,model”, where manufacturer is a
> >> string describing the name of the manufacturer (such as a stock ticker
> >> symbol), and model specifies the model number.
> >
> > The model property is very similar to compatible, except that there is
> > only one entry rather than a list of entries from most specific to
> > most generic.
>
> They seem very similar, but I think there is a conceptual difference.
> The compatible property is mainly used to select the appropriate driver
> and as such the property is typically ignored by device drivers.
> Probably there are exceptions to be found.
>
> > I think by writing the above example as
> >
> > compatible = "ampak,ap6210", "brcm,bcm4329-fmac";
> >
> > we can provide the same functionality in a slightly simpler way, the driver
> > then just goes on to look for the nvram file for each entry in sequence until
> > it finds one.
>
> Not sure why this would be simpler. Why would traversing the compatible
> string be simpler than handling the model property if present and
> otherwise fallback to the default nvram naming.
Because you have to walk the list anyway to find the other firmware files:
when you have a specialization of a device that requires listing both values
as compatible, the driver has no idea which of the entries to use, unless
you add a lookup table that adds more complexity.
Arnd
Jonas Gorski <[email protected]> writes:
> Hi,
>
> On 30 June 2016 at 21:23, Arend Van Spriel <[email protected]> wrote:
>>
>>
>> On 30-6-2016 13:31, Arnd Bergmann wrote:
>>> On Thursday, June 30, 2016 12:25:15 PM CEST Hans de Goede wrote:
>>>>> So then how about making use of a more specific compatible string?
>>>>>
>>>>> e.g.
>>>>>
>>>>> brcmf {
>>>>> compatible = "foo,ap6210", "brcm,bcm4329-fmac";
>>>>> ...
>>>>> };
>>>>>
>>>>> and if the compatible has more than one element you request
>>>>> FW_NAME_<compatible>.txt as the nvram file. Or try each comptible (and
>>>>> lastly no suffix) until you get a match. (AFAICT, this is what the
>>>>> "model" property was originally intended for anyway, but almost nobody
>>>>> did it right, and everyone put a user readable string into "model" for
>>>>> boards instead of the ePAPR defined compatible string).
>>>>
>>>> Hmm, interesting idea. Not sure how easy / hard it will be to implement
>>>> this, but from a dt binding point of view it seems elegant.
>>>>
>>>> Kalle, Arend, what do you think of this ?
>>
>> At first glance I like the suggestion, but this would mean updating the
>> bindings document for each new wifi module that we want to add. Not a
>> big problem, but it makes that I have a slight preference to using a
>> property for it, eg. brcm,module = "ap6210";
>
> If you want a separate property, then I repeat my very first
> suggestion, the well defined model property.
> e.g.
>
> brcmf@0 {
> model = "ampak,ap6210";
> compatible = "brcm,bcm4329-fmac";
> ...
> };
>
> All device nodes may have a model property, not just the top "machine" one.
I like this model property but I'm no DT expert. What others think about
it, would it work?
--
Kalle Valo
On Thu, Jul 07, 2016 at 10:46:28AM +0200, Arnd Bergmann wrote:
> On Wednesday, July 6, 2016 9:19:41 PM CEST Arend Van Spriel wrote:
> > 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 <[email protected]> wrote:
> > > 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 don't think it has been discussed or decided before as the question
> has not come up, so for now this is my personal view. Maybe one of
> the devicetree maintainers can comment on this.
Back from vacation and getting caught up.
I agree with Arnd here. In my view model is the OEM branding on the
device, compatible is the h/w. If you have different firmware related
files, that goes beyond OEM branding.
Rob
On 4-7-2016 10:55, Arnd Bergmann wrote:
> On Monday, July 4, 2016 10:41:20 AM CEST Arend Van Spriel wrote:
>> On 2-7-2016 23:30, Arnd Bergmann wrote:
>>> On Saturday, July 2, 2016 8:20:35 PM CEST Arend Van Spriel wrote:
>>>>> If you want a separate property, then I repeat my very first
>>>>> suggestion, the well defined model property.
>>>>> e.g.
>>>>>
>>>>> brcmf@0 {
>>>>> model = "ampak,ap6210";
>>>>> compatible = "brcm,bcm4329-fmac";
>>>>> ...
>>>>> };
>>>>>
>>>>> All device nodes may have a model property, not just the top "machine" one.
>>>>
>>>> I heard you the first time I just was not sure what the implications
>>>> would be to use it. Hence I suggested a vendor specific property.
>>>> However, looking up and reading the definition in ePAPRv1.1 I suppose it
>>>> is fine to use the model property:
>>>>
>>>> Property: model
>>>> Value type: <string>
>>>> Description:
>>>> The model property value is a <string> that specifies the manufacturer’s
>>>> model number of the device.
>>>>
>>>> The recommended format is: “manufacturer,model”, where manufacturer is a
>>>> string describing the name of the manufacturer (such as a stock ticker
>>>> symbol), and model specifies the model number.
>>>
>>> The model property is very similar to compatible, except that there is
>>> only one entry rather than a list of entries from most specific to
>>> most generic.
>>
>> They seem very similar, but I think there is a conceptual difference.
>> The compatible property is mainly used to select the appropriate driver
>> and as such the property is typically ignored by device drivers.
>> Probably there are exceptions to be found.
>>
>>> I think by writing the above example as
>>>
>>> compatible = "ampak,ap6210", "brcm,bcm4329-fmac";
>>>
>>> we can provide the same functionality in a slightly simpler way, the driver
>>> then just goes on to look for the nvram file for each entry in sequence until
>>> it finds one.
>>
>> Not sure why this would be simpler. Why would traversing the compatible
>> string be simpler than handling the model property if present and
>> otherwise fallback to the default nvram naming.
>
> Because you have to walk the list anyway to find the other firmware files:
> when you have a specialization of a device that requires listing both values
> as compatible, the driver has no idea which of the entries to use, unless
> you add a lookup table that adds more complexity.
Currently, the brcmfmac bindings describe a single compatible string,
ie. "brcm,bcm4329-fmac", which selects the driver/programming model. If
that programming model supports "use model property if present,
otherwise use default" there is nothing to traverse. The default way in
the driver to determine firmware and nvram filename already has a lookup
table which uses the chip id and chip revision as key, which are read
from the device.
Regards,
Arend
On Thursday, July 7, 2016 11:16:59 AM CEST Arend Van Spriel wrote:
> On 7-7-2016 10:46, Arnd Bergmann wrote:
> > On Wednesday, July 6, 2016 9:19:41 PM CEST Arend Van Spriel wrote:
> >> 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 <[email protected]> wrote:
> > I'm a bit confused by the interdependencies now. You say that there are
> > board ID/rev values stored in OTP. What exactly prevents us from just
> > using those to generate a file name to look up the nvram file on disk
> > for the remaining values?
> >
> > Do we require the firmware to be running before we can read the OTP,
> > or are there cases where the board ID in OTP is wrong or missing?
>
> Well, both. Primarily we need firmware running to get the info. If board
> ID is missing in OTP the value from nvram file is used. If board ID in
> OTP is wrong we are screwed
Ok.
> > If we can't rely on OTP for one of those reasons and instead add two
> > properties for boardtype/boardrev, I don't think that requires a
> > change of the compatible string, we would just document those two
> > as optional properties.
>
> Right. I suppose the bindings update and driver update should preferably
> be in the same kernel release although bindings are supposedly OS agnostic.
It's a one-way dependency, we shouldn't add the kernel code handling
the property without having agreed on and updated the binding first.
Arnd
On 2-7-2016 23:30, Arnd Bergmann wrote:
> On Saturday, July 2, 2016 8:20:35 PM CEST Arend Van Spriel wrote:
>>> If you want a separate property, then I repeat my very first
>>> suggestion, the well defined model property.
>>> e.g.
>>>
>>> brcmf@0 {
>>> model = "ampak,ap6210";
>>> compatible = "brcm,bcm4329-fmac";
>>> ...
>>> };
>>>
>>> All device nodes may have a model property, not just the top "machine" one.
>>
>> I heard you the first time I just was not sure what the implications
>> would be to use it. Hence I suggested a vendor specific property.
>> However, looking up and reading the definition in ePAPRv1.1 I suppose it
>> is fine to use the model property:
>>
>> Property: model
>> Value type: <string>
>> Description:
>> The model property value is a <string> that specifies the manufacturer’s
>> model number of the device.
>>
>> The recommended format is: “manufacturer,model”, where manufacturer is a
>> string describing the name of the manufacturer (such as a stock ticker
>> symbol), and model specifies the model number.
>
> The model property is very similar to compatible, except that there is
> only one entry rather than a list of entries from most specific to
> most generic.
They seem very similar, but I think there is a conceptual difference.
The compatible property is mainly used to select the appropriate driver
and as such the property is typically ignored by device drivers.
Probably there are exceptions to be found.
> I think by writing the above example as
>
> compatible = "ampak,ap6210", "brcm,bcm4329-fmac";
>
> we can provide the same functionality in a slightly simpler way, the driver
> then just goes on to look for the nvram file for each entry in sequence until
> it finds one.
Not sure why this would be simpler. Why would traversing the compatible
string be simpler than handling the model property if present and
otherwise fallback to the default nvram naming.
Regards,
Arend
On Friday, July 1, 2016 10:17:37 AM CEST Arend Van Spriel wrote:
>
> On 1-7-2016 4:08, Rob Herring wrote:
> > On Wed, Jun 29, 2016 at 04:04:31PM +0200, Hans de Goede wrote:
> >> Add a brcm,nvram_file_name dt property to allow overruling the default
> >> nvram filename for sdio devices. The idea is that we can specify a
> >> board specific nvram file, e.g. brcmfmac43362-ap6210.txt for boards
> >> with an ap6210 wifi sdio module and ship this in linux-firmware, so
> >> that wifi will work out of the box, without requiring users to find
> >> and then manually install the right nvram file for their board.
> >
> > What about putting its contents directly into DT? It's just text
> > key/value pairs so it would match up well.
>
> It would be an option, but I have no clue how to dig up documentation of
> these key,value pairs. The file is typically obtained from a wifi module
> vendor which would need to be converted to DT format, ie. prefix with
> "brcm," etc. From driver perspective this would mean it need to know
> keys. Currently, it just takes the file contents and sends it to the device.
I can see multiple ways to do this here:
- create a child node for the nvram settings and document that the properties
in there follow exactly the format that is used for the nvram file.
- have just one property that contains a copy of the entire nvram file,
serving as a backing store for the file instead of pointing to the file.
- figure out more about the actual properties that are required and then
document only the ones that are actually different between devices using
the same chip. With a per-chip file that we can put into linux-firmware,
and the data from the OTP ROM, we might need only a small subset here
that fits well enough in the DT.
> >> diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> >> index 5dbf169..2ba13a6 100644
> >> --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> >> +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
> >> @@ -11,6 +11,7 @@ Required properties:
> >> Optional properties:
> >> - brcm,drive-strength : drive strength used for SDIO pins on device in mA
> >> (default = 6).
> >> + - brcm,nvram_file_name : name of the nvram file to load
> >
> > The need for firmware file names has come up several times though
> > nothing merged to yet. There has been at least some level of agreement
> > to use "firmware-name" here.
>
> Do you mean with or without vendor prefix? Actually the device needs two
> files to initialize. The firmware that is needed to have anything
> running on the device and the nvram data that characterizes the device
> for that firmware.
I thought the outcome was to never put file names into DT for new bindings, because
it doesn't do what you want in the end: Either the "compatible" string correctly
identifies the device and can be used to construct or look up a file name, or
the compatible string is not enough to actually identify the device and should
be extended with a more specific one.
Having the module identifer in a property to use that along with the compatible
string for the file name would also work, but I think just using the compatible
string list by itself makes more sense.
Arnd
On 17-7-2016 23:45, Rob Herring wrote:
> On Thu, Jul 07, 2016 at 10:46:28AM +0200, Arnd Bergmann wrote:
>> On Wednesday, July 6, 2016 9:19:41 PM CEST Arend Van Spriel wrote:
>>> 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 <[email protected]> wrote:
>>>> 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 don't think it has been discussed or decided before as the question
>> has not come up, so for now this is my personal view. Maybe one of
>> the devicetree maintainers can comment on this.
>
> Back from vacation and getting caught up.
>
> I agree with Arnd here. In my view model is the OEM branding on the
> device, compatible is the h/w. If you have different firmware related
> files, that goes beyond OEM branding.
Thanks, Rob
We are talking about hardware variants here. So using the model property
is off the table.
Regards,
Arend
On 7-7-2016 10:46, Arnd Bergmann wrote:
> On Wednesday, July 6, 2016 9:19:41 PM CEST Arend Van Spriel wrote:
>> 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 <[email protected]> wrote:
>>> 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 don't think it has been discussed or decided before as the question
> has not come up, so for now this is my personal view. Maybe one of
> the devicetree maintainers can comment on this.
>
>>> 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.
>
> I'm a bit confused by the interdependencies now. You say that there are
> board ID/rev values stored in OTP. What exactly prevents us from just
> using those to generate a file name to look up the nvram file on disk
> for the remaining values?
>
> Do we require the firmware to be running before we can read the OTP,
> or are there cases where the board ID in OTP is wrong or missing?
Well, both. Primarily we need firmware running to get the info. If board
ID is missing in OTP the value from nvram file is used. If board ID in
OTP is wrong we are screwed :-p
> If we can't rely on OTP for one of those reasons and instead add two
> properties for boardtype/boardrev, I don't think that requires a
> change of the compatible string, we would just document those two
> as optional properties.
Right. I suppose the bindings update and driver update should preferably
be in the same kernel release although bindings are supposedly OS agnostic.
Regards,
Arend
On 1-7-2016 4:08, Rob Herring wrote:
> On Wed, Jun 29, 2016 at 04:04:31PM +0200, Hans de Goede wrote:
>> Add a brcm,nvram_file_name dt property to allow overruling the default
>> nvram filename for sdio devices. The idea is that we can specify a
>> board specific nvram file, e.g. brcmfmac43362-ap6210.txt for boards
>> with an ap6210 wifi sdio module and ship this in linux-firmware, so
>> that wifi will work out of the box, without requiring users to find
>> and then manually install the right nvram file for their board.
>
> What about putting its contents directly into DT? It's just text
> key/value pairs so it would match up well.
It would be an option, but I have no clue how to dig up documentation of
these key,value pairs. The file is typically obtained from a wifi module
vendor which would need to be converted to DT format, ie. prefix with
"brcm," etc. From driver perspective this would mean it need to know
keys. Currently, it just takes the file contents and sends it to the device.
> Also, I have to wonder how all the non-SDIO based cards don't need this
> file.
PCIe devices have more on-board storage. Also on some router platforms
it gets nvram data from flash memory.
>> Signed-off-by: Hans de Goede <[email protected]>
>> ---
>> .../devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt | 2 ++
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c | 2 ++
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 6 ++++++
>> include/linux/platform_data/brcmfmac.h | 2 ++
>> 4 files changed, 12 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
>> index 5dbf169..2ba13a6 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
>> +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm43xx-fmac.txt
>> @@ -11,6 +11,7 @@ Required properties:
>> Optional properties:
>> - brcm,drive-strength : drive strength used for SDIO pins on device in mA
>> (default = 6).
>> + - brcm,nvram_file_name : name of the nvram file to load
>
> The need for firmware file names has come up several times though
> nothing merged to yet. There has been at least some level of agreement
> to use "firmware-name" here.
Do you mean with or without vendor prefix? Actually the device needs two
files to initialize. The firmware that is needed to have anything
running on the device and the nvram data that characterizes the device
for that firmware.
Regards,
Arend
>> - interrupt-parent : the phandle for the interrupt controller to which the
>> device interrupts are connected.
>> - interrupts : specifies attributes for the out-of-band interrupt (host-wake).
>> @@ -34,6 +35,7 @@ mmc3: mmc@01c12000 {
>> brcmf: bcrmf@1 {
>> reg = <1>;
>> compatible = "brcm,bcm4329-fmac";
>> + brcm,nvram_file_name = "brcm/brcmfmac43362-ap6210.txt";
>> interrupt-parent = <&pio>;
>> interrupts = <10 8>; /* PH10 / EINT10 */
>> interrupt-names = "host-wake";
On Wednesday, July 6, 2016 9:19:41 PM CEST Arend Van Spriel wrote:
> 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 <[email protected]> wrote:
> > 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 don't think it has been discussed or decided before as the question
has not come up, so for now this is my personal view. Maybe one of
the devicetree maintainers can comment on this.
> > 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.
I'm a bit confused by the interdependencies now. You say that there are
board ID/rev values stored in OTP. What exactly prevents us from just
using those to generate a file name to look up the nvram file on disk
for the remaining values?
Do we require the firmware to be running before we can read the OTP,
or are there cases where the board ID in OTP is wrong or missing?
If we can't rely on OTP for one of those reasons and instead add two
properties for boardtype/boardrev, I don't think that requires a
change of the compatible string, we would just document those two
as optional properties.
Arnd
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 <[email protected]> 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
On Thursday, June 30, 2016 9:23:56 PM CEST Arend Van Spriel wrote:
>
> On 30-6-2016 13:31, Arnd Bergmann wrote:
> > On Thursday, June 30, 2016 12:25:15 PM CEST Hans de Goede wrote:
> >>> So then how about making use of a more specific compatible string?
> >>>
> >>> e.g.
> >>>
> >>> brcmf {
> >>> compatible = "foo,ap6210", "brcm,bcm4329-fmac";
> >>> ...
> >>> };
> >>>
> >>> and if the compatible has more than one element you request
> >>> FW_NAME_<compatible>.txt as the nvram file. Or try each comptible (and
> >>> lastly no suffix) until you get a match. (AFAICT, this is what the
> >>> "model" property was originally intended for anyway, but almost nobody
> >>> did it right, and everyone put a user readable string into "model" for
> >>> boards instead of the ePAPR defined compatible string).
> >>
> >> Hmm, interesting idea. Not sure how easy / hard it will be to implement
> >> this, but from a dt binding point of view it seems elegant.
> >>
> >> Kalle, Arend, what do you think of this ?
>
> At first glance I like the suggestion, but this would mean updating the
> bindings document for each new wifi module that we want to add. Not a
> big problem, but it makes that I have a slight preference to using a
> property for it, eg. brcm,module = "ap6210";
I think we can be a little relaxed with the requirement for updating the
binding document here, as long as the binding lists all the strings that
are understood by the driver itself and documents that there can be
additional strings in it.
In particular, documenting how a compatible string that is made up from
the board id and revision should be unproblematic.
Arnd
On 04-07-16 16:54, Arnd Bergmann wrote:
> On Monday, July 4, 2016 11:08:38 AM CEST Arend Van Spriel wrote:
>> On 4-7-2016 10:55, Arnd Bergmann wrote:
>>> On Monday, July 4, 2016 10:41:20 AM CEST Arend Van Spriel wrote:
>>>> On 2-7-2016 23:30, Arnd Bergmann wrote:
>>>>> On Saturday, July 2, 2016 8:20:35 PM CEST Arend Van Spriel wrote:
>>>>>>> If you want a separate property, then I repeat my very first
>>>>>>> suggestion, the well defined model property.
>>>>>>> e.g.
>>>>>>>
>>>>>>> brcmf@0 {
>>>>>>> model = "ampak,ap6210";
>>>>>>> compatible = "brcm,bcm4329-fmac";
>>>>>>> ...
>>>>>>> };
>>>>>>>
>>>>>>> All device nodes may have a model property, not just the top "machine" one.
>>>>>>
>>>>>> I heard you the first time I just was not sure what the implications
>>>>>> would be to use it. Hence I suggested a vendor specific property.
>>>>>> However, looking up and reading the definition in ePAPRv1.1 I suppose it
>>>>>> is fine to use the model property:
>>>>>>
>>>>>> Property: model
>>>>>> Value type: <string>
>>>>>> Description:
>>>>>> The model property value is a <string> that specifies the manufacturer’s
>>>>>> model number of the device.
>>>>>>
>>>>>> The recommended format is: “manufacturer,model”, where manufacturer is a
>>>>>> string describing the name of the manufacturer (such as a stock ticker
>>>>>> symbol), and model specifies the model number.
>>>>>
>>>>> The model property is very similar to compatible, except that there is
>>>>> only one entry rather than a list of entries from most specific to
>>>>> most generic.
>>>>
>>>> They seem very similar, but I think there is a conceptual difference.
>>>> The compatible property is mainly used to select the appropriate driver
>>>> and as such the property is typically ignored by device drivers.
>>>> Probably there are exceptions to be found.
>>>>
>>>>> I think by writing the above example as
>>>>>
>>>>> compatible = "ampak,ap6210", "brcm,bcm4329-fmac";
>>>>>
>>>>> we can provide the same functionality in a slightly simpler way, the driver
>>>>> then just goes on to look for the nvram file for each entry in sequence until
>>>>> it finds one.
>>>>
>>>> Not sure why this would be simpler. Why would traversing the compatible
>>>> string be simpler than handling the model property if present and
>>>> otherwise fallback to the default nvram naming.
>>>
>>> Because you have to walk the list anyway to find the other firmware files:
>>> when you have a specialization of a device that requires listing both values
>>> as compatible, the driver has no idea which of the entries to use, unless
>>> you add a lookup table that adds more complexity.
>>
>> Currently, the brcmfmac bindings describe a single compatible string,
>> ie. "brcm,bcm4329-fmac", which selects the driver/programming model. If
>> that programming model supports "use model property if present,
>> otherwise use default" there is nothing to traverse. The default way in
>> the driver to determine firmware and nvram filename already has a lookup
>> table which uses the chip id and chip revision as key, which are read
>> from the device.
>
> 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.
Regards,
Arend
On 1-7-2016 10:58, Jonas Gorski wrote:
> Hi,
>
> On 30 June 2016 at 21:23, Arend Van Spriel <[email protected]> wrote:
>>
>>
>> On 30-6-2016 13:31, Arnd Bergmann wrote:
>>> On Thursday, June 30, 2016 12:25:15 PM CEST Hans de Goede wrote:
>>>>> So then how about making use of a more specific compatible string?
>>>>>
>>>>> e.g.
>>>>>
>>>>> brcmf {
>>>>> compatible = "foo,ap6210", "brcm,bcm4329-fmac";
>>>>> ...
>>>>> };
>>>>>
>>>>> and if the compatible has more than one element you request
>>>>> FW_NAME_<compatible>.txt as the nvram file. Or try each comptible (and
>>>>> lastly no suffix) until you get a match. (AFAICT, this is what the
>>>>> "model" property was originally intended for anyway, but almost nobody
>>>>> did it right, and everyone put a user readable string into "model" for
>>>>> boards instead of the ePAPR defined compatible string).
>>>>
>>>> Hmm, interesting idea. Not sure how easy / hard it will be to implement
>>>> this, but from a dt binding point of view it seems elegant.
>>>>
>>>> Kalle, Arend, what do you think of this ?
>>
>> At first glance I like the suggestion, but this would mean updating the
>> bindings document for each new wifi module that we want to add. Not a
>> big problem, but it makes that I have a slight preference to using a
>> property for it, eg. brcm,module = "ap6210";
>
> If you want a separate property, then I repeat my very first
> suggestion, the well defined model property.
> e.g.
>
> brcmf@0 {
> model = "ampak,ap6210";
> compatible = "brcm,bcm4329-fmac";
> ...
> };
>
> All device nodes may have a model property, not just the top "machine" one.
I heard you the first time :-p I just was not sure what the implications
would be to use it. Hence I suggested a vendor specific property.
However, looking up and reading the definition in ePAPRv1.1 I suppose it
is fine to use the model property:
Property: model
Value type: <string>
Description:
The model property value is a <string> that specifies the manufacturer’s
model number of the device.
The recommended format is: “manufacturer,model”, where manufacturer is a
string describing the name of the manufacturer (such as a stock ticker
symbol), and model specifies the model number.
Regards,
Arend
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 <[email protected]> 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.
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.
Arnd
Hi,
On 30 June 2016 at 21:23, Arend Van Spriel <[email protected]> wrote:
>
>
> On 30-6-2016 13:31, Arnd Bergmann wrote:
>> On Thursday, June 30, 2016 12:25:15 PM CEST Hans de Goede wrote:
>>>> So then how about making use of a more specific compatible string?
>>>>
>>>> e.g.
>>>>
>>>> brcmf {
>>>> compatible = "foo,ap6210", "brcm,bcm4329-fmac";
>>>> ...
>>>> };
>>>>
>>>> and if the compatible has more than one element you request
>>>> FW_NAME_<compatible>.txt as the nvram file. Or try each comptible (and
>>>> lastly no suffix) until you get a match. (AFAICT, this is what the
>>>> "model" property was originally intended for anyway, but almost nobody
>>>> did it right, and everyone put a user readable string into "model" for
>>>> boards instead of the ePAPR defined compatible string).
>>>
>>> Hmm, interesting idea. Not sure how easy / hard it will be to implement
>>> this, but from a dt binding point of view it seems elegant.
>>>
>>> Kalle, Arend, what do you think of this ?
>
> At first glance I like the suggestion, but this would mean updating the
> bindings document for each new wifi module that we want to add. Not a
> big problem, but it makes that I have a slight preference to using a
> property for it, eg. brcm,module = "ap6210";
If you want a separate property, then I repeat my very first
suggestion, the well defined model property.
e.g.
brcmf@0 {
model = "ampak,ap6210";
compatible = "brcm,bcm4329-fmac";
...
};
All device nodes may have a model property, not just the top "machine" one.
Regards
Jonas
On Tue, Jul 5, 2016 at 3:43 PM, Arnd Bergmann <[email protected]> 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.
Regards,
Arend
> The mmc binding is less clear about that, and we may want to correct
> that. In fact, the example in
> Documentation/devicetree/bindings/mmc/mmc.txt even lists an invalid
> compatible string, so that is even worse.
>
> Arnd
On Saturday, July 2, 2016 8:20:35 PM CEST Arend Van Spriel wrote:
> > If you want a separate property, then I repeat my very first
> > suggestion, the well defined model property.
> > e.g.
> >
> > brcmf@0 {
> > model = "ampak,ap6210";
> > compatible = "brcm,bcm4329-fmac";
> > ...
> > };
> >
> > All device nodes may have a model property, not just the top "machine" one.
>
> I heard you the first time I just was not sure what the implications
> would be to use it. Hence I suggested a vendor specific property.
> However, looking up and reading the definition in ePAPRv1.1 I suppose it
> is fine to use the model property:
>
> Property: model
> Value type: <string>
> Description:
> The model property value is a <string> that specifies the manufacturer’s
> model number of the device.
>
> The recommended format is: “manufacturer,model”, where manufacturer is a
> string describing the name of the manufacturer (such as a stock ticker
> symbol), and model specifies the model number.
The model property is very similar to compatible, except that there is
only one entry rather than a list of entries from most specific to
most generic.
I think by writing the above example as
compatible = "ampak,ap6210", "brcm,bcm4329-fmac";
we can provide the same functionality in a slightly simpler way, the driver
then just goes on to look for the nvram file for each entry in sequence until
it finds one.
Arnd
On Fri, Jul 01, 2016 at 10:17:37AM +0200, Arend Van Spriel wrote:
>
>
> On 1-7-2016 4:08, Rob Herring wrote:
> > On Wed, Jun 29, 2016 at 04:04:31PM +0200, Hans de Goede wrote:
> >> Add a brcm,nvram_file_name dt property to allow overruling the default
> >> nvram filename for sdio devices. The idea is that we can specify a
> >> board specific nvram file, e.g. brcmfmac43362-ap6210.txt for boards
> >> with an ap6210 wifi sdio module and ship this in linux-firmware, so
> >> that wifi will work out of the box, without requiring users to find
> >> and then manually install the right nvram file for their board.
> >
> > What about putting its contents directly into DT? It's just text
> > key/value pairs so it would match up well.
>
> It would be an option, but I have no clue how to dig up documentation of
> these key,value pairs. The file is typically obtained from a wifi module
> vendor which would need to be converted to DT format, ie. prefix with
> "brcm," etc. From driver perspective this would mean it need to know
> keys. Currently, it just takes the file contents and sends it to the device.
Okay, if it is opaque, then probably best to treat as firmware and not
change it.
Rob