[PATCH v3] scsi: ufs: ufs-mediatek: Change dbg select by check hw version

[Stanley Chu]

Hi Peter,

On Wed, 2021-09-01 at 14:04 +0800, peter.wang at mediatek.com wrote:
> From: Peter Wang <peter.wang at mediatek.com>
> 
> Mediatek UFS dbg select setting is changed in new HW version.
> This patch check the HW version before set dbg select.
Nits: This patch checks the HW version before setting dbg select.
> 
> Signed-off-by: Peter Wang <peter.wang at mediatek.com>
> ---
>  drivers/scsi/ufs/ufs-mediatek.c |   23 +++++++++++++++++++++--
>  drivers/scsi/ufs/ufs-mediatek.h |    5 +++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-
> mediatek.c
> index d2c2516..0050e01 100644
> --- a/drivers/scsi/ufs/ufs-mediatek.c
> +++ b/drivers/scsi/ufs/ufs-mediatek.c
> @@ -296,6 +296,25 @@ static void ufs_mtk_setup_ref_clk_wait_us(struct
> ufs_hba *hba,
>  	host->ref_clk_ungating_wait_us = ungating_us;
>  }
>  
> +__no_kcsan

This is rarely used in mainstream kernel. According to my grep results,
__no_kcsan is only used by kcsan-test itself.

Besides, dbg select configuration may not be necessary if the mode is
already configured before? I just wonder that can we avoid setting
these registers every query?

> +static void ufs_mtk_dbg_sel(struct ufs_hba *hba)
> +{
> +	static u32 hw_ver;
> +
> +	if (!hw_ver)
> +		hw_ver = ufshcd_readl(hba, REG_UFS_MTK_HW_VER);

Perhaps you can keep this version in struct host->hw_ver? Maybe you
need to add a new member in that struct, for example, ip_ver.

> +
> +	if (((hw_ver >> 16) & 0xFF) >= 0x36) {
> +		ufshcd_writel(hba, 0x820820, REG_UFS_DEBUG_SEL);
> +		ufshcd_writel(hba, 0x0, REG_UFS_DEBUG_SEL_B0);
> +		ufshcd_writel(hba, 0x55555555, REG_UFS_DEBUG_SEL_B1);
> +		ufshcd_writel(hba, 0xaaaaaaaa, REG_UFS_DEBUG_SEL_B2);
> +		ufshcd_writel(hba, 0xffffffff, REG_UFS_DEBUG_SEL_B3);
> +	} else {
> +		ufshcd_writel(hba, 0x20, REG_UFS_DEBUG_SEL);
> +	}
> +}
> +
>  static int ufs_mtk_wait_link_state(struct ufs_hba *hba, u32 state,
>  				   unsigned long max_wait_ms)
>  {
> @@ -305,7 +324,7 @@ static int ufs_mtk_wait_link_state(struct ufs_hba
> *hba, u32 state,
>  	timeout = ktime_add_ms(ktime_get(), max_wait_ms);
>  	do {
>  		time_checked = ktime_get();
> -		ufshcd_writel(hba, 0x20, REG_UFS_DEBUG_SEL);
> +		ufs_mtk_dbg_sel(hba);
>  		val = ufshcd_readl(hba, REG_UFS_PROBE);
>  		val = val >> 28;
>  
> @@ -1001,7 +1020,7 @@ static void ufs_mtk_dbg_register_dump(struct
> ufs_hba *hba)
>  			 "MPHY Ctrl ");
>  
>  	/* Direct debugging information to REG_MTK_PROBE */
> -	ufshcd_writel(hba, 0x20, REG_UFS_DEBUG_SEL);
> +	ufs_mtk_dbg_sel(hba);
>  	ufshcd_dump_regs(hba, REG_UFS_PROBE, 0x4, "Debug Probe ");
>  }
>  
> diff --git a/drivers/scsi/ufs/ufs-mediatek.h b/drivers/scsi/ufs/ufs-
> mediatek.h
> index 3f0d3bb..fc40c05 100644
> --- a/drivers/scsi/ufs/ufs-mediatek.h
> +++ b/drivers/scsi/ufs/ufs-mediatek.h
> @@ -15,9 +15,14 @@
>  #define REG_UFS_REFCLK_CTRL         0x144
>  #define REG_UFS_EXTREG              0x2100
>  #define REG_UFS_MPHYCTRL            0x2200
> +#define REG_UFS_MTK_HW_VER          0x2240

HW_VER is somehow ambiguous, for example, how about REG_UFS_MTK_IP_VER?

>  #define REG_UFS_REJECT_MON          0x22AC
>  #define REG_UFS_DEBUG_SEL           0x22C0
>  #define REG_UFS_PROBE               0x22C8
> +#define REG_UFS_DEBUG_SEL_B0        0x22D0
> +#define REG_UFS_DEBUG_SEL_B1        0x22D4
> +#define REG_UFS_DEBUG_SEL_B2        0x22D8
> +#define REG_UFS_DEBUG_SEL_B3        0x22DC

Perhaps the debug select design could be simplified in the future, for
example, driver can query what it wants by reading only one register
without configuring anything first? Although this is beyond the scope
of this patch.

Thanks,
Stanley Chu

>  
>  /*
>   * Ref-clk control