| 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: <20250612203347.GA926120@bhelgaas>
Date: Thu, 12 Jun 2025 15:33:47 -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,
manivannan.sadhasivam@...aro.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: [RESEND PATCH v7 2/2] PCI: xilinx-cpm: Add support for PCIe RP
PERST# signal
On Mon, Apr 14, 2025 at 08:53:04AM +0530, Sai Krishna Musham wrote:
> Add support for handling the PCIe Root Port (RP) PERST# signal using
> the GPIO framework, along with the PCIe IP reset. This reset is
> managed by the driver and occurs after the Initial Power Up sequence
> (PCIe CEM r6.0, 2.2.1) is handled in hardware before the driver's probe
> function is called.
Please say something specific here about what this does. I *think* it
asserts both the PCIe IP reset (which I assume resets the host
controller) and PERST# (which resets any devices connected to the Root
Port), but only for devices that implement the CPM Clock and Reset
Control Registers AND describe the address of those registers via
DT "cpm_crx" AND describe a GPIO connected to PERST# via DT "reset".
> This reset mechanism is particularly useful in warm reset scenarios,
> where the power rails remain stable and only PERST# signal is toggled
> through the driver. Applying both the PCIe IP reset and the PERST#
> improves the reliability of the reset process by ensuring that both
> the Root Port controller and the Endpoint are reset synchronously
> and avoid lane errors.
>
> Adapt the implementation to use the GPIO framework for reset signal
> handling and make this reset handling optional, along with the
> `cpm_crx` property, to maintain backward compatibility with existing
> device tree binaries (DTBs).
> Additionally, clear Firewall after the link reset for CPM5NC to allow
> further PCIe transactions.
> -static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
> +static int xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
> {
> const struct xilinx_cpm_variant *variant = port->variant;
> + struct device *dev = port->dev;
> + struct gpio_desc *reset_gpio;
> + bool do_reset = false;
> +
> + if (port->crx_base && (variant->version < CPM5NC_HOST ||
> + (variant->version == CPM5NC_HOST &&
> + port->cpm5nc_fw_base))) {
Would be nicer if you could simply test for the feature, not the
specific variants, e.g.,
if (port->crx_base && port->perst_gpio) {
writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
udelay(100);
writel_relaxed(0x0, port->crx_base + variant->cpm_pcie_rst);
gpiod_set_value(port->perst_gpio, 0);
mdelay(PCIE_T_RRS_READY_MS);
}
if (port->firewall_base) {
/* Clear Firewall */
}
If you need to check the variants vs "cpm_crx", I think that should go
in xilinx_cpm_pcie_parse_dt().
> + /* Request the GPIO for PCIe reset signal and assert */
> + reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(reset_gpio),
> + "Failed to request reset GPIO\n");
> + if (reset_gpio)
> + do_reset = true;
> + }
Maybe the devm_gpiod_get_optional() could go in
xilinx_cpm_pcie_parse_dt() along with other DT stuff, as is done in
starfive_pcie_parse_dt()/starfive_pcie_host_init()?
You'd have to save the port->reset_gpio pointer so we could use it
here, but wouldn't have to return error from
xilinx_cpm_pcie_init_port().
> +
> + if (do_reset) {
> + /* Assert the PCIe IP reset */
> + writel_relaxed(0x1, port->crx_base + variant->cpm_pcie_rst);
> +
> + /*
> + * "PERST# active time", as per Table 2-10: Power Sequencing
> + * and Reset Signal Timings of the PCIe Electromechanical
> + * Specification, Revision 6.0, symbol "T_PERST".
> + */
> + udelay(100);
Whatever we need here, this should be a #define from drivers/pci/pci.h
instead of 100.
> +
> + /* Deassert the PCIe IP reset */
> + writel_relaxed(0x0, port->crx_base + variant->cpm_pcie_rst);
> +
> + /* Deassert the reset signal */
> + gpiod_set_value(reset_gpio, 0);
I think reset_gpio controls PERST#. If so, it would be nice to have
"perst" in the name to make it less ambiguous.
> + mdelay(PCIE_T_RRS_READY_MS);
We only wait PCIE_T_RRS_READY_MS for certain variants and only when
the optional "cpm_crx" and "reset" properties are present.
What about the other cases? Unless there's something that guarantees
a delay after the link comes up before we call pci_host_probe(), that
sounds like a bug in the existing driver. If it is a bug, you should
fix it in its own separate patch.
> + if (variant->version == CPM5NC_HOST &&
> + port->cpm5nc_fw_base) {
Unnecessary to test both variant->version and port->cpm5nc_fw_base
here, since only CPM5NC_HOST sets cpm5nc_fw_base.
The function of the "Firewall" should be explained in the commit log,
and it seems like the sort of thing that's likely to appear in future
variants, so "cpm5nc_" seems like it might be unnecessarily specific.
Maybe consider naming these "firewall_base" and "firewall_reset" so
the test and the writes wouldn't have to change for future variants.
> + /* Clear Firewall */
> + writel_relaxed(0x00, port->cpm5nc_fw_base +
> + variant->cpm5nc_fw_rst);
> + writel_relaxed(0x01, port->cpm5nc_fw_base +
> + variant->cpm5nc_fw_rst);
> + writel_relaxed(0x00, port->cpm5nc_fw_base +
> + variant->cpm5nc_fw_rst);
> + }
> + }
>
> if (variant->version == CPM5NC_HOST)
You didn't change this test, but it would be better if you could test
for a *feature* instead of a specific variant. Then you can avoid
changes when future chips have the same feature.
> - return;
> + return 0;
>
> if (cpm_pcie_link_up(port))
> dev_info(port->dev, "PCIe Link is UP\n");
> @@ -512,6 +574,8 @@ static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
> pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
> XILINX_CPM_PCIE_REG_RPSC_BEN,
> XILINX_CPM_PCIE_REG_RPSC);
> +
> + return 0;
> }
Powered by blists - more mailing lists