Thread (16 messages) 16 messages, 3 authors, 2026-02-24

Re: [PATCH v5 2/4] soc: qcom: ice: Add OPP-based clock scaling support for ICE

From: Konrad Dybcio <hidden>
Date: 2026-02-16 12:19:04
Also in: linux-arm-msm, linux-devicetree, linux-scsi, lkml

On 2/13/26 8:02 AM, Abhinaba Rakshit wrote:
On Thu, Feb 12, 2026 at 12:30:00PM +0100, Konrad Dybcio wrote:
quoted
On 2/11/26 10:47 AM, Abhinaba Rakshit wrote:
quoted
Register optional operation-points-v2 table for ICE device
and aquire its minimum and maximum frequency during ICE
device probe.
[...]
quoted
quoted
+	if (!ice->has_opp)
+		return -EOPNOTSUPP;
+
+	/* Clamp the freq to max if target_freq is beyond supported frequencies */
+	if (ice->max_freq && target_freq >= ice->max_freq) {
+		ice_freq = ice->max_freq;
+		goto scale_clock;
+	}
+
+	/* Clamp the freq to min if target_freq is below supported frequencies */
+	if (ice->min_freq && target_freq <= ice->min_freq) {
+		ice_freq = ice->min_freq;
+		goto scale_clock;
+	}
The OPP framework won't let you overclock the ICE if this is what these checks
are about. Plus the clk framework will perform rounding for you too
Right, maybe I can just add a check for 0 freq just to ensure the export API is
not miss used.
Something shown below:

if (!target_freq)
    return -EINVAL;

However, my main concern was for the corner cases, where:
(target_freq > max && ROUND_CEIL)
and
(target_freq < min && ROUND_FLOOR)
In both the cases, the OPP APIs will fail and the clock remains unchanged.
I would argue that's expected behavior, if the requested rate can not
be achieved, the "set_rate"-like function should fail
Hence, I added the checks to make the API as generic/robust as possible.
AFAICT we generally set storage_ctrl_rate == ice_clk_rate with some slight
play, but the latter never goes above the FMAX of the former

For the second case, I'm not sure it's valid. For "find lowest rate" I would
expect find_freq_*ceil*(rate=0). For other cases of scale-down I would expect
that we want to keep the clock at >= (or ideally == )storage_ctrl_clk anyway
so I'm not sure _floor() is useful
Please let me know, your thoughts.
quoted
quoted
+
+	switch (flags) {
Are you going to use these flags? Currently they're dead code
I agree, currently they are not used.
However, since its an export API, I want to keep the rounding FLAGS
support as it a common to have rounding flags in clock scaling APIs,
and to support any future use-cases as well.
I think you have a bit of a misconception - yes, this is an export API and
should be designed with the consumers in mind, but then it's consumed by
in-tree modules only ("what's not on the list doesn't exist"), so it's actually
generally *discouraged* (with varying levels of emphasis) to add any code that
is not immediately useful, as these functions can be updated at any point in
time down the line

Konrad
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help