Hi Rasmus, On Tue, 5 Jul 2022 at 07:47, Rasmus Villemoes <rasmus.villem...@prevas.dk> wrote: > > On 05/07/2022 11.47, Simon Glass wrote: > > Hi Tom, > > > > On Sun, 3 Jul 2022 at 06:43, Tom Rini <tr...@konsulko.com> wrote: > >> > >> On Sun, Jul 03, 2022 at 02:32:42AM -0600, Simon Glass wrote: > >>> Hi, > >>> > >>> On Sun, 3 Jul 2022 at 02:25, Simon Glass <s...@chromium.org> wrote: > >>>> > >>>> The fdt_path_offset() function is slow since it must scan the tree. > >>>> This substantial overhead now applies to all boards. > >>>> > >>>> The original code may not be ideal but it is fit for purpose and is only > >>>> needed on a few boards. > >>>> > >>>> We should revert this in time for the release. > >>>> > >>>> This reverts commit 26f981f295d00351b6f0c69b5317b254b2361cc0. > >>>> > >>>> Signed-off-by: Simon Glass <s...@chromium.org> > >>>> --- > >>>> > >>>> configs/am65x_evm_a53_defconfig | 1 + > >>>> configs/evb-ast2600_defconfig | 1 + > >>>> configs/sama7g5ek_mmc1_defconfig | 1 + > >>>> configs/sama7g5ek_mmc_defconfig | 1 + > >>>> lib/Kconfig | 7 +++++++ > >>>> lib/fdtdec.c | 7 +++++-- > >>>> 6 files changed, 16 insertions(+), 2 deletions(-) > >>> > >>> Please also see the context here: > >>> > >>> https://patchwork.ozlabs.org/project/uboot/patch/20201111142605.17034-1-a-govindr...@ti.com/ > >> > >> Previously when we've had issues of making the fast path slow, people > >> have posted measurements of before/after in order to demonstrate the > >> problem. Can we please get some logs of before/after and various > >> possible solutions? Thanks. > > > > Well this code is not needed at all for all but four boards. > > Three. One board seems to enable that config for no reason at all. And > it wouldn't work on that board, because the code was fragile and error > prone. See my detailed explanations in the original patch thread. > > It does a > > very expensive check of the DT and this can happen before relocation, > > or is SPL. I don't have a board to test with at present, but I expect > > it would cost 10s of milliseconds on AT91, for example. > > As I've said before, I prefer code which is correct out-of-the-box. I > also prefer simpler code with less ifdefs (yeah yeah, IS_ENABLED...) and > fewer configuration options to worry about. The three boards which have > aliases where the leaf nodes have duplicate names also happen to enable > some other unrelated magic config knob which happens to make the phandle > comparison work. So at the very least the code should stop comparing > phandles and just compare the node offsets; whether that check should be
I don't disagree that this is a bit odd, but it is efficient. Another approach here would be to add better documentation since the option doesn't quite work as advertised. > under a config knob can be discussed (certainly that config knob should > not have PHANDLE in it), but, again, as I've said, it should be opt-out, > and preferably with a build-time check that verifies that no two aliases > point at same basenames. Opt-out means that everything pays the penalty, though. This is real corner case. Arguably the device tree should be updated to avoid this problem. > > _If_ that 10s of milliseconds figure is true, there are other things one > could do at build time. Say have some pass over the dtb which simply > adds "u-boot,seq" properties to the target nodes of aliases, using that > if present, or add a "back pointer" "u-boot,alias" property one could > compare to name. That's a nice idea. The point of my revert was to get something in for the release. I fully expect some sort of change to go in afterwards, but I don't think people were aware of the impact of your patch (see context link above). So I still favour a revert, for now. Regards, Simon
