Re: [PATCH v5 2/2] PCI: amd-mdb: Add support for PCIe RP PERST# signal handling

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jul 18, 2025 at 04:30:32AM +0000, Musham, Sai Krishna wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Bjorn,
> 
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: Saturday, July 12, 2025 4:49 AM
> > To: Musham, Sai Krishna <sai.krishna.musham@amd.com>
> > Cc: bhelgaas@google.com; lpieralisi@kernel.org; kw@linux.com; mani@kernel.org;
> > robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; cassel@kernel.org;
> > linux-pci@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>; Gogada, Bharat
> > Kumar <bharat.kumar.gogada@amd.com>; Havalige, Thippeswamy
> > <thippeswamy.havalige@amd.com>
> > Subject: Re: [PATCH v5 2/2] PCI: amd-mdb: Add support for PCIe RP PERST#
> > signal handling
> >
> > Caution: This message originated from an External Source. Use proper caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > On Fri, Jul 11, 2025 at 10:53:57AM +0530, Sai Krishna Musham wrote:
> > > Add support for handling the AMD Versal Gen 2 MDB PCIe Root Port PERST#
> > > signal via a GPIO by parsing the new PCIe bridge node to acquire the
> > > reset GPIO. If the bridge node is not found, fall back to acquiring it
> > > from the PCIe node.
> > >
> > > As part of this, update the interrupt controller node parsing to use
> > > of_get_child_by_name() instead of of_get_next_child(), since the PCIe
> > > node now has multiple children. This ensures the correct node is
> > > selected during initialization.

> > > +      * If amd_mdb_parse_pcie_port returns -ENODEV, it indicates that the
> > > +      * PCIe Bridge node was not found in the device tree. This is not
> > > +      * considered a fatal error and will trigger a fallback where the
> > > +      * reset GPIO is acquired directly from the PCIe node.
> > > +      */
> > > +     if (ret && ret != -ENODEV) {
> > > +             return ret;
> > > +     } else if (ret == -ENODEV) {
> >
> > The "ret" checking seems unnecessarily complicated.
> >
> > > +             dev_info(dev, "Falling back to acquire reset GPIO from PCIe node\n");
> >
> > I don't think this is worthy of a message.  If there are DTs in the
> > field that were valid once, they continue to be valid forever, and
> > there's no point in complaining about them.
> >
> > https://lore.kernel.org/all/20250702-perst-v5-2-920b3d1f6ee1@qti.qualcomm.com/
> > has a good example of how to this fallback nicely.
> >
> > Otherwise looks good to me.
> 
> Thanks for the feedback. I've removed the fallback message and simplified the "ret"
> checking. Could you please confirm if this looks good for v6?
> 
>         if (ret == -ENODEV) {
> 
>                 /* Request the GPIO for PCIe reset signal and assert */
>                 pcie->perst_gpio = devm_gpiod_get_optional(dev, "reset",
>                                                            GPIOD_OUT_HIGH);
>                 if (IS_ERR(pcie->perst_gpio))
>                         return dev_err_probe(dev, PTR_ERR(pcie->perst_gpio),
>                                              "Failed to request reset GPIO\n");
>         } else if (ret) {
>                 return ret;
>         }

Looks good to me.  It's important to note that this -ENODEV fallback
uses the PERST# GPIO described in the host bridge, not in a Root Port,
but I think your comment above includes this.

Bjorn