Message-ID: <6efcdf51-bdb1-4dfc-aa5e-8b7dc8c68cd3@kernel.org>
Date: Fri, 6 Feb 2026 11:50:29 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Harshal Dev <harshal.dev@....qualcomm.com>,
 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

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.


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

Powered by blists - more mailing lists