Hi Rasmus, On Tue, 18 Apr 2023 at 23:30, Rasmus Villemoes <rasmus.villem...@prevas.dk> wrote: > > On 19/04/2023 03.49, Simon Glass wrote: > > Hi Rasmus, > > > > Returning to this old thread... > > > >> > >> There's no problematic device tree, having two nodes with the same > >> (base)name is perfectly fine and in fact sometimes expected/required (in > >> the cases where a node name is standardized; e.g. having two identical > >> eeproms on different i2c buses both would/should be called eeprom@50). > >> > >> What Simon is concerned about is that the translation from full path to > >> blob offset is now done unconditionally, and that might be costly. I'm > >> not sure it is (and didn't know that it could be), but as I've said > >> repeatedly, I prefer code that works out-of-the-box, and for the boards > >> that do need this extra check (because just looking at the basename is > >> not enough), the fact that the previous code worked seemed to be pure > >> luck - because those dtbs were compiled with -@ due to some other > >> CONFIG_ knob being set. And again, involving phandles at all is what > >> make the code fragile, so a revert that reinstates a CONFIG_ knob with > >> PHANDLE in the name is not a good way forward, assuming there is even > >> anything to fix. > >> > >> So if the performance thing is real, sure, we can introduce a > >> CONFIG_something, but only if at the same time we have a sanity check at > >> build-time that detects if one should enable/disable that option > >> (depending on whether we make it "positive" or "negative"). Something > >> like this seems to do the trick, but I haven't looked at hooking it up > >> in the build system: > >> > >> === scripts/check-alias-homonyms.py === > >> #!/usr/bin/python3 > >> > >> import sys > >> > >> sys.path.insert(0, 'scripts/dtc/pylibfdt') > >> sys.path.insert(1, 'tools') > >> > >> from dtoc import fdt > >> > >> ret = 0 > >> > >> for name in sys.argv[1:]: > >> dtb = fdt.FdtScan(name) > >> aliasnode = dtb.GetNode("/aliases") > >> if not aliasnode: > >> continue > >> basenames = dict() > >> for prop in aliasnode.props.values(): > >> alias = prop.name > >> path = prop.value > >> base = path[path.rfind('/')+1:] > >> if base in basenames: > >> basenames[base].append(alias) > >> else: > >> basenames[base] = [alias] > >> for base, aliases in basenames.items(): > >> if len(aliases) == 1: > >> continue > >> print("Warning: In %s, the aliases %s all point at nodes with > >> the basename '%s' - consider (en/dis)abling CONFIG_..." % (name, > >> ",".join(aliases), base)) > >> ret = 1 > >> > >> sys.exit(ret) > >> === > >> > >> I imagine this being run over CONFIG_OF_LIST when CONFIG_... has the > >> value that disables the fdt_path_offset() call. For concreteness, > >> something like > >> > >> config ASSUME_ALIAS_HOMONYMS # or whatever better name one can come up with > >> bool "Assume there may be nodes pointed to by aliases in DT that have > >> identical basenames" > >> help > >> In most cases, the nodes pointed to by aliases in the device tree > >> all have different basenames. When this is satisfied, the > >> fdtdec_get_alias_seq() function can avoid a somewhat expensive full walk > >> of the dtb when looking for an alias for a given node. If the device > >> tree for your board does have aliases pointing at nodes with the same > >> basenames (but of course different full paths), that full walk is > >> necessary for correctness, and you can then enable this option. > >> > >> plus > >> > >> - if (offset != fdt_path_offset(blob, prop)) > >> + if (IS_ENABLED(CONFIG_ASSUME_ALIAS_HOMONYMS) && offset != > >> fdt_path_offset(blob, prop)) > >> > >> I have no idea what running the above python script on the dtb(s) in the > >> common case of !CONFIG_ASSUME_ALIAS_HOMONYMS adds to buildtime, but I do > >> believe that before eliding some code that is necessary for correctness > >> in the general case, we need buildtime verification that it's ok. > > > > The runtime performance impact is real. We actually have quite a > > problem in general, e.g. the pinctrl drivers can take ages to operate > > before relocation because they read the DT multiple times. > > OK. For other reasons, I'm looking at the upstream dtc code, and have > some ideas for performance improvements to the "raw blob walking" code. > But I don't have any idea how large an impact it could have, if it would > help that pinctrl case, or when I'll actually find some spare cycles to > work on it.
OK. Part of the issue with pinctrl might be that drivers should be caching the DT in their internal structures, rather than scanning it each time pinctrl_select_state() is called. > > > Your script seems like a great idea to me. Perhaps it should produce > > an error if the CONFIG is not enabled? > > I was imagining that it would only run when the config is not enabled > (because when it is enabled, correctness is guaranteed at runtime), and > AFAICT it already does produce an error when it finds homonyms. But it > really was just something I wrote in five minutes, I have no idea how to > hook it up in the build system or how to propagate that error. Anybody > is welcome to take that code and polish it up and make a real patch out > of it. Probably it just needs to go in Makefile: u-boot.dtb: dts/dt.dtb python <your_script.py> $< $(call cmd,copy) > > I also think I suggested doing something else at build time, something > along the lines of 'for each alias, inject a property in the target node > with name "u-boot,dm-alias" and value the name of the alias'. Then the > run-time code could simply start by fetching the node's u-boot,dm-alias > property, and if present, check if its value starts with the required > stem. That would likely be even faster than what we do today, but the > current code would probably need to be kept as a fallback. I've thought for a long time that it would be great to have a table of phandles (array of node offsets in a property in a suitable node) and a second 'aliases' node (with a different name) that uses node offsets instead of strings. Haven't done it though... > > Of course that somewhat assumes that no node has two aliases, but the > semantics of that is already ill-defined, and I don't think it's a real > issue (and the code doing the injection could easily detect it and warn > or bail). OK Regards, Simon