[v5,04/10] scsi: ufs: ufs-qcom: Limit negotiated gear to selected PHY gear

  In the dual init scenario, the initial PHY gear is set to HS-G2, and the
first Power Mode Change (PMC) is meant to find the best matching PHY gear
for the 2nd init. However, for the first PMC, if the negotiated gear (say
HS-G4) is higher than the initial PHY gear, we cannot go ahead let PMC to
the negotiated gear happen, because the programmed UFS PHY settings may not
support the negotiated gear. Fix it by overwriting the negotiated gear with
the PHY gear.

Signed-off-by: Can Guo <quic_cang@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
  

On Thu, Nov 23, 2023 at 12:46:24AM -0800, Can Guo wrote:
> In the dual init scenario, the initial PHY gear is set to HS-G2, and the
> first Power Mode Change (PMC) is meant to find the best matching PHY gear
> for the 2nd init. However, for the first PMC, if the negotiated gear (say
> HS-G4) is higher than the initial PHY gear, we cannot go ahead let PMC to
> the negotiated gear happen, because the programmed UFS PHY settings may not
> support the negotiated gear. Fix it by overwriting the negotiated gear with
> the PHY gear.
> 

I don't quite understand this patch. If the phy_gear is G2 initially and the
negotiated gear is G4, then as per this change,

phy_gear = G4;
negotiated gear = G2;

Could you please explain how this make sense?

- Mani

> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index cc0eb37..d4edf58 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -920,8 +920,13 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>  		 * 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->phy_gear)
> +		if (dev_req_params->gear_tx > host->phy_gear) {
> +			u32 old_phy_gear = host->phy_gear;
> +
>  			host->phy_gear = dev_req_params->gear_tx;
> +			dev_req_params->gear_tx = old_phy_gear;
> +			dev_req_params->gear_rx = old_phy_gear;
> +		}
>  
>  		/* enable the device ref clock before changing to HS mode */
>  		if (!ufshcd_is_hs_mode(&hba->pwr_info) &&
> -- 
> 2.7.4
>
  

Hi Mani,

On 11/28/2023 1:45 PM, Manivannan Sadhasivam wrote:
> On Thu, Nov 23, 2023 at 12:46:24AM -0800, Can Guo wrote:
>> In the dual init scenario, the initial PHY gear is set to HS-G2, and the
>> first Power Mode Change (PMC) is meant to find the best matching PHY gear
>> for the 2nd init. However, for the first PMC, if the negotiated gear (say
>> HS-G4) is higher than the initial PHY gear, we cannot go ahead let PMC to
>> the negotiated gear happen, because the programmed UFS PHY settings may not
>> support the negotiated gear. Fix it by overwriting the negotiated gear with
>> the PHY gear.
>>
> 
> I don't quite understand this patch. If the phy_gear is G2 initially and the
> negotiated gear is G4, then as per this change,
> 
> phy_gear = G4;
> negotiated gear = G2;
> 
> Could you please explain how this make sense?

phy_gear was G2 (in the beginning) and just now changed to G4, but the 
PHY settings programmed in the beginning can only support no-G4 (not 
G4). Without this change, as the negotiated gear is G4, the power mode 
change is going to put UFS at HS-G4 mode, but the PHY settings 
programmed is no-G4. This change is to limit the negotiated gear to 
HS-G2 for the 1st init. In the 2nd init, as the new PHY gear is G4, G4 
PHY settings would be programmed, it'd be safe to put the UFS at HS-G4 mode.

Thanks,
Can Guo.
> 
> - Mani
> 
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index cc0eb37..d4edf58 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -920,8 +920,13 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>>   		 * 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->phy_gear)
>> +		if (dev_req_params->gear_tx > host->phy_gear) {
>> +			u32 old_phy_gear = host->phy_gear;
>> +
>>   			host->phy_gear = dev_req_params->gear_tx;
>> +			dev_req_params->gear_tx = old_phy_gear;
>> +			dev_req_params->gear_rx = old_phy_gear;
>> +		}
>>   
>>   		/* enable the device ref clock before changing to HS mode */
>>   		if (!ufshcd_is_hs_mode(&hba->pwr_info) &&
>> -- 
>> 2.7.4
>>
>
  

On Tue, Nov 28, 2023 at 04:05:59PM +0800, Can Guo wrote:
> Hi Mani,
> 
> On 11/28/2023 1:45 PM, Manivannan Sadhasivam wrote:
> > On Thu, Nov 23, 2023 at 12:46:24AM -0800, Can Guo wrote:
> > > In the dual init scenario, the initial PHY gear is set to HS-G2, and the
> > > first Power Mode Change (PMC) is meant to find the best matching PHY gear
> > > for the 2nd init. However, for the first PMC, if the negotiated gear (say
> > > HS-G4) is higher than the initial PHY gear, we cannot go ahead let PMC to
> > > the negotiated gear happen, because the programmed UFS PHY settings may not
> > > support the negotiated gear. Fix it by overwriting the negotiated gear with
> > > the PHY gear.
> > > 
> > 
> > I don't quite understand this patch. If the phy_gear is G2 initially and the
> > negotiated gear is G4, then as per this change,
> > 
> > phy_gear = G4;
> > negotiated gear = G2;
> > 
> > Could you please explain how this make sense?
> 
> phy_gear was G2 (in the beginning) and just now changed to G4, but the PHY
> settings programmed in the beginning can only support no-G4 (not G4).
> Without this change, as the negotiated gear is G4, the power mode change is
> going to put UFS at HS-G4 mode, but the PHY settings programmed is no-G4.

But we are going to reinit the PHY anyway, isn't it?

> This change is to limit the negotiated gear to HS-G2 for the 1st init. In
> the 2nd init, as the new PHY gear is G4, G4 PHY settings would be
> programmed, it'd be safe to put the UFS at HS-G4 mode.
> 

Why do we need to limit it since we already have the logic in place to set
whatever gear mode applicable for 1st init?

- Mani

> Thanks,
> Can Guo.
> > 
> > - Mani
> > 
> > > Signed-off-by: Can Guo <quic_cang@quicinc.com>
> > > ---
> > >   drivers/ufs/host/ufs-qcom.c | 7 ++++++-
> > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > index cc0eb37..d4edf58 100644
> > > --- a/drivers/ufs/host/ufs-qcom.c
> > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > @@ -920,8 +920,13 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
> > >   		 * 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->phy_gear)
> > > +		if (dev_req_params->gear_tx > host->phy_gear) {
> > > +			u32 old_phy_gear = host->phy_gear;
> > > +
> > >   			host->phy_gear = dev_req_params->gear_tx;
> > > +			dev_req_params->gear_tx = old_phy_gear;
> > > +			dev_req_params->gear_rx = old_phy_gear;
> > > +		}
> > >   		/* enable the device ref clock before changing to HS mode */
> > >   		if (!ufshcd_is_hs_mode(&hba->pwr_info) &&
> > > -- 
> > > 2.7.4
> > > 
> >
  

On 11/28/2023 6:52 PM, Manivannan Sadhasivam wrote:
> On Tue, Nov 28, 2023 at 04:05:59PM +0800, Can Guo wrote:
>> Hi Mani,
>>
>> On 11/28/2023 1:45 PM, Manivannan Sadhasivam wrote:
>>> On Thu, Nov 23, 2023 at 12:46:24AM -0800, Can Guo wrote:
>>>> In the dual init scenario, the initial PHY gear is set to HS-G2, and the
>>>> first Power Mode Change (PMC) is meant to find the best matching PHY gear
>>>> for the 2nd init. However, for the first PMC, if the negotiated gear (say
>>>> HS-G4) is higher than the initial PHY gear, we cannot go ahead let PMC to
>>>> the negotiated gear happen, because the programmed UFS PHY settings may not
>>>> support the negotiated gear. Fix it by overwriting the negotiated gear with
>>>> the PHY gear.
>>>>
>>>
>>> I don't quite understand this patch. If the phy_gear is G2 initially and the
>>> negotiated gear is G4, then as per this change,
>>>
>>> phy_gear = G4;
>>> negotiated gear = G2;
>>>
>>> Could you please explain how this make sense?
>>
>> phy_gear was G2 (in the beginning) and just now changed to G4, but the PHY
>> settings programmed in the beginning can only support no-G4 (not G4).
>> Without this change, as the negotiated gear is G4, the power mode change is
>> going to put UFS at HS-G4 mode, but the PHY settings programmed is no-G4.
> 
> But we are going to reinit the PHY anyway, isn't it?

We are power mode changing to HS-G4 with no-G4 PHY settings programmed, 
the power mode change operation, in the 1st init, may immediately cause 
UIC errors and lead to probe fail. We are not seeing issues as of now, 
maybe because the amount of HW used for testing is not large enough.

This change is not really related to this specific series, I can remove 
it in next version.

Thanks,
Can Guo.

> 
>> This change is to limit the negotiated gear to HS-G2 for the 1st init. In
>> the 2nd init, as the new PHY gear is G4, G4 PHY settings would be
>> programmed, it'd be safe to put the UFS at HS-G4 mode.
>>
> 
> Why do we need to limit it since we already have the logic in place to set
> whatever gear mode applicable for 1st init?
> 
> - Mani
> 
>> Thanks,
>> Can Guo.
>>>
>>> - Mani
>>>
>>>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>>>> ---
>>>>    drivers/ufs/host/ufs-qcom.c | 7 ++++++-
>>>>    1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>> index cc0eb37..d4edf58 100644
>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>> @@ -920,8 +920,13 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>>>>    		 * 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->phy_gear)
>>>> +		if (dev_req_params->gear_tx > host->phy_gear) {
>>>> +			u32 old_phy_gear = host->phy_gear;
>>>> +
>>>>    			host->phy_gear = dev_req_params->gear_tx;
>>>> +			dev_req_params->gear_tx = old_phy_gear;
>>>> +			dev_req_params->gear_rx = old_phy_gear;
>>>> +		}
>>>>    		/* enable the device ref clock before changing to HS mode */
>>>>    		if (!ufshcd_is_hs_mode(&hba->pwr_info) &&
>>>> -- 
>>>> 2.7.4
>>>>
>>>
>
  

On Tue, Nov 28, 2023 at 07:03:41PM +0800, Can Guo wrote:
> 
> 
> On 11/28/2023 6:52 PM, Manivannan Sadhasivam wrote:
> > On Tue, Nov 28, 2023 at 04:05:59PM +0800, Can Guo wrote:
> > > Hi Mani,
> > > 
> > > On 11/28/2023 1:45 PM, Manivannan Sadhasivam wrote:
> > > > On Thu, Nov 23, 2023 at 12:46:24AM -0800, Can Guo wrote:
> > > > > In the dual init scenario, the initial PHY gear is set to HS-G2, and the
> > > > > first Power Mode Change (PMC) is meant to find the best matching PHY gear
> > > > > for the 2nd init. However, for the first PMC, if the negotiated gear (say
> > > > > HS-G4) is higher than the initial PHY gear, we cannot go ahead let PMC to
> > > > > the negotiated gear happen, because the programmed UFS PHY settings may not
> > > > > support the negotiated gear. Fix it by overwriting the negotiated gear with
> > > > > the PHY gear.
> > > > > 
> > > > 
> > > > I don't quite understand this patch. If the phy_gear is G2 initially and the
> > > > negotiated gear is G4, then as per this change,
> > > > 
> > > > phy_gear = G4;
> > > > negotiated gear = G2;
> > > > 
> > > > Could you please explain how this make sense?
> > > 
> > > phy_gear was G2 (in the beginning) and just now changed to G4, but the PHY
> > > settings programmed in the beginning can only support no-G4 (not G4).
> > > Without this change, as the negotiated gear is G4, the power mode change is
> > > going to put UFS at HS-G4 mode, but the PHY settings programmed is no-G4.
> > 
> > But we are going to reinit the PHY anyway, isn't it?
> 
> We are power mode changing to HS-G4 with no-G4 PHY settings programmed, the
> power mode change operation, in the 1st init, may immediately cause UIC
> errors and lead to probe fail. We are not seeing issues as of now, maybe
> because the amount of HW used for testing is not large enough.
> 

I'm not sure what you are saying is what happening. Because, if we use the
incompatible gear setting, we should immediately see the UIC error.

> This change is not really related to this specific series, I can remove it
> in next version.
> 

Please do so. This needs to be reviewed separately.

- Mani

> Thanks,
> Can Guo.
> 
> > 
> > > This change is to limit the negotiated gear to HS-G2 for the 1st init. In
> > > the 2nd init, as the new PHY gear is G4, G4 PHY settings would be
> > > programmed, it'd be safe to put the UFS at HS-G4 mode.
> > > 
> > 
> > Why do we need to limit it since we already have the logic in place to set
> > whatever gear mode applicable for 1st init?
> > 
> > - Mani
> > 
> > > Thanks,
> > > Can Guo.
> > > > 
> > > > - Mani
> > > > 
> > > > > Signed-off-by: Can Guo <quic_cang@quicinc.com>
> > > > > ---
> > > > >    drivers/ufs/host/ufs-qcom.c | 7 ++++++-
> > > > >    1 file changed, 6 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > > > index cc0eb37..d4edf58 100644
> > > > > --- a/drivers/ufs/host/ufs-qcom.c
> > > > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > > > @@ -920,8 +920,13 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
> > > > >    		 * 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->phy_gear)
> > > > > +		if (dev_req_params->gear_tx > host->phy_gear) {
> > > > > +			u32 old_phy_gear = host->phy_gear;
> > > > > +
> > > > >    			host->phy_gear = dev_req_params->gear_tx;
> > > > > +			dev_req_params->gear_tx = old_phy_gear;
> > > > > +			dev_req_params->gear_rx = old_phy_gear;
> > > > > +		}
> > > > >    		/* enable the device ref clock before changing to HS mode */
> > > > >    		if (!ufshcd_is_hs_mode(&hba->pwr_info) &&
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > > 
> >