| Message ID | 20201112131400.3775119-1-phil@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: brcmstb: Restore initial fundamental reset | expand |
On Thu, 2020-11-12 at 13:14 +0000, Phil Elwell wrote: > Commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") > replaced a single reset function with a pointer to one of two > implementations, but also removed the call asserting the reset > at the start of brcm_pcie_setup. Doing so breaks Raspberry Pis with > VL805 XHCI controllers lacking dedicated SPI EEPROMs, which have been > used for USB booting but then need to be reset so that the kernel > can reconfigure them. The lack of a reset causes the firmware's loading > of the EEPROM image to RAM to fail, breaking USB for the kernel. > > Fixes: commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") > > Signed-off-by: Phil Elwell <phil@raspberrypi.com> > --- Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> Thanks! > drivers/pci/controller/pcie-brcmstb.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index bea86899bd5d..a90d6f69c5a1 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -869,6 +869,8 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > > /* Reset the bridge */ > pcie->bridge_sw_init_set(pcie, 1); > + pcie->perst_set(pcie, 1); > + > usleep_range(100, 200); > > /* Take the bridge out of reset */
+JimQ, On 11/12/2020 5:14 AM, Phil Elwell wrote: > Commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") > replaced a single reset function with a pointer to one of two > implementations, but also removed the call asserting the reset > at the start of brcm_pcie_setup. Doing so breaks Raspberry Pis with > VL805 XHCI controllers lacking dedicated SPI EEPROMs, which have been > used for USB booting but then need to be reset so that the kernel > can reconfigure them. The lack of a reset causes the firmware's loading > of the EEPROM image to RAM to fail, breaking USB for the kernel. > > Fixes: commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") > > Signed-off-by: Phil Elwell <phil@raspberrypi.com> This does indeed seem to have been lost during that commit, I will let JimQ comment on whether this was intentional or not. Please make sure you copy him, always, he wrote the driver after all. > --- > drivers/pci/controller/pcie-brcmstb.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index bea86899bd5d..a90d6f69c5a1 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -869,6 +869,8 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > > /* Reset the bridge */ > pcie->bridge_sw_init_set(pcie, 1); > + pcie->perst_set(pcie, 1); > + > usleep_range(100, 200); > > /* Take the bridge out of reset */ >
On Thu, Nov 12, 2020 at 10:44 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > > +JimQ, > > On 11/12/2020 5:14 AM, Phil Elwell wrote: > > Commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") > > replaced a single reset function with a pointer to one of two > > implementations, but also removed the call asserting the reset > > at the start of brcm_pcie_setup. Doing so breaks Raspberry Pis with > > VL805 XHCI controllers lacking dedicated SPI EEPROMs, which have been > > used for USB booting but then need to be reset so that the kernel > > can reconfigure them. The lack of a reset causes the firmware's loading > > of the EEPROM image to RAM to fail, breaking USB for the kernel. > > > > Fixes: commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") > > > > Signed-off-by: Phil Elwell <phil@raspberrypi.com> > > This does indeed seem to have been lost during that commit, I will let > JimQ comment on whether this was intentional or not. Please make sure > you copy him, always, he wrote the driver after all. Hello, This wasn't accidentally lost; I intentionally removed it. I was remiss in not mentioning this in comments, sorry. The reason I took it out is because (a) it breaks certain STB SOCs and (b) I considered it superfluous (see reason below). At any rate, if you must restore this line please add the following guard so everyone's board will work :-) if (pcie->type != BCM7278) brcm_pcie_perst_set(pcie, 1); As for me considering that this line is superfluous -- which apparently it is not : AFAIK PERST# is always asserted from cold start on all Brcm STB SOCs, and I expected the same on the RPi. Asserting PERST# at this point in time should be a no-op. Is this not the case? Thanks, Jim Quinlan Broadcom STB > > > --- > > drivers/pci/controller/pcie-brcmstb.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index bea86899bd5d..a90d6f69c5a1 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > @@ -869,6 +869,8 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > > > > /* Reset the bridge */ > > pcie->bridge_sw_init_set(pcie, 1); > > + pcie->perst_set(pcie, 1); > > + > > usleep_range(100, 200); > > > > /* Take the bridge out of reset */ > > > > -- > Florian
Hi Jim, On Thu, 12 Nov 2020 at 16:28, Jim Quinlan <james.quinlan@broadcom.com> wrote: > > On Thu, Nov 12, 2020 at 10:44 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > +JimQ, > > > > On 11/12/2020 5:14 AM, Phil Elwell wrote: > > > Commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") > > > replaced a single reset function with a pointer to one of two > > > implementations, but also removed the call asserting the reset > > > at the start of brcm_pcie_setup. Doing so breaks Raspberry Pis with > > > VL805 XHCI controllers lacking dedicated SPI EEPROMs, which have been > > > used for USB booting but then need to be reset so that the kernel > > > can reconfigure them. The lack of a reset causes the firmware's loading > > > of the EEPROM image to RAM to fail, breaking USB for the kernel. > > > > > > Fixes: commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") > > > > > > Signed-off-by: Phil Elwell <phil@raspberrypi.com> > > > > This does indeed seem to have been lost during that commit, I will let > > JimQ comment on whether this was intentional or not. Please make sure > > you copy him, always, he wrote the driver after all. > Hello, > > This wasn't accidentally lost; I intentionally removed it. I was > remiss in not mentioning this in comments, sorry. Yes, a comment would have been helpful. > The reason I took it out is because (a) it breaks certain STB SOCs and > (b) I considered it superfluous (see reason below). At any rate, if > you must restore this line please add the following guard so > everyone's board will work :-) > > if (pcie->type != BCM7278) > brcm_pcie_perst_set(pcie, 1); > > > > As for me considering that this line is superfluous -- which > apparently it is not : AFAIK PERST# is always asserted from cold start > on all Brcm STB SOCs, and I expected the same on the RPi. Asserting > PERST# at this point in time should be a no-op. Is this not the case? The reason it isn't superfluous here is that when using USB to boot, the Raspberry Pi BCM2711 firmware will already have configured the PCIe bus once, so another reset is necessary. Since the Linux driver used to always reset everything at start of day, regardless of the power-on reset state, there's no reason for the firmware to also reset it before handing over - and there could be some useful state in there when it comes to debugging. I'm happy to add a condition - not a BCM7278 sounds reasonable - and a comment, of course. Phil
On Thu, Nov 12, 2020 at 04:42:28PM +0000, Phil Elwell wrote: > On Thu, 12 Nov 2020 at 16:28, Jim Quinlan <james.quinlan@broadcom.com> wrote: > > As for me considering that this line is superfluous -- which > > apparently it is not : AFAIK PERST# is always asserted from cold start > > on all Brcm STB SOCs, and I expected the same on the RPi. Asserting > > PERST# at this point in time should be a no-op. Is this not the case? > > The reason it isn't superfluous here is that when using USB to boot, > the Raspberry Pi BCM2711 firmware will already have configured the > PCIe bus once, so another reset is necessary. I think that begs the question why the firmware doesn't reset the PCIe bus before handing over control to the kernel? Thanks, Lukas
On Thu, 12 Nov 2020 at 16:47, Lukas Wunner <lukas@wunner.de> wrote: > > On Thu, Nov 12, 2020 at 04:42:28PM +0000, Phil Elwell wrote: > > On Thu, 12 Nov 2020 at 16:28, Jim Quinlan <james.quinlan@broadcom.com> wrote: > > > As for me considering that this line is superfluous -- which > > > apparently it is not : AFAIK PERST# is always asserted from cold start > > > on all Brcm STB SOCs, and I expected the same on the RPi. Asserting > > > PERST# at this point in time should be a no-op. Is this not the case? > > > > The reason it isn't superfluous here is that when using USB to boot, > > the Raspberry Pi BCM2711 firmware will already have configured the > > PCIe bus once, so another reset is necessary. > > I think that begs the question why the firmware doesn't reset the > PCIe bus before handing over control to the kernel? Are you advocating removing all resets that merely reapply the power-on reset state? Phil
On Thu, Nov 12, 2020 at 11:42 AM Phil Elwell <phil@raspberrypi.com> wrote: > > Hi Jim, > > On Thu, 12 Nov 2020 at 16:28, Jim Quinlan <james.quinlan@broadcom.com> wrote: > > > > On Thu, Nov 12, 2020 at 10:44 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > > > +JimQ, > > > > > > On 11/12/2020 5:14 AM, Phil Elwell wrote: > > > > Commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") > > > > replaced a single reset function with a pointer to one of two > > > > implementations, but also removed the call asserting the reset > > > > at the start of brcm_pcie_setup. Doing so breaks Raspberry Pis with > > > > VL805 XHCI controllers lacking dedicated SPI EEPROMs, which have been > > > > used for USB booting but then need to be reset so that the kernel > > > > can reconfigure them. The lack of a reset causes the firmware's loading > > > > of the EEPROM image to RAM to fail, breaking USB for the kernel. > > > > > > > > Fixes: commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") > > > > > > > > Signed-off-by: Phil Elwell <phil@raspberrypi.com> > > > > > > This does indeed seem to have been lost during that commit, I will let > > > JimQ comment on whether this was intentional or not. Please make sure > > > you copy him, always, he wrote the driver after all. > > Hello, > > > > This wasn't accidentally lost; I intentionally removed it. I was > > remiss in not mentioning this in comments, sorry. > > > Yes, a comment would have been helpful. > > > The reason I took it out is because (a) it breaks certain STB SOCs and > > (b) I considered it superfluous (see reason below). At any rate, if > > you must restore this line please add the following guard so > > everyone's board will work :-) > > > > if (pcie->type != BCM7278) > > brcm_pcie_perst_set(pcie, 1); > > > > > > > > As for me considering that this line is superfluous -- which > > apparently it is not : AFAIK PERST# is always asserted from cold start > > on all Brcm STB SOCs, and I expected the same on the RPi. Asserting > > PERST# at this point in time should be a no-op. Is this not the case? > > > The reason it isn't superfluous here is that when using USB to boot, > the Raspberry Pi BCM2711 firmware will already have configured the > PCIe bus once, so another reset is necessary. Since the Linux driver > used to always reset everything at start of day, Hello, This is not necessarily true -- this line was added separately with commit 22e21e51ce755399fd42055a3f668ee4af370881. IIRC, at the time we could not find an example where it mattered. My feedback was along the lines that "it cannot hurt" but as it turns out later, it did. > regardless of the > power-on reset state, there's no reason for the firmware to also reset > it before handing over - and there could be some useful state in there > when it comes to debugging. Sure there's a reason -- the PCIe HW should be returned to the pristine state from which it was found, especially if you expect it to be used again. This is what the linux kernel driver does on suspend/remove/power-off. I think debuggability is not a compelling reason as it can be achieved with a debug-enabled FW image. Regards, Jim, > > I'm happy to add a condition - not a BCM7278 sounds reasonable - and a > comment, of course. > > Phil
Hi Jim, On Thu, 2020-11-12 at 11:28 -0500, Jim Quinlan wrote: > On Thu, Nov 12, 2020 at 10:44 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > > +JimQ, > > > > On 11/12/2020 5:14 AM, Phil Elwell wrote: > > > Commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") > > > replaced a single reset function with a pointer to one of two > > > implementations, but also removed the call asserting the reset > > > at the start of brcm_pcie_setup. Doing so breaks Raspberry Pis with > > > VL805 XHCI controllers lacking dedicated SPI EEPROMs, which have been > > > used for USB booting but then need to be reset so that the kernel > > > can reconfigure them. The lack of a reset causes the firmware's loading > > > of the EEPROM image to RAM to fail, breaking USB for the kernel. > > > > > > Fixes: commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") > > > > > > Signed-off-by: Phil Elwell <phil@raspberrypi.com> > > > > This does indeed seem to have been lost during that commit, I will let > > JimQ comment on whether this was intentional or not. Please make sure > > you copy him, always, he wrote the driver after all. > Hello, > > This wasn't accidentally lost; I intentionally removed it. I was > remiss in not mentioning this in comments, sorry. > > The reason I took it out is because (a) it breaks certain STB SOCs and > (b) I considered it superfluous (see reason below). At any rate, if > you must restore this line please add the following guard so > everyone's board will work :-) > > if (pcie->type != BCM7278) > brcm_pcie_perst_set(pcie, 1); This seems reasonable to me. > As for me considering that this line is superfluous -- which > apparently it is not : AFAIK PERST# is always asserted from cold start > on all Brcm STB SOCs, and I expected the same on the RPi. Asserting > PERST# at this point in time should be a no-op. Is this not the case? I introduced this with 22e21e51ce7 ("PCI: brcmstb: Assert fundamental reset on initialization"). IIRC The story is the following: - RPI's XHCI chip, which is connected to pcie-brcmstb, depends on a firmware blob. The blob is copied into memory, then the chip can access it anytime through PCIe inbound transfers. - At any boot stage the pcie-brcmstb might be reconfigured with different inbound windows. This could be in RPi's custom bootloader, U-boot or Linux (or anything else). - The only way we have to reset the XHCI chip so as for it to catch the new firmware blob addresses is through this PERST# line. Hence the need to toggle it every time the controller has been reconfigured. Hope it makes some sense. :) Regards, Nicolas
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index bea86899bd5d..a90d6f69c5a1 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -869,6 +869,8 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) /* Reset the bridge */ pcie->bridge_sw_init_set(pcie, 1); + pcie->perst_set(pcie, 1); + usleep_range(100, 200); /* Take the bridge out of reset */
Commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") replaced a single reset function with a pointer to one of two implementations, but also removed the call asserting the reset at the start of brcm_pcie_setup. Doing so breaks Raspberry Pis with VL805 XHCI controllers lacking dedicated SPI EEPROMs, which have been used for USB booting but then need to be reset so that the kernel can reconfigure them. The lack of a reset causes the firmware's loading of the EEPROM image to RAM to fail, breaking USB for the kernel. Fixes: commit 04356ac30771 ("PCI: brcmstb: Add bcm7278 PERST# support") Signed-off-by: Phil Elwell <phil@raspberrypi.com> --- drivers/pci/controller/pcie-brcmstb.c | 2 ++ 1 file changed, 2 insertions(+)