On Sat, Jul 30, 2022 at 07:27:26PM -0600, Simon Glass wrote: > Hi Tom, > > On Tue, 5 Jul 2022 at 10:42, Simon Glass <s...@chromium.org> wrote: > > > > 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. > > It looks like this didn't make it for the release. I'm not sure if > this is causing problems. > > Shall I pick it up for the upcoming release?
I don't think we should pick up the revert as I don't think there's agreement that reverting this is the right step forward among all of the interested parties. One thing I want to know at this point is, was the problematic device tree accepted upstream, or did they also say that 2 nodes with the same name but different paths is wrong, don't do that? -- Tom
signature.asc
Description: PGP signature