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

[PATCH V5 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 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: Update offset for core_clk_1us_cycles
  scsi: ufs: qcom: Configure PA_VS_CORE_CLK_40NS_CYCLES for Unipro core
    clk
  scsi: ufs: qcom: Add multiple frequency support for unipro clk
    attributes
  scsi: ufs: qcom: Align unipro clk attributes as per Hardware
    specification
  scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers function.
  scsi: ufs: qcom: Handle unipro clk HW division based on scaling
    conditions.

 drivers/ufs/host/ufs-qcom.c | 239 +++++++++++++++++++++++++++---------
 drivers/ufs/host/ufs-qcom.h |  15 ++-
 2 files changed, 197 insertions(+), 57 deletions(-)

--
2.17.1

[PATCH V5 1/6] scsi: ufs: qcom: Update offset for core_clk_1us_cycles

[Nitin Rawat]

This Patch updates offset for core_clk_1us_cycles in DME_VS_CORE_CLK_CTRL
register. Offset for core_clk_1us_cycles is changed from Qualcomm UFS
Controller V4.0.0 onwards.

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

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index f88febb23123..1108b0cd43b3 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1297,12 +1297,21 @@ 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 clk_1us_cycles)
 {
-	int err;
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	u32 mask = DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK;
 	u32 core_clk_ctrl_reg;
+	u32 offset = 0;
+	int err;
+
+	/* Bit mask and offset changed on UFS host controller V4.0.0 onwards */
+	if (host->hw_ver.major >= 4) {
+		mask = MAX_CORE_CLK_1US_CYCLES_MASK_V4;
+		offset = MAX_CORE_CLK_1US_CYCLES_OFFSET_V4;
+	}

-	if (clk_cycles > DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK)
+	if (clk_1us_cycles > mask)
 		return -EINVAL;

 	err = ufshcd_dme_get(hba,
@@ -1311,8 +1320,8 @@ static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
 	if (err)
 		return err;

-	core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK;
-	core_clk_ctrl_reg |= clk_cycles;
+	core_clk_ctrl_reg &= ~(mask << offset);
+	core_clk_ctrl_reg |= clk_1us_cycles << offset;

 	/* 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..a829296e11bb 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -129,6 +129,8 @@ 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 MAX_CORE_CLK_1US_CYCLES_MASK_V4		0xFFF
+#define MAX_CORE_CLK_1US_CYCLES_OFFSET_V4	0x10
 #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

--
2.17.1

Re: [PATCH V5 1/6] scsi: ufs: qcom: Update offset for core_clk_1us_cycles

[Manivannan Sadhasivam]

On Wed, Aug 23, 2023 at 09:14:08PM +0530, Nitin Rawat wrote:
> This Patch updates offset for core_clk_1us_cycles in DME_VS_CORE_CLK_CTRL

Please do not use "This patch" in commit message. Just reword it in imperative
form.

> register. Offset for core_clk_1us_cycles is changed from Qualcomm UFS
> Controller V4.0.0 onwards.
> 
> 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 | 19 ++++++++++++++-----
>  drivers/ufs/host/ufs-qcom.h |  2 ++
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index f88febb23123..1108b0cd43b3 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1297,12 +1297,21 @@ 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 clk_1us_cycles)

How about "cycles_in_1us", since this value specifies "Number of clk cycles in
1us"?

>  {
> -	int err;
> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> +	u32 mask = DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK;
>  	u32 core_clk_ctrl_reg;
> +	u32 offset = 0;
> +	int err;
> +
> +	/* Bit mask and offset changed on UFS host controller V4.0.0 onwards */

This is not offset value, but rather shift. Still, if you use bitfield macros
as I suggested below, you could get rid of this variable.

> +	if (host->hw_ver.major >= 4) {
> +		mask = MAX_CORE_CLK_1US_CYCLES_MASK_V4;
> +		offset = MAX_CORE_CLK_1US_CYCLES_OFFSET_V4;
> +	}
> 
> -	if (clk_cycles > DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK)
> +	if (clk_1us_cycles > mask)
>  		return -EINVAL;

	if (!FIELD_FIT(mask, cycles_in_1us))
		return -ERANGE;

> 
>  	err = ufshcd_dme_get(hba,
> @@ -1311,8 +1320,8 @@ static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
>  	if (err)
>  		return err;
> 
> -	core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK;
> -	core_clk_ctrl_reg |= clk_cycles;
> +	core_clk_ctrl_reg &= ~(mask << offset);
> +	core_clk_ctrl_reg |= clk_1us_cycles << offset;
> 

	core_clk_ctrl_reg &= ~mask;
	core_clk_ctrl_reg |= FIELD_PREP(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..a829296e11bb 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -129,6 +129,8 @@ 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 MAX_CORE_CLK_1US_CYCLES_MASK_V4		0xFFF

#define MAX_CORE_CLK_1US_CYCLES_MASK_V4				GENMASK(27, 16)
#define DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK	GENMASK(7, 0)

- Mani

> +#define MAX_CORE_CLK_1US_CYCLES_OFFSET_V4	0x10
>  #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
> 
> --
> 2.17.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH V5 1/6] scsi: ufs: qcom: Update offset for core_clk_1us_cycles

[Nitin Rawat]

On 8/28/2023 1:08 PM, Manivannan Sadhasivam wrote:
> On Wed, Aug 23, 2023 at 09:14:08PM +0530, Nitin Rawat wrote:
>> This Patch updates offset for core_clk_1us_cycles in DME_VS_CORE_CLK_CTRL
> 
> Please do not use "This patch" in commit message. Just reword it in imperative
> form.

Thanks Mani for the review. We Will address the commit text in next 
patchset.

-Nitin


> 
>> register. Offset for core_clk_1us_cycles is changed from Qualcomm UFS
>> Controller V4.0.0 onwards.
>>
>> 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 | 19 ++++++++++++++-----
>>   drivers/ufs/host/ufs-qcom.h |  2 ++
>>   2 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index f88febb23123..1108b0cd43b3 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -1297,12 +1297,21 @@ 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 clk_1us_cycles)
> 
> How about "cycles_in_1us", since this value specifies "Number of clk cycles in
> 1us"?
I Will take care of this in next patchset

-Nitin

> 
>>   {
>> -	int err;
>> +	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>> +	u32 mask = DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK;
>>   	u32 core_clk_ctrl_reg;
>> +	u32 offset = 0;
>> +	int err;
>> +
>> +	/* Bit mask and offset changed on UFS host controller V4.0.0 onwards */
> 
> This is not offset value, but rather shift. Still, if you use bitfield macros
> as I suggested below, you could get rid of this variable.

I Will take care of this in next patchset

-Nitin

> 
>> +	if (host->hw_ver.major >= 4) {
>> +		mask = MAX_CORE_CLK_1US_CYCLES_MASK_V4;
>> +		offset = MAX_CORE_CLK_1US_CYCLES_OFFSET_V4;
>> +	}
>>
>> -	if (clk_cycles > DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK)
>> +	if (clk_1us_cycles > mask)
>>   		return -EINVAL;
> 
> 	if (!FIELD_FIT(mask, cycles_in_1us))
> 		return -ERANGE;
> 
>>
>>   	err = ufshcd_dme_get(hba,
>> @@ -1311,8 +1320,8 @@ static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
>>   	if (err)
>>   		return err;
>>
>> -	core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK;
>> -	core_clk_ctrl_reg |= clk_cycles;
>> +	core_clk_ctrl_reg &= ~(mask << offset);
>> +	core_clk_ctrl_reg |= clk_1us_cycles << offset;
>>
> 
> 	core_clk_ctrl_reg &= ~mask;
> 	core_clk_ctrl_reg |= FIELD_PREP(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..a829296e11bb 100644
>> --- a/drivers/ufs/host/ufs-qcom.h
>> +++ b/drivers/ufs/host/ufs-qcom.h
>> @@ -129,6 +129,8 @@ 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 MAX_CORE_CLK_1US_CYCLES_MASK_V4		0xFFF
> 
> #define MAX_CORE_CLK_1US_CYCLES_MASK_V4				GENMASK(27, 16)
> #define DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK	GENMASK(7, 0)
> 
> - Mani


I will update it. Thanks
-Nitin

> 
>> +#define MAX_CORE_CLK_1US_CYCLES_OFFSET_V4	0x10
>>   #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
>>
>> --
>> 2.17.1
>>
> 

[PATCH V5 2/6] scsi: ufs: qcom: Configure PA_VS_CORE_CLK_40NS_CYCLES for Unipro core clk

[Nitin Rawat]

For Qualcomm UFS controller V4.0 and above PA_VS_CORE_CLK_40NS_CYCLES
attribute needs to be programmed with frequency of unipro core clk.
Hence Configure PA_VS_CORE_CLK_40NS_CYCLES attribute for Unipro core clk.

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

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 1108b0cd43b3..abc0e7f7d1b0 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,7 @@ 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
@@ -1296,12 +1296,13 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
 	phy_exit(host->generic_phy);
 }

-static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
-						       u32 clk_1us_cycles)
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
+					u32 clk_1us_cycles,
+					u32 clk_40ns_cycles)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	u32 mask = DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK;
-	u32 core_clk_ctrl_reg;
+	u32 core_clk_ctrl_reg, reg;
 	u32 offset = 0;
 	int err;

@@ -1326,9 +1327,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 (clk_40ns_cycles > 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 |= clk_40ns_cycles;
+
+		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)
@@ -1345,7 +1370,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)
@@ -1381,7 +1406,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 a829296e11bb..325f08aca260 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -133,6 +133,8 @@ enum {
 #define MAX_CORE_CLK_1US_CYCLES_OFFSET_V4	0x10
 #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 PA_VS_CORE_CLK_40NS_CYCLES	0x9007
+#define PA_VS_CORE_CLK_40NS_CYCLES_MASK	0x3F

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

Re: [PATCH V5 2/6] scsi: ufs: qcom: Configure PA_VS_CORE_CLK_40NS_CYCLES for Unipro core clk

[Manivannan Sadhasivam]

On Wed, Aug 23, 2023 at 09:14:09PM +0530, Nitin Rawat wrote:
> For Qualcomm UFS controller V4.0 and above PA_VS_CORE_CLK_40NS_CYCLES
> attribute needs to be programmed with frequency of unipro core clk.
> Hence Configure PA_VS_CORE_CLK_40NS_CYCLES attribute for Unipro core clk.
> 

Same comment applies as patch 1.

- Mani

> 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 | 45 ++++++++++++++++++++++++++++---------
>  drivers/ufs/host/ufs-qcom.h |  2 ++
>  2 files changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 1108b0cd43b3..abc0e7f7d1b0 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,7 @@ 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
> @@ -1296,12 +1296,13 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
>  	phy_exit(host->generic_phy);
>  }
> 
> -static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
> -						       u32 clk_1us_cycles)
> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
> +					u32 clk_1us_cycles,
> +					u32 clk_40ns_cycles)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>  	u32 mask = DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK;
> -	u32 core_clk_ctrl_reg;
> +	u32 core_clk_ctrl_reg, reg;
>  	u32 offset = 0;
>  	int err;
> 
> @@ -1326,9 +1327,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 (clk_40ns_cycles > 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 |= clk_40ns_cycles;
> +
> +		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)
> @@ -1345,7 +1370,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)
> @@ -1381,7 +1406,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 a829296e11bb..325f08aca260 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -133,6 +133,8 @@ enum {
>  #define MAX_CORE_CLK_1US_CYCLES_OFFSET_V4	0x10
>  #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 PA_VS_CORE_CLK_40NS_CYCLES	0x9007
> +#define PA_VS_CORE_CLK_40NS_CYCLES_MASK	0x3F
> 
>  static inline void
>  ufs_qcom_get_controller_revision(struct ufs_hba *hba,
> --
> 2.17.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH V5 2/6] scsi: ufs: qcom: Configure PA_VS_CORE_CLK_40NS_CYCLES for Unipro core clk

[Nitin Rawat]

On 8/28/2023 1:10 PM, Manivannan Sadhasivam wrote:
> On Wed, Aug 23, 2023 at 09:14:09PM +0530, Nitin Rawat wrote:
>> For Qualcomm UFS controller V4.0 and above PA_VS_CORE_CLK_40NS_CYCLES
>> attribute needs to be programmed with frequency of unipro core clk.
>> Hence Configure PA_VS_CORE_CLK_40NS_CYCLES attribute for Unipro core clk.
>>
> 
> Same comment applies as patch 1.
> 
> - Mani

Sure...Will take care of this in next patchset. Thanks

-Nitin


> 
>> 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 | 45 ++++++++++++++++++++++++++++---------
>>   drivers/ufs/host/ufs-qcom.h |  2 ++
>>   2 files changed, 37 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 1108b0cd43b3..abc0e7f7d1b0 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,7 @@ 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
>> @@ -1296,12 +1296,13 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
>>   	phy_exit(host->generic_phy);
>>   }
>>
>> -static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
>> -						       u32 clk_1us_cycles)
>> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
>> +					u32 clk_1us_cycles,
>> +					u32 clk_40ns_cycles)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>   	u32 mask = DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK;
>> -	u32 core_clk_ctrl_reg;
>> +	u32 core_clk_ctrl_reg, reg;
>>   	u32 offset = 0;
>>   	int err;
>>
>> @@ -1326,9 +1327,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 (clk_40ns_cycles > 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 |= clk_40ns_cycles;
>> +
>> +		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)
>> @@ -1345,7 +1370,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)
>> @@ -1381,7 +1406,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 a829296e11bb..325f08aca260 100644
>> --- a/drivers/ufs/host/ufs-qcom.h
>> +++ b/drivers/ufs/host/ufs-qcom.h
>> @@ -133,6 +133,8 @@ enum {
>>   #define MAX_CORE_CLK_1US_CYCLES_OFFSET_V4	0x10
>>   #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 PA_VS_CORE_CLK_40NS_CYCLES	0x9007
>> +#define PA_VS_CORE_CLK_40NS_CYCLES_MASK	0x3F
>>
>>   static inline void
>>   ufs_qcom_get_controller_revision(struct ufs_hba *hba,
>> --
>> 2.17.1
>>
> 

[PATCH V5 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.

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

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index abc0e7f7d1b0..8162b19191a9 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -671,6 +671,45 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
 	return 0;
 }

+static int ufs_qcom_cfg_core_clk_ctrl(struct ufs_hba *hba)
+{
+	struct list_head *head = &hba->clk_list_head;
+	struct ufs_clk_info *clki;
+	u32 max_freq = 0;
+	int err;
+
+	list_for_each_entry(clki, head, list) {
+		if (!IS_ERR_OR_NULL(clki->clk) &&
+			!strcmp(clki->name, "core_clk_unipro")) {
+			max_freq = clki->max_freq;
+			break;
+		}
+	}
+
+	switch (max_freq) {
+	case MHZ_403:
+		err = ufs_qcom_set_core_clk_ctrl(hba, 403, 16);
+		break;
+	case MHZ_300:
+		err = ufs_qcom_set_core_clk_ctrl(hba, 300, 12);
+		break;
+	case MHZ_201_5:
+		err = ufs_qcom_set_core_clk_ctrl(hba, 202, 8);
+		break;
+	case MHZ_150:
+		err = ufs_qcom_set_core_clk_ctrl(hba, 150, 6);
+		break;
+	case MHZ_100:
+		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4);
+		break;
+	default:
+		dev_err(hba->dev, "unipro max_freq=%u entry missing\n", max_freq);
+		err = -EINVAL;
+		break;
+	}
+
+	return err;
+}
 static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 					enum ufs_notify_change_status status)
 {
@@ -686,12 +725,15 @@ 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_cfg_core_clk_ctrl(hba);
+			if (err) {
+				dev_err(hba->dev,
+					"%s cfg core clk ctrl failed\n",
+					__func__);
+				return err;
+			}
+		}

 		/*
 		 * Some UFS devices (and may be host) have issues if LCC is
@@ -1369,8 +1411,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_cfg_core_clk_ctrl(hba);
 }

 static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
@@ -1401,12 +1442,39 @@ 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 list_head *head = &hba->clk_list_head;
+	struct ufs_clk_info *clki;
+	u32 curr_freq = 0;
+	int err;

 	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);
+
+	list_for_each_entry(clki, head, list) {
+		if (!IS_ERR_OR_NULL(clki->clk) &&
+			!strcmp(clki->name, "core_clk_unipro")) {
+			curr_freq = clk_get_rate(clki->clk);
+			break;
+		}
+	}
+	switch (curr_freq) {
+	case MHZ_37_5:
+		err = ufs_qcom_set_core_clk_ctrl(hba, 38, 2);
+		break;
+	case MHZ_75:
+		err = ufs_qcom_set_core_clk_ctrl(hba, 75, 3);
+		break;
+	case MHZ_100:
+		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4);
+		break;
+	default:
+		err = -EINVAL;
+		dev_err(hba->dev, "unipro curr_freq=%u entry missing\n", curr_freq);
+		break;
+	}
+
+	return err;
 }

 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 325f08aca260..56550fd36c4e 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -79,6 +79,15 @@ enum {
 	UFS_MEM_CQIS_VS		= 0x8,
 };

+/* QCOM UFS host controller core clk frequencies */
+#define MHZ_37_5	37500000
+#define MHZ_50		50000000
+#define MHZ_75		75000000
+#define MHZ_100		100000000
+#define MHZ_150		150000000
+#define MHZ_300		300000000
+#define MHZ_201_5	201500000
+#define MHZ_403		403000000
 #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 V5 3/6] scsi: ufs: qcom: Add multiple frequency support for unipro clk attributes

[Manivannan Sadhasivam]

On Wed, Aug 23, 2023 at 09:14:10PM +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.
> 
> 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 | 88 ++++++++++++++++++++++++++++++++-----
>  drivers/ufs/host/ufs-qcom.h |  9 ++++
>  2 files changed, 87 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index abc0e7f7d1b0..8162b19191a9 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -671,6 +671,45 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
>  	return 0;
>  }
> 
> +static int ufs_qcom_cfg_core_clk_ctrl(struct ufs_hba *hba)
> +{
> +	struct list_head *head = &hba->clk_list_head;
> +	struct ufs_clk_info *clki;
> +	u32 max_freq = 0;
> +	int err;

Let's use "ret" from now onwards. Existing "err" can be cleaned up later.

> +
> +	list_for_each_entry(clki, head, list) {
> +		if (!IS_ERR_OR_NULL(clki->clk) &&
> +			!strcmp(clki->name, "core_clk_unipro")) {

Odd indentation.

> +			max_freq = clki->max_freq;
> +			break;
> +		}
> +	}
> +
> +	switch (max_freq) {
> +	case MHZ_403:

UNIPRO_CORE_CLK_FREQ_403_MHZ?

Same applies to other defines.

> +		err = ufs_qcom_set_core_clk_ctrl(hba, 403, 16);

#define to_cycles_per_1us(freq)		(freq / (1000 * 1000))

		ret = ufs_qcom_set_core_clk_ctrl(hba, to_cycles_per_1us(max_freq), 16);

> +		break;
> +	case MHZ_300:
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 300, 12);
> +		break;
> +	case MHZ_201_5:
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 202, 8);
> +		break;
> +	case MHZ_150:
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 150, 6);
> +		break;
> +	case MHZ_100:
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4);
> +		break;
> +	default:
> +		dev_err(hba->dev, "unipro max_freq=%u entry missing\n", max_freq);

"UNIPRO clk max frequency (%u) not supported!"

> +		err = -EINVAL;

-ERANGE

> +		break;
> +	}
> +
> +	return err;
> +}
>  static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
>  					enum ufs_notify_change_status status)
>  {
> @@ -686,12 +725,15 @@ 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_cfg_core_clk_ctrl(hba);
> +			if (err) {
> +				dev_err(hba->dev,
> +					"%s cfg core clk ctrl failed\n",

"Failed to configure UNIPRO core clk"

> +					__func__);
> +				return err;
> +			}
> +		}
> 
>  		/*
>  		 * Some UFS devices (and may be host) have issues if LCC is
> @@ -1369,8 +1411,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_cfg_core_clk_ctrl(hba);
>  }
> 
>  static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
> @@ -1401,12 +1442,39 @@ 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 list_head *head = &hba->clk_list_head;
> +	struct ufs_clk_info *clki;
> +	u32 curr_freq = 0;
> +	int err;
> 
>  	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);
> +
> +	list_for_each_entry(clki, head, list) {
> +		if (!IS_ERR_OR_NULL(clki->clk) &&
> +			!strcmp(clki->name, "core_clk_unipro")) {
> +			curr_freq = clk_get_rate(clki->clk);
> +			break;
> +		}
> +	}
> +	switch (curr_freq) {
> +	case MHZ_37_5:
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 38, 2);
> +		break;
> +	case MHZ_75:
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 75, 3);
> +		break;
> +	case MHZ_100:
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4);
> +		break;
> +	default:
> +		err = -EINVAL;
> +		dev_err(hba->dev, "unipro curr_freq=%u entry missing\n", curr_freq);
> +		break;
> +	}
> +
> +	return err;

Why can't you use the existing ufs_qcom_cfg_core_clk_ctrl() function?

- Mani

>  }
> 
>  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 325f08aca260..56550fd36c4e 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -79,6 +79,15 @@ enum {
>  	UFS_MEM_CQIS_VS		= 0x8,
>  };
> 
> +/* QCOM UFS host controller core clk frequencies */
> +#define MHZ_37_5	37500000
> +#define MHZ_50		50000000
> +#define MHZ_75		75000000
> +#define MHZ_100		100000000
> +#define MHZ_150		150000000
> +#define MHZ_300		300000000
> +#define MHZ_201_5	201500000
> +#define MHZ_403		403000000
>  #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 V5 3/6] scsi: ufs: qcom: Add multiple frequency support for unipro clk attributes

[Manivannan Sadhasivam]

On Mon, Aug 28, 2023 at 01:35:37PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Aug 23, 2023 at 09:14:10PM +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.
> > 
> > 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 | 88 ++++++++++++++++++++++++++++++++-----
> >  drivers/ufs/host/ufs-qcom.h |  9 ++++
> >  2 files changed, 87 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > index abc0e7f7d1b0..8162b19191a9 100644
> > --- a/drivers/ufs/host/ufs-qcom.c
> > +++ b/drivers/ufs/host/ufs-qcom.c
> > @@ -671,6 +671,45 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, u32 gear,
> >  	return 0;
> >  }
> > 
> > +static int ufs_qcom_cfg_core_clk_ctrl(struct ufs_hba *hba)
> > +{
> > +	struct list_head *head = &hba->clk_list_head;
> > +	struct ufs_clk_info *clki;
> > +	u32 max_freq = 0;
> > +	int err;
> 
> Let's use "ret" from now onwards. Existing "err" can be cleaned up later.
> 
> > +
> > +	list_for_each_entry(clki, head, list) {
> > +		if (!IS_ERR_OR_NULL(clki->clk) &&
> > +			!strcmp(clki->name, "core_clk_unipro")) {
> 
> Odd indentation.
> 
> > +			max_freq = clki->max_freq;
> > +			break;
> > +		}
> > +	}
> > +
> > +	switch (max_freq) {
> > +	case MHZ_403:
> 
> UNIPRO_CORE_CLK_FREQ_403_MHZ?
> 
> Same applies to other defines.
> 
> > +		err = ufs_qcom_set_core_clk_ctrl(hba, 403, 16);
> 
> #define to_cycles_per_1us(freq)		(freq / (1000 * 1000))
> 
> 		ret = ufs_qcom_set_core_clk_ctrl(hba, to_cycles_per_1us(max_freq), 16);
> 

You could also use:

#define to_cycles_per_40us(freq) (freq / (25 * 1000 * 1000))

		ret = ufs_qcom_set_core_clk_ctrl(hba, to_cycles_per_1us(max_freq),
						 to_cycles_per_40us(max_freq));

This also gives me an impression that the caller could just pass the max_freq
and the ufs_qcom_set_core_clk_ctrl() function could internally calculate 1us and
40us cycles value.

- Mani

> > +		break;
> > +	case MHZ_300:
> > +		err = ufs_qcom_set_core_clk_ctrl(hba, 300, 12);
> > +		break;
> > +	case MHZ_201_5:
> > +		err = ufs_qcom_set_core_clk_ctrl(hba, 202, 8);
> > +		break;
> > +	case MHZ_150:
> > +		err = ufs_qcom_set_core_clk_ctrl(hba, 150, 6);
> > +		break;
> > +	case MHZ_100:
> > +		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4);
> > +		break;
> > +	default:
> > +		dev_err(hba->dev, "unipro max_freq=%u entry missing\n", max_freq);
> 
> "UNIPRO clk max frequency (%u) not supported!"
> 
> > +		err = -EINVAL;
> 
> -ERANGE
> 
> > +		break;
> > +	}
> > +
> > +	return err;
> > +}
> >  static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
> >  					enum ufs_notify_change_status status)
> >  {
> > @@ -686,12 +725,15 @@ 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_cfg_core_clk_ctrl(hba);
> > +			if (err) {
> > +				dev_err(hba->dev,
> > +					"%s cfg core clk ctrl failed\n",
> 
> "Failed to configure UNIPRO core clk"
> 
> > +					__func__);
> > +				return err;
> > +			}
> > +		}
> > 
> >  		/*
> >  		 * Some UFS devices (and may be host) have issues if LCC is
> > @@ -1369,8 +1411,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_cfg_core_clk_ctrl(hba);
> >  }
> > 
> >  static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
> > @@ -1401,12 +1442,39 @@ 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 list_head *head = &hba->clk_list_head;
> > +	struct ufs_clk_info *clki;
> > +	u32 curr_freq = 0;
> > +	int err;
> > 
> >  	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);
> > +
> > +	list_for_each_entry(clki, head, list) {
> > +		if (!IS_ERR_OR_NULL(clki->clk) &&
> > +			!strcmp(clki->name, "core_clk_unipro")) {
> > +			curr_freq = clk_get_rate(clki->clk);
> > +			break;
> > +		}
> > +	}
> > +	switch (curr_freq) {
> > +	case MHZ_37_5:
> > +		err = ufs_qcom_set_core_clk_ctrl(hba, 38, 2);
> > +		break;
> > +	case MHZ_75:
> > +		err = ufs_qcom_set_core_clk_ctrl(hba, 75, 3);
> > +		break;
> > +	case MHZ_100:
> > +		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4);
> > +		break;
> > +	default:
> > +		err = -EINVAL;
> > +		dev_err(hba->dev, "unipro curr_freq=%u entry missing\n", curr_freq);
> > +		break;
> > +	}
> > +
> > +	return err;
> 
> Why can't you use the existing ufs_qcom_cfg_core_clk_ctrl() function?
> 
> - Mani
> 
> >  }
> > 
> >  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 325f08aca260..56550fd36c4e 100644
> > --- a/drivers/ufs/host/ufs-qcom.h
> > +++ b/drivers/ufs/host/ufs-qcom.h
> > @@ -79,6 +79,15 @@ enum {
> >  	UFS_MEM_CQIS_VS		= 0x8,
> >  };
> > 
> > +/* QCOM UFS host controller core clk frequencies */
> > +#define MHZ_37_5	37500000
> > +#define MHZ_50		50000000
> > +#define MHZ_75		75000000
> > +#define MHZ_100		100000000
> > +#define MHZ_150		150000000
> > +#define MHZ_300		300000000
> > +#define MHZ_201_5	201500000
> > +#define MHZ_403		403000000
> >  #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 V5 4/6] scsi: ufs: qcom: Align unipro clk attributes as per Hardware specification

[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 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 | 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 8162b19191a9..491c0173603e 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1399,12 +1399,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);

@@ -1414,6 +1408,11 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
 	return ufs_qcom_cfg_core_clk_ctrl(hba);
 }

+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 V5 4/6] scsi: ufs: qcom: Align unipro clk attributes as per Hardware specification

[Manivannan Sadhasivam]

On Wed, Aug 23, 2023 at 09:14:11PM +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 with the hardware specification.
> 

Does this mean, the driver was doing the clk scaling at the wrong time? If so,
this patch should be moved ahead of all patches, should have fixes tag and CC
stable list.

- Mani

> 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 8162b19191a9..491c0173603e 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1399,12 +1399,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);
> 
> @@ -1414,6 +1408,11 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
>  	return ufs_qcom_cfg_core_clk_ctrl(hba);
>  }
> 
> +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 V5 4/6] scsi: ufs: qcom: Align unipro clk attributes as per Hardware specification

[Nitin Rawat]

On 8/28/2023 1:38 PM, Manivannan Sadhasivam wrote:
> On Wed, Aug 23, 2023 at 09:14:11PM +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 with the hardware specification.
>>
> 
> Does this mean, the driver was doing the clk scaling at the wrong time? If so,
> this patch should be moved ahead of all patches, should have fixes tag and CC
> stable list.
> 
> - Mani
> 

-- Functionality wise there is no affect with this patch. Our Qcom 
internal Hardware specification suggest to program these timers
in pre clk scale. SO we wanted to align with the HPG.

-Nitin




>> 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 8162b19191a9..491c0173603e 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -1399,12 +1399,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);
>>
>> @@ -1414,6 +1408,11 @@ static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
>>   	return ufs_qcom_cfg_core_clk_ctrl(hba);
>>   }
>>
>> +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
>>
> 

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

[Nitin Rawat]

This change configures SYS1CLK_1US_REG for pre scale up condition. Also
move ufs_qcom_cfg_timers from clk scaling post change ops to clk scaling
pre change ops 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 | 61 +++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 491c0173603e..82cf3ac4193a 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -533,7 +533,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;
@@ -564,11 +565,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) {
@@ -577,8 +583,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 */
@@ -658,7 +670,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);
 		/*
@@ -719,7 +731,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;
@@ -968,7 +980,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__);
 			/*
@@ -1401,11 +1413,24 @@ 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 err;

 	if (!ufs_qcom_cap_qunipro(host))
-		return 0;
+		goto out;
+
+	if (attr) {
+		err = ufs_qcom_cfg_timers(hba, attr->gear_rx,
+					    attr->pwr_rx, attr->hs_rate,
+					    false, true);
+		if (err)
+			dev_err(hba->dev, "%s ufs cfg timer failed\n",
+								__func__);
+	}

-	return ufs_qcom_cfg_core_clk_ctrl(hba);
+	err = ufs_qcom_cfg_core_clk_ctrl(hba);
+out:
+	return err;
 }

 static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
@@ -1441,6 +1466,7 @@ 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;
 	struct list_head *head = &hba->clk_list_head;
 	struct ufs_clk_info *clki;
 	u32 curr_freq = 0;
@@ -1449,6 +1475,9 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
 	if (!ufs_qcom_cap_qunipro(host))
 		return 0;

+	if (attr)
+		ufs_qcom_cfg_timers(hba, attr->gear_rx, attr->pwr_rx,
+					 attr->hs_rate, false, false);

 	list_for_each_entry(clki, head, list) {
 		if (!IS_ERR_OR_NULL(clki->clk) &&
@@ -1480,7 +1509,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 */
@@ -1510,11 +1538,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 V5 5/6] scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers function.

[Manivannan Sadhasivam]

On Wed, Aug 23, 2023 at 09:14:12PM +0530, Nitin Rawat wrote:
> This change configures SYS1CLK_1US_REG for pre scale up condition. Also
> move ufs_qcom_cfg_timers from clk scaling post change ops to clk scaling
> pre change ops to align with the hardware specification.
> 

Same comment as previous patch. This looks like a bug fix to me.

Also, this patch should be splitted into 2. SYS1CLK_1US_REG and
ufs_qcom_cfg_timers change.

- Mani

> 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 | 61 +++++++++++++++++++++++++------------
>  1 file changed, 42 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 491c0173603e..82cf3ac4193a 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -533,7 +533,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;
> @@ -564,11 +565,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) {
> @@ -577,8 +583,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 */
> @@ -658,7 +670,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);
>  		/*
> @@ -719,7 +731,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;
> @@ -968,7 +980,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__);
>  			/*
> @@ -1401,11 +1413,24 @@ 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 err;
> 
>  	if (!ufs_qcom_cap_qunipro(host))
> -		return 0;
> +		goto out;
> +
> +	if (attr) {
> +		err = ufs_qcom_cfg_timers(hba, attr->gear_rx,
> +					    attr->pwr_rx, attr->hs_rate,
> +					    false, true);
> +		if (err)
> +			dev_err(hba->dev, "%s ufs cfg timer failed\n",
> +								__func__);
> +	}
> 
> -	return ufs_qcom_cfg_core_clk_ctrl(hba);
> +	err = ufs_qcom_cfg_core_clk_ctrl(hba);
> +out:
> +	return err;
>  }
> 
>  static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
> @@ -1441,6 +1466,7 @@ 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;
>  	struct list_head *head = &hba->clk_list_head;
>  	struct ufs_clk_info *clki;
>  	u32 curr_freq = 0;
> @@ -1449,6 +1475,9 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
>  	if (!ufs_qcom_cap_qunipro(host))
>  		return 0;
> 
> +	if (attr)
> +		ufs_qcom_cfg_timers(hba, attr->gear_rx, attr->pwr_rx,
> +					 attr->hs_rate, false, false);
> 
>  	list_for_each_entry(clki, head, list) {
>  		if (!IS_ERR_OR_NULL(clki->clk) &&
> @@ -1480,7 +1509,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 */
> @@ -1510,11 +1538,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 V5 5/6] scsi: ufs: qcom: Refactor ufs_qcom_cfg_timers function.

[Nitin Rawat]

On 8/28/2023 1:47 PM, Manivannan Sadhasivam wrote:
> On Wed, Aug 23, 2023 at 09:14:12PM +0530, Nitin Rawat wrote:
>> This change configures SYS1CLK_1US_REG for pre scale up condition. Also
>> move ufs_qcom_cfg_timers from clk scaling post change ops to clk scaling
>> pre change ops to align with the hardware specification.
>>
> 
> Same comment as previous patch. This looks like a bug fix to me.
> 
> Also, this patch should be splitted into 2. SYS1CLK_1US_REG and
> ufs_qcom_cfg_timers change.
> 
> - Mani
> 


In this patch we are trying to refactor ufs_qcom_cfg_timers function and 
added extra argument to this function. Since it is just refactoring 
code, IMO it's better to not split in to 2 patches. We will update the 
commit message to explain more in detail

--Nitin

>> 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 | 61 +++++++++++++++++++++++++------------
>>   1 file changed, 42 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 491c0173603e..82cf3ac4193a 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -533,7 +533,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;
>> @@ -564,11 +565,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) {
>> @@ -577,8 +583,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 */
>> @@ -658,7 +670,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);
>>   		/*
>> @@ -719,7 +731,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;
>> @@ -968,7 +980,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__);
>>   			/*
>> @@ -1401,11 +1413,24 @@ 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 err;
>>
>>   	if (!ufs_qcom_cap_qunipro(host))
>> -		return 0;
>> +		goto out;
>> +
>> +	if (attr) {
>> +		err = ufs_qcom_cfg_timers(hba, attr->gear_rx,
>> +					    attr->pwr_rx, attr->hs_rate,
>> +					    false, true);
>> +		if (err)
>> +			dev_err(hba->dev, "%s ufs cfg timer failed\n",
>> +								__func__);
>> +	}
>>
>> -	return ufs_qcom_cfg_core_clk_ctrl(hba);
>> +	err = ufs_qcom_cfg_core_clk_ctrl(hba);
>> +out:
>> +	return err;
>>   }
>>
>>   static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
>> @@ -1441,6 +1466,7 @@ 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;
>>   	struct list_head *head = &hba->clk_list_head;
>>   	struct ufs_clk_info *clki;
>>   	u32 curr_freq = 0;
>> @@ -1449,6 +1475,9 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
>>   	if (!ufs_qcom_cap_qunipro(host))
>>   		return 0;
>>
>> +	if (attr)
>> +		ufs_qcom_cfg_timers(hba, attr->gear_rx, attr->pwr_rx,
>> +					 attr->hs_rate, false, false);
>>
>>   	list_for_each_entry(clki, head, list) {
>>   		if (!IS_ERR_OR_NULL(clki->clk) &&
>> @@ -1480,7 +1509,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 */
>> @@ -1510,11 +1538,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
>>
> 

[PATCH V5 6/6] scsi: ufs: qcom: Handle unipro clk HW division based on scaling conditions.

[Nitin Rawat]

Configure internal HW division of unipro core_clk based on scale up and
scale down condition. This bit should be cleared before entering any
SVS mode 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 | 49 ++++++++++++++++++++-----------------
 drivers/ufs/host/ufs-qcom.h |  2 +-
 2 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 82cf3ac4193a..d886e28b8a2a 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -95,7 +95,8 @@ 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);
+					u32 clk_40ns_cycles,
+					bool scale_up);

 static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
 {
@@ -700,19 +701,19 @@ static int ufs_qcom_cfg_core_clk_ctrl(struct ufs_hba *hba)

 	switch (max_freq) {
 	case MHZ_403:
-		err = ufs_qcom_set_core_clk_ctrl(hba, 403, 16);
+		err = ufs_qcom_set_core_clk_ctrl(hba, 403, 16, true);
 		break;
 	case MHZ_300:
-		err = ufs_qcom_set_core_clk_ctrl(hba, 300, 12);
+		err = ufs_qcom_set_core_clk_ctrl(hba, 300, 12, true);
 		break;
 	case MHZ_201_5:
-		err = ufs_qcom_set_core_clk_ctrl(hba, 202, 8);
+		err = ufs_qcom_set_core_clk_ctrl(hba, 202, 8, true);
 		break;
 	case MHZ_150:
-		err = ufs_qcom_set_core_clk_ctrl(hba, 150, 6);
+		err = ufs_qcom_set_core_clk_ctrl(hba, 150, 6, true);
 		break;
 	case MHZ_100:
-		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4);
+		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4, true);
 		break;
 	default:
 		dev_err(hba->dev, "unipro max_freq=%u entry missing\n", max_freq);
@@ -1352,7 +1353,8 @@ static void ufs_qcom_exit(struct ufs_hba *hba)

 static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
 					u32 clk_1us_cycles,
-					u32 clk_40ns_cycles)
+					u32 clk_40ns_cycles,
+					bool scale_up)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	u32 mask = DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK;
@@ -1378,18 +1380,20 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
 	core_clk_ctrl_reg &= ~(mask << offset);
 	core_clk_ctrl_reg |= clk_1us_cycles << offset;

-	/* Clear CORE_CLK_DIV_EN */
-	core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT;
+	if (scale_up)
+		core_clk_ctrl_reg |= CORE_CLK_DIV_EN_BIT;

 	err = ufshcd_dme_set(hba,
-			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
-			    core_clk_ctrl_reg);
+			     UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
+			     core_clk_ctrl_reg);
+	if (err)
+		return err;
 	/*
 	 * 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 (host->hw_ver.major >= 4) {
 		if (clk_40ns_cycles > PA_VS_CORE_CLK_40NS_CYCLES_MASK)
 			return -EINVAL;

@@ -1442,22 +1446,21 @@ 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;
+	u32 reg;

 	if (!ufs_qcom_cap_qunipro(host))
 		return 0;

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

 	/* 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;
+	if (reg & CORE_CLK_DIV_EN_BIT) {
+		reg &= ~CORE_CLK_DIV_EN_BIT;
 		err = ufshcd_dme_set(hba,
 				    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
-				    core_clk_ctrl_reg);
+				    reg);
 	}

 	return err;
@@ -1488,13 +1491,13 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
 	}
 	switch (curr_freq) {
 	case MHZ_37_5:
-		err = ufs_qcom_set_core_clk_ctrl(hba, 38, 2);
+		err = ufs_qcom_set_core_clk_ctrl(hba, 38, 2, false);
 		break;
 	case MHZ_75:
-		err = ufs_qcom_set_core_clk_ctrl(hba, 75, 3);
+		err = ufs_qcom_set_core_clk_ctrl(hba, 75, 3, false);
 		break;
 	case MHZ_100:
-		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4);
+		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4, false);
 		break;
 	default:
 		err = -EINVAL;
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 56550fd36c4e..6e8eb4bb9247 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -140,7 +140,7 @@ enum {
 /* bit and mask definitions for DME_VS_CORE_CLK_CTRL attribute */
 #define MAX_CORE_CLK_1US_CYCLES_MASK_V4		0xFFF
 #define MAX_CORE_CLK_1US_CYCLES_OFFSET_V4	0x10
-#define DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT		BIT(8)
+#define CORE_CLK_DIV_EN_BIT			BIT(8)
 #define DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK	0xFF
 #define PA_VS_CORE_CLK_40NS_CYCLES	0x9007
 #define PA_VS_CORE_CLK_40NS_CYCLES_MASK	0x3F
--
2.17.1

Re: [PATCH V5 6/6] scsi: ufs: qcom: Handle unipro clk HW division based on scaling conditions.

[Manivannan Sadhasivam]

On Wed, Aug 23, 2023 at 09:14:13PM +0530, Nitin Rawat wrote:
> Configure internal HW division of unipro core_clk based on scale up and
> scale down condition. This bit should be cleared before entering any
> SVS mode as per hardware specification.
> 

Same comment as previous patch.

- Mani

> 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 | 49 ++++++++++++++++++++-----------------
>  drivers/ufs/host/ufs-qcom.h |  2 +-
>  2 files changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 82cf3ac4193a..d886e28b8a2a 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -95,7 +95,8 @@ 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);
> +					u32 clk_40ns_cycles,
> +					bool scale_up);
> 
>  static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
>  {
> @@ -700,19 +701,19 @@ static int ufs_qcom_cfg_core_clk_ctrl(struct ufs_hba *hba)
> 
>  	switch (max_freq) {
>  	case MHZ_403:
> -		err = ufs_qcom_set_core_clk_ctrl(hba, 403, 16);
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 403, 16, true);
>  		break;
>  	case MHZ_300:
> -		err = ufs_qcom_set_core_clk_ctrl(hba, 300, 12);
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 300, 12, true);
>  		break;
>  	case MHZ_201_5:
> -		err = ufs_qcom_set_core_clk_ctrl(hba, 202, 8);
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 202, 8, true);
>  		break;
>  	case MHZ_150:
> -		err = ufs_qcom_set_core_clk_ctrl(hba, 150, 6);
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 150, 6, true);
>  		break;
>  	case MHZ_100:
> -		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4);
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4, true);
>  		break;
>  	default:
>  		dev_err(hba->dev, "unipro max_freq=%u entry missing\n", max_freq);
> @@ -1352,7 +1353,8 @@ static void ufs_qcom_exit(struct ufs_hba *hba)
> 
>  static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
>  					u32 clk_1us_cycles,
> -					u32 clk_40ns_cycles)
> +					u32 clk_40ns_cycles,
> +					bool scale_up)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>  	u32 mask = DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK;
> @@ -1378,18 +1380,20 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba,
>  	core_clk_ctrl_reg &= ~(mask << offset);
>  	core_clk_ctrl_reg |= clk_1us_cycles << offset;
> 
> -	/* Clear CORE_CLK_DIV_EN */
> -	core_clk_ctrl_reg &= ~DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT;
> +	if (scale_up)
> +		core_clk_ctrl_reg |= CORE_CLK_DIV_EN_BIT;
> 
>  	err = ufshcd_dme_set(hba,
> -			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
> -			    core_clk_ctrl_reg);
> +			     UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
> +			     core_clk_ctrl_reg);
> +	if (err)
> +		return err;
>  	/*
>  	 * 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 (host->hw_ver.major >= 4) {
>  		if (clk_40ns_cycles > PA_VS_CORE_CLK_40NS_CYCLES_MASK)
>  			return -EINVAL;
> 
> @@ -1442,22 +1446,21 @@ 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;
> +	u32 reg;
> 
>  	if (!ufs_qcom_cap_qunipro(host))
>  		return 0;
> 
> -	err = ufshcd_dme_get(hba,
> -			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
> -			    &core_clk_ctrl_reg);
> +	err = ufshcd_dme_get(hba, UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL), &reg);
> +	if (err)
> +		return err;
> 
>  	/* 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;
> +	if (reg & CORE_CLK_DIV_EN_BIT) {
> +		reg &= ~CORE_CLK_DIV_EN_BIT;
>  		err = ufshcd_dme_set(hba,
>  				    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
> -				    core_clk_ctrl_reg);
> +				    reg);
>  	}
> 
>  	return err;
> @@ -1488,13 +1491,13 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
>  	}
>  	switch (curr_freq) {
>  	case MHZ_37_5:
> -		err = ufs_qcom_set_core_clk_ctrl(hba, 38, 2);
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 38, 2, false);
>  		break;
>  	case MHZ_75:
> -		err = ufs_qcom_set_core_clk_ctrl(hba, 75, 3);
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 75, 3, false);
>  		break;
>  	case MHZ_100:
> -		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4);
> +		err = ufs_qcom_set_core_clk_ctrl(hba, 100, 4, false);
>  		break;
>  	default:
>  		err = -EINVAL;
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 56550fd36c4e..6e8eb4bb9247 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -140,7 +140,7 @@ enum {
>  /* bit and mask definitions for DME_VS_CORE_CLK_CTRL attribute */
>  #define MAX_CORE_CLK_1US_CYCLES_MASK_V4		0xFFF
>  #define MAX_CORE_CLK_1US_CYCLES_OFFSET_V4	0x10
> -#define DME_VS_CORE_CLK_CTRL_CORE_CLK_DIV_EN_BIT		BIT(8)
> +#define CORE_CLK_DIV_EN_BIT			BIT(8)
>  #define DME_VS_CORE_CLK_CTRL_MAX_CORE_CLK_1US_CYCLES_MASK	0xFF
>  #define PA_VS_CORE_CLK_40NS_CYCLES	0x9007
>  #define PA_VS_CORE_CLK_40NS_CYCLES_MASK	0x3F
> --
> 2.17.1
> 

-- 
மணிவண்ணன் சதாசிவம்

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

[Martin K. Petersen]

Nitin,

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

Which tree is this against? Doesn't apply to 6.6/scsi-queue...

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Manivannan Sadhasivam]

On Fri, Aug 25, 2023 at 05:44:01PM -0400, Martin K. Petersen wrote:
> 
> Nitin,
> 
> > This patch aligns programming sequence as per Qualcomm UFS hardware
> > specification.
> 
> Which tree is this against? Doesn't apply to 6.6/scsi-queue...
> 

Martin, thanks for considering this series. However, I'd request you to wait for
an Ack from atleast one reviewer/maintainer before merging the Qcom patches.
Reviews might be delayed, but for sure we are keeping an eye on all patches.

- Mani

> -- 
> Martin K. Petersen	Oracle Linux Engineering

-- 
மணிவண்ணன் சதாசிவம்