[PATCH V6 0/6] scsi: ufs: qcom: Align programming sequence as per HW spec

[PATCH V6 0/6] scsi: ufs: qcom: Align programming sequence as per HW spec

[Nitin Rawat]

This patch aligns programming sequence as per Qualcomm UFS
hardware specification.

changes from v5:
- Addressed Mani comment to FIELD_PREP and FIELD_FIT.
- Optimised ufs_qcom_set_core_clk_ctrl API.
- Updated commit text for few patches to capture more details.

Changes from v4:
- Addressed bjorn comment to split single patch to multiple patches.

Changes from v3:
-Addressed bjorn comment to update commit msg to capture change details.

Changes from v2:
- Addressed bao comment, removed duplicate clock timer cfg API call

Changes from v1:
- Addressed bao comment, removed wrapper function
- Tab alignment

Nitin Rawat (6):
  scsi: ufs: qcom: Align mask for core_clk_1us_cycles
  scsi: ufs: qcom: Configure PA_VS_CORE_CLK_40NS_CYCLES
  scsi: ufs: qcom: Add multiple frequency support for unipro clk
    attributes
  scsi: ufs: qcom: Align unipro clk attributes configuration as per HPG
  scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers function.
  scsi: ufs: qcom: Configure clk HW division based on scaling
    conditions.

 drivers/ufs/host/ufs-qcom.c | 240 ++++++++++++++++++++++++++----------
 drivers/ufs/host/ufs-qcom.h |  17 ++-
 2 files changed, 193 insertions(+), 64 deletions(-)

--
2.17.1

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

[Nitin Rawat]

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(-)

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,
--
2.17.1

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

[Bjorn Andersson]

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
> 

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@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
>>

[PATCH V6 2/6] scsi: ufs: qcom: Configure PA_VS_CORE_CLK_40NS_CYCLES

[Nitin Rawat]

Configure PA_VS_CORE_CLK_40NS_CYCLES with frequency of unipro core clk
for Qualcomm UFS controller V4.0 and onwards to align with the 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 | 47 ++++++++++++++++++++++++++++---------
 drivers/ufs/host/ufs-qcom.h |  2 ++
 2 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index fe36003faaa8..018e391c276e 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -93,8 +93,9 @@ static const struct __ufs_qcom_bw_table {
 static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];

 static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
-static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
-						       u32 clk_cycles);
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
+						       u32 clk_cycles,
+						       u32 clk_40ns_cycles);

 static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
 {
@@ -690,8 +691,8 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 			 * set unipro core clock cycles to 150 & clear clock
 			 * divider
 			 */
-			err = ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(hba,
-									  150);
+			err = ufs_qcom_set_core_clk_ctrl(hba,
+									  150, 6);

 		/*
 		 * Some UFS devices (and may be host) have issues if LCC is
@@ -1295,12 +1296,12 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
 	phy_power_off(host->generic_phy);
 	phy_exit(host->generic_phy);
 }
-
-static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
-						       u32 cycles_in_1us)
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
+					u32 cycles_in_1us,
+					u32 cycles_in_40ns)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	u32 core_clk_ctrl_reg;
+	u32 core_clk_ctrl_reg, reg;
 	int ret;

 	ret = ufshcd_dme_get(hba,
@@ -1325,9 +1326,33 @@ static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
 	/* Clear CORE_CLK_DIV_EN */
 	core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT;

-	return ufshcd_dme_set(hba,
+	err = ufshcd_dme_set(hba,
 			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
 			    core_clk_ctrl_reg);
+	/*
+	 * UFS host controller V4.0.0 onwards needs to program
+	 * PA_VS_CORE_CLK_40NS_CYCLES attribute per programmed
+	 * frequency of unipro core clk of UFS host controller.
+	 */
+	if (!err && (host->hw_ver.major >= 4)) {
+		if (cycles_in_40ns > PA_VS_CORE_CLK_40NS_CYCLES_MASK)
+			return -EINVAL;
+
+		err = ufshcd_dme_get(hba,
+				     UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES),
+				     &reg);
+		if (err)
+			return err;
+
+		reg &= ~PA_VS_CORE_CLK_40NS_CYCLES_MASK;
+		reg |= cycles_in_40ns;
+
+		err = ufshcd_dme_set(hba,
+				     UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES),
+				     reg);
+	}
+
+	return err;
 }

 static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
@@ -1344,7 +1369,7 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
 		return 0;

 	/* set unipro core clock cycles to 150 and clear clock divider */
-	return ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(hba, 150);
+	return ufs_qcom_set_core_clk_ctrl(hba, 150, 6);
 }

 static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
@@ -1380,7 +1405,7 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
 		return 0;

 	/* set unipro core clock cycles to 75 and clear clock divider */
-	return ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(hba, 75);
+	return ufs_qcom_set_core_clk_ctrl(hba, 75, 3);
 }

 static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 8a9d3dbec297..d81bf1a1b77a 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -132,6 +132,8 @@ enum {
 #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)
+#define PA_VS_CORE_CLK_40NS_CYCLES			0x9007
+#define PA_VS_CORE_CLK_40NS_CYCLES_MASK			GENMASK(6, 0)

 static inline void
 ufs_qcom_get_controller_revision(struct ufs_hba *hba,
--
2.17.1

Re: [PATCH V6 2/6] scsi: ufs: qcom: Configure PA_VS_CORE_CLK_40NS_CYCLES

[Bjorn Andersson]

On Fri, Sep 01, 2023 at 05:13:32PM +0530, Nitin Rawat wrote:
> Configure PA_VS_CORE_CLK_40NS_CYCLES with frequency of unipro core clk
> for Qualcomm UFS controller V4.0 and onwards to align with the 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 | 47 ++++++++++++++++++++++++++++---------
>  drivers/ufs/host/ufs-qcom.h |  2 ++
>  2 files changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index fe36003faaa8..018e391c276e 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -93,8 +93,9 @@ static const struct __ufs_qcom_bw_table {
>  static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
> 
>  static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
> -static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
> -						       u32 clk_cycles);
> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
> +						       u32 clk_cycles,
> +						       u32 clk_40ns_cycles);
> 
>  static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
>  {
> @@ -690,8 +691,8 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
>  			 * set unipro core clock cycles to 150 & clear clock
>  			 * divider
>  			 */
> -			err = ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(hba,
> -									  150);
> +			err = ufs_qcom_set_core_clk_ctrl(hba,
> +									  150, 6);

There's no reason to maintain the line wrap here, and the new
indentation looks wrong.

> 
>  		/*
>  		 * Some UFS devices (and may be host) have issues if LCC is
> @@ -1295,12 +1296,12 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
>  	phy_power_off(host->generic_phy);
>  	phy_exit(host->generic_phy);
>  }
> -
> -static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
> -						       u32 cycles_in_1us)
> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
> +					u32 cycles_in_1us,

Following my comment on this rename in the last commit, here you do
change the prototype, so here you need the rename to clarify the intent,
so make it here instead.

> +					u32 cycles_in_40ns)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> -	u32 core_clk_ctrl_reg;
> +	u32 core_clk_ctrl_reg, reg;

Please keep one variable declaration per line, in particular when using
names like "core_clk_ctrl_reg" which looks much more like a type than a
variable name...

I would have preferred "reg" instead of core_clk_ctrl_reg" and you could
have used that same variable for both sections.

>  	int ret;
> 
>  	ret = ufshcd_dme_get(hba,
> @@ -1325,9 +1326,33 @@ static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
>  	/* Clear CORE_CLK_DIV_EN */
>  	core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT;
> 
> -	return ufshcd_dme_set(hba,
> +	err = ufshcd_dme_set(hba,

I might be confused, but didn't you remove "err" in patch 1? Does this
patch compile?

>  			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
>  			    core_clk_ctrl_reg);
> +	/*
> +	 * UFS host controller V4.0.0 onwards needs to program
> +	 * PA_VS_CORE_CLK_40NS_CYCLES attribute per programmed
> +	 * frequency of unipro core clk of UFS host controller.
> +	 */
> +	if (!err && (host->hw_ver.major >= 4)) {

Rather than if (previous-block-didn't-fail), add an explicit return if
err is unfavourable. That will make it more obvious that this section
relates to version >= 4 and nothing else.


That said, you now have one function:

core_clk_ctrl(int a, int b)
{
	do stuff based on a;

	if (version > 4)
		do stuff based on b;
}

It's pretty much one function for cycles_in_1us, with a separate
function for cycles_in_40us bolted on at the end. How about just
splitting it in two functions instead, and provide a small wrapper that
calls one or both functions?

> +		if (cycles_in_40ns > PA_VS_CORE_CLK_40NS_CYCLES_MASK)
> +			return -EINVAL;
> +
> +		err = ufshcd_dme_get(hba,
> +				     UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES),
> +				     &reg);

Leaving this line unwrapped seems to end up at 89 characters wide.
Perfectly reasonable to do, in the name of readability.

> +		if (err)
> +			return err;
> +
> +		reg &= ~PA_VS_CORE_CLK_40NS_CYCLES_MASK;
> +		reg |= cycles_in_40ns;
> +
> +		err = ufshcd_dme_set(hba,
> +				     UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES),
> +				     reg);

Same as above.

Regards,
Bjorn

Re: [PATCH V6 2/6] scsi: ufs: qcom: Configure PA_VS_CORE_CLK_40NS_CYCLES

[Nitin Rawat]

On 9/1/2023 9:00 PM, Bjorn Andersson wrote:
> On Fri, Sep 01, 2023 at 05:13:32PM +0530, Nitin Rawat wrote:
>> Configure PA_VS_CORE_CLK_40NS_CYCLES with frequency of unipro core clk
>> for Qualcomm UFS controller V4.0 and onwards to align with the 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 | 47 ++++++++++++++++++++++++++++---------
>>   drivers/ufs/host/ufs-qcom.h |  2 ++
>>   2 files changed, 38 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index fe36003faaa8..018e391c276e 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -93,8 +93,9 @@ static const struct __ufs_qcom_bw_table {
>>   static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
>>
>>   static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
>> -static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
>> -						       u32 clk_cycles);
>> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
>> +						       u32 clk_cycles,
>> +						       u32 clk_40ns_cycles);
>>
>>   static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
>>   {
>> @@ -690,8 +691,8 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
>>   			 * set unipro core clock cycles to 150 & clear clock
>>   			 * divider
>>   			 */
>> -			err = ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(hba,
>> -									  150);
>> +			err = ufs_qcom_set_core_clk_ctrl(hba,
>> +									  150, 6);
> 
> There's no reason to maintain the line wrap here, and the new
> indentation looks wrong.

   Sure..will remove it next patchset.

> 
>>
>>   		/*
>>   		 * Some UFS devices (and may be host) have issues if LCC is
>> @@ -1295,12 +1296,12 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
>>   	phy_power_off(host->generic_phy);
>>   	phy_exit(host->generic_phy);
>>   }
>> -
>> -static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
>> -						       u32 cycles_in_1us)
>> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
>> +					u32 cycles_in_1us,
> 
> Following my comment on this rename in the last commit, here you do
> change the prototype, so here you need the rename to clarify the intent,
> so make it here instead.


We will change the order of patch so that we need change the prototype 
in 1 place.

-Nitin

> 
>> +					u32 cycles_in_40ns)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> -	u32 core_clk_ctrl_reg;
>> +	u32 core_clk_ctrl_reg, reg;
> 
> Please keep one variable declaration per line, in particular when using
> names like "core_clk_ctrl_reg" which looks much more like a type than a
> variable name...
> 
> I would have preferred "reg" instead of core_clk_ctrl_reg" and you could
> have used that same variable for both sections.
> 
>>   	int ret;
>>
>>   	ret = ufshcd_dme_get(hba,
>> @@ -1325,9 +1326,33 @@ static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
>>   	/* Clear CORE_CLK_DIV_EN */
>>   	core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT;
>>
>> -	return ufshcd_dme_set(hba,
>> +	err = ufshcd_dme_set(hba,
> 
> I might be confused, but didn't you remove "err" in patch 1? Does this
> patch compile?
> 
>>   			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
>>   			    core_clk_ctrl_reg);
>> +	/*
>> +	 * UFS host controller V4.0.0 onwards needs to program
>> +	 * PA_VS_CORE_CLK_40NS_CYCLES attribute per programmed
>> +	 * frequency of unipro core clk of UFS host controller.
>> +	 */
>> +	if (!err && (host->hw_ver.major >= 4)) {
> 
> Rather than if (previous-block-didn't-fail), add an explicit return if
> err is unfavourable. That will make it more obvious that this section
> relates to version >= 4 and nothing else.

Sure..Will take care in next patchset

> 
> 
> That said, you now have one function:
> 
> core_clk_ctrl(int a, int b)
> {
> 	do stuff based on a;
> 
> 	if (version > 4)
> 		do stuff based on b;
> }
> 
> It's pretty much one function for cycles_in_1us, with a separate
> function for cycles_in_40us bolted on at the end. How about just
> splitting it in two functions instead, and provide a small wrapper that
> calls one or both functions?

Sure...Will move cycles_in_40us to different function.



> 
>> +		if (cycles_in_40ns > PA_VS_CORE_CLK_40NS_CYCLES_MASK)
>> +			return -EINVAL;
>> +
>> +		err = ufshcd_dme_get(hba,
>> +				     UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES),
>> +				     &reg);
> 
> Leaving this line unwrapped seems to end up at 89 characters wide.
> Perfectly reasonable to do, in the name of readability.
> 
  Sure..Will address this in next patchset.


>> +		if (err)
>> +			return err;
>> +
>> +		reg &= ~PA_VS_CORE_CLK_40NS_CYCLES_MASK;
>> +		reg |= cycles_in_40ns;
>> +
>> +		err = ufshcd_dme_set(hba,
>> +				     UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES),
>> +				     reg);
> 
> Same as above.
Sure..Will address this in next patchset.

> 
> Regards,
> Bjorn

Thanks,
Nitin

[PATCH V6 3/6] scsi: ufs: qcom: Add multiple frequency support for unipro clk attributes

[Nitin Rawat]

Add Support to configure CORE_CLK_1US_CYCLES, PA_VS_CORE_CLK_40NS_CYCLES
for multiple unipro clock frequencies. Currently this is handled only for
only 150Mhz and 75MHz.

Since different qualcomm targets support different unipro frequency, add
support to handle all other frequencies like 403MHz, 300MHz, 202MHz,
150 MHz, 75Mhz, 37.5 MHz.

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 | 96 ++++++++++++++++++++++++++++---------
 drivers/ufs/host/ufs-qcom.h | 10 ++++
 2 files changed, 84 insertions(+), 22 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 018e391c276e..e3648e936498 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -94,8 +94,7 @@ static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];

 static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
 static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
-						       u32 clk_cycles,
-						       u32 clk_40ns_cycles);
+					bool is_max_freq);

 static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
 {
@@ -686,13 +685,14 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 			return -EINVAL;
 		}

-		if (ufs_qcom_cap_qunipro(host))
-			/*
-			 * set unipro core clock cycles to 150 & clear clock
-			 * divider
-			 */
-			err = ufs_qcom_set_core_clk_ctrl(hba,
-									  150, 6);
+		if (ufs_qcom_cap_qunipro(host)) {
+			err = ufs_qcom_set_core_clk_ctrl(hba, true);
+			if (err) {
+				dev_err(hba->dev,
+				"%s cfg core clk ctrl failed\n",
+				__func__);
+			}
+		}

 		/*
 		 * Some UFS devices (and may be host) have issues if LCC is
@@ -1297,13 +1297,67 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
 	phy_exit(host->generic_phy);
 }
 static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
-					u32 cycles_in_1us,
-					u32 cycles_in_40ns)
+					bool is_max_freq)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	struct list_head *head = &hba->clk_list_head;
 	u32 core_clk_ctrl_reg, reg;
+	struct ufs_clk_info *clki;
+	u32 cycles_in_1us, cycles_in_40ns;
 	int ret;

+	list_for_each_entry(clki, head, list) {
+		if (!IS_ERR_OR_NULL(clki->clk) &&
+			!strcmp(clki->name, "core_clk_unipro")) {
+			if (is_max_freq)
+				cycles_in_1us = clki->max_freq;
+			else
+				cycles_in_1us = clk_get_rate(clki->clk);
+			break;
+		}
+	}
+
+	if ((cycles_in_1us % (1000 * 1000)) != 0)
+		cycles_in_1us = cycles_in_1us/(1000 * 1000) + 1;
+	else
+		cycles_in_1us = cycles_in_1us/(1000 * 1000);
+
+	/*
+	 * Generic formulae for cycles_in_40ns = (freq_unipro/25) is not
+	 * applicable for all frequencies. For ex: ceil(37.5 MHz/25) will
+	 * be 2 and ceil(403 MHZ/25) will be 17 whereas Hardware
+	 * specification expect to be 16. Hence use exact hardware spec
+	 * mandated value for cycles_in_40ns instead of calculating using
+	 * generic formulae.
+	 */
+	switch (cycles_in_1us) {
+	case UNIPRO_CORE_CLK_FREQ_403_MHZ:
+		cycles_in_40ns = 16;
+		break;
+	case UNIPRO_CORE_CLK_FREQ_300_MHZ:
+		cycles_in_40ns = 12;
+		break;
+	case UNIPRO_CORE_CLK_FREQ_201_5_MHZ:
+		cycles_in_40ns = 8;
+		break;
+	case UNIPRO_CORE_CLK_FREQ_150_MHZ:
+		cycles_in_40ns = 6;
+		break;
+	case UNIPRO_CORE_CLK_FREQ_100_MHZ:
+		cycles_in_40ns = 4;
+		break;
+	case  UNIPRO_CORE_CLK_FREQ_75_MHZ:
+		cycles_in_40ns = 3;
+		break;
+	case UNIPRO_CORE_CLK_FREQ_37_5_MHZ:
+		cycles_in_40ns = 2;
+		break;
+	default:
+		ret = -EINVAL;
+		dev_err(hba->dev, "UNIPRO clk freq %u MHz not supported\n",
+					cycles_in_1us);
+	}
+
 	ret = ufshcd_dme_get(hba,
 			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
 			    &core_clk_ctrl_reg);
@@ -1326,7 +1380,7 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
 	/* Clear CORE_CLK_DIV_EN */
 	core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT;

-	err = ufshcd_dme_set(hba,
+	ret = ufshcd_dme_set(hba,
 			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
 			    core_clk_ctrl_reg);
 	/*
@@ -1334,25 +1388,25 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
 	 * PA_VS_CORE_CLK_40NS_CYCLES attribute per programmed
 	 * frequency of unipro core clk of UFS host controller.
 	 */
-	if (!err && (host->hw_ver.major >= 4)) {
+	if (!ret && (host->hw_ver.major >= 4)) {
 		if (cycles_in_40ns > PA_VS_CORE_CLK_40NS_CYCLES_MASK)
 			return -EINVAL;

-		err = ufshcd_dme_get(hba,
+		ret = ufshcd_dme_get(hba,
 				     UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES),
 				     &reg);
-		if (err)
-			return err;
+		if (ret)
+			return ret;

 		reg &= ~PA_VS_CORE_CLK_40NS_CYCLES_MASK;
 		reg |= cycles_in_40ns;

-		err = ufshcd_dme_set(hba,
+		ret = ufshcd_dme_set(hba,
 				     UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES),
 				     reg);
 	}

-	return err;
+	return ret;
 }

 static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
@@ -1368,8 +1422,7 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
 	if (!ufs_qcom_cap_qunipro(host))
 		return 0;

-	/* set unipro core clock cycles to 150 and clear clock divider */
-	return ufs_qcom_set_core_clk_ctrl(hba, 150, 6);
+	return ufs_qcom_set_core_clk_ctrl(hba, true);
 }

 static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
@@ -1404,8 +1457,7 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
 	if (!ufs_qcom_cap_qunipro(host))
 		return 0;

-	/* set unipro core clock cycles to 75 and clear clock divider */
-	return ufs_qcom_set_core_clk_ctrl(hba, 75, 3);
+	return ufs_qcom_set_core_clk_ctrl(hba, false);
 }

 static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index d81bf1a1b77a..bc176ef58e3e 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -9,6 +9,7 @@
 #include <linux/reset.h>
 #include <soc/qcom/ice.h>
 #include <ufs/ufshcd.h>
+#include <linux/math.h>

 #define MAX_UFS_QCOM_HOSTS	1
 #define MAX_U32                 (~(u32)0)
@@ -79,6 +80,15 @@ enum {
 	UFS_MEM_CQIS_VS		= 0x8,
 };

+/* QCOM UFS host controller core clk frequencies */
+#define UNIPRO_CORE_CLK_FREQ_37_5_MHZ		38
+#define UNIPRO_CORE_CLK_FREQ_75_MHZ		75
+#define UNIPRO_CORE_CLK_FREQ_100_MHZ		100
+#define UNIPRO_CORE_CLK_FREQ_150_MHZ		150
+#define UNIPRO_CORE_CLK_FREQ_300_MHZ		300
+#define UNIPRO_CORE_CLK_FREQ_201_5_MHZ		202
+#define UNIPRO_CORE_CLK_FREQ_403_MHZ		403
+
 #define UFS_CNTLR_2_x_x_VEN_REGS_OFFSET(x)	(0x000 + x)
 #define UFS_CNTLR_3_x_x_VEN_REGS_OFFSET(x)	(0x400 + x)

--
2.17.1

Re: [PATCH V6 3/6] scsi: ufs: qcom: Add multiple frequency support for unipro clk attributes

[Bjorn Andersson]

On Fri, Sep 01, 2023 at 05:13:33PM +0530, Nitin Rawat wrote:
> Add Support to configure CORE_CLK_1US_CYCLES, PA_VS_CORE_CLK_40NS_CYCLES
> for multiple unipro clock frequencies. Currently this is handled only for
> only 150Mhz and 75MHz.
> 
> Since different qualcomm targets support different unipro frequency, add
> support to handle all other frequencies like 403MHz, 300MHz, 202MHz,
> 150 MHz, 75Mhz, 37.5 MHz.
> 

Please flip your commit message around, start with the problem
description (the second paragraph), then the description of what you're
changing.

> 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 | 96 ++++++++++++++++++++++++++++---------
>  drivers/ufs/host/ufs-qcom.h | 10 ++++
>  2 files changed, 84 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 018e391c276e..e3648e936498 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -94,8 +94,7 @@ static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
> 
>  static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
>  static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
> -						       u32 clk_cycles,
> -						       u32 clk_40ns_cycles);
> +					bool is_max_freq);

Odd indentation here. That said, I doubt that you need the line wrap
now.

> 
>  static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
>  {
> @@ -686,13 +685,14 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
>  			return -EINVAL;
>  		}
> 
> -		if (ufs_qcom_cap_qunipro(host))
> -			/*
> -			 * set unipro core clock cycles to 150 & clear clock
> -			 * divider
> -			 */
> -			err = ufs_qcom_set_core_clk_ctrl(hba,
> -									  150, 6);
> +		if (ufs_qcom_cap_qunipro(host)) {
> +			err = ufs_qcom_set_core_clk_ctrl(hba, true);
> +			if (err) {
> +				dev_err(hba->dev,
> +				"%s cfg core clk ctrl failed\n",
> +				__func__);

Please don't build error messages using __func__, describe the actual
error, in English.

> +			}
> +		}
> 
>  		/*
>  		 * Some UFS devices (and may be host) have issues if LCC is
> @@ -1297,13 +1297,67 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
>  	phy_exit(host->generic_phy);
>  }
>  static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
> -					u32 cycles_in_1us,
> -					u32 cycles_in_40ns)
> +					bool is_max_freq)

You changed this function prototype in both patch 1 and patch 2, and now
you're changing it again. What if you make this transition in patch 1,
then you only need to change the prototype once.

>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> +	struct list_head *head = &hba->clk_list_head;
>  	u32 core_clk_ctrl_reg, reg;
> +	struct ufs_clk_info *clki;
> +	u32 cycles_in_1us, cycles_in_40ns;
>  	int ret;
> 
> +	list_for_each_entry(clki, head, list) {
> +		if (!IS_ERR_OR_NULL(clki->clk) &&
> +			!strcmp(clki->name, "core_clk_unipro")) {
> +			if (is_max_freq)
> +				cycles_in_1us = clki->max_freq;
> +			else
> +				cycles_in_1us = clk_get_rate(clki->clk);
> +			break;
> +		}
> +	}
> +
> +	if ((cycles_in_1us % (1000 * 1000)) != 0)
> +		cycles_in_1us = cycles_in_1us/(1000 * 1000) + 1;
> +	else
> +		cycles_in_1us = cycles_in_1us/(1000 * 1000);
> +

Iiuc, the following 34 lines only apply to hw_ver >= 4.

> +	/*
> +	 * Generic formulae for cycles_in_40ns = (freq_unipro/25) is not
> +	 * applicable for all frequencies. For ex: ceil(37.5 MHz/25) will
> +	 * be 2 and ceil(403 MHZ/25) will be 17 whereas Hardware
> +	 * specification expect to be 16. Hence use exact hardware spec
> +	 * mandated value for cycles_in_40ns instead of calculating using
> +	 * generic formulae.
> +	 */
> +	switch (cycles_in_1us) {
> +	case UNIPRO_CORE_CLK_FREQ_403_MHZ:
> +		cycles_in_40ns = 16;
> +		break;
> +	case UNIPRO_CORE_CLK_FREQ_300_MHZ:
> +		cycles_in_40ns = 12;
> +		break;
> +	case UNIPRO_CORE_CLK_FREQ_201_5_MHZ:
> +		cycles_in_40ns = 8;
> +		break;
> +	case UNIPRO_CORE_CLK_FREQ_150_MHZ:
> +		cycles_in_40ns = 6;
> +		break;
> +	case UNIPRO_CORE_CLK_FREQ_100_MHZ:
> +		cycles_in_40ns = 4;
> +		break;
> +	case  UNIPRO_CORE_CLK_FREQ_75_MHZ:
> +		cycles_in_40ns = 3;
> +		break;
> +	case UNIPRO_CORE_CLK_FREQ_37_5_MHZ:
> +		cycles_in_40ns = 2;
> +		break;
> +	default:
> +		ret = -EINVAL;

5 lines below, you're overwriting this value and moving on as if nothing
happened. Doesn't your compiler complain that you then will use
cycles_in_40ns without first initializing it in this case?

> +		dev_err(hba->dev, "UNIPRO clk freq %u MHz not supported\n",
> +					cycles_in_1us);
> +	}
> +
>  	ret = ufshcd_dme_get(hba,
>  			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
>  			    &core_clk_ctrl_reg);
> @@ -1326,7 +1380,7 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
>  	/* Clear CORE_CLK_DIV_EN */
>  	core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT;

The following 6 changes just corrects the compilation error you
shouldn't have introduced in the previous patch.

> 
> -	err = ufshcd_dme_set(hba,
> +	ret = ufshcd_dme_set(hba,
>  			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
>  			    core_clk_ctrl_reg);
>  	/*
> @@ -1334,25 +1388,25 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
>  	 * PA_VS_CORE_CLK_40NS_CYCLES attribute per programmed
>  	 * frequency of unipro core clk of UFS host controller.
>  	 */
> -	if (!err && (host->hw_ver.major >= 4)) {
> +	if (!ret && (host->hw_ver.major >= 4)) {
>  		if (cycles_in_40ns > PA_VS_CORE_CLK_40NS_CYCLES_MASK)
>  			return -EINVAL;
> 
> -		err = ufshcd_dme_get(hba,
> +		ret = ufshcd_dme_get(hba,
>  				     UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES),
>  				     &reg);
> -		if (err)
> -			return err;
> +		if (ret)
> +			return ret;
> 
>  		reg &= ~PA_VS_CORE_CLK_40NS_CYCLES_MASK;
>  		reg |= cycles_in_40ns;
> 
> -		err = ufshcd_dme_set(hba,
> +		ret = ufshcd_dme_set(hba,
>  				     UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES),
>  				     reg);
>  	}
> 
> -	return err;
> +	return ret;
>  }
> 
>  static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
> @@ -1368,8 +1422,7 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
>  	if (!ufs_qcom_cap_qunipro(host))
>  		return 0;
> 
> -	/* set unipro core clock cycles to 150 and clear clock divider */
> -	return ufs_qcom_set_core_clk_ctrl(hba, 150, 6);
> +	return ufs_qcom_set_core_clk_ctrl(hba, true);
>  }
> 
>  static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
> @@ -1404,8 +1457,7 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
>  	if (!ufs_qcom_cap_qunipro(host))
>  		return 0;
> 
> -	/* set unipro core clock cycles to 75 and clear clock divider */
> -	return ufs_qcom_set_core_clk_ctrl(hba, 75, 3);
> +	return ufs_qcom_set_core_clk_ctrl(hba, false);
>  }
> 
>  static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index d81bf1a1b77a..bc176ef58e3e 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -9,6 +9,7 @@
>  #include <linux/reset.h>
>  #include <soc/qcom/ice.h>
>  #include <ufs/ufshcd.h>
> +#include <linux/math.h>

Why is math.h needed in this header file now?

Regards,
Bjorn

> 
>  #define MAX_UFS_QCOM_HOSTS	1
>  #define MAX_U32                 (~(u32)0)
> @@ -79,6 +80,15 @@ enum {
>  	UFS_MEM_CQIS_VS		= 0x8,
>  };
> 
> +/* QCOM UFS host controller core clk frequencies */
> +#define UNIPRO_CORE_CLK_FREQ_37_5_MHZ		38
> +#define UNIPRO_CORE_CLK_FREQ_75_MHZ		75
> +#define UNIPRO_CORE_CLK_FREQ_100_MHZ		100
> +#define UNIPRO_CORE_CLK_FREQ_150_MHZ		150
> +#define UNIPRO_CORE_CLK_FREQ_300_MHZ		300
> +#define UNIPRO_CORE_CLK_FREQ_201_5_MHZ		202
> +#define UNIPRO_CORE_CLK_FREQ_403_MHZ		403
> +
>  #define UFS_CNTLR_2_x_x_VEN_REGS_OFFSET(x)	(0x000 + x)
>  #define UFS_CNTLR_3_x_x_VEN_REGS_OFFSET(x)	(0x400 + x)
> 
> --
> 2.17.1
> 

Re: [PATCH V6 3/6] scsi: ufs: qcom: Add multiple frequency support for unipro clk attributes

[Nitin Rawat]

On 9/1/2023 9:10 PM, Bjorn Andersson wrote:
> On Fri, Sep 01, 2023 at 05:13:33PM +0530, Nitin Rawat wrote:
>> Add Support to configure CORE_CLK_1US_CYCLES, PA_VS_CORE_CLK_40NS_CYCLES
>> for multiple unipro clock frequencies. Currently this is handled only for
>> only 150Mhz and 75MHz.
>>
>> Since different qualcomm targets support different unipro frequency, add
>> support to handle all other frequencies like 403MHz, 300MHz, 202MHz,
>> 150 MHz, 75Mhz, 37.5 MHz.
>>
> 
> Please flip your commit message around, start with the problem
> description (the second paragraph), then the description of what you're
> changing.

Sure will address this 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 | 96 ++++++++++++++++++++++++++++---------
>>   drivers/ufs/host/ufs-qcom.h | 10 ++++
>>   2 files changed, 84 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 018e391c276e..e3648e936498 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -94,8 +94,7 @@ static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
>>
>>   static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
>>   static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
>> -						       u32 clk_cycles,
>> -						       u32 clk_40ns_cycles);
>> +					bool is_max_freq);
> 
> Odd indentation here. That said, I doubt that you need the line wrap
> now.

Sure will address this in next patchset.

> 
>>
>>   static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
>>   {
>> @@ -686,13 +685,14 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
>>   			return -EINVAL;
>>   		}
>>
>> -		if (ufs_qcom_cap_qunipro(host))
>> -			/*
>> -			 * set unipro core clock cycles to 150 & clear clock
>> -			 * divider
>> -			 */
>> -			err = ufs_qcom_set_core_clk_ctrl(hba,
>> -									  150, 6);
>> +		if (ufs_qcom_cap_qunipro(host)) {
>> +			err = ufs_qcom_set_core_clk_ctrl(hba, true);
>> +			if (err) {
>> +				dev_err(hba->dev,
>> +				"%s cfg core clk ctrl failed\n",
>> +				__func__);
> 
> Please don't build error messages using __func__, describe the actual
> error, in English.

Sure will address this in next patchset.

> 
>> +			}
>> +		}
>>
>>   		/*
>>   		 * Some UFS devices (and may be host) have issues if LCC is
>> @@ -1297,13 +1297,67 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
>>   	phy_exit(host->generic_phy);
>>   }
>>   static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
>> -					u32 cycles_in_1us,
>> -					u32 cycles_in_40ns)
>> +					bool is_max_freq)
> 
> You changed this function prototype in both patch 1 and patch 2, and now
> you're changing it again. What if you make this transition in patch 1,
> then you only need to change the prototype once.

Sure we will change the order of patch so that prototype will change in 
1 place.

> 
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> +	struct list_head *head = &hba->clk_list_head;
>>   	u32 core_clk_ctrl_reg, reg;
>> +	struct ufs_clk_info *clki;
>> +	u32 cycles_in_1us, cycles_in_40ns;
>>   	int ret;
>>
>> +	list_for_each_entry(clki, head, list) {
>> +		if (!IS_ERR_OR_NULL(clki->clk) &&
>> +			!strcmp(clki->name, "core_clk_unipro")) {
>> +			if (is_max_freq)
>> +				cycles_in_1us = clki->max_freq;
>> +			else
>> +				cycles_in_1us = clk_get_rate(clki->clk);
>> +			break;
>> +		}
>> +	}
>> +
>> +	if ((cycles_in_1us % (1000 * 1000)) != 0)
>> +		cycles_in_1us = cycles_in_1us/(1000 * 1000) + 1;
>> +	else
>> +		cycles_in_1us = cycles_in_1us/(1000 * 1000);
>> +
> 
> Iiuc, the following 34 lines only apply to hw_ver >= 4.
Sure...Will address this in next patchset.

> 
>> +	/*
>> +	 * Generic formulae for cycles_in_40ns = (freq_unipro/25) is not
>> +	 * applicable for all frequencies. For ex: ceil(37.5 MHz/25) will
>> +	 * be 2 and ceil(403 MHZ/25) will be 17 whereas Hardware
>> +	 * specification expect to be 16. Hence use exact hardware spec
>> +	 * mandated value for cycles_in_40ns instead of calculating using
>> +	 * generic formulae.
>> +	 */
>> +	switch (cycles_in_1us) {
>> +	case UNIPRO_CORE_CLK_FREQ_403_MHZ:
>> +		cycles_in_40ns = 16;
>> +		break;
>> +	case UNIPRO_CORE_CLK_FREQ_300_MHZ:
>> +		cycles_in_40ns = 12;
>> +		break;
>> +	case UNIPRO_CORE_CLK_FREQ_201_5_MHZ:
>> +		cycles_in_40ns = 8;
>> +		break;
>> +	case UNIPRO_CORE_CLK_FREQ_150_MHZ:
>> +		cycles_in_40ns = 6;
>> +		break;
>> +	case UNIPRO_CORE_CLK_FREQ_100_MHZ:
>> +		cycles_in_40ns = 4;
>> +		break;
>> +	case  UNIPRO_CORE_CLK_FREQ_75_MHZ:
>> +		cycles_in_40ns = 3;
>> +		break;
>> +	case UNIPRO_CORE_CLK_FREQ_37_5_MHZ:
>> +		cycles_in_40ns = 2;
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
> 
- In next patchset we are returning Error from default case.

> 5 lines below, you're overwriting this value and moving on as if nothing
> happened. Doesn't your compiler complain that you then will use
> cycles_in_40ns without first initializing it in this case?
> 
>> +		dev_err(hba->dev, "UNIPRO clk freq %u MHz not supported\n",
>> +					cycles_in_1us);
>> +	}
>> +
>>   	ret = ufshcd_dme_get(hba,
>>   			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
>>   			    &core_clk_ctrl_reg);
>> @@ -1326,7 +1380,7 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
>>   	/* Clear CORE_CLK_DIV_EN */
>>   	core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT;
> 
> The following 6 changes just corrects the compilation error you
> shouldn't have introduced in the previous patch.
> 
>>
>> -	err = ufshcd_dme_set(hba,
>> +	ret = ufshcd_dme_set(hba,
>>   			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
>>   			    core_clk_ctrl_reg);
>>   	/*
>> @@ -1334,25 +1388,25 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
>>   	 * PA_VS_CORE_CLK_40NS_CYCLES attribute per programmed
>>   	 * frequency of unipro core clk of UFS host controller.
>>   	 */
>> -	if (!err && (host->hw_ver.major >= 4)) {
>> +	if (!ret && (host->hw_ver.major >= 4)) {
>>   		if (cycles_in_40ns > PA_VS_CORE_CLK_40NS_CYCLES_MASK)
>>   			return -EINVAL;
>>
>> -		err = ufshcd_dme_get(hba,
>> +		ret = ufshcd_dme_get(hba,
>>   				     UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES),
>>   				     &reg);
>> -		if (err)
>> -			return err;
>> +		if (ret)
>> +			return ret;
>>
>>   		reg &= ~PA_VS_CORE_CLK_40NS_CYCLES_MASK;
>>   		reg |= cycles_in_40ns;
>>
>> -		err = ufshcd_dme_set(hba,
>> +		ret = ufshcd_dme_set(hba,
>>   				     UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES),
>>   				     reg);
>>   	}
>>
>> -	return err;
>> +	return ret;
>>   }
>>
>>   static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
>> @@ -1368,8 +1422,7 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
>>   	if (!ufs_qcom_cap_qunipro(host))
>>   		return 0;
>>
>> -	/* set unipro core clock cycles to 150 and clear clock divider */
>> -	return ufs_qcom_set_core_clk_ctrl(hba, 150, 6);
>> +	return ufs_qcom_set_core_clk_ctrl(hba, true);
>>   }
>>
>>   static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
>> @@ -1404,8 +1457,7 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
>>   	if (!ufs_qcom_cap_qunipro(host))
>>   		return 0;
>>
>> -	/* set unipro core clock cycles to 75 and clear clock divider */
>> -	return ufs_qcom_set_core_clk_ctrl(hba, 75, 3);
>> +	return ufs_qcom_set_core_clk_ctrl(hba, false);
>>   }
>>
>>   static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
>> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
>> index d81bf1a1b77a..bc176ef58e3e 100644
>> --- a/drivers/ufs/host/ufs-qcom.h
>> +++ b/drivers/ufs/host/ufs-qcom.h
>> @@ -9,6 +9,7 @@
>>   #include <linux/reset.h>
>>   #include <soc/qcom/ice.h>
>>   #include <ufs/ufshcd.h>
>> +#include <linux/math.h>
> 
> Why is math.h needed in this header file now?
Removed it.
> 
> Regards,
> Bjorn
> 
>>
>>   #define MAX_UFS_QCOM_HOSTS	1
>>   #define MAX_U32                 (~(u32)0)
>> @@ -79,6 +80,15 @@ enum {
>>   	UFS_MEM_CQIS_VS		= 0x8,
>>   };
>>
>> +/* QCOM UFS host controller core clk frequencies */
>> +#define UNIPRO_CORE_CLK_FREQ_37_5_MHZ		38
>> +#define UNIPRO_CORE_CLK_FREQ_75_MHZ		75
>> +#define UNIPRO_CORE_CLK_FREQ_100_MHZ		100
>> +#define UNIPRO_CORE_CLK_FREQ_150_MHZ		150
>> +#define UNIPRO_CORE_CLK_FREQ_300_MHZ		300
>> +#define UNIPRO_CORE_CLK_FREQ_201_5_MHZ		202
>> +#define UNIPRO_CORE_CLK_FREQ_403_MHZ		403
>> +
>>   #define UFS_CNTLR_2_x_x_VEN_REGS_OFFSET(x)	(0x000 + x)
>>   #define UFS_CNTLR_3_x_x_VEN_REGS_OFFSET(x)	(0x400 + x)
>>
>> --
>> 2.17.1
>>

[PATCH V6 4/6] scsi: ufs: qcom: Align unipro clk attributes configuration as per HPG

[Nitin Rawat]

Currently CORE_CLK_1US_CYCLES, PA_VS_CORE_CLK_40NS_CYCLES are configured
in clk scaling post change ops.

Move this to clk scaling pre change ops to align completely with hardware
specification. This doesn't bring any functionality change.

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 | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index e3648e936498..d670fcc27ffb 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1410,12 +1410,6 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
 }

 static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
-{
-	/* nothing to do as of now */
-	return 0;
-}
-
-static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);

@@ -1425,6 +1419,11 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
 	return ufs_qcom_set_core_clk_ctrl(hba, true);
 }

+static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
+{
+	return 0;
+}
+
 static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
--
2.17.1

Re: [PATCH V6 4/6] scsi: ufs: qcom: Align unipro clk attributes configuration as per HPG

[Bjorn Andersson]

On Fri, Sep 01, 2023 at 05:13:34PM +0530, Nitin Rawat wrote:
> Currently CORE_CLK_1US_CYCLES, PA_VS_CORE_CLK_40NS_CYCLES are configured
> in clk scaling post change ops.
> 
> Move this to clk scaling pre change ops to align completely with hardware
> specification. This doesn't bring any functionality change.
> 

How can applying the clock scaling configuration, and "aligning with
hardware specification" not "bring any functionality change"?

If the code is called in a way where there is no difference between pre
and post callbacks, then state that - but it begs the question, why do
we have this "flexible" (complex) callback scheme if it doesn't matter.

Regards,
Bjorn

Re: [PATCH V6 4/6] scsi: ufs: qcom: Align unipro clk attributes configuration as per HPG

[Nitin Rawat]

On 9/1/2023 9:13 PM, Bjorn Andersson wrote:
> On Fri, Sep 01, 2023 at 05:13:34PM +0530, Nitin Rawat wrote:
>> Currently CORE_CLK_1US_CYCLES, PA_VS_CORE_CLK_40NS_CYCLES are configured
>> in clk scaling post change ops.
>>
>> Move this to clk scaling pre change ops to align completely with hardware
>> specification. This doesn't bring any functionality change.
>>
> 
> How can applying the clock scaling configuration, and "aligning with
> hardware specification" not "bring any functionality change"?
> 
> If the code is called in a way where there is no difference between pre
> and post callbacks, then state that - but it begs the question, why do
> we have this "flexible" (complex) callback scheme if it doesn't matter.
> 
> Regards,
> Bjorn

Hi Bjorn,

Here my intention is to align the sequence completely with HPG. 
Functionality w.r.t to clock scaling is not impacted here.
I'll update the commit text to capture more details in next patchset.

Thanks,
Nitin

[PATCH V6 5/6] scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers function.

[Nitin Rawat]

a)Configure SYS1CLK_1US_REG for clock scaleup pre ops.
b)Move ufs_qcom_cfg_timers from clk scaling post change ops
to clk scaling pre change ops to align with the HPG.
c)Introduce a new argument is_pre_scale_up for ufs_qcom_cfg_timers
to configure sys_1us timer for max freq as per HPG.

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 | 63 +++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 17 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index d670fcc27ffb..c251c98a74f0 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -532,7 +532,8 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
  * Return: zero for success and non-zero in case of a failure.
  */
 static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
-			       u32 hs, u32 rate, bool update_link_startup_timer)
+				 u32 hs, u32 rate, bool link_startup,
+				 bool is_pre_scale_up)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct ufs_clk_info *clki;
@@ -563,11 +564,16 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
 	/*
 	 * The Qunipro controller does not use following registers:
 	 * SYS1CLK_1US_REG, TX_SYMBOL_CLK_1US_REG, CLK_NS_REG &
-	 * UFS_REG_PA_LINK_STARTUP_TIMER
-	 * But UTP controller uses SYS1CLK_1US_REG register for Interrupt
-	 * Aggregation logic.
-	*/
-	if (ufs_qcom_cap_qunipro(host) && !ufshcd_is_intr_aggr_allowed(hba))
+	 * UFS_REG_PA_LINK_STARTUP_TIMER.
+	 * However UTP controller uses SYS1CLK_1US_REG register for Interrupt
+	 * Aggregation logic and Auto hibern8 logic.
+	 * It is mandatory to write SYS1CLK_1US_REG register on UFS host
+	 * controller V4.0.0 onwards.
+	 */
+	if (ufs_qcom_cap_qunipro(host) &&
+	    !(ufshcd_is_intr_aggr_allowed(hba) ||
+	    ufshcd_is_auto_hibern8_supported(hba) ||
+	    host->hw_ver.major >= 4))
 		return 0;

 	if (gear == 0) {
@@ -576,8 +582,14 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
 	}

 	list_for_each_entry(clki, &hba->clk_list_head, list) {
-		if (!strcmp(clki->name, "core_clk"))
-			core_clk_rate = clk_get_rate(clki->clk);
+		if (!strcmp(clki->name, "core_clk")) {
+			if (is_pre_scale_up)
+				core_clk_rate = clki->max_freq;
+			else
+				core_clk_rate = clk_get_rate(clki->clk);
+			break;
+		}
+
 	}

 	/* If frequency is smaller than 1MHz, set to 1MHz */
@@ -657,7 +669,7 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
 		mb();
 	}

-	if (update_link_startup_timer && host->hw_ver.major != 0x5) {
+	if (link_startup && host->hw_ver.major != 0x5) {
 		ufshcd_writel(hba, ((core_clk_rate / MSEC_PER_SEC) * 100),
 			      REG_UFS_CFG0);
 		/*
@@ -679,7 +691,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 	switch (status) {
 	case PRE_CHANGE:
 		if (ufs_qcom_cfg_timers(hba, UFS_PWM_G1, SLOWAUTO_MODE,
-					0, true)) {
+					0, true, false)) {
 			dev_err(hba->dev, "%s: ufs_qcom_cfg_timers() failed\n",
 				__func__);
 			return -EINVAL;
@@ -927,7 +939,7 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
 	case POST_CHANGE:
 		if (ufs_qcom_cfg_timers(hba, dev_req_params->gear_rx,
 					dev_req_params->pwr_rx,
-					dev_req_params->hs_rate, false)) {
+					dev_req_params->hs_rate, false, false)) {
 			dev_err(hba->dev, "%s: ufs_qcom_cfg_timers() failed\n",
 				__func__);
 			/*
@@ -1412,10 +1424,22 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
 static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	struct ufs_pa_layer_attr *attr = &host->dev_req_params;
+	int ret;

 	if (!ufs_qcom_cap_qunipro(host))
 		return 0;

+	if (attr) {
+		ret = ufs_qcom_cfg_timers(hba, attr->gear_rx,
+					attr->pwr_rx, attr->hs_rate,
+					false, true);
+		if (ret) {
+			dev_err(hba->dev, "%s ufs cfg timer failed\n",
+						__func__);
+			return ret;
+		}
+	}
 	return ufs_qcom_set_core_clk_ctrl(hba, true);
 }

@@ -1452,10 +1476,21 @@ static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
 static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	struct ufs_pa_layer_attr *attr = &host->dev_req_params;
+	int ret;

 	if (!ufs_qcom_cap_qunipro(host))
 		return 0;

+	if (attr) {
+		ret = ufs_qcom_cfg_timers(hba, attr->gear_rx, attr->pwr_rx,
+					attr->hs_rate, false, false);
+		if (ret) {
+			dev_err(hba->dev, "%s ufs cfg timer failed\n",
+					__func__);
+			return ret;
+		}
+	}
 	return ufs_qcom_set_core_clk_ctrl(hba, false);
 }

@@ -1463,7 +1498,6 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
 		bool scale_up, enum ufs_notify_change_status status)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	struct ufs_pa_layer_attr *dev_req_params = &host->dev_req_params;
 	int err = 0;

 	/* check the host controller state before sending hibern8 cmd */
@@ -1493,11 +1527,6 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
 			return err;
 		}

-		ufs_qcom_cfg_timers(hba,
-				    dev_req_params->gear_rx,
-				    dev_req_params->pwr_rx,
-				    dev_req_params->hs_rate,
-				    false);
 		ufs_qcom_icc_update_bw(host);
 		ufshcd_uic_hibern8_exit(hba);
 	}
--
2.17.1

Re: [PATCH V6 5/6] scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers function.

[Bjorn Andersson]

On Fri, Sep 01, 2023 at 05:13:35PM +0530, Nitin Rawat wrote:
> a)Configure SYS1CLK_1US_REG for clock scaleup pre ops.
> b)Move ufs_qcom_cfg_timers from clk scaling post change ops
> to clk scaling pre change ops to align with the HPG.
> c)Introduce a new argument is_pre_scale_up for ufs_qcom_cfg_timers
> to configure sys_1us timer for max freq as per HPG.

You forgot to describe the problem you're trying to solve. This is just
a summary of the changes done in the code.

> 
> 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 | 63 +++++++++++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index d670fcc27ffb..c251c98a74f0 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -532,7 +532,8 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
>   * Return: zero for success and non-zero in case of a failure.
>   */
>  static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
> -			       u32 hs, u32 rate, bool update_link_startup_timer)

Again, it doesn't seem like this variable name change has any functional
impact, so please don't change it.

PS. You're very welcome to send separate cleanup patches just making the
code easier to read, but please do so separately.


You're changing the prototype of the function, but you're not updating
the kernel-doc above, please correct this.

> +				 u32 hs, u32 rate, bool link_startup,
> +				 bool is_pre_scale_up)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>  	struct ufs_clk_info *clki;
> @@ -563,11 +564,16 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
>  	/*
>  	 * The Qunipro controller does not use following registers:
>  	 * SYS1CLK_1US_REG, TX_SYMBOL_CLK_1US_REG, CLK_NS_REG &
> -	 * UFS_REG_PA_LINK_STARTUP_TIMER
> -	 * But UTP controller uses SYS1CLK_1US_REG register for Interrupt
> -	 * Aggregation logic.
> -	*/
> -	if (ufs_qcom_cap_qunipro(host) && !ufshcd_is_intr_aggr_allowed(hba))
> +	 * UFS_REG_PA_LINK_STARTUP_TIMER.
> +	 * However UTP controller uses SYS1CLK_1US_REG register for Interrupt
> +	 * Aggregation logic and Auto hibern8 logic.
> +	 * It is mandatory to write SYS1CLK_1US_REG register on UFS host
> +	 * controller V4.0.0 onwards.
> +	 */
> +	if (ufs_qcom_cap_qunipro(host) &&
> +	    !(ufshcd_is_intr_aggr_allowed(hba) ||
> +	    ufshcd_is_auto_hibern8_supported(hba) ||

This line is indented to the start of the condition, but it's actually
part of the !() expression starting on the line above. Please indent it
to align with the ufshcd_is_intr_aggr_allowed() to make this obvious.

> +	    host->hw_ver.major >= 4))

I think that this condition is added so that the return will only happen
on hw_ver < 4. But if it's "significant", why is it hidden in the inner
expression and not part of the outer expression.

  cap && !(aggr || h8 || ver >= 4)

vs

  ver < 4 && cap && !(aggr || h8)

The latter prunes the option space much faster than the latter.


Second, there are two changes to this condition in this one patch; one
affects hw_ver >= 4, and the other affects any previous target
supporting hibernation.

Please separate these changes, to facilitate debugging any effects of
the hibernation change.

Regards,
Bjorn

Re: [PATCH V6 5/6] scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers function.

[Nitin Rawat]

On 9/1/2023 9:36 PM, Bjorn Andersson wrote:
> On Fri, Sep 01, 2023 at 05:13:35PM +0530, Nitin Rawat wrote:
>> a)Configure SYS1CLK_1US_REG for clock scaleup pre ops.
>> b)Move ufs_qcom_cfg_timers from clk scaling post change ops
>> to clk scaling pre change ops to align with the HPG.
>> c)Introduce a new argument is_pre_scale_up for ufs_qcom_cfg_timers
>> to configure sys_1us timer for max freq as per HPG.
> 
> You forgot to describe the problem you're trying to solve. This is just
> a summary of the changes done in the code.
Sure..Will update the commit text to describe the problem we are solving
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 | 63 +++++++++++++++++++++++++++----------
>>   1 file changed, 46 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index d670fcc27ffb..c251c98a74f0 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -532,7 +532,8 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
>>    * Return: zero for success and non-zero in case of a failure.
>>    */
>>   static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
>> -			       u32 hs, u32 rate, bool update_link_startup_timer)
> 
> Again, it doesn't seem like this variable name change has any functional
> impact, so please don't change it.
Sure..Will take care of this in next patchset.

> 
> PS. You're very welcome to send separate cleanup patches just making the
> code easier to read, but please do so separately.
Sure.

> 
> 
> You're changing the prototype of the function, but you're not updating
> the kernel-doc above, please correct this.
I'll update the kernel doc in next patchset.

> 
>> +				 u32 hs, u32 rate, bool link_startup,
>> +				 bool is_pre_scale_up)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>   	struct ufs_clk_info *clki;
>> @@ -563,11 +564,16 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
>>   	/*
>>   	 * The Qunipro controller does not use following registers:
>>   	 * SYS1CLK_1US_REG, TX_SYMBOL_CLK_1US_REG, CLK_NS_REG &
>> -	 * UFS_REG_PA_LINK_STARTUP_TIMER
>> -	 * But UTP controller uses SYS1CLK_1US_REG register for Interrupt
>> -	 * Aggregation logic.
>> -	*/
>> -	if (ufs_qcom_cap_qunipro(host) && !ufshcd_is_intr_aggr_allowed(hba))
>> +	 * UFS_REG_PA_LINK_STARTUP_TIMER.
>> +	 * However UTP controller uses SYS1CLK_1US_REG register for Interrupt
>> +	 * Aggregation logic and Auto hibern8 logic.
>> +	 * It is mandatory to write SYS1CLK_1US_REG register on UFS host
>> +	 * controller V4.0.0 onwards.
>> +	 */
>> +	if (ufs_qcom_cap_qunipro(host) &&
>> +	    !(ufshcd_is_intr_aggr_allowed(hba) ||
>> +	    ufshcd_is_auto_hibern8_supported(hba) ||
> 
> This line is indented to the start of the condition, but it's actually
> part of the !() expression starting on the line above. Please indent it
> to align with the ufshcd_is_intr_aggr_allowed() to make this obvious.
- Sure..Will correct the code identation.


> 
>> +	    host->hw_ver.major >= 4))
> 
> I think that this condition is added so that the return will only happen
> on hw_ver < 4. But if it's "significant", why is it hidden in the inner
> expression and not part of the outer expression.
> 
>    cap && !(aggr || h8 || ver >= 4)
> 
> vs
> 
>    ver < 4 && cap && !(aggr || h8)
> 
> The latter prunes the option space much faster than the latter.
Sure..Will take care of this in next patchset.

> 
> 
> Second, there are two changes to this condition in this one patch; one
> affects hw_ver >= 4, and the other affects any previous target
> supporting hibernation.
> 
> Please separate these changes, to facilitate debugging any effects of
> the hibernation change.
Sure...Would send separate patch to include hibernation change.


> 
> Regards,
> Bjorn

Thanks,
Nitin

[PATCH V6 6/6] scsi: ufs: qcom: Configure clk HW division based on scaling conditions.

[Nitin Rawat]

a) Enable internal HW division of unipro core_clk for scale up condition.
b) Clear internal HW division of unipro core_clk for scale down condition.

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 | 31 +++++++++++++++++--------------
 drivers/ufs/host/ufs-qcom.h |  2 +-
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index c251c98a74f0..2ddda9356abc 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1389,18 +1389,21 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
 		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;
+	/* Enable CORE_CLK_DIV_EN for scale up condition */
+	if (is_max_freq)
+		core_clk_ctrl_reg |= CORE_CLK_DIV_EN_BIT;

 	ret = ufshcd_dme_set(hba,
 			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
 			    core_clk_ctrl_reg);
+	if (ret)
+		return ret;
 	/*
 	 * UFS host controller V4.0.0 onwards needs to program
 	 * PA_VS_CORE_CLK_40NS_CYCLES attribute per programmed
 	 * frequency of unipro core clk of UFS host controller.
 	 */
-	if (!ret && (host->hw_ver.major >= 4)) {
+	if (host->hw_ver.major >= 4) {
 		if (cycles_in_40ns > PA_VS_CORE_CLK_40NS_CYCLES_MASK)
 			return -EINVAL;

@@ -1451,26 +1454,26 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
 static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	int err;
-	u32 core_clk_ctrl_reg;
+	int ret;
+	u32 reg;

 	if (!ufs_qcom_cap_qunipro(host))
 		return 0;

-	err = ufshcd_dme_get(hba,
+	ret = ufshcd_dme_get(hba,
 			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
-			    &core_clk_ctrl_reg);
+			    &reg);
+	if (ret)
+		return ret;

 	/* make sure CORE_CLK_DIV_EN is cleared */
-	if (!err &&
-	    (core_clk_ctrl_reg & DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT)) {
-		core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT;
-		err = ufshcd_dme_set(hba,
+	if (reg & CORE_CLK_DIV_EN_BIT) {
+		reg &= ~CORE_CLK_DIV_EN_BIT;
+		ret = ufshcd_dme_set(hba,
 				    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
-				    core_clk_ctrl_reg);
+				    reg);
 	}
-
-	return err;
+	return ret;
 }

 static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index bc176ef58e3e..bf0c370c79c7 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -141,7 +141,7 @@ enum {
 /* bit and mask definitions for DME_VS_CORE_CLK_CTRL attribute */
 #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)
+#define CORE_CLK_DIV_EN_BIT				BIT(8)
 #define PA_VS_CORE_CLK_40NS_CYCLES			0x9007
 #define PA_VS_CORE_CLK_40NS_CYCLES_MASK			GENMASK(6, 0)

--
2.17.1

Re: [PATCH V6 6/6] scsi: ufs: qcom: Configure clk HW division based on scaling conditions.

[Bjorn Andersson]

On Fri, Sep 01, 2023 at 05:13:36PM +0530, Nitin Rawat wrote:
> a) Enable internal HW division of unipro core_clk for scale up condition.
> b) Clear internal HW division of unipro core_clk for scale down condition.
> 

This commit message isn't good.

> 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 | 31 +++++++++++++++++--------------
>  drivers/ufs/host/ufs-qcom.h |  2 +-
>  2 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index c251c98a74f0..2ddda9356abc 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1389,18 +1389,21 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
>  		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;
> +	/* Enable CORE_CLK_DIV_EN for scale up condition */
> +	if (is_max_freq)
> +		core_clk_ctrl_reg |= CORE_CLK_DIV_EN_BIT;
> 
>  	ret = ufshcd_dme_set(hba,
>  			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
>  			    core_clk_ctrl_reg);
> +	if (ret)
> +		return ret;

Nice, but don't mix logical and stylistic changes in the same patch.

>  	/*
>  	 * UFS host controller V4.0.0 onwards needs to program
>  	 * PA_VS_CORE_CLK_40NS_CYCLES attribute per programmed
>  	 * frequency of unipro core clk of UFS host controller.
>  	 */
> -	if (!ret && (host->hw_ver.major >= 4)) {
> +	if (host->hw_ver.major >= 4) {
>  		if (cycles_in_40ns > PA_VS_CORE_CLK_40NS_CYCLES_MASK)
>  			return -EINVAL;
> 
> @@ -1451,26 +1454,26 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
>  static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> -	int err;
> -	u32 core_clk_ctrl_reg;
> +	int ret;
> +	u32 reg;

This just obfuscates the logical changes you're making.


Frankly, I've been staring at this for several minutes now. The commit
message says there is a change here (but fails to describe what problem
is solves or what the change is)...but I can't find the change!

Regards,
Bjorn

> 
>  	if (!ufs_qcom_cap_qunipro(host))
>  		return 0;
> 
> -	err = ufshcd_dme_get(hba,
> +	ret = ufshcd_dme_get(hba,
>  			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
> -			    &core_clk_ctrl_reg);
> +			    &reg);
> +	if (ret)
> +		return ret;
> 
>  	/* make sure CORE_CLK_DIV_EN is cleared */
> -	if (!err &&
> -	    (core_clk_ctrl_reg & DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT)) {
> -		core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT;
> -		err = ufshcd_dme_set(hba,
> +	if (reg & CORE_CLK_DIV_EN_BIT) {
> +		reg &= ~CORE_CLK_DIV_EN_BIT;
> +		ret = ufshcd_dme_set(hba,
>  				    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
> -				    core_clk_ctrl_reg);
> +				    reg);
>  	}
> -
> -	return err;
> +	return ret;
>  }
> 
>  static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index bc176ef58e3e..bf0c370c79c7 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -141,7 +141,7 @@ enum {
>  /* bit and mask definitions for DME_VS_CORE_CLK_CTRL attribute */
>  #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)
> +#define CORE_CLK_DIV_EN_BIT				BIT(8)
>  #define PA_VS_CORE_CLK_40NS_CYCLES			0x9007
>  #define PA_VS_CORE_CLK_40NS_CYCLES_MASK			GENMASK(6, 0)
> 
> --
> 2.17.1
> 

Re: [PATCH V6 0/6] scsi: ufs: qcom: Align programming sequence as per HW spec

[Konrad Dybcio]

On 1.09.2023 13:43, Nitin Rawat wrote:
> This patch aligns programming sequence as per Qualcomm UFS
> hardware specification.
> 
> changes from v5:
> - Addressed Mani comment to FIELD_PREP and FIELD_FIT.
> - Optimised ufs_qcom_set_core_clk_ctrl API.
> - Updated commit text for few patches to capture more details.
> 
Any reason I received this twice?

Konrad

Re: [PATCH V6 0/6] scsi: ufs: qcom: Align programming sequence as per HW spec

[Andrew Halaney]

On Fri, Sep 01, 2023 at 05:13:30PM +0530, Nitin Rawat wrote:
> This patch aligns programming sequence as per Qualcomm UFS
> hardware specification.

reading this series, it is difficult for me to understand as a user of
the driver if this should have any noticeable effect.

Some of the patches mention that there is no functional change, some
only say align with the HPG but change programming sequence, frequency,
etc if I understand correctly on a quick glance.

I think being a bit verbose in some of the patches with respect to
explaining the effect of the patch (or lack of a noticeable effect)
would be a beneficial improvement to this series if there's another
version.

I agree that aligning with the HPG instead of doing some undefined
sequence is a good idea, I'm just reading some of the changes and
thinking "I have no idea if this is going to fix something (no Fixes:
tag but it almost sounds like one), will this improve something, or will this
just change the programming sequence to a known and recommended
sequence?".

Thanks for the patches,
Andrew

Re: [PATCH V6 0/6] scsi: ufs: qcom: Align programming sequence as per HW spec

[Bjorn Andersson]

On Fri, Sep 01, 2023 at 09:56:05AM -0500, Andrew Halaney wrote:
> On Fri, Sep 01, 2023 at 05:13:30PM +0530, Nitin Rawat wrote:
> > This patch aligns programming sequence as per Qualcomm UFS
> > hardware specification.
> 
> reading this series, it is difficult for me to understand as a user of
> the driver if this should have any noticeable effect.
> 
> Some of the patches mention that there is no functional change, some
> only say align with the HPG but change programming sequence, frequency,
> etc if I understand correctly on a quick glance.
> 
> I think being a bit verbose in some of the patches with respect to
> explaining the effect of the patch (or lack of a noticeable effect)
> would be a beneficial improvement to this series if there's another
> version.
> 
> I agree that aligning with the HPG instead of doing some undefined
> sequence is a good idea, I'm just reading some of the changes and
> thinking "I have no idea if this is going to fix something (no Fixes:
> tag but it almost sounds like one), will this improve something, or will this
> just change the programming sequence to a known and recommended
> sequence?".
> 

Very valid feedback, Andrew.

Correct or not, there are a fair amount of users out there who runs the
current implementation. Changes to that should be described in a way
that doesn't depend on inside-knowledge.

Regards,
Bjorn

[PATCH V6 0/6] scsi: ufs: qcom: Align programming sequence as per HW spec

[Nitin Rawat]

This patch aligns programming sequence as per Qualcomm UFS
hardware specification.

changes from v5:
- Addressed Mani comment to FIELD_PREP and FIELD_FIT.
- Optimised ufs_qcom_set_core_clk_ctrl API.
- Updated commit text for few patches to capture more details.

Changes from v4:
- Addressed bjorn comment to split single patch to multiple patches.

Changes from v3:
-Addressed bjorn comment to update commit msg to capture change details.

Changes from v2:
- Addressed bao comment, removed duplicate clock timer cfg API call

Changes from v1:
- Addressed bao comment, removed wrapper function
- Tab alignment

Nitin Rawat (6):
  scsi: ufs: qcom: Align mask for core_clk_1us_cycles
  scsi: ufs: qcom: Configure PA_VS_CORE_CLK_40NS_CYCLES
  scsi: ufs: qcom: Add multiple frequency support for unipro clk
    attributes
  scsi: ufs: qcom: Align unipro clk attributes configuration as per HPG
  scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers function.
  scsi: ufs: qcom: Configure clk HW division based on scaling
    conditions.

 drivers/ufs/host/ufs-qcom.c | 240 ++++++++++++++++++++++++++----------
 drivers/ufs/host/ufs-qcom.h |  17 ++-
 2 files changed, 193 insertions(+), 64 deletions(-)

--
2.17.1

[PATCH V6 4/6] scsi: ufs: qcom: Align unipro clk attributes configuration as per HPG

[Nitin Rawat]

Currently CORE_CLK_1US_CYCLES, PA_VS_CORE_CLK_40NS_CYCLES are configured
in clk scaling post change ops.

Move this to clk scaling pre change ops to align completely with hardware
specification. This doesn't bring any functionality change.

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 | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index e3648e936498..d670fcc27ffb 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1410,12 +1410,6 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
 }

 static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
-{
-	/* nothing to do as of now */
-	return 0;
-}
-
-static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);

@@ -1425,6 +1419,11 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
 	return ufs_qcom_set_core_clk_ctrl(hba, true);
 }

+static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
+{
+	return 0;
+}
+
 static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
--
2.17.1