Re: [PATCH RFC 2/2] PCI: rockchip-host: Hot-reset the endpoint before using the link

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] PCI: rockchip-host: PHY lanes powered off before hot reset in init
- [Medium] PCI: rockchip-host: PCIE_CORE_AXI_LINK_DOWN cleared before 100 ms reset wait
--

commit 4a844e565a69f3a1a297df4c2424b03cd25ab7cb
Author: Igor Velkov <iav@iav.lv>

PCI: rockchip-host: Hot-reset the endpoint before using the link

This commit adds an endpoint hot-reset sequence during probe and resume
after initial link training, holding the reset for at least 2 ms, waiting
for the link to retrain, clearing PCIE_CORE_AXI_LINK_DOWN, and waiting
100 ms before configuration accesses.

> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index b4499fc3f90b1..f75c2c8358f5d 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -400,6 +400,57 @@ static int rockchip_pcie_init_port_retry(struct rockchip_pcie *rockchip)
>  	return err;
>  }
>  
> +/* Hot-reset the endpoint; *link_us: time until the link came back, 0 if not reset */
> +static int rockchip_pcie_hot_reset(struct rockchip_pcie *rockchip, s64 *link_us)
> +{
> +	u32 status;
> +	ktime_t start;
> +	int err;
> +
> +	*link_us = 0;
> +	rockchip_pcie_write(rockchip, PCIE_CLIENT_HOT_RESET_ASSERT,
> +			    PCIE_CLIENT_HOT_RESET_CTRL);
> +	err = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_INT_STATUS,
> +				 status, status & PCIE_CLIENT_INT_HOT_RST,
> +				 100, 100 * USEC_PER_MSEC);
> +	rockchip_pcie_write(rockchip, PCIE_CLIENT_INT_HOT_RST,
> +			    PCIE_CLIENT_INT_STATUS);
> +	/* Hold the reset at least 2 ms, as pci_reset_secondary_bus() does */
> +	if (!err)
> +		usleep_range(2000, 3000);
> +	rockchip_pcie_write(rockchip, PCIE_CLIENT_HOT_RESET_DEASSERT,
> +			    PCIE_CLIENT_HOT_RESET_CTRL);
> +	if (err)
> +		return err;
> +
> +	start = ktime_get();
> +	err = readl_poll_timeout(rockchip->apb_base + PCIE_CLIENT_BASIC_STATUS1,
> +				 status, PCIE_LINK_UP(status), 20,
> +				 LINK_TRAIN_TIMEOUT);
> +	*link_us = ktime_us_delta(ktime_get(), start);
> +
> +	/* Clear the AXI link-down status before config traffic, as the Cadence host driver does */
> +	rockchip_pcie_write(rockchip, 0, PCIE_CORE_AXI_LINK_DOWN);
> +
> +	return err;
> +}
> +
> +/* A JMB585 sometimes comes out of reset unable to answer config requests */
> +static int rockchip_pcie_reset_endpoint(struct rockchip_pcie *rockchip)
> +{
> +	struct device *dev = rockchip->dev;
> +	s64 link_us;
> +	int err;
> +
> +	err = rockchip_pcie_hot_reset(rockchip, &link_us);
> +	dev_info(dev, "hot reset: link back in %lld us, err %d\n", link_us, err);
> +	if (err)
> +		return err;
> +
> +	msleep(PCIE_RESET_CONFIG_WAIT_MS);
> +	return 0;
> +}

[Severity: Medium]
In rockchip_pcie_hot_reset(), PCIE_CORE_AXI_LINK_DOWN is cleared immediately
after PCIE_LINK_UP(status) first polls true, before
rockchip_pcie_reset_endpoint() sleeps for 100 ms in
msleep(PCIE_RESET_CONFIG_WAIT_MS).

In the Cadence host driver, cdns_pci_map_bus() in
drivers/pci/controller/cadence/pcie-cadence-host.c checks that the link is up
and clears CDNS_PCIE_AT_LINKDOWN right before config accesses:

	/* Check that the link is up */
	if (!(cdns_pcie_readl(pcie, CDNS_PCIE_LM_BASE) & 0x1))
		return NULL;
	/* Clear AXI link-down status */
	cdns_pcie_writel(pcie, CDNS_PCIE_AT_LINKDOWN, 0x0);

Here, neither rockchip_pcie_reset_endpoint() after the 100 ms sleep nor
rockchip_pcie_rd_other_conf() / rockchip_pcie_wr_other_conf() verifies that
the link is still up or clears PCIE_CORE_AXI_LINK_DOWN.

If the endpoint link bounces or retrains during the 100 ms post-reset
initialization window, can hardware re-latch PCIE_CORE_AXI_LINK_DOWN (or
leave the link down) so that the first subsequent config read in
pci_host_probe() or pci_power_up() still triggers a synchronous external
abort or SError?

[ ... ]

> @@ -908,6 +959,10 @@ static int rockchip_pcie_resume_noirq(struct device *dev)
>  	if (err)
>  		goto err_pcie_resume;
>  
> +	err = rockchip_pcie_reset_endpoint(rockchip);
> +	if (err)
> +		goto err_err_deinit_port;
> +
>  	err = rockchip_pcie_cfg_atu(rockchip);
>  	if (err)
>  		goto err_err_deinit_port;
> @@ -965,6 +1020,10 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  	if (err)
>  		goto err_vpcie;
>  
> +	err = rockchip_pcie_reset_endpoint(rockchip);
> +	if (err)
> +		goto err_deinit_port;

[Severity: Medium]
Both rockchip_pcie_probe() and rockchip_pcie_resume_noirq() call
rockchip_pcie_init_port_retry() -> rockchip_pcie_host_init_port() before
calling rockchip_pcie_reset_endpoint() -> rockchip_pcie_hot_reset().

At the end of initial link training in rockchip_pcie_host_init_port(), the
driver latches rockchip->lanes_map and powers off all unused PHY lanes:

	/* Power off unused lane(s) */
	rockchip->lanes_map = rockchip_pcie_lane_map(rockchip);
	for (i = 0; i < MAX_LANE_NUM; i++) {
		if (!(rockchip->lanes_map & BIT(i))) {
			dev_dbg(dev, "idling lane %d\n", i);
			phy_power_off(rockchip->phys[i]);
		}
	}

When rockchip_pcie_hot_reset() then issues a hot reset and retrains the link,
those inactive PHY lanes have already been powered off, and neither
rockchip->lanes_map nor the PHY power state is updated after the hot reset.

If a multi-lane endpoint (such as the x2 JMB585 on Helios64 or an x4 device)
initially trains at a degraded width before the hot reset, will the powered-off
PHY lanes prevent the link from training to its full width during the hot
reset? Conversely, if a lane that trained initially fails to train during the
hot reset, will the newly unused PHY lane remain powered on with a stale
rockchip->lanes_map?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-rockchip-pcie-resume-v1-0-58e54a5cf5f1@iav.lv?part=2