2008-10-26 20:47:46

by Elias Oltmanns

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Fix reset sequence for AR5212 in general and RF5111 in particular

"Nick Kossifidis" <[email protected]> wrote:
> 2008/10/26 Elias Oltmanns <[email protected]>:
>>
>
>> 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.
>>
>
> 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 ?

Is this what you had in mind? It works for me.

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.

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

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

diff --git a/drivers/net/wireless/ath5k/hw.c b/drivers/net/wireless/ath5k/hw.c
index ad1a5b4..68cf2ae 100644
--- a/drivers/net/wireless/ath5k/hw.c
+++ b/drivers/net/wireless/ath5k/hw.c
@@ -828,7 +828,8 @@ int ath5k_hw_reset(struct ath5k_hw *ah, enum ieee80211_if_types op_mode,
/*
* Write some more initial register settings
*/
- if (ah->ah_version == AR5K_AR5212) {
+ if (ah->ah_version == AR5K_AR5212 &&
+ ah->ah_radio > AR5K_RF5111) {
ath5k_hw_reg_write(ah, 0x0002a002, 0x982c);

if (channel->hw_value == CHANNEL_G)
@@ -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;

diff --git a/drivers/net/wireless/ath5k/initvals.c b/drivers/net/wireless/ath5k/initvals.c
index 2806b21..cf7ebd1 100644
--- a/drivers/net/wireless/ath5k/initvals.c
+++ b/drivers/net/wireless/ath5k/initvals.c
@@ -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 } },
};


2008-10-27 20:38:40

by Nick Kossifidis

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Fix reset sequence for AR5212 in general and RF5111 in particular

2008/10/27 Elias Oltmanns <[email protected]>:
> "Nick Kossifidis" <[email protected]> wrote:
>> 2008/10/26 Elias Oltmanns <[email protected]>:
>>>
>>
>>> /*
>>> * Write some more initial register settings
>>> */
>>> - 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
>
> 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

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

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

--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

2008-10-26 22:33:42

by Nick Kossifidis

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Fix reset sequence for AR5212 in general and RF5111 in particular

2008/10/26 Elias Oltmanns <[email protected]>:
>
> /*
> * Write some more initial register settings
> */
> - 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

> ath5k_hw_reg_write(ah, 0x0002a002, 0x982c);
>
> if (channel->hw_value == CHANNEL_G)
> @@ -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 ;-)

--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick

2008-10-26 22:53:20

by Elias Oltmanns

[permalink] [raw]
Subject: Re: [PATCH] ath5k: Fix reset sequence for AR5212 in general and RF5111 in particular

"Nick Kossifidis" <[email protected]> wrote:
> 2008/10/26 Elias Oltmanns <[email protected]>:
>>
>
>> /*
>> * Write some more initial register settings
>> */
>> - 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

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

>
>> ath5k_hw_reg_write(ah, 0x0002a002, 0x982c);
>>
>> if (channel->hw_value == CHANNEL_G)
>> @@ -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 ;-)

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