Messages in this thread Patch in this message | | | Date | Mon, 28 Feb 2022 17:45:49 +0000 | Subject | Re: [PATCHv4 2/2] PCI: fu740: Force gen1 for initial device probe | From | 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
| |