Re: [PATCH 1/2] ufs: host: ufs-qcom: Clear qunipro_g4_sel for HW version major 5

[Manivannan Sadhasivam]

On Wed, Nov 16, 2022 at 02:17:31PM +0200, Abel Vesa wrote:
> On SM8550, depending on the Qunipro, we can run with G5 or G4.
> For now, when the major version is 5 or above, we go with G5.
> Therefore, we need to specifically tell UFS HC that.
> 
> Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> ---
>  drivers/ufs/host/ufs-qcom.c | 4 ++++
>  drivers/ufs/host/ufs-qcom.h | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index ca60a5b0292b..72334aefe81c 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -227,6 +227,10 @@ static void ufs_qcom_select_unipro_mode(struct ufs_qcom_host *host)
>  	ufshcd_rmwl(host->hba, QUNIPRO_SEL,
>  		   ufs_qcom_cap_qunipro(host) ? QUNIPRO_SEL : 0,
>  		   REG_UFS_CFG1);
> +
> +	if (host->hw_ver.major == 0x05)
> +		ufshcd_rmwl(host->hba, QUNIPRO_G4_SEL, 0, REG_UFS_CFG0);

So this means, G5 will be used all the time even if the UFS device doesn't
support it (ie., G4 device), which is not ideal.

Since you have already based this series on my UFS gear 4 series, you should
resend this on top of my next version that I'm about to submit. There I have
proposed reinitializing the UFS device after switching to max gear supported by
both controller and device.

Based on that information, you could do:

```
	if (host->hw_ver.major == 0x05) {
		if (host->hs_hear == UFS_HS_G5)
			ufshcd_rmwl(host->hba, QUNIPRO_G4_SEL, 0, REG_UFS_CFG0);
		else
			ufshcd_rmwl(host->hba, QUNIPRO_G4_SEL, 1, REG_UFS_CFG0);
	}
```

By this way, if the device doesn't support G5, G4 will be used.

Btw, please use a valid definition instead of 0/1 above.

> +
>  	/* make sure above configuration is applied before we return */
>  	mb();
>  }
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 751ded3e3531..10621055bf7f 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -36,6 +36,7 @@ enum {
>  	/* On older UFS revisions, this register is called "RETRY_TIMER_REG" */
>  	REG_UFS_PARAM0                      = 0xD0,
>  	REG_UFS_PA_LINK_STARTUP_TIMER       = 0xD8,
> +	REG_UFS_CFG0                        = 0xD8,

Hmm, so what is the offset of REG_UFS_PA_LINK_STARTUP_TIMER?

>  	REG_UFS_CFG1                        = 0xDC,
>  	REG_UFS_CFG2                        = 0xE0,
>  	REG_UFS_HW_VERSION                  = 0xE4,
> @@ -75,6 +76,7 @@ enum {
>  
>  /* bit definitions for REG_UFS_CFG1 register */
>  #define QUNIPRO_SEL		BIT(0)
> +#define QUNIPRO_G4_SEL		BIT(5)

Since this bit belongs to CFG0 register, it should be added separately and not
with CFG1 definitions.

Thanks,
Mani

>  #define UFS_PHY_SOFT_RESET	BIT(1)
>  #define UTP_DBG_RAMS_EN		BIT(17)
>  #define TEST_BUS_EN		BIT(18)
> -- 
> 2.34.1
> 

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