Re: [PATCH 5/5] ath5k: Update reset code

[Bob Copeland <me@bobcopeland.com>]

On Sat, Jan 31, 2009 at 04:31:47AM +0200, Nick Kossifidis wrote:
>  * Update reset and sync with HALs
>  * Clean up eeprom settings and tweaking of initvals and put them on separate functions
>  * Set/Restore 32KHz ref clk operation
>  * Add some more documentation
>  TODO: Spur mitigation, tpc, half/quarter rate, compression etc

Awesome work!  I just double checked it, I have some minor comments,
on the whole looks very good.

>  Signed-Off-by: Nick Kossifidis <mickflemm@gmail.com>

Should be "Signed-off-by:" (lowercase O) according to checkpatch.

> [...]
> +bool ath5k_eeprom_is_hb63(struct ath5k_hw *ah)
> +{
> +	u16 data;
> +
> +	ath5k_hw_eeprom_read(ah, AR5K_EEPROM_IS_HB63, &data);
> +
> +	if ((ah->ah_mac_version == (AR5K_SREV_AR2425 >> 4) && data))

Don't think it makes a difference, but judging from the double parens,
I think you meant:

> +	if ((ah->ah_mac_version == (AR5K_SREV_AR2425 >> 4)) && data)

Do we need to check for eeprom version 5.4 or better?  HAL does. 

> [...]
> @@ -2156,7 +2157,8 @@
>  #define	AR5K_PHY_ANT_CTL_TXRX_EN	0x00000001	/* Enable TX/RX (?) */
>  #define	AR5K_PHY_ANT_CTL_SECTORED_ANT	0x00000004	/* Sectored Antenna */
>  #define	AR5K_PHY_ANT_CTL_HITUNE5	0x00000008	/* Hitune5 (?) */
> -#define	AR5K_PHY_ANT_CTL_SWTABLE_IDLE	0x00000010	/* Switch table idle (?) */
> +#define	AR5K_PHY_ANT_CTL_SWTABLE_IDLE	0x000003f9	/* Switch table idle (?) */
> +#define	AR5K_PHY_ANT_CTL_SWTABLE_IDLE_S	4

That doesn't look right.. should be 3f0?  (still probably works, I guess).

> -#define	AR5K_PHY_OFDM_SELFCORR_CYPWR_THR1_S	0
> +#define	AR5K_PHY_OFDM_SELFCORR_CYPWR_THR1_S	1

Heh, we should write a little tool to check the shifts and masks :)

> - * Write the OFDM timings for the AR5212 upon reset. This is a helper for
> - * ath5k_hw_reset(). This seems to tune the PLL a specified frequency
> - * depending on the bandwidth of the channel.
> + * Write the delta slope coefficient (used on pilot tracking ?) for OFDM
> + * operration on the AR5212 upon reset. This is a helper for ath5k_hw_reset().

operation

>   *
> + * Since delta slope is floating point we split it on it's exponent and

its

> + * mantissa and provide these values on hw.
> + *
> + * For more infos i think this pattent is related

patent

> @@ -53,13 +57,20 @@ static inline int ath5k_hw_write_ofdm_timings(struct ath5k_hw *ah,
>  		!(channel->hw_value & CHANNEL_OFDM))
>  		BUG();
>  
> -	/* Seems there are two PLLs, one for baseband sampling and one
> -	 * for tuning. Tuning basebands are 40 MHz or 80MHz when in
> -	 * turbo. */
> -	clock = channel->hw_value & CHANNEL_TURBO ? 80 : 40;
> +	/* Get coefficient
> +	 * ALGO: coef = (5 * clock * carrier_freq) / 2)
> +	 * we scale coef by shifting clock value by 24 for
> +	 * better precision since we use integers */
> +	/* TODO: Half/quarter rate */
> +	clock = channel->hw_value & CHANNEL_TURBO ?
> +				ath5k_hw_htoclock(1, true) :
> +				ath5k_hw_htoclock(1, false);
> +


Could be: 

    clock = ath5k_hw_htoclock(1, channel->hw_value & CHANNEL_TURBO);

>  	coef_scaled = ((5 * (clock << 24)) / 2) /
>  	channel->center_freq;
>  
> +	/* Get exponent
> +	 * ALGO: coe_exp = 14 - highest set bit position */
>  	for (coef_exp = 31; coef_exp > 0; coef_exp--)
>  		if ((coef_scaled >> coef_exp) & 0x1)
>  			break;

I know this is existing code, but we could use something like
get_bitmask_order() here or fls() instead of the open-coded loop.  
For some other patch.

>  /*
> + * If there is an external 32KHz crystal available, use it
> + * as ref. clock instead of 32/40MHz clock and baseband clocks
> + * to save power durring sleep or restore normal 32/40MHz

during

> + * operation.
> + *
> + * XXX: When operating on 32KHz certain PHY registers (27 - 31,
> + * 	123 - 127) require delay on access.
> + */
> +void ath5k_hw_set_sleep_clock(struct ath5k_hw *ah, bool enable)

ACK

> +static bool ath5k_hw_chan_has_spur_noise(struct ath5k_hw *ah,
> +				struct ieee80211_channel *channel)
> +{

ACK

> +	/* Set DAC/ADC delays */
> +	if (ah->ah_version == AR5K_AR5212) {
> +		if (ah->ah_mac_version == (AR5K_SREV_AR2417 >> 4))
> +			data = AR5K_PHY_SCAL_32MHZ_2417;
> +		else if (ath5k_eeprom_is_hb63(ah))
> +			data = AR5K_PHY_SCAL_32MHZ_HB63;
> +		else
> +			data = AR5K_PHY_SCAL_32MHZ;
> +		ath5k_hw_reg_write(ah, data, AR5K_PHY_SCAL);
> +		data = 0;

why data = 0; ?

> +	}
> +
> +	/* Set fast ADC */
> +	if ((ah->ah_radio == AR5K_RF5413) ||
> +	(ah->ah_mac_version == (AR5K_SREV_AR2417 >> 4))) {
> +		data = 1;
> +
> +		if (channel->center_freq == 2462 ||
> +		channel->center_freq == 2467)
> +			data = 0;
> +
> +		if (ath5k_hw_reg_read(ah, AR5K_PHY_FAST_ADC) != data)
> +				ath5k_hw_reg_write(ah, data,
> +						AR5K_PHY_FAST_ADC);
> +		data = 0;

ditto

> +	}
> +
> +	/* Fix for first revision of the RF5112 RF chipset */
> +	if (ah->ah_radio >= AR5K_RF5112 &&
> +			ah->ah_radio_5ghz_revision <
> +			AR5K_SREV_RAD_5112A) {
> +		ath5k_hw_reg_write(ah, AR5K_PHY_CCKTXCTL_WORLD,
> +				AR5K_PHY_CCKTXCTL);
> +		if (channel->hw_value & CHANNEL_5GHZ)
> +			data = 0xffb81020;
> +		else
> +			data = 0xffb80d20;
> +		ath5k_hw_reg_write(ah, data, AR5K_PHY_FRAME_CTL);
> +		data = 0;

ditto

> +	}
> +
> +	if (ah->ah_mac_srev < AR5K_SREV_AR5211) {
> +		/* 5311 has different tx/rx latency masks
> +		 * from 5211, since we deal 5311 the same
> +		 * as 5211 when setting initvals, shift
> +		 * values here to their proper locations */
> +		data = ath5k_hw_reg_read(ah, AR5K_USEC_5211);
> +		ath5k_hw_reg_write(ah, data & (AR5K_USEC_1 |
> +				AR5K_USEC_32 |
> +				AR5K_USEC_TX_LATENCY_5211 |
> +				AR5K_REG_SM(29,
> +				AR5K_USEC_RX_LATENCY_5210)),
> +				AR5K_USEC_5211);
> +		/* Clear QCU/DCU clock gating register */
> +		ath5k_hw_reg_write(ah, 0, AR5K_QCUDCU_CLKGT);
> +		/* Set DAC/ADC delays */
> +		ath5k_hw_reg_write(ah, 0x08, AR5K_PHY_SCAL);
> +		/* Enable PCU FIFO corruption ECO */
> +		AR5K_REG_ENABLE_BITS(ah, AR5K_DIAG_SW_5211,
> +					AR5K_DIAG_SW_ECO_ENABLE);
> +		data = 0;

ditto

> +	}
> +
> +	return;

No need for return.

> +}
> +
> +static void ath5k_hw_commit_eeprom_settings(struct ath5k_hw *ah,
> +		struct ieee80211_channel *channel, u8 *ant, u8 ee_mode)
> +{
> +	struct ath5k_eeprom_info *ee = &ah->ah_capabilities.cap_eeprom;
> +	int16_t cck_ofdm_pwr_delta = 0;

I don't think you need to assign it, unless gcc is particularly dumb.

> +
> +	/* Set CCK to OFDM power delta */
> +	if (ah->ah_phy_revision > AR5K_SREV_PHY_5212A) {
> +		/* Adjust power delta for channel 14 */
> +		if (channel->center_freq == 2484)
> +			cck_ofdm_pwr_delta =
> +				((ee->ee_cck_ofdm_power_delta -
> +				ee->ee_scaled_cck_delta) * 2) / 10;
> +		else
> +			cck_ofdm_pwr_delta =
> +				ee->ee_cck_ofdm_power_delta;

missing * 2 / 10 here?

> +
> +		if (channel->hw_value == CHANNEL_G)
> +			ath5k_hw_reg_write(ah,
> +			AR5K_REG_SM((ee->ee_cck_ofdm_power_delta * -1),
> [...]
> +		AR5K_REG_WRITE_BITS(ah, AR5K_PHY_GAIN,
> +				AR5K_PHY_GAIN_TXRX_ATTEN,
> +				ee->ee_atn_tx_rx[ee_mode]);
> +
> +		/* ADC/PGA dezired size */

desired

> [...]
> +
> +	/* Thres64 (ANI) */

Thresh62 ?

> +	AR5K_REG_WRITE_BITS(ah, AR5K_PHY_NF,
> +			AR5K_PHY_NF_THRESH62,
> +			ee->ee_thr_62[ee_mode]);
> +

> +	/* I/Q correction
> +	 * TODO: Per channel i/q infos ? */
> +	AR5K_REG_ENABLE_BITS(ah, AR5K_PHY_IQ,
> +		AR5K_PHY_IQ_CORR_ENABLE |
> +		(ee->ee_i_cal[ee_mode] << AR5K_PHY_IQ_CORR_Q_I_COFF_S) |
> +		ee->ee_q_cal[ee_mode]);
> +
> +	/* Heavy clipping -disable for now */
> +	if (ah->ah_ee_version >= AR5K_EEPROM_VERSION_5_1)
> +		ath5k_hw_reg_write(ah, 0, AR5K_PHY_HEAVY_CLIP_ENABLE);
> +
> +	return;

No need for return.

> +}

My eyes got tired, so I skipped a lot :)

> +
> +	/* QoS NOACK Policy */
> +	if (ah->ah_version == AR5K_AR5212) {
> +		ath5k_hw_reg_write(ah,
> +			AR5K_REG_SM(2, AR5K_QOS_NOACK_2BIT_VALUES) |
> +			AR5K_REG_SM(5, AR5K_QOS_NOACK_BIT_OFFSET)  |
> +			AR5K_REG_SM(0, AR5K_QOS_NOACK_BYTE_OFFSET),

AR5K_QOS_NOACK_BYTE_OFFSET_S is also wrong, should be 7.

> +			AR5K_QOS_NOACK);
> +	}
> +

Thanks!
-- 
Bob Copeland %% www.bobcopeland.com