Message ID | 20090801054649.GA8390@makis (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
On 08/01/2009 07:46 AM, Nick Kossifidis wrote: > --- a/drivers/net/wireless/ath/ath5k/phy.c > +++ b/drivers/net/wireless/ath/ath5k/phy.c > @@ -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 ;). > + 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.) > + ah->ah_cal_tstamp = current_time; > + ah->ah_swi_mask = AR5K_SWI_FULL_CALIBRATION; > + AR5K_REG_ENABLE_BITS(ah, AR5K_CR, AR5K_CR_SWI); > + } > + > +} -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2009-08-01 at 10:19 +0200, Jiri Slaby wrote: > > + 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
On 08/01/2009 10:21 AM, Johannes Berg wrote:
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2009-08-01 at 10:24 +0200, Jiri Slaby wrote: > On 08/01/2009 10:21 AM, Johannes Berg wrote: > > 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
2009/8/1 Johannes Berg <johannes@sipsolutions.net>: > On Sat, 2009-08-01 at 10:24 +0200, Jiri Slaby wrote: >> On 08/01/2009 10:21 AM, Johannes Berg wrote: >> > 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 > Where can i find documentation on this ?
On 08/01/2009 10:31 AM, Nick Kossifidis wrote: > 2009/8/1 Johannes Berg <johannes@sipsolutions.net>: >> time_is_after_jiffies() or so. ... > Where can i find documentation on this ? I wouldn't say there is anything else than include/linux/jiffies.h. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2009/8/1 Johannes Berg <johannes@sipsolutions.net>: > On Sat, 2009-08-01 at 10:24 +0200, Jiri Slaby wrote: >> On 08/01/2009 10:21 AM, Johannes Berg wrote: >> > 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 ACK will resend asap ;-)
On Mon, Aug 03, 2009 at 08:54:47PM +0300, Nick Kossifidis wrote: > 2009/8/1 Johannes Berg <johannes@sipsolutions.net>: > > On Sat, 2009-08-01 at 10:24 +0200, Jiri Slaby wrote: > >> On 08/01/2009 10:21 AM, Johannes Berg wrote: > >> > 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 > > 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
2009/8/7 John W. Linville <linville@tuxdriver.com>: > > Last I saw in this thread... > Yup, sorry for the delay ;-( > Can we have a clean repost of the whole series (including proper > Signed-off-by and any Acked-by, Tested-by, etc)? sure > 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 ;-) > I'm going to drop the original 1-4 now and wait for a clean repost. > ack ;-)
diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h index 1047a6c..76cf5b2 100644 --- a/drivers/net/wireless/ath/ath5k/ath5k.h +++ b/drivers/net/wireless/ath/ath5k/ath5k.h @@ -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); diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c index b64731b..d84ccad 100644 --- a/drivers/net/wireless/ath/ath5k/base.c +++ b/drivers/net/wireless/ath/ath5k/base.c @@ -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); + } diff --git a/drivers/net/wireless/ath/ath5k/base.h b/drivers/net/wireless/ath/ath5k/base.h index 778e422..667bd9d 100644 --- a/drivers/net/wireless/ath/ath5k/base.h +++ b/drivers/net/wireless/ath/ath5k/base.h @@ -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 */ diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c index 6afba98..d30bc3b 100644 --- a/drivers/net/wireless/ath/ath5k/phy.c +++ b/drivers/net/wireless/ath/ath5k/phy.c @@ -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 *
* 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). 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). v2: kill tasklet on stop_hw, stop/wake queues Signed-off-by: Nick Kossifidis <mickflemm@gmail.com> --- 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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html