diff mbox series

[v2] PCI: brcmstb: Restore initial fundamental reset

Message ID 20201112172709.1817-1-phil@raspberrypi.com (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series [v2] PCI: brcmstb: Restore initial fundamental reset | expand

Commit Message

Phil Elwell Nov. 12, 2020, 5:27 p.m. UTC
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>
---
Changes in v2:
  - Exclude BCM7278 from the initial reset
  - Ack from Nicolas
---
 drivers/pci/controller/pcie-brcmstb.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Stefan Wahren Nov. 12, 2020, 6:05 p.m. UTC | #1
Hi,

Am 12.11.20 um 18:27 schrieb 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>
> Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
> Changes in v2:
>   - Exclude BCM7278 from the initial reset
>   - Ack from Nicolas
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index bea86899bd5d..83aa85bfe8e3 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -869,6 +869,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  
>  	/* Reset the bridge */
>  	pcie->bridge_sw_init_set(pcie, 1);
> +
> +	/* Assert the fundemental reset, except on BCM7278 */
> +	if (pcie->type != BCM7278)
> +		pcie->perst_set(pcie, 1);

since only the Raspberry Pi 4 requires the reset i had changed the logic
to pcie->type == BCM2711. But this isn't totally clean, since we need
this quirk only for the Raspberry Pi 4 and not for CM4 (which also has a
BCM2711 SoC). So maybe we should introduce BCM2711_RPI4 to be on the
safe side. Sorry for the bikeshedding.

Stefan

> +
>  	usleep_range(100, 200);
>  
>  	/* Take the bridge out of reset */
Jim Quinlan Nov. 12, 2020, 6:38 p.m. UTC | #2
On Thu, Nov 12, 2020 at 12:27 PM Phil Elwell <phil@raspberrypi.com> 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>
> ---
> Changes in v2:
>   - Exclude BCM7278 from the initial reset
>   - Ack from Nicolas
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index bea86899bd5d..83aa85bfe8e3 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -869,6 +869,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>
>         /* Reset the bridge */
>         pcie->bridge_sw_init_set(pcie, 1);
> +
> +       /* Assert the fundemental reset, except on BCM7278 */
> +       if (pcie->type != BCM7278)
> +               pcie->perst_set(pcie, 1);
I'm okay with this although I  would rather it not be needed.
Regards,
Jim
> +
>         usleep_range(100, 200);
>
>         /* Take the bridge out of reset */
> --
> 2.25.1
>
Lorenzo Pieralisi Jan. 18, 2021, 12:40 p.m. UTC | #3
On Thu, Nov 12, 2020 at 01:38:13PM -0500, Jim Quinlan wrote:
> On Thu, Nov 12, 2020 at 12:27 PM Phil Elwell <phil@raspberrypi.com> 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>
> > ---
> > Changes in v2:
> >   - Exclude BCM7278 from the initial reset
> >   - Ack from Nicolas
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index bea86899bd5d..83aa85bfe8e3 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -869,6 +869,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> >
> >         /* Reset the bridge */
> >         pcie->bridge_sw_init_set(pcie, 1);
> > +
> > +       /* Assert the fundemental reset, except on BCM7278 */
> > +       if (pcie->type != BCM7278)
> > +               pcie->perst_set(pcie, 1);
> I'm okay with this although I  would rather it not be needed.

Can I merge this patch as is then ?

Thanks,
Lorenzo

> Regards,
> Jim
> > +
> >         usleep_range(100, 200);
> >
> >         /* Take the bridge out of reset */
> > --
> > 2.25.1
> >
Nicolas Saenz Julienne Jan. 18, 2021, 12:44 p.m. UTC | #4
On Mon, 2021-01-18 at 12:40 +0000, Lorenzo Pieralisi wrote:
> On Thu, Nov 12, 2020 at 01:38:13PM -0500, Jim Quinlan wrote:
> > On Thu, Nov 12, 2020 at 12:27 PM Phil Elwell <phil@raspberrypi.com> 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>
> > > ---
> > > Changes in v2:
> > >   - Exclude BCM7278 from the initial reset
> > >   - Ack from Nicolas
> > > ---
> > >  drivers/pci/controller/pcie-brcmstb.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > index bea86899bd5d..83aa85bfe8e3 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -869,6 +869,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > > 
> > >         /* Reset the bridge */
> > >         pcie->bridge_sw_init_set(pcie, 1);
> > > +
> > > +       /* Assert the fundemental reset, except on BCM7278 */
> > > +       if (pcie->type != BCM7278)
> > > +               pcie->perst_set(pcie, 1);
> > I'm okay with this although I  would rather it not be needed.
> 
> Can I merge this patch as is then ?

No. IIUC the consensus was to fix this in firmware. There is a u-boot fix
available in their mailing list, and I think RPi's firmware was also patched
accordingly (@Phil please confirm).

Regards,
Nicolas
Phil Elwell Jan. 18, 2021, 1:33 p.m. UTC | #5
On Mon, 18 Jan 2021 at 12:44, Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> On Mon, 2021-01-18 at 12:40 +0000, Lorenzo Pieralisi wrote:
> > On Thu, Nov 12, 2020 at 01:38:13PM -0500, Jim Quinlan wrote:
> > > On Thu, Nov 12, 2020 at 12:27 PM Phil Elwell <phil@raspberrypi.com> 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>
> > > > ---
> > > > Changes in v2:
> > > >   - Exclude BCM7278 from the initial reset
> > > >   - Ack from Nicolas
> > > > ---
> > > >  drivers/pci/controller/pcie-brcmstb.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > > index bea86899bd5d..83aa85bfe8e3 100644
> > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > @@ -869,6 +869,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> > > >
> > > >         /* Reset the bridge */
> > > >         pcie->bridge_sw_init_set(pcie, 1);
> > > > +
> > > > +       /* Assert the fundemental reset, except on BCM7278 */
> > > > +       if (pcie->type != BCM7278)
> > > > +               pcie->perst_set(pcie, 1);
> > > I'm okay with this although I  would rather it not be needed.
> >
> > Can I merge this patch as is then ?
>
> No. IIUC the consensus was to fix this in firmware. There is a u-boot fix
> available in their mailing list, and I think RPi's firmware was also patched
> accordingly (@Phil please confirm).

Any users running firmware released since mid-November should not
require the precautionary reset in order to have functioning USB.
Having said that, we'll be keeping this patch in our repo for the
benefit of everybody else.

Phil
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index bea86899bd5d..83aa85bfe8e3 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -869,6 +869,11 @@  static int brcm_pcie_setup(struct brcm_pcie *pcie)
 
 	/* Reset the bridge */
 	pcie->bridge_sw_init_set(pcie, 1);
+
+	/* Assert the fundemental reset, except on BCM7278 */
+	if (pcie->type != BCM7278)
+		pcie->perst_set(pcie, 1);
+
 	usleep_range(100, 200);
 
 	/* Take the bridge out of reset */