diff mbox series

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

Message ID 1630476252-2031-1-git-send-email-peter.wang@mediatek.com (mailing list archive)
State Changes Requested
Headers show
Series [v3] scsi: ufs: ufs-mediatek: Change dbg select by check hw version | expand

Commit Message

Peter Wang (王信友) Sept. 1, 2021, 6:04 a.m. UTC 
From: Peter Wang <peter.wang@mediatek.com>

Mediatek UFS dbg select setting is changed in new HW version.
This patch check the HW version before set dbg select.

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/scsi/ufs/ufs-mediatek.c |   23 +++++++++++++++++++++--
 drivers/scsi/ufs/ufs-mediatek.h |    5 +++++
 2 files changed, 26 insertions(+), 2 deletions(-)

Comments

Stanley Chu Sept. 1, 2021, 7:35 a.m. UTC | #1 
Hi Peter,

On Wed, 2021-09-01 at 14:04 +0800, peter.wang@mediatek.com wrote:
> From: Peter Wang <peter.wang@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@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
Peter Wang (王信友) Sept. 2, 2021, 2:12 a.m. UTC | #2 
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@mediatek.com wrote:
> > From: Peter Wang <peter.wang@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@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
> 
>
diff mbox series

Patch

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
+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);
+
+	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
 #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
 
 /*
  * Ref-clk control