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

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

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