Re: [PATCH v3 1/4] phy: qcom: qmp-pcie: Skip PHY reset if already up

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `skip_reset` condition is evaluated independently in both `qmp_pcie_init()` and `qmp_pcie_power_on()`, which can lead to unbalanced reset operations and leave the PHY permanently in reset.
--

--- Patch [1]: [PATCH v3 1/4] phy: qcom: qmp-pcie: Skip PHY reset if already up ---
commit 3002076fcac7c2d6108104d2911e0cbb6c6bf224
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

phy: qcom: qmp-pcie: Skip PHY reset if already up

This patch extends the skip_init handling by introducing a skip_reset
condition for the qmp-pcie phy. It skips asserting and deasserting the
no-csr reset if the bootloader has already powered up the PCIe PHY and
it is operational. This reuses the bootloader-initialized PHY state to
save boot time and avoid redundant PCIe link retraining.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index d3effad7a074b..49e10151cb1e4 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -4862,6 +4862,7 @@ static int qmp_pcie_init(struct phy *phy)
>  	struct qmp_pcie *qmp = phy_get_drvdata(phy);
>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
>  	void __iomem *pcs = qmp->pcs;
> +	bool skip_reset;
>  	int ret;
>  
>  	/*
> @@ -4877,6 +4878,9 @@ static int qmp_pcie_init(struct phy *phy)
>  		qphy_checkbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START) &&
>  		qphy_checkbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], cfg->pwrdn_ctrl);
>  
> +	skip_reset = qmp->skip_init && !qphy_checkbits(pcs, cfg->regs[QPHY_PCS_STATUS],
> +							    cfg->phy_status);
> +
>  	if (!qmp->skip_init && !cfg->tbls.serdes_num) {
>  		dev_err(qmp->dev, "Init sequence not available\n");
>  		return -ENODATA;

[ ... ]

> @@ -4900,13 +4904,15 @@ static int qmp_pcie_init(struct phy *phy)
>  		}
>  	}
>  
> -	ret = reset_control_assert(qmp->nocsr_reset);
> -	if (ret) {
> -		dev_err(qmp->dev, "no-csr reset assert failed\n");
> -		goto err_assert_reset;
> -	}
> +	if (!skip_reset) {
> +		ret = reset_control_assert(qmp->nocsr_reset);
> +		if (ret) {
> +			dev_err(qmp->dev, "no-csr reset assert failed\n");
> +			goto err_assert_reset;
> +		}
>  
> -	usleep_range(200, 300);
> +		usleep_range(200, 300);
> +	}
>  
>  	if (!qmp->skip_init) {
>  		ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets);

[ ... ]

> @@ -4956,8 +4962,11 @@ static int qmp_pcie_power_on(struct phy *phy)
>  	void __iomem *pcs = qmp->pcs;
>  	void __iomem *status;
>  	unsigned int mask, val;
> +	bool skip_reset;
>  	int ret;
>  
> +	skip_reset = qmp->skip_init && !qphy_checkbits(pcs, cfg->regs[QPHY_PCS_STATUS],
> +							    cfg->phy_status);

[Severity: High]
Will this lead to unbalanced reset operations if the hardware state changes
between qmp_pcie_init() and qmp_pcie_power_on()?

In qmp_pcie_init(), skip_reset is computed by reading the live PHY status
register. If it evaluates to false, nocsr_reset is asserted. However,
asserting the reset could alter the state of the PHY status register.

When qmp_pcie_power_on() subsequently executes, it reads the status register
again here. If the register now indicates the PHY is ready, skip_reset would
evaluate to true, and the driver would skip reset_control_deassert() below,
leaving the PHY permanently in reset.

Could we cache the skip_reset decision in struct qmp_pcie during init rather
than re-evaluating the live register here?

>  	/*
>  	 * Write CSR register for PHY that doesn't support no_csr reset or has not
>  	 * been initialized.
> @@ -4981,10 +4990,12 @@ static int qmp_pcie_power_on(struct phy *phy)
>  	if (ret)
>  		return ret;
>  
> -	ret = reset_control_deassert(qmp->nocsr_reset);
> -	if (ret) {
> -		dev_err(qmp->dev, "no-csr reset deassert failed\n");
> -		goto err_disable_pipe_clk;
> +	if (!skip_reset) {
> +		ret = reset_control_deassert(qmp->nocsr_reset);
> +		if (ret) {
> +			dev_err(qmp->dev, "no-csr reset deassert failed\n");
> +			goto err_disable_pipe_clk;
> +		}
>  	}
>  
>  	if (qmp->skip_init)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-link_retain-v3-0-81a9d187bb61@oss.qualcomm.com?part=1