Re: [PATCH V4 09/11] scsi: ufs: qcom : Refactor phy_power_on/off calls

[Nitin Rawat <quic_nitirawa@quicinc.com>]

On 5/9/2025 5:05 PM, Konrad Dybcio wrote:
> On 5/3/25 6:24 PM, Nitin Rawat wrote:
>> Commit 3f6d1767b1a0 ("phy: ufs-qcom: Refactor all init steps into
>> phy_poweron") removes the phy_power_on/off from ufs_qcom_setup_clocks
>> to suspend/resume func.
>>
>> To have a better power saving, remove the phy_power_on/off calls from
>> resume/suspend path and put them back to ufs_qcom_setup_clocks, so that
>> PHY regulators & clks can be turned on/off along with UFS's clocks.
>>
>> Since phy phy_power_on is separated out from phy calibrate, make
>> separate calls to phy_power_on and phy_calibrate calls from ufs qcom
>> driver.
>>
>> Co-developed-by: Can Guo <quic_cang@quicinc.com>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 55 ++++++++++++++++---------------------
>>   1 file changed, 23 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 2cd44ee522b8..ff35cd15c72f 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -639,26 +639,17 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
>>   	enum ufs_notify_change_status status)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> -	struct phy *phy = host->generic_phy;
>>   
>>   	if (status == PRE_CHANGE)
>>   		return 0;
>>   
>> -	if (ufs_qcom_is_link_off(hba)) {
>> -		/*
>> -		 * Disable the tx/rx lane symbol clocks before PHY is
>> -		 * powered down as the PLL source should be disabled
>> -		 * after downstream clocks are disabled.
>> -		 */
>> +	if (!ufs_qcom_is_link_active(hba))
> 
> so is_link_off and !is_link_active are not the same thing - this also allows
> for disabling the resources when in hibern8/broken states - is that intended?

Yes is_link_off and !is_link_Active is not same thing. !is_link_active 
also include link in hibern8 state where lane clock is intended to be
disabled because PHY is powered down in hibern8.



> 
>>   		ufs_qcom_disable_lane_clks(host);
>> -		phy_power_off(phy);
>>   
>> -		/* reset the connected UFS device during power down */
>> -		ufs_qcom_device_reset_ctrl(hba, true);
>>   
>> -	} else if (!ufs_qcom_is_link_active(hba)) {
>> -		ufs_qcom_disable_lane_clks(host);
>> -	}
>> +	/* reset the connected UFS device during power down */
>> +	if (ufs_qcom_is_link_off(hba) && host->device_reset)
>> +		ufs_qcom_device_reset_ctrl(hba, true);
> 
> similarly this will not be allowed in hibern8/broken states now
> 
>>   
>>   	return ufs_qcom_ice_suspend(host);
>>   }
>> @@ -666,26 +657,11 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
>>   static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> -	struct phy *phy = host->generic_phy;
>>   	int err;
>>   
>> -	if (ufs_qcom_is_link_off(hba)) {
>> -		err = phy_power_on(phy);
>> -		if (err) {
>> -			dev_err(hba->dev, "%s: failed PHY power on: %d\n",
>> -				__func__, err);
>> -			return err;
>> -		}
>> -
>> -		err = ufs_qcom_enable_lane_clks(host);
>> -		if (err)
>> -			return err;
>> -
>> -	} else if (!ufs_qcom_is_link_active(hba)) {
>> -		err = ufs_qcom_enable_lane_clks(host);
>> -		if (err)
>> -			return err;
>> -	}
>> +	err = ufs_qcom_enable_lane_clks(host);
>> +	if (err)
>> +		return err;
>>   
>>   	return ufs_qcom_ice_resume(host);
>>   }
>> @@ -1042,6 +1018,8 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>>   				 enum ufs_notify_change_status status)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> +	struct phy *phy = host->generic_phy;
>> +	int err;
>>   
>>   	/*
>>   	 * In case ufs_qcom_init() is not yet done, simply ignore.
>> @@ -1060,10 +1038,22 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>>   				/* disable device ref_clk */
>>   				ufs_qcom_dev_ref_clk_ctrl(host, false);
>>   			}
>> +			err = phy_power_off(phy);
> 
> a newline to separate the blocks would improve readability> +			if (err) {
>> +				dev_err(hba->dev, "%s: phy power off failed, ret=%d\n",
>> +					__func__, err);
>> +					return err;

Sure will add in next patchset.

> 
> please indent the return statement a tab earlier so it doesn't look
> like it's an argument to dev_err()

Sure will add in next patchset.

> 
> putting PHY calls in the function that promises to toggle clocks could
> also use a comment, maybe extending the kerneldoc to say that certain
> clocks come from the PHY so it needs to be managed together
> 
Sure will add a comment or update in kernel doc in next patchset.

> Konrad


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy