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@xxxxxxxxxxxxxxx> > --- > 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 >
