Re: [PATCH v3 01/12] dt-bindings: crypto: qcom,ice: Allow power-domain and iface clk

[Harshal Dev <harshal.dev@oss.qualcomm.com>]

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