Re: [PATCH v3] PCI: rockchip: Don't lengthen PERST# when power is already valid

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PCI: rockchip: missing T_PERST-CLK delay in rockchip_pcie_host_init_port()
--

commit 3ad27c7e477b1d892784f009b205c0444bfcd8fb
Author: Enrique Hernández Bello <ehbello@gmail.com>

PCI: rockchip: Don't lengthen PERST# when power is already valid

This commit checks whether all PCIe voltage supplies are already enabled
before the driver enables them during probe and resume, and skips the
PCIE_T_PVPERL_MS sleep before deasserting PERST# when power was already valid
to avoid lengthening the PERST# pulse.

> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index 5b071bdc4f903..2bfbd3481c5b2 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -315,7 +315,9 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
>  			    PCIE_CLIENT_CONFIG);
>  
> -	msleep(PCIE_T_PVPERL_MS);
> +	if (!rockchip->supplies_pre_enabled)
> +		msleep(PCIE_T_PVPERL_MS);
> +
>  	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);

[Severity: Medium]
When rockchip->supplies_pre_enabled is true during rockchip_pcie_probe() ->
rockchip_pcie_host_init_port() or rockchip_pcie_resume_noirq() ->
rockchip_pcie_host_init_port(), does skipping msleep(PCIE_T_PVPERL_MS)
without a fallback fsleep(PCIE_T_PERST_CLK_US) violate the PCIe CEM
T_PERST-CLK timing requirement?

Commit c47f90be4c89 ("PCI: rockchip-host: Fix rockchip_pcie_host_init_port()
PERST# handling") added msleep(PCIE_T_PVPERL_MS) to satisfy both T_PVPERL
(100 ms after power is stable) and T_PERST-CLK (PCIE_T_PERST_CLK_US, 100 us
minimum after the reference clock is stable before deasserting PERST#).

Even when the voltage regulators are already enabled,
rockchip_pcie_host_init_port() calls rockchip_pcie_init_port() in
drivers/pci/controller/pcie-rockchip.c, which resets the PHY via phy_init() ->
rockchip_pcie_phy_init() and then configures and locks the 100 MHz reference
clock PLL in phy_power_on() -> rockchip_pcie_phy_power_on() in
drivers/phy/rockchip/phy-rockchip-pcie.c:

	phy_wr_cfg(rk_phy, PHY_CFG_CLK_TEST, PHY_CFG_SEPE_RATE);
	phy_wr_cfg(rk_phy, PHY_CFG_CLK_SCC, PHY_CFG_PLL_100M);

	err = regmap_read_poll_timeout(rk_phy->reg_base,
				       rk_phy->phy_data->pcie_status,
				       status,
				       !(status & PHY_PLL_OUTPUT),
				       200, 100000);

and then polls PCIE_CLIENT_SIDE_BAND_STATUS in rockchip_pcie_init_port():

	err = readx_poll_timeout(rockchip_pcie_read_addr,
				 PCIE_CLIENT_SIDE_BAND_STATUS,
				 regs, !(regs & PCIE_CLIENT_PHY_ST),
				 RK_PHY_PLL_LOCK_SLEEP_US,
				 RK_PHY_PLL_LOCK_TIMEOUT_US);

Once rockchip_pcie_init_port() returns with the reference clock stable,
rockchip_pcie_host_init_port() only performs a few APB MMIO register accesses
(and rockchip_pcie_set_power_limit(), which is a no-op when vpcie3v3 is
absent) before deasserting rockchip->perst_gpio. Additionally, on the
rockchip_pcie_resume_noirq() path, PERST# is only asserted at the start of
rockchip_pcie_host_init_port(), so if the PLL lock poll loops succeed on their
first read, the active PERST# pulse width is also not guaranteed to meet
PCIE_T_PERST_US (100 us).

Should an else branch with fsleep(PCIE_T_PERST_CLK_US) be used when
rockchip->supplies_pre_enabled is true so the 100 us reference clock
stabilization delay before PERST# deassertion is still met?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028005603.2399220-1-ehbello@gmail.com?part=1