Re: [PATCH] ath9k_hw: Fix instable target power control b/w CCK/OFDM

[Felix Fietkau <nbd@openwrt.org>]

On 2011-04-05 5:10 PM, Rajkumar Manoharan wrote:
> On Tue, Apr 05, 2011 at 07:10:06PM +0530, Felix Fietkau wrote:
>>  On 2011-04-05 3:05 PM, Rajkumar Manoharan wrote:
>>  >  The problem is that when the attenuation is increased,
>>  >  the rate will start to drop from MCS7 ->   MCS6, and finally
>>  >  will see MCS1 ->   CCK_11Mbps. When the rate is changed b/w
>>  >  CCK and OFDM, it will use register desired_scale to calculate
>>  >  how much tx gain need to change.
>>  >
>>  >  The output power with the same tx gain for CCK and OFDM modulated
>>  >  signals are different. This difference is constant for AR9280
>>  >  but not AR9285/AR9271. It has different PA architecture
>>  >  a constant. So it should be calibrated against this PA
>>  >  characteristic.
>>  >
>>  >  The driver has to read the calibrated values from EEPROM and set
>>  >  the tx power registers accordingly.
>>  >
>>  >  Signed-off-by: Rajkumar Manoharan<rmanoharan@atheros.com>
>>  >  ---
>>  >  diff --git a/drivers/net/wireless/ath/ath9k/eeprom_4k.c b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
>>  >  index bc77a30..8f0cd0e 100644
>>  >  --- a/drivers/net/wireless/ath/ath9k/eeprom_4k.c
>>  >  +++ b/drivers/net/wireless/ath/ath9k/eeprom_4k.c
>>  >  @@ -781,6 +781,7 @@ static void ath9k_hw_4k_set_board_values(struct ath_hw *ah,
>>  >    {
>>  >    	struct modal_eep_4k_header *pModal;
>>  >    	struct ar5416_eeprom_4k *eep =&ah->eeprom.map4k;
>>  >  +	struct base_eep_header_4k *pBase =&eep->baseEepHeader;
>>  >    	u8 txRxAttenLocal;
>>  >    	u8 ob[5], db1[5], db2[5];
>>  >    	u8 ant_div_control1, ant_div_control2;
>>  >  @@ -1003,6 +1004,56 @@ static void ath9k_hw_4k_set_board_values(struct ath_hw *ah,
>>  >    				AR_PHY_SETTLING_SWITCH,
>>  >    				pModal->swSettleHt40);
>>  >    	}
>>  >  +	if (AR_SREV_9271(ah) || AR_SREV_9285(ah)) {
>>  >  +		u8 bb_desired_scale = (pModal->bb_scale_smrt_antenna&
>>  >  +				EEP_4K_BB_DESIRED_SCALE_MASK);
>>  >  +		if ((pBase->txGainType == 0)&&   (bb_desired_scale != 0)) {
>>  >  +			u32 pwrctrl, pwrctrl_n;
>>  >  +
>>  >  +			/* AR_PHY_TX_PWRCTRL8 */
>>  >  +			pwrctrl_n = REG_READ(ah, AR_PHY_TX_PWRCTRL8);
>>  >  +			pwrctrl = (u32) bb_desired_scale;
>>  >  +			NSO(pwrctrl, 5, bb_desired_scale, 5);
>>  I think the NSO macro is somewhat confusing. How about something like
>>  this instead, to make it more readable:
>>
>>  u32 mask;
>>  mask = BIT(0) | BIT(5) | BIT(10) | BIT(15) | BIT(20);
>>  pwrctl = mask * bb_desired_scale;
>>
> NSO - N times Shift and OR. Sounds reasonable. isn't it?
The name may be reasonable, but it's not really obvious, and IMHO the 
macro is not really necessary either.
I'm not sure if the compiler can always optimize away the for loop - and 
even if it can, I still think think the bitmask + multiplication 
approach is somewhat more readable and does not require introducing yet 
another magic macro.

You could also make the code more efficient and smaller by using REG_RMW 
instead of masking pwrctl_n vs pwrctl manually.

- Felix