| 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: <20250711231830.GA2313060@bhelgaas>
Date: Fri, 11 Jul 2025 18:18:30 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Sai Krishna Musham <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,
michal.simek@....com, bharat.kumar.gogada@....com,
thippeswamy.havalige@....com
Subject: Re: [PATCH v5 2/2] PCI: amd-mdb: Add support for PCIe RP PERST#
signal handling
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@....com>
> ---
> 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
>
> v4 https://lore.kernel.org/all/20250626054906.3277029-1-sai.krishna.musham@amd.com/
> v3 https://lore.kernel.org/r/20250618080931.2472366-1-sai.krishna.musham@amd.com/
> v2 https://lore.kernel.org/r/20250429090046.1512000-1-sai.krishna.musham@amd.com/
> v1 https://lore.kernel.org/r/20250326041507.98232-1-sai.krishna.musham@amd.com/
> ---
> 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@qti.qualcomm.com/
has a good example of how to this fallback nicely.
Otherwise looks good to me.
> +
> + /* 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
>
Powered by blists - more mailing lists