[PATCH v5 0/8] Support Multi-frequency scale for UFS

[PATCH v5 0/8] Support Multi-frequency scale for UFS

[Ziqi Chen]

With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
plans. However, the gear speed is only toggled between min and max during
clock scaling. Enable multi-level gear scaling by mapping clock frequencies
to gear speeds, so that when devfreq scales clock frequencies we can put
the UFS link at the appropraite gear speeds accordingly.

This series has been tested on below platforms -
sm8550 mtp + UFS3.1
SM8650 MTP + UFS3.1
SM8750 MTP + UFS4.0

Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-HDK
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-HDK 

v1 -> v2:
1. Withdraw old patch 8/8 "ARM: dts: msm: Use Operation Points V2 for UFS on SM8650"
2. Add new patch 8/8 "ABI: sysfs-driver-ufs: Add missing UFS sysfs addributes"
3. Modify commit message for  "scsi: ufs: core: Pass target_freq to clk_scale_notify() vops" and "scsi: ufs: qcom: Pass target_freq to clk scale pre and post change"
4. In "scsi: ufs: qcom: Pass target_freq to clk scale pre and post change", use common Macro HZ_PER_MHZ in function ufs_qcom_set_core_clk_ctrl()
5. In "scsi: ufs: qcom: Implement the freq_to_gear_speed() vops", print out freq and gear info as debugging message
6. In "scsi: ufs: core: Enable multi-level gear scaling", rename the lable "do_pmc" to "config_pwr_mode"
7. In "scsi: ufs: core: Toggle Write Booster during clock", initialize the local variables "wb_en" as "false"

v2 -> v3:
1. Change 'vops' to 'vop' in all commit message
2. keep the indentation consistent for clk_scale_notify() definition.
3. In "scsi: ufs: core: Add a vop to map clock frequency to gear speed", "scsi: ufs: qcom: Implement the freq_to_gear_speed() vop"
   and "scsi: ufs: core: Enable multi-level gear scaling", remove the parameter 'gear' and use it as return result in function freq_to_gear_speed()
4. In "scsi: ufs: qcom: Implement the freq_to_gear_speed(), removed the variable 'ret' in function ufs_qcom_freq_to_gear_speed()
5. In "scsi: ufs: core: Enable multi-level gear scaling", use assignment instead memcpy() in function ufshcd_scale_gear()
6. Improve the grammar of attributes' descriptions in “ABI: sysfs-driver-ufs: Add missing UFS sysfs attributes”
7. Typo fixed for some commit messages.

v3 -> v4:
1. In "scsi: ufs: core: Toggle Write Booster during clock scaling base on gear speed":
	a. Add comment for default initialized wb_gear
	b. Remove the unnecessary variable “wb_en" in function ufshcd_clock_scaling_unprepare()
2. Typo fixed for commit message of "scsi: ufs: core: Enable multi-level gear scaling"
3. Make the description words are more standardized in "ABI: sysfs-driver-ufs: Add missing UFS sysfs attributes"

v4 -> v5:
1. keep the identation consistent
2. Change the data type from 'int' to 'u32' vop freq_to_gear_speed() return value

Can Guo (6):
  scsi: ufs: core: Pass target_freq to clk_scale_notify() vop
  scsi: ufs: qcom: Pass target_freq to clk scale pre and post change
  scsi: ufs: core: Add a vop to map clock frequency to gear speed
  scsi: ufs: qcom: Implement the freq_to_gear_speed() vop
  scsi: ufs: core: Enable multi-level gear scaling
  scsi: ufs: core: Toggle Write Booster during clock scaling base on
    gear speed

Ziqi Chen (2):
  scsi: ufs: core: Check if scaling up is required when disable clkscale
  ABI: sysfs-driver-ufs: Add missing UFS sysfs attributes

 Documentation/ABI/testing/sysfs-driver-ufs | 33 +++++++++++
 drivers/ufs/core/ufshcd-priv.h             | 15 ++++-
 drivers/ufs/core/ufshcd.c                  | 64 ++++++++++++++++------
 drivers/ufs/host/ufs-mediatek.c            |  1 +
 drivers/ufs/host/ufs-qcom.c                | 62 ++++++++++++++++-----
 include/ufs/ufshcd.h                       |  9 ++-
 6 files changed, 149 insertions(+), 35 deletions(-)

-- 
2.34.1

[PATCH v5 1/8] scsi: ufs: core: Pass target_freq to clk_scale_notify() vop

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

Instead of only two frequencies, if OPP V2 is used, the UFS devfreq clock
scaling may scale the clock among multiple frequencies, so just passing
up/down to vop clk_scale_notify() is not enough to cover the intermediate
clock freqs between the min and max freqs. Hence pass the target_freq ,
which will be used in successive commits, to clk_scale_notify() to allow
the vop to perform corresponding configurations with regard to the clock
freqs.

Signed-off-by: Can Guo <quic_cang@quicinc.com>
Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
---

v1 -> v2:
Modify commit message to make it more clear.

v2 -> v3:
Change 'vops' to 'vop' in commit message.

v4 -> v5:
keep the indentation consistent for clk_scale_notify() definition.
---
 drivers/ufs/core/ufshcd-priv.h  | 7 ++++---
 drivers/ufs/core/ufshcd.c       | 4 ++--
 drivers/ufs/host/ufs-mediatek.c | 1 +
 drivers/ufs/host/ufs-qcom.c     | 5 +++--
 include/ufs/ufshcd.h            | 4 ++--
 5 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 9ffd94ddf8c7..0549b65f71ed 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -117,11 +117,12 @@ static inline u32 ufshcd_vops_get_ufs_hci_version(struct ufs_hba *hba)
 	return ufshcd_readl(hba, REG_UFS_VERSION);
 }
 
-static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba,
-			bool up, enum ufs_notify_change_status status)
+static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba, bool up,
+					       unsigned long target_freq,
+					       enum ufs_notify_change_status status)
 {
 	if (hba->vops && hba->vops->clk_scale_notify)
-		return hba->vops->clk_scale_notify(hba, up, status);
+		return hba->vops->clk_scale_notify(hba, up, target_freq, status);
 	return 0;
 }
 
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index acc3607bbd9c..8d295cc827cc 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1157,7 +1157,7 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
 	int ret = 0;
 	ktime_t start = ktime_get();
 
-	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
+	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, freq, PRE_CHANGE);
 	if (ret)
 		goto out;
 
@@ -1168,7 +1168,7 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
 	if (ret)
 		goto out;
 
-	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
+	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, freq, POST_CHANGE);
 	if (ret) {
 		if (hba->use_pm_opp)
 			ufshcd_opp_set_rate(hba,
diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 135cd78109e2..977dd0caaef6 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -1643,6 +1643,7 @@ static void ufs_mtk_clk_scale(struct ufs_hba *hba, bool scale_up)
 }
 
 static int ufs_mtk_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
+				    unsigned long target_freq,
 				    enum ufs_notify_change_status status)
 {
 	if (!ufshcd_is_clkscaling_supported(hba))
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 68040b2ab5f8..b6eef975dc46 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1333,8 +1333,9 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
 	return ufs_qcom_set_core_clk_ctrl(hba, false);
 }
 
-static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
-		bool scale_up, enum ufs_notify_change_status status)
+static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
+				     unsigned long target_freq,
+				     enum ufs_notify_change_status status)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	int err;
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index d7aca9e61684..02802981f07f 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -344,8 +344,8 @@ struct ufs_hba_variant_ops {
 	void    (*exit)(struct ufs_hba *);
 	u32	(*get_ufs_hci_version)(struct ufs_hba *);
 	int	(*set_dma_mask)(struct ufs_hba *);
-	int	(*clk_scale_notify)(struct ufs_hba *, bool,
-				    enum ufs_notify_change_status);
+	int	(*clk_scale_notify)(struct ufs_hba *, bool, unsigned long,
+				enum ufs_notify_change_status);
 	int	(*setup_clocks)(struct ufs_hba *, bool,
 				enum ufs_notify_change_status);
 	int	(*hce_enable_notify)(struct ufs_hba *,
-- 
2.34.1

Re: [PATCH v5 1/8] scsi: ufs: core: Pass target_freq to clk_scale_notify() vop

[Peter Wang (王信友)]

On Thu, 2025-02-13 at 16:00 +0800, Ziqi Chen wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> From: Can Guo <quic_cang@quicinc.com>
> 
> Instead of only two frequencies, if OPP V2 is used, the UFS devfreq
> clock
> scaling may scale the clock among multiple frequencies, so just
> passing
> up/down to vop clk_scale_notify() is not enough to cover the
> intermediate
> clock freqs between the min and max freqs. Hence pass the target_freq
> ,
> which will be used in successive commits, to clk_scale_notify() to
> allow
> the vop to perform corresponding configurations with regard to the
> clock
> freqs.
> 
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Reviewed-by: Bean Huo <beanhuo@micron.com>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> 
> v1 -> v2:
> Modify commit message to make it more clear.
> 
> v2 -> v3:
> Change 'vops' to 'vop' in commit message.
> 
> v4 -> v5:
> keep the indentation consistent for clk_scale_notify() definition.
> 
> 

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

[PATCH v5 2/8] scsi: ufs: qcom: Pass target_freq to clk scale pre and post change

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

Instead of only two frequencies, if OPP V2 is used, the UFS devfreq clock
scaling may scale the clock among multiple frequencies. In the case of
scaling up, the devfreq may decide to scale the clock to an intermediate
freq based on load, but the clock scale up pre change operation uses
settings for the max clock freq unconditionally. Fix it by passing the
target_freq to clock scale up pre change so that the correct settings for
the target_freq can be used.

In the case of scaling down, the clock scale down post change operation
is doing fine, because it reads the actual clock rate to tell freq, but to
keep symmetry with clock scale up pre change operation, just use the
target_freq instead of reading clock rate.

Signed-off-by: Can Guo <quic_cang@quicinc.com>
Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
---

v1 -> v2:
1. Modify commit message to make it more clear.
2. Use common Macro HZ_PER_MHZ in function ufs_qcom_set_core_clk_ctrl().

v2 -> v3:
Commit message typo fixed.
---
 drivers/ufs/host/ufs-qcom.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index b6eef975dc46..a1eb3cab45e4 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -15,6 +15,7 @@
 #include <linux/platform_device.h>
 #include <linux/reset-controller.h>
 #include <linux/time.h>
+#include <linux/units.h>
 
 #include <soc/qcom/ice.h>
 
@@ -97,7 +98,7 @@ static const struct __ufs_qcom_bw_table {
 };
 
 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, bool is_scale_up);
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq);
 
 static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
 {
@@ -524,7 +525,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 			return -EINVAL;
 		}
 
-		err = ufs_qcom_set_core_clk_ctrl(hba, true);
+		err = ufs_qcom_set_core_clk_ctrl(hba, ULONG_MAX);
 		if (err)
 			dev_err(hba->dev, "cfg core clk ctrl failed\n");
 		/*
@@ -1231,7 +1232,7 @@ static int ufs_qcom_set_clk_40ns_cycles(struct ufs_hba *hba,
 	return ufshcd_dme_set(hba, UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES), reg);
 }
 
-static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct list_head *head = &hba->clk_list_head;
@@ -1245,10 +1246,11 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
 		    !strcmp(clki->name, "core_clk_unipro")) {
 			if (!clki->max_freq)
 				cycles_in_1us = 150; /* default for backwards compatibility */
-			else if (is_scale_up)
-				cycles_in_1us = ceil(clki->max_freq, (1000 * 1000));
+			else if (freq == ULONG_MAX)
+				cycles_in_1us = ceil(clki->max_freq, HZ_PER_MHZ);
 			else
-				cycles_in_1us = ceil(clk_get_rate(clki->clk), (1000 * 1000));
+				cycles_in_1us = ceil(freq, HZ_PER_MHZ);
+
 			break;
 		}
 	}
@@ -1285,7 +1287,7 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
 	return ufs_qcom_set_clk_40ns_cycles(hba, cycles_in_1us);
 }
 
-static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
+static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba, unsigned long freq)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct ufs_pa_layer_attr *attr = &host->dev_req_params;
@@ -1298,7 +1300,7 @@ static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
 		return ret;
 	}
 	/* set unipro core clock attributes and clear clock divider */
-	return ufs_qcom_set_core_clk_ctrl(hba, true);
+	return ufs_qcom_set_core_clk_ctrl(hba, freq);
 }
 
 static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
@@ -1327,10 +1329,10 @@ static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
 	return err;
 }
 
-static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
+static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba, unsigned long freq)
 {
 	/* set unipro core clock attributes and clear clock divider */
-	return ufs_qcom_set_core_clk_ctrl(hba, false);
+	return ufs_qcom_set_core_clk_ctrl(hba, freq);
 }
 
 static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
@@ -1349,7 +1351,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
 		if (err)
 			return err;
 		if (scale_up)
-			err = ufs_qcom_clk_scale_up_pre_change(hba);
+			err = ufs_qcom_clk_scale_up_pre_change(hba, target_freq);
 		else
 			err = ufs_qcom_clk_scale_down_pre_change(hba);
 
@@ -1361,7 +1363,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
 		if (scale_up)
 			err = ufs_qcom_clk_scale_up_post_change(hba);
 		else
-			err = ufs_qcom_clk_scale_down_post_change(hba);
+			err = ufs_qcom_clk_scale_down_post_change(hba, target_freq);
 
 
 		if (err) {
-- 
2.34.1

Re: [PATCH v5 2/8] scsi: ufs: qcom: Pass target_freq to clk scale pre and post change

[Peter Wang (王信友)]

On Thu, 2025-02-13 at 16:00 +0800, Ziqi Chen wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> From: Can Guo <quic_cang@quicinc.com>
> 
> Instead of only two frequencies, if OPP V2 is used, the UFS devfreq
> clock
> scaling may scale the clock among multiple frequencies. In the case
> of
> scaling up, the devfreq may decide to scale the clock to an
> intermediate
> freq based on load, but the clock scale up pre change operation uses
> settings for the max clock freq unconditionally. Fix it by passing
> the
> target_freq to clock scale up pre change so that the correct settings
> for
> the target_freq can be used.
> 
> In the case of scaling down, the clock scale down post change
> operation
> is doing fine, because it reads the actual clock rate to tell freq,
> but to
> keep symmetry with clock scale up pre change operation, just use the
> target_freq instead of reading clock rate.
> 
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Reviewed-by: Bean Huo <beanhuo@micron.com>
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

[PATCH v5 3/8] scsi: ufs: core: Add a vop to map clock frequency to gear speed

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

Add a vop to map UFS host controller clock frequencies to the corresponding
maximum supported UFS high speed gear speeds. During clock scaling, we can
map the target clock frequency, demanded by devfreq, to the maximum
supported gear speed, so that devfreq can scale the gear to the highest
gear speed supported at the target clock frequency, instead of just scaling
up/down the gear between the min and max gear speeds.

Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
---
v2 -> v3:
1. Remove the parameter 'gear' and use it as function return result.
2. Change "vops" into "vop" in commit message.

v4 -> v5:
1. keep the indentation consistent for vop freq_to_gear_speed.
2. Change the return value type of vop freq_to_gear_speed from 'int' to 'u32'.
---
 drivers/ufs/core/ufshcd-priv.h | 8 ++++++++
 include/ufs/ufshcd.h           | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 0549b65f71ed..983b0a8dafb5 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -277,6 +277,14 @@ static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba)
 	return -EOPNOTSUPP;
 }
 
+static inline u32 ufshcd_vops_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq)
+{
+	if (hba->vops && hba->vops->freq_to_gear_speed)
+		return hba->vops->freq_to_gear_speed(hba, freq);
+
+	return 0;
+}
+
 extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[];
 
 /**
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 02802981f07f..b34301de3cf8 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -336,6 +336,7 @@ struct ufs_pwr_mode_info {
  * @get_outstanding_cqs: called to get outstanding completion queues
  * @config_esi: called to config Event Specific Interrupt
  * @config_scsi_dev: called to configure SCSI device parameters
+ * @freq_to_gear_speed: called to map clock frequency to the max supported gear speed
  */
 struct ufs_hba_variant_ops {
 	const char *name;
@@ -387,6 +388,7 @@ struct ufs_hba_variant_ops {
 				       unsigned long *ocqs);
 	int	(*config_esi)(struct ufs_hba *hba);
 	void	(*config_scsi_dev)(struct scsi_device *sdev);
+	u32	(*freq_to_gear_speed)(struct ufs_hba *hba, unsigned long freq);
 };
 
 /* clock gating state  */
-- 
2.34.1

Re: [PATCH v5 3/8] scsi: ufs: core: Add a vop to map clock frequency to gear speed

[Peter Wang (王信友)]

On Thu, 2025-02-13 at 16:00 +0800, Ziqi Chen wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> From: Can Guo <quic_cang@quicinc.com>
> 
> Add a vop to map UFS host controller clock frequencies to the
> corresponding
> maximum supported UFS high speed gear speeds. During clock scaling,
> we can
> map the target clock frequency, demanded by devfreq, to the maximum
> supported gear speed, so that devfreq can scale the gear to the
> highest
> gear speed supported at the target clock frequency, instead of just
> scaling
> up/down the gear between the min and max gear speeds.
> 
> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> Reviewed-by: Bean Huo <beanhuo@micron.com>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> v2 -> v3:
> 1. Remove the parameter 'gear' and use it as function return result.
> 2. Change "vops" into "vop" in commit message.
> 
> v4 -> v5:
> 1. keep the indentation consistent for vop freq_to_gear_speed.
> 2. Change the return value type of vop freq_to_gear_speed from 'int'
> to 'u32'.
> ---
> 

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

[PATCH v5 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vop

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

Implement the freq_to_gear_speed() vop to map the unipro core clock
frequency to the corresponding maximum supported gear speed.

Signed-off-by: Can Guo <quic_cang@quicinc.com>
Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
---

v1 -> v2:
Print out freq and gear info as debugging message.

v2 -> v3:
1. Change "vops" to "vop" in commit message.
2. Removed variable 'ret' in function ufs_qcom_freq_to_gear_speed().
3. Removed parameters '*gear' and use gear value as return value for
   funtion ufs_qcom_freq_to_gear_speed().

v3 -> v4:
Change the data type of 'gear' from 'int' to 'u32'.
---
 drivers/ufs/host/ufs-qcom.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index a1eb3cab45e4..3353acaaa2ae 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1804,6 +1804,36 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
 	return ret;
 }
 
+static u32 ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq)
+{
+	u32 gear = 0;
+
+	switch (freq) {
+	case 403000000:
+		gear = UFS_HS_G5;
+		break;
+	case 300000000:
+		gear = UFS_HS_G4;
+		break;
+	case 201500000:
+		gear = UFS_HS_G3;
+		break;
+	case 150000000:
+	case 100000000:
+		gear = UFS_HS_G2;
+		break;
+	case 75000000:
+	case 37500000:
+		gear = UFS_HS_G1;
+		break;
+	default:
+		dev_err(hba->dev, "%s: Unsupported clock freq : %lu\n", __func__, freq);
+		break;
+	}
+
+	return gear;
+}
+
 /*
  * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
  *
@@ -1834,6 +1864,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
 	.op_runtime_config	= ufs_qcom_op_runtime_config,
 	.get_outstanding_cqs	= ufs_qcom_get_outstanding_cqs,
 	.config_esi		= ufs_qcom_config_esi,
+	.freq_to_gear_speed	= ufs_qcom_freq_to_gear_speed,
 };
 
 /**
-- 
2.34.1

Re: [PATCH v5 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vop

[Peter Wang (王信友)]

On Thu, 2025-02-13 at 16:00 +0800, Ziqi Chen wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> From: Can Guo <quic_cang@quicinc.com>
> 
> Implement the freq_to_gear_speed() vop to map the unipro core clock
> frequency to the corresponding maximum supported gear speed.
> 
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Reviewed-by: Bean Huo <beanhuo@micron.com>
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> 
> v1 -> v2:
> Print out freq and gear info as debugging message.
> 
> v2 -> v3:
> 1. Change "vops" to "vop" in commit message.
> 2. Removed variable 'ret' in function ufs_qcom_freq_to_gear_speed().
> 3. Removed parameters '*gear' and use gear value as return value for
>    funtion ufs_qcom_freq_to_gear_speed().
> 
> v3 -> v4:
> Change the data type of 'gear' from 'int' to 'u32'.
> ---
> 

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

[PATCH v5 5/8] scsi: ufs: core: Enable multi-level gear scaling

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
plans. However, the gear speed is only toggled between min and max during
clock scaling. Enable multi-level gear scaling by mapping clock frequencies
to gear speeds, so that when devfreq scales clock frequencies we can put
the UFS link at the appropriate gear speeds accordingly.

Signed-off-by: Can Guo <quic_cang@quicinc.com>
Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
---

v1 -> v2:
Rename the lable "do_pmc" to "config_pwr_mode".

v2 -> v3:
Use assignment instead memcpy() in function ufshcd_scale_gear().

v3 -> v4:
Typo fixed for commit message.

v4 -> v5:
Change the data type of "new_gear" from 'int' to 'u32'.
---
 drivers/ufs/core/ufshcd.c | 44 ++++++++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8d295cc827cc..9908c0d6a1e1 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1308,16 +1308,26 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
 /**
  * ufshcd_scale_gear - scale up/down UFS gear
  * @hba: per adapter instance
+ * @target_gear: target gear to scale to
  * @scale_up: True for scaling up gear and false for scaling down
  *
  * Return: 0 for success; -EBUSY if scaling can't happen at this time;
  * non-zero for any other errors.
  */
-static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
+static int ufshcd_scale_gear(struct ufs_hba *hba, u32 target_gear, bool scale_up)
 {
 	int ret = 0;
 	struct ufs_pa_layer_attr new_pwr_info;
 
+	if (target_gear) {
+		new_pwr_info = hba->pwr_info;
+		new_pwr_info.gear_tx = target_gear;
+		new_pwr_info.gear_rx = target_gear;
+
+		goto config_pwr_mode;
+	}
+
+	/* Legacy gear scaling, in case vops_freq_to_gear_speed() is not implemented */
 	if (scale_up) {
 		memcpy(&new_pwr_info, &hba->clk_scaling.saved_pwr_info,
 		       sizeof(struct ufs_pa_layer_attr));
@@ -1338,6 +1348,7 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
 		}
 	}
 
+config_pwr_mode:
 	/* check if the power mode needs to be changed or not? */
 	ret = ufshcd_config_pwr_mode(hba, &new_pwr_info);
 	if (ret)
@@ -1408,15 +1419,19 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool sc
 static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
 				bool scale_up)
 {
+	u32 old_gear = hba->pwr_info.gear_rx;
+	u32 new_gear = 0;
 	int ret = 0;
 
+	new_gear = ufshcd_vops_freq_to_gear_speed(hba, freq);
+
 	ret = ufshcd_clock_scaling_prepare(hba, 1 * USEC_PER_SEC);
 	if (ret)
 		return ret;
 
 	/* scale down the gear before scaling down clocks */
 	if (!scale_up) {
-		ret = ufshcd_scale_gear(hba, false);
+		ret = ufshcd_scale_gear(hba, new_gear, false);
 		if (ret)
 			goto out_unprepare;
 	}
@@ -1424,13 +1439,13 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
 	ret = ufshcd_scale_clks(hba, freq, scale_up);
 	if (ret) {
 		if (!scale_up)
-			ufshcd_scale_gear(hba, true);
+			ufshcd_scale_gear(hba, old_gear, true);
 		goto out_unprepare;
 	}
 
 	/* scale up the gear after scaling up clocks */
 	if (scale_up) {
-		ret = ufshcd_scale_gear(hba, true);
+		ret = ufshcd_scale_gear(hba, new_gear, true);
 		if (ret) {
 			ufshcd_scale_clks(hba, hba->devfreq->previous_freq,
 					  false);
@@ -1723,6 +1738,8 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct ufs_clk_info *clki;
+	unsigned long freq;
 	u32 value;
 	int err = 0;
 
@@ -1746,14 +1763,21 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 
 	if (value) {
 		ufshcd_resume_clkscaling(hba);
-	} else {
-		ufshcd_suspend_clkscaling(hba);
-		err = ufshcd_devfreq_scale(hba, ULONG_MAX, true);
-		if (err)
-			dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
-					__func__, err);
+		goto out_rel;
 	}
 
+	clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
+	freq = clki->max_freq;
+
+	ufshcd_suspend_clkscaling(hba);
+	err = ufshcd_devfreq_scale(hba, freq, true);
+	if (err)
+		dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
+				__func__, err);
+	else
+		hba->clk_scaling.target_freq = freq;
+
+out_rel:
 	ufshcd_release(hba);
 	ufshcd_rpm_put_sync(hba);
 out:
-- 
2.34.1

Re: [PATCH v5 5/8] scsi: ufs: core: Enable multi-level gear scaling

[Peter Wang (王信友)]

On Thu, 2025-02-13 at 16:00 +0800, Ziqi Chen wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> From: Can Guo <quic_cang@quicinc.com>
> 
> With OPP V2 enabled, devfreq can scale clocks amongst multiple
> frequency
> plans. However, the gear speed is only toggled between min and max
> during
> clock scaling. Enable multi-level gear scaling by mapping clock
> frequencies
> to gear speeds, so that when devfreq scales clock frequencies we can
> put
> the UFS link at the appropriate gear speeds accordingly.
> 
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Reviewed-by: Bean Huo <beanhuo@micron.com>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> 
> v1 -> v2:
> Rename the lable "do_pmc" to "config_pwr_mode".
> 
> v2 -> v3:
> Use assignment instead memcpy() in function ufshcd_scale_gear().
> 
> v3 -> v4:
> Typo fixed for commit message.
> 
> v4 -> v5:
> Change the data type of "new_gear" from 'int' to 'u32'.
> ---
> 

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH v5 5/8] scsi: ufs: core: Enable multi-level gear scaling

[Neil Armstrong]

Hi,

On 13/02/2025 09:00, Ziqi Chen wrote:
> From: Can Guo <quic_cang@quicinc.com>
> 
> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
> plans. However, the gear speed is only toggled between min and max during
> clock scaling. Enable multi-level gear scaling by mapping clock frequencies
> to gear speeds, so that when devfreq scales clock frequencies we can put
> the UFS link at the appropriate gear speeds accordingly.
> 
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Reviewed-by: Bean Huo <beanhuo@micron.com>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> 
> v1 -> v2:
> Rename the lable "do_pmc" to "config_pwr_mode".
> 
> v2 -> v3:
> Use assignment instead memcpy() in function ufshcd_scale_gear().
> 
> v3 -> v4:
> Typo fixed for commit message.
> 
> v4 -> v5:
> Change the data type of "new_gear" from 'int' to 'u32'.
> ---
>   drivers/ufs/core/ufshcd.c | 44 ++++++++++++++++++++++++++++++---------
>   1 file changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 8d295cc827cc..9908c0d6a1e1 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -1308,16 +1308,26 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
>   /**
>    * ufshcd_scale_gear - scale up/down UFS gear
>    * @hba: per adapter instance
> + * @target_gear: target gear to scale to
>    * @scale_up: True for scaling up gear and false for scaling down
>    *
>    * Return: 0 for success; -EBUSY if scaling can't happen at this time;
>    * non-zero for any other errors.
>    */
> -static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
> +static int ufshcd_scale_gear(struct ufs_hba *hba, u32 target_gear, bool scale_up)
>   {
>   	int ret = 0;
>   	struct ufs_pa_layer_attr new_pwr_info;
>   
> +	if (target_gear) {
> +		new_pwr_info = hba->pwr_info;
> +		new_pwr_info.gear_tx = target_gear;
> +		new_pwr_info.gear_rx = target_gear;
> +
> +		goto config_pwr_mode;
> +	}
> +
> +	/* Legacy gear scaling, in case vops_freq_to_gear_speed() is not implemented */
>   	if (scale_up) {
>   		memcpy(&new_pwr_info, &hba->clk_scaling.saved_pwr_info,
>   		       sizeof(struct ufs_pa_layer_attr));
> @@ -1338,6 +1348,7 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>   		}
>   	}
>   
> +config_pwr_mode:
>   	/* check if the power mode needs to be changed or not? */
>   	ret = ufshcd_config_pwr_mode(hba, &new_pwr_info);
>   	if (ret)
> @@ -1408,15 +1419,19 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool sc
>   static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
>   				bool scale_up)
>   {
> +	u32 old_gear = hba->pwr_info.gear_rx;
> +	u32 new_gear = 0;
>   	int ret = 0;
>   
> +	new_gear = ufshcd_vops_freq_to_gear_speed(hba, freq);
> +
>   	ret = ufshcd_clock_scaling_prepare(hba, 1 * USEC_PER_SEC);
>   	if (ret)
>   		return ret;
>   
>   	/* scale down the gear before scaling down clocks */
>   	if (!scale_up) {
> -		ret = ufshcd_scale_gear(hba, false);
> +		ret = ufshcd_scale_gear(hba, new_gear, false);
>   		if (ret)
>   			goto out_unprepare;
>   	}
> @@ -1424,13 +1439,13 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
>   	ret = ufshcd_scale_clks(hba, freq, scale_up);
>   	if (ret) {
>   		if (!scale_up)
> -			ufshcd_scale_gear(hba, true);
> +			ufshcd_scale_gear(hba, old_gear, true);
>   		goto out_unprepare;
>   	}
>   
>   	/* scale up the gear after scaling up clocks */
>   	if (scale_up) {
> -		ret = ufshcd_scale_gear(hba, true);
> +		ret = ufshcd_scale_gear(hba, new_gear, true);
>   		if (ret) {
>   			ufshcd_scale_clks(hba, hba->devfreq->previous_freq,
>   					  false);
> @@ -1723,6 +1738,8 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
>   		struct device_attribute *attr, const char *buf, size_t count)
>   {
>   	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	struct ufs_clk_info *clki;
> +	unsigned long freq;
>   	u32 value;
>   	int err = 0;
>   
> @@ -1746,14 +1763,21 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
>   
>   	if (value) {
>   		ufshcd_resume_clkscaling(hba);
> -	} else {
> -		ufshcd_suspend_clkscaling(hba);
> -		err = ufshcd_devfreq_scale(hba, ULONG_MAX, true);
> -		if (err)
> -			dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
> -					__func__, err);
> +		goto out_rel;
>   	}
>   
> +	clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
> +	freq = clki->max_freq;
> +
> +	ufshcd_suspend_clkscaling(hba);
> +	err = ufshcd_devfreq_scale(hba, freq, true);
> +	if (err)
> +		dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
> +				__func__, err);
> +	else
> +		hba->clk_scaling.target_freq = freq;
> +
> +out_rel:
>   	ufshcd_release(hba);
>   	ufshcd_rpm_put_sync(hba);
>   out:

This change causes UFS crash on the RB3gen2, after UFS successfully probe and scan:

[    9.383728] ufshcd-qcom 1d84000.ufshc: pwr ctrl cmd 0x2 with mode 0x11 completion timeout
[    9.393272] ufshcd-qcom 1d84000.ufshc: UFS Host state=1
[    9.403126] msm_dpu ae01000.display-controller: [drm:adreno_request_fw [msm]] *ERROR* failed to load a660_sqe.fw
[    9.408484] ufshcd-qcom 1d84000.ufshc: 0 outstanding reqs, tasks=0x0
[    9.425577] ufshcd-qcom 1d84000.ufshc: saved_err=0x0, saved_uic_err=0x0
[    9.432433] ufshcd-qcom 1d84000.ufshc: Device power mode=1, UIC link state=1
[    9.439716] ufshcd-qcom 1d84000.ufshc: PM in progress=0, sys. suspended=0
[    9.446742] ufshcd-qcom 1d84000.ufshc: Auto BKOPS=0, Host self-block=0
[    9.453468] ufshcd-qcom 1d84000.ufshc: Clk gate=1
...
[   10.529259] ufshcd-qcom 1d84000.ufshc: ufshcd_change_power_mode: power mode change failed -110
[   10.538102] ufshcd-qcom 1d84000.ufshc: ufshcd_scale_gear: failed err -110, old gear: (tx 1 rx 1), new gear: (tx 4 rx 4)
[   10.542841] WARNING: CPU: 0 PID: 274 at drivers/ufs/core/ufshcd.c:5504 ufshcd_sl_intr+0x64c/0x6a4
...

CI Run sample: https://git.codelinaro.org/linaro/qcomlt/ci/staging/cdba-tester/-/jobs/233352#L1479

Bisect run log:
# bad: [0af2f6be1b4281385b618cb86ad946eded089ac8] Linux 6.15-rc1
# good: [38fec10eb60d687e30c8c6b5420d86e8149f7557] Linux 6.14
git bisect start 'v6.15-rc1' 'v6.14'
# bad: [fd71def6d9abc5ae362fb9995d46049b7b0ed391] Merge tag 'for-6.15-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux
git bisect bad fd71def6d9abc5ae362fb9995d46049b7b0ed391
# good: [fb1ceb29b27cda91af35851ebab01f298d82162e] Merge tag 'platform-drivers-x86-v6.15-1' of git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86
git bisect good fb1ceb29b27cda91af35851ebab01f298d82162e
# good: [1952e19c02ae8ea0c663d30b19be14344b543068] Merge tag 'wireless-next-2025-03-20' of https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next
git bisect good 1952e19c02ae8ea0c663d30b19be14344b543068
# bad: [1a9239bb4253f9076b5b4b2a1a4e8d7defd77a95] Merge tag 'net-next-6.15' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next
git bisect bad 1a9239bb4253f9076b5b4b2a1a4e8d7defd77a95
# good: [22093997ac9220d3c606313efbf4ce564962d095] Merge tag 'ata-6.15-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/libata/linux
git bisect good 22093997ac9220d3c606313efbf4ce564962d095
# bad: [336b4dae6dfecc9aa53a3a68c71b9c1c1d466388] Merge tag 'iommu-updates-v6.15' of git://git.kernel.org/pub/scm/linux/kernel/git/iommu/linux
git bisect bad 336b4dae6dfecc9aa53a3a68c71b9c1c1d466388
# bad: [0711f1966a523d77d4c5f00776a7bd073d56251a] scsi: mpt3sas: Fix buffer overflow in mpt3sas_send_mctp_passthru_req()
git bisect bad 0711f1966a523d77d4c5f00776a7bd073d56251a
# good: [369552fd03f296261023872b8fc983d1fc55c8e9] Merge patch series "mpt3sas driver udpates"
git bisect good 369552fd03f296261023872b8fc983d1fc55c8e9
# bad: [42273e893157501ae119ea5459f3a7d2420c56d6] Merge patch series "scsi: scsi_debug: Add more tape support"
git bisect bad 42273e893157501ae119ea5459f3a7d2420c56d6
# bad: [7e72900272b61c11f2fd4020d4f186124d0d171b] Merge patch series "Support Multi-frequency scale for UFS"
git bisect bad 7e72900272b61c11f2fd4020d4f186124d0d171b
# good: [c02fe9e222d16bed8c270608c42f77bc62562ac3] scsi: ufs: qcom: Implement the freq_to_gear_speed() vop
git bisect good c02fe9e222d16bed8c270608c42f77bc62562ac3
# bad: [eff26ad4c34fc78303c14be749e10ca61c4d211f] scsi: ufs: core: Check if scaling up is required when disable clkscale
git bisect bad eff26ad4c34fc78303c14be749e10ca61c4d211f
# bad: [129b44c27c8a51cb74b2f68fde57f2a2e7f5696b] scsi: ufs: core: Enable multi-level gear scaling
git bisect bad 129b44c27c8a51cb74b2f68fde57f2a2e7f5696b
# first bad commit: [129b44c27c8a51cb74b2f68fde57f2a2e7f5696b] scsi: ufs: core: Enable multi-level gear scaling

#regzbot introduced 129b44c27c8a51cb74b2f68fde57f2a2e7f5696b

Thanks,
Neil

Re: [PATCH v5 5/8] scsi: ufs: core: Enable multi-level gear scaling

[Ziqi Chen]

Hi Neil,

We analyzed this issue today , I don't think it is related to this patch:

[    9.383728] ufshcd-qcom 1d84000.ufshc: pwr ctrl cmd 0x2 with mode 
0x11 completion timeout
...
...
...
[    9.588545] host_regs: 00000090: 00000002 15710000 00000002 00000003

The timeout error log shows the mode 0x11 is the correct argument3 value 
of this pwr mode DME_SET cmd.

int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
{
     struct uic_command uic_cmd = {
         .command = UIC_CMD_DME_SET,
         .argument1 = UIC_ARG_MIB(PA_PWRMODE),
         .argument3 = mode,
     };
...
...
  dev_err(hba->dev,
         "uic cmd 0x%x with arg3 0x%x completion timeout\n",
          uic_cmd->command, uic_cmd->argument3);


This prints means that we gave correct argument3 here.

But from the host register dump , we can see the argument3 written to 
register (address 0x***9C) is '0x00000003' which is a invalid value.

And according to the UFSHCI JEDEC, the return value of ConfigResultCode 
regiser (address 0x***0x98) is 0x00000002 also told us the value written 
to argument3 register is a INVALID_MIB_ATTRIBUTE_VALUE, this is the 
reason causes this timeout issue.

" 02h INVALID_MIB_ATTRIBUTE_VALUE "

However, though we don't know the final root cause, we can know there is 
mistake occur during writing register. The argument3 value is 0x11, but 
after writing to register , the readback value from register is 0x3... 
Anyway , this patch didn't touch the path of register writing.

Are you easy to reproduce this issue ? How many instances do you 
observed ? We never received similar report from our internal UFS test 
team and stabitily test team. If it is easy to reproduce , you can add 
readback prints at the place where after writing register to check it.

ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
{
     lockdep_assert_held(&hba->uic_cmd_mutex);

     WARN_ON(hba->active_uic_cmd);

     hba->active_uic_cmd = uic_cmd;

     /* Write Args */
     ufshcd_writel(hba, uic_cmd->argument1, REG_UIC_COMMAND_ARG_1);
     ufshcd_writel(hba, uic_cmd->argument2, REG_UIC_COMMAND_ARG_2);
     ufshcd_writel(hba, uic_cmd->argument3, REG_UIC_COMMAND_ARG_3);

-> you can add print here:
   pr_err ("READ BACK argument3 from register 0x%x: 0x%x",
       REG_UIC_COMMAND_ARG_3, ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3);



Best Regards,
Ziqi

On 4/24/2025 11:35 PM, Neil Armstrong wrote:
> Hi,
> 
> On 13/02/2025 09:00, Ziqi Chen wrote:
>> From: Can Guo <quic_cang@quicinc.com>
>>
>> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
>> plans. However, the gear speed is only toggled between min and max during
>> clock scaling. Enable multi-level gear scaling by mapping clock 
>> frequencies
>> to gear speeds, so that when devfreq scales clock frequencies we can put
>> the UFS link at the appropriate gear speeds accordingly.
>>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Reviewed-by: Bean Huo <beanhuo@micron.com>
>> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
>> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>
>> v1 -> v2:
>> Rename the lable "do_pmc" to "config_pwr_mode".
>>
>> v2 -> v3:
>> Use assignment instead memcpy() in function ufshcd_scale_gear().
>>
>> v3 -> v4:
>> Typo fixed for commit message.
>>
>> v4 -> v5:
>> Change the data type of "new_gear" from 'int' to 'u32'.
>> ---
>>   drivers/ufs/core/ufshcd.c | 44 ++++++++++++++++++++++++++++++---------
>>   1 file changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 8d295cc827cc..9908c0d6a1e1 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -1308,16 +1308,26 @@ static int ufshcd_wait_for_doorbell_clr(struct 
>> ufs_hba *hba,
>>   /**
>>    * ufshcd_scale_gear - scale up/down UFS gear
>>    * @hba: per adapter instance
>> + * @target_gear: target gear to scale to
>>    * @scale_up: True for scaling up gear and false for scaling down
>>    *
>>    * Return: 0 for success; -EBUSY if scaling can't happen at this time;
>>    * non-zero for any other errors.
>>    */
>> -static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>> +static int ufshcd_scale_gear(struct ufs_hba *hba, u32 target_gear, 
>> bool scale_up)
>>   {
>>       int ret = 0;
>>       struct ufs_pa_layer_attr new_pwr_info;
>> +    if (target_gear) {
>> +        new_pwr_info = hba->pwr_info;
>> +        new_pwr_info.gear_tx = target_gear;
>> +        new_pwr_info.gear_rx = target_gear;
>> +
>> +        goto config_pwr_mode;
>> +    }
>> +
>> +    /* Legacy gear scaling, in case vops_freq_to_gear_speed() is not 
>> implemented */
>>       if (scale_up) {
>>           memcpy(&new_pwr_info, &hba->clk_scaling.saved_pwr_info,
>>                  sizeof(struct ufs_pa_layer_attr));
>> @@ -1338,6 +1348,7 @@ static int ufshcd_scale_gear(struct ufs_hba 
>> *hba, bool scale_up)
>>           }
>>       }
>> +config_pwr_mode:
>>       /* check if the power mode needs to be changed or not? */
>>       ret = ufshcd_config_pwr_mode(hba, &new_pwr_info);
>>       if (ret)
>> @@ -1408,15 +1419,19 @@ static void 
>> ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool sc
>>   static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long 
>> freq,
>>                   bool scale_up)
>>   {
>> +    u32 old_gear = hba->pwr_info.gear_rx;
>> +    u32 new_gear = 0;
>>       int ret = 0;
>> +    new_gear = ufshcd_vops_freq_to_gear_speed(hba, freq);
>> +
>>       ret = ufshcd_clock_scaling_prepare(hba, 1 * USEC_PER_SEC);
>>       if (ret)
>>           return ret;
>>       /* scale down the gear before scaling down clocks */
>>       if (!scale_up) {
>> -        ret = ufshcd_scale_gear(hba, false);
>> +        ret = ufshcd_scale_gear(hba, new_gear, false);
>>           if (ret)
>>               goto out_unprepare;
>>       }
>> @@ -1424,13 +1439,13 @@ static int ufshcd_devfreq_scale(struct ufs_hba 
>> *hba, unsigned long freq,
>>       ret = ufshcd_scale_clks(hba, freq, scale_up);
>>       if (ret) {
>>           if (!scale_up)
>> -            ufshcd_scale_gear(hba, true);
>> +            ufshcd_scale_gear(hba, old_gear, true);
>>           goto out_unprepare;
>>       }
>>       /* scale up the gear after scaling up clocks */
>>       if (scale_up) {
>> -        ret = ufshcd_scale_gear(hba, true);
>> +        ret = ufshcd_scale_gear(hba, new_gear, true);
>>           if (ret) {
>>               ufshcd_scale_clks(hba, hba->devfreq->previous_freq,
>>                         false);
>> @@ -1723,6 +1738,8 @@ static ssize_t 
>> ufshcd_clkscale_enable_store(struct device *dev,
>>           struct device_attribute *attr, const char *buf, size_t count)
>>   {
>>       struct ufs_hba *hba = dev_get_drvdata(dev);
>> +    struct ufs_clk_info *clki;
>> +    unsigned long freq;
>>       u32 value;
>>       int err = 0;
>> @@ -1746,14 +1763,21 @@ static ssize_t 
>> ufshcd_clkscale_enable_store(struct device *dev,
>>       if (value) {
>>           ufshcd_resume_clkscaling(hba);
>> -    } else {
>> -        ufshcd_suspend_clkscaling(hba);
>> -        err = ufshcd_devfreq_scale(hba, ULONG_MAX, true);
>> -        if (err)
>> -            dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
>> -                    __func__, err);
>> +        goto out_rel;
>>       }
>> +    clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, 
>> list);
>> +    freq = clki->max_freq;
>> +
>> +    ufshcd_suspend_clkscaling(hba);
>> +    err = ufshcd_devfreq_scale(hba, freq, true);
>> +    if (err)
>> +        dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
>> +                __func__, err);
>> +    else
>> +        hba->clk_scaling.target_freq = freq;
>> +
>> +out_rel:
>>       ufshcd_release(hba);
>>       ufshcd_rpm_put_sync(hba);
>>   out:
> 
> This change causes UFS crash on the RB3gen2, after UFS successfully 
> probe and scan:
> 
> [    9.383728] ufshcd-qcom 1d84000.ufshc: pwr ctrl cmd 0x2 with mode 
> 0x11 completion timeout
> [    9.393272] ufshcd-qcom 1d84000.ufshc: UFS Host state=1
> [    9.403126] msm_dpu ae01000.display-controller: 
> [drm:adreno_request_fw [msm]] *ERROR* failed to load a660_sqe.fw
> [    9.408484] ufshcd-qcom 1d84000.ufshc: 0 outstanding reqs, tasks=0x0
> [    9.425577] ufshcd-qcom 1d84000.ufshc: saved_err=0x0, saved_uic_err=0x0
> [    9.432433] ufshcd-qcom 1d84000.ufshc: Device power mode=1, UIC link 
> state=1
> [    9.439716] ufshcd-qcom 1d84000.ufshc: PM in progress=0, sys. 
> suspended=0
> [    9.446742] ufshcd-qcom 1d84000.ufshc: Auto BKOPS=0, Host self-block=0
> [    9.453468] ufshcd-qcom 1d84000.ufshc: Clk gate=1
> ...
> [   10.529259] ufshcd-qcom 1d84000.ufshc: ufshcd_change_power_mode: 
> power mode change failed -110
> [   10.538102] ufshcd-qcom 1d84000.ufshc: ufshcd_scale_gear: failed err 
> -110, old gear: (tx 1 rx 1), new gear: (tx 4 rx 4)
> [   10.542841] WARNING: CPU: 0 PID: 274 at drivers/ufs/core/ 
> ufshcd.c:5504 ufshcd_sl_intr+0x64c/0x6a4
> ...
> 
> CI Run sample: https://git.codelinaro.org/linaro/qcomlt/ci/staging/cdba- 
> tester/-/jobs/233352#L1479
> 
> Bisect run log:
> # bad: [0af2f6be1b4281385b618cb86ad946eded089ac8] Linux 6.15-rc1
> # good: [38fec10eb60d687e30c8c6b5420d86e8149f7557] Linux 6.14
> git bisect start 'v6.15-rc1' 'v6.14'
> # bad: [fd71def6d9abc5ae362fb9995d46049b7b0ed391] Merge tag 'for-6.15- 
> tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux
> git bisect bad fd71def6d9abc5ae362fb9995d46049b7b0ed391
> # good: [fb1ceb29b27cda91af35851ebab01f298d82162e] Merge tag 'platform- 
> drivers-x86-v6.15-1' of git://git.kernel.org/pub/scm/linux/kernel/git/ 
> pdx86/platform-drivers-x86
> git bisect good fb1ceb29b27cda91af35851ebab01f298d82162e
> # good: [1952e19c02ae8ea0c663d30b19be14344b543068] Merge tag 'wireless- 
> next-2025-03-20' of https://git.kernel.org/pub/scm/linux/kernel/git/ 
> wireless/wireless-next
> git bisect good 1952e19c02ae8ea0c663d30b19be14344b543068
> # bad: [1a9239bb4253f9076b5b4b2a1a4e8d7defd77a95] Merge tag 'net- 
> next-6.15' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next
> git bisect bad 1a9239bb4253f9076b5b4b2a1a4e8d7defd77a95
> # good: [22093997ac9220d3c606313efbf4ce564962d095] Merge tag 'ata-6.15- 
> rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/libata/linux
> git bisect good 22093997ac9220d3c606313efbf4ce564962d095
> # bad: [336b4dae6dfecc9aa53a3a68c71b9c1c1d466388] Merge tag 'iommu- 
> updates-v6.15' of git://git.kernel.org/pub/scm/linux/kernel/git/iommu/linux
> git bisect bad 336b4dae6dfecc9aa53a3a68c71b9c1c1d466388
> # bad: [0711f1966a523d77d4c5f00776a7bd073d56251a] scsi: mpt3sas: Fix 
> buffer overflow in mpt3sas_send_mctp_passthru_req()
> git bisect bad 0711f1966a523d77d4c5f00776a7bd073d56251a
> # good: [369552fd03f296261023872b8fc983d1fc55c8e9] Merge patch series 
> "mpt3sas driver udpates"
> git bisect good 369552fd03f296261023872b8fc983d1fc55c8e9
> # bad: [42273e893157501ae119ea5459f3a7d2420c56d6] Merge patch series 
> "scsi: scsi_debug: Add more tape support"
> git bisect bad 42273e893157501ae119ea5459f3a7d2420c56d6
> # bad: [7e72900272b61c11f2fd4020d4f186124d0d171b] Merge patch series 
> "Support Multi-frequency scale for UFS"
> git bisect bad 7e72900272b61c11f2fd4020d4f186124d0d171b
> # good: [c02fe9e222d16bed8c270608c42f77bc62562ac3] scsi: ufs: qcom: 
> Implement the freq_to_gear_speed() vop
> git bisect good c02fe9e222d16bed8c270608c42f77bc62562ac3
> # bad: [eff26ad4c34fc78303c14be749e10ca61c4d211f] scsi: ufs: core: Check 
> if scaling up is required when disable clkscale
> git bisect bad eff26ad4c34fc78303c14be749e10ca61c4d211f
> # bad: [129b44c27c8a51cb74b2f68fde57f2a2e7f5696b] scsi: ufs: core: 
> Enable multi-level gear scaling
> git bisect bad 129b44c27c8a51cb74b2f68fde57f2a2e7f5696b
> # first bad commit: [129b44c27c8a51cb74b2f68fde57f2a2e7f5696b] scsi: 
> ufs: core: Enable multi-level gear scaling
> 
> #regzbot introduced 129b44c27c8a51cb74b2f68fde57f2a2e7f5696b
> 
> Thanks,
> Neil
> 

Re: [PATCH v5 5/8] scsi: ufs: core: Enable multi-level gear scaling

[neil.armstrong]

Hi,

On 25/04/2025 09:29, Ziqi Chen wrote:
> Hi Neil,
> 
> We analyzed this issue today , I don't think it is related to this patch:

The bisect point to the patch where the issue appears, it may be another one of this patchset.

> 
> [    9.383728] ufshcd-qcom 1d84000.ufshc: pwr ctrl cmd 0x2 with mode 0x11 completion timeout
> ...
> ...
> ...
> [    9.588545] host_regs: 00000090: 00000002 15710000 00000002 00000003
> 
> The timeout error log shows the mode 0x11 is the correct argument3 value of this pwr mode DME_SET cmd.
> 
> int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
> {
>      struct uic_command uic_cmd = {
>          .command = UIC_CMD_DME_SET,
>          .argument1 = UIC_ARG_MIB(PA_PWRMODE),
>          .argument3 = mode,
>      };
> ...
> ...
>   dev_err(hba->dev,
>          "uic cmd 0x%x with arg3 0x%x completion timeout\n",
>           uic_cmd->command, uic_cmd->argument3);
> 
> 
> This prints means that we gave correct argument3 here.
> 
> But from the host register dump , we can see the argument3 written to register (address 0x***9C) is '0x00000003' which is a invalid value.
> 
> And according to the UFSHCI JEDEC, the return value of ConfigResultCode regiser (address 0x***0x98) is 0x00000002 also told us the value written to argument3 register is a INVALID_MIB_ATTRIBUTE_VALUE, this is the reason causes this timeout issue.
> 
> " 02h INVALID_MIB_ATTRIBUTE_VALUE "
> 
> However, though we don't know the final root cause, we can know there is mistake occur during writing register. The argument3 value is 0x11, but after writing to register , the readback value from register is 0x3... Anyway , this patch didn't touch the path of register writing.
> 
> Are you easy to reproduce this issue ? How many instances do you observed ? We never received similar report from our internal UFS test team and stabitily test team. If it is easy to reproduce , you can add readback prints at the place where after writing register to check it.

The issue is easy to reproduce, just boot the rb3gen2 with vanilla v6.15-rc1 and default defconfig.

> 
> ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
> {
>      lockdep_assert_held(&hba->uic_cmd_mutex);
> 
>      WARN_ON(hba->active_uic_cmd);
> 
>      hba->active_uic_cmd = uic_cmd;
> 
>      /* Write Args */
>      ufshcd_writel(hba, uic_cmd->argument1, REG_UIC_COMMAND_ARG_1);
>      ufshcd_writel(hba, uic_cmd->argument2, REG_UIC_COMMAND_ARG_2);
>      ufshcd_writel(hba, uic_cmd->argument3, REG_UIC_COMMAND_ARG_3);
> 
> -> you can add print here:
>    pr_err ("READ BACK argument3 from register 0x%x: 0x%x",
>        REG_UIC_COMMAND_ARG_3, ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3);

I've run the kernel with:
=====================><=================================
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0534390c2a35..232328ff6365 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2494,6 +2494,14 @@ ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
         ufshcd_writel(hba, uic_cmd->argument2, REG_UIC_COMMAND_ARG_2);
         ufshcd_writel(hba, uic_cmd->argument3, REG_UIC_COMMAND_ARG_3);

+       {
+               uint32_t reg3 = ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3);
+
+               if (reg3 != uic_cmd->argument3)
+                       pr_err("different READ BACK argument3 from register 0x% != 0x%x",
+                               uic_cmd->argument3, reg3);
+       }
+
         ufshcd_add_uic_command_trace(hba, uic_cmd, UFS_CMD_SEND);

         /* Write UIC Cmd */
=====================><=================================

And this print never appears.

But the log clearly shows the kernel tries to scale back from gear1 to gear4:

 >> [   10.538102] ufshcd-qcom 1d84000.ufshc: ufshcd_scale_gear: failed err -110, old gear: (tx 1 rx 1), new gear: (tx 4 rx 4)

did you validate the gear scaling on the sc7280 with the mainline UFS & PHY drivers ?

Neil

> 
> 
> 
> Best Regards,
> Ziqi
> 
> On 4/24/2025 11:35 PM, Neil Armstrong wrote:
>> Hi,
>>
>> On 13/02/2025 09:00, Ziqi Chen wrote:
>>> From: Can Guo <quic_cang@quicinc.com>
>>>
>>> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
>>> plans. However, the gear speed is only toggled between min and max during
>>> clock scaling. Enable multi-level gear scaling by mapping clock frequencies
>>> to gear speeds, so that when devfreq scales clock frequencies we can put
>>> the UFS link at the appropriate gear speeds accordingly.
>>>
>>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>> Reviewed-by: Bean Huo <beanhuo@micron.com>
>>> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
>>> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> ---
>>>
>>> v1 -> v2:
>>> Rename the lable "do_pmc" to "config_pwr_mode".
>>>
>>> v2 -> v3:
>>> Use assignment instead memcpy() in function ufshcd_scale_gear().
>>>
>>> v3 -> v4:
>>> Typo fixed for commit message.
>>>
>>> v4 -> v5:
>>> Change the data type of "new_gear" from 'int' to 'u32'.
>>> ---
>>>   drivers/ufs/core/ufshcd.c | 44 ++++++++++++++++++++++++++++++---------
>>>   1 file changed, 34 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>>> index 8d295cc827cc..9908c0d6a1e1 100644
>>> --- a/drivers/ufs/core/ufshcd.c
>>> +++ b/drivers/ufs/core/ufshcd.c
>>> @@ -1308,16 +1308,26 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
>>>   /**
>>>    * ufshcd_scale_gear - scale up/down UFS gear
>>>    * @hba: per adapter instance
>>> + * @target_gear: target gear to scale to
>>>    * @scale_up: True for scaling up gear and false for scaling down
>>>    *
>>>    * Return: 0 for success; -EBUSY if scaling can't happen at this time;
>>>    * non-zero for any other errors.
>>>    */
>>> -static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>>> +static int ufshcd_scale_gear(struct ufs_hba *hba, u32 target_gear, bool scale_up)
>>>   {
>>>       int ret = 0;
>>>       struct ufs_pa_layer_attr new_pwr_info;
>>> +    if (target_gear) {
>>> +        new_pwr_info = hba->pwr_info;
>>> +        new_pwr_info.gear_tx = target_gear;
>>> +        new_pwr_info.gear_rx = target_gear;
>>> +
>>> +        goto config_pwr_mode;
>>> +    }
>>> +
>>> +    /* Legacy gear scaling, in case vops_freq_to_gear_speed() is not implemented */
>>>       if (scale_up) {
>>>           memcpy(&new_pwr_info, &hba->clk_scaling.saved_pwr_info,
>>>                  sizeof(struct ufs_pa_layer_attr));
>>> @@ -1338,6 +1348,7 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>>>           }
>>>       }
>>> +config_pwr_mode:
>>>       /* check if the power mode needs to be changed or not? */
>>>       ret = ufshcd_config_pwr_mode(hba, &new_pwr_info);
>>>       if (ret)
>>> @@ -1408,15 +1419,19 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool sc
>>>   static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
>>>                   bool scale_up)
>>>   {
>>> +    u32 old_gear = hba->pwr_info.gear_rx;
>>> +    u32 new_gear = 0;
>>>       int ret = 0;
>>> +    new_gear = ufshcd_vops_freq_to_gear_speed(hba, freq);
>>> +
>>>       ret = ufshcd_clock_scaling_prepare(hba, 1 * USEC_PER_SEC);
>>>       if (ret)
>>>           return ret;
>>>       /* scale down the gear before scaling down clocks */
>>>       if (!scale_up) {
>>> -        ret = ufshcd_scale_gear(hba, false);
>>> +        ret = ufshcd_scale_gear(hba, new_gear, false);
>>>           if (ret)
>>>               goto out_unprepare;
>>>       }
>>> @@ -1424,13 +1439,13 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
>>>       ret = ufshcd_scale_clks(hba, freq, scale_up);
>>>       if (ret) {
>>>           if (!scale_up)
>>> -            ufshcd_scale_gear(hba, true);
>>> +            ufshcd_scale_gear(hba, old_gear, true);
>>>           goto out_unprepare;
>>>       }
>>>       /* scale up the gear after scaling up clocks */
>>>       if (scale_up) {
>>> -        ret = ufshcd_scale_gear(hba, true);
>>> +        ret = ufshcd_scale_gear(hba, new_gear, true);
>>>           if (ret) {
>>>               ufshcd_scale_clks(hba, hba->devfreq->previous_freq,
>>>                         false);
>>> @@ -1723,6 +1738,8 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
>>>           struct device_attribute *attr, const char *buf, size_t count)
>>>   {
>>>       struct ufs_hba *hba = dev_get_drvdata(dev);
>>> +    struct ufs_clk_info *clki;
>>> +    unsigned long freq;
>>>       u32 value;
>>>       int err = 0;
>>> @@ -1746,14 +1763,21 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
>>>       if (value) {
>>>           ufshcd_resume_clkscaling(hba);
>>> -    } else {
>>> -        ufshcd_suspend_clkscaling(hba);
>>> -        err = ufshcd_devfreq_scale(hba, ULONG_MAX, true);
>>> -        if (err)
>>> -            dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
>>> -                    __func__, err);
>>> +        goto out_rel;
>>>       }
>>> +    clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
>>> +    freq = clki->max_freq;
>>> +
>>> +    ufshcd_suspend_clkscaling(hba);
>>> +    err = ufshcd_devfreq_scale(hba, freq, true);
>>> +    if (err)
>>> +        dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
>>> +                __func__, err);
>>> +    else
>>> +        hba->clk_scaling.target_freq = freq;
>>> +
>>> +out_rel:
>>>       ufshcd_release(hba);
>>>       ufshcd_rpm_put_sync(hba);
>>>   out:
>>
>> This change causes UFS crash on the RB3gen2, after UFS successfully probe and scan:
>>
>> [    9.383728] ufshcd-qcom 1d84000.ufshc: pwr ctrl cmd 0x2 with mode 0x11 completion timeout
>> [    9.393272] ufshcd-qcom 1d84000.ufshc: UFS Host state=1
>> [    9.403126] msm_dpu ae01000.display-controller: [drm:adreno_request_fw [msm]] *ERROR* failed to load a660_sqe.fw
>> [    9.408484] ufshcd-qcom 1d84000.ufshc: 0 outstanding reqs, tasks=0x0
>> [    9.425577] ufshcd-qcom 1d84000.ufshc: saved_err=0x0, saved_uic_err=0x0
>> [    9.432433] ufshcd-qcom 1d84000.ufshc: Device power mode=1, UIC link state=1
>> [    9.439716] ufshcd-qcom 1d84000.ufshc: PM in progress=0, sys. suspended=0
>> [    9.446742] ufshcd-qcom 1d84000.ufshc: Auto BKOPS=0, Host self-block=0
>> [    9.453468] ufshcd-qcom 1d84000.ufshc: Clk gate=1
>> ...
>> [   10.529259] ufshcd-qcom 1d84000.ufshc: ufshcd_change_power_mode: power mode change failed -110
>> [   10.538102] ufshcd-qcom 1d84000.ufshc: ufshcd_scale_gear: failed err -110, old gear: (tx 1 rx 1), new gear: (tx 4 rx 4)
>> [   10.542841] WARNING: CPU: 0 PID: 274 at drivers/ufs/core/ ufshcd.c:5504 ufshcd_sl_intr+0x64c/0x6a4
>> ...
>>
>> CI Run sample: https://git.codelinaro.org/linaro/qcomlt/ci/staging/cdba- tester/-/jobs/233352#L1479
>>
>> Bisect run log:
>> # bad: [0af2f6be1b4281385b618cb86ad946eded089ac8] Linux 6.15-rc1
>> # good: [38fec10eb60d687e30c8c6b5420d86e8149f7557] Linux 6.14
>> git bisect start 'v6.15-rc1' 'v6.14'
>> # bad: [fd71def6d9abc5ae362fb9995d46049b7b0ed391] Merge tag 'for-6.15- tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux
>> git bisect bad fd71def6d9abc5ae362fb9995d46049b7b0ed391
>> # good: [fb1ceb29b27cda91af35851ebab01f298d82162e] Merge tag 'platform- drivers-x86-v6.15-1' of git://git.kernel.org/pub/scm/linux/kernel/git/ pdx86/platform-drivers-x86
>> git bisect good fb1ceb29b27cda91af35851ebab01f298d82162e
>> # good: [1952e19c02ae8ea0c663d30b19be14344b543068] Merge tag 'wireless- next-2025-03-20' of https://git.kernel.org/pub/scm/linux/kernel/git/ wireless/wireless-next
>> git bisect good 1952e19c02ae8ea0c663d30b19be14344b543068
>> # bad: [1a9239bb4253f9076b5b4b2a1a4e8d7defd77a95] Merge tag 'net- next-6.15' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next
>> git bisect bad 1a9239bb4253f9076b5b4b2a1a4e8d7defd77a95
>> # good: [22093997ac9220d3c606313efbf4ce564962d095] Merge tag 'ata-6.15- rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/libata/linux
>> git bisect good 22093997ac9220d3c606313efbf4ce564962d095
>> # bad: [336b4dae6dfecc9aa53a3a68c71b9c1c1d466388] Merge tag 'iommu- updates-v6.15' of git://git.kernel.org/pub/scm/linux/kernel/git/iommu/linux
>> git bisect bad 336b4dae6dfecc9aa53a3a68c71b9c1c1d466388
>> # bad: [0711f1966a523d77d4c5f00776a7bd073d56251a] scsi: mpt3sas: Fix buffer overflow in mpt3sas_send_mctp_passthru_req()
>> git bisect bad 0711f1966a523d77d4c5f00776a7bd073d56251a
>> # good: [369552fd03f296261023872b8fc983d1fc55c8e9] Merge patch series "mpt3sas driver udpates"
>> git bisect good 369552fd03f296261023872b8fc983d1fc55c8e9
>> # bad: [42273e893157501ae119ea5459f3a7d2420c56d6] Merge patch series "scsi: scsi_debug: Add more tape support"
>> git bisect bad 42273e893157501ae119ea5459f3a7d2420c56d6
>> # bad: [7e72900272b61c11f2fd4020d4f186124d0d171b] Merge patch series "Support Multi-frequency scale for UFS"
>> git bisect bad 7e72900272b61c11f2fd4020d4f186124d0d171b
>> # good: [c02fe9e222d16bed8c270608c42f77bc62562ac3] scsi: ufs: qcom: Implement the freq_to_gear_speed() vop
>> git bisect good c02fe9e222d16bed8c270608c42f77bc62562ac3
>> # bad: [eff26ad4c34fc78303c14be749e10ca61c4d211f] scsi: ufs: core: Check if scaling up is required when disable clkscale
>> git bisect bad eff26ad4c34fc78303c14be749e10ca61c4d211f
>> # bad: [129b44c27c8a51cb74b2f68fde57f2a2e7f5696b] scsi: ufs: core: Enable multi-level gear scaling
>> git bisect bad 129b44c27c8a51cb74b2f68fde57f2a2e7f5696b
>> # first bad commit: [129b44c27c8a51cb74b2f68fde57f2a2e7f5696b] scsi: ufs: core: Enable multi-level gear scaling
>>
>> #regzbot introduced 129b44c27c8a51cb74b2f68fde57f2a2e7f5696b
>>
>> Thanks,
>> Neil
>>
> 

Re: [PATCH v5 5/8] scsi: ufs: core: Enable multi-level gear scaling

[Ziqi Chen]

[-- Attachment #1: Type: text/plain, Size: 15593 bytes --]

Hi Neil,

Thanks for report~
I got one rb3gen2 device and be able to reproduce this issue today.
I made a change to fix this corner case. It already been verifed from
my side. Could also help double check attached patch whether can fix 
this issue from your side?

Thanks,
Ziqi

On 4/25/2025 3:43 PM, neil.armstrong@linaro.org wrote:
> Hi,
> 
> On 25/04/2025 09:29, Ziqi Chen wrote:
>> Hi Neil,
>>
>> We analyzed this issue today , I don't think it is related to this patch:
> 
> The bisect point to the patch where the issue appears, it may be another 
> one of this patchset.
> 
>>
>> [    9.383728] ufshcd-qcom 1d84000.ufshc: pwr ctrl cmd 0x2 with mode 
>> 0x11 completion timeout
>> ...
>> ...
>> ...
>> [    9.588545] host_regs: 00000090: 00000002 15710000 00000002 00000003
>>
>> The timeout error log shows the mode 0x11 is the correct argument3 
>> value of this pwr mode DME_SET cmd.
>>
>> int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
>> {
>>      struct uic_command uic_cmd = {
>>          .command = UIC_CMD_DME_SET,
>>          .argument1 = UIC_ARG_MIB(PA_PWRMODE),
>>          .argument3 = mode,
>>      };
>> ...
>> ...
>>   dev_err(hba->dev,
>>          "uic cmd 0x%x with arg3 0x%x completion timeout\n",
>>           uic_cmd->command, uic_cmd->argument3);
>>
>>
>> This prints means that we gave correct argument3 here.
>>
>> But from the host register dump , we can see the argument3 written to 
>> register (address 0x***9C) is '0x00000003' which is a invalid value.
>>
>> And according to the UFSHCI JEDEC, the return value of 
>> ConfigResultCode regiser (address 0x***0x98) is 0x00000002 also told 
>> us the value written to argument3 register is a 
>> INVALID_MIB_ATTRIBUTE_VALUE, this is the reason causes this timeout 
>> issue.
>>
>> " 02h INVALID_MIB_ATTRIBUTE_VALUE "
>>
>> However, though we don't know the final root cause, we can know there 
>> is mistake occur during writing register. The argument3 value is 0x11, 
>> but after writing to register , the readback value from register is 
>> 0x3... Anyway , this patch didn't touch the path of register writing.
>>
>> Are you easy to reproduce this issue ? How many instances do you 
>> observed ? We never received similar report from our internal UFS test 
>> team and stabitily test team. If it is easy to reproduce , you can add 
>> readback prints at the place where after writing register to check it.
> 
> The issue is easy to reproduce, just boot the rb3gen2 with vanilla 
> v6.15-rc1 and default defconfig.
> 
>>
>> ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>> {
>>      lockdep_assert_held(&hba->uic_cmd_mutex);
>>
>>      WARN_ON(hba->active_uic_cmd);
>>
>>      hba->active_uic_cmd = uic_cmd;
>>
>>      /* Write Args */
>>      ufshcd_writel(hba, uic_cmd->argument1, REG_UIC_COMMAND_ARG_1);
>>      ufshcd_writel(hba, uic_cmd->argument2, REG_UIC_COMMAND_ARG_2);
>>      ufshcd_writel(hba, uic_cmd->argument3, REG_UIC_COMMAND_ARG_3);
>>
>> -> you can add print here:
>>    pr_err ("READ BACK argument3 from register 0x%x: 0x%x",
>>        REG_UIC_COMMAND_ARG_3, ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3);
> 
> I've run the kernel with:
> =====================><=================================
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0534390c2a35..232328ff6365 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2494,6 +2494,14 @@ ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, 
> struct uic_command *uic_cmd)
>          ufshcd_writel(hba, uic_cmd->argument2, REG_UIC_COMMAND_ARG_2);
>          ufshcd_writel(hba, uic_cmd->argument3, REG_UIC_COMMAND_ARG_3);
> 
> +       {
> +               uint32_t reg3 = ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3);
> +
> +               if (reg3 != uic_cmd->argument3)
> +                       pr_err("different READ BACK argument3 from 
> register 0x% != 0x%x",
> +                               uic_cmd->argument3, reg3);
> +       }
> +
>          ufshcd_add_uic_command_trace(hba, uic_cmd, UFS_CMD_SEND);
> 
>          /* Write UIC Cmd */
> =====================><=================================
> 
> And this print never appears.
> 
> But the log clearly shows the kernel tries to scale back from gear1 to 
> gear4:
> 
>  >> [   10.538102] ufshcd-qcom 1d84000.ufshc: ufshcd_scale_gear: failed 
> err -110, old gear: (tx 1 rx 1), new gear: (tx 4 rx 4)
> 
> did you validate the gear scaling on the sc7280 with the mainline UFS & 
> PHY drivers ?
> 
> Neil
> 
>>
>>
>>
>> Best Regards,
>> Ziqi
>>
>> On 4/24/2025 11:35 PM, Neil Armstrong wrote:
>>> Hi,
>>>
>>> On 13/02/2025 09:00, Ziqi Chen wrote:
>>>> From: Can Guo <quic_cang@quicinc.com>
>>>>
>>>> With OPP V2 enabled, devfreq can scale clocks amongst multiple 
>>>> frequency
>>>> plans. However, the gear speed is only toggled between min and max 
>>>> during
>>>> clock scaling. Enable multi-level gear scaling by mapping clock 
>>>> frequencies
>>>> to gear speeds, so that when devfreq scales clock frequencies we can 
>>>> put
>>>> the UFS link at the appropriate gear speeds accordingly.
>>>>
>>>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>>>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>> Reviewed-by: Bean Huo <beanhuo@micron.com>
>>>> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
>>>> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>> Rename the lable "do_pmc" to "config_pwr_mode".
>>>>
>>>> v2 -> v3:
>>>> Use assignment instead memcpy() in function ufshcd_scale_gear().
>>>>
>>>> v3 -> v4:
>>>> Typo fixed for commit message.
>>>>
>>>> v4 -> v5:
>>>> Change the data type of "new_gear" from 'int' to 'u32'.
>>>> ---
>>>>   drivers/ufs/core/ufshcd.c | 44 +++++++++++++++++++++++++++++ 
>>>> +---------
>>>>   1 file changed, 34 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>>>> index 8d295cc827cc..9908c0d6a1e1 100644
>>>> --- a/drivers/ufs/core/ufshcd.c
>>>> +++ b/drivers/ufs/core/ufshcd.c
>>>> @@ -1308,16 +1308,26 @@ static int 
>>>> ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
>>>>   /**
>>>>    * ufshcd_scale_gear - scale up/down UFS gear
>>>>    * @hba: per adapter instance
>>>> + * @target_gear: target gear to scale to
>>>>    * @scale_up: True for scaling up gear and false for scaling down
>>>>    *
>>>>    * Return: 0 for success; -EBUSY if scaling can't happen at this 
>>>> time;
>>>>    * non-zero for any other errors.
>>>>    */
>>>> -static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>>>> +static int ufshcd_scale_gear(struct ufs_hba *hba, u32 target_gear, 
>>>> bool scale_up)
>>>>   {
>>>>       int ret = 0;
>>>>       struct ufs_pa_layer_attr new_pwr_info;
>>>> +    if (target_gear) {
>>>> +        new_pwr_info = hba->pwr_info;
>>>> +        new_pwr_info.gear_tx = target_gear;
>>>> +        new_pwr_info.gear_rx = target_gear;
>>>> +
>>>> +        goto config_pwr_mode;
>>>> +    }
>>>> +
>>>> +    /* Legacy gear scaling, in case vops_freq_to_gear_speed() is 
>>>> not implemented */
>>>>       if (scale_up) {
>>>>           memcpy(&new_pwr_info, &hba->clk_scaling.saved_pwr_info,
>>>>                  sizeof(struct ufs_pa_layer_attr));
>>>> @@ -1338,6 +1348,7 @@ static int ufshcd_scale_gear(struct ufs_hba 
>>>> *hba, bool scale_up)
>>>>           }
>>>>       }
>>>> +config_pwr_mode:
>>>>       /* check if the power mode needs to be changed or not? */
>>>>       ret = ufshcd_config_pwr_mode(hba, &new_pwr_info);
>>>>       if (ret)
>>>> @@ -1408,15 +1419,19 @@ static void 
>>>> ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool sc
>>>>   static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long 
>>>> freq,
>>>>                   bool scale_up)
>>>>   {
>>>> +    u32 old_gear = hba->pwr_info.gear_rx;
>>>> +    u32 new_gear = 0;
>>>>       int ret = 0;
>>>> +    new_gear = ufshcd_vops_freq_to_gear_speed(hba, freq);
>>>> +
>>>>       ret = ufshcd_clock_scaling_prepare(hba, 1 * USEC_PER_SEC);
>>>>       if (ret)
>>>>           return ret;
>>>>       /* scale down the gear before scaling down clocks */
>>>>       if (!scale_up) {
>>>> -        ret = ufshcd_scale_gear(hba, false);
>>>> +        ret = ufshcd_scale_gear(hba, new_gear, false);
>>>>           if (ret)
>>>>               goto out_unprepare;
>>>>       }
>>>> @@ -1424,13 +1439,13 @@ static int ufshcd_devfreq_scale(struct 
>>>> ufs_hba *hba, unsigned long freq,
>>>>       ret = ufshcd_scale_clks(hba, freq, scale_up);
>>>>       if (ret) {
>>>>           if (!scale_up)
>>>> -            ufshcd_scale_gear(hba, true);
>>>> +            ufshcd_scale_gear(hba, old_gear, true);
>>>>           goto out_unprepare;
>>>>       }
>>>>       /* scale up the gear after scaling up clocks */
>>>>       if (scale_up) {
>>>> -        ret = ufshcd_scale_gear(hba, true);
>>>> +        ret = ufshcd_scale_gear(hba, new_gear, true);
>>>>           if (ret) {
>>>>               ufshcd_scale_clks(hba, hba->devfreq->previous_freq,
>>>>                         false);
>>>> @@ -1723,6 +1738,8 @@ static ssize_t 
>>>> ufshcd_clkscale_enable_store(struct device *dev,
>>>>           struct device_attribute *attr, const char *buf, size_t count)
>>>>   {
>>>>       struct ufs_hba *hba = dev_get_drvdata(dev);
>>>> +    struct ufs_clk_info *clki;
>>>> +    unsigned long freq;
>>>>       u32 value;
>>>>       int err = 0;
>>>> @@ -1746,14 +1763,21 @@ static ssize_t 
>>>> ufshcd_clkscale_enable_store(struct device *dev,
>>>>       if (value) {
>>>>           ufshcd_resume_clkscaling(hba);
>>>> -    } else {
>>>> -        ufshcd_suspend_clkscaling(hba);
>>>> -        err = ufshcd_devfreq_scale(hba, ULONG_MAX, true);
>>>> -        if (err)
>>>> -            dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
>>>> -                    __func__, err);
>>>> +        goto out_rel;
>>>>       }
>>>> +    clki = list_first_entry(&hba->clk_list_head, struct 
>>>> ufs_clk_info, list);
>>>> +    freq = clki->max_freq;
>>>> +
>>>> +    ufshcd_suspend_clkscaling(hba);
>>>> +    err = ufshcd_devfreq_scale(hba, freq, true);
>>>> +    if (err)
>>>> +        dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
>>>> +                __func__, err);
>>>> +    else
>>>> +        hba->clk_scaling.target_freq = freq;
>>>> +
>>>> +out_rel:
>>>>       ufshcd_release(hba);
>>>>       ufshcd_rpm_put_sync(hba);
>>>>   out:
>>>
>>> This change causes UFS crash on the RB3gen2, after UFS successfully 
>>> probe and scan:
>>>
>>> [    9.383728] ufshcd-qcom 1d84000.ufshc: pwr ctrl cmd 0x2 with mode 
>>> 0x11 completion timeout
>>> [    9.393272] ufshcd-qcom 1d84000.ufshc: UFS Host state=1
>>> [    9.403126] msm_dpu ae01000.display-controller: 
>>> [drm:adreno_request_fw [msm]] *ERROR* failed to load a660_sqe.fw
>>> [    9.408484] ufshcd-qcom 1d84000.ufshc: 0 outstanding reqs, tasks=0x0
>>> [    9.425577] ufshcd-qcom 1d84000.ufshc: saved_err=0x0, 
>>> saved_uic_err=0x0
>>> [    9.432433] ufshcd-qcom 1d84000.ufshc: Device power mode=1, UIC 
>>> link state=1
>>> [    9.439716] ufshcd-qcom 1d84000.ufshc: PM in progress=0, sys. 
>>> suspended=0
>>> [    9.446742] ufshcd-qcom 1d84000.ufshc: Auto BKOPS=0, Host self- 
>>> block=0
>>> [    9.453468] ufshcd-qcom 1d84000.ufshc: Clk gate=1
>>> ...
>>> [   10.529259] ufshcd-qcom 1d84000.ufshc: ufshcd_change_power_mode: 
>>> power mode change failed -110
>>> [   10.538102] ufshcd-qcom 1d84000.ufshc: ufshcd_scale_gear: failed 
>>> err -110, old gear: (tx 1 rx 1), new gear: (tx 4 rx 4)
>>> [   10.542841] WARNING: CPU: 0 PID: 274 at drivers/ufs/core/ 
>>> ufshcd.c:5504 ufshcd_sl_intr+0x64c/0x6a4
>>> ...
>>>
>>> CI Run sample: https://git.codelinaro.org/linaro/qcomlt/ci/staging/ 
>>> cdba- tester/-/jobs/233352#L1479
>>>
>>> Bisect run log:
>>> # bad: [0af2f6be1b4281385b618cb86ad946eded089ac8] Linux 6.15-rc1
>>> # good: [38fec10eb60d687e30c8c6b5420d86e8149f7557] Linux 6.14
>>> git bisect start 'v6.15-rc1' 'v6.14'
>>> # bad: [fd71def6d9abc5ae362fb9995d46049b7b0ed391] Merge tag 
>>> 'for-6.15- tag' of git://git.kernel.org/pub/scm/linux/kernel/git/ 
>>> kdave/linux
>>> git bisect bad fd71def6d9abc5ae362fb9995d46049b7b0ed391
>>> # good: [fb1ceb29b27cda91af35851ebab01f298d82162e] Merge tag 
>>> 'platform- drivers-x86-v6.15-1' of git://git.kernel.org/pub/scm/ 
>>> linux/kernel/git/ pdx86/platform-drivers-x86
>>> git bisect good fb1ceb29b27cda91af35851ebab01f298d82162e
>>> # good: [1952e19c02ae8ea0c663d30b19be14344b543068] Merge tag 
>>> 'wireless- next-2025-03-20' of https://git.kernel.org/pub/scm/linux/ 
>>> kernel/git/ wireless/wireless-next
>>> git bisect good 1952e19c02ae8ea0c663d30b19be14344b543068
>>> # bad: [1a9239bb4253f9076b5b4b2a1a4e8d7defd77a95] Merge tag 'net- 
>>> next-6.15' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/ 
>>> net-next
>>> git bisect bad 1a9239bb4253f9076b5b4b2a1a4e8d7defd77a95
>>> # good: [22093997ac9220d3c606313efbf4ce564962d095] Merge tag 
>>> 'ata-6.15- rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/ 
>>> libata/linux
>>> git bisect good 22093997ac9220d3c606313efbf4ce564962d095
>>> # bad: [336b4dae6dfecc9aa53a3a68c71b9c1c1d466388] Merge tag 'iommu- 
>>> updates-v6.15' of git://git.kernel.org/pub/scm/linux/kernel/git/ 
>>> iommu/linux
>>> git bisect bad 336b4dae6dfecc9aa53a3a68c71b9c1c1d466388
>>> # bad: [0711f1966a523d77d4c5f00776a7bd073d56251a] scsi: mpt3sas: Fix 
>>> buffer overflow in mpt3sas_send_mctp_passthru_req()
>>> git bisect bad 0711f1966a523d77d4c5f00776a7bd073d56251a
>>> # good: [369552fd03f296261023872b8fc983d1fc55c8e9] Merge patch series 
>>> "mpt3sas driver udpates"
>>> git bisect good 369552fd03f296261023872b8fc983d1fc55c8e9
>>> # bad: [42273e893157501ae119ea5459f3a7d2420c56d6] Merge patch series 
>>> "scsi: scsi_debug: Add more tape support"
>>> git bisect bad 42273e893157501ae119ea5459f3a7d2420c56d6
>>> # bad: [7e72900272b61c11f2fd4020d4f186124d0d171b] Merge patch series 
>>> "Support Multi-frequency scale for UFS"
>>> git bisect bad 7e72900272b61c11f2fd4020d4f186124d0d171b
>>> # good: [c02fe9e222d16bed8c270608c42f77bc62562ac3] scsi: ufs: qcom: 
>>> Implement the freq_to_gear_speed() vop
>>> git bisect good c02fe9e222d16bed8c270608c42f77bc62562ac3
>>> # bad: [eff26ad4c34fc78303c14be749e10ca61c4d211f] scsi: ufs: core: 
>>> Check if scaling up is required when disable clkscale
>>> git bisect bad eff26ad4c34fc78303c14be749e10ca61c4d211f
>>> # bad: [129b44c27c8a51cb74b2f68fde57f2a2e7f5696b] scsi: ufs: core: 
>>> Enable multi-level gear scaling
>>> git bisect bad 129b44c27c8a51cb74b2f68fde57f2a2e7f5696b
>>> # first bad commit: [129b44c27c8a51cb74b2f68fde57f2a2e7f5696b] scsi: 
>>> ufs: core: Enable multi-level gear scaling
>>>
>>> #regzbot introduced 129b44c27c8a51cb74b2f68fde57f2a2e7f5696b
>>>
>>> Thanks,
>>> Neil
>>>
>>
> 

[-- Attachment #2: 0001-scsi-ufs-qcom-check-negotiatory-max-gear-before-retu.patch --]
[-- Type: text/plain, Size: 932 bytes --]

From be2f444c5cc30cccd410c24ebf4b5d33dc3a2b1d Mon Sep 17 00:00:00 2001
From: Ziqi Chen <quic_ziqichen@quicinc.com>
Date: Tue, 29 Apr 2025 18:08:44 +0800
Subject: [PATCH] scsi: ufs: qcom: check negotiatory max gear before return
 freq matched gear

Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 1b37449fbffc..864be1e25c44 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1903,9 +1903,11 @@ static u32 ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq)
 		break;
 	default:
 		dev_err(hba->dev, "%s: Unsupported clock freq : %lu\n", __func__, freq);
-		break;
+		return gear;
 	}
 
+	gear = hba->max_pwr_info.is_valid ? min_t(u32, gear, hba->max_pwr_info.info.gear_rx) : gear;
+
 	return gear;
 }
 
-- 
2.34.1

Re: [PATCH v5 5/8] scsi: ufs: core: Enable multi-level gear scaling

[neil.armstrong]

Hi,

On 29/04/2025 12:23, Ziqi Chen wrote:
> Hi Neil,
> 
> Thanks for report~
> I got one rb3gen2 device and be able to reproduce this issue today.
> I made a change to fix this corner case. It already been verifed from
> my side. Could also help double check attached patch whether can fix this issue from your side?

This also fixes the issue on my side

Please add:

Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on RB3Gen2

If you send it on the lists.

Thanks,
Neil


> 
> Thanks,
> Ziqi
> 
> On 4/25/2025 3:43 PM, neil.armstrong@linaro.org wrote:
>> Hi,
>>
>> On 25/04/2025 09:29, Ziqi Chen wrote:
>>> Hi Neil,
>>>
>>> We analyzed this issue today , I don't think it is related to this patch:
>>
>> The bisect point to the patch where the issue appears, it may be another one of this patchset.
>>
>>>
>>> [    9.383728] ufshcd-qcom 1d84000.ufshc: pwr ctrl cmd 0x2 with mode 0x11 completion timeout
>>> ...
>>> ...
>>> ...
>>> [    9.588545] host_regs: 00000090: 00000002 15710000 00000002 00000003
>>>
>>> The timeout error log shows the mode 0x11 is the correct argument3 value of this pwr mode DME_SET cmd.
>>>
>>> int ufshcd_uic_change_pwr_mode(struct ufs_hba *hba, u8 mode)
>>> {
>>>      struct uic_command uic_cmd = {
>>>          .command = UIC_CMD_DME_SET,
>>>          .argument1 = UIC_ARG_MIB(PA_PWRMODE),
>>>          .argument3 = mode,
>>>      };
>>> ...
>>> ...
>>>   dev_err(hba->dev,
>>>          "uic cmd 0x%x with arg3 0x%x completion timeout\n",
>>>           uic_cmd->command, uic_cmd->argument3);
>>>
>>>
>>> This prints means that we gave correct argument3 here.
>>>
>>> But from the host register dump , we can see the argument3 written to register (address 0x***9C) is '0x00000003' which is a invalid value.
>>>
>>> And according to the UFSHCI JEDEC, the return value of ConfigResultCode regiser (address 0x***0x98) is 0x00000002 also told us the value written to argument3 register is a INVALID_MIB_ATTRIBUTE_VALUE, this is the reason causes this timeout issue.
>>>
>>> " 02h INVALID_MIB_ATTRIBUTE_VALUE "
>>>
>>> However, though we don't know the final root cause, we can know there is mistake occur during writing register. The argument3 value is 0x11, but after writing to register , the readback value from register is 0x3... Anyway , this patch didn't touch the path of register writing.
>>>
>>> Are you easy to reproduce this issue ? How many instances do you observed ? We never received similar report from our internal UFS test team and stabitily test team. If it is easy to reproduce , you can add readback prints at the place where after writing register to check it.
>>
>> The issue is easy to reproduce, just boot the rb3gen2 with vanilla v6.15-rc1 and default defconfig.
>>
>>>
>>> ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>>> {
>>>      lockdep_assert_held(&hba->uic_cmd_mutex);
>>>
>>>      WARN_ON(hba->active_uic_cmd);
>>>
>>>      hba->active_uic_cmd = uic_cmd;
>>>
>>>      /* Write Args */
>>>      ufshcd_writel(hba, uic_cmd->argument1, REG_UIC_COMMAND_ARG_1);
>>>      ufshcd_writel(hba, uic_cmd->argument2, REG_UIC_COMMAND_ARG_2);
>>>      ufshcd_writel(hba, uic_cmd->argument3, REG_UIC_COMMAND_ARG_3);
>>>
>>> -> you can add print here:
>>>    pr_err ("READ BACK argument3 from register 0x%x: 0x%x",
>>>        REG_UIC_COMMAND_ARG_3, ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3);
>>
>> I've run the kernel with:
>> =====================><=================================
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 0534390c2a35..232328ff6365 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -2494,6 +2494,14 @@ ufshcd_dispatch_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>>          ufshcd_writel(hba, uic_cmd->argument2, REG_UIC_COMMAND_ARG_2);
>>          ufshcd_writel(hba, uic_cmd->argument3, REG_UIC_COMMAND_ARG_3);
>>
>> +       {
>> +               uint32_t reg3 = ufshcd_readl(hba, REG_UIC_COMMAND_ARG_3);
>> +
>> +               if (reg3 != uic_cmd->argument3)
>> +                       pr_err("different READ BACK argument3 from register 0x% != 0x%x",
>> +                               uic_cmd->argument3, reg3);
>> +       }
>> +
>>          ufshcd_add_uic_command_trace(hba, uic_cmd, UFS_CMD_SEND);
>>
>>          /* Write UIC Cmd */
>> =====================><=================================
>>
>> And this print never appears.
>>
>> But the log clearly shows the kernel tries to scale back from gear1 to gear4:
>>
>>  >> [   10.538102] ufshcd-qcom 1d84000.ufshc: ufshcd_scale_gear: failed err -110, old gear: (tx 1 rx 1), new gear: (tx 4 rx 4)
>>
>> did you validate the gear scaling on the sc7280 with the mainline UFS & PHY drivers ?
>>
>> Neil
>>
>>>
>>>
>>>
>>> Best Regards,
>>> Ziqi
>>>
>>> On 4/24/2025 11:35 PM, Neil Armstrong wrote:
>>>> Hi,
>>>>
>>>> On 13/02/2025 09:00, Ziqi Chen wrote:
>>>>> From: Can Guo <quic_cang@quicinc.com>
>>>>>
>>>>> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
>>>>> plans. However, the gear speed is only toggled between min and max during
>>>>> clock scaling. Enable multi-level gear scaling by mapping clock frequencies
>>>>> to gear speeds, so that when devfreq scales clock frequencies we can put
>>>>> the UFS link at the appropriate gear speeds accordingly.
>>>>>
>>>>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>>>>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>>> Reviewed-by: Bean Huo <beanhuo@micron.com>
>>>>> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
>>>>> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>> ---
>>>>>
>>>>> v1 -> v2:
>>>>> Rename the lable "do_pmc" to "config_pwr_mode".
>>>>>
>>>>> v2 -> v3:
>>>>> Use assignment instead memcpy() in function ufshcd_scale_gear().
>>>>>
>>>>> v3 -> v4:
>>>>> Typo fixed for commit message.
>>>>>
>>>>> v4 -> v5:
>>>>> Change the data type of "new_gear" from 'int' to 'u32'.
>>>>> ---
>>>>>   drivers/ufs/core/ufshcd.c | 44 +++++++++++++++++++++++++++++ +---------
>>>>>   1 file changed, 34 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>>>>> index 8d295cc827cc..9908c0d6a1e1 100644
>>>>> --- a/drivers/ufs/core/ufshcd.c
>>>>> +++ b/drivers/ufs/core/ufshcd.c
>>>>> @@ -1308,16 +1308,26 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
>>>>>   /**
>>>>>    * ufshcd_scale_gear - scale up/down UFS gear
>>>>>    * @hba: per adapter instance
>>>>> + * @target_gear: target gear to scale to
>>>>>    * @scale_up: True for scaling up gear and false for scaling down
>>>>>    *
>>>>>    * Return: 0 for success; -EBUSY if scaling can't happen at this time;
>>>>>    * non-zero for any other errors.
>>>>>    */
>>>>> -static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>>>>> +static int ufshcd_scale_gear(struct ufs_hba *hba, u32 target_gear, bool scale_up)
>>>>>   {
>>>>>       int ret = 0;
>>>>>       struct ufs_pa_layer_attr new_pwr_info;
>>>>> +    if (target_gear) {
>>>>> +        new_pwr_info = hba->pwr_info;
>>>>> +        new_pwr_info.gear_tx = target_gear;
>>>>> +        new_pwr_info.gear_rx = target_gear;
>>>>> +
>>>>> +        goto config_pwr_mode;
>>>>> +    }
>>>>> +
>>>>> +    /* Legacy gear scaling, in case vops_freq_to_gear_speed() is not implemented */
>>>>>       if (scale_up) {
>>>>>           memcpy(&new_pwr_info, &hba->clk_scaling.saved_pwr_info,
>>>>>                  sizeof(struct ufs_pa_layer_attr));
>>>>> @@ -1338,6 +1348,7 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>>>>>           }
>>>>>       }
>>>>> +config_pwr_mode:
>>>>>       /* check if the power mode needs to be changed or not? */
>>>>>       ret = ufshcd_config_pwr_mode(hba, &new_pwr_info);
>>>>>       if (ret)
>>>>> @@ -1408,15 +1419,19 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool sc
>>>>>   static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
>>>>>                   bool scale_up)
>>>>>   {
>>>>> +    u32 old_gear = hba->pwr_info.gear_rx;
>>>>> +    u32 new_gear = 0;
>>>>>       int ret = 0;
>>>>> +    new_gear = ufshcd_vops_freq_to_gear_speed(hba, freq);
>>>>> +
>>>>>       ret = ufshcd_clock_scaling_prepare(hba, 1 * USEC_PER_SEC);
>>>>>       if (ret)
>>>>>           return ret;
>>>>>       /* scale down the gear before scaling down clocks */
>>>>>       if (!scale_up) {
>>>>> -        ret = ufshcd_scale_gear(hba, false);
>>>>> +        ret = ufshcd_scale_gear(hba, new_gear, false);
>>>>>           if (ret)
>>>>>               goto out_unprepare;
>>>>>       }
>>>>> @@ -1424,13 +1439,13 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
>>>>>       ret = ufshcd_scale_clks(hba, freq, scale_up);
>>>>>       if (ret) {
>>>>>           if (!scale_up)
>>>>> -            ufshcd_scale_gear(hba, true);
>>>>> +            ufshcd_scale_gear(hba, old_gear, true);
>>>>>           goto out_unprepare;
>>>>>       }
>>>>>       /* scale up the gear after scaling up clocks */
>>>>>       if (scale_up) {
>>>>> -        ret = ufshcd_scale_gear(hba, true);
>>>>> +        ret = ufshcd_scale_gear(hba, new_gear, true);
>>>>>           if (ret) {
>>>>>               ufshcd_scale_clks(hba, hba->devfreq->previous_freq,
>>>>>                         false);
>>>>> @@ -1723,6 +1738,8 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
>>>>>           struct device_attribute *attr, const char *buf, size_t count)
>>>>>   {
>>>>>       struct ufs_hba *hba = dev_get_drvdata(dev);
>>>>> +    struct ufs_clk_info *clki;
>>>>> +    unsigned long freq;
>>>>>       u32 value;
>>>>>       int err = 0;
>>>>> @@ -1746,14 +1763,21 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
>>>>>       if (value) {
>>>>>           ufshcd_resume_clkscaling(hba);
>>>>> -    } else {
>>>>> -        ufshcd_suspend_clkscaling(hba);
>>>>> -        err = ufshcd_devfreq_scale(hba, ULONG_MAX, true);
>>>>> -        if (err)
>>>>> -            dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
>>>>> -                    __func__, err);
>>>>> +        goto out_rel;
>>>>>       }
>>>>> +    clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
>>>>> +    freq = clki->max_freq;
>>>>> +
>>>>> +    ufshcd_suspend_clkscaling(hba);
>>>>> +    err = ufshcd_devfreq_scale(hba, freq, true);
>>>>> +    if (err)
>>>>> +        dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
>>>>> +                __func__, err);
>>>>> +    else
>>>>> +        hba->clk_scaling.target_freq = freq;
>>>>> +
>>>>> +out_rel:
>>>>>       ufshcd_release(hba);
>>>>>       ufshcd_rpm_put_sync(hba);
>>>>>   out:
>>>>
>>>> This change causes UFS crash on the RB3gen2, after UFS successfully probe and scan:
>>>>
>>>> [    9.383728] ufshcd-qcom 1d84000.ufshc: pwr ctrl cmd 0x2 with mode 0x11 completion timeout
>>>> [    9.393272] ufshcd-qcom 1d84000.ufshc: UFS Host state=1
>>>> [    9.403126] msm_dpu ae01000.display-controller: [drm:adreno_request_fw [msm]] *ERROR* failed to load a660_sqe.fw
>>>> [    9.408484] ufshcd-qcom 1d84000.ufshc: 0 outstanding reqs, tasks=0x0
>>>> [    9.425577] ufshcd-qcom 1d84000.ufshc: saved_err=0x0, saved_uic_err=0x0
>>>> [    9.432433] ufshcd-qcom 1d84000.ufshc: Device power mode=1, UIC link state=1
>>>> [    9.439716] ufshcd-qcom 1d84000.ufshc: PM in progress=0, sys. suspended=0
>>>> [    9.446742] ufshcd-qcom 1d84000.ufshc: Auto BKOPS=0, Host self- block=0
>>>> [    9.453468] ufshcd-qcom 1d84000.ufshc: Clk gate=1
>>>> ...
>>>> [   10.529259] ufshcd-qcom 1d84000.ufshc: ufshcd_change_power_mode: power mode change failed -110
>>>> [   10.538102] ufshcd-qcom 1d84000.ufshc: ufshcd_scale_gear: failed err -110, old gear: (tx 1 rx 1), new gear: (tx 4 rx 4)
>>>> [   10.542841] WARNING: CPU: 0 PID: 274 at drivers/ufs/core/ ufshcd.c:5504 ufshcd_sl_intr+0x64c/0x6a4
>>>> ...
>>>>
>>>> CI Run sample: https://git.codelinaro.org/linaro/qcomlt/ci/staging/ cdba- tester/-/jobs/233352#L1479
>>>>
>>>> Bisect run log:
>>>> # bad: [0af2f6be1b4281385b618cb86ad946eded089ac8] Linux 6.15-rc1
>>>> # good: [38fec10eb60d687e30c8c6b5420d86e8149f7557] Linux 6.14
>>>> git bisect start 'v6.15-rc1' 'v6.14'
>>>> # bad: [fd71def6d9abc5ae362fb9995d46049b7b0ed391] Merge tag 'for-6.15- tag' of git://git.kernel.org/pub/scm/linux/kernel/git/ kdave/linux
>>>> git bisect bad fd71def6d9abc5ae362fb9995d46049b7b0ed391
>>>> # good: [fb1ceb29b27cda91af35851ebab01f298d82162e] Merge tag 'platform- drivers-x86-v6.15-1' of git://git.kernel.org/pub/scm/ linux/kernel/git/ pdx86/platform-drivers-x86
>>>> git bisect good fb1ceb29b27cda91af35851ebab01f298d82162e
>>>> # good: [1952e19c02ae8ea0c663d30b19be14344b543068] Merge tag 'wireless- next-2025-03-20' of https://git.kernel.org/pub/scm/linux/ kernel/git/ wireless/wireless-next
>>>> git bisect good 1952e19c02ae8ea0c663d30b19be14344b543068
>>>> # bad: [1a9239bb4253f9076b5b4b2a1a4e8d7defd77a95] Merge tag 'net- next-6.15' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/ net-next
>>>> git bisect bad 1a9239bb4253f9076b5b4b2a1a4e8d7defd77a95
>>>> # good: [22093997ac9220d3c606313efbf4ce564962d095] Merge tag 'ata-6.15- rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/ libata/linux
>>>> git bisect good 22093997ac9220d3c606313efbf4ce564962d095
>>>> # bad: [336b4dae6dfecc9aa53a3a68c71b9c1c1d466388] Merge tag 'iommu- updates-v6.15' of git://git.kernel.org/pub/scm/linux/kernel/git/ iommu/linux
>>>> git bisect bad 336b4dae6dfecc9aa53a3a68c71b9c1c1d466388
>>>> # bad: [0711f1966a523d77d4c5f00776a7bd073d56251a] scsi: mpt3sas: Fix buffer overflow in mpt3sas_send_mctp_passthru_req()
>>>> git bisect bad 0711f1966a523d77d4c5f00776a7bd073d56251a
>>>> # good: [369552fd03f296261023872b8fc983d1fc55c8e9] Merge patch series "mpt3sas driver udpates"
>>>> git bisect good 369552fd03f296261023872b8fc983d1fc55c8e9
>>>> # bad: [42273e893157501ae119ea5459f3a7d2420c56d6] Merge patch series "scsi: scsi_debug: Add more tape support"
>>>> git bisect bad 42273e893157501ae119ea5459f3a7d2420c56d6
>>>> # bad: [7e72900272b61c11f2fd4020d4f186124d0d171b] Merge patch series "Support Multi-frequency scale for UFS"
>>>> git bisect bad 7e72900272b61c11f2fd4020d4f186124d0d171b
>>>> # good: [c02fe9e222d16bed8c270608c42f77bc62562ac3] scsi: ufs: qcom: Implement the freq_to_gear_speed() vop
>>>> git bisect good c02fe9e222d16bed8c270608c42f77bc62562ac3
>>>> # bad: [eff26ad4c34fc78303c14be749e10ca61c4d211f] scsi: ufs: core: Check if scaling up is required when disable clkscale
>>>> git bisect bad eff26ad4c34fc78303c14be749e10ca61c4d211f
>>>> # bad: [129b44c27c8a51cb74b2f68fde57f2a2e7f5696b] scsi: ufs: core: Enable multi-level gear scaling
>>>> git bisect bad 129b44c27c8a51cb74b2f68fde57f2a2e7f5696b
>>>> # first bad commit: [129b44c27c8a51cb74b2f68fde57f2a2e7f5696b] scsi: ufs: core: Enable multi-level gear scaling
>>>>
>>>> #regzbot introduced 129b44c27c8a51cb74b2f68fde57f2a2e7f5696b
>>>>
>>>> Thanks,
>>>> Neil
>>>>
>>>
>>

[PATCH v5 6/8] scsi: ufs: core: Check if scaling up is required when disable clkscale

[Ziqi Chen]

When disabling clkscale via the clkscale_enable sysfs entry, UFS driver
shall perform scaling up once regardless. Check if scaling up is required
or not first to avoid repetitive work.

Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Co-developed-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 9908c0d6a1e1..727ca930c2ef 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1770,6 +1770,10 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 	freq = clki->max_freq;
 
 	ufshcd_suspend_clkscaling(hba);
+
+	if (!ufshcd_is_devfreq_scaling_required(hba, freq, true))
+		goto out_rel;
+
 	err = ufshcd_devfreq_scale(hba, freq, true);
 	if (err)
 		dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
-- 
2.34.1

[PATCH v5 7/8] scsi: ufs: core: Toggle Write Booster during clock scaling base on gear speed

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

During clock scaling, Write Booster is toggled on or off based on
whether the clock is scaled up or down. However, with OPP V2 powered
multi-level gear scaling, the gear can be scaled amongst multiple gear
speeds, e.g., it may scale down from G5 to G4, or from G4 to G2. To provide
flexibilities, add a new field for clock scaling such that during clock
scaling Write Booster can be enabled or disabled based on gear speeds but
not based on scaling up or down.

Signed-off-by: Can Guo <quic_cang@quicinc.com>
Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
Reviewed-by: Peter Wang <peter.wang@mediatek.com>

---

v1 - > v2:
Initialize the local variables "wb_en" as "false".
v3 -> v4:
1. Add comment for default initialized wb_gear.
2. Remove the unnecessary variable “wb_en" in function
   ufshcd_clock_scaling_unprepare().
---
 drivers/ufs/core/ufshcd.c | 12 ++++++++----
 include/ufs/ufshcd.h      |  3 +++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 727ca930c2ef..628bf2aebbeb 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1393,13 +1393,13 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba, u64 timeout_us)
 	return ret;
 }
 
-static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool scale_up)
+static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err)
 {
 	up_write(&hba->clk_scaling_lock);
 
-	/* Enable Write Booster if we have scaled up else disable it */
+	/* Enable Write Booster if current gear requires it else disable it */
 	if (ufshcd_enable_wb_if_scaling_up(hba) && !err)
-		ufshcd_wb_toggle(hba, scale_up);
+		ufshcd_wb_toggle(hba, hba->pwr_info.gear_rx >= hba->clk_scaling.wb_gear);
 
 	mutex_unlock(&hba->wb_mutex);
 
@@ -1454,7 +1454,7 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
 	}
 
 out_unprepare:
-	ufshcd_clock_scaling_unprepare(hba, ret, scale_up);
+	ufshcd_clock_scaling_unprepare(hba, ret);
 	return ret;
 }
 
@@ -1814,6 +1814,10 @@ static void ufshcd_init_clk_scaling(struct ufs_hba *hba)
 	if (!hba->clk_scaling.min_gear)
 		hba->clk_scaling.min_gear = UFS_HS_G1;
 
+	if (!hba->clk_scaling.wb_gear)
+		/* Use intermediate gear speed HS_G3 as the default wb_gear */
+		hba->clk_scaling.wb_gear = UFS_HS_G3;
+
 	INIT_WORK(&hba->clk_scaling.suspend_work,
 		  ufshcd_clk_scaling_suspend_work);
 	INIT_WORK(&hba->clk_scaling.resume_work,
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index b34301de3cf8..6633c1a1c224 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -447,6 +447,8 @@ struct ufs_clk_gating {
  * @resume_work: worker to resume devfreq
  * @target_freq: frequency requested by devfreq framework
  * @min_gear: lowest HS gear to scale down to
+ * @wb_gear: enable Write Booster when HS gear scales above or equal to it, else
+ *		disable Write Booster
  * @is_enabled: tracks if scaling is currently enabled or not, controlled by
  *		clkscale_enable sysfs node
  * @is_allowed: tracks if scaling is currently allowed or not, used to block
@@ -467,6 +469,7 @@ struct ufs_clk_scaling {
 	struct work_struct resume_work;
 	unsigned long target_freq;
 	u32 min_gear;
+	u32 wb_gear;
 	bool is_enabled;
 	bool is_allowed;
 	bool is_initialized;
-- 
2.34.1

[PATCH v5 8/8] ABI: sysfs-driver-ufs: Add missing UFS sysfs attributes

[Ziqi Chen]

Add UFS driver sysfs attributes clkscale_enable, clkgate_enable and
clkgate_delay_ms to this document.

Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
---
v1 -> v2:
It is a new patch be added to this series since v2.

v2 -> v3:
1. Typo fixed for commit message.
2. Improve the grammar of attributes' descriptions.

V3 -> v4:
The use of words is more standardized.
---
 Documentation/ABI/testing/sysfs-driver-ufs | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index 5fa6655aee84..da8d1437d3f4 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1559,3 +1559,36 @@ Description:
 		Symbol - HCMID. This file shows the UFSHCD manufacturer id.
 		The Manufacturer ID is defined by JEDEC in JEDEC-JEP106.
 		The file is read only.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/clkscale_enable
+What:		/sys/bus/platform/devices/*.ufs/clkscale_enable
+Date:		January 2025
+Contact:	Ziqi Chen <quic_ziqichen@quicinc.com>
+Description:
+		This attribute shows whether the UFS clock scaling is enabled or not.
+		And it can be used to enable/disable the clock scaling by writing
+		1 or 0 to this attribute.
+
+		The attribute is read/write.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/clkgate_enable
+What:		/sys/bus/platform/devices/*.ufs/clkgate_enable
+Date:		January 2025
+Contact:	Ziqi Chen <quic_ziqichen@quicinc.com>
+Description:
+		This attribute shows whether the UFS clock gating is enabled or not.
+		And it can be used to enable/disable the clock gating by writing
+		1 or 0 to this attribute.
+
+		The attribute is read/write.
+
+What:		/sys/bus/platform/drivers/ufshcd/*/clkgate_delay_ms
+What:		/sys/bus/platform/devices/*.ufs/clkgate_delay_ms
+Date:		January 2025
+Contact:	Ziqi Chen <quic_ziqichen@quicinc.com>
+Description:
+		This attribute shows and sets the number of milliseconds of idle time
+		before the UFS driver starts to perform clock gating. This can
+		prevent the UFS from frequently performing clock gating/ungating.
+
+		The attribute is read/write.
-- 
2.34.1

Re: [PATCH v5 0/8] Support Multi-frequency scale for UFS

[Martin K. Petersen]

Ziqi,

> With OPP V2 enabled, devfreq can scale clocks amongst multiple
> frequency plans. However, the gear speed is only toggled between min
> and max during clock scaling. Enable multi-level gear scaling by
> mapping clock frequencies to gear speeds, so that when devfreq scales
> clock frequencies we can put the UFS link at the appropraite gear
> speeds accordingly.

Applied to 6.15/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v5 0/8] Support Multi-frequency scale for UFS

[Martin K. Petersen]

On Thu, 13 Feb 2025 16:00:00 +0800, Ziqi Chen wrote:

> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
> plans. However, the gear speed is only toggled between min and max during
> clock scaling. Enable multi-level gear scaling by mapping clock frequencies
> to gear speeds, so that when devfreq scales clock frequencies we can put
> the UFS link at the appropraite gear speeds accordingly.
> 
> This series has been tested on below platforms -
> sm8550 mtp + UFS3.1
> SM8650 MTP + UFS3.1
> SM8750 MTP + UFS4.0
> 
> [...]

Applied to 6.15/scsi-queue, thanks!

[1/8] scsi: ufs: core: Pass target_freq to clk_scale_notify() vop
      https://git.kernel.org/mkp/scsi/c/5e011fcc7d16
[2/8] scsi: ufs: qcom: Pass target_freq to clk scale pre and post change
      https://git.kernel.org/mkp/scsi/c/367a0f017c61
[3/8] scsi: ufs: core: Add a vop to map clock frequency to gear speed
      https://git.kernel.org/mkp/scsi/c/d7bead60b08e
[4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vop
      https://git.kernel.org/mkp/scsi/c/c02fe9e222d1
[5/8] scsi: ufs: core: Enable multi-level gear scaling
      https://git.kernel.org/mkp/scsi/c/129b44c27c8a
[6/8] scsi: ufs: core: Check if scaling up is required when disable clkscale
      https://git.kernel.org/mkp/scsi/c/eff26ad4c34f
[7/8] scsi: ufs: core: Toggle Write Booster during clock scaling base on gear speed
      https://git.kernel.org/mkp/scsi/c/2a25cbaa81d2
[8/8] ABI: sysfs-driver-ufs: Add missing UFS sysfs attributes
      https://git.kernel.org/mkp/scsi/c/6d7696b4d447

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v5 0/8] Support Multi-frequency scale for UFS

[Luca Weiss]

Hi Ziqi,

On Thu Feb 13, 2025 at 9:00 AM CET, Ziqi Chen wrote:
> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
> plans. However, the gear speed is only toggled between min and max during
> clock scaling. Enable multi-level gear scaling by mapping clock frequencies
> to gear speeds, so that when devfreq scales clock frequencies we can put
> the UFS link at the appropraite gear speeds accordingly.

I believe this series is causing issues on SM6350:

[    0.859449] ufshcd-qcom 1d84000.ufshc: ufs_qcom_freq_to_gear_speed: Unsupported clock freq : 200000000
[    0.886668] ufshcd-qcom 1d84000.ufshc: UNIPRO clk freq 200 MHz not supported
[    0.903791] devfreq 1d84000.ufshc: dvfs failed with (-22) error

That's with this patch, I actually haven't tried without on v6.15-rc3
https://lore.kernel.org/all/20250314-sm6350-ufs-things-v1-2-3600362cc52c@fairphone.com/

I believe the issue appears because core clk and unipro clk rates don't
match on this platform, so this 200 MHz for GCC_UFS_PHY_AXI_CLK is not a
valid unipro clock rate, but for GCC_UFS_PHY_UNIPRO_CORE_CLK it's
specified to 150 MHz in the opp table.

Regards
Luca

>
> This series has been tested on below platforms -
> sm8550 mtp + UFS3.1
> SM8650 MTP + UFS3.1
> SM8750 MTP + UFS4.0

Re: [PATCH v5 0/8] Support Multi-frequency scale for UFS

[Ziqi Chen]

Hi Luca,

Thanks for your report.
Really,  6350 is a special platform that the UFS_PHY_AXI_CLK doesn't
match to the UFS_PHY_UNIPRO_CORE_CLK. We already found out the root
cause and discussing the fix. We will submit change to fix this corner
case.

BRs
Ziqi

On 4/26/2025 3:48 AM, Luca Weiss wrote:
> Hi Ziqi,
> 
> On Thu Feb 13, 2025 at 9:00 AM CET, Ziqi Chen wrote:
>> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
>> plans. However, the gear speed is only toggled between min and max during
>> clock scaling. Enable multi-level gear scaling by mapping clock frequencies
>> to gear speeds, so that when devfreq scales clock frequencies we can put
>> the UFS link at the appropraite gear speeds accordingly.
> 
> I believe this series is causing issues on SM6350:
> 
> [    0.859449] ufshcd-qcom 1d84000.ufshc: ufs_qcom_freq_to_gear_speed: Unsupported clock freq : 200000000
> [    0.886668] ufshcd-qcom 1d84000.ufshc: UNIPRO clk freq 200 MHz not supported
> [    0.903791] devfreq 1d84000.ufshc: dvfs failed with (-22) error
> 
> That's with this patch, I actually haven't tried without on v6.15-rc3
> https://lore.kernel.org/all/20250314-sm6350-ufs-things-v1-2-3600362cc52c@fairphone.com/
> 
> I believe the issue appears because core clk and unipro clk rates don't
> match on this platform, so this 200 MHz for GCC_UFS_PHY_AXI_CLK is not a
> valid unipro clock rate, but for GCC_UFS_PHY_UNIPRO_CORE_CLK it's
> specified to 150 MHz in the opp table.
> 
> Regards
> Luca
> 
>>
>> This series has been tested on below platforms -
>> sm8550 mtp + UFS3.1
>> SM8650 MTP + UFS3.1
>> SM8750 MTP + UFS4.0

Re: [PATCH v5 0/8] Support Multi-frequency scale for UFS

[Ziqi Chen]

[-- Attachment #1: Type: text/plain, Size: 2029 bytes --]

Hi Luca,

We made changes to fix this special platform issue and verified it can
fix this issue.
Could you help double check if attached 3 patched can fix it from you side?
If it is OK from you side as well, we will submit the final patches to
upstream

Thanks a lot~

BRs
Ziqi

On 4/27/2025 4:14 PM, Ziqi Chen wrote:
> Hi Luca,
> 
> Thanks for your report.
> Really,  6350 is a special platform that the UFS_PHY_AXI_CLK doesn't
> match to the UFS_PHY_UNIPRO_CORE_CLK. We already found out the root
> cause and discussing the fix. We will submit change to fix this corner
> case.
> 
> BRs
> Ziqi
> 
> On 4/26/2025 3:48 AM, Luca Weiss wrote:
>> Hi Ziqi,
>>
>> On Thu Feb 13, 2025 at 9:00 AM CET, Ziqi Chen wrote:
>>> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
>>> plans. However, the gear speed is only toggled between min and max 
>>> during
>>> clock scaling. Enable multi-level gear scaling by mapping clock 
>>> frequencies
>>> to gear speeds, so that when devfreq scales clock frequencies we can put
>>> the UFS link at the appropraite gear speeds accordingly.
>>
>> I believe this series is causing issues on SM6350:
>>
>> [    0.859449] ufshcd-qcom 1d84000.ufshc: ufs_qcom_freq_to_gear_speed: 
>> Unsupported clock freq : 200000000
>> [    0.886668] ufshcd-qcom 1d84000.ufshc: UNIPRO clk freq 200 MHz not 
>> supported
>> [    0.903791] devfreq 1d84000.ufshc: dvfs failed with (-22) error
>>
>> That's with this patch, I actually haven't tried without on v6.15-rc3
>> https://lore.kernel.org/all/20250314-sm6350-ufs-things- 
>> v1-2-3600362cc52c@fairphone.com/
>>
>> I believe the issue appears because core clk and unipro clk rates don't
>> match on this platform, so this 200 MHz for GCC_UFS_PHY_AXI_CLK is not a
>> valid unipro clock rate, but for GCC_UFS_PHY_UNIPRO_CORE_CLK it's
>> specified to 150 MHz in the opp table.
>>
>> Regards
>> Luca
>>
>>>
>>> This series has been tested on below platforms -
>>> sm8550 mtp + UFS3.1
>>> SM8650 MTP + UFS3.1
>>> SM8750 MTP + UFS4.0
> 

[-- Attachment #2: 0001-ufs-host-ufs-qcom-Map-OPP-freq-to-UniPro-Core-Clock-.patch --]
[-- Type: text/plain, Size: 1815 bytes --]

From c36df86976b2d8d6c637f5a7872281c846866e43 Mon Sep 17 00:00:00 2001
From: Can Guo <quic_cang@quicinc.com>
Date: Sun, 27 Apr 2025 00:05:59 -0700
Subject: [PATCH 1/3] ufs: host: ufs-qcom: Map OPP freq to UniPro Core Clock
 freq

Signed-off-by: Can Guo <quic_cang@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 43 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index c0761ccc1381..8cb8f60e8c89 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1922,11 +1922,52 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
 	return ret;
 }
 
+static unsigned long ufs_qcom_opp_to_clk_freq(struct ufs_hba *hba, unsigned long freq, char *name)
+{
+	struct ufs_clk_info *clki;
+	struct dev_pm_opp *opp;
+	unsigned long unipro_freq;
+	int idx = 0;
+	bool found = false;
+
+	opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true);
+	if (IS_ERR(opp)) {
+		dev_err(hba->dev, "Failed to find OPP for exact frequency %lu\n", freq);
+		return 0;
+	}
+
+	list_for_each_entry(clki, &hba->clk_list_head, list) {
+		if (!strcmp(cliki->name, name)) {
+			found = true;
+			break;
+		}
+
+		idx ++;
+	}
+
+	if (!found) {
+		dev_err(hba->dev, "Failed to find %s in clk list\n", name);
+		dev_pm_opp_put(opp);
+		return 0;
+	}
+
+	unipro_freq = dev_pm_opp_get_freq_indexed(opp, idx);
+
+	dev_pm_opp_put(opp);
+
+	return unipro_freq;
+}
+
 static u32 ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq)
 {
 	u32 gear = 0;
+	unsigned long unipro_freq;
+
+	if (!hba->use_pm_opp)
+		return gear;
 
-	switch (freq) {
+	unipro_freq = ufs_qcom_opp_to_clk_freq(hba, freq, "core_clk_unipro");
+	switch (unipro_freq) {
 	case 403000000:
 		gear = UFS_HS_G5;
 		break;
-- 
2.34.1


[-- Attachment #3: 0002-ufs-host-ufs-qcom-Fix-ufs_qcom_core_clk_ctrl.patch --]
[-- Type: text/plain, Size: 3813 bytes --]

From 655ed70ea2e439ba73f0ed950c924f7ad59cc83f Mon Sep 17 00:00:00 2001
From: Can Guo <quic_cang@quicinc.com>
Date: Sun, 27 Apr 2025 00:22:38 -0700
Subject: [PATCH 2/3] ufs: host: ufs-qcom: Fix ufs_qcom_core_clk_ctrl()

Signed-off-by: Can Guo <quic_cang@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 8cb8f60e8c89..398cc21f2ce4 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -103,7 +103,8 @@ static const struct __ufs_qcom_bw_table {
 };
 
 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, unsigned long freq);
+static unsigned long ufs_qcom_opp_to_clk_freq(struct ufs_hba *hba, unsigned long freq, char *name);
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up, unsigned long freq);
 
 static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
 {
@@ -602,7 +603,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 			return -EINVAL;
 		}
 
-		err = ufs_qcom_set_core_clk_ctrl(hba, ULONG_MAX);
+		err = ufs_qcom_set_core_clk_ctrl(hba, true, ULONG_MAX);
 		if (err)
 			dev_err(hba->dev, "cfg core clk ctrl failed\n");
 		/*
@@ -1360,24 +1361,38 @@ static int ufs_qcom_set_clk_40ns_cycles(struct ufs_hba *hba,
 	return ufshcd_dme_set(hba, UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES), reg);
 }
 
-static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq)
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up, unsigned long freq)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct list_head *head = &hba->clk_list_head;
 	struct ufs_clk_info *clki;
 	u32 cycles_in_1us = 0;
 	u32 core_clk_ctrl_reg;
+	unsigned long clk_freq;
 	int err;
 
 	list_for_each_entry(clki, head, list) {
 		if (!IS_ERR_OR_NULL(clki->clk) &&
 		    !strcmp(clki->name, "core_clk_unipro")) {
-			if (!clki->max_freq)
+			if (!clki->max_freq) {
 				cycles_in_1us = 150; /* default for backwards compatibility */
-			else if (freq == ULONG_MAX)
+				break;
+			}
+
+			if (freq == ULONG_MAX) {
 				cycles_in_1us = ceil(clki->max_freq, HZ_PER_MHZ);
-			else
-				cycles_in_1us = ceil(freq, HZ_PER_MHZ);
+				break;
+			}
+
+			if (!hba->use_pm_opp) {
+				if (is_scale_up)
+					cycles_in_1us = ceil(clki->max_freq, HZ_PER_MHZ);
+				else
+					cycles_in_1us = ceil(clk_get_rate(clki->clk), HZ_PER_MHZ);
+			} else {
+				clk_freq = ufs_qcom_opp_to_clk_freq(hba, freq, "core_clk_unipro");
+				cycles_in_1us = ceil(clk_freq, HZ_PER_MHZ);
+			}
 
 			break;
 		}
@@ -1425,7 +1440,7 @@ static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba, unsigned long f
 		return ret;
 	}
 	/* set unipro core clock attributes and clear clock divider */
-	return ufs_qcom_set_core_clk_ctrl(hba, freq);
+	return ufs_qcom_set_core_clk_ctrl(hba, true, freq);
 }
 
 static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
@@ -1457,7 +1472,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, unsigned long freq)
 {
 	/* set unipro core clock attributes and clear clock divider */
-	return ufs_qcom_set_core_clk_ctrl(hba, freq);
+	return ufs_qcom_set_core_clk_ctrl(hba, false, freq);
 }
 
 static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
@@ -1937,7 +1952,7 @@ static unsigned long ufs_qcom_opp_to_clk_freq(struct ufs_hba *hba, unsigned long
 	}
 
 	list_for_each_entry(clki, &hba->clk_list_head, list) {
-		if (!strcmp(cliki->name, name)) {
+		if (!strcmp(clki->name, name)) {
 			found = true;
 			break;
 		}
-- 
2.34.1


[-- Attachment #4: 0003-ufs-host-ufs-qcom-Fix-ufs_qcom_cfg_timers.patch --]
[-- Type: text/plain, Size: 3966 bytes --]

From 7655132c9e5436a637aeafb273a639236122533a Mon Sep 17 00:00:00 2001
From: Can Guo <quic_cang@quicinc.com>
Date: Sun, 27 Apr 2025 00:37:39 -0700
Subject: [PATCH 3/3] ufs: host: ufs-qcom: Fix ufs_qcom_cfg_timers()

Signed-off-by: Can Guo <quic_cang@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 51 +++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 398cc21f2ce4..b2bf80070711 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -544,13 +544,14 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
  *
  * @hba: host controller instance
  * @is_pre_scale_up: flag to check if pre scale up condition.
+ * @freq: target opp freq
  * Return: zero for success and non-zero in case of a failure.
  */
-static int ufs_qcom_cfg_timers(struct ufs_hba *hba, bool is_pre_scale_up)
+static int ufs_qcom_cfg_timers(struct ufs_hba *hba, bool is_pre_scale_up, unsigned long freq)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct ufs_clk_info *clki;
-	unsigned long core_clk_rate = 0;
+	unsigned long clk_freq = 0;
 	u32 core_clk_cycles_per_us;
 
 	/*
@@ -564,20 +565,30 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, bool is_pre_scale_up)
 
 	list_for_each_entry(clki, &hba->clk_list_head, list) {
 		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);
+			if (freq == ULONG_MAX) {
+				clk_freq = clki->max_freq;
+				break;
+			}
+
+			if (!hba->use_pm_opp) {
+				if (is_pre_scale_up)
+					clk_freq = clki->max_freq;
+				else
+					clk_freq = clk_get_rate(clki->clk);
+			} else {
+				clk_freq = ufs_qcom_opp_to_clk_freq(hba, freq, "core_clk");
+			}
+
 			break;
 		}
 
 	}
 
 	/* If frequency is smaller than 1MHz, set to 1MHz */
-	if (core_clk_rate < DEFAULT_CLK_RATE_HZ)
-		core_clk_rate = DEFAULT_CLK_RATE_HZ;
+	if (clk_freq < DEFAULT_CLK_RATE_HZ)
+		clk_freq = DEFAULT_CLK_RATE_HZ;
 
-	core_clk_cycles_per_us = core_clk_rate / USEC_PER_SEC;
+	core_clk_cycles_per_us = clk_freq / USEC_PER_SEC;
 	if (ufshcd_readl(hba, REG_UFS_SYS1CLK_1US) != core_clk_cycles_per_us) {
 		ufshcd_writel(hba, core_clk_cycles_per_us, REG_UFS_SYS1CLK_1US);
 		/*
@@ -597,7 +608,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 
 	switch (status) {
 	case PRE_CHANGE:
-		if (ufs_qcom_cfg_timers(hba, false)) {
+		if (ufs_qcom_cfg_timers(hba, false, ULONG_MAX)) {
 			dev_err(hba->dev, "%s: ufs_qcom_cfg_timers() failed\n",
 				__func__);
 			return -EINVAL;
@@ -875,17 +886,6 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
 
 		break;
 	case POST_CHANGE:
-		if (ufs_qcom_cfg_timers(hba, false)) {
-			dev_err(hba->dev, "%s: ufs_qcom_cfg_timers() failed\n",
-				__func__);
-			/*
-			 * we return error code at the end of the routine,
-			 * but continue to configure UFS_PHY_TX_LANE_ENABLE
-			 * and bus voting as usual
-			 */
-			ret = -EINVAL;
-		}
-
 		/* cache the power mode parameters to use internally */
 		memcpy(&host->dev_req_params,
 				dev_req_params, sizeof(*dev_req_params));
@@ -1434,7 +1434,7 @@ static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba, unsigned long f
 {
 	int ret;
 
-	ret = ufs_qcom_cfg_timers(hba, true);
+	ret = ufs_qcom_cfg_timers(hba, true, freq);
 	if (ret) {
 		dev_err(hba->dev, "%s ufs cfg timer failed\n", __func__);
 		return ret;
@@ -1471,6 +1471,13 @@ 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, unsigned long freq)
 {
+	int ret;
+
+	ret = ufs_qcom_cfg_timers(hba, false, freq);
+	if (ret) {
+		dev_err(hba->dev, "%s: ufs_qcom_cfg_timers() failed\n",	__func__);
+		ret = -EINVAL;
+	}
 	/* set unipro core clock attributes and clear clock divider */
 	return ufs_qcom_set_core_clk_ctrl(hba, false, freq);
 }
-- 
2.34.1

Re: [PATCH v5 0/8] Support Multi-frequency scale for UFS

[Luca Weiss]

Hi Ziqi,

On Mon Apr 28, 2025 at 10:06 AM CEST, Ziqi Chen wrote:
> Hi Luca,
>
> We made changes to fix this special platform issue and verified it can
> fix this issue.
> Could you help double check if attached 3 patched can fix it from you side?
> If it is OK from you side as well, we will submit the final patches to
> upstream

With these 3 patches applied the errors are gone and I don't see any
UFS-related warnings/errors in dmesg anymore.

Let me know if I should check on anything else. Thanks for the quick
fix!

Regards
Luca

>
> Thanks a lot~
>
> BRs
> Ziqi
>
> On 4/27/2025 4:14 PM, Ziqi Chen wrote:
>> Hi Luca,
>> 
>> Thanks for your report.
>> Really,  6350 is a special platform that the UFS_PHY_AXI_CLK doesn't
>> match to the UFS_PHY_UNIPRO_CORE_CLK. We already found out the root
>> cause and discussing the fix. We will submit change to fix this corner
>> case.
>> 
>> BRs
>> Ziqi
>> 
>> On 4/26/2025 3:48 AM, Luca Weiss wrote:
>>> Hi Ziqi,
>>>
>>> On Thu Feb 13, 2025 at 9:00 AM CET, Ziqi Chen wrote:
>>>> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
>>>> plans. However, the gear speed is only toggled between min and max 
>>>> during
>>>> clock scaling. Enable multi-level gear scaling by mapping clock 
>>>> frequencies
>>>> to gear speeds, so that when devfreq scales clock frequencies we can put
>>>> the UFS link at the appropraite gear speeds accordingly.
>>>
>>> I believe this series is causing issues on SM6350:
>>>
>>> [    0.859449] ufshcd-qcom 1d84000.ufshc: ufs_qcom_freq_to_gear_speed: 
>>> Unsupported clock freq : 200000000
>>> [    0.886668] ufshcd-qcom 1d84000.ufshc: UNIPRO clk freq 200 MHz not 
>>> supported
>>> [    0.903791] devfreq 1d84000.ufshc: dvfs failed with (-22) error
>>>
>>> That's with this patch, I actually haven't tried without on v6.15-rc3
>>> https://lore.kernel.org/all/20250314-sm6350-ufs-things- 
>>> v1-2-3600362cc52c@fairphone.com/
>>>
>>> I believe the issue appears because core clk and unipro clk rates don't
>>> match on this platform, so this 200 MHz for GCC_UFS_PHY_AXI_CLK is not a
>>> valid unipro clock rate, but for GCC_UFS_PHY_UNIPRO_CORE_CLK it's
>>> specified to 150 MHz in the opp table.
>>>
>>> Regards
>>> Luca
>>>
>>>>
>>>> This series has been tested on below platforms -
>>>> sm8550 mtp + UFS3.1
>>>> SM8650 MTP + UFS3.1
>>>> SM8750 MTP + UFS4.0
>>