[PATCH 0/8] PCI: aardvark: Fix support for Turris MOX and Compex wifi cards

[PATCH 0/8] PCI: aardvark: Fix support for Turris MOX and Compex wifi cards

[Pali Rohár]

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

[PATCH 1/8] PCI: aardvark: Set controller speed from Device Tree max-link-speed

[Pali Rohár]

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.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 32 ++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 2a20b649f40c..ad4f0fa57624 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -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

[PATCH 2/8] dts: espressobin: Define max-link-speed for pcie0

[Pali Rohár]

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.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
index 42e992f9c8a5..6705618162d5 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
@@ -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

Re: [PATCH 2/8] dts: espressobin: Define max-link-speed for pcie0

[Marek Behun]

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


On Wed, 15 Apr 2020 18:00:48 +0200
Pali Rohár <pali@kernel.org> wrote:

> 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.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
> index 42e992f9c8a5..6705618162d5 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
> @@ -47,6 +47,7 @@
>  	phys = <&comphy1 0>;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
> +	max-link-speed = <2>;
>  };
>  
>  /* J6 */

[PATCH 3/8] PCI: aardvark: Start link training immediately after enabling link training

[Pali Rohár]

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.

Fixes: f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready before
training link")

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index ad4f0fa57624..756b31c4d20b 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -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

[PATCH 4/8] PCI: aardvark: Do not overwrite Link Status register and ASPM Control bits in Link Control register

[Pali Rohár]

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.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 756b31c4d20b..02c69fc9aadf 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -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

[PATCH 5/8] PCI: aardvark: Set final controller speed based on negotiated link speed

[Pali Rohár]

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.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 35 +++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 02c69fc9aadf..a83bbc86e428 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -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

Re: [PATCH 5/8] PCI: aardvark: Set final controller speed based on negotiated link speed

[Marek Behun]

On Wed, 15 Apr 2020 18:03:45 +0200
Pali Rohár <pali@kernel.org> wrote:

> 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.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/pci/controller/pci-aardvark.c | 35 +++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 02c69fc9aadf..a83bbc86e428 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -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);
>  

This do {} while(1) should be a for cycle iterating the max_link_speed
variable, and probably in a separate function
advk_pcie_negotiate_link_speed.

A3700, which is the only SOC to use this driver, does not support PCIe
gen 3, so this shouldn't be tried, at least if
.compatible == "marvell,armada-3700-pcie".

Marek

[PATCH 6/8] PCI: aardvark: Add support for issuing PERST via GPIO

[Pali Rohár]

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.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 30 ++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index a83bbc86e428..6a97a3838098 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -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

Re: [PATCH 6/8] PCI: aardvark: Add support for issuing PERST via GPIO

[Marek Behun]

On Wed, 15 Apr 2020 18:03:46 +0200
Pali Rohár <pali@kernel.org> wrote:

> +	if (IS_ERR(pcie->reset_gpio)) {
> +		if (PTR_ERR(pcie->reset_gpio) == -ENOENT) {
> +			pcie->reset_gpio = NULL;

this assignment is redundant, the variable is already NULL, such
structures are zeroed after allocation

> +		} 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);
> +		}
> +	}

[PATCH 7/8] dts: aardvark: Route pcie reset pin to gpio function and define reset-gpios for pcie

[Pali Rohár]

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.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 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(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
index 6705618162d5..8ad4dce280c3 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
@@ -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 */
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
index bb42d1e6a4e9..e496bd9d4737 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
@@ -128,10 +128,6 @@
 	};
 };
 
-&pcie_reset_pins {
-	function = "gpio";
-};
-
 &pcie0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 000c135e39b7..7909c146eabf 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -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

Re: [PATCH 7/8] dts: aardvark: Route pcie reset pin to gpio function and define reset-gpios for pcie

[Marek Behun]

On Wed, 15 Apr 2020 18:03:47 +0200
Pali Rohár <pali@kernel.org> wrote:

> 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.
> 

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.

[PATCH 8/8] PCI: aardvark: Add FIXME for code which access PCIE_CORE_CMD_STATUS_REG

[Pali Rohár]

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.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 6a97a3838098..a1cebc734f2d 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -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

Re: [PATCH 8/8] PCI: aardvark: Add FIXME for code which access PCIE_CORE_CMD_STATUS_REG

[Pali Rohár]

On Wednesday 15 April 2020 18:03:48 Pali Rohár wrote:
> 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.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/pci/controller/pci-aardvark.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 6a97a3838098..a1cebc734f2d 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -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.

Re: [PATCH 0/8] PCI: aardvark: Fix support for Turris MOX and Compex wifi cards

[Tomasz Maciej Nowak]

W dniu 15.04.2020 o 18:00, Pali Rohár pisze:
> 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(-)
> 

For the whole series

Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>

-- 
TMN

Re: [PATCH 0/8] PCI: aardvark: Fix support for Turris MOX and Compex wifi cards

[Marek Behun]

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