| Message ID | 20220221210347.1335004-2-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv4,1/2] PCI: fu740: fix finding GPIOs | expand |
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
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 >> >
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.
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;
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(+)