linux-kernel - Re: [PATCH v2 2/6] dt-bindings: pwm: amlogic: add new compatible for meson8 pwm type

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 22 Nov 2023 19:09:00 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Jerome Brunet <jbrunet@...libre.com>
Cc:     neil.armstrong@...aro.org, Rob Herring <robh@...nel.org>,
        JunYi Zhao <junyi.zhao@...ogic.com>,
        devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Kevin Hilman <khilman@...libre.com>,
        Thierry Reding <thierry.reding@...il.com>,
        linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
        linux-amlogic@...ts.infradead.org,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH v2 2/6] dt-bindings: pwm: amlogic: add new compatible for
 meson8 pwm type

On 22/11/2023 17:14, Jerome Brunet wrote:
> 
> On Wed 22 Nov 2023 at 16:46, Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> wrote:
> 
> 
>>>>>>
>>>>>> Again, where the "v2" is defined? Where is any document explaining the
>>>>>> mapping between version blocks and SoC parts? Why do you list here only
>>>>>> major version? Blocks almost always have also minor (e.g. v2.0).
>>>>>
>>>>> Again, v2 does has nothing to do with the HW. Never wrote it was.
>>>>> The HW remains the same.
>>>>
>>>> Don't add compatibles which are not related to HW, but represent
>>>> software versioning. Software does not matter for the bindings.
>>>
>>> What I did I explicitly what is recommended in Grant's presentation from
>>> 2013. 10y old, but I assume slide 10 "Making an incompatible update" is
>>> still valid.
>>>
>>> https://elinux.org/images/1/1e/DT_Binding_Process_glikely_ksummit_2013_10_28.pdf
>>>
>>> Breaking the ABI of the old compatible would break all boards which use
>>> u-boot DT and pass it to the kernel, because the meaning of the clock
>>> property would change.
>>
>> You broke U-Boot now as well - it will get your new DTS from the kernel
>> and stop working.
> 
> U-boot will continue to match the old compatible and work properly.
> When the dts using the new compatible lands in u-boot, it won't
> match until proper driver support is added. It is a lot better than
> breaking the ABI, which would have silently broke u-boot.
> 
> I don't really see a way around that.
> 
> If you have better way to fix a bad interface, feel free to share it.
> 
>>
>>>
>>> Doing things has suggested in this slide, and this patch, allows every
>>> device to continue to work properly, whether the DT given is the one
>>> shipped with u-boot (using the old compatible for now) or the kernel.
>>
>> OK, that explains the reasons. I read your commit msg and nothing like
>> this was mentioned there. What's more, you did not deprecate the old
>> binding, thus the confusion - it looked like you add entirely new
>> hardware (although you put "deprecated" but in some unrelated place, not
>> next to the compatibles).
> 
> The old interface being obsoleted by the new one is mentionned in the
> commit description, the comments in the bindings and the bindings itself.
> Thanks a lot for pointing out the placement mistake. I'll fix it.
> 
> The commit description says:
> * What the patch does
> * Why it does it:
>   * Why the old bindings is bad/broken
>   * How the new ones fixes the problem
> * Why a single compatible properly describes, IMO, all the related HW.
> 
> This describes the entirety of what the change does.
> That seemed clear enough for Rob. If that is not enough for you and you
> would like it reworded, could please provide a few suggestions ?

You did not deprecate the compatibles, so this has to be fixed. You put
the compatible in some other place, not really relevant.

> 
>>
>> Anyway, the main point of Neil was that you started using generic
>> compatible for all SoCs, which is wrong as well. I guess this was the
>> original discussion.
> 
> The whole reason for this change is to properly describe the HW, which
> is the 100% same on all the SoCs, or SoC families, concerned. The only

You still need specific compatibles, because the hardware is not 100%
the same. Programming model can, but hardware differs. Many times
engineers thought that devices are 100% compatible and then turned out
they are not. I am bored to repeat all this again and again.

> reason there was a lot of old compatibles is because it was used to match
> data in the driver (this is clearly wrong). This data would now be
> passed through DT.
> 
> I have been clear about this in the change description.
> 
> So why is it wrong to have single compatible for a type of device that
> is 100% the same HW ?

Because it is generic, not specific (you match "foo" against "bar" SoC).
The chapter from writing-bindings you referenced earlier mentioned this.
You need ability to add quirks and customize for these minor hardware
differences, even if programming model is the same.

> 
> It is lot a easier to apply a rule correctly when the intent is clear.
> 
>>
>> Best regards,
>> Krzysztof
> 

Best regards,
Krzysztof