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

[Michal Simek]

Hi Simon,

first of all sorry for late reply. I am traveling and I have pretty busy
time now.

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.



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




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

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform