diff mbox series

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

Message ID 20230901114336.31339-2-quic_nitirawa@quicinc.com (mailing list archive)
State Superseded
Headers show
Series scsi: ufs: qcom: Align programming sequence as per HW spec | expand

Commit Message

Nitin Rawat Sept. 1, 2023, 11:43 a.m. UTC 
Align core_clk_1us_cycles mask for Qualcomm UFS Controller V4.0.0
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(-)

--
2.17.1

Comments

Bjorn Andersson Sept. 1, 2023, 3:17 p.m. UTC | #1 
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
>
Nitin Rawat Sept. 4, 2023, 2:31 p.m. UTC | #2 
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 mbox series

Patch

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,