Message-ID: <434ca669-e136-1bbf-7907-0d2110851a18@xilinx.com>
Date:   Fri, 22 Jan 2021 10:00:11 +0100
From:   Michal Simek <michal.simek@...inx.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Michal Simek <michal.simek@...inx.com>
CC:     <linux-kernel@...r.kernel.org>, <monstr@...str.eu>,
        <git@...inx.com>, Kalyani Akula <kalyani.akula@...inx.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Manish Narani <manish.narani@...inx.com>,
        Rajan Vaja <rajan.vaja@...inx.com>,
        Rob Herring <robh+dt@...nel.org>, <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 06/12] arm64: dts: zynqmp: Add label for zynqmp_ipi

Hi,

On 1/21/21 11:29 PM, Laurent Pinchart wrote:
> Hi Michal,
> 
> I've just realized I forgot to reply to this e-mail, sorry.
> 
> On Tue, Dec 08, 2020 at 08:26:41AM +0100, Michal Simek wrote:
>> On 07. 12. 20 23:16, Laurent Pinchart wrote:
>>> On Mon, Dec 07, 2020 at 10:39:25AM +0100, Michal Simek wrote:
>>>> On 06. 12. 20 23:46, Laurent Pinchart wrote:
>>>>> On Wed, Dec 02, 2020 at 03:06:05PM +0100, Michal Simek wrote:
>>>>>> Add label which is used by bootloader for adding bootloader specific flag.
>>>>>>
>>>>>> Signed-off-by: Michal Simek <michal.simek@...inx.com>
>>>>>> ---
>>>>>>
>>>>>> U-Boot needs to add u-boot,dm-pre-reloc; property
>>>>>
>>>>> I'm not entirely sure what best practice rules are in this area, but
>>>>> shouldn't U-Boot locate the node by name instead of label ?
>>>>
>>>> Labels are not listed in dt binding and there are two approaches how to
>>>> reference nodes. Via full path with node name or via labels.
>>>> I do normally use labels which are much simple.
>>>
>>> Note that labels require the DTB to be compiled with the -@ option,
>>> otherwise they're not present in the binary.
>>
>> U-Boot is using different concept. You can see that there are a lot of
>> -u-boot.dtsi files in dts folders. These are automatically included to
>> DTS before DTC is called. It means you don't need to build overlay to
>> get merged.
>>
>>>> And also if you take a look how dtb looks like (convert back to dts) you
>>>> can see that for example aliases are using full path (just &label) but
>>>> clocks/gic which is the part of <> is handled via phandles as numbers.
>>>>
>>>> And labels names can vary and shouldn't be the part of binding doc as
>>>> far as I know. But I can be wrong of course.
>>>
>>> The DT bindings should document the interface with the operating system,
>>> and if applicable, the boot loader. If the boot loader requires a
>>> particular label, then it becomes part of the ABI, and I think it should
>>> be documented in the bindings.
>>
>> We have been discussing with Rob some month ago but didn't have a time
>> to do step further. Just keep it short Rob was ok to keep bootloader
>> binding inside Linux repo.
> 
> I think that makes sense, DT bindings are meant to be OS-agnostic, so
> boot loader requirements should be documented there.
> 
>> There is no hardcoding for a particular name. There is just a need to
>> have any label. U-Boot needs to have one property(e.g.
>> u-boot,dm-pre-reloc;) just to do early allocation.
>> The name is just reference and none is really looking for it. It is just
>> a way how to include it in much easier way.
> 
> Just to make sure I understand this issue correctly, does this mean that
> you need to reference the node in a *-u-boot.dtsi file, and want a label
> to do so ? The label name needs to be the same in the base file (taken
> from the Linux source tree) and the *-u-boot.dtsi file (in the U-Boot
> source tree) in that case. Isn't it the role of DT bindings to document
> such requirements ?

I prefer to have all nodes with labels just in case you need to
reference it. Simply based on experience it happens time to time that
something needs to be aligned, new property added and sometimes we touch
dt overlays. It means I prefer to have labels for all nodes.

In connection to u-boot. U-Boot introduced *-u-boot.dtsi files for
bootloader specific configurations. I think that's sort of bad design
and it should be done differently by simply document/align this binding
with the kernel. And in this u-boot,dm-pre-reloc case it should be
handle differently and any transition is required.
Normally *-u-boot.dtsi should just reference node in "root" dts via labels.
And u-boot is trying to align dts with the kernel.

And last part on this if make sense to also document labels as
requirement. I don't think it is good idea. You can have in general
multiple instances of the same IP which you need to add some properties
to. You would have to defined all of them for all existing SOCs which
will be very painful. It also takes so much time to upstream it that's
why checking one more thing there is IMHO just additional step.

I think that everybody should be trying to align all things together
across all projects. That's for example what I want to do in this gtr/dp
between Linux/U-Boot.

Thanks,
Michal

Powered by blists - more mailing lists