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

[Manivannan Sadhasivam]

On Tue, Jun 09, 2026 at 03:17:23AM +0530, 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 transition phase to include
> iface clock and clock-names, attaching the opp-table to core clock
> remains optional 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.
> 
> Reviewed-by: Harshal Dev <harshal.dev@xxxxxxxxxxxxxxxx>
> Signed-off-by: Abhinaba Rakshit <abhinaba.rakshit@xxxxxxxxxxxxxxxx>
> ---
>  drivers/soc/qcom/ice.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/soc/qcom/ice.h |  2 ++
>  2 files changed, 95 insertions(+)
> 
> 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.

> +	if (ice->has_opp && ice->core_clk_freq)

Can core clk be 0 if OPP is used?

> +		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.

> +	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

> + *         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;

> +}
> +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.

> +	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.

> +	 */
> +	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?

- Mani

-- 
மணிவண்ணன் சதாசிவம்