[PATCH 1/6] ARM: dts: sam9x60: Add OHCI and EHCI DT nodes

[Marek Vasut]

On 1/4/23 11:11, Eugen.Hristev at microchip.com wrote:
> On 1/4/23 11:38, Marek Vasut wrote:
>> On 1/4/23 08:30, Eugen.Hristev at microchip.com wrote:
>>> On 1/3/23 01:12, Marek Vasut wrote:
>>>> On 12/23/22 13:33, Sergiu Moga wrote:
>>>>> Add the OHCI and EHCI DT nodes for the sam9x60 boards.
>>>>>
>>>>> Signed-off-by: Sergiu Moga <sergiu.moga at microchip.com>
>>>>> ---
>>>>>     arch/arm/dts/at91-sam9x60_curiosity.dts | 21 +++++++++++++++++++++
>>>>>     arch/arm/dts/sam9x60.dtsi               | 18 ++++++++++++++++++
>>>>
>>>> Board and SoC DT changes should be in separate patches.
>>>>
>>>>>     arch/arm/dts/sam9x60ek.dts              | 21 +++++++++++++++++++++
>>>>>     3 files changed, 60 insertions(+)
>>>>
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/arch/arm/dts/sam9x60.dtsi b/arch/arm/dts/sam9x60.dtsi
>>>>> index 17224ef771..e36a540f78 100644
>>>>> --- a/arch/arm/dts/sam9x60.dtsi
>>>>> +++ b/arch/arm/dts/sam9x60.dtsi
>>>>> @@ -69,6 +69,24 @@
>>>>>                 #size-cells = <1>;
>>>>>                 ranges;
>>>>>
>>>>> +             usb1: ohci at 600000 {
>>>>
>>>> This should be usb@ instead of ohci@ , if you run "make dtbs_check" on
>>>> this DT in Linux (please do), you would likely get a warning , see Linux
>>>> Documentation/devicetree/bindings/usb/usb.yaml .
>>>
>>>
>>> If in Linux we have ohci@ , then we need to have the same in U-boot.
>>
>> You should update the Linux DT to usb@ too to avoid dtbs_check warnings.
>>
>>> We can accept the change to usb@ , if there is a pending patch in Linux.
>>
>> I can make the same argument about Linux, since DTs are OS agnostic. My
>> comment is OS agnostic too and does not apply specifically to U-Boot or
>> Linux, it applies to DT.
> 
> Definitely you can comment in Linux , for sure.
> 
>>
>>> U-boot is not the place to review the devicetree.
>>
>> I strongly disagree with this statement.
> 
> There are a few decisions behind this statement.
> First, the fact that we no longer take bindings into Uboot, and rely
> solely on bindings in Linux. This means the bindings were reviewed and
> validated in Linux.

Linux
1e5f532c27371 ("ARM: dts: at91: sam9x60: add device tree for soc and board")
which added these ohci@ nodes has been accepted after Linux
14ec072a19ad7 ("dt-bindings: usb: Convert USB HCD generic binding to YAML")
was already in place.

Therefore, nobody ran the dtbs_check at that point, which is somewhat 
understandable, since the dtbs_check at that point was in very early stages.

This however shows the review process is not flawless and subsequent 
updates may be necessary.

One such update example is Linux:
979813d2ab705 ("ARM: dts: at91: use generic name for reset controller")
which does:
-                       reset_controller: rstc at fffffe00 {
+                       reset_controller: reset-controller at fffffe00 {
or another one:
6a743ea387e63 ("ARM: dts: at91: Use the generic "rtc" node name for the 
rtt IPs")
-                       rtt: rtt at fffffe20 {
+                       rtt: rtc at fffffe20 {
or another one:
4b6140b96dfe4 ("ARM: dts: at91: Use the generic "crypto" node name for 
the crypto IPs")
-                       sha: sha at f002c000 {
+                       sha: crypto at f002c000 {

> Sure there are a few exceptions but not the usual rule.
> Second, we rely on devicetree kernel.org mailing list to review all the
> DTs and binding compliance in Linux.
> This means that the DT has been validated and checked as ABI in Linux.

I think the ABI argument does not work, your own engineers would've 
caused multiple ABI breaks by now, see above.

> Hence there is no place to question that in Uboot or any other project.

As you can see above, Linux kernel review process is not perfect and the 
code is constantly evolving rather than begin set in stone. I believe 
other projects and their reviewers are no worse than Linux kernel ones, 
and can provide valid review feedback too.

> The DT must be the same and ABI across all software that uses the DT. If
> this was validated and agreed upon in Linux, we can't question that here.
> So if you disagree with this statement, it's your choice of course.

I strongly disagree.

>> What does it matter where the review feedback came from ?
>> What does matter is that you can improve the DT based on that feedback,
>> if the feedback is valid.
> 
> I agree with that, but if we want to improve things, we need to patch
> things either in Linux first, or patch projects together.
> The fact that you are against some patches while they have the same
> identical node in Linux, is not a reason to not allow devicetree nodes
> to be added to Uboot.
> DT syncing with Linux is always a must, and that is part of what Sergiu
> is doing with these patches.

See below.

>>> The devicetree must be in sync with Linux.
>>
>> I agree with this statement.
>>
>> Please update the Linux DT too.
> 
> That is one approach but as I said, not a reason to reject a node that
> is identical with Linux.

Ultimately, I am not the at91 maintainer, so I have no say in this.

I provided review feedback, that the DT checker would warn on the node 
name, and explained how to deal with it.

Whether the feedback is accepted and the DT is improved , or , the 
feedback is discarded and the DT checker keeps producing warnings , is 
not my decision to make.