Re: [PATCH V5 10/11] scsi: ufs: qcom : Introduce phy_power_on/off wrapper function

Nitin Rawat
2025-05-21T22:19:09Z

On 5/21/2025 7:31 PM, Manivannan Sadhasivam wrote:
> On Thu, May 15, 2025 at 09:57:21PM +0530, Nitin Rawat wrote:
>
> Subject should mention ufs_qcom_phy_power_{on/off} as phy_power_{on/off} are
> generic APIs.
>
>> There can be scenarios where phy_power_on is called when PHY is
>> already on (phy_count=1). For instance, ufs_qcom_power_up_sequence
>> can be called multiple times from ufshcd_link_startup as part of
>> ufshcd_hba_enable call for each link startup retries(max retries =3),
>> causing the PHY reference count to increase and leading to inconsistent
>> phy behavior.
>>
>> Similarly, there can be scenarios where phy_power_on or phy_power_off
>> might be called directly from the UFS controller driver when the PHY
>> is already powered on or off respectiely, causing similar issues.
>>
>> To fix this, introduce ufs_qcom_phy_power_on and ufs_qcom_phy_power_off
>> wrappers for phy_power_on and phy_power_off. These wrappers will use an
>> is_phy_pwr_on flag to check if the PHY is already powered on or off,
>> avoiding redundant calls. Protect the is_phy_pwr_on flag with a mutex
>> to ensure safe usage and prevent race conditions.
>>
>
> This smells like the phy_power_{on/off} calls are not balanced and you are
> trying to workaround that in the UFS driver.

Hi Mani,

Yes, there can be scenarios that were not previously encountered because
phy_power_on and phy_power_off were only called during system suspend
(spm_lvl = 5). However, with phy_power_on now moved to
ufs_qcom_setup_clocks, there is a slightly more probability of
phy_power_on being called twice, i.e., phy_power_on being invoked when
the PHY is already on.

For instance, if the PHY power is already on and the UFS driver calls
ufs_qcom_setup_clocks from an error handling context, phy_power_on could
be called again which may increase phy_count and can cause inconsistent
phy bheaviour . Therefore, we need to have a flag, is_phy_pwr_on, in the
controller driver, protected by a mutex, to indicate the state of
phy_power_on and phy_power_off.

This approach is also present in Qualcomm downstream UFS driver and
similiar solution in mtk ufs driver to have flag in controller
indictring phy power state in their upstream UFS drivers.

Regards,
Nitin

>> + host->is_phy_pwr_on = false;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>> {
>> struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> @@ -507,7 +539,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>> return ret;
>>
>> if (phy->power_count) {
>> - phy_power_off(phy);
>> + ufs_qcom_phy_power_off(hba);
>> phy_exit(phy);
>> }
>>
>> @@ -524,7 +556,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>> goto out_disable_phy;
>>
>> /* power on phy - start serdes and phy's power and clocks */
>> - ret = phy_power_on(phy);
>> + ret = ufs_qcom_phy_power_on(hba);
>> if (ret) {
>> dev_err(hba->dev, "%s: phy power on failed, ret = %d\n",
>> __func__, ret);
>> @@ -1121,7 +1153,6 @@ 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;
>>
>> /*
>> @@ -1142,7 +1173,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>> ufs_qcom_dev_ref_clk_ctrl(host, false);
>> }
>>
>> - err = phy_power_off(phy);
>> + err = ufs_qcom_phy_power_off(hba);
>> if (err) {
>> dev_err(hba->dev, "phy power off failed, ret=%d\n", err);
>> return err;
>> @@ -1151,7 +1182,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>> break;
>> case POST_CHANGE:
>> if (on) {
>> - err = phy_power_on(phy);
>> + err = ufs_qcom_phy_power_on(hba);
>> if (err) {
>> dev_err(hba->dev, "phy power on failed, ret = %d\n", err);
>> return err;
>> @@ -1339,10 +1370,9 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>> static void ufs_qcom_exit(struct ufs_hba *hba)
>> {
>> struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> - struct phy *phy = host->generic_phy;
>>
>> ufs_qcom_disable_lane_clks(host);
>> - phy_power_off(phy);
>> + ufs_qcom_phy_power_off(hba);
>> phy_exit(host->generic_phy);
>> }
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
>> index 0a5cfc2dd4f7..f51b774e17be 100644
>> --- a/drivers/ufs/host/ufs-qcom.h
>> +++ b/drivers/ufs/host/ufs-qcom.h
>> @@ -281,6 +281,10 @@ struct ufs_qcom_host {
>> u32 phy_gear;
>>
>> bool esi_enabled;
>> + /* flag to check if phy is powered on */
>> + bool is_phy_pwr_on;
>> + /* Protect the usage of is_phy_pwr_on against racing */
>> + struct mutex phy_mutex;
>> };
>>
>> struct ufs_qcom_drvdata {
>> --
>> 2.48.1
>>
>