Re: [PATCH V6 1/6] scsi: ufs: qcom: Align mask for core_clk_1us_cycles

[Nitin Rawat]

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@xxxxxxxxxxx>
> > Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@xxxxxxxxxxx>
> > Signed-off-by: Nitin Rawat <quic_nitirawa@xxxxxxxxxxx>
> > ---
> >   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