linux-kernel - Re: [PATCH V6 1/6] scsi: ufs: qcom: Align mask for core_clk_1us

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230901151713.GQ818859@hu-bjorande-lv.qualcomm.com>
Date:   Fri, 1 Sep 2023 08:17:13 -0700
From:   Bjorn Andersson <quic_bjorande@...cinc.com>
To:     Nitin Rawat <quic_nitirawa@...cinc.com>
CC:     <mani@...nel.org>, <agross@...nel.org>, <andersson@...nel.org>,
        <konrad.dybcio@...aro.org>, <jejb@...ux.ibm.com>,
        <martin.petersen@...cle.com>, <quic_cang@...cinc.com>,
        <quic_nguyenb@...cinc.com>, <linux-scsi@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        "Naveen Kumar Goud Arepalli" <quic_narepall@...cinc.com>
Subject: Re: [PATCH V6 1/6] scsi: ufs: qcom: Align mask for
 core_clk_1us_cycles

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