| Message ID | 20160709232834.31654-2-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Sunday, July 10, 2016 1:28:32 AM CEST Martin Blumenstingl wrote: > +- qca,check-eeprom-endianness: When enabled, the driver checks if the > + endianness of the EEPROM (based on the two > + magic bytes at the start of the EEPROM) > + matches the host system's native endianness. > + The data will be swapped accordingly if there > + is a mismatch. > + Leaving this disabled means that the EEPROM > + data will be interpreted in the system's > + native endianness, regardless of the magic > + bytes. > + Enable this option only when the magic bytes > + are known to indicate the correct endianness > + of the EEPROM data, because otherwise the > + EEPROM data may be swapped and thus may be > + parsed incorrectly. Defining properties in terms of "native" endianess is usually not a good idea, as some architectures (ARMv6+, PowerPC, ...) allow running either big-endian or little-endian without changing the endianess of device registers in the process. It's better to document the property as defaulting to a specific endianess unless you have an override or the autodetection flag, regardless of the CPU endianess. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jul 10, 2016 at 10:52 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Sunday, July 10, 2016 1:28:32 AM CEST Martin Blumenstingl wrote: >> +- qca,check-eeprom-endianness: When enabled, the driver checks if the >> + endianness of the EEPROM (based on the two >> + magic bytes at the start of the EEPROM) >> + matches the host system's native endianness. >> + The data will be swapped accordingly if there >> + is a mismatch. >> + Leaving this disabled means that the EEPROM >> + data will be interpreted in the system's >> + native endianness, regardless of the magic >> + bytes. >> + Enable this option only when the magic bytes >> + are known to indicate the correct endianness >> + of the EEPROM data, because otherwise the >> + EEPROM data may be swapped and thus may be >> + parsed incorrectly. > > Defining properties in terms of "native" endianess is usually not a good > idea, as some architectures (ARMv6+, PowerPC, ...) allow running either > big-endian or little-endian without changing the endianess of device > registers in the process. > > It's better to document the property as defaulting to a specific endianess > unless you have an override or the autodetection flag, regardless of > the CPU endianess. ath9k reads the data from the EEPROM into memory. With that property disabled ath9k simply assumes that the endianness of the values in the EEPROM are having the correct endianness for the host system (in other words: no swap is being applied). I am not sure I understand you correctly, but isn't what you are explaining an issue in the ath9k code, rather than in this documentation? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday, July 10, 2016 10:54:50 PM CEST Martin Blumenstingl wrote: > On Sun, Jul 10, 2016 at 10:52 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Sunday, July 10, 2016 1:28:32 AM CEST Martin Blumenstingl wrote: > >> +- qca,check-eeprom-endianness: When enabled, the driver checks if the > >> + endianness of the EEPROM (based on the two > >> + magic bytes at the start of the EEPROM) > >> + matches the host system's native endianness. > >> + The data will be swapped accordingly if there > >> + is a mismatch. > >> + Leaving this disabled means that the EEPROM > >> + data will be interpreted in the system's > >> + native endianness, regardless of the magic > >> + bytes. > >> + Enable this option only when the magic bytes > >> + are known to indicate the correct endianness > >> + of the EEPROM data, because otherwise the > >> + EEPROM data may be swapped and thus may be > >> + parsed incorrectly. > > > > Defining properties in terms of "native" endianess is usually not a good > > idea, as some architectures (ARMv6+, PowerPC, ...) allow running either > > big-endian or little-endian without changing the endianess of device > > registers in the process. > > > > It's better to document the property as defaulting to a specific endianess > > unless you have an override or the autodetection flag, regardless of > > the CPU endianess. > ath9k reads the data from the EEPROM into memory. With that property > disabled ath9k simply assumes that the endianness of the values in the > EEPROM are having the correct endianness for the host system (in other > words: no swap is being applied). > I am not sure I understand you correctly, but isn't what you are > explaining an issue in the ath9k code, rather than in this > documentation? I looked at the code more to find that out now, but I'm more confused now, as the eeprom seems to be read as a byte stream, and the endianess conversion that the driver performs is not on the data values in it, but seems to instead swap the bytes in each 16-bit word, regardless of the contents (the values inside of the byte stream are always interpreted as big-endian). Is that a correct observation? What I see in ath_pci_eeprom_read() is that the 16-bit words are taken from the lower 16 bit of the little-endian AR_EEPROM_STATUS_DATA register. As this is coming from a PCI register, it must have a device specific endianess that is identical on all CPUs, so in the description above, mentioning CPU endianess is a bit confusing. I could not find the code that does the conditional byteswap, instead this function static bool ar9300_eeprom_read_byte(struct ath_hw *ah, int address, u8 *buffer) { u16 val; if (unlikely(!ath9k_hw_nvram_read(ah, address / 2, &val))) return false; *buffer = (val >> (8 * (address % 2))) & 0xff; return true; } evidently assumes that the lower 8 bit of the 16-bit data from PCI come first, i.e. it byteswaps on big-endian CPUs to get the bytestream back into the order in which it is stored in the EEPROM. Interestingly, this also seems to happen for ath_ahb_eeprom_read() even though on that one the value does not get swapped by the bus accessor, so presumably big-endian machines with a ahb based ath9k store their eeprom byte-reversed? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 11, 2016 at 12:01 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> ath9k reads the data from the EEPROM into memory. With that property >> disabled ath9k simply assumes that the endianness of the values in the >> EEPROM are having the correct endianness for the host system (in other >> words: no swap is being applied). >> I am not sure I understand you correctly, but isn't what you are >> explaining an issue in the ath9k code, rather than in this >> documentation? > > I looked at the code more to find that out now, but I'm more confused > now, as the eeprom seems to be read as a byte stream, and the endianess > conversion that the driver performs is not on the data values in it, > but seems to instead swap the bytes in each 16-bit word, regardless > of the contents (the values inside of the byte stream are always > interpreted as big-endian). Is that a correct observation? that seems to be the case for the ar9003 eeprom. Other implementations are doing it different, look at ath9k_hw_ar9287_check_eeprom for example: first ath9k_hw_nvram_swap_data checks the two magic bytes at the beginning of the data and swaps the bytes in each 16-bit word if the magic bytes don't match the magic bytes for the "native system endianness" (see AR5416_EEPROM_MAGIC). Then more swapping is applied. I asked for more details about the EEPROM format (specifically the endianness part) here [0] as I don't have access to the datasheets (all I have is the ath9k code) > What I see in ath_pci_eeprom_read() is that the 16-bit words are taken > from the lower 16 bit of the little-endian AR_EEPROM_STATUS_DATA > register. As this is coming from a PCI register, it must have a device > specific endianess that is identical on all CPUs, so in the description > above, mentioning CPU endianess is a bit confusing. I could not find > the code that does the conditional byteswap, instead this function > > static bool ar9300_eeprom_read_byte(struct ath_hw *ah, int address, > u8 *buffer) > { > u16 val; > > if (unlikely(!ath9k_hw_nvram_read(ah, address / 2, &val))) > return false; > > *buffer = (val >> (8 * (address % 2))) & 0xff; > return true; > } > > evidently assumes that the lower 8 bit of the 16-bit data from PCI > come first, i.e. it byteswaps on big-endian CPUs to get the bytestream > back into the order in which it is stored in the EEPROM. Please have a look at the ath9k_hw_nvram_swap_data function and eeprom_ops.check_eeprom (for example ath9k_hw_ar9287_check_eeprom). These are performing the conditional swapping (in addition to whatever previous swapping there was). The basic code works like this: read the full EEPROM data into memory (either from PCI bus, ath9k_platform_data or request_firmware), then eeprom_ops.check_eeprom will call ath9k_hw_nvram_swap_data for 16-bit word swapping and afterwards the check_eeprom implementation will doe further swapping. Apart from that your findings seem correct (at least this is identical to how I would interpret the code). > Interestingly, this also seems to happen for ath_ahb_eeprom_read() > even though on that one the value does not get swapped by the bus > accessor, so presumably big-endian machines with a ahb based ath9k > store their eeprom byte-reversed? on AHB the eeprom data has to be provided via ath9k_platform_data / request_firmware mechanism. Thus there is no bus specific swapping, only the ath9k_hw_nvram_swap_data / eeprom_ops.check_eeprom swapping is applied in this case. [0] http://thread.gmane.org/gmane.linux.kernel.wireless.general/153569 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes: > Add documentation how devicetree can be used to configure ath9k based > devices. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > .../devicetree/bindings/net/wireless/qca,ath9k.txt | 59 ++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > > diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > new file mode 100644 > index 0000000..7c62c59 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > @@ -0,0 +1,59 @@ > +* Qualcomm Atheros ath9k wireless devices > + > +This node provides properties for configuring the ath9k wireless device. The > +node is expected to be specified as a child node of the PCI controller to > +which the wireless chip is connected. > + > +Required properties: > +- compatible: Should be "qca,ath9k" Isn't this supposed to use the chipset name? ath9k is the driver name and something like ar9462 is the chip name. I know in ath10k we used "qca,ath10k" but I'm starting to suspect that was a mistake.
On Wednesday, July 13, 2016 10:02:39 AM CEST Kalle Valo wrote: > Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes: > > > Add documentation how devicetree can be used to configure ath9k based > > devices. > > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > --- > > .../devicetree/bindings/net/wireless/qca,ath9k.txt | 59 ++++++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > > > > diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > > new file mode 100644 > > index 0000000..7c62c59 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt > > @@ -0,0 +1,59 @@ > > +* Qualcomm Atheros ath9k wireless devices > > + > > +This node provides properties for configuring the ath9k wireless device. The > > +node is expected to be specified as a child node of the PCI controller to > > +which the wireless chip is connected. > > + > > +Required properties: > > +- compatible: Should be "qca,ath9k" > > Isn't this supposed to use the chipset name? ath9k is the driver name > and something like ar9462 is the chip name. I know in ath10k we used > "qca,ath10k" but I'm starting to suspect that was a mistake. > You are right, but it's actually more complicated than that. For PCI devices, the format of the compatible strings is defined in http://www.o3one.org/hwdocs/openfirmware/pci_supplement_2_1.pdf and it doesn't use the "vendor,device" syntax at all but instead uses pciVVVV,DDDD where VVVV and DDDD are the hexadecimal (without leading 0x) vendor and device ID numbers. The document also specifies seven other formats and in theory you should list them all, but that really only makes sense for Open Firmware that programatically creates those strings. For a hand-written DTS source, I'd just use the shortest one of those. Linux actually doesn't care at all, as PCI drivers don't use the compatible string but instead look at the config register values that were read by the PCI core. For USB, we do basically the same thing, but have very few examples of that as Linux only recently started supporting this. For SDIO devices we should have done the same thing but screwed up when we made the generic binding and we have no policy, the example even lists one that makes no sense at all ("brcm,bcm43xx-fmac") as it is neither a specific device nor something derived from the IDs. For on-chip devices, we should follow the common policy for on-chip devices and use the "vendor,chip-funcion" with fallbacks for compatible devices such as: compatible = "tplink,tp9343-wifi", "qca,qca9561-wifi", "atheros,ar9001-wifi"; For a chip that is branded by TP-Link and derived from a Qualcomm Atheros chip it is 100% compatible with, and in turn derived from an older implementation (just guessing which one was the original ath9k). Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, July 11, 2016 11:21:26 PM CEST Martin Blumenstingl wrote: > On Mon, Jul 11, 2016 at 12:01 AM, Arnd Bergmann <arnd@arndb.de> wrote: > >> ath9k reads the data from the EEPROM into memory. With that property > >> disabled ath9k simply assumes that the endianness of the values in the > >> EEPROM are having the correct endianness for the host system (in other > >> words: no swap is being applied). > >> I am not sure I understand you correctly, but isn't what you are > >> explaining an issue in the ath9k code, rather than in this > >> documentation? > > > > I looked at the code more to find that out now, but I'm more confused > > now, as the eeprom seems to be read as a byte stream, and the endianess > > conversion that the driver performs is not on the data values in it, > > but seems to instead swap the bytes in each 16-bit word, regardless > > of the contents (the values inside of the byte stream are always > > interpreted as big-endian). Is that a correct observation? > that seems to be the case for the ar9003 eeprom. Other implementations > are doing it different, look at ath9k_hw_ar9287_check_eeprom for > example: first ath9k_hw_nvram_swap_data checks the two magic bytes at > the beginning of the data and swaps the bytes in each 16-bit word if > the magic bytes don't match the magic bytes for the "native system > endianness" (see AR5416_EEPROM_MAGIC). Then more swapping is applied. > I asked for more details about the EEPROM format (specifically the > endianness part) here [0] as I don't have access to the datasheets > (all I have is the ath9k code) Ok. > > What I see in ath_pci_eeprom_read() is that the 16-bit words are taken > > from the lower 16 bit of the little-endian AR_EEPROM_STATUS_DATA > > register. As this is coming from a PCI register, it must have a device > > specific endianess that is identical on all CPUs, so in the description > > above, mentioning CPU endianess is a bit confusing. I could not find > > the code that does the conditional byteswap, instead this function > > > > static bool ar9300_eeprom_read_byte(struct ath_hw *ah, int address, > > u8 *buffer) > > { > > u16 val; > > > > if (unlikely(!ath9k_hw_nvram_read(ah, address / 2, &val))) > > return false; > > > > *buffer = (val >> (8 * (address % 2))) & 0xff; > > return true; > > } > > > > evidently assumes that the lower 8 bit of the 16-bit data from PCI > > come first, i.e. it byteswaps on big-endian CPUs to get the bytestream > > back into the order in which it is stored in the EEPROM. > Please have a look at the ath9k_hw_nvram_swap_data function and > eeprom_ops.check_eeprom (for example ath9k_hw_ar9287_check_eeprom). > These are performing the conditional swapping (in addition to whatever > previous swapping there was). > The basic code works like this: read the full EEPROM data into memory > (either from PCI bus, ath9k_platform_data or request_firmware), then > eeprom_ops.check_eeprom will call ath9k_hw_nvram_swap_data for 16-bit > word swapping and afterwards the check_eeprom implementation will doe > further swapping. > Apart from that your findings seem correct (at least this is identical > to how I would interpret the code). Ok, so my interpretation of what this is done for is that the swap in ath9k_hw_nvram_swap_data() is done to compensate for the data that is read byte-reversed from the PCI bus and it is does not swap when the data is read from a file. The result is a structure with big-endian 16-bit and 32-bit members but all fields in the right place. The swapping in ath9k_hw_ar9287_check_eeprom() then turns the big-endian fields into little-endian fields so it can be used on little-endian CPUs without going through le16_to_cpu(). However, the whole thing still looks fragile to me as it doesn't seem to handle the case where we want to swap the values but not the bus. My guess is that we still want to fix the driver to handle this more consistently in order to decide whether a DT property is needed or not. > > Interestingly, this also seems to happen for ath_ahb_eeprom_read() > > even though on that one the value does not get swapped by the bus > > accessor, so presumably big-endian machines with a ahb based ath9k > > store their eeprom byte-reversed? > on AHB the eeprom data has to be provided via ath9k_platform_data / > request_firmware mechanism. Thus there is no bus specific swapping, > only the ath9k_hw_nvram_swap_data / eeprom_ops.check_eeprom swapping > is applied in this case. I guess the header then indicates that none of the swapping is performed. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt new file mode 100644 index 0000000..7c62c59 --- /dev/null +++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt @@ -0,0 +1,59 @@ +* Qualcomm Atheros ath9k wireless devices + +This node provides properties for configuring the ath9k wireless device. The +node is expected to be specified as a child node of the PCI controller to +which the wireless chip is connected. + +Required properties: +- compatible: Should be "qca,ath9k" + +Optional properties: +- reg: Address and length of the register set for the device. +- qca,clk-25mhz: Defines that a 25MHz clock is used +- 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). +- qca,check-eeprom-endianness: When enabled, the driver checks if the + endianness of the EEPROM (based on the two + magic bytes at the start of the EEPROM) + matches the host system's native endianness. + The data will be swapped accordingly if there + is a mismatch. + Leaving this disabled means that the EEPROM + data will be interpreted in the system's + native endianness, regardless of the magic + bytes. + Enable this option only when the magic bytes + are known to indicate the correct endianness + of the EEPROM data, because otherwise the + EEPROM data may be swapped and thus may be + parsed incorrectly. +- qca,disable-2ghz: Overrides the settings from the EEPROM and disables the + 2.4GHz band. Setting this property is only needed + when the RF circuit does not support the 2.4GHz band + while it is enabled nevertheless in the EEPROM. +- qca,disable-5ghz: Overrides the settings from the EEPROM and disables the + 5GHz band. Setting this property is only needed when + the RF circuit does not support the 5GHz band while + it is enabled nevertheless in the EEPROM. +- 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. + +pci { + pcie@0 { + reg = <0 0 0 0 0>; + #interrupt-cells = <1>; + #size-cells = <2>; + #address-cells = <3>; + device_type = "pci"; + + ath9k@0,0 { + compatible = "qca,ath9k"; + reg = <0 0 0 0 0>; + qca,disable-5ghz; + }; + }; +};
Add documentation how devicetree can be used to configure ath9k based devices. Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- .../devicetree/bindings/net/wireless/qca,ath9k.txt | 59 ++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt