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

[Simon Glass]

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?

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

Regards,
Simon