Re: [PATCH v2 16/17] scsi: ufs: qcom: Use ufshcd_rmwl() where applicable

[Andrew Halaney]

On Fri, Dec 08, 2023 at 12:29:01PM +0530, Manivannan Sadhasivam wrote:
> Instead of using both ufshcd_readl() and ufshcd_writel() to read/modify/
> write a register, let's make use of the existing helper.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> ---
>  drivers/ufs/host/ufs-qcom.c | 12 ++++--------
>  drivers/ufs/host/ufs-qcom.h |  3 +++
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 26aa8904c823..549a08645391 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -387,9 +387,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>   */
>  static void ufs_qcom_enable_hw_clk_gating(struct ufs_hba *hba)
>  {
> -	ufshcd_writel(hba,
> -		ufshcd_readl(hba, REG_UFS_CFG2) | REG_UFS_CFG2_CGC_EN_ALL,
> -		REG_UFS_CFG2);
> +	ufshcd_rmwl(hba, REG_UFS_CFG2_CGC_EN_ALL, REG_UFS_CFG2_CGC_EN_ALL,
> +		    REG_UFS_CFG2);
>  
>  	/* Ensure that HW clock gating is enabled before next operations */
>  	mb();
> @@ -1689,11 +1688,8 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>  		platform_msi_domain_free_irqs(hba->dev);
>  	} else {
>  		if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 &&
> -		    host->hw_ver.step == 0) {
> -			ufshcd_writel(hba,
> -				      ufshcd_readl(hba, REG_UFS_CFG3) | 0x1F000,
> -				      REG_UFS_CFG3);
> -		}
> +		    host->hw_ver.step == 0)
> +			ufshcd_rmwl(hba, ESI_VEC_MASK, 0x1f00, REG_UFS_CFG3);

Is this right? I feel like you accidentally just shifted what was written
prior >> 4 bits.

Before: 0x1f000
After:  0x01f00

>  		ufshcd_mcq_enable_esi(hba);
>  	}
>  
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 385480499e71..2ce63a1c7f2f 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -102,6 +102,9 @@ enum {
>  #define TMRLUT_HW_CGC_EN	BIT(6)
>  #define OCSC_HW_CGC_EN		BIT(7)
>  
> +/* bit definitions for REG_UFS_CFG3 register */
> +#define ESI_VEC_MASK		GENMASK(22, 12)
> +

I'll leave it to someone with the docs to review this field. It at least
contains the bits set above, fwiw. It would be neat to see that
converted to using the field + a FIELD_PREP to set the bits IMO, but I
honestly don't have a lot of experience using those APIs so feel free to
reject that.

>  /* bit definitions for REG_UFS_PARAM0 */
>  #define MAX_HS_GEAR_MASK	GENMASK(6, 4)
>  #define UFS_QCOM_MAX_GEAR(x)	FIELD_GET(MAX_HS_GEAR_MASK, (x))
> -- 
> 2.25.1
>