[PATCH 1/2] scsi: ufs: ufs-qcom: Update PHY settings only when scaling to higher gears

[PATCH 1/2] scsi: ufs: ufs-qcom: Update PHY settings only when scaling to higher gears

[Manivannan Sadhasivam]

The "hs_gear" variable is used to program the PHY settings (submode) during
ufs_qcom_power_up_sequence(). Currently, it is being updated every time the
agreed gear changes. Due to this, if the gear got downscaled before suspend
(runtime/system), then while resuming, the PHY settings for the lower gear
will be applied first and later when scaling to max gear with REINIT, the
PHY settings for the max gear will be applied.

This adds a latency while resuming and also really not needed as the PHY
gear settings are backwards compatible i.e., we can continue using the PHY
settings for max gear with lower gear speed.

So let's update the "hs_gear" variable _only_ when the agreed gear is
greater than the current one. This guarantees that the PHY settings will be
changed only during probe time and fatal error condition.

Due to this, UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH can now be skipped
when the PM operation is in progress.

Cc: stable@vger.kernel.org
Fixes: 96a7141da332 ("scsi: ufs: core: Add support for reinitializing the UFS device")
Reported-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/core/ufshcd.c   | 3 ++-
 drivers/ufs/host/ufs-qcom.c | 9 +++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 34472871610d..1f0a9d96e613 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8782,7 +8782,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 	if (ret)
 		goto out;
 
-	if (hba->quirks & UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH) {
+	if (!hba->pm_op_in_progress &&
+	    (hba->quirks & UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH)) {
 		/* Reset the device and controller before doing reinit */
 		ufshcd_device_reset(hba);
 		ufshcd_hba_stop(hba);
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 78689d3479e4..ebb8054a3b3e 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -909,8 +909,13 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
 			return ret;
 		}
 
-		/* Use the agreed gear */
-		host->hs_gear = dev_req_params->gear_tx;
+		/*
+		 * Update hs_gear only when the gears are scaled to a higher value. This is because,
+		 * the PHY gear settings are backwards compatible and we only need to change the PHY
+		 * settings while scaling to higher gears.
+		 */
+		if (dev_req_params->gear_tx > host->hs_gear)
+			host->hs_gear = dev_req_params->gear_tx;
 
 		/* enable the device ref clock before changing to HS mode */
 		if (!ufshcd_is_hs_mode(&hba->pwr_info) &&
-- 
2.25.1

[PATCH 2/2] scsi: ufs: ufs-qcom: Rename "hs_gear" to "phy_gear"

[Manivannan Sadhasivam]

The "hs_gear" variable is used to cache the gear setting for the PHY that
will be used during ufs_qcom_power_up_sequence(). But it creates ambiguity
with the gear setting used by the ufshcd driver.

So let's rename it to "phy_gear" to make it explicit that this variable
caches the gear setting for the PHY.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 14 +++++++-------
 drivers/ufs/host/ufs-qcom.h |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index ebb8054a3b3e..93a72d0a1751 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -460,7 +460,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 		return ret;
 	}
 
-	phy_set_mode_ext(phy, PHY_MODE_UFS_HS_B, host->hs_gear);
+	phy_set_mode_ext(phy, PHY_MODE_UFS_HS_B, host->phy_gear);
 
 	/* power on phy - start serdes and phy's power and clocks */
 	ret = phy_power_on(phy);
@@ -910,12 +910,12 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
 		}
 
 		/*
-		 * Update hs_gear only when the gears are scaled to a higher value. This is because,
-		 * the PHY gear settings are backwards compatible and we only need to change the PHY
-		 * settings while scaling to higher gears.
+		 * Update phy_gear only when the gears are scaled to a higher value. This is
+		 * because, the PHY gear settings are backwards compatible and we only need to
+		 * change the PHY gear settings while scaling to higher gears.
 		 */
-		if (dev_req_params->gear_tx > host->hs_gear)
-			host->hs_gear = dev_req_params->gear_tx;
+		if (dev_req_params->gear_tx > host->phy_gear)
+			host->phy_gear = dev_req_params->gear_tx;
 
 		/* enable the device ref clock before changing to HS mode */
 		if (!ufshcd_is_hs_mode(&hba->pwr_info) &&
@@ -1282,7 +1282,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 	 * Power up the PHY using the minimum supported gear (UFS_HS_G2).
 	 * Switching to max gear will be performed during reinit if supported.
 	 */
-	host->hs_gear = UFS_HS_G2;
+	host->phy_gear = UFS_HS_G2;
 
 	return 0;
 
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index dc27395ecba1..8d8613eff959 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -227,7 +227,7 @@ struct ufs_qcom_host {
 
 	struct gpio_desc *device_reset;
 
-	u32 hs_gear;
+	u32 phy_gear;
 
 	int esi_base;
 	bool esi_enabled;
-- 
2.25.1

Re: [PATCH 1/2] scsi: ufs: ufs-qcom: Update PHY settings only when scaling to higher gears

[Konrad Dybcio]

On 8.09.2023 16:53, Manivannan Sadhasivam wrote:
> The "hs_gear" variable is used to program the PHY settings (submode) during
> ufs_qcom_power_up_sequence(). Currently, it is being updated every time the
> agreed gear changes. Due to this, if the gear got downscaled before suspend
> (runtime/system), then while resuming, the PHY settings for the lower gear
> will be applied first and later when scaling to max gear with REINIT, the
> PHY settings for the max gear will be applied.
> 
> This adds a latency while resuming and also really not needed as the PHY
> gear settings are backwards compatible i.e., we can continue using the PHY
> settings for max gear with lower gear speed.
> 
> So let's update the "hs_gear" variable _only_ when the agreed gear is
> greater than the current one. This guarantees that the PHY settings will be
> changed only during probe time and fatal error condition.
> 
> Due to this, UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH can now be skipped
> when the PM operation is in progress.
> 
> Cc: stable@vger.kernel.org
> Fixes: 96a7141da332 ("scsi: ufs: core: Add support for reinitializing the UFS device")
> Reported-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
Would that not increase power consumption?

I'd presume that the PHY needs to work harder at higher gear
settings to preserve signal integrity with more data flow.

And if so, would that power consumption increase be measurable?
Or is it so small that it doesn't matter?

Konrad

Re: [PATCH 1/2] scsi: ufs: ufs-qcom: Update PHY settings only when scaling to higher gears

[Manivannan Sadhasivam]

On Sat, Sep 09, 2023 at 01:04:52PM +0200, Konrad Dybcio wrote:
> On 8.09.2023 16:53, Manivannan Sadhasivam wrote:
> > The "hs_gear" variable is used to program the PHY settings (submode) during
> > ufs_qcom_power_up_sequence(). Currently, it is being updated every time the
> > agreed gear changes. Due to this, if the gear got downscaled before suspend
> > (runtime/system), then while resuming, the PHY settings for the lower gear
> > will be applied first and later when scaling to max gear with REINIT, the
> > PHY settings for the max gear will be applied.
> > 
> > This adds a latency while resuming and also really not needed as the PHY
> > gear settings are backwards compatible i.e., we can continue using the PHY
> > settings for max gear with lower gear speed.
> > 
> > So let's update the "hs_gear" variable _only_ when the agreed gear is
> > greater than the current one. This guarantees that the PHY settings will be
> > changed only during probe time and fatal error condition.
> > 
> > Due to this, UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH can now be skipped
> > when the PM operation is in progress.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 96a7141da332 ("scsi: ufs: core: Add support for reinitializing the UFS device")
> > Reported-by: Can Guo <quic_cang@quicinc.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> Would that not increase power consumption?
> 
> I'd presume that the PHY needs to work harder at higher gear
> settings to preserve signal integrity with more data flow.
> 
> And if so, would that power consumption increase be measurable?
> Or is it so small that it doesn't matter?
> 

Well, the power consumption won't be much. Currently the PHY driver supports
only 2 PHY init sequence, default one (G3/G4) and G4/G5. When ufshcd decides to
run at G4/G5, second sequence would be used and for rest of the gears, first one
would be used. So even today, the G3/G4 sequence is used when ufshcd decides to
downscale to lowest gear G1.

Moreover, on future SoCs the init sequence won't be compatible i.e., we cannot
switch between them. For these reasons, it makes sense to stick to the init
sequence of max gear.

- Mani

> Konrad

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

Re: [PATCH 1/2] scsi: ufs: ufs-qcom: Update PHY settings only when scaling to higher gears

[Can Guo]

On 9/8/2023 10:53 PM, Manivannan Sadhasivam wrote:
> The "hs_gear" variable is used to program the PHY settings (submode) during
> ufs_qcom_power_up_sequence(). Currently, it is being updated every time the
> agreed gear changes. Due to this, if the gear got downscaled before suspend
> (runtime/system), then while resuming, the PHY settings for the lower gear
> will be applied first and later when scaling to max gear with REINIT, the
> PHY settings for the max gear will be applied.
>
> This adds a latency while resuming and also really not needed as the PHY
> gear settings are backwards compatible i.e., we can continue using the PHY
> settings for max gear with lower gear speed.
>
> So let's update the "hs_gear" variable _only_ when the agreed gear is
> greater than the current one. This guarantees that the PHY settings will be
> changed only during probe time and fatal error condition.
>
> Due to this, UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH can now be skipped
> when the PM operation is in progress.
>
> Cc: stable@vger.kernel.org
> Fixes: 96a7141da332 ("scsi: ufs: core: Add support for reinitializing the UFS device")
> Reported-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/core/ufshcd.c   | 3 ++-
>   drivers/ufs/host/ufs-qcom.c | 9 +++++++--
>   2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 34472871610d..1f0a9d96e613 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8782,7 +8782,8 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
>   	if (ret)
>   		goto out;
>   
> -	if (hba->quirks & UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH) {
> +	if (!hba->pm_op_in_progress &&
> +	    (hba->quirks & UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH)) {
>   		/* Reset the device and controller before doing reinit */
>   		ufshcd_device_reset(hba);
>   		ufshcd_hba_stop(hba);
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 78689d3479e4..ebb8054a3b3e 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -909,8 +909,13 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>   			return ret;
>   		}
>   
> -		/* Use the agreed gear */
> -		host->hs_gear = dev_req_params->gear_tx;
> +		/*
> +		 * Update hs_gear only when the gears are scaled to a higher value. This is because,
> +		 * the PHY gear settings are backwards compatible and we only need to change the PHY
> +		 * settings while scaling to higher gears.
> +		 */
> +		if (dev_req_params->gear_tx > host->hs_gear)
> +			host->hs_gear = dev_req_params->gear_tx;
>   
>   		/* enable the device ref clock before changing to HS mode */
>   		if (!ufshcd_is_hs_mode(&hba->pwr_info) &&

Reviewed-by: Can Guo <quic_cang@quicinc.com>

Tested-by: Can Guo <quic_cang@quicinc.com>

Re: [PATCH 2/2] scsi: ufs: ufs-qcom: Rename "hs_gear" to "phy_gear"

[Can Guo]

On 9/8/2023 10:53 PM, Manivannan Sadhasivam wrote:
> The "hs_gear" variable is used to cache the gear setting for the PHY that
> will be used during ufs_qcom_power_up_sequence(). But it creates ambiguity
> with the gear setting used by the ufshcd driver.
>
> So let's rename it to "phy_gear" to make it explicit that this variable
> caches the gear setting for the PHY.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/host/ufs-qcom.c | 14 +++++++-------
>   drivers/ufs/host/ufs-qcom.h |  2 +-
>   2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index ebb8054a3b3e..93a72d0a1751 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -460,7 +460,7 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>   		return ret;
>   	}
>   
> -	phy_set_mode_ext(phy, PHY_MODE_UFS_HS_B, host->hs_gear);
> +	phy_set_mode_ext(phy, PHY_MODE_UFS_HS_B, host->phy_gear);
>   
>   	/* power on phy - start serdes and phy's power and clocks */
>   	ret = phy_power_on(phy);
> @@ -910,12 +910,12 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>   		}
>   
>   		/*
> -		 * Update hs_gear only when the gears are scaled to a higher value. This is because,
> -		 * the PHY gear settings are backwards compatible and we only need to change the PHY
> -		 * settings while scaling to higher gears.
> +		 * Update phy_gear only when the gears are scaled to a higher value. This is
> +		 * because, the PHY gear settings are backwards compatible and we only need to
> +		 * change the PHY gear settings while scaling to higher gears.
>   		 */
> -		if (dev_req_params->gear_tx > host->hs_gear)
> -			host->hs_gear = dev_req_params->gear_tx;
> +		if (dev_req_params->gear_tx > host->phy_gear)
> +			host->phy_gear = dev_req_params->gear_tx;
>   
>   		/* enable the device ref clock before changing to HS mode */
>   		if (!ufshcd_is_hs_mode(&hba->pwr_info) &&
> @@ -1282,7 +1282,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>   	 * Power up the PHY using the minimum supported gear (UFS_HS_G2).
>   	 * Switching to max gear will be performed during reinit if supported.
>   	 */
> -	host->hs_gear = UFS_HS_G2;
> +	host->phy_gear = UFS_HS_G2;
>   
>   	return 0;
>   
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index dc27395ecba1..8d8613eff959 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -227,7 +227,7 @@ struct ufs_qcom_host {
>   
>   	struct gpio_desc *device_reset;
>   
> -	u32 hs_gear;
> +	u32 phy_gear;
>   
>   	int esi_base;
>   	bool esi_enabled;

Reviewed-by: Can Guo <quic_cang@quicinc.com>

Tested-by: Can Guo <quic_cang@quicinc.com>

Re: [PATCH 1/2] scsi: ufs: ufs-qcom: Update PHY settings only when scaling to higher gears

[Martin K. Petersen]

Manivannan,

> The "hs_gear" variable is used to program the PHY settings (submode)
> during ufs_qcom_power_up_sequence(). Currently, it is being updated
> every time the agreed gear changes. Due to this, if the gear got
> downscaled before suspend (runtime/system), then while resuming, the
> PHY settings for the lower gear will be applied first and later when
> scaling to max gear with REINIT, the PHY settings for the max gear
> will be applied.

Applied to 6.7/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering