[PATCH 00/11] Add explicit power-domain and clock voting for QCOM-ICE

[PATCH 00/11] Add explicit power-domain and clock voting for QCOM-ICE

[Harshal Dev]

When the kernel is booted without the 'clk_ignore_unused' command‑line
flag, votes for unused clocks and power domains are dropped by the kernel
post late_init and deferred probe timeout. Depending on the relative
timing between the ICE probe and the kernel disabling the unused clocks
and power domains occasional unclocked register accesses or 'stuck' clocks
are observed during QCOM‑ICE probe.
When the 'iface' clock is not voted on, unclocked register access would
be observed. On the other hand, if the associated power-domain for ICE
is not enabled, a 'stuck' clock is observed.

This patch series resolves both of these problems by adding explicit
power‑domain enablement and 'iface' clock‑vote handling to the QCOM‑ICE
driver.

The clock 'stuck' issue was first reported on Qualcomm RideSX4 (sa8775p)
platform: https://lore.kernel.org/all/ZZYTYsaNUuWQg3tR@x1/

Issue with unclocked ICE register access is easily reproducible on
on Qualcomm RB3Gen2 (kodiak) platform when 'clk_ignore_unused' is
not passed on the kernel command-line.

This patch series has been validated on: SM8650-MTP, RB3Gen2 and
Lemans-EVK.

Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
Harshal Dev (11):
      dt-bindings: crypto: qcom,ice: Require power-domain and iface clk
      arm64: dts: qcom: kaanpali: Add power-domain and iface clk for ice node
      arm64: dts: qcom: lemans: Add power-domain and iface clk for ice node
      arm64: dts: qcom: monaco: Add power-domain and iface clk for ice node
      arm64: dts: qcom: sc7180: Add power-domain and iface clk for ice node
      arm64: dts: qcom: kodiak: Add power-domain and iface clk for ice node
      arm64: dts: qcom: sm8450: Add power-domain and iface clk for ice node
      arm64: dts: qcom: sm8550: Add power-domain and iface clk for ice node
      arm64: dts: qcom: sm8650: Add power-domain and iface clk for ice node
      arm64: dts: qcom: sm8750: Add power-domain and iface clk for ice node
      soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

 .../bindings/crypto/qcom,inline-crypto-engine.yaml   | 14 +++++++++++++-
 arch/arm64/boot/dts/qcom/kaanapali.dtsi              |  6 +++++-
 arch/arm64/boot/dts/qcom/kodiak.dtsi                 |  6 +++++-
 arch/arm64/boot/dts/qcom/lemans.dtsi                 |  6 +++++-
 arch/arm64/boot/dts/qcom/monaco.dtsi                 |  6 +++++-
 arch/arm64/boot/dts/qcom/sc7180.dtsi                 |  6 +++++-
 arch/arm64/boot/dts/qcom/sm8450.dtsi                 |  6 +++++-
 arch/arm64/boot/dts/qcom/sm8550.dtsi                 |  6 +++++-
 arch/arm64/boot/dts/qcom/sm8650.dtsi                 |  6 +++++-
 arch/arm64/boot/dts/qcom/sm8750.dtsi                 |  6 +++++-
 drivers/soc/qcom/ice.c                               | 20 ++++++++++++++++++++
 11 files changed, 78 insertions(+), 10 deletions(-)
---
base-commit: 0f853ca2a798ead9d24d39cad99b0966815c582a
change-id: 20260120-qcom_ice_power_and_clk_vote-769704f5036a

Best regards,
-- 
Harshal Dev <hdev@qti.qualcomm.com>

[PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Harshal Dev]

Update the inline-crypto engine DT binding to reflect that power-domain and
clock-names are now mandatory. Also update the maximum number of clocks
that can be specified to two. These new fields are mandatory because ICE
needs to vote on the power domain before it attempts to vote on the core
and iface clocks to avoid clock 'stuck' issues.

Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
 .../bindings/crypto/qcom,inline-crypto-engine.yaml         | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
index c3408dcf5d20..1c2416117d4c 100644
--- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
+++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
@@ -28,12 +28,20 @@ properties:
     maxItems: 1
 
   clocks:
+    maxItems: 2
+
+  clock-names:
+    maxItems: 2
+
+  power-domains:
     maxItems: 1
 
 required:
   - compatible
   - reg
   - clocks
+  - clock-names
+  - power-domains
 
 additionalProperties: false
 
@@ -45,6 +53,10 @@ examples:
       compatible = "qcom,sm8550-inline-crypto-engine",
                    "qcom,inline-crypto-engine";
       reg = <0x01d88000 0x8000>;
-      clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
+      clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>,
+               <&gcc GCC_UFS_PHY_AHB_CLK>;
+      clock-names = "ice_core_clk",
+                    "iface_clk";
+      power-domains = <&gcc UFS_PHY_GDSC>;
     };
 ...

-- 
2.34.1

[PATCH 02/11] arm64: dts: qcom: kaanpali: Add power-domain and iface clk for ice node

[Harshal Dev]

Qualcomm in-line crypto engine (ICE) platform driver specifies and votes
for it's own resources. Before accessing ICE hardware, the 'core' and
'iface' clocks must be turned on by the driver. This can only be done if
the GCC_UFS_PHY_GDSC power domain is enabled. Specify both the
GCC_UFS_PHY_GDSC power domain and 'core' and 'iface' clocks in the ICE node
for kaanapali.

Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/kaanapali.dtsi | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/kaanapali.dtsi b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
index 9ef57ad0ca71..7dea93d80636 100644
--- a/arch/arm64/boot/dts/qcom/kaanapali.dtsi
+++ b/arch/arm64/boot/dts/qcom/kaanapali.dtsi
@@ -868,7 +868,11 @@ ice: crypto@1d88000 {
 				     "qcom,inline-crypto-engine";
 			reg = <0x0 0x01d88000 0x0 0x18000>;
 
-			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
+			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>,
+				 <&gcc GCC_UFS_PHY_AHB_CLK>;
+			clock-names = "ice_core_clk",
+				      "iface_clk";
+			power-domains = <&gcc GCC_UFS_PHY_GDSC>;
 		};
 
 		tcsr_mutex: hwlock@1f40000 {

-- 
2.34.1

[PATCH 03/11] arm64: dts: qcom: lemans: Add power-domain and iface clk for ice node

[Harshal Dev]

Qualcomm in-line crypto engine (ICE) platform driver specifies and votes
for it's own resources. Before accessing ICE hardware, the 'core' and
'iface' clocks must be turned on by the driver. This can only be done if
the UFS_PHY_GDSC power domain is enabled. Specify both the UFS_PHY_GDSC
power domain and 'core' and 'iface' clocks in the ICE node for lemans.

Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/lemans.dtsi | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/lemans.dtsi b/arch/arm64/boot/dts/qcom/lemans.dtsi
index 7c46f493300c..0312702020d5 100644
--- a/arch/arm64/boot/dts/qcom/lemans.dtsi
+++ b/arch/arm64/boot/dts/qcom/lemans.dtsi
@@ -2774,7 +2774,11 @@ ice: crypto@1d88000 {
 			compatible = "qcom,sa8775p-inline-crypto-engine",
 				     "qcom,inline-crypto-engine";
 			reg = <0x0 0x01d88000 0x0 0x18000>;
-			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
+			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>,
+				 <&gcc GCC_UFS_PHY_AHB_CLK>;
+			clock-names = "ice_core_clk",
+				      "iface_clk";
+			power-domains = <&gcc UFS_PHY_GDSC>;
 		};
 
 		cryptobam: dma-controller@1dc4000 {

-- 
2.34.1

[PATCH 04/11] arm64: dts: qcom: monaco: Add power-domain and iface clk for ice node

[Harshal Dev]

Qualcomm in-line crypto engine (ICE) platform driver specifies and votes
for it's own resources. Before accessing ICE hardware, the 'core' and
'iface' clocks must be turned on by the driver. This can only be done if
the GCC_UFS_PHY_GDSC power domain is enabled. Specify both the
GCC_UFS_PHY_GDSC power domain and 'core' and 'iface' clocks in the ICE node
for monaco.

Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/monaco.dtsi | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/monaco.dtsi b/arch/arm64/boot/dts/qcom/monaco.dtsi
index 83780cd3dc8a..b97a35ef7d6d 100644
--- a/arch/arm64/boot/dts/qcom/monaco.dtsi
+++ b/arch/arm64/boot/dts/qcom/monaco.dtsi
@@ -2725,7 +2725,11 @@ ice: crypto@1d88000 {
 			compatible = "qcom,qcs8300-inline-crypto-engine",
 				     "qcom,inline-crypto-engine";
 			reg = <0x0 0x01d88000 0x0 0x18000>;
-			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
+			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>,
+				 <&gcc GCC_UFS_PHY_AHB_CLK>;
+			clock-names = "ice_core_clk",
+				      "iface_clk";
+			power-domains = <&gcc GCC_UFS_PHY_GDSC>;
 		};
 
 		crypto: crypto@1dfa000 {

-- 
2.34.1

[PATCH 05/11] arm64: dts: qcom: sc7180: Add power-domain and iface clk for ice node

[Harshal Dev]

Qualcomm in-line crypto engine (ICE) platform driver specifies and votes
for it's own resources. Before accessing ICE hardware, the 'core' and
'iface' clocks must be turned on by the driver. This can only be done if
the UFS_PHY_GDSC power domain is enabled. Specify both the UFS_PHY_GDSC
power domain and 'core' and 'iface' clocks in the ICE node for sc7180.

Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 45b9864e3304..74eb895cf4da 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -1605,7 +1605,11 @@ ice: crypto@1d90000 {
 			compatible = "qcom,sc7180-inline-crypto-engine",
 				     "qcom,inline-crypto-engine";
 			reg = <0 0x01d90000 0 0x8000>;
-			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
+			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>,
+				 <&gcc GCC_UFS_PHY_AHB_CLK>;
+			clock-names = "ice_core_clk",
+				      "iface_clk";
+			power-domains = <&gcc UFS_PHY_GDSC>;
 		};
 
 		ipa: ipa@1e40000 {

-- 
2.34.1

[PATCH 06/11] arm64: dts: qcom: kodiak: Add power-domain and iface clk for ice node

[Harshal Dev]

Qualcomm in-line crypto engine (ICE) platform driver specifies and votes
for it's own resources. Before accessing ICE hardware, the 'core' and
'iface' clocks must be turned on by the driver. This can only be done if
the GCC_UFS_PHY_GDSC power domain is enabled. Specify both the
GCC_UFS_PHY_GDSC power domain and 'core' and 'iface' clocks in the ICE node
for kodiak.

Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/kodiak.dtsi | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/kodiak.dtsi b/arch/arm64/boot/dts/qcom/kodiak.dtsi
index f478c5e1d2d5..8f2bda7af74c 100644
--- a/arch/arm64/boot/dts/qcom/kodiak.dtsi
+++ b/arch/arm64/boot/dts/qcom/kodiak.dtsi
@@ -2574,7 +2574,11 @@ ice: crypto@1d88000 {
 			compatible = "qcom,sc7280-inline-crypto-engine",
 				     "qcom,inline-crypto-engine";
 			reg = <0 0x01d88000 0 0x8000>;
-			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
+			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>,
+				 <&gcc GCC_UFS_PHY_AHB_CLK>;
+			clock-names = "ice_core_clk",
+				      "iface_clk";
+			power-domains = <&gcc GCC_UFS_PHY_GDSC>;
 		};
 
 		cryptobam: dma-controller@1dc4000 {

-- 
2.34.1

[PATCH 07/11] arm64: dts: qcom: sm8450: Add power-domain and iface clk for ice node

[Harshal Dev]

Qualcomm in-line crypto engine (ICE) platform driver specifies and votes
for it's own resources. Before accessing ICE hardware, the 'core' and
'iface' clocks must be turned on by the driver. This can only be done if
the UFS_PHY_GDSC power domain is enabled. Specify both the UFS_PHY_GDSC
power domain and 'core' and 'iface' clocks in the ICE node for sm8450.

Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 920a2d1c04d0..3d243e757fa1 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -5374,7 +5374,11 @@ ice: crypto@1d88000 {
 			compatible = "qcom,sm8450-inline-crypto-engine",
 				     "qcom,inline-crypto-engine";
 			reg = <0 0x01d88000 0 0x8000>;
-			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
+			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>,
+				 <&gcc GCC_UFS_PHY_AHB_CLK>;
+			clock-names = "ice_core_clk",
+				      "iface_clk";
+			power-domains = <&gcc UFS_PHY_GDSC>;
 		};
 
 		cryptobam: dma-controller@1dc4000 {

-- 
2.34.1

[PATCH 08/11] arm64: dts: qcom: sm8550: Add power-domain and iface clk for ice node

[Harshal Dev]

Qualcomm in-line crypto engine (ICE) platform driver specifies and votes
for it's own resources. Before accessing ICE hardware, the 'core' and
'iface' clocks must be turned on by the driver. This can only be done if
the UFS_PHY_GDSC power domain is enabled. Specify both the UFS_PHY_GDSC
power domain and 'core' and 'iface' clocks in the ICE node for sm8550.

Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/sm8550.dtsi | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index e3f93f4f412d..b6c8c76429ba 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -2449,7 +2449,11 @@ ice: crypto@1d88000 {
 				     "qcom,inline-crypto-engine";
 			reg = <0 0x01d88000 0 0x18000>;
 
-			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
+			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>,
+				 <&gcc GCC_UFS_PHY_AHB_CLK>;
+			clock-names = "ice_core_clk",
+				      "iface_clk";
+			power-domains = <&gcc UFS_PHY_GDSC>;
 		};
 
 		tcsr_mutex: hwlock@1f40000 {

-- 
2.34.1

[PATCH 09/11] arm64: dts: qcom: sm8650: Add power-domain and iface clk for ice node

[Harshal Dev]

Qualcomm in-line crypto engine (ICE) platform driver specifies and votes
for it's own resources. Before accessing ICE hardware, the 'core' and
'iface' clocks must be turned on by the driver. This can only be done if
the UFS_PHY_GDSC power domain is enabled. Specify both the UFS_PHY_GDSC
power domain and 'core' and 'iface' clocks in the ICE node for sm8650.

Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/sm8650.dtsi | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index 357e43b90740..c32a817efdb4 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -4081,7 +4081,11 @@ ice: crypto@1d88000 {
 				     "qcom,inline-crypto-engine";
 			reg = <0 0x01d88000 0 0x18000>;
 
-			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
+			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>,
+				 <&gcc GCC_UFS_PHY_AHB_CLK>;
+			clock-names = "ice_core_clk",
+				      "iface_clk";
+			power-domains = <&gcc UFS_PHY_GDSC>;
 		};
 
 		cryptobam: dma-controller@1dc4000 {

-- 
2.34.1

[PATCH 10/11] arm64: dts: qcom: sm8750: Add power-domain and iface clk for ice node

[Harshal Dev]

Qualcomm in-line crypto engine (ICE) platform driver specifies and votes
for it's own resources. Before accessing ICE hardware, the 'core' and
'iface' clocks must be turned on by the driver. This can only be done if
the GCC_UFS_PHY_GDSC power domain is enabled. Specify both the
GCC_UFS_PHY_GDSC power domain and 'core' and 'iface' clocks in the ICE node
for sm8750.

Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/sm8750.dtsi | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
index a76bf5193a70..11352cb16baa 100644
--- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
@@ -2074,7 +2074,11 @@ ice: crypto@1d88000 {
 				     "qcom,inline-crypto-engine";
 			reg = <0x0 0x01d88000 0x0 0x18000>;
 
-			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
+			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>,
+				 <&gcc GCC_UFS_PHY_AHB_CLK>;
+			clock-names = "ice_core_clk",
+				      "iface_clk";
+			power-domains = <&gcc GCC_UFS_PHY_GDSC>;
 		};
 
 		cryptobam: dma-controller@1dc4000 {

-- 
2.34.1

[PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Harshal Dev]

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

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Krzysztof Kozlowski]

On 23/01/2026 08:11, Harshal Dev wrote:
> Update the inline-crypto engine DT binding to reflect that power-domain and
> clock-names are now mandatory. Also update the maximum number of clocks
> that can be specified to two. These new fields are mandatory because ICE
> needs to vote on the power domain before it attempts to vote on the core
> and iface clocks to avoid clock 'stuck' issues.
> 
> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
> ---
>  .../bindings/crypto/qcom,inline-crypto-engine.yaml         | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> index c3408dcf5d20..1c2416117d4c 100644
> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> @@ -28,12 +28,20 @@ properties:
>      maxItems: 1
>  
>    clocks:
> +    maxItems: 2

This is ABI break and your commit msg suggests things were not perfect,
but it is not explicit - was this working or not? How is it that ICE was
never tested?

> +
> +  clock-names:
> +    maxItems: 2

Why "yellowpony" is a correct name? Please look at existing code to see
how this is done.


> +
> +  power-domains:
>      maxItems: 1
>  
>  required:
>    - compatible
>    - reg
>    - clocks
> +  - clock-names
> +  - power-domains

Another ABI break...

Best regards,
Krzysztof

Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Krzysztof Kozlowski]

On 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

Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Krzysztof Kozlowski]

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

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Harshal Dev]

Hi Krzysztof,

On 1/23/2026 2:27 PM, Krzysztof Kozlowski wrote:
> On 23/01/2026 08:11, Harshal Dev wrote:
>> Update the inline-crypto engine DT binding to reflect that power-domain and
>> clock-names are now mandatory. Also update the maximum number of clocks
>> that can be specified to two. These new fields are mandatory because ICE
>> needs to vote on the power domain before it attempts to vote on the core
>> and iface clocks to avoid clock 'stuck' issues.
>>
>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>> ---
>>  .../bindings/crypto/qcom,inline-crypto-engine.yaml         | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>> index c3408dcf5d20..1c2416117d4c 100644
>> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>> @@ -28,12 +28,20 @@ properties:
>>      maxItems: 1
>>  
>>    clocks:
>> +    maxItems: 2
> 
> This is ABI break and your commit msg suggests things were not perfect,
> but it is not explicit - was this working or not? How is it that ICE was
> never tested?
> 

I took some time to educate myself on the point of DT bindings stability being a
strict requirement now, so I understand how these changes are breaking ABI, I'll
send a better version of this again.

As for your question of how it was working till now, it seems that
things were tested with the 'clk_ignore_unused' flag, or with CONFIG_SCSI_UFS_QCOM
flag being override set to 'y'. When this is done, QCOM-ICE (on which QCOM-UFS
depends) initiates probe _before_ the unused clocks and power-domains are
disabled by the kernel. And so, the un-clocked register access or clock 'stuck'
isn't observed (since the clocks and power domains are already enabled).
Perhaps I should write this scenario explicitly in the commit message?

To maintain backward compatibility, let me introduce minItems and maxItems for clocks.
When the Linux distro uses CONFIG_SCSI_UFS_QCOM=y, we can do with just 1 clock as
before.

>> +
>> +  clock-names:
>> +    maxItems: 2
> 
> Why "yellowpony" is a correct name? Please look at existing code to see
> how this is done.
> 

Ack, I will try to list down the acceptable clock names here.

> 
>> +
>> +  power-domains:
>>      maxItems: 1
>>  
>>  required:
>>    - compatible
>>    - reg
>>    - clocks
>> +  - clock-names
>> +  - power-domains
> 
> Another ABI break...

Let me avoid adding clock-names and power-domains as required. Since for scenarios
where a Linux distro uses CONFIG_SCSI_UFS_QCOM=y, this isn't mandatory as I explained above.

> 
> Best regards,
> Krzysztof

Regards,
Harshal

Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Harshal Dev]

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

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Konrad Dybcio]

On 1/23/26 12:04 PM, Harshal Dev wrote:
> Hi Krzysztof,
> 
> On 1/23/2026 2:27 PM, Krzysztof Kozlowski wrote:
>> On 23/01/2026 08:11, Harshal Dev wrote:
>>> Update the inline-crypto engine DT binding to reflect that power-domain and
>>> clock-names are now mandatory. Also update the maximum number of clocks
>>> that can be specified to two. These new fields are mandatory because ICE
>>> needs to vote on the power domain before it attempts to vote on the core
>>> and iface clocks to avoid clock 'stuck' issues.
>>>
>>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>>> ---
>>>  .../bindings/crypto/qcom,inline-crypto-engine.yaml         | 14 +++++++++++++-
>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>> index c3408dcf5d20..1c2416117d4c 100644
>>> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>> @@ -28,12 +28,20 @@ properties:
>>>      maxItems: 1
>>>  
>>>    clocks:
>>> +    maxItems: 2
>>
>> This is ABI break and your commit msg suggests things were not perfect,
>> but it is not explicit - was this working or not? How is it that ICE was
>> never tested?
>>
> 
> I took some time to educate myself on the point of DT bindings stability being a
> strict requirement now, so I understand how these changes are breaking ABI, I'll
> send a better version of this again.
> 
> As for your question of how it was working till now, it seems that
> things were tested with the 'clk_ignore_unused' flag, or with CONFIG_SCSI_UFS_QCOM
> flag being override set to 'y'. When this is done, QCOM-ICE (on which QCOM-UFS
> depends) initiates probe _before_ the unused clocks and power-domains are
> disabled by the kernel. And so, the un-clocked register access or clock 'stuck'
> isn't observed (since the clocks and power domains are already enabled).
> Perhaps I should write this scenario explicitly in the commit message?
> 
> To maintain backward compatibility, let me introduce minItems and maxItems for clocks.
> When the Linux distro uses CONFIG_SCSI_UFS_QCOM=y, we can do with just 1 clock as
> before.

You must not assume any particular kernel configuration

clk_ignore_unused is a hack which leads to situations like this, since
the bootloader doesn't clean up clocks it turned on, which leads to
situations like this where someone who previously wrote this binding
didn't care enough to **actually** test whether this device can operate
with only the set of clocks it requires

I believe in this case it absolutely makes sense to break things, but
you must put the backstory in writing, in the commit message

Konrad

Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Konrad Dybcio]

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

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Harshal Dev]

Hi Krzysztof and Konrad,

On 1/26/2026 3:59 PM, Konrad Dybcio wrote:
> On 1/23/26 12:04 PM, Harshal Dev wrote:
>> Hi Krzysztof,
>>
>> On 1/23/2026 2:27 PM, Krzysztof Kozlowski wrote:
>>> On 23/01/2026 08:11, Harshal Dev wrote:
>>>> Update the inline-crypto engine DT binding to reflect that power-domain and
>>>> clock-names are now mandatory. Also update the maximum number of clocks
>>>> that can be specified to two. These new fields are mandatory because ICE
>>>> needs to vote on the power domain before it attempts to vote on the core
>>>> and iface clocks to avoid clock 'stuck' issues.
>>>>
>>>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>>>> ---
>>>>  .../bindings/crypto/qcom,inline-crypto-engine.yaml         | 14 +++++++++++++-
>>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>> index c3408dcf5d20..1c2416117d4c 100644
>>>> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>> @@ -28,12 +28,20 @@ properties:
>>>>      maxItems: 1
>>>>  
>>>>    clocks:
>>>> +    maxItems: 2
>>>
>>> This is ABI break and your commit msg suggests things were not perfect,
>>> but it is not explicit - was this working or not? How is it that ICE was
>>> never tested?
>>>
>>
>> I took some time to educate myself on the point of DT bindings stability being a
>> strict requirement now, so I understand how these changes are breaking ABI, I'll
>> send a better version of this again.
>>
>> As for your question of how it was working till now, it seems that
>> things were tested with the 'clk_ignore_unused' flag, or with CONFIG_SCSI_UFS_QCOM
>> flag being override set to 'y'. When this is done, QCOM-ICE (on which QCOM-UFS
>> depends) initiates probe _before_ the unused clocks and power-domains are
>> disabled by the kernel. And so, the un-clocked register access or clock 'stuck'
>> isn't observed (since the clocks and power domains are already enabled).
>> Perhaps I should write this scenario explicitly in the commit message?
>>
>> To maintain backward compatibility, let me introduce minItems and maxItems for clocks.
>> When the Linux distro uses CONFIG_SCSI_UFS_QCOM=y, we can do with just 1 clock as
>> before.
> 
> You must not assume any particular kernel configuration
> 
> clk_ignore_unused is a hack which leads to situations like this, since
> the bootloader doesn't clean up clocks it turned on, which leads to
> situations like this where someone who previously wrote this binding
> didn't care enough to **actually** test whether this device can operate
> with only the set of clocks it requires
> 
> I believe in this case it absolutely makes sense to break things, but
> you must put the backstory in writing, in the commit message
> 

I took some more time to think this through, and I agree with you now Konrad.

These DT bindings appear to be invalid from day-1. ICE being an independent
and common IP for both UFS and SDCC, it cannot operate correctly without its
power-domain and clocks being enabled first. Hence, it should be mandatory for
them to be specified in the DT-node and the same should be reflected in the DT
binding.

The only reason I can think of for omitting the 'power-domain' and 'iface' clock
in the original DT-binding for ICE is because we failed to test the driver on
a production kernel where the 'clk_ignore_unused' flag is not passed on the cmdline.
Or if we did test that way, we were just lucky to not run into a timing scenario
where the probe for the driver is attempted _after_ the clocks are turned off by the
kernel.

Sending a new patch, which makes these two resources optional (to preserve the DT
binding) would either imply that we are make this bug fix optional as well or
asking the reporter to resort to some workaround such as overriding
CONFIG_SCSI_UFS_QCOM to 'y'.

Let us know your thoughts on this Krzysztof.

Thanks,
Harshal

> Konrad

Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Harshal Dev]

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

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Krzysztof Kozlowski]

On 03/02/2026 10:26, Harshal Dev wrote:
> Hi Krzysztof and Konrad,
> 
> On 1/26/2026 3:59 PM, Konrad Dybcio wrote:
>> On 1/23/26 12:04 PM, Harshal Dev wrote:
>>> Hi Krzysztof,
>>>
>>> On 1/23/2026 2:27 PM, Krzysztof Kozlowski wrote:
>>>> On 23/01/2026 08:11, Harshal Dev wrote:
>>>>> Update the inline-crypto engine DT binding to reflect that power-domain and
>>>>> clock-names are now mandatory. Also update the maximum number of clocks
>>>>> that can be specified to two. These new fields are mandatory because ICE
>>>>> needs to vote on the power domain before it attempts to vote on the core
>>>>> and iface clocks to avoid clock 'stuck' issues.
>>>>>
>>>>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>>>>> ---
>>>>>  .../bindings/crypto/qcom,inline-crypto-engine.yaml         | 14 +++++++++++++-
>>>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>>> index c3408dcf5d20..1c2416117d4c 100644
>>>>> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>>> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>>> @@ -28,12 +28,20 @@ properties:
>>>>>      maxItems: 1
>>>>>  
>>>>>    clocks:
>>>>> +    maxItems: 2
>>>>
>>>> This is ABI break and your commit msg suggests things were not perfect,
>>>> but it is not explicit - was this working or not? How is it that ICE was
>>>> never tested?
>>>>
>>>
>>> I took some time to educate myself on the point of DT bindings stability being a
>>> strict requirement now, so I understand how these changes are breaking ABI, I'll
>>> send a better version of this again.
>>>
>>> As for your question of how it was working till now, it seems that
>>> things were tested with the 'clk_ignore_unused' flag, or with CONFIG_SCSI_UFS_QCOM
>>> flag being override set to 'y'. When this is done, QCOM-ICE (on which QCOM-UFS
>>> depends) initiates probe _before_ the unused clocks and power-domains are
>>> disabled by the kernel. And so, the un-clocked register access or clock 'stuck'
>>> isn't observed (since the clocks and power domains are already enabled).
>>> Perhaps I should write this scenario explicitly in the commit message?
>>>
>>> To maintain backward compatibility, let me introduce minItems and maxItems for clocks.
>>> When the Linux distro uses CONFIG_SCSI_UFS_QCOM=y, we can do with just 1 clock as
>>> before.
>>
>> You must not assume any particular kernel configuration
>>
>> clk_ignore_unused is a hack which leads to situations like this, since
>> the bootloader doesn't clean up clocks it turned on, which leads to
>> situations like this where someone who previously wrote this binding
>> didn't care enough to **actually** test whether this device can operate
>> with only the set of clocks it requires
>>
>> I believe in this case it absolutely makes sense to break things, but
>> you must put the backstory in writing, in the commit message
>>
> 
> I took some more time to think this through, and I agree with you now Konrad.
> 
> These DT bindings appear to be invalid from day-1. ICE being an independent
> and common IP for both UFS and SDCC, it cannot operate correctly without its
> power-domain and clocks being enabled first. Hence, it should be mandatory for
> them to be specified in the DT-node and the same should be reflected in the DT
> binding.
> 
> The only reason I can think of for omitting the 'power-domain' and 'iface' clock
> in the original DT-binding for ICE is because we failed to test the driver on
> a production kernel where the 'clk_ignore_unused' flag is not passed on the cmdline.

That's a reason to change ABI in the bindings, but not a reason to break
in-kernel or out of tree DTS.

> Or if we did test that way, we were just lucky to not run into a timing scenario
> where the probe for the driver is attempted _after_ the clocks are turned off by the
> kernel.
> 
> Sending a new patch, which makes these two resources optional (to preserve the DT
> binding) would either imply that we are make this bug fix optional as well or
> asking the reporter to resort to some workaround such as overriding
> CONFIG_SCSI_UFS_QCOM to 'y'.

Either I do not understand the point or you still insist on breaking a
working DTS on kernels with clk_ignore_unused, just because what
exactly? You claim it did not work, but in fact it did work. So you
claim it worked by luck, right? And what this patchset achieves? It
breaks this "work by luck" into "100% not working and broken". I do not
see how is this an improvement.

My NAK for driver change stays. This is wrong approach - you cannot
break working DTS.

Best regards,
Krzysztof

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Harshal Dev]

Hi Krzysztof,

On 2/5/2026 4:47 PM, Krzysztof Kozlowski wrote:
> On 03/02/2026 10:26, Harshal Dev wrote:
>> Hi Krzysztof and Konrad,
>>
>> On 1/26/2026 3:59 PM, Konrad Dybcio wrote:
>>> On 1/23/26 12:04 PM, Harshal Dev wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 1/23/2026 2:27 PM, Krzysztof Kozlowski wrote:
>>>>> On 23/01/2026 08:11, Harshal Dev wrote:
>>>>>> Update the inline-crypto engine DT binding to reflect that power-domain and
>>>>>> clock-names are now mandatory. Also update the maximum number of clocks
>>>>>> that can be specified to two. These new fields are mandatory because ICE
>>>>>> needs to vote on the power domain before it attempts to vote on the core
>>>>>> and iface clocks to avoid clock 'stuck' issues.
>>>>>>
>>>>>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>>>>>> ---
>>>>>>  .../bindings/crypto/qcom,inline-crypto-engine.yaml         | 14 +++++++++++++-
>>>>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>>>> index c3408dcf5d20..1c2416117d4c 100644
>>>>>> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>>>> @@ -28,12 +28,20 @@ properties:
>>>>>>      maxItems: 1
>>>>>>  
>>>>>>    clocks:
>>>>>> +    maxItems: 2
>>>>>
>>>>> This is ABI break and your commit msg suggests things were not perfect,
>>>>> but it is not explicit - was this working or not? How is it that ICE was
>>>>> never tested?
>>>>>
>>>>
>>>> I took some time to educate myself on the point of DT bindings stability being a
>>>> strict requirement now, so I understand how these changes are breaking ABI, I'll
>>>> send a better version of this again.
>>>>
>>>> As for your question of how it was working till now, it seems that
>>>> things were tested with the 'clk_ignore_unused' flag, or with CONFIG_SCSI_UFS_QCOM
>>>> flag being override set to 'y'. When this is done, QCOM-ICE (on which QCOM-UFS
>>>> depends) initiates probe _before_ the unused clocks and power-domains are
>>>> disabled by the kernel. And so, the un-clocked register access or clock 'stuck'
>>>> isn't observed (since the clocks and power domains are already enabled).
>>>> Perhaps I should write this scenario explicitly in the commit message?
>>>>
>>>> To maintain backward compatibility, let me introduce minItems and maxItems for clocks.
>>>> When the Linux distro uses CONFIG_SCSI_UFS_QCOM=y, we can do with just 1 clock as
>>>> before.
>>>
>>> You must not assume any particular kernel configuration
>>>
>>> clk_ignore_unused is a hack which leads to situations like this, since
>>> the bootloader doesn't clean up clocks it turned on, which leads to
>>> situations like this where someone who previously wrote this binding
>>> didn't care enough to **actually** test whether this device can operate
>>> with only the set of clocks it requires
>>>
>>> I believe in this case it absolutely makes sense to break things, but
>>> you must put the backstory in writing, in the commit message
>>>
>>
>> I took some more time to think this through, and I agree with you now Konrad.
>>
>> These DT bindings appear to be invalid from day-1. ICE being an independent
>> and common IP for both UFS and SDCC, it cannot operate correctly without its
>> power-domain and clocks being enabled first. Hence, it should be mandatory for
>> them to be specified in the DT-node and the same should be reflected in the DT
>> binding.
>>
>> The only reason I can think of for omitting the 'power-domain' and 'iface' clock
>> in the original DT-binding for ICE is because we failed to test the driver on
>> a production kernel where the 'clk_ignore_unused' flag is not passed on the cmdline.
> 
> That's a reason to change ABI in the bindings, but not a reason to break
> in-kernel or out of tree DTS.
> 
>> Or if we did test that way, we were just lucky to not run into a timing scenario
>> where the probe for the driver is attempted _after_ the clocks are turned off by the
>> kernel.
>>
>> Sending a new patch, which makes these two resources optional (to preserve the DT
>> binding) would either imply that we are make this bug fix optional as well or
>> asking the reporter to resort to some workaround such as overriding
>> CONFIG_SCSI_UFS_QCOM to 'y'.
> 
> Either I do not understand the point or you still insist on breaking a
> working DTS on kernels with clk_ignore_unused, just because what
> exactly? You claim it did not work, but in fact it did work. So you
> claim it worked by luck, right? And what this patchset achieves? It
> breaks this "work by luck" into "100% not working and broken". I do not
> see how is this an improvement.
> 

My point is something more fundamental. It worked before and it will still continue
to work if:
1. We pass the 'clk_ignore_unused' flag. or,
2. If the Linux distro is overriding CONFIG_SCSI_UFS_QCOM to 'y'.

But that does not change the fact that the current DT binding does not fully describe all
the resources required by the hardware block to function correctly.

> My NAK for driver change stays. This is wrong approach - you cannot
> break working DTS.
> 

I agree that this patch in it's current form will break both the in-kernel and
out of tree DTS written in accordance with the old binding. If this isn't acceptable
at all then like you said we need to move forward in a way that preserves them.

But I am trying to highlight that if we go forward this way, we are all agreeing that
kernels with these old DTS can only work under the two conditions I described above. If the
kernel doesn't obey either of those conditions, the kernel won't boot.

If we are all aligned that preserving the older DTS is very important, since they
still 'work' under certain conditions, then sure. I'm fine with sending a new patch
that doesn't break the ABI as per my initial response. Let me know your thoughts.

Cheers,
Harshal

> Best regards,
> Krzysztof

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Krzysztof Kozlowski]

On 06/02/2026 11:07, Harshal Dev wrote:
> Hi Krzysztof,
> 
> On 2/5/2026 4:47 PM, Krzysztof Kozlowski wrote:
>> On 03/02/2026 10:26, Harshal Dev wrote:
>>> Hi Krzysztof and Konrad,
>>>
>>> On 1/26/2026 3:59 PM, Konrad Dybcio wrote:
>>>> On 1/23/26 12:04 PM, Harshal Dev wrote:
>>>>> Hi Krzysztof,
>>>>>
>>>>> On 1/23/2026 2:27 PM, Krzysztof Kozlowski wrote:
>>>>>> On 23/01/2026 08:11, Harshal Dev wrote:
>>>>>>> Update the inline-crypto engine DT binding to reflect that power-domain and
>>>>>>> clock-names are now mandatory. Also update the maximum number of clocks
>>>>>>> that can be specified to two. These new fields are mandatory because ICE
>>>>>>> needs to vote on the power domain before it attempts to vote on the core
>>>>>>> and iface clocks to avoid clock 'stuck' issues.
>>>>>>>
>>>>>>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>>>>>>> ---
>>>>>>>  .../bindings/crypto/qcom,inline-crypto-engine.yaml         | 14 +++++++++++++-
>>>>>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>>>>> index c3408dcf5d20..1c2416117d4c 100644
>>>>>>> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>>>>> @@ -28,12 +28,20 @@ properties:
>>>>>>>      maxItems: 1
>>>>>>>  
>>>>>>>    clocks:
>>>>>>> +    maxItems: 2
>>>>>>
>>>>>> This is ABI break and your commit msg suggests things were not perfect,
>>>>>> but it is not explicit - was this working or not? How is it that ICE was
>>>>>> never tested?
>>>>>>
>>>>>
>>>>> I took some time to educate myself on the point of DT bindings stability being a
>>>>> strict requirement now, so I understand how these changes are breaking ABI, I'll
>>>>> send a better version of this again.
>>>>>
>>>>> As for your question of how it was working till now, it seems that
>>>>> things were tested with the 'clk_ignore_unused' flag, or with CONFIG_SCSI_UFS_QCOM
>>>>> flag being override set to 'y'. When this is done, QCOM-ICE (on which QCOM-UFS
>>>>> depends) initiates probe _before_ the unused clocks and power-domains are
>>>>> disabled by the kernel. And so, the un-clocked register access or clock 'stuck'
>>>>> isn't observed (since the clocks and power domains are already enabled).
>>>>> Perhaps I should write this scenario explicitly in the commit message?
>>>>>
>>>>> To maintain backward compatibility, let me introduce minItems and maxItems for clocks.
>>>>> When the Linux distro uses CONFIG_SCSI_UFS_QCOM=y, we can do with just 1 clock as
>>>>> before.
>>>>
>>>> You must not assume any particular kernel configuration
>>>>
>>>> clk_ignore_unused is a hack which leads to situations like this, since
>>>> the bootloader doesn't clean up clocks it turned on, which leads to
>>>> situations like this where someone who previously wrote this binding
>>>> didn't care enough to **actually** test whether this device can operate
>>>> with only the set of clocks it requires
>>>>
>>>> I believe in this case it absolutely makes sense to break things, but
>>>> you must put the backstory in writing, in the commit message
>>>>
>>>
>>> I took some more time to think this through, and I agree with you now Konrad.
>>>
>>> These DT bindings appear to be invalid from day-1. ICE being an independent
>>> and common IP for both UFS and SDCC, it cannot operate correctly without its
>>> power-domain and clocks being enabled first. Hence, it should be mandatory for
>>> them to be specified in the DT-node and the same should be reflected in the DT
>>> binding.
>>>
>>> The only reason I can think of for omitting the 'power-domain' and 'iface' clock
>>> in the original DT-binding for ICE is because we failed to test the driver on
>>> a production kernel where the 'clk_ignore_unused' flag is not passed on the cmdline.
>>
>> That's a reason to change ABI in the bindings, but not a reason to break
>> in-kernel or out of tree DTS.
>>
>>> Or if we did test that way, we were just lucky to not run into a timing scenario
>>> where the probe for the driver is attempted _after_ the clocks are turned off by the
>>> kernel.
>>>
>>> Sending a new patch, which makes these two resources optional (to preserve the DT
>>> binding) would either imply that we are make this bug fix optional as well or
>>> asking the reporter to resort to some workaround such as overriding
>>> CONFIG_SCSI_UFS_QCOM to 'y'.
>>
>> Either I do not understand the point or you still insist on breaking a
>> working DTS on kernels with clk_ignore_unused, just because what
>> exactly? You claim it did not work, but in fact it did work. So you
>> claim it worked by luck, right? And what this patchset achieves? It
>> breaks this "work by luck" into "100% not working and broken". I do not
>> see how is this an improvement.
>>
> 
> My point is something more fundamental. It worked before and it will still continue
> to work if:
> 1. We pass the 'clk_ignore_unused' flag. or,
> 2. If the Linux distro is overriding CONFIG_SCSI_UFS_QCOM to 'y'.

I do not agree with this. I already commented about your driver. If you
do not believe me, apply your driver patch and show the test results of
existing working device.


> 
> But that does not change the fact that the current DT binding does not fully describe all
> the resources required by the hardware block to function correctly.
> 
>> My NAK for driver change stays. This is wrong approach - you cannot
>> break working DTS.
>>
> 
> I agree that this patch in it's current form will break both the in-kernel and
> out of tree DTS written in accordance with the old binding. If this isn't acceptable

What? You just said few lines above:
"it will still continue to work if:"

So either this will continue to work or not. I don't understand this
thread and honestly do not have patience for it. I gave you already
reasoning what is wrong and why it is. Now it is just wasting my time.

Best regards,
Krzysztof

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Harshal Dev]

Hi Krzysztof,

On 2/6/2026 4:20 PM, Krzysztof Kozlowski wrote:
> On 06/02/2026 11:07, Harshal Dev wrote:
>> Hi Krzysztof,
>>
>> On 2/5/2026 4:47 PM, Krzysztof Kozlowski wrote:
>>> On 03/02/2026 10:26, Harshal Dev wrote:
>>>> Hi Krzysztof and Konrad,
>>>>
>>>> On 1/26/2026 3:59 PM, Konrad Dybcio wrote:
>>>>> On 1/23/26 12:04 PM, Harshal Dev wrote:
>>>>>> Hi Krzysztof,
>>>>>>
>>>>>> On 1/23/2026 2:27 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 23/01/2026 08:11, Harshal Dev wrote:
>>>>>>>> Update the inline-crypto engine DT binding to reflect that power-domain and
>>>>>>>> clock-names are now mandatory. Also update the maximum number of clocks
>>>>>>>> that can be specified to two. These new fields are mandatory because ICE
>>>>>>>> needs to vote on the power domain before it attempts to vote on the core
>>>>>>>> and iface clocks to avoid clock 'stuck' issues.
>>>>>>>>
>>>>>>>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>>>>>>>> ---
>>>>>>>>  .../bindings/crypto/qcom,inline-crypto-engine.yaml         | 14 +++++++++++++-
>>>>>>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>>>>>> index c3408dcf5d20..1c2416117d4c 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>>>>>> @@ -28,12 +28,20 @@ properties:
>>>>>>>>      maxItems: 1
>>>>>>>>  
>>>>>>>>    clocks:
>>>>>>>> +    maxItems: 2
>>>>>>>
>>>>>>> This is ABI break and your commit msg suggests things were not perfect,
>>>>>>> but it is not explicit - was this working or not? How is it that ICE was
>>>>>>> never tested?
>>>>>>>
>>>>>>
>>>>>> I took some time to educate myself on the point of DT bindings stability being a
>>>>>> strict requirement now, so I understand how these changes are breaking ABI, I'll
>>>>>> send a better version of this again.
>>>>>>
>>>>>> As for your question of how it was working till now, it seems that
>>>>>> things were tested with the 'clk_ignore_unused' flag, or with CONFIG_SCSI_UFS_QCOM
>>>>>> flag being override set to 'y'. When this is done, QCOM-ICE (on which QCOM-UFS
>>>>>> depends) initiates probe _before_ the unused clocks and power-domains are
>>>>>> disabled by the kernel. And so, the un-clocked register access or clock 'stuck'
>>>>>> isn't observed (since the clocks and power domains are already enabled).
>>>>>> Perhaps I should write this scenario explicitly in the commit message?
>>>>>>
>>>>>> To maintain backward compatibility, let me introduce minItems and maxItems for clocks.
>>>>>> When the Linux distro uses CONFIG_SCSI_UFS_QCOM=y, we can do with just 1 clock as
>>>>>> before.
>>>>>
>>>>> You must not assume any particular kernel configuration
>>>>>
>>>>> clk_ignore_unused is a hack which leads to situations like this, since
>>>>> the bootloader doesn't clean up clocks it turned on, which leads to
>>>>> situations like this where someone who previously wrote this binding
>>>>> didn't care enough to **actually** test whether this device can operate
>>>>> with only the set of clocks it requires
>>>>>
>>>>> I believe in this case it absolutely makes sense to break things, but
>>>>> you must put the backstory in writing, in the commit message
>>>>>
>>>>
>>>> I took some more time to think this through, and I agree with you now Konrad.
>>>>
>>>> These DT bindings appear to be invalid from day-1. ICE being an independent
>>>> and common IP for both UFS and SDCC, it cannot operate correctly without its
>>>> power-domain and clocks being enabled first. Hence, it should be mandatory for
>>>> them to be specified in the DT-node and the same should be reflected in the DT
>>>> binding.
>>>>
>>>> The only reason I can think of for omitting the 'power-domain' and 'iface' clock
>>>> in the original DT-binding for ICE is because we failed to test the driver on
>>>> a production kernel where the 'clk_ignore_unused' flag is not passed on the cmdline.
>>>
>>> That's a reason to change ABI in the bindings, but not a reason to break
>>> in-kernel or out of tree DTS.
>>>
>>>> Or if we did test that way, we were just lucky to not run into a timing scenario
>>>> where the probe for the driver is attempted _after_ the clocks are turned off by the
>>>> kernel.
>>>>
>>>> Sending a new patch, which makes these two resources optional (to preserve the DT
>>>> binding) would either imply that we are make this bug fix optional as well or
>>>> asking the reporter to resort to some workaround such as overriding
>>>> CONFIG_SCSI_UFS_QCOM to 'y'.
>>>
>>> Either I do not understand the point or you still insist on breaking a
>>> working DTS on kernels with clk_ignore_unused, just because what
>>> exactly? You claim it did not work, but in fact it did work. So you
>>> claim it worked by luck, right? And what this patchset achieves? It
>>> breaks this "work by luck" into "100% not working and broken". I do not
>>> see how is this an improvement.
>>>
>>
>> My point is something more fundamental. It worked before and it will still continue
>> to work if:
>> 1. We pass the 'clk_ignore_unused' flag. or,
>> 2. If the Linux distro is overriding CONFIG_SCSI_UFS_QCOM to 'y'.
> 
> I do not agree with this. I already commented about your driver. If you
> do not believe me, apply your driver patch and show the test results of
> existing working device.
> 
>

Apologies, it seems like I failed to explain correctly what I meant.
Here I was talking about the existing in-tree ICE driver and not about this particular DT
binding commit. This commit, as you rightly said and I mentioned below too, breaks backward
compatibility for existing in-tree and out-of-tree DTS.

>>
>> But that does not change the fact that the current DT binding does not fully describe all
>> the resources required by the hardware block to function correctly.
>>
>>> My NAK for driver change stays. This is wrong approach - you cannot
>>> break working DTS.
>>>
>>
>> I agree that this patch in it's current form will break both the in-kernel and
>> out of tree DTS written in accordance with the old binding. If this isn't acceptable
> 
> What? You just said few lines above:
> "it will still continue to work if:"
>

I hope I am clear now, 'it' referred to the in-tree ICE driver and not to this particular
DT schema commit. :)
 
> So either this will continue to work or not. I don't understand this
> thread and honestly do not have patience for it. I gave you already
> reasoning what is wrong and why it is. Now it is just wasting my time.
> 

Apologies again for the confusion. I totally agree, as replied previously too, that the
updated DT binding breaks backward compatibility. Like I said, I will post another patch
to preserve the correctness of existing in-tree and out-of-tree DTS.

The only point I am trying to highlight for everyone's awareness is that as per this bug
report https://lore.kernel.org/all/ZZYTYsaNUuWQg3tR@x1/ the kernel fails to boot with the
existing DTS when the above two conditions aren't satisfied.

Thank you,
Harshal

> Best regards,
> Krzysztof

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Krzysztof Kozlowski]

On 09/02/2026 06:43, Harshal Dev wrote:
>>>> Either I do not understand the point or you still insist on breaking a
>>>> working DTS on kernels with clk_ignore_unused, just because what
>>>> exactly? You claim it did not work, but in fact it did work. So you
>>>> claim it worked by luck, right? And what this patchset achieves? It
>>>> breaks this "work by luck" into "100% not working and broken". I do not
>>>> see how is this an improvement.
>>>>
>>>
>>> My point is something more fundamental. It worked before and it will still continue
>>> to work if:
>>> 1. We pass the 'clk_ignore_unused' flag. or,
>>> 2. If the Linux distro is overriding CONFIG_SCSI_UFS_QCOM to 'y'.
>>
>> I do not agree with this. I already commented about your driver. If you
>> do not believe me, apply your driver patch and show the test results of
>> existing working device.
>>
>>
> 
> Apologies, it seems like I failed to explain correctly what I meant.
> Here I was talking about the existing in-tree ICE driver and not about this particular DT
> binding commit. This commit, as you rightly said and I mentioned below too, breaks backward
> compatibility for existing in-tree and out-of-tree DTS.

I was also talking about existing in-tree ICE driver, not about my
commit. I do not believe anything will work fine with existing in-tree
ICE driver.

We shifted away from this binding in the discussion, imagine it does not
matter. Just look at your driver code.

Best regards,
Krzysztof

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Manivannan Sadhasivam]

On Mon, Feb 09, 2026 at 11:13:06AM +0530, Harshal Dev wrote:
> Hi Krzysztof,
> 
> On 2/6/2026 4:20 PM, Krzysztof Kozlowski wrote:
> > On 06/02/2026 11:07, Harshal Dev wrote:
> >> Hi Krzysztof,
> >>
> >> On 2/5/2026 4:47 PM, Krzysztof Kozlowski wrote:
> >>> On 03/02/2026 10:26, Harshal Dev wrote:
> >>>> Hi Krzysztof and Konrad,
> >>>>
> >>>> On 1/26/2026 3:59 PM, Konrad Dybcio wrote:
> >>>>> On 1/23/26 12:04 PM, Harshal Dev wrote:
> >>>>>> Hi Krzysztof,
> >>>>>>
> >>>>>> On 1/23/2026 2:27 PM, Krzysztof Kozlowski wrote:
> >>>>>>> On 23/01/2026 08:11, Harshal Dev wrote:
> >>>>>>>> Update the inline-crypto engine DT binding to reflect that power-domain and
> >>>>>>>> clock-names are now mandatory. Also update the maximum number of clocks
> >>>>>>>> that can be specified to two. These new fields are mandatory because ICE
> >>>>>>>> needs to vote on the power domain before it attempts to vote on the core
> >>>>>>>> and iface clocks to avoid clock 'stuck' issues.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
> >>>>>>>> ---
> >>>>>>>>  .../bindings/crypto/qcom,inline-crypto-engine.yaml         | 14 +++++++++++++-
> >>>>>>>>  1 file changed, 13 insertions(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> >>>>>>>> index c3408dcf5d20..1c2416117d4c 100644
> >>>>>>>> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> >>>>>>>> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> >>>>>>>> @@ -28,12 +28,20 @@ properties:
> >>>>>>>>      maxItems: 1
> >>>>>>>>  
> >>>>>>>>    clocks:
> >>>>>>>> +    maxItems: 2
> >>>>>>>
> >>>>>>> This is ABI break and your commit msg suggests things were not perfect,
> >>>>>>> but it is not explicit - was this working or not? How is it that ICE was
> >>>>>>> never tested?
> >>>>>>>
> >>>>>>
> >>>>>> I took some time to educate myself on the point of DT bindings stability being a
> >>>>>> strict requirement now, so I understand how these changes are breaking ABI, I'll
> >>>>>> send a better version of this again.
> >>>>>>
> >>>>>> As for your question of how it was working till now, it seems that
> >>>>>> things were tested with the 'clk_ignore_unused' flag, or with CONFIG_SCSI_UFS_QCOM
> >>>>>> flag being override set to 'y'. When this is done, QCOM-ICE (on which QCOM-UFS
> >>>>>> depends) initiates probe _before_ the unused clocks and power-domains are
> >>>>>> disabled by the kernel. And so, the un-clocked register access or clock 'stuck'
> >>>>>> isn't observed (since the clocks and power domains are already enabled).
> >>>>>> Perhaps I should write this scenario explicitly in the commit message?
> >>>>>>
> >>>>>> To maintain backward compatibility, let me introduce minItems and maxItems for clocks.
> >>>>>> When the Linux distro uses CONFIG_SCSI_UFS_QCOM=y, we can do with just 1 clock as
> >>>>>> before.
> >>>>>
> >>>>> You must not assume any particular kernel configuration
> >>>>>
> >>>>> clk_ignore_unused is a hack which leads to situations like this, since
> >>>>> the bootloader doesn't clean up clocks it turned on, which leads to
> >>>>> situations like this where someone who previously wrote this binding
> >>>>> didn't care enough to **actually** test whether this device can operate
> >>>>> with only the set of clocks it requires
> >>>>>
> >>>>> I believe in this case it absolutely makes sense to break things, but
> >>>>> you must put the backstory in writing, in the commit message
> >>>>>
> >>>>
> >>>> I took some more time to think this through, and I agree with you now Konrad.
> >>>>
> >>>> These DT bindings appear to be invalid from day-1. ICE being an independent
> >>>> and common IP for both UFS and SDCC, it cannot operate correctly without its
> >>>> power-domain and clocks being enabled first. Hence, it should be mandatory for
> >>>> them to be specified in the DT-node and the same should be reflected in the DT
> >>>> binding.
> >>>>
> >>>> The only reason I can think of for omitting the 'power-domain' and 'iface' clock
> >>>> in the original DT-binding for ICE is because we failed to test the driver on
> >>>> a production kernel where the 'clk_ignore_unused' flag is not passed on the cmdline.
> >>>
> >>> That's a reason to change ABI in the bindings, but not a reason to break
> >>> in-kernel or out of tree DTS.
> >>>
> >>>> Or if we did test that way, we were just lucky to not run into a timing scenario
> >>>> where the probe for the driver is attempted _after_ the clocks are turned off by the
> >>>> kernel.
> >>>>
> >>>> Sending a new patch, which makes these two resources optional (to preserve the DT
> >>>> binding) would either imply that we are make this bug fix optional as well or
> >>>> asking the reporter to resort to some workaround such as overriding
> >>>> CONFIG_SCSI_UFS_QCOM to 'y'.
> >>>
> >>> Either I do not understand the point or you still insist on breaking a
> >>> working DTS on kernels with clk_ignore_unused, just because what
> >>> exactly? You claim it did not work, but in fact it did work. So you
> >>> claim it worked by luck, right? And what this patchset achieves? It
> >>> breaks this "work by luck" into "100% not working and broken". I do not
> >>> see how is this an improvement.
> >>>
> >>
> >> My point is something more fundamental. It worked before and it will still continue
> >> to work if:
> >> 1. We pass the 'clk_ignore_unused' flag. or,
> >> 2. If the Linux distro is overriding CONFIG_SCSI_UFS_QCOM to 'y'.
> > 
> > I do not agree with this. I already commented about your driver. If you
> > do not believe me, apply your driver patch and show the test results of
> > existing working device.
> > 
> >
> 
> Apologies, it seems like I failed to explain correctly what I meant.
> Here I was talking about the existing in-tree ICE driver and not about this particular DT
> binding commit. This commit, as you rightly said and I mentioned below too, breaks backward
> compatibility for existing in-tree and out-of-tree DTS.
> 
> >>
> >> But that does not change the fact that the current DT binding does not fully describe all
> >> the resources required by the hardware block to function correctly.
> >>
> >>> My NAK for driver change stays. This is wrong approach - you cannot
> >>> break working DTS.
> >>>
> >>
> >> I agree that this patch in it's current form will break both the in-kernel and
> >> out of tree DTS written in accordance with the old binding. If this isn't acceptable
> > 
> > What? You just said few lines above:
> > "it will still continue to work if:"
> >
> 
> I hope I am clear now, 'it' referred to the in-tree ICE driver and not to this particular
> DT schema commit. :)
>  
> > So either this will continue to work or not. I don't understand this
> > thread and honestly do not have patience for it. I gave you already
> > reasoning what is wrong and why it is. Now it is just wasting my time.
> > 
> 
> Apologies again for the confusion. I totally agree, as replied previously too, that the
> updated DT binding breaks backward compatibility. Like I said, I will post another patch
> to preserve the correctness of existing in-tree and out-of-tree DTS.
> 

The ICE hardware cannot work without 'iface' clock and the power domain, which
are shared with the UFS PHY. One can argue that ICE is actually a part of the
peripherals like UFS/eMMC, but I don't have access to internal layout, so cannot
comment on that. I ran into this issue today when I tried to rmmod ice driver
together with ufs_qcom driver and got SError when reloading the module because
ice driver was trying to access unclocked/unpowered register.

But you should mark the resources as 'required' in the binding and justify the
ABI break. No need to preserve backwards compatibility here as the binding was
wrong from day one.

> The only point I am trying to highlight for everyone's awareness is that as per this bug
> report https://lore.kernel.org/all/ZZYTYsaNUuWQg3tR@x1/ the kernel fails to boot with the
> existing DTS when the above two conditions aren't satisfied.
> 

And you sent the fix after almost 2 years. Atleast I'm happy that you got around
to fix it.

- Mani

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

Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Manivannan Sadhasivam]

On Fri, Jan 23, 2026 at 12:41:35PM +0530, 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.

You don't need to vote for a single power domain since genpd will do that for
you before the driver probes.

> Also updated the suspend and resume callbacks to handle votes on these
> resources.
> 
> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>

Where is the Fixes tag?

> ---
>  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);

This is not needed as the power domain would be enabled at this point.

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

Use clk_bulk API to enable all clocks in one go.

>  	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);

Same here.

>  	clk_disable_unprepare(ice->core_clk);
> +	pm_runtime_put_sync(ice->dev);

Not needed.

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

Same here. Use devm_clk_bulk_get_all_enabled().

>  	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);

If you want to mark & enable the runtime PM status, you should just do:

	devm_pm_runtime_set_active_enabled();	

But this is not really needed in this patch. You can add it in a separate patch
for the sake of correctness.

- Mani

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

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Bjorn Andersson]

On Fri, Feb 20, 2026 at 08:01:59PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Feb 09, 2026 at 11:13:06AM +0530, Harshal Dev wrote:
> > On 2/6/2026 4:20 PM, Krzysztof Kozlowski wrote:
> > > On 06/02/2026 11:07, Harshal Dev wrote:
> > >> On 2/5/2026 4:47 PM, Krzysztof Kozlowski wrote:
> > >>> On 03/02/2026 10:26, Harshal Dev wrote:
> > >>>> On 1/26/2026 3:59 PM, Konrad Dybcio wrote:
> > >>>>> On 1/23/26 12:04 PM, Harshal Dev wrote:
> > >>>>>> On 1/23/2026 2:27 PM, Krzysztof Kozlowski wrote:
> > >>>>>>> On 23/01/2026 08:11, Harshal Dev wrote:
[..]
> > >>> My NAK for driver change stays. This is wrong approach - you cannot
> > >>> break working DTS.
> > >>>
> > >>
> > >> I agree that this patch in it's current form will break both the in-kernel and
> > >> out of tree DTS written in accordance with the old binding. If this isn't acceptable
> > > 
> > > What? You just said few lines above:
> > > "it will still continue to work if:"
> > >
> > 
> > I hope I am clear now, 'it' referred to the in-tree ICE driver and not to this particular
> > DT schema commit. :)
> >  
> > > So either this will continue to work or not. I don't understand this
> > > thread and honestly do not have patience for it. I gave you already
> > > reasoning what is wrong and why it is. Now it is just wasting my time.
> > > 
> > 
> > Apologies again for the confusion. I totally agree, as replied previously too, that the
> > updated DT binding breaks backward compatibility. Like I said, I will post another patch
> > to preserve the correctness of existing in-tree and out-of-tree DTS.
> > 
> 
> The ICE hardware cannot work without 'iface' clock and the power domain, which
> are shared with the UFS PHY. One can argue that ICE is actually a part of the
> peripherals like UFS/eMMC, but I don't have access to internal layout, so cannot
> comment on that. I ran into this issue today when I tried to rmmod ice driver
> together with ufs_qcom driver and got SError when reloading the module because
> ice driver was trying to access unclocked/unpowered register.
> 
> But you should mark the resources as 'required' in the binding and justify the
> ABI break. No need to preserve backwards compatibility here as the binding was
> wrong from day one.
> 

Marking it "required" in the binding, implies that it's fine for the
driver to fail in its absence. If I understand correctly that will
prevent UFS and eMMC from probing, unless you have a DTB from "the
future".

Even if I merge the dt-binding change through the qcom-tree (together
with the driver change) I will not guarantee that torvalds/master will
remain bisectable - because dts changes and driver changes goes in
different branches.


As such, the pragmatic approach is to introduce the clock as optional
and then once we're "certain" that the dts changes has propagated we
can consider breaking the backwards compatibility.

Regards,
Bjorn

> > The only point I am trying to highlight for everyone's awareness is that as per this bug
> > report https://lore.kernel.org/all/ZZYTYsaNUuWQg3tR@x1/ the kernel fails to boot with the
> > existing DTS when the above two conditions aren't satisfied.
> > 
> 
> And you sent the fix after almost 2 years. Atleast I'm happy that you got around
> to fix it.
> 
> - Mani
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Abel Vesa]

On 26-01-23 12:41:35, 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.

Your intention here is to fix the patch that introduced the ICE driver,
so I suggest you add Fixes tag. Same for all other patches.
They will have to be backported all the way.

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Harshal Dev]

Hi Bjorn and Manivannan,

On 2/20/2026 9:29 PM, Bjorn Andersson wrote:
> On Fri, Feb 20, 2026 at 08:01:59PM +0530, Manivannan Sadhasivam wrote:
>> On Mon, Feb 09, 2026 at 11:13:06AM +0530, Harshal Dev wrote:
>>> On 2/6/2026 4:20 PM, Krzysztof Kozlowski wrote:
>>>> On 06/02/2026 11:07, Harshal Dev wrote:
>>>>> On 2/5/2026 4:47 PM, Krzysztof Kozlowski wrote:
>>>>>> On 03/02/2026 10:26, Harshal Dev wrote:
>>>>>>> On 1/26/2026 3:59 PM, Konrad Dybcio wrote:
>>>>>>>> On 1/23/26 12:04 PM, Harshal Dev wrote:
>>>>>>>>> On 1/23/2026 2:27 PM, Krzysztof Kozlowski wrote:
>>>>>>>>>> On 23/01/2026 08:11, Harshal Dev wrote:
> [..]
>>>>>> My NAK for driver change stays. This is wrong approach - you cannot
>>>>>> break working DTS.
>>>>>>
>>>>>
>>>>> I agree that this patch in it's current form will break both the in-kernel and
>>>>> out of tree DTS written in accordance with the old binding. If this isn't acceptable
>>>>
>>>> What? You just said few lines above:
>>>> "it will still continue to work if:"
>>>>
>>>
>>> I hope I am clear now, 'it' referred to the in-tree ICE driver and not to this particular
>>> DT schema commit. :)
>>>  
>>>> So either this will continue to work or not. I don't understand this
>>>> thread and honestly do not have patience for it. I gave you already
>>>> reasoning what is wrong and why it is. Now it is just wasting my time.
>>>>
>>>
>>> Apologies again for the confusion. I totally agree, as replied previously too, that the
>>> updated DT binding breaks backward compatibility. Like I said, I will post another patch
>>> to preserve the correctness of existing in-tree and out-of-tree DTS.
>>>
>>
>> The ICE hardware cannot work without 'iface' clock and the power domain, which
>> are shared with the UFS PHY. One can argue that ICE is actually a part of the
>> peripherals like UFS/eMMC, but I don't have access to internal layout, so cannot
>> comment on that. I ran into this issue today when I tried to rmmod ice driver
>> together with ufs_qcom driver and got SError when reloading the module because
>> ice driver was trying to access unclocked/unpowered register.
>>
>> But you should mark the resources as 'required' in the binding and justify the
>> ABI break. No need to preserve backwards compatibility here as the binding was
>> wrong from day one.
>>
> 
> Marking it "required" in the binding, implies that it's fine for the
> driver to fail in its absence. If I understand correctly that will
> prevent UFS and eMMC from probing, unless you have a DTB from "the
> future".
> 
> Even if I merge the dt-binding change through the qcom-tree (together
> with the driver change) I will not guarantee that torvalds/master will
> remain bisectable - because dts changes and driver changes goes in
> different branches.
> 
> 
> As such, the pragmatic approach is to introduce the clock as optional
> and then once we're "certain" that the dts changes has propagated we
> can consider breaking the backwards compatibility.
> 

Apologies for the late response, I am partially on vacation since last week.

First of all, thank you for acknowledging that in order to fix the bug with
the driver we need to break the DT backward compatibility. Now the only issue
as Bjorn mentioned is preserving bisectability since the changes reach the
top from different trees.

I agree, I can send a version 2 of this patch where I keep the iface clock
and power-domain as optional, and also accommodate the same in the driver
sources to ensure that UFS/EMMC probe does not fail even if the dts changes have
not yet reached the top tree. Once all the changes are merged, I will send
another patch to update the DT bindings for clocks and power-domain to 'required'
with accompanying changes on the driver side.

And so, kernels from that version on-wards will not probe UFS/EMMC with older DTS
which do not specify the iface and/or the power-domain. In my opinion, a probe 
failure is a lot better than observing an un-clocked register access when the
DT bindings show the iface-clock and power-domain as optional.

I hope everyone is fine with this plan.

Regards,
Harshal

> Regards,
> Bjorn
> 
>>> The only point I am trying to highlight for everyone's awareness is that as per this bug
>>> report https://lore.kernel.org/all/ZZYTYsaNUuWQg3tR@x1/ the kernel fails to boot with the
>>> existing DTS when the above two conditions aren't satisfied.
>>>
>>
>> And you sent the fix after almost 2 years. Atleast I'm happy that you got around
>> to fix it.
>>
>> - Mani
>>
>> -- 
>> மணிவண்ணன் சதாசிவம்

Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Harshal Dev]

Hi Manivannan,

On 2/20/2026 8:14 PM, Manivannan Sadhasivam wrote:
> On Fri, Jan 23, 2026 at 12:41:35PM +0530, 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.
> 
> You don't need to vote for a single power domain since genpd will do that for
> you before the driver probes.
>

Unfortunately, without enabling the power domain during probe, I am seeing occasional
clock stuck messages on LeMans RB8. Am I missing something? Could you point me to any
docs with more information on the the genpd framework?

Logs for reference:

[    6.195019] gcc_ufs_phy_ice_core_clk status stuck at 'off'
[    6.195031] WARNING: CPU: 5 PID: 208 at drivers/clk/qcom/clk-branch.c:87 clk_branch_toggle+0x174/0x18c

[...]

[    6.248412] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    6.248415] pc : clk_branch_toggle+0x174/0x18c
[    6.248417] lr : clk_branch_toggle+0x174/0x18c
[    6.248418] sp : ffff80008217b770
[    6.248419] x29: ffff80008217b780 x28: ffff80008217bbb0 x27: ffffadf880a5f07c
[    6.248422] x26: ffffadf880a5c1d8 x25: 0000000000000001 x24: 0000000000000001
[    6.248424] x23: ffffadf8a0d1e740 x22: 0000000000000001 x21: ffffadf8a1d06160
[    6.248426] x20: ffffadf89f86e5a8 x19: 0000000000000000 x18: fffffffffffe9050
[    6.248429] x17: 000000000404006d x16: ffffadf89f8166c4 x15: ffffadf8a1ab6c70
[    6.347820] x14: 0000000000000000 x13: ffffadf8a1ab6cf8 x12: 000000000000060f
[    6.355145] x11: 0000000000000205 x10: ffffadf8a1b11d70 x9 : ffffadf8a1ab6cf8
[    6.362470] x8 : 00000000ffffefff x7 : ffffadf8a1b0ecf8 x6 : 0000000000000205
[    6.369795] x5 : ffff000ef1ceb408 x4 : 40000000fffff205 x3 : ffff521650ba3000
[    6.377120] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000928dd780
[    6.384444] Call trace:
[    6.386962]  clk_branch_toggle+0x174/0x18c (P)
[    6.391530]  clk_branch2_enable+0x1c/0x28
[    6.395644]  clk_core_enable+0x6c/0xac
[    6.399502]  clk_enable+0x2c/0x4c
[    6.402913]  devm_clk_get_optional_enabled+0xac/0x108
[    6.408096]  qcom_ice_create.part.0+0x50/0x2fc [qcom_ice]
[    6.413646]  qcom_ice_probe+0x58/0xa8 [qcom_ice]
[    6.418384]  platform_probe+0x5c/0x98
[    6.422153]  really_probe+0xbc/0x29c
[    6.425826]  __driver_probe_device+0x78/0x12c
[    6.430307]  driver_probe_device+0x3c/0x15c
[    6.434605]  __driver_attach+0x90/0x19c
[    6.438547]  bus_for_each_dev+0x7c/0xe0
[    6.442486]  driver_attach+0x24/0x30
[    6.446158]  bus_add_driver+0xe4/0x208
[    6.450013]  driver_register+0x5c/0x124
[    6.453954]  __platform_driver_register+0x24/0x30
[    6.458780]  qcom_ice_driver_init+0x24/0x1000 [qcom_ice]
[    6.464229]  do_one_initcall+0x80/0x1c8
[    6.468173]  do_init_module+0x58/0x234
[    6.472028]  load_module+0x1a84/0x1c84
[    6.475881]  init_module_from_file+0x88/0xcc
[    6.480262]  __arm64_sys_finit_module+0x144/0x330
[    6.485097]  invoke_syscall+0x48/0x10c
[    6.488954]  el0_svc_common.constprop.0+0xc0/0xe0
[    6.493790]  do_el0_svc+0x1c/0x28
[    6.497203]  el0_svc+0x34/0xec
[    6.500348]  el0t_64_sync_handler+0xa0/0xe4
[    6.504645]  el0t_64_sync+0x198/0x19c
[    6.508414] ---[ end trace 0000000000000000 ]---
[    6.514544] qcom-ice 1d88000.crypto: probe with driver qcom-ice failed
 
>> Also updated the suspend and resume callbacks to handle votes on these
>> resources.
>>
>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
> 
> Where is the Fixes tag?

Ack, I will add it in v2 of this patch.

> 
>> ---
>>  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);
> 
> This is not needed as the power domain would be enabled at this point.

Would this be enabled due to the genpd framework? I am not observing that
during probe. Because this call is made by the UFS/EMMC driver, perhaps you
mean the situation at this point is different?

> 
>>  	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;
>> +	}
> 
> Use clk_bulk API to enable all clocks in one go.

Ack, I'll use clk_bulk_prepare_enable().

> 
>>  	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);
> 
> Same here.

Ack, clk_bulk_disable_unprepare() would look good.
As Konrad pointed out, if iface clock is not present in DT, thse APIs are
fine with NULL pointers here.

> 
>>  	clk_disable_unprepare(ice->core_clk);
>> +	pm_runtime_put_sync(ice->dev);
> 
> Not needed.
> 
>>  	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);
>> +
> 
> Same here. Use devm_clk_bulk_get_all_enabled().

As per discussion on the DT binding patch, I can do this once we decide to break the
DT backward compatibility with a subsequent patch which makes both clocks mandatory.
For v2, I am planning to continue to treat the 'iface' clock as optional via
devm_clk_get_optional() API.

> 
>>  	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);
> 
> If you want to mark & enable the runtime PM status, you should just do:
> 
> 	devm_pm_runtime_set_active_enabled();	
> 
> But this is not really needed in this patch. You can add it in a separate patch
> for the sake of correctness.

If my understanding is correct, I need to call pm_runtime_get_sync() to enable
the power domain after enabling the PM runtime to ensure further calls to enable
the iface clock do not encounter failure. Just calling devm_pm_runtime_set_active_enabled()
will only enable the PM runtime and set it's status to 'active'. It will not enable
the power domain.

Please do let me know if there is something more to this that I am missing.

Regards,
Harshal

> 
> - Mani
> 

Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Harshal Dev]

Hi Abel,

On 2/21/2026 5:15 PM, Abel Vesa wrote:
> On 26-01-23 12:41:35, 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.
> 
> Your intention here is to fix the patch that introduced the ICE driver,
> so I suggest you add Fixes tag. Same for all other patches.
> They will have to be backported all the way.

Ack, I will add the Fixes tag and ensure back-porting.

Regards,
Harshal

Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain and iface clk

[Manivannan Sadhasivam]

On Fri, Feb 20, 2026 at 09:59:41AM -0600, Bjorn Andersson wrote:
> On Fri, Feb 20, 2026 at 08:01:59PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Feb 09, 2026 at 11:13:06AM +0530, Harshal Dev wrote:
> > > On 2/6/2026 4:20 PM, Krzysztof Kozlowski wrote:
> > > > On 06/02/2026 11:07, Harshal Dev wrote:
> > > >> On 2/5/2026 4:47 PM, Krzysztof Kozlowski wrote:
> > > >>> On 03/02/2026 10:26, Harshal Dev wrote:
> > > >>>> On 1/26/2026 3:59 PM, Konrad Dybcio wrote:
> > > >>>>> On 1/23/26 12:04 PM, Harshal Dev wrote:
> > > >>>>>> On 1/23/2026 2:27 PM, Krzysztof Kozlowski wrote:
> > > >>>>>>> On 23/01/2026 08:11, Harshal Dev wrote:
> [..]
> > > >>> My NAK for driver change stays. This is wrong approach - you cannot
> > > >>> break working DTS.
> > > >>>
> > > >>
> > > >> I agree that this patch in it's current form will break both the in-kernel and
> > > >> out of tree DTS written in accordance with the old binding. If this isn't acceptable
> > > > 
> > > > What? You just said few lines above:
> > > > "it will still continue to work if:"
> > > >
> > > 
> > > I hope I am clear now, 'it' referred to the in-tree ICE driver and not to this particular
> > > DT schema commit. :)
> > >  
> > > > So either this will continue to work or not. I don't understand this
> > > > thread and honestly do not have patience for it. I gave you already
> > > > reasoning what is wrong and why it is. Now it is just wasting my time.
> > > > 
> > > 
> > > Apologies again for the confusion. I totally agree, as replied previously too, that the
> > > updated DT binding breaks backward compatibility. Like I said, I will post another patch
> > > to preserve the correctness of existing in-tree and out-of-tree DTS.
> > > 
> > 
> > The ICE hardware cannot work without 'iface' clock and the power domain, which
> > are shared with the UFS PHY. One can argue that ICE is actually a part of the
> > peripherals like UFS/eMMC, but I don't have access to internal layout, so cannot
> > comment on that. I ran into this issue today when I tried to rmmod ice driver
> > together with ufs_qcom driver and got SError when reloading the module because
> > ice driver was trying to access unclocked/unpowered register.
> > 
> > But you should mark the resources as 'required' in the binding and justify the
> > ABI break. No need to preserve backwards compatibility here as the binding was
> > wrong from day one.
> > 
> 
> Marking it "required" in the binding, implies that it's fine for the
> driver to fail in its absence. If I understand correctly that will
> prevent UFS and eMMC from probing, unless you have a DTB from "the
> future".
> 
> Even if I merge the dt-binding change through the qcom-tree (together
> with the driver change) I will not guarantee that torvalds/master will
> remain bisectable - because dts changes and driver changes goes in
> different branches.
> 

Yeah, that's true.

> 
> As such, the pragmatic approach is to introduce the clock as optional
> and then once we're "certain" that the dts changes has propagated we
> can consider breaking the backwards compatibility.
> 

Only if we remember to mark it required some point, fine with me!

- Mani

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

Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Manivannan Sadhasivam]

On Tue, Mar 03, 2026 at 02:11:06PM +0530, Harshal Dev wrote:
> Hi Manivannan,
> 
> On 2/20/2026 8:14 PM, Manivannan Sadhasivam wrote:
> > On Fri, Jan 23, 2026 at 12:41:35PM +0530, 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.
> > 
> > You don't need to vote for a single power domain since genpd will do that for
> > you before the driver probes.
> >
> 
> Unfortunately, without enabling the power domain during probe, I am seeing occasional
> clock stuck messages on LeMans RB8. Am I missing something? Could you point me to any
> docs with more information on the the genpd framework?
> 

genpd_dev_pm_attach() called before a platform driver probe(), powers ON the
domain.

> Logs for reference:
> 
> [    6.195019] gcc_ufs_phy_ice_core_clk status stuck at 'off'
> [    6.195031] WARNING: CPU: 5 PID: 208 at drivers/clk/qcom/clk-branch.c:87 clk_branch_toggle+0x174/0x18c
> 
> [...]
> 
> [    6.248412] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    6.248415] pc : clk_branch_toggle+0x174/0x18c
> [    6.248417] lr : clk_branch_toggle+0x174/0x18c
> [    6.248418] sp : ffff80008217b770
> [    6.248419] x29: ffff80008217b780 x28: ffff80008217bbb0 x27: ffffadf880a5f07c
> [    6.248422] x26: ffffadf880a5c1d8 x25: 0000000000000001 x24: 0000000000000001
> [    6.248424] x23: ffffadf8a0d1e740 x22: 0000000000000001 x21: ffffadf8a1d06160
> [    6.248426] x20: ffffadf89f86e5a8 x19: 0000000000000000 x18: fffffffffffe9050
> [    6.248429] x17: 000000000404006d x16: ffffadf89f8166c4 x15: ffffadf8a1ab6c70
> [    6.347820] x14: 0000000000000000 x13: ffffadf8a1ab6cf8 x12: 000000000000060f
> [    6.355145] x11: 0000000000000205 x10: ffffadf8a1b11d70 x9 : ffffadf8a1ab6cf8
> [    6.362470] x8 : 00000000ffffefff x7 : ffffadf8a1b0ecf8 x6 : 0000000000000205
> [    6.369795] x5 : ffff000ef1ceb408 x4 : 40000000fffff205 x3 : ffff521650ba3000
> [    6.377120] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000928dd780
> [    6.384444] Call trace:
> [    6.386962]  clk_branch_toggle+0x174/0x18c (P)
> [    6.391530]  clk_branch2_enable+0x1c/0x28
> [    6.395644]  clk_core_enable+0x6c/0xac
> [    6.399502]  clk_enable+0x2c/0x4c
> [    6.402913]  devm_clk_get_optional_enabled+0xac/0x108
> [    6.408096]  qcom_ice_create.part.0+0x50/0x2fc [qcom_ice]
> [    6.413646]  qcom_ice_probe+0x58/0xa8 [qcom_ice]
> [    6.418384]  platform_probe+0x5c/0x98
> [    6.422153]  really_probe+0xbc/0x29c
> [    6.425826]  __driver_probe_device+0x78/0x12c
> [    6.430307]  driver_probe_device+0x3c/0x15c
> [    6.434605]  __driver_attach+0x90/0x19c
> [    6.438547]  bus_for_each_dev+0x7c/0xe0
> [    6.442486]  driver_attach+0x24/0x30
> [    6.446158]  bus_add_driver+0xe4/0x208
> [    6.450013]  driver_register+0x5c/0x124
> [    6.453954]  __platform_driver_register+0x24/0x30
> [    6.458780]  qcom_ice_driver_init+0x24/0x1000 [qcom_ice]
> [    6.464229]  do_one_initcall+0x80/0x1c8
> [    6.468173]  do_init_module+0x58/0x234
> [    6.472028]  load_module+0x1a84/0x1c84
> [    6.475881]  init_module_from_file+0x88/0xcc
> [    6.480262]  __arm64_sys_finit_module+0x144/0x330
> [    6.485097]  invoke_syscall+0x48/0x10c
> [    6.488954]  el0_svc_common.constprop.0+0xc0/0xe0
> [    6.493790]  do_el0_svc+0x1c/0x28
> [    6.497203]  el0_svc+0x34/0xec
> [    6.500348]  el0t_64_sync_handler+0xa0/0xe4
> [    6.504645]  el0t_64_sync+0x198/0x19c
> [    6.508414] ---[ end trace 0000000000000000 ]---
> [    6.514544] qcom-ice 1d88000.crypto: probe with driver qcom-ice failed
>  
> >> Also updated the suspend and resume callbacks to handle votes on these
> >> resources.
> >>
> >> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
> > 
> > Where is the Fixes tag?
> 
> Ack, I will add it in v2 of this patch.
> 
> > 
> >> ---
> >>  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);
> > 
> > This is not needed as the power domain would be enabled at this point.
> 
> Would this be enabled due to the genpd framework? I am not observing that
> during probe. Because this call is made by the UFS/EMMC driver, perhaps you
> mean the situation at this point is different?
> 

If you pass 'power-domains' property in DT, genpd will power it ON at this
point.

> > 
> >>  	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;
> >> +	}
> > 
> > Use clk_bulk API to enable all clocks in one go.
> 
> Ack, I'll use clk_bulk_prepare_enable().
> 
> > 
> >>  	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);
> > 
> > Same here.
> 
> Ack, clk_bulk_disable_unprepare() would look good.
> As Konrad pointed out, if iface clock is not present in DT, thse APIs are
> fine with NULL pointers here.
> 
> > 
> >>  	clk_disable_unprepare(ice->core_clk);
> >> +	pm_runtime_put_sync(ice->dev);
> > 
> > Not needed.
> > 
> >>  	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);
> >> +
> > 
> > Same here. Use devm_clk_bulk_get_all_enabled().
> 
> As per discussion on the DT binding patch, I can do this once we decide to break the
> DT backward compatibility with a subsequent patch which makes both clocks mandatory.
> For v2, I am planning to continue to treat the 'iface' clock as optional via
> devm_clk_get_optional() API.
> 

Even if you do not mark 'iface' as 'required', this API will work just fine. It
will get and enable whatever clocks defined in the DT node. It is upto the
binding to define, what all should be present.

> > 
> >>  	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);
> > 
> > If you want to mark & enable the runtime PM status, you should just do:
> > 
> > 	devm_pm_runtime_set_active_enabled();	
> > 
> > But this is not really needed in this patch. You can add it in a separate patch
> > for the sake of correctness.
> 
> If my understanding is correct, I need to call pm_runtime_get_sync() to enable
> the power domain after enabling the PM runtime to ensure further calls to enable
> the iface clock do not encounter failure. Just calling devm_pm_runtime_set_active_enabled()
> will only enable the PM runtime and set it's status to 'active'. It will not enable
> the power domain.
> 

Again, you DO NOT need to handle a single power domain in the driver, genpd will
do it for you. If that is not helping, then something else is going wrong.

- Mani

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

Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Harshal Dev]

Hi Manivannan,

On 3/3/2026 10:38 PM, Manivannan Sadhasivam wrote:
> On Tue, Mar 03, 2026 at 02:11:06PM +0530, Harshal Dev wrote:
>> Hi Manivannan,
>>
>> On 2/20/2026 8:14 PM, Manivannan Sadhasivam wrote:
>>> On Fri, Jan 23, 2026 at 12:41:35PM +0530, 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.
>>>
>>> You don't need to vote for a single power domain since genpd will do that for
>>> you before the driver probes.
>>>
>>
>> Unfortunately, without enabling the power domain during probe, I am seeing occasional
>> clock stuck messages on LeMans RB8. Am I missing something? Could you point me to any
>> docs with more information on the the genpd framework?
>>
> 
> genpd_dev_pm_attach() called before a platform driver probe(), powers ON the
> domain.

You are correct. I just double confirmed this. I am going to remove all pm_runtime_* API
calls from this commit as they aren't needed. Ack.

> 
>> Logs for reference:
>>
>> [    6.195019] gcc_ufs_phy_ice_core_clk status stuck at 'off'
>> [    6.195031] WARNING: CPU: 5 PID: 208 at drivers/clk/qcom/clk-branch.c:87 clk_branch_toggle+0x174/0x18c
>>
>> [...]
>>
>> [    6.248412] pstate: 604000c5 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [    6.248415] pc : clk_branch_toggle+0x174/0x18c
>> [    6.248417] lr : clk_branch_toggle+0x174/0x18c
>> [    6.248418] sp : ffff80008217b770
>> [    6.248419] x29: ffff80008217b780 x28: ffff80008217bbb0 x27: ffffadf880a5f07c
>> [    6.248422] x26: ffffadf880a5c1d8 x25: 0000000000000001 x24: 0000000000000001
>> [    6.248424] x23: ffffadf8a0d1e740 x22: 0000000000000001 x21: ffffadf8a1d06160
>> [    6.248426] x20: ffffadf89f86e5a8 x19: 0000000000000000 x18: fffffffffffe9050
>> [    6.248429] x17: 000000000404006d x16: ffffadf89f8166c4 x15: ffffadf8a1ab6c70
>> [    6.347820] x14: 0000000000000000 x13: ffffadf8a1ab6cf8 x12: 000000000000060f
>> [    6.355145] x11: 0000000000000205 x10: ffffadf8a1b11d70 x9 : ffffadf8a1ab6cf8
>> [    6.362470] x8 : 00000000ffffefff x7 : ffffadf8a1b0ecf8 x6 : 0000000000000205
>> [    6.369795] x5 : ffff000ef1ceb408 x4 : 40000000fffff205 x3 : ffff521650ba3000
>> [    6.377120] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000928dd780
>> [    6.384444] Call trace:
>> [    6.386962]  clk_branch_toggle+0x174/0x18c (P)
>> [    6.391530]  clk_branch2_enable+0x1c/0x28
>> [    6.395644]  clk_core_enable+0x6c/0xac
>> [    6.399502]  clk_enable+0x2c/0x4c
>> [    6.402913]  devm_clk_get_optional_enabled+0xac/0x108
>> [    6.408096]  qcom_ice_create.part.0+0x50/0x2fc [qcom_ice]
>> [    6.413646]  qcom_ice_probe+0x58/0xa8 [qcom_ice]
>> [    6.418384]  platform_probe+0x5c/0x98
>> [    6.422153]  really_probe+0xbc/0x29c
>> [    6.425826]  __driver_probe_device+0x78/0x12c
>> [    6.430307]  driver_probe_device+0x3c/0x15c
>> [    6.434605]  __driver_attach+0x90/0x19c
>> [    6.438547]  bus_for_each_dev+0x7c/0xe0
>> [    6.442486]  driver_attach+0x24/0x30
>> [    6.446158]  bus_add_driver+0xe4/0x208
>> [    6.450013]  driver_register+0x5c/0x124
>> [    6.453954]  __platform_driver_register+0x24/0x30
>> [    6.458780]  qcom_ice_driver_init+0x24/0x1000 [qcom_ice]
>> [    6.464229]  do_one_initcall+0x80/0x1c8
>> [    6.468173]  do_init_module+0x58/0x234
>> [    6.472028]  load_module+0x1a84/0x1c84
>> [    6.475881]  init_module_from_file+0x88/0xcc
>> [    6.480262]  __arm64_sys_finit_module+0x144/0x330
>> [    6.485097]  invoke_syscall+0x48/0x10c
>> [    6.488954]  el0_svc_common.constprop.0+0xc0/0xe0
>> [    6.493790]  do_el0_svc+0x1c/0x28
>> [    6.497203]  el0_svc+0x34/0xec
>> [    6.500348]  el0t_64_sync_handler+0xa0/0xe4
>> [    6.504645]  el0t_64_sync+0x198/0x19c
>> [    6.508414] ---[ end trace 0000000000000000 ]---
>> [    6.514544] qcom-ice 1d88000.crypto: probe with driver qcom-ice failed
>>  
>>>> Also updated the suspend and resume callbacks to handle votes on these
>>>> resources.
>>>>
>>>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>>>
>>> Where is the Fixes tag?
>>
>> Ack, I will add it in v2 of this patch.
>>
>>>
>>>> ---
>>>>  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);
>>>
>>> This is not needed as the power domain would be enabled at this point.
>>
>> Would this be enabled due to the genpd framework? I am not observing that
>> during probe. Because this call is made by the UFS/EMMC driver, perhaps you
>> mean the situation at this point is different?
>>
> 
> If you pass 'power-domains' property in DT, genpd will power it ON at this
> point.

Ack.

> 
>>>
>>>>  	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;
>>>> +	}
>>>
>>> Use clk_bulk API to enable all clocks in one go.
>>
>> Ack, I'll use clk_bulk_prepare_enable().
>>
>>>
>>>>  	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);
>>>
>>> Same here.
>>
>> Ack, clk_bulk_disable_unprepare() would look good.
>> As Konrad pointed out, if iface clock is not present in DT, thse APIs are
>> fine with NULL pointers here.
>>
>>>
>>>>  	clk_disable_unprepare(ice->core_clk);
>>>> +	pm_runtime_put_sync(ice->dev);
>>>
>>> Not needed.
>>>
>>>>  	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);
>>>> +
>>>
>>> Same here. Use devm_clk_bulk_get_all_enabled().
>>
>> As per discussion on the DT binding patch, I can do this once we decide to break the
>> DT backward compatibility with a subsequent patch which makes both clocks mandatory.
>> For v2, I am planning to continue to treat the 'iface' clock as optional via
>> devm_clk_get_optional() API.
>>
> 
> Even if you do not mark 'iface' as 'required', this API will work just fine. It
> will get and enable whatever clocks defined in the DT node. It is upto the
> binding to define, what all should be present.

Agreed Manivannan, however, I realize that for legacy DT bindings, where ICE instance is
specified as part of the UFS/EMMC driver node, qcom_ice_create() receives the storage
device, if we call devm_clk_bulk_get_all_enabled() then all clocks specified in the
storage node would be returned and enabled. However, qcom_ice_create() should only enable
clocks relevant for ICE operation, i.e., core and iface clocks. iface being optional
for the time being as discussed.

And so, for suspend() and resume() as well, it seems I will have to continue with preparing
and enabling/disabling both the clocks individually.

> 
>>>
>>>>  	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);
>>>
>>> If you want to mark & enable the runtime PM status, you should just do:
>>>
>>> 	devm_pm_runtime_set_active_enabled();	
>>>
>>> But this is not really needed in this patch. You can add it in a separate patch
>>> for the sake of correctness.
>>
>> If my understanding is correct, I need to call pm_runtime_get_sync() to enable
>> the power domain after enabling the PM runtime to ensure further calls to enable
>> the iface clock do not encounter failure. Just calling devm_pm_runtime_set_active_enabled()
>> will only enable the PM runtime and set it's status to 'active'. It will not enable
>> the power domain.
>>
> 
> Again, you DO NOT need to handle a single power domain in the driver, genpd will
> do it for you. If that is not helping, then something else is going wrong.
> 

Ack.

Regards,
Harshal

> - Mani
> 

Re: [PATCH 11/11] soc: qcom: ice: Add explicit power-domain and clock voting calls for ICE

[Manivannan Sadhasivam]

On Mon, Mar 09, 2026 at 05:29:34PM +0530, Harshal Dev wrote:

[...]

> >> As per discussion on the DT binding patch, I can do this once we decide to break the
> >> DT backward compatibility with a subsequent patch which makes both clocks mandatory.
> >> For v2, I am planning to continue to treat the 'iface' clock as optional via
> >> devm_clk_get_optional() API.
> >>
> > 
> > Even if you do not mark 'iface' as 'required', this API will work just fine. It
> > will get and enable whatever clocks defined in the DT node. It is upto the
> > binding to define, what all should be present.
> 
> Agreed Manivannan, however, I realize that for legacy DT bindings, where ICE instance is
> specified as part of the UFS/EMMC driver node, qcom_ice_create() receives the storage
> device, if we call devm_clk_bulk_get_all_enabled() then all clocks specified in the
> storage node would be returned and enabled. However, qcom_ice_create() should only enable
> clocks relevant for ICE operation, i.e., core and iface clocks. iface being optional
> for the time being as discussed.
> 

Yikes! This design is too ugly... But anyhow, we have to live with it.

> And so, for suspend() and resume() as well, it seems I will have to continue with preparing
> and enabling/disabling both the clocks individually.
> 

Ok fine.

- Mani

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