| Message ID | 1467209074-15634-1-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 29 June 2016 at 16:04, Hans de Goede <hdegoede@redhat.com> 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
Hi, On 29-06-16 16:42, Jonas Gorski wrote: > Hi, > > On 29 June 2016 at 16:04, Hans de Goede <hdegoede@redhat.com> 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
Hans de Goede <hdegoede@redhat.com> writes: > Hi, > > On 29-06-16 16:42, Jonas Gorski wrote: >> Hi, >> >> On 29 June 2016 at 16:04, Hans de Goede <hdegoede@redhat.com> 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?
Hi, On 29-06-16 19:00, Kalle Valo wrote: > Hans de Goede <hdegoede@redhat.com> writes: > >> Hi, >> >> On 29-06-16 16:42, Jonas Gorski wrote: >>> Hi, >>> >>> On 29 June 2016 at 16:04, Hans de Goede <hdegoede@redhat.com> 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
On 29-6-2016 20:01, Hans de Goede wrote: > Hi, > > On 29-06-16 19:00, Kalle Valo wrote: >> Hans de Goede <hdegoede@redhat.com> writes: >> >>> Hi, >>> >>> On 29-06-16 16:42, Jonas Gorski wrote: >>>> Hi, >>>> >>>> On 29 June 2016 at 16:04, Hans de Goede <hdegoede@redhat.com> 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 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 <hdegoede@redhat.com> writes: >>> >>>> Hi, >>>> >>>> On 29-06-16 16:42, Jonas Gorski wrote: >>>>> Hi, >>>>> >>>>> On 29 June 2016 at 16:04, Hans de Goede <hdegoede@redhat.com> 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 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
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
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
Arend Van Spriel <arend.vanspriel@broadcom.com> 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?
Arend Van Spriel <arend.vanspriel@broadcom.com> 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
Priit Laes <plaes@plaes.org> 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...
Hi, On 30-06-16 10:46, Kalle Valo wrote: > Arend Van Spriel <arend.vanspriel@broadcom.com> 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
Hi, On 30-06-16 11:02, Kalle Valo wrote: > Priit Laes <plaes@plaes.org> 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
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 <hdegoede@redhat.com> writes: >>> >>>> Hi, >>>> >>>> On 29-06-16 16:42, Jonas Gorski wrote: >>>>> Hi, >>>>> >>>>> On 29 June 2016 at 16:04, Hans de Goede <hdegoede@redhat.com> 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
Hans de Goede <hdegoede@redhat.com> writes: > Hi, > > On 30-06-16 11:02, Kalle Valo wrote: >> Priit Laes <plaes@plaes.org> 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
Hi, On 30-06-16 11:58, Kalle Valo wrote: > Hans de Goede <hdegoede@redhat.com> writes: > >> Hi, >> >> On 30-06-16 11:02, Kalle Valo wrote: >>> Priit Laes <plaes@plaes.org> 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
Hi, On 30 June 2016 at 12:04, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 30-06-16 11:58, Kalle Valo wrote: >> >> Hans de Goede <hdegoede@redhat.com> writes: >> >>> Hi, >>> >>> On 30-06-16 11:02, Kalle Valo wrote: >>>> >>>> Priit Laes <plaes@plaes.org> 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 30-06-16 12:18, Jonas Gorski wrote: > Hi, > > On 30 June 2016 at 12:04, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> >> On 30-06-16 11:58, Kalle Valo wrote: >>> >>> Hans de Goede <hdegoede@redhat.com> writes: >>> >>>> Hi, >>>> >>>> On 30-06-16 11:02, Kalle Valo wrote: >>>>> >>>>> Priit Laes <plaes@plaes.org> 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
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 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
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 <hdegoede@redhat.com> > --- > .../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 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 <hdegoede@redhat.com> >> --- >> .../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 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
Hi, On 30 June 2016 at 21:23, Arend Van Spriel <arend.vanspriel@broadcom.com> 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 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
Jonas Gorski <jonas.gorski@gmail.com> writes: > Hi, > > On 30 June 2016 at 21:23, Arend Van Spriel <arend.vanspriel@broadcom.com> 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?
On 1-7-2016 10:58, Jonas Gorski wrote: > Hi, > > On 30 June 2016 at 21:23, Arend Van Spriel <arend.vanspriel@broadcom.com> 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 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 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 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
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 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 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
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 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 Tue, Jul 5, 2016 at 3:43 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday, July 4, 2016 8:36:05 PM CEST Arend van Spriel wrote: >> On 04-07-16 16:54, Arnd Bergmann wrote: >> > On Monday, July 4, 2016 11:08:38 AM CEST Arend Van Spriel wrote: >> > >> > In drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c I already see >> > over a dozen different chips being supported, bcm4329 is only one of >> > them. In particular, there seem to be some that have various modules: >> > >> > BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43241_CHIP_ID, 0x0000001F, 43241B0), >> > BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43241_CHIP_ID, 0x00000020, 43241B4), >> > BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43241_CHIP_ID, 0xFFFFFFC0, 43241B5), >> > >> > So if you have a bcm43241, that compatible string probably should >> > include both brcm,bcm43241-b4-fmac and brcm,bcm43241-fmac, possibly also >> > brcm,bcm4329-fmac, to show that it is a superset of the programming >> > interface of that one. >> >> Hi Arnd, >> >> I have to disagree here. The compatible string "brcm,bcm4329-fmac" is >> chosen as the bcm4329 chip was the first supported and we never added >> others as there is no other programming required. For all supported >> devices the same device tree properties apply and are handled the same. >> As such there is no need to come up with a new compatible string. > > Generally speaking, the way that the compatible strings work is that you > add a new one whenever you get a new piece of hardware, and you can leave > the first one as a fallback so you don't have to change the driver. > > Adding the new string for the actual device is important though, > since you might only discover later that they are not 100% compatible > and that you in fact need to know the difference. > > For discoverable buses like sdio or usb, it may actually be better > to not identify the device through the compatible property at all, > and instead use a string that is generated from the actual identifier > as the primary key, as e.g. documented in > Documentation/devicetree/bindings/usb/usb-device.txt Well, that is my point. We do not need to identify the device here. We can obtain that info, ie. chip id and revision, from the device itself as our wifi chips have a discoverable AXI backplane. What is missing is that we have no way to tell what board/module this device is integrated on, which makes it impossible to select the correct nvram file. The model property can fill that gap just nicely. 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 Wednesday, July 6, 2016 10:08:55 AM CEST Arend Van Spriel wrote: > On Tue, Jul 5, 2016 at 3:43 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Monday, July 4, 2016 8:36:05 PM CEST Arend van Spriel wrote: > >> On 04-07-16 16:54, Arnd Bergmann wrote: > >> > On Monday, July 4, 2016 11:08:38 AM CEST Arend Van Spriel wrote: > >> > > >> > In drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c I already see > >> > over a dozen different chips being supported, bcm4329 is only one of > >> > them. In particular, there seem to be some that have various modules: > >> > > >> > BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43241_CHIP_ID, 0x0000001F, 43241B0), > >> > BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43241_CHIP_ID, 0x00000020, 43241B4), > >> > BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43241_CHIP_ID, 0xFFFFFFC0, 43241B5), > >> > > >> > So if you have a bcm43241, that compatible string probably should > >> > include both brcm,bcm43241-b4-fmac and brcm,bcm43241-fmac, possibly also > >> > brcm,bcm4329-fmac, to show that it is a superset of the programming > >> > interface of that one. > >> > >> Hi Arnd, > >> > >> I have to disagree here. The compatible string "brcm,bcm4329-fmac" is > >> chosen as the bcm4329 chip was the first supported and we never added > >> others as there is no other programming required. For all supported > >> devices the same device tree properties apply and are handled the same. > >> As such there is no need to come up with a new compatible string. > > > > Generally speaking, the way that the compatible strings work is that you > > add a new one whenever you get a new piece of hardware, and you can leave > > the first one as a fallback so you don't have to change the driver. > > > > Adding the new string for the actual device is important though, > > since you might only discover later that they are not 100% compatible > > and that you in fact need to know the difference. > > > > For discoverable buses like sdio or usb, it may actually be better > > to not identify the device through the compatible property at all, > > and instead use a string that is generated from the actual identifier > > as the primary key, as e.g. documented in > > Documentation/devicetree/bindings/usb/usb-device.txt > > Well, that is my point. We do not need to identify the device here. We > can obtain that info, ie. chip id and revision, from the device itself > as our wifi chips have a discoverable AXI backplane. What is missing > is that we have no way to tell what board/module this device is > integrated on, which makes it impossible to select the correct nvram > file. The model property can fill that gap just nicely. We have multiple inconsistencies here, and it's not this driver that is particularly messed up, but I think using the model property here would make it worse: All existing uses of the model property in arch/arm/boot/dts and most of the ones in arch/powerpc/boot/dts are against the intended usage in one way or another, but adding different kind of incorrect usage won't improve that. The only way I can see the model property being used correctly would be to have it match the first entry in the compatible property, but that is completely redundant, so we tend to omit it, except for the root node in which it is required. For the root node however, the historic practice that has crept in on ARM is to put something completely different in there, which is a human-readable description of the machine rather than something we can use as a unique indentifier. I'd just consider the "model" property burned, and not use it for anything that doesn't already use it, just like we handle "device_type": a few things require it, nothing else should use it. 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
On 6-7-2016 15:42, Arnd Bergmann wrote: > On Wednesday, July 6, 2016 10:08:55 AM CEST Arend Van Spriel wrote: >> On Tue, Jul 5, 2016 at 3:43 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Monday, July 4, 2016 8:36:05 PM CEST Arend van Spriel wrote: >>>> On 04-07-16 16:54, Arnd Bergmann wrote: >>>>> On Monday, July 4, 2016 11:08:38 AM CEST Arend Van Spriel wrote: >>>>> >>>>> In drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c I already see >>>>> over a dozen different chips being supported, bcm4329 is only one of >>>>> them. In particular, there seem to be some that have various modules: >>>>> >>>>> BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43241_CHIP_ID, 0x0000001F, 43241B0), >>>>> BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43241_CHIP_ID, 0x00000020, 43241B4), >>>>> BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43241_CHIP_ID, 0xFFFFFFC0, 43241B5), >>>>> >>>>> So if you have a bcm43241, that compatible string probably should >>>>> include both brcm,bcm43241-b4-fmac and brcm,bcm43241-fmac, possibly also >>>>> brcm,bcm4329-fmac, to show that it is a superset of the programming >>>>> interface of that one. >>>> >>>> Hi Arnd, >>>> >>>> I have to disagree here. The compatible string "brcm,bcm4329-fmac" is >>>> chosen as the bcm4329 chip was the first supported and we never added >>>> others as there is no other programming required. For all supported >>>> devices the same device tree properties apply and are handled the same. >>>> As such there is no need to come up with a new compatible string. >>> >>> Generally speaking, the way that the compatible strings work is that you >>> add a new one whenever you get a new piece of hardware, and you can leave >>> the first one as a fallback so you don't have to change the driver. >>> >>> Adding the new string for the actual device is important though, >>> since you might only discover later that they are not 100% compatible >>> and that you in fact need to know the difference. >>> >>> For discoverable buses like sdio or usb, it may actually be better >>> to not identify the device through the compatible property at all, >>> and instead use a string that is generated from the actual identifier >>> as the primary key, as e.g. documented in >>> Documentation/devicetree/bindings/usb/usb-device.txt >> >> Well, that is my point. We do not need to identify the device here. We >> can obtain that info, ie. chip id and revision, from the device itself >> as our wifi chips have a discoverable AXI backplane. What is missing >> is that we have no way to tell what board/module this device is >> integrated on, which makes it impossible to select the correct nvram >> file. The model property can fill that gap just nicely. > > We have multiple inconsistencies here, and it's not this driver that is > particularly messed up, but I think using the model property here > would make it worse: > > All existing uses of the model property in arch/arm/boot/dts and most of > the ones in arch/powerpc/boot/dts are against the intended usage in > one way or another, but adding different kind of incorrect usage won't > improve that. > > The only way I can see the model property being used correctly would > be to have it match the first entry in the compatible property, but > that is completely redundant, so we tend to omit it, except for the > root node in which it is required. For the root node however, the > historic practice that has crept in on ARM is to put something completely > different in there, which is a human-readable description of the > machine rather than something we can use as a unique indentifier. > > I'd just consider the "model" property burned, and not use it for anything > that doesn't already use it, just like we handle "device_type": a few > things require it, nothing else should use it. If that is the agreed approach in devicetree arena I am fine with it. I have been unaware of this and just looked at the suggestion from Jonas seeing a solution to the problem at hand. > I agree with your point that the "compatible" property in case of brcmfmac > is really odd because it is not required to identify the hardware when > the SDIO device identification is sufficient, and we just need it to guard > the definition of the other properties. However it seems that now we > actually do need to identify the hardware because we cannot read the > board ID and revision that is supposed to come from nvram but also needed > to read the nvram contents from a file. The board ID and rev in the nvram may not be used if the device has these stored in OTP. However, the problem is that the device need firmware+nvram loaded into it before we can start the firmware on the device. Now if we were to add boardtype and boardrev properties in the binding to identify the hardware, I suppose a new compatible value would be required. Regards, Arend
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 <arnd@arndb.de> 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 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 <arnd@arndb.de> 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 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 <arnd@arndb.de> 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 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 <arnd@arndb.de> 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 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 <arnd@arndb.de> 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
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;
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 <hdegoede@redhat.com> --- .../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(+)