Hi Jim,
On Thu, 12 Nov 2020 at 16:28, Jim Quinlan <james.quinlan@xxxxxxxxxxxx> wrote:
>
> On Thu, Nov 12, 2020 at 10:44 AM Florian Fainelli <f.fainelli@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> >
> > 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
