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 <s...@chromium.org>: > Hi Michal, > > On Friday, 4 September 2015, Michal Simek <mon...@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@XXX { > > >> }; > > >> uart1: uart@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
