Re: [PATCH v1] PCI: brcmstb: Fix regression regarding missing PCIe linkup

[Stefan Wahren]

Hi Jim,

Am 24.05.22 um 18:54 schrieb Jim Quinlan:
> On Mon, May 23, 2022 at 6:10 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > On Sat, May 21, 2022 at 02:51:42PM -0400, Jim Quinlan wrote:
> > > On Sat, May 21,
> > > 2CONFIG_INITRAMFS_SOURCE="/work3/jq921458/cpio/54-arm64-rootfs.cpio022
> > > at 12:43 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > > On Wed, May 18, 2022 at 03:42:11PM -0400, Jim Quinlan wrote:
> > > > > commit 93e41f3fca3d ("PCI: brcmstb: Add control of subdevice
> > > > > voltage regulators")
> > > > >
> > > > > introduced a regression on the PCIe RPi4 Compute Module.  If the
> > > > > PCIe endpoint node described in [2] was missing, no linkup would
> > > > > be attempted, and subsequent accesses would cause a panic
> > > > > because this particular PCIe HW causes a CPU abort on illegal
> > > > > accesses (instead of returning 0xffffffff).
> > > > >
> > > > > We fix this by allowing the DT endpoint subnode to be missing.
> > > > > This is important for platforms like the CM4 which have a
> > > > > standard PCIe socket and the endpoint device is unknown.
> > > > I think the problem here is that on the CM, we try to enumerate
> > > > devices that are not powered up, isn't it?  The commit log should
> > > > say something about that power situation and how the driver learns
> > > > about the power regulators instead of just pointing at an DT
> > > > endpoint node.
> > > This is incorrect.  The regression occurred because the code
> > > mistakenly skips PCIe-linkup if the PCI portdrv DT node does not
> > > exist. With our RC HW, doing a config space access to bus 1 w/o
> > > first linking up results in a CPU abort.  This regression has
> > > nothing to do with EP power at all.
> > OK, I think I'm starting to see, but I'm still missing some things.
> >
> > 67211aadcb4b ("PCI: brcmstb: Add mechanism to turn on subdev
> > regulators") added pci_subdev_regulators_add_bus() as an .add_bus()
> > method.  This is called by pci_alloc_child_bus(), and if the DT
> > describes any regulators for the bridge leading to the new child bus,
> > we turn them on.
> >
> > Then 93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage
> > regulators") added brcm_pcie_add_bus() and made *it* the .add_bus()
> > method.  It turns on the regulators and brings the link up, but it
> > skips both if there's no DT node for the bridge to the new bus.
> Hi Bjorn,
>
> Yes, I meant it to skip the turning on of the regulators if the DT
> node was missing
> but I failed to notice that it would also skip the pcie linkup as well.  As you
> may have guessed, all of my test systems have the PCIe root port
> DT node.
>
> > I guess RPi4 CM has no DT node to describe regulators, so we skip both
> > turning them on *and* bringing the link up?
> Yes. One repo did not have this node (Cyril/debina?), one did
> (https://github.com/raspberrypi/firmware/tree/master/boot).
> Of course there is nothing wrong with omitting the node; it should
> have pcie linkup regardless.
Please ignore the vendor tree, because you only have to care about 
mainline kernel and DT here.
> > But above you say it's the *endpoint* node that doesn't exist.  The
> > existing code looks like it's checking for the *bridge* node
> > (bus->dev->of_node).  We haven't even enumerated the devices on the
> > child bus, so we don't know about them at this point.
> You are absolutely correct and I must change the commit message
> to say the "root port DT node".   I'm sorry; this mistake likely did not
> help you understand the fix. :-(
>
> > What happens if there is a DT node for the bridge, but it doesn't
> > describe any regulators?  I assume regulator_bulk_get() will fail, and
> > it looks like that might still keep us from bringing the link up?
> The regulator_bulk_get()  func does not fail if the regulators are not
> present.  Instead it "gets"
> a dummy device and issues a warning per missing regulator.
> A version of my pullreq submitted code to prescan the DT node and call
> regulator_bulk_get() with
> only the names of the regulators present, but IIRC this was NAKd.
> Hopefully I will not be swamped with RPi developers'  emails when they
> think these warnings are an issue.

This won't be the first driver complaining about missing regulators and 
won't be the last one. So don't expect an email from me ;-)

Best regards