PCI: aardvark: Fix support for Turris MOX and Compex wifi cards

This patch series fixes PCI aardvark controller to work on Turris MOX with Compex WLE900VX (and also other ath10k) wifi cards.

Patches are available also in my git repository in branch pci-aardvark:
https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=pci-aardvark

Pali Rohár (8):
PCI: aardvark: Set controller speed from Device Tree max-link-speed
dts: espressobin: Define max-link-speed for pcie0
PCI: aardvark: Start link training immediately after enabling link
training
PCI: aardvark: Do not overwrite Link Status register and ASPM Control
bits in Link Control register
PCI: aardvark: Set final controller speed based on negotiated link
speed
PCI: aardvark: Add support for issuing PERST via GPIO
dts: aardvark: Route pcie reset pin to gpio function and define
reset-gpios for pcie
PCI: aardvark: Add FIXME for code which access
PCIE_CORE_CMD_STATUS_REG

 .../dts/marvell/armada-3720-espressobin.dtsi  |   2 +
 .../dts/marvell/armada-3720-turris-mox.dts    |   4 -
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi  |   2 +-
 drivers/pci/controller/pci-aardvark.c         | 118 +++++++++++++++---
 4 files changed, 106 insertions(+), 20 deletions(-)

2.20.1

bindings/pci/pci.txt defines standard DT property max-link-speed for specifying PCI gen of link. Read this property from Device Tree via of_pci_get_max_link_speed() function and use it for configuring aardvark PCI controller gen speed. Before this change aardvark PCI gen speed was configured always to hardcoded value gen2. When Device Tree does not specify max-link-speed property use by default gen3 value, maximum which aardvark PCI controller supports.

---
 drivers/pci/controller/pci-aardvark.c | 32 ++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

index 2a20b649f40c..ad4f0fa57624 100644
@@ -253,8 +253,30 @@ static void advk_pcie_wait_for_retrain(struct advk_pcie *pcie)
 	}
 }
 
+static void advk_pcie_setup_link_speed(struct advk_pcie *pcie, int link_speed)
+{
+	u32 reg;
+
+	dev_info(&pcie->pdev->dev, "setup link speed to %d\n", link_speed);
+
+	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
+	reg &= ~PCIE_GEN_SEL_MSK;
+
+	if (link_speed == 3)
+		reg |= SPEED_GEN_3;
+	else if (link_speed == 2)
+		reg |= SPEED_GEN_2;
+	else
+		reg |= SPEED_GEN_1;
+
+	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
+}
+
 static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 {
+	struct device *dev = &pcie->pdev->dev;
+	struct device_node *node = dev->of_node;
+	int max_link_speed;
 	u32 reg;
 
 	/* Set to Direct mode */

@@ -288,11 +310,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 		PCIE_CORE_CTRL2_TD_ENABLE;
 	advk_writel(pcie, reg, PCIE_CORE_CTRL2_REG);
 
-	/* Set GEN2 */
-	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
-	reg &= ~PCIE_GEN_SEL_MSK;
-	reg |= SPEED_GEN_2;
-	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
+	/* Set max link speed */
+	max_link_speed = of_pci_get_max_link_speed(node);
+	if (max_link_speed <= 0 || max_link_speed > 3)
+		max_link_speed = 3;
+	advk_pcie_setup_link_speed(pcie, max_link_speed);
 
 	/* Set lane X1 */
 	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);

2.20.1

Previously aardvark PCI controller set speed to gen2. Now it reads speed from Device Tree and as default use maximal possible speed which is gen3.

Because Espressobin has advertised only PCI Express 2.0 capability and previous value was gen2, define max-link-speed to 2, so there would not be any configuration change.

---
 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi | 1 +
 1 file changed, 1 insertion(+)

index 42e992f9c8a5..6705618162d5 100644
@@ -47,6 +47,7 @@
 	phys = <&comphy1 0>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
+	max-link-speed = <2>;
 };
 
 /* J6 */

2.20.1

When chaning dts files in arch/arm64/boot/dts/marvell, use subject prefix
arm64: dts: marvell:
for espressobin for example
arm64: dts: marvell: espressobin:

instead of
dts: espressobin
or
dts: aardvark

Marek

Adding even 100ms (PCI_PM_D3COLD_WAIT) delay between enabling link training and starting link training cause that some Compex WLE900VX cards are not detected.

So move code for enabling link training after PCI_PM_D3COLD_WAIT delay.

This change fixes Compex WLE900VX cards detection on Turris MOX after cold boot.

training link")

---
 drivers/pci/controller/pci-aardvark.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

index ad4f0fa57624..756b31c4d20b 100644
@@ -322,11 +322,6 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg |= LANE_COUNT_1;
 	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
 
-	/* Enable link training */
-	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
-	reg |= LINK_TRAINING_EN;
-	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
-
 	/* Enable MSI */
 	reg = advk_readl(pcie, PCIE_CORE_CTRL2_REG);
 	reg |= PCIE_CORE_CTRL2_MSI_ENABLE;

@@ -368,6 +363,16 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	 */
 	msleep(PCI_PM_D3COLD_WAIT);
 
+	/*
+	 * Do "Enable link training" and "Start link training" in a row without
+	 * any delay between them. Adding even 100ms delay (PCI_PM_D3COLD_WAIT)
+	 * cause that some Compex WLE900VX cards are not detected.
+	 */
+
+	/* Enable link training */
+	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
+	reg |= LINK_TRAINING_EN;
+	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
 	/* Start link training */
 	reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
 	reg |= PCIE_CORE_LINK_TRAINING;

2.20.1

Trying to overwrite or change Link Status register does not have any effect as this is read-only register. Trying to overwrite bits for Negotiated Link Width value in Link Status register does not make sense. So remove code which is doing it.

In future proper change of link width can be done via Lane Count Select bits in PCIe Control 0 register.

Trying to unconditionally enable ASPM L0s via ASPM Control bits in Link Control register is wrong. There should be at least some detection if endpoint supports L0s as support for it is not mandatory.

Moreover ASPM Control bits in Link Control register are controlled by pcie/aspm.c code which sets it according to system ASPM settings, immediately after aardvark driver probe callback finish. So setting these bits by aardvark driver has no long running effect.

So remove code which touches ASPM L0s bits from aardvark driver and let kernel's ASPM implementation to set ASPM state properly.

Some users are reporting issues that this code which unconditionally set ASPM L0s bits in Link Control register is problematic for some Intel wifi cards. And disabling that code fixes support for those cards. See e.g.:
https://bugzilla.kernel.org/show_bug.cgi?id=196339

If problem with Intel wifi cards occur also after this commit then driver independent pcie/aspm.c code could be modified / hooked to not enable ASPM L0s state for affected problematic cards.

---
 drivers/pci/controller/pci-aardvark.c | 4 ----
 1 file changed, 4 deletions(-)

index 756b31c4d20b..02c69fc9aadf 100644
@@ -380,10 +380,6 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 
 	advk_pcie_wait_for_link(pcie);
 
-	reg = PCIE_CORE_LINK_L0S_ENTRY |
-		(1 << PCIE_CORE_LINK_WIDTH_SHIFT);
-	advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
-
 	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
 	reg |= PCIE_CORE_CMD_MEM_ACCESS_EN |
 		PCIE_CORE_CMD_IO_ACCESS_EN |

2.20.1

Some Compex WLE900VX gen1 cards are unstable or even not detected when aardvark PCI controller speed is set to gen2. Moreover when ASPM code tries to retrain link second time then these cards stop responding and link goes down. If aardvark PCI controller is set to gen1 then these cards work fine without any problem.

Unconditionally forcing aardvark controller to gen1 speed (either via DT property max-link-speed or hardcoded value in driver itself) is not a solution as it would have performance impact for fast gen2 sata cards.

To overcome this problem, try all 3 possible speeds (gen3, gen2, gen1) supported by aardvark PCI controller with respect to max-link-speed setting and after successful link training choose final controller speed based on Negotiated Link Speed from Link Status register, which should match card speed.

I tested this change with Compex cards WLE200NX (pcie 1.0, gen1, ath9k), WLE900VX (pcie 1.1, gen1, ath10k) and WLE1216V5-20 (pcie 2.0, gen2, ath10k) on Turris MOX. Tomasz Maciej Nowak tested JJPlus JWX6051 (ath9k), Intel 622ANHMW, MT76 U7612E-H1 and Compex WLE1216V2-20 cards on Espressobin.

---
 drivers/pci/controller/pci-aardvark.c | 35 +++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

index 02c69fc9aadf..a83bbc86e428 100644
@@ -40,6 +40,7 @@
 #define PCIE_CORE_LINK_CTRL_STAT_REG				0xd0
 #define     PCIE_CORE_LINK_L0S_ENTRY				BIT(0)
 #define     PCIE_CORE_LINK_TRAINING				BIT(5)
+#define     PCIE_CORE_LINK_SPEED_SHIFT				16
 #define     PCIE_CORE_LINK_WIDTH_SHIFT				20
 #define PCIE_CORE_ERR_CAPCTL_REG				0x118
 #define     PCIE_CORE_ERR_CAPCTL_ECRC_CHK_TX			BIT(5)

@@ -276,7 +277,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 {
 	struct device *dev = &pcie->pdev->dev;
 	struct device_node *node = dev->of_node;
-	int max_link_speed;
+	int max_link_speed, neg_link_speed;
 	u32 reg;
 
 	/* Set to Direct mode */

@@ -378,7 +379,37 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg |= PCIE_CORE_LINK_TRAINING;
 	advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
 
-	advk_pcie_wait_for_link(pcie);
+	do {
+		if (advk_pcie_wait_for_link(pcie) < 0) {
+			max_link_speed--;
+		} else {
+			reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
+
+			neg_link_speed =
+				(reg >> PCIE_CORE_LINK_SPEED_SHIFT) & 0xf;
+			dev_info(dev, "negotiated link speed %d\n",
+				neg_link_speed);
+
+			if (neg_link_speed == max_link_speed)
+				break;
+
+			if (neg_link_speed > 0 && neg_link_speed <= 3)
+				max_link_speed = neg_link_speed;
+			else
+				max_link_speed--;
+		}
+
+		if (max_link_speed == 0)
+			break;
+
+		/* Set new decreased max link speed */
+		advk_pcie_setup_link_speed(pcie, max_link_speed);
+
+		/* And start link training again */
+		reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
+		reg |= PCIE_CORE_LINK_TRAINING;
+		advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
+	} while (1);
 
 	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
 	reg |= PCIE_CORE_CMD_MEM_ACCESS_EN |

2.20.1

bindings/pci/pci.txt defines standard DT property reset-gpios for specifying PERST GPIO. Read this property from Device Tree via devm_gpiod_get_from_of_node() function. As this property is optional, function may return -ENOENT. During initialization of aardvark PCI controller toggle supplied GPIO to issue PERST.

Some Compex ath10k cards (e.g. WLE900VX or WLE1216) are not detected after reboot when PERST is not issued during driver initialization. And Compex WLE1216 cards need to be in reset state for at least 1ms otherwise they are not detected too.

Tested on Turris MOX and after this change Compex cards are detected also after rebooting board.

---
 drivers/pci/controller/pci-aardvark.c | 30 ++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

index a83bbc86e428..6a97a3838098 100644
@@ -9,6 +9,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/gpio.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>

@@ -18,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/msi.h>
 #include <linux/of_address.h>
+#include <linux/of_gpio.h>
 #include <linux/of_pci.h>
 
 #include "../pci.h"

@@ -203,6 +205,7 @@ struct advk_pcie {
 	u16 msi_msg;
 	int root_bus_nr;
 	struct pci_bridge_emul bridge;
+	struct gpio_desc *reset_gpio;
 };
 
 static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)

@@ -280,6 +283,14 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	int max_link_speed, neg_link_speed;
 	u32 reg;
 
+	if (pcie->reset_gpio) {
+		dev_info(dev, "issuing PERST via reset GPIO for 1ms\n");
+		gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+		/* Detection of some Compex WLE1216 cards needs at least 1ms */
+		mdelay(1);
+		gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+	}
+
 	/* Set to Direct mode */
 	reg = advk_readl(pcie, CTRL_CONFIG_REG);
 	reg &= ~(CTRL_MODE_MASK << CTRL_MODE_SHIFT);

@@ -358,7 +369,8 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 
 	/*
 	 * PERST# signal could have been asserted by pinctrl subsystem before
-	 * probe() callback has been called, making the endpoint going into
+	 * probe() callback has been called or issued explicitly by reset gpio
+	 * routine at beginning of this function, making the endpoint going into
 	 * fundamental reset. As required by PCI Express spec a delay for at
 	 * least 100ms after such a reset before link training is needed.
 	 */

@@ -1043,6 +1055,22 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	}
 	pcie->root_bus_nr = bus->start;
 
+	/* Returns -ENOENT if reset-gpios property is not populated */
+	pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
+						       "reset-gpios", 0,
+						       GPIOD_OUT_LOW,
+						       "pcie1-reset");
+	if (IS_ERR(pcie->reset_gpio)) {
+		if (PTR_ERR(pcie->reset_gpio) == -ENOENT) {
+			pcie->reset_gpio = NULL;
+		} else {
+			if (PTR_ERR(pcie->reset_gpio) != -EPROBE_DEFER)
+				dev_err(dev, "Failed to retrieve reset GPIO (%ld)\n",
+					PTR_ERR(pcie->reset_gpio));
+			return PTR_ERR(pcie->reset_gpio);
+		}
+	}
+
 	advk_pcie_setup_hw(pcie);
 
 	advk_sw_pci_bridge_init(pcie);

2.20.1

Marvell version of u-boot for Espressobin set pcie reset pin to gpio and toggle it when initializing u-boot aardvark driver.

To not depend on bootloader version and state of Espressobin HW, route pcie reset pin to gpio function and define reset-gpios also in kernel. So pcie aardvark driver can trigger needed reset.

Turris MOX dts file has already defined reset-gpios and configured pcie reset pin to gpio function, so unify Espressobin and Turris MOX dts files.

---
 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi | 1 +
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts   | 4 ----
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi             | 2 +-
 3 files changed, 2 insertions(+), 5 deletions(-)

index 6705618162d5..8ad4dce280c3 100644
@@ -48,6 +48,7 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
 	max-link-speed = <2>;
+	reset-gpios = <&gpiosb 3 GPIO_ACTIVE_LOW>;
 };
 
 /* J6 */

index bb42d1e6a4e9..e496bd9d4737 100644
@@ -128,10 +128,6 @@
 	};
 };
 
-&pcie_reset_pins {
-	function = "gpio";
-};
-
 &pcie0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;

index 000c135e39b7..7909c146eabf 100644
@@ -317,7 +317,7 @@
 
 				pcie_reset_pins: pcie-reset-pins {
 					groups = "pcie1";
-					function = "pcie";
+					function = "gpio";
 				};
 
 				pcie_clkreq_pins: pcie-clkreq-pins {

2.20.1

Lets specify in the commit message the other information we found out.

This pin, according to specification, can be in two modes:
- GPIO (controlled by the GPIO subsystem)
- EP_PCIE1_Resetn (which should be controlled by PCIe subsystem)

Commit f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready before training link") says that when pinctrl driver changes this pin's mode from GPIO to PCIe, the signal is asserted for a little while. Since this pin is in GPIO mode after reset (and if U-Boot probes its own pci-aardvark driver, it also leaves it in GPIO mode), this always happens.

We found out that we are unable to control this pin when in PCIe mode. There is a register in the PCIe registers of this SOC, called PERSTN_GPIO_EN (D0088004[3]), but changing the value of this register does not change the pin output when measuring with voltmeter. We do not know if this is a bug in the SOC, or if it works only when PCIe controller is in a certain state.

So now the state of things is that the PERST signal is issued, but only by chance, due to pinctrl machinations mentioned above. We think that the PERST signal should be instead issued in a known way from the pci-aardvark driver, therefore we change the function of this pin to GPIO, so that the driver can issue it via GPIO subsystem.

Some of this explanation should also go as a comment into the dtsi file.

Register PCIE_CORE_CMD_STATUS_REG is applicable only when aardvark controller is configured for Endpoint mode. Which is not the case of current kernel driver.

---
 drivers/pci/controller/pci-aardvark.c | 6 ++++++
 1 file changed, 6 insertions(+)

index 6a97a3838098..a1cebc734f2d 100644
@@ -423,6 +423,12 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 		advk_writel(pcie, reg, PCIE_CORE_LINK_CTRL_STAT_REG);
 	} while (1);
 
+	/*
+	 * FIXME: Following code which access PCIE_CORE_CMD_STATUS_REG register
+	 *        is suspicious. This register is applicable only when the PCI
+	 *        controller is configured for Endpoint mode. And not when it
+	 *        is configured for Root Complex.
+	 */
 	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
 	reg |= PCIE_CORE_CMD_MEM_ACCESS_EN |
 		PCIE_CORE_CMD_IO_ACCESS_EN |

2.20.1

Just to note, above code needs to be removed or fixed or properly commented why is correct and what is doing.

I really do not know, so I'm adding at least FIXME comment to document suspicious code which may cause unexpected problems.

W dniu 15.04.2020 o 18:00, Pali Rohár pisze:

For the whole series

TMN

Pali, I tested this series with Compex WLE900VX and with a ASMedia SATA card.

Both are visible with these patches.

But if I enable the pci-driver in U-Boot, kernel reports "link never came up" fo the WLE900VX card. The SATA card works in this case.

advk-pcie d0070000.pcie: issuing PERST via reset GPIO for 1ms advk-pcie d0070000.pcie: setup link speed to 2 advk-pcie d0070000.pcie: link never came up advk-pcie d0070000.pcie: setup link speed to 1 advk-pcie d0070000.pcie: link never came up

We should try to somehow reset the whole PCIe controller in Linux. There are the PCIe Core Warm Reset and PCIe PHY Warm Reset register. I also think that maybe we should try to reset the whole PCI comphy.

Marek