Hi Tim, On Fri, 11 Jun 2021 at 11:30, Tim Harvey <thar...@gateworks.com> wrote: > > On Fri, Jun 11, 2021 at 10:23 AM Sean Anderson <sean.ander...@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.ander...@seco.com> > > wrote: > > >> > > >> > > >> > > >> On 6/11/21 12:32 PM, Tim Harvey wrote: > > >> > On Thu, Apr 29, 2021 at 9:48 AM Tim Harvey <thar...@gateworks.com> > > wrote: > > >> >> > > >> >> On Thu, Apr 29, 2021 at 9:10 AM Simon Glass <s...@chromium.org> > > wrote: > > >> >>> > > >> >>> Hi Tim, > > >> >>> > > >> >>> On Fri, 16 Apr 2021 at 14:30, Tim Harvey <thar...@gateworks.com> > > wrote: > > >> >>>> > > >> >>>> When looking for an alias with the highest id skip aliases for > > nodes > > >> >>>> that are disabled. > > >> >>>> > > >> >>>> Signed-off-by: Tim Harvey <thar...@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
