Re: [PATCH v8 1/5] soc: qcom: ice: Add OPP-based clock scaling support for ICE

[Abhinaba Rakshit <abhinaba.rakshit@oss.qualcomm.com>]

On Fri, Apr 17, 2026 at 06:55:14PM +0530, Harshal Dev wrote:
> 
> 
> On 4/9/2026 5:14 PM, Abhinaba Rakshit wrote:
> > Register optional operation-points-v2 table for ICE device
> > during device probe. Attach the OPP-table with only the ICE
> > core clock. Since, dtbinding is on a trasition phase to include
> > iface clock and clock-names, attaching the opp-table to core clock
> > remains options such that it does not cause probe failures.
> > 
> > Introduce clock scaling API qcom_ice_scale_clk which scale ICE
> > core clock based on the target frequency provided and if a valid
> > OPP-table is registered. Use round_ceil passed to decide on the
> > rounding of the clock freq against OPP-table. Clock scaling is
> > disabled when a valid OPP-table is not registered.
> > 
> > This ensures when an ICE-device specific OPP table is available,
> > use the PM OPP framework to manage frequency scaling and maintain
> > proper power-domain constraints.
> > 
> > Also, ensure to drop the votes in suspend to prevent power/thermal
> > retention. Subsequently restore the frequency in resume from
> > core_clk_freq which stores the last ICE core clock operating frequency.
> > 
> > Signed-off-by: Abhinaba Rakshit <abhinaba.rakshit@oss.qualcomm.com>
> > ---
> >  drivers/soc/qcom/ice.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/soc/qcom/ice.h |  2 ++
> >  2 files changed, 94 insertions(+)
> > 
> > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> > index bf4ab2d9e5c0360d8fe6135cc35f93b6b09e7a0e..9e869e6abc6300c7608b4d9a18e7f3e80c93f5e7 100644
> > --- a/drivers/soc/qcom/ice.c
> > +++ b/drivers/soc/qcom/ice.c
> > @@ -16,6 +16,7 @@
> 
> [..]
> 
> > @@ -742,6 +800,40 @@ static int qcom_ice_probe(struct platform_device *pdev)
> >  	if (IS_ERR(engine))
> >  		return PTR_ERR(engine);
> >  
> > +	/* qcom_ice_create() may return NULL if scm calls are not available */
> > +	if (!engine)
> > +		return -EOPNOTSUPP;
> > +
> > +	err = devm_pm_opp_set_clkname(&pdev->dev, "core");
> > +	if (err && err != -ENOENT) {
> > +		dev_err(&pdev->dev, "Unable to set core clkname to OPP-table\n");
> > +		return err;
> > +	}
> > +
> > +	/* OPP table is optional */
> > +	err = devm_pm_opp_of_add_table(&pdev->dev);
> > +	if (err && err != -ENODEV) {
> > +		dev_err(&pdev->dev, "Invalid OPP table in Device tree\n");
> > +		return err;
> > +	}
> > +
> > +	/*
> > +	 * The OPP table is optional. devm_pm_opp_of_add_table() returns
> > +	 * -ENODEV when no OPP table is present in DT, which is not treated
> > +	 * as an error. Therefore, track successful OPP registration only
> > +	 * when the return value is 0.
> > +	 */
> > +	engine->has_opp = (err == 0);
> > +	if (!engine->has_opp)
> > +		dev_info(&pdev->dev, "ICE OPP table is not registered, please update your DT\n");
> > +
> > +	/*
> > +	 * Store the core clock rate for suspend resume cycles,
> > +	 * against OPP aware DVFS operations. core_clk_freq will
> > +	 * have a valid value only for non-legacy bindings.
> > +	 */
> > +	engine->core_clk_freq = clk_get_rate(engine->core_clk);
> > +
> 
> When you are calling 4-5 functions in a function, it's probably time to define another
> function to keep things simple. Maybe qcom_ice_attach_opp_table().
> 
> Also, I still have issues with engine->has_opp = (err == 0), mostly because I don't
> see this style used at other placed in the kernel. I would still suggest that you
> make it simpler, but I won't hard-request it.
> 
> /* The same explanatory comment as before */
> if (err == -ENODEV)
> 	engine->has_opp = false;
>         dev_info(...);
> else
> 	engine->has_opp = true;
> 
> With these optional suggestions, feel free to add:
> 
> Reviewed-by: Harshal Dev <harshal.dev@oss.qualcomm.com>

Thanks for the review.

Regarding the points you mentioned:
At the moment, not much is happening here beyond registering the OPP table and
caching the core clock rate. This is executed once per device probe, and the logic
is fairly localized to the ICE probe path. Because of that, it didn’t feel like its
reusable or demands its own helper.
That said, I do see your point about modularity. If this logic grows further, or if
we end up needing the same sequence elsewhere, I agree it would make sense to factor
it out into a separate function at that time.

Regarding engine->has_opp = (err == 0), I understand your concern.
However, I intend to keep it this way, as it keeps the flow consise avoiding much of
if-else branching and also serves the purpose.

Abhinaba Rakshit