Re: [PATCH 1/2] soc: qcom: ice: enable ICE clock scaling API

[Bryan O'Donoghue <bod@kernel.org>]

On 01/10/2025 12:38, Abhinaba Rakshit wrote:
> Add ICE clock scaling API based on the parsed clk supported
> frequencies from dt entry.
> 
> Signed-off-by: Abhinaba Rakshit <abhinaba.rakshit@oss.qualcomm.com>
> ---
>   drivers/soc/qcom/ice.c | 25 +++++++++++++++++++++++++
>   include/soc/qcom/ice.h |  1 +
>   2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index c467b55b41744ebec0680f5112cc4bb1ba00c513..ec8d6bb9f426deee1038616282176bfc8e5b9ec1 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -97,6 +97,8 @@ struct qcom_ice {
>   	struct clk *core_clk;
>   	bool use_hwkm;
>   	bool hwkm_init_complete;
> +	u32 max_freq;
> +	u32 min_freq;
>   };
> 
>   static bool qcom_ice_check_supported(struct qcom_ice *ice)
> @@ -514,10 +516,25 @@ int qcom_ice_import_key(struct qcom_ice *ice,
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_import_key);
> 
> +int qcom_ice_scale_clk(struct qcom_ice *ice, bool scale_up)
> +{
> +	int ret = 0;
> +
> +	if (scale_up && ice->max_freq)
> +		ret = clk_set_rate(ice->core_clk, ice->max_freq);
> +	else if (!scale_up && ice->min_freq)
> +		ret = clk_set_rate(ice->core_clk, ice->min_freq);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_scale_clk);
> +
>   static struct qcom_ice *qcom_ice_create(struct device *dev,
>   					void __iomem *base)
>   {
>   	struct qcom_ice *engine;
> +	const __be32 *prop;
> +	int len;
> 
>   	if (!qcom_scm_is_available())
>   		return ERR_PTR(-EPROBE_DEFER);
> @@ -549,6 +566,14 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>   	if (IS_ERR(engine->core_clk))
>   		return ERR_CAST(engine->core_clk);
> 
> +	prop = of_get_property(dev->of_node, "freq-table-hz", &len);
> +	if (!prop || len < 2 * sizeof(uint32_t)) {
> +		dev_err(dev, "Freq-hz property not found or invalid length\n");

If this error really happened you should pus the result code up the call 
stack also since either case can be an error you can inform the user of 
which error happened in your output string.

> +	} else {
> +		engine->min_freq = be32_to_cpu(prop[0]);
> +		engine->max_freq = be32_to_cpu(prop[1]);

You check for zero later on in the code but, is zero a valid value to be 
returned here ?

e.g. is it valid to specify "freq-table-hz" in your DT but then set max 
to zero ? min to zero ?

If not you may as well reject zero and dispense with the checks later on.

> +	}
> +
>   	if (!qcom_ice_check_supported(engine))
>   		return ERR_PTR(-EOPNOTSUPP);
> 
> diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h
> index 4bee553f0a59d86ec6ce20f7c7b4bce28a706415..b701ec9e062f70152f6dea8bf6c4637ab6ef20f1 100644
> --- a/include/soc/qcom/ice.h
> +++ b/include/soc/qcom/ice.h
> @@ -30,5 +30,6 @@ 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, bool scale_up);
> 
>   #endif /* __QCOM_ICE_H__ */
> 
> --
> 2.34.1
> 
>