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

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(+)

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
>

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