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

[Harshal Dev]

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@xxxxxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxx>


>  	platform_set_drvdata(pdev, engine);
>  
>  	return 0;
> diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h
> index 4bee553f0a59d86ec6ce20f7c7b4bce28a706415..4eb58a264d416e71228ed4b13e7f53c549261fdc 100644
> --- a/include/soc/qcom/ice.h
> +++ b/include/soc/qcom/ice.h
> @@ -30,5 +30,7 @@ int qcom_ice_import_key(struct qcom_ice *ice,
>  			const u8 *raw_key, size_t raw_key_size,
>  			u8 lt_key[BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE]);
>  struct qcom_ice *devm_of_qcom_ice_get(struct device *dev);
> +int qcom_ice_scale_clk(struct qcom_ice *ice, unsigned long target_freq,
> +		       bool round_ceil);
>  
>  #endif /* __QCOM_ICE_H__ */
>