Since Qualcomm inline-crypto engine (ICE) is now a dedicated driver
de-coupled from the QCOM UFS driver, it should explicitly vote for it's
needed resources during probe, specifically the UFS_PHY_GDSC power-domain
and the 'core' and 'iface' clocks.
Also updated the suspend and resume callbacks to handle votes on these
resources.
Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
drivers/soc/qcom/ice.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
index b203bc685cad..4b50d05ca02a 100644
--- a/drivers/soc/qcom/ice.c
+++ b/drivers/soc/qcom/ice.c
@@ -16,6 +16,8 @@
#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
#include <linux/firmware/qcom/qcom_scm.h>
@@ -108,6 +110,7 @@ struct qcom_ice {
void __iomem *base;
struct clk *core_clk;
+ struct clk *iface_clk;
bool use_hwkm;
bool hwkm_init_complete;
u8 hwkm_version;
@@ -310,12 +313,20 @@ int qcom_ice_resume(struct qcom_ice *ice)
struct device *dev = ice->dev;
int err;
+ pm_runtime_get_sync(dev);
err = clk_prepare_enable(ice->core_clk);
if (err) {
dev_err(dev, "failed to enable core clock (%d)\n",
err);
return err;
}
+
+ err = clk_prepare_enable(ice->iface_clk);
+ if (err) {
+ dev_err(dev, "failed to enable iface clock (%d)\n",
+ err);
+ return err;
+ }
qcom_ice_hwkm_init(ice);
return qcom_ice_wait_bist_status(ice);
}
@@ -323,7 +334,9 @@ EXPORT_SYMBOL_GPL(qcom_ice_resume);
int qcom_ice_suspend(struct qcom_ice *ice)
{
+ clk_disable_unprepare(ice->iface_clk);
clk_disable_unprepare(ice->core_clk);
+ pm_runtime_put_sync(ice->dev);
ice->hwkm_init_complete = false;
return 0;
@@ -584,6 +597,10 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
if (IS_ERR(engine->core_clk))
return ERR_CAST(engine->core_clk);
+ engine->iface_clk = devm_clk_get_enabled(dev, "iface_clk");
+ if (IS_ERR(engine->iface_clk))
+ return ERR_CAST(engine->iface_clk);
+
if (!qcom_ice_check_supported(engine))
return ERR_PTR(-EOPNOTSUPP);
@@ -725,6 +742,9 @@ static int qcom_ice_probe(struct platform_device *pdev)
return PTR_ERR(base);
}
+ devm_pm_runtime_enable(&pdev->dev);
+ pm_runtime_get_sync(&pdev->dev);
+
engine = qcom_ice_create(&pdev->dev, base);
if (IS_ERR(engine))
return PTR_ERR(engine);
--
2.34.1On 23/01/2026 08:11, Harshal Dev wrote:
> Since Qualcomm inline-crypto engine (ICE) is now a dedicated driver
> de-coupled from the QCOM UFS driver, it should explicitly vote for it's
> needed resources during probe, specifically the UFS_PHY_GDSC power-domain
> and the 'core' and 'iface' clocks.
> Also updated the suspend and resume callbacks to handle votes on these
> resources.
>
> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
> ---
> drivers/soc/qcom/ice.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index b203bc685cad..4b50d05ca02a 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -16,6 +16,8 @@
> #include <linux/of.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
>
> #include <linux/firmware/qcom/qcom_scm.h>
>
> @@ -108,6 +110,7 @@ struct qcom_ice {
> void __iomem *base;
>
> struct clk *core_clk;
> + struct clk *iface_clk;
> bool use_hwkm;
> bool hwkm_init_complete;
> u8 hwkm_version;
> @@ -310,12 +313,20 @@ int qcom_ice_resume(struct qcom_ice *ice)
> struct device *dev = ice->dev;
> int err;
>
> + pm_runtime_get_sync(dev);
> err = clk_prepare_enable(ice->core_clk);
> if (err) {
> dev_err(dev, "failed to enable core clock (%d)\n",
> err);
> return err;
> }
> +
> + err = clk_prepare_enable(ice->iface_clk);
> + if (err) {
> + dev_err(dev, "failed to enable iface clock (%d)\n",
> + err);
> + return err;
> + }
> qcom_ice_hwkm_init(ice);
> return qcom_ice_wait_bist_status(ice);
> }
> @@ -323,7 +334,9 @@ EXPORT_SYMBOL_GPL(qcom_ice_resume);
>
> int qcom_ice_suspend(struct qcom_ice *ice)
> {
> + clk_disable_unprepare(ice->iface_clk);
> clk_disable_unprepare(ice->core_clk);
> + pm_runtime_put_sync(ice->dev);
> ice->hwkm_init_complete = false;
>
> return 0;
> @@ -584,6 +597,10 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
> if (IS_ERR(engine->core_clk))
> return ERR_CAST(engine->core_clk);
>
> + engine->iface_clk = devm_clk_get_enabled(dev, "iface_clk");
> + if (IS_ERR(engine->iface_clk))
> + return ERR_CAST(engine->iface_clk);
And here actual breakage of ALL in-tree and out-of-tree DTS.
NAK.
Please read internal guideline.
Best regards,
Krzysztof
On 23/01/2026 09:58, Krzysztof Kozlowski wrote: >> >> return 0; >> @@ -584,6 +597,10 @@ static struct qcom_ice *qcom_ice_create(struct device *dev, >> if (IS_ERR(engine->core_clk)) >> return ERR_CAST(engine->core_clk); >> >> + engine->iface_clk = devm_clk_get_enabled(dev, "iface_clk"); >> + if (IS_ERR(engine->iface_clk)) >> + return ERR_CAST(engine->iface_clk); > > And here actual breakage of ALL in-tree and out-of-tree DTS. > > NAK. > > Please read internal guideline. Internal docs are pretty scattered and messy so I failed to find this there, which is surprising because this was frequent feedback. Therefore please update Kernel Upstreaming internal page with following: With few exceptions, it is not allowed to break the ABI, by making bindings or driver changes, where the existing or out of tree DTS would fail to boot. Updating in-tree DTS does not matter here, because DTS goes via different branch, thus driver branch would be always broken. This is explicitly documented in DT rules and explained also in maintainer-soc profile. You need to either provide strong justification for ABI break or make the changes backwards compatible. Best regards, Krzysztof
Hi Krzysztof, On 1/23/2026 4:27 PM, Krzysztof Kozlowski wrote: > On 23/01/2026 09:58, Krzysztof Kozlowski wrote: >>> >>> return 0; >>> @@ -584,6 +597,10 @@ static struct qcom_ice *qcom_ice_create(struct device *dev, >>> if (IS_ERR(engine->core_clk)) >>> return ERR_CAST(engine->core_clk); >>> >>> + engine->iface_clk = devm_clk_get_enabled(dev, "iface_clk"); >>> + if (IS_ERR(engine->iface_clk)) >>> + return ERR_CAST(engine->iface_clk); >> >> And here actual breakage of ALL in-tree and out-of-tree DTS. >> >> NAK. >> >> Please read internal guideline. > > Internal docs are pretty scattered and messy so I failed to find this > there, which is surprising because this was frequent feedback. Therefore > please update Kernel Upstreaming internal page with following: > > With few exceptions, it is not allowed to break the ABI, by making > bindings or driver changes, where the existing or out of tree DTS would > fail to boot. Updating in-tree DTS does not matter here, because DTS > goes via different branch, thus driver branch would be always broken. > This is explicitly documented in DT rules and explained also in > maintainer-soc profile. > > You need to either provide strong justification for ABI break or make > the changes backwards compatible. > Ack and understood. Let me write this in a way that makes it backward compatible by using devm_clk_get_optional_enabled(). Like I explained, for Linux distros where CONFIG_SCSI_UFS_QCOM is override set to 'y'. This clock vote isn't really needed during probe. In qcom_ice_suspend/resume(). I'll only prepare/un-prepare this clock if it was found during probe. Hope this sounds good from DT perspective. > Best regards, > Krzysztof Thanks, Harshal
On 1/23/26 12:12 PM, Harshal Dev wrote: > Hi Krzysztof, > > On 1/23/2026 4:27 PM, Krzysztof Kozlowski wrote: >> On 23/01/2026 09:58, Krzysztof Kozlowski wrote: >>>> >>>> return 0; >>>> @@ -584,6 +597,10 @@ static struct qcom_ice *qcom_ice_create(struct device *dev, >>>> if (IS_ERR(engine->core_clk)) >>>> return ERR_CAST(engine->core_clk); >>>> >>>> + engine->iface_clk = devm_clk_get_enabled(dev, "iface_clk"); >>>> + if (IS_ERR(engine->iface_clk)) >>>> + return ERR_CAST(engine->iface_clk); >>> >>> And here actual breakage of ALL in-tree and out-of-tree DTS. >>> >>> NAK. >>> >>> Please read internal guideline. >> >> Internal docs are pretty scattered and messy so I failed to find this >> there, which is surprising because this was frequent feedback. Therefore >> please update Kernel Upstreaming internal page with following: >> >> With few exceptions, it is not allowed to break the ABI, by making >> bindings or driver changes, where the existing or out of tree DTS would >> fail to boot. Updating in-tree DTS does not matter here, because DTS >> goes via different branch, thus driver branch would be always broken. >> This is explicitly documented in DT rules and explained also in >> maintainer-soc profile. >> >> You need to either provide strong justification for ABI break or make >> the changes backwards compatible. If the ICE can not be powered on alone without this change (i.e. no UFS, just ICE), then please spell it out explicitly, Harshal. That makes for a valid reason to break the ABI. Plus the fact that without an OPP table, the voltage requirements cannot be guaranteed to be met > > Ack and understood. Let me write this in a way that makes it backward > compatible by using devm_clk_get_optional_enabled(). Like I explained, for > Linux distros where CONFIG_SCSI_UFS_QCOM is override set to 'y'. This > clock vote isn't really needed during probe. This is really a side-effect that we shouldn't be depending on, or even considering as a backup, since the UFS driver may change independently and stop behaving this way one day > In qcom_ice_suspend/resume(). I'll only prepare/un-prepare this clock > if it was found during probe. Clock APIs generally happily eat nullptrs Konrad
Hi Konrad, On 1/30/2026 4:16 PM, Konrad Dybcio wrote: > On 1/23/26 12:12 PM, Harshal Dev wrote: >> Hi Krzysztof, >> >> On 1/23/2026 4:27 PM, Krzysztof Kozlowski wrote: >>> On 23/01/2026 09:58, Krzysztof Kozlowski wrote: >>>>> >>>>> return 0; >>>>> @@ -584,6 +597,10 @@ static struct qcom_ice *qcom_ice_create(struct device *dev, >>>>> if (IS_ERR(engine->core_clk)) >>>>> return ERR_CAST(engine->core_clk); >>>>> >>>>> + engine->iface_clk = devm_clk_get_enabled(dev, "iface_clk"); >>>>> + if (IS_ERR(engine->iface_clk)) >>>>> + return ERR_CAST(engine->iface_clk); >>>> >>>> And here actual breakage of ALL in-tree and out-of-tree DTS. >>>> >>>> NAK. >>>> >>>> Please read internal guideline. >>> >>> Internal docs are pretty scattered and messy so I failed to find this >>> there, which is surprising because this was frequent feedback. Therefore >>> please update Kernel Upstreaming internal page with following: >>> >>> With few exceptions, it is not allowed to break the ABI, by making >>> bindings or driver changes, where the existing or out of tree DTS would >>> fail to boot. Updating in-tree DTS does not matter here, because DTS >>> goes via different branch, thus driver branch would be always broken. >>> This is explicitly documented in DT rules and explained also in >>> maintainer-soc profile. >>> >>> You need to either provide strong justification for ABI break or make >>> the changes backwards compatible. > > If the ICE can not be powered on alone without this change (i.e. no UFS, > just ICE), then please spell it out explicitly, Harshal. That makes for a > valid reason to break the ABI. > > Plus the fact that without an OPP table, the voltage requirements cannot > be guaranteed to be met > Ack, I have endorsed and stated this point on the DT-binding commit. I'll wait for Krzysztof's view before updating the commit message to strongly reflect this point. >> >> Ack and understood. Let me write this in a way that makes it backward >> compatible by using devm_clk_get_optional_enabled(). Like I explained, for >> Linux distros where CONFIG_SCSI_UFS_QCOM is override set to 'y'. This >> clock vote isn't really needed during probe. > > This is really a side-effect that we shouldn't be depending on, or > even considering as a backup, since the UFS driver may change > independently and stop behaving this way one day > >> In qcom_ice_suspend/resume(). I'll only prepare/un-prepare this clock >> if it was found during probe. > > Clock APIs generally happily eat nullptrs Ack, then I guess in either case we can continue to keep the calls to prepare/un-prepare. Anyways, I am convinced we should specify and use the 'iface' clock. Thanks, Harshal > > Konrad
© 2016 - 2026 Red Hat, Inc.