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

["Musham, Sai Krishna" <sai.krishna.musham@amd.com>]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, August 13, 2025 4:31 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 v7 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 Tue, Aug 12, 2025 at 02:41:15PM -0500, Bjorn Helgaas wrote:
> > On Thu, Aug 07, 2025 at 01:10:19PM +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 host bridge 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
> > > host bridge node now has multiple children. This ensures the correct
> > > node is selected during initialization.
> > ...
>
> > >   * @slcr: MDB System Level Control and Status Register (SLCR) base
> > >   * @intx_domain: INTx IRQ domain pointer
> > >   * @mdb_domain: MDB IRQ domain pointer
> > > + * @perst_gpio: GPIO descriptor for PERST# signal handling
> > >   * @intx_irq: INTx IRQ interrupt number
> > >   */
> > >  struct amd_mdb_pcie {
> > > @@ -63,6 +65,7 @@ struct amd_mdb_pcie {
> > >     void __iomem                    *slcr;
> > >     struct irq_domain               *intx_domain;
> > >     struct irq_domain               *mdb_domain;
> > > +   struct gpio_desc                *perst_gpio;
> > >     int                             intx_irq;
> > >  };
> > >
> > > @@ -284,7 +287,7 @@ static int amd_mdb_pcie_init_irq_domains(struct
> amd_mdb_pcie *pcie,
> > >     struct device_node *pcie_intc_node;
> > >     int err;
> > >
> > > -   pcie_intc_node = of_get_next_child(node, NULL);
> > > +   pcie_intc_node = of_get_child_by_name(node, "interrupt-controller");
> > >     if (!pcie_intc_node) {
> > >             dev_err(dev, "No PCIe Intc node found\n");
> > >             return -ENODEV;
> > > @@ -402,6 +405,28 @@ static int amd_mdb_setup_irq(struct amd_mdb_pcie
> *pcie,
> > >     return 0;
> > >  }
> > >
> > > +static int amd_mdb_parse_pcie_port(struct amd_mdb_pcie *pcie)
> > > +{
> > > +   struct device *dev = pcie->pci.dev;
> > > +   struct device_node *pcie_port_node __maybe_unused;
> > > +
> > > +   /*
> > > +    * This platform currently supports only one Root Port, so the loop
> > > +    * will execute only once.
> > > +    * TODO: Enhance the driver to handle multiple Root Ports in the future.
> > > +    */
> > > +   for_each_child_of_node_with_prefix(dev->of_node, pcie_port_node, "pcie") {
> >
> > This is only the second user of for_each_child_of_node_with_prefix()
> > in the whole tree.  Also the only use of "__maybe_unused" in
> > drivers/pci/controller/.
> >
> > Most of the PCI controller drivers use
> > for_each_available_child_of_node_scoped(); can we do the same here?
>
> I suppose you used for_each_child_of_node_with_prefix() because, as
> you mentioned in the commit log, there may be multiple children
> (interrupt controller and Root Port(s))?
>
> I think of_irq_parse_and_map_pci() takes care of much of the INTx
> setup based on interrupt-map, so many drivers don't contain any
> explicit INTx stuff.  But amd-mdb must be an exception for some
> reason.
>
> Most bindings don't include an interrupt-controller child in the pcie@
> stanza.  I don't know enough about device tree to understand why
> amd-mdb needs it.
>

Thanks Bjorn for pointing out the of_irq_parse_and_map_pci() API. I'll update the driver
to use it for INTx setup based on the interrupt-map, and will explore removing the
interrupt-controller node if it's no longer needed.

> Bjorn

Regards,
Sai Krishna