[ath9k-devel] [PATCH 0/5] ath9k: EEPROM swapping improvements

[ath9k-devel] [PATCH 0/5] ath9k: EEPROM swapping improvements

[Martin Blumenstingl]

There are two types of swapping the EEPROM data in the ath9k driver.
Before this series one type of swapping could not be used without the
other.

The first type of swapping looks at the "magic bytes" at the start of
the EEPROM data and performs swab16 on the EEPROM contents if needed.
The second type of swapping is EEPROM format specific and swaps
specific fields within the EEPROM itself (swab16, swab32 - depends on
the EEPROM format).

With this series the second part now looks at the EEPMISC register
inside the EEPROM, which uses a bit to indicate if the EEPROM data
is Big Endian (this is also done by the FreeBSD kernel).
This has a nice advantage: currently there are some out-of-tree hacks
(in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
Big Endian system (= no swab16 is performed) but the EEPROM itself
indicates that it's data is Little Endian. Until now the out-of-tree
code simply did a swab16 before passing the data to ath9k, so ath9k
first did the swab16 - this also enabled the format specific swapping.
These out-of-tree hacks are still working with the new logic, but it
is recommended to remove them.

The last patch in this series allows enabling the EEPROM endianness
swapping via devicetree. This should be fine now since it addresses
the concerns raised by Arnd Bergmann in in the original OF support
patches: [0].

This series depends on my other series (V5):
"add devicetree support to ath9k" - see [1]


[0] http://www.spinics.net/lists/linux-wireless/msg152634.html
[1] https://marc.info/?l=linux-wireless&m=147178988827847&w=2


Martin Blumenstingl (5):
  ath9k: Add a #define for the EEPROM "eepmisc" endianness bit
  ath9k: Set the "big endian" bit of the AR9003 EEPROM templates
  ath9k: Add an eeprom_ops callback for retrieving the eepmisc value
  ath9k: Make the EEPROM swapping check use the eepmisc register
  ath9k: Make EEPROM endianness swapping configurable via devicetree

 .../devicetree/bindings/net/wireless/qca,ath9k.txt | 16 ++++++
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.c     | 21 +++++---
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.h     |  7 ++-
 drivers/net/wireless/ath/ath9k/eeprom.c            | 57 ++++++++++++++++------
 drivers/net/wireless/ath/ath9k/eeprom.h            |  5 +-
 drivers/net/wireless/ath/ath9k/eeprom_4k.c         | 10 +++-
 drivers/net/wireless/ath/ath9k/eeprom_9287.c       | 10 +++-
 drivers/net/wireless/ath/ath9k/eeprom_def.c        | 10 +++-
 drivers/net/wireless/ath/ath9k/init.c              |  6 ++-
 9 files changed, 110 insertions(+), 32 deletions(-)

-- 
2.9.3

[ath9k-devel] [PATCH 1/5] ath9k: Add a #define for the EEPROM "eepmisc" endianness bit

[Martin Blumenstingl]

This replaces a magic number with a named #define. Additionally it
removes two "eeprom format" specific #defines for the "big endianness"
bit which are the same on all eeprom formats.
---
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.c | 3 ++-
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.h | 1 -
 drivers/net/wireless/ath/ath9k/eeprom.h        | 4 +++-
 drivers/net/wireless/ath/ath9k/eeprom_4k.c     | 2 +-
 drivers/net/wireless/ath/ath9k/eeprom_9287.c   | 2 +-
 drivers/net/wireless/ath/ath9k/eeprom_def.c    | 2 +-
 6 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
index 08607d7..ea7b819 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
@@ -3468,7 +3468,8 @@ static u32 ath9k_hw_ar9003_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 					AR5416_OPFLAGS_N_5G_HT20));
 	PR_EEP("Disable 5Ghz HT40", !!(pBase->opCapFlags.opFlags &
 					AR5416_OPFLAGS_N_5G_HT40));
-	PR_EEP("Big Endian", !!(pBase->opCapFlags.eepMisc & 0x01));
+	PR_EEP("Big Endian", !!(pBase->opCapFlags.eepMisc &
+				AR5416_EEPMISC_BIG_ENDIAN));
 	PR_EEP("RF Silent", pBase->rfSilent);
 	PR_EEP("BT option", pBase->blueToothOptions);
 	PR_EEP("Device Cap", pBase->deviceCap);
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h
index 107bcfb..0a4c736 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h
+++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h
@@ -38,7 +38,6 @@
 #define AR9300_NUM_CTLS_2G           12
 #define AR9300_NUM_BAND_EDGES_5G     8
 #define AR9300_NUM_BAND_EDGES_2G     4
-#define AR9300_EEPMISC_BIG_ENDIAN    0x01
 #define AR9300_EEPMISC_WOW           0x02
 #define AR9300_CUSTOMER_DATA_SIZE    20
 
diff --git a/drivers/net/wireless/ath/ath9k/eeprom.h b/drivers/net/wireless/ath/ath9k/eeprom.h
index 4465c65..c466ada 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.h
+++ b/drivers/net/wireless/ath/ath9k/eeprom.h
@@ -161,6 +161,9 @@
 #define AR5416_EEP_TXGAIN_ORIGINAL         0
 #define AR5416_EEP_TXGAIN_HIGH_POWER       1
 
+/* Endianness of EEPROM content */
+#define AR5416_EEPMISC_BIG_ENDIAN          0x01
+
 #define AR5416_EEP4K_START_LOC                64
 #define AR5416_EEP4K_NUM_2G_CAL_PIERS         3
 #define AR5416_EEP4K_NUM_2G_CCK_TARGET_POWERS 3
@@ -191,7 +194,6 @@
 #define AR9287_NUM_CTLS              	12
 #define AR9287_NUM_BAND_EDGES        	4
 #define AR9287_PD_GAIN_ICEPTS           1
-#define AR9287_EEPMISC_BIG_ENDIAN       0x01
 #define AR9287_EEPMISC_WOW              0x02
 #define AR9287_MAX_CHAINS               2
 #define AR9287_ANT_16S                  32
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_4k.c b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
index 5da0826..780cb49 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_4k.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
@@ -154,7 +154,7 @@ static u32 ath9k_hw_4k_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 					AR5416_OPFLAGS_N_5G_HT20));
 	PR_EEP("Disable 5Ghz HT40", !!(pBase->opCapFlags &
 					AR5416_OPFLAGS_N_5G_HT40));
-	PR_EEP("Big Endian", !!(pBase->eepMisc & 0x01));
+	PR_EEP("Big Endian", !!(pBase->eepMisc & AR5416_EEPMISC_BIG_ENDIAN));
 	PR_EEP("Cal Bin Major Ver", (pBase->binBuildNumber >> 24) & 0xFF);
 	PR_EEP("Cal Bin Minor Ver", (pBase->binBuildNumber >> 16) & 0xFF);
 	PR_EEP("Cal Bin Build", (pBase->binBuildNumber >> 8) & 0xFF);
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_9287.c b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
index 1a019a3..f483ba2 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_9287.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
@@ -150,7 +150,7 @@ static u32 ath9k_hw_ar9287_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 					AR5416_OPFLAGS_N_5G_HT20));
 	PR_EEP("Disable 5Ghz HT40", !!(pBase->opCapFlags &
 					AR5416_OPFLAGS_N_5G_HT40));
-	PR_EEP("Big Endian", !!(pBase->eepMisc & 0x01));
+	PR_EEP("Big Endian", !!(pBase->eepMisc & AR5416_EEPMISC_BIG_ENDIAN));
 	PR_EEP("Cal Bin Major Ver", (pBase->binBuildNumber >> 24) & 0xFF);
 	PR_EEP("Cal Bin Minor Ver", (pBase->binBuildNumber >> 16) & 0xFF);
 	PR_EEP("Cal Bin Build", (pBase->binBuildNumber >> 8) & 0xFF);
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_def.c b/drivers/net/wireless/ath/ath9k/eeprom_def.c
index 959682f..39b1b27 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_def.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_def.c
@@ -232,7 +232,7 @@ static u32 ath9k_hw_def_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 					AR5416_OPFLAGS_N_5G_HT20));
 	PR_EEP("Disable 5Ghz HT40", !!(pBase->opCapFlags &
 					AR5416_OPFLAGS_N_5G_HT40));
-	PR_EEP("Big Endian", !!(pBase->eepMisc & 0x01));
+	PR_EEP("Big Endian", !!(pBase->eepMisc & AR5416_EEPMISC_BIG_ENDIAN));
 	PR_EEP("Cal Bin Major Ver", (pBase->binBuildNumber >> 24) & 0xFF);
 	PR_EEP("Cal Bin Minor Ver", (pBase->binBuildNumber >> 16) & 0xFF);
 	PR_EEP("Cal Bin Build", (pBase->binBuildNumber >> 8) & 0xFF);
-- 
2.9.3

[ath9k-devel] [PATCH 2/5] ath9k: Set the "big endian" bit of the AR9003 EEPROM templates

[Martin Blumenstingl]

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.
---
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.c | 10 +++++-----
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.h |  6 ++++++
 2 files changed, 11 insertions(+), 5 deletions(-)

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 \
-- 
2.9.3

[ath9k-devel] [PATCH 3/5] ath9k: Add an eeprom_ops callback for retrieving the eepmisc value

[Martin Blumenstingl]

This allows deciding if we have to swap the EEPROM data (so it matches
the system's native endianness) even if no byte-swapping (swab16, based on
the first two bytes in the EEPROM) is needed.
---
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.c | 8 +++++++-
 drivers/net/wireless/ath/ath9k/eeprom.h        | 1 +
 drivers/net/wireless/ath/ath9k/eeprom_4k.c     | 8 +++++++-
 drivers/net/wireless/ath/ath9k/eeprom_9287.c   | 8 +++++++-
 drivers/net/wireless/ath/ath9k/eeprom_def.c    | 8 +++++++-
 5 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
index 6669e36..7d62d1c 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
@@ -5498,6 +5498,11 @@ unsigned int ar9003_get_paprd_scale_factor(struct ath_hw *ah,
 	}
 }
 
+static u8 ar9003_get_eepmisc(struct ath_hw *ah)
+{
+	return ah->eeprom.map4k.baseEepHeader.eepMisc;
+}
+
 const struct eeprom_ops eep_ar9300_ops = {
 	.check_eeprom = ath9k_hw_ar9300_check_eeprom,
 	.get_eeprom = ath9k_hw_ar9300_get_eeprom,
@@ -5508,5 +5513,6 @@ const struct eeprom_ops eep_ar9300_ops = {
 	.set_board_values = ath9k_hw_ar9300_set_board_values,
 	.set_addac = ath9k_hw_ar9300_set_addac,
 	.set_txpower = ath9k_hw_ar9300_set_txpower,
-	.get_spur_channel = ath9k_hw_ar9300_get_spur_channel
+	.get_spur_channel = ath9k_hw_ar9300_get_spur_channel,
+	.get_eepmisc = ar9003_get_eepmisc
 };
diff --git a/drivers/net/wireless/ath/ath9k/eeprom.h b/drivers/net/wireless/ath/ath9k/eeprom.h
index c466ada..408cfa7 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.h
+++ b/drivers/net/wireless/ath/ath9k/eeprom.h
@@ -655,6 +655,7 @@ struct eeprom_ops {
 			   u16 cfgCtl, u8 twiceAntennaReduction,
 			   u8 powerLimit, bool test);
 	u16 (*get_spur_channel)(struct ath_hw *ah, u16 i, bool is2GHz);
+	u8 (*get_eepmisc)(struct ath_hw *ah);
 };
 
 void ath9k_hw_analog_shift_regwrite(struct ath_hw *ah, u32 reg, u32 val);
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_4k.c b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
index 780cb49..7cd9c64 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_4k.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
@@ -1064,6 +1064,11 @@ static u16 ath9k_hw_4k_get_spur_channel(struct ath_hw *ah, u16 i, bool is2GHz)
 	return ah->eeprom.map4k.modalHeader.spurChans[i].spurChan;
 }
 
+static u8 ath9k_hw_4k_get_eepmisc(struct ath_hw *ah)
+{
+	return ah->eeprom.map4k.baseEepHeader.eepMisc;
+}
+
 const struct eeprom_ops eep_4k_ops = {
 	.check_eeprom		= ath9k_hw_4k_check_eeprom,
 	.get_eeprom		= ath9k_hw_4k_get_eeprom,
@@ -1073,5 +1078,6 @@ const struct eeprom_ops eep_4k_ops = {
 	.get_eeprom_rev		= ath9k_hw_4k_get_eeprom_rev,
 	.set_board_values	= ath9k_hw_4k_set_board_values,
 	.set_txpower		= ath9k_hw_4k_set_txpower,
-	.get_spur_channel	= ath9k_hw_4k_get_spur_channel
+	.get_spur_channel	= ath9k_hw_4k_get_spur_channel,
+	.get_eepmisc		= ath9k_hw_4k_get_eepmisc
 };
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_9287.c b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
index f483ba2..e1ba9bc 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_9287.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
@@ -986,6 +986,11 @@ static u16 ath9k_hw_ar9287_get_spur_channel(struct ath_hw *ah,
 	return ah->eeprom.map9287.modalHeader.spurChans[i].spurChan;
 }
 
+static u8 ath9k_hw_ar9287_get_eepmisc(struct ath_hw *ah)
+{
+	return ah->eeprom.map9287.baseEepHeader.eepMisc;
+}
+
 const struct eeprom_ops eep_ar9287_ops = {
 	.check_eeprom		= ath9k_hw_ar9287_check_eeprom,
 	.get_eeprom		= ath9k_hw_ar9287_get_eeprom,
@@ -995,5 +1000,6 @@ const struct eeprom_ops eep_ar9287_ops = {
 	.get_eeprom_rev		= ath9k_hw_ar9287_get_eeprom_rev,
 	.set_board_values	= ath9k_hw_ar9287_set_board_values,
 	.set_txpower		= ath9k_hw_ar9287_set_txpower,
-	.get_spur_channel	= ath9k_hw_ar9287_get_spur_channel
+	.get_spur_channel	= ath9k_hw_ar9287_get_spur_channel,
+	.get_eepmisc		= ath9k_hw_ar9287_get_eepmisc
 };
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_def.c b/drivers/net/wireless/ath/ath9k/eeprom_def.c
index 39b1b27..087bdb7 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_def.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_def.c
@@ -1317,6 +1317,11 @@ static u16 ath9k_hw_def_get_spur_channel(struct ath_hw *ah, u16 i, bool is2GHz)
 	return ah->eeprom.def.modalHeader[is2GHz].spurChans[i].spurChan;
 }
 
+static u8 ath9k_hw_def_get_eepmisc(struct ath_hw *ah)
+{
+	return ah->eeprom.def.baseEepHeader.eepMisc;
+}
+
 const struct eeprom_ops eep_def_ops = {
 	.check_eeprom		= ath9k_hw_def_check_eeprom,
 	.get_eeprom		= ath9k_hw_def_get_eeprom,
@@ -1327,5 +1332,6 @@ const struct eeprom_ops eep_def_ops = {
 	.set_board_values	= ath9k_hw_def_set_board_values,
 	.set_addac		= ath9k_hw_def_set_addac,
 	.set_txpower		= ath9k_hw_def_set_txpower,
-	.get_spur_channel	= ath9k_hw_def_get_spur_channel
+	.get_spur_channel	= ath9k_hw_def_get_spur_channel,
+	.get_eepmisc		= ath9k_hw_def_get_eepmisc
 };
-- 
2.9.3

[ath9k-devel] [PATCH 4/5] ath9k: Make the EEPROM swapping check use the eepmisc register

[Martin Blumenstingl]

There are two ways of swapping the EEPROM data in the ath9k driver:
1) swab16 based on the first two EEPROM "magic" bytes (same for all
   EEPROM formats)
2) field and EEPROM format specific swab16/swab32 (different for
   eeprom_def, eeprom_4k and eeprom_9287)

The result of the first check was used to also enable the second swap.
This behavior seems incorrect, since the data may only be byte-swapped
(afterwards the data could be in the correct endianness).
Thus we introduce a separate check based on the "eepmisc" register
(which is part of the EEPROM data). When bit 0 is set, then the EEPROM
format specific values are in "big endian". This is also done by the
FreeBSD kernel, see [0] for example.

This allows us to parse EEPROMs with the "correct" magic bytes but
swapped EEPROM format specific values. These EEPROMs (mostly found in
lantiq and broadcom based big endian MIPS based devices) only worked
due to platform specific "hacks" which swapped the EEPROM so the
magic was inverted, which also enabled the format specific swapping.
With this patch the old behavior is still supported, but neither
recommended nor needed anymore.

[0]
https://github.com/freebsd/freebsd/blob/50719b56d9ce8d7d4beb53b16e9edb2e9a4a7a18/sys/dev/ath/ath_hal/ah_eeprom_9287.c#L351
---
 drivers/net/wireless/ath/ath9k/eeprom.c | 57 ++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/eeprom.c b/drivers/net/wireless/ath/ath9k/eeprom.c
index a449588..8138a1d 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom.c
@@ -155,11 +155,19 @@ bool ath9k_hw_nvram_read(struct ath_hw *ah, u32 off, u16 *data)
 	return ret;
 }
 
+#ifdef __BIG_ENDIAN
+#define EXPECTED_EEPMISC_VAL AR5416_EEPMISC_BIG_ENDIAN
+#else
+#define EXPECTED_EEPMISC_VAL 0
+#endif
+
 int ath9k_hw_nvram_swap_data(struct ath_hw *ah, bool *swap_needed, int size)
 {
 	u16 magic;
 	u16 *eepdata;
+	u8 eepmisc;
 	int i;
+	bool needs_byteswap = false;
 	struct ath_common *common = ath9k_hw_common(ah);
 
 	if (!ath9k_hw_nvram_read(ah, AR5416_EEPROM_MAGIC_OFFSET, &magic)) {
@@ -167,36 +175,53 @@ int ath9k_hw_nvram_swap_data(struct ath_hw *ah, bool *swap_needed, int size)
 		return -EIO;
 	}
 
-	*swap_needed = false;
 	if (swab16(magic) == AR5416_EEPROM_MAGIC) {
+		needs_byteswap = true;
+		ath_dbg(common, EEPROM,
+			"EEPROM needs byte-swapping to correct endianness.\n");
+	} else if (magic != AR5416_EEPROM_MAGIC) {
+		if (ath9k_hw_use_flash(ah)) {
+			ath_dbg(common, EEPROM,
+				"Ignoring invalid EEPROM magic (0x%04x).\n",
+				magic);
+		} else {
+			ath_err(common,
+				"Invalid EEPROM magic (0x%04x).\n", magic);
+			return -EINVAL;
+		}
+	}
+
+	if (needs_byteswap) {
 		if (ah->ah_flags & AH_NO_EEP_SWAP) {
 			ath_info(common,
 				 "Ignoring endianness difference in EEPROM magic bytes.\n");
 		} else {
-			*swap_needed = true;
-		}
-	} else if (magic != AR5416_EEPROM_MAGIC) {
-		if (ath9k_hw_use_flash(ah))
-			return 0;
+			eepdata = (u16 *)(&ah->eeprom);
 
-		ath_err(common,
-			"Invalid EEPROM Magic (0x%04x).\n", magic);
-		return -EINVAL;
+			for (i = 0; i < size; i++)
+				eepdata[i] = swab16(eepdata[i]);
+		}
 	}
 
-	eepdata = (u16 *)(&ah->eeprom);
-
-	if (*swap_needed) {
-		ath_dbg(common, EEPROM,
-			"EEPROM Endianness is not native.. Changing.\n");
+	*swap_needed = false;
 
-		for (i = 0; i < size; i++)
-			eepdata[i] = swab16(eepdata[i]);
+	eepmisc = ah->eep_ops->get_eepmisc(ah);
+	if ((eepmisc & AR5416_EEPMISC_BIG_ENDIAN) != EXPECTED_EEPMISC_VAL) {
+		if (ah->ah_flags & AH_NO_EEP_SWAP) {
+			ath_info(common,
+				 "Ignoring endianness difference in eepmisc register.\n");
+		} else {
+			*swap_needed = true;
+			ath_dbg(common, EEPROM,
+				"EEPROM needs swapping according to the eepmisc register.\n");
+		}
 	}
 
 	return 0;
 }
 
+#undef EXPECTED_EEPMISC_VAL
+
 bool ath9k_hw_nvram_validate_checksum(struct ath_hw *ah, int size)
 {
 	u32 i, sum = 0;
-- 
2.9.3

[ath9k-devel] [PATCH 5/5] ath9k: Make EEPROM endianness swapping configurable via devicetree

[Martin Blumenstingl]

The endianness swapping is disabled by default since many devices have
EEPROMs with incorrect endianness magic bytes (at the beginning).
Devices where the EEPROM is known to indicate the correct endianness
can enable the new flag to enable swapping when required.
This behavior is consistent with ath9k_platform_data where the endian
check also has to be enabled explicitly.
---
 .../devicetree/bindings/net/wireless/qca,ath9k.txt       | 16 ++++++++++++++++
 drivers/net/wireless/ath/ath9k/init.c                    |  6 +++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
index 98065ad..05c54c4 100644
--- a/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
+++ b/Documentation/devicetree/bindings/net/wireless/qca,ath9k.txt
@@ -17,6 +17,22 @@ Optional properties:
 			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 (using two checks,
+				one is based on the two magic bytes at the
+				start of the EEPROM and a second one which
+				relies on a flag within the EEPROM data)
+				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 always be interpreted in the
+				system's native endianness.
+				Enable this option only when the EEPROMs
+				endianness identifiers are known to be
+				correct, because otherwise the EEPROM data
+				may be swapped and thus interpreted
+				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
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index c123145..d123977 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -581,6 +581,11 @@ static int ath9k_of_init(struct ath_softc *sc)
 	if (of_property_read_bool(np, "qca,disable-5ghz"))
 		ah->disable_5ghz = true;
 
+	if (of_property_read_bool(np, "qca,check-eeprom-endianness"))
+		ah->ah_flags &= ~AH_NO_EEP_SWAP;
+	else
+		ah->ah_flags |= AH_NO_EEP_SWAP;
+
 	if (of_property_read_bool(np, "qca,no-eeprom")) {
 		/* ath9k-eeprom-<bus>-<id>.bin */
 		scnprintf(eeprom_name, sizeof(eeprom_name),
@@ -597,7 +602,6 @@ static int ath9k_of_init(struct ath_softc *sc)
 		ether_addr_copy(common->macaddr, mac);
 
 	ah->ah_flags &= ~AH_USE_EEPROM;
-	ah->ah_flags |= AH_NO_EEP_SWAP;
 
 	return 0;
 }
-- 
2.9.3

[ath9k-devel] [PATCH 1/5] ath9k: Add a #define for the EEPROM "eepmisc" endianness bit

[Arnd Bergmann]

On Sunday, August 21, 2016 4:49:02 PM CEST Martin Blumenstingl wrote:
> This replaces a magic number with a named #define. Additionally it
> removes two "eeprom format" specific #defines for the "big endianness"
> bit which are the same on all eeprom formats.
> 

Looks good, but missing a Signed-off-by: tag.

	Arnd

[ath9k-devel] [PATCH 2/5] ath9k: Set the "big endian" bit of the AR9003 EEPROM templates

[Arnd Bergmann]

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

[ath9k-devel] [PATCH 5/5] ath9k: Make EEPROM endianness swapping configurable via devicetree

[Arnd Bergmann]

On Sunday, August 21, 2016 4:49:06 PM CEST Martin Blumenstingl wrote:
> +- qca,check-eeprom-endianness: When enabled, the driver checks if the
> +                               endianness of the EEPROM (using two checks,
> +                               one is based on the two magic bytes at the
> +                               start of the EEPROM and a second one which
> +                               relies on a flag within the EEPROM data)
> +                               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 always be interpreted in the
> +                               system's native endianness.
> +                               Enable this option only when the EEPROMs
> +                               endianness identifiers are known to be
> +                               correct, because otherwise the EEPROM data
> +                               may be swapped and thus interpreted
> +                               incorrectly.

The property should not describe the driver behavior, but instead
describe what the hardware is like.

A default of "system's native endianess" should not be specified
in a binding, as the same DTB can be used with both big-endian or
little-endian kernels on some architectures (ARM and PowerPC among
others), so disabling the property means that it is guaranteed to
be broken on one of the two.

If we have no reliable way to check the endianess in all combinations,
could we have two properties "eeprom-big-endian" and
"eeprom-little-endian" and mandate that one of them is always used
when discovering the device using DT?

	Arnd

[ath9k-devel] [PATCH 2/5] ath9k: Set the "big endian" bit of the AR9003 EEPROM templates

[Martin Blumenstingl]

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)

[ath9k-devel] [PATCH 2/5] ath9k: Set the "big endian" bit of the AR9003 EEPROM templates

[Arnd Bergmann]

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

[ath9k-devel] [PATCH 2/5] ath9k: Set the "big endian" bit of the AR9003 EEPROM templates

[Martin Blumenstingl]

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.

[ath9k-devel] [PATCH 5/5] ath9k: Make EEPROM endianness swapping configurable via devicetree

[Martin Blumenstingl]

On Mon, Aug 22, 2016 at 1:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sunday, August 21, 2016 4:49:06 PM CEST Martin Blumenstingl wrote:
>> +- qca,check-eeprom-endianness: When enabled, the driver checks if the
>> +                               endianness of the EEPROM (using two checks,
>> +                               one is based on the two magic bytes at the
>> +                               start of the EEPROM and a second one which
>> +                               relies on a flag within the EEPROM data)
>> +                               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 always be interpreted in the
>> +                               system's native endianness.
>> +                               Enable this option only when the EEPROMs
>> +                               endianness identifiers are known to be
>> +                               correct, because otherwise the EEPROM data
>> +                               may be swapped and thus interpreted
>> +                               incorrectly.
>
> The property should not describe the driver behavior, but instead
> describe what the hardware is like.
>
> A default of "system's native endianess" should not be specified
> in a binding, as the same DTB can be used with both big-endian or
> little-endian kernels on some architectures (ARM and PowerPC among
> others), so disabling the property means that it is guaranteed to
> be broken on one of the two.
OK, I went back to have a separate look at the whole issue again.
Let's recap, there are two types of swapping:
1. swab16 for the whole EEPROM data
2. EEPROM format specific swapping (for all u16 and u32 values)

Actually I am not 100% sure what #1 exists. In OpenWrt and LEDE it's
only used by the brcm63xx and lantiq targets (I personally have only
lantiq based devices, so that's what I can test with).
Without patch 4 from this series the EEPROM data is loaded like this:
1. out of tree code extracts the EEPROM data from NOR/SPI/NAND flash
2. board specific entry (usually device-tree) tells the code to apply
swab16 to the data
3. board specific entry (usually device-tree again) sets the
"endian_check" property in the ath9k_platform_data to true
4.a ath9k sees that the magic bytes are not matching and applies swab16
4.b while doing that it notifies the EEPROM format specific swapping

However, with patch 4 from this series steps 4.a and 4.b are replaced with:
4. ath9k checks the eepMisc field of the EEPROM and applies the EEPROM
format specific swapping

Currently this code is still guarded by a check whether swapping is
enabled or not.
However, FreeBSD uses the same check but has no guarding if-clause for it.

TL;DR: if we remove if (ah->ah_flags & AH_NO_EEP_SWAP) from patch 4 we
don't need an extra device-tree property.

The question is how we can test this properly:
I can test this on the boards I own (3 in total) - but I don't think
that this is enough.
Maybe we can test this together with some LEDE people - this should
get us test-coverage for embedded devices.
I'm open to more/better suggestions


Regards,
Martin

[ath9k-devel] [PATCH 5/5] ath9k: Make EEPROM endianness swapping configurable via devicetree

[Arnd Bergmann]

On Sunday 28 August 2016, Martin Blumenstingl wrote:
> On Mon, Aug 22, 2016 at 1:52 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sunday, August 21, 2016 4:49:06 PM CEST Martin Blumenstingl wrote:
> >> +- qca,check-eeprom-endianness: When enabled, the driver checks if the
> >> +                               endianness of the EEPROM (using two checks,
> >> +                               one is based on the two magic bytes at the
> >> +                               start of the EEPROM and a second one which
> >> +                               relies on a flag within the EEPROM data)
> >> +                               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 always be interpreted in the
> >> +                               system's native endianness.
> >> +                               Enable this option only when the EEPROMs
> >> +                               endianness identifiers are known to be
> >> +                               correct, because otherwise the EEPROM data
> >> +                               may be swapped and thus interpreted
> >> +                               incorrectly.
> >
> > The property should not describe the driver behavior, but instead
> > describe what the hardware is like.
> >
> > A default of "system's native endianess" should not be specified
> > in a binding, as the same DTB can be used with both big-endian or
> > little-endian kernels on some architectures (ARM and PowerPC among
> > others), so disabling the property means that it is guaranteed to
> > be broken on one of the two.
> OK, I went back to have a separate look at the whole issue again.
> Let's recap, there are two types of swapping:
> 1. swab16 for the whole EEPROM data
> 2. EEPROM format specific swapping (for all u16 and u32 values)

Right, this is part of what makes the suggested DT property
a bit problematic (it's not obvious which swap is referred to),
though the other part is more important.

Note that for #1, there isn't really a big-endian and a little-endian
variant, only one that is correct and one that is incorrect (i.e.
fields are in the wrong place). In either case, it should be
independent of the CPU endianess.

> Actually I am not 100% sure what #1 exists. In OpenWrt and LEDE it's
> only used by the brcm63xx and lantiq targets (I personally have only
> lantiq based devices, so that's what I can test with).

Ok.

> Without patch 4 from this series the EEPROM data is loaded like this:
> 1. out of tree code extracts the EEPROM data from NOR/SPI/NAND flash
> 2. board specific entry (usually device-tree) tells the code to apply
> swab16 to the data
> 3. board specific entry (usually device-tree again) sets the
> "endian_check" property in the ath9k_platform_data to true
> 4.a ath9k sees that the magic bytes are not matching and applies swab16
> 4.b while doing that it notifies the EEPROM format specific swapping
> 
> However, with patch 4 from this series steps 4.a and 4.b are replaced with:
> 4. ath9k checks the eepMisc field of the EEPROM and applies the EEPROM
> format specific swapping

I think the intention of the patch is right, but it needs to go further:
It seems wrong to have 'u32' members e.g. in

struct modal_eep_ar9287_header {
        u32 antCtrlChain[AR9287_MAX_CHAINS];
        u32 antCtrlCommon;

and then do a swab32() on them. I suspect these should just be __le32 
(and __le16, respectively where needed), using appropriate accessors
everywhere that those fields are being read instead of having one function
that tries to turn them into cpu-native ordering.

If we can manage to convert the code into doing this consistently,
maybe only the 16-bit swaps of the data stream are left over.

	Arnd

[ath9k-devel] [PATCH 5/5] ath9k: Make EEPROM endianness swapping configurable via devicetree

[Martin Blumenstingl]

On Mon, Aug 29, 2016 at 2:10 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> Without patch 4 from this series the EEPROM data is loaded like this:
>> 1. out of tree code extracts the EEPROM data from NOR/SPI/NAND flash
>> 2. board specific entry (usually device-tree) tells the code to apply
>> swab16 to the data
>> 3. board specific entry (usually device-tree again) sets the
>> "endian_check" property in the ath9k_platform_data to true
>> 4.a ath9k sees that the magic bytes are not matching and applies swab16
>> 4.b while doing that it notifies the EEPROM format specific swapping
>>
>> However, with patch 4 from this series steps 4.a and 4.b are replaced with:
>> 4. ath9k checks the eepMisc field of the EEPROM and applies the EEPROM
>> format specific swapping
>
> I think the intention of the patch is right, but it needs to go further:
> It seems wrong to have 'u32' members e.g. in
>
> struct modal_eep_ar9287_header {
>         u32 antCtrlChain[AR9287_MAX_CHAINS];
>         u32 antCtrlCommon;
>
> and then do a swab32() on them. I suspect these should just be __le32
> (and __le16, respectively where needed), using appropriate accessors
> everywhere that those fields are being read instead of having one function
> that tries to turn them into cpu-native ordering.
I'm not sure if we want those fields to be __le32:
BIT(0) in the eepMisc field indicates whether to interpret the data as
big or little endian.
When this bit is set we would want these fields to be __be32 instead -
so I guess the current implementation is "OK" for this specific
use-case.

> If we can manage to convert the code into doing this consistently,
> maybe only the 16-bit swaps of the data stream are left over.
 we don't need the 16bit swap anymore - at least on the lantiq devices
that I know (not sure about other platforms / devices though).


Martin

[ath9k-devel] [PATCH 5/5] ath9k: Make EEPROM endianness swapping configurable via devicetree

[Arnd Bergmann]

On Monday 29 August 2016, Martin Blumenstingl wrote:
> > and then do a swab32() on them. I suspect these should just be __le32
> > (and __le16, respectively where needed), using appropriate accessors
> > everywhere that those fields are being read instead of having one function
> > that tries to turn them into cpu-native ordering.
> I'm not sure if we want those fields to be __le32:
> BIT(0) in the eepMisc field indicates whether to interpret the data as
> big or little endian.
> When this bit is set we would want these fields to be __be32 instead -
> so I guess the current implementation is "OK" for this specific
> use-case.

Ok, I see. It's still confusing because it's different from how other
drivers handle this (case in point: I was very confused by this). 

Do you see any downsides in changing the code to consistently annotate
the eeprom as either __le or __be (whichever is more common), using
the respective endianess accessors, and then doing the swap if the
data we read is the opposite way?

	Arnd

[ath9k-devel] [PATCH 5/5] ath9k: Make EEPROM endianness swapping configurable via devicetree

[Martin Blumenstingl]

On Mon, Aug 29, 2016 at 11:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 29 August 2016, Martin Blumenstingl wrote:
>> > and then do a swab32() on them. I suspect these should just be __le32
>> > (and __le16, respectively where needed), using appropriate accessors
>> > everywhere that those fields are being read instead of having one function
>> > that tries to turn them into cpu-native ordering.
>> I'm not sure if we want those fields to be __le32:
>> BIT(0) in the eepMisc field indicates whether to interpret the data as
>> big or little endian.
>> When this bit is set we would want these fields to be __be32 instead -
>> so I guess the current implementation is "OK" for this specific
>> use-case.
>
> Ok, I see. It's still confusing because it's different from how other
> drivers handle this (case in point: I was very confused by this).
lesson learned: specification should state that data is _either_ big
or little endian, no mix between these two

> Do you see any downsides in changing the code to consistently annotate
> the eeprom as either __le or __be (whichever is more common), using
> the respective endianess accessors, and then doing the swap if the
> data we read is the opposite way?
I am assuming you mean leNN_to_cpu() and beNN_to_cpu() here?

old logic:
- reading data: simply access the struct fields

LE system:
- LE EEPROM -> no swap will be applied
- BE EEPROM -> swab16 / swab32 the fields
BE system:
- LE EEPROM -> swab16 / swab32 the fields
- BE EEPROM -> no swap will be applied

new logic (assuming that we went for __le16/__le32 fields):
- reading data: use le16_to_cpu and le32_to_cpu for all fields

LE system:
- LE EEPROM -> no swap will be applied
- BE EEPROM -> be16_to_cpu / be32_to_cpu (or swab16 / swab32 as before)
BE system:
- LE EEPROM -> le16_to_cpu / le32_to_cpu (or swab16 / swab32 as before)
- BE EEPROM -> no swap will be applied

There is one downside of the "new approach" I can think of: you need
to swap the data twice in some cases (BE EEPROM on a BE machine).
- first swap while writing the data to __le16/__le32 fields
- second swap while reading the data from the __le16/__le32 fields
If you forget to swap a field in either place then things will be broken.

Maybe someone else wants to state his/her opinion on this - I guess
some fresh thoughts could help us a lot!


Regards,
Martin

[ath9k-devel] [PATCH 5/5] ath9k: Make EEPROM endianness swapping configurable via devicetree

[Arnd Bergmann]

On Tuesday 30 August 2016, Martin Blumenstingl wrote:
> new logic (assuming that we went for __le16/__le32 fields):
> - reading data: use le16_to_cpu and le32_to_cpu for all fields
> 
> LE system:
> - LE EEPROM -> no swap will be applied
> - BE EEPROM -> be16_to_cpu / be32_to_cpu (or swab16 / swab32 as before)
> BE system:
> - LE EEPROM -> le16_to_cpu / le32_to_cpu (or swab16 / swab32 as before)
> - BE EEPROM -> no swap will be applied

I think this should be:

LE and BE systems:
 - LE EEPROM -> no swap will be applied
 - BE EEPROM -> or swab16 / swab32

The upside of this is that we no longer care about what the CPU is,
and in my opinion that makes the code easier to understand.

> There is one downside of the "new approach" I can think of: you need
> to swap the data twice in some cases (BE EEPROM on a BE machine).
> - first swap while writing the data to __le16/__le32 fields
> - second swap while reading the data from the __le16/__le32 fields
> If you forget to swap a field in either place then things will be broken.

Correct. Fortunately, "make C=1" with sparse helps you find those bugs
at compile time.

> Maybe someone else wants to state his/her opinion on this - I guess
> some fresh thoughts could help us a lot!

Yes, that would be helpful. It's possible I'm missing something here,
or that changing this will just add confusion with other people.

	Arnd

[ath9k-devel] [PATCH v2 0/7] ath9k: EEPROM swapping improvements

[Martin Blumenstingl]

There are two types of swapping the EEPROM data in the ath9k driver.
Before this series one type of swapping could not be used without the
other.

The first type of swapping looks at the "magic bytes" at the start of
the EEPROM data and performs swab16 on the EEPROM contents if needed.
The second type of swapping is EEPROM format specific and swaps
specific fields within the EEPROM itself (swab16, swab32 - depends on
the EEPROM format).

With this series the second part now looks at the EEPMISC register
inside the EEPROM, which uses a bit to indicate if the EEPROM data
is Big Endian (this is also done by the FreeBSD kernel).
This has a nice advantage: currently there are some out-of-tree hacks
(in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
Big Endian system (= no swab16 is performed) but the EEPROM itself
indicates that it's data is Little Endian. Until now the out-of-tree
code simply did a swab16 before passing the data to ath9k, so ath9k
first did the swab16 - this also enabled the format specific swapping.
These out-of-tree hacks are still working with the new logic, but it
is recommended to remove them. This implementation is based on a
discussion with Arnd Bergmann who raised concerns about the
robustness and portability of the swapping logic in the original OF
support patch review, see [0].

After a second round of patches (= v1 of this series) neither Arnd
Bergmann nor I were really happy with the complexity of the EEPROM
swapping logic. Based on a discussion (see [1] and [2]) we decided
that ath9k should use a defined format (specifying the endianness
of the data - I went with __le16 and __le32) when accessing the
EEPROM fields. A benefit of this is that we enable the EEPMISC based
swapping logic by default, just like the FreeBSD driver, see [3]. On
the devices which I have tested (see below) ath9k now works without
having to specify the "endian_check" field in ath9k_platform_data (or
a similar logic which could provide this via devicetree) as ath9k now
detects the endianness automatically. Only EEPROMs which are mangled
by some out-of-tree code still need the endian_check flag (or one can
simply remove that mangling from the out-of-tree code).

Testing:
- tested by myself on AR9287 with Big Endian EEPROM
- tested by myself on AR9227 with Little Endian EEPROM
- tested by myself on AR9381 (using the ar9003_eeprom implementation,
  which did not suffer from this whole problem)
- how do we proceed with testing? maybe we could keep this in a
  feature-branch and add these patches to LEDE once we have an ACK to
  get more people to test this

This series depends on my other series (v7):
"add devicetree support to ath9k" - see [4]

Changes since v1:
- reworked description in the cover-letter to describe the reasons
  behind the new patch 7
- reworked patch "Set the "big endian" bit of the AR9003 EEPROM
  templates" as ar9003_eeprom.c sets all values as Little Endian, thus
  the Big Endian bit should never be set (the new patch makes this
  clear)
- dropped "ath9k: Make EEPROM endianness swapping configurable via
  devicetree" as it is not needed anymore with the new logic from
  patch 7
- added patches 4 and 5 as small cleanup (this made it easier to
  implement the le{16,32}_to_cpu() changes where needed)


[0] http://www.spinics.net/lists/linux-wireless/msg152634.html
[1] https://marc.info/?l=linux-wireless&m=147250597503174&w=2
[2] https://marc.info/?l=linux-wireless&m=147254388611344&w=2
[3] https://github.com/freebsd/freebsd/blob/50719b56d9ce8d7d4beb53b16e9edb2e9a4a7a18/sys/dev/ath/ath_hal/ah_eeprom_9287.c#L351
[4] https://marc.info/?l=linux-wireless&m=147544488619822&w=2

Martin Blumenstingl (7):
  ath9k: Add a #define for the EEPROM "eepmisc" endianness bit
  ath9k: indicate that the AR9003 EEPROM template values are little
    endian
  ath9k: Add an eeprom_ops callback for retrieving the eepmisc value
  ath9k: replace eeprom_param EEP_MINOR_REV with get_eeprom_rev
  ath9k: consistently use get_eeprom_rev(ah)
  ath9k: Make the EEPROM swapping check use the eepmisc register
  ath9k: define all EEPROM fields in Little Endian format

 drivers/net/wireless/ath/ath9k/ar5008_phy.c    |   2 +-
 drivers/net/wireless/ath/ath9k/ar9002_hw.c     |   6 +-
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.c |  21 ++--
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.h |   4 +-
 drivers/net/wireless/ath/ath9k/eeprom.c        |  42 ++++---
 drivers/net/wireless/ath/ath9k/eeprom.h        |  85 +++++++------
 drivers/net/wireless/ath/ath9k/eeprom_4k.c     | 137 +++++++++------------
 drivers/net/wireless/ath/ath9k/eeprom_9287.c   | 129 +++++++++----------
 drivers/net/wireless/ath/ath9k/eeprom_def.c    | 163 ++++++++++++-------------
 drivers/net/wireless/ath/ath9k/xmit.c          |   3 +-
 10 files changed, 289 insertions(+), 303 deletions(-)

-- 
2.10.0

[ath9k-devel] [PATCH v2 1/7] ath9k: Add a #define for the EEPROM "eepmisc" endianness bit

[Martin Blumenstingl]

This replaces a magic number with a named #define. Additionally it
removes two "eeprom format" specific #defines for the "big endianness"
bit which are the same on all eeprom formats.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.c | 3 ++-
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.h | 1 -
 drivers/net/wireless/ath/ath9k/eeprom.h        | 4 +++-
 drivers/net/wireless/ath/ath9k/eeprom_4k.c     | 2 +-
 drivers/net/wireless/ath/ath9k/eeprom_9287.c   | 2 +-
 drivers/net/wireless/ath/ath9k/eeprom_def.c    | 2 +-
 6 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
index 08607d7..ea7b819 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
@@ -3468,7 +3468,8 @@ static u32 ath9k_hw_ar9003_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 					AR5416_OPFLAGS_N_5G_HT20));
 	PR_EEP("Disable 5Ghz HT40", !!(pBase->opCapFlags.opFlags &
 					AR5416_OPFLAGS_N_5G_HT40));
-	PR_EEP("Big Endian", !!(pBase->opCapFlags.eepMisc & 0x01));
+	PR_EEP("Big Endian", !!(pBase->opCapFlags.eepMisc &
+				AR5416_EEPMISC_BIG_ENDIAN));
 	PR_EEP("RF Silent", pBase->rfSilent);
 	PR_EEP("BT option", pBase->blueToothOptions);
 	PR_EEP("Device Cap", pBase->deviceCap);
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h
index 107bcfb..0a4c736 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h
+++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h
@@ -38,7 +38,6 @@
 #define AR9300_NUM_CTLS_2G           12
 #define AR9300_NUM_BAND_EDGES_5G     8
 #define AR9300_NUM_BAND_EDGES_2G     4
-#define AR9300_EEPMISC_BIG_ENDIAN    0x01
 #define AR9300_EEPMISC_WOW           0x02
 #define AR9300_CUSTOMER_DATA_SIZE    20
 
diff --git a/drivers/net/wireless/ath/ath9k/eeprom.h b/drivers/net/wireless/ath/ath9k/eeprom.h
index 4465c65..c466ada 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.h
+++ b/drivers/net/wireless/ath/ath9k/eeprom.h
@@ -161,6 +161,9 @@
 #define AR5416_EEP_TXGAIN_ORIGINAL         0
 #define AR5416_EEP_TXGAIN_HIGH_POWER       1
 
+/* Endianness of EEPROM content */
+#define AR5416_EEPMISC_BIG_ENDIAN          0x01
+
 #define AR5416_EEP4K_START_LOC                64
 #define AR5416_EEP4K_NUM_2G_CAL_PIERS         3
 #define AR5416_EEP4K_NUM_2G_CCK_TARGET_POWERS 3
@@ -191,7 +194,6 @@
 #define AR9287_NUM_CTLS              	12
 #define AR9287_NUM_BAND_EDGES        	4
 #define AR9287_PD_GAIN_ICEPTS           1
-#define AR9287_EEPMISC_BIG_ENDIAN       0x01
 #define AR9287_EEPMISC_WOW              0x02
 #define AR9287_MAX_CHAINS               2
 #define AR9287_ANT_16S                  32
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_4k.c b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
index 5da0826..780cb49 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_4k.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
@@ -154,7 +154,7 @@ static u32 ath9k_hw_4k_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 					AR5416_OPFLAGS_N_5G_HT20));
 	PR_EEP("Disable 5Ghz HT40", !!(pBase->opCapFlags &
 					AR5416_OPFLAGS_N_5G_HT40));
-	PR_EEP("Big Endian", !!(pBase->eepMisc & 0x01));
+	PR_EEP("Big Endian", !!(pBase->eepMisc & AR5416_EEPMISC_BIG_ENDIAN));
 	PR_EEP("Cal Bin Major Ver", (pBase->binBuildNumber >> 24) & 0xFF);
 	PR_EEP("Cal Bin Minor Ver", (pBase->binBuildNumber >> 16) & 0xFF);
 	PR_EEP("Cal Bin Build", (pBase->binBuildNumber >> 8) & 0xFF);
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_9287.c b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
index 1a019a3..f483ba2 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_9287.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
@@ -150,7 +150,7 @@ static u32 ath9k_hw_ar9287_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 					AR5416_OPFLAGS_N_5G_HT20));
 	PR_EEP("Disable 5Ghz HT40", !!(pBase->opCapFlags &
 					AR5416_OPFLAGS_N_5G_HT40));
-	PR_EEP("Big Endian", !!(pBase->eepMisc & 0x01));
+	PR_EEP("Big Endian", !!(pBase->eepMisc & AR5416_EEPMISC_BIG_ENDIAN));
 	PR_EEP("Cal Bin Major Ver", (pBase->binBuildNumber >> 24) & 0xFF);
 	PR_EEP("Cal Bin Minor Ver", (pBase->binBuildNumber >> 16) & 0xFF);
 	PR_EEP("Cal Bin Build", (pBase->binBuildNumber >> 8) & 0xFF);
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_def.c b/drivers/net/wireless/ath/ath9k/eeprom_def.c
index 959682f..39b1b27 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_def.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_def.c
@@ -232,7 +232,7 @@ static u32 ath9k_hw_def_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 					AR5416_OPFLAGS_N_5G_HT20));
 	PR_EEP("Disable 5Ghz HT40", !!(pBase->opCapFlags &
 					AR5416_OPFLAGS_N_5G_HT40));
-	PR_EEP("Big Endian", !!(pBase->eepMisc & 0x01));
+	PR_EEP("Big Endian", !!(pBase->eepMisc & AR5416_EEPMISC_BIG_ENDIAN));
 	PR_EEP("Cal Bin Major Ver", (pBase->binBuildNumber >> 24) & 0xFF);
 	PR_EEP("Cal Bin Minor Ver", (pBase->binBuildNumber >> 16) & 0xFF);
 	PR_EEP("Cal Bin Build", (pBase->binBuildNumber >> 8) & 0xFF);
-- 
2.10.0

[ath9k-devel] [PATCH v2 2/7] ath9k: indicate that the AR9003 EEPROM template values are little endian

[Martin Blumenstingl]

The eepMisc field was not set explicitly. The default value of 0 means
that the values in the EEPROM (template) should be interpreted as little
endian. However, this is not clear until comparing the AR9003 code with
the other EEPROM formats.
To make the code easier to understand we explicitly state that the values
are little endian - there are no functional changes with this patch.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.c | 10 +++++-----
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.h |  3 +++
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
index ea7b819..270d4ae 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_LITTLE_ENDIAN,
 		},
 		.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_LITTLE_ENDIAN,
 		},
 		.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_LITTLE_ENDIAN,
 		},
 		.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_LITTLE_ENDIAN,
 		},
 		.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_LITTLE_ENDIAN,
 		},
 		.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..7dc7205 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h
+++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.h
@@ -69,6 +69,9 @@
 #define AR9300_BASE_ADDR 0x3ff
 #define AR9300_BASE_ADDR_512 0x1ff
 
+/* AR5416_EEPMISC_BIG_ENDIAN not set indicates little endian */
+#define AR9300_EEPMISC_LITTLE_ENDIAN 0
+
 #define AR9300_OTP_BASE \
 		((AR_SREV_9340(ah) || AR_SREV_9550(ah)) ? 0x30000 : 0x14000)
 #define AR9300_OTP_STATUS \
-- 
2.10.0

[ath9k-devel] [PATCH v2 3/7] ath9k: Add an eeprom_ops callback for retrieving the eepmisc value

[Martin Blumenstingl]

This allows deciding if we have to swap the EEPROM data (so it matches
the system's native endianness) even if no byte-swapping (swab16, based on
the first two bytes in the EEPROM) is needed.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/wireless/ath/ath9k/ar9003_eeprom.c | 8 +++++++-
 drivers/net/wireless/ath/ath9k/eeprom.h        | 1 +
 drivers/net/wireless/ath/ath9k/eeprom_4k.c     | 8 +++++++-
 drivers/net/wireless/ath/ath9k/eeprom_9287.c   | 8 +++++++-
 drivers/net/wireless/ath/ath9k/eeprom_def.c    | 8 +++++++-
 5 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
index 270d4ae..3dbfd86 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
@@ -5498,6 +5498,11 @@ unsigned int ar9003_get_paprd_scale_factor(struct ath_hw *ah,
 	}
 }
 
+static u8 ar9003_get_eepmisc(struct ath_hw *ah)
+{
+	return ah->eeprom.map4k.baseEepHeader.eepMisc;
+}
+
 const struct eeprom_ops eep_ar9300_ops = {
 	.check_eeprom = ath9k_hw_ar9300_check_eeprom,
 	.get_eeprom = ath9k_hw_ar9300_get_eeprom,
@@ -5508,5 +5513,6 @@ const struct eeprom_ops eep_ar9300_ops = {
 	.set_board_values = ath9k_hw_ar9300_set_board_values,
 	.set_addac = ath9k_hw_ar9300_set_addac,
 	.set_txpower = ath9k_hw_ar9300_set_txpower,
-	.get_spur_channel = ath9k_hw_ar9300_get_spur_channel
+	.get_spur_channel = ath9k_hw_ar9300_get_spur_channel,
+	.get_eepmisc = ar9003_get_eepmisc
 };
diff --git a/drivers/net/wireless/ath/ath9k/eeprom.h b/drivers/net/wireless/ath/ath9k/eeprom.h
index c466ada..408cfa7 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.h
+++ b/drivers/net/wireless/ath/ath9k/eeprom.h
@@ -655,6 +655,7 @@ struct eeprom_ops {
 			   u16 cfgCtl, u8 twiceAntennaReduction,
 			   u8 powerLimit, bool test);
 	u16 (*get_spur_channel)(struct ath_hw *ah, u16 i, bool is2GHz);
+	u8 (*get_eepmisc)(struct ath_hw *ah);
 };
 
 void ath9k_hw_analog_shift_regwrite(struct ath_hw *ah, u32 reg, u32 val);
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_4k.c b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
index 780cb49..7cd9c64 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_4k.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
@@ -1064,6 +1064,11 @@ static u16 ath9k_hw_4k_get_spur_channel(struct ath_hw *ah, u16 i, bool is2GHz)
 	return ah->eeprom.map4k.modalHeader.spurChans[i].spurChan;
 }
 
+static u8 ath9k_hw_4k_get_eepmisc(struct ath_hw *ah)
+{
+	return ah->eeprom.map4k.baseEepHeader.eepMisc;
+}
+
 const struct eeprom_ops eep_4k_ops = {
 	.check_eeprom		= ath9k_hw_4k_check_eeprom,
 	.get_eeprom		= ath9k_hw_4k_get_eeprom,
@@ -1073,5 +1078,6 @@ const struct eeprom_ops eep_4k_ops = {
 	.get_eeprom_rev		= ath9k_hw_4k_get_eeprom_rev,
 	.set_board_values	= ath9k_hw_4k_set_board_values,
 	.set_txpower		= ath9k_hw_4k_set_txpower,
-	.get_spur_channel	= ath9k_hw_4k_get_spur_channel
+	.get_spur_channel	= ath9k_hw_4k_get_spur_channel,
+	.get_eepmisc		= ath9k_hw_4k_get_eepmisc
 };
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_9287.c b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
index f483ba2..e1ba9bc 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_9287.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
@@ -986,6 +986,11 @@ static u16 ath9k_hw_ar9287_get_spur_channel(struct ath_hw *ah,
 	return ah->eeprom.map9287.modalHeader.spurChans[i].spurChan;
 }
 
+static u8 ath9k_hw_ar9287_get_eepmisc(struct ath_hw *ah)
+{
+	return ah->eeprom.map9287.baseEepHeader.eepMisc;
+}
+
 const struct eeprom_ops eep_ar9287_ops = {
 	.check_eeprom		= ath9k_hw_ar9287_check_eeprom,
 	.get_eeprom		= ath9k_hw_ar9287_get_eeprom,
@@ -995,5 +1000,6 @@ const struct eeprom_ops eep_ar9287_ops = {
 	.get_eeprom_rev		= ath9k_hw_ar9287_get_eeprom_rev,
 	.set_board_values	= ath9k_hw_ar9287_set_board_values,
 	.set_txpower		= ath9k_hw_ar9287_set_txpower,
-	.get_spur_channel	= ath9k_hw_ar9287_get_spur_channel
+	.get_spur_channel	= ath9k_hw_ar9287_get_spur_channel,
+	.get_eepmisc		= ath9k_hw_ar9287_get_eepmisc
 };
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_def.c b/drivers/net/wireless/ath/ath9k/eeprom_def.c
index 39b1b27..087bdb7 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_def.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_def.c
@@ -1317,6 +1317,11 @@ static u16 ath9k_hw_def_get_spur_channel(struct ath_hw *ah, u16 i, bool is2GHz)
 	return ah->eeprom.def.modalHeader[is2GHz].spurChans[i].spurChan;
 }
 
+static u8 ath9k_hw_def_get_eepmisc(struct ath_hw *ah)
+{
+	return ah->eeprom.def.baseEepHeader.eepMisc;
+}
+
 const struct eeprom_ops eep_def_ops = {
 	.check_eeprom		= ath9k_hw_def_check_eeprom,
 	.get_eeprom		= ath9k_hw_def_get_eeprom,
@@ -1327,5 +1332,6 @@ const struct eeprom_ops eep_def_ops = {
 	.set_board_values	= ath9k_hw_def_set_board_values,
 	.set_addac		= ath9k_hw_def_set_addac,
 	.set_txpower		= ath9k_hw_def_set_txpower,
-	.get_spur_channel	= ath9k_hw_def_get_spur_channel
+	.get_spur_channel	= ath9k_hw_def_get_spur_channel,
+	.get_eepmisc		= ath9k_hw_def_get_eepmisc
 };
-- 
2.10.0

[ath9k-devel] [PATCH v2 4/7] ath9k: replace eeprom_param EEP_MINOR_REV with get_eeprom_rev

[Martin Blumenstingl]

get_eeprom(ah, EEP_MINOR_REV) and get_eeprom_rev(ah) are both doing the
same thing: returning the EEPROM revision (12 lowest bits). Make the
code consistent by using get_eeprom_rev(ah) everywhere.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/wireless/ath/ath9k/ar5008_phy.c  | 2 +-
 drivers/net/wireless/ath/ath9k/ar9002_hw.c   | 6 ++----
 drivers/net/wireless/ath/ath9k/eeprom.h      | 1 -
 drivers/net/wireless/ath/ath9k/eeprom_4k.c   | 5 -----
 drivers/net/wireless/ath/ath9k/eeprom_9287.c | 6 +-----
 drivers/net/wireless/ath/ath9k/eeprom_def.c  | 2 --
 6 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar5008_phy.c b/drivers/net/wireless/ath/ath9k/ar5008_phy.c
index 8eea8d2..7922550 100644
--- a/drivers/net/wireless/ath/ath9k/ar5008_phy.c
+++ b/drivers/net/wireless/ath/ath9k/ar5008_phy.c
@@ -524,7 +524,7 @@ static bool ar5008_hw_set_rf_regs(struct ath_hw *ah,
 		return true;
 
 	/* Setup rf parameters */
-	eepMinorRev = ah->eep_ops->get_eeprom(ah, EEP_MINOR_REV);
+	eepMinorRev = ah->eep_ops->get_eeprom_rev(ah);
 
 	for (i = 0; i < ah->iniBank6.ia_rows; i++)
 		ah->analogBank6Data[i] = INI_RA(&ah->iniBank6, i, modesIndex);
diff --git a/drivers/net/wireless/ath/ath9k/ar9002_hw.c b/drivers/net/wireless/ath/ath9k/ar9002_hw.c
index d480d2f..ae68f67 100644
--- a/drivers/net/wireless/ath/ath9k/ar9002_hw.c
+++ b/drivers/net/wireless/ath/ath9k/ar9002_hw.c
@@ -108,8 +108,7 @@ static void ar9280_20_hw_init_rxgain_ini(struct ath_hw *ah)
 {
 	u32 rxgain_type;
 
-	if (ah->eep_ops->get_eeprom(ah, EEP_MINOR_REV) >=
-	    AR5416_EEP_MINOR_VER_17) {
+	if (ah->eep_ops->get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_17) {
 		rxgain_type = ah->eep_ops->get_eeprom(ah, EEP_RXGAIN_TYPE);
 
 		if (rxgain_type == AR5416_EEP_RXGAIN_13DB_BACKOFF)
@@ -129,8 +128,7 @@ static void ar9280_20_hw_init_rxgain_ini(struct ath_hw *ah)
 
 static void ar9280_20_hw_init_txgain_ini(struct ath_hw *ah, u32 txgain_type)
 {
-	if (ah->eep_ops->get_eeprom(ah, EEP_MINOR_REV) >=
-	    AR5416_EEP_MINOR_VER_19) {
+	if (ah->eep_ops->get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_19) {
 		if (txgain_type == AR5416_EEP_TXGAIN_HIGH_POWER)
 			INIT_INI_ARRAY(&ah->iniModesTxGain,
 				       ar9280Modes_high_power_tx_gain_9280_2);
diff --git a/drivers/net/wireless/ath/ath9k/eeprom.h b/drivers/net/wireless/ath/ath9k/eeprom.h
index 408cfa7..8ef8090 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.h
+++ b/drivers/net/wireless/ath/ath9k/eeprom.h
@@ -230,7 +230,6 @@ enum eeprom_param {
 	EEP_DB_5,
 	EEP_OB_2,
 	EEP_DB_2,
-	EEP_MINOR_REV,
 	EEP_TX_MASK,
 	EEP_RX_MASK,
 	EEP_FSTCLK_5G,
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_4k.c b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
index 7cd9c64..d466871 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_4k.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
@@ -254,9 +254,6 @@ static u32 ath9k_hw_4k_get_eeprom(struct ath_hw *ah,
 	struct ar5416_eeprom_4k *eep = &ah->eeprom.map4k;
 	struct modal_eep_4k_header *pModal = &eep->modalHeader;
 	struct base_eep_header_4k *pBase = &eep->baseEepHeader;
-	u16 ver_minor;
-
-	ver_minor = pBase->version & AR5416_EEP_VER_MINOR_MASK;
 
 	switch (param) {
 	case EEP_NFTHRESH_2:
@@ -279,8 +276,6 @@ static u32 ath9k_hw_4k_get_eeprom(struct ath_hw *ah,
 		return pModal->ob_0;
 	case EEP_DB_2:
 		return pModal->db1_1;
-	case EEP_MINOR_REV:
-		return ver_minor;
 	case EEP_TX_MASK:
 		return pBase->txMask;
 	case EEP_RX_MASK:
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_9287.c b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
index e1ba9bc..7f85bd1 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_9287.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
@@ -250,9 +250,7 @@ static u32 ath9k_hw_ar9287_get_eeprom(struct ath_hw *ah,
 	struct ar9287_eeprom *eep = &ah->eeprom.map9287;
 	struct modal_eep_ar9287_header *pModal = &eep->modalHeader;
 	struct base_eep_ar9287_header *pBase = &eep->baseEepHeader;
-	u16 ver_minor;
-
-	ver_minor = pBase->version & AR9287_EEP_VER_MINOR_MASK;
+	u16 ver_minor = ath9k_hw_ar9287_get_eeprom_rev(ah);
 
 	switch (param) {
 	case EEP_NFTHRESH_2:
@@ -271,8 +269,6 @@ static u32 ath9k_hw_ar9287_get_eeprom(struct ath_hw *ah,
 		return pBase->opCapFlags;
 	case EEP_RF_SILENT:
 		return pBase->rfSilent;
-	case EEP_MINOR_REV:
-		return ver_minor;
 	case EEP_TX_MASK:
 		return pBase->txMask;
 	case EEP_RX_MASK:
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_def.c b/drivers/net/wireless/ath/ath9k/eeprom_def.c
index 087bdb7..e9af4a5 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_def.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_def.c
@@ -380,8 +380,6 @@ static u32 ath9k_hw_def_get_eeprom(struct ath_hw *ah,
 		return pModal[1].ob;
 	case EEP_DB_2:
 		return pModal[1].db;
-	case EEP_MINOR_REV:
-		return AR5416_VER_MASK;
 	case EEP_TX_MASK:
 		return pBase->txMask;
 	case EEP_RX_MASK:
-- 
2.10.0

[ath9k-devel] [PATCH v2 5/7] ath9k: consistently use get_eeprom_rev(ah)

[Martin Blumenstingl]

The AR5416_VER_MASK macro does the same as get_eeprom_rev, except that
one has to know the actual EEPROM type (and providing a reference to
that in a variable named "eep"). Additionally the eeprom_*.c
implementations used the same shifting logic multiple times to get the
eeprom revision which was also unnecessary duplication of
get_eeprom_rev.

Also use the AR5416_EEP_VER_MINOR_MASK macro where needed and introduce
a similar macro (AR5416_EEP_VER_MAJOR_MASK) for the major version.
Finally drop AR9287_EEP_VER_MINOR_MASK since it simply duplicates the
already defined AR5416_EEP_VER_MINOR_MASK.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/wireless/ath/ath9k/eeprom.h      |  4 +--
 drivers/net/wireless/ath/ath9k/eeprom_4k.c   | 32 ++++++++++------------
 drivers/net/wireless/ath/ath9k/eeprom_9287.c | 19 +++++++------
 drivers/net/wireless/ath/ath9k/eeprom_def.c  | 41 +++++++++++++++-------------
 drivers/net/wireless/ath/ath9k/xmit.c        |  3 +-
 5 files changed, 52 insertions(+), 47 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/eeprom.h b/drivers/net/wireless/ath/ath9k/eeprom.h
index 8ef8090..5a24fb5 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.h
+++ b/drivers/net/wireless/ath/ath9k/eeprom.h
@@ -99,7 +99,6 @@
 #define FBIN2FREQ(x, y)		((y) ? (2300 + x) : (4800 + 5 * x))
 #define ath9k_hw_use_flash(_ah)	(!(_ah->ah_flags & AH_USE_EEPROM))
 
-#define AR5416_VER_MASK (eep->baseEepHeader.version & AR5416_EEP_VER_MINOR_MASK)
 #define OLC_FOR_AR9280_20_LATER (AR_SREV_9280_20_OR_LATER(ah) && \
 				 ah->eep_ops->get_eeprom(ah, EEP_OL_PWRCTRL))
 #define OLC_FOR_AR9287_10_LATER (AR_SREV_9287_11_OR_LATER(ah) && \
@@ -121,6 +120,8 @@
 
 #define AR5416_EEP_NO_BACK_VER       0x1
 #define AR5416_EEP_VER               0xE
+#define AR5416_EEP_VER_MAJOR_SHIFT   12
+#define AR5416_EEP_VER_MAJOR_MASK    0xF000
 #define AR5416_EEP_VER_MINOR_MASK    0x0FFF
 #define AR5416_EEP_MINOR_VER_2       0x2
 #define AR5416_EEP_MINOR_VER_3       0x3
@@ -177,7 +178,6 @@
 #define AR9280_TX_GAIN_TABLE_SIZE 22
 
 #define AR9287_EEP_VER               0xE
-#define AR9287_EEP_VER_MINOR_MASK    0xFFF
 #define AR9287_EEP_MINOR_VER_1       0x1
 #define AR9287_EEP_MINOR_VER_2       0x2
 #define AR9287_EEP_MINOR_VER_3       0x3
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_4k.c b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
index d466871..76ce109 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_4k.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
@@ -20,12 +20,17 @@
 
 static int ath9k_hw_4k_get_eeprom_ver(struct ath_hw *ah)
 {
-	return ((ah->eeprom.map4k.baseEepHeader.version >> 12) & 0xF);
+	u16 version = ah->eeprom.map4k.baseEepHeader.version;
+
+	return (version & AR5416_EEP_VER_MAJOR_MASK) >>
+		AR5416_EEP_VER_MAJOR_SHIFT;
 }
 
 static int ath9k_hw_4k_get_eeprom_rev(struct ath_hw *ah)
 {
-	return ((ah->eeprom.map4k.baseEepHeader.version) & 0xFFF);
+	u16 version = ah->eeprom.map4k.baseEepHeader.version;
+
+	return version & AR5416_EEP_VER_MINOR_MASK;
 }
 
 #define SIZE_EEPROM_4K (sizeof(struct ar5416_eeprom_4k) / sizeof(u16))
@@ -136,8 +141,8 @@ static u32 ath9k_hw_4k_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 		goto out;
 	}
 
-	PR_EEP("Major Version", pBase->version >> 12);
-	PR_EEP("Minor Version", pBase->version & 0xFFF);
+	PR_EEP("Major Version", ath9k_hw_4k_get_eeprom_ver(ah));
+	PR_EEP("Minor Version", ath9k_hw_4k_get_eeprom_rev(ah));
 	PR_EEP("Checksum", pBase->checksum);
 	PR_EEP("Length", pBase->length);
 	PR_EEP("RegDomain1", pBase->regDmn[0]);
@@ -314,14 +319,12 @@ static void ath9k_hw_set_4k_power_cal_table(struct ath_hw *ah,
 
 	xpdMask = pEepData->modalHeader.xpdGain;
 
-	if ((pEepData->baseEepHeader.version & AR5416_EEP_VER_MINOR_MASK) >=
-	    AR5416_EEP_MINOR_VER_2) {
+	if (ath9k_hw_4k_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_2)
 		pdGainOverlap_t2 =
 			pEepData->modalHeader.pdGainOverlap;
-	} else {
+	else
 		pdGainOverlap_t2 = (u16)(MS(REG_READ(ah, AR_PHY_TPCRG5),
 					    AR_PHY_TPCRG5_PD_GAIN_OVERLAP));
-	}
 
 	pCalBChans = pEepData->calFreqPier2G;
 	numPiers = AR5416_EEP4K_NUM_2G_CAL_PIERS;
@@ -607,10 +610,8 @@ static void ath9k_hw_4k_set_txpower(struct ath_hw *ah,
 
 	memset(ratesArray, 0, sizeof(ratesArray));
 
-	if ((pEepData->baseEepHeader.version & AR5416_EEP_VER_MINOR_MASK) >=
-	    AR5416_EEP_MINOR_VER_2) {
+	if (ath9k_hw_4k_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_2)
 		ht40PowerIncForPdadc = pModal->ht40PowerIncForPdadc;
-	}
 
 	ath9k_hw_set_4k_power_per_rate_table(ah, chan,
 					     &ratesArray[0], cfgCtl,
@@ -730,8 +731,7 @@ static void ath9k_hw_4k_set_gain(struct ath_hw *ah,
 		SM(pModal->iqCalQCh[0], AR_PHY_TIMING_CTRL4_IQCORR_Q_Q_COFF),
 		AR_PHY_TIMING_CTRL4_IQCORR_Q_Q_COFF | AR_PHY_TIMING_CTRL4_IQCORR_Q_I_COFF);
 
-	if ((eep->baseEepHeader.version & AR5416_EEP_VER_MINOR_MASK) >=
-	    AR5416_EEP_MINOR_VER_3) {
+	if (ath9k_hw_4k_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_3) {
 		txRxAttenLocal = pModal->txRxAttenCh[0];
 
 		REG_RMW_FIELD(ah, AR_PHY_GAIN_2GHZ,
@@ -1009,16 +1009,14 @@ static void ath9k_hw_4k_set_board_values(struct ath_hw *ah,
 	REG_RMW_FIELD(ah, AR_PHY_EXT_CCA0, AR_PHY_EXT_CCA0_THRESH62,
 		      pModal->thresh62);
 
-	if ((eep->baseEepHeader.version & AR5416_EEP_VER_MINOR_MASK) >=
-						AR5416_EEP_MINOR_VER_2) {
+	if (ath9k_hw_4k_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_2) {
 		REG_RMW_FIELD(ah, AR_PHY_RF_CTL2, AR_PHY_TX_END_DATA_START,
 			      pModal->txFrameToDataStart);
 		REG_RMW_FIELD(ah, AR_PHY_RF_CTL2, AR_PHY_TX_END_PA_ON,
 			      pModal->txFrameToPaOn);
 	}
 
-	if ((eep->baseEepHeader.version & AR5416_EEP_VER_MINOR_MASK) >=
-						AR5416_EEP_MINOR_VER_3) {
+	if (ath9k_hw_4k_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_3) {
 		if (IS_CHAN_HT40(chan))
 			REG_RMW_FIELD(ah, AR_PHY_SETTLING,
 				      AR_PHY_SETTLING_SWITCH,
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_9287.c b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
index 7f85bd1..daeaf12 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_9287.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
@@ -22,12 +22,17 @@
 
 static int ath9k_hw_ar9287_get_eeprom_ver(struct ath_hw *ah)
 {
-	return (ah->eeprom.map9287.baseEepHeader.version >> 12) & 0xF;
+	u16 version = ah->eeprom.map9287.baseEepHeader.version;
+
+	return (version & AR5416_EEP_VER_MAJOR_MASK) >>
+		AR5416_EEP_VER_MAJOR_SHIFT;
 }
 
 static int ath9k_hw_ar9287_get_eeprom_rev(struct ath_hw *ah)
 {
-	return (ah->eeprom.map9287.baseEepHeader.version) & 0xFFF;
+	u16 version = ah->eeprom.map9287.baseEepHeader.version;
+
+	return version & AR5416_EEP_VER_MINOR_MASK;
 }
 
 static bool __ath9k_hw_ar9287_fill_eeprom(struct ath_hw *ah)
@@ -132,8 +137,8 @@ static u32 ath9k_hw_ar9287_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 		goto out;
 	}
 
-	PR_EEP("Major Version", pBase->version >> 12);
-	PR_EEP("Minor Version", pBase->version & 0xFFF);
+	PR_EEP("Major Version", ath9k_hw_ar9287_get_eeprom_ver(ah));
+	PR_EEP("Minor Version", ath9k_hw_ar9287_get_eeprom_rev(ah));
 	PR_EEP("Checksum", pBase->checksum);
 	PR_EEP("Length", pBase->length);
 	PR_EEP("RegDomain1", pBase->regDmn[0]);
@@ -383,8 +388,7 @@ static void ath9k_hw_set_ar9287_power_cal_table(struct ath_hw *ah,
 
 	xpdMask = pEepData->modalHeader.xpdGain;
 
-	if ((pEepData->baseEepHeader.version & AR9287_EEP_VER_MINOR_MASK) >=
-	    AR9287_EEP_MINOR_VER_2)
+	if (ath9k_hw_ar9287_get_eeprom_rev(ah) >= AR9287_EEP_MINOR_VER_2)
 		pdGainOverlap_t2 = pEepData->modalHeader.pdGainOverlap;
 	else
 		pdGainOverlap_t2 = (u16)(MS(REG_READ(ah, AR_PHY_TPCRG5),
@@ -733,8 +737,7 @@ static void ath9k_hw_ar9287_set_txpower(struct ath_hw *ah,
 
 	memset(ratesArray, 0, sizeof(ratesArray));
 
-	if ((pEepData->baseEepHeader.version & AR9287_EEP_VER_MINOR_MASK) >=
-	    AR9287_EEP_MINOR_VER_2)
+	if (ath9k_hw_ar9287_get_eeprom_rev(ah) >= AR9287_EEP_MINOR_VER_2)
 		ht40PowerIncForPdadc = pModal->ht40PowerIncForPdadc;
 
 	ath9k_hw_set_ar9287_power_per_rate_table(ah, chan,
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_def.c b/drivers/net/wireless/ath/ath9k/eeprom_def.c
index e9af4a5..bd5bd62 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_def.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_def.c
@@ -79,12 +79,17 @@ static void ath9k_olc_get_pdadcs(struct ath_hw *ah,
 
 static int ath9k_hw_def_get_eeprom_ver(struct ath_hw *ah)
 {
-	return ((ah->eeprom.def.baseEepHeader.version >> 12) & 0xF);
+	u16 version = ah->eeprom.def.baseEepHeader.version;
+
+	return (version & AR5416_EEP_VER_MAJOR_MASK) >>
+		AR5416_EEP_VER_MAJOR_SHIFT;
 }
 
 static int ath9k_hw_def_get_eeprom_rev(struct ath_hw *ah)
 {
-	return ((ah->eeprom.def.baseEepHeader.version) & 0xFFF);
+	u16 version = ah->eeprom.def.baseEepHeader.version;
+
+	return version & AR5416_EEP_VER_MINOR_MASK;
 }
 
 #define SIZE_EEPROM_DEF (sizeof(struct ar5416_eeprom_def) / sizeof(u16))
@@ -214,8 +219,8 @@ static u32 ath9k_hw_def_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 		goto out;
 	}
 
-	PR_EEP("Major Version", pBase->version >> 12);
-	PR_EEP("Minor Version", pBase->version & 0xFFF);
+	PR_EEP("Major Version", ath9k_hw_def_get_eeprom_ver(ah));
+	PR_EEP("Minor Version", ath9k_hw_def_get_eeprom_rev(ah));
 	PR_EEP("Checksum", pBase->checksum);
 	PR_EEP("Length", pBase->length);
 	PR_EEP("RegDomain1", pBase->regDmn[0]);
@@ -391,27 +396,27 @@ static u32 ath9k_hw_def_get_eeprom(struct ath_hw *ah,
 	case EEP_TXGAIN_TYPE:
 		return pBase->txGainType;
 	case EEP_OL_PWRCTRL:
-		if (AR5416_VER_MASK >= AR5416_EEP_MINOR_VER_19)
+		if (ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_19)
 			return pBase->openLoopPwrCntl ? true : false;
 		else
 			return false;
 	case EEP_RC_CHAIN_MASK:
-		if (AR5416_VER_MASK >= AR5416_EEP_MINOR_VER_19)
+		if (ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_19)
 			return pBase->rcChainMask;
 		else
 			return 0;
 	case EEP_DAC_HPWR_5G:
-		if (AR5416_VER_MASK >= AR5416_EEP_MINOR_VER_20)
+		if (ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_20)
 			return pBase->dacHiPwrMode_5G;
 		else
 			return 0;
 	case EEP_FRAC_N_5G:
-		if (AR5416_VER_MASK >= AR5416_EEP_MINOR_VER_22)
+		if (ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_22)
 			return pBase->frac_n_5g;
 		else
 			return 0;
 	case EEP_PWR_TABLE_OFFSET:
-		if (AR5416_VER_MASK >= AR5416_EEP_MINOR_VER_21)
+		if (ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_21)
 			return pBase->pwr_table_offset;
 		else
 			return AR5416_PWR_TABLE_OFFSET_DB;
@@ -434,7 +439,7 @@ static void ath9k_hw_def_set_gain(struct ath_hw *ah,
 				  u8 txRxAttenLocal, int regChainOffset, int i)
 {
 	ENABLE_REG_RMW_BUFFER(ah);
-	if (AR5416_VER_MASK >= AR5416_EEP_MINOR_VER_3) {
+	if (ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_3) {
 		txRxAttenLocal = pModal->txRxAttenCh[i];
 
 		if (AR_SREV_9280_20_OR_LATER(ah)) {
@@ -603,7 +608,7 @@ static void ath9k_hw_def_set_board_values(struct ath_hw *ah,
 			      pModal->thresh62);
 	}
 
-	if (AR5416_VER_MASK >= AR5416_EEP_MINOR_VER_2) {
+	if (ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_2) {
 		REG_RMW_FIELD(ah, AR_PHY_RF_CTL2,
 			      AR_PHY_TX_END_DATA_START,
 			      pModal->txFrameToDataStart);
@@ -611,7 +616,7 @@ static void ath9k_hw_def_set_board_values(struct ath_hw *ah,
 			      pModal->txFrameToPaOn);
 	}
 
-	if (AR5416_VER_MASK >= AR5416_EEP_MINOR_VER_3) {
+	if (ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_3) {
 		if (IS_CHAN_HT40(chan))
 			REG_RMW_FIELD(ah, AR_PHY_SETTLING,
 				      AR_PHY_SETTLING_SWITCH,
@@ -619,13 +624,14 @@ static void ath9k_hw_def_set_board_values(struct ath_hw *ah,
 	}
 
 	if (AR_SREV_9280_20_OR_LATER(ah) &&
-	    AR5416_VER_MASK >= AR5416_EEP_MINOR_VER_19)
+	    ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_19)
 		REG_RMW_FIELD(ah, AR_PHY_CCK_TX_CTRL,
 			      AR_PHY_CCK_TX_CTRL_TX_DAC_SCALE_CCK,
 			      pModal->miscBits);
 
 
-	if (AR_SREV_9280_20(ah) && AR5416_VER_MASK >= AR5416_EEP_MINOR_VER_20) {
+	if (AR_SREV_9280_20(ah) &&
+	    ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_20) {
 		if (IS_CHAN_2GHZ(chan))
 			REG_RMW_FIELD(ah, AR_AN_TOP1, AR_AN_TOP1_DACIPMODE,
 					eep->baseEepHeader.dacLpMode);
@@ -796,8 +802,7 @@ static void ath9k_hw_set_def_power_cal_table(struct ath_hw *ah,
 
 	pwr_table_offset = ah->eep_ops->get_eeprom(ah, EEP_PWR_TABLE_OFFSET);
 
-	if ((pEepData->baseEepHeader.version & AR5416_EEP_VER_MINOR_MASK) >=
-	    AR5416_EEP_MINOR_VER_2) {
+	if (ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_2) {
 		pdGainOverlap_t2 =
 			pEepData->modalHeader[modalIdx].pdGainOverlap;
 	} else {
@@ -1169,10 +1174,8 @@ static void ath9k_hw_def_set_txpower(struct ath_hw *ah,
 
 	memset(ratesArray, 0, sizeof(ratesArray));
 
-	if ((pEepData->baseEepHeader.version & AR5416_EEP_VER_MINOR_MASK) >=
-	    AR5416_EEP_MINOR_VER_2) {
+	if (ath9k_hw_def_get_eeprom_rev(ah) >= AR5416_EEP_MINOR_VER_2)
 		ht40PowerIncForPdadc = pModal->ht40PowerIncForPdadc;
-	}
 
 	ath9k_hw_set_def_power_per_rate_table(ah, chan,
 					       &ratesArray[0], cfgCtl,
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 52bfbb9..2f35e7a 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1132,8 +1132,9 @@ static u8 ath_get_rate_txpower(struct ath_softc *sc, struct ath_buf *bf,
 		if (is_40) {
 			u8 power_ht40delta;
 			struct ar5416_eeprom_def *eep = &ah->eeprom.def;
+			u16 eeprom_rev = ah->eep_ops->get_eeprom_rev(ah);
 
-			if (AR5416_VER_MASK >= AR5416_EEP_MINOR_VER_2) {
+			if (eeprom_rev >= AR5416_EEP_MINOR_VER_2) {
 				bool is_2ghz;
 				struct modal_eep_header *pmodal;
 
-- 
2.10.0

[ath9k-devel] [PATCH v2 6/7] ath9k: Make the EEPROM swapping check use the eepmisc register

[Martin Blumenstingl]

There are two ways of swapping the EEPROM data in the ath9k driver:
1) swab16 based on the first two EEPROM "magic" bytes (same for all
   EEPROM formats)
2) field and EEPROM format specific swab16/swab32 (different for
   eeprom_def, eeprom_4k and eeprom_9287)

The result of the first check was used to also enable the second swap.
This behavior seems incorrect, since the data may only be byte-swapped
(afterwards the data could be in the correct endianness).
Thus we introduce a separate check based on the "eepmisc" register
(which is part of the EEPROM data). When bit 0 is set, then the EEPROM
format specific values are in "big endian". This is also done by the
FreeBSD kernel, see [0] for example.

This allows us to parse EEPROMs with the "correct" magic bytes but
swapped EEPROM format specific values. These EEPROMs (mostly found in
lantiq and broadcom based big endian MIPS based devices) only worked
due to platform specific "hacks" which swapped the EEPROM so the
magic was inverted, which also enabled the format specific swapping.
With this patch the old behavior is still supported, but neither
recommended nor needed anymore.

[0]
https://github.com/freebsd/freebsd/blob/50719b56d9ce8d7d4beb53b16e9edb2e9a4a7a18/sys/dev/ath/ath_hal/ah_eeprom_9287.c#L351

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/wireless/ath/ath9k/eeprom.c | 57 ++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/eeprom.c b/drivers/net/wireless/ath/ath9k/eeprom.c
index a449588..0e46797 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom.c
@@ -155,11 +155,19 @@ bool ath9k_hw_nvram_read(struct ath_hw *ah, u32 off, u16 *data)
 	return ret;
 }
 
+#ifdef __BIG_ENDIAN
+#define EXPECTED_EEPMISC_ENDIAN AR5416_EEPMISC_BIG_ENDIAN
+#else
+#define EXPECTED_EEPMISC_ENDIAN 0
+#endif
+
 int ath9k_hw_nvram_swap_data(struct ath_hw *ah, bool *swap_needed, int size)
 {
 	u16 magic;
 	u16 *eepdata;
+	u8 eepmisc;
 	int i;
+	bool needs_byteswap = false;
 	struct ath_common *common = ath9k_hw_common(ah);
 
 	if (!ath9k_hw_nvram_read(ah, AR5416_EEPROM_MAGIC_OFFSET, &magic)) {
@@ -167,36 +175,53 @@ int ath9k_hw_nvram_swap_data(struct ath_hw *ah, bool *swap_needed, int size)
 		return -EIO;
 	}
 
-	*swap_needed = false;
 	if (swab16(magic) == AR5416_EEPROM_MAGIC) {
+		needs_byteswap = true;
+		ath_dbg(common, EEPROM,
+			"EEPROM needs byte-swapping to correct endianness.\n");
+	} else if (magic != AR5416_EEPROM_MAGIC) {
+		if (ath9k_hw_use_flash(ah)) {
+			ath_dbg(common, EEPROM,
+				"Ignoring invalid EEPROM magic (0x%04x).\n",
+				magic);
+		} else {
+			ath_err(common,
+				"Invalid EEPROM magic (0x%04x).\n", magic);
+			return -EINVAL;
+		}
+	}
+
+	if (needs_byteswap) {
 		if (ah->ah_flags & AH_NO_EEP_SWAP) {
 			ath_info(common,
 				 "Ignoring endianness difference in EEPROM magic bytes.\n");
 		} else {
-			*swap_needed = true;
-		}
-	} else if (magic != AR5416_EEPROM_MAGIC) {
-		if (ath9k_hw_use_flash(ah))
-			return 0;
+			eepdata = (u16 *)(&ah->eeprom);
 
-		ath_err(common,
-			"Invalid EEPROM Magic (0x%04x).\n", magic);
-		return -EINVAL;
+			for (i = 0; i < size; i++)
+				eepdata[i] = swab16(eepdata[i]);
+		}
 	}
 
-	eepdata = (u16 *)(&ah->eeprom);
-
-	if (*swap_needed) {
-		ath_dbg(common, EEPROM,
-			"EEPROM Endianness is not native.. Changing.\n");
+	*swap_needed = false;
 
-		for (i = 0; i < size; i++)
-			eepdata[i] = swab16(eepdata[i]);
+	eepmisc = ah->eep_ops->get_eepmisc(ah);
+	if ((eepmisc & AR5416_EEPMISC_BIG_ENDIAN) != EXPECTED_EEPMISC_ENDIAN) {
+		if (ah->ah_flags & AH_NO_EEP_SWAP) {
+			ath_info(common,
+				 "Ignoring endianness difference in eepmisc register.\n");
+		} else {
+			*swap_needed = true;
+			ath_dbg(common, EEPROM,
+				"EEPROM needs swapping according to the eepmisc register.\n");
+		}
 	}
 
 	return 0;
 }
 
+#undef EXPECTED_EEPMISC_VAL
+
 bool ath9k_hw_nvram_validate_checksum(struct ath_hw *ah, int size)
 {
 	u32 i, sum = 0;
-- 
2.10.0

[ath9k-devel] [PATCH v2 7/7] ath9k: define all EEPROM fields in Little Endian format

[Martin Blumenstingl]

The ar9300_eeprom logic is already using only 8-bit (endian neutral),
__le16 and __le32 fields to state explicitly how the values should be
interpreted.
All other EEPROM implementations (4k, 9287 and def) were using u16 and
u32 fields with additional logic to swap the values (read from the
original EEPROM) so they match the current CPUs endianness.

The EEPROM format defaults to "all values are Little Endian", indicated
by the absence of the AR5416_EEPMISC_BIG_ENDIAN in the u8 EEPMISC
register. If we detect that the EEPROM indicates Big Endian mode
(AR5416_EEPMISC_BIG_ENDIAN is set in the EEPMISC register) then we'll
swap the values to convert them into Little Endian. This is done by
activating the EEPMISC based logic in ath9k_hw_nvram_swap_data even if
AH_NO_EEP_SWAP is set (this makes ath9k behave like the FreeBSD driver,
which also does not have a flag to enable swapping based on the
AR5416_EEPMISC_BIG_ENDIAN bit). Before this logic was only used to
enable swapping when "current CPU endianness != EEPROM endianness".

After changing all relevant fields to __le16 and __le32 sparse was used
to check that all code which reads any of these fields uses
le{16,32}_to_cpu.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/wireless/ath/ath9k/eeprom.c      |  27 ++-----
 drivers/net/wireless/ath/ath9k/eeprom.h      |  75 ++++++++++--------
 drivers/net/wireless/ath/ath9k/eeprom_4k.c   |  94 +++++++++-------------
 drivers/net/wireless/ath/ath9k/eeprom_9287.c |  98 ++++++++++-------------
 drivers/net/wireless/ath/ath9k/eeprom_def.c  | 114 ++++++++++++---------------
 5 files changed, 174 insertions(+), 234 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/eeprom.c b/drivers/net/wireless/ath/ath9k/eeprom.c
index 0e46797..fb80ec8 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom.c
@@ -155,17 +155,10 @@ bool ath9k_hw_nvram_read(struct ath_hw *ah, u32 off, u16 *data)
 	return ret;
 }
 
-#ifdef __BIG_ENDIAN
-#define EXPECTED_EEPMISC_ENDIAN AR5416_EEPMISC_BIG_ENDIAN
-#else
-#define EXPECTED_EEPMISC_ENDIAN 0
-#endif
-
 int ath9k_hw_nvram_swap_data(struct ath_hw *ah, bool *swap_needed, int size)
 {
 	u16 magic;
 	u16 *eepdata;
-	u8 eepmisc;
 	int i;
 	bool needs_byteswap = false;
 	struct ath_common *common = ath9k_hw_common(ah);
@@ -203,25 +196,17 @@ int ath9k_hw_nvram_swap_data(struct ath_hw *ah, bool *swap_needed, int size)
 		}
 	}
 
-	*swap_needed = false;
-
-	eepmisc = ah->eep_ops->get_eepmisc(ah);
-	if ((eepmisc & AR5416_EEPMISC_BIG_ENDIAN) != EXPECTED_EEPMISC_ENDIAN) {
-		if (ah->ah_flags & AH_NO_EEP_SWAP) {
-			ath_info(common,
-				 "Ignoring endianness difference in eepmisc register.\n");
-		} else {
-			*swap_needed = true;
-			ath_dbg(common, EEPROM,
-				"EEPROM needs swapping according to the eepmisc register.\n");
-		}
+	if (ah->eep_ops->get_eepmisc(ah) & AR5416_EEPMISC_BIG_ENDIAN) {
+		*swap_needed = true;
+		ath_dbg(common, EEPROM,
+			"Big Endian EEPROM detected according to EEPMISC register.\n");
+	} else {
+		*swap_needed = false;
 	}
 
 	return 0;
 }
 
-#undef EXPECTED_EEPMISC_VAL
-
 bool ath9k_hw_nvram_validate_checksum(struct ath_hw *ah, int size)
 {
 	u32 i, sum = 0;
diff --git a/drivers/net/wireless/ath/ath9k/eeprom.h b/drivers/net/wireless/ath/ath9k/eeprom.h
index 5a24fb5..30bf722 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom.h
+++ b/drivers/net/wireless/ath/ath9k/eeprom.h
@@ -23,6 +23,17 @@
 #include <net/cfg80211.h>
 #include "ar9003_eeprom.h"
 
+/* helpers to swap EEPROM fields, which are stored as __le16 or __le32. Since
+ * we are 100% sure about it we __force these to u16/u32 for the swab calls to
+ * silence the sparse checks. These macros are used when we have a Big Endian
+ * EEPROM (according to AR5416_EEPMISC_BIG_ENDIAN) and need to convert the
+ * fields to __le16/__le32.
+ */
+#define EEPROM_FIELD_SWAB16(field) \
+	(field = (__force __le16)swab16((__force u16)field))
+#define EEPROM_FIELD_SWAB32(field) \
+	(field = (__force __le32)swab32((__force u32)field))
+
 #ifdef __BIG_ENDIAN
 #define AR5416_EEPROM_MAGIC 0x5aa5
 #else
@@ -270,19 +281,19 @@ enum ath9k_hal_freq_band {
 };
 
 struct base_eep_header {
-	u16 length;
-	u16 checksum;
-	u16 version;
+	__le16 length;
+	__le16 checksum;
+	__le16 version;
 	u8 opCapFlags;
 	u8 eepMisc;
-	u16 regDmn[2];
+	__le16 regDmn[2];
 	u8 macAddr[6];
 	u8 rxMask;
 	u8 txMask;
-	u16 rfSilent;
-	u16 blueToothOptions;
-	u16 deviceCap;
-	u32 binBuildNumber;
+	__le16 rfSilent;
+	__le16 blueToothOptions;
+	__le16 deviceCap;
+	__le32 binBuildNumber;
 	u8 deviceType;
 	u8 pwdclkind;
 	u8 fastClk5g;
@@ -300,33 +311,33 @@ struct base_eep_header {
 } __packed;
 
 struct base_eep_header_4k {
-	u16 length;
-	u16 checksum;
-	u16 version;
+	__le16 length;
+	__le16 checksum;
+	__le16 version;
 	u8 opCapFlags;
 	u8 eepMisc;
-	u16 regDmn[2];
+	__le16 regDmn[2];
 	u8 macAddr[6];
 	u8 rxMask;
 	u8 txMask;
-	u16 rfSilent;
-	u16 blueToothOptions;
-	u16 deviceCap;
-	u32 binBuildNumber;
+	__le16 rfSilent;
+	__le16 blueToothOptions;
+	__le16 deviceCap;
+	__le32 binBuildNumber;
 	u8 deviceType;
 	u8 txGainType;
 } __packed;
 
 
 struct spur_chan {
-	u16 spurChan;
+	__le16 spurChan;
 	u8 spurRangeLow;
 	u8 spurRangeHigh;
 } __packed;
 
 struct modal_eep_header {
-	u32 antCtrlChain[AR5416_MAX_CHAINS];
-	u32 antCtrlCommon;
+	__le32 antCtrlChain[AR5416_MAX_CHAINS];
+	__le32 antCtrlCommon;
 	u8 antennaGainCh[AR5416_MAX_CHAINS];
 	u8 switchSettling;
 	u8 txRxAttenCh[AR5416_MAX_CHAINS];
@@ -361,7 +372,7 @@ struct modal_eep_header {
 	u8 db_ch1;
 	u8 lna_ctl;
 	u8 miscBits;
-	u16 xpaBiasLvlFreq[3];
+	__le16 xpaBiasLvlFreq[3];
 	u8 futureModal[6];
 
 	struct spur_chan spurChans[AR_EEPROM_MODAL_SPURS];
@@ -375,8 +386,8 @@ struct calDataPerFreqOpLoop {
 } __packed;
 
 struct modal_eep_4k_header {
-	u32 antCtrlChain[AR5416_EEP4K_MAX_CHAINS];
-	u32 antCtrlCommon;
+	__le32 antCtrlChain[AR5416_EEP4K_MAX_CHAINS];
+	__le32 antCtrlCommon;
 	u8 antennaGainCh[AR5416_EEP4K_MAX_CHAINS];
 	u8 switchSettling;
 	u8 txRxAttenCh[AR5416_EEP4K_MAX_CHAINS];
@@ -440,19 +451,19 @@ struct modal_eep_4k_header {
 } __packed;
 
 struct base_eep_ar9287_header {
-	u16 length;
-	u16 checksum;
-	u16 version;
+	__le16 length;
+	__le16 checksum;
+	__le16 version;
 	u8 opCapFlags;
 	u8 eepMisc;
-	u16 regDmn[2];
+	__le16 regDmn[2];
 	u8 macAddr[6];
 	u8 rxMask;
 	u8 txMask;
-	u16 rfSilent;
-	u16 blueToothOptions;
-	u16 deviceCap;
-	u32 binBuildNumber;
+	__le16 rfSilent;
+	__le16 blueToothOptions;
+	__le16 deviceCap;
+	__le32 binBuildNumber;
 	u8 deviceType;
 	u8 openLoopPwrCntl;
 	int8_t pwrTableOffset;
@@ -462,8 +473,8 @@ struct base_eep_ar9287_header {
 } __packed;
 
 struct modal_eep_ar9287_header {
-	u32 antCtrlChain[AR9287_MAX_CHAINS];
-	u32 antCtrlCommon;
+	__le32 antCtrlChain[AR9287_MAX_CHAINS];
+	__le32 antCtrlCommon;
 	int8_t antennaGainCh[AR9287_MAX_CHAINS];
 	u8 switchSettling;
 	u8 txRxAttenCh[AR9287_MAX_CHAINS];
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_4k.c b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
index 76ce109..4a01ebe 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_4k.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
@@ -20,7 +20,7 @@
 
 static int ath9k_hw_4k_get_eeprom_ver(struct ath_hw *ah)
 {
-	u16 version = ah->eeprom.map4k.baseEepHeader.version;
+	u16 version = le16_to_cpu(ah->eeprom.map4k.baseEepHeader.version);
 
 	return (version & AR5416_EEP_VER_MAJOR_MASK) >>
 		AR5416_EEP_VER_MAJOR_SHIFT;
@@ -28,7 +28,7 @@ static int ath9k_hw_4k_get_eeprom_ver(struct ath_hw *ah)
 
 static int ath9k_hw_4k_get_eeprom_rev(struct ath_hw *ah)
 {
-	u16 version = ah->eeprom.map4k.baseEepHeader.version;
+	u16 version = le16_to_cpu(ah->eeprom.map4k.baseEepHeader.version);
 
 	return version & AR5416_EEP_VER_MINOR_MASK;
 }
@@ -76,8 +76,8 @@ static bool ath9k_hw_4k_fill_eeprom(struct ath_hw *ah)
 static u32 ath9k_dump_4k_modal_eeprom(char *buf, u32 len, u32 size,
 				      struct modal_eep_4k_header *modal_hdr)
 {
-	PR_EEP("Chain0 Ant. Control", modal_hdr->antCtrlChain[0]);
-	PR_EEP("Ant. Common Control", modal_hdr->antCtrlCommon);
+	PR_EEP("Chain0 Ant. Control", le16_to_cpu(modal_hdr->antCtrlChain[0]));
+	PR_EEP("Ant. Common Control", le32_to_cpu(modal_hdr->antCtrlCommon));
 	PR_EEP("Chain0 Ant. Gain", modal_hdr->antennaGainCh[0]);
 	PR_EEP("Switch Settle", modal_hdr->switchSettling);
 	PR_EEP("Chain0 TxRxAtten", modal_hdr->txRxAttenCh[0]);
@@ -132,6 +132,7 @@ static u32 ath9k_hw_4k_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 {
 	struct ar5416_eeprom_4k *eep = &ah->eeprom.map4k;
 	struct base_eep_header_4k *pBase = &eep->baseEepHeader;
+	u32 binBuildNumber = le32_to_cpu(pBase->binBuildNumber);
 
 	if (!dump_base_hdr) {
 		len += scnprintf(buf + len, size - len,
@@ -143,10 +144,10 @@ static u32 ath9k_hw_4k_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 
 	PR_EEP("Major Version", ath9k_hw_4k_get_eeprom_ver(ah));
 	PR_EEP("Minor Version", ath9k_hw_4k_get_eeprom_rev(ah));
-	PR_EEP("Checksum", pBase->checksum);
-	PR_EEP("Length", pBase->length);
-	PR_EEP("RegDomain1", pBase->regDmn[0]);
-	PR_EEP("RegDomain2", pBase->regDmn[1]);
+	PR_EEP("Checksum", le16_to_cpu(pBase->checksum));
+	PR_EEP("Length", le16_to_cpu(pBase->length));
+	PR_EEP("RegDomain1", le16_to_cpu(pBase->regDmn[0]));
+	PR_EEP("RegDomain2", le16_to_cpu(pBase->regDmn[1]));
 	PR_EEP("TX Mask", pBase->txMask);
 	PR_EEP("RX Mask", pBase->rxMask);
 	PR_EEP("Allow 5GHz", !!(pBase->opCapFlags & AR5416_OPFLAGS_11A));
@@ -160,9 +161,9 @@ static u32 ath9k_hw_4k_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 	PR_EEP("Disable 5Ghz HT40", !!(pBase->opCapFlags &
 					AR5416_OPFLAGS_N_5G_HT40));
 	PR_EEP("Big Endian", !!(pBase->eepMisc & AR5416_EEPMISC_BIG_ENDIAN));
-	PR_EEP("Cal Bin Major Ver", (pBase->binBuildNumber >> 24) & 0xFF);
-	PR_EEP("Cal Bin Minor Ver", (pBase->binBuildNumber >> 16) & 0xFF);
-	PR_EEP("Cal Bin Build", (pBase->binBuildNumber >> 8) & 0xFF);
+	PR_EEP("Cal Bin Major Ver", (binBuildNumber >> 24) & 0xFF);
+	PR_EEP("Cal Bin Minor Ver", (binBuildNumber >> 16) & 0xFF);
+	PR_EEP("Cal Bin Build", (binBuildNumber >> 8) & 0xFF);
 	PR_EEP("TX Gain type", pBase->txGainType);
 
 	len += scnprintf(buf + len, size - len, "%20s : %pM\n", "MacAddress",
@@ -194,54 +195,31 @@ static int ath9k_hw_4k_check_eeprom(struct ath_hw *ah)
 		return err;
 
 	if (need_swap)
-		el = swab16(eep->baseEepHeader.length);
+		el = swab16((__force u16)eep->baseEepHeader.length);
 	else
-		el = eep->baseEepHeader.length;
+		el = le16_to_cpu(eep->baseEepHeader.length);
 
 	el = min(el / sizeof(u16), SIZE_EEPROM_4K);
 	if (!ath9k_hw_nvram_validate_checksum(ah, el))
 		return -EINVAL;
 
 	if (need_swap) {
-		u32 integer;
-		u16 word;
-
-		word = swab16(eep->baseEepHeader.length);
-		eep->baseEepHeader.length = word;
-
-		word = swab16(eep->baseEepHeader.checksum);
-		eep->baseEepHeader.checksum = word;
-
-		word = swab16(eep->baseEepHeader.version);
-		eep->baseEepHeader.version = word;
-
-		word = swab16(eep->baseEepHeader.regDmn[0]);
-		eep->baseEepHeader.regDmn[0] = word;
-
-		word = swab16(eep->baseEepHeader.regDmn[1]);
-		eep->baseEepHeader.regDmn[1] = word;
-
-		word = swab16(eep->baseEepHeader.rfSilent);
-		eep->baseEepHeader.rfSilent = word;
-
-		word = swab16(eep->baseEepHeader.blueToothOptions);
-		eep->baseEepHeader.blueToothOptions = word;
-
-		word = swab16(eep->baseEepHeader.deviceCap);
-		eep->baseEepHeader.deviceCap = word;
-
-		integer = swab32(eep->modalHeader.antCtrlCommon);
-		eep->modalHeader.antCtrlCommon = integer;
-
-		for (i = 0; i < AR5416_EEP4K_MAX_CHAINS; i++) {
-			integer = swab32(eep->modalHeader.antCtrlChain[i]);
-			eep->modalHeader.antCtrlChain[i] = integer;
-		}
-
-		for (i = 0; i < AR_EEPROM_MODAL_SPURS; i++) {
-			word = swab16(eep->modalHeader.spurChans[i].spurChan);
-			eep->modalHeader.spurChans[i].spurChan = word;
-		}
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.length);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.checksum);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.version);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.regDmn[0]);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.regDmn[1]);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.rfSilent);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.blueToothOptions);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.deviceCap);
+		EEPROM_FIELD_SWAB32(eep->modalHeader.antCtrlCommon);
+
+		for (i = 0; i < AR5416_EEP4K_MAX_CHAINS; i++)
+			EEPROM_FIELD_SWAB32(eep->modalHeader.antCtrlChain[i]);
+
+		for (i = 0; i < AR_EEPROM_MODAL_SPURS; i++)
+			EEPROM_FIELD_SWAB16(
+				eep->modalHeader.spurChans[i].spurChan);
 	}
 
 	if (!ath9k_hw_nvram_check_version(ah, AR5416_EEP_VER,
@@ -270,13 +248,13 @@ static u32 ath9k_hw_4k_get_eeprom(struct ath_hw *ah,
 	case EEP_MAC_MSW:
 		return get_unaligned_be16(pBase->macAddr + 4);
 	case EEP_REG_0:
-		return pBase->regDmn[0];
+		return le16_to_cpu(pBase->regDmn[0]);
 	case EEP_OP_CAP:
-		return pBase->deviceCap;
+		return le16_to_cpu(pBase->deviceCap);
 	case EEP_OP_MODE:
 		return pBase->opCapFlags;
 	case EEP_RF_SILENT:
-		return pBase->rfSilent;
+		return le16_to_cpu(pBase->rfSilent);
 	case EEP_OB_2:
 		return pModal->ob_0;
 	case EEP_DB_2:
@@ -724,7 +702,7 @@ static void ath9k_hw_4k_set_gain(struct ath_hw *ah,
 {
 	ENABLE_REG_RMW_BUFFER(ah);
 	REG_RMW(ah, AR_PHY_SWITCH_CHAIN_0,
-		pModal->antCtrlChain[0], 0);
+		le32_to_cpu(pModal->antCtrlChain[0]), 0);
 
 	REG_RMW(ah, AR_PHY_TIMING_CTRL4(0),
 		SM(pModal->iqCalICh[0], AR_PHY_TIMING_CTRL4_IQCORR_Q_I_COFF) |
@@ -790,7 +768,7 @@ static void ath9k_hw_4k_set_board_values(struct ath_hw *ah,
 	pModal = &eep->modalHeader;
 	txRxAttenLocal = 23;
 
-	REG_WRITE(ah, AR_PHY_SWITCH_COM, pModal->antCtrlCommon);
+	REG_WRITE(ah, AR_PHY_SWITCH_COM, le32_to_cpu(pModal->antCtrlCommon));
 
 	/* Single chain for 4K EEPROM*/
 	ath9k_hw_4k_set_gain(ah, pModal, eep, txRxAttenLocal);
@@ -1054,7 +1032,7 @@ static void ath9k_hw_4k_set_board_values(struct ath_hw *ah,
 
 static u16 ath9k_hw_4k_get_spur_channel(struct ath_hw *ah, u16 i, bool is2GHz)
 {
-	return ah->eeprom.map4k.modalHeader.spurChans[i].spurChan;
+	return le16_to_cpu(ah->eeprom.map4k.modalHeader.spurChans[i].spurChan);
 }
 
 static u8 ath9k_hw_4k_get_eepmisc(struct ath_hw *ah)
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_9287.c b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
index daeaf12..9611f02 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_9287.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_9287.c
@@ -22,7 +22,7 @@
 
 static int ath9k_hw_ar9287_get_eeprom_ver(struct ath_hw *ah)
 {
-	u16 version = ah->eeprom.map9287.baseEepHeader.version;
+	u16 version = le16_to_cpu(ah->eeprom.map9287.baseEepHeader.version);
 
 	return (version & AR5416_EEP_VER_MAJOR_MASK) >>
 		AR5416_EEP_VER_MAJOR_SHIFT;
@@ -30,7 +30,7 @@ static int ath9k_hw_ar9287_get_eeprom_ver(struct ath_hw *ah)
 
 static int ath9k_hw_ar9287_get_eeprom_rev(struct ath_hw *ah)
 {
-	u16 version = ah->eeprom.map9287.baseEepHeader.version;
+	u16 version = le16_to_cpu(ah->eeprom.map9287.baseEepHeader.version);
 
 	return version & AR5416_EEP_VER_MINOR_MASK;
 }
@@ -79,9 +79,9 @@ static bool ath9k_hw_ar9287_fill_eeprom(struct ath_hw *ah)
 static u32 ar9287_dump_modal_eeprom(char *buf, u32 len, u32 size,
 				    struct modal_eep_ar9287_header *modal_hdr)
 {
-	PR_EEP("Chain0 Ant. Control", modal_hdr->antCtrlChain[0]);
-	PR_EEP("Chain1 Ant. Control", modal_hdr->antCtrlChain[1]);
-	PR_EEP("Ant. Common Control", modal_hdr->antCtrlCommon);
+	PR_EEP("Chain0 Ant. Control", le16_to_cpu(modal_hdr->antCtrlChain[0]));
+	PR_EEP("Chain1 Ant. Control", le16_to_cpu(modal_hdr->antCtrlChain[1]));
+	PR_EEP("Ant. Common Control", le32_to_cpu(modal_hdr->antCtrlCommon));
 	PR_EEP("Chain0 Ant. Gain", modal_hdr->antennaGainCh[0]);
 	PR_EEP("Chain1 Ant. Gain", modal_hdr->antennaGainCh[1]);
 	PR_EEP("Switch Settle", modal_hdr->switchSettling);
@@ -128,6 +128,7 @@ static u32 ath9k_hw_ar9287_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 {
 	struct ar9287_eeprom *eep = &ah->eeprom.map9287;
 	struct base_eep_ar9287_header *pBase = &eep->baseEepHeader;
+	u32 binBuildNumber = le32_to_cpu(pBase->binBuildNumber);
 
 	if (!dump_base_hdr) {
 		len += scnprintf(buf + len, size - len,
@@ -139,10 +140,10 @@ static u32 ath9k_hw_ar9287_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 
 	PR_EEP("Major Version", ath9k_hw_ar9287_get_eeprom_ver(ah));
 	PR_EEP("Minor Version", ath9k_hw_ar9287_get_eeprom_rev(ah));
-	PR_EEP("Checksum", pBase->checksum);
-	PR_EEP("Length", pBase->length);
-	PR_EEP("RegDomain1", pBase->regDmn[0]);
-	PR_EEP("RegDomain2", pBase->regDmn[1]);
+	PR_EEP("Checksum", le16_to_cpu(pBase->checksum));
+	PR_EEP("Length", le16_to_cpu(pBase->length));
+	PR_EEP("RegDomain1", le16_to_cpu(pBase->regDmn[0]));
+	PR_EEP("RegDomain2", le16_to_cpu(pBase->regDmn[1]));
 	PR_EEP("TX Mask", pBase->txMask);
 	PR_EEP("RX Mask", pBase->rxMask);
 	PR_EEP("Allow 5GHz", !!(pBase->opCapFlags & AR5416_OPFLAGS_11A));
@@ -156,9 +157,9 @@ static u32 ath9k_hw_ar9287_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 	PR_EEP("Disable 5Ghz HT40", !!(pBase->opCapFlags &
 					AR5416_OPFLAGS_N_5G_HT40));
 	PR_EEP("Big Endian", !!(pBase->eepMisc & AR5416_EEPMISC_BIG_ENDIAN));
-	PR_EEP("Cal Bin Major Ver", (pBase->binBuildNumber >> 24) & 0xFF);
-	PR_EEP("Cal Bin Minor Ver", (pBase->binBuildNumber >> 16) & 0xFF);
-	PR_EEP("Cal Bin Build", (pBase->binBuildNumber >> 8) & 0xFF);
+	PR_EEP("Cal Bin Major Ver", (binBuildNumber >> 24) & 0xFF);
+	PR_EEP("Cal Bin Minor Ver", (binBuildNumber >> 16) & 0xFF);
+	PR_EEP("Cal Bin Build", (binBuildNumber >> 8) & 0xFF);
 	PR_EEP("Power Table Offset", pBase->pwrTableOffset);
 	PR_EEP("OpenLoop Power Ctrl", pBase->openLoopPwrCntl);
 
@@ -182,8 +183,7 @@ static u32 ath9k_hw_ar9287_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 
 static int ath9k_hw_ar9287_check_eeprom(struct ath_hw *ah)
 {
-	u32 el, integer;
-	u16 word;
+	u32 el;
 	int i, err;
 	bool need_swap;
 	struct ar9287_eeprom *eep = &ah->eeprom.map9287;
@@ -193,51 +193,31 @@ static int ath9k_hw_ar9287_check_eeprom(struct ath_hw *ah)
 		return err;
 
 	if (need_swap)
-		el = swab16(eep->baseEepHeader.length);
+		el = swab16((__force u16)eep->baseEepHeader.length);
 	else
-		el = eep->baseEepHeader.length;
+		el = le16_to_cpu(eep->baseEepHeader.length);
 
 	el = min(el / sizeof(u16), SIZE_EEPROM_AR9287);
 	if (!ath9k_hw_nvram_validate_checksum(ah, el))
 		return -EINVAL;
 
 	if (need_swap) {
-		word = swab16(eep->baseEepHeader.length);
-		eep->baseEepHeader.length = word;
-
-		word = swab16(eep->baseEepHeader.checksum);
-		eep->baseEepHeader.checksum = word;
-
-		word = swab16(eep->baseEepHeader.version);
-		eep->baseEepHeader.version = word;
-
-		word = swab16(eep->baseEepHeader.regDmn[0]);
-		eep->baseEepHeader.regDmn[0] = word;
-
-		word = swab16(eep->baseEepHeader.regDmn[1]);
-		eep->baseEepHeader.regDmn[1] = word;
-
-		word = swab16(eep->baseEepHeader.rfSilent);
-		eep->baseEepHeader.rfSilent = word;
-
-		word = swab16(eep->baseEepHeader.blueToothOptions);
-		eep->baseEepHeader.blueToothOptions = word;
-
-		word = swab16(eep->baseEepHeader.deviceCap);
-		eep->baseEepHeader.deviceCap = word;
-
-		integer = swab32(eep->modalHeader.antCtrlCommon);
-		eep->modalHeader.antCtrlCommon = integer;
-
-		for (i = 0; i < AR9287_MAX_CHAINS; i++) {
-			integer = swab32(eep->modalHeader.antCtrlChain[i]);
-			eep->modalHeader.antCtrlChain[i] = integer;
-		}
-
-		for (i = 0; i < AR_EEPROM_MODAL_SPURS; i++) {
-			word = swab16(eep->modalHeader.spurChans[i].spurChan);
-			eep->modalHeader.spurChans[i].spurChan = word;
-		}
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.length);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.checksum);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.version);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.regDmn[0]);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.regDmn[1]);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.rfSilent);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.blueToothOptions);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.deviceCap);
+		EEPROM_FIELD_SWAB32(eep->modalHeader.antCtrlCommon);
+
+		for (i = 0; i < AR9287_MAX_CHAINS; i++)
+			EEPROM_FIELD_SWAB32(eep->modalHeader.antCtrlChain[i]);
+
+		for (i = 0; i < AR_EEPROM_MODAL_SPURS; i++)
+			EEPROM_FIELD_SWAB16(
+				eep->modalHeader.spurChans[i].spurChan);
 	}
 
 	if (!ath9k_hw_nvram_check_version(ah, AR9287_EEP_VER,
@@ -267,13 +247,13 @@ static u32 ath9k_hw_ar9287_get_eeprom(struct ath_hw *ah,
 	case EEP_MAC_MSW:
 		return get_unaligned_be16(pBase->macAddr + 4);
 	case EEP_REG_0:
-		return pBase->regDmn[0];
+		return le16_to_cpu(pBase->regDmn[0]);
 	case EEP_OP_CAP:
-		return pBase->deviceCap;
+		return le16_to_cpu(pBase->deviceCap);
 	case EEP_OP_MODE:
 		return pBase->opCapFlags;
 	case EEP_RF_SILENT:
-		return pBase->rfSilent;
+		return le16_to_cpu(pBase->rfSilent);
 	case EEP_TX_MASK:
 		return pBase->txMask;
 	case EEP_RX_MASK:
@@ -878,13 +858,13 @@ static void ath9k_hw_ar9287_set_board_values(struct ath_hw *ah,
 
 	pModal = &eep->modalHeader;
 
-	REG_WRITE(ah, AR_PHY_SWITCH_COM, pModal->antCtrlCommon);
+	REG_WRITE(ah, AR_PHY_SWITCH_COM, le32_to_cpu(pModal->antCtrlCommon));
 
 	for (i = 0; i < AR9287_MAX_CHAINS; i++)	{
 		regChainOffset = i * 0x1000;
 
 		REG_WRITE(ah, AR_PHY_SWITCH_CHAIN_0 + regChainOffset,
-			  pModal->antCtrlChain[i]);
+			  le32_to_cpu(pModal->antCtrlChain[i]));
 
 		REG_WRITE(ah, AR_PHY_TIMING_CTRL4(0) + regChainOffset,
 			  (REG_READ(ah, AR_PHY_TIMING_CTRL4(0) + regChainOffset)
@@ -982,7 +962,9 @@ static void ath9k_hw_ar9287_set_board_values(struct ath_hw *ah,
 static u16 ath9k_hw_ar9287_get_spur_channel(struct ath_hw *ah,
 					    u16 i, bool is2GHz)
 {
-	return ah->eeprom.map9287.modalHeader.spurChans[i].spurChan;
+	__le16 spur_ch = ah->eeprom.map9287.modalHeader.spurChans[i].spurChan;
+
+	return le16_to_cpu(spur_ch);
 }
 
 static u8 ath9k_hw_ar9287_get_eepmisc(struct ath_hw *ah)
diff --git a/drivers/net/wireless/ath/ath9k/eeprom_def.c b/drivers/net/wireless/ath/ath9k/eeprom_def.c
index bd5bd62..7d52234 100644
--- a/drivers/net/wireless/ath/ath9k/eeprom_def.c
+++ b/drivers/net/wireless/ath/ath9k/eeprom_def.c
@@ -79,7 +79,7 @@ static void ath9k_olc_get_pdadcs(struct ath_hw *ah,
 
 static int ath9k_hw_def_get_eeprom_ver(struct ath_hw *ah)
 {
-	u16 version = ah->eeprom.def.baseEepHeader.version;
+	u16 version = le16_to_cpu(ah->eeprom.def.baseEepHeader.version);
 
 	return (version & AR5416_EEP_VER_MAJOR_MASK) >>
 		AR5416_EEP_VER_MAJOR_SHIFT;
@@ -87,7 +87,7 @@ static int ath9k_hw_def_get_eeprom_ver(struct ath_hw *ah)
 
 static int ath9k_hw_def_get_eeprom_rev(struct ath_hw *ah)
 {
-	u16 version = ah->eeprom.def.baseEepHeader.version;
+	u16 version = le16_to_cpu(ah->eeprom.def.baseEepHeader.version);
 
 	return version & AR5416_EEP_VER_MINOR_MASK;
 }
@@ -135,10 +135,10 @@ static bool ath9k_hw_def_fill_eeprom(struct ath_hw *ah)
 static u32 ath9k_def_dump_modal_eeprom(char *buf, u32 len, u32 size,
 				       struct modal_eep_header *modal_hdr)
 {
-	PR_EEP("Chain0 Ant. Control", modal_hdr->antCtrlChain[0]);
-	PR_EEP("Chain1 Ant. Control", modal_hdr->antCtrlChain[1]);
-	PR_EEP("Chain2 Ant. Control", modal_hdr->antCtrlChain[2]);
-	PR_EEP("Ant. Common Control", modal_hdr->antCtrlCommon);
+	PR_EEP("Chain0 Ant. Control", le16_to_cpu(modal_hdr->antCtrlChain[0]));
+	PR_EEP("Chain1 Ant. Control", le16_to_cpu(modal_hdr->antCtrlChain[1]));
+	PR_EEP("Chain2 Ant. Control", le16_to_cpu(modal_hdr->antCtrlChain[2]));
+	PR_EEP("Ant. Common Control", le32_to_cpu(modal_hdr->antCtrlCommon));
 	PR_EEP("Chain0 Ant. Gain", modal_hdr->antennaGainCh[0]);
 	PR_EEP("Chain1 Ant. Gain", modal_hdr->antennaGainCh[1]);
 	PR_EEP("Chain2 Ant. Gain", modal_hdr->antennaGainCh[2]);
@@ -194,9 +194,9 @@ static u32 ath9k_def_dump_modal_eeprom(char *buf, u32 len, u32 size,
 	PR_EEP("Chain1 OutputBias", modal_hdr->ob_ch1);
 	PR_EEP("Chain1 DriverBias", modal_hdr->db_ch1);
 	PR_EEP("LNA Control", modal_hdr->lna_ctl);
-	PR_EEP("XPA Bias Freq0", modal_hdr->xpaBiasLvlFreq[0]);
-	PR_EEP("XPA Bias Freq1", modal_hdr->xpaBiasLvlFreq[1]);
-	PR_EEP("XPA Bias Freq2", modal_hdr->xpaBiasLvlFreq[2]);
+	PR_EEP("XPA Bias Freq0", le16_to_cpu(modal_hdr->xpaBiasLvlFreq[0]));
+	PR_EEP("XPA Bias Freq1", le16_to_cpu(modal_hdr->xpaBiasLvlFreq[1]));
+	PR_EEP("XPA Bias Freq2", le16_to_cpu(modal_hdr->xpaBiasLvlFreq[2]));
 
 	return len;
 }
@@ -206,6 +206,7 @@ static u32 ath9k_hw_def_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 {
 	struct ar5416_eeprom_def *eep = &ah->eeprom.def;
 	struct base_eep_header *pBase = &eep->baseEepHeader;
+	u32 binBuildNumber = le32_to_cpu(pBase->binBuildNumber);
 
 	if (!dump_base_hdr) {
 		len += scnprintf(buf + len, size - len,
@@ -221,10 +222,10 @@ static u32 ath9k_hw_def_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 
 	PR_EEP("Major Version", ath9k_hw_def_get_eeprom_ver(ah));
 	PR_EEP("Minor Version", ath9k_hw_def_get_eeprom_rev(ah));
-	PR_EEP("Checksum", pBase->checksum);
-	PR_EEP("Length", pBase->length);
-	PR_EEP("RegDomain1", pBase->regDmn[0]);
-	PR_EEP("RegDomain2", pBase->regDmn[1]);
+	PR_EEP("Checksum", le16_to_cpu(pBase->checksum));
+	PR_EEP("Length", le16_to_cpu(pBase->length));
+	PR_EEP("RegDomain1", le16_to_cpu(pBase->regDmn[0]));
+	PR_EEP("RegDomain2", le16_to_cpu(pBase->regDmn[1]));
 	PR_EEP("TX Mask", pBase->txMask);
 	PR_EEP("RX Mask", pBase->rxMask);
 	PR_EEP("Allow 5GHz", !!(pBase->opCapFlags & AR5416_OPFLAGS_11A));
@@ -238,9 +239,9 @@ static u32 ath9k_hw_def_dump_eeprom(struct ath_hw *ah, bool dump_base_hdr,
 	PR_EEP("Disable 5Ghz HT40", !!(pBase->opCapFlags &
 					AR5416_OPFLAGS_N_5G_HT40));
 	PR_EEP("Big Endian", !!(pBase->eepMisc & AR5416_EEPMISC_BIG_ENDIAN));
-	PR_EEP("Cal Bin Major Ver", (pBase->binBuildNumber >> 24) & 0xFF);
-	PR_EEP("Cal Bin Minor Ver", (pBase->binBuildNumber >> 16) & 0xFF);
-	PR_EEP("Cal Bin Build", (pBase->binBuildNumber >> 8) & 0xFF);
+	PR_EEP("Cal Bin Major Ver", (binBuildNumber >> 24) & 0xFF);
+	PR_EEP("Cal Bin Minor Ver", (binBuildNumber >> 16) & 0xFF);
+	PR_EEP("Cal Bin Build", (binBuildNumber >> 8) & 0xFF);
 	PR_EEP("OpenLoop Power Ctrl", pBase->openLoopPwrCntl);
 
 	len += scnprintf(buf + len, size - len, "%20s : %pM\n", "MacAddress",
@@ -273,61 +274,40 @@ static int ath9k_hw_def_check_eeprom(struct ath_hw *ah)
 		return err;
 
 	if (need_swap)
-		el = swab16(eep->baseEepHeader.length);
+		el = swab16((__force u16)eep->baseEepHeader.length);
 	else
-		el = eep->baseEepHeader.length;
+		el = le16_to_cpu(eep->baseEepHeader.length);
 
 	el = min(el / sizeof(u16), SIZE_EEPROM_DEF);
 	if (!ath9k_hw_nvram_validate_checksum(ah, el))
 		return -EINVAL;
 
 	if (need_swap) {
-		u32 integer, j;
-		u16 word;
+		u32 j;
 
-		word = swab16(eep->baseEepHeader.length);
-		eep->baseEepHeader.length = word;
-
-		word = swab16(eep->baseEepHeader.checksum);
-		eep->baseEepHeader.checksum = word;
-
-		word = swab16(eep->baseEepHeader.version);
-		eep->baseEepHeader.version = word;
-
-		word = swab16(eep->baseEepHeader.regDmn[0]);
-		eep->baseEepHeader.regDmn[0] = word;
-
-		word = swab16(eep->baseEepHeader.regDmn[1]);
-		eep->baseEepHeader.regDmn[1] = word;
-
-		word = swab16(eep->baseEepHeader.rfSilent);
-		eep->baseEepHeader.rfSilent = word;
-
-		word = swab16(eep->baseEepHeader.blueToothOptions);
-		eep->baseEepHeader.blueToothOptions = word;
-
-		word = swab16(eep->baseEepHeader.deviceCap);
-		eep->baseEepHeader.deviceCap = word;
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.length);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.checksum);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.version);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.regDmn[0]);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.regDmn[1]);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.rfSilent);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.blueToothOptions);
+		EEPROM_FIELD_SWAB16(eep->baseEepHeader.deviceCap);
 
 		for (j = 0; j < ARRAY_SIZE(eep->modalHeader); j++) {
 			struct modal_eep_header *pModal =
 				&eep->modalHeader[j];
-			integer = swab32(pModal->antCtrlCommon);
-			pModal->antCtrlCommon = integer;
+			EEPROM_FIELD_SWAB32(pModal->antCtrlCommon);
 
-			for (i = 0; i < AR5416_MAX_CHAINS; i++) {
-				integer = swab32(pModal->antCtrlChain[i]);
-				pModal->antCtrlChain[i] = integer;
-			}
-			for (i = 0; i < 3; i++) {
-				word = swab16(pModal->xpaBiasLvlFreq[i]);
-				pModal->xpaBiasLvlFreq[i] = word;
-			}
+			for (i = 0; i < AR5416_MAX_CHAINS; i++)
+				EEPROM_FIELD_SWAB32(pModal->antCtrlChain[i]);
 
-			for (i = 0; i < AR_EEPROM_MODAL_SPURS; i++) {
-				word = swab16(pModal->spurChans[i].spurChan);
-				pModal->spurChans[i].spurChan = word;
-			}
+			for (i = 0; i < 3; i++)
+				EEPROM_FIELD_SWAB16(pModal->xpaBiasLvlFreq[i]);
+
+			for (i = 0; i < AR_EEPROM_MODAL_SPURS; i++)
+				EEPROM_FIELD_SWAB16(
+					pModal->spurChans[i].spurChan);
 		}
 	}
 
@@ -337,7 +317,7 @@ static int ath9k_hw_def_check_eeprom(struct ath_hw *ah)
 
 	/* Enable fixup for AR_AN_TOP2 if necessary */
 	if ((ah->hw_version.devid == AR9280_DEVID_PCI) &&
-	    ((eep->baseEepHeader.version & 0xff) > 0x0a) &&
+	    ((le16_to_cpu(eep->baseEepHeader.version) & 0xff) > 0x0a) &&
 	    (eep->baseEepHeader.pwdclkind == 0))
 		ah->need_an_top2_fixup = true;
 
@@ -370,13 +350,13 @@ static u32 ath9k_hw_def_get_eeprom(struct ath_hw *ah,
 	case EEP_MAC_MSW:
 		return get_unaligned_be16(pBase->macAddr + 4);
 	case EEP_REG_0:
-		return pBase->regDmn[0];
+		return le16_to_cpu(pBase->regDmn[0]);
 	case EEP_OP_CAP:
-		return pBase->deviceCap;
+		return le16_to_cpu(pBase->deviceCap);
 	case EEP_OP_MODE:
 		return pBase->opCapFlags;
 	case EEP_RF_SILENT:
-		return pBase->rfSilent;
+		return le16_to_cpu(pBase->rfSilent);
 	case EEP_OB_5:
 		return pModal[0].ob;
 	case EEP_DB_5:
@@ -490,11 +470,13 @@ static void ath9k_hw_def_set_board_values(struct ath_hw *ah,
 	struct ar5416_eeprom_def *eep = &ah->eeprom.def;
 	int i, regChainOffset;
 	u8 txRxAttenLocal;
+	u32 antCtrlCommon;
 
 	pModal = &(eep->modalHeader[IS_CHAN_2GHZ(chan)]);
 	txRxAttenLocal = IS_CHAN_2GHZ(chan) ? 23 : 44;
+	antCtrlCommon = le32_to_cpu(pModal->antCtrlCommon);
 
-	REG_WRITE(ah, AR_PHY_SWITCH_COM, pModal->antCtrlCommon & 0xffff);
+	REG_WRITE(ah, AR_PHY_SWITCH_COM, antCtrlCommon & 0xffff);
 
 	for (i = 0; i < AR5416_MAX_CHAINS; i++) {
 		if (AR_SREV_9280(ah)) {
@@ -508,7 +490,7 @@ static void ath9k_hw_def_set_board_values(struct ath_hw *ah,
 			regChainOffset = i * 0x1000;
 
 		REG_WRITE(ah, AR_PHY_SWITCH_CHAIN_0 + regChainOffset,
-			  pModal->antCtrlChain[i]);
+			  le32_to_cpu(pModal->antCtrlChain[i]));
 
 		REG_WRITE(ah, AR_PHY_TIMING_CTRL4(0) + regChainOffset,
 			  (REG_READ(ah, AR_PHY_TIMING_CTRL4(0) + regChainOffset) &
@@ -655,7 +637,7 @@ static void ath9k_hw_def_set_board_values(struct ath_hw *ah,
 static void ath9k_hw_def_set_addac(struct ath_hw *ah,
 				   struct ath9k_channel *chan)
 {
-#define XPA_LVL_FREQ(cnt) (pModal->xpaBiasLvlFreq[cnt])
+#define XPA_LVL_FREQ(cnt) (le16_to_cpu(pModal->xpaBiasLvlFreq[cnt]))
 	struct modal_eep_header *pModal;
 	struct ar5416_eeprom_def *eep = &ah->eeprom.def;
 	u8 biaslevel;
@@ -1315,7 +1297,9 @@ static void ath9k_hw_def_set_txpower(struct ath_hw *ah,
 
 static u16 ath9k_hw_def_get_spur_channel(struct ath_hw *ah, u16 i, bool is2GHz)
 {
-	return ah->eeprom.def.modalHeader[is2GHz].spurChans[i].spurChan;
+	__le16 spch = ah->eeprom.def.modalHeader[is2GHz].spurChans[i].spurChan;
+
+	return le16_to_cpu(spch);
 }
 
 static u8 ath9k_hw_def_get_eepmisc(struct ath_hw *ah)
-- 
2.10.0

[ath9k-devel] [PATCH v2 0/7] ath9k: EEPROM swapping improvements

[Kalle Valo]

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:

> There are two types of swapping the EEPROM data in the ath9k driver.
> Before this series one type of swapping could not be used without the
> other.
>
> The first type of swapping looks at the "magic bytes" at the start of
> the EEPROM data and performs swab16 on the EEPROM contents if needed.
> The second type of swapping is EEPROM format specific and swaps
> specific fields within the EEPROM itself (swab16, swab32 - depends on
> the EEPROM format).
>
> With this series the second part now looks at the EEPMISC register
> inside the EEPROM, which uses a bit to indicate if the EEPROM data
> is Big Endian (this is also done by the FreeBSD kernel).
> This has a nice advantage: currently there are some out-of-tree hacks
> (in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
> Big Endian system (= no swab16 is performed) but the EEPROM itself
> indicates that it's data is Little Endian. Until now the out-of-tree
> code simply did a swab16 before passing the data to ath9k, so ath9k
> first did the swab16 - this also enabled the format specific swapping.
> These out-of-tree hacks are still working with the new logic, but it
> is recommended to remove them. This implementation is based on a
> discussion with Arnd Bergmann who raised concerns about the
> robustness and portability of the swapping logic in the original OF
> support patch review, see [0].
>
> After a second round of patches (= v1 of this series) neither Arnd
> Bergmann nor I were really happy with the complexity of the EEPROM
> swapping logic. Based on a discussion (see [1] and [2]) we decided
> that ath9k should use a defined format (specifying the endianness
> of the data - I went with __le16 and __le32) when accessing the
> EEPROM fields. A benefit of this is that we enable the EEPMISC based
> swapping logic by default, just like the FreeBSD driver, see [3]. On
> the devices which I have tested (see below) ath9k now works without
> having to specify the "endian_check" field in ath9k_platform_data (or
> a similar logic which could provide this via devicetree) as ath9k now
> detects the endianness automatically. Only EEPROMs which are mangled
> by some out-of-tree code still need the endian_check flag (or one can
> simply remove that mangling from the out-of-tree code).
>
> Testing:
> - tested by myself on AR9287 with Big Endian EEPROM
> - tested by myself on AR9227 with Little Endian EEPROM
> - tested by myself on AR9381 (using the ar9003_eeprom implementation,
>   which did not suffer from this whole problem)
> - how do we proceed with testing? maybe we could keep this in a
>   feature-branch and add these patches to LEDE once we have an ACK to
>   get more people to test this
>
> This series depends on my other series (v7):
> "add devicetree support to ath9k" - see [4]

I think this looks pretty good. If there's a bug somewhere it should be
quite easy to fix so I'm not that worried and would be willing to take
these as soon as I have applied the dependency series. IIRC your
devicetree patches will have at least one more review round so that will
take some time still. In the meantime it would be great if LEDE folks
could take a look at these and comment (or test).

-- 
Kalle Valo

[ath9k-devel] [PATCH v2 0/7] ath9k: EEPROM swapping improvements

[Valo, Kalle]

Kalle Valo <kvalo@codeaurora.org> writes:

> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>
>> There are two types of swapping the EEPROM data in the ath9k driver.
>> Before this series one type of swapping could not be used without the
>> other.
>>
>> The first type of swapping looks at the "magic bytes" at the start of
>> the EEPROM data and performs swab16 on the EEPROM contents if needed.
>> The second type of swapping is EEPROM format specific and swaps
>> specific fields within the EEPROM itself (swab16, swab32 - depends on
>> the EEPROM format).
>>
>> With this series the second part now looks at the EEPMISC register
>> inside the EEPROM, which uses a bit to indicate if the EEPROM data
>> is Big Endian (this is also done by the FreeBSD kernel).
>> This has a nice advantage: currently there are some out-of-tree hacks
>> (in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
>> Big Endian system (= no swab16 is performed) but the EEPROM itself
>> indicates that it's data is Little Endian. Until now the out-of-tree
>> code simply did a swab16 before passing the data to ath9k, so ath9k
>> first did the swab16 - this also enabled the format specific swapping.
>> These out-of-tree hacks are still working with the new logic, but it
>> is recommended to remove them. This implementation is based on a
>> discussion with Arnd Bergmann who raised concerns about the
>> robustness and portability of the swapping logic in the original OF
>> support patch review, see [0].
>>
>> After a second round of patches (= v1 of this series) neither Arnd
>> Bergmann nor I were really happy with the complexity of the EEPROM
>> swapping logic. Based on a discussion (see [1] and [2]) we decided
>> that ath9k should use a defined format (specifying the endianness
>> of the data - I went with __le16 and __le32) when accessing the
>> EEPROM fields. A benefit of this is that we enable the EEPMISC based
>> swapping logic by default, just like the FreeBSD driver, see [3]. On
>> the devices which I have tested (see below) ath9k now works without
>> having to specify the "endian_check" field in ath9k_platform_data (or
>> a similar logic which could provide this via devicetree) as ath9k now
>> detects the endianness automatically. Only EEPROMs which are mangled
>> by some out-of-tree code still need the endian_check flag (or one can
>> simply remove that mangling from the out-of-tree code).
>>
>> Testing:
>> - tested by myself on AR9287 with Big Endian EEPROM
>> - tested by myself on AR9227 with Little Endian EEPROM
>> - tested by myself on AR9381 (using the ar9003_eeprom implementation,
>>   which did not suffer from this whole problem)
>> - how do we proceed with testing? maybe we could keep this in a
>>   feature-branch and add these patches to LEDE once we have an ACK to
>>   get more people to test this
>>
>> This series depends on my other series (v7):
>> "add devicetree support to ath9k" - see [4]
>
> I think this looks pretty good. If there's a bug somewhere it should be
> quite easy to fix so I'm not that worried and would be willing to take
> these as soon as I have applied the dependency series. IIRC your
> devicetree patches will have at least one more review round so that will
> take some time still. In the meantime it would be great if LEDE folks
> could take a look at these and comment (or test).

So are everyone happy with this? I haven't seen any comments. If I don't
here anything I'm planning to take these, most likely for 4.11.

-- 
Kalle Valo

[ath9k-devel] [PATCH v2 0/7] ath9k: EEPROM swapping improvements

[Martin Blumenstingl]

On Fri, Nov 25, 2016 at 4:06 PM, Valo, Kalle <kvalo@qca.qualcomm.com> wrote:
> Kalle Valo <kvalo@codeaurora.org> writes:
>
>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>
>>> There are two types of swapping the EEPROM data in the ath9k driver.
>>> Before this series one type of swapping could not be used without the
>>> other.
>>>
>>> The first type of swapping looks at the "magic bytes" at the start of
>>> the EEPROM data and performs swab16 on the EEPROM contents if needed.
>>> The second type of swapping is EEPROM format specific and swaps
>>> specific fields within the EEPROM itself (swab16, swab32 - depends on
>>> the EEPROM format).
>>>
>>> With this series the second part now looks at the EEPMISC register
>>> inside the EEPROM, which uses a bit to indicate if the EEPROM data
>>> is Big Endian (this is also done by the FreeBSD kernel).
>>> This has a nice advantage: currently there are some out-of-tree hacks
>>> (in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
>>> Big Endian system (= no swab16 is performed) but the EEPROM itself
>>> indicates that it's data is Little Endian. Until now the out-of-tree
>>> code simply did a swab16 before passing the data to ath9k, so ath9k
>>> first did the swab16 - this also enabled the format specific swapping.
>>> These out-of-tree hacks are still working with the new logic, but it
>>> is recommended to remove them. This implementation is based on a
>>> discussion with Arnd Bergmann who raised concerns about the
>>> robustness and portability of the swapping logic in the original OF
>>> support patch review, see [0].
>>>
>>> After a second round of patches (= v1 of this series) neither Arnd
>>> Bergmann nor I were really happy with the complexity of the EEPROM
>>> swapping logic. Based on a discussion (see [1] and [2]) we decided
>>> that ath9k should use a defined format (specifying the endianness
>>> of the data - I went with __le16 and __le32) when accessing the
>>> EEPROM fields. A benefit of this is that we enable the EEPMISC based
>>> swapping logic by default, just like the FreeBSD driver, see [3]. On
>>> the devices which I have tested (see below) ath9k now works without
>>> having to specify the "endian_check" field in ath9k_platform_data (or
>>> a similar logic which could provide this via devicetree) as ath9k now
>>> detects the endianness automatically. Only EEPROMs which are mangled
>>> by some out-of-tree code still need the endian_check flag (or one can
>>> simply remove that mangling from the out-of-tree code).
>>>
>>> Testing:
>>> - tested by myself on AR9287 with Big Endian EEPROM
>>> - tested by myself on AR9227 with Little Endian EEPROM
>>> - tested by myself on AR9381 (using the ar9003_eeprom implementation,
>>>   which did not suffer from this whole problem)
>>> - how do we proceed with testing? maybe we could keep this in a
>>>   feature-branch and add these patches to LEDE once we have an ACK to
>>>   get more people to test this
>>>
>>> This series depends on my other series (v7):
>>> "add devicetree support to ath9k" - see [4]
>>
>> I think this looks pretty good. If there's a bug somewhere it should be
>> quite easy to fix so I'm not that worried and would be willing to take
>> these as soon as I have applied the dependency series. IIRC your
>> devicetree patches will have at least one more review round so that will
>> take some time still. In the meantime it would be great if LEDE folks
>> could take a look at these and comment (or test).
>
> So are everyone happy with this? I haven't seen any comments. If I don't
> here anything I'm planning to take these, most likely for 4.11.
after being busy due to <daytime job and other things in life> I'm
currently trying to get the patches into LEDE: [0] (so far there are
no major objections)
once we get this into LEDE we'll see pretty soon if there are any
problems or not -> 4.11 sounds good to me!


Regards,
Martin


[0] http://lists.infradead.org/pipermail/lede-dev/2016-November/004231.html

[ath9k-devel] [PATCH v2 0/7] ath9k: EEPROM swapping improvements

[Martin Blumenstingl]

Hello Kalle,

On Fri, Nov 25, 2016 at 4:06 PM, Valo, Kalle <kvalo@qca.qualcomm.com> wrote:
> Kalle Valo <kvalo@codeaurora.org> writes:
>
>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>
>>> There are two types of swapping the EEPROM data in the ath9k driver.
>>> Before this series one type of swapping could not be used without the
>>> other.
>>>
>>> The first type of swapping looks at the "magic bytes" at the start of
>>> the EEPROM data and performs swab16 on the EEPROM contents if needed.
>>> The second type of swapping is EEPROM format specific and swaps
>>> specific fields within the EEPROM itself (swab16, swab32 - depends on
>>> the EEPROM format).
>>>
>>> With this series the second part now looks at the EEPMISC register
>>> inside the EEPROM, which uses a bit to indicate if the EEPROM data
>>> is Big Endian (this is also done by the FreeBSD kernel).
>>> This has a nice advantage: currently there are some out-of-tree hacks
>>> (in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
>>> Big Endian system (= no swab16 is performed) but the EEPROM itself
>>> indicates that it's data is Little Endian. Until now the out-of-tree
>>> code simply did a swab16 before passing the data to ath9k, so ath9k
>>> first did the swab16 - this also enabled the format specific swapping.
>>> These out-of-tree hacks are still working with the new logic, but it
>>> is recommended to remove them. This implementation is based on a
>>> discussion with Arnd Bergmann who raised concerns about the
>>> robustness and portability of the swapping logic in the original OF
>>> support patch review, see [0].
>>>
>>> After a second round of patches (= v1 of this series) neither Arnd
>>> Bergmann nor I were really happy with the complexity of the EEPROM
>>> swapping logic. Based on a discussion (see [1] and [2]) we decided
>>> that ath9k should use a defined format (specifying the endianness
>>> of the data - I went with __le16 and __le32) when accessing the
>>> EEPROM fields. A benefit of this is that we enable the EEPMISC based
>>> swapping logic by default, just like the FreeBSD driver, see [3]. On
>>> the devices which I have tested (see below) ath9k now works without
>>> having to specify the "endian_check" field in ath9k_platform_data (or
>>> a similar logic which could provide this via devicetree) as ath9k now
>>> detects the endianness automatically. Only EEPROMs which are mangled
>>> by some out-of-tree code still need the endian_check flag (or one can
>>> simply remove that mangling from the out-of-tree code).
>>>
>>> Testing:
>>> - tested by myself on AR9287 with Big Endian EEPROM
>>> - tested by myself on AR9227 with Little Endian EEPROM
>>> - tested by myself on AR9381 (using the ar9003_eeprom implementation,
>>>   which did not suffer from this whole problem)
>>> - how do we proceed with testing? maybe we could keep this in a
>>>   feature-branch and add these patches to LEDE once we have an ACK to
>>>   get more people to test this
>>>
>>> This series depends on my other series (v7):
>>> "add devicetree support to ath9k" - see [4]
>>
>> I think this looks pretty good. If there's a bug somewhere it should be
>> quite easy to fix so I'm not that worried and would be willing to take
>> these as soon as I have applied the dependency series. IIRC your
>> devicetree patches will have at least one more review round so that will
>> take some time still. In the meantime it would be great if LEDE folks
>> could take a look at these and comment (or test).
>
> So are everyone happy with this? I haven't seen any comments. If I don't
> here anything I'm planning to take these, most likely for 4.11.
the patches have been in LEDE for almost two weeks now and I did not
see any reports of ath9k breakage (footnote below).

It seems that there are a few devices out there where the whole EEPROM
is swab16'ed which switches the position of the 1-byte fields
opCapFlags and eepMisc.
those still work fine with the new code, however I had a second patch
in LEDE [0] which results in ath9k_platform_data.endian_check NOT
being set anymore.
that endian_check flag was used before to swab16 the whole EEPROM, to
correct the position of the 1-byte fields again.
Currently we are fixing this in the firmware hotplug script: [1]
This is definitely not a blocker for this series though (if we want to
have a devicetree replacement for "ath9k_platform_data.endian_check"
then I'd work on that within a separate series, but I somewhat
consider these EEPROMs as "broken" so fixing them in
userspace/firmware hotplug script is fine for me)


Regards,
Martin


[0] https://git.lede-project.org/?p=source.git;a=commitdiff;h=a20616863d32d91163043b6657a63c836bd9c5ba
[1] https://git.lede-project.org/?p=source.git;a=commitdiff;h=afa37092663d00aa0abf8c61943d9a1b5558b144

[ath9k-devel] [PATCH v2 0/7] ath9k: EEPROM swapping improvements

[Valo, Kalle]

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:

> Hello Kalle,
>
> On Fri, Nov 25, 2016 at 4:06 PM, Valo, Kalle <kvalo@qca.qualcomm.com> wrote:
>> Kalle Valo <kvalo@codeaurora.org> writes:
>>
>>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:
>>>
>>>> There are two types of swapping the EEPROM data in the ath9k driver.
>>>> Before this series one type of swapping could not be used without the
>>>> other.
>>>>
>>>> The first type of swapping looks at the "magic bytes" at the start of
>>>> the EEPROM data and performs swab16 on the EEPROM contents if needed.
>>>> The second type of swapping is EEPROM format specific and swaps
>>>> specific fields within the EEPROM itself (swab16, swab32 - depends on
>>>> the EEPROM format).
>>>>
>>>> With this series the second part now looks at the EEPMISC register
>>>> inside the EEPROM, which uses a bit to indicate if the EEPROM data
>>>> is Big Endian (this is also done by the FreeBSD kernel).
>>>> This has a nice advantage: currently there are some out-of-tree hacks
>>>> (in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
>>>> Big Endian system (= no swab16 is performed) but the EEPROM itself
>>>> indicates that it's data is Little Endian. Until now the out-of-tree
>>>> code simply did a swab16 before passing the data to ath9k, so ath9k
>>>> first did the swab16 - this also enabled the format specific swapping.
>>>> These out-of-tree hacks are still working with the new logic, but it
>>>> is recommended to remove them. This implementation is based on a
>>>> discussion with Arnd Bergmann who raised concerns about the
>>>> robustness and portability of the swapping logic in the original OF
>>>> support patch review, see [0].
>>>>
>>>> After a second round of patches (= v1 of this series) neither Arnd
>>>> Bergmann nor I were really happy with the complexity of the EEPROM
>>>> swapping logic. Based on a discussion (see [1] and [2]) we decided
>>>> that ath9k should use a defined format (specifying the endianness
>>>> of the data - I went with __le16 and __le32) when accessing the
>>>> EEPROM fields. A benefit of this is that we enable the EEPMISC based
>>>> swapping logic by default, just like the FreeBSD driver, see [3]. On
>>>> the devices which I have tested (see below) ath9k now works without
>>>> having to specify the "endian_check" field in ath9k_platform_data (or
>>>> a similar logic which could provide this via devicetree) as ath9k now
>>>> detects the endianness automatically. Only EEPROMs which are mangled
>>>> by some out-of-tree code still need the endian_check flag (or one can
>>>> simply remove that mangling from the out-of-tree code).
>>>>
>>>> Testing:
>>>> - tested by myself on AR9287 with Big Endian EEPROM
>>>> - tested by myself on AR9227 with Little Endian EEPROM
>>>> - tested by myself on AR9381 (using the ar9003_eeprom implementation,
>>>>   which did not suffer from this whole problem)
>>>> - how do we proceed with testing? maybe we could keep this in a
>>>>   feature-branch and add these patches to LEDE once we have an ACK to
>>>>   get more people to test this
>>>>
>>>> This series depends on my other series (v7):
>>>> "add devicetree support to ath9k" - see [4]
>>>
>>> I think this looks pretty good. If there's a bug somewhere it should be
>>> quite easy to fix so I'm not that worried and would be willing to take
>>> these as soon as I have applied the dependency series. IIRC your
>>> devicetree patches will have at least one more review round so that will
>>> take some time still. In the meantime it would be great if LEDE folks
>>> could take a look at these and comment (or test).
>>
>> So are everyone happy with this? I haven't seen any comments. If I don't
>> here anything I'm planning to take these, most likely for 4.11.
>
> the patches have been in LEDE for almost two weeks now and I did not
> see any reports of ath9k breakage (footnote below).
>
> It seems that there are a few devices out there where the whole EEPROM
> is swab16'ed which switches the position of the 1-byte fields
> opCapFlags and eepMisc.
> those still work fine with the new code, however I had a second patch
> in LEDE [0] which results in ath9k_platform_data.endian_check NOT
> being set anymore.
> that endian_check flag was used before to swab16 the whole EEPROM, to
> correct the position of the 1-byte fields again.
> Currently we are fixing this in the firmware hotplug script: [1]
> This is definitely not a blocker for this series though (if we want to
> have a devicetree replacement for "ath9k_platform_data.endian_check"
> then I'd work on that within a separate series, but I somewhat
> consider these EEPROMs as "broken" so fixing them in
> userspace/firmware hotplug script is fine for me)

Sounds good to me, thanks for the thorough followup. I'm planning to
apply these any day.

-- 
Kalle Valo

[ath9k-devel] [PATCH v2 0/7] ath9k: EEPROM swapping improvements

[Adrian Chadd]

hi,

On 12 December 2016 at 12:05, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:

>
> It seems that there are a few devices out there where the whole EEPROM
> is swab16'ed which switches the position of the 1-byte fields
> opCapFlags and eepMisc.
> those still work fine with the new code, however I had a second patch
> in LEDE [0] which results in ath9k_platform_data.endian_check NOT
> being set anymore.
> that endian_check flag was used before to swab16 the whole EEPROM, to
> correct the position of the 1-byte fields again.
> Currently we are fixing this in the firmware hotplug script: [1]
> This is definitely not a blocker for this series though (if we want to
> have a devicetree replacement for "ath9k_platform_data.endian_check"
> then I'd work on that within a separate series, but I somewhat
> consider these EEPROMs as "broken" so fixing them in
> userspace/firmware hotplug script is fine for me)

As a reference - the reference driver has been doign this for a while.
It attempts to detect the endianness by looking at the 0xa55a
signature endian and figuring out which endian the actual contents are
in.

So just FYI yeah, this is a "thing" for reasons I don't quite know.



-adrian

>
>
> Regards,
> Martin
>
>
> [0] https://git.lede-project.org/?p=source.git;a=commitdiff;h=a20616863d32d91163043b6657a63c836bd9c5ba
> [1] https://git.lede-project.org/?p=source.git;a=commitdiff;h=afa37092663d00aa0abf8c61943d9a1b5558b144

[ath9k-devel] [PATCH v2 0/7] ath9k: EEPROM swapping improvements

[Valo, Kalle]

Martin Blumenstingl <martin.blumenstingl@googlemail.com> writes:

> There are two types of swapping the EEPROM data in the ath9k driver.
> Before this series one type of swapping could not be used without the
> other.
>
> The first type of swapping looks at the "magic bytes" at the start of
> the EEPROM data and performs swab16 on the EEPROM contents if needed.
> The second type of swapping is EEPROM format specific and swaps
> specific fields within the EEPROM itself (swab16, swab32 - depends on
> the EEPROM format).
>
> With this series the second part now looks at the EEPMISC register
> inside the EEPROM, which uses a bit to indicate if the EEPROM data
> is Big Endian (this is also done by the FreeBSD kernel).
> This has a nice advantage: currently there are some out-of-tree hacks
> (in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
> Big Endian system (= no swab16 is performed) but the EEPROM itself
> indicates that it's data is Little Endian. Until now the out-of-tree
> code simply did a swab16 before passing the data to ath9k, so ath9k
> first did the swab16 - this also enabled the format specific swapping.
> These out-of-tree hacks are still working with the new logic, but it
> is recommended to remove them. This implementation is based on a
> discussion with Arnd Bergmann who raised concerns about the
> robustness and portability of the swapping logic in the original OF
> support patch review, see [0].
>
> After a second round of patches (= v1 of this series) neither Arnd
> Bergmann nor I were really happy with the complexity of the EEPROM
> swapping logic. Based on a discussion (see [1] and [2]) we decided
> that ath9k should use a defined format (specifying the endianness
> of the data - I went with __le16 and __le32) when accessing the
> EEPROM fields. A benefit of this is that we enable the EEPMISC based
> swapping logic by default, just like the FreeBSD driver, see [3]. On
> the devices which I have tested (see below) ath9k now works without
> having to specify the "endian_check" field in ath9k_platform_data (or
> a similar logic which could provide this via devicetree) as ath9k now
> detects the endianness automatically. Only EEPROMs which are mangled
> by some out-of-tree code still need the endian_check flag (or one can
> simply remove that mangling from the out-of-tree code).
>
> Testing:
> - tested by myself on AR9287 with Big Endian EEPROM
> - tested by myself on AR9227 with Little Endian EEPROM
> - tested by myself on AR9381 (using the ar9003_eeprom implementation,
>   which did not suffer from this whole problem)
> - how do we proceed with testing? maybe we could keep this in a
>   feature-branch and add these patches to LEDE once we have an ACK to
>   get more people to test this
>
> This series depends on my other series (v7):
> "add devicetree support to ath9k" - see [4]
>
> Changes since v1:
> - reworked description in the cover-letter to describe the reasons
>   behind the new patch 7
> - reworked patch "Set the "big endian" bit of the AR9003 EEPROM
>   templates" as ar9003_eeprom.c sets all values as Little Endian, thus
>   the Big Endian bit should never be set (the new patch makes this
>   clear)
> - dropped "ath9k: Make EEPROM endianness swapping configurable via
>   devicetree" as it is not needed anymore with the new logic from
>   patch 7
> - added patches 4 and 5 as small cleanup (this made it easier to
>   implement the le{16,32}_to_cpu() changes where needed)
>
>
> [0] http://www.spinics.net/lists/linux-wireless/msg152634.html
> [1] https://marc.info/?l=linux-wireless&m=147250597503174&w=2
> [2] https://marc.info/?l=linux-wireless&m=147254388611344&w=2
> [3] https://github.com/freebsd/freebsd/blob/50719b56d9ce8d7d4beb53b16e9edb2e9a4a7a18/sys/dev/ath/ath_hal/ah_eeprom_9287.c#L351
> [4] https://marc.info/?l=linux-wireless&m=147544488619822&w=2
>
> Martin Blumenstingl (7):
>   ath9k: Add a #define for the EEPROM "eepmisc" endianness bit
>   ath9k: indicate that the AR9003 EEPROM template values are little
>     endian
>   ath9k: Add an eeprom_ops callback for retrieving the eepmisc value
>   ath9k: replace eeprom_param EEP_MINOR_REV with get_eeprom_rev
>   ath9k: consistently use get_eeprom_rev(ah)
>   ath9k: Make the EEPROM swapping check use the eepmisc register
>   ath9k: define all EEPROM fields in Little Endian format

Applied to ath-next on ath.git, thanks.

(My automatic "accepted" email failed, so had to send this manually.)

-- 
Kalle Valo

[ath9k-devel] [PATCH v2 0/7] ath9k: EEPROM swapping improvements

[Martin Blumenstingl]

Hi Adrian,

On Wed, Dec 14, 2016 at 7:45 AM, Adrian Chadd <adrian@freebsd.org> wrote:
> hi,
>
> On 12 December 2016 at 12:05, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>
>>
>> It seems that there are a few devices out there where the whole EEPROM
>> is swab16'ed which switches the position of the 1-byte fields
>> opCapFlags and eepMisc.
>> those still work fine with the new code, however I had a second patch
>> in LEDE [0] which results in ath9k_platform_data.endian_check NOT
>> being set anymore.
>> that endian_check flag was used before to swab16 the whole EEPROM, to
>> correct the position of the 1-byte fields again.
>> Currently we are fixing this in the firmware hotplug script: [1]
>> This is definitely not a blocker for this series though (if we want to
>> have a devicetree replacement for "ath9k_platform_data.endian_check"
>> then I'd work on that within a separate series, but I somewhat
>> consider these EEPROMs as "broken" so fixing them in
>> userspace/firmware hotplug script is fine for me)
>
> As a reference - the reference driver has been doign this for a while.
> It attempts to detect the endianness by looking at the 0xa55a
> signature endian and figuring out which endian the actual contents are
> in.
>
> So just FYI yeah, this is a "thing" for reasons I don't quite know.
on all devices I have seen so far (all customer devices, no
development boards) these two magic bytes *can* be used to detect the
endianness of the data that is written to the PCI memory (and thus
whether swapping of that data is required or not).
however, there are many devices (roughly 50% of the ones I've seen)
where the magic bytes cannot be used to swap the actual EEPROM (=
calibration) data because they are "inverted". reading the eepMisc
byte works fine on all devices I've seen so far *if* the manufacturer
did not swab16 all data (PCI memory and EEPROM data).

on the other hand you are right: all four devices which were broken
had the correct magic bytes at the start, but as long as this is not
the case for all devices we cannot use it without some feature-flag.

as an (unrelated) side-note: I've also some EEPROMs where the length
matches neither the "magic bytes endianness" nor the "eepMisc
endianness". I consider these broken as well, but fortunately ath9k
has a fallback for this issue.


Regards,
Martin