[PATCH] pci: Do not enable PCIe ASMedia ASM2824 workaround by default

[PATCH] pci: Do not enable PCIe ASMedia ASM2824 workaround by default

[Pali Rohár]

PCIe ASMedia ASM2824 workaround code unconditionally increase size of all
SPL binaries with PCIe support, even those which do not use ASMedia ASM2824
PCIe switch.

Moreover this workaround is enabled for all existing hardware and also all
future PCIe hardware, which opens a hole that other PCIe vendors may
introduce same HW issue as ASMedia and nobody would notice it because
U-Boot automatically apply workaround for it.

Last iteration of Linux kernel patch for this workaround is bound just to
ASMedia ASM2824 vendor+device id and not enabled on all PCIe hardware.

So do not unconditionally enable PCIe ASMedia ASM2824 workaround on all
hardware. Instead introduce a new config option which is disabled by
default. This decrease SPL size and also ensure that workaround is not
blindly or inexactly enabled.

To make workaround code local, move it into separate file, so pci_auto.c
contains only genetic auto configuration code.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
Hello! What do you think about this change? I think it is good
compromise between enable this workaround for all builds on all boards
and enable it only based on device id. Or would it be better to restrict
this workaround just for ASM2824 device like the last iteration of
kernel patch?
---
 drivers/pci/Kconfig                   |   6 +
 drivers/pci/Makefile                  |   1 +
 drivers/pci/pci_auto.c                | 171 +-----------------------
 drivers/pci/pcie_asm2824_workaround.c | 183 ++++++++++++++++++++++++++
 4 files changed, 194 insertions(+), 167 deletions(-)
 create mode 100644 drivers/pci/pcie_asm2824_workaround.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 47cd074aa14c..16894929214a 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -327,4 +327,10 @@ config PCIE_UNIPHIER
 	  Say Y here if you want to enable PCIe controller support on
 	  UniPhier SoCs.
 
+config PCIE_ASM2824_WORKAROUND
+	bool "ASMedia ASM2824 PCIe workaround"
+	help
+	  Say Y here if you want to enable link training failures for
+	  ASMedia ASM2824 PCIe switch.
+
 endif
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 04f623652f09..4aa4e7c7285d 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -50,3 +50,4 @@ obj-$(CONFIG_PCI_OCTEONTX) += pci_octeontx.o
 obj-$(CONFIG_PCIE_OCTEON) += pcie_octeon.o
 obj-$(CONFIG_PCIE_DW_SIFIVE) += pcie_dw_sifive.o
 obj-$(CONFIG_PCIE_UNIPHIER) += pcie_uniphier.o
+obj-$(CONFIG_PCIE_ASM2824_WORKAROUND) += pcie_asm2824_workaround.o
diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
index c7968926a17f..851bf3bc55b1 100644
--- a/drivers/pci/pci_auto.c
+++ b/drivers/pci/pci_auto.c
@@ -13,7 +13,6 @@
 #include <errno.h>
 #include <log.h>
 #include <pci.h>
-#include <time.h>
 #include "pci_internal.h"
 
 /* the user can define CONFIG_SYS_PCI_CACHE_LINE_SIZE to avoid problems */
@@ -182,167 +181,7 @@ static void dm_pciauto_setup_device(struct udevice *dev,
 	dm_pci_write_config8(dev, PCI_LATENCY_TIMER, 0x80);
 }
 
-/*
- * Check if the link of a downstream PCIe port operates correctly.
- *
- * For that check if the optional Data Link Layer Link Active status gets
- * on within a 200ms period or failing that wait until the completion of
- * that period and check if link training has shown the completed status
- * continuously throughout the second half of that period.
- *
- * Observation with the ASMedia ASM2824 Gen 3 switch indicates it takes
- * 11-44ms to indicate the Data Link Layer Link Active status at 2.5GT/s,
- * though it may take a couple of link training iterations.
- */
-static bool dm_pciauto_exp_link_stable(struct udevice *dev, int pcie_off)
-{
-	u64 loops = 0, trcount = 0, ntrcount = 0, flips = 0;
-	bool dllla, lnktr, plnktr;
-	u16 exp_lnksta;
-	pci_dev_t bdf;
-	u64 end;
-
-	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA, &exp_lnksta);
-	plnktr = !!(exp_lnksta & PCI_EXP_LNKSTA_LT);
-
-	end = get_ticks() + usec_to_tick(200000);
-	do {
-		dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA,
-				     &exp_lnksta);
-		dllla = !!(exp_lnksta & PCI_EXP_LNKSTA_DLLLA);
-		lnktr = !!(exp_lnksta & PCI_EXP_LNKSTA_LT);
-
-		flips += plnktr ^ lnktr;
-		if (lnktr) {
-			ntrcount = 0;
-			trcount++;
-		} else {
-			ntrcount++;
-		}
-		loops++;
-
-		plnktr = lnktr;
-	} while (!dllla && get_ticks() < end);
-
-	bdf = dm_pci_get_bdf(dev);
-	debug("PCI Autoconfig: %02x.%02x.%02x: Fixup link: DL active: %u; "
-	      "%3llu flips, %6llu loops of which %6llu while training, "
-	      "final %6llu stable\n",
-	      PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf),
-	      (unsigned int)dllla,
-	      (unsigned long long)flips, (unsigned long long)loops,
-	      (unsigned long long)trcount, (unsigned long long)ntrcount);
-
-	return dllla || ntrcount >= loops / 2;
-}
-
-/*
- * Retrain the link of a downstream PCIe port by hand if necessary.
- *
- * This is needed at least where a downstream port of the ASMedia ASM2824
- * Gen 3 switch is wired to the upstream port of the Pericom PI7C9X2G304
- * Gen 2 switch, and observed with the Delock Riser Card PCI Express x1 >
- * 2 x PCIe x1 device, P/N 41433, plugged into the SiFive HiFive Unmatched
- * board.
- *
- * In such a configuration the switches are supposed to negotiate the link
- * speed of preferably 5.0GT/s, falling back to 2.5GT/s.  However the link
- * continues switching between the two speeds indefinitely and the data
- * link layer never reaches the active state, with link training reported
- * repeatedly active ~84% of the time.  Forcing the target link speed to
- * 2.5GT/s with the upstream ASM2824 device makes the two switches talk to
- * each other correctly however.  And more interestingly retraining with a
- * higher target link speed afterwards lets the two successfully negotiate
- * 5.0GT/s.
- *
- * As this can potentially happen with any device and is cheap in the case
- * of correctly operating hardware, let's do it for all downstream ports,
- * for root complexes, PCIe switches and PCI/PCI-X to PCIe bridges.
- *
- * First check if automatic link training may have failed to complete, as
- * indicated by the optional Data Link Layer Link Active status being off
- * and the Link Bandwidth Management Status indicating that hardware has
- * changed the link speed or width in an attempt to correct unreliable
- * link operation.  If this is the case, then check if the link operates
- * correctly by seeing whether it is being trained excessively.  If it is,
- * then conclude the link is broken.
- *
- * In that case restrict the speed to 2.5GT/s, observing that the Target
- * Link Speed field is sticky and therefore the link will stay restricted
- * even after a device reset is later made by an OS that is unaware of the
- * problem.  With the speed restricted request that the link be retrained
- * and check again if the link operates correctly.  If not, then set the
- * Target Link Speed back to the original value.
- *
- * This requires the presence of the Link Control 2 register, so make sure
- * the PCI Express Capability Version is at least 2.  Also don't try, for
- * obvious reasons, to limit the speed if 2.5GT/s is the only link speed
- * supported.
- */
-static void dm_pciauto_exp_fixup_link(struct udevice *dev, int pcie_off)
-{
-	u16 exp_lnksta, exp_lnkctl, exp_lnkctl2;
-	u16 exp_flags, exp_type, exp_version;
-	u32 exp_lnkcap;
-	pci_dev_t bdf;
-
-	dm_pci_read_config16(dev, pcie_off + PCI_EXP_FLAGS, &exp_flags);
-	exp_version = exp_flags & PCI_EXP_FLAGS_VERS;
-	if (exp_version < 2)
-		return;
-
-	exp_type = (exp_flags & PCI_EXP_FLAGS_TYPE) >> 4;
-	switch (exp_type) {
-	case PCI_EXP_TYPE_ROOT_PORT:
-	case PCI_EXP_TYPE_DOWNSTREAM:
-	case PCI_EXP_TYPE_PCIE_BRIDGE:
-		break;
-	default:
-		return;
-	}
-
-	dm_pci_read_config32(dev, pcie_off + PCI_EXP_LNKCAP, &exp_lnkcap);
-	if ((exp_lnkcap & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB)
-		return;
-
-	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA, &exp_lnksta);
-	if ((exp_lnksta & (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_DLLLA)) !=
-	    PCI_EXP_LNKSTA_LBMS)
-		return;
-
-	if (dm_pciauto_exp_link_stable(dev, pcie_off))
-		return;
-
-	bdf = dm_pci_get_bdf(dev);
-	printf("PCI Autoconfig: %02x.%02x.%02x: "
-	       "Downstream link non-functional\n",
-	       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
-	printf("PCI Autoconfig: %02x.%02x.%02x: "
-	       "Retrying with speed restricted to 2.5GT/s...\n",
-	       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
-
-	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKCTL, &exp_lnkctl);
-	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKCTL2, &exp_lnkctl2);
-
-	dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
-			      (exp_lnkctl2 & ~PCI_EXP_LNKCTL2_TLS) |
-			      PCI_EXP_LNKCTL2_TLS_2_5GT);
-	dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
-			      exp_lnkctl | PCI_EXP_LNKCTL_RL);
-
-	if (dm_pciauto_exp_link_stable(dev, pcie_off)) {
-		printf("PCI Autoconfig: %02x.%02x.%02x: Succeeded!\n",
-		       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
-	} else {
-		printf("PCI Autoconfig: %02x.%02x.%02x: Failed!\n",
-		       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
-
-		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
-				      exp_lnkctl2);
-		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
-				      exp_lnkctl | PCI_EXP_LNKCTL_RL);
-	}
-}
+void pcie_asm2824_workaround(struct udevice *dev);
 
 void dm_pciauto_prescan_setup_bridge(struct udevice *dev, int sub_bus)
 {
@@ -353,7 +192,6 @@ void dm_pciauto_prescan_setup_bridge(struct udevice *dev, int sub_bus)
 	u8 io_32;
 	struct udevice *ctlr = pci_get_controller(dev);
 	struct pci_controller *ctlr_hose = dev_get_uclass_priv(ctlr);
-	int pcie_off;
 
 	pci_mem = ctlr_hose->pci_mem;
 	pci_prefetch = ctlr_hose->pci_prefetch;
@@ -440,10 +278,9 @@ void dm_pciauto_prescan_setup_bridge(struct udevice *dev, int sub_bus)
 		}
 	}
 
-	/* For PCIe devices see if we need to retrain the link by hand */
-	pcie_off = dm_pci_find_capability(dev, PCI_CAP_ID_EXP);
-	if (pcie_off)
-		dm_pciauto_exp_fixup_link(dev, pcie_off);
+#ifdef CONFIG_PCIE_ASM2824_WORKAROUND
+	pcie_asm2824_workaround(dev);
+#endif
 
 	/* Enable memory and I/O accesses, enable bus master */
 	dm_pci_write_config16(dev, PCI_COMMAND, cmdstat | PCI_COMMAND_MASTER);
diff --git a/drivers/pci/pcie_asm2824_workaround.c b/drivers/pci/pcie_asm2824_workaround.c
new file mode 100644
index 000000000000..9415346faf22
--- /dev/null
+++ b/drivers/pci/pcie_asm2824_workaround.c
@@ -0,0 +1,183 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ASMedia ASM2824 PCIe workaround
+ *
+ * Copyright (c) 2021  Maciej W. Rozycki <macro@orcam.me.uk>
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <pci.h>
+#include <time.h>
+
+/*
+ * Check if the link of a downstream PCIe port operates correctly.
+ *
+ * For that check if the optional Data Link Layer Link Active status gets
+ * on within a 200ms period or failing that wait until the completion of
+ * that period and check if link training has shown the completed status
+ * continuously throughout the second half of that period.
+ *
+ * Observation with the ASMedia ASM2824 Gen 3 switch indicates it takes
+ * 11-44ms to indicate the Data Link Layer Link Active status at 2.5GT/s,
+ * though it may take a couple of link training iterations.
+ */
+static bool dm_pciauto_exp_link_stable(struct udevice *dev, int pcie_off)
+{
+	u64 loops = 0, trcount = 0, ntrcount = 0, flips = 0;
+	bool dllla, lnktr, plnktr;
+	u16 exp_lnksta;
+	pci_dev_t bdf;
+	u64 end;
+
+	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA, &exp_lnksta);
+	plnktr = !!(exp_lnksta & PCI_EXP_LNKSTA_LT);
+
+	end = get_ticks() + usec_to_tick(200000);
+	do {
+		dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA,
+				     &exp_lnksta);
+		dllla = !!(exp_lnksta & PCI_EXP_LNKSTA_DLLLA);
+		lnktr = !!(exp_lnksta & PCI_EXP_LNKSTA_LT);
+
+		flips += plnktr ^ lnktr;
+		if (lnktr) {
+			ntrcount = 0;
+			trcount++;
+		} else {
+			ntrcount++;
+		}
+		loops++;
+
+		plnktr = lnktr;
+	} while (!dllla && get_ticks() < end);
+
+	bdf = dm_pci_get_bdf(dev);
+	debug("PCI Autoconfig: %02x.%02x.%02x: Fixup link: DL active: %u; "
+	      "%3llu flips, %6llu loops of which %6llu while training, "
+	      "final %6llu stable\n",
+	      PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf),
+	      (unsigned int)dllla,
+	      (unsigned long long)flips, (unsigned long long)loops,
+	      (unsigned long long)trcount, (unsigned long long)ntrcount);
+
+	return dllla || ntrcount >= loops / 2;
+}
+
+/*
+ * Retrain the link of a downstream PCIe port by hand if necessary.
+ *
+ * This is needed at least where a downstream port of the ASMedia ASM2824
+ * Gen 3 switch is wired to the upstream port of the Pericom PI7C9X2G304
+ * Gen 2 switch, and observed with the Delock Riser Card PCI Express x1 >
+ * 2 x PCIe x1 device, P/N 41433, plugged into the SiFive HiFive Unmatched
+ * board.
+ *
+ * In such a configuration the switches are supposed to negotiate the link
+ * speed of preferably 5.0GT/s, falling back to 2.5GT/s.  However the link
+ * continues switching between the two speeds indefinitely and the data
+ * link layer never reaches the active state, with link training reported
+ * repeatedly active ~84% of the time.  Forcing the target link speed to
+ * 2.5GT/s with the upstream ASM2824 device makes the two switches talk to
+ * each other correctly however.  And more interestingly retraining with a
+ * higher target link speed afterwards lets the two successfully negotiate
+ * 5.0GT/s.
+ *
+ * As this can potentially happen with any device and is cheap in the case
+ * of correctly operating hardware, let's do it for all downstream ports,
+ * for root complexes, PCIe switches and PCI/PCI-X to PCIe bridges.
+ *
+ * First check if automatic link training may have failed to complete, as
+ * indicated by the optional Data Link Layer Link Active status being off
+ * and the Link Bandwidth Management Status indicating that hardware has
+ * changed the link speed or width in an attempt to correct unreliable
+ * link operation.  If this is the case, then check if the link operates
+ * correctly by seeing whether it is being trained excessively.  If it is,
+ * then conclude the link is broken.
+ *
+ * In that case restrict the speed to 2.5GT/s, observing that the Target
+ * Link Speed field is sticky and therefore the link will stay restricted
+ * even after a device reset is later made by an OS that is unaware of the
+ * problem.  With the speed restricted request that the link be retrained
+ * and check again if the link operates correctly.  If not, then set the
+ * Target Link Speed back to the original value.
+ *
+ * This requires the presence of the Link Control 2 register, so make sure
+ * the PCI Express Capability Version is at least 2.  Also don't try, for
+ * obvious reasons, to limit the speed if 2.5GT/s is the only link speed
+ * supported.
+ */
+static void dm_pciauto_exp_fixup_link(struct udevice *dev, int pcie_off)
+{
+	u16 exp_lnksta, exp_lnkctl, exp_lnkctl2;
+	u16 exp_flags, exp_type, exp_version;
+	u32 exp_lnkcap;
+	pci_dev_t bdf;
+
+	dm_pci_read_config16(dev, pcie_off + PCI_EXP_FLAGS, &exp_flags);
+	exp_version = exp_flags & PCI_EXP_FLAGS_VERS;
+	if (exp_version < 2)
+		return;
+
+	exp_type = (exp_flags & PCI_EXP_FLAGS_TYPE) >> 4;
+	switch (exp_type) {
+	case PCI_EXP_TYPE_ROOT_PORT:
+	case PCI_EXP_TYPE_DOWNSTREAM:
+	case PCI_EXP_TYPE_PCIE_BRIDGE:
+		break;
+	default:
+		return;
+	}
+
+	dm_pci_read_config32(dev, pcie_off + PCI_EXP_LNKCAP, &exp_lnkcap);
+	if ((exp_lnkcap & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB)
+		return;
+
+	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA, &exp_lnksta);
+	if ((exp_lnksta & (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_DLLLA)) !=
+	    PCI_EXP_LNKSTA_LBMS)
+		return;
+
+	if (dm_pciauto_exp_link_stable(dev, pcie_off))
+		return;
+
+	bdf = dm_pci_get_bdf(dev);
+	printf("PCI Autoconfig: %02x.%02x.%02x: "
+	       "Downstream link non-functional\n",
+	       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
+	printf("PCI Autoconfig: %02x.%02x.%02x: "
+	       "Retrying with speed restricted to 2.5GT/s...\n",
+	       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
+
+	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKCTL, &exp_lnkctl);
+	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKCTL2, &exp_lnkctl2);
+
+	dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
+			      (exp_lnkctl2 & ~PCI_EXP_LNKCTL2_TLS) |
+			      PCI_EXP_LNKCTL2_TLS_2_5GT);
+	dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
+			      exp_lnkctl | PCI_EXP_LNKCTL_RL);
+
+	if (dm_pciauto_exp_link_stable(dev, pcie_off)) {
+		printf("PCI Autoconfig: %02x.%02x.%02x: Succeeded!\n",
+		       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
+	} else {
+		printf("PCI Autoconfig: %02x.%02x.%02x: Failed!\n",
+		       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
+
+		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
+				      exp_lnkctl2);
+		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
+				      exp_lnkctl | PCI_EXP_LNKCTL_RL);
+	}
+}
+
+void pcie_asm2824_workaround(struct udevice *dev)
+{
+	int pcie_off;
+
+	/* For PCIe devices see if we need to retrain the link by hand */
+	pcie_off = dm_pci_find_capability(dev, PCI_CAP_ID_EXP);
+	if (pcie_off)
+		dm_pciauto_exp_fixup_link(dev, pcie_off);
+}
-- 
2.20.1

Re: [PATCH] pci: Do not enable PCIe ASMedia ASM2824 workaround by default

[Stefan Roese]

On 4/6/22 17:09, Pali Rohár wrote:
> PCIe ASMedia ASM2824 workaround code unconditionally increase size of all
> SPL binaries with PCIe support, even those which do not use ASMedia ASM2824
> PCIe switch.
> 
> Moreover this workaround is enabled for all existing hardware and also all
> future PCIe hardware, which opens a hole that other PCIe vendors may
> introduce same HW issue as ASMedia and nobody would notice it because
> U-Boot automatically apply workaround for it.

I agree, that we should not do this unconditionally in all cases,
without the option for users to disable this. Or even have it disabled
by default for most boards.

> Last iteration of Linux kernel patch for this workaround is bound just to
> ASMedia ASM2824 vendor+device id and not enabled on all PCIe hardware.
> 
> So do not unconditionally enable PCIe ASMedia ASM2824 workaround on all
> hardware. Instead introduce a new config option which is disabled by
> default. This decrease SPL size and also ensure that workaround is not
> blindly or inexactly enabled.
> 
> To make workaround code local, move it into separate file, so pci_auto.c
> contains only genetic auto configuration code.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> Hello! What do you think about this change? I think it is good
> compromise between enable this workaround for all builds on all boards
> and enable it only based on device id. Or would it be better to restrict
> this workaround just for ASM2824 device like the last iteration of
> kernel patch?

I'm not sure if we should name this "workaround" ASM2824, even though
it's currently (only) targeted exactly for this PCIe switch. It might
be helpful for other PCIe switches as well. So perhaps it's better to
give this function a more generic name instead? With this change, it
makes perhaps also sense to keep this function in pci_auto.c but also
rename the Kconfig option to some more generic version.

Another comment below...

> ---
>   drivers/pci/Kconfig                   |   6 +
>   drivers/pci/Makefile                  |   1 +
>   drivers/pci/pci_auto.c                | 171 +-----------------------
>   drivers/pci/pcie_asm2824_workaround.c | 183 ++++++++++++++++++++++++++
>   4 files changed, 194 insertions(+), 167 deletions(-)
>   create mode 100644 drivers/pci/pcie_asm2824_workaround.c
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 47cd074aa14c..16894929214a 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -327,4 +327,10 @@ config PCIE_UNIPHIER
>   	  Say Y here if you want to enable PCIe controller support on
>   	  UniPhier SoCs.
>   
> +config PCIE_ASM2824_WORKAROUND
> +	bool "ASMedia ASM2824 PCIe workaround"
> +	help
> +	  Say Y here if you want to enable link training failures for
> +	  ASMedia ASM2824 PCIe switch.

"to enable link training failures" sound misleading IMHO. Perhaps somthing
like this is better:

"...to enable link re-training upon failures for PCIe switches. This
seems to be helpful on the ASMedia ASM2824 PCIe switch."

Thanks,
Stefan

> +
>   endif
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 04f623652f09..4aa4e7c7285d 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -50,3 +50,4 @@ obj-$(CONFIG_PCI_OCTEONTX) += pci_octeontx.o
>   obj-$(CONFIG_PCIE_OCTEON) += pcie_octeon.o
>   obj-$(CONFIG_PCIE_DW_SIFIVE) += pcie_dw_sifive.o
>   obj-$(CONFIG_PCIE_UNIPHIER) += pcie_uniphier.o
> +obj-$(CONFIG_PCIE_ASM2824_WORKAROUND) += pcie_asm2824_workaround.o
> diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
> index c7968926a17f..851bf3bc55b1 100644
> --- a/drivers/pci/pci_auto.c
> +++ b/drivers/pci/pci_auto.c
> @@ -13,7 +13,6 @@
>   #include <errno.h>
>   #include <log.h>
>   #include <pci.h>
> -#include <time.h>
>   #include "pci_internal.h"
>   
>   /* the user can define CONFIG_SYS_PCI_CACHE_LINE_SIZE to avoid problems */
> @@ -182,167 +181,7 @@ static void dm_pciauto_setup_device(struct udevice *dev,
>   	dm_pci_write_config8(dev, PCI_LATENCY_TIMER, 0x80);
>   }
>   
> -/*
> - * Check if the link of a downstream PCIe port operates correctly.
> - *
> - * For that check if the optional Data Link Layer Link Active status gets
> - * on within a 200ms period or failing that wait until the completion of
> - * that period and check if link training has shown the completed status
> - * continuously throughout the second half of that period.
> - *
> - * Observation with the ASMedia ASM2824 Gen 3 switch indicates it takes
> - * 11-44ms to indicate the Data Link Layer Link Active status at 2.5GT/s,
> - * though it may take a couple of link training iterations.
> - */
> -static bool dm_pciauto_exp_link_stable(struct udevice *dev, int pcie_off)
> -{
> -	u64 loops = 0, trcount = 0, ntrcount = 0, flips = 0;
> -	bool dllla, lnktr, plnktr;
> -	u16 exp_lnksta;
> -	pci_dev_t bdf;
> -	u64 end;
> -
> -	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA, &exp_lnksta);
> -	plnktr = !!(exp_lnksta & PCI_EXP_LNKSTA_LT);
> -
> -	end = get_ticks() + usec_to_tick(200000);
> -	do {
> -		dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA,
> -				     &exp_lnksta);
> -		dllla = !!(exp_lnksta & PCI_EXP_LNKSTA_DLLLA);
> -		lnktr = !!(exp_lnksta & PCI_EXP_LNKSTA_LT);
> -
> -		flips += plnktr ^ lnktr;
> -		if (lnktr) {
> -			ntrcount = 0;
> -			trcount++;
> -		} else {
> -			ntrcount++;
> -		}
> -		loops++;
> -
> -		plnktr = lnktr;
> -	} while (!dllla && get_ticks() < end);
> -
> -	bdf = dm_pci_get_bdf(dev);
> -	debug("PCI Autoconfig: %02x.%02x.%02x: Fixup link: DL active: %u; "
> -	      "%3llu flips, %6llu loops of which %6llu while training, "
> -	      "final %6llu stable\n",
> -	      PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf),
> -	      (unsigned int)dllla,
> -	      (unsigned long long)flips, (unsigned long long)loops,
> -	      (unsigned long long)trcount, (unsigned long long)ntrcount);
> -
> -	return dllla || ntrcount >= loops / 2;
> -}
> -
> -/*
> - * Retrain the link of a downstream PCIe port by hand if necessary.
> - *
> - * This is needed at least where a downstream port of the ASMedia ASM2824
> - * Gen 3 switch is wired to the upstream port of the Pericom PI7C9X2G304
> - * Gen 2 switch, and observed with the Delock Riser Card PCI Express x1 >
> - * 2 x PCIe x1 device, P/N 41433, plugged into the SiFive HiFive Unmatched
> - * board.
> - *
> - * In such a configuration the switches are supposed to negotiate the link
> - * speed of preferably 5.0GT/s, falling back to 2.5GT/s.  However the link
> - * continues switching between the two speeds indefinitely and the data
> - * link layer never reaches the active state, with link training reported
> - * repeatedly active ~84% of the time.  Forcing the target link speed to
> - * 2.5GT/s with the upstream ASM2824 device makes the two switches talk to
> - * each other correctly however.  And more interestingly retraining with a
> - * higher target link speed afterwards lets the two successfully negotiate
> - * 5.0GT/s.
> - *
> - * As this can potentially happen with any device and is cheap in the case
> - * of correctly operating hardware, let's do it for all downstream ports,
> - * for root complexes, PCIe switches and PCI/PCI-X to PCIe bridges.
> - *
> - * First check if automatic link training may have failed to complete, as
> - * indicated by the optional Data Link Layer Link Active status being off
> - * and the Link Bandwidth Management Status indicating that hardware has
> - * changed the link speed or width in an attempt to correct unreliable
> - * link operation.  If this is the case, then check if the link operates
> - * correctly by seeing whether it is being trained excessively.  If it is,
> - * then conclude the link is broken.
> - *
> - * In that case restrict the speed to 2.5GT/s, observing that the Target
> - * Link Speed field is sticky and therefore the link will stay restricted
> - * even after a device reset is later made by an OS that is unaware of the
> - * problem.  With the speed restricted request that the link be retrained
> - * and check again if the link operates correctly.  If not, then set the
> - * Target Link Speed back to the original value.
> - *
> - * This requires the presence of the Link Control 2 register, so make sure
> - * the PCI Express Capability Version is at least 2.  Also don't try, for
> - * obvious reasons, to limit the speed if 2.5GT/s is the only link speed
> - * supported.
> - */
> -static void dm_pciauto_exp_fixup_link(struct udevice *dev, int pcie_off)
> -{
> -	u16 exp_lnksta, exp_lnkctl, exp_lnkctl2;
> -	u16 exp_flags, exp_type, exp_version;
> -	u32 exp_lnkcap;
> -	pci_dev_t bdf;
> -
> -	dm_pci_read_config16(dev, pcie_off + PCI_EXP_FLAGS, &exp_flags);
> -	exp_version = exp_flags & PCI_EXP_FLAGS_VERS;
> -	if (exp_version < 2)
> -		return;
> -
> -	exp_type = (exp_flags & PCI_EXP_FLAGS_TYPE) >> 4;
> -	switch (exp_type) {
> -	case PCI_EXP_TYPE_ROOT_PORT:
> -	case PCI_EXP_TYPE_DOWNSTREAM:
> -	case PCI_EXP_TYPE_PCIE_BRIDGE:
> -		break;
> -	default:
> -		return;
> -	}
> -
> -	dm_pci_read_config32(dev, pcie_off + PCI_EXP_LNKCAP, &exp_lnkcap);
> -	if ((exp_lnkcap & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB)
> -		return;
> -
> -	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA, &exp_lnksta);
> -	if ((exp_lnksta & (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_DLLLA)) !=
> -	    PCI_EXP_LNKSTA_LBMS)
> -		return;
> -
> -	if (dm_pciauto_exp_link_stable(dev, pcie_off))
> -		return;
> -
> -	bdf = dm_pci_get_bdf(dev);
> -	printf("PCI Autoconfig: %02x.%02x.%02x: "
> -	       "Downstream link non-functional\n",
> -	       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
> -	printf("PCI Autoconfig: %02x.%02x.%02x: "
> -	       "Retrying with speed restricted to 2.5GT/s...\n",
> -	       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
> -
> -	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKCTL, &exp_lnkctl);
> -	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKCTL2, &exp_lnkctl2);
> -
> -	dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
> -			      (exp_lnkctl2 & ~PCI_EXP_LNKCTL2_TLS) |
> -			      PCI_EXP_LNKCTL2_TLS_2_5GT);
> -	dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
> -			      exp_lnkctl | PCI_EXP_LNKCTL_RL);
> -
> -	if (dm_pciauto_exp_link_stable(dev, pcie_off)) {
> -		printf("PCI Autoconfig: %02x.%02x.%02x: Succeeded!\n",
> -		       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
> -	} else {
> -		printf("PCI Autoconfig: %02x.%02x.%02x: Failed!\n",
> -		       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
> -
> -		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
> -				      exp_lnkctl2);
> -		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
> -				      exp_lnkctl | PCI_EXP_LNKCTL_RL);
> -	}
> -}
> +void pcie_asm2824_workaround(struct udevice *dev);
>   
>   void dm_pciauto_prescan_setup_bridge(struct udevice *dev, int sub_bus)
>   {
> @@ -353,7 +192,6 @@ void dm_pciauto_prescan_setup_bridge(struct udevice *dev, int sub_bus)
>   	u8 io_32;
>   	struct udevice *ctlr = pci_get_controller(dev);
>   	struct pci_controller *ctlr_hose = dev_get_uclass_priv(ctlr);
> -	int pcie_off;
>   
>   	pci_mem = ctlr_hose->pci_mem;
>   	pci_prefetch = ctlr_hose->pci_prefetch;
> @@ -440,10 +278,9 @@ void dm_pciauto_prescan_setup_bridge(struct udevice *dev, int sub_bus)
>   		}
>   	}
>   
> -	/* For PCIe devices see if we need to retrain the link by hand */
> -	pcie_off = dm_pci_find_capability(dev, PCI_CAP_ID_EXP);
> -	if (pcie_off)
> -		dm_pciauto_exp_fixup_link(dev, pcie_off);
> +#ifdef CONFIG_PCIE_ASM2824_WORKAROUND
> +	pcie_asm2824_workaround(dev);
> +#endif
>   
>   	/* Enable memory and I/O accesses, enable bus master */
>   	dm_pci_write_config16(dev, PCI_COMMAND, cmdstat | PCI_COMMAND_MASTER);
> diff --git a/drivers/pci/pcie_asm2824_workaround.c b/drivers/pci/pcie_asm2824_workaround.c
> new file mode 100644
> index 000000000000..9415346faf22
> --- /dev/null
> +++ b/drivers/pci/pcie_asm2824_workaround.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * ASMedia ASM2824 PCIe workaround
> + *
> + * Copyright (c) 2021  Maciej W. Rozycki <macro@orcam.me.uk>
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <pci.h>
> +#include <time.h>
> +
> +/*
> + * Check if the link of a downstream PCIe port operates correctly.
> + *
> + * For that check if the optional Data Link Layer Link Active status gets
> + * on within a 200ms period or failing that wait until the completion of
> + * that period and check if link training has shown the completed status
> + * continuously throughout the second half of that period.
> + *
> + * Observation with the ASMedia ASM2824 Gen 3 switch indicates it takes
> + * 11-44ms to indicate the Data Link Layer Link Active status at 2.5GT/s,
> + * though it may take a couple of link training iterations.
> + */
> +static bool dm_pciauto_exp_link_stable(struct udevice *dev, int pcie_off)
> +{
> +	u64 loops = 0, trcount = 0, ntrcount = 0, flips = 0;
> +	bool dllla, lnktr, plnktr;
> +	u16 exp_lnksta;
> +	pci_dev_t bdf;
> +	u64 end;
> +
> +	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA, &exp_lnksta);
> +	plnktr = !!(exp_lnksta & PCI_EXP_LNKSTA_LT);
> +
> +	end = get_ticks() + usec_to_tick(200000);
> +	do {
> +		dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA,
> +				     &exp_lnksta);
> +		dllla = !!(exp_lnksta & PCI_EXP_LNKSTA_DLLLA);
> +		lnktr = !!(exp_lnksta & PCI_EXP_LNKSTA_LT);
> +
> +		flips += plnktr ^ lnktr;
> +		if (lnktr) {
> +			ntrcount = 0;
> +			trcount++;
> +		} else {
> +			ntrcount++;
> +		}
> +		loops++;
> +
> +		plnktr = lnktr;
> +	} while (!dllla && get_ticks() < end);
> +
> +	bdf = dm_pci_get_bdf(dev);
> +	debug("PCI Autoconfig: %02x.%02x.%02x: Fixup link: DL active: %u; "
> +	      "%3llu flips, %6llu loops of which %6llu while training, "
> +	      "final %6llu stable\n",
> +	      PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf),
> +	      (unsigned int)dllla,
> +	      (unsigned long long)flips, (unsigned long long)loops,
> +	      (unsigned long long)trcount, (unsigned long long)ntrcount);
> +
> +	return dllla || ntrcount >= loops / 2;
> +}
> +
> +/*
> + * Retrain the link of a downstream PCIe port by hand if necessary.
> + *
> + * This is needed at least where a downstream port of the ASMedia ASM2824
> + * Gen 3 switch is wired to the upstream port of the Pericom PI7C9X2G304
> + * Gen 2 switch, and observed with the Delock Riser Card PCI Express x1 >
> + * 2 x PCIe x1 device, P/N 41433, plugged into the SiFive HiFive Unmatched
> + * board.
> + *
> + * In such a configuration the switches are supposed to negotiate the link
> + * speed of preferably 5.0GT/s, falling back to 2.5GT/s.  However the link
> + * continues switching between the two speeds indefinitely and the data
> + * link layer never reaches the active state, with link training reported
> + * repeatedly active ~84% of the time.  Forcing the target link speed to
> + * 2.5GT/s with the upstream ASM2824 device makes the two switches talk to
> + * each other correctly however.  And more interestingly retraining with a
> + * higher target link speed afterwards lets the two successfully negotiate
> + * 5.0GT/s.
> + *
> + * As this can potentially happen with any device and is cheap in the case
> + * of correctly operating hardware, let's do it for all downstream ports,
> + * for root complexes, PCIe switches and PCI/PCI-X to PCIe bridges.
> + *
> + * First check if automatic link training may have failed to complete, as
> + * indicated by the optional Data Link Layer Link Active status being off
> + * and the Link Bandwidth Management Status indicating that hardware has
> + * changed the link speed or width in an attempt to correct unreliable
> + * link operation.  If this is the case, then check if the link operates
> + * correctly by seeing whether it is being trained excessively.  If it is,
> + * then conclude the link is broken.
> + *
> + * In that case restrict the speed to 2.5GT/s, observing that the Target
> + * Link Speed field is sticky and therefore the link will stay restricted
> + * even after a device reset is later made by an OS that is unaware of the
> + * problem.  With the speed restricted request that the link be retrained
> + * and check again if the link operates correctly.  If not, then set the
> + * Target Link Speed back to the original value.
> + *
> + * This requires the presence of the Link Control 2 register, so make sure
> + * the PCI Express Capability Version is at least 2.  Also don't try, for
> + * obvious reasons, to limit the speed if 2.5GT/s is the only link speed
> + * supported.
> + */
> +static void dm_pciauto_exp_fixup_link(struct udevice *dev, int pcie_off)
> +{
> +	u16 exp_lnksta, exp_lnkctl, exp_lnkctl2;
> +	u16 exp_flags, exp_type, exp_version;
> +	u32 exp_lnkcap;
> +	pci_dev_t bdf;
> +
> +	dm_pci_read_config16(dev, pcie_off + PCI_EXP_FLAGS, &exp_flags);
> +	exp_version = exp_flags & PCI_EXP_FLAGS_VERS;
> +	if (exp_version < 2)
> +		return;
> +
> +	exp_type = (exp_flags & PCI_EXP_FLAGS_TYPE) >> 4;
> +	switch (exp_type) {
> +	case PCI_EXP_TYPE_ROOT_PORT:
> +	case PCI_EXP_TYPE_DOWNSTREAM:
> +	case PCI_EXP_TYPE_PCIE_BRIDGE:
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	dm_pci_read_config32(dev, pcie_off + PCI_EXP_LNKCAP, &exp_lnkcap);
> +	if ((exp_lnkcap & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB)
> +		return;
> +
> +	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA, &exp_lnksta);
> +	if ((exp_lnksta & (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_DLLLA)) !=
> +	    PCI_EXP_LNKSTA_LBMS)
> +		return;
> +
> +	if (dm_pciauto_exp_link_stable(dev, pcie_off))
> +		return;
> +
> +	bdf = dm_pci_get_bdf(dev);
> +	printf("PCI Autoconfig: %02x.%02x.%02x: "
> +	       "Downstream link non-functional\n",
> +	       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
> +	printf("PCI Autoconfig: %02x.%02x.%02x: "
> +	       "Retrying with speed restricted to 2.5GT/s...\n",
> +	       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
> +
> +	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKCTL, &exp_lnkctl);
> +	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKCTL2, &exp_lnkctl2);
> +
> +	dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
> +			      (exp_lnkctl2 & ~PCI_EXP_LNKCTL2_TLS) |
> +			      PCI_EXP_LNKCTL2_TLS_2_5GT);
> +	dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
> +			      exp_lnkctl | PCI_EXP_LNKCTL_RL);
> +
> +	if (dm_pciauto_exp_link_stable(dev, pcie_off)) {
> +		printf("PCI Autoconfig: %02x.%02x.%02x: Succeeded!\n",
> +		       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
> +	} else {
> +		printf("PCI Autoconfig: %02x.%02x.%02x: Failed!\n",
> +		       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
> +
> +		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
> +				      exp_lnkctl2);
> +		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
> +				      exp_lnkctl | PCI_EXP_LNKCTL_RL);
> +	}
> +}
> +
> +void pcie_asm2824_workaround(struct udevice *dev)
> +{
> +	int pcie_off;
> +
> +	/* For PCIe devices see if we need to retrain the link by hand */
> +	pcie_off = dm_pci_find_capability(dev, PCI_CAP_ID_EXP);
> +	if (pcie_off)
> +		dm_pciauto_exp_fixup_link(dev, pcie_off);
> +}

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH] pci: Do not enable PCIe ASMedia ASM2824 workaround by default

[Maciej W. Rozycki]

On Thu, 7 Apr 2022, Stefan Roese wrote:

> > Hello! What do you think about this change? I think it is good
> > compromise between enable this workaround for all builds on all boards
> > and enable it only based on device id. Or would it be better to restrict
> > this workaround just for ASM2824 device like the last iteration of
> > kernel patch?
> 
> I'm not sure if we should name this "workaround" ASM2824, even though
> it's currently (only) targeted exactly for this PCIe switch. It might
> be helpful for other PCIe switches as well. So perhaps it's better to
> give this function a more generic name instead? With this change, it
> makes perhaps also sense to keep this function in pci_auto.c but also
> rename the Kconfig option to some more generic version.

 By now I have become somewhat tired arguing and explaining matters over 
and over again as things have been moving as slow as molasses in this 
area, but one point I want to raise here is while it is indeed the ASM2824 
device that seems problematic, it may actually be downstream, so you won't 
know it's there until you go through the workaround, as observed with the 
root port of the SiFive FU740-C000 SOC (which has a separate workaround in 
U-boot, clearly for the same issue; cf. `pcie_sifive_force_gen1').  So it 
looks like the erratum is going to show up with some device combinations 
in which the device enumerator may not have a way to know an ASM2824 is 
there until the workaround applied to an upstream device has let the link 
work.

 And as I previously already mentioned the Linux version of the workaround 
is only activated by the vendor:device ID because you cannot busy-loop 
polling on the Link Training bit in Linux (while you can do it in U-Boot, 
because U-Boot is not an OS).  Arguably I could have broadened it to cover 
all Gen 3+ devices and poll on the Data Link Layer Link Active bit, which 
doesn't require busy-looping for meaningful results, but that would still 
leave Gen 2 devices out and chances are the system boots from U-Boot with 
the generic workaround applied and the link already negotiated at 2.5GT/s.

 NB the ASM2824 switch has been used with option cards as well, e.g. 
<https://www.amazon.com/dp/B07PRN2QCV>, so it can be there in any system 
that has a connector of any kind that lets one use PCIe option cards.

 FWIW,

  Maciej

Re: [PATCH] pci: Do not enable PCIe ASMedia ASM2824 workaround by default

[Pali Rohár]

On Friday 08 April 2022 00:18:48 Maciej W. Rozycki wrote:
> On Thu, 7 Apr 2022, Stefan Roese wrote:
> 
> > > Hello! What do you think about this change? I think it is good
> > > compromise between enable this workaround for all builds on all boards
> > > and enable it only based on device id. Or would it be better to restrict
> > > this workaround just for ASM2824 device like the last iteration of
> > > kernel patch?
> > 
> > I'm not sure if we should name this "workaround" ASM2824, even though
> > it's currently (only) targeted exactly for this PCIe switch. It might
> > be helpful for other PCIe switches as well. So perhaps it's better to
> > give this function a more generic name instead? With this change, it
> > makes perhaps also sense to keep this function in pci_auto.c but also
> > rename the Kconfig option to some more generic version.
> 
>  By now I have become somewhat tired arguing and explaining matters over 
> and over again as things have been moving as slow as molasses in this 
> area, but one point I want to raise here is while it is indeed the ASM2824 
> device that seems problematic, it may actually be downstream, so you won't 
> know it's there until you go through the workaround, as observed with the 
> root port of the SiFive FU740-C000 SOC (which has a separate workaround in 
> U-boot, clearly for the same issue; cf. `pcie_sifive_force_gen1').  So it 
> looks like the erratum is going to show up with some device combinations 
> in which the device enumerator may not have a way to know an ASM2824 is 
> there until the workaround applied to an upstream device has let the link 
> work.

I see that Linux patch was not not merged yet and there are already
comments that this issue is probably board or arch specific and maybe
should be in arch/riscv linux dir:
https://lore.kernel.org/linux-pci/20220421202711.GA1415244@bhelgaas/

From that comment I have feeling that the issue could be really specific
to board or combination of connected devices (ASM2824+PI7C9X2G304) as
there is really no other report about this issue.

In any case it is weird.

>  And as I previously already mentioned the Linux version of the workaround 
> is only activated by the vendor:device ID because you cannot busy-loop 
> polling on the Link Training bit in Linux (while you can do it in U-Boot, 
> because U-Boot is not an OS).

Is is not _only_ because of this. For a longer time there is a direction
to specify exact list of _affected_ hw which needs workaround. And not
usage of wildcard which match all hardware, even unaffected. See for
example comments which are adding workaround for broken GIC HW:
https://lore.kernel.org/lkml/87ilsutb6w.wl-maz@kernel.org/

And this makes sense, workarounds should be targeted.

> Arguably I could have broadened it to cover 
> all Gen 3+ devices and poll on the Data Link Layer Link Active bit, which 
> doesn't require busy-looping for meaningful results, but that would still 
> leave Gen 2 devices out and chances are the system boots from U-Boot with 
> the generic workaround applied and the link already negotiated at 2.5GT/s.
> 
>  NB the ASM2824 switch has been used with option cards as well, e.g. 
> <https://www.amazon.com/dp/B07PRN2QCV>, so it can be there in any system 
> that has a connector of any kind that lets one use PCIe option cards.
> 
>  FWIW,
> 
>   Maciej

Re: [PATCH] pci: Do not enable PCIe ASMedia ASM2824 workaround by default

[Maciej W. Rozycki]

On Sun, 1 May 2022, Pali Rohár wrote:

> >  By now I have become somewhat tired arguing and explaining matters over 
> > and over again as things have been moving as slow as molasses in this 
> > area, but one point I want to raise here is while it is indeed the ASM2824 
> > device that seems problematic, it may actually be downstream, so you won't 
> > know it's there until you go through the workaround, as observed with the 
> > root port of the SiFive FU740-C000 SOC (which has a separate workaround in 
> > U-boot, clearly for the same issue; cf. `pcie_sifive_force_gen1').  So it 
> > looks like the erratum is going to show up with some device combinations 
> > in which the device enumerator may not have a way to know an ASM2824 is 
> > there until the workaround applied to an upstream device has let the link 
> > work.
> 
> I see that Linux patch was not not merged yet and there are already
> comments that this issue is probably board or arch specific and maybe
> should be in arch/riscv linux dir:
> https://lore.kernel.org/linux-pci/20220421202711.GA1415244@bhelgaas/

 Maybe, maybe not, and given the lack of cooperation shown by ASMedia I am 
not going to spend any money on their hardware to get this figured out, 
such as <https://www.amazon.com/dp/B07PRN2QCV> previously mentioned (plus 
the necessary M.2 to PCIe slot adapters).

 Main concerns with my change so far appear to be some coding style issues 
which are due to my lack of following of the development of newer internal 
interfaces.  That'll be fixed with the next iteration when I get to it 
(hopefully this coming weekend).  I have a lot of stuff in progress (and a 
job and personal life too) and need to prioritise.

> >From that comment I have feeling that the issue could be really specific
> to board or combination of connected devices (ASM2824+PI7C9X2G304) as
> there is really no other report about this issue.
> 
> In any case it is weird.

 As I say, from the findings so far this seems to me like a protocol 
rather than signalling (electrical) issue, i.e. a design problem with 
either or both chips.  A board design fault (such as improper decoupling, 
crosstalk, or whatever) could only result in a signalling issue.  And if 
it was a signalling issue, then the 5GT/s rate couldn't be negotiated, 
whether automatically or by hand as with this workaround, and then work 
reliably over days to weeks.

> >  And as I previously already mentioned the Linux version of the workaround 
> > is only activated by the vendor:device ID because you cannot busy-loop 
> > polling on the Link Training bit in Linux (while you can do it in U-Boot, 
> > because U-Boot is not an OS).
> 
> Is is not _only_ because of this. For a longer time there is a direction

 It is exactly and only for this.  I think I know why I wrote it this way.

> to specify exact list of _affected_ hw which needs workaround. And not
> usage of wildcard which match all hardware, even unaffected. See for
> example comments which are adding workaround for broken GIC HW:
> https://lore.kernel.org/lkml/87ilsutb6w.wl-maz@kernel.org/
> 
> And this makes sense, workarounds should be targeted.

 Your observation only makes sense for a problem contained within a device 
itself and not for a problem with establishing a link between two devices.

 If the problem is caused by a downstream device, then you cannot match on 
it to activate a workaround because you won't know it's there until you 
have activated the workaround.  It hasn't been determined here whether the 
problem is the upstream device, the downstream device or a combination of 
the two.

 And I don't think it is appropriate to put the burden onto the users to 
determine what the case is, by deliberately releasing a limited fix in a 
hope that something will break for someone and they will report it.  It is 
an engineer's responsibility to ensure the quality of the solution made is 
of the highest quality feasible.  If we knew what the exact actual cause 
is, then we could keep the workaround limited to the cases identified by 
the cause.

 Also FWIW the next iteration of my Linux change will no longer match the 
quirk on a device ID and will instead use the PCI bridge class ID with a 
further qualification within.  I had a concern about the safety of the 
change, but I have got it mentally resolved now and only the potentially 
unsafe part will be further qualified by a device ID.

 Sadly I still need to limit the workaround to data link layer link active 
reporting capable devices, which will undoubtedly exclude some up to 5GT/s 
only hardware, due to the need to avoid busy-looping on the Link Training 
bit, which is a no-no in an OS.  So the U-Boot code will remain the only 
workaround for some configurations.

  Maciej

Re: [PATCH] pci: Do not enable PCIe ASMedia ASM2824 workaround by default

[Maciej W. Rozycki]

On Thu, 5 May 2022, Maciej W. Rozycki wrote:

> > I see that Linux patch was not not merged yet and there are already
> > comments that this issue is probably board or arch specific and maybe
> > should be in arch/riscv linux dir:
> > https://lore.kernel.org/linux-pci/20220421202711.GA1415244@bhelgaas/
> 
>  Maybe, maybe not, and given the lack of cooperation shown by ASMedia I am 
> not going to spend any money on their hardware to get this figured out, 
> such as <https://www.amazon.com/dp/B07PRN2QCV> previously mentioned (plus 
> the necessary M.2 to PCIe slot adapters).
> 
>  Main concerns with my change so far appear to be some coding style issues 
> which are due to my lack of following of the development of newer internal 
> interfaces.  That'll be fixed with the next iteration when I get to it 
> (hopefully this coming weekend).  I have a lot of stuff in progress (and a 
> job and personal life too) and need to prioritise.

 FYI it didn't happen as I got distracted with an emergency PSU repair in 
another system where I discovered leaks from electrolytic capacitors that 
posed a risk of damaging other components or the PCB even if powered off, 
so faulty parts had to be replaced as a matter of urgency.

 And then I've had a need to put the Unmatched board in GCC verification, 
which it has been running ever since, certainly longer than expected.  It 
means I cannot take it offline to do Linux kernel verification for the 
foreseeable future.

 Therefore I have no new ETA at the moment, but as always I'll do my best.

  Maciej

[PATCH v2] pci: Do not enable PCIe GEN3 link retrain workaround by default

[Pali Rohár]

PCIe GEN3 link retrain workaround, specially designed for system with PCIe
ASMedia ASM2824 Switch and other Endpoint devices, unconditionally increase
size of all SPL binaries with PCIe support, even those which do not require it.

Moreover this workaround is enabled for all existing hardware and also all
future PCIe hardware, which opens a hole that other PCIe vendors may
introduce same HW issue as on systems where this workaround is required and
nobody would notice it because U-Boot automatically apply workaround for it.

So do not unconditionally enable this workaround on all hardware. Instead
introduce a new config option which is disabled by default. This decrease
SPL size and also ensure that workaround is not blindly or inexactly
enabled.

To make workaround code local, move it into separate file, so pci_auto.c
contains only generic auto configuration code.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
Changes in v2:
* Rename config option to CONFIG_PCIE_GEN3_RETRAIN_WORKAROUND as it is
  generic workaround and does not have to be needed only for ASMedia switch
  (moreover it looks like issue is triggered by combination of ASMedia
   switch with other endpoint PCIe device)
* Rephrase Kconfig help text as suggected by Stefan
---
 drivers/pci/Kconfig                        |   9 +
 drivers/pci/Makefile                       |   1 +
 drivers/pci/pci_auto.c                     | 171 +------------------
 drivers/pci/pcie_gen3_retrain_workaround.c | 183 +++++++++++++++++++++
 4 files changed, 197 insertions(+), 167 deletions(-)
 create mode 100644 drivers/pci/pcie_gen3_retrain_workaround.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 22f4995453ed..f56c8e89141c 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -365,4 +365,13 @@ config PCIE_UNIPHIER
 	  Say Y here if you want to enable PCIe controller support on
 	  UniPhier SoCs.
 
+config PCIE_GEN3_RETRAIN_WORKAROUND
+	bool "PCIe GEN3 retrain workaround"
+	help
+	  Say Y here if you want to enable link re-training upon failures
+	  for all PCIe Root Ports, PCIe Switch Downstream Ports and also
+	  for PCIe to PCI/PCI-X Bridges. This seems to be helpful on the
+	  ASMedia ASM2824 PCIe Switch.
+	  Compliant PCIe hierarchy does not need this workaround.
+
 endif
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index cfcd6fd6c52f..57587205d198 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -49,3 +49,4 @@ obj-$(CONFIG_PCI_OCTEONTX) += pci_octeontx.o
 obj-$(CONFIG_PCIE_OCTEON) += pcie_octeon.o
 obj-$(CONFIG_PCIE_DW_SIFIVE) += pcie_dw_sifive.o
 obj-$(CONFIG_PCIE_UNIPHIER) += pcie_uniphier.o
+obj-$(CONFIG_PCIE_GEN3_RETRAIN_WORKAROUND) += pcie_gen3_retrain_workaround.o
diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
index c7968926a17f..4583ff8b384e 100644
--- a/drivers/pci/pci_auto.c
+++ b/drivers/pci/pci_auto.c
@@ -13,7 +13,6 @@
 #include <errno.h>
 #include <log.h>
 #include <pci.h>
-#include <time.h>
 #include "pci_internal.h"
 
 /* the user can define CONFIG_SYS_PCI_CACHE_LINE_SIZE to avoid problems */
@@ -182,167 +181,7 @@ static void dm_pciauto_setup_device(struct udevice *dev,
 	dm_pci_write_config8(dev, PCI_LATENCY_TIMER, 0x80);
 }
 
-/*
- * Check if the link of a downstream PCIe port operates correctly.
- *
- * For that check if the optional Data Link Layer Link Active status gets
- * on within a 200ms period or failing that wait until the completion of
- * that period and check if link training has shown the completed status
- * continuously throughout the second half of that period.
- *
- * Observation with the ASMedia ASM2824 Gen 3 switch indicates it takes
- * 11-44ms to indicate the Data Link Layer Link Active status at 2.5GT/s,
- * though it may take a couple of link training iterations.
- */
-static bool dm_pciauto_exp_link_stable(struct udevice *dev, int pcie_off)
-{
-	u64 loops = 0, trcount = 0, ntrcount = 0, flips = 0;
-	bool dllla, lnktr, plnktr;
-	u16 exp_lnksta;
-	pci_dev_t bdf;
-	u64 end;
-
-	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA, &exp_lnksta);
-	plnktr = !!(exp_lnksta & PCI_EXP_LNKSTA_LT);
-
-	end = get_ticks() + usec_to_tick(200000);
-	do {
-		dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA,
-				     &exp_lnksta);
-		dllla = !!(exp_lnksta & PCI_EXP_LNKSTA_DLLLA);
-		lnktr = !!(exp_lnksta & PCI_EXP_LNKSTA_LT);
-
-		flips += plnktr ^ lnktr;
-		if (lnktr) {
-			ntrcount = 0;
-			trcount++;
-		} else {
-			ntrcount++;
-		}
-		loops++;
-
-		plnktr = lnktr;
-	} while (!dllla && get_ticks() < end);
-
-	bdf = dm_pci_get_bdf(dev);
-	debug("PCI Autoconfig: %02x.%02x.%02x: Fixup link: DL active: %u; "
-	      "%3llu flips, %6llu loops of which %6llu while training, "
-	      "final %6llu stable\n",
-	      PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf),
-	      (unsigned int)dllla,
-	      (unsigned long long)flips, (unsigned long long)loops,
-	      (unsigned long long)trcount, (unsigned long long)ntrcount);
-
-	return dllla || ntrcount >= loops / 2;
-}
-
-/*
- * Retrain the link of a downstream PCIe port by hand if necessary.
- *
- * This is needed at least where a downstream port of the ASMedia ASM2824
- * Gen 3 switch is wired to the upstream port of the Pericom PI7C9X2G304
- * Gen 2 switch, and observed with the Delock Riser Card PCI Express x1 >
- * 2 x PCIe x1 device, P/N 41433, plugged into the SiFive HiFive Unmatched
- * board.
- *
- * In such a configuration the switches are supposed to negotiate the link
- * speed of preferably 5.0GT/s, falling back to 2.5GT/s.  However the link
- * continues switching between the two speeds indefinitely and the data
- * link layer never reaches the active state, with link training reported
- * repeatedly active ~84% of the time.  Forcing the target link speed to
- * 2.5GT/s with the upstream ASM2824 device makes the two switches talk to
- * each other correctly however.  And more interestingly retraining with a
- * higher target link speed afterwards lets the two successfully negotiate
- * 5.0GT/s.
- *
- * As this can potentially happen with any device and is cheap in the case
- * of correctly operating hardware, let's do it for all downstream ports,
- * for root complexes, PCIe switches and PCI/PCI-X to PCIe bridges.
- *
- * First check if automatic link training may have failed to complete, as
- * indicated by the optional Data Link Layer Link Active status being off
- * and the Link Bandwidth Management Status indicating that hardware has
- * changed the link speed or width in an attempt to correct unreliable
- * link operation.  If this is the case, then check if the link operates
- * correctly by seeing whether it is being trained excessively.  If it is,
- * then conclude the link is broken.
- *
- * In that case restrict the speed to 2.5GT/s, observing that the Target
- * Link Speed field is sticky and therefore the link will stay restricted
- * even after a device reset is later made by an OS that is unaware of the
- * problem.  With the speed restricted request that the link be retrained
- * and check again if the link operates correctly.  If not, then set the
- * Target Link Speed back to the original value.
- *
- * This requires the presence of the Link Control 2 register, so make sure
- * the PCI Express Capability Version is at least 2.  Also don't try, for
- * obvious reasons, to limit the speed if 2.5GT/s is the only link speed
- * supported.
- */
-static void dm_pciauto_exp_fixup_link(struct udevice *dev, int pcie_off)
-{
-	u16 exp_lnksta, exp_lnkctl, exp_lnkctl2;
-	u16 exp_flags, exp_type, exp_version;
-	u32 exp_lnkcap;
-	pci_dev_t bdf;
-
-	dm_pci_read_config16(dev, pcie_off + PCI_EXP_FLAGS, &exp_flags);
-	exp_version = exp_flags & PCI_EXP_FLAGS_VERS;
-	if (exp_version < 2)
-		return;
-
-	exp_type = (exp_flags & PCI_EXP_FLAGS_TYPE) >> 4;
-	switch (exp_type) {
-	case PCI_EXP_TYPE_ROOT_PORT:
-	case PCI_EXP_TYPE_DOWNSTREAM:
-	case PCI_EXP_TYPE_PCIE_BRIDGE:
-		break;
-	default:
-		return;
-	}
-
-	dm_pci_read_config32(dev, pcie_off + PCI_EXP_LNKCAP, &exp_lnkcap);
-	if ((exp_lnkcap & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB)
-		return;
-
-	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA, &exp_lnksta);
-	if ((exp_lnksta & (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_DLLLA)) !=
-	    PCI_EXP_LNKSTA_LBMS)
-		return;
-
-	if (dm_pciauto_exp_link_stable(dev, pcie_off))
-		return;
-
-	bdf = dm_pci_get_bdf(dev);
-	printf("PCI Autoconfig: %02x.%02x.%02x: "
-	       "Downstream link non-functional\n",
-	       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
-	printf("PCI Autoconfig: %02x.%02x.%02x: "
-	       "Retrying with speed restricted to 2.5GT/s...\n",
-	       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
-
-	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKCTL, &exp_lnkctl);
-	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKCTL2, &exp_lnkctl2);
-
-	dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
-			      (exp_lnkctl2 & ~PCI_EXP_LNKCTL2_TLS) |
-			      PCI_EXP_LNKCTL2_TLS_2_5GT);
-	dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
-			      exp_lnkctl | PCI_EXP_LNKCTL_RL);
-
-	if (dm_pciauto_exp_link_stable(dev, pcie_off)) {
-		printf("PCI Autoconfig: %02x.%02x.%02x: Succeeded!\n",
-		       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
-	} else {
-		printf("PCI Autoconfig: %02x.%02x.%02x: Failed!\n",
-		       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
-
-		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
-				      exp_lnkctl2);
-		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
-				      exp_lnkctl | PCI_EXP_LNKCTL_RL);
-	}
-}
+void pcie_gen3_retrain_workaround(struct udevice *dev);
 
 void dm_pciauto_prescan_setup_bridge(struct udevice *dev, int sub_bus)
 {
@@ -353,7 +192,6 @@ void dm_pciauto_prescan_setup_bridge(struct udevice *dev, int sub_bus)
 	u8 io_32;
 	struct udevice *ctlr = pci_get_controller(dev);
 	struct pci_controller *ctlr_hose = dev_get_uclass_priv(ctlr);
-	int pcie_off;
 
 	pci_mem = ctlr_hose->pci_mem;
 	pci_prefetch = ctlr_hose->pci_prefetch;
@@ -440,10 +278,9 @@ void dm_pciauto_prescan_setup_bridge(struct udevice *dev, int sub_bus)
 		}
 	}
 
-	/* For PCIe devices see if we need to retrain the link by hand */
-	pcie_off = dm_pci_find_capability(dev, PCI_CAP_ID_EXP);
-	if (pcie_off)
-		dm_pciauto_exp_fixup_link(dev, pcie_off);
+#ifdef CONFIG_PCIE_GEN3_RETRAIN_WORKAROUND
+	pcie_gen3_retrain_workaround(dev);
+#endif
 
 	/* Enable memory and I/O accesses, enable bus master */
 	dm_pci_write_config16(dev, PCI_COMMAND, cmdstat | PCI_COMMAND_MASTER);
diff --git a/drivers/pci/pcie_gen3_retrain_workaround.c b/drivers/pci/pcie_gen3_retrain_workaround.c
new file mode 100644
index 000000000000..e4fe3368652d
--- /dev/null
+++ b/drivers/pci/pcie_gen3_retrain_workaround.c
@@ -0,0 +1,183 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * PCIe GEN3 retrain workaround
+ *
+ * Copyright (c) 2021  Maciej W. Rozycki <macro@orcam.me.uk>
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <pci.h>
+#include <time.h>
+
+/*
+ * Check if the link of a downstream PCIe port operates correctly.
+ *
+ * For that check if the optional Data Link Layer Link Active status gets
+ * on within a 200ms period or failing that wait until the completion of
+ * that period and check if link training has shown the completed status
+ * continuously throughout the second half of that period.
+ *
+ * Observation with the ASMedia ASM2824 Gen 3 switch indicates it takes
+ * 11-44ms to indicate the Data Link Layer Link Active status at 2.5GT/s,
+ * though it may take a couple of link training iterations.
+ */
+static bool dm_pciauto_exp_link_stable(struct udevice *dev, int pcie_off)
+{
+	u64 loops = 0, trcount = 0, ntrcount = 0, flips = 0;
+	bool dllla, lnktr, plnktr;
+	u16 exp_lnksta;
+	pci_dev_t bdf;
+	u64 end;
+
+	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA, &exp_lnksta);
+	plnktr = !!(exp_lnksta & PCI_EXP_LNKSTA_LT);
+
+	end = get_ticks() + usec_to_tick(200000);
+	do {
+		dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA,
+				     &exp_lnksta);
+		dllla = !!(exp_lnksta & PCI_EXP_LNKSTA_DLLLA);
+		lnktr = !!(exp_lnksta & PCI_EXP_LNKSTA_LT);
+
+		flips += plnktr ^ lnktr;
+		if (lnktr) {
+			ntrcount = 0;
+			trcount++;
+		} else {
+			ntrcount++;
+		}
+		loops++;
+
+		plnktr = lnktr;
+	} while (!dllla && get_ticks() < end);
+
+	bdf = dm_pci_get_bdf(dev);
+	debug("PCI Autoconfig: %02x.%02x.%02x: Fixup link: DL active: %u; "
+	      "%3llu flips, %6llu loops of which %6llu while training, "
+	      "final %6llu stable\n",
+	      PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf),
+	      (unsigned int)dllla,
+	      (unsigned long long)flips, (unsigned long long)loops,
+	      (unsigned long long)trcount, (unsigned long long)ntrcount);
+
+	return dllla || ntrcount >= loops / 2;
+}
+
+/*
+ * Retrain the link of a downstream PCIe port by hand if necessary.
+ *
+ * This is needed at least where a downstream port of the ASMedia ASM2824
+ * Gen 3 switch is wired to the upstream port of the Pericom PI7C9X2G304
+ * Gen 2 switch, and observed with the Delock Riser Card PCI Express x1 >
+ * 2 x PCIe x1 device, P/N 41433, plugged into the SiFive HiFive Unmatched
+ * board.
+ *
+ * In such a configuration the switches are supposed to negotiate the link
+ * speed of preferably 5.0GT/s, falling back to 2.5GT/s.  However the link
+ * continues switching between the two speeds indefinitely and the data
+ * link layer never reaches the active state, with link training reported
+ * repeatedly active ~84% of the time.  Forcing the target link speed to
+ * 2.5GT/s with the upstream ASM2824 device makes the two switches talk to
+ * each other correctly however.  And more interestingly retraining with a
+ * higher target link speed afterwards lets the two successfully negotiate
+ * 5.0GT/s.
+ *
+ * As this can potentially happen with any device and is cheap in the case
+ * of correctly operating hardware, let's do it for all downstream ports,
+ * for root complexes, PCIe switches and PCI/PCI-X to PCIe bridges.
+ *
+ * First check if automatic link training may have failed to complete, as
+ * indicated by the optional Data Link Layer Link Active status being off
+ * and the Link Bandwidth Management Status indicating that hardware has
+ * changed the link speed or width in an attempt to correct unreliable
+ * link operation.  If this is the case, then check if the link operates
+ * correctly by seeing whether it is being trained excessively.  If it is,
+ * then conclude the link is broken.
+ *
+ * In that case restrict the speed to 2.5GT/s, observing that the Target
+ * Link Speed field is sticky and therefore the link will stay restricted
+ * even after a device reset is later made by an OS that is unaware of the
+ * problem.  With the speed restricted request that the link be retrained
+ * and check again if the link operates correctly.  If not, then set the
+ * Target Link Speed back to the original value.
+ *
+ * This requires the presence of the Link Control 2 register, so make sure
+ * the PCI Express Capability Version is at least 2.  Also don't try, for
+ * obvious reasons, to limit the speed if 2.5GT/s is the only link speed
+ * supported.
+ */
+static void dm_pciauto_exp_fixup_link(struct udevice *dev, int pcie_off)
+{
+	u16 exp_lnksta, exp_lnkctl, exp_lnkctl2;
+	u16 exp_flags, exp_type, exp_version;
+	u32 exp_lnkcap;
+	pci_dev_t bdf;
+
+	dm_pci_read_config16(dev, pcie_off + PCI_EXP_FLAGS, &exp_flags);
+	exp_version = exp_flags & PCI_EXP_FLAGS_VERS;
+	if (exp_version < 2)
+		return;
+
+	exp_type = (exp_flags & PCI_EXP_FLAGS_TYPE) >> 4;
+	switch (exp_type) {
+	case PCI_EXP_TYPE_ROOT_PORT:
+	case PCI_EXP_TYPE_DOWNSTREAM:
+	case PCI_EXP_TYPE_PCIE_BRIDGE:
+		break;
+	default:
+		return;
+	}
+
+	dm_pci_read_config32(dev, pcie_off + PCI_EXP_LNKCAP, &exp_lnkcap);
+	if ((exp_lnkcap & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB)
+		return;
+
+	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKSTA, &exp_lnksta);
+	if ((exp_lnksta & (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_DLLLA)) !=
+	    PCI_EXP_LNKSTA_LBMS)
+		return;
+
+	if (dm_pciauto_exp_link_stable(dev, pcie_off))
+		return;
+
+	bdf = dm_pci_get_bdf(dev);
+	printf("PCI Autoconfig: %02x.%02x.%02x: "
+	       "Downstream link non-functional\n",
+	       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
+	printf("PCI Autoconfig: %02x.%02x.%02x: "
+	       "Retrying with speed restricted to 2.5GT/s...\n",
+	       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
+
+	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKCTL, &exp_lnkctl);
+	dm_pci_read_config16(dev, pcie_off + PCI_EXP_LNKCTL2, &exp_lnkctl2);
+
+	dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
+			      (exp_lnkctl2 & ~PCI_EXP_LNKCTL2_TLS) |
+			      PCI_EXP_LNKCTL2_TLS_2_5GT);
+	dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
+			      exp_lnkctl | PCI_EXP_LNKCTL_RL);
+
+	if (dm_pciauto_exp_link_stable(dev, pcie_off)) {
+		printf("PCI Autoconfig: %02x.%02x.%02x: Succeeded!\n",
+		       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
+	} else {
+		printf("PCI Autoconfig: %02x.%02x.%02x: Failed!\n",
+		       PCI_BUS(bdf), PCI_DEV(bdf), PCI_FUNC(bdf));
+
+		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL2,
+				      exp_lnkctl2);
+		dm_pci_write_config16(dev, pcie_off + PCI_EXP_LNKCTL,
+				      exp_lnkctl | PCI_EXP_LNKCTL_RL);
+	}
+}
+
+void pcie_gen3_retrain_workaround(struct udevice *dev)
+{
+	int pcie_off;
+
+	/* For PCIe devices see if we need to retrain the link by hand */
+	pcie_off = dm_pci_find_capability(dev, PCI_CAP_ID_EXP);
+	if (pcie_off)
+		dm_pciauto_exp_fixup_link(dev, pcie_off);
+}
-- 
2.20.1

Re: [PATCH v2] pci: Do not enable PCIe GEN3 link retrain workaround by default

[Simon Glass]

On Sat, 27 Aug 2022 at 06:30, Pali Rohár <pali@kernel.org> wrote:
>
> PCIe GEN3 link retrain workaround, specially designed for system with PCIe
> ASMedia ASM2824 Switch and other Endpoint devices, unconditionally increase
> size of all SPL binaries with PCIe support, even those which do not require it.
>
> Moreover this workaround is enabled for all existing hardware and also all
> future PCIe hardware, which opens a hole that other PCIe vendors may
> introduce same HW issue as on systems where this workaround is required and
> nobody would notice it because U-Boot automatically apply workaround for it.
>
> So do not unconditionally enable this workaround on all hardware. Instead
> introduce a new config option which is disabled by default. This decrease
> SPL size and also ensure that workaround is not blindly or inexactly
> enabled.
>
> To make workaround code local, move it into separate file, so pci_auto.c
> contains only generic auto configuration code.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> Changes in v2:
> * Rename config option to CONFIG_PCIE_GEN3_RETRAIN_WORKAROUND as it is
>   generic workaround and does not have to be needed only for ASMedia switch
>   (moreover it looks like issue is triggered by combination of ASMedia
>    switch with other endpoint PCIe device)
> * Rephrase Kconfig help text as suggected by Stefan
> ---
>  drivers/pci/Kconfig                        |   9 +
>  drivers/pci/Makefile                       |   1 +
>  drivers/pci/pci_auto.c                     | 171 +------------------
>  drivers/pci/pcie_gen3_retrain_workaround.c | 183 +++++++++++++++++++++
>  4 files changed, 197 insertions(+), 167 deletions(-)
>  create mode 100644 drivers/pci/pcie_gen3_retrain_workaround.c

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2] pci: Do not enable PCIe GEN3 link retrain workaround by default

[Maciej W. Rozycki]

On Sat, 27 Aug 2022, Pali Rohár wrote:

> Moreover this workaround is enabled for all existing hardware and also all
> future PCIe hardware, which opens a hole that other PCIe vendors may
> introduce same HW issue as on systems where this workaround is required and
> nobody would notice it because U-Boot automatically apply workaround for it.

 Why is it a problem?  Is the intent to cause hassle to end users and 
force them to take action when they have a non-working piece of hardware?  

 I'd say in 99% of cases this will only cause frustration and they won't 
bother.  They will just conclude that either piece of hardware involved is 
broken and will throw it away.  Just as I almost did.  The seller has 
offered me a refund, which seems thought to be a universal solution 
nowadays (but I need to do what I meant to and getting money back doesn't 
solve it).

 And at least I know what U-boot (or indeed firmware) is and have a 
general understanding of how computers work.  Most people just want to 
plug stuff in and use it for whatever their need is.  Expecting them to 
take action to get things working is wasting their time (which BTW seems 
to have been a growing trend in last ~30 years: putting burden on the end 
user to get our problems solved, which saves our time and money at the 
expense of end user's).

 NB I'm slowly getting fed up with the amount of non-working stuff piling 
up around.  Every other piece of equipment I try doesn't work for one 
reason or another and I need to either chase bugs myself or to spend days 
and weeks to persuade someone at least to believe a problem is there to 
get that sorted.  All in my free time I'd rather spend on something else.  
I'd welcome things working automagically for a change so that I could 
focus on what I mean to be doing, and therefore I take breaking things 
deliberately as a major offence.

 FWIW,

  Maciej

Re: [PATCH v2] pci: Do not enable PCIe GEN3 link retrain workaround by default

[Pali Rohár]

On Tuesday 30 August 2022 10:04:51 Maciej W. Rozycki wrote:
> On Sat, 27 Aug 2022, Pali Rohár wrote:
> 
> > Moreover this workaround is enabled for all existing hardware and also all
> > future PCIe hardware, which opens a hole that other PCIe vendors may
> > introduce same HW issue as on systems where this workaround is required and
> > nobody would notice it because U-Boot automatically apply workaround for it.
> 
>  Why is it a problem?

I think I wrote it. One issue is that it is increasing size of SPL image
and we really should not include into SPL things which are not required
for all target platforms. Lot of boards have size constrained memory
requirements and unnecessary features should not be automatically
enabled.

> Is the intent to cause hassle to end users and 
> force them to take action when they have a non-working piece of hardware?  

Vendors should really test their hardware without any automatic
workaround. Otherwise we are going into the hell if some workarounds are
automatically enabled and nobody notice broken behavior. And vendors
really build software with default options and do not care about it if
default options are suitable.

There is already direction to make all workaround targeted and enabled
only for platforms / hardware which really need it.

So enabling some workaround for all platforms which are produced and
will be produced in the future is step backward.

>  I'd say in 99% of cases this will only cause frustration and they won't 
> bother.  They will just conclude that either piece of hardware involved is 
> broken and will throw it away.  Just as I almost did.  The seller has 
> offered me a refund, which seems thought to be a universal solution 
> nowadays (but I need to do what I meant to and getting money back doesn't 
> solve it).
> 
>  And at least I know what U-boot (or indeed firmware) is and have a 
> general understanding of how computers work.  Most people just want to 
> plug stuff in and use it for whatever their need is.  Expecting them to 
> take action to get things working is wasting their time (which BTW seems 
> to have been a growing trend in last ~30 years: putting burden on the end 
> user to get our problems solved, which saves our time and money at the 
> expense of end user's).

The another issue here is that it was not fully investigated where the
issue is. If it is processor specific, PCIe switch specific or endpoint
specific, or combination of these options. It was just observed that
proposed workaround fix this issue on one specific combination. And you
confirmed this in previous post, that you are unsure if it is not
specific to downstream ports of the switch.

We really should not include such "bloatware" code to be enabled for
everything, on every one board. You are the first who reported this
issue. Nobody else complained about it, so I really do not see reason
why all other users and developers must be forced to have it in their
U-Boot binaries. Moreover lot of boards on which is U-Boot running do
not have PCIe bus exported on the slot and have PCIe devices integrated
and soldered on the board. Why on the earth I have to need this
workaround also on these boards (which are moreover sized constrained)?

>  NB I'm slowly getting fed up with the amount of non-working stuff piling 
> up around.  Every other piece of equipment I try doesn't work for one 
> reason or another and I need to either chase bugs myself or to spend days 
> and weeks to persuade someone at least to believe a problem is there to 
> get that sorted.  All in my free time I'd rather spend on something else.  
> I'd welcome things working automagically for a change so that I could 
> focus on what I mean to be doing, and therefore I take breaking things 
> deliberately as a major offence.
> 
>  FWIW,
> 
>   Maciej

What other U-Boot developers think?

Re: [PATCH v2] pci: Do not enable PCIe GEN3 link retrain workaround by default

[Stefan Roese]

On 30.08.22 11:19, Pali Rohár wrote:
> On Tuesday 30 August 2022 10:04:51 Maciej W. Rozycki wrote:
>> On Sat, 27 Aug 2022, Pali Rohár wrote:
>>
>>> Moreover this workaround is enabled for all existing hardware and also all
>>> future PCIe hardware, which opens a hole that other PCIe vendors may
>>> introduce same HW issue as on systems where this workaround is required and
>>> nobody would notice it because U-Boot automatically apply workaround for it.
>>
>>   Why is it a problem?
> 
> I think I wrote it. One issue is that it is increasing size of SPL image
> and we really should not include into SPL things which are not required
> for all target platforms. Lot of boards have size constrained memory
> requirements and unnecessary features should not be automatically
> enabled.

I have to agree with Pali here. We need to be careful with size increase
in the SPL images, as some of the build targets are very limited here.
So making this workaround configurable is definitely a good idea.

The question remains, at least for me, if the Kconfig option should be
enable per default or not. For SPL my suggestions is to disable is per
default because of the size remarks above. For U-Boot proper I'm not so
sure. Please see below...

>> Is the intent to cause hassle to end users and
>> force them to take action when they have a non-working piece of hardware?
> 
> Vendors should really test their hardware without any automatic
> workaround. Otherwise we are going into the hell if some workarounds are
> automatically enabled and nobody notice broken behavior. And vendors
> really build software with default options and do not care about it if
> default options are suitable.
> 
> There is already direction to make all workaround targeted and enabled
> only for platforms / hardware which really need it.
> 
> So enabling some workaround for all platforms which are produced and
> will be produced in the future is step backward.

All this makes sense to me. You both have good points AFAICT. Please
see below...

>>   I'd say in 99% of cases this will only cause frustration and they won't
>> bother.  They will just conclude that either piece of hardware involved is
>> broken and will throw it away.  Just as I almost did.  The seller has
>> offered me a refund, which seems thought to be a universal solution
>> nowadays (but I need to do what I meant to and getting money back doesn't
>> solve it).
>>
>>   And at least I know what U-boot (or indeed firmware) is and have a
>> general understanding of how computers work.  Most people just want to
>> plug stuff in and use it for whatever their need is.  Expecting them to
>> take action to get things working is wasting their time (which BTW seems
>> to have been a growing trend in last ~30 years: putting burden on the end
>> user to get our problems solved, which saves our time and money at the
>> expense of end user's).
> 
> The another issue here is that it was not fully investigated where the
> issue is. If it is processor specific, PCIe switch specific or endpoint
> specific, or combination of these options. It was just observed that
> proposed workaround fix this issue on one specific combination. And you
> confirmed this in previous post, that you are unsure if it is not
> specific to downstream ports of the switch.
> 
> We really should not include such "bloatware" code to be enabled for
> everything, on every one board. You are the first who reported this
> issue. Nobody else complained about it, so I really do not see reason
> why all other users and developers must be forced to have it in their
> U-Boot binaries. Moreover lot of boards on which is U-Boot running do
> not have PCIe bus exported on the slot and have PCIe devices integrated
> and soldered on the board. Why on the earth I have to need this
> workaround also on these boards (which are moreover sized constrained)?

Agreed. But I also understand the reasoning from Maciej, at least in
parts. Thinking a bit more about this, my preference would be to still
include this workaround per default in U-Boot proper though. To not
make things too complicated here.

Just my 0.02$.

>>   NB I'm slowly getting fed up with the amount of non-working stuff piling
>> up around.  Every other piece of equipment I try doesn't work for one
>> reason or another and I need to either chase bugs myself or to spend days
>> and weeks to persuade someone at least to believe a problem is there to
>> get that sorted.  All in my free time I'd rather spend on something else.
>> I'd welcome things working automagically for a change so that I could
>> focus on what I mean to be doing, and therefore I take breaking things
>> deliberately as a major offence.
>>
>>   FWIW,
>>
>>    Maciej
> 
> What other U-Boot developers think?

Please see above.

Thanks,
Stefan

Re: [PATCH v2] pci: Do not enable PCIe GEN3 link retrain workaround by default

[Pali Rohár]

On Tuesday 30 August 2022 13:15:26 Stefan Roese wrote:
> On 30.08.22 11:19, Pali Rohár wrote:
> > On Tuesday 30 August 2022 10:04:51 Maciej W. Rozycki wrote:
> > > On Sat, 27 Aug 2022, Pali Rohár wrote:
> > > 
> > > > Moreover this workaround is enabled for all existing hardware and also all
> > > > future PCIe hardware, which opens a hole that other PCIe vendors may
> > > > introduce same HW issue as on systems where this workaround is required and
> > > > nobody would notice it because U-Boot automatically apply workaround for it.
> > > 
> > >   Why is it a problem?
> > 
> > I think I wrote it. One issue is that it is increasing size of SPL image
> > and we really should not include into SPL things which are not required
> > for all target platforms. Lot of boards have size constrained memory
> > requirements and unnecessary features should not be automatically
> > enabled.
> 
> I have to agree with Pali here. We need to be careful with size increase
> in the SPL images, as some of the build targets are very limited here.
> So making this workaround configurable is definitely a good idea.
> 
> The question remains, at least for me, if the Kconfig option should be
> enable per default or not. For SPL my suggestions is to disable is per
> default because of the size remarks above. For U-Boot proper I'm not so
> sure. Please see below...
> 
> > > Is the intent to cause hassle to end users and
> > > force them to take action when they have a non-working piece of hardware?
> > 
> > Vendors should really test their hardware without any automatic
> > workaround. Otherwise we are going into the hell if some workarounds are
> > automatically enabled and nobody notice broken behavior. And vendors
> > really build software with default options and do not care about it if
> > default options are suitable.
> > 
> > There is already direction to make all workaround targeted and enabled
> > only for platforms / hardware which really need it.
> > 
> > So enabling some workaround for all platforms which are produced and
> > will be produced in the future is step backward.
> 
> All this makes sense to me. You both have good points AFAICT. Please
> see below...
> 
> > >   I'd say in 99% of cases this will only cause frustration and they won't
> > > bother.  They will just conclude that either piece of hardware involved is
> > > broken and will throw it away.  Just as I almost did.  The seller has
> > > offered me a refund, which seems thought to be a universal solution
> > > nowadays (but I need to do what I meant to and getting money back doesn't
> > > solve it).
> > > 
> > >   And at least I know what U-boot (or indeed firmware) is and have a
> > > general understanding of how computers work.  Most people just want to
> > > plug stuff in and use it for whatever their need is.  Expecting them to
> > > take action to get things working is wasting their time (which BTW seems
> > > to have been a growing trend in last ~30 years: putting burden on the end
> > > user to get our problems solved, which saves our time and money at the
> > > expense of end user's).
> > 
> > The another issue here is that it was not fully investigated where the
> > issue is. If it is processor specific, PCIe switch specific or endpoint
> > specific, or combination of these options. It was just observed that
> > proposed workaround fix this issue on one specific combination. And you
> > confirmed this in previous post, that you are unsure if it is not
> > specific to downstream ports of the switch.
> > 
> > We really should not include such "bloatware" code to be enabled for
> > everything, on every one board. You are the first who reported this
> > issue. Nobody else complained about it, so I really do not see reason
> > why all other users and developers must be forced to have it in their
> > U-Boot binaries. Moreover lot of boards on which is U-Boot running do
> > not have PCIe bus exported on the slot and have PCIe devices integrated
> > and soldered on the board. Why on the earth I have to need this
> > workaround also on these boards (which are moreover sized constrained)?
> 
> Agreed. But I also understand the reasoning from Maciej, at least in
> parts. Thinking a bit more about this, my preference would be to still
> include this workaround per default in U-Boot proper though. To not
> make things too complicated here.
> 
> Just my 0.02$.

I understand it.

Anyway, I would really to know where is the issue (in which part of PCIe
hierarchy) and what exactly is affected.

I think that deep understanding of the issue is important or at least
confirmation from the vendor (which we know that it would not come).

> > >   NB I'm slowly getting fed up with the amount of non-working stuff piling
> > > up around.  Every other piece of equipment I try doesn't work for one
> > > reason or another and I need to either chase bugs myself or to spend days
> > > and weeks to persuade someone at least to believe a problem is there to
> > > get that sorted.  All in my free time I'd rather spend on something else.
> > > I'd welcome things working automagically for a change so that I could
> > > focus on what I mean to be doing, and therefore I take breaking things
> > > deliberately as a major offence.
> > > 
> > >   FWIW,
> > > 
> > >    Maciej
> > 
> > What other U-Boot developers think?
> 
> Please see above.
> 
> Thanks,
> Stefan

Re: [PATCH v2] pci: Do not enable PCIe GEN3 link retrain workaround by default

[Maciej W. Rozycki]

On Tue, 30 Aug 2022, Pali Rohár wrote:

> > > Moreover this workaround is enabled for all existing hardware and also all
> > > future PCIe hardware, which opens a hole that other PCIe vendors may
> > > introduce same HW issue as on systems where this workaround is required and
> > > nobody would notice it because U-Boot automatically apply workaround for it.
> > 
> >  Why is it a problem?
> 
> I think I wrote it. One issue is that it is increasing size of SPL image
> and we really should not include into SPL things which are not required
> for all target platforms. Lot of boards have size constrained memory
> requirements and unnecessary features should not be automatically
> enabled.

 I agree on the SPL side.

> > Is the intent to cause hassle to end users and 
> > force them to take action when they have a non-working piece of hardware?  
> 
> Vendors should really test their hardware without any automatic
> workaround. Otherwise we are going into the hell if some workarounds are
> automatically enabled and nobody notice broken behavior. And vendors
> really build software with default options and do not care about it if
> default options are suitable.

 I suspect that most vendors of PCIe option cards do not verify them with 
U-Boot at all.

> There is already direction to make all workaround targeted and enabled
> only for platforms / hardware which really need it.
> 
> So enabling some workaround for all platforms which are produced and
> will be produced in the future is step backward.

 Well workarounds for option cards do necessarily have to be included with 
all hardware that has external PCI/e connectivity I'm afraid.

 The ASMedia ASM2824 switch is used with the StarTech PEX8M2E2 dual M.2
M-Key adapter.  Also sold under the Ableconn brand as PEXM2-130.  And M.2
M-Key to PCIe slot adapters are widely available.

 The Pericom PI7C9X2G304 switch is used with the Delock 41433 dual PCIe
slot adapter.  Also sold under the SYBA IOCrest brand as SI-PEX60016.
They have both been withdrawn AFAICT now, so availability may vary though.

 You can plug these option cards into any system.

> >  And at least I know what U-boot (or indeed firmware) is and have a 
> > general understanding of how computers work.  Most people just want to 
> > plug stuff in and use it for whatever their need is.  Expecting them to 
> > take action to get things working is wasting their time (which BTW seems 
> > to have been a growing trend in last ~30 years: putting burden on the end 
> > user to get our problems solved, which saves our time and money at the 
> > expense of end user's).
> 
> The another issue here is that it was not fully investigated where the
> issue is.

 It was and the outcome detailed along with the original submission.

> If it is processor specific, PCIe switch specific or endpoint
> specific, or combination of these options. It was just observed that
> proposed workaround fix this issue on one specific combination. And you
> confirmed this in previous post, that you are unsure if it is not
> specific to downstream ports of the switch.

 It was determined that the two generic PCIe switches named above fail to 
negotiate link.  I would expect other parts from the same vendors to show 
the same problem as IP blocks get reused between silicon.

> We really should not include such "bloatware" code to be enabled for
> everything, on every one board. You are the first who reported this
> issue. Nobody else complained about it, so I really do not see reason
> why all other users and developers must be forced to have it in their
> U-Boot binaries. Moreover lot of boards on which is U-Boot running do
> not have PCIe bus exported on the slot and have PCIe devices integrated
> and soldered on the board. Why on the earth I have to need this
> workaround also on these boards (which are moreover sized constrained)?

 You need to have a way to specify the absence of slot connectivity in 
board configuration then.  This will allow one to say:

	depends on PCI_SLOT || BADLY_BROKEN_BOARD

etc.

  Maciej

Re: [PATCH v2] pci: Do not enable PCIe GEN3 link retrain workaround by default

[Maciej W. Rozycki]

On Tue, 30 Aug 2022, Stefan Roese wrote:

> > I think I wrote it. One issue is that it is increasing size of SPL image
> > and we really should not include into SPL things which are not required
> > for all target platforms. Lot of boards have size constrained memory
> > requirements and unnecessary features should not be automatically
> > enabled.
> 
> I have to agree with Pali here. We need to be careful with size increase
> in the SPL images, as some of the build targets are very limited here.
> So making this workaround configurable is definitely a good idea.

 I object to having just a single workaround configurable as I expect it 
to be beyond the capability of people on average to get right.  You simply 
start getting lost at one point, just as I am with all the random options 
the Linux kernel has nowadays.  And the dozens added with every release.

 I agree on the SPL side and on a global option.  I think it only makes 
sense to have individual workarounds selectable for onboard devices with 
hardware that has no external PCI/e connectivity.  Those should not be 
exposed to the user and instead implicitly selected by the scriptery based 
on the base machine chosen.

> The question remains, at least for me, if the Kconfig option should be
> enable per default or not. For SPL my suggestions is to disable is per
> default because of the size remarks above. For U-Boot proper I'm not so
> sure. Please see below...

 FWIW Linux has a global PCI_QUIRKS option available in the expert mode 
only that lets one disable all workarounds:

config PCI_QUIRKS
	default y
	bool "Enable PCI quirk workarounds" if EXPERT
	help
	  This enables workarounds for various PCI chipset bugs/quirks.
	  Disable this only if your target machine is unaffected by PCI
	  quirks.

for U-Boot I could envisage something like:

config PCI_QUIRKS
	bool "Enable PCI quirk workarounds" if EXPERT
	default y if !SPL
	[...]

Leaving just a single workaround out, especially for option card devices 
is asking for people getting it wrong.

 If there are individual workarounds required by hardware with no external 
PCI/e connectivity, then we could have say:

config PCI_QUIRKS
	bool "Enable PCI quirk workarounds" if EXPERT
	default y if !SPL
	select SPECIFIC_QUIRK_FOR_BADLY_BROKEN_BOARD

config BADLY_BROKEN_BOARD
	bool "Enable support for Badly Broken Board"
	select SPECIFIC_QUIRK_FOR_BADLY_BROKEN_BOARD

config SPECIFIC_QUIRK_FOR_BADLY_BROKEN_BOARD
	bool

and then wire the specific quirk to SPECIFIC_QUIRK_FOR_BADLY_BROKEN_BOARD 
rather than PCI_QUIRKS in the Makefile system.

 Thank you for your input.

  Maciej

Re: [PATCH v2] pci: Do not enable PCIe GEN3 link retrain workaround by default

[Maciej W. Rozycki]

On Tue, 30 Aug 2022, Pali Rohár wrote:

> > Agreed. But I also understand the reasoning from Maciej, at least in
> > parts. Thinking a bit more about this, my preference would be to still
> > include this workaround per default in U-Boot proper though. To not
> > make things too complicated here.
> > 
> > Just my 0.02$.
> 
> I understand it.
> 
> Anyway, I would really to know where is the issue (in which part of PCIe
> hierarchy) and what exactly is affected.

 Here's the hierarchy tree of the system affected:

-[0000:00]---00.0-[01-0b]----00.0-[02-0b]--+-00.0-[03]--
                                           +-02.0-[04]----00.0
                                           +-03.0-[05-09]----00.0-[06-09]--+-01.0-[07]--+-00.0
                                           |                               |            \-00.3
                                           |                               \-02.0-[08-09]----00.0-[09]--+-01.0
                                           |                                                            \-02.0
                                           +-04.0-[0a]----00.0
                                           \-08.0-[0b]--+-00.0
                                                        \-00.1

and the issue is between 0000:02:03.0 and 0000:05:00.0.  This has nothing 
to do with the host CPU and the ASM2824 part is a generic PCIe switch also 
available on option cards with slots.  So it can be plugged by a user into 
any system out there that has PCIe slot connectivity (also a conventional 
PCI system via a PCI-to-PCIe reverse bridge or IIUC a Thunderbolt bridge).  
Of course if a given system has no external PCI/e connectivity and no 
affected devices onboard, then there is no point in having the workaround 
included in the firmware.

> I think that deep understanding of the issue is important or at least
> confirmation from the vendor (which we know that it would not come).

 Indeed that would help a lot, but we need to live with what we have.

 FWIW I have finally found time and an availability slot with my HiFive 
Unmatched hardware to get an updated version of the Linux fix made, 
verified and posted upstream; cf. 
<https://lore.kernel.org/linux-pci/alpine.DEB.2.21.2209061238050.2275@angie.orcam.me.uk/>.

  Maciej