[PATCH] fdtdec: fdtdec_get_aliases_highest_id: skip aliases to disabled nodes

[Simon Glass]

Hi Tim,

On Fri, 11 Jun 2021 at 11:30, Tim Harvey <tharvey at gateworks.com> wrote:
>
> On Fri, Jun 11, 2021 at 10:23 AM Sean Anderson <sean.anderson at seco.com> wrote:
> >
> >
> >
> > On 6/11/21 1:16 PM, Tim Harvey wrote:
> >  > On Fri, Jun 11, 2021 at 10:09 AM Sean Anderson <sean.anderson at seco.com> wrote:
> >  >>
> >  >>
> >  >>
> >  >> On 6/11/21 12:32 PM, Tim Harvey wrote:
> >  >>   > On Thu, Apr 29, 2021 at 9:48 AM Tim Harvey <tharvey at gateworks.com> wrote:
> >  >>   >>
> >  >>   >> On Thu, Apr 29, 2021 at 9:10 AM Simon Glass <sjg at chromium.org> wrote:
> >  >>   >>>
> >  >>   >>> Hi Tim,
> >  >>   >>>
> >  >>   >>> On Fri, 16 Apr 2021 at 14:30, Tim Harvey <tharvey at gateworks.com> wrote:
> >  >>   >>>>
> >  >>   >>>> When looking for an alias with the highest id skip aliases for nodes
> >  >>   >>>> that are disabled.
> >  >>   >>>>
> >  >>   >>>> Signed-off-by: Tim Harvey <tharvey at gateworks.com>
> >  >>   >>>> ---
> >  >>   >>>>   lib/fdtdec.c | 2 ++
> >  >>   >>>>   1 file changed, 2 insertions(+)
> >  >>   >>>>
> >  >>   >>>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> >  >>   >>>> index 864589193b..d47195525a 100644
> >  >>   >>>> --- a/lib/fdtdec.c
> >  >>   >>>> +++ b/lib/fdtdec.c
> >  >>   >>>> @@ -546,6 +546,8 @@ int fdtdec_get_alias_highest_id(const void *blob, const char *base)
> >  >>   >>>>                  if (*prop != '/' || prop[len - 1] ||
> >  >>   >>>>                      strncmp(name, base, base_len))
> >  >>   >>>>                          continue;
> >  >>   >>>> +               if (!fdtdec_get_is_enabled(blob, fdt_path_offset(blob, prop)))
> >  >>   >>>> +                       continue;
> >  >>   >>>
> >  >>   >>> We really can't do this here. It is quite an expensive operation to
> >  >>   >>> locate the node for a path.
> >  >>   >>>
> >  >>   >>> Why is this needed? It seems odd to have an alias pointing to a disabled device.
> >  >>   >>>
> >  >>   >>
> >  >>   >> Simon,
> >  >>   >>
> >  >>   >> The issue I ran into here was with an imx6 based board that does not
> >  >>   >> use the FEC ethernet on the SoC. In this case imx6qdl.dtsi delcares an
> >  >>   >> alias 'thernet0 = &fec' yet the fec node is not enabled. However
> >  >>   >> because fdtdec_get_alias_highest_id does not skip this alias to a
> >  >>   >> disabled device any enumerated ethernet devices get an index of 1
> >  >>   >> instead of 0 which is incorrect and causes the mac addresses to be
> >  >>   >> misaligned.
> >  >>
> >  >> Is this due getting mac address from the OTP? As I understand it, the
> >  >> OTP mac addresses are for fec0/fec1, and not for whatever ethernet0
> >  >> happens to be.
> >  >>
> >  >
> >  > Sean,
> >  >
> >  > No, the issue is that U-Boot thinks there is an fec device simply
> >  > because an alias points to it even though the fec node pointed to is
> >  > disabled so when U-Boot finds an ethernet device that is enumerated at
> >  > runtime (think pcie or usb based mac) those start at index 1 instead
> >  > of index 0 and end up wanting to be assigned mac's from eth1addr
> >  > instead of ethaddr.
> >
> > Ok, and this behavior was different in the past? E.g. it used to be that
> > these interfaces were assigned to ethernet0, and so their mac address
> > was stored in ethaddr. And then something changed and now they are
> > ethernet1 and are trying to get the incorrect mac address?
> >
>
> Sean,
>
> Yes, in the past before using driver-model this did not occur as
> everything was statically declared in board files.
>
> > Do you know whether it would be possible to remove the alias at the same
> > time you mark the fec as disabled?
> >
>
> The dt's are all merged together. The base SoC dt declares the alias
> to internal fec ethernet device and that alias can be overridden by
> any dts that includes that base soc dt but if you have a gbe that is
> discovered and not declared in dt there will be no alias to override
> it. When I submitted this patch I looked at what the kernel code did
> and noticed it did not reserve indexes/slots for disabled devices so I
> figure that's what the bootloader should do also.

Can you try enabled OF_LIVE? It is possible that it might be different
there. Even if not, it should be a cheap fix.

As mentioned the problem with your patch is that the path operation is
very expensive. I'm fine with it if you want to add it as a new CONFIG
option, so it doesn't affect all platforms. But if you do that, please
do update test-fdt.c for the sandbox tests.

Regards,
Simon