| Message ID | 1489439116-4233-1-git-send-email-albeu@free.fr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Mon, Mar 13, 2017 at 10:05:09PM +0100, Alban wrote: > The current binding only cover PCI devices so extend it for SoC devices. > > Most SoC platforms use an MTD partition for the calibration data > instead of an EEPROM. The qca,no-eeprom property was added to allow > loading the EEPROM content using firmware loading. This new binding > replace this hack with NVMEM cells, so we also mark the qca,no-eeprom > property as deprecated in case anyone ever used it. > > Signed-off-by: Alban <albeu@free.fr> > --- > .../devicetree/bindings/net/wireless/qca,ath9k.txt | 41 ++++++++++++++++++++-- > 1 file changed, 38 insertions(+), 3 deletions(-) For the subject, "dt-bindings: net: ..." and one nit below, otherwise: Acked-by: Rob Herring <robh@kernel.org> > > diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > index b7396c8..61f5f6d 100644 > --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > @@ -27,16 +27,34 @@ Required properties: > - 0034 for AR9462 > - 0036 for AR9565 > - 0037 for AR9485 > + For SoC devices the compatible should be "qca,<soctype>-wmac" > + and one of the following fallbacks: > + - "qca,ar9100-wmac" > + - "qca,ar9330-wmac" > + - "qca,ar9340-wmac" > + - "qca,qca9550-wmac" > + - "qca,qca9530-wmac" > - reg: Address and length of the register set for the device. > > +Required properties for SoC devices: > +- interrupt-parent: phandle of the parent interrupt controller. > +- interrupts: Interrupt specifier for the controllers interrupt. > + > Optional properties: > +- mac-address: See ethernet.txt in the parent directory > +- local-mac-address: See ethernet.txt in the parent directory > +- clock-names: has to be "ref" > +- clocks: phandle of the reference clock > +- resets: phandle of the reset line > +- nvmem-cell-names: has to be "eeprom" and/or "address" > +- nvmem-cells: phandle to the eeprom nvmem cell and/or to the mac address > + nvmem cell. > + > +Deprecated properties: > - qca,no-eeprom: Indicates that there is no physical EEPROM connected to the > ath9k wireless chip (in this case the calibration / > EEPROM data will be loaded from userspace using the > kernel firmware loader). > -- mac-address: See ethernet.txt in the parent directory > -- local-mac-address: See ethernet.txt in the parent directory > - > > In this example, the node is defined as child node of the PCI controller: > &pci0 { > @@ -46,3 +64,20 @@ In this example, the node is defined as child node of the PCI controller: > qca,no-eeprom; > }; > }; > + > +In this example it is defined as a SoC device: > + wmac@180c0000 { wifi@... > + compatible = "qca,ar9132-wmac", "qca,ar9100-wmac"; > + reg = <0x180c0000 0x30000>; > + > + interrupt-parent = <&cpuintc>; > + interrupts = <2>; > + > + clock-names = "ref"; > + clocks = <&extosc>; > + > + nvmem-cell-names = "eeprom", "address"; > + nvmem-cells = <&wmac_eeprom>, <&wmac_address>; > + > + resets = <&rst 22>; > + }; > -- > 2.7.4 >
On Monday, March 13, 2017 10:05:09 PM CEST Alban wrote: > The current binding only cover PCI devices so extend it for SoC devices. > > Most SoC platforms use an MTD partition for the calibration data > instead of an EEPROM. The qca,no-eeprom property was added to allow > loading the EEPROM content using firmware loading. This new binding > replace this hack with NVMEM cells, so we also mark the qca,no-eeprom > property as deprecated in case anyone ever used it. Please don't mark "qca,no-eeprom" as deprecated then. If some devices geniously need to rely on userspace for extracting and processing the calibration data, it should be stay a optional properties. For example: A device that can't do easily without "qca,no-eeprom" is the AVM FRITZ!WLAN Repeater 300E. For this device, the caldata is stored in the flash, however for whatever reason the vendor choose to "reverse" it. (like completely back to front, not byteswapped or something). So an extra "unreversing step" is required. So, it would require some sort of a special nvmem-provider-processor as an alternative. > Signed-off-by: Alban <albeu@free.fr> > --- > diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > index b7396c8..61f5f6d 100644 > --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > @@ -27,16 +27,34 @@ Required properties: > - 0034 for AR9462 > - 0036 for AR9565 > - 0037 for AR9485 > + For SoC devices the compatible should be "qca,<soctype>-wmac" > + and one of the following fallbacks: > + - "qca,ar9100-wmac" > + - "qca,ar9330-wmac" > + - "qca,ar9340-wmac" > + - "qca,qca9550-wmac" > + - "qca,qca9530-wmac" > - reg: Address and length of the register set for the device. > > +Required properties for SoC devices: > +- interrupt-parent: phandle of the parent interrupt controller. > +- interrupts: Interrupt specifier for the controllers interrupt. > + > Optional properties: > +- mac-address: See ethernet.txt in the parent directory > +- local-mac-address: See ethernet.txt in the parent directory > +- clock-names: has to be "ref" > +- clocks: phandle of the reference clock > +- resets: phandle of the reset line > +- nvmem-cell-names: has to be "eeprom" and/or "address" > +- nvmem-cells: phandle to the eeprom nvmem cell and/or to the mac address > + nvmem cell. > + > +Deprecated properties: > - qca,no-eeprom: Indicates that there is no physical EEPROM connected to the > ath9k wireless chip (in this case the calibration / > EEPROM data will be loaded from userspace using the > kernel firmware loader). > -- mac-address: See ethernet.txt in the parent directory > -- local-mac-address: See ethernet.txt in the parent directory > - It sounds like you want to deprecate mac-address and local-mac-address as well. If so you sould add this to the commit as well. From my point of view, people mostly flat-out patched the eeprom-image if they wanted to set the mac-address. However, this was an extra step, if nvmem does away with it, I'm completely fine with deprecating these properties. Thanks, Christian
On Mon, 27 Mar 2017 18:11:15 +0200 Christian Lamparter <chunkeey@googlemail.com> wrote: > On Monday, March 13, 2017 10:05:09 PM CEST Alban wrote: > > The current binding only cover PCI devices so extend it for SoC devices. > > > > Most SoC platforms use an MTD partition for the calibration data > > instead of an EEPROM. The qca,no-eeprom property was added to allow > > loading the EEPROM content using firmware loading. This new binding > > replace this hack with NVMEM cells, so we also mark the qca,no-eeprom > > property as deprecated in case anyone ever used it. > > Please don't mark "qca,no-eeprom" as deprecated then. > If some devices geniously need to rely on userspace for extracting > and processing the calibration data, it should be stay a > optional properties. Deprecated just mean that it shouldn't be used for new devices. But as it is not used by any board, misuse the firmware loading API and firmware loader user helper are deprecated in udev, I find we could also just drop it. > For example: A device that can't do easily without "qca,no-eeprom" is > the AVM FRITZ!WLAN Repeater 300E. For this device, the caldata > is stored in the flash, however for whatever reason the vendor > choose to "reverse" it. (like completely back to front, not byteswapped > or something). So an extra "unreversing step" is required. So, it would > require some sort of a special nvmem-provider-processor as an > alternative. Or just handle this special eeprom format in the ath9k driver. I doubt that this case is so common that it would justify adding a whole new layer to nvmem. > > Signed-off-by: Alban <albeu@free.fr> > > --- > > diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > > index b7396c8..61f5f6d 100644 > > --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > > +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > > @@ -27,16 +27,34 @@ Required properties: > > - 0034 for AR9462 > > - 0036 for AR9565 > > - 0037 for AR9485 > > + For SoC devices the compatible should be "qca,<soctype>-wmac" > > + and one of the following fallbacks: > > + - "qca,ar9100-wmac" > > + - "qca,ar9330-wmac" > > + - "qca,ar9340-wmac" > > + - "qca,qca9550-wmac" > > + - "qca,qca9530-wmac" > > - reg: Address and length of the register set for the device. > > > > +Required properties for SoC devices: > > +- interrupt-parent: phandle of the parent interrupt controller. > > +- interrupts: Interrupt specifier for the controllers interrupt. > > + > > Optional properties: > > +- mac-address: See ethernet.txt in the parent directory > > +- local-mac-address: See ethernet.txt in the parent directory > > +- clock-names: has to be "ref" > > +- clocks: phandle of the reference clock > > +- resets: phandle of the reset line > > +- nvmem-cell-names: has to be "eeprom" and/or "address" > > +- nvmem-cells: phandle to the eeprom nvmem cell and/or to the mac address > > + nvmem cell. > > + > > +Deprecated properties: > > - qca,no-eeprom: Indicates that there is no physical EEPROM > > connected to the ath9k wireless chip (in this case the calibration / > > EEPROM data will be loaded from userspace > > using the kernel firmware loader). > > -- mac-address: See ethernet.txt in the parent directory > > -- local-mac-address: See ethernet.txt in the parent directory > > - > It sounds like you want to deprecate mac-address and > local-mac-address as well. If so you sould add this to the commit as > well. From my point of view, people mostly flat-out patched the > eeprom-image if they wanted to set the mac-address. However, this was > an extra step, if nvmem does away with it, I'm completely fine with > deprecating these properties. The produced diff is very misleading because the mac-address properties get lumped with the other new optional properties. But if you look closely it just move qca,no-eeprom to the deprecated section. Alban
On Tuesday, March 28, 2017 10:44:41 AM CEST Alban wrote: > On Mon, 27 Mar 2017 18:11:15 +0200 > Christian Lamparter <chunkeey@googlemail.com> wrote: > > > On Monday, March 13, 2017 10:05:09 PM CEST Alban wrote: > > > The current binding only cover PCI devices so extend it for SoC devices. > > > > > > Most SoC platforms use an MTD partition for the calibration data > > > instead of an EEPROM. The qca,no-eeprom property was added to allow > > > loading the EEPROM content using firmware loading. This new binding > > > replace this hack with NVMEM cells, so we also mark the qca,no-eeprom > > > property as deprecated in case anyone ever used it. > > > > Please don't mark "qca,no-eeprom" as deprecated then. > > If some devices geniously need to rely on userspace for extracting > > and processing the calibration data, it should be stay a > > optional properties. > > Deprecated just mean that it shouldn't be used for new devices. > But as it is not used by any board, misuse the firmware loading API and > firmware loader user helper are deprecated in udev, I find we could also > just drop it. But LEDE/OpenWRT rely on the firmware loading API more than ever and currently there is not a replacement for it. From what I know, Luis tried to replace it with his sysdata approach: <https://lkml.org/lkml/2016/12/16/204> however, this was disliked by Greg KH and Linus for the following reasons. <https://lkml.org/lkml/2016/6/16/995>: |So I absolutely abhor "changes for changes sake". | |If the existing code works for existing drivers, let them keep it. | |And if a new interface is truly more flexible, then it should be able |to implement the old interface with no changes, so that drivers |shouldn't need to be changed/upgraded. | |Then, drivers that actually _want_ new features, or that can take |advantage of new interfaces to actually make things *simpler*, can |choose to make those changes. But those changes should have real |advantages. |[...] your nvmem approach would need to be as universal and powerful as the "qca,no-eeprom" + userspace solutions in order to deprecate it. > > For example: A device that can't do easily without "qca,no-eeprom" is > > the AVM FRITZ!WLAN Repeater 300E. For this device, the caldata > > is stored in the flash, however for whatever reason the vendor > > choose to "reverse" it. (like completely back to front, not byteswapped > > or something). So an extra "unreversing step" is required. So, it would > > require some sort of a special nvmem-provider-processor as an > > alternative. > > Or just handle this special eeprom format in the ath9k driver. I doubt > that this case is so common that it would justify adding a whole new > layer to nvmem. Well, you'll have to deal with it in nvmem, if you want it to deprecate "qca,no-eeprom". I looked into 10-ath9k-eeprom [0] of LEDE's AR71XX target and I noticed that quite a few devices patch the MACs of the wifi. If you look at the code for the Airtight C-55 and C-60, Meraki MR18, Meraki Z1, you'll notice that each one has to add a fixed value (+1, +2, ...) to the extraced MAC-Address. So how would you replicate this, with "nvmem-cell-names = address" without some sort of nvmem-provider-processor? Also, there's another usecase of a nvmem-provider-processor. For example, one could be convert all the different types of ascii-macs (Either strings like "00:11:22:33:44:55", "00.11.22.33..." or "00112233..." ) to their binary representation. For AR71XX, this is mostly done by ath79_parse_ascii_mac: https://github.com/lede-project/source/blob/master/target/linux/ar71xx/files/arch/mips/ath79/dev-eth.c#L1204 and grep lists the following devices: mach-dgl-5500-a1.c, mach-dhp-1565-a1.c, mach-dir-505-a1.c, mach-dir-615-c1.c mach-dir-615-i1.c, mach-dir-825-b1.c, mach-dir-825-c1.c, mach-tew-673gru.c mach-tew-712br.c, mach-tew-732br.c, mach-tew-823dru.c I did a quick check: All of them use the extracted MACs for ath9k and/or ethernet. Note: I think Ralink/MediaTek will have the same issues. > > > Optional properties: > > > +- mac-address: See ethernet.txt in the parent directory > > > +- local-mac-address: See ethernet.txt in the parent directory > > > [...] > > > + > > > +Deprecated properties: > > > - qca,no-eeprom: Indicates that there is no physical EEPROM > > > connected to the ath9k wireless chip (in this case the calibration / > > > EEPROM data will be loaded from userspace > > > using the kernel firmware loader). > > > -- mac-address: See ethernet.txt in the parent directory > > > -- local-mac-address: See ethernet.txt in the parent directory > > > - > > It sounds like you want to deprecate mac-address and > > local-mac-address as well. If so you sould add this to the commit as > > well. From my point of view, people mostly flat-out patched the > > eeprom-image if they wanted to set the mac-address. However, this was > > an extra step, if nvmem does away with it, I'm completely fine with > > deprecating these properties. > > The produced diff is very misleading because the mac-address properties > get lumped with the other new optional properties. But if you look > closely it just move qca,no-eeprom to the deprecated section. Yes ok. This is the case. Thanks, Christian [0] <https://github.com/lede-project/source/blob/master/target/linux/ar71xx/base-files/etc/hotplug.d/firmware/10-ath9k-eeprom#L85>
> But LEDE/OpenWRT rely on the firmware loading API more than ever and > currently there is not a replacement for it. .... > I looked into 10-ath9k-eeprom [0] of LEDE's AR71XX target and I noticed > that quite a few devices patch the MACs of the wifi. > If you look at the code for the Airtight C-55 and C-60, Meraki MR18, > Meraki Z1, you'll notice that each one has to add a fixed value (+1, > +2, ...) to the extraced MAC-Address. So how would you replicate this, > with "nvmem-cell-names = address" without some sort of > nvmem-provider-processor? ... > https://github.com/lede-project/source/blob/master/target/linux/ar71xx/files/arch/mips/ath79/dev-eth.c#L1204 > > and grep lists the following devices: > mach-dgl-5500-a1.c, mach-dhp-1565-a1.c, mach-dir-505-a1.c, mach-dir-615-c1.c > mach-dir-615-i1.c, mach-dir-825-b1.c, mach-dir-825-c1.c, mach-tew-673gru.c > mach-tew-712br.c, mach-tew-732br.c, mach-tew-823dru.c I would say a big part of the problem is that all of these use cases are outside of mainline. Why should mainline support something which is not actually used in mainline. So i would suggest your first step is to bring some of these devices into mainline. Once in mainline, it becomes a mainline issue, and people will help get it solved. Andrew
On Tuesday, March 28, 2017 5:18:37 PM CEST Andrew Lunn wrote: > > But LEDE/OpenWRT rely on the firmware loading API more than ever and > > currently there is not a replacement for it. > > .... > > > > I looked into 10-ath9k-eeprom [0] of LEDE's AR71XX target and I noticed > > that quite a few devices patch the MACs of the wifi. > > If you look at the code for the Airtight C-55 and C-60, Meraki MR18, > > Meraki Z1, you'll notice that each one has to add a fixed value (+1, > > +2, ...) to the extraced MAC-Address. So how would you replicate this, > > with "nvmem-cell-names = address" without some sort of > > nvmem-provider-processor? > > ... > > > https://github.com/lede-project/source/blob/master/target/linux/ar71xx/files/arch/mips/ath79/dev-eth.c#L1204 > > > > and grep lists the following devices: > > mach-dgl-5500-a1.c, mach-dhp-1565-a1.c, mach-dir-505-a1.c, mach-dir-615-c1.c > > mach-dir-615-i1.c, mach-dir-825-b1.c, mach-dir-825-c1.c, mach-tew-673gru.c > > mach-tew-712br.c, mach-tew-732br.c, mach-tew-823dru.c > > I would say a big part of the problem is that all of these use cases > are outside of mainline. Why should mainline support something which > is not actually used in mainline. > > So i would suggest your first step is to bring some of these devices > into mainline. Once in mainline, it becomes a mainline issue, and > people will help get it solved. > Oh, in that case you should probably go "all out" and ask on the LKML to remove all of the ath9k and ath10k ahb work. From what I know all the "users" are running some sort of OpenWRT/LEDE or a derivative. This is because Atheros/QCA provided a SDK based on OpenWRT. Alban has been trying to convert the platform to device-tree and add them to the mainline for a while now: https://patchwork.kernel.org/patch/6514551/ So, you are questioning this work as well. Thanks, Christian
> Oh, in that case you should probably go "all out" and ask on the > LKML to remove all of the ath9k and ath10k ahb work. From what I > know all the "users" are running some sort of OpenWRT/LEDE or a > derivative. This is because Atheros/QCA provided a SDK based on > OpenWRT. > > Alban has been trying to convert the platform to device-tree > and add them to the mainline for a while now: > > https://patchwork.kernel.org/patch/6514551/ > > So, you are questioning this work as well. Not at all. Ralph Sennhauser has been doing a great job of getting all the Marvell devices into Mainline, and i help as much as i can, being one of the Marvell SoC Maintainers. I'm just saying, get a few boards which require these facilities into the mainline, and then you have a much stranger base to argue from. Andrew
On Tuesday, March 28, 2017 6:41:59 PM CEST Andrew Lunn wrote: > > Oh, in that case you should probably go "all out" and ask on the > > LKML to remove all of the ath9k and ath10k ahb work. From what I > > know all the "users" are running some sort of OpenWRT/LEDE or a > > derivative. This is because Atheros/QCA provided a SDK based on > > OpenWRT. > > > > Alban has been trying to convert the platform to device-tree > > and add them to the mainline for a while now: > > > > https://patchwork.kernel.org/patch/6514551/ > > > > So, you are questioning this work as well. > > Not at all. Ralph Sennhauser has been doing a great job of getting all > the Marvell devices into Mainline, and i help as much as i can, being > one of the Marvell SoC Maintainers. > > I'm just saying, get a few boards which require these facilities into > the mainline, and then you have a much stronger base to argue from. I was arguing not to deprecate "qca,no-eeprom" property. based on this quote from Linus' <https://lkml.org/lkml/2016/6/16/995>: |if a new interface is truly more flexible, then it should be able |to implement the old interface with no changes, so that drivers |shouldn't need to be changed/upgraded. what stronger point to do you want? Thanks, Christian
Alban <albeu@free.fr> writes: > The current binding only cover PCI devices so extend it for SoC devices. > > Most SoC platforms use an MTD partition for the calibration data > instead of an EEPROM. The qca,no-eeprom property was added to allow > loading the EEPROM content using firmware loading. This new binding > replace this hack with NVMEM cells, so we also mark the qca,no-eeprom > property as deprecated in case anyone ever used it. A patchset like this should definitely have a cover letter and explain the backround and reasoning for this patchset. Currently I'm just guessing. > Signed-off-by: Alban <albeu@free.fr> Please use your full name both in From and S-o-b lines.
diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt index b7396c8..61f5f6d 100644 --- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt @@ -27,16 +27,34 @@ Required properties: - 0034 for AR9462 - 0036 for AR9565 - 0037 for AR9485 + For SoC devices the compatible should be "qca,<soctype>-wmac" + and one of the following fallbacks: + - "qca,ar9100-wmac" + - "qca,ar9330-wmac" + - "qca,ar9340-wmac" + - "qca,qca9550-wmac" + - "qca,qca9530-wmac" - reg: Address and length of the register set for the device. +Required properties for SoC devices: +- interrupt-parent: phandle of the parent interrupt controller. +- interrupts: Interrupt specifier for the controllers interrupt. + Optional properties: +- mac-address: See ethernet.txt in the parent directory +- local-mac-address: See ethernet.txt in the parent directory +- clock-names: has to be "ref" +- clocks: phandle of the reference clock +- resets: phandle of the reset line +- nvmem-cell-names: has to be "eeprom" and/or "address" +- nvmem-cells: phandle to the eeprom nvmem cell and/or to the mac address + nvmem cell. + +Deprecated properties: - qca,no-eeprom: Indicates that there is no physical EEPROM connected to the ath9k wireless chip (in this case the calibration / EEPROM data will be loaded from userspace using the kernel firmware loader). -- mac-address: See ethernet.txt in the parent directory -- local-mac-address: See ethernet.txt in the parent directory - In this example, the node is defined as child node of the PCI controller: &pci0 { @@ -46,3 +64,20 @@ In this example, the node is defined as child node of the PCI controller: qca,no-eeprom; }; }; + +In this example it is defined as a SoC device: + wmac@180c0000 { + compatible = "qca,ar9132-wmac", "qca,ar9100-wmac"; + reg = <0x180c0000 0x30000>; + + interrupt-parent = <&cpuintc>; + interrupts = <2>; + + clock-names = "ref"; + clocks = <&extosc>; + + nvmem-cell-names = "eeprom", "address"; + nvmem-cells = <&wmac_eeprom>, <&wmac_address>; + + resets = <&rst 22>; + };
The current binding only cover PCI devices so extend it for SoC devices. Most SoC platforms use an MTD partition for the calibration data instead of an EEPROM. The qca,no-eeprom property was added to allow loading the EEPROM content using firmware loading. This new binding replace this hack with NVMEM cells, so we also mark the qca,no-eeprom property as deprecated in case anyone ever used it. Signed-off-by: Alban <albeu@free.fr> --- .../devicetree/bindings/net/wireless/qca,ath9k.txt | 41 ++++++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-)