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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>