Re: [PATCH 4/4] phy: qcom-qmp-pcie: Split phy_ops into init/exit/reset/power_on

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] phy: qcom-qmp-pcie: nocsr_reset left asserted when skip_init is false
--

commit 09fee607f4ad44d50cee755a3e356ff08d5879e2
Author: Qiang Yu <qiang.yu@oss.qualcomm.com>

phy: qcom-qmp-pcie: Split phy_ops into init/exit/reset/power_on

This commit splits the combined power_on and power_off operations into
separate init, exit, reset, and power_on functions. It reorders regulator,
clock, and reset toggling to align with HPG requirements. It also introduces
a skip_reset field to share the initialization state between functions.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index d30194c6ed17f..2b6741a4120c5 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c

[ ... ]

> @@ -5833,12 +5809,62 @@ static int qmp_pcie_exit(struct phy *phy)
>  		reset_control_bulk_assert(cfg->num_resets, qmp->resets);
>  
>  	clk_bulk_disable_unprepare(ARRAY_SIZE(qmp_pciephy_clk_l), qmp->clks);
> +	clk_bulk_disable_unprepare(qmp->num_pipe_clks, qmp->pipe_clks);
>  
>  	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>  
>  	return 0;
>  }
>  
> +static int qmp_pcie_reset(struct phy *phy)
> +{
> +	struct qmp_pcie *qmp = phy_get_drvdata(phy);
> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
> +	int ret;
> +
> +	if (qmp->skip_reset)
> +		return 0;
> +
> +	if (qmp->skip_init) {
> +		ret = reset_control_assert(qmp->nocsr_reset);
> +		if (ret) {
> +			dev_err(qmp->dev, "no-csr reset assert failed\n");
> +			return ret;
> +		}
> +
> +		usleep_range(200, 300);
> +
> +		ret = reset_control_deassert(qmp->nocsr_reset);
> +		if (ret) {
> +			dev_err(qmp->dev, "no-csr reset deassert failed\n");
> +			return ret;
> +		}
> +
> +		return 0;
> +	}
> +
> +	/*
> +	 * Toggle BCR reset for PHY that doesn't support no_csr reset or has not
> +	 * been initialized.
> +	 */
> +	ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets);
[Severity: High]
Does this fallback path in qmp_pcie_reset() fail to deassert nocsr_reset
when skip_init evaluates to false?

Since qmp_pcie_exit() explicitly asserts nocsr_reset on exit, if skip_init
is false on the next initialization (for example, after a system suspend or
resume where power loss clears PHY CSRs like QPHY_START_CTRL), wouldn't this
execution path only toggle the bulk resets and leave nocsr_reset stuck in
the asserted state?

> +	if (ret) {
> +		dev_err(qmp->dev, "reset assert failed\n");
> +		return ret;
> +	}
> +
> +	usleep_range(200, 300);
> +
> +	ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets);
> +	if (ret) {
> +		dev_err(qmp->dev, "reset deassert failed\n");
> +		reset_control_bulk_assert(cfg->num_resets, qmp->resets);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int qmp_pcie_power_on(struct phy *phy)
>  {
>  	struct qmp_pcie *qmp = phy_get_drvdata(phy);

[ ... ]

> @@ -5870,22 +5893,6 @@ static int qmp_pcie_power_on(struct phy *phy)
>  	qmp_pcie_init_registers(qmp, &cfg->tbls);
>  	qmp_pcie_init_registers(qmp, mode_tbls);
>  
> -skip_tbls_init:
> -	ret = clk_bulk_prepare_enable(qmp->num_pipe_clks, qmp->pipe_clks);
> -	if (ret)
> -		return ret;
> -
> -	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)
> -		goto skip_serdes_start;
> -
>  	/* Pull PHY out of reset state */
>  	qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
[Severity: High]
With the explicit nocsr_reset deassertion removed from qmp_pcie_power_on(),
does the code now rely entirely on qmp_pcie_reset() to deassert it?

Since qmp_pcie_reset() doesn't seem to deassert nocsr_reset when skip_init
is false, this appears to set the PHY up to silently fail or time out on
initialization.

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915-align_pcie_init_sequence_0916-v1-0-0b2195836f30@oss.qualcomm.com?part=4