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

[Peter Wang]

On Wed, 2021-09-01 at 15:35 +0800, Stanley Chu wrote:
> 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?
> 

Yes, will remove __no_kcsan and add a ip_ver in mediatek host struct
to avoid KCSAN warning.
Dbg select value will clear by hci reset. Maybe we cand add a variable
in mediatek host sruct to record if need set dbg select.

> > +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?
> 

Sure, could change naming for easy understand.

> >  #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
> 
>