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

Thu May 19 11:04:18 PDT 2022

On Thu, May 19, 2022 at 12:10 PM Bjorn Helgaas <helgaas at kernel.org> wrote:
>
> [+to Rob for my naive DT questions]
>
> 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 assume you're referring specifically to making this optional in the
> DT:
>
>     /* PCIe endpoint */
>     pci-ep at 0,0 {
>             assigned-addresses =
>                 <0x82010000 0x0 0xf8000000 0x6 0x00000000 0x0 0x2000>;
>             reg = <0x0 0x0 0x0 0x0 0x0>;
>             compatible = "pci14e4,1688";
>     };
>
Actually, both that and the node that contains it, i.e. pci at 0,0.

> I don't really understand what's going on here, but I assume this
> describes a [14e4:1688] device, which the PCI database says is a
> NetXtreme BCM5761 10/100/1000BASE-T Ethernet
> (https://pci-ids.ucw.cz/read/PC/14e4/1688)

Yes.  I use an assortment of PCIe endpoint devices for testing.
>
> Why do you *ever* need this stanza?  "git grep pci-ep
> Documentation/devicetree/bindings/pci/" says no other DT has one.

You'll find one in
"Documentation/devicetree/bindings/pci/nvidia,tegra-pcie.txt", line
~240, although this
is a board DTS example.  They use "pci at 0,0" for endpoint 02:00.0,
whereas I find "pci-ep" to
be more descriptive.

Note that  the "pci-ep at 0,0" node is in the "example" section of
brcm,stb-pcie.yaml; but nothing
says it is required.  I believe it was added it because a reviewer
asked me to, but if I remember
incorrectly,  it does illustrate that "pcie at 0,0" is not the endpoint
device node as many would think.

Note that the regression occurred because "pci at 0,0" was missing, not
"pci-ep at 0,0" as I first thought.

>
> If the link does come up, I assume normal PCI enumeration would
> discover the [14e4:1688] or whatever device is plugged into a CM4
> socket, and it would read and assign BARs as needed.  Why do we need
> to describe any of this in the DT?
The only  reason one needs to describe this node is  when a regulator is
under the root port, in my case pci at 0,0.  In the example this is

                            vpcie3v3-supply = <&vreg7>;

This was the entire reason behind the original patchset.
>
> If the link doesn't come up, it looks like you set the "refusal_mode"
> so subsequent config accesses fail gracefully instead of with a CPU
> abort.
Yes.
>
> [Tangent: since you never clear "refusal_mode", I assume there's no
> possibility of hot-adding a device.  A device must be put in the slot
> before power-up, right?]
Yes, we do not have the HW functionality to support hotplug.

>
> > [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 at gmail.com>
> > ---
> >  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)
>
> It's a big leap from "DT endpoint is optional" to "bus->number == 1 if
> DT endpoint is missing" (if that's even what it means).  Help me
> connect the dots here.
The brcm_pcie_add_bus() function returned immediately and skipped linkup
when (!dev->of_node). That clause was removed from that function, which
is the true fix for the regression,  but you can see thiscondition
is still tested in pci_subdev_regulators_add_bus().

I added the "busno != 1" as an added precaution,
as the brcmstb RC driver only cares about pcie linkup and turning on
regulators when busno==1.

If this regulator mechanism becomes a feature any RC driver may use --
as it was in
v8 of the original patch but was moved to pcie-brcamstb only to avoid conflicts
with Pali's upcoming RC functionality improvements -- I would probably consider
removing the busno==1 clause.

Regards and thanks,
Jim Quinlan
Broadcom S

>
> I *guess* this is really saying "we only want to bring the link up for
> RPs"?
>
> And "bus->number == 1" assumes the RP is on bus 0, there's only one
> RP, and that RP's secondary bus is 1?  So it's only in that case
> (we're adding the secondary bus of the RP), that we need to manually
> bring up the link?
>
> >               return 0;
> >
> >       ret = pci_subdev_regulators_add_bus(bus);
> > -     if (ret)
> > +     if (ret) {
> > +             pcie->refusal_mode = true;
>
> Is this related?  It doesn't *look* related to making the DT endpoint
> optional.
>
> >               return ret;
> > +     }
> >
> >       /* Grab the regulators for suspend/resume */
> >       pcie->sr = bus->dev.driver_data;
> >
> > base-commit: ef1302160bfb19f804451d0e919266703501c875
> > prerequisite-patch-id: 23a425390a4226bd70bbff459148c80f5e28379c
> > prerequisite-patch-id: e3f2875124b46b2b1cf9ea28883bf0c864b79479
> > prerequisite-patch-id: 9cdd706ee2038c7b393c4d65ff76a1873df1ca03
> > prerequisite-patch-id: 332ac90be6e4e4110e27bdd1caaff212c129f547
> > prerequisite-patch-id: 32a74f87cbfe9e8d52c34a4edeee6d271925665a
> > prerequisite-patch-id: f57cdf7ec7080bb8c95782bc7c3ec672db8ec1ce
> > prerequisite-patch-id: 18dc9236aed47f708f5c854afd832f3c80be5ea7
> > prerequisite-patch-id: dd147c6854c4ca12a9a8bd4f5714968a59d60e4e
> > --
> > 2.17.1
> >