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

Abhinaba Rakshit
2026-04-07T23:45:38Z

On Fri, Apr 03, 2026 at 10:50:29PM +0530, Harshal Dev wrote:
> >>> +{
> >>> + unsigned long ice_freq = target_freq;
> >>> + struct dev_pm_opp *opp;
> >>> + int ret;
> >>> +
> >>> + if (!ice->has_opp)
> >>> + return -EOPNOTSUPP;
> >>> +
>
> [...]
>
> >>> +
> >>> static struct qcom_ice *qcom_ice_create(struct device *dev,
> >>> - void __iomem *base)
> >>> + void __iomem *base,
> >>> + bool is_legacy_binding)
> >>
> >> You don't need to introduce is_legacy_binding.
> >>
> >> Since you only need to add the OPP table when this function gets called from ICE probe,
> >> you should not touch this function. Instead, you should call devm_pm_opp_of_add_table()
> >> in ICE probe before calling qcom_ice_create() then once qcom_ice_create() is success, you
> >> can store the clk rate in the returned qcom_ice *engine ptr by calling clk_get_rate().
> >
> > This was added as part of the review comment from Krzysztof:
> > https://lore.kernel.org/all/20260128-daft-seriema-of-promotion-c50eb5@quoll/
> >
> > While I agree moving this to qcom_ice_probe would be more cleaner without needing
> > to change the API, most of our initializing code for driver by parsing the DT node
> > happens through qcom_ice_create, which keeps qcom_ice_probe much simpler.
> > Please let me know, if you think otherwise.
> >
>
> Seems like a suggestion from Krzysztof and not something based on strong opinion. Again,
> you can choose to do this if you spin a v8, I feel it's cleaner.

This comment raises an important design point around whether the OPP table
should be registered in qcom_ice_create versus qcom_ice_probe.
I agree that moving OPP-table registration to qcom_ice_probe would be a cleaner and
more maintainable approach. Doing so avoids the need to distinguish between legacy and
non-legacy bindings at the API level, and keeps qcom_ice_create reusable for both cases.
This also aligns well with the intent of qcom_ice_create, which should focus purely
on common/basic hardware initialization.

Additionally, qcom_ice_create currently has no dependency on the OPP table being
registered beforehand. Clock scaling is a performance optimization and does not
impose a hard requirement for ICE operation or enablement.
From that perspective, there is no technical necessity for OPP registration to
occur within qcom_ice_create.
Ack, will update it in v8 patchset.

> > Also, I don't see any reason for moving the clk_get_rate() logic to qcom_ice_probe
> > though as it will not be set on legacy targets in that case.
>
> I thought only new DT nodes will be specifying the OPP table requiring us to store the
> clk rate and restore later. If legacy DT nodes also possess the OPP table, then ignore
> this comment.

No, clk_rate is not needed for legacy bindings. It is only needed for DVFS operation
across suspend resume cycles.
Hence, its value makes sense only for non-legacy bindings.
It shoud not be faked for legacy bindings as clk rates can also be scaled from
storage driver in-case of legacy bindings.
Ack, will update in patchset v8.

> >
> >>> {
> >>> struct qcom_ice *engine;
> >>> + int err;
> >>>
> >>> if (!qcom_scm_is_available())
> >>> return ERR_PTR(-EPROBE_DEFER);
> >>> @@ -584,6 +640,26 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
> >>> if (IS_ERR(engine->core_clk))
> >>> return ERR_CAST(engine->core_clk);
> >>>
> >>> + /*
> >>> + * Register the OPP table only when ICE is described as a standalone
> >>> + * device node. Older platforms place ICE inside the storage controller
> >>> + * node, so they don't need an OPP table here, as they are handled in
> >>> + * storage controller.
> >>> + */
> >>> + if (!is_legacy_binding) {
> >>> + /* OPP table is optional */
> >>> + err = devm_pm_opp_of_add_table(dev);
> >>> + if (err && err != -ENODEV) {
> >>> + dev_err(dev, "Invalid OPP table in Device tree\n");
> >>> + return ERR_PTR(err);
> >>> + }
> >>> + engine->has_opp = (err == 0);
> >>
> >> Let's keep it readable and simple. engine->has_opps = true; here and false in error handle above.
> >
> > Well there are 3 cases to it:
> >
> > 1. err == 0 which implies devm_pm_opp_of_add_table is successful and we can set engine->has_opp =true.
> > 2. err == -ENODEV which implies there is no opp table in the DT node.
> > In that case, we don't fail the driver simply go ahead and log in the check below.
> > This is done since OPP-table is optional.
> > 3. err == any other error code. Something very wrong happened with devm_pm_opp_of_add_table
> > and driver should fail.
> >
> > Hence, we have the condition (err == 0) for setting has_opp flag.
>
> My suggestion is you either explain this in concise comments or simplify the assignment of has_opp
> to make it obvious.

Sure, will add appropriate comment here.

Abhinaba Rakshit