| Message ID | 20240910151845.17308-6-svarbanov@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | Add PCIe support for bcm2712 | expand |
On 9/10/24 08:18, Stanimir Varbanov wrote: > RootCtl bits might reset by perst_n during probe, re-enable > CRS SVE here in pcie_start_link. > > Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> This looks like a bug fix, and we should explain what is the user visible effect of that, if any.
Hi Florian, On 9/10/24 19:59, Florian Fainelli wrote: > On 9/10/24 08:18, Stanimir Varbanov wrote: >> RootCtl bits might reset by perst_n during probe, re-enable >> CRS SVE here in pcie_start_link. >> >> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> > > This looks like a bug fix, and we should explain what is the user > visible effect of that, if any. It is definitely a bugfix. Otherwise, CRS Software Visibility is important feature from pcie1.1. Not enabling it on Root Port could lead to infinite configuration retry cycles when enumerate endpoints which supports CRS. For more information [1] and [2]. I spent some time debugging it and found that this is not the proper solution. I think the issue comes from wrongly implemented .add_bus pci_ops. Looks like .add_bus op shouldn't call brcm_pcie_start_link() but invoke before pci_host_probe(), then the issue will fix by itself. What I observed is that pci_enable_crs() is setting CSR Software Visibility Enable bit but the controller is ignoring it without error (reading the Root Control register returns zero). This means that the controller is not ready to accept configuration write requests at that time, that's why I tried the following diff which seems to work: static struct pci_ops brcm_pcie_ops = { .map_bus = brcm_pcie_map_bus, .read = pci_generic_config_read, .write = pci_generic_config_write, - .add_bus = brcm_pcie_add_bus, - .remove_bus = brcm_pcie_remove_bus, }; static struct pci_ops brcm7425_pcie_ops = { @@ -1983,6 +2018,9 @@ static int brcm_pcie_probe(struct platform_device *pdev) platform_set_drvdata(pdev, pcie); + //TODO: check for error + brcm_pcie_start_link(pcie); + ret = pci_host_probe(bridge); if (!ret && !brcm_pcie_link_up(pcie)) ret = -ENODEV; Of course this change would work on RPi5 because there are no regulators. I will drop the patch from the series for now and work on a proper solution. regards, ~Stan [1] https://patchwork.kernel.org/project/linux-pci/patch/53FFA54D.9000907@gmail.com/ [2] https://blog.linuxplumbersconf.org/2017/ocw/system/presentations/4732/original/crs.pdf
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 7bd85566c242..f2a7a8e93a74 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -1271,7 +1271,7 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie) { struct device *dev = pcie->dev; void __iomem *base = pcie->base; - u16 nlw, cls, lnksta; + u16 nlw, cls, lnksta, tmp16; bool ssc_good = false; int ret, i; @@ -1319,6 +1319,17 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie) pci_speed_string(pcie_link_speed[cls]), nlw, ssc_good ? "(SSC)" : "(!SSC)"); + /* + * RootCtl bits are reset by perst_n, which undoes pci_enable_crs() + * called prior to pci_add_new_bus() during probe. Re-enable here. + */ + tmp16 = readw(base + BRCM_PCIE_CAP_REGS + PCI_EXP_RTCAP); + if (tmp16 & PCI_EXP_RTCAP_CRSVIS) { + tmp16 = readw(base + BRCM_PCIE_CAP_REGS + PCI_EXP_RTCTL); + u16p_replace_bits(&tmp16, 1, PCI_EXP_RTCTL_CRSSVE); + writew(tmp16, base + BRCM_PCIE_CAP_REGS + PCI_EXP_RTCTL); + } + return 0; }
RootCtl bits might reset by perst_n during probe, re-enable CRS SVE here in pcie_start_link. Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> --- drivers/pci/controller/pcie-brcmstb.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)