Re: [PATCH v8 2/2] PCI: xilinx-cpm: Add support for PCIe RP PERST# signal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect gating of the PCIe reset sequence leaves the endpoint permanently in reset if only the PERST# GPIO is provided.
- [Medium] Using mdelay() instead of msleep() for a 100ms delay in a sleepable context busily hogs the CPU unnecessarily.
- [Low] The explicit declaration of `pcie_port_node` is completely shadowed by the macro `for_each_child_of_node_with_prefix`.
--

--- Patch [2]: [PATCH v8 2/2] PCI: xilinx-cpm: Add support for PCIe RP PERST# signal ---
commit 1f7ff063bcead5bbf6ae44c2226031dc6d2d092c
Author: Sai Krishna Musham <sai.krishna.musham@amd.com>
PCI: xilinx-cpm: Add support for PCIe RP PERST# signal

This commit adds support for the PCIe Root Port PERST# signal using the GPIO
framework, along with the PCIe IP reset. Both the reset GPIO and the reset
controller are made optional to maintain backward compatibility with existing
device trees. It also updates the interrupt controller node parsing to use
of_get_child_by_name() instead of of_get_next_child().

> diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-cpm.c
> index d38f27e207619..040d37643991a 100644
> --- a/drivers/pci/controller/pcie-xilinx-cpm.c
> +++ b/drivers/pci/controller/pcie-xilinx-cpm.c

[ ... ]

> @@ -469,6 +476,22 @@ static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie *port)
>  	return 0;
>  }
>  
> +/**
> + * xilinx_cpm_pcie_reset - Reset the PCIe controller and deassert PERST#
> + * @port: PCIe port information
> + *
> + * Reset the PCIe controller and then release the PERST# signal so that the
> + * link can train once the bridge is enabled.
> + */
> +static void xilinx_cpm_pcie_reset(struct xilinx_cpm_pcie *port)
> +{
> +	reset_control_assert(port->rstc);
> +	udelay(PCIE_T_PERST_US);
> +	reset_control_deassert(port->rstc);
> +	gpiod_set_value_cansleep(port->perst_gpio, 0);
> +	mdelay(PCIE_RESET_CONFIG_WAIT_MS);

[Severity: Medium]
Since this is in a context that can safely sleep, as indicated by the preceding
gpiod_set_value_cansleep() call, should this be msleep() instead of mdelay() to
avoid busily hogging the CPU?

> +}
> +
>  /**
>   * xilinx_cpm_pcie_init_port - Initialize hardware
>   * @port: PCIe port information
> @@ -480,6 +503,9 @@ static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
>  	if (variant->version == CPM5NC_HOST)
>  		return;
>  
> +	if (port->perst_gpio && port->rstc)
> +		xilinx_cpm_pcie_reset(port);

[Severity: High]
If a platform provides the reset GPIO but omits the optional IP reset
controller, port->rstc will be NULL and this condition will be false.

Since the PERST# GPIO is acquired with GPIOD_OUT_HIGH in
xilinx_cpm_pcie_parse_port(), it starts asserted.

Could this leave the endpoint permanently in reset for platforms that only
provide the GPIO?

> +
>  	if (cpm_pcie_link_up(port))
>  		dev_info(port->dev, "PCIe Link is UP\n");

[ ... ]

> @@ -512,6 +538,43 @@ static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
>  		   XILINX_CPM_PCIE_REG_RPSC);
>  }
>  
> +/**
> + * xilinx_cpm_pcie_parse_port - Parse the PCIe Root Port child node
> + * @port: PCIe port information
> + *
> + * Read the PERST# GPIO from the Root Port child node.
> + *
> + * Return: '0' on success and error value on failure
> + */
> +static int xilinx_cpm_pcie_parse_port(struct xilinx_cpm_pcie *port)
> +{
> +	struct device *dev = port->dev;
> +	struct device_node *pcie_port_node __maybe_unused;

[Severity: Low]
Is this explicit declaration of pcie_port_node necessary?

The for_each_child_of_node_with_prefix() macro internally declares a scoped loop
variable with the same name, which completely shadows this outer variable.

> +
> +	/*
> +	 * 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") {
> +		port->perst_gpio = devm_fwnode_gpiod_get(dev,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807123507.3418671-1-sai.krishna.musham@amd.com?part=2