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

On Mon, Feb 16, 2026 at 01:18:57PM +0100, Konrad Dybcio wrote:

On 2/13/26 8:02 AM, Abhinaba Rakshit wrote:

quoted

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

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

quoted

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

Clear, I guess, the idea is to ensure ice-clk <= storage-clk in case of scale_up
and ice-clk >= storage-clk in case of scale_down.

Best would be to use, _floor for scale_up and _ceil for scale_down.
 

quoted

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

Sure, understood
Will make sure to update in next patchset and make use of rounding flags.

Abhinaba Rakshit