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(-)

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;
 }
 
-- 
2.35.3

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

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