[PATCH v2] ath9k: add MSI support

[PATCH v2] ath9k: add MSI support

[Russell Hu]

On new Intel platforms like ApolloLake, legacy interrupt mechanism
(INTx) is not supported, so WLAN modules are not working because
interrupts are missing, therefore this patch is to add MSI support to
ath9k.  With module paremeter "use_msi=1", ath9k driver would try to
use MSI instead of INTx.

Signed-off-by: Russell Hu <rhu@qti.qualcomm.com>
---
 drivers/net/wireless/ath/ath9k/hw.c   | 33 ++++++++++++++++++------
 drivers/net/wireless/ath/ath9k/hw.h   |  3 +++
 drivers/net/wireless/ath/ath9k/init.c |  4 +++
 drivers/net/wireless/ath/ath9k/mac.c  | 47 +++++++++++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath9k/pci.c  | 21 +++++++++++++++-
 drivers/net/wireless/ath/ath9k/reg.h  | 15 +++++++++++
 6 files changed, 115 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 8c5c2dd8fa7f..cd0f023ccf77 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -922,6 +922,7 @@ static void ath9k_hw_init_interrupt_masks(struct ath_hw *ah,
 		AR_IMR_RXERR |
 		AR_IMR_RXORN |
 		AR_IMR_BCNMISC;
+	u32 msi_cfg = 0;
 
 	if (AR_SREV_9340(ah) || AR_SREV_9550(ah) || AR_SREV_9531(ah) ||
 	    AR_SREV_9561(ah))
@@ -929,22 +930,30 @@ static void ath9k_hw_init_interrupt_masks(struct ath_hw *ah,
 
 	if (AR_SREV_9300_20_OR_LATER(ah)) {
 		imr_reg |= AR_IMR_RXOK_HP;
-		if (ah->config.rx_intr_mitigation)
+		if (ah->config.rx_intr_mitigation) {
 			imr_reg |= AR_IMR_RXINTM | AR_IMR_RXMINTR;
-		else
+			msi_cfg |= AR_INTCFG_MSI_RXINTM | AR_INTCFG_MSI_RXMINTR;
+		} else {
 			imr_reg |= AR_IMR_RXOK_LP;
-
+			msi_cfg |= AR_INTCFG_MSI_RXOK;
+		}
 	} else {
-		if (ah->config.rx_intr_mitigation)
+		if (ah->config.rx_intr_mitigation) {
 			imr_reg |= AR_IMR_RXINTM | AR_IMR_RXMINTR;
-		else
+			msi_cfg |= AR_INTCFG_MSI_RXINTM | AR_INTCFG_MSI_RXMINTR;
+		} else {
 			imr_reg |= AR_IMR_RXOK;
+			msi_cfg |= AR_INTCFG_MSI_RXOK;
+		}
 	}
 
-	if (ah->config.tx_intr_mitigation)
+	if (ah->config.tx_intr_mitigation) {
 		imr_reg |= AR_IMR_TXINTM | AR_IMR_TXMINTR;
-	else
+		msi_cfg |= AR_INTCFG_MSI_TXINTM | AR_INTCFG_MSI_TXMINTR;
+	} else {
 		imr_reg |= AR_IMR_TXOK;
+		msi_cfg |= AR_INTCFG_MSI_TXOK;
+	}
 
 	ENABLE_REGWRITE_BUFFER(ah);
 
@@ -952,6 +961,16 @@ static void ath9k_hw_init_interrupt_masks(struct ath_hw *ah,
 	ah->imrs2_reg |= AR_IMR_S2_GTT;
 	REG_WRITE(ah, AR_IMR_S2, ah->imrs2_reg);
 
+	if (ah->msi_enabled) {
+		ah->msi_reg = REG_READ(ah, AR_PCIE_MSI);
+		ah->msi_reg |= AR_PCIE_MSI_HW_DBI_WR_EN;
+		ah->msi_reg &= AR_PCIE_MSI_HW_INT_PENDING_ADDR_MSI_64;
+		REG_WRITE(ah, AR_INTCFG, msi_cfg);
+		ath_dbg(ath9k_hw_common(ah), ANY,
+			"value of AR_INTCFG=0x%X, msi_cfg=0x%X\n",
+			REG_READ(ah, AR_INTCFG), msi_cfg);
+	}
+
 	if (!AR_SREV_9100(ah)) {
 		REG_WRITE(ah, AR_INTR_SYNC_CAUSE, 0xFFFFFFFF);
 		REG_WRITE(ah, AR_INTR_SYNC_ENABLE, sync_default);
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 4ac70827d142..0d6c07c77372 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -977,6 +977,9 @@ struct ath_hw {
 	bool tpc_enabled;
 	u8 tx_power[Ar5416RateSize];
 	u8 tx_power_stbc[Ar5416RateSize];
+	bool msi_enabled;
+	u32 msi_mask;
+	u32 msi_reg;
 };
 
 struct ath_bus_ops {
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index bb7936090b91..b6b7a353fea4 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -75,6 +75,10 @@ MODULE_PARM_DESC(use_chanctx, "Enable channel context for concurrency");
 
 #endif /* CONFIG_ATH9K_CHANNEL_CONTEXT */
 
+int ath9k_use_msi;
+module_param_named(use_msi, ath9k_use_msi, int, 0444);
+MODULE_PARM_DESC(use_msi, "Use MSI instead of INTx if possible");
+
 bool is_ath9k_unloaded;
 
 #ifdef CONFIG_MAC80211_LEDS
diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
index 77c94f9e7b61..58d02c19b6d0 100644
--- a/drivers/net/wireless/ath/ath9k/mac.c
+++ b/drivers/net/wireless/ath/ath9k/mac.c
@@ -832,6 +832,43 @@ static void __ath9k_hw_enable_interrupts(struct ath_hw *ah)
 	}
 	ath_dbg(common, INTERRUPT, "AR_IMR 0x%x IER 0x%x\n",
 		REG_READ(ah, AR_IMR), REG_READ(ah, AR_IER));
+
+	if (ah->msi_enabled) {
+		u32 _msi_reg = 0;
+		u32 i = 0;
+		u32 msi_pend_addr_mask = AR_PCIE_MSI_HW_INT_PENDING_ADDR_MSI_64;
+
+		ath_dbg(ath9k_hw_common(ah), INTERRUPT,
+			"Enabling MSI, msi_mask=0x%X\n", ah->msi_mask);
+
+		REG_WRITE(ah, AR_INTR_PRIO_ASYNC_ENABLE, ah->msi_mask);
+		REG_WRITE(ah, AR_INTR_PRIO_ASYNC_MASK, ah->msi_mask);
+		ath_dbg(ath9k_hw_common(ah), INTERRUPT,
+			"AR_INTR_PRIO_ASYNC_ENABLE=0x%X, AR_INTR_PRIO_ASYNC_MASK=0x%X\n",
+			REG_READ(ah, AR_INTR_PRIO_ASYNC_ENABLE),
+			REG_READ(ah, AR_INTR_PRIO_ASYNC_MASK));
+
+		if (ah->msi_reg == 0)
+			ah->msi_reg = REG_READ(ah, AR_PCIE_MSI);
+
+		ath_dbg(ath9k_hw_common(ah), INTERRUPT,
+			"AR_PCIE_MSI=0x%X, ah->msi_reg = 0x%X\n",
+			AR_PCIE_MSI, ah->msi_reg);
+
+		i = 0;
+		do {
+			REG_WRITE(ah, AR_PCIE_MSI,
+				  (ah->msi_reg | AR_PCIE_MSI_ENABLE)
+				  & msi_pend_addr_mask);
+			_msi_reg = REG_READ(ah, AR_PCIE_MSI);
+			i++;
+		} while ((_msi_reg & AR_PCIE_MSI_ENABLE) == 0 && i < 200);
+
+		if (i >= 200)
+			ath_err(ath9k_hw_common(ah),
+				"%s: _msi_reg = 0x%X\n",
+				__func__, _msi_reg);
+	}
 }
 
 void ath9k_hw_resume_interrupts(struct ath_hw *ah)
@@ -878,12 +915,21 @@ void ath9k_hw_set_interrupts(struct ath_hw *ah)
 	if (!(ints & ATH9K_INT_GLOBAL))
 		ath9k_hw_disable_interrupts(ah);
 
+	if (ah->msi_enabled) {
+		ath_dbg(common, INTERRUPT, "Clearing AR_INTR_PRIO_ASYNC_ENABLE\n");
+
+		REG_WRITE(ah, AR_INTR_PRIO_ASYNC_ENABLE, 0);
+		REG_READ(ah, AR_INTR_PRIO_ASYNC_ENABLE);
+	}
+
 	ath_dbg(common, INTERRUPT, "New interrupt mask 0x%x\n", ints);
 
 	mask = ints & ATH9K_INT_COMMON;
 	mask2 = 0;
 
+	ah->msi_mask = 0;
 	if (ints & ATH9K_INT_TX) {
+		ah->msi_mask |= AR_INTR_PRIO_TX;
 		if (ah->config.tx_intr_mitigation)
 			mask |= AR_IMR_TXMINTR | AR_IMR_TXINTM;
 		else {
@@ -898,6 +944,7 @@ void ath9k_hw_set_interrupts(struct ath_hw *ah)
 			mask |= AR_IMR_TXEOL;
 	}
 	if (ints & ATH9K_INT_RX) {
+		ah->msi_mask |= AR_INTR_PRIO_RXLP | AR_INTR_PRIO_RXHP;
 		if (AR_SREV_9300_20_OR_LATER(ah)) {
 			mask |= AR_IMR_RXERR | AR_IMR_RXOK_HP;
 			if (ah->config.rx_intr_mitigation) {
diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
index 223606311261..645f0fbd9179 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -22,6 +22,8 @@
 #include <linux/module.h>
 #include "ath9k.h"
 
+extern int ath9k_use_msi;
+
 static const struct pci_device_id ath_pci_id_table[] = {
 	{ PCI_VDEVICE(ATHEROS, 0x0023) }, /* PCI   */
 	{ PCI_VDEVICE(ATHEROS, 0x0024) }, /* PCI-E */
@@ -889,6 +891,7 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	u32 val;
 	int ret = 0;
 	char hw_name[64];
+	int msi_enabled = 0;
 
 	if (pcim_enable_device(pdev))
 		return -EIO;
@@ -960,7 +963,20 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	sc->mem = pcim_iomap_table(pdev)[0];
 	sc->driver_data = id->driver_data;
 
-	ret = request_irq(pdev->irq, ath_isr, IRQF_SHARED, "ath9k", sc);
+	if (ath9k_use_msi) {
+		if (pci_enable_msi(pdev) == 0) {
+			msi_enabled = 1;
+			dev_err(&pdev->dev, "Using MSI\n");
+		} else {
+			dev_err(&pdev->dev, "Using INTx\n");
+		}
+	}
+
+	if (!msi_enabled)
+		ret = request_irq(pdev->irq, ath_isr, IRQF_SHARED, "ath9k", sc);
+	else
+		ret = request_irq(pdev->irq, ath_isr, 0, "ath9k", sc);
+
 	if (ret) {
 		dev_err(&pdev->dev, "request_irq failed\n");
 		goto err_irq;
@@ -974,6 +990,9 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err_init;
 	}
 
+	sc->sc_ah->msi_enabled = msi_enabled;
+	sc->sc_ah->msi_reg = 0;
+
 	ath9k_hw_name(sc->sc_ah, hw_name, sizeof(hw_name));
 	wiphy_info(hw->wiphy, "%s mem=0x%lx, irq=%d\n",
 		   hw_name, (unsigned long)sc->mem, pdev->irq);
diff --git a/drivers/net/wireless/ath/ath9k/reg.h b/drivers/net/wireless/ath/ath9k/reg.h
index 80ff69f99229..653e79611830 100644
--- a/drivers/net/wireless/ath/ath9k/reg.h
+++ b/drivers/net/wireless/ath/ath9k/reg.h
@@ -146,6 +146,14 @@
 #define AR_MACMISC_MISC_OBS_BUS_MSB_S   15
 #define AR_MACMISC_MISC_OBS_BUS_1       1
 
+#define AR_INTCFG               0x005C
+#define AR_INTCFG_MSI_RXOK      0x00000000
+#define AR_INTCFG_MSI_RXINTM    0x00000004
+#define AR_INTCFG_MSI_RXMINTR   0x00000006
+#define AR_INTCFG_MSI_TXOK      0x00000000
+#define AR_INTCFG_MSI_TXINTM    0x00000010
+#define AR_INTCFG_MSI_TXMINTR   0x00000018
+
 #define AR_DATABUF_SIZE		0x0060
 #define AR_DATABUF_SIZE_MASK	0x00000FFF
 
@@ -1256,6 +1264,13 @@ enum {
 #define AR_PCIE_MSI                             (AR_SREV_9340(ah) ? 0x40d8 : \
 						 (AR_SREV_9300_20_OR_LATER(ah) ? 0x40a4 : 0x4094))
 #define AR_PCIE_MSI_ENABLE                       0x00000001
+#define AR_PCIE_MSI_HW_DBI_WR_EN                 0x02000000
+#define AR_PCIE_MSI_HW_INT_PENDING_ADDR          0xFFA0C1FF /* bits 8..11: value must be 0x5060 */
+#define AR_PCIE_MSI_HW_INT_PENDING_ADDR_MSI_64   0xFFA0C9FF /* bits 8..11: value must be 0x5064 */
+
+#define AR_INTR_PRIO_TX               0x00000001
+#define AR_INTR_PRIO_RXLP             0x00000002
+#define AR_INTR_PRIO_RXHP             0x00000004
 
 #define AR_INTR_PRIO_SYNC_ENABLE  (AR_SREV_9340(ah) ? 0x4088 : 0x40c4)
 #define AR_INTR_PRIO_ASYNC_MASK   (AR_SREV_9340(ah) ? 0x408c : 0x40c8)
-- 
2.11.0

Re: [v2] ath9k: add MSI support

[Daniel Drake]

Hi Russell,

> On new Intel platforms like ApolloLake, legacy interrupt mechanism
> (INTx) is not supported

Could you please share the background on what you are claiming here.
I have multiple ApolloLake laptops here with many legacy interrupts
being used in /proc/interrupts.

I do see this ath9k problem on multiple Acer ApolloLake laptops, however
I also have an Asus E402NA ApolloLake laptop on hand where the exact same
ath9k miniPCIe card is working fine with legacy interrupts.

> With module paremeter "use_msi=1", ath9k driver would try to
> use MSI instead of INTx.

In the previous patch review it was suggested that MSI should become
the default - not a quirk or parameter.
https://lkml.org/lkml/2017/9/26/64


I have tested your patch on Acer Aspire ES1-432. It does not work -
I still can't connect to wifi.
/proc/interrupts shows that no MSI interrupts are delivered, the
counters are 0.

lspci -vv shows:
        Capabilities: [50] MSI: Enable+ Count=1/4 Maskable+ 64bit+
                Address: 00000000fee0f00c  Data: 4142
                Masking: 0000000e  Pending: 00000000

So MSI is enabled and the vector number is 0x42 (decimal 66).
However my kernel log is now totally spammed with:
  do_IRQ: 0.64 No irq handler for vector

My assumption here is that the ath9k hardware implementation of
MSI is buggy, and it is therefore corrupting the MSI vector number
by zeroing out the lower 2 bits (e.g. 66 -> 64).

It would be very useful if Qualcomm could confirm if this behaviour
is really true and if it could potentially be fixed with a new ath9k
firmware version.

For more info please see:
   https://marc.info/?l=linux-pci&m=150238260826803&w=2
   https://marc.info/?t=150631283200001&r=1&w=2
   https://marc.info/?l=linux-pci&m=150831581725596&w=2

Thanks
Daniel


> diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
> index 8c5c2dd8fa7f..cd0f023ccf77 100644
> --- a/drivers/net/wireless/ath/ath9k/hw.c
> +++ b/drivers/net/wireless/ath/ath9k/hw.c
> @@ -922,6 +922,7 @@ static void ath9k_hw_init_interrupt_masks(struct ath_hw *ah,
>  		AR_IMR_RXERR |
>  		AR_IMR_RXORN |
>  		AR_IMR_BCNMISC;
> +	u32 msi_cfg = 0;
>  
>  	if (AR_SREV_9340(ah) || AR_SREV_9550(ah) || AR_SREV_9531(ah) ||
>  	    AR_SREV_9561(ah))
> @@ -929,22 +930,30 @@ static void ath9k_hw_init_interrupt_masks(struct ath_hw *ah,
>  
>  	if (AR_SREV_9300_20_OR_LATER(ah)) {
>  		imr_reg |= AR_IMR_RXOK_HP;
> -		if (ah->config.rx_intr_mitigation)
> +		if (ah->config.rx_intr_mitigation) {
>  			imr_reg |= AR_IMR_RXINTM | AR_IMR_RXMINTR;
> -		else
> +			msi_cfg |= AR_INTCFG_MSI_RXINTM | AR_INTCFG_MSI_RXMINTR;
> +		} else {
>  			imr_reg |= AR_IMR_RXOK_LP;
> -
> +			msi_cfg |= AR_INTCFG_MSI_RXOK;
> +		}
>  	} else {
> -		if (ah->config.rx_intr_mitigation)
> +		if (ah->config.rx_intr_mitigation) {
>  			imr_reg |= AR_IMR_RXINTM | AR_IMR_RXMINTR;
> -		else
> +			msi_cfg |= AR_INTCFG_MSI_RXINTM | AR_INTCFG_MSI_RXMINTR;
> +		} else {
>  			imr_reg |= AR_IMR_RXOK;
> +			msi_cfg |= AR_INTCFG_MSI_RXOK;
> +		}
>  	}
>  
> -	if (ah->config.tx_intr_mitigation)
> +	if (ah->config.tx_intr_mitigation) {
>  		imr_reg |= AR_IMR_TXINTM | AR_IMR_TXMINTR;
> -	else
> +		msi_cfg |= AR_INTCFG_MSI_TXINTM | AR_INTCFG_MSI_TXMINTR;
> +	} else {
>  		imr_reg |= AR_IMR_TXOK;
> +		msi_cfg |= AR_INTCFG_MSI_TXOK;
> +	}
>  
>  	ENABLE_REGWRITE_BUFFER(ah);
>  
> @@ -952,6 +961,16 @@ static void ath9k_hw_init_interrupt_masks(struct ath_hw *ah,
>  	ah->imrs2_reg |= AR_IMR_S2_GTT;
>  	REG_WRITE(ah, AR_IMR_S2, ah->imrs2_reg);
>  
> +	if (ah->msi_enabled) {
> +		ah->msi_reg = REG_READ(ah, AR_PCIE_MSI);
> +		ah->msi_reg |= AR_PCIE_MSI_HW_DBI_WR_EN;
> +		ah->msi_reg &= AR_PCIE_MSI_HW_INT_PENDING_ADDR_MSI_64;
> +		REG_WRITE(ah, AR_INTCFG, msi_cfg);
> +		ath_dbg(ath9k_hw_common(ah), ANY,
> +			"value of AR_INTCFG=0x%X, msi_cfg=0x%X\n",
> +			REG_READ(ah, AR_INTCFG), msi_cfg);
> +	}
> +
>  	if (!AR_SREV_9100(ah)) {
>  		REG_WRITE(ah, AR_INTR_SYNC_CAUSE, 0xFFFFFFFF);
>  		REG_WRITE(ah, AR_INTR_SYNC_ENABLE, sync_default);
> diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
> index 4ac70827d142..0d6c07c77372 100644
> --- a/drivers/net/wireless/ath/ath9k/hw.h
> +++ b/drivers/net/wireless/ath/ath9k/hw.h
> @@ -977,6 +977,9 @@ struct ath_hw {
>  	bool tpc_enabled;
>  	u8 tx_power[Ar5416RateSize];
>  	u8 tx_power_stbc[Ar5416RateSize];
> +	bool msi_enabled;
> +	u32 msi_mask;
> +	u32 msi_reg;
>  };
>  
>  struct ath_bus_ops {
> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
> index bb7936090b91..b6b7a353fea4 100644
> --- a/drivers/net/wireless/ath/ath9k/init.c
> +++ b/drivers/net/wireless/ath/ath9k/init.c
> @@ -75,6 +75,10 @@ MODULE_PARM_DESC(use_chanctx, "Enable channel context for concurrency");
>  
>  #endif /* CONFIG_ATH9K_CHANNEL_CONTEXT */
>  
> +int ath9k_use_msi;
> +module_param_named(use_msi, ath9k_use_msi, int, 0444);
> +MODULE_PARM_DESC(use_msi, "Use MSI instead of INTx if possible");
> +
>  bool is_ath9k_unloaded;
>  
>  #ifdef CONFIG_MAC80211_LEDS
> diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c
> index 77c94f9e7b61..58d02c19b6d0 100644
> --- a/drivers/net/wireless/ath/ath9k/mac.c
> +++ b/drivers/net/wireless/ath/ath9k/mac.c
> @@ -832,6 +832,43 @@ static void __ath9k_hw_enable_interrupts(struct ath_hw *ah)
>  	}
>  	ath_dbg(common, INTERRUPT, "AR_IMR 0x%x IER 0x%x\n",
>  		REG_READ(ah, AR_IMR), REG_READ(ah, AR_IER));
> +
> +	if (ah->msi_enabled) {
> +		u32 _msi_reg = 0;
> +		u32 i = 0;
> +		u32 msi_pend_addr_mask = AR_PCIE_MSI_HW_INT_PENDING_ADDR_MSI_64;
> +
> +		ath_dbg(ath9k_hw_common(ah), INTERRUPT,
> +			"Enabling MSI, msi_mask=0x%X\n", ah->msi_mask);
> +
> +		REG_WRITE(ah, AR_INTR_PRIO_ASYNC_ENABLE, ah->msi_mask);
> +		REG_WRITE(ah, AR_INTR_PRIO_ASYNC_MASK, ah->msi_mask);
> +		ath_dbg(ath9k_hw_common(ah), INTERRUPT,
> +			"AR_INTR_PRIO_ASYNC_ENABLE=0x%X, AR_INTR_PRIO_ASYNC_MASK=0x%X\n",
> +			REG_READ(ah, AR_INTR_PRIO_ASYNC_ENABLE),
> +			REG_READ(ah, AR_INTR_PRIO_ASYNC_MASK));
> +
> +		if (ah->msi_reg == 0)
> +			ah->msi_reg = REG_READ(ah, AR_PCIE_MSI);
> +
> +		ath_dbg(ath9k_hw_common(ah), INTERRUPT,
> +			"AR_PCIE_MSI=0x%X, ah->msi_reg = 0x%X\n",
> +			AR_PCIE_MSI, ah->msi_reg);
> +
> +		i = 0;
> +		do {
> +			REG_WRITE(ah, AR_PCIE_MSI,
> +				  (ah->msi_reg | AR_PCIE_MSI_ENABLE)
> +				  & msi_pend_addr_mask);
> +			_msi_reg = REG_READ(ah, AR_PCIE_MSI);
> +			i++;
> +		} while ((_msi_reg & AR_PCIE_MSI_ENABLE) == 0 && i < 200);
> +
> +		if (i >= 200)
> +			ath_err(ath9k_hw_common(ah),
> +				"%s: _msi_reg = 0x%X\n",
> +				__func__, _msi_reg);
> +	}
>  }
>  
>  void ath9k_hw_resume_interrupts(struct ath_hw *ah)
> @@ -878,12 +915,21 @@ void ath9k_hw_set_interrupts(struct ath_hw *ah)
>  	if (!(ints & ATH9K_INT_GLOBAL))
>  		ath9k_hw_disable_interrupts(ah);
>  
> +	if (ah->msi_enabled) {
> +		ath_dbg(common, INTERRUPT, "Clearing AR_INTR_PRIO_ASYNC_ENABLE\n");
> +
> +		REG_WRITE(ah, AR_INTR_PRIO_ASYNC_ENABLE, 0);
> +		REG_READ(ah, AR_INTR_PRIO_ASYNC_ENABLE);
> +	}
> +
>  	ath_dbg(common, INTERRUPT, "New interrupt mask 0x%x\n", ints);
>  
>  	mask = ints & ATH9K_INT_COMMON;
>  	mask2 = 0;
>  
> +	ah->msi_mask = 0;
>  	if (ints & ATH9K_INT_TX) {
> +		ah->msi_mask |= AR_INTR_PRIO_TX;
>  		if (ah->config.tx_intr_mitigation)
>  			mask |= AR_IMR_TXMINTR | AR_IMR_TXINTM;
>  		else {
> @@ -898,6 +944,7 @@ void ath9k_hw_set_interrupts(struct ath_hw *ah)
>  			mask |= AR_IMR_TXEOL;
>  	}
>  	if (ints & ATH9K_INT_RX) {
> +		ah->msi_mask |= AR_INTR_PRIO_RXLP | AR_INTR_PRIO_RXHP;
>  		if (AR_SREV_9300_20_OR_LATER(ah)) {
>  			mask |= AR_IMR_RXERR | AR_IMR_RXOK_HP;
>  			if (ah->config.rx_intr_mitigation) {
> diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
> index 223606311261..645f0fbd9179 100644
> --- a/drivers/net/wireless/ath/ath9k/pci.c
> +++ b/drivers/net/wireless/ath/ath9k/pci.c
> @@ -22,6 +22,8 @@
>  #include <linux/module.h>
>  #include "ath9k.h"
>  
> +extern int ath9k_use_msi;
> +
>  static const struct pci_device_id ath_pci_id_table[] = {
>  	{ PCI_VDEVICE(ATHEROS, 0x0023) }, /* PCI   */
>  	{ PCI_VDEVICE(ATHEROS, 0x0024) }, /* PCI-E */
> @@ -889,6 +891,7 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	u32 val;
>  	int ret = 0;
>  	char hw_name[64];
> +	int msi_enabled = 0;
>  
>  	if (pcim_enable_device(pdev))
>  		return -EIO;
> @@ -960,7 +963,20 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	sc->mem = pcim_iomap_table(pdev)[0];
>  	sc->driver_data = id->driver_data;
>  
> -	ret = request_irq(pdev->irq, ath_isr, IRQF_SHARED, "ath9k", sc);
> +	if (ath9k_use_msi) {
> +		if (pci_enable_msi(pdev) == 0) {
> +			msi_enabled = 1;
> +			dev_err(&pdev->dev, "Using MSI\n");
> +		} else {
> +			dev_err(&pdev->dev, "Using INTx\n");
> +		}
> +	}
> +
> +	if (!msi_enabled)
> +		ret = request_irq(pdev->irq, ath_isr, IRQF_SHARED, "ath9k", sc);
> +	else
> +		ret = request_irq(pdev->irq, ath_isr, 0, "ath9k", sc);
> +
>  	if (ret) {
>  		dev_err(&pdev->dev, "request_irq failed\n");
>  		goto err_irq;
> @@ -974,6 +990,9 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		goto err_init;
>  	}
>  
> +	sc->sc_ah->msi_enabled = msi_enabled;
> +	sc->sc_ah->msi_reg = 0;
> +
>  	ath9k_hw_name(sc->sc_ah, hw_name, sizeof(hw_name));
>  	wiphy_info(hw->wiphy, "%s mem=0x%lx, irq=%d\n",
>  		   hw_name, (unsigned long)sc->mem, pdev->irq);
> diff --git a/drivers/net/wireless/ath/ath9k/reg.h b/drivers/net/wireless/ath/ath9k/reg.h
> index 80ff69f99229..653e79611830 100644
> --- a/drivers/net/wireless/ath/ath9k/reg.h
> +++ b/drivers/net/wireless/ath/ath9k/reg.h
> @@ -146,6 +146,14 @@
>  #define AR_MACMISC_MISC_OBS_BUS_MSB_S   15
>  #define AR_MACMISC_MISC_OBS_BUS_1       1
>  
> +#define AR_INTCFG               0x005C
> +#define AR_INTCFG_MSI_RXOK      0x00000000
> +#define AR_INTCFG_MSI_RXINTM    0x00000004
> +#define AR_INTCFG_MSI_RXMINTR   0x00000006
> +#define AR_INTCFG_MSI_TXOK      0x00000000
> +#define AR_INTCFG_MSI_TXINTM    0x00000010
> +#define AR_INTCFG_MSI_TXMINTR   0x00000018
> +
>  #define AR_DATABUF_SIZE		0x0060
>  #define AR_DATABUF_SIZE_MASK	0x00000FFF
>  
> @@ -1256,6 +1264,13 @@ enum {
>  #define AR_PCIE_MSI                             (AR_SREV_9340(ah) ? 0x40d8 : \
>  						 (AR_SREV_9300_20_OR_LATER(ah) ? 0x40a4 : 0x4094))
>  #define AR_PCIE_MSI_ENABLE                       0x00000001
> +#define AR_PCIE_MSI_HW_DBI_WR_EN                 0x02000000
> +#define AR_PCIE_MSI_HW_INT_PENDING_ADDR          0xFFA0C1FF /* bits 8..11: value must be 0x5060 */
> +#define AR_PCIE_MSI_HW_INT_PENDING_ADDR_MSI_64   0xFFA0C9FF /* bits 8..11: value must be 0x5064 */
> +
> +#define AR_INTR_PRIO_TX               0x00000001
> +#define AR_INTR_PRIO_RXLP             0x00000002
> +#define AR_INTR_PRIO_RXHP             0x00000004
>  
>  #define AR_INTR_PRIO_SYNC_ENABLE  (AR_SREV_9340(ah) ? 0x4088 : 0x40c4)
>  #define AR_INTR_PRIO_ASYNC_MASK   (AR_SREV_9340(ah) ? 0x408c : 0x40c8)

Re: [v2] ath9k: add MSI support

[Kalle Valo]

Daniel Drake <drake@endlessm.com> writes:

>> On new Intel platforms like ApolloLake, legacy interrupt mechanism
>> (INTx) is not supported
>
> Could you please share the background on what you are claiming here.
> I have multiple ApolloLake laptops here with many legacy interrupts
> being used in /proc/interrupts.
>
> I do see this ath9k problem on multiple Acer ApolloLake laptops, however
> I also have an Asus E402NA ApolloLake laptop on hand where the exact same
> ath9k miniPCIe card is working fine with legacy interrupts.
>
>> With module paremeter "use_msi=3D1", ath9k driver would try to
>> use MSI instead of INTx.
>
> In the previous patch review it was suggested that MSI should become
> the default - not a quirk or parameter.
> https://lkml.org/lkml/2017/9/26/64

Enabling MSI by default is just too invasive, ath9k is used in so many
different enviroments that risk of regressions is high. MSI needs a lot
of testing before we can even consider enabling it by default.

> I have tested your patch on Acer Aspire ES1-432. It does not work -
> I still can't connect to wifi.
> /proc/interrupts shows that no MSI interrupts are delivered, the
> counters are 0.
>
> lspci -vv shows:
>         Capabilities: [50] MSI: Enable+ Count=3D1/4 Maskable+ 64bit+
>                 Address: 00000000fee0f00c  Data: 4142
>                 Masking: 0000000e  Pending: 00000000
>
> So MSI is enabled and the vector number is 0x42 (decimal 66).
> However my kernel log is now totally spammed with:
>   do_IRQ: 0.64 No irq handler for vector
>
> My assumption here is that the ath9k hardware implementation of
> MSI is buggy, and it is therefore corrupting the MSI vector number
> by zeroing out the lower 2 bits (e.g. 66 -> 64).
>
> It would be very useful if Qualcomm could confirm if this behaviour
> is really true and if it could potentially be fixed with a new ath9k
> firmware version.

ath9k does not have firmware.

--=20
Kalle Valo=

Re: [v2] ath9k: add MSI support

[Daniel Drake]

On Mon, Nov 13, 2017 at 4:48 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Enabling MSI by default is just too invasive, ath9k is used in so many
> different enviroments that risk of regressions is high. MSI needs a lot
> of testing before we can even consider enabling it by default.

And it seems like we already found a regression here - the MSI Message
Data is being corrupted as described in my last mail. Can't be fixed
in firmware, but it would be good to have confirmation of the hardware
behavivour, and maybe some other solution is possible? Are you
following this up within Qualcomm?

>> I have tested your patch on Acer Aspire ES1-432. It does not work -
>> I still can't connect to wifi.
>> /proc/interrupts shows that no MSI interrupts are delivered, the
>> counters are 0.
>>
>> lspci -vv shows:
>>         Capabilities: [50] MSI: Enable+ Count=1/4 Maskable+ 64bit+
>>                 Address: 00000000fee0f00c  Data: 4142
>>                 Masking: 0000000e  Pending: 00000000
>>
>> So MSI is enabled and the vector number is 0x42 (decimal 66).
>> However my kernel log is now totally spammed with:
>>   do_IRQ: 0.64 No irq handler for vector
>>
>> My assumption here is that the ath9k hardware implementation of
>> MSI is buggy, and it is therefore corrupting the MSI vector number
>> by zeroing out the lower 2 bits (e.g. 66 -> 64).

Thanks
Daniel

Re: [v2] ath9k: add MSI support

[Kalle Valo]

Daniel Drake <drake@endlessm.com> writes:

> On Mon, Nov 13, 2017 at 4:48 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrot=
e:
>> Enabling MSI by default is just too invasive, ath9k is used in so many
>> different enviroments that risk of regressions is high. MSI needs a lot
>> of testing before we can even consider enabling it by default.
>
> And it seems like we already found a regression here - the MSI Message
> Data is being corrupted as described in my last mail.

Exactly.

> Can't be fixed in firmware, but it would be good to have confirmation
> of the hardware behavivour, and maybe some other solution is possible?
> Are you following this up within Qualcomm?

No time to do that right now, sorry.

--=20
Kalle Valo=

Re: [v2] ath9k: add MSI support

[Daniel Drake]

On Tue, Nov 14, 2017 at 8:15 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Can't be fixed in firmware, but it would be good to have confirmation
>> of the hardware behavivour, and maybe some other solution is possible?
>> Are you following this up within Qualcomm?
>
> No time to do that right now, sorry.

I got several autoresponders from people on this thread from Qualcomm
Taiwan. Would it be useful for us to drop off a sample of the affected
product at your Taipei or Hsinchu office so that you can investigate
further?

Thanks
Daniel

Re: [v2] ath9k: add MSI support

[Daniel Drake]

On Wed, Nov 15, 2017 at 7:38 AM, Daniel Drake <drake@endlessm.com> wrote:
> On Tue, Nov 14, 2017 at 8:15 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>>> Can't be fixed in firmware, but it would be good to have confirmation
>>> of the hardware behavivour, and maybe some other solution is possible?
>>> Are you following this up within Qualcomm?
>>
>> No time to do that right now, sorry.
>
> I got several autoresponders from people on this thread from Qualcomm
> Taiwan. Would it be useful for us to drop off a sample of the affected
> product at your Taipei or Hsinchu office so that you can investigate
> further?

Ping - how can we collaborate on this?

Also, we have been testing the MSI support patch and while it seems to
be working fine on AR9565, multiple users hit failures on AR9462. The
most common report is that the system simply cannot maintain the
connection with the AP for more than a few seconds. It hits a check in
mac80211 where it sends a nullfunc to the AP and expects an ack in
less than 500ms, but it disconnects since it doesn't see the ack.

https://marc.info/?l=linux-wireless&m=151027741010422&w=2

We also reproduced a problem in our office with AR9462. With the MSI
support patch in use, we ping a server every second for 1000 seconds
while monitoring "iw dev wlp2s0 link" output. With the MSI support
patch in place, this test fails every time; the connection is dropped
in less than 1000s.
With the patch reverted everything is fine.

We ran the same test with AR9565 in MSI mode and it worked fine.

Daniel

Re: [v2] ath9k: add MSI support

[Kalle Valo]

(Adding AceLan)

Daniel Drake <drake@endlessm.com> writes:

> On Wed, Nov 15, 2017 at 7:38 AM, Daniel Drake <drake@endlessm.com> wrote:
>> On Tue, Nov 14, 2017 at 8:15 PM, Kalle Valo <kvalo@qca.qualcomm.com> wro=
te:
>>>> Can't be fixed in firmware, but it would be good to have confirmation
>>>> of the hardware behavivour, and maybe some other solution is possible?
>>>> Are you following this up within Qualcomm?
>>>
>>> No time to do that right now, sorry.
>>
>> I got several autoresponders from people on this thread from Qualcomm
>> Taiwan. Would it be useful for us to drop off a sample of the affected
>> product at your Taipei or Hsinchu office so that you can investigate
>> further?
>
> Ping - how can we collaborate on this?

Are you asking me? While looking at my todo list for this year I doubt I
can find time to help with the MSI implementation or bugfixing.

But my plan is that first I would apply Russel's patch which makes it
possible to enable MSI with a module parameter:

https://patchwork.kernel.org/patch/9999249/

Then if AceLan could rebase the quirk patches and resubmit those so that
I can apply those:

https://patchwork.kernel.org/patch/9971097/
https://patchwork.kernel.org/patch/9971095/
https://patchwork.kernel.org/patch/9971093/
https://patchwork.kernel.org/patch/9971091/
https://patchwork.kernel.org/patch/9971089/

Also I don't see the need to have five different patches for the quirks,
having just one patch adding all those should be ok.

Are everyone happy with this plan?

> Also, we have been testing the MSI support patch and while it seems to
> be working fine on AR9565, multiple users hit failures on AR9462. The
> most common report is that the system simply cannot maintain the
> connection with the AP for more than a few seconds. It hits a check in
> mac80211 where it sends a nullfunc to the AP and expects an ack in
> less than 500ms, but it disconnects since it doesn't see the ack.
>
> https://marc.info/?l=3Dlinux-wireless&m=3D151027741010422&w=3D2
>
> We also reproduced a problem in our office with AR9462. With the MSI
> support patch in use, we ping a server every second for 1000 seconds
> while monitoring "iw dev wlp2s0 link" output. With the MSI support
> patch in place, this test fails every time; the connection is dropped
> in less than 1000s.
> With the patch reverted everything is fine.

I'm not really surprised and this is exactly why I was against of
enabling MSI by default. It can cause all sorts of weird problems.

--=20
Kalle Valo=

Re: [v2] ath9k: add MSI support

[Andy Shevchenko]

On Mon, Jan 8, 2018 at 2:24 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> (Adding AceLan)
>
> Daniel Drake <drake@endlessm.com> writes:
>
>> On Wed, Nov 15, 2017 at 7:38 AM, Daniel Drake <drake@endlessm.com> wrote:
>>> On Tue, Nov 14, 2017 at 8:15 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>>>>> Can't be fixed in firmware, but it would be good to have confirmation
>>>>> of the hardware behavivour, and maybe some other solution is possible?
>>>>> Are you following this up within Qualcomm?
>>>>
>>>> No time to do that right now, sorry.
>>>
>>> I got several autoresponders from people on this thread from Qualcomm
>>> Taiwan. Would it be useful for us to drop off a sample of the affected
>>> product at your Taipei or Hsinchu office so that you can investigate
>>> further?
>>
>> Ping - how can we collaborate on this?
>
> Are you asking me? While looking at my todo list for this year I doubt I
> can find time to help with the MSI implementation or bugfixing.
>
> But my plan is that first I would apply Russel's patch which makes it
> possible to enable MSI with a module parameter:
>
> https://patchwork.kernel.org/patch/9999249/

Just in case it was missed during review:

The variables like

+ bool msi_enabled;

usually are redundant because PCI core keeps track of MSI/MSI-X status
(enabled/disabled)
So, if there is no MSI-X involved or MSI-X is handled in the same way
as MSI in the driver, one can use

pci_dev_msi_enabled() instead.

> Are everyone happy with this plan?

Sounds reasonable.

-- 
With Best Regards,
Andy Shevchenko

Re: [v2] ath9k: add MSI support

[Daniel Drake]

On Mon, Jan 8, 2018 at 6:24 AM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> (Adding AceLan)
>
> Daniel Drake <drake@endlessm.com> writes:
>
>> On Wed, Nov 15, 2017 at 7:38 AM, Daniel Drake <drake@endlessm.com> wrote:
>>> On Tue, Nov 14, 2017 at 8:15 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>>>>> Can't be fixed in firmware, but it would be good to have confirmation
>>>>> of the hardware behavivour, and maybe some other solution is possible?
>>>>> Are you following this up within Qualcomm?
>>>>
>>>> No time to do that right now, sorry.
>>>
>>> I got several autoresponders from people on this thread from Qualcomm
>>> Taiwan. Would it be useful for us to drop off a sample of the affected
>>> product at your Taipei or Hsinchu office so that you can investigate
>>> further?
>>
>> Ping - how can we collaborate on this?
>
> Are you asking me? While looking at my todo list for this year I doubt I
> can find time to help with the MSI implementation or bugfixing.

So far you are the only Qualcomm person to reply to the many mails I
have written on this topic, so I appreciate the response. I have sunk
many hours into this unfortunate situation so I'd really appreciate if
you could point me to someone at Qualcomm who can provide a response.
I am willing to continue doing the hard work, but I do need some
Qualcomm help in getting past brick walls.

> But my plan is that first I would apply Russel's patch which makes it
> possible to enable MSI with a module parameter:
>
> https://patchwork.kernel.org/patch/9999249/

This isn't enough to fix many of the systems that are affected by this
issue. You add the parameter, enable it, and MSI support totally fails
to deliver any interrupts. Pasting again from earlier:

I have tested your patch on Acer Aspire ES1-432. It does not work - I
still can't connect to wifi.
/proc/interrupts shows that no MSI interrupts are delivered, the counters are 0.

lspci -vv shows:
        Capabilities: [50] MSI: Enable+ Count=1/4 Maskable+ 64bit+
                Address: 00000000fee0f00c  Data: 4142
                Masking: 0000000e  Pending: 00000000

So MSI is enabled and the vector number is 0x42 (decimal 66).
However my kernel log is now totally spammed with:
  do_IRQ: 0.64 No irq handler for vector

My assumption here is that the ath9k hardware implementation of MSI is
buggy, and it is therefore corrupting the MSI vector number by zeroing
out the lower 2 bits (e.g. 66 -> 64).

It would be very useful if Qualcomm could confirm if this behaviour is
really true.

For more info please see:
   https://marc.info/?l=linux-pci&m=150238260826803&w=2
   https://marc.info/?t=150631283200001&r=1&w=2
   https://marc.info/?l=linux-pci&m=150831581725596&w=2

Thanks,
Daniel

Re: [v2] ath9k: add MSI support

[AceLan Kao]

Hi Kalle,

I'm happy to see Russel's patch can be merged,
and I'm willing to rebase and merge those quirks into one commit and
submit it again.
Although the solution is not perfect, but it's nice to have.
Thanks.

Best regards,
AceLan Kao.


2018-01-09 7:07 GMT+08:00 Daniel Drake <drake@endlessm.com>:
> On Mon, Jan 8, 2018 at 6:24 AM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> (Adding AceLan)
>>
>> Daniel Drake <drake@endlessm.com> writes:
>>
>>> On Wed, Nov 15, 2017 at 7:38 AM, Daniel Drake <drake@endlessm.com> wrote:
>>>> On Tue, Nov 14, 2017 at 8:15 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>>>>>> Can't be fixed in firmware, but it would be good to have confirmation
>>>>>> of the hardware behavivour, and maybe some other solution is possible?
>>>>>> Are you following this up within Qualcomm?
>>>>>
>>>>> No time to do that right now, sorry.
>>>>
>>>> I got several autoresponders from people on this thread from Qualcomm
>>>> Taiwan. Would it be useful for us to drop off a sample of the affected
>>>> product at your Taipei or Hsinchu office so that you can investigate
>>>> further?
>>>
>>> Ping - how can we collaborate on this?
>>
>> Are you asking me? While looking at my todo list for this year I doubt I
>> can find time to help with the MSI implementation or bugfixing.
>
> So far you are the only Qualcomm person to reply to the many mails I
> have written on this topic, so I appreciate the response. I have sunk
> many hours into this unfortunate situation so I'd really appreciate if
> you could point me to someone at Qualcomm who can provide a response.
> I am willing to continue doing the hard work, but I do need some
> Qualcomm help in getting past brick walls.
>
>> But my plan is that first I would apply Russel's patch which makes it
>> possible to enable MSI with a module parameter:
>>
>> https://patchwork.kernel.org/patch/9999249/
>
> This isn't enough to fix many of the systems that are affected by this
> issue. You add the parameter, enable it, and MSI support totally fails
> to deliver any interrupts. Pasting again from earlier:
>
> I have tested your patch on Acer Aspire ES1-432. It does not work - I
> still can't connect to wifi.
> /proc/interrupts shows that no MSI interrupts are delivered, the counters are 0.
>
> lspci -vv shows:
>         Capabilities: [50] MSI: Enable+ Count=1/4 Maskable+ 64bit+
>                 Address: 00000000fee0f00c  Data: 4142
>                 Masking: 0000000e  Pending: 00000000
>
> So MSI is enabled and the vector number is 0x42 (decimal 66).
> However my kernel log is now totally spammed with:
>   do_IRQ: 0.64 No irq handler for vector
>
> My assumption here is that the ath9k hardware implementation of MSI is
> buggy, and it is therefore corrupting the MSI vector number by zeroing
> out the lower 2 bits (e.g. 66 -> 64).
>
> It would be very useful if Qualcomm could confirm if this behaviour is
> really true.
>
> For more info please see:
>    https://marc.info/?l=linux-pci&m=150238260826803&w=2
>    https://marc.info/?t=150631283200001&r=1&w=2
>    https://marc.info/?l=linux-pci&m=150831581725596&w=2
>
> Thanks,
> Daniel

Re: [v2] ath9k: add MSI support

[Kalle Valo]

Russell Hu <rhu@qti.qualcomm.com> wrote:

> On new Intel platforms like ApolloLake, legacy interrupt mechanism
> (INTx) is not supported, so WLAN modules are not working because
> interrupts are missing, therefore this patch is to add MSI support to
> ath9k.  With module paremeter "use_msi=1", ath9k driver would try to
> use MSI instead of INTx.
> 
> Signed-off-by: Russell Hu <rhu@qti.qualcomm.com>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

7368160f0ab0 ath9k: add MSI support

-- 
https://patchwork.kernel.org/patch/9999249/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches