| Message ID | 20221201230810.1019834-1-ahalaney@redhat.com |
|---|---|
| Headers | show |
| Series | scsi: ufs: ufs-qcom: Debug clean ups | expand |
On Thu, Dec 01 2022 at 15:09 -0800, Andrew Halaney wrote: >This patch series attempts to clean up some debug code paths in the >ufs-qcom driver. > >Andrew Halaney (4): > scsi: ufs: ufs-qcom: Drop unnecessary NULL checks > scsi: ufs: ufs-qcom: Clean up dbg_register_dump > scsi: ufs: ufs-qcom: Remove usage of dbg_print_en > scsi: ufs: ufs-qcom: Use dev_err() where possible > > drivers/ufs/host/ufs-qcom.c | 135 +++++++++++++----------------------- > drivers/ufs/host/ufs-qcom.h | 11 --- > 2 files changed, 48 insertions(+), 98 deletions(-) > >-- >2.38.1 > > This series makes sense to me. Thanks. Reviewed-by: Asutosh Das <quic_asutoshd@quicinc.com> -asd
On Thu, Dec 01, 2022 at 05:08:08PM -0600, Andrew Halaney wrote: > The current implementation has abstractions that don't give any > benefits. > > The print_fn callback (and its only callback implementation, > ufs_qcom_dump_regs_wrapper()) was only used by > ufs_qcom_print_hw_debug_reg_all() and just multiplies len by 4 > before calling ufshcd_dump_regs(). > > ufs_qcom_print_hw_debug_reg_all() is only called by > ufs_qcom_dump_dbg_regs(). > > There's no real gain in those abstractions, so let's just do the work > directly in ufs_qcom_dump_dbg_regs() (the dbg_register_dump callback). > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Thanks, Mani > --- > drivers/ufs/host/ufs-qcom.c | 106 ++++++++++++++++-------------------- > 1 file changed, 47 insertions(+), 59 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 70e25f9f8ca8..1b0dfbbdcdf3 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -52,12 +52,6 @@ static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd) > return container_of(rcd, struct ufs_qcom_host, rcdev); > } > > -static void ufs_qcom_dump_regs_wrapper(struct ufs_hba *hba, int offset, int len, > - const char *prefix, void *priv) > -{ > - ufshcd_dump_regs(hba, offset, len * 4, prefix); > -} > - > static int ufs_qcom_host_clk_get(struct device *dev, > const char *name, struct clk **clk_out, bool optional) > { > @@ -1195,58 +1189,6 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, > return err; > } > > -static void ufs_qcom_print_hw_debug_reg_all(struct ufs_hba *hba, > - void *priv, void (*print_fn)(struct ufs_hba *hba, > - int offset, int num_regs, const char *str, void *priv)) > -{ > - u32 reg; > - struct ufs_qcom_host *host; > - > - host = ufshcd_get_variant(hba); > - if (!(host->dbg_print_en & UFS_QCOM_DBG_PRINT_REGS_EN)) > - return; > - > - reg = ufs_qcom_get_debug_reg_offset(host, UFS_UFS_DBG_RD_REG_OCSC); > - print_fn(hba, reg, 44, "UFS_UFS_DBG_RD_REG_OCSC ", priv); > - > - reg = ufshcd_readl(hba, REG_UFS_CFG1); > - reg |= UTP_DBG_RAMS_EN; > - ufshcd_writel(hba, reg, REG_UFS_CFG1); > - > - reg = ufs_qcom_get_debug_reg_offset(host, UFS_UFS_DBG_RD_EDTL_RAM); > - print_fn(hba, reg, 32, "UFS_UFS_DBG_RD_EDTL_RAM ", priv); > - > - reg = ufs_qcom_get_debug_reg_offset(host, UFS_UFS_DBG_RD_DESC_RAM); > - print_fn(hba, reg, 128, "UFS_UFS_DBG_RD_DESC_RAM ", priv); > - > - reg = ufs_qcom_get_debug_reg_offset(host, UFS_UFS_DBG_RD_PRDT_RAM); > - print_fn(hba, reg, 64, "UFS_UFS_DBG_RD_PRDT_RAM ", priv); > - > - /* clear bit 17 - UTP_DBG_RAMS_EN */ > - ufshcd_rmwl(hba, UTP_DBG_RAMS_EN, 0, REG_UFS_CFG1); > - > - reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_UAWM); > - print_fn(hba, reg, 4, "UFS_DBG_RD_REG_UAWM ", priv); > - > - reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_UARM); > - print_fn(hba, reg, 4, "UFS_DBG_RD_REG_UARM ", priv); > - > - reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_TXUC); > - print_fn(hba, reg, 48, "UFS_DBG_RD_REG_TXUC ", priv); > - > - reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_RXUC); > - print_fn(hba, reg, 27, "UFS_DBG_RD_REG_RXUC ", priv); > - > - reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_DFC); > - print_fn(hba, reg, 19, "UFS_DBG_RD_REG_DFC ", priv); > - > - reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_TRLUT); > - print_fn(hba, reg, 34, "UFS_DBG_RD_REG_TRLUT ", priv); > - > - reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_TMRLUT); > - print_fn(hba, reg, 9, "UFS_DBG_RD_REG_TMRLUT ", priv); > -} > - > static void ufs_qcom_enable_test_bus(struct ufs_qcom_host *host) > { > if (host->dbg_print_en & UFS_QCOM_DBG_PRINT_TEST_BUS_EN) { > @@ -1365,10 +1307,56 @@ int ufs_qcom_testbus_config(struct ufs_qcom_host *host) > > static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba) > { > + u32 reg; > + struct ufs_qcom_host *host; > + > + host = ufshcd_get_variant(hba); > + > ufshcd_dump_regs(hba, REG_UFS_SYS1CLK_1US, 16 * 4, > "HCI Vendor Specific Registers "); > > - ufs_qcom_print_hw_debug_reg_all(hba, NULL, ufs_qcom_dump_regs_wrapper); > + if (!(host->dbg_print_en & UFS_QCOM_DBG_PRINT_REGS_EN)) > + return; > + > + reg = ufs_qcom_get_debug_reg_offset(host, UFS_UFS_DBG_RD_REG_OCSC); > + ufshcd_dump_regs(hba, reg, 44 * 4, "UFS_UFS_DBG_RD_REG_OCSC "); > + > + reg = ufshcd_readl(hba, REG_UFS_CFG1); > + reg |= UTP_DBG_RAMS_EN; > + ufshcd_writel(hba, reg, REG_UFS_CFG1); > + > + reg = ufs_qcom_get_debug_reg_offset(host, UFS_UFS_DBG_RD_EDTL_RAM); > + ufshcd_dump_regs(hba, reg, 32 * 4, "UFS_UFS_DBG_RD_EDTL_RAM "); > + > + reg = ufs_qcom_get_debug_reg_offset(host, UFS_UFS_DBG_RD_DESC_RAM); > + ufshcd_dump_regs(hba, reg, 128 * 4, "UFS_UFS_DBG_RD_DESC_RAM "); > + > + reg = ufs_qcom_get_debug_reg_offset(host, UFS_UFS_DBG_RD_PRDT_RAM); > + ufshcd_dump_regs(hba, reg, 64 * 4, "UFS_UFS_DBG_RD_PRDT_RAM "); > + > + /* clear bit 17 - UTP_DBG_RAMS_EN */ > + ufshcd_rmwl(hba, UTP_DBG_RAMS_EN, 0, REG_UFS_CFG1); > + > + reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_UAWM); > + ufshcd_dump_regs(hba, reg, 4 * 4, "UFS_DBG_RD_REG_UAWM "); > + > + reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_UARM); > + ufshcd_dump_regs(hba, reg, 4 * 4, "UFS_DBG_RD_REG_UARM "); > + > + reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_TXUC); > + ufshcd_dump_regs(hba, reg, 48 * 4, "UFS_DBG_RD_REG_TXUC "); > + > + reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_RXUC); > + ufshcd_dump_regs(hba, reg, 27 * 4, "UFS_DBG_RD_REG_RXUC "); > + > + reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_DFC); > + ufshcd_dump_regs(hba, reg, 19 * 4, "UFS_DBG_RD_REG_DFC "); > + > + reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_TRLUT); > + ufshcd_dump_regs(hba, reg, 34 * 4, "UFS_DBG_RD_REG_TRLUT "); > + > + reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_TMRLUT); > + ufshcd_dump_regs(hba, reg, 9 * 4, "UFS_DBG_RD_REG_TMRLUT "); > } > > /** > -- > 2.38.1 >
On Thu, Dec 01, 2022 at 05:08:09PM -0600, Andrew Halaney wrote: > This bitmask is unconditionally set in the current driver, > so all conditionals using it can be considered bit rot. > > Let's take the current default conditional path everywhere and remove > dbg_print_en from the driver. > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Thanks, Mani > --- > drivers/ufs/host/ufs-qcom.c | 18 +++--------------- > drivers/ufs/host/ufs-qcom.h | 11 ----------- > 2 files changed, 3 insertions(+), 26 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 1b0dfbbdcdf3..b1fcff1fad0c 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -22,9 +22,6 @@ > #include <ufs/ufshci.h> > #include <ufs/ufs_quirks.h> > > -#define UFS_QCOM_DEFAULT_DBG_PRINT_EN \ > - (UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_TEST_BUS_EN) > - > enum { > TSTBUS_UAWM, > TSTBUS_UARM, > @@ -1040,7 +1037,6 @@ static int ufs_qcom_init(struct ufs_hba *hba) > if (hba->dev->id < MAX_UFS_QCOM_HOSTS) > ufs_qcom_hosts[hba->dev->id] = host; > > - host->dbg_print_en |= UFS_QCOM_DEFAULT_DBG_PRINT_EN; > ufs_qcom_get_default_testbus_cfg(host); > err = ufs_qcom_testbus_config(host); > if (err) { > @@ -1191,14 +1187,9 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, > > static void ufs_qcom_enable_test_bus(struct ufs_qcom_host *host) > { > - if (host->dbg_print_en & UFS_QCOM_DBG_PRINT_TEST_BUS_EN) { > - ufshcd_rmwl(host->hba, UFS_REG_TEST_BUS_EN, > - UFS_REG_TEST_BUS_EN, REG_UFS_CFG1); > - ufshcd_rmwl(host->hba, TEST_BUS_EN, TEST_BUS_EN, REG_UFS_CFG1); > - } else { > - ufshcd_rmwl(host->hba, UFS_REG_TEST_BUS_EN, 0, REG_UFS_CFG1); > - ufshcd_rmwl(host->hba, TEST_BUS_EN, 0, REG_UFS_CFG1); > - } > + ufshcd_rmwl(host->hba, UFS_REG_TEST_BUS_EN, > + UFS_REG_TEST_BUS_EN, REG_UFS_CFG1); > + ufshcd_rmwl(host->hba, TEST_BUS_EN, TEST_BUS_EN, REG_UFS_CFG1); > } > > static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host) > @@ -1315,9 +1306,6 @@ static void ufs_qcom_dump_dbg_regs(struct ufs_hba *hba) > ufshcd_dump_regs(hba, REG_UFS_SYS1CLK_1US, 16 * 4, > "HCI Vendor Specific Registers "); > > - if (!(host->dbg_print_en & UFS_QCOM_DBG_PRINT_REGS_EN)) > - return; > - > reg = ufs_qcom_get_debug_reg_offset(host, UFS_UFS_DBG_RD_REG_OCSC); > ufshcd_dump_regs(hba, reg, 44 * 4, "UFS_UFS_DBG_RD_REG_OCSC "); > > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h > index 44466a395bb5..e567e4636357 100644 > --- a/drivers/ufs/host/ufs-qcom.h > +++ b/drivers/ufs/host/ufs-qcom.h > @@ -113,15 +113,6 @@ enum { > MASK_CLK_NS_REG = 0xFFFC00, > }; > > -/* QCOM UFS debug print bit mask */ > -#define UFS_QCOM_DBG_PRINT_REGS_EN BIT(0) > -#define UFS_QCOM_DBG_PRINT_ICE_REGS_EN BIT(1) > -#define UFS_QCOM_DBG_PRINT_TEST_BUS_EN BIT(2) > - > -#define UFS_QCOM_DBG_PRINT_ALL \ > - (UFS_QCOM_DBG_PRINT_REGS_EN | UFS_QCOM_DBG_PRINT_ICE_REGS_EN | \ > - UFS_QCOM_DBG_PRINT_TEST_BUS_EN) > - > /* QUniPro Vendor specific attributes */ > #define PA_VS_CONFIG_REG1 0x9000 > #define DME_VS_CORE_CLK_CTRL 0xD002 > @@ -212,8 +203,6 @@ struct ufs_qcom_host { > > u32 dev_ref_clk_en_mask; > > - /* Bitmask for enabling debug prints */ > - u32 dbg_print_en; > struct ufs_qcom_testbus testbus; > > /* Reset control of HCI */ > -- > 2.38.1 >
Andrew, > This patch series attempts to clean up some debug code paths in the > ufs-qcom driver. Applied to 6.3/scsi-staging, thanks!