Hi Rasmus, On 19/05/22 14:40, Rasmus Villemoes wrote: > Asking if the alias we found actually points at the device tree node > we passed in (in the guise of its offset from blob) can be done simply > by asking if the fdt_path_offset() of the alias' path is identical to > offset. > > In fact, the current method suffers from the possibility of false > negatives: dtc does not necessarily emit a phandle property for a node > just because it is referenced in /aliases; it only emits a phandle > property for a node if it is referenced in <angle brackets> > somewhere. So if both the node we passed in and the alias node we're > considering don't have phandles, fdt_get_phandle() returns 0 for both. > > Since the proper check is so simple, there's no reason to hide that > behind a config option (and if one really wanted that, it should be > called something else because there's no need to involve phandle in > the check). > > Signed-off-by: Rasmus Villemoes <rasmus.villem...@prevas.dk> > --- > 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, 2 insertions(+), 16 deletions(-) > > diff --git a/configs/am65x_evm_a53_defconfig b/configs/am65x_evm_a53_defconfig > index 9f41b397c3..a635d6d137 100644 > --- a/configs/am65x_evm_a53_defconfig > +++ b/configs/am65x_evm_a53_defconfig > @@ -170,4 +170,3 @@ CONFIG_USB_GADGET_VENDOR_NUM=0x0451 > CONFIG_USB_GADGET_PRODUCT_NUM=0x6162 > CONFIG_USB_GADGET_DOWNLOAD=y > CONFIG_OF_LIBFDT_OVERLAY=y > -CONFIG_PHANDLE_CHECK_SEQ=y > diff --git a/configs/evb-ast2600_defconfig b/configs/evb-ast2600_defconfig > index ea75762926..015df20f09 100644 > --- a/configs/evb-ast2600_defconfig > +++ b/configs/evb-ast2600_defconfig > @@ -84,4 +84,3 @@ CONFIG_WDT=y > CONFIG_SHA384=y > CONFIG_HEXDUMP=y > # CONFIG_EFI_LOADER is not set > -CONFIG_PHANDLE_CHECK_SEQ=y > diff --git a/configs/sama7g5ek_mmc1_defconfig > b/configs/sama7g5ek_mmc1_defconfig > index 42d96f7c02..3f33eb1142 100644 > --- a/configs/sama7g5ek_mmc1_defconfig > +++ b/configs/sama7g5ek_mmc1_defconfig > @@ -76,4 +76,3 @@ CONFIG_TIMER=y > CONFIG_MCHP_PIT64B_TIMER=y > CONFIG_OF_LIBFDT_OVERLAY=y > # CONFIG_EFI_LOADER_HII is not set > -CONFIG_PHANDLE_CHECK_SEQ=y > diff --git a/configs/sama7g5ek_mmc_defconfig b/configs/sama7g5ek_mmc_defconfig > index e03a6ba9af..6266eb0b59 100644 > --- a/configs/sama7g5ek_mmc_defconfig > +++ b/configs/sama7g5ek_mmc_defconfig > @@ -76,4 +76,3 @@ CONFIG_TIMER=y > CONFIG_MCHP_PIT64B_TIMER=y > CONFIG_OF_LIBFDT_OVERLAY=y > # CONFIG_EFI_LOADER_HII is not set > -CONFIG_PHANDLE_CHECK_SEQ=y > diff --git a/lib/Kconfig b/lib/Kconfig > index acc0ac081a..884569f9b1 100644 > --- a/lib/Kconfig > +++ b/lib/Kconfig > @@ -958,11 +958,4 @@ config LMB_RESERVED_REGIONS > Define the number of supported reserved regions in the library logical > memory blocks. > > -config PHANDLE_CHECK_SEQ > - bool "Enable phandle check while getting sequence number" > - help > - When there are multiple device tree nodes with same name, > - enable this config option to distinguish them using > - phandles in fdtdec_get_alias_seq() function. > - > endmenu > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > index e20f6aad9c..ffa78f97ca 100644 > --- a/lib/fdtdec.c > +++ b/lib/fdtdec.c > @@ -516,11 +516,8 @@ int fdtdec_get_alias_seq(const void *blob, const char > *base, int offset, > * Adding an extra check to distinguish DT nodes with > * same name > */ > - if (IS_ENABLED(CONFIG_PHANDLE_CHECK_SEQ)) { > - if (fdt_get_phandle(blob, offset) != > - fdt_get_phandle(blob, fdt_path_offset(blob, prop))) > - continue; > - } > + if (offset != fdt_path_offset(blob, prop)) > + continue;
The offset over here is the offset of the dt node and the offset that we get from fdt_path_offset(blob, prop) is the offset of the aliases property. I don't think these both offsets will match for any case. That is the reason behind comparing phandles and not offsets. Also, as fdt_path_offset() slow and this would effect the U-Boot startup. To avert the time penalty on all boards, this extra check put under a config option. Thanks, Aswath > > val = trailing_strtol(name); > if (val != -1)