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@xxxxxxxxxxxx wrote:
> > From: Peter Wang <peter.wang@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > ---
> > 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
>
>
