ath5k: Update reset code

* 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

Signed-off-by Nick Kossifidis <[email protected]>

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

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

drivers/net/wireless/ath5k/reset.c +0 -3

@@ -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. */

/*
+ * 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

+ * 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)

drivers/net/wireless/ath5k/reset.c +5 -0

+	}
+
+	/* Set fast ADC */
+	if ((ah->ah_radio == AR5K_RF5413) ||
+	(ah->ah_mac_version == (AR5K_SREV_AR2417 >> 4))) {

drivers/net/wireless/ath5k/reset.c +4 -0

+	}
+
+	/* Fix for first revision of the RF5112 RF chipset */
+	if (ah->ah_radio >= AR5K_RF5112 &&

drivers/net/wireless/ath5k/reset.c +12 -0

+	}
+
+	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,

drivers/net/wireless/ath5k/reset.c +6 -0

+	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 ? */

+}

Quoted 5 lines

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

Quoted 1 line

Signed-off-by Nick Kossifidis <[email protected]>

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

drivers/net/wireless/ath5k/eeprom.c int ath5k_eeprom_read_mac(struct ath5k_hw *ah, u8 *mac) · line 1458

Quoted 8 lines

[...]

drivers/net/wireless/ath5k/eeprom.c +7 -0

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

Quoted 1 line

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

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

drivers/net/wireless/ath5k/reg.h line 2161

Quoted 8 lines

[...]

drivers/net/wireless/ath5k/reg.h +2 -1

@@ -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).

drivers/net/wireless/ath5k/reg.h line 2201

Quoted 2 lines

drivers/net/wireless/ath5k/reg.h +1 -1

-#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 :)

drivers/net/wireless/ath5k/reset.c line 41

Quoted 5 lines

drivers/net/wireless/ath5k/reset.c +2 -3

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

Quoted 2 lines

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

its

Quoted 3 lines

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

patent

Quoted 7 lines

drivers/net/wireless/ath5k/reset.c +0 -3

@@ -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. */

drivers/net/wireless/ath5k/reset.c static inline int ath5k_hw_write_ofdm_timings(struct ath5k_hw *ah, · line 68

Quoted 10 lines

drivers/net/wireless/ath5k/reset.c +9 -1

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

drivers/net/wireless/ath5k/reset.c static inline int ath5k_hw_write_ofdm_timings(struct ath5k_hw *ah, · line 76

Quoted 8 lines

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

      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.

Quoted 4 lines

/*
+ * 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

drivers/net/wireless/ath5k/reset.c int ath5k_hw_nic_wakeup(struct ath5k_hw *ah, int flags, bool initial) · line 612

Quoted 10 lines

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

+     /* 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; ?

drivers/net/wireless/ath5k/reset.c int ath5k_hw_nic_wakeup(struct ath5k_hw *ah, int flags, bool initial) · line 539

Quoted 4 lines

drivers/net/wireless/ath5k/reset.c +3 -0

+     }
+
+     return;

No need for return.

drivers/net/wireless/ath5k/reset.c int ath5k_hw_nic_wakeup(struct ath5k_hw *ah, int flags, bool initial) · line 673

Quoted 7 lines

drivers/net/wireless/ath5k/reset.c +7 -0

+}
+
+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.

drivers/net/wireless/ath5k/reset.c int ath5k_hw_nic_wakeup(struct ath5k_hw *ah, int flags, bool initial) · line 684

Quoted 11 lines

drivers/net/wireless/ath5k/reset.c +11 -0

+
+     /* 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?

drivers/net/wireless/ath5k/reset.c int ath5k_hw_nic_wakeup(struct ath5k_hw *ah, int flags, bool initial) · line 750

Quoted 10 lines

drivers/net/wireless/ath5k/reset.c +9 -0

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

drivers/net/wireless/ath5k/reset.c int ath5k_hw_nic_wakeup(struct ath5k_hw *ah, int flags, bool initial) · line 778

Quoted 3 lines

[...]

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

+
+     /* Thres64 (ANI) */

Thresh62 ?

drivers/net/wireless/ath5k/reset.c int ath5k_hw_reset(struct ath5k_hw *ah, enum nl80211_iftype op_mode, · line 1148

Quoted 7 lines

drivers/net/wireless/ath5k/reset.c +7 -0

+
+     /* 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.

Quoted 11 lines

drivers/net/wireless/ath5k/reg.h line 2161

Quoted 8 lines

[...]

drivers/net/wireless/ath5k/reg.h +2 -1

@@ -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).

I know, but HAL does this...

	AR_PHY_BIS(ah, 68, 0xFFFFFC06,
		(ee->ee_antennaControl[0][arrayMode] << 4) | 0x1);

~0xFFFFFC06 = 3F9

My intention is to reimplement hw_htoclock to account for half/quarter rate so it'll soon be
clock = ath5k_hw_htoclock(1, channel->hw_value);

Quoted 11 lines

Quoted 10 lines

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

+     /* 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;

Quoted 2 lines

why data = 0; ?

Instead of having multiple temporary variables, i use only "data". To be sure that "data" is ok for use i always zero it each time i use it so that we don't write any weird stuff by mistake.

Quoted 12 lines · Nick Kossifidis

Quoted 11 lines

drivers/net/wireless/ath5k/reg.h line 2161

Quoted 8 lines

[...]

drivers/net/wireless/ath5k/reg.h +2 -1

@@ -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).

Quoted 5 lines

I know, but HAL does this...

      AR_PHY_BIS(ah, 68, 0xFFFFFC06,
              (ee->ee_antennaControl[0][arrayMode] << 4) | 0x1);

~0xFFFFFC06 = 3F9

How about splitting it up and making the 0x1 a separate flag?

Quoted 3 lines

My intention is to reimplement hw_htoclock to account for half/quarter rate so it'll soon be
clock = ath5k_hw_htoclock(1, channel->hw_value);

AFAIK Sam's HAL might have a bug there. If I remember correctly, the MAC runs at the same clock speed with Half/Quarter rate.
It's done that way in the legacy HAL. Though there is a possibility of it being hardware dependent, I'm pretty sure that it is that way at least on 5413.

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

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

+     /* 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;

Quoted 5 lines

Quoted 2 lines

why data = 0; ?

Instead of having multiple temporary variables, i use only "data". To be sure that "data" is ok for use i always zero it each time i use it so that we don't write any weird stuff by mistake.

Wouldn't multiple temporary values be preferable, since the compiler optimizes that stuff anyway?

Quoted 9 lines

Quoted 3 lines

My intention is to reimplement hw_htoclock to account for half/quarter rate so it'll soon be
clock = ath5k_hw_htoclock(1, channel->hw_value);

AFAIK Sam's HAL might have a bug there. If I remember correctly, the MAC runs at the same clock speed with Half/Quarter rate.
It's done that way in the legacy HAL. Though there is a possibility of it being hardware dependent, I'm pretty sure that it is that way at least on 5413.

Both HALs tweak clock this way on ar5212SetDeltaSlope on all chips. If clock depends on channel bandwidth for turbo it makes sense to also change on half/quarter rate operation.

Quoted 2 lines

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

Yup :P

Thanks a lot for your comments, i'll address them asap and come up with a new version of this patch ;-)

Quoted 4 lines

Quoted 2 lines

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

Yup :P

I just wrote one, I can post it later :) It also found:

AR5K_PHY_SIGMA_DELTA_ADC_CLIP 0x01ff3000 13

Should be 12 or a different mask...

Quoted 2 lines

Thanks a lot for your comments, i'll address them asap and come up with a new version of this patch ;-)

My computer also hangs with the whole series... reverting the reset part makes it work. I'll try and narrow that down later when I get some time.

Are you all ok on the rest 4 patches ?

Quoted 4 lines

Quoted 2 lines

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

Yup :P

I just wrote one, I can post it later :) It also found:

AR5K_PHY_SIGMA_DELTA_ADC_CLIP 0x01ff3000 13

Should be 12 or a different mask...

2009/2/1 Bob Copeland <[email protected]>:

Quoted 8 lines · Nick Kossifidis

Quoted 4 lines

Quoted 2 lines

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

Yup :P

I just wrote one, I can post it later :) It also found:

Quoted 4 lines

AR5K_PHY_SIGMA_DELTA_ADC_CLIP 0x01ff3000 13

Should be 12 or a different mask...

13 is correct

0x01ff3000 -> 1111111111110000000000000

Quoted 14 lines · Nick Kossifidis

2009/2/1 Bob Copeland <[email protected]>:

Quoted 12 lines · Nick Kossifidis

Quoted 4 lines

Quoted 2 lines

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

Yup :P

I just wrote one, I can post it later :) It also found:

AR5K_PHY_SIGMA_DELTA_ADC_CLIP 0x01ff3000 13

Should be 12 or a different mask...

Quoted 3 lines

13 is correct

0x01ff3000 -> 1111111111110000000000000

I think it's 1111111110011000000000000? :)

This mask makes 12 seem more likely.

- Felix

2009/2/1 Felix Fietkau <[email protected]>:

Quoted 20 lines

Quoted 9 lines · Nick Kossifidis

2009/2/1 Bob Copeland <[email protected]>:

Quoted 8 lines · Nick Kossifidis

Quoted 4 lines

Quoted 2 lines

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

Yup :P

I just wrote one, I can post it later :) It also found:

Quoted 8 lines

Quoted 4 lines

AR5K_PHY_SIGMA_DELTA_ADC_CLIP 0x01ff3000 13

Should be 12 or a different mask...

13 is correct

0x01ff3000 -> 1111111111110000000000000

I think it's 1111111110011000000000000? :)

Quoted 4 lines

This mask makes 12 seem more likely.

- Felix

Maybe it's in big endian format (we don't know anything about this reg anyway :P), both HALs have 13 plus it doesn't make sense to have a field split.

Quoted 21 lines · Nick Kossifidis

2009/2/1 Felix Fietkau <[email protected]>:

Quoted 20 lines

Quoted 9 lines · Nick Kossifidis

2009/2/1 Bob Copeland <[email protected]>:

Quoted 8 lines · Nick Kossifidis

Quoted 4 lines

Quoted 2 lines

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

Yup :P

I just wrote one, I can post it later :) It also found:

Quoted 8 lines

Quoted 4 lines

AR5K_PHY_SIGMA_DELTA_ADC_CLIP 0x01ff3000 13

Should be 12 or a different mask...

13 is correct

0x01ff3000 -> 1111111111110000000000000

I think it's 1111111110011000000000000? :)

Quoted 8 lines

Quoted 4 lines

This mask makes 12 seem more likely.

- Felix

Maybe it's in big endian format (we don't know anything about this reg anyway :P), both HALs have 13 plus it doesn't make sense to have a field split.

Ah, figured it out. It's a typo :)
Here are the settings from Sam's HAL:

#define   AR_PHY_SIGMA_DELTA_ADC_CLIP     0x01FFE000
#define   AR_PHY_SIGMA_DELTA_ADC_CLIP_S   13

Which does make a lot more sense than 0x01FF3000 :)

- Felix

int ath5k_hw_reset(struct ath5k_hw *ah, enum nl80211_iftype op_mode,
struct ieee80211_channel *channel, bool change_channel)

drivers/net/wireless/ath5k/reset.c +6 -0

+		if (change_channel) {
+			/*
+			 * Save frame sequence count
+			 * For revs. after Oahu, only save
+			 * seq num for DCU 0 (Global seq num)
+			 */

On Sat, 31 Jan 2009 04:31:47 +0200, Nick Kossifidis wrote

Quoted 2 lines

int ath5k_hw_reset(struct ath5k_hw *ah, enum nl80211_iftype op_mode,
struct ieee80211_channel *channel, bool change_channel)

Here's another thing I just noticed:

Quoted 2 lines

{
+ u32 s_seq[10], s_ant, s_led[3], staid1_flags, tsf_up, tsf_lo;

s_seq is now an array (we used to initialize s_seq to 0)

[...]

Quoted 6 lines

drivers/net/wireless/ath5k/reset.c +6 -0

+             if (change_channel) {
+                     /*
+                      * Save frame sequence count
+                      * For revs. after Oahu, only save
+                      * seq num for DCU 0 (Global seq num)
+                      */

drivers/net/wireless/ath5k/reset.c int ath5k_hw_reset(struct ath5k_hw *ah, enum nl80211_iftype op_mode, · line 900

Quoted 10 lines

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

+                     if (ah->ah_mac_srev < AR5K_SREV_AR5211) {
+
+                             for (i = 0; i < 10; i++)
+                                     s_seq[i] = ath5k_hw_reg_read(ah,
+                                             AR5K_QUEUE_DCU_SEQNUM(i));
+
+                     } else {
+                             s_seq[0] = ath5k_hw_reg_read(ah,
+                                             AR5K_QUEUE_DCU_SEQNUM(0));
+                     }

We only save the DCU sequence values if changing a channel

[...]

drivers/net/wireless/ath5k/reset.c int ath5k_hw_reset(struct ath5k_hw *ah, enum nl80211_iftype op_mode, · line 1092

Quoted 11 lines

      if (ah->ah_version != AR5K_AR5210) {

drivers/net/wireless/ath5k/reset.c +9 -1

-             ath5k_hw_reg_write(ah, s_seq, AR5K_QUEUE_DFS_SEQNUM(0));
+
+             if (ah->ah_mac_srev < AR5K_SREV_AR5211) {
+                     for (i = 0; i < 10; i++)
+                             ath5k_hw_reg_write(ah, s_seq[i],
+                                     AR5K_QUEUE_DCU_SEQNUM(i));
+             } else {
+                     ath5k_hw_reg_write(ah, s_seq[0],
+                             AR5K_QUEUE_DCU_SEQNUM(0));
+             }

But always write it. The old code did this too, the only difference was we explicitly initialized the s_seq[0] to 0. Since it's an array, gcc won't warn about it but s_seq[] probably has random crap in it.

Nice catch ;-)

Another difference i remembered is that we now don't set the TPC register to 3f (until we fix the whole tx power stuff). I'll try to reproduce this with my ar2425 as it turns it also has problems.

Quoted 5 lines

Nice catch ;-)

Another difference i remembered is that we now don't set the TPC register to 3f (until we fix the whole tx power stuff). I'll try to reproduce this with my ar2425 as it turns it also has problems.

And the winning hunk is:

       if (ah->ah_version != AR5K_AR5210) {
               ath5k_hw_reg_write(ah, 0xffffffff, AR5K_PISR);

drivers/net/wireless/ath5k/reset.c +0 -4

-               /* If we later allow tuning for this, store into sc structure */
-               data = AR5K_TUNE_RSSI_THRES |
-                       AR5K_TUNE_BMISS_THRES << AR5K_RSSI_THR_BMISS_S;
-               ath5k_hw_reg_write(ah, data, AR5K_RSSI_THR);
       }

(hmm, bmiss threshold, should've seen that before...)

With that reverted from the rest of the patchset, I no longer get the interrupt storm. I do seem to get an occasional lockup at association time, but I haven't caught an oops for that yet. It could be unrelated.

2009/2/4 Bob Copeland <[email protected]>:

Quoted 17 lines · Nick Kossifidis

Quoted 5 lines

Nice catch ;-)

Another difference i remembered is that we now don't set the TPC register to 3f (until we fix the whole tx power stuff). I'll try to reproduce this with my ar2425 as it turns it also has problems.

And the winning hunk is:

       if (ah->ah_version != AR5K_AR5210) {
               ath5k_hw_reg_write(ah, 0xffffffff, AR5K_PISR);

drivers/net/wireless/ath5k/reset.c +0 -4

-               /* If we later allow tuning for this, store into sc structure */
-               data = AR5K_TUNE_RSSI_THRES |
-                       AR5K_TUNE_BMISS_THRES << AR5K_RSSI_THR_BMISS_S;
-               ath5k_hw_reg_write(ah, data, AR5K_RSSI_THR);
       }

Quoted 6 lines

(hmm, bmiss threshold, should've seen that before...)

With that reverted from the rest of the patchset, I no longer get the interrupt storm. I do seem to get an occasional lockup at association time, but I haven't caught an oops for that yet. It could be unrelated.

O.K. i got that from legacy HAL. It first reads the register and if it's zeroed it sets to the default value of 0x781 (which is the same really, it's AR5K_TUNE_RSSI_THRES | AR5K_TUNE_BMISS_THRES << AR5K_RSSI_THR_BMISS_S), if not it saves it and restores it after reset is done.

So can you see why this

       /* Save RSSI thresholds (they 'll get zeroed from initvals) */
       rssi_thr = ath5k_hw_reg_read(ah, AR5K_RSSI_THR);
       /* If they are not set, set the default value */
       if (!rssi_thr)
               rssi_thr = AR5K_INIT_RSSI_THR;

doesn't work ? What is the value of rssi_trh if not zero ?

* Update reset and sync with HALs
* Clean up eeprom settings and tweaking of initvals and put them on

* Set/Restore 32KHz ref clk operation
* Add some more documentation
TODO: Spur mitigation, tpc, half/quarter rate, compression etc

v2: Address comments from Bob and Felix and fix RSSI threshold bug introduced on the first version of the patch

Signed-off-by Nick Kossifidis <[email protected]>

On Wed, 4 Feb 2009 23:57:06 +0200, Nick Kossifidis wrote

Quoted 2 lines

* Update reset and sync with HALs
* Clean up eeprom settings and tweaking of initvals and put them on

separate functions

Quoted 8 lines

* Set/Restore 32KHz ref clk operation
* Add some more documentation
TODO: Spur mitigation, tpc, half/quarter rate, compression etc

v2: Address comments from Bob and Felix and fix RSSI threshold bug introduced on the first version of the patch

Signed-off-by Nick Kossifidis <[email protected]>

This works much better, except I still get some hard hangs occasionally. Doesn't appear to be bmiss storm this time. I did it with a VT up and didn't get an oops; I guess I'll try a serial console or some printks.

I could trigger it via a few suspend-resume cycles, so I guess it's the reset from ath5k_init(). Did you still have any problems with the 2425?

On Wed, 4 Feb 2009 23:57:06 +0200, Nick Kossifidis wrote

Quoted 2 lines

* Update reset and sync with HALs
* Clean up eeprom settings and tweaking of initvals and put them on

separate functions

Quoted 8 lines

* Set/Restore 32KHz ref clk operation
* Add some more documentation
TODO: Spur mitigation, tpc, half/quarter rate, compression etc

v2: Address comments from Bob and Felix and fix RSSI threshold bug introduced on the first version of the patch

Signed-off-by Nick Kossifidis <[email protected]>

This works much better, except I still get some hard hangs occasionally. Doesn't appear to be bmiss storm this time. I did it with a VT up and didn't get an oops; I guess I'll try a serial console or some printks.

I could trigger it via a few suspend-resume cycles, so I guess it's the reset from ath5k_init(). Did you still have any problems with the 2425?

I think this is because we call reset too many times. Try removing call to reset from config_interface, after some tests i did it seems

I think this is because we call reset too many times. Try removing call to reset from config_interface, after some tests i did it seems wrong (we don't need to reset pcu and if we do, we don't need to reset the whole chip and re run phy initialization etc). I don't get any problems with ar2425 but i didn't get any before anyway (i never got a hang, just a few bmiss interrupts, not a storm).

Quoted 6 lines

I think this is because we call reset too many times. Try removing call to reset from config_interface, after some tests i did it seems wrong (we don't need to reset pcu and if we do, we don't need to reset the whole chip and re run phy initialization etc). I don't get any problems with ar2425 but i didn't get any before anyway (i never got a hang, just a few bmiss interrupts, not a storm).

Yep, with BMISS interrupts turned off and config_interface no longer calling reset, it's quite stable now. Maybe there was some timing thing at play. If we add a patch to remove those then I'm satisfied :)

O.K. do you want me to put these fixes on this patch or post a separate ?

Quoted 1 line

O.K. do you want me to put these fixes on this patch or post a separate ?

I think a separate patch is fine, it's a valid change by itself.

--
Bob Copeland %% www.bobcopeland.com

2009/2/7 Bob Copeland <[email protected]>:

Quoted 9 lines · Nick Kossifidis

Quoted 1 line

O.K. do you want me to put these fixes on this patch or post a separate ?

I think a separate patch is fine, it's a valid change by itself.

--
Bob Copeland %% www.bobcopeland.com

O.K. i'm sending a separate patch for this ;-)

John is it ok to push these 5 patches on wireless-testing or should i re-send the whole series along with the next one ?

Quoted 13 lines

2009/2/7 Bob Copeland <[email protected]>:

Quoted 9 lines · Nick Kossifidis

Quoted 1 line

O.K. do you want me to put these fixes on this patch or post a separate ?

I think a separate patch is fine, it's a valid change by itself.

--
Bob Copeland %% www.bobcopeland.com

O.K. i'm sending a separate patch for this ;-)

Quoted 2 lines

John is it ok to push these 5 patches on wireless-testing or should i re-send the whole series along with the next one ?

There has been a ton of discussion in this thread. It would make things more clear in my inbox if you reposted the whole revised series.

Thanks!

On Wed, 4 Feb 2009 23:57:06 +0200, Nick Kossifidis wrote

Quoted 2 lines

* Update reset and sync with HALs
* Clean up eeprom settings and tweaking of initvals and put them on

separate functions

Quoted 7 lines

* Set/Restore 32KHz ref clk operation
* Add some more documentation
TODO: Spur mitigation, tpc, half/quarter rate, compression etc

v2: Address comments from Bob and Felix and fix RSSI threshold bug introduced on the first version of the patch

Quoted 1 line

Signed-off-by Nick Kossifidis <[email protected]>

This works much better, except I still get some hard hangs occasionally. Doesn't appear to be bmiss storm this time. I did it with a VT up and didn't get an oops; I guess I'll try a serial console or some printks.

Quoted 4 lines

This works much better, except I still get some hard hangs occasionally. Doesn't appear to be bmiss storm this time. I did it with a VT up and didn't get an oops; I guess I'll try a serial console or some printks.

Did you disable BMISS interrupts on base.c ?