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

[Abhinaba Rakshit]

> > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> > index 5f20108aa03ebe9a47a10fba9afde420add0f34a..519d08c4727a6cb2dc5991216a2c042ed6218857 100644
> > --- a/drivers/soc/qcom/ice.c
> > +++ b/drivers/soc/qcom/ice.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/xarray.h>
> > +#include <linux/pm_opp.h>
> >  
> >  #include <linux/firmware/qcom/qcom_scm.h>
> >  
> > @@ -113,6 +114,8 @@ struct qcom_ice {
> >  	bool use_hwkm;
> >  	bool hwkm_init_complete;
> >  	u8 hwkm_version;
> > +	unsigned long core_clk_freq;
> > +	bool has_opp;
> >  };
> >  
> >  static DEFINE_XARRAY(ice_handles);
> > @@ -315,6 +318,10 @@ int qcom_ice_resume(struct qcom_ice *ice)
> >  	struct device *dev = ice->dev;
> >  	int err;
> >  
> > +	/* Restore the ICE core clk freq */
> 
> Redundant comment.

Ack. Will drop.

> > +	if (ice->has_opp && ice->core_clk_freq)
> 
> Can core clk be 0 if OPP is used?

In the current logic, core_clk_freq will always be non-zero if has_opp is true.
I included the check to decouple the two variables defensively, but I agree it's
redundant if we assume the OPP table is the sole source of frequency scaling here.
I will simplify this to *if (ice->has_opp)* and ensure core_clk_freq is initialized
within the OPP registration block.

> > +		dev_pm_opp_set_rate(ice->dev, ice->core_clk_freq);
> > +
> >  	err = clk_prepare_enable(ice->core_clk);
> >  	if (err) {
> >  		dev_err(dev, "Failed to enable core clock: %d\n", err);
> > @@ -335,6 +342,11 @@ int qcom_ice_suspend(struct qcom_ice *ice)
> >  {
> >  	clk_disable_unprepare(ice->iface_clk);
> >  	clk_disable_unprepare(ice->core_clk);
> > +
> > +	/* Drop the clock votes while suspend */
> 
> Redundant comment.

Ack. Will drop.

> > +	if (ice->has_opp)
> > +		dev_pm_opp_set_rate(ice->dev, 0);
> > +
> >  	ice->hwkm_init_complete = false;
> >  
> >  	return 0;
> > @@ -560,6 +572,51 @@ int qcom_ice_import_key(struct qcom_ice *ice,
> >  }
> >  EXPORT_SYMBOL_GPL(qcom_ice_import_key);
> >  
> > +/**
> > + * qcom_ice_scale_clk() - Scale ICE clock for DVFS-aware operations
> > + * @ice: ICE driver data
> > + * @target_freq: requested frequency in Hz
> > + * @round_ceil: when true, selects nearest freq >= @target_freq;
> > + *              otherwise, selects nearest freq <= @target_freq
> > + *
> > + * Selects an OPP frequency based on @target_freq and the rounding direction
> > + * specified by @round_ceil, then programs it using dev_pm_opp_set_rate(),
> > + * including any voltage or power-domain transitions handled by the OPP
> > + * framework. Updates ice->core_clk_freq on success.
> > + *
> > + * Return: 0 on success; -EOPNOTSUPP if no OPP table; or error from
> 
> s/error/errno

Ack. Will update.

> > + *         dev_pm_opp_set_rate()/OPP lookup.
> > + */
> > +int qcom_ice_scale_clk(struct qcom_ice *ice, unsigned long target_freq,
> > +		       bool round_ceil)
> > +{
> > +	unsigned long ice_freq = target_freq;
> > +	struct dev_pm_opp *opp;
> > +	int ret;
> > +
> > +	if (!ice->has_opp)
> > +		return -EOPNOTSUPP;
> > +
> > +	if (round_ceil)
> > +		opp = dev_pm_opp_find_freq_ceil(ice->dev, &ice_freq);
> > +	else
> > +		opp = dev_pm_opp_find_freq_floor(ice->dev, &ice_freq);
> > +
> > +	if (IS_ERR(opp))
> > +		return PTR_ERR(opp);
> > +	dev_pm_opp_put(opp);
> > +
> > +	ret = dev_pm_opp_set_rate(ice->dev, ice_freq);
> > +	if (ret) {
> > +		dev_err(ice->dev, "Unable to scale ICE clock rate\n");
> > +		return ret;
> > +	}
> > +	ice->core_clk_freq = ice_freq;
> > +
> > +	return ret;
> 
> return 0;

Ack. Will update.

> > +}
> > +EXPORT_SYMBOL_GPL(qcom_ice_scale_clk);
> > +
> >  static struct qcom_ice *qcom_ice_create(struct device *dev,
> >  					void __iomem *base)
> >  {
> > @@ -738,6 +795,7 @@ static int qcom_ice_probe(struct platform_device *pdev)
> >  	unsigned long phandle = pdev->dev.of_node->phandle;
> >  	struct qcom_ice *engine;
> >  	void __iomem *base;
> > +	int err;
> >  
> >  	guard(mutex)(&ice_mutex);
> >  
> > @@ -756,6 +814,41 @@ static int qcom_ice_probe(struct platform_device *pdev)
> >  		return PTR_ERR(engine);
> >  	}
> >  
> > +	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");
> > +		/* Store the error pointer for devm_of_qcom_ice_get() */
> > +		xa_store(&ice_handles, phandle, ERR_PTR(err), GFP_KERNEL);
> > +		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");
> > +		/* Store the error pointer for devm_of_qcom_ice_get() */
> > +		xa_store(&ice_handles, phandle, ERR_PTR(err), GFP_KERNEL);
> > +		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 err is not -ENODEV.
> > +	 */
> > +	if (err == -ENODEV)
> > +		dev_info(&pdev->dev, "ICE OPP table is not registered, please update your DT\n");
> 
> dev_dbg() please. No need to spam old DTs.

I intentionally used dev_info() here as it would provide a quick diagnostic hint
for KPI/performance regressions as mentioned in the cover-letter, which can be difficult
to trace. But I’m fine switching to dev_dbg() to avoid log noise if that’s preferred.

> > +	else
> > +		engine->has_opp = true;
> > +
> > +	/*
> > +	 * 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.
> 
> use full 80 column width for comments.

Ack. Will reformat the comment to include it within 80 columns.

> > +	 */
> > +	engine->core_clk_freq = clk_get_rate(engine->core_clk);
> 
> Why can't you conditionally cache the freq by moving it to the above else
> condition?

For core_clk_freq, I agree moving it under the else improves clarity and clearly
defines the purpose of the variable. I kept it outside earlier to avoid tying it
strictly to OPP presence, but I can move it for better readability.

Abhinaba Rakshit