[AMD Official Use Only - AMD Internal Distribution Only]
Hi Bjorn,
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Tuesday, July 22, 2025 3:25 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 v6 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 Sat, Jul 19, 2025 at 08:39:51AM +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.
>
> > +static int amd_mdb_parse_pcie_port(struct amd_mdb_pcie *pcie)
> > +{
> > + struct device *dev = pcie->pci.dev;
> > + struct device_node *pcie_port_node;
> > +
> > + pcie_port_node = of_get_next_child_with_prefix(dev->of_node, NULL, "pcie");
> > + if (!pcie_port_node) {
> > + dev_err(dev, "No PCIe Bridge node found\n");
> > + return -ENODEV;
> > + }
>
> Sorry I didn't notice this before. I don't think we want to emit a
> message here either because existing DTs in the field will not have a
> Root Port node, and we will just fall back to the 'reset' in the PCIe
> node.
>
> There's really nothing wrong in that case, so no need to annoy users
> with messages they can't fix.
>
> IIUC, PERST# in the DT is optional anyway (you use
> devm_gpiod_get_optional() below).
>
Thanks for pointing that out - you're right. There's no need to emit
a message when the PCIe bridge node is missing. I'll remove it
to avoid unnecessary logs during fall back to the 'reset' in the
PCIe node.
Regarding the use of devm_gpiod_get_optional(): since the optional
variant for fwnode isn't available in gpiolib-devres.c, using
devm_gpiod_get_optional() would only check the PCIe node itself,
not its child. That's why I opted for devm_fwnode_gpiod_get() and
handled the optional reset GPIO manually by assigning NULL when
'reset-gpios' is not present in bridge node - please correct me if I'm
missing anything here. Thanks.
> > + /* Request the GPIO for PCIe reset signal and assert */
> > + pcie->perst_gpio = devm_fwnode_gpiod_get(dev,
> of_fwnode_handle(pcie_port_node),
> > + "reset", GPIOD_OUT_HIGH, NULL);
> > + if (IS_ERR(pcie->perst_gpio)) {
> > + if (PTR_ERR(pcie->perst_gpio) != -ENOENT) {
> > + of_node_put(pcie_port_node);
> > + return dev_err_probe(dev, PTR_ERR(pcie->perst_gpio),
> > + "Failed to request reset GPIO\n");
> > + }
> > + pcie->perst_gpio = NULL;
> > + }
> > +
> > + of_node_put(pcie_port_node);
> > +
> > + return 0;
> > +}
>
> > @@ -444,6 +483,7 @@ static int amd_mdb_pcie_probe(struct platform_device
> *pdev)
> > struct device *dev = &pdev->dev;
> > struct amd_mdb_pcie *pcie;
> > struct dw_pcie *pci;
> > + int ret;
> >
> > pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> > if (!pcie)
> > @@ -454,6 +494,26 @@ static int amd_mdb_pcie_probe(struct platform_device
> *pdev)
> >
> > platform_set_drvdata(pdev, pcie);
> >
> > + ret = amd_mdb_parse_pcie_port(pcie);
> > +
> > + /*
> > + * 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 == -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;
> > + }
> > +
> > return amd_mdb_add_pcie_port(pcie, pdev);
> > }
> >
> > --
> > 2.44.1
> >
Regards,
Sai Krishna
