[RFC/RFT 0/6] ath9k: ASPM fixes

[RFC/RFT 0/6] ath9k: ASPM fixes

[Stanislaw Gruszka]

This patch series try to fix ath9k ASPM (but some some of them
are cleanup only patches).

Patches do not cope with situation when CONFIG_PCIEASPM
is enabled and ASPM settings can be changed dynamically via 
/sys/module/pcie_aspm/parameters/policy
I hope I will address that soon.

These patches needs wide testing as they may work on some systems
and not work on others, that can depend on PCIe bridges.

Stanislaw

[RFC/RFT 1/6] ath9k: skip ->config_pci_powersave() if PCIe port has ASPM disabled

[Stanislaw Gruszka]

We receive many bug reports about system hang during suspend/resume
when ath9k driver is in use. Adrian Chadd remarked that this problem
happens on systems that have ASPM disabled.

To do not hit the bug, skip doing ->config_pci_powersave magic if PCIe
downstream port device, which ath9k device is connected to, has ASPM
disabled.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/ath/ath9k/ar9002_hw.c |    6 +-----
 drivers/net/wireless/ath/ath9k/ar9003_hw.c |    6 +-----
 drivers/net/wireless/ath/ath9k/hw.c        |    1 -
 drivers/net/wireless/ath/ath9k/hw.h        |    2 +-
 drivers/net/wireless/ath/ath9k/pci.c       |   23 +++++++++++++++++++++++
 5 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9002_hw.c b/drivers/net/wireless/ath/ath9k/ar9002_hw.c
index 9ff7c30..44d9d8d 100644
--- a/drivers/net/wireless/ath/ath9k/ar9002_hw.c
+++ b/drivers/net/wireless/ath/ath9k/ar9002_hw.c
@@ -309,11 +309,7 @@ static void ar9002_hw_configpcipowersave(struct ath_hw *ah,
 	u8 i;
 	u32 val;
 
-	if (ah->is_pciexpress != true)
-		return;
-
-	/* Do not touch SerDes registers */
-	if (ah->config.pcie_powersave_enable == 2)
+	if (ah->is_pciexpress != true || ah->aspm_enabled != true)
 		return;
 
 	/* Nothing to do on restore for 11N */
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_hw.c b/drivers/net/wireless/ath/ath9k/ar9003_hw.c
index 8efdec2..ad2bb2b 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_hw.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_hw.c
@@ -519,11 +519,7 @@ static void ar9003_hw_configpcipowersave(struct ath_hw *ah,
 					 int restore,
 					 int power_off)
 {
-	if (ah->is_pciexpress != true)
-		return;
-
-	/* Do not touch SerDes registers */
-	if (ah->config.pcie_powersave_enable == 2)
+	if (ah->is_pciexpress != true || ah->aspm_enabled != true)
 		return;
 
 	/* Nothing to do on restore for 11N */
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 8006ce0..6bda08e 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -378,7 +378,6 @@ static void ath9k_hw_init_config(struct ath_hw *ah)
 	ah->config.additional_swba_backoff = 0;
 	ah->config.ack_6mb = 0x0;
 	ah->config.cwm_ignore_extcca = 0;
-	ah->config.pcie_powersave_enable = 0;
 	ah->config.pcie_clock_req = 0;
 	ah->config.pcie_waen = 0;
 	ah->config.analog_shiftreg = 1;
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 6acd0f9..7dd78e7 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -219,7 +219,6 @@ struct ath9k_ops_config {
 	int additional_swba_backoff;
 	int ack_6mb;
 	u32 cwm_ignore_extcca;
-	u8 pcie_powersave_enable;
 	bool pcieSerDesWrite;
 	u8 pcie_clock_req;
 	u32 pcie_waen;
@@ -673,6 +672,7 @@ struct ath_hw {
 
 	bool sw_mgmt_crypto;
 	bool is_pciexpress;
+	bool aspm_enabled;
 	bool is_monitoring;
 	bool need_an_top2_fixup;
 	u16 tx_trig_level;
diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
index 3bad0b2..480e25b 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -123,6 +123,27 @@ static const struct ath_bus_ops ath_pci_bus_ops = {
 	.extn_synch_en = ath_pci_extn_synch_enable,
 };
 
+static void ath_pci_check_aspm(struct ath_softc *sc)
+{
+	struct ath_hw *ah = sc->sc_ah;
+	struct pci_dev *pdev = to_pci_dev(sc->dev);
+	struct pci_dev *parent;
+	u8 aspm;
+
+	ah->aspm_enabled = false;
+
+	if (!pci_is_pcie(pdev))
+		return;
+
+	parent = pdev->bus->self;
+	if (WARN_ON(!parent))
+		return;
+
+	pci_read_config_byte(parent, ATH_PCIE_CAP_LINK_CTRL, &aspm);
+	if (aspm & (ATH_PCIE_CAP_LINK_L0S | ATH_PCIE_CAP_LINK_L1))
+		ah->aspm_enabled = true;
+}
+
 static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	void __iomem *mem;
@@ -230,6 +251,8 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err_init;
 	}
 
+	ath_pci_check_aspm(sc);
+
 	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)mem, pdev->irq);
-- 
1.7.1

[RFC/RFT 2/6] ath9k: remove ->config_pci_powersave() redundant argument

[Stanislaw Gruszka]

We always call ->config_pci_powersave() with both restore and power_off
arguments equal to 0 or both equal to 1, so merge them into one
argument.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/ath/ath9k/ar9002_hw.c |    5 ++---
 drivers/net/wireless/ath/ath9k/ar9003_hw.c |    5 ++---
 drivers/net/wireless/ath/ath9k/hw-ops.h    |    5 ++---
 drivers/net/wireless/ath/ath9k/hw.c        |    2 +-
 drivers/net/wireless/ath/ath9k/hw.h        |    3 +--
 drivers/net/wireless/ath/ath9k/main.c      |    6 +++---
 6 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9002_hw.c b/drivers/net/wireless/ath/ath9k/ar9002_hw.c
index 44d9d8d..70a18d1 100644
--- a/drivers/net/wireless/ath/ath9k/ar9002_hw.c
+++ b/drivers/net/wireless/ath/ath9k/ar9002_hw.c
@@ -303,8 +303,7 @@ static void ar9002_hw_init_mode_gain_regs(struct ath_hw *ah)
  * register as the other analog registers.  Hence the 9 writes.
  */
 static void ar9002_hw_configpcipowersave(struct ath_hw *ah,
-					 int restore,
-					 int power_off)
+					 bool power_off)
 {
 	u8 i;
 	u32 val;
@@ -313,7 +312,7 @@ static void ar9002_hw_configpcipowersave(struct ath_hw *ah,
 		return;
 
 	/* Nothing to do on restore for 11N */
-	if (!restore) {
+	if (!power_off /* !restore */) {
 		if (AR_SREV_9280_20_OR_LATER(ah)) {
 			/*
 			 * AR9280 2.0 or later chips use SerDes values from the
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_hw.c b/drivers/net/wireless/ath/ath9k/ar9003_hw.c
index ad2bb2b..e3d58bd 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_hw.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_hw.c
@@ -516,14 +516,13 @@ static void ar9003_hw_init_mode_gain_regs(struct ath_hw *ah)
  * register as the other analog registers.  Hence the 9 writes.
  */
 static void ar9003_hw_configpcipowersave(struct ath_hw *ah,
-					 int restore,
-					 int power_off)
+					 bool power_off)
 {
 	if (ah->is_pciexpress != true || ah->aspm_enabled != true)
 		return;
 
 	/* Nothing to do on restore for 11N */
-	if (!restore) {
+	if (!power_off /* !restore */) {
 		/* set bit 19 to allow forcing of pcie core into L1 state */
 		REG_SET_BIT(ah, AR_PCIE_PM_CTRL, AR_PCIE_PM_CTRL_ENA);
 
diff --git a/drivers/net/wireless/ath/ath9k/hw-ops.h b/drivers/net/wireless/ath/ath9k/hw-ops.h
index cb29e88..8c12385 100644
--- a/drivers/net/wireless/ath/ath9k/hw-ops.h
+++ b/drivers/net/wireless/ath/ath9k/hw-ops.h
@@ -22,10 +22,9 @@
 /* Hardware core and driver accessible callbacks */
 
 static inline void ath9k_hw_configpcipowersave(struct ath_hw *ah,
-					       int restore,
-					       int power_off)
+					       bool power_off)
 {
-	ath9k_hw_ops(ah)->config_pci_powersave(ah, restore, power_off);
+	ath9k_hw_ops(ah)->config_pci_powersave(ah, power_off);
 }
 
 static inline void ath9k_hw_rxena(struct ath_hw *ah)
diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 6bda08e..4ef7313 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -597,7 +597,7 @@ static int __ath9k_hw_init(struct ath_hw *ah)
 
 
 	if (ah->is_pciexpress)
-		ath9k_hw_configpcipowersave(ah, 0, 0);
+		ath9k_hw_configpcipowersave(ah, false);
 	else
 		ath9k_hw_disablepcie(ah);
 
diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 7dd78e7..2f25577 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -601,8 +601,7 @@ struct ath_hw_private_ops {
  */
 struct ath_hw_ops {
 	void (*config_pci_powersave)(struct ath_hw *ah,
-				     int restore,
-				     int power_off);
+				     bool power_off);
 	void (*rx_enable)(struct ath_hw *ah);
 	void (*set_desc_link)(void *ds, u32 link);
 	bool (*calibrate)(struct ath_hw *ah,
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 9098aaa..651f633 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -887,7 +887,7 @@ static void ath_radio_enable(struct ath_softc *sc, struct ieee80211_hw *hw)
 	ath9k_ps_wakeup(sc);
 	spin_lock_bh(&sc->sc_pcu_lock);
 
-	ath9k_hw_configpcipowersave(ah, 0, 0);
+	ath9k_hw_configpcipowersave(ah, false);
 
 	if (!ah->curchan)
 		ah->curchan = ath9k_cmn_get_curchannel(sc->hw, ah);
@@ -967,7 +967,7 @@ void ath_radio_disable(struct ath_softc *sc, struct ieee80211_hw *hw)
 
 	ath9k_hw_phy_disable(ah);
 
-	ath9k_hw_configpcipowersave(ah, 1, 1);
+	ath9k_hw_configpcipowersave(ah, true);
 
 	spin_unlock_bh(&sc->sc_pcu_lock);
 	ath9k_ps_restore(sc);
@@ -1066,7 +1066,7 @@ static int ath9k_start(struct ieee80211_hw *hw)
 	init_channel = ath9k_cmn_get_curchannel(hw, ah);
 
 	/* Reset SERDES registers */
-	ath9k_hw_configpcipowersave(ah, 0, 0);
+	ath9k_hw_configpcipowersave(ah, false);
 
 	/*
 	 * The basic interface to setting the hardware in a good
-- 
1.7.1

[RFC/RFT 3/6] ath9k: merge common ->config_pci_powersave() checks

[Stanislaw Gruszka]

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/ath/ath9k/ar9002_hw.c |    3 ---
 drivers/net/wireless/ath/ath9k/ar9003_hw.c |    3 ---
 drivers/net/wireless/ath/ath9k/hw-ops.h    |    3 +++
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/ar9002_hw.c b/drivers/net/wireless/ath/ath9k/ar9002_hw.c
index 70a18d1..b54ab78 100644
--- a/drivers/net/wireless/ath/ath9k/ar9002_hw.c
+++ b/drivers/net/wireless/ath/ath9k/ar9002_hw.c
@@ -308,9 +308,6 @@ static void ar9002_hw_configpcipowersave(struct ath_hw *ah,
 	u8 i;
 	u32 val;
 
-	if (ah->is_pciexpress != true || ah->aspm_enabled != true)
-		return;
-
 	/* Nothing to do on restore for 11N */
 	if (!power_off /* !restore */) {
 		if (AR_SREV_9280_20_OR_LATER(ah)) {
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_hw.c b/drivers/net/wireless/ath/ath9k/ar9003_hw.c
index e3d58bd..9cf5d13 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_hw.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_hw.c
@@ -518,9 +518,6 @@ static void ar9003_hw_init_mode_gain_regs(struct ath_hw *ah)
 static void ar9003_hw_configpcipowersave(struct ath_hw *ah,
 					 bool power_off)
 {
-	if (ah->is_pciexpress != true || ah->aspm_enabled != true)
-		return;
-
 	/* Nothing to do on restore for 11N */
 	if (!power_off /* !restore */) {
 		/* set bit 19 to allow forcing of pcie core into L1 state */
diff --git a/drivers/net/wireless/ath/ath9k/hw-ops.h b/drivers/net/wireless/ath/ath9k/hw-ops.h
index 8c12385..f7d4e2a 100644
--- a/drivers/net/wireless/ath/ath9k/hw-ops.h
+++ b/drivers/net/wireless/ath/ath9k/hw-ops.h
@@ -24,6 +24,9 @@
 static inline void ath9k_hw_configpcipowersave(struct ath_hw *ah,
 					       bool power_off)
 {
+	if (ah->is_pciexpress != true || ah->aspm_enabled != true)
+		return;
+
 	ath9k_hw_ops(ah)->config_pci_powersave(ah, power_off);
 }
 
-- 
1.7.1

[RFC/RFT 4/6] ath9k: use common PCIe ASPM definces instead of custom ones

[Stanislaw Gruszka]

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/ath/ath9k/hw.h  |    4 ----
 drivers/net/wireless/ath/ath9k/pci.c |   11 ++++++-----
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 2f25577..3f941f0 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -1028,10 +1028,6 @@ void ath9k_ani_reset(struct ath_hw *ah, bool is_scanning);
 void ath9k_hw_proc_mib_event(struct ath_hw *ah);
 void ath9k_hw_ani_monitor(struct ath_hw *ah, struct ath9k_channel *chan);
 
-#define ATH_PCIE_CAP_LINK_CTRL	0x70
-#define ATH_PCIE_CAP_LINK_L0S	1
-#define ATH_PCIE_CAP_LINK_L1	2
-
 #define ATH9K_CLOCK_RATE_CCK		22
 #define ATH9K_CLOCK_RATE_5GHZ_OFDM	40
 #define ATH9K_CLOCK_RATE_2GHZ_OFDM	44
diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
index 480e25b..29eba63 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -16,6 +16,7 @@
 
 #include <linux/nl80211.h>
 #include <linux/pci.h>
+#include <linux/pci-aspm.h>
 #include <linux/ath9k_platform.h>
 #include "ath9k.h"
 
@@ -99,9 +100,9 @@ static void ath_pci_bt_coex_prep(struct ath_common *common)
 	if (!pci_is_pcie(pdev))
 		return;
 
-	pci_read_config_byte(pdev, ATH_PCIE_CAP_LINK_CTRL, &aspm);
-	aspm &= ~(ATH_PCIE_CAP_LINK_L0S | ATH_PCIE_CAP_LINK_L1);
-	pci_write_config_byte(pdev, ATH_PCIE_CAP_LINK_CTRL, aspm);
+	pci_read_config_byte(pdev, PCI_EXP_LNKCTL, &aspm);
+	aspm &= ~(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
+	pci_write_config_byte(pdev, PCI_EXP_LNKCTL, aspm);
 }
 
 static void ath_pci_extn_synch_enable(struct ath_common *common)
@@ -139,8 +140,8 @@ static void ath_pci_check_aspm(struct ath_softc *sc)
 	if (WARN_ON(!parent))
 		return;
 
-	pci_read_config_byte(parent, ATH_PCIE_CAP_LINK_CTRL, &aspm);
-	if (aspm & (ATH_PCIE_CAP_LINK_L0S | ATH_PCIE_CAP_LINK_L1))
+	pci_read_config_byte(parent, PCI_EXP_LNKCTL, &aspm);
+	if (aspm & (PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1))
 		ah->aspm_enabled = true;
 }
 
-- 
1.7.1

Re: [RFC/RFT 4/6] ath9k: use common PCIe ASPM definces instead of custom ones

[Rajkumar Manoharan]

On Fri, Jul 22, 2011 at 03:31:50PM +0200, Stanislaw Gruszka wrote:
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/net/wireless/ath/ath9k/hw.h  |    4 ----
>  drivers/net/wireless/ath/ath9k/pci.c |   11 ++++++-----
>  2 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
> index 2f25577..3f941f0 100644
> --- a/drivers/net/wireless/ath/ath9k/hw.h
> +++ b/drivers/net/wireless/ath/ath9k/hw.h
> @@ -1028,10 +1028,6 @@ void ath9k_ani_reset(struct ath_hw *ah, bool is_scanning);
>  void ath9k_hw_proc_mib_event(struct ath_hw *ah);
>  void ath9k_hw_ani_monitor(struct ath_hw *ah, struct ath9k_channel *chan);
>  
> -#define ATH_PCIE_CAP_LINK_CTRL	0x70
> -#define ATH_PCIE_CAP_LINK_L0S	1
> -#define ATH_PCIE_CAP_LINK_L1	2
> -
>  #define ATH9K_CLOCK_RATE_CCK		22
>  #define ATH9K_CLOCK_RATE_5GHZ_OFDM	40
>  #define ATH9K_CLOCK_RATE_2GHZ_OFDM	44
> diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
> index 480e25b..29eba63 100644
> --- a/drivers/net/wireless/ath/ath9k/pci.c
> +++ b/drivers/net/wireless/ath/ath9k/pci.c
> @@ -16,6 +16,7 @@
>  
>  #include <linux/nl80211.h>
>  #include <linux/pci.h>
> +#include <linux/pci-aspm.h>
>  #include <linux/ath9k_platform.h>
>  #include "ath9k.h"
>  
> @@ -99,9 +100,9 @@ static void ath_pci_bt_coex_prep(struct ath_common *common)
>  	if (!pci_is_pcie(pdev))
>  		return;
>  
> -	pci_read_config_byte(pdev, ATH_PCIE_CAP_LINK_CTRL, &aspm);
> -	aspm &= ~(ATH_PCIE_CAP_LINK_L0S | ATH_PCIE_CAP_LINK_L1);
> -	pci_write_config_byte(pdev, ATH_PCIE_CAP_LINK_CTRL, aspm);
> +	pci_read_config_byte(pdev, PCI_EXP_LNKCTL, &aspm);
> +	aspm &= ~(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
> +	pci_write_config_byte(pdev, PCI_EXP_LNKCTL, aspm);
>  }
>
There seems to be where address diff b/w ATH_PCIE_CAP_LINK_CTRL &
PCI_EXP_LNKCTL. It has to be like pcie_config_aspm_dev. Isn't it?

>  static void ath_pci_extn_synch_enable(struct ath_common *common)
> @@ -139,8 +140,8 @@ static void ath_pci_check_aspm(struct ath_softc *sc)
>  	if (WARN_ON(!parent))
>  		return;
>  
> -	pci_read_config_byte(parent, ATH_PCIE_CAP_LINK_CTRL, &aspm);
> -	if (aspm & (ATH_PCIE_CAP_LINK_L0S | ATH_PCIE_CAP_LINK_L1))
> +	pci_read_config_byte(parent, PCI_EXP_LNKCTL, &aspm);
Same as above.

--
Rajkumar

Re: [RFC/RFT 4/6] ath9k: use common PCIe ASPM definces instead of custom ones

[Stanislaw Gruszka]

On Sat, Jul 23, 2011 at 10:16:32AM +0530, Rajkumar Manoharan wrote:
> > +	pci_read_config_byte(pdev, PCI_EXP_LNKCTL, &aspm);
> > +	aspm &= ~(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
> > +	pci_write_config_byte(pdev, PCI_EXP_LNKCTL, aspm);
> >  }
> >
> There seems to be where address diff b/w ATH_PCIE_CAP_LINK_CTRL &
> PCI_EXP_LNKCTL. It has to be like pcie_config_aspm_dev. Isn't it?

Good point. I'll correct and repost.

Thanks
Stanislaw

[RFC/RFT 5/6] ath9k: do btcoex ASPM disabling at initialization time

[Stanislaw Gruszka]

Disable ASPM in pci .probe, to do not change settings when
pci_bus_sem is not held.

Disable ASPM on upstream (device) and downstream (PCIe port) component.
According to e1000e driver authors this is required. I did not find that
requirement in PCIe spec, but it seems to be logical for me.

This need to be fixed for CONFIG_PCIEASPM, that will be done later
during merge common code into pcie core.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/ath/ath9k/hw.h   |    1 -
 drivers/net/wireless/ath/ath9k/main.c |    2 -
 drivers/net/wireless/ath/ath9k/pci.c  |   44 ++++++++++++++++++---------------
 3 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
index 3f941f0..6abf26d 100644
--- a/drivers/net/wireless/ath/ath9k/hw.h
+++ b/drivers/net/wireless/ath/ath9k/hw.h
@@ -871,7 +871,6 @@ struct ath_bus_ops {
 	enum ath_bus_type ath_bus_type;
 	void (*read_cachesize)(struct ath_common *common, int *csz);
 	bool (*eeprom_read)(struct ath_common *common, u32 off, u16 *data);
-	void (*bt_coex_prep)(struct ath_common *common);
 	void (*extn_synch_en)(struct ath_common *common);
 };
 
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index 651f633..b0389c3 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1141,8 +1141,6 @@ static int ath9k_start(struct ieee80211_hw *hw)
 					   AR_STOMP_LOW_WLAN_WGHT);
 		ath9k_hw_btcoex_enable(ah);
 
-		if (common->bus_ops->bt_coex_prep)
-			common->bus_ops->bt_coex_prep(common);
 		if (ah->btcoex_hw.scheme == ATH_BTCOEX_CFG_3WIRE)
 			ath9k_btcoex_timer_resume(sc);
 	}
diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
index 29eba63..835a50a 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -88,23 +88,6 @@ static bool ath_pci_eeprom_read(struct ath_common *common, u32 off, u16 *data)
 	return true;
 }
 
-/*
- * Bluetooth coexistance requires disabling ASPM.
- */
-static void ath_pci_bt_coex_prep(struct ath_common *common)
-{
-	struct ath_softc *sc = (struct ath_softc *) common->priv;
-	struct pci_dev *pdev = to_pci_dev(sc->dev);
-	u8 aspm;
-
-	if (!pci_is_pcie(pdev))
-		return;
-
-	pci_read_config_byte(pdev, PCI_EXP_LNKCTL, &aspm);
-	aspm &= ~(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
-	pci_write_config_byte(pdev, PCI_EXP_LNKCTL, aspm);
-}
-
 static void ath_pci_extn_synch_enable(struct ath_common *common)
 {
 	struct ath_softc *sc = (struct ath_softc *) common->priv;
@@ -120,11 +103,10 @@ static const struct ath_bus_ops ath_pci_bus_ops = {
 	.ath_bus_type = ATH_PCI,
 	.read_cachesize = ath_pci_read_cachesize,
 	.eeprom_read = ath_pci_eeprom_read,
-	.bt_coex_prep = ath_pci_bt_coex_prep,
 	.extn_synch_en = ath_pci_extn_synch_enable,
 };
 
-static void ath_pci_check_aspm(struct ath_softc *sc)
+static void ath_pci_aspm_init(struct ath_softc *sc)
 {
 	struct ath_hw *ah = sc->sc_ah;
 	struct pci_dev *pdev = to_pci_dev(sc->dev);
@@ -137,6 +119,27 @@ static void ath_pci_check_aspm(struct ath_softc *sc)
 		return;
 
 	parent = pdev->bus->self;
+
+	if (ah->btcoex_hw.scheme != ATH_BTCOEX_CFG_NONE) {
+		/* Bluetooth coexistance requires disabling ASPM. */
+		pci_read_config_byte(pdev, PCI_EXP_LNKCTL, &aspm);
+		aspm &= ~(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
+		pci_write_config_byte(pdev, PCI_EXP_LNKCTL, aspm);
+
+		/*
+		 * Both upstream and downstream PCIe components should
+		 * have the same ASPM settings.
+		 */
+		if (WARN_ON(!parent))
+			return;
+
+		pci_read_config_byte(parent, PCI_EXP_LNKCTL, &aspm);
+		aspm &= ~(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
+		pci_write_config_byte(parent, PCI_EXP_LNKCTL, aspm);
+
+		return;
+	}
+
 	if (WARN_ON(!parent))
 		return;
 
@@ -252,7 +255,8 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err_init;
 	}
 
-	ath_pci_check_aspm(sc);
+	/* Need to be called after we discover hw capabilities */
+	ath_pci_aspm_init(sc);
 
 	ath9k_hw_name(sc->sc_ah, hw_name, sizeof(hw_name));
 	wiphy_info(hw->wiphy, "%s mem=0x%lx, irq=%d\n",
-- 
1.7.1

Re: [RFC/RFT 5/6] ath9k: do btcoex ASPM disabling at initialization time

[Rajkumar Manoharan]

On Fri, Jul 22, 2011 at 03:31:51PM +0200, Stanislaw Gruszka wrote:
> Disable ASPM in pci .probe, to do not change settings when
> pci_bus_sem is not held.
> 
> Disable ASPM on upstream (device) and downstream (PCIe port) component.
> According to e1000e driver authors this is required. I did not find that
> requirement in PCIe spec, but it seems to be logical for me.
> 
> This need to be fixed for CONFIG_PCIEASPM, that will be done later
> during merge common code into pcie core.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/net/wireless/ath/ath9k/hw.h   |    1 -
>  drivers/net/wireless/ath/ath9k/main.c |    2 -
>  drivers/net/wireless/ath/ath9k/pci.c  |   44 ++++++++++++++++++---------------
>  3 files changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h
> index 3f941f0..6abf26d 100644
> --- a/drivers/net/wireless/ath/ath9k/hw.h
> +++ b/drivers/net/wireless/ath/ath9k/hw.h
> @@ -871,7 +871,6 @@ struct ath_bus_ops {
>  	enum ath_bus_type ath_bus_type;
>  	void (*read_cachesize)(struct ath_common *common, int *csz);
>  	bool (*eeprom_read)(struct ath_common *common, u32 off, u16 *data);
> -	void (*bt_coex_prep)(struct ath_common *common);
>  	void (*extn_synch_en)(struct ath_common *common);
>  };
>  
> diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
> index 651f633..b0389c3 100644
> --- a/drivers/net/wireless/ath/ath9k/main.c
> +++ b/drivers/net/wireless/ath/ath9k/main.c
> @@ -1141,8 +1141,6 @@ static int ath9k_start(struct ieee80211_hw *hw)
>  					   AR_STOMP_LOW_WLAN_WGHT);
>  		ath9k_hw_btcoex_enable(ah);
>  
> -		if (common->bus_ops->bt_coex_prep)
> -			common->bus_ops->bt_coex_prep(common);
>  		if (ah->btcoex_hw.scheme == ATH_BTCOEX_CFG_3WIRE)
>  			ath9k_btcoex_timer_resume(sc);
>  	}
> diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
> index 29eba63..835a50a 100644
> --- a/drivers/net/wireless/ath/ath9k/pci.c
> +++ b/drivers/net/wireless/ath/ath9k/pci.c
> @@ -88,23 +88,6 @@ static bool ath_pci_eeprom_read(struct ath_common *common, u32 off, u16 *data)
>  	return true;
>  }
>  
> -/*
> - * Bluetooth coexistance requires disabling ASPM.
> - */
> -static void ath_pci_bt_coex_prep(struct ath_common *common)
> -{
> -	struct ath_softc *sc = (struct ath_softc *) common->priv;
> -	struct pci_dev *pdev = to_pci_dev(sc->dev);
> -	u8 aspm;
> -
> -	if (!pci_is_pcie(pdev))
> -		return;
> -
> -	pci_read_config_byte(pdev, PCI_EXP_LNKCTL, &aspm);
> -	aspm &= ~(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
> -	pci_write_config_byte(pdev, PCI_EXP_LNKCTL, aspm);
> -}
> -
>  static void ath_pci_extn_synch_enable(struct ath_common *common)
>  {
>  	struct ath_softc *sc = (struct ath_softc *) common->priv;
> @@ -120,11 +103,10 @@ static const struct ath_bus_ops ath_pci_bus_ops = {
>  	.ath_bus_type = ATH_PCI,
>  	.read_cachesize = ath_pci_read_cachesize,
>  	.eeprom_read = ath_pci_eeprom_read,
> -	.bt_coex_prep = ath_pci_bt_coex_prep,
>  	.extn_synch_en = ath_pci_extn_synch_enable,
>  };
>  
> -static void ath_pci_check_aspm(struct ath_softc *sc)
> +static void ath_pci_aspm_init(struct ath_softc *sc)
>  {
>  	struct ath_hw *ah = sc->sc_ah;
>  	struct pci_dev *pdev = to_pci_dev(sc->dev);
> @@ -137,6 +119,27 @@ static void ath_pci_check_aspm(struct ath_softc *sc)
>  		return;
>  
>  	parent = pdev->bus->self;
> +
> +	if (ah->btcoex_hw.scheme != ATH_BTCOEX_CFG_NONE) {
> +		/* Bluetooth coexistance requires disabling ASPM. */
> +		pci_read_config_byte(pdev, PCI_EXP_LNKCTL, &aspm);
> +		aspm &= ~(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
> +		pci_write_config_byte(pdev, PCI_EXP_LNKCTL, aspm);
> +
> +		/*
> +		 * Both upstream and downstream PCIe components should
> +		 * have the same ASPM settings.
> +		 */
> +		if (WARN_ON(!parent))
> +			return;
> +
> +		pci_read_config_byte(parent, PCI_EXP_LNKCTL, &aspm);
> +		aspm &= ~(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
> +		pci_write_config_byte(parent, PCI_EXP_LNKCTL, aspm);
> +
Why dont you use pci_disable_link_state instead?
> +		return;
> +	}
> +
>  	if (WARN_ON(!parent))
>  		return;
>  
> @@ -252,7 +255,8 @@ static int ath_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		goto err_init;
>  	}
>  
> -	ath_pci_check_aspm(sc);
> +	/* Need to be called after we discover hw capabilities */
> +	ath_pci_aspm_init(sc);
>  
>  	ath9k_hw_name(sc->sc_ah, hw_name, sizeof(hw_name));
>  	wiphy_info(hw->wiphy, "%s mem=0x%lx, irq=%d\n",
> -- 
> 1.7.1
> 

Re: [RFC/RFT 5/6] ath9k: do btcoex ASPM disabling at initialization time

[Stanislaw Gruszka]

On Sat, Jul 23, 2011 at 10:21:01AM +0530, Rajkumar Manoharan wrote:
> On Fri, Jul 22, 2011 at 03:31:51PM +0200, Stanislaw Gruszka wrote:
> > +		pci_read_config_byte(parent, PCI_EXP_LNKCTL, &aspm);
> > +		aspm &= ~(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
> > +		pci_write_config_byte(parent, PCI_EXP_LNKCTL, aspm);
> > +
> Why dont you use pci_disable_link_state instead?

Because pci_disable_link_state is noop for CONFIG_PCIEASPM disabled.
I have further patches (based on stuff in e1000e, not finished yet)
that integrate this into pci core and work on both cases, CONFIG_PCIEASPM
disabled and enabled.

Re: [RFC/RFT 5/6] ath9k: do btcoex ASPM disabling at initialization time

[Adrian Chadd]

Thanks very much for picking up the diagnoses we did and joining the
pieces together with other APSM issues with other chipsets. :-)

I'm glad it's finally being resolved!



Adrian


On 25 July 2011 17:16, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> On Sat, Jul 23, 2011 at 10:21:01AM +0530, Rajkumar Manoharan wrote:
>> On Fri, Jul 22, 2011 at 03:31:51PM +0200, Stanislaw Gruszka wrote:
>> > +           pci_read_config_byte(parent, PCI_EXP_LNKCTL, &aspm);
>> > +           aspm &= ~(PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1);
>> > +           pci_write_config_byte(parent, PCI_EXP_LNKCTL, aspm);
>> > +
>> Why dont you use pci_disable_link_state instead?
>
> Because pci_disable_link_state is noop for CONFIG_PCIEASPM disabled.
> I have further patches (based on stuff in e1000e, not finished yet)
> that integrate this into pci core and work on both cases, CONFIG_PCIEASPM
> disabled and enabled.
>

[RFC/RFT 6/6] ath9k: always initialize hw registers related to PCIe PM

[Stanislaw Gruszka]

We might not initialize registers if ah->aspm_enabled == false, assure
we do this.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/ath/ath9k/hw.c  |    5 +----
 drivers/net/wireless/ath/ath9k/pci.c |    4 ++++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index 4ef7313..7084bb5 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -595,10 +595,7 @@ static int __ath9k_hw_init(struct ath_hw *ah)
 
 	ath9k_hw_init_mode_regs(ah);
 
-
-	if (ah->is_pciexpress)
-		ath9k_hw_configpcipowersave(ah, false);
-	else
+	if (!ah->is_pciexpress)
 		ath9k_hw_disablepcie(ah);
 
 	if (!AR_SREV_9300_20_OR_LATER(ah))
diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
index 835a50a..d1c75ec 100644
--- a/drivers/net/wireless/ath/ath9k/pci.c
+++ b/drivers/net/wireless/ath/ath9k/pci.c
@@ -118,6 +118,10 @@ static void ath_pci_aspm_init(struct ath_softc *sc)
 	if (!pci_is_pcie(pdev))
 		return;
 
+	/* initalize PCIe PM registers if device introduce itself as PCIe */
+	if (ah->is_pciexpress)
+		ath9k_hw_ops(ah)->config_pci_powersave(ah, false);
+
 	parent = pdev->bus->self;
 
 	if (ah->btcoex_hw.scheme != ATH_BTCOEX_CFG_NONE) {
-- 
1.7.1

Re: [RFC/RFT 6/6] ath9k: always initialize hw registers related to PCIe PM

[Rajkumar Manoharan]

On Fri, Jul 22, 2011 at 03:31:52PM +0200, Stanislaw Gruszka wrote:
> We might not initialize registers if ah->aspm_enabled == false, assure
> we do this.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/net/wireless/ath/ath9k/hw.c  |    5 +----
>  drivers/net/wireless/ath/ath9k/pci.c |    4 ++++
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
> index 4ef7313..7084bb5 100644
> --- a/drivers/net/wireless/ath/ath9k/hw.c
> +++ b/drivers/net/wireless/ath/ath9k/hw.c
> @@ -595,10 +595,7 @@ static int __ath9k_hw_init(struct ath_hw *ah)
>  
>  	ath9k_hw_init_mode_regs(ah);
>  
> -
> -	if (ah->is_pciexpress)
> -		ath9k_hw_configpcipowersave(ah, false);
> -	else
> +	if (!ah->is_pciexpress)
>  		ath9k_hw_disablepcie(ah);
>  
>  	if (!AR_SREV_9300_20_OR_LATER(ah))
> diff --git a/drivers/net/wireless/ath/ath9k/pci.c b/drivers/net/wireless/ath/ath9k/pci.c
> index 835a50a..d1c75ec 100644
> --- a/drivers/net/wireless/ath/ath9k/pci.c
> +++ b/drivers/net/wireless/ath/ath9k/pci.c
> @@ -118,6 +118,10 @@ static void ath_pci_aspm_init(struct ath_softc *sc)
>  	if (!pci_is_pcie(pdev))
>  		return;
>  
> +	/* initalize PCIe PM registers if device introduce itself as PCIe */
> +	if (ah->is_pciexpress)
> +		ath9k_hw_ops(ah)->config_pci_powersave(ah, false);
> +
Use ath9k_hw_configpcipowersave wrapper instead. And ensure that there is no
sideeffect by changing SERDES config from __ath9k_hw_init to pci_probe.

--
Rajkumar
>  	parent = pdev->bus->self;
>  
>  	if (ah->btcoex_hw.scheme != ATH_BTCOEX_CFG_NONE) {
> -- 
> 1.7.1
> 

Re: [RFC/RFT 6/6] ath9k: always initialize hw registers related to PCIe PM

[Stanislaw Gruszka]

On Sat, Jul 23, 2011 at 10:41:30AM +0530, Rajkumar Manoharan wrote:
> > +	/* initalize PCIe PM registers if device introduce itself as PCIe */
> > +	if (ah->is_pciexpress)
> > +		ath9k_hw_ops(ah)->config_pci_powersave(ah, false);
> > +
> Use ath9k_hw_configpcipowersave wrapper instead.
I changed wrapper to check ah->aspm_enabled, so if I would use it here
it will not setup registers. I think I should comment that. 

> And ensure that there is no
> sideeffect by changing SERDES config from __ath9k_hw_init to pci_probe.
Hmm, not sure if I understand what I have to do :-(

We have something like that:

ath_pci_probe
  ath9k_init_device
    ath9k_init_softc
      ath9k_hw_init
	__ath9k_hw_init
            ath9k_hw_configpcipowersave();

I changed it this way:

ath_pci_probe
  ath9k_init_device
    ath9k_init_softc
      ath9k_hw_init
	__ath9k_hw_init

      ath9k_init_queues
      ath9k_init_btcoex
   some other inits
     
  ath9k_hw_configpcipowersave();

Can this cause some side effects?

Re: [RFC/RFT 6/6] ath9k: always initialize hw registers related to PCIe PM

[Stanislaw Gruszka]

On Mon, Jul 25, 2011 at 11:29:30AM +0200, Stanislaw Gruszka wrote:
> On Sat, Jul 23, 2011 at 10:41:30AM +0530, Rajkumar Manoharan wrote:
> > > +	/* initalize PCIe PM registers if device introduce itself as PCIe */
> > > +	if (ah->is_pciexpress)
> > > +		ath9k_hw_ops(ah)->config_pci_powersave(ah, false);
> > > +
> > Use ath9k_hw_configpcipowersave wrapper instead.
> I changed wrapper to check ah->aspm_enabled, so if I would use it here
> it will not setup registers. I think I should comment that. 
> 
> > And ensure that there is no
> > sideeffect by changing SERDES config from __ath9k_hw_init to pci_probe.
> Hmm, not sure if I understand what I have to do :-(
> 
> We have something like that:
> 
> ath_pci_probe
>   ath9k_init_device
>     ath9k_init_softc
>       ath9k_hw_init
> 	__ath9k_hw_init
>             ath9k_hw_configpcipowersave();
> 
> I changed it this way:
> 
> ath_pci_probe
>   ath9k_init_device
>     ath9k_init_softc
>       ath9k_hw_init
> 	__ath9k_hw_init
> 
>       ath9k_init_queues
>       ath9k_init_btcoex
>    some other inits
>      
>   ath9k_hw_configpcipowersave();
> 
> Can this cause some side effects?

Actually this movement is not necessary, in the way things I did
currently. Is enough to replace ath9k_hw_configpcipowersave() by
ath9k_hw_ops(ah)->config_pci_powersave() in __ath9k_hw_init().
In such case SERDES and PCIe PM registers will be always initialized.

However movement would be needed if we want to initialize registers,
only when ASPM is enabled, hence we first need to discovery that,
and then eventually call ath9k_hw_configpcipowersave().

Which option is correct: always initialize SERDES and PCIe PM registers
or do it only if ASPM is enabled?

Stanislaw

Re: [RFC/RFT 6/6] ath9k: always initialize hw registers related to PCIe PM

[Rajkumar Manoharan]

On Mon, Jul 25, 2011 at 04:01:52PM +0200, Stanislaw Gruszka wrote:
> On Mon, Jul 25, 2011 at 11:29:30AM +0200, Stanislaw Gruszka wrote:
> > On Sat, Jul 23, 2011 at 10:41:30AM +0530, Rajkumar Manoharan wrote:
> > > > +	/* initalize PCIe PM registers if device introduce itself as PCIe */
> > > > +	if (ah->is_pciexpress)
> > > > +		ath9k_hw_ops(ah)->config_pci_powersave(ah, false);
> > > > +
> > > Use ath9k_hw_configpcipowersave wrapper instead.
> > I changed wrapper to check ah->aspm_enabled, so if I would use it here
> > it will not setup registers. I think I should comment that. 
> >
Yeah. I got it. I think ah->aspm_enabled alone is sufficient in the wrapper
function. 
> > > And ensure that there is no
> > > sideeffect by changing SERDES config from __ath9k_hw_init to pci_probe.
> > Hmm, not sure if I understand what I have to do :-(
> > 
> > We have something like that:
> > 
> > ath_pci_probe
> >   ath9k_init_device
> >     ath9k_init_softc
> >       ath9k_hw_init
> > 	__ath9k_hw_init
> >             ath9k_hw_configpcipowersave();
> > 
> > I changed it this way:
> > 
> > ath_pci_probe
> >   ath9k_init_device
> >     ath9k_init_softc
> >       ath9k_hw_init
> > 	__ath9k_hw_init
> > 
> >       ath9k_init_queues
> >       ath9k_init_btcoex
> >    some other inits
> >      
> >   ath9k_hw_configpcipowersave();
> > 
> > Can this cause some side effects?
> 
> Actually this movement is not necessary, in the way things I did
> currently. Is enough to replace ath9k_hw_configpcipowersave() by
> ath9k_hw_ops(ah)->config_pci_powersave() in __ath9k_hw_init().
> In such case SERDES and PCIe PM registers will be always initialized.
> 
> However movement would be needed if we want to initialize registers,
> only when ASPM is enabled, hence we first need to discovery that,
> and then eventually call ath9k_hw_configpcipowersave().
> 
> Which option is correct: always initialize SERDES and PCIe PM registers
> or do it only if ASPM is enabled?
>
Good point. Let them configured on ASPM case. please cross check with other
embed platform.

--
Rajkumar