ath5k: Fix reset sequence for AR5212 in general and RF5111 in particular

Take care to handle register 0xa228 exactly as in the HAL released by Atheros. This change is required to make ath5k work again on my system since commit 2203d6be (ath5k: Misc hw_reset updates), thus fixing a regression in 2.6.27 and therefore hopefully eligible for inclusion into a stable release.

Cc stable <[email protected]>

Signed-off-by Elias Oltmanns <[email protected]>

---
Applies to 2.6.27. Please let me know if I should do something differently and resend.

 drivers/net/wireless/ath5k/hw.c       |    1 +
 drivers/net/wireless/ath5k/initvals.c |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

2008/10/23 Elias Oltmanns <[email protected]>:

Quoted 9 lines

Take care to handle register 0xa228 exactly as in the HAL released by

Atheros. This change is required to make ath5k work again on my system since commit 2203d6be (ath5k: Misc hw_reset updates), thus fixing a regression in 2.6.27 and therefore hopefully eligible for inclusion into a stable release.

Cc stable <[email protected]>

Signed-off-by Elias Oltmanns <[email protected]>

Quoted 8 lines

---
Applies to 2.6.27. Please let me know if I should do something differently and resend.

 drivers/net/wireless/ath5k/hw.c       |    1 +
 drivers/net/wireless/ath5k/initvals.c |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

Current code works fine on my CM6 (AR5212 + RF5111) and i see nothing like

data |= 0x00000800

on the HAL Atheros released

Problem is that we are updating 0xa228 (RSSI threshold in this case) for all AR5212 based cards and we are not checking right now for BB revision. These settings according to Atheros HAL are only for BB revisions > 2. I'm going to update reset.c this week hopefully.

BTW can you please run latest ath_info on your card ? Let's see what's the rssi threshold stored on EEPROM and how much different is from my card.

Let me quote from legacy-hal:

ar5212/ar5212_reset.c:
373: OS_REG_RMW_FIELD(ah, AR_PHY_DAG_CTRLCCK,
374: AR_PHY_DAG_CTRLCCK_RSSI_THR, 2);

ah_internal.h:
598: #define OS_REG_RMW_FIELD(_a, _r, _f, _v) \
599: OS_REG_WRITE(_a, _r, \
600: (OS_REG_READ(_a, _r) &~ _f) | (((_v) << _f##_S) & _f))

Please note the last argument to OS_REG_RMW_FIELD() which is 2 in this case. Unless I've made a mistake, this translates into the

data |= 0x00000800;

in my patch.

There definitely is no way for me to connect to an ap (or even get a list of aps) with current 2.6.27.4. Perhaps we'll hear something from Nils on that matter too.

2008/10/26 Elias Oltmanns <[email protected]>:

Quoted 16 lines

Let me quote from legacy-hal:

ar5212/ar5212_reset.c:
373: OS_REG_RMW_FIELD(ah, AR_PHY_DAG_CTRLCCK,
374: AR_PHY_DAG_CTRLCCK_RSSI_THR, 2);

ah_internal.h:
598: #define OS_REG_RMW_FIELD(_a, _r, _f, _v) \
599: OS_REG_WRITE(_a, _r, \
600: (OS_REG_READ(_a, _r) &~ _f) | (((_v) << _f##_S) & _f))

Please note the last argument to OS_REG_RMW_FIELD() which is 2 in this case. Unless I've made a mistake, this translates into the

Quoted 8 lines

data |= 0x00000800;

in my patch.

There definitely is no way for me to connect to an ap (or even get a list of aps) with current 2.6.27.4. Perhaps we'll hear something from Nils on that matter too.

Some lines above...

	/* Overwrite INI values for revised chipsets */
	if (AH_PRIVATE(ah)->ah_phyRev >= AR_PHY_CHIP_ID_REV_2) {

so

		/* Add barker RSSI thresh enable as disabled */
		OS_REG_CLR_BIT(ah, AR_PHY_DAG_CTRLCCK,
			AR_PHY_DAG_CTRLCCK_EN_RSSI_THR);
		OS_REG_RMW_FIELD(ah, AR_PHY_DAG_CTRLCCK,
			AR_PHY_DAG_CTRLCCK_RSSI_THR, 2);

is for phy (BB) revisions > 2 since you have RF5111 this code should not run but (and here is our bug) instead of checking bb revision we run this code for all AR5212 chips

		/*
		 * Write some more initial register settings
		 */
		if (ah->ah_version == AR5K_AR5212) {

So can you please change the check on ath5k_hw_reset + add initial rssi threshold setting and retry ?

2008/10/26 Elias Oltmanns <[email protected]>:

drivers/net/wireless/ath5k/hw.c int ath5k_hw_reset(struct ath5k_hw *ah, enum ieee80211_if_types op_mode, · line 832

Quoted 8 lines

               /*
                * Write some more initial register settings
                */

drivers/net/wireless/ath5k/hw.c +2 -1

-               if (ah->ah_version == AR5K_AR5212) {
+               if (ah->ah_version == AR5K_AR5212 &&
+                   ah->ah_radio > AR5K_RF5111) {

even better: ah->ah_phy_revision > AR5K_SREV_PHY_5212

drivers/net/wireless/ath5k/hw.c int ath5k_hw_reset(struct ath5k_hw *ah, enum ieee80211_if_types op_mode, · line 865

Quoted 12 lines

                       ath5k_hw_reg_write(ah, 0x0002a002, 0x982c);

                       if (channel->hw_value == CHANNEL_G)

drivers/net/wireless/ath5k/hw.c +1 -0

@@ -858,6 +859,7 @@ int ath5k_hw_reset(struct ath5k_hw *ah, enum ieee80211_if_types op_mode,
 
                       data = ath5k_hw_reg_read(ah, 0xa228);
                       data &= 0xfffe03ff;
+                       data |= 0x00000800;
                       ath5k_hw_reg_write(ah, data, 0xa228);
                       data = 0;
 

Can you please use the macros from ath5k.h to set these parameters ? (i got the above from decompiling binary HAL that's why i have fixed masks -no magic values- but now that we know what's going on we must make the code human-readable)
And also please change 0xa228 with it's name from reg.h :

2557 #define AR5K_PHY_DAG_CCK_CTL                    0xa228
2558 #define AR5K_PHY_DAG_CCK_CTL_EN_RSSI_THR        0x00000200
2559 #define AR5K_PHY_DAG_CCK_CTL_RSSI_THR           0x0001fc00
2560 #define AR5K_PHY_DAG_CCK_CTL_RSSI_THR_S         10

Normaly i'll update reset.c this week so you can always wait for a more generic fix ;-)

[Bcc'ing Nils who has reported similar issues with ath5k on 2.6.27. Nils, can you perhaps test my patch below and report back? ]

Quoted 18 lines · Nick Kossifidis

2008/10/23 Elias Oltmanns <[email protected]>:

Quoted 9 lines

Take care to handle register 0xa228 exactly as in the HAL released by

Atheros. This change is required to make ath5k work again on my system since commit 2203d6be (ath5k: Misc hw_reset updates), thus fixing a regression in 2.6.27 and therefore hopefully eligible for inclusion into a stable release.

Cc stable <[email protected]>

Signed-off-by Elias Oltmanns <[email protected]>

Quoted 7 lines

---
Applies to 2.6.27. Please let me know if I should do something differently and resend.

 drivers/net/wireless/ath5k/hw.c       |    1 +
 drivers/net/wireless/ath5k/initvals.c |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

Quoted 6 lines

Current code works fine on my CM6 (AR5212 + RF5111) and i see nothing like

data |= 0x00000800

on the HAL Atheros released

Let me quote from legacy-hal:

ar5212/ar5212_reset.c:
373: OS_REG_RMW_FIELD(ah, AR_PHY_DAG_CTRLCCK,
374: AR_PHY_DAG_CTRLCCK_RSSI_THR, 2);

ah_internal.h:
598: #define OS_REG_RMW_FIELD(_a, _r, _f, _v) \
599: OS_REG_WRITE(_a, _r, \
600: (OS_REG_READ(_a, _r) &~ _f) | (((_v) << _f##_S) & _f))

Please note the last argument to OS_REG_RMW_FIELD() which is 2 in this case. Unless I've made a mistake, this translates into the

data |= 0x00000800;

in my patch.

There definitely is no way for me to connect to an ap (or even get a list of aps) with current 2.6.27.4. Perhaps we'll hear something from Nils on that matter too.

Quoted 8 lines

Problem is that we are updating 0xa228 (RSSI threshold in this case) for all AR5212 based cards and we are not checking right now for BB revision. These settings according to Atheros HAL are only for BB revisions > 2. I'm going to update reset.c this week hopefully.

BTW can you please run latest ath_info on your card ? Let's see what's the rssi threshold stored on EEPROM and how much different is from my card.

Well, I don't see how to get that information from the dump. Please have a look for yourself, I'm going to send you my dump by pm.

Since my original email has not appeared on linux-wireless (due to a mistake of mine), I'm attaching the patch again, so others can have a look at it and, more importantly, Nils can test it.

Regards,

Elias

--------
From: Elias Oltmanns <[email protected]>
Subject: [PATCH] ath5k: Fix reset sequence for AR5212 in general and RF5111 in particular

Take care to handle register 0xa228 exactly as in the HAL released by Atheros. This change is required to make ath5k work again on my system since commit 2203d6be (ath5k: Misc hw_reset updates), thus fixing a regression in 2.6.27 and therefore hopefully eligible for inclusion into a stable release.

Cc stable <[email protected]>

Signed-off-by Elias Oltmanns <[email protected]>

---

 drivers/net/wireless/ath5k/hw.c       |    1 +
 drivers/net/wireless/ath5k/initvals.c |    2 ++
 2 files changed, 3 insertions(+), 0 deletions(-)

(quoted diff) +0 -0

b/drivers/net/wireless/ath5k/hw.c index ad1a5b4..f1a7f71 100644

drivers/net/wireless/ath5k/hw.c +1 -0

@@ -858,6 +858,7 @@ int ath5k_hw_reset(struct ath5k_hw *ah, enum
 ieee80211_if_types op_mode,
 
 			data = ath5k_hw_reg_read(ah, 0xa228);
 			data &= 0xfffe03ff;
+			data |= 0x00000800;
 			ath5k_hw_reg_write(ah, data, 0xa228);
 			data = 0;

(quoted diff) +0 -0

b/drivers/net/wireless/ath5k/initvals.c index 2806b21..cf7ebd1 100644

drivers/net/wireless/ath5k/initvals.c +2 -0

@@ -810,6 +810,8 @@ static const struct ath5k_ini_mode
 ar5212_rf5111_ini_mode_end[] = { { 0x00000000, 0x00000000, 0x00000000,
 0x00000000, 0x00000000 } }, { AR5K_PHY(642),
 		{ 0xd03e6788, 0xd03e6788, 0xd03e6788, 0xd03e6788, 0xd03e6788 } },
+	{ 0xa228,
+		{ 0x000001b5, 0x000001b5, 0x000001b5, 0x000001b5, 0x000001b5 } },
 	{ 0xa23c,
 		{ 0x13c889af, 0x13c889af, 0x13c889af, 0x13c889af, 0x13c889af } },
 };

Quoted 10 lines · Nick Kossifidis

2008/10/26 Elias Oltmanns <[email protected]>:

Quoted 8 lines

               /*
                * Write some more initial register settings
                */

drivers/net/wireless/ath5k/hw.c +2 -1

-               if (ah->ah_version == AR5K_AR5212) {
+               if (ah->ah_version == AR5K_AR5212 &&
+                   ah->ah_radio > AR5K_RF5111) {

Quoted 1 line

even better: ah->ah_phy_revision > AR5K_SREV_PHY_5212

Just to be absolutely sure: ah->ah_phy_revision >= AR5K_SREV_PHY_5212 right?

Well, I'm always happy to leave the work to others ;-). The only reason why I kept resending patches was to make sure that a minimal fix would get merged into a stable release eventually. Presumably, a rework of the reset sequence in itself would not be appropriate for stable. Still, if you'll take care get something into stable, that's fine with me. Otherwise, just let me know and I'll keep those patches coming and you occupied reviewing them ;-).

Regards,

Elias

2008/10/27 Elias Oltmanns <[email protected]>:

Quoted 14 lines

Quoted 11 lines · Nick Kossifidis

2008/10/26 Elias Oltmanns <[email protected]>:

Review

Quoted 8 lines

               /*
                * Write some more initial register settings
                */

(quoted diff) +2 -1

-               if (ah->ah_version == AR5K_AR5212) {
+               if (ah->ah_version == AR5K_AR5212 &&
+                   ah->ah_radio > AR5K_RF5111) {

even better: ah->ah_phy_revision > AR5K_SREV_PHY_5212

Quoted 3 lines

Just to be absolutely sure: ah->ah_phy_revision >= AR5K_SREV_PHY_5212 right?

AR5K_SREV_PHY_5212 is bb rev 1 so its

ah->ah_phy > AR5K_SREV_PHY_5212

Quoted 10 lines

Well, I'm always happy to leave the work to others ;-). The only reason why I kept resending patches was to make sure that a minimal fix would get merged into a stable release eventually. Presumably, a rework of the reset sequence in itself would not be appropriate for stable. Still, if you'll take care get something into stable, that's fine with me. Otherwise, just let me know and I'll keep those patches coming and you occupied reviewing them ;-).

Quoted 4 lines

Regards,

Elias

O.K. then we 'll include your patch on stable and my work will go on wireless-dev for further testing ;-)

Right, so here is another attempt. Unfortunately, in 2.6.27 no constants have been defined for the phy revisions, nor the 0xa228 register and the bit masks involved in resetting this register. Perhaps we should consult the stable team on this matter because the minimal approach, as presented below, certainly lacks even the smallest degree of readability even though we actually know reasonably well what we are doing here. The changelog, by the way, will be adapted to quote your changeset fixing this in mainline once it has been committed. What do you think?

2008/10/29 Elias Oltmanns <[email protected]>:

Quoted 11 lines

Right, so here is another attempt. Unfortunately, in 2.6.27 no constants have been defined for the phy revisions, nor the 0xa228 register and the bit masks involved in resetting this register. Perhaps we should consult the stable team on this matter because the minimal approach, as presented below, certainly lacks even the smallest degree of readability even though we actually know reasonably well what we are doing here. The changelog, by the way, will be adapted to quote your changeset fixing this in mainline once it has been committed. What do you think?

Patch looks OK, thanks a lot ;-)

John can we get this get in stable until i update reset.c on wireless-testing ? Would it be possible later to update mainline from wireless-testing ?

Quoted 5 lines

Patch looks OK, thanks a lot ;-)

John can we get this get in stable until i update reset.c on wireless-testing ? Would it be possible later to update mainline from wireless-testing ?

Reading Documentation/stable_kernel_rules.txt, I very much doubt that this patch will go in unless we can at least point out something equivalent in mainline. So, we'll have to wait until your changes to reset.c have reached mainline, I'm afraid.

Quoted 6 lines

Quoted 5 lines

Patch looks OK, thanks a lot ;-)

John can we get this get in stable until i update reset.c on wireless-testing ? Would it be possible later to update mainline from wireless-testing ?

Quoted 4 lines

Reading Documentation/stable_kernel_rules.txt, I very much doubt that this patch will go in unless we can at least point out something equivalent in mainline. So, we'll have to wait until your changes to reset.c have reached mainline, I'm afraid.

Or, we could just send your patch to both stable and for 2.6.28. Then Nick can fix up the difference or revert it in his rework. The rework is 2.6.29 material, right?

2008/10/29 Bob Copeland <[email protected]>:

Quoted 13 lines · Elias Oltmanns

Quoted 11 lines

Quoted 6 lines

Patch looks OK, thanks a lot ;-)

John can we get this get in stable until i update reset.c on wireless-testing ? Would it be possible later to update mainline from wireless-testing ?

Reading Documentation/stable_kernel_rules.txt, I very much doubt that this patch will go in unless we can at least point out something equivalent in mainline. So, we'll have to wait until your changes to reset.c have reached mainline, I'm afraid.

Quoted 4 lines

Or, we could just send your patch to both stable and for 2.6.28. Then Nick can fix up the difference or revert it in his rework. The rework is 2.6.29 material, right?

ACK

Quoted 14 lines · Nick Kossifidis

2008/10/29 Bob Copeland <[email protected]>:

Quoted 13 lines · Elias Oltmanns

Quoted 11 lines

Quoted 6 lines

Patch looks OK, thanks a lot ;-)

John can we get this get in stable until i update reset.c on wireless-testing ? Would it be possible later to update mainline from wireless-testing ?

Reading Documentation/stable_kernel_rules.txt, I very much doubt that this patch will go in unless we can at least point out something equivalent in mainline. So, we'll have to wait until your changes to reset.c have reached mainline, I'm afraid.

Quoted 6 lines

Quoted 4 lines

Or, we could just send your patch to both stable and for 2.6.28. Then Nick can fix up the difference or revert it in his rework. The rework is 2.6.29 material, right?

ACK

In that case, I'd suggest the following patch for 2.6.28-rc2. Once it has been merged, I'll send the previous one off to the stable team.

Regards,

Elias
--------
From: Elias Oltmanns <[email protected]>
Subject: ath5k: Fix reset sequence for AR5212 in general and RF5111 in particular

Take care to handle register 0xa228 exactly as in the HAL released by Atheros. This change is required to make ath5k work again on my system since commit 2203d6be (ath5k: Misc hw_reset updates), thus fixing a regression in 2.6.27 and therefore hopefully eligible for inclusion into a stable release.

v2: Only overwrite initial register values on later revisions of AR5212
chips.
v3: Use standard macros to manipulate the register.
v4: Use appropriate constants (new to 2.6.28) for register access.

Signed-off-by Elias Oltmanns <[email protected]>

---

 drivers/net/wireless/ath5k/initvals.c |    2 ++
 drivers/net/wireless/ath5k/reset.c    |   25 ++++++++++---------------
 2 files changed, 12 insertions(+), 15 deletions(-)

drivers/net/wireless/ath5k/initvals.c +2 -0

index ea2e1a2..ceaa6c4 100644
@@ -806,6 +806,8 @@ static const struct ath5k_ini_mode ar5212_rf5111_ini_mode_end[] = {
 		{ 0x00000000, 0x00000000, 0x00000000, 0x00000000, 0x00000000 } },
 	{ AR5K_PHY(642),
 		{ 0xd03e6788, 0xd03e6788, 0xd03e6788, 0xd03e6788, 0xd03e6788 } },
+	{ 0xa228,
+		{ 0x000001b5, 0x000001b5, 0x000001b5, 0x000001b5, 0x000001b5 } },
 	{ 0xa23c,
 		{ 0x13c889af, 0x13c889af, 0x13c889af, 0x13c889af, 0x13c889af } },
 };

drivers/net/wireless/ath5k/reset.c +10 -15

index 8f18868..8fdb092 100644
@@ -537,9 +537,10 @@ int ath5k_hw_reset(struct ath5k_hw *ah, enum nl80211_iftype op_mode,
 		mdelay(1);
 
 		/*
-		 * Write some more initial register settings
+		 * Write some more initial register settings for revised chips
 		 */
-		if (ah->ah_version == AR5K_AR5212) {
+		if (ah->ah_version == AR5K_AR5212 &&
+		    ah->ah_phy_revision > AR5K_SREV_PHY_5212) {
 			ath5k_hw_reg_write(ah, 0x0002a002, 0x982c);
 
 			if (channel->hw_value == CHANNEL_G)

@@ -558,19 +559,13 @@ int ath5k_hw_reset(struct ath5k_hw *ah, enum nl80211_iftype op_mode,
 			else
 				ath5k_hw_reg_write(ah, 0x00000000, 0x994c);
 
-			/* Some bits are disabled here, we know nothing about
-			 * register 0xa228 yet, most of the times this ends up
-			 * with a value 0x9b5 -haven't seen any dump with
-			 * a different value- */
-			/* Got this from decompiling binary HAL */
-			data = ath5k_hw_reg_read(ah, 0xa228);
-			data &= 0xfffffdff;
-			ath5k_hw_reg_write(ah, data, 0xa228);
-
-			data = ath5k_hw_reg_read(ah, 0xa228);
-			data &= 0xfffe03ff;
-			ath5k_hw_reg_write(ah, data, 0xa228);
-			data = 0;
+			/* Got this from legacy-hal */
+			AR5K_REG_DISABLE_BITS(ah, AR5K_PHY_DAG_CCK_CTL,
+					      AR5K_PHY_DAG_CCK_CTL_EN_RSSI_THR);
+
+			AR5K_REG_MASKED_BITS(ah, AR5K_PHY_DAG_CCK_CTL,
+					     2 << AR5K_PHY_DAG_CCK_CTL_RSSI_THR_S,
+					     ~AR5K_PHY_DAG_CCK_CTL_RSSI_THR);
 
 			/* Just write 0x9b5 ? */
 			/* ath5k_hw_reg_write(ah, 0x000009b5, 0xa228); */

Well, I already merged v3...what is new here?

Well, I already merged v3...what is new here?