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

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

[Marek Behún]

Hello,

this is the second version of the patch series for Armada 3720 PCIe
controller (aardvark). It's main purpose is to fix some bugs regarding
buggy ath10k cards, but we also found out some suspicious stuff about
the driver and the SOC itself, which we try to address.

Changes since v1:
- commit titles and messages were reviewed and some of them were rewritten
- patches 1 and 5 from v1 which touch PCIe speed configuration were
  reworked into one patch
- patch 2 from v1 was removed, it is not needed anymore
- patch 7 from v1 now touches the device tree of armada-3720-db
- a patch was added that tries to enable PCIe PHY via generic-phy API
  (if a phandle to the PHY is found in the device tree)
- a patch describing the new PCIe node DT properties was added
- a patch was added that moves the PHY phandle from board device trees
  to armada-37xx.dtsi

Marek and Pali

Marek Behún (5):
  PCI: aardvark: improve link training
  PCI: aardvark: add PHY support
  dt-bindings: PCI: aardvark: describe new properties
  arm64: dts: marvell: armada-37xx: set pcie_reset_pin to gpio function
  arm64: dts: marvell: armada-37xx: move PCIe comphy handle property

Pali Rohár (4):
  PCI: aardvark: train link immediately after enabling training
  PCI: aardvark: don't write to read-only register
  PCI: aardvark: issue PERST via GPIO
  PCI: aardvark: add FIXME comment for PCIE_CORE_CMD_STATUS_REG access

 .../devicetree/bindings/pci/aardvark-pci.txt  |   4 +
 .../arm64/boot/dts/marvell/armada-3720-db.dts |   3 +
 .../dts/marvell/armada-3720-espressobin.dtsi  |   2 +-
 .../dts/marvell/armada-3720-turris-mox.dts    |   5 -
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi  |   3 +-
 drivers/pci/controller/pci-aardvark.c         | 219 +++++++++++++++---
 6 files changed, 203 insertions(+), 33 deletions(-)

-- 
2.24.1

[PATCH v2 1/9] PCI: aardvark: train link immediately after enabling training

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Adding even 100ms (PCI_PM_D3COLD_WAIT) delay between enabling link
training and starting link training causes detection issues with some
buggy cards (such as Compex WLE900VX).

Move the code which enables link training immediately before the one
which starts link traning.

This fixes detection issues of Compex WLE900VX card on Turris MOX after
cold boot.

Fixes: f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready...")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/pci-aardvark.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 2a20b649f40c..b59198a102d0 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -300,11 +300,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;
@@ -346,7 +341,15 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	 */
 	msleep(PCI_PM_D3COLD_WAIT);
 
-	/* Start link training */
+	/* 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 immediately after enabling it. This solves
+	 * problems for some buggy cards.
+	 */
 	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);
-- 
2.24.1

[PATCH v2 2/9] PCI: aardvark: don't write to read-only register

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Trying to change Link Status register does not have any effect as this
is a read-only register. Trying to overwrite bits for Negotiated Link
Width does not make sense.

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 isn't 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 probes. So setting these bits by
aardvark driver has no long running effect.

Remove code which touches ASPM L0s bits from this driver and let
kernel's ASPM implementation to set ASPM state properly.

Some users are reporting issues that this code is problematic for some
Intel wifi cards and removing it fixes them, see e.g.:
https://bugzilla.kernel.org/show_bug.cgi?id=196339

If problems with Intel wifi cards occur even after this commit, then
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 b59198a102d0..551d98174613 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -356,10 +356,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.24.1

[PATCH v2 3/9] PCI: aardvark: improve link training

[Marek Behún]

Currently the aardvark driver trains link in PCIe gen2 mode. This may
cause some buggy gen 1 cards (such as Compex WLE900VX) to be unstable or
even not detected. Moreover when ASPM code tries to retrain link second
time, these cards may stop responding and link goes down. If gen1 is
used this does not happen.

Unconditionally forcing gen1 is not a good solution since it may have
performance impact on gen2 cards.

To overcome this, read 'max-link-speed' property (as defined in PCI
device tree bindings) and use this as max gen mode. Then iteratively try
link training at this mode or lower until successful. After successful
link training choose final controlled gen based on Negotiated Link Speed
from Link Status register, which should match card speed.

Signed-off-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/controller/pci-aardvark.c | 111 ++++++++++++++++++++------
 1 file changed, 86 insertions(+), 25 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 551d98174613..606bae1e7a88 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)
@@ -201,6 +202,7 @@ struct advk_pcie {
 	struct mutex msi_used_lock;
 	u16 msi_msg;
 	int root_bus_nr;
+	int link_gen;
 	struct pci_bridge_emul bridge;
 };
 
@@ -225,20 +227,16 @@ static int advk_pcie_link_up(struct advk_pcie *pcie)
 
 static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
 {
-	struct device *dev = &pcie->pdev->dev;
 	int retries;
 
 	/* check if the link is up or not */
 	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
-		if (advk_pcie_link_up(pcie)) {
-			dev_info(dev, "link up\n");
+		if (advk_pcie_link_up(pcie))
 			return 0;
-		}
 
 		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
 	}
 
-	dev_err(dev, "link never came up\n");
 	return -ETIMEDOUT;
 }
 
@@ -253,6 +251,84 @@ static void advk_pcie_wait_for_retrain(struct advk_pcie *pcie)
 	}
 }
 
+static int advk_pcie_train_at_gen(struct advk_pcie *pcie, int gen)
+{
+	int ret, neg_gen;
+	u32 reg;
+
+	/* Setup link speed */
+	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
+	reg &= ~PCIE_GEN_SEL_MSK;
+	if (gen == 2)
+		reg |= SPEED_GEN_2;
+	else
+		reg |= SPEED_GEN_1;
+	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
+
+	/*
+	 * Enable link training. This is not needed in every call to this
+	 * function, just once suffices, but it does not break anything either.
+	 */
+	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
+	reg |= LINK_TRAINING_EN;
+	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
+
+	/*
+	 * Start link training immediately after enabling it. This solves
+	 * problems for some buggy cards.
+	 */
+	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);
+
+	ret = advk_pcie_wait_for_link(pcie);
+	if (ret)
+		return ret;
+
+	reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
+	neg_gen = (reg >> PCIE_CORE_LINK_SPEED_SHIFT) & 0xf;
+
+	return neg_gen;
+}
+
+static void advk_pcie_train_link(struct advk_pcie *pcie)
+{
+	struct device *dev = &pcie->pdev->dev;
+	int neg_gen = -1, gen;
+
+	/*
+	 * Try link training at link gen specified by device tree property
+	 * 'max-link-speed' (defaults to 2, since this controller does not
+	 * support higher gen). If this fails, iteratively train at lower gen.
+	 */
+	for (gen = pcie->link_gen; gen > 0; --gen) {
+		neg_gen = advk_pcie_train_at_gen(pcie, gen);
+		if (neg_gen > 0)
+			break;
+	}
+
+	if (neg_gen < 0)
+		goto err;
+
+	/*
+	 * After successful training if negotiated gen is lower than requested,
+	 * train again on negotiated gen. This solves some stability issues for
+	 * some buggy gen1 cards.
+	 */
+	if (neg_gen < gen) {
+		gen = neg_gen;
+		neg_gen = advk_pcie_train_at_gen(pcie, gen);
+	}
+
+	if (neg_gen == gen) {
+		dev_info(dev, "link up at gen %i\n", gen);
+		return;
+	}
+
+err:
+	dev_err(dev, "link never came up\n");
+}
+
 static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 {
 	u32 reg;
@@ -288,12 +364,6 @@ 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 lane X1 */
 	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
 	reg &= ~LANE_CNT_MSK;
@@ -341,20 +411,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	 */
 	msleep(PCI_PM_D3COLD_WAIT);
 
-	/* 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 immediately after enabling it. This solves
-	 * problems for some buggy cards.
-	 */
-	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);
-
-	advk_pcie_wait_for_link(pcie);
+	advk_pcie_train_link(pcie);
 
 	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
 	reg |= PCIE_CORE_CMD_MEM_ACCESS_EN |
@@ -988,6 +1045,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	}
 	pcie->root_bus_nr = bus->start;
 
+	pcie->link_gen = of_pci_get_max_link_speed(dev->of_node);
+	if (pcie->link_gen < 1 || pcie->link_gen > 2)
+		pcie->link_gen = 2;
+
 	advk_pcie_setup_hw(pcie);
 
 	advk_sw_pci_bridge_init(pcie);
-- 
2.24.1

[PATCH v2 4/9] PCI: aardvark: issue PERST via GPIO

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Add support for issuing PERST via GPIO specified in 'reset-gpios'
property (as described in PCI device tree bindings).

Some buggy cards (e.g. Compex WLE900VX or WLE1216) are not detected
after reboot when PERST is not issued during driver initialization.

Tested on Turris MOX.

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

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 606bae1e7a88..e2d18094d8ca 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"
@@ -204,6 +206,7 @@ struct advk_pcie {
 	int root_bus_nr;
 	int link_gen;
 	struct pci_bridge_emul bridge;
+	struct gpio_desc *reset_gpio;
 };
 
 static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
@@ -329,10 +332,23 @@ static void advk_pcie_train_link(struct advk_pcie *pcie)
 	dev_err(dev, "link never came up\n");
 }
 
+static void advk_pcie_issue_perst(struct advk_pcie *pcie)
+{
+	if (!pcie->reset_gpio)
+		return;
+
+	dev_info(&pcie->pdev->dev, "issuing PERST via reset GPIO for 1ms\n");
+	gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+	usleep_range(1000, 2000);
+	gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+}
+
 static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 {
 	u32 reg;
 
+	advk_pcie_issue_perst(pcie);
+
 	/* Set to Direct mode */
 	reg = advk_readl(pcie, CTRL_CONFIG_REG);
 	reg &= ~(CTRL_MODE_MASK << CTRL_MODE_SHIFT);
@@ -1045,6 +1061,22 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	}
 	pcie->root_bus_nr = bus->start;
 
+	pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
+						       "reset-gpios", 0,
+						       GPIOD_OUT_LOW,
+						       "pcie1-reset");
+	ret = PTR_ERR_OR_ZERO(pcie->reset_gpio);
+	if (ret) {
+		if (ret == -ENOENT) {
+			pcie->reset_gpio = NULL;
+		} else {
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Failed to get reset-gpio: %i\n",
+					ret);
+			return ret;
+		}
+	}
+
 	pcie->link_gen = of_pci_get_max_link_speed(dev->of_node);
 	if (pcie->link_gen < 1 || pcie->link_gen > 2)
 		pcie->link_gen = 2;
-- 
2.24.1

[PATCH v2 5/9] PCI: aardvark: add FIXME comment for PCIE_CORE_CMD_STATUS_REG access

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Register PCIE_CORE_CMD_STATUS_REG is applicable only when the controller
is configured for Endpoint mode, which is not the case for the current
version of this driver.

Add a FIXME comment, since this needs to be explained, removed or fixed.

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 e2d18094d8ca..e893d7d8859f 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -429,6 +429,12 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 
 	advk_pcie_train_link(pcie);
 
+	/*
+	 * 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.24.1

[PATCH v2 6/9] PCI: aardvark: add PHY support

[Marek Behún]

With recent proposed changes for U-Boot it is possible that bootloader
won't initialize the PHY for this controller (currently the PHY is
initialized regardless whether PCI is used in U-Boot, but with these
proposed changes the PHY is initialized only on request).

Since the mvebu-a3700-comphy driver by Miquèl Raynal supports enabling
PCIe PHY, and since Linux' functionality should be independent on what
bootloader did, add code for enabling generic PHY if found in device OF
node.

The mvebu-a3700-comphy driver does PHY powering via SMC calls to ARM
Trusted Firmware. The corresponding code in ARM Trusted Firmware skips
one register write which U-Boot does not: step 7 ("Enable TX"), see [1].
Instead ARM Trusted Firmware expects PCIe driver to do this step,
probably because the register is in PCIe controller address space,
instead of PHY address space. We therefore add this step into the
advk_pcie_setup_hw function.

[1] https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/drivers/marvell/comphy/phy-comphy-3700.c?h=v2.3-rc2#n836

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Miquèl Raynal <miquel.raynal@bootlin.com>
---
 drivers/pci/controller/pci-aardvark.c | 69 +++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index e893d7d8859f..2a48f77f82fd 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/pci.h>
 #include <linux/init.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/msi.h>
 #include <linux/of_address.h>
@@ -104,6 +105,8 @@
 #define     PCIE_CORE_CTRL2_STRICT_ORDER_ENABLE	BIT(5)
 #define     PCIE_CORE_CTRL2_OB_WIN_ENABLE	BIT(6)
 #define     PCIE_CORE_CTRL2_MSI_ENABLE		BIT(10)
+#define PCIE_CORE_REF_CLK_REG			(CONTROL_BASE_ADDR + 0x14)
+#define     PCIE_CORE_REF_CLK_TX_ENABLE		BIT(1)
 #define PCIE_MSG_LOG_REG			(CONTROL_BASE_ADDR + 0x30)
 #define PCIE_ISR0_REG				(CONTROL_BASE_ADDR + 0x40)
 #define PCIE_MSG_PM_PME_MASK			BIT(7)
@@ -207,6 +210,7 @@ struct advk_pcie {
 	int link_gen;
 	struct pci_bridge_emul bridge;
 	struct gpio_desc *reset_gpio;
+	struct phy *phy;
 };
 
 static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
@@ -349,6 +353,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 
 	advk_pcie_issue_perst(pcie);
 
+	/* Enable TX */
+	reg = advk_readl(pcie, PCIE_CORE_REF_CLK_REG);
+	reg |= PCIE_CORE_REF_CLK_TX_ENABLE;
+	advk_writel(pcie, reg, PCIE_CORE_REF_CLK_REG);
+
 	/* Set to Direct mode */
 	reg = advk_readl(pcie, CTRL_CONFIG_REG);
 	reg &= ~(CTRL_MODE_MASK << CTRL_MODE_SHIFT);
@@ -1030,6 +1039,62 @@ static irqreturn_t advk_pcie_irq_handler(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static void __maybe_unused advk_pcie_disable_phy(struct advk_pcie *pcie)
+{
+	phy_power_off(pcie->phy);
+	phy_exit(pcie->phy);
+}
+
+static int advk_pcie_enable_phy(struct advk_pcie *pcie)
+{
+	int ret;
+
+	if (!pcie->phy)
+		return 0;
+
+	ret = phy_init(pcie->phy);
+	if (ret)
+		return ret;
+
+	ret = phy_set_mode(pcie->phy, PHY_MODE_PCIE);
+	if (ret) {
+		phy_exit(pcie->phy);
+		return ret;
+	}
+
+	ret = phy_power_on(pcie->phy);
+	if (ret) {
+		phy_exit(pcie->phy);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int advk_pcie_setup_phy(struct advk_pcie *pcie)
+{
+	struct device *dev = &pcie->pdev->dev;
+	struct device_node *node = dev->of_node;
+	int ret = 0;
+
+	pcie->phy = devm_of_phy_get(dev, node, NULL);
+	if (IS_ERR(pcie->phy) && (PTR_ERR(pcie->phy) == -EPROBE_DEFER))
+		return PTR_ERR(pcie->phy);
+
+	/* Old bindings miss the PHY handle */
+	if (IS_ERR(pcie->phy)) {
+		dev_warn(dev, "PHY unavailable (%ld)\n", PTR_ERR(pcie->phy));
+		pcie->phy = NULL;
+		return 0;
+	}
+
+	ret = advk_pcie_enable_phy(pcie);
+	if (ret)
+		dev_err(dev, "Failed to initialize PHY (%d)\n", ret);
+
+	return ret;
+}
+
 static int advk_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1087,6 +1152,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	if (pcie->link_gen < 1 || pcie->link_gen > 2)
 		pcie->link_gen = 2;
 
+	ret = advk_pcie_setup_phy(pcie);
+	if (ret)
+		return ret;
+
 	advk_pcie_setup_hw(pcie);
 
 	advk_sw_pci_bridge_init(pcie);
-- 
2.24.1

[PATCH v2 7/9] dt-bindings: PCI: aardvark: describe new properties

[Marek Behún]

Document the possibility to reference a PHY and reset-gpios and to set
max-link-speed property.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/pci/aardvark-pci.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/aardvark-pci.txt b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
index 310ef7145c47..2b8ca920a7fa 100644
--- a/Documentation/devicetree/bindings/pci/aardvark-pci.txt
+++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt
@@ -19,6 +19,9 @@ contain the following properties:
  - interrupt-map-mask and interrupt-map: standard PCI properties to
    define the mapping of the PCIe interface to interrupt numbers.
  - bus-range: PCI bus numbers covered
+ - phys: the PCIe PHY handle
+ - max-link-speed: see pci.txt
+ - reset-gpios: see pci.txt
 
 In addition, the Device Tree describing an Aardvark PCIe controller
 must include a sub-node that describes the legacy interrupt controller
@@ -48,6 +51,7 @@ Example:
 				<0 0 0 2 &pcie_intc 1>,
 				<0 0 0 3 &pcie_intc 2>,
 				<0 0 0 4 &pcie_intc 3>;
+		phys = <&comphy1 0>;
 		pcie_intc: interrupt-controller {
 			interrupt-controller;
 			#interrupt-cells = <1>;
-- 
2.24.1

[PATCH v2 8/9] arm64: dts: marvell: armada-37xx: set pcie_reset_pin to gpio function

[Marek Behún]

We found out that we are unable to control the PERST# signal via the
default pin dedicated to be PERST# pin (GPIO2[3] pin) on A3700 SOC when
this pin is in EP_PCIE1_Resetn mode. There is a register in the PCIe
register space 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.

Commit f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready
before training link") says that when this pin changes pinctrl mode
from EP_PCIE1_Resetn to GPIO, the PERST# signal is asserted for a brief
moment.

So currently the situation is that on A3700 boards the PERST# signal is
asserted in U-Boot (because the code in U-Boot issues reset via this pin
via GPIO mode), and then in Linux by the obscure and undocumented
mechanism described by the above mentioned commit.

We want to issue PERST# signal in a known way, therefore this patch
changes the pcie_reset_pin function from "pcie" to "gpio" and adds the
reset-gpios property to the PCIe node in device tree files of
EspressoBin and Armada 3720 Dev Board (Turris Mox device tree already
has this property and uDPU does not have a PCIe port).

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Remi Pommarel <repk@triplefau.lt>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 arch/arm64/boot/dts/marvell/armada-3720-db.dts           | 3 +++
 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 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-db.dts b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
index f2cc00594d64..3e5789f37206 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-db.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
@@ -128,6 +128,9 @@ phy1: ethernet-phy@1 {
 
 /* CON15(V2.0)/CON17(V1.4) : PCIe / CON15(V2.0)/CON12(V1.4) :mini-PCIe */
 &pcie0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
+	reset-gpios = <&gpiosb 3 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
 
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
index 42e992f9c8a5..c92ad664cb0e 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 @@ &pcie0 {
 	phys = <&comphy1 0>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
+	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 @@ rtc@6f {
 	};
 };
 
-&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 @@ sdio_pins: sdio-pins {
 
 				pcie_reset_pins: pcie-reset-pins {
 					groups = "pcie1";
-					function = "pcie";
+					function = "gpio";
 				};
 
 				pcie_clkreq_pins: pcie-clkreq-pins {
-- 
2.24.1

[PATCH v2 9/9] arm64: dts: marvell: armada-37xx: move PCIe comphy handle property

[Marek Behún]

Move the comphy handle property of the PCIe node from board specific
device tree files (EspressoBin and Turris Mox) to the generic
armada-37xx.dtsi.

This is correct since this is the only possible PCIe PHY configuration
on Armada 37xx, so when PCIe is enabled on any board, this handle is
correct.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
 arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi | 1 -
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts   | 1 -
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi             | 1 +
 3 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
index c92ad664cb0e..b97218c72727 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-3720-espressobin.dtsi
@@ -44,7 +44,6 @@ vcc_sd_reg1: regulator {
 /* J9 */
 &pcie0 {
 	status = "okay";
-	phys = <&comphy1 0>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&pcie_reset_pins &pcie_clkreq_pins>;
 	reset-gpios = <&gpiosb 3 GPIO_ACTIVE_LOW>;
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 e496bd9d4737..15c1cf5c5b69 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
@@ -134,7 +134,6 @@ &pcie0 {
 	status = "okay";
 	max-link-speed = <2>;
 	reset-gpios = <&gpiosb 3 GPIO_ACTIVE_LOW>;
-	phys = <&comphy1 0>;
 
 	/* enabled by U-Boot if PCIe module is present */
 	status = "disabled";
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 7909c146eabf..5aaad64a793d 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -493,6 +493,7 @@ pcie0: pcie@d0070000 {
 					<0 0 0 2 &pcie_intc 1>,
 					<0 0 0 3 &pcie_intc 2>,
 					<0 0 0 4 &pcie_intc 3>;
+			phys = <&comphy1 0>;
 			pcie_intc: interrupt-controller {
 				interrupt-controller;
 				#interrupt-cells = <1>;
-- 
2.24.1

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

[Pali Rohár]

On Tuesday 21 April 2020 13:16:52 Marek Behún wrote:
> Hello,
> 
> this is the second version of the patch series for Armada 3720 PCIe
> controller (aardvark). It's main purpose is to fix some bugs regarding
> buggy ath10k cards, but we also found out some suspicious stuff about
> the driver and the SOC itself, which we try to address.
> 
> Changes since v1:
> - commit titles and messages were reviewed and some of them were rewritten
> - patches 1 and 5 from v1 which touch PCIe speed configuration were
>   reworked into one patch
> - patch 2 from v1 was removed, it is not needed anymore
> - patch 7 from v1 now touches the device tree of armada-3720-db
> - a patch was added that tries to enable PCIe PHY via generic-phy API
>   (if a phandle to the PHY is found in the device tree)
> - a patch describing the new PCIe node DT properties was added
> - a patch was added that moves the PHY phandle from board device trees
>   to armada-37xx.dtsi
> 
> Marek and Pali
> 
> Marek Behún (5):
>   PCI: aardvark: improve link training
>   PCI: aardvark: add PHY support
>   dt-bindings: PCI: aardvark: describe new properties
>   arm64: dts: marvell: armada-37xx: set pcie_reset_pin to gpio function
>   arm64: dts: marvell: armada-37xx: move PCIe comphy handle property
> 
> Pali Rohár (4):
>   PCI: aardvark: train link immediately after enabling training
>   PCI: aardvark: don't write to read-only register
>   PCI: aardvark: issue PERST via GPIO
>   PCI: aardvark: add FIXME comment for PCIE_CORE_CMD_STATUS_REG access
> 
>  .../devicetree/bindings/pci/aardvark-pci.txt  |   4 +
>  .../arm64/boot/dts/marvell/armada-3720-db.dts |   3 +
>  .../dts/marvell/armada-3720-espressobin.dtsi  |   2 +-
>  .../dts/marvell/armada-3720-turris-mox.dts    |   5 -
>  arch/arm64/boot/dts/marvell/armada-37xx.dtsi  |   3 +-
>  drivers/pci/controller/pci-aardvark.c         | 219 +++++++++++++++---
>  6 files changed, 203 insertions(+), 33 deletions(-)
> 
> -- 
> 2.24.1
> 

Hello!

I tested whole patch series on Turris MOX with following wifi cards and
all of them are working fine from cold boot and also after board reboot.

WLE1216V5-20 (gen2, ath10k), WLE900VX (gen1, ath10k), WLE200N2 (gen1,
ath9k), WLE200NX (gen1, ath9k)

Tested-by: Pali Rohár <pali@kernel.org>

Re: [PATCH v2 2/9] PCI: aardvark: don't write to read-only register

[Bjorn Helgaas]

[+cc Rob]

In the next round, please capitalize the first word of the subjects of
the whole series to match:

  $ git log --oneline drivers/pci/controller/pci-aardvark.c
  4e5be6f81be7 ("PCI: aardvark: Use pci_parse_request_of_pci_ranges()")
  e078723f9ccc ("PCI: aardvark: Fix big endian support")
  7fbcb5da811b ("PCI: aardvark: Don't rely on jiffies while holding spinlock")
  c0f05a6ab525 ("PCI: aardvark: Fix PCI_EXP_RTCTL register configuration")
  f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready before training link")
  364b3f1ff8f0 ("PCI: aardvark: Use LTSSM state to build link training flag")

The important thing for the subject of this patch is not the "don't
write to read-only register" part; it's true that there's no point in
writing to read-only registers, but removing that write would not fix
any bugs.

The important thing is that we shouldn't blindly enable ASPM L0s, so
that's what the subject should mention.

On Tue, Apr 21, 2020 at 01:16:54PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Trying to change Link Status register does not have any effect as this
> is a read-only register. Trying to overwrite bits for Negotiated Link
> Width does not make sense.
> 
> 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 isn't 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 probes. So setting these bits by
> aardvark driver has no long running effect.
> 
> Remove code which touches ASPM L0s bits from this driver and let
> kernel's ASPM implementation to set ASPM state properly.
> 
> Some users are reporting issues that this code is problematic for some
> Intel wifi cards and removing it fixes them, see e.g.:
> https://bugzilla.kernel.org/show_bug.cgi?id=196339
> 
> If problems with Intel wifi cards occur even after this commit, then
> 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 b59198a102d0..551d98174613 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -356,10 +356,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.24.1
> 

Re: [PATCH v2 2/9] PCI: aardvark: don't write to read-only register

[Pali Rohár]

Hello Bjorn!

On Thursday 23 April 2020 12:27:13 Bjorn Helgaas wrote:
> In the next round, please capitalize the first word of the subjects of
> the whole series to match:

Thank you for the review, I will fix subjects of all patches it in V3.

>   $ git log --oneline drivers/pci/controller/pci-aardvark.c
>   4e5be6f81be7 ("PCI: aardvark: Use pci_parse_request_of_pci_ranges()")
>   e078723f9ccc ("PCI: aardvark: Fix big endian support")
>   7fbcb5da811b ("PCI: aardvark: Don't rely on jiffies while holding spinlock")
>   c0f05a6ab525 ("PCI: aardvark: Fix PCI_EXP_RTCTL register configuration")
>   f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready before training link")
>   364b3f1ff8f0 ("PCI: aardvark: Use LTSSM state to build link training flag")
> 
> The important thing for the subject of this patch is not the "don't
> write to read-only register" part; it's true that there's no point in
> writing to read-only registers, but removing that write would not fix
> any bugs.
> 
> The important thing is that we shouldn't blindly enable ASPM L0s, so
> that's what the subject should mention.

Ok understood, I will fix it in V3.

Re: [PATCH v2 3/9] PCI: aardvark: improve link training

[Bjorn Helgaas]

[+cc Rob]

On Tue, Apr 21, 2020 at 01:16:55PM +0200, Marek Behún wrote:
> Currently the aardvark driver trains link in PCIe gen2 mode. This may
> cause some buggy gen 1 cards (such as Compex WLE900VX) to be unstable or
> even not detected. Moreover when ASPM code tries to retrain link second
> time, these cards may stop responding and link goes down. If gen1 is
> used this does not happen.

Does this patch make the retrain done by ASPM reliable?  If aardvark
can't work reliably at gen2, you may need to just restrict it to gen1.

> Unconditionally forcing gen1 is not a good solution since it may have
> performance impact on gen2 cards.
> 
> To overcome this, read 'max-link-speed' property (as defined in PCI
> device tree bindings) and use this as max gen mode. Then iteratively try
> link training at this mode or lower until successful. After successful
> link training choose final controlled gen based on Negotiated Link Speed
> from Link Status register, which should match card speed.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/pci/controller/pci-aardvark.c | 111 ++++++++++++++++++++------
>  1 file changed, 86 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 551d98174613..606bae1e7a88 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)
> @@ -201,6 +202,7 @@ struct advk_pcie {
>  	struct mutex msi_used_lock;
>  	u16 msi_msg;
>  	int root_bus_nr;
> +	int link_gen;
>  	struct pci_bridge_emul bridge;
>  };
>  
> @@ -225,20 +227,16 @@ static int advk_pcie_link_up(struct advk_pcie *pcie)
>  
>  static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
>  {
> -	struct device *dev = &pcie->pdev->dev;
>  	int retries;
>  
>  	/* check if the link is up or not */
>  	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> -		if (advk_pcie_link_up(pcie)) {
> -			dev_info(dev, "link up\n");
> +		if (advk_pcie_link_up(pcie))
>  			return 0;
> -		}
>  
>  		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
>  	}
>  
> -	dev_err(dev, "link never came up\n");
>  	return -ETIMEDOUT;
>  }
>  
> @@ -253,6 +251,84 @@ static void advk_pcie_wait_for_retrain(struct advk_pcie *pcie)
>  	}
>  }
>  
> +static int advk_pcie_train_at_gen(struct advk_pcie *pcie, int gen)
> +{
> +	int ret, neg_gen;
> +	u32 reg;
> +
> +	/* Setup link speed */
> +	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
> +	reg &= ~PCIE_GEN_SEL_MSK;
> +	if (gen == 2)
> +		reg |= SPEED_GEN_2;
> +	else
> +		reg |= SPEED_GEN_1;
> +	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> +
> +	/*
> +	 * Enable link training. This is not needed in every call to this
> +	 * function, just once suffices, but it does not break anything either.
> +	 */
> +	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
> +	reg |= LINK_TRAINING_EN;
> +	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> +
> +	/*
> +	 * Start link training immediately after enabling it. This solves
> +	 * problems for some buggy cards.
> +	 */
> +	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);
> +
> +	ret = advk_pcie_wait_for_link(pcie);
> +	if (ret)
> +		return ret;
> +
> +	reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
> +	neg_gen = (reg >> PCIE_CORE_LINK_SPEED_SHIFT) & 0xf;
> +
> +	return neg_gen;
> +}
> +
> +static void advk_pcie_train_link(struct advk_pcie *pcie)
> +{
> +	struct device *dev = &pcie->pdev->dev;
> +	int neg_gen = -1, gen;
> +
> +	/*
> +	 * Try link training at link gen specified by device tree property
> +	 * 'max-link-speed' (defaults to 2, since this controller does not
> +	 * support higher gen). If this fails, iteratively train at lower gen.
> +	 */
> +	for (gen = pcie->link_gen; gen > 0; --gen) {
> +		neg_gen = advk_pcie_train_at_gen(pcie, gen);
> +		if (neg_gen > 0)
> +			break;
> +	}
> +
> +	if (neg_gen < 0)
> +		goto err;
> +
> +	/*
> +	 * After successful training if negotiated gen is lower than requested,
> +	 * train again on negotiated gen. This solves some stability issues for
> +	 * some buggy gen1 cards.
> +	 */
> +	if (neg_gen < gen) {
> +		gen = neg_gen;
> +		neg_gen = advk_pcie_train_at_gen(pcie, gen);
> +	}
> +
> +	if (neg_gen == gen) {
> +		dev_info(dev, "link up at gen %i\n", gen);
> +		return;
> +	}
> +
> +err:
> +	dev_err(dev, "link never came up\n");
> +}
> +
>  static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  {
>  	u32 reg;
> @@ -288,12 +364,6 @@ 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 lane X1 */
>  	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
>  	reg &= ~LANE_CNT_MSK;
> @@ -341,20 +411,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  	 */
>  	msleep(PCI_PM_D3COLD_WAIT);
>  
> -	/* 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 immediately after enabling it. This solves
> -	 * problems for some buggy cards.
> -	 */
> -	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);
> -
> -	advk_pcie_wait_for_link(pcie);
> +	advk_pcie_train_link(pcie);
>  
>  	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
>  	reg |= PCIE_CORE_CMD_MEM_ACCESS_EN |
> @@ -988,6 +1045,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  	}
>  	pcie->root_bus_nr = bus->start;
>  
> +	pcie->link_gen = of_pci_get_max_link_speed(dev->of_node);
> +	if (pcie->link_gen < 1 || pcie->link_gen > 2)

This is a DT error, isn't it?  Maybe should warn about it and mention
how you're dealing with it?

> +		pcie->link_gen = 2;
> +
>  	advk_pcie_setup_hw(pcie);
>  
>  	advk_sw_pci_bridge_init(pcie);
> -- 
> 2.24.1
> 

Re: [PATCH v2 4/9] PCI: aardvark: issue PERST via GPIO

[Bjorn Helgaas]

[+cc Rob]

On Tue, Apr 21, 2020 at 01:16:56PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Add support for issuing PERST via GPIO specified in 'reset-gpios'
> property (as described in PCI device tree bindings).
> 
> Some buggy cards (e.g. Compex WLE900VX or WLE1216) are not detected
> after reboot when PERST is not issued during driver initialization.

Does this slot support hotplug?  If so, I don't think this fix will
help the hot-add case, will it?

> Tested on Turris MOX.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/pci/controller/pci-aardvark.c | 32 +++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 606bae1e7a88..e2d18094d8ca 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"
> @@ -204,6 +206,7 @@ struct advk_pcie {
>  	int root_bus_nr;
>  	int link_gen;
>  	struct pci_bridge_emul bridge;
> +	struct gpio_desc *reset_gpio;
>  };
>  
>  static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
> @@ -329,10 +332,23 @@ static void advk_pcie_train_link(struct advk_pcie *pcie)
>  	dev_err(dev, "link never came up\n");
>  }
>  
> +static void advk_pcie_issue_perst(struct advk_pcie *pcie)
> +{
> +	if (!pcie->reset_gpio)
> +		return;
> +
> +	dev_info(&pcie->pdev->dev, "issuing PERST via reset GPIO for 1ms\n");
> +	gpiod_set_value_cansleep(pcie->reset_gpio, 1);
> +	usleep_range(1000, 2000);
> +	gpiod_set_value_cansleep(pcie->reset_gpio, 0);
> +}
> +
>  static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  {
>  	u32 reg;
>  
> +	advk_pcie_issue_perst(pcie);
> +
>  	/* Set to Direct mode */
>  	reg = advk_readl(pcie, CTRL_CONFIG_REG);
>  	reg &= ~(CTRL_MODE_MASK << CTRL_MODE_SHIFT);
> @@ -1045,6 +1061,22 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  	}
>  	pcie->root_bus_nr = bus->start;
>  
> +	pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
> +						       "reset-gpios", 0,
> +						       GPIOD_OUT_LOW,
> +						       "pcie1-reset");
> +	ret = PTR_ERR_OR_ZERO(pcie->reset_gpio);
> +	if (ret) {
> +		if (ret == -ENOENT) {
> +			pcie->reset_gpio = NULL;
> +		} else {
> +			if (ret != -EPROBE_DEFER)
> +				dev_err(dev, "Failed to get reset-gpio: %i\n",
> +					ret);
> +			return ret;
> +		}
> +	}
> +
>  	pcie->link_gen = of_pci_get_max_link_speed(dev->of_node);
>  	if (pcie->link_gen < 1 || pcie->link_gen > 2)
>  		pcie->link_gen = 2;
> -- 
> 2.24.1
> 

Re: [PATCH v2 5/9] PCI: aardvark: add FIXME comment for PCIE_CORE_CMD_STATUS_REG access

[Bjorn Helgaas]

[+cc Rob]

On Tue, Apr 21, 2020 at 01:16:57PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Register PCIE_CORE_CMD_STATUS_REG is applicable only when the controller
> is configured for Endpoint mode, which is not the case for the current
> version of this driver.
> 
> Add a FIXME comment, since this needs to be explained, removed or fixed.

If it's not applicable, why not just remove it?

> 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 e2d18094d8ca..e893d7d8859f 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -429,6 +429,12 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  
>  	advk_pcie_train_link(pcie);
>  
> +	/*
> +	 * 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.24.1
> 

Re: [PATCH v2 3/9] PCI: aardvark: improve link training

[Pali Rohár]

On Thursday 23 April 2020 13:39:14 Bjorn Helgaas wrote:
> [+cc Rob]
> 
> On Tue, Apr 21, 2020 at 01:16:55PM +0200, Marek Behún wrote:
> > Currently the aardvark driver trains link in PCIe gen2 mode. This may
> > cause some buggy gen 1 cards (such as Compex WLE900VX) to be unstable or
> > even not detected. Moreover when ASPM code tries to retrain link second
> > time, these cards may stop responding and link goes down. If gen1 is
> > used this does not happen.
> 
> Does this patch make the retrain done by ASPM reliable?

Yes, after this patch all my tested cards work fine. I tried to enable
ASPM for tested cards and there were no problem with link training
issued by ASPM kernel code.

So this patch makes link retrain done by ASPM kernel code reliable.

> If aardvark can't work reliably at gen2, you may need to just restrict it to gen1.

aardvark gen2 does not work reliable with WLE900VX gen1 card. It works
reliable with WLE200NX gen1 card and works also reliable with gen2
WLE1216V5-20 card.

Marek also tested aardvark gen2 with sata gen2 cards and it worked
reliable too.

So result is that aardvark gen2 works reliable with gen2 cards and does
not work reliable with gen1 cards.

Therefore we decided to set aardvark gen to match card gen and then
aardvark with tested cards work reliable.

Also more people on espressobin forums were manually patching kernel for
particular gen1 cards to set aardvark to gen1 module because they had
same symptoms as we are observing.

> > Unconditionally forcing gen1 is not a good solution since it may have
> > performance impact on gen2 cards.
> > 
> > To overcome this, read 'max-link-speed' property (as defined in PCI
> > device tree bindings) and use this as max gen mode. Then iteratively try
> > link training at this mode or lower until successful. After successful
> > link training choose final controlled gen based on Negotiated Link Speed
> > from Link Status register, which should match card speed.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 111 ++++++++++++++++++++------
> >  1 file changed, 86 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index 551d98174613..606bae1e7a88 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)
> > @@ -201,6 +202,7 @@ struct advk_pcie {
> >  	struct mutex msi_used_lock;
> >  	u16 msi_msg;
> >  	int root_bus_nr;
> > +	int link_gen;
> >  	struct pci_bridge_emul bridge;
> >  };
> >  
> > @@ -225,20 +227,16 @@ static int advk_pcie_link_up(struct advk_pcie *pcie)
> >  
> >  static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
> >  {
> > -	struct device *dev = &pcie->pdev->dev;
> >  	int retries;
> >  
> >  	/* check if the link is up or not */
> >  	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> > -		if (advk_pcie_link_up(pcie)) {
> > -			dev_info(dev, "link up\n");
> > +		if (advk_pcie_link_up(pcie))
> >  			return 0;
> > -		}
> >  
> >  		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
> >  	}
> >  
> > -	dev_err(dev, "link never came up\n");
> >  	return -ETIMEDOUT;
> >  }
> >  
> > @@ -253,6 +251,84 @@ static void advk_pcie_wait_for_retrain(struct advk_pcie *pcie)
> >  	}
> >  }
> >  
> > +static int advk_pcie_train_at_gen(struct advk_pcie *pcie, int gen)
> > +{
> > +	int ret, neg_gen;
> > +	u32 reg;
> > +
> > +	/* Setup link speed */
> > +	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
> > +	reg &= ~PCIE_GEN_SEL_MSK;
> > +	if (gen == 2)
> > +		reg |= SPEED_GEN_2;
> > +	else
> > +		reg |= SPEED_GEN_1;
> > +	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> > +
> > +	/*
> > +	 * Enable link training. This is not needed in every call to this
> > +	 * function, just once suffices, but it does not break anything either.
> > +	 */
> > +	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
> > +	reg |= LINK_TRAINING_EN;
> > +	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
> > +
> > +	/*
> > +	 * Start link training immediately after enabling it. This solves
> > +	 * problems for some buggy cards.
> > +	 */
> > +	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);
> > +
> > +	ret = advk_pcie_wait_for_link(pcie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	reg = advk_readl(pcie, PCIE_CORE_LINK_CTRL_STAT_REG);
> > +	neg_gen = (reg >> PCIE_CORE_LINK_SPEED_SHIFT) & 0xf;
> > +
> > +	return neg_gen;
> > +}
> > +
> > +static void advk_pcie_train_link(struct advk_pcie *pcie)
> > +{
> > +	struct device *dev = &pcie->pdev->dev;
> > +	int neg_gen = -1, gen;
> > +
> > +	/*
> > +	 * Try link training at link gen specified by device tree property
> > +	 * 'max-link-speed' (defaults to 2, since this controller does not
> > +	 * support higher gen). If this fails, iteratively train at lower gen.
> > +	 */
> > +	for (gen = pcie->link_gen; gen > 0; --gen) {
> > +		neg_gen = advk_pcie_train_at_gen(pcie, gen);
> > +		if (neg_gen > 0)
> > +			break;
> > +	}
> > +
> > +	if (neg_gen < 0)
> > +		goto err;
> > +
> > +	/*
> > +	 * After successful training if negotiated gen is lower than requested,
> > +	 * train again on negotiated gen. This solves some stability issues for
> > +	 * some buggy gen1 cards.
> > +	 */
> > +	if (neg_gen < gen) {
> > +		gen = neg_gen;
> > +		neg_gen = advk_pcie_train_at_gen(pcie, gen);
> > +	}
> > +
> > +	if (neg_gen == gen) {
> > +		dev_info(dev, "link up at gen %i\n", gen);
> > +		return;
> > +	}
> > +
> > +err:
> > +	dev_err(dev, "link never came up\n");
> > +}
> > +
> >  static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> >  {
> >  	u32 reg;
> > @@ -288,12 +364,6 @@ 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 lane X1 */
> >  	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
> >  	reg &= ~LANE_CNT_MSK;
> > @@ -341,20 +411,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> >  	 */
> >  	msleep(PCI_PM_D3COLD_WAIT);
> >  
> > -	/* 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 immediately after enabling it. This solves
> > -	 * problems for some buggy cards.
> > -	 */
> > -	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);
> > -
> > -	advk_pcie_wait_for_link(pcie);
> > +	advk_pcie_train_link(pcie);
> >  
> >  	reg = advk_readl(pcie, PCIE_CORE_CMD_STATUS_REG);
> >  	reg |= PCIE_CORE_CMD_MEM_ACCESS_EN |
> > @@ -988,6 +1045,10 @@ static int advk_pcie_probe(struct platform_device *pdev)
> >  	}
> >  	pcie->root_bus_nr = bus->start;
> >  
> > +	pcie->link_gen = of_pci_get_max_link_speed(dev->of_node);
> > +	if (pcie->link_gen < 1 || pcie->link_gen > 2)
> 
> This is a DT error, isn't it?  Maybe should warn about it and mention
> how you're dealing with it?

Well, negative and zero link_gen is DT error.

But documentation in pci.txt says:

- max-link-speed:
   If present this property specifies PCI gen for link capability.  Host
   drivers could add this as a strategy to avoid unnecessary operation for
   unsupported link speed, for instance, trying to do training for
   unsupported link speed, etc.  Must be '4' for gen4, '3' for gen3, '2'
   for gen2, and '1' for gen1. Any other values are invalid.

I understand this as maximal property which driver could decrease if
needed. I think that '4' and '3' are also valid values as they describe
upper limit and we could map them to 2.

> > +		pcie->link_gen = 2;
> > +
> >  	advk_pcie_setup_hw(pcie);
> >  
> >  	advk_sw_pci_bridge_init(pcie);
> > -- 
> > 2.24.1
> > 

Re: [PATCH v2 4/9] PCI: aardvark: issue PERST via GPIO

[Pali Rohár]

On Thursday 23 April 2020 13:41:51 Bjorn Helgaas wrote:
> [+cc Rob]
> 
> On Tue, Apr 21, 2020 at 01:16:56PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > Add support for issuing PERST via GPIO specified in 'reset-gpios'
> > property (as described in PCI device tree bindings).
> > 
> > Some buggy cards (e.g. Compex WLE900VX or WLE1216) are not detected
> > after reboot when PERST is not issued during driver initialization.
> 
> Does this slot support hotplug?

I have no idea. I have not heard that anybody tried hotplugging cards
with this aardvark pcie controller at runtime.

This patch fixes initialization only at boot time when cards were
plugged prior powering board on.

> If so, I don't think this fix will help the hot-add case, will it?

I even do not know if aardvark HW supports it. And if yes, I think it is
unimplemented and/or broken.

In documentation there is some interrupt register which could signal it,
but I it is not used by kernel's pci-aardvark.c driver.

> > Tested on Turris MOX.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 32 +++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index 606bae1e7a88..e2d18094d8ca 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"
> > @@ -204,6 +206,7 @@ struct advk_pcie {
> >  	int root_bus_nr;
> >  	int link_gen;
> >  	struct pci_bridge_emul bridge;
> > +	struct gpio_desc *reset_gpio;
> >  };
> >  
> >  static inline void advk_writel(struct advk_pcie *pcie, u32 val, u64 reg)
> > @@ -329,10 +332,23 @@ static void advk_pcie_train_link(struct advk_pcie *pcie)
> >  	dev_err(dev, "link never came up\n");
> >  }
> >  
> > +static void advk_pcie_issue_perst(struct advk_pcie *pcie)
> > +{
> > +	if (!pcie->reset_gpio)
> > +		return;
> > +
> > +	dev_info(&pcie->pdev->dev, "issuing PERST via reset GPIO for 1ms\n");
> > +	gpiod_set_value_cansleep(pcie->reset_gpio, 1);
> > +	usleep_range(1000, 2000);
> > +	gpiod_set_value_cansleep(pcie->reset_gpio, 0);
> > +}
> > +
> >  static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> >  {
> >  	u32 reg;
> >  
> > +	advk_pcie_issue_perst(pcie);
> > +
> >  	/* Set to Direct mode */
> >  	reg = advk_readl(pcie, CTRL_CONFIG_REG);
> >  	reg &= ~(CTRL_MODE_MASK << CTRL_MODE_SHIFT);
> > @@ -1045,6 +1061,22 @@ static int advk_pcie_probe(struct platform_device *pdev)
> >  	}
> >  	pcie->root_bus_nr = bus->start;
> >  
> > +	pcie->reset_gpio = devm_gpiod_get_from_of_node(dev, dev->of_node,
> > +						       "reset-gpios", 0,
> > +						       GPIOD_OUT_LOW,
> > +						       "pcie1-reset");
> > +	ret = PTR_ERR_OR_ZERO(pcie->reset_gpio);
> > +	if (ret) {
> > +		if (ret == -ENOENT) {
> > +			pcie->reset_gpio = NULL;
> > +		} else {
> > +			if (ret != -EPROBE_DEFER)
> > +				dev_err(dev, "Failed to get reset-gpio: %i\n",
> > +					ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	pcie->link_gen = of_pci_get_max_link_speed(dev->of_node);
> >  	if (pcie->link_gen < 1 || pcie->link_gen > 2)
> >  		pcie->link_gen = 2;
> > -- 
> > 2.24.1
> > 

Re: [PATCH v2 5/9] PCI: aardvark: add FIXME comment for PCIE_CORE_CMD_STATUS_REG access

[Pali Rohár]

On Thursday 23 April 2020 13:44:13 Bjorn Helgaas wrote:
> [+cc Rob]
> 
> On Tue, Apr 21, 2020 at 01:16:57PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > Register PCIE_CORE_CMD_STATUS_REG is applicable only when the controller
> > is configured for Endpoint mode, which is not the case for the current
> > version of this driver.
> > 
> > Add a FIXME comment, since this needs to be explained, removed or fixed.
> 
> If it's not applicable, why not just remove it?

Problem is hat if I remove this part of code, ath10k cards stops
working. So for some unknown reasons setting this "not applicable"
register is required.

I have in-progress V3 version of this patch series with updated comment
and commit message of this patch (include this observation).

> > 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 e2d18094d8ca..e893d7d8859f 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -429,6 +429,12 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> >  
> >  	advk_pcie_train_link(pcie);
> >  
> > +	/*
> > +	 * 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.24.1
> > 

Re: [PATCH v2 4/9] PCI: aardvark: issue PERST via GPIO

[Bjorn Helgaas]

On Thu, Apr 23, 2020 at 09:02:02PM +0200, Pali Rohár wrote:
> On Thursday 23 April 2020 13:41:51 Bjorn Helgaas wrote:
> > [+cc Rob]
> > 
> > On Tue, Apr 21, 2020 at 01:16:56PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > > 
> > > Add support for issuing PERST via GPIO specified in 'reset-gpios'
> > > property (as described in PCI device tree bindings).
> > > 
> > > Some buggy cards (e.g. Compex WLE900VX or WLE1216) are not detected
> > > after reboot when PERST is not issued during driver initialization.
> > 
> > Does this slot support hotplug?
> 
> I have no idea. I have not heard that anybody tried hotplugging cards
> with this aardvark pcie controller at runtime.
> 
> This patch fixes initialization only at boot time when cards were
> plugged prior powering board on.
> 
> > If so, I don't think this fix will help the hot-add case, will it?
> 
> I even do not know if aardvark HW supports it. And if yes, I think it is
> unimplemented and/or broken.
> 
> In documentation there is some interrupt register which could signal it,
> but I it is not used by kernel's pci-aardvark.c driver.

"lspci -vv" will show you whether the hardware claims to support it,
e.g.,

  00:1c.0 PCI bridge: Intel Corporation Sunrise Point-LP PCI Express Root Port #1
    Capabilities: [40] Express (v2) Root Port (Slot+), MSI 00
      SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug- Surprise-

If the right combination of bits are set there, pciehp will claim the
port and support hotplug.

Bjorn

Re: [PATCH v2 4/9] PCI: aardvark: issue PERST via GPIO

[Pali Rohár]

On Thursday 23 April 2020 17:17:14 Bjorn Helgaas wrote:
> On Thu, Apr 23, 2020 at 09:02:02PM +0200, Pali Rohár wrote:
> > On Thursday 23 April 2020 13:41:51 Bjorn Helgaas wrote:
> > > [+cc Rob]
> > > 
> > > On Tue, Apr 21, 2020 at 01:16:56PM +0200, Marek Behún wrote:
> > > > From: Pali Rohár <pali@kernel.org>
> > > > 
> > > > Add support for issuing PERST via GPIO specified in 'reset-gpios'
> > > > property (as described in PCI device tree bindings).
> > > > 
> > > > Some buggy cards (e.g. Compex WLE900VX or WLE1216) are not detected
> > > > after reboot when PERST is not issued during driver initialization.
> > > 
> > > Does this slot support hotplug?
> > 
> > I have no idea. I have not heard that anybody tried hotplugging cards
> > with this aardvark pcie controller at runtime.
> > 
> > This patch fixes initialization only at boot time when cards were
> > plugged prior powering board on.
> > 
> > > If so, I don't think this fix will help the hot-add case, will it?
> > 
> > I even do not know if aardvark HW supports it. And if yes, I think it is
> > unimplemented and/or broken.
> > 
> > In documentation there is some interrupt register which could signal it,
> > but I it is not used by kernel's pci-aardvark.c driver.
> 
> "lspci -vv" will show you whether the hardware claims to support it,
> e.g.,
> 
>   00:1c.0 PCI bridge: Intel Corporation Sunrise Point-LP PCI Express Root Port #1
>     Capabilities: [40] Express (v2) Root Port (Slot+), MSI 00
>       SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug- Surprise-
> 
> If the right combination of bits are set there, pciehp will claim the
> port and support hotplug.

aardvark controller does not have pci bridge on bus. Kernel aardvark
driver uses pci_bridge_emul_init() for registering emulated pci bridge.

Is hotplug flag from that emulated pci bridge relevant here?

Re: [PATCH v2 4/9] PCI: aardvark: issue PERST via GPIO

[Bjorn Helgaas]

On Fri, Apr 24, 2020 at 12:23:12AM +0200, Pali Rohár wrote:
> On Thursday 23 April 2020 17:17:14 Bjorn Helgaas wrote:
> > On Thu, Apr 23, 2020 at 09:02:02PM +0200, Pali Rohár wrote:
> > > On Thursday 23 April 2020 13:41:51 Bjorn Helgaas wrote:
> > > > [+cc Rob]
> > > > 
> > > > On Tue, Apr 21, 2020 at 01:16:56PM +0200, Marek Behún wrote:
> > > > > From: Pali Rohár <pali@kernel.org>
> > > > > 
> > > > > Add support for issuing PERST via GPIO specified in 'reset-gpios'
> > > > > property (as described in PCI device tree bindings).
> > > > > 
> > > > > Some buggy cards (e.g. Compex WLE900VX or WLE1216) are not detected
> > > > > after reboot when PERST is not issued during driver initialization.
> > > > 
> > > > Does this slot support hotplug?
> > > 
> > > I have no idea. I have not heard that anybody tried hotplugging cards
> > > with this aardvark pcie controller at runtime.
> > > 
> > > This patch fixes initialization only at boot time when cards were
> > > plugged prior powering board on.
> > > 
> > > > If so, I don't think this fix will help the hot-add case, will it?
> > > 
> > > I even do not know if aardvark HW supports it. And if yes, I think it is
> > > unimplemented and/or broken.
> > > 
> > > In documentation there is some interrupt register which could signal it,
> > > but I it is not used by kernel's pci-aardvark.c driver.
> > 
> > "lspci -vv" will show you whether the hardware claims to support it,
> > e.g.,
> > 
> >   00:1c.0 PCI bridge: Intel Corporation Sunrise Point-LP PCI Express Root Port #1
> >     Capabilities: [40] Express (v2) Root Port (Slot+), MSI 00
> >       SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug- Surprise-
> > 
> > If the right combination of bits are set there, pciehp will claim the
> > port and support hotplug.
> 
> aardvark controller does not have pci bridge on bus. Kernel aardvark
> driver uses pci_bridge_emul_init() for registering emulated pci bridge.
> 
> Is hotplug flag from that emulated pci bridge relevant here?

I doubt it.

Re: [PATCH v2 4/9] PCI: aardvark: issue PERST via GPIO

[Pali Rohár]

On Thursday 23 April 2020 17:40:25 Bjorn Helgaas wrote:
> On Fri, Apr 24, 2020 at 12:23:12AM +0200, Pali Rohár wrote:
> > On Thursday 23 April 2020 17:17:14 Bjorn Helgaas wrote:
> > > On Thu, Apr 23, 2020 at 09:02:02PM +0200, Pali Rohár wrote:
> > > > On Thursday 23 April 2020 13:41:51 Bjorn Helgaas wrote:
> > > > > [+cc Rob]
> > > > > 
> > > > > On Tue, Apr 21, 2020 at 01:16:56PM +0200, Marek Behún wrote:
> > > > > > From: Pali Rohár <pali@kernel.org>
> > > > > > 
> > > > > > Add support for issuing PERST via GPIO specified in 'reset-gpios'
> > > > > > property (as described in PCI device tree bindings).
> > > > > > 
> > > > > > Some buggy cards (e.g. Compex WLE900VX or WLE1216) are not detected
> > > > > > after reboot when PERST is not issued during driver initialization.
> > > > > 
> > > > > Does this slot support hotplug?
> > > > 
> > > > I have no idea. I have not heard that anybody tried hotplugging cards
> > > > with this aardvark pcie controller at runtime.
> > > > 
> > > > This patch fixes initialization only at boot time when cards were
> > > > plugged prior powering board on.
> > > > 
> > > > > If so, I don't think this fix will help the hot-add case, will it?
> > > > 
> > > > I even do not know if aardvark HW supports it. And if yes, I think it is
> > > > unimplemented and/or broken.
> > > > 
> > > > In documentation there is some interrupt register which could signal it,
> > > > but I it is not used by kernel's pci-aardvark.c driver.
> > > 
> > > "lspci -vv" will show you whether the hardware claims to support it,
> > > e.g.,
> > > 
> > >   00:1c.0 PCI bridge: Intel Corporation Sunrise Point-LP PCI Express Root Port #1
> > >     Capabilities: [40] Express (v2) Root Port (Slot+), MSI 00
> > >       SltCap: AttnBtn- PwrCtrl- MRL- AttnInd- PwrInd- HotPlug- Surprise-
> > > 
> > > If the right combination of bits are set there, pciehp will claim the
> > > port and support hotplug.
> > 
> > aardvark controller does not have pci bridge on bus. Kernel aardvark
> > driver uses pci_bridge_emul_init() for registering emulated pci bridge.
> > 
> > Is hotplug flag from that emulated pci bridge relevant here?
> 
> I doubt it.

In case you are interested, here is output for that emulated pci bridge:

# lspci -vv -s 00:00.0
00:00.0 PCI bridge: Marvell Technology Group Ltd. Device 0100 (prog-if 00 [Normal decode])
        Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
        Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
        Latency: 0, Cache Line Size: 64 bytes
        Interrupt: pin A routed to IRQ 44
        Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
        I/O behind bridge: None
        Memory behind bridge: e8000000-e82fffff [size=3M]
        Prefetchable memory behind bridge: None
        Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
        [virtual] Expansion ROM at e8300000 [disabled] [size=2K]
        BridgeCtl: Parity- SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
                PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
        Capabilities: [40] Express (v1) Root Port (Slot-), MSI 00
                DevCap: MaxPayload 512 bytes, PhantFunc 0
                        ExtTag- RBE+
                DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
                        RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
                        MaxPayload 256 bytes, MaxReadReq 512 bytes
                DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend-
                LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Exit Latency L0s <128ns, L1 <2us
                        ClockPM- Surprise- LLActRep- BwNot- ASPMOptComp+
                LnkCtl: ASPM Disabled; RCB 64 bytes Disabled- CommClk-
                        ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
                LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk- DLActive- BWMgmt- ABWMgmt-
                RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible-
                RootCap: CRSVisible-
                RootSta: PME ReqID 0000, PMEStatus- PMEPending-
        Kernel driver in use: pcieport

There is no "SltCap" line in lspci output.

Re: [PATCH v2 4/9] PCI: aardvark: issue PERST via GPIO

[Pali Rohár]

On Tuesday 21 April 2020 13:16:56 Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Add support for issuing PERST via GPIO specified in 'reset-gpios'
> property (as described in PCI device tree bindings).
> 
> Some buggy cards (e.g. Compex WLE900VX or WLE1216) are not detected
> after reboot when PERST is not issued during driver initialization.
> 
> Tested on Turris MOX.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/pci/controller/pci-aardvark.c | 32 +++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 606bae1e7a88..e2d18094d8ca 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
...
> +static void advk_pcie_issue_perst(struct advk_pcie *pcie)
> +{
> +	if (!pcie->reset_gpio)
> +		return;
> +
> +	dev_info(&pcie->pdev->dev, "issuing PERST via reset GPIO for 1ms\n");
> +	gpiod_set_value_cansleep(pcie->reset_gpio, 1);
> +	usleep_range(1000, 2000);
> +	gpiod_set_value_cansleep(pcie->reset_gpio, 0);
> +}

After more testing we will have to increase this timeout to 10ms as some
Compex cards are sometimes not detected with current 1ms timeout. I will
do it in V3 patch series.


Bjorn, do you know if there is a defined timeout in PCIE specification
how long should be card in PERST?

I looked into others pci kernel drivers and basically every driver is
using its own timeout.

pcie-kirin.c --> usleep_range(20000, 25000);
pcie-qcom.c --> usleep_range(1000, 1000 + 500); msleep(100);
pci-mvebu.c --> udelay(100);
pci-tegra.c --> usleep_range(1000, 2000);
pcie-iproc.c --> udelay(250);
pcie-mediatek.c --> no delay
pci-imx6.c --> msleep(100);

But I guess that this timeout should not depend on driver or pci
controller, but rather on connected card or on some recommended value if
defined by PCIE specification.

Re: [PATCH v2 3/9] PCI: aardvark: improve link training

[Pali Rohár]

On Thursday 23 April 2020 20:56:27 Pali Rohár wrote:
> On Thursday 23 April 2020 13:39:14 Bjorn Helgaas wrote:
> > [+cc Rob]
> > 
> > On Tue, Apr 21, 2020 at 01:16:55PM +0200, Marek Behún wrote:
> > > Currently the aardvark driver trains link in PCIe gen2 mode. This may
> > > cause some buggy gen 1 cards (such as Compex WLE900VX) to be unstable or
> > > even not detected. Moreover when ASPM code tries to retrain link second
> > > time, these cards may stop responding and link goes down. If gen1 is
> > > used this does not happen.
> > 
> > Does this patch make the retrain done by ASPM reliable?
> 
> Yes, after this patch all my tested cards work fine. I tried to enable
> ASPM for tested cards and there were no problem with link training
> issued by ASPM kernel code.
> 
> So this patch makes link retrain done by ASPM kernel code reliable.

It works fine also for WLE200NX card. ASPM kernel code decides that for
this card ASPM needs to be disabled. In dmesg output I see:

[    3.229229] pci 0000:01:00.0: disabling ASPM on pre-1.1 PCIe device. You can enable it with 'pcie_aspm=force'

Kernel disables ASPM, retrains link and card is working fine.

Re: [PATCH v2 7/9] dt-bindings: PCI: aardvark: describe new properties

[Rob Herring]

On Tue, 21 Apr 2020 13:16:59 +0200, Marek Behún wrote:
> Document the possibility to reference a PHY and reset-gpios and to set
> max-link-speed property.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/pci/aardvark-pci.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>