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

[Jim Quinlan <jim2101024@gmail.com>]

On Wed, May 25, 2022 at 3:21 AM Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
> Hi Jim,
>
> Am 24.05.22 um 18:54 schrieb Jim Quinlan:
> > On Mon, May 23, 2022 at 6:10 PM Bjorn Helgaas <helgaas@kernel.org> 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@kernel.org> 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.
Okay, good to know.

> >
> >> 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 ;-)
Perhaps I complain too much :-)

Cheers,
Jim Quinlan
Broadcom STB
>
> Best regards
>