Message-ID: <b217a08a-2755-4ef8-bf39-af1c3e628cf8@oss.qualcomm.com>
Date: Mon, 9 Feb 2026 11:13:06 +0530
From: Harshal Dev <harshal.dev@....qualcomm.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
 <conor+dt@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        Abel Vesa <abel.vesa@....qualcomm.com>,
        cros-qcom-dts-watchers@...omium.org
Cc: Brian Masney <bmasney@...hat.com>,
        Neeraj Soni <neeraj.soni@....qualcomm.com>,
        Gaurav Kashyap <gaurav.kashyap@....qualcomm.com>,
        linux-arm-msm@...r.kernel.org, linux-crypto@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/11] dt-bindings: crypto: qcom,ice: Require power-domain
 and iface clk

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

Powered by blists - more mailing lists