Update the inline-crypto engine DT binding in a backward compatible manner
to allow specifying up to two clocks along with their names and associated
power-domain.
When the 'clk_ignore_unused' flag is not passed on the kernel command line
occasional unclocked ICE hardware register access are observed when the
kernel disables the unused 'iface' clock before ICE can probe. On the other
hand, when the 'pd_ignore_unused' flag is not passed on the command line,
clock 'stuck' issues are observed if the power-domain required by ICE
hardware is unused and thus disabled before ICE probe could happen.
To avoid these scenarios, the 'iface' clock and the associated power-domain
should be specified in the ICE device tree node and enabled by ICE.
Fixes: f6ff91a47ac57 ("dt-bindings: crypto: Add Qualcomm Inline Crypto Engine")
Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
---
.../bindings/crypto/qcom,inline-crypto-engine.yaml | 16 +++++++++++++++-
1 file changed, 15 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 876bf90ed96e..99c541e7fa8c 100644
--- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
+++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
@@ -30,6 +30,16 @@ properties:
maxItems: 1
clocks:
+ minItems: 1
+ maxItems: 2
+
+ clock-names:
+ minItems: 1
+ items:
+ - const: core
+ - const: iface
+
+ power-domains:
maxItems: 1
operating-points-v2: true
@@ -52,7 +62,11 @@ 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 = "core",
+ "iface";
+ power-domains = <&gcc UFS_PHY_GDSC>;
operating-points-v2 = <&ice_opp_table>;
--
2.34.1On Tue, Mar 17, 2026 at 02:50:40PM +0530, Harshal Dev wrote:
> Update the inline-crypto engine DT binding in a backward compatible manner
> to allow specifying up to two clocks along with their names and associated
> power-domain.
This should come after the "why" part.
>
> When the 'clk_ignore_unused' flag is not passed on the kernel command line
> occasional unclocked ICE hardware register access are observed when the
> kernel disables the unused 'iface' clock before ICE can probe. On the other
> hand, when the 'pd_ignore_unused' flag is not passed on the command line,
> clock 'stuck' issues are observed if the power-domain required by ICE
> hardware is unused and thus disabled before ICE probe could happen.
You can simply say that ICE requires these clocks and these power
domains to function. Accessing the hardware can fail if they are
disabled by the kernel for whater reasons.
>
> To avoid these scenarios, the 'iface' clock and the associated power-domain
> should be specified in the ICE device tree node and enabled by ICE.
>
> Fixes: f6ff91a47ac57 ("dt-bindings: crypto: Add Qualcomm Inline Crypto Engine")
> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
> ---
> .../bindings/crypto/qcom,inline-crypto-engine.yaml | 16 +++++++++++++++-
> 1 file changed, 15 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 876bf90ed96e..99c541e7fa8c 100644
> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> @@ -30,6 +30,16 @@ properties:
> maxItems: 1
>
> clocks:
> + minItems: 1
> + maxItems: 2
> +
> + clock-names:
> + minItems: 1
> + items:
> + - const: core
> + - const: iface
> +
> + power-domains:
> maxItems: 1
>
> operating-points-v2: true
> @@ -52,7 +62,11 @@ 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 = "core",
> + "iface";
We don't actually need names here. You can use indices instead, making
the change completely backwards-compatible.
> + power-domains = <&gcc UFS_PHY_GDSC>;
>
> operating-points-v2 = <&ice_opp_table>;
>
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
On Tue, Mar 17, 2026 at 05:12:36PM +0200, Dmitry Baryshkov wrote:
> On Tue, Mar 17, 2026 at 02:50:40PM +0530, Harshal Dev wrote:
> > Update the inline-crypto engine DT binding in a backward compatible manner
> > to allow specifying up to two clocks along with their names and associated
> > power-domain.
>
> This should come after the "why" part.
>
> >
> > When the 'clk_ignore_unused' flag is not passed on the kernel command line
> > occasional unclocked ICE hardware register access are observed when the
> > kernel disables the unused 'iface' clock before ICE can probe. On the other
> > hand, when the 'pd_ignore_unused' flag is not passed on the command line,
> > clock 'stuck' issues are observed if the power-domain required by ICE
> > hardware is unused and thus disabled before ICE probe could happen.
>
> You can simply say that ICE requires these clocks and these power
> domains to function. Accessing the hardware can fail if they are
> disabled by the kernel for whater reasons.
Yeah, mentioning clk_ignore_unused/pd is redundant here.
>
> >
> > To avoid these scenarios, the 'iface' clock and the associated power-domain
> > should be specified in the ICE device tree node and enabled by ICE.
And this repeats the first paragraph.
> >
> > Fixes: f6ff91a47ac57 ("dt-bindings: crypto: Add Qualcomm Inline Crypto Engine")
> > Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
> > ---
> > .../bindings/crypto/qcom,inline-crypto-engine.yaml | 16 +++++++++++++++-
> > 1 file changed, 15 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 876bf90ed96e..99c541e7fa8c 100644
> > --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> > +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> > @@ -30,6 +30,16 @@ properties:
> > maxItems: 1
> >
> > clocks:
> > + minItems: 1
> > + maxItems: 2
> > +
> > + clock-names:
> > + minItems: 1
> > + items:
> > + - const: core
> > + - const: iface
> > +
> > + power-domains:
> > maxItems: 1
I do not see how you implemented my feedback, at all. Nothing from it.
I even provided you final clarifications. What is the point of asking me
for the third time, again, what should you do, if you just ignore it?
1. What the DTS is doing here?
2. How did you address "with explanation why this is a fix thus why this
should go to current cycle." - where is this part?
3. Where is Eliza and Milos?
I was repeating the last 2 points multiple times already.
Best regards,
Krzysztof
On 3/18/2026 12:52 PM, Krzysztof Kozlowski wrote:
> On Tue, Mar 17, 2026 at 05:12:36PM +0200, Dmitry Baryshkov wrote:
>> On Tue, Mar 17, 2026 at 02:50:40PM +0530, Harshal Dev wrote:
>>> Update the inline-crypto engine DT binding in a backward compatible manner
>>> to allow specifying up to two clocks along with their names and associated
>>> power-domain.
>>
>> This should come after the "why" part.
>>
>>>
>>> When the 'clk_ignore_unused' flag is not passed on the kernel command line
>>> occasional unclocked ICE hardware register access are observed when the
>>> kernel disables the unused 'iface' clock before ICE can probe. On the other
>>> hand, when the 'pd_ignore_unused' flag is not passed on the command line,
>>> clock 'stuck' issues are observed if the power-domain required by ICE
>>> hardware is unused and thus disabled before ICE probe could happen.
>>
>> You can simply say that ICE requires these clocks and these power
>> domains to function. Accessing the hardware can fail if they are
>> disabled by the kernel for whater reasons.
>
> Yeah, mentioning clk_ignore_unused/pd is redundant here.
Ack.
>
>>
>>>
>>> To avoid these scenarios, the 'iface' clock and the associated power-domain
>>> should be specified in the ICE device tree node and enabled by ICE.
>
> And this repeats the first paragraph.
Ack.
>
>>>
>>> Fixes: f6ff91a47ac57 ("dt-bindings: crypto: Add Qualcomm Inline Crypto Engine")
>>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>>> ---
>>> .../bindings/crypto/qcom,inline-crypto-engine.yaml | 16 +++++++++++++++-
>>> 1 file changed, 15 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 876bf90ed96e..99c541e7fa8c 100644
>>> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>> @@ -30,6 +30,16 @@ properties:
>>> maxItems: 1
>>>
>>> clocks:
>>> + minItems: 1
>>> + maxItems: 2
>>> +
>>> + clock-names:
>>> + minItems: 1
>>> + items:
>>> + - const: core
>>> + - const: iface
>>> +
>>> + power-domains:
>>> maxItems: 1
>
>
> 1. What the DTS is doing here?
Okay. I will add a description of the expectation imposed by this binding on
the DTS in the commit message of this patch.
> 2. How did you address "with explanation why this is a fix thus why this
> should go to current cycle." - where is this part?
My mistake, I will explicitly write in the commit message that this change
is fixing the issues caused by missing power-domain and clocks in the DTS
by preserving backward-compatibility for old devices and constraining these
resources for new ones, i.e, Eliza and Milos.
> 3. Where is Eliza and Milos?
I will merge the unnecessary commit I introduced as Patch 2 into this commit
and explain that we are making the clock and power-domain 'required' for
Eliza and Milos since they constitute unreleased ABI and can carry this
new constraint.
Regards,
Harshal
>
> Best regards,
> Krzysztof
>
On 18/03/2026 11:30, Harshal Dev wrote:
>
>
> On 3/18/2026 12:52 PM, Krzysztof Kozlowski wrote:
>> On Tue, Mar 17, 2026 at 05:12:36PM +0200, Dmitry Baryshkov wrote:
>>> On Tue, Mar 17, 2026 at 02:50:40PM +0530, Harshal Dev wrote:
>>>> Update the inline-crypto engine DT binding in a backward compatible manner
>>>> to allow specifying up to two clocks along with their names and associated
>>>> power-domain.
>>>
>>> This should come after the "why" part.
>>>
>>>>
>>>> When the 'clk_ignore_unused' flag is not passed on the kernel command line
>>>> occasional unclocked ICE hardware register access are observed when the
>>>> kernel disables the unused 'iface' clock before ICE can probe. On the other
>>>> hand, when the 'pd_ignore_unused' flag is not passed on the command line,
>>>> clock 'stuck' issues are observed if the power-domain required by ICE
>>>> hardware is unused and thus disabled before ICE probe could happen.
>>>
>>> You can simply say that ICE requires these clocks and these power
>>> domains to function. Accessing the hardware can fail if they are
>>> disabled by the kernel for whater reasons.
>>
>> Yeah, mentioning clk_ignore_unused/pd is redundant here.
>
> Ack.
>
>>
>>>
>>>>
>>>> To avoid these scenarios, the 'iface' clock and the associated power-domain
>>>> should be specified in the ICE device tree node and enabled by ICE.
>>
>> And this repeats the first paragraph.
>
> Ack.
>
>>
>>>>
>>>> Fixes: f6ff91a47ac57 ("dt-bindings: crypto: Add Qualcomm Inline Crypto Engine")
>>>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>>>> ---
>>>> .../bindings/crypto/qcom,inline-crypto-engine.yaml | 16 +++++++++++++++-
>>>> 1 file changed, 15 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 876bf90ed96e..99c541e7fa8c 100644
>>>> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>>>> @@ -30,6 +30,16 @@ properties:
>>>> maxItems: 1
>>>>
>>>> clocks:
>>>> + minItems: 1
>>>> + maxItems: 2
>>>> +
>>>> + clock-names:
>>>> + minItems: 1
>>>> + items:
>>>> + - const: core
>>>> + - const: iface
>>>> +
>>>> + power-domains:
>>>> maxItems: 1
>>
>>
>> 1. What the DTS is doing here?
>
> Okay. I will add a description of the expectation imposed by this binding on
> the DTS in the commit message of this patch.
You target this to fixes. Your subject and PATCH prefix should state
that. DTS is irrelevant in the git history in this context.
I stated previous the reason why this must go to the fixes.
Best regards,
Krzysztof
Hi Dmitry ,
On 3/17/2026 8:42 PM, Dmitry Baryshkov wrote:
> On Tue, Mar 17, 2026 at 02:50:40PM +0530, Harshal Dev wrote:
>> Update the inline-crypto engine DT binding in a backward compatible manner
>> to allow specifying up to two clocks along with their names and associated
>> power-domain.
>
> This should come after the "why" part.
Ack.
>
>>
>> When the 'clk_ignore_unused' flag is not passed on the kernel command line
>> occasional unclocked ICE hardware register access are observed when the
>> kernel disables the unused 'iface' clock before ICE can probe. On the other
>> hand, when the 'pd_ignore_unused' flag is not passed on the command line,
>> clock 'stuck' issues are observed if the power-domain required by ICE
>> hardware is unused and thus disabled before ICE probe could happen.
>
> You can simply say that ICE requires these clocks and these power
> domains to function. Accessing the hardware can fail if they are
> disabled by the kernel for whater reasons.
>
Ack.
>>
>> To avoid these scenarios, the 'iface' clock and the associated power-domain
>> should be specified in the ICE device tree node and enabled by ICE.
>>
>> Fixes: f6ff91a47ac57 ("dt-bindings: crypto: Add Qualcomm Inline Crypto Engine")
>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>> ---
>> .../bindings/crypto/qcom,inline-crypto-engine.yaml | 16 +++++++++++++++-
>> 1 file changed, 15 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 876bf90ed96e..99c541e7fa8c 100644
>> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
>> @@ -30,6 +30,16 @@ properties:
>> maxItems: 1
>>
>> clocks:
>> + minItems: 1
>> + maxItems: 2
>> +
>> + clock-names:
>> + minItems: 1
>> + items:
>> + - const: core
>> + - const: iface
>> +
>> + power-domains:
>> maxItems: 1
>>
>> operating-points-v2: true
>> @@ -52,7 +62,11 @@ 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 = "core",
>> + "iface";
>
> We don't actually need names here. You can use indices instead, making
> the change completely backwards-compatible.
>
I do not have very concrete objections to this. But introducing the clock
names isn't breaking backward compatibility either. I wanted to continue
using the names since the ICE driver has been following the tradition of
referring these clocks via names since it was part of the UFS/EMMC driver.
This also helps me avoid touching the ICE driver source code for specifying
the index of the clocks.
Let me know if continuing to use the names is a no-go from you for some
other reason.
Thanks,
Harshal
>> + power-domains = <&gcc UFS_PHY_GDSC>;
>>
>> operating-points-v2 = <&ice_opp_table>;
>>
>>
>> --
>> 2.34.1
>>
>
© 2016 - 2026 Red Hat, Inc.