| Message ID | 20160821144906.30984-3-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Sunday, August 21, 2016 4:49:03 PM CEST Martin Blumenstingl wrote: > We will default to the system's native endianness for the eepmisc value. > This may be overwritten by the actual calibration data. If it is not > overwritten we interpret the template data in it's native endianness, > meaning that no swapping is required. I'm still skeptical about this one. What is the significance of "native endianess" here? You are keying the endianess of the eeprom tables off the way the CPU operates, but for a PCI device there is no correlation between those two. Arnd
On Mon, Aug 22, 2016 at 1:47 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Sunday, August 21, 2016 4:49:03 PM CEST Martin Blumenstingl wrote: >> We will default to the system's native endianness for the eepmisc value. >> This may be overwritten by the actual calibration data. If it is not >> overwritten we interpret the template data in it's native endianness, >> meaning that no swapping is required. > > I'm still skeptical about this one. What is the significance of "native > endianess" here? You are keying the endianess of the eeprom tables off the > way the CPU operates, but for a PCI device there is no correlation between > those two. (the ar9003 eeprom format and handling is different compared to 9287, def and 4k) ar9003_eeprom.c contains EEPROM templates -> these are compiled into the ath9k kernel module. Values from these templates can be overwritten by the EEPROM found on the actual hardware. This change tries to handle the case where the values in the hardware EEPROM do not override any of the template values (means final EEPROM data = template data). In this case the we can simply rely on the endianness which was used to compile ath9k.ko. I also noted that this is missing a signed-off-by tag as well (sorry for that, I will re-send it together with the other patch next weekend)
On Monday, August 22, 2016 1:56:46 PM CEST Martin Blumenstingl wrote: > On Mon, Aug 22, 2016 at 1:47 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Sunday, August 21, 2016 4:49:03 PM CEST Martin Blumenstingl wrote: > >> We will default to the system's native endianness for the eepmisc value. > >> This may be overwritten by the actual calibration data. If it is not > >> overwritten we interpret the template data in it's native endianness, > >> meaning that no swapping is required. > > > > I'm still skeptical about this one. What is the significance of "native > > endianess" here? You are keying the endianess of the eeprom tables off the > > way the CPU operates, but for a PCI device there is no correlation between > > those two. > (the ar9003 eeprom format and handling is different compared to 9287, > def and 4k) > ar9003_eeprom.c contains EEPROM templates -> these are compiled into > the ath9k kernel module. Values from these templates can be > overwritten by the EEPROM found on the actual hardware. > This change tries to handle the case where the values in the hardware > EEPROM do not override any of the template values (means final EEPROM > data = template data). In this case the we can simply rely on the > endianness which was used to compile ath9k.ko. Ok, I see what you mean now. However, looking at the source now, I also see #define LE16(x) cpu_to_le16(x) #define LE32(x) cpu_to_le32(x) .baseEepHeader = { .regDmn = { LE16(0), LE16(0x1f) }, suggesting that the fields are meant to be little-endian in object code, and your patch does not change that. In fact, Felix's ffdc4cbe5b17 ("ath9k_hw: clean up EEPROM endian handling on AR9003") seems to have corrected this already. Arnd
On Mon, Aug 22, 2016 at 5:31 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday, August 22, 2016 1:56:46 PM CEST Martin Blumenstingl wrote: >> On Mon, Aug 22, 2016 at 1:47 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Sunday, August 21, 2016 4:49:03 PM CEST Martin Blumenstingl wrote: >> >> We will default to the system's native endianness for the eepmisc value. >> >> This may be overwritten by the actual calibration data. If it is not >> >> overwritten we interpret the template data in it's native endianness, >> >> meaning that no swapping is required. >> > >> > I'm still skeptical about this one. What is the significance of "native >> > endianess" here? You are keying the endianess of the eeprom tables off the >> > way the CPU operates, but for a PCI device there is no correlation between >> > those two. >> (the ar9003 eeprom format and handling is different compared to 9287, >> def and 4k) >> ar9003_eeprom.c contains EEPROM templates -> these are compiled into >> the ath9k kernel module. Values from these templates can be >> overwritten by the EEPROM found on the actual hardware. >> This change tries to handle the case where the values in the hardware >> EEPROM do not override any of the template values (means final EEPROM >> data = template data). In this case the we can simply rely on the >> endianness which was used to compile ath9k.ko. > > Ok, I see what you mean now. However, looking at the source now, I > also see > > #define LE16(x) cpu_to_le16(x) > #define LE32(x) cpu_to_le32(x) > .baseEepHeader = { > .regDmn = { LE16(0), LE16(0x1f) }, > > suggesting that the fields are meant to be little-endian in object > code, and your patch does not change that. In fact, Felix's > ffdc4cbe5b17 ("ath9k_hw: clean up EEPROM endian handling on AR9003") > seems to have corrected this already. oh, I totally missed that - thanks for the hint! This means that eepMisc should be 0 (indicates little endian) in all cases (it had this value, but the code was not explicit about it). so please ignore this patch for now, it might cause more harm than good.
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c index ea7b819..6669e36 100644 --- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c +++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c @@ -53,7 +53,7 @@ static const struct ar9300_eeprom ar9300_default = { .txrxMask = 0x77, /* 4 bits tx and 4 bits rx */ .opCapFlags = { .opFlags = AR5416_OPFLAGS_11G | AR5416_OPFLAGS_11A, - .eepMisc = 0, + .eepMisc = AR9300_EEPMISC_DEFAULT_VALUE, }, .rfSilent = 0, .blueToothOptions = 0, @@ -631,7 +631,7 @@ static const struct ar9300_eeprom ar9300_x113 = { .txrxMask = 0x77, /* 4 bits tx and 4 bits rx */ .opCapFlags = { .opFlags = AR5416_OPFLAGS_11A, - .eepMisc = 0, + .eepMisc = AR9300_EEPMISC_DEFAULT_VALUE, }, .rfSilent = 0, .blueToothOptions = 0, @@ -1210,7 +1210,7 @@ static const struct ar9300_eeprom ar9300_h112 = { .txrxMask = 0x77, /* 4 bits tx and 4 bits rx */ .opCapFlags = { .opFlags = AR5416_OPFLAGS_11G | AR5416_OPFLAGS_11A, - .eepMisc = 0, + .eepMisc = AR9300_EEPMISC_DEFAULT_VALUE, }, .rfSilent = 0, .blueToothOptions = 0, @@ -1789,7 +1789,7 @@ static const struct ar9300_eeprom ar9300_x112 = { .txrxMask = 0x77, /* 4 bits tx and 4 bits rx */ .opCapFlags = { .opFlags = AR5416_OPFLAGS_11G | AR5416_OPFLAGS_11A, - .eepMisc = 0, + .eepMisc = AR9300_EEPMISC_DEFAULT_VALUE, }, .rfSilent = 0, .blueToothOptions = 0, @@ -2367,7 +2367,7 @@ static const struct ar9300_eeprom ar9300_h116 = { .txrxMask = 0x33, /* 4 bits tx and 4 bits rx */ .opCapFlags = { .opFlags = AR5416_OPFLAGS_11G | AR5416_OPFLAGS_11A, - .eepMisc = 0, + .eepMisc = AR9300_EEPMISC_DEFAULT_VALUE, }, .rfSilent = 0, .blueToothOptions = 0, diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h index 0a4c736..7e06f12 100644 --- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h +++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h @@ -69,6 +69,12 @@ #define AR9300_BASE_ADDR 0x3ff #define AR9300_BASE_ADDR_512 0x1ff +#ifdef __BIG_ENDIAN +#define AR9300_EEPMISC_DEFAULT_VALUE AR5416_EEPMISC_BIG_ENDIAN +#else +#define AR9300_EEPMISC_DEFAULT_VALUE 0 +#endif + #define AR9300_OTP_BASE \ ((AR_SREV_9340(ah) || AR_SREV_9550(ah)) ? 0x30000 : 0x14000) #define AR9300_OTP_STATUS \