| Message ID | 20231208065902.11006-17-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: ufs: qcom: Code cleanups | expand |
On Fri, Dec 08, 2023 at 12:29:01PM +0530, Manivannan Sadhasivam wrote: > Instead of using both ufshcd_readl() and ufshcd_writel() to read/modify/ > write a register, let's make use of the existing helper. > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/ufs/host/ufs-qcom.c | 12 ++++-------- > drivers/ufs/host/ufs-qcom.h | 3 +++ > 2 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index 26aa8904c823..549a08645391 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -387,9 +387,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) > */ > static void ufs_qcom_enable_hw_clk_gating(struct ufs_hba *hba) > { > - ufshcd_writel(hba, > - ufshcd_readl(hba, REG_UFS_CFG2) | REG_UFS_CFG2_CGC_EN_ALL, > - REG_UFS_CFG2); > + ufshcd_rmwl(hba, REG_UFS_CFG2_CGC_EN_ALL, REG_UFS_CFG2_CGC_EN_ALL, > + REG_UFS_CFG2); > > /* Ensure that HW clock gating is enabled before next operations */ > mb(); > @@ -1689,11 +1688,8 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba) > platform_msi_domain_free_irqs(hba->dev); > } else { > if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 && > - host->hw_ver.step == 0) { > - ufshcd_writel(hba, > - ufshcd_readl(hba, REG_UFS_CFG3) | 0x1F000, > - REG_UFS_CFG3); > - } > + host->hw_ver.step == 0) > + ufshcd_rmwl(hba, ESI_VEC_MASK, 0x1f00, REG_UFS_CFG3); Is this right? I feel like you accidentally just shifted what was written prior >> 4 bits. Before: 0x1f000 After: 0x01f00 > ufshcd_mcq_enable_esi(hba); > } > > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h > index 385480499e71..2ce63a1c7f2f 100644 > --- a/drivers/ufs/host/ufs-qcom.h > +++ b/drivers/ufs/host/ufs-qcom.h > @@ -102,6 +102,9 @@ enum { > #define TMRLUT_HW_CGC_EN BIT(6) > #define OCSC_HW_CGC_EN BIT(7) > > +/* bit definitions for REG_UFS_CFG3 register */ > +#define ESI_VEC_MASK GENMASK(22, 12) > + I'll leave it to someone with the docs to review this field. It at least contains the bits set above, fwiw. It would be neat to see that converted to using the field + a FIELD_PREP to set the bits IMO, but I honestly don't have a lot of experience using those APIs so feel free to reject that. > /* bit definitions for REG_UFS_PARAM0 */ > #define MAX_HS_GEAR_MASK GENMASK(6, 4) > #define UFS_QCOM_MAX_GEAR(x) FIELD_GET(MAX_HS_GEAR_MASK, (x)) > -- > 2.25.1 >
On Fri, Dec 08, 2023 at 12:02:03PM -0600, Andrew Halaney wrote: > On Fri, Dec 08, 2023 at 12:29:01PM +0530, Manivannan Sadhasivam wrote: > > Instead of using both ufshcd_readl() and ufshcd_writel() to read/modify/ > > write a register, let's make use of the existing helper. > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/ufs/host/ufs-qcom.c | 12 ++++-------- > > drivers/ufs/host/ufs-qcom.h | 3 +++ > > 2 files changed, 7 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > > index 26aa8904c823..549a08645391 100644 > > --- a/drivers/ufs/host/ufs-qcom.c > > +++ b/drivers/ufs/host/ufs-qcom.c > > @@ -387,9 +387,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) > > */ > > static void ufs_qcom_enable_hw_clk_gating(struct ufs_hba *hba) > > { > > - ufshcd_writel(hba, > > - ufshcd_readl(hba, REG_UFS_CFG2) | REG_UFS_CFG2_CGC_EN_ALL, > > - REG_UFS_CFG2); > > + ufshcd_rmwl(hba, REG_UFS_CFG2_CGC_EN_ALL, REG_UFS_CFG2_CGC_EN_ALL, > > + REG_UFS_CFG2); > > > > /* Ensure that HW clock gating is enabled before next operations */ > > mb(); > > @@ -1689,11 +1688,8 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba) > > platform_msi_domain_free_irqs(hba->dev); > > } else { > > if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 && > > - host->hw_ver.step == 0) { > > - ufshcd_writel(hba, > > - ufshcd_readl(hba, REG_UFS_CFG3) | 0x1F000, > > - REG_UFS_CFG3); > > - } > > + host->hw_ver.step == 0) > > + ufshcd_rmwl(hba, ESI_VEC_MASK, 0x1f00, REG_UFS_CFG3); > > Is this right? I feel like you accidentally just shifted what was written > prior >> 4 bits. > > Before: 0x1f000 > After: 0x01f00 > Doh! you are right, I accidentally missed one 0. Since the series is already merged, I will send a follow up patch. > > ufshcd_mcq_enable_esi(hba); > > } > > > > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h > > index 385480499e71..2ce63a1c7f2f 100644 > > --- a/drivers/ufs/host/ufs-qcom.h > > +++ b/drivers/ufs/host/ufs-qcom.h > > @@ -102,6 +102,9 @@ enum { > > #define TMRLUT_HW_CGC_EN BIT(6) > > #define OCSC_HW_CGC_EN BIT(7) > > > > +/* bit definitions for REG_UFS_CFG3 register */ > > +#define ESI_VEC_MASK GENMASK(22, 12) > > + > > I'll leave it to someone with the docs to review this field. It at least > contains the bits set above, fwiw. It would be neat to see that > converted to using the field + a FIELD_PREP to set the bits IMO, but I > honestly don't have a lot of experience using those APIs so feel free to > reject that. > Fair point. The reason for hardcoding the value in this patch was I didn't get information on how the value works. But now I got that info, so will address this in the follow up patch I mentioned above. - Mani > > /* bit definitions for REG_UFS_PARAM0 */ > > #define MAX_HS_GEAR_MASK GENMASK(6, 4) > > #define UFS_QCOM_MAX_GEAR(x) FIELD_GET(MAX_HS_GEAR_MASK, (x)) > > -- > > 2.25.1 > > >
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index 26aa8904c823..549a08645391 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -387,9 +387,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba) */ static void ufs_qcom_enable_hw_clk_gating(struct ufs_hba *hba) { - ufshcd_writel(hba, - ufshcd_readl(hba, REG_UFS_CFG2) | REG_UFS_CFG2_CGC_EN_ALL, - REG_UFS_CFG2); + ufshcd_rmwl(hba, REG_UFS_CFG2_CGC_EN_ALL, REG_UFS_CFG2_CGC_EN_ALL, + REG_UFS_CFG2); /* Ensure that HW clock gating is enabled before next operations */ mb(); @@ -1689,11 +1688,8 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba) platform_msi_domain_free_irqs(hba->dev); } else { if (host->hw_ver.major == 6 && host->hw_ver.minor == 0 && - host->hw_ver.step == 0) { - ufshcd_writel(hba, - ufshcd_readl(hba, REG_UFS_CFG3) | 0x1F000, - REG_UFS_CFG3); - } + host->hw_ver.step == 0) + ufshcd_rmwl(hba, ESI_VEC_MASK, 0x1f00, REG_UFS_CFG3); ufshcd_mcq_enable_esi(hba); } diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h index 385480499e71..2ce63a1c7f2f 100644 --- a/drivers/ufs/host/ufs-qcom.h +++ b/drivers/ufs/host/ufs-qcom.h @@ -102,6 +102,9 @@ enum { #define TMRLUT_HW_CGC_EN BIT(6) #define OCSC_HW_CGC_EN BIT(7) +/* bit definitions for REG_UFS_CFG3 register */ +#define ESI_VEC_MASK GENMASK(22, 12) + /* bit definitions for REG_UFS_PARAM0 */ #define MAX_HS_GEAR_MASK GENMASK(6, 4) #define UFS_QCOM_MAX_GEAR(x) FIELD_GET(MAX_HS_GEAR_MASK, (x))
Instead of using both ufshcd_readl() and ufshcd_writel() to read/modify/ write a register, let's make use of the existing helper. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/ufs/host/ufs-qcom.c | 12 ++++-------- drivers/ufs/host/ufs-qcom.h | 3 +++ 2 files changed, 7 insertions(+), 8 deletions(-)