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

[Manivannan Sadhasivam <mani@kernel.org>]

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
> 

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