Re: [PATCH v5 06/10] scsi: ufs: ufs-qcom: Limit HS-G5 Rate-A to hosts with HW version 5

[Can Guo]

On 11/28/2023 6:55 PM, Manivannan Sadhasivam wrote:
> On Tue, Nov 28, 2023 at 03:48:02PM +0800, Can Guo wrote:
> > Hi Mani,
> >
> > On 11/28/2023 1:55 PM, Manivannan Sadhasivam wrote:
> > > On Thu, Nov 23, 2023 at 12:46:26AM -0800, Can Guo wrote:
> > > > Qcom UFS hosts, with HW ver 5, can only support up to HS-G5 Rate-A due to
> > > > HW limitations. If the HS-G5 PHY gear is used, update host_params->hs_rate
> > > > to Rate-A, so that the subsequent power mode changes shall stick to Rate-A.
> > > >
> > > > Signed-off-by: Can Guo <quic_cang@xxxxxxxxxxx>
> > >
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > >
> > > One question below...
> > >
> > > > ---
> > > >    drivers/ufs/host/ufs-qcom.c | 18 +++++++++++++++++-
> > > >    1 file changed, 17 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > > index 9613ad9..6756f8d 100644
> > > > --- a/drivers/ufs/host/ufs-qcom.c
> > > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > > @@ -442,9 +442,25 @@ static u32 ufs_qcom_get_hs_gear(struct ufs_hba *hba)
> > > >    static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> > > >    {
> > > >    	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> > > > +	struct ufs_host_params *host_params = &host->host_params;
> > > >    	struct phy *phy = host->generic_phy;
> > > > +	enum phy_mode mode;
> > > >    	int ret;
> > > > +	/*
> > > > +	 * HW ver 5 can only support up to HS-G5 Rate-A due to HW limitations.
> > > > +	 * If the HS-G5 PHY gear is used, update host_params->hs_rate to Rate-A,
> > > > +	 * so that the subsequent power mode change shall stick to Rate-A.
> > > > +	 */
> > > > +	if (host->hw_ver.major == 0x5) {
> > > > +		if (host->phy_gear == UFS_HS_G5)
> > > > +			host_params->hs_rate = PA_HS_MODE_A;
> > > > +		else
> > > > +			host_params->hs_rate = PA_HS_MODE_B;
> > >
> > > Is this 'else' part really needed? Since there wouldn't be any 2nd init, I think
> > > we can skip that.
> >
> > We need it because, even there is only one init, if a UFS3.1 device is
> > attached, phy_gear is given as UFS_HS_G4 in ufs_qcom_set_phy_gear(), hence
> > we need to put the UFS at HS-G4 Rate B, not Rate A.
>
> But the default hs_rate is PA_HS_MODE_B only and the else condition would be not
> needed for the 1st init.

You are right, but still we need this in case the UFS device version is 
not populated, meaning dual init can also happen to SM8550. We need to 
apply the right hs_rate in case the 2nd init asks for HS_G4.

Thanks,
Can Guo.

> - Mani
>
> > Thanks,
> > Can Guo.
> >
> > > - Mani
> > >
> > > > +	}
> > > > +
> > > > +	mode = host_params->hs_rate == PA_HS_MODE_B ? PHY_MODE_UFS_HS_B : PHY_MODE_UFS_HS_A;
> > > > +
> > > >    	/* Reset UFS Host Controller and PHY */
> > > >    	ret = ufs_qcom_host_reset(hba);
> > > >    	if (ret)
> > > > @@ -459,7 +475,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->phy_gear);
> > > > +	phy_set_mode_ext(phy, mode, host->phy_gear);
> > > >    	/* power on phy - start serdes and phy's power and clocks */
> > > >    	ret = phy_power_on(phy);
> > > > --
> > > > 2.7.4