Re: [PATCH 7/8] ath5k: disable 32KHz sleep clock operation

[Felix Fietkau <nbd@openwrt.org>]

On 2011-07-04 4:58 PM, Nick Kossifidis wrote:
> 2011/7/4 Felix Fietkau<nbd@openwrt.org>:
>>  While 32 KHz sleep clock might provide some power saving benefits,
>>  it is also a major source of stability issues, on OpenWrt it produced
>>  some reproducible data bus errors on register accesses on several
>>  different MIPS platforms.
>>
>>  All the Atheros drivers that I can find do not enable this feature,
>>  so it makes sense to leave it disabled in ath5k as well.
>>
>>  Signed-off-by: Felix Fietkau<nbd@openwrt.org>
>>  ---
>>    drivers/net/wireless/ath/ath5k/reset.c |    9 ---------
>>    1 files changed, 0 insertions(+), 9 deletions(-)
>>
>>  diff --git a/drivers/net/wireless/ath/ath5k/reset.c b/drivers/net/wireless/ath/ath5k/reset.c
>>  index 55276ce..192c0cb 100644
>>  --- a/drivers/net/wireless/ath/ath5k/reset.c
>>  +++ b/drivers/net/wireless/ath/ath5k/reset.c
>>  @@ -1285,15 +1285,6 @@ int ath5k_hw_reset(struct ath5k_hw *ah, enum nl80211_iftype op_mode,
>>           */
>>          ath5k_hw_dma_init(ah);
>>
>>  -
>>  -       /* Enable 32KHz clock function for AR5212+ chips
>>  -        * Set clocks to 32KHz operation and use an
>>  -        * external 32KHz crystal when sleeping if one
>>  -        * exists */
>>  -       if (ah->ah_version == AR5K_AR5212&&
>>  -           op_mode != NL80211_IFTYPE_AP)
>>  -               ath5k_hw_set_sleep_clock(ah, true);
>>  -
>>          /*
>>           * Disable beacons and reset the TSF
>>           */
>>  --
>>  1.7.3.2
>>
>
> a) LegacyHAL and Sam's HAL both enable it
At least in the Legacy HAL (and in all other HALs that I looked a) I 
found this in the attach function:
     ahp->ah_enable32kHzClock = DONT_USE_32KHZ;/* XXX */

> b) Not many cards have a 32KHz crystal anyway (disabled on EEPROM)
> c) Please don't just remove code, if you want to disable it you can
> always comment it out
OK.

> d) Why not make it a module parameter instead ?
I'm not sure 32 KHz has been tested properly and found to be stable 
anywhere, so I'm not convinced it will be useful to anybody. Also, I 
think a debugfs parameter might be better because then it can be 
enabled/disabled per card.

- Felix