Hi Aswath, On Fri, 27 Nov 2020 at 07:05, Aswath Govindraju <a-govindr...@ti.com> wrote: > > On 22/11/20 4:37 am, Simon Glass wrote: > > 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. > > > > I apologize for the delay in response.
No hurry! > > > > 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? > > > Currently live tree has not been enabled in the configurations of the > AM65 board and there are some issues that I am facing after enabling it. OK. I think it would be a good idea to figure that out. It should be a case of making sure all drivers are converted. > > > If not, can we put this code behind a Kconfig so the extra time > > penalty is only incurred on boards that need it? > > > > I think putting it under Kconfig is a good idea and I think I will do that. OK. > > Also, I have alternate method to implement the comparison that does not > use fdt_path_offset(), compares by getting the path name. I have > attached it to this mail. I think it is slightly better in terms of time > penalty. Can you please look at it and suggest if it is better to implement. Unfortunately fdt_parent_offset() has the same problem since it needs to scan the tree again to find the parent. Regards, Simon