Hi Michal,
On 18 September 2015 at 19:07, Michal Simek monstr@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@chromium.org:
Hi Michal,
On Friday, 4 September 2015, Michal Simek monstr@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.
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