Re: [PATCH v5 phy-next 10/27] scsi: ufs: qcom: keep parallel track of PHY power state

Manivannan Sadhasivam
2026-03-24T05:30:48Z

On Fri, Mar 20, 2026 at 12:32:24AM +0200, Vladimir Oltean wrote:
> As explained in the similar ufs-exynos.c change, PHY consumer drivers
> should not look at the phy->power_count, because in the general case
> there might also be other consumers who have called phy_power_on() too,
> so the fact that the power_count is non-zero does not mean that we did.
>
> Moreover, struct phy will become opaque soon, so the qcom UFS driver
> will not be able to apply this pattern. Keep parallel track of the PHY
> power state, instead of looking at a field which will become unavailable
> (phy->power_count).
>
> About treating the phy_power_off() return code: from an API perspective,
> this should have probably returned void, otherwise consumers would be
> stuck in a state they can't escape. The provider, phy-qcom-qmp-ufs.c,
> does return 0 in its power_off() implementation. I consider it safe to
> discard potential errors from phy_power_off() instead of complicating
> the phy_powered_on logic.
>

You could even simplify the code by getting rid of the 'phy_powered_on' check
altogether. There is no real need to track the PHY power state in this driver.
It is safe to call phy_power_off() without any checks.

- Mani

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: Nitin Rawat <quic_nitirawa@quicinc.com>
>
> v4->v5: patch is new
> ---
> drivers/ufs/host/ufs-qcom.c | 9 +++++++--
> drivers/ufs/host/ufs-qcom.h | 1 +
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 375fd24ba458..3b8bd9968235 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -508,9 +508,10 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> if (ret)
> return ret;
>
> - if (phy->power_count)
> + if (host->phy_powered_on) {
> phy_power_off(phy);
> -
> + host->phy_powered_on = false;
> + }
>
> /* phy initialization - calibrate the phy */
> ret = phy_init(phy);
> @@ -531,6 +532,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> __func__, ret);
> goto out_disable_phy;
> }
> + host->phy_powered_on = true;
>
> ret = phy_calibrate(phy);
> if (ret) {
> @@ -1268,6 +1270,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
> dev_err(hba->dev, "phy power off failed, ret=%d\n", err);
> return err;
> }
> + host->phy_powered_on = false;
> }
> break;
> case POST_CHANGE:
> @@ -1277,6 +1280,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
> dev_err(hba->dev, "phy power on failed, ret = %d\n", err);
> return err;
> }
> + host->phy_powered_on = true;
>
> /* enable the device ref clock for HS mode*/
> if (ufshcd_is_hs_mode(&hba->pwr_info))
> @@ -1467,6 +1471,7 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
>
> ufs_qcom_disable_lane_clks(host);
> phy_power_off(host->generic_phy);
> + host->phy_powered_on = false;
> phy_exit(host->generic_phy);
> }
>
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 1111ab34da01..72ce0687fa42 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -282,6 +282,7 @@ struct ufs_qcom_host {
> struct clk_bulk_data *clks;
> u32 num_clks;
> bool is_lane_clks_enabled;
> + bool phy_powered_on;
>
> struct icc_path *icc_ddr;
> struct icc_path *icc_cpu;
> --
> 2.43.0
>

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