| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<DM4PR12MB6158961BAA5D9A3F56102901CD3DA@DM4PR12MB6158.namprd12.prod.outlook.com>
Date: Fri, 22 Aug 2025 10:11:16 +0000
From: "Musham, Sai Krishna" <sai.krishna.musham@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "lpieralisi@...nel.org"
<lpieralisi@...nel.org>, "kw@...ux.com" <kw@...ux.com>, "mani@...nel.org"
<mani@...nel.org>, "robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
"cassel@...nel.org" <cassel@...nel.org>, "linux-pci@...r.kernel.org"
<linux-pci@...r.kernel.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "Simek, Michal" <michal.simek@....com>,
"Gogada, Bharat Kumar" <bharat.kumar.gogada@....com>, "Havalige, Thippeswamy"
<thippeswamy.havalige@....com>
Subject: RE: [PATCH v7 2/2] PCI: amd-mdb: Add support for PCIe RP PERST#
signal handling
[AMD Official Use Only - AMD Internal Distribution Only]
Hi Bjorn,
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@...nel.org>
> Sent: Wednesday, August 13, 2025 1:11 AM
> To: Musham, Sai Krishna <sai.krishna.musham@....com>
> Cc: bhelgaas@...gle.com; lpieralisi@...nel.org; kw@...ux.com; mani@...nel.org;
> robh@...nel.org; krzk+dt@...nel.org; conor+dt@...nel.org; cassel@...nel.org;
> linux-pci@...r.kernel.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; Simek, Michal <michal.simek@....com>; Gogada, Bharat
> Kumar <bharat.kumar.gogada@....com>; Havalige, Thippeswamy
> <thippeswamy.havalige@....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 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.
> >
> > Signed-off-by: Sai Krishna Musham <sai.krishna.musham@....com>
> > ---
> > Changes in v7:
> > - Use for_each_child_of_node_with_prefix() to iterate through PCIe
> > Bridge nodes.
> >
> > Changes in v6:
> > - Simplified error checking condition logic.
> > - Removed unnecessary fallback message.
> >
> > 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@intel.com/
> > - 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
> >
> > v6 https://lore.kernel.org/all/20250719030951.3616385-1-
> sai.krishna.musham@....com/
> > v5 https://lore.kernel.org/all/20250711052357.3859719-1-
> sai.krishna.musham@....com/
> > v4 https://lore.kernel.org/all/20250626054906.3277029-1-
> sai.krishna.musham@....com/
> > v3 https://lore.kernel.org/r/20250618080931.2472366-1-
> sai.krishna.musham@....com/
> > v2 https://lore.kernel.org/r/20250429090046.1512000-1-
> sai.krishna.musham@....com/
> > v1 https://lore.kernel.org/r/20250326041507.98232-1-
> sai.krishna.musham@....com/
> > ---
> > drivers/pci/controller/dwc/pcie-amd-mdb.c | 52 ++++++++++++++++++++++-
> > 1 file changed, 51 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..3c6e837465bb 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,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?
>
> The apple, kirin, mt7621, mtk, and qcom drivers are examples. I think
> the qcom structure is pretty good, and it has a similar fallback path
> for DTs without Root Port nodes (qcom_pcie_parse_legacy_binding()):
>
> qcom_pcie_probe
> ret = qcom_pcie_parse_ports
> for_each_available_child_of_node_scoped(dev->of_node, of_port)
> qcom_pcie_parse_port(of_port)
> reset = devm_fwnode_gpiod_get(..., "reset", ...)
> if (ret)
> qcom_pcie_parse_legacy_binding
>
> IIUC the current amd-mdb hardware only supports a single Root Port, so
> I don't think you need a TODO, since there's no point in that
> enhancement until hardware supports multiple RPs.
>
> But I probably *would* add a check here so that if we run the current
> driver on future hardware that does have multiple Root Ports with
> separate resets for each RP, there's at least a chance that the first
> RP will work. E.g.,
>
> amd_mdb_parse_pcie_port(...)
> {
> if (pcie->perst_gpio) {
> dev_warn("Ignoring extra Root Port\n");
> return 0;
> }
>
> pcie->perst_gpio = devm_fwnode_gpiod_get(...);
>
Thanks for the feedback. I'll switch to using for_each_available_child_of_node_scoped()
and send it as a separate patch.
> > + 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))
> > + return dev_err_probe(dev, PTR_ERR(pcie->perst_gpio),
> > + "Failed to request reset GPIO\n");
> > + return 0;
> > + }
> > +
> > + return -ENODEV;
> > +}
> > +
> > static int amd_mdb_add_pcie_port(struct amd_mdb_pcie *pcie,
> > struct platform_device *pdev)
> > {
> > @@ -426,6 +451,12 @@ 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);
> > + gpiod_set_value_cansleep(pcie->perst_gpio, 0);
> > + mdelay(PCIE_RESET_CONFIG_WAIT_MS);
> > + }
> > +
> > err = dw_pcie_host_init(pp);
> > if (err) {
> > dev_err(dev, "Failed to initialize host, err=%d\n", err);
> > @@ -444,6 +475,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 +486,24 @@ 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 Host Bridge node.
> > + */
> > + if (ret) {
> > + if (ret != -ENODEV)
> > + return ret;
> > +
> > + 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.43.0
> >
Powered by blists - more mailing lists