ath5k: Use SWI to trigger calibration

PATCH In review Nick Kossifidis 8 participants 23 messages Jul 31, 2009 – Aug 7, 2009 Atheros ATH5K wireless driver

lore

Share & tools

lore

Nick Kossifidis

author

2009-07-31 18:10 UTC #0 lore raw

PATCH 4/4 ath5k: Use SWI to trigger calibration

* Get rid of calibration timer, instead use a software interrupt to schedule the calibration tasklet.

---
 drivers/net/wireless/ath/ath5k/ath5k.h |   16 ++++++++++++++++
 drivers/net/wireless/ath/ath5k/base.c  |   28 ++++++++++++++++++----------
 drivers/net/wireless/ath/ath5k/base.h  |    3 ++-
 drivers/net/wireless/ath/ath5k/phy.c   |   20 ++++++++++++++++++++
 4 files changed, 56 insertions(+), 11 deletions(-)

drivers/net/wireless/ath/ath5k/ath5k.h +16 -0

index 1047a6c..76cf5b2 100644
@@ -919,6 +919,12 @@ enum ath5k_int {
 	AR5K_INT_NOCARD	= 0xffffffff
 };
 
+/* Software interrupts used for calibration */
+enum ath5k_software_interrupt {
+	AR5K_SWI_FULL_CALIBRATION = 0x01,
+	AR5K_SWI_SHORT_CALIBRATION = 0x02,
+};
+
 /*
  * Power management
  */

@@ -1123,6 +1129,15 @@ struct ath5k_hw {
 	/* noise floor from last periodic calibration */
 	s32			ah_noise_floor;
 
+	/* Calibration timestamp */
+	u32			ah_cal_tstamp;
+
+	/* Calibration interval (secs) */
+	u8			ah_cal_intval;
+
+	/* Software interrupt mask */
+	u8			ah_swi_mask;
+
 	/*
 	 * Function pointers
 	 */

@@ -1276,6 +1291,7 @@ extern int ath5k_hw_channel(struct ath5k_hw *ah, struct ieee80211_channel *chann
 /* PHY calibration */
 extern int ath5k_hw_phy_calibrate(struct ath5k_hw *ah, struct ieee80211_channel *channel);
 extern int ath5k_hw_noise_floor_calibration(struct ath5k_hw *ah, short freq);
+extern void ath5k_hw_calibration_poll(struct ath5k_hw *ah);
 /* Spur mitigation */
 bool ath5k_hw_chan_has_spur_noise(struct ath5k_hw *ah,
 				struct ieee80211_channel *channel);

drivers/net/wireless/ath/ath5k/base.c +18 ~~-10~~

index b64731b..02fa71a 100644
@@ -59,7 +59,7 @@
 #include "reg.h"
 #include "debug.h"
 
-static int ath5k_calinterval = 10; /* Calibrate PHY every 10 secs (TODO: Fixme) */
+static u8 ath5k_calinterval = 10; /* Calibrate PHY every 10 secs (TODO: Fixme) */
 static int modparam_nohwcrypt;
 module_param_named(nohwcrypt, modparam_nohwcrypt, bool, S_IRUGO);
 MODULE_PARM_DESC(nohwcrypt, "Disable hardware encryption.");

@@ -376,7 +376,7 @@ static int 	ath5k_stop_hw(struct ath5k_softc *sc);
 static irqreturn_t ath5k_intr(int irq, void *dev_id);
 static void 	ath5k_tasklet_reset(unsigned long data);
 
-static void 	ath5k_calibrate(unsigned long data);
+static void 	ath5k_tasklet_calibrate(unsigned long data);
 
 /*
  * Module init/exit functions

@@ -799,8 +799,8 @@ ath5k_attach(struct pci_dev *pdev, struct ieee80211_hw *hw)
 	tasklet_init(&sc->rxtq, ath5k_tasklet_rx, (unsigned long)sc);
 	tasklet_init(&sc->txtq, ath5k_tasklet_tx, (unsigned long)sc);
 	tasklet_init(&sc->restq, ath5k_tasklet_reset, (unsigned long)sc);
+	tasklet_init(&sc->calib, ath5k_tasklet_calibrate, (unsigned long)sc);
 	tasklet_init(&sc->beacontq, ath5k_tasklet_beacon, (unsigned long)sc);
-	setup_timer(&sc->calib_tim, ath5k_calibrate, (unsigned long)sc);
 
 	ret = ath5k_eeprom_read_mac(ah, mac);
 	if (ret) {

@@ -2366,7 +2366,7 @@ ath5k_init(struct ath5k_softc *sc)
 	sc->curband = &sc->sbands[sc->curchan->band];
 	sc->imask = AR5K_INT_RXOK | AR5K_INT_RXERR | AR5K_INT_RXEOL |
 		AR5K_INT_RXORN | AR5K_INT_TXDESC | AR5K_INT_TXEOL |
-		AR5K_INT_FATAL | AR5K_INT_GLOBAL;
+		AR5K_INT_FATAL | AR5K_INT_GLOBAL | AR5K_INT_SWI;
 	ret = ath5k_reset(sc, NULL);
 	if (ret)
 		goto done;

@@ -2383,8 +2383,8 @@ ath5k_init(struct ath5k_softc *sc)
 	/* Set ack to be sent at low bit-rates */
 	ath5k_hw_set_ack_bitrate_high(ah, false);
 
-	mod_timer(&sc->calib_tim, round_jiffies(jiffies +
-			msecs_to_jiffies(ath5k_calinterval * 1000)));
+	/* Set PHY calibration timestamp and inteval */
+	ah->ah_cal_intval = ath5k_calinterval;
 
 	ret = 0;
 done:

@@ -2477,7 +2477,6 @@ ath5k_stop_hw(struct ath5k_softc *sc)
 	mmiowb();
 	mutex_unlock(&sc->lock);
 
-	del_timer_sync(&sc->calib_tim);
 	tasklet_kill(&sc->rxtq);
 	tasklet_kill(&sc->txtq);
 	tasklet_kill(&sc->restq);

@@ -2536,6 +2535,9 @@ ath5k_intr(int irq, void *dev_id)
 			if (status & AR5K_INT_BMISS) {
 				/* TODO */
 			}
+			if (status & AR5K_INT_SWI) {
+				tasklet_schedule(&sc->calib);
+			}
 			if (status & AR5K_INT_MIB) {
 				/*
 				 * These stats are also used for ANI i think

@@ -2552,6 +2554,8 @@ ath5k_intr(int irq, void *dev_id)
 	if (unlikely(!counter))
 		ATH5K_WARN(sc, "too many interrupts, giving up for now\n");
 
+	ath5k_hw_calibration_poll(ah);
+
 	return IRQ_HANDLED;
 }

@@ -2568,11 +2572,15 @@ ath5k_tasklet_reset(unsigned long data)
  * for temperature/environment changes.
  */
 static void
-ath5k_calibrate(unsigned long data)
+ath5k_tasklet_calibrate(unsigned long data)
 {
 	struct ath5k_softc *sc = (void *)data;
 	struct ath5k_hw *ah = sc->ah;
 
+	/* Only full calibration for now */
+	if (ah->ah_swi_mask != AR5K_SWI_FULL_CALIBRATION)
+		return;
+
 	ATH5K_DBG(sc, ATH5K_DEBUG_CALIBRATE, "channel %u/%x\n",
 		ieee80211_frequency_to_channel(sc->curchan->center_freq),
 		sc->curchan->hw_value);

@@ -2590,8 +2598,8 @@ ath5k_calibrate(unsigned long data)
 			ieee80211_frequency_to_channel(
 				sc->curchan->center_freq));
 
-	mod_timer(&sc->calib_tim, round_jiffies(jiffies +
-			msecs_to_jiffies(ath5k_calinterval * 1000)));
+	ah->ah_swi_mask = 0;
+
 }

drivers/net/wireless/ath/ath5k/base.h +2 -1

index 778e422..667bd9d 100644
@@ -177,6 +177,8 @@ struct ath5k_softc {
 
 	struct ath5k_rfkill	rf_kill;
 
+	struct tasklet_struct	calib;		/* calibration tasklet */
+
 	spinlock_t		block;		/* protects beacon */
 	struct tasklet_struct	beacontq;	/* beacon intr tasklet */
 	struct ath5k_buf	*bbuf;		/* beacon buffer */

@@ -187,7 +189,6 @@ struct ath5k_softc {
 	unsigned int		nexttbtt;	/* next beacon time in TU */
 	struct ath5k_txq	*cabq;		/* content after beacon */
 
-	struct timer_list	calib_tim;	/* calibration timer */
 	int 			power_level;	/* Requested tx power in dbm */
 	bool			assoc;		/* assocate state */
 	bool			enable_beacon;	/* true if beacons are on */

drivers/net/wireless/ath/ath5k/phy.c +20 -0

index 6afba98..d30bc3b 100644
@@ -1104,6 +1104,26 @@ int ath5k_hw_channel(struct ath5k_hw *ah, struct ieee80211_channel *channel)
   PHY calibration
 \*****************/
 
+void
+ath5k_hw_calibration_poll(struct ath5k_hw *ah)
+{
+	u32 current_time = (jiffies / HZ);
+	u32 cal_intval = ah->ah_cal_intval;
+
+	if (!ah->ah_cal_tstamp)
+		ah->ah_cal_tstamp = current_time;
+
+	/* For now we always do full calibration
+	 * Mark software interrupt mask and fire software
+	 * interrupt (bit gets auto-cleared) */
+	if ((current_time - ah->ah_cal_tstamp) >= cal_intval) {
+		ah->ah_cal_tstamp = current_time;
+		ah->ah_swi_mask = AR5K_SWI_FULL_CALIBRATION;
+		AR5K_REG_ENABLE_BITS(ah, AR5K_CR, AR5K_CR_SWI);
+	}
+
+}
+
 /**
  * ath5k_hw_noise_floor_calibration - perform PHY noise floor calibration
  *

Luis R. Rodriguez

2009-07-31 18:16 UTC #1 lore raw

Quoted 2 lines · Nick Kossifidis

* Get rid of calibration timer, instead use a software interrupt to schedule the calibration tasklet.

An example of itemizing one change. Explain the why, not the how.

Luis

Nick Kossifidis

maintainer

2009-07-31 18:25 UTC #2 lore raw

2009/7/31 Luis R. Rodriguez <[email protected]>:

Quoted 7 lines

Quoted 2 lines · Nick Kossifidis
* Get rid of calibration timer, instead use a software interrupt to schedule the calibration tasklet.
An example of itemizing one change. Explain the why, not the how.

We 've discussed this with various people on wireless summit.

a) We don't need a timer for this, there is no need for accuracy even with round_jiffies i think this is a waste of resources. Also we don't need to run calibration if we are idle (no interrupts).

b) When we add ANI support we 'll just extend the poll function and calibration tasklet and handle all periodic phy calibration on one place (much cleaner).

c) Having calibration on a tasklet is better since during calibration we can't transmit or receive (antennas are detached to measure noise floor), previously calibration could run in parallel with tx/rx and interfere (packet loss).

Signature · 3 lines

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

Luis R. Rodriguez

2009-07-31 18:48 UTC #3 lore raw

Quoted 10 lines · Nick Kossifidis

2009/7/31 Luis R. Rodriguez <[email protected]>:
Quoted 7 lines
Quoted 2 lines · Nick Kossifidis
* Get rid of calibration timer, instead use a software interrupt to schedule the calibration tasklet.
An example of itemizing one change. Explain the why, not the how.
We 've discussed this with various people on wireless summit.

Not everyone attended.

Quoted 4 lines

a) We don't need a timer for this, there is no need for accuracy even with round_jiffies i think this is a waste of resources. Also we don't need to run calibration if we are idle (no interrupts).

Quoted 8 lines

b) When we add ANI support we 'll just extend the poll function and calibration tasklet and handle all periodic phy calibration on one place (much cleaner).
c) Having calibration on a tasklet is better since during calibration we can't transmit or receive (antennas are detached to measure noise floor), previously calibration could run in parallel with tx/rx and interfere (packet loss).

It seems this good explanation can be mangled into the commit log entry.

Luis

Pavel Roskin

2009-07-31 19:09 UTC #4 lore raw

In reply to Nick Kossifidis

Quoted 3 lines · Nick Kossifidis

a) We don't need a timer for this, there is no need for accuracy even with round_jiffies i think this is a waste of resources. Also we don't need to run calibration if we are idle (no interrupts).

It doesn't sound right to me.

Can it be that the device gets so miscalibrated during the silence that it won't be able to receive signal once it's there? Say, the device is set to channel 1 but it actually listens to channel 3, and there are some weak beacons on channel 1.

And then there is an issue of the frequency straying outside the permitted spectrum. Perhaps the transmission will trigger interrupts and thus recalibration, but in case of scanning, the first probe requests could be on wrong frequencies.

I would probably leave the timer and allow the interrupts make the delays shorter.

Signature · 2 lines

Regards,
Pavel Roskin

Nick Kossifidis

maintainer

2009-07-31 19:35 UTC #5 lore raw

2009/7/31 Pavel Roskin <[email protected]>:

Quoted 6 lines · Nick Kossifidis

Quoted 4 lines
a) We don't need a timer for this, there is no need for accuracy even with round_jiffies i think this is a waste of resources. Also we don't need to run calibration if we are idle (no interrupts).

Quoted 7 lines

It doesn't sound right to me.
Can it be that the device gets so miscalibrated during the silence that it won't be able to receive signal once it's there? Say, the device is set to channel 1 but it actually listens to channel 3, and there are some weak beacons on channel 1.

Nope calibration deals mostly with noise immunity, it calculates the noise floor and fixes QAM constellation. It's not related to the synthesizer so it's not related to the channel frequency.

Quoted 5 lines

And then there is an issue of the frequency straying outside the permitted spectrum. Perhaps the transmission will trigger interrupts and thus recalibration, but in case of scanning, the first probe requests could be on wrong frequencies.

On each reset we trigger calibration so no worries there.

Signature · 3 lines

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

Pavel Roskin

2009-07-31 19:43 UTC #6 lore raw

Quoted 3 lines · Nick Kossifidis

Nope calibration deals mostly with noise immunity, it calculates the noise floor and fixes QAM constellation. It's not related to the synthesizer so it's not related to the channel frequency.

OK then, good to know.

Signature · 2 lines

Regards,
Pavel Roskin

Bob Copeland

2009-07-31 19:11 UTC #7 lore raw

In reply to Nick Kossifidis

Quoted 1 line · Nick Kossifidis

Also we don't need to run calibration if we are idle (no interrupts).

I think this is the big win right now...

Quoted 4 lines

c) Having calibration on a tasklet is better since during calibration we can't transmit or receive (antennas are detached to measure noise floor), previously calibration could run in parallel with tx/rx and interfere (packet loss).

This can still happen, no? Two tasklets can run in parallel on different processors, as long as they are different tasklets.

In practice, this won't happen much because tasklets run on the cpu that scheduled them, and irq affinity is such (at least on my hardware) that it's almost always the same CPU. But I think to make the above true it needs to stop the queues etc when doing calibration.

Also we are missing tasklet_kill()?

Signature · 1 line

Bob Copeland %% www.bobcopeland.com

Nick Kossifidis

maintainer

2009-07-31 19:52 UTC #8 lore raw

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

Quoted 10 lines · Nick Kossifidis

Quoted 1 line
Also we don't need to run calibration if we are idle (no interrupts).
I think this is the big win right now...
Quoted 4 lines
c) Having calibration on a tasklet is better since during calibration we can't transmit or receive (antennas are detached to measure noise floor), previously calibration could run in parallel with tx/rx and interfere (packet loss).

Quoted 9 lines

This can still happen, no? Two tasklets can run in parallel on different processors, as long as they are different tasklets.
In practice, this won't happen much because tasklets run on the cpu that scheduled them, and irq affinity is such (at least on my hardware) that it's almost always the same CPU. But I think to make the above true it needs to stop the queues etc when doing calibration.

ACK, how do we do that ?
ieee80211_stop_queues / ieee80211_wake_queues

Quoted 2 lines

Also we are missing tasklet_kill()?

Yup, resending...

Signature · 3 lines

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

Bob Copeland

2009-08-01 13:23 UTC #9 lore raw

Quoted 5 lines · Nick Kossifidis

Quoted 2 lines
make the above true it needs to stop the queues etc when doing calibration.
ACK, how do we do that ?
ieee80211_stop_queues / ieee80211_wake_queues

Yup. And disabling (at least rx and swba) interrupts on the device is also probably a good idea.

Signature · 1 line

Bob Copeland %% www.bobcopeland.com

Nick Kossifidis

maintainer

2009-08-01 13:31 UTC #10 lore raw

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

Quoted 10 lines · Nick Kossifidis

Quoted 5 lines
Quoted 2 lines
make the above true it needs to stop the queues etc when doing calibration.
ACK, how do we do that ?
ieee80211_stop_queues / ieee80211_wake_queues
Yup. And disabling (at least rx and swba) interrupts on the device is also probably a good idea.

I thought about calling ath5k_hw_stop_rx_pcu but i don't think it's necessary i mean it won't receive anyway, why mess with pcu ?

Signature · 3 lines

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

Bob Copeland

2009-08-02 16:15 UTC #11 lore raw

Quoted 2 lines · Nick Kossifidis

I thought about calling ath5k_hw_stop_rx_pcu but i don't think it's necessary i mean it won't receive anyway, why mess with pcu ?

Yeah, agreed, we can just ignore rx.

Signature · 1 line

Bob Copeland %% www.bobcopeland.com

Nick Kossifidis

author

2009-07-31 18:34 UTC #12 lore raw

In reply to Nick Kossifidis

* Get rid of calibration timer, instead use a software interrupt to schedule the calibration tasklet.

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

---
 drivers/net/wireless/ath/ath5k/ath5k.h |   16 ++++++++++++++++
 drivers/net/wireless/ath/ath5k/base.c  |   28 ++++++++++++++++++----------
 drivers/net/wireless/ath/ath5k/base.h  |    3 ++-
 drivers/net/wireless/ath/ath5k/phy.c   |   20 ++++++++++++++++++++
 4 files changed, 56 insertions(+), 11 deletions(-)

drivers/net/wireless/ath/ath5k/ath5k.h +16 -0

index 1047a6c..76cf5b2 100644
@@ -919,6 +919,12 @@ enum ath5k_int {
 	AR5K_INT_NOCARD	= 0xffffffff
 };
 
+/* Software interrupts used for calibration */
+enum ath5k_software_interrupt {
+	AR5K_SWI_FULL_CALIBRATION = 0x01,
+	AR5K_SWI_SHORT_CALIBRATION = 0x02,
+};
+
 /*
  * Power management
  */

@@ -1123,6 +1129,15 @@ struct ath5k_hw {
 	/* noise floor from last periodic calibration */
 	s32			ah_noise_floor;
 
+	/* Calibration timestamp */
+	u32			ah_cal_tstamp;
+
+	/* Calibration interval (secs) */
+	u8			ah_cal_intval;
+
+	/* Software interrupt mask */
+	u8			ah_swi_mask;
+
 	/*
 	 * Function pointers
 	 */

@@ -1276,6 +1291,7 @@ extern int ath5k_hw_channel(struct ath5k_hw *ah, struct ieee80211_channel *chann
 /* PHY calibration */
 extern int ath5k_hw_phy_calibrate(struct ath5k_hw *ah, struct ieee80211_channel *channel);
 extern int ath5k_hw_noise_floor_calibration(struct ath5k_hw *ah, short freq);
+extern void ath5k_hw_calibration_poll(struct ath5k_hw *ah);
 /* Spur mitigation */
 bool ath5k_hw_chan_has_spur_noise(struct ath5k_hw *ah,
 				struct ieee80211_channel *channel);

drivers/net/wireless/ath/ath5k/base.c +18 ~~-10~~

index b64731b..02fa71a 100644
@@ -59,7 +59,7 @@
 #include "reg.h"
 #include "debug.h"
 
-static int ath5k_calinterval = 10; /* Calibrate PHY every 10 secs (TODO: Fixme) */
+static u8 ath5k_calinterval = 10; /* Calibrate PHY every 10 secs (TODO: Fixme) */
 static int modparam_nohwcrypt;
 module_param_named(nohwcrypt, modparam_nohwcrypt, bool, S_IRUGO);
 MODULE_PARM_DESC(nohwcrypt, "Disable hardware encryption.");

@@ -376,7 +376,7 @@ static int 	ath5k_stop_hw(struct ath5k_softc *sc);
 static irqreturn_t ath5k_intr(int irq, void *dev_id);
 static void 	ath5k_tasklet_reset(unsigned long data);
 
-static void 	ath5k_calibrate(unsigned long data);
+static void 	ath5k_tasklet_calibrate(unsigned long data);
 
 /*
  * Module init/exit functions

@@ -799,8 +799,8 @@ ath5k_attach(struct pci_dev *pdev, struct ieee80211_hw *hw)
 	tasklet_init(&sc->rxtq, ath5k_tasklet_rx, (unsigned long)sc);
 	tasklet_init(&sc->txtq, ath5k_tasklet_tx, (unsigned long)sc);
 	tasklet_init(&sc->restq, ath5k_tasklet_reset, (unsigned long)sc);
+	tasklet_init(&sc->calib, ath5k_tasklet_calibrate, (unsigned long)sc);
 	tasklet_init(&sc->beacontq, ath5k_tasklet_beacon, (unsigned long)sc);
-	setup_timer(&sc->calib_tim, ath5k_calibrate, (unsigned long)sc);
 
 	ret = ath5k_eeprom_read_mac(ah, mac);
 	if (ret) {

@@ -2366,7 +2366,7 @@ ath5k_init(struct ath5k_softc *sc)
 	sc->curband = &sc->sbands[sc->curchan->band];
 	sc->imask = AR5K_INT_RXOK | AR5K_INT_RXERR | AR5K_INT_RXEOL |
 		AR5K_INT_RXORN | AR5K_INT_TXDESC | AR5K_INT_TXEOL |
-		AR5K_INT_FATAL | AR5K_INT_GLOBAL;
+		AR5K_INT_FATAL | AR5K_INT_GLOBAL | AR5K_INT_SWI;
 	ret = ath5k_reset(sc, NULL);
 	if (ret)
 		goto done;

@@ -2383,8 +2383,8 @@ ath5k_init(struct ath5k_softc *sc)
 	/* Set ack to be sent at low bit-rates */
 	ath5k_hw_set_ack_bitrate_high(ah, false);
 
-	mod_timer(&sc->calib_tim, round_jiffies(jiffies +
-			msecs_to_jiffies(ath5k_calinterval * 1000)));
+	/* Set PHY calibration timestamp and inteval */
+	ah->ah_cal_intval = ath5k_calinterval;
 
 	ret = 0;
 done:

@@ -2477,7 +2477,6 @@ ath5k_stop_hw(struct ath5k_softc *sc)
 	mmiowb();
 	mutex_unlock(&sc->lock);
 
-	del_timer_sync(&sc->calib_tim);
 	tasklet_kill(&sc->rxtq);
 	tasklet_kill(&sc->txtq);
 	tasklet_kill(&sc->restq);

@@ -2536,6 +2535,9 @@ ath5k_intr(int irq, void *dev_id)
 			if (status & AR5K_INT_BMISS) {
 				/* TODO */
 			}
+			if (status & AR5K_INT_SWI) {
+				tasklet_schedule(&sc->calib);
+			}
 			if (status & AR5K_INT_MIB) {
 				/*
 				 * These stats are also used for ANI i think

@@ -2552,6 +2554,8 @@ ath5k_intr(int irq, void *dev_id)
 	if (unlikely(!counter))
 		ATH5K_WARN(sc, "too many interrupts, giving up for now\n");
 
+	ath5k_hw_calibration_poll(ah);
+
 	return IRQ_HANDLED;
 }

@@ -2568,11 +2572,15 @@ ath5k_tasklet_reset(unsigned long data)
  * for temperature/environment changes.
  */
 static void
-ath5k_calibrate(unsigned long data)
+ath5k_tasklet_calibrate(unsigned long data)
 {
 	struct ath5k_softc *sc = (void *)data;
 	struct ath5k_hw *ah = sc->ah;
 
+	/* Only full calibration for now */
+	if (ah->ah_swi_mask != AR5K_SWI_FULL_CALIBRATION)
+		return;
+
 	ATH5K_DBG(sc, ATH5K_DEBUG_CALIBRATE, "channel %u/%x\n",
 		ieee80211_frequency_to_channel(sc->curchan->center_freq),
 		sc->curchan->hw_value);

@@ -2590,8 +2598,8 @@ ath5k_calibrate(unsigned long data)
 			ieee80211_frequency_to_channel(
 				sc->curchan->center_freq));
 
-	mod_timer(&sc->calib_tim, round_jiffies(jiffies +
-			msecs_to_jiffies(ath5k_calinterval * 1000)));
+	ah->ah_swi_mask = 0;
+
 }

drivers/net/wireless/ath/ath5k/base.h +2 -1

index 778e422..667bd9d 100644
@@ -177,6 +177,8 @@ struct ath5k_softc {
 
 	struct ath5k_rfkill	rf_kill;
 
+	struct tasklet_struct	calib;		/* calibration tasklet */
+
 	spinlock_t		block;		/* protects beacon */
 	struct tasklet_struct	beacontq;	/* beacon intr tasklet */
 	struct ath5k_buf	*bbuf;		/* beacon buffer */

@@ -187,7 +189,6 @@ struct ath5k_softc {
 	unsigned int		nexttbtt;	/* next beacon time in TU */
 	struct ath5k_txq	*cabq;		/* content after beacon */
 
-	struct timer_list	calib_tim;	/* calibration timer */
 	int 			power_level;	/* Requested tx power in dbm */
 	bool			assoc;		/* assocate state */
 	bool			enable_beacon;	/* true if beacons are on */

drivers/net/wireless/ath/ath5k/phy.c +20 -0

index 6afba98..d30bc3b 100644
@@ -1104,6 +1104,26 @@ int ath5k_hw_channel(struct ath5k_hw *ah, struct ieee80211_channel *channel)
   PHY calibration
 \*****************/
 
+void
+ath5k_hw_calibration_poll(struct ath5k_hw *ah)
+{
+	u32 current_time = (jiffies / HZ);
+	u32 cal_intval = ah->ah_cal_intval;
+
+	if (!ah->ah_cal_tstamp)
+		ah->ah_cal_tstamp = current_time;
+
+	/* For now we always do full calibration
+	 * Mark software interrupt mask and fire software
+	 * interrupt (bit gets auto-cleared) */
+	if ((current_time - ah->ah_cal_tstamp) >= cal_intval) {
+		ah->ah_cal_tstamp = current_time;
+		ah->ah_swi_mask = AR5K_SWI_FULL_CALIBRATION;
+		AR5K_REG_ENABLE_BITS(ah, AR5K_CR, AR5K_CR_SWI);
+	}
+
+}
+
 /**
  * ath5k_hw_noise_floor_calibration - perform PHY noise floor calibration
  *

Nick Kossifidis

author

2009-08-01 05:46 UTC #13 lore raw

In reply to Nick Kossifidis

* Get rid of calibration timer, instead use a software interrupt to schedule the calibration tasklet.

a) We don't need a timer for this, there is no need for accuracy even with round_jiffies i think this is a waste of resources. Also we don't need to run calibration if we are idle (no interrupts).

b) When we add ANI support we 'll just extend the poll function and calibration tasklet and handle all periodic phy calibration on one place (much cleaner).

v2: kill tasklet on stop_hw, stop/wake queues

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

---
 drivers/net/wireless/ath/ath5k/ath5k.h |   16 ++++++++++++++
 drivers/net/wireless/ath/ath5k/base.c  |   36 +++++++++++++++++++++++--------
 drivers/net/wireless/ath/ath5k/base.h  |    3 +-
 drivers/net/wireless/ath/ath5k/phy.c   |   20 +++++++++++++++++
 4 files changed, 64 insertions(+), 11 deletions(-)

drivers/net/wireless/ath/ath5k/ath5k.h +16 -0

index 1047a6c..76cf5b2 100644
@@ -919,6 +919,12 @@ enum ath5k_int {
 	AR5K_INT_NOCARD	= 0xffffffff
 };
 
+/* Software interrupts used for calibration */
+enum ath5k_software_interrupt {
+	AR5K_SWI_FULL_CALIBRATION = 0x01,
+	AR5K_SWI_SHORT_CALIBRATION = 0x02,
+};
+
 /*
  * Power management
  */

@@ -1123,6 +1129,15 @@ struct ath5k_hw {
 	/* noise floor from last periodic calibration */
 	s32			ah_noise_floor;
 
+	/* Calibration timestamp */
+	u32			ah_cal_tstamp;
+
+	/* Calibration interval (secs) */
+	u8			ah_cal_intval;
+
+	/* Software interrupt mask */
+	u8			ah_swi_mask;
+
 	/*
 	 * Function pointers
 	 */

@@ -1276,6 +1291,7 @@ extern int ath5k_hw_channel(struct ath5k_hw *ah, struct ieee80211_channel *chann
 /* PHY calibration */
 extern int ath5k_hw_phy_calibrate(struct ath5k_hw *ah, struct ieee80211_channel *channel);
 extern int ath5k_hw_noise_floor_calibration(struct ath5k_hw *ah, short freq);
+extern void ath5k_hw_calibration_poll(struct ath5k_hw *ah);
 /* Spur mitigation */
 bool ath5k_hw_chan_has_spur_noise(struct ath5k_hw *ah,
 				struct ieee80211_channel *channel);

drivers/net/wireless/ath/ath5k/base.c +26 ~~-10~~

index b64731b..d84ccad 100644
@@ -59,7 +59,7 @@
 #include "reg.h"
 #include "debug.h"
 
-static int ath5k_calinterval = 10; /* Calibrate PHY every 10 secs (TODO: Fixme) */
+static u8 ath5k_calinterval = 10; /* Calibrate PHY every 10 secs (TODO: Fixme) */
 static int modparam_nohwcrypt;
 module_param_named(nohwcrypt, modparam_nohwcrypt, bool, S_IRUGO);
 MODULE_PARM_DESC(nohwcrypt, "Disable hardware encryption.");

@@ -376,7 +376,7 @@ static int 	ath5k_stop_hw(struct ath5k_softc *sc);
 static irqreturn_t ath5k_intr(int irq, void *dev_id);
 static void 	ath5k_tasklet_reset(unsigned long data);
 
-static void 	ath5k_calibrate(unsigned long data);
+static void 	ath5k_tasklet_calibrate(unsigned long data);
 
 /*
  * Module init/exit functions

@@ -799,8 +799,8 @@ ath5k_attach(struct pci_dev *pdev, struct ieee80211_hw *hw)
 	tasklet_init(&sc->rxtq, ath5k_tasklet_rx, (unsigned long)sc);
 	tasklet_init(&sc->txtq, ath5k_tasklet_tx, (unsigned long)sc);
 	tasklet_init(&sc->restq, ath5k_tasklet_reset, (unsigned long)sc);
+	tasklet_init(&sc->calib, ath5k_tasklet_calibrate, (unsigned long)sc);
 	tasklet_init(&sc->beacontq, ath5k_tasklet_beacon, (unsigned long)sc);
-	setup_timer(&sc->calib_tim, ath5k_calibrate, (unsigned long)sc);
 
 	ret = ath5k_eeprom_read_mac(ah, mac);
 	if (ret) {

@@ -2366,7 +2366,7 @@ ath5k_init(struct ath5k_softc *sc)
 	sc->curband = &sc->sbands[sc->curchan->band];
 	sc->imask = AR5K_INT_RXOK | AR5K_INT_RXERR | AR5K_INT_RXEOL |
 		AR5K_INT_RXORN | AR5K_INT_TXDESC | AR5K_INT_TXEOL |
-		AR5K_INT_FATAL | AR5K_INT_GLOBAL;
+		AR5K_INT_FATAL | AR5K_INT_GLOBAL | AR5K_INT_SWI;
 	ret = ath5k_reset(sc, NULL);
 	if (ret)
 		goto done;

@@ -2383,8 +2383,8 @@ ath5k_init(struct ath5k_softc *sc)
 	/* Set ack to be sent at low bit-rates */
 	ath5k_hw_set_ack_bitrate_high(ah, false);
 
-	mod_timer(&sc->calib_tim, round_jiffies(jiffies +
-			msecs_to_jiffies(ath5k_calinterval * 1000)));
+	/* Set PHY calibration inteval */
+	ah->ah_cal_intval = ath5k_calinterval;
 
 	ret = 0;
 done:

@@ -2477,10 +2477,10 @@ ath5k_stop_hw(struct ath5k_softc *sc)
 	mmiowb();
 	mutex_unlock(&sc->lock);
 
-	del_timer_sync(&sc->calib_tim);
 	tasklet_kill(&sc->rxtq);
 	tasklet_kill(&sc->txtq);
 	tasklet_kill(&sc->restq);
+	tasklet_kill(&sc->calib);
 	tasklet_kill(&sc->beacontq);
 
 	ath5k_rfkill_hw_stop(sc->ah);

@@ -2536,6 +2536,9 @@ ath5k_intr(int irq, void *dev_id)
 			if (status & AR5K_INT_BMISS) {
 				/* TODO */
 			}
+			if (status & AR5K_INT_SWI) {
+				tasklet_schedule(&sc->calib);
+			}
 			if (status & AR5K_INT_MIB) {
 				/*
 				 * These stats are also used for ANI i think

@@ -2552,6 +2555,8 @@ ath5k_intr(int irq, void *dev_id)
 	if (unlikely(!counter))
 		ATH5K_WARN(sc, "too many interrupts, giving up for now\n");
 
+	ath5k_hw_calibration_poll(ah);
+
 	return IRQ_HANDLED;
 }

@@ -2568,11 +2573,19 @@ ath5k_tasklet_reset(unsigned long data)
  * for temperature/environment changes.
  */
 static void
-ath5k_calibrate(unsigned long data)
+ath5k_tasklet_calibrate(unsigned long data)
 {
 	struct ath5k_softc *sc = (void *)data;
 	struct ath5k_hw *ah = sc->ah;
 
+	/* Only full calibration for now */
+	if (ah->ah_swi_mask != AR5K_SWI_FULL_CALIBRATION)
+		return;
+
+	/* Stop queues so that calibration
+	 * doesn't interfere with tx */
+	ieee80211_stop_queues(sc->hw);
+
 	ATH5K_DBG(sc, ATH5K_DEBUG_CALIBRATE, "channel %u/%x\n",
 		ieee80211_frequency_to_channel(sc->curchan->center_freq),
 		sc->curchan->hw_value);

@@ -2590,8 +2603,11 @@ ath5k_calibrate(unsigned long data)
 			ieee80211_frequency_to_channel(
 				sc->curchan->center_freq));
 
-	mod_timer(&sc->calib_tim, round_jiffies(jiffies +
-			msecs_to_jiffies(ath5k_calinterval * 1000)));
+	ah->ah_swi_mask = 0;
+
+	/* Wake queues */
+	ieee80211_wake_queues(sc->hw);
+
 }

drivers/net/wireless/ath/ath5k/base.h +2 -1

index 778e422..667bd9d 100644
@@ -177,6 +177,8 @@ struct ath5k_softc {
 
 	struct ath5k_rfkill	rf_kill;
 
+	struct tasklet_struct	calib;		/* calibration tasklet */
+
 	spinlock_t		block;		/* protects beacon */
 	struct tasklet_struct	beacontq;	/* beacon intr tasklet */
 	struct ath5k_buf	*bbuf;		/* beacon buffer */

@@ -187,7 +189,6 @@ struct ath5k_softc {
 	unsigned int		nexttbtt;	/* next beacon time in TU */
 	struct ath5k_txq	*cabq;		/* content after beacon */
 
-	struct timer_list	calib_tim;	/* calibration timer */
 	int 			power_level;	/* Requested tx power in dbm */
 	bool			assoc;		/* assocate state */
 	bool			enable_beacon;	/* true if beacons are on */

drivers/net/wireless/ath/ath5k/phy.c +20 -0

index 6afba98..d30bc3b 100644
@@ -1104,6 +1104,26 @@ int ath5k_hw_channel(struct ath5k_hw *ah, struct ieee80211_channel *channel)
   PHY calibration
 \*****************/
 
+void
+ath5k_hw_calibration_poll(struct ath5k_hw *ah)
+{
+	u32 current_time = (jiffies / HZ);
+	u32 cal_intval = ah->ah_cal_intval;
+
+	if (!ah->ah_cal_tstamp)
+		ah->ah_cal_tstamp = current_time;
+
+	/* For now we always do full calibration
+	 * Mark software interrupt mask and fire software
+	 * interrupt (bit gets auto-cleared) */
+	if ((current_time - ah->ah_cal_tstamp) >= cal_intval) {
+		ah->ah_cal_tstamp = current_time;
+		ah->ah_swi_mask = AR5K_SWI_FULL_CALIBRATION;
+		AR5K_REG_ENABLE_BITS(ah, AR5K_CR, AR5K_CR_SWI);
+	}
+
+}
+
 /**
  * ath5k_hw_noise_floor_calibration - perform PHY noise floor calibration
  *

Jiri Slaby

2009-08-01 08:19 UTC #14 lore raw

drivers/net/wireless/ath/ath5k/phy.c int ath5k_hw_channel(struct ath5k_hw *ah, struct ieee80211_channel *channel) · line 1110

Quoted 10 lines · Nick Kossifidis

drivers/net/wireless/ath/ath5k/phy.c +4 -0

@@ -1104,6 +1104,26 @@ int ath5k_hw_channel(struct ath5k_hw *ah, struct ieee80211_channel *channel)
   PHY calibration
 \*****************/
 
+void
+ath5k_hw_calibration_poll(struct ath5k_hw *ah)
+{
+	u32 current_time = (jiffies / HZ);

jiffies are long. And they start from negative to catch such issues. You were lucky and/or tested after 5 minutes of uptime ;).

drivers/net/wireless/ath/ath5k/phy.c int ath5k_hw_channel(struct ath5k_hw *ah, struct ieee80211_channel *channel) · line 1119

Quoted 9 lines

drivers/net/wireless/ath/ath5k/phy.c +9 -0

+	u32 cal_intval = ah->ah_cal_intval;
+
+	if (!ah->ah_cal_tstamp)
+		ah->ah_cal_tstamp = current_time;
+
+	/* For now we always do full calibration
+	 * Mark software interrupt mask and fire software
+	 * interrupt (bit gets auto-cleared) */
+	if ((current_time - ah->ah_cal_tstamp) >= cal_intval) {

Aiee, this should be converted to time_after(). You don't count with a wrap here. (The same as above.)

Quoted 6 lines

drivers/net/wireless/ath/ath5k/phy.c +6 -0

+		ah->ah_cal_tstamp = current_time;
+		ah->ah_swi_mask = AR5K_SWI_FULL_CALIBRATION;
+		AR5K_REG_ENABLE_BITS(ah, AR5K_CR, AR5K_CR_SWI);
+	}
+
+}

Johannes Berg

2009-08-01 08:22 UTC #15 lore raw

Quoted 4 lines · Jiri Slaby

drivers/net/wireless/ath/ath5k/phy.c int ath5k_hw_channel(struct ath5k_hw *ah, struct ieee80211_channel *channel) · line 1110
Quoted 1 line
drivers/net/wireless/ath/ath5k/phy.c +1 -0
+	u32 current_time = (jiffies / HZ);
jiffies are long. And they start from negative to catch such issues. You were lucky and/or tested after 5 minutes of uptime ;).

Actually, jiffies are unsigned long, but wrap after 5 minutes of uptime :)

johannes

Jiri Slaby

2009-08-01 08:24 UTC #16 lore raw

Quoted 1 line · Johannes Berg

Actually, jiffies are unsigned long

Yeah, this is what I meant. I agree I messed it up when I didn't write the unsigned explicitly. It might have been confusing, thanks for pointing out.

Johannes Berg

2009-08-01 08:28 UTC #17 lore raw

Quoted 6 lines · Jiri Slaby

Quoted 1 line · Johannes Berg
Actually, jiffies are unsigned long
Yeah, this is what I meant. I agree I messed it up when I didn't write the unsigned explicitly. It might have been confusing, thanks for pointing out.

And, in addition to what you said, the code shouldn't divide jiffies, it should multiply the timeout, add it to the start time, and then use time_is_after_jiffies() or so.

johannes

Nick Kossifidis

maintainer

2009-08-01 08:31 UTC #18 lore raw

2009/8/1 Johannes Berg <[email protected]>:

Quoted 8 lines · Jiri Slaby

Quoted 6 lines · Johannes Berg
Quoted 1 line
Actually, jiffies are unsigned long
Yeah, this is what I meant. I agree I messed it up when I didn't write the unsigned explicitly. It might have been confusing, thanks for pointing out.

Quoted 6 lines

And, in addition to what you said, the code shouldn't divide jiffies, it should multiply the timeout, add it to the start time, and then use time_is_after_jiffies() or so.
johannes

Where can i find documentation on this ?

Signature · 3 lines

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

Jiri Slaby

2009-08-01 08:35 UTC #19 lore raw

Quoted 2 lines · Nick Kossifidis

2009/8/1 Johannes Berg <[email protected]>:
Quoted 1 line
time_is_after_jiffies() or so.

...

Quoted 1 line

Where can i find documentation on this ?

I wouldn't say there is anything else than include/linux/jiffies.h.

Nick Kossifidis

maintainer

2009-08-03 17:54 UTC #20 lore raw

In reply to Johannes Berg

2009/8/1 Johannes Berg <[email protected]>:

Quoted 8 lines · Jiri Slaby

Quoted 6 lines · Johannes Berg
Quoted 1 line
Actually, jiffies are unsigned long
Yeah, this is what I meant. I agree I messed it up when I didn't write the unsigned explicitly. It might have been confusing, thanks for pointing out.

Quoted 5 lines

And, in addition to what you said, the code shouldn't divide jiffies, it should multiply the timeout, add it to the start time, and then use time_is_after_jiffies() or so.
johannes

ACK will resend asap ;-)

Signature · 3 lines

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

John W. Linville

2009-08-07 15:01 UTC #21 lore raw

Quoted 9 lines · Nick Kossifidis

2009/8/1 Johannes Berg <[email protected]>:
Quoted 8 lines · Jiri Slaby
Quoted 6 lines · Johannes Berg
Quoted 1 line
Actually, jiffies are unsigned long
Yeah, this is what I meant. I agree I messed it up when I didn't write the unsigned explicitly. It might have been confusing, thanks for pointing out.

Quoted 7 lines

Quoted 5 lines
And, in addition to what you said, the code shouldn't divide jiffies, it should multiply the timeout, add it to the start time, and then use time_is_after_jiffies() or so.
johannes
ACK will resend asap ;-)

Last I saw in this thread...

Can we have a clean repost of the whole series (including proper Signed-off-by and any Acked-by, Tested-by, etc)? Also, in the future I think it would be good for you to take some of the comments from Luis to heart -- explain the 'why' not the 'what' in the changelogs, do one thing per patch, etc.

I'm going to drop the original 1-4 now and wait for a clean repost.

Thanks,

John

Signature · 2 lines

John W. Linville		Someday the world will need a hero, and you
[email protected]			might be all we have.  Be ready.

Nick Kossifidis

maintainer

2009-08-07 19:50 UTC #22 lore raw

2009/8/7 John W. Linville <[email protected]>:

Quoted 3 lines

Last I saw in this thread...

Yup, sorry for the delay ;-(

Quoted 2 lines

Can we have a clean repost of the whole series (including proper Signed-off-by and any Acked-by, Tested-by, etc)?

sure

Quoted 4 lines

Also, in the future
I think it would be good for you to take some of the comments from Luis to heart -- explain the 'why' not the 'what' in the changelogs, do one thing per patch, etc.

I'll try and make them more complete from now on ;-)

Quoted 2 lines

I'm going to drop the original 1-4 now and wait for a clean repost.

ack ;-)

Signature · 3 lines

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