On 08/06/2022 16.18, Eugen.Hristev@microchip.com wrote:
On 5/19/22 3:16 PM, Rasmus Villemoes wrote:
On 19/05/2022 13.50, Aswath Govindraju wrote:
Understood, thanks for the explanation. I am good with this patch.
Acked-by: Aswath Govindraju a-govindraju@ti.com
Thanks.
For completeness, to expand on this:
it's somewhat fragile to rely on (at least one of) the nodes in question to even have a phandle.
One way in which to ensure all nodes (with a label) do get a phandle is to build with the -@ flag to dtc, which is implied by setting CONFIG_OF_LIBFDT_OVERLAY=y. Three of the four _defconfigs that set PHANDLE_CHECK_SEQ also set that option, so for them it was guaranteed to work, but mostly by chance - if I randomly discovered CONFIG_PHANDLE_CHECK_SEQ and found I needed to enable it for my board, I wouldn't know to also enable CONFIG_OF_LIBFDT_OVERLAY. And if I didn't need overlay support as such, it would also bloat the .dtb (and U-Boot itself) needlessly; the difference for sama7g5ek_mmc_defconfig is 34784 bytes for the current .dtb and 26920 bytes if one disables OF_LIBFDT_OVERLAY.
The last _defconfig with PHANDLE_CHECK_SEQ=y didn't actually seem to need it; I've built it and looked at u-boot.dtb, and there are no collisions in basenames in the aliases. Commit ddd778ae doesn't say anything about why it was added.
Rasmus
Hi Rasmus,
For sama7g5 boards, we have the following situation:
some node { i2c1: i2c@600: { some props; }; };
some other node { i2c2: i2c@600: { other props; }; };
and then we want to be able to reference aliases { i2c0 = &i2c1; i2c1 = &i2c2; };
Without CONFIG_PHANDLE_CHECK_SEQ , both i2c0 and i2c1 aliases pointed to the same 'i2c@600' node, because the code was searching through the DT for 'i2c@600' and it was hitting the first in both cases.
With your patch, it can happen that we hit the same problem again ? Or we should be safe ?
I removed the config option partly because one shouldn't need to know about some magic config option to get correct behaviour. The check which was previously hidden behind that config knob is now done unconditionally, and doesn't implicitly rely on the .dtb being built with -@, and at a lower cost (because two redundant phandle lookups are gone, and those phandle lookups was what was making the check non-robust anyway).
Rasmus
And no, the aliases were each pointing at the correct node (the i2c0 = &i2c1 is merely a shorthand for i2c0 = "/the/full/path/to/the/node"). What this check is about is when one comes with a pointer to some node and wants to know if there is some i2cX alias pointing at that node, and if so, what the value of X is - and without properly comparing the full paths but merely the name of the node passed in to the basename of the value of the alias, one would incorrectly assign the seq number 0 to both nodes.