[AMD Official Use Only - AMD Internal Distribution Only] Hi Bjorn, > -----Original Message----- > From: Bjorn Helgaas <helgaas@xxxxxxxxxx> > Sent: Saturday, July 12, 2025 4:49 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 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. > > > > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@xxxxxxx> > > --- > > Changes in v5: > > - Add fall back mechanism to acquire reset GPIO from PCIe node when PCIe > Bridge > > node is not present. > > > > Changes in v4: > > - Resolve kernel test robot warning. > > https://lore.kernel.org/oe-kbuild-all/202506241020.rPD1a2Vr-lkp@xxxxxxxxx/ > > - Update commit message. > > > > Changes in v3: > > - Implement amd_mdb_parse_pcie_port to parse bridge node for reset-gpios > property. > > > > Changes in v2: > > - Change delay to PCIE_T_PVPERL_MS > > > > v4 https://lore.kernel.org/all/20250626054906.3277029-1- > sai.krishna.musham@xxxxxxx/ > > v3 https://lore.kernel.org/r/20250618080931.2472366-1- > sai.krishna.musham@xxxxxxx/ > > v2 https://lore.kernel.org/r/20250429090046.1512000-1- > sai.krishna.musham@xxxxxxx/ > > v1 https://lore.kernel.org/r/20250326041507.98232-1- > sai.krishna.musham@xxxxxxx/ > > --- > > drivers/pci/controller/dwc/pcie-amd-mdb.c | 63 ++++++++++++++++++++++- > > 1 file changed, 62 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-amd-mdb.c > b/drivers/pci/controller/dwc/pcie-amd-mdb.c > > index 9f7251a16d32..d633463dc9fe 100644 > > --- a/drivers/pci/controller/dwc/pcie-amd-mdb.c > > +++ b/drivers/pci/controller/dwc/pcie-amd-mdb.c > > @@ -18,6 +18,7 @@ > > #include <linux/resource.h> > > #include <linux/types.h> > > > > +#include "../../pci.h" > > #include "pcie-designware.h" > > > > #define AMD_MDB_TLP_IR_STATUS_MISC 0x4C0 > > @@ -56,6 +57,7 @@ > > * @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,34 @@ 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; > > + > > + 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; > > + } > > + > > + /* 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; > > +} > > + > > static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie, > > struct platform_device *pdev) > > { > > @@ -426,6 +457,14 @@ static int amd_mdb_add_pcie_port(struct > amd_mdb_pcie *pcie, > > > > pp->ops = &amd_mdb_pcie_host_ops; > > > > + if (pcie->perst_gpio) { > > + mdelay(PCIE_T_PVPERL_MS); > > + > > + /* Deassert the reset signal */ > > + gpiod_set_value_cansleep(pcie->perst_gpio, 0); > > + mdelay(PCIE_T_RRS_READY_MS); > > + } > > + > > err = dw_pcie_host_init(pp); > > if (err) { > > dev_err(dev, "Failed to initialize host, err=%d\n", err); > > @@ -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,27 @@ 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 && 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@xxxxxxxxxxxxxxxx/ > 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; } > > + > > + /* 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"); > > + } > > + > > return amd_mdb_add_pcie_port(pcie, pdev); > > } > > > > -- > > 2.44.1 > > Regards, Sai Krishna
