[RESEND PATCH] fdt: Use phandle to distinguish DT nodes with same name

[Simon Glass]

Hi Aswath,

On Fri, 27 Nov 2020 at 07:05, Aswath Govindraju <a-govindraju at ti.com> wrote:
>
> On 22/11/20 4:37 am, Simon Glass wrote:
> > Hi,
> >
> > On Wed, 18 Nov 2020 at 10:55, Vignesh Raghavendra <vigneshr at 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-govindraju at 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-govindraju at 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 at 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 at 4000000 {
> >>                 compatible = "ti,am654-dwc3";
> >>                 reg = <0x0 0x4000000 0x0 0x4000>;
> >>                 #address-cells = <1>;
> >>                 #size-cells = <1>;
> >>                 ranges = <0x0 0x0 0x4000000 0x20000>;
> >>                 ...
> >>
> >>                 usb0: usb at 10000 {
> >>                         compatible = "snps,dwc3";
> >>                         reg = <0x10000 0x10000>;
> >>                         ...
> >>                 };
> >>         };
> >>
> >>         dwc3_1: dwc3 at 4020000 {
> >>                 compatible = "ti,am654-dwc3";
> >>                 reg = <0x0 0x4020000 0x0 0x4000>;
> >>                 #address-cells = <1>;
> >>                 #size-cells = <1>;
> >>                 ranges = <0x0 0x0 0x4020000 0x20000>;
> >>                 ...
> >>
> >>                 usb1: usb at 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 at 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