Hi, On Wed, 18 Nov 2020 at 10:55, Vignesh Raghavendra <vigne...@ti.com> wrote: > > > > On 11/18/20 8:44 PM, Aswath Govindraju wrote: > > Hi Simon, > > > > On 18/11/20 8:07 pm, Simon Glass wrote: > >> Hi Aswath, > >> > >> On Mon, 16 Nov 2020 at 07:29, Aswath Govindraju <a-govindr...@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. > >>> > >>> Fix it by comparing the phandles of DT nodes after the node names match. > >>> > >>> Signed-off-by: Aswath Govindraju <a-govindr...@ti.com> > >>> --- > >>> > >>> Resending this patch as it was held awaiting for moderator approval > >>> because > >>> patch was sent by non-member. > >>> > >>> lib/fdtdec.c | 5 +++++ > >>> 1 file changed, 5 insertions(+) > >>> > >>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c > >>> index 2015907dee7d..9e1bfe0b519e 100644 > >>> --- a/lib/fdtdec.c > >>> +++ b/lib/fdtdec.c > >>> @@ -478,6 +478,11 @@ int fdtdec_get_alias_seq(const void *blob, const > >>> char *base, int offset, > >>> slash = strrchr(prop, '/'); > >>> if (strcmp(slash + 1, find_name)) > >>> continue; > >>> + > >>> + if (fdt_get_phandle(blob, offset) != > >>> + fdt_get_phandle(blob, fdt_path_offset(blob, prop))) > >>> + continue; > >> > >> The call to fdt_path_offset() is very slow. Perhaps we can do this > >> check only with livetree? What situation is causing a problem for you? > >> What are the node / alias names? > > > > In the case of live tree for getting the sequence number the node > > pointers are compared. So, I don't think this problem would come up. > > > > As for the use case, > > > > In AM654 Device tree there are two instances of USB controllers and both > > the controller nodes have the same name usb@10000 > > > > If dfu is performed through the port connected to second controller. > > Then based on the dr_mode of first controller the instance number to be > > used in dfu command will vary. In order to make the instance number for > > dfu command to be independent, aliases can be used(If aliases are > > defined then the sequence number is assigned as the alias number.). > > > > The problem with current method for acquiring sequence number using > > aliases is that only the name of the node is compared with node name > > from the aliases property. So in the above case both the controllers > > will have the same name. This leads to the first alias number being used > > for the both the controllers to assign sequence number. > > > > > > aliases { > > serial2 = &main_uart0; > > ethernet0 = &cpsw_port1; > > usb0 = &usb0; // This property being used to > > //alias both the controllers > > usb1 = &usb1; > > }; > > > To explain a bit more, here is the DT snippet around usb0 and usb1 > > dwc3_0: dwc3@4000000 { > compatible = "ti,am654-dwc3"; > reg = <0x0 0x4000000 0x0 0x4000>; > #address-cells = <1>; > #size-cells = <1>; > ranges = <0x0 0x0 0x4000000 0x20000>; > ... > > usb0: usb@10000 { > compatible = "snps,dwc3"; > reg = <0x10000 0x10000>; > ... > }; > }; > > dwc3_1: dwc3@4020000 { > compatible = "ti,am654-dwc3"; > reg = <0x0 0x4020000 0x0 0x4000>; > #address-cells = <1>; > #size-cells = <1>; > ranges = <0x0 0x0 0x4020000 0x20000>; > ... > > usb1: usb@10000 { > compatible = "snps,dwc3"; > reg = <0x10000 0x10000>; > ... > }; > }; > > In above case, (with CONFIG_OF_LIVE disabled), > fdtdec_get_alias_seq() fails to pick the correct instance for USB > controller for a given index. This is because fdtdec_get_alias_seq() > only compares the leaf node name (usb@10000) with alias path and thus > both usb instances match to usb0. > > > > > So, to distinguish nodes with same name, phandles can be used while > > assigning sequence numbers.
Thank you both for the detai. I understand it and in fact I think this has come up before. Would it be OK to use livetree? If not, can we put this code behind a Kconfig so the extra time penalty is only incurred on boards that need it? Regards, Simon