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