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

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

On Thu, Oct 02, 2025 at 01:21:22AM +0100, Bryan O'Donoghue wrote:
> 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.

Okay.
However, we will move to using OPP table and hence, this logic might change.

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

Yes, zero is valid here (means "no scaling"), will document it in patchset v2. 

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