[U-Boot] [PATCH 11/13] arm: zynq: dts: Add U-Boot device tree additions

[Simon Glass]

Hi Michal,

On 18 September 2015 at 19:07, Michal Simek <monstr at monstr.eu> wrote:
> Hi Simon,
>
> first of all sorry for late reply. I am traveling and I have pretty busy
> time now.

That's OK, I don't think there is any particular hurry.

>
> 2015-09-09 20:07 GMT+02:00 Simon Glass <sjg at chromium.org>:
>>
>> Hi Michal,
>>
>> On Friday, 4 September 2015, Michal Simek <monstr at monstr.eu> wrote:
>> >
>> > Hi Simon,
>> >
>> > <cut some previous parts>
>> >
>> > >>>>>>>>
>> > >>>>>>>> We also use this with fdtgrep to remove nodes not needed for
>> > >>>>>>>> SPL. So
>> > >>>>>>>> we would have to come up with a tool to make that work. At
>> > >>>>>>>> present
>> > >>>>>>>> 'fdtgrep -p u-boot,dm-pre-reloc' picks out all the nodes we
>> > >>>>>>>> want (it
>> > >>>>>>>> finds nodes with that property).
>> > >>>>>>>>
>> > >>>>>>>> I'm actually not sure that this approach is any easier/better.
>> > >>>>>>>> What
>> > >>>>>>>> are the advantages?
>> > >>>>>>>
>> > >>>>>>> The question is if current solution which you are using is fully
>> > >>>>>>> compatible with binding. Adding bootloader property to the HW
>> > >>>>>>> node
>> > >>>>>>> doesn't look like a best solution.
>> > >>>>>>> On the other hand chosen node is the location where OS specific
>> > >>>>>>> properties are coming that's why I have suggested to use it.
>> > >>>>>>
>> > >>>>>>
>> > >>>>>> I like Michal's idea.
>> > >>>>>> We need some work for fdtgrep, but I believe it is worthwhile.
>> > >>>>>>
>> > >>>>>> From Michal's recent patches
>> > >>>>>> (http://patchwork.ozlabs.org/patch/498609/),
>> > >>>>>> I guess he is tackling on syncing device trees between Linux and
>> > >>>>>> U-boot,
>> > >>>>>> and this is right thing to do.
>> > >>>>>>
>> > >>>>>> Inserting the U-boot specific property here and there
>> > >>>>>> makes it difficult.
>> > >>>>>
>> > >>>>> Yes it is attractive.
>> > >>>>>
>> > >>>>> With this scheme we cannot put the properties into .dtsi (i.e.
>> > >>>>> make
>> > >>>>> them common for the soc). Is there a way around that or would we
>> > >>>>> just
>> > >>>>> have to live with it?
>> > >>>>
>> > >>>> Why not? You can add chosen node to dtsi without any problem which
>> > >>>> should be shared for all boards. The only one question remains
>> > >>>> which is
>> > >>>> what exactly you need to add to dtsi.
>> > >>>> One example is Zynq. We have 2 serial PS IPs. Which one you want to
>> > >>>> add?
>> > >>>> Both?
>> > >>>
>> > >>> If you have something like this:
>> > >>>
>> > >>> soc {
>> > >>>    uart0 {
>> > >>>    };
>> > >>>    uart1 {
>> > >>>    };
>> > >>> }
>> > >>>
>> > >>> in the common .dtsi then you can currently put the marker in the soc
>> > >>> node. I'm not sure how you do that with your scheme. If we put it in
>> > >>> the .dtsi then the .dts will override it.
>> > >>>
>> > >>> Does this matter?
>> > >>
>> > >> As far as I understand DTSI is shared across all boards.
>> > >> And DTS can overwrite DTSI at any time.
>> > >>
>> > >> Let me extend this to make it more clear
>> > >> soc: soc {
>> > >>     uart0: uart at XXX {
>> > >>     };
>> > >>     uart1: uart at XXX {
>> > >>     };
>> > >>  }
>> > >>
>> > >>
>> > >> In DTSI I would put probably this to show everybody what needs to be
>> > >> there.
>> > >> chosen {
>> > >>         u-boot,dm-pre-reloc = &soc &uart0 &uart1;
>> > >> }
>> > >>
>> > >> And in DTS if you have only one uart DTS will overwrite it.
>> > >> chosen {
>> > >>         u-boot,dm-pre-reloc = &soc &uart0;
>> > >> }
>> > >>
>> > >> Or the next option is to make code smarter and detect that uart1 is
>> > >> disabled that's why it is not used and only chosen from DTSI should
>> > >> be
>> > >> enough.
>> > >
>> > > Yes I see - you just overwrite the property in the main file. If it
>> > > includes pinctrl nodes, gpio nodes, etc. then you would have to add
>> > > those also.
>> >
>> > Right - DTSI is most like a pattern to follow and DTS is doing that
>> > platform specific stuff. It means in DTSI you can have guidance what to
>> > use.
>>
>>
>> Do you mean we should add a comment to the .dtsi to tell you want you
>> need in your .dts? Is this improving anything?
>
>
> what I think it will be the best to add that line to DTSI with all IPs which
> should be pre relocated.
> Then algorithm should be like this
>
> for_each_IP_in_property {
> if (status=ok)
>   relocate
> else
> ignore it
> }
>
> If you have others nodes in DTS then you have to rewrite it.

Normally the .dts file will specify the UART which means that it will
often need rewriting with this scheme.

The potential for stuff-ups and confusion is there. With the current
scheme at least marking can be kept as a property of the device (where
arguably it belongs) rather than having to be recreated for each
board.

Perhaps we could allow multiple properties in /chosen and scan them
all (u-boot,dm-pre-reloc-soc, u-boot,dm-pre-reloc-pinctrl, etc.)?

>
>
>>
>>
>> >
>> > Also in algorithm can be checking if that IP has status okay or disabled
>> > and do relocation or not.
>>
>>
>> Can you please rephrase that? I don't understand.
>> >
>> >
>> > > For mainline Rockchip this means that each board would
>> > > have to have something like:
>> > >
>> > >  chosen {
>> > >          u-boot,dm-pre-reloc = &mmc0, &serial0, &dmc, &power, &syscon0
>> > > &syscon1 &pinctrl &gpio3 &gpio8 &leds &led_work &led_power;
>> > >  }
>> > >
>> > > That seems a bit unwieldy to me. What do you think?
>> >
>> > (note: without commas)
>> > I think that this solution is compatible with the binding and it is
>> > better than adding bootloader specific property to nodes which I have
>> > never seen before. Chosen node was used for that maybe even from
>> > beginning.
>>
>>
>> There are Linux-specific properties, so I really don't see the
>> difference. Don't forget that we are using device tree to control our
>> boot loader, so it would be unusual to not see at least some
>> U-Boot-specific properties.
>>
>> My understanding of /chosen is that it is for the OS. But in any case
>> it would still be a binding change, wouldn't it? What makes you think
>> that this scheme would be more acceptable as a binding change?
>
>
> I think we should move this discussion to device-tree mailing list.
> I simple just don't think that OS/bootloader property in the device node is
> a good idea.
> Maybe there is better solution but i think adding OS/Bootloader to chosen is
> just better option
> then added this to every node.
> Not sure if sequence matter or not but via one property you can also control
> it.

OK, by all means. But please cc the U-Boot mailing list.

I'm not tied to the current solution at all. It does work and
something like it is necessary. It allows SOC people to set things up
so that it is relatively easy for people to write now .dts files.

But I'd like to change it once someone has figured out the
implications of the change:

- Impact on maintainability of .dts/.dtsi files
- Mechanism for implementation of device tree subset feature
(currently fdtgrep / SPL)
- Code changes required in U-Boot
- Maintaining the efficiency (the algorithm above would require many
scans of the DT)
- A patch :-)

I feel in general that there is some conflict between hard-nosed
efficiency and perceived beauty in device tree bindings. This doesn't
seem to matter much in Linux, but in U-Boot it is more important. I'd
like to err on the side of keeping things simple and fast, bringing in
new functionality as needed (perhaps behind an option) rather than
requiring a lump of 10KB of parsing code just to get get a serial
driver running.

>
>
>
>>
>> >
>> >
>> > Regarding technical aspects - I have never checked if there is any line
>> > length limitation on parameter side which can be problematic. On the
>> > other hand if yes phandles should be used.
>>
>>
>> No there is not limit I know of. It should be fine.
>> >
>> >
>> > Regarding compactness of this solution. From one line you can see what
>> > will be pre relocated which looks pretty compact to me and you can easy
>> > check if something is missing.
>>
>>
>> Yes it is nice to have this in once place. But it's not really in one
>> place as you need to reference nodes from elsewhere, and update all
>> .dts files if something in the .dtsi changes. Granted that shouldn't
>> happen often.
>>
>> One advantage is that this copes with boards that have a common .dtsi
>> but different drivers needed before relocation - e.g. one board might
>> want to use a serial port attached to a /soc node, another might want
>> to use something on /pci. But I have not seen this happen yet.
>>
>> So overall i can see benefits and drawbacks. I'm on the fence.
>
>
> TBH there should be any ACK from DT guys before this u-boot property was
> used.
> From that discussion should come how this can be used.
> In past I have some experience with using incorrect binding in private
> drivers and it ends up
> in disaster that's why I want to make sure before we start to use this that
> it was properly review.

Well it does follow device tree guidelines - it has a' u-boot,' prefix
just like all the properties which have a 'linux,' prefix. So I think
it is sane from that point of view. It's just that it's a pain to add
it to several nodes, and not obvious that another approach (a list of
nodes somewhere) is better.

Yes I'd really like the 'DT guys' to get involved in this sort of
thing, including contributing patches to U-Boot in this area. It takes
a lot of energy to go through these discussions and having something
on the inside track who can figure these things out and help build out
the infrastructure would be a big help.

As Tom suggests we could try to figure this out at ELCE? I can go to
the BoF session if there is time to discuss it. So far the topics seem
to be quite 'high-end' :-) But we could talk about it at the U-Boot
one.

Regards,
Simon