[PATCH] PCI: brcmstb: Restore initial fundamental reset

[Jim Quinlan]

On Thu, Nov 12, 2020 at 11:42 AM Phil Elwell <phil at raspberrypi.com> wrote:
>
> Hi Jim,
>
> On Thu, 12 Nov 2020 at 16:28, Jim Quinlan <james.quinlan at broadcom.com> wrote:
> >
> > On Thu, Nov 12, 2020 at 10:44 AM Florian Fainelli <f.fainelli at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4167 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.infradead.org/pipermail/linux-rpi-kernel/attachments/20201112/a6426f3a/attachment.p7s>