PCI: aardvark controller fixes

From: Pali Rohár <[email protected]>

There are lot of undocumented interrupt bits. Fix all *_ALL_MASK macros to define all interrupt bits, so that driver can properly mask all interrupts, including those which are undocumented.

Fixes 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")

Signed-off-by Pali Rohár <[email protected]>

Reviewed-by Marek Behún <[email protected]>

Signed-off-by Marek Behún <[email protected]>

Cc [email protected]

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

drivers/pci/controller/pci-aardvark.c +3 -3

index f7eebf453e83..3448a8c446d4 100644
@@ -107,13 +107,13 @@
 #define     PCIE_ISR0_MSI_INT_PENDING		BIT(24)
 #define     PCIE_ISR0_INTX_ASSERT(val)		BIT(16 + (val))
 #define     PCIE_ISR0_INTX_DEASSERT(val)	BIT(20 + (val))
-#define	    PCIE_ISR0_ALL_MASK			GENMASK(26, 0)
+#define     PCIE_ISR0_ALL_MASK			GENMASK(31, 0)
 #define PCIE_ISR1_REG				(CONTROL_BASE_ADDR + 0x48)
 #define PCIE_ISR1_MASK_REG			(CONTROL_BASE_ADDR + 0x4C)
 #define     PCIE_ISR1_POWER_STATE_CHANGE	BIT(4)
 #define     PCIE_ISR1_FLUSH			BIT(5)
 #define     PCIE_ISR1_INTX_ASSERT(val)		BIT(8 + (val))
-#define     PCIE_ISR1_ALL_MASK			GENMASK(11, 4)
+#define     PCIE_ISR1_ALL_MASK			GENMASK(31, 0)
 #define PCIE_MSI_ADDR_LOW_REG			(CONTROL_BASE_ADDR + 0x50)
 #define PCIE_MSI_ADDR_HIGH_REG			(CONTROL_BASE_ADDR + 0x54)
 #define PCIE_MSI_STATUS_REG			(CONTROL_BASE_ADDR + 0x58)

@@ -199,7 +199,7 @@
 #define     PCIE_IRQ_MSI_INT2_DET		BIT(21)
 #define     PCIE_IRQ_RC_DBELL_DET		BIT(22)
 #define     PCIE_IRQ_EP_STATUS			BIT(23)
-#define     PCIE_IRQ_ALL_MASK			0xfff0fb
+#define     PCIE_IRQ_ALL_MASK			GENMASK(31, 0)
 #define     PCIE_IRQ_ENABLE_INTS_MASK		PCIE_IRQ_CORE_INT
 
 /* Transaction types */

Signature · 1 line

2.32.0

[CC Sasha for stable kernel rules]

Quoted 11 lines · Marek Behún

From: Pali Rohár <[email protected]>

There are lot of undocumented interrupt bits. Fix all *_ALL_MASK macros to define all interrupt bits, so that driver can properly mask all interrupts, including those which are undocumented.

Fixes 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")

Signed-off-by Pali Rohár <[email protected]>

Reviewed-by Marek Behún <[email protected]>

Signed-off-by Marek Behún <[email protected]>

Cc [email protected]

I don't think this is a fix and I don't think it should be sent to stable kernels in _preparation_ for other fixes to come (that may never land in mainline).

Quoted 15 lines · On Wed, 6 Oct 2021 10:13:38 +0100 Lorenzo Pieralisi…

[CC Sasha for stable kernel rules]

Quoted 11 lines · Marek Behún

From: Pali Rohár <[email protected]>

There are lot of undocumented interrupt bits. Fix all *_ALL_MASK macros to define all interrupt bits, so that driver can properly mask all interrupts, including those which are undocumented.

Fixes 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")

Signed-off-by Pali Rohár <[email protected]>

Reviewed-by Marek Behún <[email protected]>

Signed-off-by Marek Behún <[email protected]>

Cc [email protected]

Quoted 3 lines

I don't think this is a fix and I don't think it should be sent to stable kernels in _preparation_ for other fixes to come (that may never land in mainline).

This patch is a fix because it fixes masking interrupts, which may prevent spurious interrupts - we don't want interrupts which kernel didn't request for.

I agree that the commit message should probably mention this, though... :(

Marek

From: Pali Rohár <[email protected]>

Fix multiple link training issues in aardvark driver. The main reason of these issues was misunderstanding of what certain registers do, since their names and comments were misleading: before commit 96be36dbffac ("PCI:
aardvark: Replace custom macros by standard linux/pci_regs.h macros"), the pci-aardvark.c driver used custom macros for accessing standard PCIe Root Bridge registers, and misleading comments did not help to understand what the code was really doing.

After doing more tests and experiments I've come to the conclusion that the SPEED_GEN register in aardvark sets the PCIe revision / generation compliance and forces maximal link speed. Both GEN3 and GEN2 values set the read-only PCI_EXP_FLAGS_VERS bits (PCIe capabilities version of Root Bridge) to value 2, while GEN1 value sets PCI_EXP_FLAGS_VERS to 1, which matches with PCI Express specifications revisions 3, 2 and 1 respectively. Changing SPEED_GEN also sets the read-only bits PCI_EXP_LNKCAP_SLS and PCI_EXP_LNKCAP2_SLS to corresponding speed.

(Note that PCI Express rev 1 specification does not define PCI_EXP_LNKCAP2
and PCI_EXP_LNKCTL2 registers and when SPEED_GEN is set to GEN1 (which also sets PCI_EXP_FLAGS_VERS set to 1), lspci cannot access PCI_EXP_LNKCAP2 and PCI_EXP_LNKCTL2 registers.)

Changing PCIe link speed can be done via PCI_EXP_LNKCTL2_TLS bits of PCI_EXP_LNKCTL2 register. Armada 3700 Functional Specifications says that the default value of PCI_EXP_LNKCTL2_TLS is based on SPEED_GEN value, but tests showed that the default value is always 8.0 GT/s, independently of speed set by SPEED_GEN. So after setting SPEED_GEN, we must also set value in PCI_EXP_LNKCTL2 register via PCI_EXP_LNKCTL2_TLS bits.

Triggering PCI_EXP_LNKCTL_RL bit immediately after setting LINK_TRAINING_EN bit actually doesn't do anything. Tests have shown that a delay is needed after enabling LINK_TRAINING_EN bit. As triggering PCI_EXP_LNKCTL_RL currently does nothing, remove it.

Commit 43fc679ced18 ("PCI: aardvark: Improve link training") introduced code which sets SPEED_GEN register based on negotiated link speed from PCI_EXP_LNKSTA_CLS bits of PCI_EXP_LNKSTA register. This code was added to fix detection of Compex WLE900VX (Atheros QCA9880) WiFi GEN1 PCIe cards, as otherwise these cards were "invisible" on PCIe bus (probably because they crashed). But apparently more people reported the same issues with these cards also with other PCIe controllers [1] and I was able to reproduce this issue also with other "noname" WiFi cards based on Atheros QCA9890 chip (with the same PCI vendor/device ids as Atheros QCA9880). So this is not an issue in aardvark but rather an issue in Atheros QCA98xx chips. Also, this issue only exists if the kernel is compiled with PCIe ASPM support, and a generic workaround for this is to change PCIe Bridge to 2.5 GT/s link speed via PCI_EXP_LNKCTL2_TLS_2_5GT bits in PCI_EXP_LNKCTL2 register [2], before triggering PCI_EXP_LNKCTL_RL bit. This workaround also works when SPEED_GEN is set to value GEN2 (5 GT/s). So remove this hack completely in the aardvark driver and always set SPEED_GEN to value from 'max-link-speed' DT property. Fix for Atheros QCA98xx chips is handled separately by patch [2].

These two things (code for triggering PCI_EXP_LNKCTL_RL bit and changing SPEED_GEN value) also explain why commit 6964494582f5 ("PCI: aardvark:
Train link immediately after enabling training") somehow fixed detection of those problematic Compex cards with Atheros chips: if triggering link retraining (via PCI_EXP_LNKCTL_RL bit) was done immediately after enabling link training (via LINK_TRAINING_EN), it did nothing. If there was a specific delay, aardvark HW already initialized PCIe link and therefore triggering link retraining caused the above issue. Compex cards triggered link down event and disappeared from the PCIe bus.

Commit f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready before training link") added 100ms sleep before calling 'Start link training' command and explained that it is a requirement of PCI Express specification. But the code after this 100ms sleep was not doing 'Start link training', rather it triggered PCI_EXP_LNKCTL_RL bit via PCIe Root Bridge to put link into Recovery state.

The required delay after fundamental reset is already done in function advk_pcie_wait_for_link() which also checks whether PCIe link is up. So after removing the code which triggers PCI_EXP_LNKCTL_RL bit on PCIe Root Bridge, there is no need to wait 100ms again. Remove the extra msleep() call and update comment about the delay required by the PCI Express specification.

According to Marvell Armada 3700 Functional Specifications, Link training should be enabled via aardvark register LINK_TRAINING_EN after selecting PCIe generation and x1 lane. There is no need to disable it prior resetting card via PERST# signal. This disabling code was introduced in commit 5169a9851daa ("PCI: aardvark: Issue PERST via GPIO") as a workaround for some Atheros cards. It turns out that this also is Atheros specific issue and affects any PCIe controller, not only aardvark. Moreover this Atheros issue was triggered by juggling with PCI_EXP_LNKCTL_RL, LINK_TRAINING_EN and SPEED_GEN bits interleaved with sleeps. Now, after removing triggering PCI_EXP_LNKCTL_RL, there is no need to explicitly disable LINK_TRAINING_EN bit. So remove this code too. The problematic Compex cards described in previous git commits are correctly detected in advk_pcie_train_link() function even after applying all these changes.

Note that with this patch, and also prior this patch, some NVMe disks which support PCIe GEN3 with 8 GT/s speed are negotiated only at the lowest link speed 2.5 GT/s, independently of SPEED_GEN value. After manually triggering PCI_EXP_LNKCTL_RL bit (e.g. from userspace via setpci), these NVMe disks change link speed to 5 GT/s when SPEED_GEN was configured to GEN2. This issue first needs to be properly investigated. I will send a fix in the future.

On the other hand, some other GEN2 PCIe cards with 5 GT/s speed are autonomously by HW autonegotiated at full 5 GT/s speed without need of any software interaction.

Armada 3700 Functional Specifications describes the following steps for link training: set SPEED_GEN to GEN2, enable LINK_TRAINING_EN, poll until link training is complete, trigger PCI_EXP_LNKCTL_RL, poll until signal rate is 5 GT/s, poll until link training is complete, enable ASPM L0s.

The requirement for triggering PCI_EXP_LNKCTL_RL can be explained by the need to achieve 5 GT/s speed (as changing link speed is done by throw to recovery state entered by PCI_EXP_LNKCTL_RL) or maybe as a part of enabling ASPM L0s (but in this case ASPM L0s should have been enabled prior PCI_EXP_LNKCTL_RL).

It is unknown why the original pci-aardvark.c driver was triggering PCI_EXP_LNKCTL_RL bit before waiting for the link to be up. This does not align with neither PCIe base specifications nor with Armada 3700 Functional Specification. (Note that in older versions of aardvark, this bit was called incorrectly PCIE_CORE_LINK_TRAINING, so this may be the reason.)

It is also unknown why Armada 3700 Functional Specification says that it is needed to trigger PCI_EXP_LNKCTL_RL for GEN2 mode, as according to PCIe base specification 5 GT/s speed negotiation is supposed to be entirely autonomous, even if initial speed is 2.5 GT/s.

[1] - https://lore.kernel.org/linux-pci/[email protected]/
[2] - https://lore.kernel.org/linux-pci/[email protected]/

Signed-off-by Pali Rohár <[email protected]>

Reviewed-by Marek Behún <[email protected]>

Signed-off-by Marek Behún <[email protected]>

Cc [email protected] # f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready before training link")

Cc [email protected] # 6964494582f5 ("PCI: aardvark: Train link immediately after enabling training")

Cc [email protected] # 43fc679ced18 ("PCI: aardvark: Improve link training")

Cc [email protected] # 5169a9851daa ("PCI: aardvark: Issue PERST via GPIO")

Cc [email protected] # 96be36dbffac ("PCI: aardvark: Replace custom macros by standard linux/pci_regs.h macros")

Cc [email protected] # d0c6a3475b03 ("PCI: aardvark: Move PCIe reset card code to advk_pcie_train_link()")

Cc [email protected] # 1d1cd163d0de ("PCI: aardvark: Update comment about disabling link training")

---
 drivers/pci/controller/pci-aardvark.c | 117 ++++++++------------------
 1 file changed, 34 insertions(+), 83 deletions(-)

drivers/pci/controller/pci-aardvark.c +34 -83

index 9c509d5a9afa..2c66cdbb8dd6 100644
@@ -258,11 +258,6 @@ static inline u32 advk_readl(struct advk_pcie *pcie, u64 reg)
 	return readl(pcie->base + reg);
 }
 
-static inline u16 advk_read16(struct advk_pcie *pcie, u64 reg)
-{
-	return advk_readl(pcie, (reg & ~0x3)) >> ((reg & 0x3) * 8);
-}
-
 static int advk_pcie_link_up(struct advk_pcie *pcie)
 {
 	u32 val, ltssm_state;

@@ -300,23 +295,9 @@ static void advk_pcie_wait_for_retrain(struct advk_pcie *pcie)
 
 static void advk_pcie_issue_perst(struct advk_pcie *pcie)
 {
-	u32 reg;
-
 	if (!pcie->reset_gpio)
 		return;
 
-	/*
-	 * As required by PCI Express spec (PCI Express Base Specification, REV.
-	 * 4.0 PCI Express, February 19 2014, 6.6.1 Conventional Reset) a delay
-	 * for at least 100ms after de-asserting PERST# signal is needed before
-	 * link training is enabled. So ensure that link training is disabled
-	 * prior de-asserting PERST# signal to fulfill that PCI Express spec
-	 * requirement.
-	 */
-	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
-	reg &= ~LINK_TRAINING_EN;
-	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
-
 	/* 10ms delay is needed for some cards */
 	dev_info(&pcie->pdev->dev, "issuing PERST via reset GPIO for 10ms\n");
 	gpiod_set_value_cansleep(pcie->reset_gpio, 1);

@@ -324,53 +305,46 @@ static void advk_pcie_issue_perst(struct advk_pcie *pcie)
 	gpiod_set_value_cansleep(pcie->reset_gpio, 0);
 }
 
-static int advk_pcie_train_at_gen(struct advk_pcie *pcie, int gen)
+static void advk_pcie_train_link(struct advk_pcie *pcie)
 {
-	int ret, neg_gen;
+	struct device *dev = &pcie->pdev->dev;
 	u32 reg;
+	int ret;
 
-	/* Setup link speed */
+	/*
+	 * Setup PCIe rev / gen compliance based on device tree property
+	 * 'max-link-speed' which also forces maximal link speed.
+	 */
 	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
 	reg &= ~PCIE_GEN_SEL_MSK;
-	if (gen == 3)
+	if (pcie->link_gen == 3)
 		reg |= SPEED_GEN_3;
-	else if (gen == 2)
+	else if (pcie->link_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.
+	 * Set maximal link speed value also into PCIe Link Control 2 register.
+	 * Armada 3700 Functional Specification says that default value is based
+	 * on SPEED_GEN but tests showed that default value is always 8.0 GT/s.
 	 */
+	reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKCTL2);
+	reg &= ~PCI_EXP_LNKCTL2_TLS;
+	if (pcie->link_gen == 3)
+		reg |= PCI_EXP_LNKCTL2_TLS_8_0GT;
+	else if (pcie->link_gen == 2)
+		reg |= PCI_EXP_LNKCTL2_TLS_5_0GT;
+	else
+		reg |= PCI_EXP_LNKCTL2_TLS_2_5GT;
+	advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKCTL2);
+
+	/* Enable link training after selecting PCIe generation */
 	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_PCIEXP_CAP + PCI_EXP_LNKCTL);
-	reg |= PCI_EXP_LNKCTL_RL;
-	advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKCTL);
-
-	ret = advk_pcie_wait_for_link(pcie);
-	if (ret)
-		return ret;
-
-	reg = advk_read16(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_LNKSTA);
-	neg_gen = reg & PCI_EXP_LNKSTA_CLS;
-
-	return neg_gen;
-}
-
-static void advk_pcie_train_link(struct advk_pcie *pcie)
-{
-	struct device *dev = &pcie->pdev->dev;
-	int neg_gen = -1, gen;
-
 	/*
 	 * Reset PCIe card via PERST# signal. Some cards are not detected
 	 * during link training when they are in some non-initial state.

@@ -381,41 +355,18 @@ static void advk_pcie_train_link(struct advk_pcie *pcie)
 	 * PERST# signal could have been asserted by pinctrl subsystem before
 	 * probe() callback has been called or issued explicitly by reset gpio
 	 * function advk_pcie_issue_perst(), 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.
-	 */
-	msleep(PCI_PM_D3COLD_WAIT);
-
-	/*
-	 * Try link training at link gen specified by device tree property
-	 * 'max-link-speed'. 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.
+	 * fundamental reset. As required by PCI Express spec (PCI Express
+	 * Base Specification, REV. 4.0 PCI Express, February 19 2014, 6.6.1
+	 * Conventional Reset) a delay for at least 100ms after such a reset
+	 * before sending a Configuration Request to the device is needed.
+	 * So wait until PCIe link is up. Function advk_pcie_wait_for_link()
+	 * waits for link at least 900ms.
 	 */
-	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");
+	ret = advk_pcie_wait_for_link(pcie);
+	if (ret < 0)
+		dev_err(dev, "link never came up\n");
+	else
+		dev_info(dev, "link up\n");
 }
 
 /*

Signature · 1 line

2.32.0

Quoted 139 lines · Marek Behún

From: Pali Rohár <[email protected]>

Fix multiple link training issues in aardvark driver. The main reason of these issues was misunderstanding of what certain registers do, since their names and comments were misleading: before commit 96be36dbffac ("PCI:
aardvark: Replace custom macros by standard linux/pci_regs.h macros"), the pci-aardvark.c driver used custom macros for accessing standard PCIe Root Bridge registers, and misleading comments did not help to understand what the code was really doing.

After doing more tests and experiments I've come to the conclusion that the SPEED_GEN register in aardvark sets the PCIe revision / generation compliance and forces maximal link speed. Both GEN3 and GEN2 values set the read-only PCI_EXP_FLAGS_VERS bits (PCIe capabilities version of Root Bridge) to value 2, while GEN1 value sets PCI_EXP_FLAGS_VERS to 1, which matches with PCI Express specifications revisions 3, 2 and 1 respectively. Changing SPEED_GEN also sets the read-only bits PCI_EXP_LNKCAP_SLS and PCI_EXP_LNKCAP2_SLS to corresponding speed.

(Note that PCI Express rev 1 specification does not define PCI_EXP_LNKCAP2
and PCI_EXP_LNKCTL2 registers and when SPEED_GEN is set to GEN1 (which also sets PCI_EXP_FLAGS_VERS set to 1), lspci cannot access PCI_EXP_LNKCAP2 and PCI_EXP_LNKCTL2 registers.)

Changing PCIe link speed can be done via PCI_EXP_LNKCTL2_TLS bits of PCI_EXP_LNKCTL2 register. Armada 3700 Functional Specifications says that the default value of PCI_EXP_LNKCTL2_TLS is based on SPEED_GEN value, but tests showed that the default value is always 8.0 GT/s, independently of speed set by SPEED_GEN. So after setting SPEED_GEN, we must also set value in PCI_EXP_LNKCTL2 register via PCI_EXP_LNKCTL2_TLS bits.

Triggering PCI_EXP_LNKCTL_RL bit immediately after setting LINK_TRAINING_EN bit actually doesn't do anything. Tests have shown that a delay is needed after enabling LINK_TRAINING_EN bit. As triggering PCI_EXP_LNKCTL_RL currently does nothing, remove it.

Commit 43fc679ced18 ("PCI: aardvark: Improve link training") introduced code which sets SPEED_GEN register based on negotiated link speed from PCI_EXP_LNKSTA_CLS bits of PCI_EXP_LNKSTA register. This code was added to fix detection of Compex WLE900VX (Atheros QCA9880) WiFi GEN1 PCIe cards, as otherwise these cards were "invisible" on PCIe bus (probably because they crashed). But apparently more people reported the same issues with these cards also with other PCIe controllers [1] and I was able to reproduce this issue also with other "noname" WiFi cards based on Atheros QCA9890 chip (with the same PCI vendor/device ids as Atheros QCA9880). So this is not an issue in aardvark but rather an issue in Atheros QCA98xx chips. Also, this issue only exists if the kernel is compiled with PCIe ASPM support, and a generic workaround for this is to change PCIe Bridge to 2.5 GT/s link speed via PCI_EXP_LNKCTL2_TLS_2_5GT bits in PCI_EXP_LNKCTL2 register [2], before triggering PCI_EXP_LNKCTL_RL bit. This workaround also works when SPEED_GEN is set to value GEN2 (5 GT/s). So remove this hack completely in the aardvark driver and always set SPEED_GEN to value from 'max-link-speed' DT property. Fix for Atheros QCA98xx chips is handled separately by patch [2].

These two things (code for triggering PCI_EXP_LNKCTL_RL bit and changing SPEED_GEN value) also explain why commit 6964494582f5 ("PCI: aardvark:
Train link immediately after enabling training") somehow fixed detection of those problematic Compex cards with Atheros chips: if triggering link retraining (via PCI_EXP_LNKCTL_RL bit) was done immediately after enabling link training (via LINK_TRAINING_EN), it did nothing. If there was a specific delay, aardvark HW already initialized PCIe link and therefore triggering link retraining caused the above issue. Compex cards triggered link down event and disappeared from the PCIe bus.

Commit f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready before training link") added 100ms sleep before calling 'Start link training' command and explained that it is a requirement of PCI Express specification. But the code after this 100ms sleep was not doing 'Start link training', rather it triggered PCI_EXP_LNKCTL_RL bit via PCIe Root Bridge to put link into Recovery state.

The required delay after fundamental reset is already done in function advk_pcie_wait_for_link() which also checks whether PCIe link is up. So after removing the code which triggers PCI_EXP_LNKCTL_RL bit on PCIe Root Bridge, there is no need to wait 100ms again. Remove the extra msleep() call and update comment about the delay required by the PCI Express specification.

According to Marvell Armada 3700 Functional Specifications, Link training should be enabled via aardvark register LINK_TRAINING_EN after selecting PCIe generation and x1 lane. There is no need to disable it prior resetting card via PERST# signal. This disabling code was introduced in commit 5169a9851daa ("PCI: aardvark: Issue PERST via GPIO") as a workaround for some Atheros cards. It turns out that this also is Atheros specific issue and affects any PCIe controller, not only aardvark. Moreover this Atheros issue was triggered by juggling with PCI_EXP_LNKCTL_RL, LINK_TRAINING_EN and SPEED_GEN bits interleaved with sleeps. Now, after removing triggering PCI_EXP_LNKCTL_RL, there is no need to explicitly disable LINK_TRAINING_EN bit. So remove this code too. The problematic Compex cards described in previous git commits are correctly detected in advk_pcie_train_link() function even after applying all these changes.

Note that with this patch, and also prior this patch, some NVMe disks which support PCIe GEN3 with 8 GT/s speed are negotiated only at the lowest link speed 2.5 GT/s, independently of SPEED_GEN value. After manually triggering PCI_EXP_LNKCTL_RL bit (e.g. from userspace via setpci), these NVMe disks change link speed to 5 GT/s when SPEED_GEN was configured to GEN2. This issue first needs to be properly investigated. I will send a fix in the future.

On the other hand, some other GEN2 PCIe cards with 5 GT/s speed are autonomously by HW autonegotiated at full 5 GT/s speed without need of any software interaction.

Armada 3700 Functional Specifications describes the following steps for link training: set SPEED_GEN to GEN2, enable LINK_TRAINING_EN, poll until link training is complete, trigger PCI_EXP_LNKCTL_RL, poll until signal rate is 5 GT/s, poll until link training is complete, enable ASPM L0s.

The requirement for triggering PCI_EXP_LNKCTL_RL can be explained by the need to achieve 5 GT/s speed (as changing link speed is done by throw to recovery state entered by PCI_EXP_LNKCTL_RL) or maybe as a part of enabling ASPM L0s (but in this case ASPM L0s should have been enabled prior PCI_EXP_LNKCTL_RL).

It is unknown why the original pci-aardvark.c driver was triggering PCI_EXP_LNKCTL_RL bit before waiting for the link to be up. This does not align with neither PCIe base specifications nor with Armada 3700 Functional Specification. (Note that in older versions of aardvark, this bit was called incorrectly PCIE_CORE_LINK_TRAINING, so this may be the reason.)

It is also unknown why Armada 3700 Functional Specification says that it is needed to trigger PCI_EXP_LNKCTL_RL for GEN2 mode, as according to PCIe base specification 5 GT/s speed negotiation is supposed to be entirely autonomous, even if initial speed is 2.5 GT/s.

[1] - https://lore.kernel.org/linux-pci/[email protected]/
[2] - https://lore.kernel.org/linux-pci/[email protected]/

Signed-off-by Pali Rohár <[email protected]>

Reviewed-by Marek Behún <[email protected]>

Signed-off-by Marek Behún <[email protected]>

Cc [email protected] # f4c7d053d7f7 ("PCI: aardvark: Wait for endpoint to be ready before training link")

Cc [email protected] # 6964494582f5 ("PCI: aardvark: Train link immediately after enabling training")

Cc [email protected] # 43fc679ced18 ("PCI: aardvark: Improve link training")

Cc [email protected] # 5169a9851daa ("PCI: aardvark: Issue PERST via GPIO")

Cc [email protected] # 96be36dbffac ("PCI: aardvark: Replace custom macros by standard linux/pci_regs.h macros")

Cc [email protected] # d0c6a3475b03 ("PCI: aardvark: Move PCIe reset card code to advk_pcie_train_link()")

Cc [email protected] # 1d1cd163d0de ("PCI: aardvark: Update comment about disabling link training")

Quoted 3 lines

---
 drivers/pci/controller/pci-aardvark.c | 117 ++++++++------------------
 1 file changed, 34 insertions(+), 83 deletions(-)  

This is a very convoluted fix; it is well explained but the number of patches going into stable and the complexity of it make me ask how confident you are this won't trigger any regressions.

It is just a heads-up to make you think whether it is better to hold the stable tags till we are confident enough.

Please let me know.

as requested I am sending v2 of this series.

Changes since v1:
- updated commit message in patch 6 as suggested by Mark
- updated patch 6 to also drop the early return
- changed the LTSSM definitions to enum in patch 12
- dropped the Fixes tags for those patches where it was inappropriate

[...]

Quoted 9 lines · Marek Behún

as requested I am sending v2 of this series.

Changes since v1:
- updated commit message in patch 6 as suggested by Mark
- updated patch 6 to also drop the early return
- changed the LTSSM definitions to enum in patch 12
- dropped the Fixes tags for those patches where it was inappropriate

[...]

I rewrote some logs, dropped some stable tags. This series raised a couple of interesting questions that may be relevant for PCI core as well, it'd be great if those won't be lost but I feel it is time to merge this series to help Marek/Pali cut the patch delta and give this code wider testing.

I have applied it to pci/aardvark, please _do_ have a look and report back any issue with it.