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.
>
> 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.
>
> I would think that lack of regulator description in the DT would mean
> that any regulators are always on and the OS doesn't need to do
> anything.pci_subdev_regulators_remove_bus
I agree.
>
> > > What happens if we turn on the power but don't find any
> > > downstream devices?
> >
> > They are turned off to conserve power.pci_subdev_regulators_remove_bus
> >
> > > From looking at the code, I assume we just leave the power on.
> > > Maybe that's what you want, I dunno.
>
> > For STB and Cable Modem products we do not leave the power on. In
> > fact, our Cable Modem group was the first to request this feature.
> > It appears that the RPi CM4 always keeps endpoint power on but I do
> > not know for sure.
>
> I'm confused. Why can't we tell by looking at pcie-brcmstb.c? All I
> know is what's in pcie-brcmstb.c; I have no idea which things apply to
> which products.
I was just adding background information but I see that I really
didn't answer your
question. Allow me another chance:
When brcm_pcie_add_bus() is invoked, we will "get" and enable any
regulators that are present in the DT node. If the busno==1, we will
will also attempt pcie-linkup. If PCIe linkup fails, which can happen for
multiple reasons but most due to a missing device, we turn
on "refusal" mode to prevent our unforgiving PCIe HW from causing an
abort on any subsequent PCIe config-space accesses. Further,
a failed linkup will have brcm_pcie_probe() stopping and removing
the root bus, which in turn invokes brcm_pcie_remove_bus() (actually
named pci_subdev_regulators_remove_bus() as it may someday find its
way into bus.c), which invokes regulator_bulk_disable() on any
regulators that were enabled by the probe.
>
> The only calls to regulator_bulk_disable() are in
> pci_subdev_regulators_remove_bus(), brcm_pcie_suspend(), and
> brcm_pcie_resume(). I don't think the fact that enumeration didn't
> find any devices necessarily leads to any of those. What am I
> missing? (This is really a tangent that isn't critical for fixing the
> regression.)
If there was no linkup during the probe, the probe follows this
path before it returns with error:
brcm_pcie_probe()
brcm_pcie_remove()
pci_stop_root_bus();
pci_remove_root_bus();
pci_remove_bus_device()
pci_remove_bus_device()
pci_subdev_regulators_remove_bus()
regulator_bulk_disable()
>
> > > I added Rafael because this seems vaguely similar to runtime power
> > > management, and if we can integrate with that somehow, I'd sure like
> > > to avoid building a parallel infrastructure for it.
> > >
> > > The current path we're on is to move some of this code that's
> > > currently in pcie-brcmstb.c to the PCIe portdrv [0]. I'm a little
> > > hesitant about that because ACPI does just fine without it. If we're
> > > adding new DT functionality that could not be implemented via ACPI,
> > > that's one thing. But I'm not convinced this is that new.
> >
> > AFAICT, Broadcom STB and Cable Modem products do not have/use/want
> > ACPI. We are fine with keeping this "PCIe regulator" feature
> > private to our driver and giving you speedy and full support in
> > maintaining it.
>
> I don't mean that you should use ACPI, only that ACPI platforms can do
> this sort of power control using the existing PCI core infrastructure,
> and maybe there's a way for OF/DT platforms to hook into that same
> infrastructure to minimize the driver-specific work. E.g., maybe
> there's a way to extend platform_pci_set_power_state() and similar to
> manage these regulators.
Got it.
Unless you object, I plan on sending you a v2 of my regression fix which will
correct the commit message, change the "if (busno == 1)" conditional to
only guard the pcie linkup call, and add further comments.
I have noted and will also address your other concerns and suggestions
in a future
patchset as I think it is best that I get my hands on a CM4 board
before I submit
any more changes.
Kind Regards,
Jim Quinlan
Broadcom STB
>
> > > [0] https://lore.kernel.org/r/20211110221456.11977-6-jim2101024@xxxxxxxxx
> > >
> > > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=215925
> > > > [2] Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > >
> > > > Fixes: 93e41f3fca3d ("PCI: brcmstb: Add control of subdevice voltage regulators")
> > > > Fixes: 830aa6f29f07 ("PCI: brcmstb: Split brcm_pcie_setup() into two funcs")
> > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215925
> > > > Signed-off-by: Jim Quinlan <jim2101024@xxxxxxxxx>
> > > > ---
> > > > drivers/pci/controller/pcie-brcmstb.c | 8 +++++---
> > > > 1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > > index ba5c120816b2..adca74e235cb 100644
> > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > @@ -540,16 +540,18 @@ static int pci_subdev_regulators_add_bus(struct pci_bus *bus)
> > > >
> > > > static int brcm_pcie_add_bus(struct pci_bus *bus)
> > > > {
> > > > - struct device *dev = &bus->dev;
> > > > struct brcm_pcie *pcie = (struct brcm_pcie *) bus->sysdata;
> > > > int ret;
> > > >
> > > > - if (!dev->of_node || !bus->parent || !pci_is_root_bus(bus->parent))
> > > > + /* Only busno==1 requires us to linkup */
> > > > + if ((int)bus->number != 1)
> > > > return 0;
> > > >
> > > > ret = pci_subdev_regulators_add_bus(bus);
> > > > - if (ret)
> > > > + if (ret) {
> > > > + pcie->refusal_mode = true;
> > > > return ret;
> > > > + }
> > > >
> > > > /* Grab the regulators for suspend/resume */
> > > > pcie->sr = bus->dev.driver_data;
> > >
> > > IIUC, this path:
> > >
> > > pci_alloc_child_bus
> > > brcm_pcie_add_bus # .add_bus method
> > > pci_subdev_regulators_add_bus # in pcie-brcmstb.c for now
> > > alloc_subdev_regulators # in pcie-brcmstb.c for now
> > > regulator_bulk_get
> > > regulator_bulk_enable
> > > brcm_pcie_linkup # bring link up
> > >
> > > is basically so we can leave power to downstream devices off, then
> > > turn it on when we're ready to enumerate those downstream devices.
> >
> > Yes -- it is the "chicken-and-egg" problem. Ideally, we would like
> > for the endpoint driver to turn on its own regulators, but even to
> > know which endpoint driver to probe we must turn on the regulator to
> > establish linkup.
>
> I don't think having an endpoint driver turn on power to its device is
> the right goal. As you said, if the power is off, we don't know
> whether there's an endpoint or what it is, so the driver isn't in the
> picture (I know sometimes endpoints are described in DT, and that
> might be needed for non-discoverable properties, but I don't think
> it's a good way to *enumerate* the device).
>
> I don't know much about ACPI power management, but I kind of think it
> turns on power to *everything* initially so we can enumerate all the
> devices (Rafael or others, please correct me!) After enumeration, we
> can turn off devices we don't need, and the power management framework
> already supports turning devices on again when we use them.
>
> > > I think the brcmstb root bus is always bus 0, it only has a single
> > > Root Port on the root bus, and it always leads to bus 1, so it sort of
> > > makes sense that we only need to turn on power when we're about to
> > > scan "bus->number == 1".
> >
> > Correct.
> >
> > > But this power management seems like a pattern that other
> > > controllers will use. Other controllers will have several Root
> > > Ports, so checking the bus number won't work for them. Instead of
> > > checking the bus number, I think brcmstb should check more
> > > directly for a power regulator.
> >
> > I agree. That is why I said that we should consider removing the
> > "busno==1" conditional if we want this feature for general use. If
> > you want, I can submit a V2 that removes this conditional.
>
> If it's as easy as dropping a needlessly platform-dependent check, we
> should absolutely do it. Are you saying the patch would just become
> this?
>
> > I have a series of patches coming up that address some of these concerns.
> > Can we please take this up then but allow us to escape "revert jail" first?
>
> Of course. That's why I labeled these "tangents"; they're just things
> for future work that I noticed and didn't want to forget.
>
> Bjorn
