[PATCH] PCI: brcmstb: Restore initial fundamental reset

[PATCH] PCI: brcmstb: Restore initial fundamental reset

[Phil Elwell]

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

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 */
-- 
2.25.1

Re: [PATCH] PCI: brcmstb: Restore initial fundamental reset

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

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 */


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] PCI: brcmstb: Restore initial fundamental reset

[Florian Fainelli]

+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 */
> 

-- 
Florian

Re: [PATCH] PCI: brcmstb: Restore initial fundamental reset

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 2430 bytes --]

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

Re: [PATCH] PCI: brcmstb: Restore initial fundamental reset

[Phil Elwell]

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

Re: [PATCH] PCI: brcmstb: Restore initial fundamental reset

[Lukas Wunner]

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

Re: [PATCH] PCI: brcmstb: Restore initial fundamental reset

[Phil Elwell]

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

Re: [PATCH] PCI: brcmstb: Restore initial fundamental reset

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 3320 bytes --]

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

Re: [PATCH] PCI: brcmstb: Restore initial fundamental reset

[Nicolas Saenz Julienne]

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