ath5k: Switch from read-and-clear to write-to-clear method when handling PISR/SISR registers

Since card has 12 tx queues and we want to keep track of the interrupts
per queue we can't fit all these interrupt bits on a single register.
So we have 5 registers, the primary interrupt status register (PISR) and
the 4 secondary interupt status registers (SISRs).

In order to be able to read them all at once (atomic operation) Atheros
introduced the Read-And-Clear registers to make things easier. So when
reading RAC_PISR register, hw does a read on PISR and all SISRs, returns
the value of PISR, copies all SISR values to their shadow copies (RAC_SISRx)
and clears PISR and SISRs. This saves us from reading PISR/SISRs in a sequence.

So far we 've used this approach and MadWiFi/Windows driver etc also used it
for years.

It turns out this operation is not atomic after all (at least not on all cards)
That means it's possible to loose some interrupts because they came after the
copy step and hw cleared them on the clean step !

That's probably the reason we got missed beacons, got stuck queues etc and
couldn't figure out what was going on.

With this patch we switch from RaC operation to an alternative method (that
makes more sense IMHO anyway, I just chose to be on the safe side so far).
Instead of reading RAC registers, we read the normal PISR/SISR registers and
clear any bits we got by writing them back on the register. This will clear only
the bits we got on our read step and leave any new bits unaffected (at least
that's what docs say). So if any new interrupts come up we won't miss it.

I've tested this with an AR5213 and an AR2425 and it seems O.K.

Many thanks to Adrian Chadd for debuging this and reviewing the patch !

v2: Make sure we don't clear PISR bits that map to SISR generated interrupts
(added a comment on the code for this)

Signed-off-by: Nick Kossifidis <mickflemm@gmail.com>
Signed-off-by: John W. Linville <linville@tuxdriver.com>

1 files changed, 160 insertions, 85 deletions

diff --git a/drivers/net/wireless/ath/ath5k/dma.c b/drivers/net/wireless/ath/ath5k/dma.c
index 2481f9c7f4b..b5db6e78f88 100644
--- a/drivers/net/wireless/ath/ath5k/dma.c
+++ b/drivers/net/wireless/ath/ath5k/dma.c
@@ -450,7 +450,6 @@ int ath5k_hw_set_txdp(struct ath5k_hw *ah, unsigned int queue, u32 phys_addr)
  *
  * XXX: Link this with tx DMA size ?
  * XXX: Use it to save interrupts ?
- * TODO: Needs testing, i think it's related to bmiss...
  */
 int ath5k_hw_update_tx_triglevel(struct ath5k_hw *ah, bool increase)
 {
@@ -523,62 +522,161 @@ bool ath5k_hw_is_intr_pending(struct ath5k_hw *ah)
  * being mapped on some standard non hw-specific positions
  * (check out &ath5k_int).
  *
- * NOTE: We use read-and-clear register, so after this function is called ISR
- * is zeroed.
+ * NOTE: We do write-to-clear, so the active PISR/SISR bits at the time this
+ * function gets called are cleared on return.
  */
 int ath5k_hw_get_isr(struct ath5k_hw *ah, enum ath5k_int *interrupt_mask)
 {
-	u32 data;
+	u32 data = 0;
 
 	/*
-	 * Read interrupt status from the Interrupt Status register
-	 * on 5210
+	 * Read interrupt status from Primary Interrupt
+	 * Register.
+	 *
+	 * Note: PISR/SISR Not available on 5210
 	 */
 	if (ah->ah_version == AR5K_AR5210) {
-		data = ath5k_hw_reg_read(ah, AR5K_ISR);
-		if (unlikely(data == AR5K_INT_NOCARD)) {
-			*interrupt_mask = data;
+		u32 isr = 0;
+		isr = ath5k_hw_reg_read(ah, AR5K_ISR);
+		if (unlikely(isr == AR5K_INT_NOCARD)) {
+			*interrupt_mask = isr;
 			return -ENODEV;
 		}
-	} else {
+
 		/*
-		 * Read interrupt status from Interrupt
-		 * Status Register shadow copy (Read And Clear)
-		 *
-		 * Note: PISR/SISR Not available on 5210
+		 * Filter out the non-common bits from the interrupt
+		 * status.
 		 */
-		data = ath5k_hw_reg_read(ah, AR5K_RAC_PISR);
-		if (unlikely(data == AR5K_INT_NOCARD)) {
-			*interrupt_mask = data;
+		*interrupt_mask = (isr & AR5K_INT_COMMON) & ah->ah_imr;
+
+		/* Hanlde INT_FATAL */
+		if (unlikely(isr & (AR5K_ISR_SSERR | AR5K_ISR_MCABT
+						| AR5K_ISR_DPERR)))
+			*interrupt_mask |= AR5K_INT_FATAL;
+
+		/*
+		 * XXX: BMISS interrupts may occur after association.
+		 * I found this on 5210 code but it needs testing. If this is
+		 * true we should disable them before assoc and re-enable them
+		 * after a successful assoc + some jiffies.
+			interrupt_mask &= ~AR5K_INT_BMISS;
+		 */
+
+		data = isr;
+	} else {
+		u32 pisr = 0;
+		u32 pisr_clear = 0;
+		u32 sisr0 = 0;
+		u32 sisr1 = 0;
+		u32 sisr2 = 0;
+		u32 sisr3 = 0;
+		u32 sisr4 = 0;
+
+		/* Read PISR and SISRs... */
+		pisr = ath5k_hw_reg_read(ah, AR5K_PISR);
+		if (unlikely(pisr == AR5K_INT_NOCARD)) {
+			*interrupt_mask = pisr;
 			return -ENODEV;
 		}
-	}
 
-	/*
-	 * Get abstract interrupt mask (driver-compatible)
-	 */
-	*interrupt_mask = (data & AR5K_INT_COMMON) & ah->ah_imr;
+		sisr0 = ath5k_hw_reg_read(ah, AR5K_SISR0);
+		sisr1 = ath5k_hw_reg_read(ah, AR5K_SISR1);
+		sisr2 = ath5k_hw_reg_read(ah, AR5K_SISR2);
+		sisr3 = ath5k_hw_reg_read(ah, AR5K_SISR3);
+		sisr4 = ath5k_hw_reg_read(ah, AR5K_SISR4);
 
-	if (ah->ah_version != AR5K_AR5210) {
-		u32 sisr2 = ath5k_hw_reg_read(ah, AR5K_RAC_SISR2);
+		/*
+		 * PISR holds the logical OR of interrupt bits
+		 * from SISR registers:
+		 *
+		 * TXOK and TXDESC  -> Logical OR of TXOK and TXDESC
+		 *			per-queue bits on SISR0
+		 *
+		 * TXERR and TXEOL -> Logical OR of TXERR and TXEOL
+		 *			per-queue bits on SISR1
+		 *
+		 * TXURN -> Logical OR of TXURN per-queue bits on SISR2
+		 *
+		 * HIUERR -> Logical OR of MCABT, SSERR and DPER bits on SISR2
+		 *
+		 * BCNMISC -> Logical OR of TIM, CAB_END, DTIM_SYNC
+		 *		BCN_TIMEOUT, CAB_TIMEOUT and DTIM
+		 *		(and TSFOOR ?) bits on SISR2
+		 *
+		 * QCBRORN and QCBRURN -> Logical OR of QCBRORN and
+		 *			QCBRURN per-queue bits on SISR3
+		 * QTRIG -> Logical OR of QTRIG per-queue bits on SISR4
+		 *
+		 * If we clean these bits on PISR we 'll also clear all
+		 * related bits from SISRs, e.g. if we write the TXOK bit on
+		 * PISR we 'll clean all TXOK bits from SISR0 so if a new TXOK
+		 * interrupt got fired for another queue while we were reading
+		 * the interrupt registers and we write back the TXOK bit on
+		 * PISR we 'll lose it. So make sure that we don't write back
+		 * on PISR any bits that come from SISRs. Clearing them from
+		 * SISRs will also clear PISR so no need to worry here.
+		 */
 
-		/*HIU = Host Interface Unit (PCI etc)*/
-		if (unlikely(data & (AR5K_ISR_HIUERR)))
-			*interrupt_mask |= AR5K_INT_FATAL;
+		pisr_clear = pisr & ~AR5K_ISR_BITS_FROM_SISRS;
 
-		/*Beacon Not Ready*/
-		if (unlikely(data & (AR5K_ISR_BNR)))
-			*interrupt_mask |= AR5K_INT_BNR;
+		/*
+		 * Write to clear them...
+		 * Note: This means that each bit we write back
+		 * to the registers will get cleared, leaving the
+		 * rest unaffected. So this won't affect new interrupts
+		 * we didn't catch while reading/processing, we 'll get
+		 * them next time get_isr gets called.
+		 */
+		ath5k_hw_reg_write(ah, sisr0, AR5K_SISR0);
+		ath5k_hw_reg_write(ah, sisr1, AR5K_SISR1);
+		ath5k_hw_reg_write(ah, sisr2, AR5K_SISR2);
+		ath5k_hw_reg_write(ah, sisr3, AR5K_SISR3);
+		ath5k_hw_reg_write(ah, sisr4, AR5K_SISR4);
+		ath5k_hw_reg_write(ah, pisr_clear, AR5K_PISR);
+		/* Flush previous write */
+		ath5k_hw_reg_read(ah, AR5K_PISR);
 
-		if (unlikely(sisr2 & (AR5K_SISR2_SSERR |
-					AR5K_SISR2_DPERR |
-					AR5K_SISR2_MCABT)))
-			*interrupt_mask |= AR5K_INT_FATAL;
+		/*
+		 * Filter out the non-common bits from the interrupt
+		 * status.
+		 */
+		*interrupt_mask = (pisr & AR5K_INT_COMMON) & ah->ah_imr;
+
+
+		/* We treat TXOK,TXDESC, TXERR and TXEOL
+		 * the same way (schedule the tx tasklet)
+		 * so we track them all together per queue */
+		if (pisr & AR5K_ISR_TXOK)
+			ah->ah_txq_isr_txok_all |= AR5K_REG_MS(sisr0,
+						AR5K_SISR0_QCU_TXOK);
+
+		if (pisr & AR5K_ISR_TXDESC)
+			ah->ah_txq_isr_txok_all |= AR5K_REG_MS(sisr0,
+						AR5K_SISR0_QCU_TXDESC);
 
-		if (data & AR5K_ISR_TIM)
+		if (pisr & AR5K_ISR_TXERR)
+			ah->ah_txq_isr_txok_all |= AR5K_REG_MS(sisr1,
+						AR5K_SISR1_QCU_TXERR);
+
+		if (pisr & AR5K_ISR_TXEOL)
+			ah->ah_txq_isr_txok_all |= AR5K_REG_MS(sisr1,
+						AR5K_SISR1_QCU_TXEOL);
+
+		/* Currently this is not much usefull since we treat
+		 * all queues the same way if we get a TXURN (update
+		 * tx trigger level) but we might need it later on*/
+		if (pisr & AR5K_ISR_TXURN)
+			ah->ah_txq_isr_txurn |= AR5K_REG_MS(sisr2,
+						AR5K_SISR2_QCU_TXURN);
+
+		/* Misc Beacon related interrupts */
+
+		/* For AR5211 */
+		if (pisr & AR5K_ISR_TIM)
 			*interrupt_mask |= AR5K_INT_TIM;
 
-		if (data & AR5K_ISR_BCNMISC) {
+		/* For AR5212+ */
+		if (pisr & AR5K_ISR_BCNMISC) {
 			if (sisr2 & AR5K_SISR2_TIM)
 				*interrupt_mask |= AR5K_INT_TIM;
 			if (sisr2 & AR5K_SISR2_DTIM)
@@ -591,63 +689,40 @@ int ath5k_hw_get_isr(struct ath5k_hw *ah, enum ath5k_int *interrupt_mask)
 				*interrupt_mask |= AR5K_INT_CAB_TIMEOUT;
 		}
 
-		if (data & AR5K_ISR_RXDOPPLER)
+		/* Below interrupts are unlikely to happen */
+
+		/* HIU = Host Interface Unit (PCI etc)
+		 * Can be one of MCABT, SSERR, DPERR from SISR2 */
+		if (unlikely(pisr & (AR5K_ISR_HIUERR)))
+			*interrupt_mask |= AR5K_INT_FATAL;
+
+
+		/*Beacon Not Ready*/
+		if (unlikely(pisr & (AR5K_ISR_BNR)))
+			*interrupt_mask |= AR5K_INT_BNR;
+
+		if (unlikely(pisr & (AR5K_ISR_RXDOPPLER)))
 			*interrupt_mask |= AR5K_INT_RX_DOPPLER;
-		if (data & AR5K_ISR_QCBRORN) {
+
+		if (unlikely(pisr & (AR5K_ISR_QCBRORN))) {
 			*interrupt_mask |= AR5K_INT_QCBRORN;
-			ah->ah_txq_isr |= AR5K_REG_MS(
-					ath5k_hw_reg_read(ah, AR5K_RAC_SISR3),
-					AR5K_SISR3_QCBRORN);
+			ah->ah_txq_isr_qcborn |= AR5K_REG_MS(sisr3,
+						AR5K_SISR3_QCBRORN);
 		}
-		if (data & AR5K_ISR_QCBRURN) {
+
+		if (unlikely(pisr & (AR5K_ISR_QCBRURN))) {
 			*interrupt_mask |= AR5K_INT_QCBRURN;
-			ah->ah_txq_isr |= AR5K_REG_MS(
-					ath5k_hw_reg_read(ah, AR5K_RAC_SISR3),
-					AR5K_SISR3_QCBRURN);
+			ah->ah_txq_isr_qcburn |= AR5K_REG_MS(sisr3,
+						AR5K_SISR3_QCBRURN);
 		}
-		if (data & AR5K_ISR_QTRIG) {
+
+		if (unlikely(pisr & (AR5K_ISR_QTRIG))) {
 			*interrupt_mask |= AR5K_INT_QTRIG;
-			ah->ah_txq_isr |= AR5K_REG_MS(
-					ath5k_hw_reg_read(ah, AR5K_RAC_SISR4),
-					AR5K_SISR4_QTRIG);
+			ah->ah_txq_isr_qtrig |= AR5K_REG_MS(sisr4,
+						AR5K_SISR4_QTRIG);
 		}
 
-		if (data & AR5K_ISR_TXOK)
-			ah->ah_txq_isr |= AR5K_REG_MS(
-					ath5k_hw_reg_read(ah, AR5K_RAC_SISR0),
-					AR5K_SISR0_QCU_TXOK);
-
-		if (data & AR5K_ISR_TXDESC)
-			ah->ah_txq_isr |= AR5K_REG_MS(
-					ath5k_hw_reg_read(ah, AR5K_RAC_SISR0),
-					AR5K_SISR0_QCU_TXDESC);
-
-		if (data & AR5K_ISR_TXERR)
-			ah->ah_txq_isr |= AR5K_REG_MS(
-					ath5k_hw_reg_read(ah, AR5K_RAC_SISR1),
-					AR5K_SISR1_QCU_TXERR);
-
-		if (data & AR5K_ISR_TXEOL)
-			ah->ah_txq_isr |= AR5K_REG_MS(
-					ath5k_hw_reg_read(ah, AR5K_RAC_SISR1),
-					AR5K_SISR1_QCU_TXEOL);
-
-		if (data & AR5K_ISR_TXURN)
-			ah->ah_txq_isr |= AR5K_REG_MS(
-					ath5k_hw_reg_read(ah, AR5K_RAC_SISR2),
-					AR5K_SISR2_QCU_TXURN);
-	} else {
-		if (unlikely(data & (AR5K_ISR_SSERR | AR5K_ISR_MCABT
-				| AR5K_ISR_HIUERR | AR5K_ISR_DPERR)))
-			*interrupt_mask |= AR5K_INT_FATAL;
-
-		/*
-		 * XXX: BMISS interrupts may occur after association.
-		 * I found this on 5210 code but it needs testing. If this is
-		 * true we should disable them before assoc and re-enable them
-		 * after a successful assoc + some jiffies.
-			interrupt_mask &= ~AR5K_INT_BMISS;
-		 */
+		data = pisr;
 	}
 
 	/*