[PATCH v2 0/4] devfreq: check the get_cur_freq() return value and use it in ufshcd

[PATCH v2 0/4] devfreq: check the get_cur_freq() return value and use it in ufshcd

[Bean Huo]

The devfreq core has three users of the optional ->get_cur_freq()
callback. Two of them check the return value, the third one does not and
passes an uninitialized frequency to the transition notifiers when the
callback fails. Patch 1 fixes that.

Patch 2 writes down what a driver is expected to return from the
callback. Today this has to be found by reading the devfreq core.

Patch 3 records the frequency the controller starts at.
ufshcd_init_clocks() puts the controller at its highest frequency, but
nothing writes that down, so clk_scaling.target_freq stays 0 and devfreq
starts with previous_freq at 0 as well. With use_pm_opp this makes
ufshcd_devfreq_get_dev_status() report 0 Hz, the ondemand governor then
asks for the maximum frequency, and ufshcd_devfreq_target() runs a full
ufshcd_devfreq_scale() that holds up the queue for up to a second only to
set the same OPP and the same gear again.

Patch 4 adds the ->get_cur_freq() callback to ufshcd. Without it the
cur_freq attribute shows the last frequency the governor selected, which
is wrong whenever the controller is scaled outside the governor, for
example after writing 0 to clkscale_enable.

The patches touch two subsystems. Patches 1 and 2 are for the devfreq
tree, patches 3 and 4 are for the SCSI tree. The two halves are
independent, at build time and at run time, and can be applied in either
order.

Patch was tested on a Radxa Dragon Q6A (1d84000.ufshc):

  before "echo 0 > clkscale_enable":  cur_freq 75000000, target_freq 75000000
  after  "echo 0 > clkscale_enable":  cur_freq 300000000, target_freq 75000000

Without it both files report 75000000 and keep doing so for as long as
clock scaling stays disabled. A 4 GiB direct read after enabling clock
scaling again counted the transitions in trans_stat and attributed time
to the 300000000 state, so the frequency the callback returns is one that
devfreq recognises.

One thing to be aware of: devfreq_monitor_resume() copies previous_freq
from the callback, but it does not call devfreq_update_status(). A
frequency change made while the governor was suspended therefore does not
show up as a transition. That is how devfreq behaves today and this
series does not change it.

Changes since v1:
- New patch 3, so that target_freq and devfreq's previous_freq are not 0
  at boot (suggested by Stanley Jhu).
- Patch 4: drop the !cur_freq check, it cannot happen any more.
- Drop the now stale comment in ufshcd_devfreq_get_dev_status().
- Patches 1 and 2 are unchanged.
- The devfreq and the ufshcd patches no longer depend on each other.
- Avri's Reviewed-by is on patches 1, 2 and 4. Patch 3 is new, so it does
  not carry it. Avri, please note that patch 4 changed since you reviewed
  it, the !cur_freq check is gone. Tell me if you want the tag dropped.


Bean Huo (4):
  PM / devfreq: Fall back to previous_freq when get_cur_freq() fails
  PM / devfreq: Add more details to the get_cur_freq() comment
  scsi: ufs: core: Record the frequency the controller starts at
  scsi: ufs: core: Report the current clock frequency to devfreq

 drivers/devfreq/devfreq.c |  5 ++---
 drivers/ufs/core/ufshcd.c | 37 +++++++++++++++++++++++++++++++------
 include/linux/devfreq.h   |  7 +++++--
 3 files changed, 38 insertions(+), 11 deletions(-)


base-commit: e30626823a406725ce29bc75cb8ec467d3e1e326
-- 
2.34.1

[PATCH v2 1/4] PM / devfreq: Fall back to previous_freq when get_cur_freq() fails

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

devfreq_set_target() calls the optional ->get_cur_freq() callback to get
the frequency that is passed as freqs.old to the DEVFREQ_PRECHANGE and
DEVFREQ_POSTCHANGE notifiers, but it does not check the return value. If
the callback fails without setting @freq, cur_freq is never assigned, and
an uninitialized stack value is passed to the notifiers.

hisi_uncore_get_cur_freq() can hit this. It returns -ENODEV without
setting @freq when its PCC channel is missing. On the mailbox error path
it sets @freq to 0 instead, so that the core does not read a random
value.

The other two callers, cur_freq_show() and devfreq_monitor_resume(),
already check the return value and use devfreq->previous_freq when the
callback fails. Do the same in devfreq_set_target().

This does not seem to cause a visible problem today. The passive governor
is the only DEVFREQ_TRANSITION_NOTIFIER user in the tree, and it only
reads freqs.new. So this patch is not marked for stable.

Fixes: 0fe3a66410a3 ("PM / devfreq: Add new DEVFREQ_TRANSITION_NOTIFIER notifier")
Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@sandisk.com>
---
 drivers/devfreq/devfreq.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index f08fc6966eae..f20d9a660779 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -348,9 +348,8 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
 	unsigned long cur_freq;
 	int err = 0;
 
-	if (devfreq->profile->get_cur_freq)
-		devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
-	else
+	if (!devfreq->profile->get_cur_freq ||
+	    devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq))
 		cur_freq = devfreq->previous_freq;
 
 	freqs.old = cur_freq;
-- 
2.34.1

[PATCH v2 2/4] PM / devfreq: Add more details to the get_cur_freq() comment

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

The comment for ->get_cur_freq() only says that the device should
provide the frequency at which it is operating. It does not tell the
driver author which unit to use, what the return value means, or that
the frequency should be one of @freq_table. These have to be found by
reading the devfreq core.

Add these details to the comment.

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@sandisk.com>
---
 include/linux/devfreq.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index dc1075dc3446..809ef29b9af5 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -90,8 +90,11 @@ struct devfreq_dev_status {
  *			use this directly. Instead, governors are recommended
  *			to use devfreq_update_stats() along with
  *			devfreq.last_status.
- * @get_cur_freq:	The device should provide the current frequency
- *			at which it is operating.
+ * @get_cur_freq:	The device should provide the frequency, in Hz, at
+ *			which it is currently operating, and return 0, or a
+ *			negative errno on failure. The frequency should be one
+ *			of @freq_table, as it is used for the transition
+ *			statistics.
  * @exit:		An optional callback that is called when devfreq
  *			is removing the devfreq object due to error or
  *			from devfreq_remove_device() call. If the user
-- 
2.34.1

[PATCH v2 3/4] scsi: ufs: core: Record the frequency the controller starts at

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

ufshcd_init_clocks() puts the controller at its highest frequency, but
nothing writes that down. clk_scaling.target_freq stays 0, and
devfreq_dev_profile.initial_freq is never set, so devfreq->previous_freq
is 0 as well.

With use_pm_opp this shows up in a few places. The target_freq attribute
reads 0 until the governor scales for the first time.
ufshcd_devfreq_get_dev_status() reports 0 Hz, which makes the ondemand
governor ask for the maximum frequency. ufshcd_devfreq_target() then
sees 0 != max and runs a full ufshcd_devfreq_scale(), which holds up the
queue for up to a second only to set the same OPP and the same gear
again. Without OPPs the frequency is not reported as 0, but
previous_freq is, and devfreq_update_status() then drops the first
time_in_state update.

Record the maximum frequency in ufshcd_devfreq_init() instead.
ufshcd_add_lus() runs after ufshcd_probe_hba() has geared up to
hba->max_pwr_info.info, so the clocks and the gear are both at their
maximum by the time we get here. The only difference is that the first
governor poll no longer redoes work that is already done. From the
second poll on nothing changes, because target_freq held the maximum
frequency there anyway.

That first scale also re-applied the gear that
ufshcd_vops_freq_to_gear_speed() maps the maximum frequency to, so it
quietly corrected the link if the OPP table and the gear negotiated at
probe disagreed. That does not happen any more. On ufs-qcom the two
cannot disagree, because ufs_qcom_negotiate_pwr_mode() clamps the gear
through ufshcd_negotiate_pwr_params() against the same controller
capability the OPP table is written from.

clki->max_freq is the right value in both modes.
ufshcd_parse_clock_min_max_freq() fills it from the highest OPP, and
ufshcd_clkscale_enable_store() already uses it the same way.

Suggested-by: Stanley Jhu <stanleyjhu@google.com>
Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/ufs/core/ufshcd.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 2ba244cf40ac..351c76094b9f 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1681,11 +1681,6 @@ static int ufshcd_devfreq_get_dev_status(struct device *dev,
 	if (!scaling->window_start_t)
 		goto start_window;
 
-	/*
-	 * If current frequency is 0, then the ondemand governor considers
-	 * there's no initial frequency set. And it always requests to set
-	 * to max. frequency.
-	 */
 	if (hba->use_pm_opp) {
 		stat->current_frequency = hba->clk_scaling.target_freq;
 	} else {
@@ -1727,12 +1722,21 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
 	if (list_empty(clk_list))
 		return 0;
 
+	clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+
 	if (!hba->use_pm_opp) {
-		clki = list_first_entry(clk_list, struct ufs_clk_info, list);
 		dev_pm_opp_add(hba->dev, clki->min_freq, 0);
 		dev_pm_opp_add(hba->dev, clki->max_freq, 0);
 	}
 
+	/*
+	 * ufshcd_init_clocks() has already set the clocks to the highest
+	 * frequency, and nothing has changed them since. Save that frequency,
+	 * so that devfreq and the clock scaling code know where we start.
+	 */
+	hba->clk_scaling.target_freq = clki->max_freq;
+	hba->vps->devfreq_profile.initial_freq = clki->max_freq;
+
 	ufshcd_vops_config_scaling_param(hba, &hba->vps->devfreq_profile,
 					 &hba->vps->ondemand_data);
 	devfreq = devfreq_add_device(hba->dev,
-- 
2.34.1

[PATCH v2 4/4] scsi: ufs: core: Report the current clock frequency to devfreq

[Bean Huo]

From: Bean Huo <beanhuo@micron.com>

When a driver does not provide a ->get_cur_freq() callback, the cur_freq
sysfs attribute shows devfreq->previous_freq, which only tracks the
scaling that the governor itself did.

The UFS controller is also scaled outside the governor. The clearest
example is writing 0 to clkscale_enable: ufshcd_clkscale_enable_store()
sets the clocks to max_freq through ufshcd_devfreq_scale() and suspends
the governor, so devfreq_set_target() is never called. After that,
cur_freq keeps showing the last frequency the governor chose instead of
the one the controller runs at, and it does so as long as clock scaling
stays disabled.

Add ufshcd_devfreq_get_cur_freq(). It reports clk_scaling.target_freq
when OPPs are used and the first clock's curr_freq otherwise, the same
values that ufshcd_devfreq_get_dev_status() reports.

Signed-off-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@sandisk.com>
---
 drivers/ufs/core/ufshcd.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 351c76094b9f..bac95e891de5 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1711,6 +1711,26 @@ static int ufshcd_devfreq_get_dev_status(struct device *dev,
 	return 0;
 }
 
+static int ufshcd_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	if (!ufshcd_is_clkscaling_supported(hba))
+		return -EINVAL;
+
+	if (hba->use_pm_opp) {
+		*freq = hba->clk_scaling.target_freq;
+	} else {
+		struct ufs_clk_info *clki;
+
+		clki = list_first_entry(&hba->clk_list_head,
+					struct ufs_clk_info, list);
+		*freq = clki->curr_freq;
+	}
+
+	return 0;
+}
+
 static int ufshcd_devfreq_init(struct ufs_hba *hba)
 {
 	struct list_head *clk_list = &hba->clk_list_head;
@@ -9616,6 +9636,7 @@ static struct ufs_hba_variant_params ufs_hba_vps = {
 	.devfreq_profile.polling_ms	= 100,
 	.devfreq_profile.target		= ufshcd_devfreq_target,
 	.devfreq_profile.get_dev_status	= ufshcd_devfreq_get_dev_status,
+	.devfreq_profile.get_cur_freq	= ufshcd_devfreq_get_cur_freq,
 	.ondemand_data.upthreshold	= 70,
 	.ondemand_data.downdifferential	= 5,
 };
-- 
2.34.1

Re: [PATCH v2 0/4] devfreq: check the get_cur_freq() return value and use it in ufshcd

[Stanley Jhu]

On Mon, 7 Sep 2026 21:21:36 +0200, Bean Huo wrote:
> Changes since v1:
> - New patch 3, so that target_freq and devfreq's previous_freq are not 0
>   at boot (suggested by Stanley Jhu).
> - Patch 4: drop the !cur_freq check, it cannot happen any more.

Thanks for addressing the initial frequency desync in v2.

For the series:
Reviewed-by: Stanley Jhu <stanleyjhu@google.com>

Re: [PATCH v2 0/4] devfreq: check the get_cur_freq() return value and use it in ufshcd

[Martin K. Petersen (Oracle)]

Bean,

> The patches touch two subsystems. Patches 1 and 2 are for the devfreq
> tree, patches 3 and 4 are for the SCSI tree. The two halves are
> independent, at build time and at run time, and can be applied in either
> order.

Patches 3 + 4 applied to 7.4/scsi-staging, thanks!

-- 
Martin K. Petersen