| Message ID | 20201202154156.9502-1-a-govindraju@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [v2] fdt: Use phandle to distinguish DT nodes with same name | expand |
Hi Aswath, On Wed, 2 Dec 2020 at 08:42, Aswath Govindraju <a-govindraju@ti.com> wrote: > > While assigning the sequence number to subsystem instances by reading the > aliases property, only DT nodes names are compared and not the complete > path. This causes a problem when there are two DT nodes with same name but > have different paths. > > In arch/arm/dts/k3-am65-main.dtsi there are two USB controllers with the > same device tree node name but different path. When aliases are defined for > these USB controllers then fdtdec_get_alias_seq() fails to pick the correct > instance for a given index. > > fdt_path_offset() function is slow and this would effect the uboot startup. U-Boot > To avert the time penalty on all boards, apply this extra check only when > required by using a config option. > > Fix it by comparing the phandles of DT nodes after the node names match, > under a config option. > > Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> > --- > > Changes since v1: > - Added a config option as fdt_path_offset() slows down the u-boot start > up and would be better to be enabled only when required > - Added an example case in commit message where the following fix is > required. > > lib/Kconfig | 8 ++++++++ > lib/fdtdec.c | 11 +++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/lib/Kconfig b/lib/Kconfig > index 8efb154f7344..9813a3a5e0ba 100644 > --- a/lib/Kconfig > +++ b/lib/Kconfig > @@ -685,3 +685,11 @@ config LIB_ELF > This supports fir 32 bit and 64 bit versions. > > endmenu > + > +config PHANDLE_CHECK_SEQ > + bool "Enable phandle check while getting sequence number" > + default n > + 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. > diff --git a/lib/fdtdec.c b/lib/fdtdec.c > index ee1bd41b0818..60aa1bd8cf1b 100644 > --- a/lib/fdtdec.c > +++ b/lib/fdtdec.c > @@ -500,6 +500,17 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset, > slash = strrchr(prop, '/'); > if (strcmp(slash + 1, find_name)) > continue; > + > + /* > + * Adding an extra check to distinguish DT nodes with > + * same name > + */ > + #ifdef CONFIG_PHANDLE_CHECK_SEQ if (IS_ENABLED(CONFIG... (did you not get a warning from patman on that?) > + if (fdt_get_phandle(blob, offset) != > + fdt_get_phandle(blob, fdt_path_offset(blob, prop))) > + continue; > + #endif > + > val = trailing_strtol(name); > if (val != -1) { > *seqp = val; > -- > 2.17.1 > Regards, SImon
Hi Simon, On 03/12/20 3:51 am, Simon Glass wrote: > Hi Aswath, > > On Wed, 2 Dec 2020 at 08:42, Aswath Govindraju <a-govindraju@ti.com> wrote: >> >> While assigning the sequence number to subsystem instances by reading the >> aliases property, only DT nodes names are compared and not the complete >> path. This causes a problem when there are two DT nodes with same name but >> have different paths. >> >> In arch/arm/dts/k3-am65-main.dtsi there are two USB controllers with the >> same device tree node name but different path. When aliases are defined for >> these USB controllers then fdtdec_get_alias_seq() fails to pick the correct >> instance for a given index. >> >> fdt_path_offset() function is slow and this would effect the uboot startup. > > U-Boot > Corrected this. >> To avert the time penalty on all boards, apply this extra check only when >> required by using a config option. >> >> Fix it by comparing the phandles of DT nodes after the node names match, >> under a config option. >> >> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> >> --- >> >> Changes since v1: >> - Added a config option as fdt_path_offset() slows down the u-boot start >> up and would be better to be enabled only when required >> - Added an example case in commit message where the following fix is >> required. >> >> lib/Kconfig | 8 ++++++++ >> lib/fdtdec.c | 11 +++++++++++ >> 2 files changed, 19 insertions(+) >> >> diff --git a/lib/Kconfig b/lib/Kconfig >> index 8efb154f7344..9813a3a5e0ba 100644 >> --- a/lib/Kconfig >> +++ b/lib/Kconfig >> @@ -685,3 +685,11 @@ config LIB_ELF >> This supports fir 32 bit and 64 bit versions. >> >> endmenu >> + >> +config PHANDLE_CHECK_SEQ >> + bool "Enable phandle check while getting sequence number" >> + default n >> + 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. >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c >> index ee1bd41b0818..60aa1bd8cf1b 100644 >> --- a/lib/fdtdec.c >> +++ b/lib/fdtdec.c >> @@ -500,6 +500,17 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset, >> slash = strrchr(prop, '/'); >> if (strcmp(slash + 1, find_name)) >> continue; >> + >> + /* >> + * Adding an extra check to distinguish DT nodes with >> + * same name >> + */ >> + #ifdef CONFIG_PHANDLE_CHECK_SEQ > > if (IS_ENABLED(CONFIG... > Corrected this. > (did you not get a warning from patman on that?) No, I am not getting it. Thank you for the comments. I made the changes that you suggested and I posted a respin. Thanks, Aswath > >> + if (fdt_get_phandle(blob, offset) != >> + fdt_get_phandle(blob, fdt_path_offset(blob, prop))) >> + continue; >> + #endif >> + >> val = trailing_strtol(name); >> if (val != -1) { >> *seqp = val; >> -- >> 2.17.1 >> > > Regards, > SImon >
diff --git a/lib/Kconfig b/lib/Kconfig index 8efb154f7344..9813a3a5e0ba 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -685,3 +685,11 @@ config LIB_ELF This supports fir 32 bit and 64 bit versions. endmenu + +config PHANDLE_CHECK_SEQ + bool "Enable phandle check while getting sequence number" + default n + 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. diff --git a/lib/fdtdec.c b/lib/fdtdec.c index ee1bd41b0818..60aa1bd8cf1b 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -500,6 +500,17 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset, slash = strrchr(prop, '/'); if (strcmp(slash + 1, find_name)) continue; + + /* + * Adding an extra check to distinguish DT nodes with + * same name + */ + #ifdef CONFIG_PHANDLE_CHECK_SEQ + if (fdt_get_phandle(blob, offset) != + fdt_get_phandle(blob, fdt_path_offset(blob, prop))) + continue; + #endif + val = trailing_strtol(name); if (val != -1) { *seqp = val;
While assigning the sequence number to subsystem instances by reading the aliases property, only DT nodes names are compared and not the complete path. This causes a problem when there are two DT nodes with same name but have different paths. In arch/arm/dts/k3-am65-main.dtsi there are two USB controllers with the same device tree node name but different path. When aliases are defined for these USB controllers then fdtdec_get_alias_seq() fails to pick the correct instance for a given index. fdt_path_offset() function is slow and this would effect the uboot startup. To avert the time penalty on all boards, apply this extra check only when required by using a config option. Fix it by comparing the phandles of DT nodes after the node names match, under a config option. Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> --- Changes since v1: - Added a config option as fdt_path_offset() slows down the u-boot start up and would be better to be enabled only when required - Added an example case in commit message where the following fix is required. lib/Kconfig | 8 ++++++++ lib/fdtdec.c | 11 +++++++++++ 2 files changed, 19 insertions(+)