[AMD Official Use Only - AMD Internal Distribution Only]
Hi Bjorn,
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Wednesday, August 13, 2025 4:31 AM
> To: Musham, Sai Krishna <sai.krishna.musham@xxxxxxx>
> Cc: bhelgaas@xxxxxxxxxx; lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx; mani@xxxxxxxxxx;
> robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; cassel@xxxxxxxxxx;
> linux-pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>; Gogada, Bharat
> Kumar <bharat.kumar.gogada@xxxxxxx>; Havalige, Thippeswamy
> <thippeswamy.havalige@xxxxxxx>
> 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
