| Message ID | 20230901114336.31339-2-quic_nitirawa@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs: qcom: Align programming sequence as per HW spec | expand |
On Fri, Sep 01, 2023 at 05:13:31PM +0530, Nitin Rawat wrote: > Align core_clk_1us_cycles mask for Qualcomm UFS Controller V4.0.0 "Align clk mask for ... as per hardware specification."? Are you trying to say "The DME_VS_CORE_CLK_CTRL register has changed in v4 of the Qualcomm UFS controller, introduce support for the new register layout"? You're not aligning the code to match the hardware specification, you're fixing the code because the register has changed. > onwards as per Hardware Specification. > > Co-developed-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com> > Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com> > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> > --- > drivers/ufs/host/ufs-qcom.c | 28 ++++++++++++++++++---------- > drivers/ufs/host/ufs-qcom.h | 5 +++-- > 2 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c > index f88febb23123..fe36003faaa8 100644 > --- a/drivers/ufs/host/ufs-qcom.c > +++ b/drivers/ufs/host/ufs-qcom.c > @@ -1297,22 +1297,30 @@ static void ufs_qcom_exit(struct ufs_hba *hba) > } > > static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba, > - u32 clk_cycles) > + u32 cycles_in_1us) This is a nice clarification, but changing the function prototype gives a sense that you changed the parameters - and that's not the case. So if you drop this rename, you make the purpose of the patch clearer. > { > - int err; > + struct ufs_qcom_host *host = ufshcd_get_variant(hba); > u32 core_clk_ctrl_reg; > + int ret; Renaming err to ret is unrelated and only unnecessary complexity to the patch. > > - if (clk_cycles > DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK) > - return -EINVAL; > - > - err = ufshcd_dme_get(hba, > + ret = ufshcd_dme_get(hba, > UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL), > &core_clk_ctrl_reg); > - if (err) > - return err; > + if (ret) > + return ret; > > - core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK; > - core_clk_ctrl_reg |= clk_cycles; > + /* Bit mask is different for UFS host controller V4.0.0 onwards */ > + if (host->hw_ver.major >= 4) { > + if (!FIELD_FIT(CLK_1US_CYCLES_MASK_V4, cycles_in_1us)) > + return -ERANGE; > + core_clk_ctrl_reg &= ~CLK_1US_CYCLES_MASK_V4; > + core_clk_ctrl_reg |= FIELD_PREP(CLK_1US_CYCLES_MASK_V4, cycles_in_1us); > + } else { > + if (!FIELD_FIT(CLK_1US_CYCLES_MASK, cycles_in_1us)) > + return -ERANGE; > + core_clk_ctrl_reg &= ~CLK_1US_CYCLES_MASK; > + core_clk_ctrl_reg |= FIELD_PREP(CLK_1US_CYCLES_MASK, cycles_in_1us); > + } > > /* Clear CORE_CLK_DIV_EN */ > core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT; > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h > index d6f8e74bd538..8a9d3dbec297 100644 > --- a/drivers/ufs/host/ufs-qcom.h > +++ b/drivers/ufs/host/ufs-qcom.h > @@ -129,8 +129,9 @@ enum { > #define PA_VS_CONFIG_REG1 0x9000 > #define DME_VS_CORE_CLK_CTRL 0xD002 > /* bit and mask definitions for DME_VS_CORE_CLK_CTRL attribute */ > -#define DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT BIT(8) > -#define DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK 0xFF > +#define CLK_1US_CYCLES_MASK_V4 GENMASK(27, 16) > +#define CLK_1US_CYCLES_MASK GENMASK(7, 0) > +#define DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT BIT(8) Hard to say without applying the patch, please double check that the values here have matching indentation level. Thank you, Bjorn > > static inline void > ufs_qcom_get_controller_revision(struct ufs_hba *hba, > -- > 2.17.1 >
On 9/1/2023 8:47 PM, Bjorn Andersson wrote: > On Fri, Sep 01, 2023 at 05:13:31PM +0530, Nitin Rawat wrote: >> Align core_clk_1us_cycles mask for Qualcomm UFS Controller V4.0.0 > > "Align clk mask for ... as per hardware specification."? Are you trying > to say "The DME_VS_CORE_CLK_CTRL register has changed in v4 of the > Qualcomm UFS controller, introduce support for the new register layout"? > > You're not aligning the code to match the hardware specification, you're > fixing the code because the register has changed. > >> onwards as per Hardware Specification. >> Hi Bjorn, Yes, will update the commit text in next patchset >> Co-developed-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com> >> Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@quicinc.com> >> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com> >> --- >> drivers/ufs/host/ufs-qcom.c | 28 ++++++++++++++++++---------- >> drivers/ufs/host/ufs-qcom.h | 5 +++-- >> 2 files changed, 21 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c >> index f88febb23123..fe36003faaa8 100644 >> --- a/drivers/ufs/host/ufs-qcom.c >> +++ b/drivers/ufs/host/ufs-qcom.c >> @@ -1297,22 +1297,30 @@ static void ufs_qcom_exit(struct ufs_hba *hba) >> } >> >> static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba, >> - u32 clk_cycles) >> + u32 cycles_in_1us) > > This is a nice clarification, but changing the function prototype gives > a sense that you changed the parameters - and that's not the case. > > So if you drop this rename, you make the purpose of the patch clearer. > >> { >> - int err; >> + struct ufs_qcom_host *host = ufshcd_get_variant(hba); >> u32 core_clk_ctrl_reg; >> + int ret; > > Renaming err to ret is unrelated and only unnecessary complexity to the > patch. > ok..Would take care of this comment in next patchset. >> >> - if (clk_cycles > DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK) >> - return -EINVAL; >> - >> - err = ufshcd_dme_get(hba, >> + ret = ufshcd_dme_get(hba, >> UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL), >> &core_clk_ctrl_reg); >> - if (err) >> - return err; >> + if (ret) >> + return ret; >> >> - core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK; >> - core_clk_ctrl_reg |= clk_cycles; >> + /* Bit mask is different for UFS host controller V4.0.0 onwards */ >> + if (host->hw_ver.major >= 4) { >> + if (!FIELD_FIT(CLK_1US_CYCLES_MASK_V4, cycles_in_1us)) >> + return -ERANGE; >> + core_clk_ctrl_reg &= ~CLK_1US_CYCLES_MASK_V4; >> + core_clk_ctrl_reg |= FIELD_PREP(CLK_1US_CYCLES_MASK_V4, cycles_in_1us); >> + } else { >> + if (!FIELD_FIT(CLK_1US_CYCLES_MASK, cycles_in_1us)) >> + return -ERANGE; >> + core_clk_ctrl_reg &= ~CLK_1US_CYCLES_MASK; >> + core_clk_ctrl_reg |= FIELD_PREP(CLK_1US_CYCLES_MASK, cycles_in_1us); >> + } >> >> /* Clear CORE_CLK_DIV_EN */ >> core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT; >> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h >> index d6f8e74bd538..8a9d3dbec297 100644 >> --- a/drivers/ufs/host/ufs-qcom.h >> +++ b/drivers/ufs/host/ufs-qcom.h >> @@ -129,8 +129,9 @@ enum { >> #define PA_VS_CONFIG_REG1 0x9000 >> #define DME_VS_CORE_CLK_CTRL 0xD002 >> /* bit and mask definitions for DME_VS_CORE_CLK_CTRL attribute */ >> -#define DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT BIT(8) >> -#define DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK 0xFF >> +#define CLK_1US_CYCLES_MASK_V4 GENMASK(27, 16) >> +#define CLK_1US_CYCLES_MASK GENMASK(7, 0) >> +#define DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT BIT(8) > > Hard to say without applying the patch, please double check that the > values here have matching indentation level. > > Thank you, > Bjorn -Yes I applied and confirmed it's proper. Thanks, Nitin > >> >> static inline void >> ufs_qcom_get_controller_revision(struct ufs_hba *hba, >> -- >> 2.17.1 >>
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index f88febb23123..fe36003faaa8 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -1297,22 +1297,30 @@ static void ufs_qcom_exit(struct ufs_hba *hba) } static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba, - u32 clk_cycles) + u32 cycles_in_1us) { - int err; + struct ufs_qcom_host *host = ufshcd_get_variant(hba); u32 core_clk_ctrl_reg; + int ret; - if (clk_cycles > DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK) - return -EINVAL; - - err = ufshcd_dme_get(hba, + ret = ufshcd_dme_get(hba, UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL), &core_clk_ctrl_reg); - if (err) - return err; + if (ret) + return ret; - core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK; - core_clk_ctrl_reg |= clk_cycles; + /* Bit mask is different for UFS host controller V4.0.0 onwards */ + if (host->hw_ver.major >= 4) { + if (!FIELD_FIT(CLK_1US_CYCLES_MASK_V4, cycles_in_1us)) + return -ERANGE; + core_clk_ctrl_reg &= ~CLK_1US_CYCLES_MASK_V4; + core_clk_ctrl_reg |= FIELD_PREP(CLK_1US_CYCLES_MASK_V4, cycles_in_1us); + } else { + if (!FIELD_FIT(CLK_1US_CYCLES_MASK, cycles_in_1us)) + return -ERANGE; + core_clk_ctrl_reg &= ~CLK_1US_CYCLES_MASK; + core_clk_ctrl_reg |= FIELD_PREP(CLK_1US_CYCLES_MASK, cycles_in_1us); + } /* Clear CORE_CLK_DIV_EN */ core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT; diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h index d6f8e74bd538..8a9d3dbec297 100644 --- a/drivers/ufs/host/ufs-qcom.h +++ b/drivers/ufs/host/ufs-qcom.h @@ -129,8 +129,9 @@ enum { #define PA_VS_CONFIG_REG1 0x9000 #define DME_VS_CORE_CLK_CTRL 0xD002 /* bit and mask definitions for DME_VS_CORE_CLK_CTRL attribute */ -#define DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT BIT(8) -#define DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK 0xFF +#define CLK_1US_CYCLES_MASK_V4 GENMASK(27, 16) +#define CLK_1US_CYCLES_MASK GENMASK(7, 0) +#define DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT BIT(8) static inline void ufs_qcom_get_controller_revision(struct ufs_hba *hba,