[v2,3/7] scsi: ufs: ufs-qcom: Allow the first init start with the maximum supported gear

Commit Message

Can Guo Nov. 7, 2023, 4:46 a.m. UTC

From: Can Guo <[e-mail]>

During host driver init, the phy_gear is set to the minimum supported gear
(HS_G2). Then, during the first power mode change, the negotiated gear, say
HS-G4, is updated to the phy_gear variable so that in the second init the
updated phy_gear can be used to program the PHY.

But the current code only allows update the phy_gear to a higher value. If
one wants to start the first init with the maximum support gear, say HS-G4,
the phy_gear is not updated to HS-G3 if the device only supports HS-G3.

The original check added there is intend to make sure the phy_gear won't be
updated when gear is scaled down (during clock scaling). Update the check
so that one can start the first init with the maximum support gear without
breaking the original fix by checking the ufshcd_state, that is, allow
update to phy_gear only if power mode change is invoked from
ufshcd_probe_hba().

This change is a preparation patch for the next patches in the same series.

Signed-off-by: Can Guo <[e-mail]>
---
 drivers/ufs/host/ufs-qcom.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Manivannan Sadhasivam Nov. 8, 2023, 5:23 a.m. UTC | #1

On Mon, Nov 06, 2023 at 08:46:09PM -0800, Can Guo wrote:
> From: Can Guo <[e-mail]>
> 
> During host driver init, the phy_gear is set to the minimum supported gear
> (HS_G2). Then, during the first power mode change, the negotiated gear, say
> HS-G4, is updated to the phy_gear variable so that in the second init the
> updated phy_gear can be used to program the PHY.
> 
> But the current code only allows update the phy_gear to a higher value. If
> one wants to start the first init with the maximum support gear, say HS-G4,
> the phy_gear is not updated to HS-G3 if the device only supports HS-G3.
> 

Can you elaborate when this can happen? AFAICS, there are 3 possibilities of
initial phy gear with this series:

1. If ufshc is < 5.0, then G2 will be used.
2. If ufshc is >= 5.0 and if the version is populated in register, then that
gear will be used. Most likely that gear can be G4/G5 depending on the device
connected.
3. If ufshc is >=5.0 and version is not populated, then G4 will be used.

In all the above cases, I do not see any necessity to switch the phy gear
setting to lower one while scaling. Since the gears are backwards compatible,
we always use one phy gear sequence. Moreover, we only have 2 init sequences.

Please correct me if I'm missing anything.

- Mani

> The original check added there is intend to make sure the phy_gear won't be
> updated when gear is scaled down (during clock scaling). Update the check
> so that one can start the first init with the maximum support gear without
> breaking the original fix by checking the ufshcd_state, that is, allow
> update to phy_gear only if power mode change is invoked from
> ufshcd_probe_hba().
> 
> This change is a preparation patch for the next patches in the same series.
> 
> Signed-off-by: Can Guo <[e-mail]>
> ---
>  drivers/ufs/host/ufs-qcom.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index cc0eb37..60b35ca 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -916,11 +916,12 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>  		}
>  
>  		/*
> -		 * 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.
> +		 * During UFS driver probe, always update the PHY gear to match the negotiated
> +		 * gear, so that, if quirk UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH is enabled,
> +		 * the second init can program the optimal PHY settings. This allows one to start
> +		 * the first init with either the minimum or the maximum support gear.
>  		 */
> -		if (dev_req_params->gear_tx > host->phy_gear)
> +		if (hba->ufshcd_state == UFSHCD_STATE_RESET)
>  			host->phy_gear = dev_req_params->gear_tx;
>  
>  		/* enable the device ref clock before changing to HS mode */
> -- 
> 2.7.4
>

Can Guo Nov. 8, 2023, 8:21 a.m. UTC | #2

Hi Mani,

On 11/8/2023 1:23 PM, Manivannan Sadhasivam wrote:
> On Mon, Nov 06, 2023 at 08:46:09PM -0800, Can Guo wrote:
>> From: Can Guo <[e-mail]>
>>
>> During host driver init, the phy_gear is set to the minimum supported gear
>> (HS_G2). Then, during the first power mode change, the negotiated gear, say
>> HS-G4, is updated to the phy_gear variable so that in the second init the
>> updated phy_gear can be used to program the PHY.
>>
>> But the current code only allows update the phy_gear to a higher value. If
>> one wants to start the first init with the maximum support gear, say HS-G4,
>> the phy_gear is not updated to HS-G3 if the device only supports HS-G3.
>>
> 
> Can you elaborate when this can happen? AFAICS, there are 3 possibilities of
> initial phy gear with this series:
> 
> 1. If ufshc is < 5.0, then G2 will be used.
> 2. If ufshc is >= 5.0 and if the version is populated in register, then that
> gear will be used. Most likely that gear can be G4/G5 depending on the device
> connected.
> 3. If ufshc is >=5.0 and version is not populated, then G4 will be used.
> 
> In all the above cases, I do not see any necessity to switch the phy gear
> setting to lower one while scaling. Since the gears are backwards compatible,
> we always use one phy gear sequence. Moreover, we only have 2 init sequences.
> 
> Please correct me if I'm missing anything.
> 
> - Mani
In the next patch, I am setting the initial PHY gear to max HS gear read
from UFS host cap register, so that we don't need to keep updating the 
initial value for host->phy_gear for different HW versions in future. 
FYI, for HW ver 5 and 6, it is HS-G5. In future, the max gear might 
become HS-G6 or higher on newer HW verions.

I the case #3, if HS-G5 is set to host->phy_gear, the first init uses 
HS-G5, then after negotiation if the agreed gear is HS-G4, we need to 
update host->phy_gear to HS-G4 (a lower value) such that we use a power 
saving PHY gear settings during the 2nd init.

If the commit message is making you confused, I can update it in next 
version. Please let me if I made any mistakes here.

Thanks,
Can Guo.

Can Guo Nov. 8, 2023, 8:23 a.m. UTC | #3

Hi Mani,

On 11/8/2023 4:21 PM, Can Guo wrote:
> Hi Mani,
> 
> On 11/8/2023 1:23 PM, Manivannan Sadhasivam wrote:
>> On Mon, Nov 06, 2023 at 08:46:09PM -0800, Can Guo wrote:
>>> From: Can Guo <[e-mail]>
>>>
>>> During host driver init, the phy_gear is set to the minimum supported 
>>> gear
>>> (HS_G2). Then, during the first power mode change, the negotiated 
>>> gear, say
>>> HS-G4, is updated to the phy_gear variable so that in the second init 
>>> the
>>> updated phy_gear can be used to program the PHY.
>>>
>>> But the current code only allows update the phy_gear to a higher 
>>> value. If
>>> one wants to start the first init with the maximum support gear, say 
>>> HS-G4,
>>> the phy_gear is not updated to HS-G3 if the device only supports HS-G3.
>>>
>>
>> Can you elaborate when this can happen? AFAICS, there are 3 
>> possibilities of
>> initial phy gear with this series:
>>
>> 1. If ufshc is < 5.0, then G2 will be used.
>> 2. If ufshc is >= 5.0 and if the version is populated in register, 
>> then that
>> gear will be used. Most likely that gear can be G4/G5 depending on the 
>> device
>> connected.
>> 3. If ufshc is >=5.0 and version is not populated, then G4 will be used.
I think this made you confused, in case #3, HS-G5 is used as initial PHY 
gear, not G4 (in the last patch in this series).

Thanks,
Can Guo.
>>
>> In all the above cases, I do not see any necessity to switch the phy gear
>> setting to lower one while scaling. Since the gears are backwards 
>> compatible,
>> we always use one phy gear sequence. Moreover, we only have 2 init 
>> sequences.
>>
>> Please correct me if I'm missing anything.
>>
>> - Mani
> In the next patch, I am setting the initial PHY gear to max HS gear read
> from UFS host cap register, so that we don't need to keep updating the 
> initial value for host->phy_gear for different HW versions in future. 
> FYI, for HW ver 5 and 6, it is HS-G5. In future, the max gear might 
> become HS-G6 or higher on newer HW verions.
> 
> I the case #3, if HS-G5 is set to host->phy_gear, the first init uses 
> HS-G5, then after negotiation if the agreed gear is HS-G4, we need to 
> update host->phy_gear to HS-G4 (a lower value) such that we use a power 
> saving PHY gear settings during the 2nd init.
> 
> If the commit message is making you confused, I can update it in next 
> version. Please let me if I made any mistakes here.
> 
> Thanks,
> Can Guo.

Manivannan Sadhasivam Nov. 9, 2023, 3:47 a.m. UTC | #4

On Wed, Nov 08, 2023 at 04:21:52PM +0800, Can Guo wrote:
> Hi Mani,
> 
> On 11/8/2023 1:23 PM, Manivannan Sadhasivam wrote:
> > On Mon, Nov 06, 2023 at 08:46:09PM -0800, Can Guo wrote:
> > > From: Can Guo <[e-mail]>
> > > 
> > > During host driver init, the phy_gear is set to the minimum supported gear
> > > (HS_G2). Then, during the first power mode change, the negotiated gear, say
> > > HS-G4, is updated to the phy_gear variable so that in the second init the
> > > updated phy_gear can be used to program the PHY.
> > > 
> > > But the current code only allows update the phy_gear to a higher value. If
> > > one wants to start the first init with the maximum support gear, say HS-G4,
> > > the phy_gear is not updated to HS-G3 if the device only supports HS-G3.
> > > 
> > 
> > Can you elaborate when this can happen? AFAICS, there are 3 possibilities of
> > initial phy gear with this series:
> > 
> > 1. If ufshc is < 5.0, then G2 will be used.
> > 2. If ufshc is >= 5.0 and if the version is populated in register, then that
> > gear will be used. Most likely that gear can be G4/G5 depending on the device
> > connected.
> > 3. If ufshc is >=5.0 and version is not populated, then G4 will be used.
> > 
> > In all the above cases, I do not see any necessity to switch the phy gear
> > setting to lower one while scaling. Since the gears are backwards compatible,
> > we always use one phy gear sequence. Moreover, we only have 2 init sequences.
> > 
> > Please correct me if I'm missing anything.
> > 
> > - Mani
> In the next patch, I am setting the initial PHY gear to max HS gear read
> from UFS host cap register, so that we don't need to keep updating the
> initial value for host->phy_gear for different HW versions in future. FYI,
> for HW ver 5 and 6, it is HS-G5. In future, the max gear might become HS-G6
> or higher on newer HW verions.
> 
> I the case #3, if HS-G5 is set to host->phy_gear, the first init uses HS-G5,
> then after negotiation if the agreed gear is HS-G4, we need to update
> host->phy_gear to HS-G4 (a lower value) such that we use a power saving PHY
> gear settings during the 2nd init.
> 
> If the commit message is making you confused, I can update it in next
> version. Please let me if I made any mistakes here.
> 

I see redundancy while setting the phy_gear and it is leading to confusion.
In ufs_qcom_set_host_params(), first you are setting phy_gear based on
ufs_qcom_get_hs_gear(), then changing it again with the version check for v5.

I don't see a necessity for "host->phy_gear = host_params->hs_tx_gear", since in
the later check, you are covering both version <5 and >=5.

Btw, it would be better to move this logic to a separate function like
ufs_qcom_get_phy_gear() to align with ufs_qcom_get_hs_gear().

- Mani

> Thanks,
> Can Guo.

Can Guo Nov. 9, 2023, 4:07 a.m. UTC | #5

Hi Mani,

On 11/9/2023 11:47 AM, Manivannan Sadhasivam wrote:
> On Wed, Nov 08, 2023 at 04:21:52PM +0800, Can Guo wrote:
>> Hi Mani,
>>
>> On 11/8/2023 1:23 PM, Manivannan Sadhasivam wrote:
>>> On Mon, Nov 06, 2023 at 08:46:09PM -0800, Can Guo wrote:
>>>> From: Can Guo <[e-mail]>
>>>>
>>>> During host driver init, the phy_gear is set to the minimum supported gear
>>>> (HS_G2). Then, during the first power mode change, the negotiated gear, say
>>>> HS-G4, is updated to the phy_gear variable so that in the second init the
>>>> updated phy_gear can be used to program the PHY.
>>>>
>>>> But the current code only allows update the phy_gear to a higher value. If
>>>> one wants to start the first init with the maximum support gear, say HS-G4,
>>>> the phy_gear is not updated to HS-G3 if the device only supports HS-G3.
>>>>
>>>
>>> Can you elaborate when this can happen? AFAICS, there are 3 possibilities of
>>> initial phy gear with this series:
>>>
>>> 1. If ufshc is < 5.0, then G2 will be used.
>>> 2. If ufshc is >= 5.0 and if the version is populated in register, then that
>>> gear will be used. Most likely that gear can be G4/G5 depending on the device
>>> connected.
>>> 3. If ufshc is >=5.0 and version is not populated, then G4 will be used.
>>>
>>> In all the above cases, I do not see any necessity to switch the phy gear
>>> setting to lower one while scaling. Since the gears are backwards compatible,
>>> we always use one phy gear sequence. Moreover, we only have 2 init sequences.
>>>
>>> Please correct me if I'm missing anything.
>>>
>>> - Mani
>> In the next patch, I am setting the initial PHY gear to max HS gear read
>> from UFS host cap register, so that we don't need to keep updating the
>> initial value for host->phy_gear for different HW versions in future. FYI,
>> for HW ver 5 and 6, it is HS-G5. In future, the max gear might become HS-G6
>> or higher on newer HW verions.
>>
>> I the case #3, if HS-G5 is set to host->phy_gear, the first init uses HS-G5,
>> then after negotiation if the agreed gear is HS-G4, we need to update
>> host->phy_gear to HS-G4 (a lower value) such that we use a power saving PHY
>> gear settings during the 2nd init.
>>
>> If the commit message is making you confused, I can update it in next
>> version. Please let me if I made any mistakes here.
>>
> 
> I see redundancy while setting the phy_gear and it is leading to confusion.
> In ufs_qcom_set_host_params(), first you are setting phy_gear based on
> ufs_qcom_get_hs_gear(), then changing it again with the version check for v5.
> 
> I don't see a necessity for "host->phy_gear = host_params->hs_tx_gear", since in
> the later check, you are covering both version <5 and >=5.
In the case of HW version >= 5, we are only overwriting the PHY gear 
only if the UFS device cannot support HS-G5. Hence, we need to give 
host->phy_gear an initial value.

> 
> Btw, it would be better to move this logic to a separate function like
> ufs_qcom_get_phy_gear() to align with ufs_qcom_get_hs_gear().
> 
OK, will do in next version.

Thanks,
Can Guo.
> - Mani
> 
>> Thanks,
>> Can Guo.
>

Manivannan Sadhasivam Nov. 9, 2023, 7:47 a.m. UTC | #6

On Thu, Nov 09, 2023 at 12:07:37PM +0800, Can Guo wrote:
> Hi Mani,
> 
> On 11/9/2023 11:47 AM, Manivannan Sadhasivam wrote:
> > On Wed, Nov 08, 2023 at 04:21:52PM +0800, Can Guo wrote:
> > > Hi Mani,
> > > 
> > > On 11/8/2023 1:23 PM, Manivannan Sadhasivam wrote:
> > > > On Mon, Nov 06, 2023 at 08:46:09PM -0800, Can Guo wrote:
> > > > > From: Can Guo <[e-mail]>
> > > > > 
> > > > > During host driver init, the phy_gear is set to the minimum supported gear
> > > > > (HS_G2). Then, during the first power mode change, the negotiated gear, say
> > > > > HS-G4, is updated to the phy_gear variable so that in the second init the
> > > > > updated phy_gear can be used to program the PHY.
> > > > > 
> > > > > But the current code only allows update the phy_gear to a higher value. If
> > > > > one wants to start the first init with the maximum support gear, say HS-G4,
> > > > > the phy_gear is not updated to HS-G3 if the device only supports HS-G3.
> > > > > 
> > > > 
> > > > Can you elaborate when this can happen? AFAICS, there are 3 possibilities of
> > > > initial phy gear with this series:
> > > > 
> > > > 1. If ufshc is < 5.0, then G2 will be used.
> > > > 2. If ufshc is >= 5.0 and if the version is populated in register, then that
> > > > gear will be used. Most likely that gear can be G4/G5 depending on the device
> > > > connected.
> > > > 3. If ufshc is >=5.0 and version is not populated, then G4 will be used.
> > > > 
> > > > In all the above cases, I do not see any necessity to switch the phy gear
> > > > setting to lower one while scaling. Since the gears are backwards compatible,
> > > > we always use one phy gear sequence. Moreover, we only have 2 init sequences.
> > > > 
> > > > Please correct me if I'm missing anything.
> > > > 
> > > > - Mani
> > > In the next patch, I am setting the initial PHY gear to max HS gear read
> > > from UFS host cap register, so that we don't need to keep updating the
> > > initial value for host->phy_gear for different HW versions in future. FYI,
> > > for HW ver 5 and 6, it is HS-G5. In future, the max gear might become HS-G6
> > > or higher on newer HW verions.
> > > 
> > > I the case #3, if HS-G5 is set to host->phy_gear, the first init uses HS-G5,
> > > then after negotiation if the agreed gear is HS-G4, we need to update
> > > host->phy_gear to HS-G4 (a lower value) such that we use a power saving PHY
> > > gear settings during the 2nd init.
> > > 
> > > If the commit message is making you confused, I can update it in next
> > > version. Please let me if I made any mistakes here.
> > > 
> > 
> > I see redundancy while setting the phy_gear and it is leading to confusion.
> > In ufs_qcom_set_host_params(), first you are setting phy_gear based on
> > ufs_qcom_get_hs_gear(), then changing it again with the version check for v5.
> > 
> > I don't see a necessity for "host->phy_gear = host_params->hs_tx_gear", since in
> > the later check, you are covering both version <5 and >=5.
> In the case of HW version >= 5, we are only overwriting the PHY gear only if
> the UFS device cannot support HS-G5. Hence, we need to give host->phy_gear
> an initial value.
> 

Okay, I overlooked that part, thanks for clarifying. I think if we move the
logic to ufs_qcom_get_phy_gear(), it will be clear.

- Mani

> > 
> > Btw, it would be better to move this logic to a separate function like
> > ufs_qcom_get_phy_gear() to align with ufs_qcom_get_hs_gear().
> > 
> OK, will do in next version.
> 
> Thanks,
> Can Guo.
> > - Mani
> > 
> > > Thanks,
> > > Can Guo.
> >

Patch

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index cc0eb37..60b35ca 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -916,11 +916,12 @@  static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
 		}
 
 		/*
-		 * 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.
+		 * During UFS driver probe, always update the PHY gear to match the negotiated
+		 * gear, so that, if quirk UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH is enabled,
+		 * the second init can program the optimal PHY settings. This allows one to start
+		 * the first init with either the minimum or the maximum support gear.
 		 */
-		if (dev_req_params->gear_tx > host->phy_gear)
+		if (hba->ufshcd_state == UFSHCD_STATE_RESET)
 			host->phy_gear = dev_req_params->gear_tx;
 
 		/* enable the device ref clock before changing to HS mode */