[PATCH v3 0/3] Add a quirk to work around link retrain errata of Pericom PCIe brigdes

[PATCH v3 0/3] Add a quirk to work around link retrain errata of Pericom PCIe brigdes

[Stefan Mätje]

This patch provides a quirk that works around PCIe link retrain issues
with some Pericom PCIe-to-PCI bridges.

This patches should be backported to the stable kernels ([1/3] and [2/3]). The
problem shows up since our customers use kernels 4.10 and later.

The original patch was only for the Pericom PI7C9X111SL bridge. This is the
only hardware I can test with.

In the meantime Brett Hull <bhull@redhat.com> brought to my attention that
the PI7C9X110 and PI7C9X130 devices are also affected. I had access to the
errata sheet for the PI7C9X130 PCI bridge and its has the same issue
documented.

Therefore the patch will now handle all three mentioned devices.

I'd like to quote the errata sheet PI7C9X111SLB_errata_rev1.2_102711.pdf

> In Reverse Mode, retrain Link bit is not cleared automatically; this bit
> needs to be cleared manually by configuration write after it is set.
>
> Problem:
> In Reverse mode, after setting Retrain Link (bit 5 of register C0h), this
> bit will stay on and PI7C9x111SL will continuously retrain until this bit
> is cleared by another Configuration Write to register C0h.
>
> Workaround:
> Issue another configuration write to clear Retrain Link bit after setting
> this bit. No delay is required between these two configuration write.

There is no public URL to download these errata sheets. Because Pericom has
been acquired by Diodes Inc. all information has to be downloaded from their
web site. Following the link below one can find a datasheet and there is a
button to request additional documents like the errata sheet for instance.

https://www.diodes.com/products/connectivity-and-timing/pcie-packet-switchbridges/pcie-pci-bridges/part/PI7C9X111SL#tab-details

I'm still using msleep(1) here because I don't want to change the timing for
other parts of the kernel unintentionally.
Some notes regarding this:

a) The link retraining is fast. The time measured on two different PCs and PCIe
	bridges is below 5µs.
b) My slower test PC needs ~2.5µs for a checking the PCI_EXP_LNKSTA register the
	faster test PC needs ~1.5µs for that. So it comes the slower test PC runs
	through the wait loop without calling msleep(1) but the faster machine needs
	to wait once with msleep(1).
	In this case waiting with udelay(10) would perhaps be more appropriate but
	I didn't want to burden this pure bug fix patch with a timing change that
	possibly needs discussion.

History:
	[V3]
		- Split into more patches to make backporting easier.
		- Applied Andy Shevchenko's recommendations regarding wording and
			code style.
	[V2]
		https://lore.kernel.org/linux-pci/20190305173122.11875-1-stefan.maetje@esd.eu/
	[V1]
		https://lore.kernel.org/linux-pci/20181101192229.48352-2-stefan.maetje@esd.eu/


Stefan Mätje (3):
  PCI/ASPM: Prepare stand-alone pcie_retrain_link() function
  PCI/ASPM: Work around link retrain errata of Pericom PCIe-to-PCI
    bridges
  PCI/ASPM: Trivial rework wait code in pcie_retrain_link()

 drivers/pci/pcie/aspm.c | 47 ++++++++++++++++++++++++++++++++---------------
 drivers/pci/quirks.c    | 20 ++++++++++++++++++++
 include/linux/pci.h     |  2 ++
 3 files changed, 54 insertions(+), 15 deletions(-)

--
2.15.0

[PATCH v3 1/3] PCI/ASPM: Prepare stand-alone pcie_retrain_link() function

[Stefan Mätje]

From: Stefan Mätje <stefan.maetje@esd.eu>

Prepare stand-alone pcie_retrain_link() function for the
Pericom retrain link quirk patch.

Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
---
 drivers/pci/pcie/aspm.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 727e3c1ef9a4..bb203f1e095d 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -196,6 +196,29 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
 	link->clkpm_capable = (blacklist) ? 0 : capable;
 }

+static bool pcie_retrain_link(struct pcie_link_state *link)
+{
+	struct pci_dev *parent = link->pdev;
+	unsigned long start_jiffies;
+	u16 reg16;
+
+	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &reg16);
+	reg16 |= PCI_EXP_LNKCTL_RL;
+	pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);
+
+	/* Wait for link training end. Break out after waiting for timeout */
+	start_jiffies = jiffies;
+	for (;;) {
+		pcie_capability_read_word(parent, PCI_EXP_LNKSTA, &reg16);
+		if (!(reg16 & PCI_EXP_LNKSTA_LT))
+			break;
+		if (time_after(jiffies, start_jiffies + LINK_RETRAIN_TIMEOUT))
+			break;
+		msleep(1);
+	}
+	return !(reg16 & PCI_EXP_LNKSTA_LT);
+}
+
 /*
  * pcie_aspm_configure_common_clock: check if the 2 ends of a link
  *   could use common clock. If they are, configure them to use the
@@ -205,7 +228,6 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
 {
 	int same_clock = 1;
 	u16 reg16, parent_reg, child_reg[8];
-	unsigned long start_jiffies;
 	struct pci_dev *child, *parent = link->pdev;
 	struct pci_bus *linkbus = parent->subordinate;
 	/*
@@ -264,20 +286,7 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
 	pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);

 	/* Retrain link */
-	reg16 |= PCI_EXP_LNKCTL_RL;
-	pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);
-
-	/* Wait for link training end. Break out after waiting for timeout */
-	start_jiffies = jiffies;
-	for (;;) {
-		pcie_capability_read_word(parent, PCI_EXP_LNKSTA, &reg16);
-		if (!(reg16 & PCI_EXP_LNKSTA_LT))
-			break;
-		if (time_after(jiffies, start_jiffies + LINK_RETRAIN_TIMEOUT))
-			break;
-		msleep(1);
-	}
-	if (!(reg16 & PCI_EXP_LNKSTA_LT))
+	if (pcie_retrain_link(link))
 		return;

 	/* Training failed. Restore common clock configurations */
--
2.15.0

[PATCH v3 2/3] PCI/ASPM: Work around link retrain errata of Pericom PCIe-to-PCI bridges

[Stefan Mätje]

Due to an erratum in some Pericom PCIe-to-PCI bridges in reverse mode the
retrain link bit needs to be cleared again manually to allow the link
training to complete successfully.

If it is not cleared manually the link training is continuously restarted
and all devices below the PCI-to-PCIe bridge can't be accessed any more.
That means drivers for devices below the bridge will be loaded but won't
work or even crash because the driver is only reading 0xffff.

See the Pericom Errata Sheet PI7C9X111SLB_errata_rev1.2_102711.pdf for
details.

Devices known as affected so far are: PI7C9X110, PI7C9X111SL, PI7C9X130

The patch introduces a new flag clear_retrain_link in the struct pci_dev.
This flag is set in quirk_enable_clear_retrain_link() for the affected
devices in the pci_fixup_header in quirks.c

In preparation to this patch the link retraining code was moved to
pcie_retrain_link(). This function now applies the work around to clear the
PCI_EXP_LNKCTL_RL bit again if clear_retrain_link bit is set in the pci_dev
structure of the link parent device.

Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
---
 drivers/pci/pcie/aspm.c | 10 ++++++++++
 drivers/pci/quirks.c    | 20 ++++++++++++++++++++
 include/linux/pci.h     |  2 ++
 3 files changed, 32 insertions(+)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index bb203f1e095d..53c34f50f419 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -205,6 +205,16 @@ static bool pcie_retrain_link(struct pcie_link_state *link)
 	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &reg16);
 	reg16 |= PCI_EXP_LNKCTL_RL;
 	pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);
+	if (parent->clear_retrain_link) {
+		/*
+		 * Due to an erratum in some devices the retrain link bit
+		 * needs to be cleared again manually to allow the link
+		 * training to succeed.
+		 */
+		reg16 &= ~PCI_EXP_LNKCTL_RL;
+		pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);
+		pci_info(parent, "Apply PCIe clear link retrain bit workaround\n");
+	}
 
 	/* Wait for link training end. Break out after waiting for timeout */
 	start_jiffies = jiffies;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a59ad09ce911..5bf490771ed5 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2245,6 +2245,26 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10f1, quirk_disable_aspm_l0s);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x10f4, quirk_disable_aspm_l0s);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1508, quirk_disable_aspm_l0s);
 
+#ifdef CONFIG_PCIEASPM
+/*
+ * Some Pericom PCIe-to-PCI bridges in reverse mode expose the erratum that
+ * they need the PCIe link retrain bit cleared after starting the link retrain
+ * process to allow this process to finish.
+ *
+ * Affected devices: PI7C9X110, PI7C9X111SL, PI7C9X130
+ *
+ * See also the Pericom Errata Sheet PI7C9X111SLB_errata_rev1.2_102711.pdf.
+ */
+static void quirk_enable_clear_retrain_link(struct pci_dev *dev)
+{
+	dev->clear_retrain_link = 1;
+	pci_info(dev, "Enable PCIe link retrain quirk\n");
+}
+DECLARE_PCI_FIXUP_HEADER(0x12d8, 0xe110, quirk_enable_clear_retrain_link);
+DECLARE_PCI_FIXUP_HEADER(0x12d8, 0xe111, quirk_enable_clear_retrain_link);
+DECLARE_PCI_FIXUP_HEADER(0x12d8, 0xe130, quirk_enable_clear_retrain_link);
+#endif /* CONFIG_PCIEASPM */
+
 static void fixup_rev1_53c810(struct pci_dev *dev)
 {
 	u32 class = dev->class;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 77448215ef5b..21424d91ed38 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -355,6 +355,8 @@ struct pci_dev {
 	struct pcie_link_state	*link_state;	/* ASPM link state */
 	unsigned int	ltr_path:1;	/* Latency Tolerance Reporting
 					   supported from root to here */
+	unsigned int	clear_retrain_link:1;	/* Need to clear link retrain
+						   bit to succeed retraining */
 #endif
 	unsigned int	eetlp_prefix_path:1;	/* End-to-End TLP Prefix */
 
-- 
2.15.0

[PATCH v3 3/3] PCI/ASPM: Trivial rework wait code in pcie_retrain_link()

[Stefan Mätje]

Transform wait code to a "do {} while (time_before())" loop as recommended
by reviewer.

Signed-off-by: Stefan Mätje <stefan.maetje@esd.eu>
---
 drivers/pci/pcie/aspm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 53c34f50f419..94e3499e4c1b 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -199,7 +199,7 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
 static bool pcie_retrain_link(struct pcie_link_state *link)
 {
 	struct pci_dev *parent = link->pdev;
-	unsigned long start_jiffies;
+	unsigned long end_jiffies;
 	u16 reg16;
 
 	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &reg16);
@@ -217,15 +217,13 @@ static bool pcie_retrain_link(struct pcie_link_state *link)
 	}
 
 	/* Wait for link training end. Break out after waiting for timeout */
-	start_jiffies = jiffies;
-	for (;;) {
+	end_jiffies = jiffies + LINK_RETRAIN_TIMEOUT;
+	do {
 		pcie_capability_read_word(parent, PCI_EXP_LNKSTA, &reg16);
 		if (!(reg16 & PCI_EXP_LNKSTA_LT))
 			break;
-		if (time_after(jiffies, start_jiffies + LINK_RETRAIN_TIMEOUT))
-			break;
 		msleep(1);
-	}
+	} while (time_before(jiffies, end_jiffies));
 	return !(reg16 & PCI_EXP_LNKSTA_LT);
 }
 
-- 
2.15.0

Re: [PATCH v3 0/3] Add a quirk to work around link retrain errata of Pericom PCIe brigdes

[Andy Shevchenko]

On Fri, Mar 29, 2019 at 06:07:33PM +0100, Stefan Mätje wrote:
> This patch provides a quirk that works around PCIe link retrain issues
> with some Pericom PCIe-to-PCI bridges.
> 
> This patches should be backported to the stable kernels ([1/3] and [2/3]). The
> problem shows up since our customers use kernels 4.10 and later.
> 
> The original patch was only for the Pericom PI7C9X111SL bridge. This is the
> only hardware I can test with.
> 
> In the meantime Brett Hull <bhull@redhat.com> brought to my attention that
> the PI7C9X110 and PI7C9X130 devices are also affected. I had access to the
> errata sheet for the PI7C9X130 PCI bridge and its has the same issue
> documented.
> 
> Therefore the patch will now handle all three mentioned devices.
> 
> I'd like to quote the errata sheet PI7C9X111SLB_errata_rev1.2_102711.pdf
> 
> > In Reverse Mode, retrain Link bit is not cleared automatically; this bit
> > needs to be cleared manually by configuration write after it is set.
> >
> > Problem:
> > In Reverse mode, after setting Retrain Link (bit 5 of register C0h), this
> > bit will stay on and PI7C9x111SL will continuously retrain until this bit
> > is cleared by another Configuration Write to register C0h.
> >
> > Workaround:
> > Issue another configuration write to clear Retrain Link bit after setting
> > this bit. No delay is required between these two configuration write.
> 
> There is no public URL to download these errata sheets. Because Pericom has
> been acquired by Diodes Inc. all information has to be downloaded from their
> web site. Following the link below one can find a datasheet and there is a
> button to request additional documents like the errata sheet for instance.
> 
> https://www.diodes.com/products/connectivity-and-timing/pcie-packet-switchbridges/pcie-pci-bridges/part/PI7C9X111SL#tab-details
> 
> I'm still using msleep(1) here because I don't want to change the timing for
> other parts of the kernel unintentionally.
> Some notes regarding this:
> 
> a) The link retraining is fast. The time measured on two different PCs and PCIe
> 	bridges is below 5µs.
> b) My slower test PC needs ~2.5µs for a checking the PCI_EXP_LNKSTA register the
> 	faster test PC needs ~1.5µs for that. So it comes the slower test PC runs
> 	through the wait loop without calling msleep(1) but the faster machine needs
> 	to wait once with msleep(1).
> 	In this case waiting with udelay(10) would perhaps be more appropriate but
> 	I didn't want to burden this pure bug fix patch with a timing change that
> 	possibly needs discussion.
> 
> History:
> 	[V3]
> 		- Split into more patches to make backporting easier.
> 		- Applied Andy Shevchenko's recommendations regarding wording and
> 			code style.
> 	[V2]
> 		https://lore.kernel.org/linux-pci/20190305173122.11875-1-stefan.maetje@esd.eu/
> 	[V1]
> 		https://lore.kernel.org/linux-pci/20181101192229.48352-2-stefan.maetje@esd.eu/
> 

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> Stefan Mätje (3):
>   PCI/ASPM: Prepare stand-alone pcie_retrain_link() function
>   PCI/ASPM: Work around link retrain errata of Pericom PCIe-to-PCI
>     bridges
>   PCI/ASPM: Trivial rework wait code in pcie_retrain_link()
> 
>  drivers/pci/pcie/aspm.c | 47 ++++++++++++++++++++++++++++++++---------------
>  drivers/pci/quirks.c    | 20 ++++++++++++++++++++
>  include/linux/pci.h     |  2 ++
>  3 files changed, 54 insertions(+), 15 deletions(-)
> 
> --
> 2.15.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/3] Add a quirk to work around link retrain errata of Pericom PCIe brigdes

[Bjorn Helgaas]

On Fri, Mar 29, 2019 at 06:07:33PM +0100, Stefan Mätje wrote:
> This patch provides a quirk that works around PCIe link retrain issues
> with some Pericom PCIe-to-PCI bridges.
> 
> This patches should be backported to the stable kernels ([1/3] and [2/3]). The
> problem shows up since our customers use kernels 4.10 and later.
> 
> The original patch was only for the Pericom PI7C9X111SL bridge. This is the
> only hardware I can test with.
> 
> In the meantime Brett Hull <bhull@redhat.com> brought to my attention that
> the PI7C9X110 and PI7C9X130 devices are also affected. I had access to the
> errata sheet for the PI7C9X130 PCI bridge and its has the same issue
> documented.
> 
> Therefore the patch will now handle all three mentioned devices.
> 
> I'd like to quote the errata sheet PI7C9X111SLB_errata_rev1.2_102711.pdf
> 
> > In Reverse Mode, retrain Link bit is not cleared automatically; this bit
> > needs to be cleared manually by configuration write after it is set.
> >
> > Problem:
> > In Reverse mode, after setting Retrain Link (bit 5 of register C0h), this
> > bit will stay on and PI7C9x111SL will continuously retrain until this bit
> > is cleared by another Configuration Write to register C0h.
> >
> > Workaround:
> > Issue another configuration write to clear Retrain Link bit after setting
> > this bit. No delay is required between these two configuration write.
> 
> There is no public URL to download these errata sheets. Because Pericom has
> been acquired by Diodes Inc. all information has to be downloaded from their
> web site. Following the link below one can find a datasheet and there is a
> button to request additional documents like the errata sheet for instance.
> 
> https://www.diodes.com/products/connectivity-and-timing/pcie-packet-switchbridges/pcie-pci-bridges/part/PI7C9X111SL#tab-details
> 
> I'm still using msleep(1) here because I don't want to change the timing for
> other parts of the kernel unintentionally.
> Some notes regarding this:
> 
> a) The link retraining is fast. The time measured on two different PCs and PCIe
> 	bridges is below 5µs.
> b) My slower test PC needs ~2.5µs for a checking the PCI_EXP_LNKSTA register the
> 	faster test PC needs ~1.5µs for that. So it comes the slower test PC runs
> 	through the wait loop without calling msleep(1) but the faster machine needs
> 	to wait once with msleep(1).
> 	In this case waiting with udelay(10) would perhaps be more appropriate but
> 	I didn't want to burden this pure bug fix patch with a timing change that
> 	possibly needs discussion.
> 
> History:
> 	[V3]
> 		- Split into more patches to make backporting easier.
> 		- Applied Andy Shevchenko's recommendations regarding wording and
> 			code style.
> 	[V2]
> 		https://lore.kernel.org/linux-pci/20190305173122.11875-1-stefan.maetje@esd.eu/
> 	[V1]
> 		https://lore.kernel.org/linux-pci/20181101192229.48352-2-stefan.maetje@esd.eu/
> 
> 
> Stefan Mätje (3):
>   PCI/ASPM: Prepare stand-alone pcie_retrain_link() function
>   PCI/ASPM: Work around link retrain errata of Pericom PCIe-to-PCI
>     bridges
>   PCI/ASPM: Trivial rework wait code in pcie_retrain_link()
> 
>  drivers/pci/pcie/aspm.c | 47 ++++++++++++++++++++++++++++++++---------------
>  drivers/pci/quirks.c    | 20 ++++++++++++++++++++
>  include/linux/pci.h     |  2 ++
>  3 files changed, 54 insertions(+), 15 deletions(-)

Thanks, applied to pci/enumeration for v5.2.

I added stable tags for the first two patches.  I also moved the quirk
and related bits out from under CONFIG_PCIEASPM because I don't think
the erratum is actually related to ASPM; it just happens that ASPM is
the only place we currently retrain links.  But we *could* retrain
links for other reasons, e.g., changing link speed or error recovery,
and we would also want the quirk then.