| Message ID | 1630476252-2031-1-git-send-email-peter.wang@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] scsi: ufs: ufs-mediatek: Change dbg select by check hw version | expand |
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
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 --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