Re: [PATCH v2] PCI: rzg3s-host: Re-enumerate the bus on PCIe link-state changes

On Tue, Jun 30, 2026 at 02:17:20PM +0000, John Madieu wrote:
> The RZ/G3E PCIe controller does not expose the standard PCIe Slot
> Capability registers, so the generic pciehp driver cannot be used. The
> only link-state signal the hardware provides is the DL_UpDown bit in the
> PEIS0 event status register, which is raised on every Data Link layer
> up/down transition.
> 
> Enable DL_UpDown in PEIE0 and hook up an interrupt handler so the driver
> can react to link-state changes: a device that trains after boot gets
> enumerated, and a device that disappears on link loss is removed. This
> provides hotplug-like behaviour without the PCI hotplug core, which is
> unavailable for the reason above.
> 
> On a DL_UpDown event the handler acks the W1C status bit and schedules a
> worker that inspects PCSTAT1.DL_DOWN_STS:
> 
>   - link up: re-run max link speed negotiation, wait for the link to
>     settle and pci_rescan_bus() the root bus;
>   - link down: walk the bus in reverse and
>     pci_stop_and_remove_bus_device() each child.
> 
> Both paths take pci_lock_rescan_remove() to serialise against the PCI
> core.
> 
> Link events are only acted upon once the controller is fully
> initialised. A DL_UpDown latched while the registers are not configured,
> for example when the event IRQ is used as a system wakeup source during
> resume, is acknowledged but does not schedule a rescan.

This is not correct. I don't see the patch configuring 'event IRQ' as a wakeup
source.

Rest of the patch LGTM. If you confirm above, I can reword while applying.

- Mani

> The
> hw_initialized flag, set at the end of controller setup and cleared on
> suspend, gates this.
> 
> While at it, make probe tolerant of an absent device. Previously, if the
> link failed to come up during rzg3s_pcie_host_init(), probe tore the
> controller back down and failed. Distinguish this case with -ENODEV,
> leave the controller and refclk running, and let the link-up path
> enumerate the device once it appears.
> 
> Signed-off-by: John Madieu <[email protected]>
> ---
> This patch applies on top of the RZ/V2H(P) PCIe series:
> 
>   [PATCH v5 0/4] Add PCIe support for RZ/V2H(P) SoC
>   https://lore.kernel.org/r/[email protected]
> 
> Changes in v2:
> - Rebased on top of the RZ/V2H(P) PCIe support series from Lad
>   Prabhakar. No functional change.
> 
>  drivers/pci/controller/pcie-rzg3s-host.c | 153 +++++++++++++++++++++--
>  1 file changed, 143 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 8187a82..80e0c0d 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> @@ -34,6 +34,7 @@
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
>  #include <linux/units.h>
> +#include <linux/workqueue.h>
> 
>  #include "../pci.h"
> 
> @@ -322,7 +323,12 @@ struct rzg3s_pcie_port {
>   * @msi: MSI data structure
>   * @port: PCIe Root Port
>   * @hw_lock: lock for access to the HW resources
> + * @link_work: work for DL_UpDown link-state change handling
> + * @event_irq: PCIe event interrupt for DL_UpDown detection
>   * @intx_irqs: INTx interrupts
> + * @hw_initialized: set once the controller HW is fully initialised; gates
> + *                  DL_UpDown event handling against events latched while
> + *                  the registers are not configured
>   * @max_link_speed: maximum supported link speed
>   * @controller_id: PCIe controller identifier, used for System Controller access
>   * @num_lanes: The number of lanes
> @@ -339,7 +345,10 @@ struct rzg3s_pcie_host {
>  	struct rzg3s_pcie_msi msi;
>  	struct rzg3s_pcie_port port;
>  	raw_spinlock_t hw_lock;
> +	struct work_struct link_work;
> +	int event_irq;
>  	int intx_irqs[PCI_NUM_INTX];
> +	bool hw_initialized;
>  	int max_link_speed;
>  	enum rzg3s_pcie_controller_id controller_id;
>  	u8 num_lanes;
> @@ -619,6 +628,30 @@ static irqreturn_t rzg3s_pcie_msi_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
> 
> +static irqreturn_t rzg3s_pcie_event_irq(int irq, void *data)
> +{
> +	struct rzg3s_pcie_host *host = data;
> +	u32 status;
> +
> +	status = readl_relaxed(host->axi + RZG3S_PCI_PEIS0);
> +
> +	if (!(status & RZG3S_PCI_PEIS0_DL_UPDOWN))
> +		return IRQ_NONE;
> +
> +	/* Clear the DL_UpDown status (W1C) */
> +	writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIS0);
> +
> +	/*
> +	 * Drop the event until the controller is fully initialised. The
> +	 * event IRQ may act as a system wakeup source and fire during
> +	 * resume before the HW registers have been reconfigured.
> +	 */
> +	if (READ_ONCE(host->hw_initialized))
> +		schedule_work(&host->link_work);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static void rzg3s_pcie_msi_irq_ack(struct irq_data *d)
>  {
>  	struct rzg3s_pcie_msi *msi = irq_data_get_irq_chip_data(d);
> @@ -1151,6 +1184,47 @@ static int rzg3s_pcie_set_max_link_speed(struct rzg3s_pcie_host *host)
>  	return ret;
>  }
> 
> +static void rzg3s_pcie_link_work(struct work_struct *work)
> +{
> +	struct rzg3s_pcie_host *host =
> +		container_of(work, struct rzg3s_pcie_host, link_work);
> +	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
> +	struct pci_bus *bus = bridge->bus;
> +	u32 val;
> +
> +	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT1);
> +	if (val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS) {
> +		struct pci_dev *dev, *tmp;
> +
> +		dev_info(host->dev, "PCIe link down, removing devices\n");
> +
> +		pci_lock_rescan_remove();
> +		list_for_each_entry_safe_reverse(dev, tmp,
> +						 &bus->devices, bus_list)
> +			pci_stop_and_remove_bus_device(dev);
> +		pci_unlock_rescan_remove();
> +	} else {
> +		int ret;
> +
> +		dev_info(host->dev, "PCIe link up, rescanning bus\n");
> +
> +		/*
> +		 * Attempt link speed negotiation now that the link is up.
> +		 * Failure is non-fatal: the device works at the negotiated
> +		 * speed.
> +		 */
> +		ret = rzg3s_pcie_set_max_link_speed(host);
> +		if (ret)
> +			dev_info(host->dev, "Failed to set max link speed\n");
> +
> +		msleep(PCIE_RESET_CONFIG_WAIT_MS);
> +
> +		pci_lock_rescan_remove();
> +		pci_rescan_bus(bus);
> +		pci_unlock_rescan_remove();
> +	}
> +}
> +
>  static int rzg3s_pcie_config_init(struct rzg3s_pcie_host *host)
>  {
>  	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
> @@ -1268,8 +1342,8 @@ static void rzg3s_pcie_irq_init(struct rzg3s_pcie_host *host)
>  		       RZG3S_PCI_PEIS0_RX_DLLP_PM_ENTER,
>  		       host->axi + RZG3S_PCI_PEIS0);
> 
> -	/* Disable all interrupts */
> -	writel_relaxed(0, host->axi + RZG3S_PCI_PEIE0);
> +	/* Enable DL_UpDown interrupt for link state change detection */
> +	writel_relaxed(RZG3S_PCI_PEIS0_DL_UPDOWN, host->axi + RZG3S_PCI_PEIE0);
> 
>  	/* Clear all parity and ecc error interrupts */
>  	writel_relaxed(~0U, host->axi + RZG3S_PCI_PEIS1);
> @@ -1435,16 +1509,21 @@ static int rzg3s_pcie_host_init(struct rzg3s_pcie_host *host)
>  				 PCIE_LINK_WAIT_SLEEP_MS * MILLI,
>  				 PCIE_LINK_WAIT_SLEEP_MS * MILLI *
>  				 PCIE_LINK_WAIT_MAX_RETRIES);
> -	if (ret)
> -		goto config_deinit_post;
> +	if (ret) {
> +		/*
> +		 * Link is down. Leave the controller running so the
> +		 * DL_UpDown handler can enumerate a device that appears
> +		 * later.
> +		 */
> +		dev_info(host->dev, "PCIe link down, waiting for DL_UpDown\n");
> +		ret = -ENODEV;
> +	}
> 
>  	val = readl_relaxed(host->axi + RZG3S_PCI_PCSTAT2);
>  	dev_info(host->dev, "PCIe link status [0x%x]\n", val);
> 
> -	return 0;
> +	return ret;
> 
> -config_deinit_post:
> -	host->data->config_deinit(host);
>  config_deinit_and_refclk:
>  	clk_disable_unprepare(host->port.refclk);
>  config_deinit:
> @@ -1706,8 +1785,15 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
> 
>  	ret = rzg3s_pcie_host_init(host);
>  	if (ret) {
> -		dev_err_probe(dev, ret, "Failed to initialize the HW!\n");
> -		goto teardown_irqdomain;
> +		if (ret != -ENODEV) {
> +			dev_err_probe(dev, ret,
> +				      "Failed to initialize the HW!\n");
> +			goto teardown_irqdomain;
> +		}
> +
> +		/* Link is down: hotplug via DL_UpDown will recover. */
> +		WRITE_ONCE(host->hw_initialized, true);
> +		return 0;
>  	}
> 
>  	ret = rzg3s_pcie_set_max_link_speed(host);
> @@ -1716,6 +1802,8 @@ rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
> 
>  	msleep(PCIE_RESET_CONFIG_WAIT_MS);
> 
> +	WRITE_ONCE(host->hw_initialized, true);
> +
>  	return 0;
> 
>  teardown_irqdomain:
> @@ -1803,6 +1891,7 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
>  		of_parse_phandle(np, "renesas,sysc", 0);
>  	struct rzg3s_pcie_host *host;
>  	struct rzg3s_sysc *sysc;
> +	const char *evt_name;
>  	int ret;
> 
>  	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*host));
> @@ -1880,6 +1969,7 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
>  		goto rpm_disable;
> 
>  	raw_spin_lock_init(&host->hw_lock);
> +	INIT_WORK(&host->link_work, rzg3s_pcie_link_work);
> 
>  	ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_init_irqdomain,
>  				    rzg3s_pcie_teardown_irqdomain);
> @@ -1893,8 +1983,39 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto host_probe_teardown;
> 
> +	/*
> +	 * Request the PCIe event IRQ at the end of probe to avoid
> +	 * spurious link-state events during controller setup and bus
> +	 * enumeration. From here on, DL_UpDown events trigger the link
> +	 * worker to (re)scan the bus.
> +	 */
> +	host->event_irq = platform_get_irq_byname(pdev, "pcie_evt");
> +	if (host->event_irq < 0) {
> +		ret = host->event_irq;
> +		goto pci_host_remove;
> +	}
> +
> +	evt_name = devm_kasprintf(dev, GFP_KERNEL, "%s-pcie-evt",
> +				  dev_name(dev));
> +	if (!evt_name) {
> +		ret = -ENOMEM;
> +		goto pci_host_remove;
> +	}
> +
> +	ret = request_irq(host->event_irq, rzg3s_pcie_event_irq, 0,
> +			  evt_name, host);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to request pcie_evt IRQ\n");
> +		goto pci_host_remove;
> +	}
> +
>  	return 0;
> 
> +pci_host_remove:
> +	pci_lock_rescan_remove();
> +	pci_stop_root_bus(bridge->bus);
> +	pci_remove_root_bus(bridge->bus);
> +	pci_unlock_rescan_remove();
>  host_probe_teardown:
>  	rzg3s_pcie_teardown_irqdomain(host);
>  	host->data->config_deinit(host);
> @@ -1924,9 +2045,19 @@ static int rzg3s_pcie_suspend_noirq(struct device *dev)
>  	struct rzg3s_sysc *sysc = host->sysc;
>  	int ret;
> 
> +	/*
> +	 * Stop accepting DL_UpDown events, then drain any worker that may
> +	 * already be running, before tearing the controller down.
> +	 */
> +	WRITE_ONCE(host->hw_initialized, false);
> +	cancel_work_sync(&host->link_work);
> +
>  	ret = pm_runtime_put_sync(dev);
> -	if (ret)
> +	if (ret) {
> +		/* Suspend aborted; keep handling DL_UpDown events. */
> +		WRITE_ONCE(host->hw_initialized, true);
>  		return ret;
> +	}
> 
>  	clk_disable_unprepare(port->refclk);
> 
> @@ -1957,6 +2088,8 @@ config_reinit:
>  refclk_restore:
>  	clk_prepare_enable(port->refclk);
>  	pm_runtime_resume_and_get(dev);
> +	/* Controller is alive again; resume DL_UpDown handling. */
> +	WRITE_ONCE(host->hw_initialized, true);
>  	return ret;
>  }
> 
> --
> 2.43.0
> 

-- 
மணிவண்ணன் சதாசிவம்