[PATCHv4 1/2] PCI: fu740: fix finding GPIOs

[PATCHv4 1/2] PCI: fu740: fix finding GPIOs

[Ben Dooks]

The calls to devm_gpiod_get_optional() have the -gpios at the end of
the name but the GPIO core code is already adding the suffix during
the lookup. This means the PCIe driver is not finding the necessary
reset or power lines to allow initialisation of the PCIe.

This bug has not been noticed as if U-Boot has setup the GPIO lines
for the hardware when it does the PCIe initialisation (either by
booting from PCIe or user command to access PCIe) then the PCIe
will work in Linux. The U-Boot as supplied by SiFive does not by
default initialise any PCIe component.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/pci/controller/dwc/pcie-fu740.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
index 00cde9a248b5..842b7202b96e 100644
--- a/drivers/pci/controller/dwc/pcie-fu740.c
+++ b/drivers/pci/controller/dwc/pcie-fu740.c
@@ -259,11 +259,11 @@ static int fu740_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(afp->mgmt_base);
 
 	/* Fetch GPIOs */
-	afp->reset = devm_gpiod_get_optional(dev, "reset-gpios", GPIOD_OUT_LOW);
+	afp->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(afp->reset))
 		return dev_err_probe(dev, PTR_ERR(afp->reset), "unable to get reset-gpios\n");
 
-	afp->pwren = devm_gpiod_get_optional(dev, "pwren-gpios", GPIOD_OUT_LOW);
+	afp->pwren = devm_gpiod_get_optional(dev, "pwren", GPIOD_OUT_LOW);
 	if (IS_ERR(afp->pwren))
 		return dev_err_probe(dev, PTR_ERR(afp->pwren), "unable to get pwren-gpios\n");
 
-- 
2.34.1

[PATCHv4 1/2] PCI: fu740: fix finding GPIOs

[Ben Dooks]

The calls to devm_gpiod_get_optional() have the -gpios at the end of
the name but the GPIO core code is already adding the suffix during
the lookup. This means the PCIe driver is not finding the necessary
reset or power lines to allow initialisation of the PCIe.

This bug has not been noticed as if U-Boot has setup the GPIO lines
for the hardware when it does the PCIe initialisation (either by
booting from PCIe or user command to access PCIe) then the PCIe
will work in Linux. The U-Boot as supplied by SiFive does not by
default initialise any PCIe component.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/pci/controller/dwc/pcie-fu740.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
index 00cde9a248b5..842b7202b96e 100644
--- a/drivers/pci/controller/dwc/pcie-fu740.c
+++ b/drivers/pci/controller/dwc/pcie-fu740.c
@@ -259,11 +259,11 @@ static int fu740_pcie_probe(struct platform_device *pdev)
 		return PTR_ERR(afp->mgmt_base);
 
 	/* Fetch GPIOs */
-	afp->reset = devm_gpiod_get_optional(dev, "reset-gpios", GPIOD_OUT_LOW);
+	afp->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(afp->reset))
 		return dev_err_probe(dev, PTR_ERR(afp->reset), "unable to get reset-gpios\n");
 
-	afp->pwren = devm_gpiod_get_optional(dev, "pwren-gpios", GPIOD_OUT_LOW);
+	afp->pwren = devm_gpiod_get_optional(dev, "pwren", GPIOD_OUT_LOW);
 	if (IS_ERR(afp->pwren))
 		return dev_err_probe(dev, PTR_ERR(afp->pwren), "unable to get pwren-gpios\n");
 
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe

[Ben Dooks]

The fu740 PCIe core does not probe any devices on the SiFive Unmatched
board without this fix from U-Boot (or having U-Boot explicitly start
the PCIe via either boot-script or user command).

The fix claims to set the link-speed to gen1 to get the probe
to work. As this is a copy from U-Boot, the code is assumed to be
correct and does fix the issue on the Unmatched. The code is at:
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c#L271

The code has been this way since the driver was commited in:
https://source.denx.de/u-boot/u-boot/-/commit/416395c772018c6bf52aad36aca163115001793f

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/pci/controller/dwc/pcie-fu740.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
index 842b7202b96e..19501ec8c487 100644
--- a/drivers/pci/controller/dwc/pcie-fu740.c
+++ b/drivers/pci/controller/dwc/pcie-fu740.c
@@ -177,11 +177,30 @@ static void fu740_pcie_init_phy(struct fu740_pcie *afp)
 	fu740_phyregwrite(1, PCIEX8MGMT_PHY_LANE3_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
 }
 
+/* This is copied from u-boot. Force system to gen1 otherwise nothing probes
+ * as found on the SiFive Unmatched board.
+ */
+static void fu740_pcie_force_gen1(struct dw_pcie *dw, struct fu740_pcie *afp )
+{
+	unsigned val;
+
+	dw_pcie_dbi_ro_wr_en(dw);
+
+	val = dw_pcie_readl_dbi(dw, 0x70 + PCI_EXP_LNKCAP);
+	pr_info("%s: link-cap was %08x\n", __func__, val);
+	dw_pcie_writel_dbi(dw, 0x70 + PCI_EXP_LNKCAP, val | 0xf);
+
+	dw_pcie_dbi_ro_wr_dis(dw);
+}
+
 static int fu740_pcie_start_link(struct dw_pcie *pci)
 {
 	struct device *dev = pci->dev;
 	struct fu740_pcie *afp = dev_get_drvdata(dev);
 
+	/* Force PCIe gen1 otherwise Unmatched board does not probe */
+	fu740_pcie_force_gen1(pci, afp);
+
 	/* Enable LTSSM */
 	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
 	return 0;
-- 
2.34.1

[PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe

[Ben Dooks]

The fu740 PCIe core does not probe any devices on the SiFive Unmatched
board without this fix from U-Boot (or having U-Boot explicitly start
the PCIe via either boot-script or user command).

The fix claims to set the link-speed to gen1 to get the probe
to work. As this is a copy from U-Boot, the code is assumed to be
correct and does fix the issue on the Unmatched. The code is at:
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c#L271

The code has been this way since the driver was commited in:
https://source.denx.de/u-boot/u-boot/-/commit/416395c772018c6bf52aad36aca163115001793f

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/pci/controller/dwc/pcie-fu740.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
index 842b7202b96e..19501ec8c487 100644
--- a/drivers/pci/controller/dwc/pcie-fu740.c
+++ b/drivers/pci/controller/dwc/pcie-fu740.c
@@ -177,11 +177,30 @@ static void fu740_pcie_init_phy(struct fu740_pcie *afp)
 	fu740_phyregwrite(1, PCIEX8MGMT_PHY_LANE3_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
 }
 
+/* This is copied from u-boot. Force system to gen1 otherwise nothing probes
+ * as found on the SiFive Unmatched board.
+ */
+static void fu740_pcie_force_gen1(struct dw_pcie *dw, struct fu740_pcie *afp )
+{
+	unsigned val;
+
+	dw_pcie_dbi_ro_wr_en(dw);
+
+	val = dw_pcie_readl_dbi(dw, 0x70 + PCI_EXP_LNKCAP);
+	pr_info("%s: link-cap was %08x\n", __func__, val);
+	dw_pcie_writel_dbi(dw, 0x70 + PCI_EXP_LNKCAP, val | 0xf);
+
+	dw_pcie_dbi_ro_wr_dis(dw);
+}
+
 static int fu740_pcie_start_link(struct dw_pcie *pci)
 {
 	struct device *dev = pci->dev;
 	struct fu740_pcie *afp = dev_get_drvdata(dev);
 
+	/* Force PCIe gen1 otherwise Unmatched board does not probe */
+	fu740_pcie_force_gen1(pci, afp);
+
 	/* Enable LTSSM */
 	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
 	return 0;
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCHv4 1/2] PCI: fu740: fix finding GPIOs

[Bjorn Helgaas]

On Mon, Feb 21, 2022 at 09:03:46PM +0000, Ben Dooks wrote:
> The calls to devm_gpiod_get_optional() have the -gpios at the end of
> the name but the GPIO core code is already adding the suffix during
> the lookup. This means the PCIe driver is not finding the necessary
> reset or power lines to allow initialisation of the PCIe.
> 
> This bug has not been noticed as if U-Boot has setup the GPIO lines
> for the hardware when it does the PCIe initialisation (either by
> booting from PCIe or user command to access PCIe) then the PCIe
> will work in Linux. The U-Boot as supplied by SiFive does not by
> default initialise any PCIe component.

Lorenzo, if you apply this, would you mind s/fix/Fix/ in the subject?
Or maybe even update to "Drop redundant '-gpios' from DT GPIO lookup"?

> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/pci/controller/dwc/pcie-fu740.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
> index 00cde9a248b5..842b7202b96e 100644
> --- a/drivers/pci/controller/dwc/pcie-fu740.c
> +++ b/drivers/pci/controller/dwc/pcie-fu740.c
> @@ -259,11 +259,11 @@ static int fu740_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(afp->mgmt_base);
>  
>  	/* Fetch GPIOs */
> -	afp->reset = devm_gpiod_get_optional(dev, "reset-gpios", GPIOD_OUT_LOW);
> +	afp->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>  	if (IS_ERR(afp->reset))
>  		return dev_err_probe(dev, PTR_ERR(afp->reset), "unable to get reset-gpios\n");
>  
> -	afp->pwren = devm_gpiod_get_optional(dev, "pwren-gpios", GPIOD_OUT_LOW);
> +	afp->pwren = devm_gpiod_get_optional(dev, "pwren", GPIOD_OUT_LOW);
>  	if (IS_ERR(afp->pwren))
>  		return dev_err_probe(dev, PTR_ERR(afp->pwren), "unable to get pwren-gpios\n");
>  
> -- 
> 2.34.1
> 

Re: [PATCHv4 1/2] PCI: fu740: fix finding GPIOs

[Bjorn Helgaas]

On Mon, Feb 21, 2022 at 09:03:46PM +0000, Ben Dooks wrote:
> The calls to devm_gpiod_get_optional() have the -gpios at the end of
> the name but the GPIO core code is already adding the suffix during
> the lookup. This means the PCIe driver is not finding the necessary
> reset or power lines to allow initialisation of the PCIe.
> 
> This bug has not been noticed as if U-Boot has setup the GPIO lines
> for the hardware when it does the PCIe initialisation (either by
> booting from PCIe or user command to access PCIe) then the PCIe
> will work in Linux. The U-Boot as supplied by SiFive does not by
> default initialise any PCIe component.

Lorenzo, if you apply this, would you mind s/fix/Fix/ in the subject?
Or maybe even update to "Drop redundant '-gpios' from DT GPIO lookup"?

> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/pci/controller/dwc/pcie-fu740.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
> index 00cde9a248b5..842b7202b96e 100644
> --- a/drivers/pci/controller/dwc/pcie-fu740.c
> +++ b/drivers/pci/controller/dwc/pcie-fu740.c
> @@ -259,11 +259,11 @@ static int fu740_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(afp->mgmt_base);
>  
>  	/* Fetch GPIOs */
> -	afp->reset = devm_gpiod_get_optional(dev, "reset-gpios", GPIOD_OUT_LOW);
> +	afp->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>  	if (IS_ERR(afp->reset))
>  		return dev_err_probe(dev, PTR_ERR(afp->reset), "unable to get reset-gpios\n");
>  
> -	afp->pwren = devm_gpiod_get_optional(dev, "pwren-gpios", GPIOD_OUT_LOW);
> +	afp->pwren = devm_gpiod_get_optional(dev, "pwren", GPIOD_OUT_LOW);
>  	if (IS_ERR(afp->pwren))
>  		return dev_err_probe(dev, PTR_ERR(afp->pwren), "unable to get pwren-gpios\n");
>  
> -- 
> 2.34.1
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe

[Bjorn Helgaas]

On Mon, Feb 21, 2022 at 09:03:47PM +0000, Ben Dooks wrote:
> The fu740 PCIe core does not probe any devices on the SiFive Unmatched
> board without this fix from U-Boot (or having U-Boot explicitly start
> the PCIe via either boot-script or user command).
> 
> The fix claims to set the link-speed to gen1 to get the probe
> to work. As this is a copy from U-Boot, the code is assumed to be
> correct and does fix the issue on the Unmatched. The code is at:

Maybe something like:

  Limit the link to Gen1 speed.

since "the fix claims" and "the code is assumed" is sort of
weasel-worded.

The subject says "for initial device probe," but if you change
PCI_EXP_LNKCAP, I assume that limits the link speed forever, even
after a retrain?

> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c#L271

Maybe use this so the link doesn't become stale when more things are
added to pcie_dw_sifive.c:

  https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c?id=v2022.01#L271

> The code has been this way since the driver was commited in:
> https://source.denx.de/u-boot/u-boot/-/commit/416395c772018c6bf52aad36aca163115001793f

s/commited/committed/

> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/pci/controller/dwc/pcie-fu740.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
> index 842b7202b96e..19501ec8c487 100644
> --- a/drivers/pci/controller/dwc/pcie-fu740.c
> +++ b/drivers/pci/controller/dwc/pcie-fu740.c
> @@ -177,11 +177,30 @@ static void fu740_pcie_init_phy(struct fu740_pcie *afp)
>  	fu740_phyregwrite(1, PCIEX8MGMT_PHY_LANE3_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
>  }
>  
> +/* This is copied from u-boot. Force system to gen1 otherwise nothing probes
> + * as found on the SiFive Unmatched board.
> + */

s/u-boot/U-Boot/

Use usual multi-line comment style.

> +static void fu740_pcie_force_gen1(struct dw_pcie *dw, struct fu740_pcie *afp )
> +{
> +	unsigned val;

u32, since that's what dw_pcie_readl_dbi() returns and
dw_pcie_writel_dbi() expects.

> +
> +	dw_pcie_dbi_ro_wr_en(dw);
> +
> +	val = dw_pcie_readl_dbi(dw, 0x70 + PCI_EXP_LNKCAP);

I assume 0x70 is the offset of the PCIe Capability.  There should be a
#define for that.

> +	pr_info("%s: link-cap was %08x\n", __func__, val);

  dev_info(pci->dev, "...");

> +	dw_pcie_writel_dbi(dw, 0x70 + PCI_EXP_LNKCAP, val | 0xf);

I don't understand this.  Per PCIe r6.0, sec 7.5.3.6, 1111b is a
reserved encoding for the low four bits of PCI_EXP_LNKCAP.

If you want PCI_EXP_LNKCAP to advertise only 2.5 GT/s, the low four
bits should be 0001b to indicate Supported Link Speeds Vector field
bit 0, which is defined as 2.5 GT/s.

> +	dw_pcie_dbi_ro_wr_dis(dw);
> +}
> +
>  static int fu740_pcie_start_link(struct dw_pcie *pci)
>  {
>  	struct device *dev = pci->dev;
>  	struct fu740_pcie *afp = dev_get_drvdata(dev);
>  
> +	/* Force PCIe gen1 otherwise Unmatched board does not probe */
> +	fu740_pcie_force_gen1(pci, afp);

I guess the "Unmatched" board is the only thing we need to care about
here?  Are there or will there be other boards that don't need this?

>  	/* Enable LTSSM */
>  	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
>  	return 0;
> -- 
> 2.34.1
> 

Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe

[Bjorn Helgaas]

On Mon, Feb 21, 2022 at 09:03:47PM +0000, Ben Dooks wrote:
> The fu740 PCIe core does not probe any devices on the SiFive Unmatched
> board without this fix from U-Boot (or having U-Boot explicitly start
> the PCIe via either boot-script or user command).
> 
> The fix claims to set the link-speed to gen1 to get the probe
> to work. As this is a copy from U-Boot, the code is assumed to be
> correct and does fix the issue on the Unmatched. The code is at:

Maybe something like:

  Limit the link to Gen1 speed.

since "the fix claims" and "the code is assumed" is sort of
weasel-worded.

The subject says "for initial device probe," but if you change
PCI_EXP_LNKCAP, I assume that limits the link speed forever, even
after a retrain?

> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c#L271

Maybe use this so the link doesn't become stale when more things are
added to pcie_dw_sifive.c:

  https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c?id=v2022.01#L271

> The code has been this way since the driver was commited in:
> https://source.denx.de/u-boot/u-boot/-/commit/416395c772018c6bf52aad36aca163115001793f

s/commited/committed/

> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/pci/controller/dwc/pcie-fu740.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
> index 842b7202b96e..19501ec8c487 100644
> --- a/drivers/pci/controller/dwc/pcie-fu740.c
> +++ b/drivers/pci/controller/dwc/pcie-fu740.c
> @@ -177,11 +177,30 @@ static void fu740_pcie_init_phy(struct fu740_pcie *afp)
>  	fu740_phyregwrite(1, PCIEX8MGMT_PHY_LANE3_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
>  }
>  
> +/* This is copied from u-boot. Force system to gen1 otherwise nothing probes
> + * as found on the SiFive Unmatched board.
> + */

s/u-boot/U-Boot/

Use usual multi-line comment style.

> +static void fu740_pcie_force_gen1(struct dw_pcie *dw, struct fu740_pcie *afp )
> +{
> +	unsigned val;

u32, since that's what dw_pcie_readl_dbi() returns and
dw_pcie_writel_dbi() expects.

> +
> +	dw_pcie_dbi_ro_wr_en(dw);
> +
> +	val = dw_pcie_readl_dbi(dw, 0x70 + PCI_EXP_LNKCAP);

I assume 0x70 is the offset of the PCIe Capability.  There should be a
#define for that.

> +	pr_info("%s: link-cap was %08x\n", __func__, val);

  dev_info(pci->dev, "...");

> +	dw_pcie_writel_dbi(dw, 0x70 + PCI_EXP_LNKCAP, val | 0xf);

I don't understand this.  Per PCIe r6.0, sec 7.5.3.6, 1111b is a
reserved encoding for the low four bits of PCI_EXP_LNKCAP.

If you want PCI_EXP_LNKCAP to advertise only 2.5 GT/s, the low four
bits should be 0001b to indicate Supported Link Speeds Vector field
bit 0, which is defined as 2.5 GT/s.

> +	dw_pcie_dbi_ro_wr_dis(dw);
> +}
> +
>  static int fu740_pcie_start_link(struct dw_pcie *pci)
>  {
>  	struct device *dev = pci->dev;
>  	struct fu740_pcie *afp = dev_get_drvdata(dev);
>  
> +	/* Force PCIe gen1 otherwise Unmatched board does not probe */
> +	fu740_pcie_force_gen1(pci, afp);

I guess the "Unmatched" board is the only thing we need to care about
here?  Are there or will there be other boards that don't need this?

>  	/* Enable LTSSM */
>  	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
>  	return 0;
> -- 
> 2.34.1
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe

[Maciej W. Rozycki]

On Wed, 23 Feb 2022, Bjorn Helgaas wrote:

> > +	dw_pcie_dbi_ro_wr_dis(dw);
> > +}
> > +
> >  static int fu740_pcie_start_link(struct dw_pcie *pci)
> >  {
> >  	struct device *dev = pci->dev;
> >  	struct fu740_pcie *afp = dev_get_drvdata(dev);
> >  
> > +	/* Force PCIe gen1 otherwise Unmatched board does not probe */
> > +	fu740_pcie_force_gen1(pci, afp);
> 
> I guess the "Unmatched" board is the only thing we need to care about
> here?  Are there or will there be other boards that don't need this?

 I wonder if this is the other side of a supposed erratum observed here:

<https://lore.kernel.org/all/alpine.DEB.2.21.2202010240190.58572@angie.orcam.me.uk/>

Downstream there's the same ASMedia ASM2824 PCIe switch whose downstream 
ports don't want to train with a Pericom part above Gen1.

 Of course we don't know an ASM2824 is there until we have successfully 
negotiated the link, so we may have to generalise my proposal if we can 
find a way similar to what I have done for U-boot that does not disturb 
Linux's operation.  This is because there are PCIe option cards out there 
with the ASM2824 onboard, so it could be possible for the problem to 
trigger anywhere where the conditions for the erratum are met.

 Also in that case retraining should work with the cap removed to get a 
higher final speed just as with the Pericom part.

  Maciej

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe

[Maciej W. Rozycki]

On Wed, 23 Feb 2022, Bjorn Helgaas wrote:

> > +	dw_pcie_dbi_ro_wr_dis(dw);
> > +}
> > +
> >  static int fu740_pcie_start_link(struct dw_pcie *pci)
> >  {
> >  	struct device *dev = pci->dev;
> >  	struct fu740_pcie *afp = dev_get_drvdata(dev);
> >  
> > +	/* Force PCIe gen1 otherwise Unmatched board does not probe */
> > +	fu740_pcie_force_gen1(pci, afp);
> 
> I guess the "Unmatched" board is the only thing we need to care about
> here?  Are there or will there be other boards that don't need this?

 I wonder if this is the other side of a supposed erratum observed here:

<https://lore.kernel.org/all/alpine.DEB.2.21.2202010240190.58572@angie.orcam.me.uk/>

Downstream there's the same ASMedia ASM2824 PCIe switch whose downstream 
ports don't want to train with a Pericom part above Gen1.

 Of course we don't know an ASM2824 is there until we have successfully 
negotiated the link, so we may have to generalise my proposal if we can 
find a way similar to what I have done for U-boot that does not disturb 
Linux's operation.  This is because there are PCIe option cards out there 
with the ASM2824 onboard, so it could be possible for the problem to 
trigger anywhere where the conditions for the erratum are met.

 Also in that case retraining should work with the cap removed to get a 
higher final speed just as with the Pericom part.

  Maciej

Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe

[Ben Dooks]

On 23/02/2022 20:51, Bjorn Helgaas wrote:
> On Mon, Feb 21, 2022 at 09:03:47PM +0000, Ben Dooks wrote:
>> The fu740 PCIe core does not probe any devices on the SiFive Unmatched
>> board without this fix from U-Boot (or having U-Boot explicitly start
>> the PCIe via either boot-script or user command).
>>
>> The fix claims to set the link-speed to gen1 to get the probe
>> to work. As this is a copy from U-Boot, the code is assumed to be
>> correct and does fix the issue on the Unmatched. The code is at:
> 
> Maybe something like:
> 
>    Limit the link to Gen1 speed.
> 
> since "the fix claims" and "the code is assumed" is sort of
> weasel-worded.
> 
> The subject says "for initial device probe," but if you change
> PCI_EXP_LNKCAP, I assume that limits the link speed forever, even
> after a retrain?

Yes, thought I had checked this but having just gone back and booted
things again, the following is observed:

- U-Boot "pci enum" and then unpatched kernel -> link 8GT/s
- U-Boot "pci enum" and patched kernel -> link 2.5GT/s
- U-Boot no pci and patched kernel > link 2.5 GT/s

Clearly there needs to be some fix for this, either have two rounds
of soft-reset on the first probe, or if the device does probe at a
degraded link, do something about it.

I am not clear what the correct fix for this is.

1) Detect no U-Boot initialisation and force a two stage probe
2) If no devices on initial probe, go around and do #1
3) Always do a two stage reset
4) Something else.

Do any other systems see this issue?

I assume we need to go back and re-do this. At least this is a 100%
reliable repeat on the Unmatched board in our test rack.

>> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c#L271
> 
> Maybe use this so the link doesn't become stale when more things are
> added to pcie_dw_sifive.c:
> 
>   https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c?id=v2022.01#L271

Thanks, useful to know.

>> The code has been this way since the driver was commited in:
>> https://source.denx.de/u-boot/u-boot/-/commit/416395c772018c6bf52aad36aca163115001793f
> 
> s/commited/committed/

Oops,

> 
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   drivers/pci/controller/dwc/pcie-fu740.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
>> index 842b7202b96e..19501ec8c487 100644
>> --- a/drivers/pci/controller/dwc/pcie-fu740.c
>> +++ b/drivers/pci/controller/dwc/pcie-fu740.c
>> @@ -177,11 +177,30 @@ static void fu740_pcie_init_phy(struct fu740_pcie *afp)
>>   	fu740_phyregwrite(1, PCIEX8MGMT_PHY_LANE3_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
>>   }
>>   
>> +/* This is copied from u-boot. Force system to gen1 otherwise nothing probes
>> + * as found on the SiFive Unmatched board.
>> + */
> 
> s/u-boot/U-Boot/
> 
> Use usual multi-line comment style.

Ok.

> 
>> +static void fu740_pcie_force_gen1(struct dw_pcie *dw, struct fu740_pcie *afp )
>> +{
>> +	unsigned val;
> 
> u32, since that's what dw_pcie_readl_dbi() returns and
> dw_pcie_writel_dbi() expects.
> 
>> +
>> +	dw_pcie_dbi_ro_wr_en(dw);
>> +
>> +	val = dw_pcie_readl_dbi(dw, 0x70 + PCI_EXP_LNKCAP);
> 
> I assume 0x70 is the offset of the PCIe Capability.  There should be a
> #define for that.

Will fix.

>> +	pr_info("%s: link-cap was %08x\n", __func__, val);
> 
>    dev_info(pci->dev, "...");
> 
>> +	dw_pcie_writel_dbi(dw, 0x70 + PCI_EXP_LNKCAP, val | 0xf);
> 
> I don't understand this.  Per PCIe r6.0, sec 7.5.3.6, 1111b is a
> reserved encoding for the low four bits of PCI_EXP_LNKCAP.
> 
> If you want PCI_EXP_LNKCAP to advertise only 2.5 GT/s, the low four
> bits should be 0001b to indicate Supported Link Speeds Vector field
> bit 0, which is defined as 2.5 GT/s.

Yeah, this does not make sense now. I sort of assumed it was W1C
type thing (write-1-clear). I'm just wondering if this is now some
sort of undefined behaviour or they originally meant to clear out
some bits only... It does work, just shows the following from lspci;

  LnkCap: Port #0, Speed unknown, Width x8, ASPM L0s L1, Exit Latency 
L0s <4us, L1 <4us
          ClockPM- Surprise- LLActRep+ BwNot- ASPMOptComp+
  LnkCtl: ASPM Disabled; RCB 64 bytes, Disabled- CommClk+
         ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
   LnkSta: Speed 2.5GT/s (downgraded), Width x8 (ok)
         TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt-


>> +	dw_pcie_dbi_ro_wr_dis(dw);
>> +}
>> +
>>   static int fu740_pcie_start_link(struct dw_pcie *pci)
>>   {
>>   	struct device *dev = pci->dev;
>>   	struct fu740_pcie *afp = dev_get_drvdata(dev);
>>   
>> +	/* Force PCIe gen1 otherwise Unmatched board does not probe */
>> +	fu740_pcie_force_gen1(pci, afp);
> 
> I guess the "Unmatched" board is the only thing we need to care about
> here?  Are there or will there be other boards that don't need this?
> 
>>   	/* Enable LTSSM */
>>   	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
>>   	return 0;
>> -- 
>> 2.34.1
>>
> 


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe

[Ben Dooks]

On 23/02/2022 20:51, Bjorn Helgaas wrote:
> On Mon, Feb 21, 2022 at 09:03:47PM +0000, Ben Dooks wrote:
>> The fu740 PCIe core does not probe any devices on the SiFive Unmatched
>> board without this fix from U-Boot (or having U-Boot explicitly start
>> the PCIe via either boot-script or user command).
>>
>> The fix claims to set the link-speed to gen1 to get the probe
>> to work. As this is a copy from U-Boot, the code is assumed to be
>> correct and does fix the issue on the Unmatched. The code is at:
> 
> Maybe something like:
> 
>    Limit the link to Gen1 speed.
> 
> since "the fix claims" and "the code is assumed" is sort of
> weasel-worded.
> 
> The subject says "for initial device probe," but if you change
> PCI_EXP_LNKCAP, I assume that limits the link speed forever, even
> after a retrain?

Yes, thought I had checked this but having just gone back and booted
things again, the following is observed:

- U-Boot "pci enum" and then unpatched kernel -> link 8GT/s
- U-Boot "pci enum" and patched kernel -> link 2.5GT/s
- U-Boot no pci and patched kernel > link 2.5 GT/s

Clearly there needs to be some fix for this, either have two rounds
of soft-reset on the first probe, or if the device does probe at a
degraded link, do something about it.

I am not clear what the correct fix for this is.

1) Detect no U-Boot initialisation and force a two stage probe
2) If no devices on initial probe, go around and do #1
3) Always do a two stage reset
4) Something else.

Do any other systems see this issue?

I assume we need to go back and re-do this. At least this is a 100%
reliable repeat on the Unmatched board in our test rack.

>> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c#L271
> 
> Maybe use this so the link doesn't become stale when more things are
> added to pcie_dw_sifive.c:
> 
>   https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pcie_dw_sifive.c?id=v2022.01#L271

Thanks, useful to know.

>> The code has been this way since the driver was commited in:
>> https://source.denx.de/u-boot/u-boot/-/commit/416395c772018c6bf52aad36aca163115001793f
> 
> s/commited/committed/

Oops,

> 
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>   drivers/pci/controller/dwc/pcie-fu740.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
>> index 842b7202b96e..19501ec8c487 100644
>> --- a/drivers/pci/controller/dwc/pcie-fu740.c
>> +++ b/drivers/pci/controller/dwc/pcie-fu740.c
>> @@ -177,11 +177,30 @@ static void fu740_pcie_init_phy(struct fu740_pcie *afp)
>>   	fu740_phyregwrite(1, PCIEX8MGMT_PHY_LANE3_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
>>   }
>>   
>> +/* This is copied from u-boot. Force system to gen1 otherwise nothing probes
>> + * as found on the SiFive Unmatched board.
>> + */
> 
> s/u-boot/U-Boot/
> 
> Use usual multi-line comment style.

Ok.

> 
>> +static void fu740_pcie_force_gen1(struct dw_pcie *dw, struct fu740_pcie *afp )
>> +{
>> +	unsigned val;
> 
> u32, since that's what dw_pcie_readl_dbi() returns and
> dw_pcie_writel_dbi() expects.
> 
>> +
>> +	dw_pcie_dbi_ro_wr_en(dw);
>> +
>> +	val = dw_pcie_readl_dbi(dw, 0x70 + PCI_EXP_LNKCAP);
> 
> I assume 0x70 is the offset of the PCIe Capability.  There should be a
> #define for that.

Will fix.

>> +	pr_info("%s: link-cap was %08x\n", __func__, val);
> 
>    dev_info(pci->dev, "...");
> 
>> +	dw_pcie_writel_dbi(dw, 0x70 + PCI_EXP_LNKCAP, val | 0xf);
> 
> I don't understand this.  Per PCIe r6.0, sec 7.5.3.6, 1111b is a
> reserved encoding for the low four bits of PCI_EXP_LNKCAP.
> 
> If you want PCI_EXP_LNKCAP to advertise only 2.5 GT/s, the low four
> bits should be 0001b to indicate Supported Link Speeds Vector field
> bit 0, which is defined as 2.5 GT/s.

Yeah, this does not make sense now. I sort of assumed it was W1C
type thing (write-1-clear). I'm just wondering if this is now some
sort of undefined behaviour or they originally meant to clear out
some bits only... It does work, just shows the following from lspci;

  LnkCap: Port #0, Speed unknown, Width x8, ASPM L0s L1, Exit Latency 
L0s <4us, L1 <4us
          ClockPM- Surprise- LLActRep+ BwNot- ASPMOptComp+
  LnkCtl: ASPM Disabled; RCB 64 bytes, Disabled- CommClk+
         ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
   LnkSta: Speed 2.5GT/s (downgraded), Width x8 (ok)
         TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt-


>> +	dw_pcie_dbi_ro_wr_dis(dw);
>> +}
>> +
>>   static int fu740_pcie_start_link(struct dw_pcie *pci)
>>   {
>>   	struct device *dev = pci->dev;
>>   	struct fu740_pcie *afp = dev_get_drvdata(dev);
>>   
>> +	/* Force PCIe gen1 otherwise Unmatched board does not probe */
>> +	fu740_pcie_force_gen1(pci, afp);
> 
> I guess the "Unmatched" board is the only thing we need to care about
> here?  Are there or will there be other boards that don't need this?
> 
>>   	/* Enable LTSSM */
>>   	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
>>   	return 0;
>> -- 
>> 2.34.1
>>
> 


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe

[Ben Dooks]

On 23/02/2022 21:19, Maciej W. Rozycki wrote:
> On Wed, 23 Feb 2022, Bjorn Helgaas wrote:
> 
>>> +	dw_pcie_dbi_ro_wr_dis(dw);
>>> +}
>>> +
>>>   static int fu740_pcie_start_link(struct dw_pcie *pci)
>>>   {
>>>   	struct device *dev = pci->dev;
>>>   	struct fu740_pcie *afp = dev_get_drvdata(dev);
>>>   
>>> +	/* Force PCIe gen1 otherwise Unmatched board does not probe */
>>> +	fu740_pcie_force_gen1(pci, afp);
>>
>> I guess the "Unmatched" board is the only thing we need to care about
>> here?  Are there or will there be other boards that don't need this?
> 
>   I wonder if this is the other side of a supposed erratum observed here:
> 
> <https://lore.kernel.org/all/alpine.DEB.2.21.2202010240190.58572@angie.orcam.me.uk/>
> 
> Downstream there's the same ASMedia ASM2824 PCIe switch whose downstream
> ports don't want to train with a Pericom part above Gen1.
> 
>   Of course we don't know an ASM2824 is there until we have successfully
> negotiated the link, so we may have to generalise my proposal if we can
> find a way similar to what I have done for U-boot that does not disturb
> Linux's operation.  This is because there are PCIe option cards out there
> with the ASM2824 onboard, so it could be possible for the problem to
> trigger anywhere where the conditions for the erratum are met.
> 
>   Also in that case retraining should work with the cap removed to get a
> higher final speed just as with the Pericom part.

Possibly. I have just been working on a patch to better force Gen1
speeds and then restore the config which is working on my Unmatched
board.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe

[Ben Dooks]

On 23/02/2022 21:19, Maciej W. Rozycki wrote:
> On Wed, 23 Feb 2022, Bjorn Helgaas wrote:
> 
>>> +	dw_pcie_dbi_ro_wr_dis(dw);
>>> +}
>>> +
>>>   static int fu740_pcie_start_link(struct dw_pcie *pci)
>>>   {
>>>   	struct device *dev = pci->dev;
>>>   	struct fu740_pcie *afp = dev_get_drvdata(dev);
>>>   
>>> +	/* Force PCIe gen1 otherwise Unmatched board does not probe */
>>> +	fu740_pcie_force_gen1(pci, afp);
>>
>> I guess the "Unmatched" board is the only thing we need to care about
>> here?  Are there or will there be other boards that don't need this?
> 
>   I wonder if this is the other side of a supposed erratum observed here:
> 
> <https://lore.kernel.org/all/alpine.DEB.2.21.2202010240190.58572@angie.orcam.me.uk/>
> 
> Downstream there's the same ASMedia ASM2824 PCIe switch whose downstream
> ports don't want to train with a Pericom part above Gen1.
> 
>   Of course we don't know an ASM2824 is there until we have successfully
> negotiated the link, so we may have to generalise my proposal if we can
> find a way similar to what I have done for U-boot that does not disturb
> Linux's operation.  This is because there are PCIe option cards out there
> with the ASM2824 onboard, so it could be possible for the problem to
> trigger anywhere where the conditions for the erratum are met.
> 
>   Also in that case retraining should work with the cap removed to get a
> higher final speed just as with the Pericom part.

Possibly. I have just been working on a patch to better force Gen1
speeds and then restore the config which is working on my Unmatched
board.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv