All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] fdtdec: fdtdec_get_aliases_highest_id: skip aliases to disabled nodes
@ 2021-04-16 21:30 Tim Harvey
  2021-04-29 16:09 ` Simon Glass
  0 siblings, 1 reply; 9+ messages in thread
From: Tim Harvey @ 2021-04-16 21:30 UTC (permalink / raw)
  To: u-boot

When looking for an alias with the highest id skip aliases for nodes
that are disabled.

Signed-off-by: Tim Harvey <tharvey@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;
 
 		val = trailing_strtol(name);
 		if (val > max) {
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH] fdtdec: fdtdec_get_aliases_highest_id: skip aliases to disabled nodes
  2021-04-16 21:30 [PATCH] fdtdec: fdtdec_get_aliases_highest_id: skip aliases to disabled nodes Tim Harvey
@ 2021-04-29 16:09 ` Simon Glass
  2021-04-29 16:48   ` Tim Harvey
  0 siblings, 1 reply; 9+ messages in thread
From: Simon Glass @ 2021-04-29 16:09 UTC (permalink / raw)
  To: u-boot

Hi Tim,

On Fri, 16 Apr 2021 at 14:30, Tim Harvey <tharvey@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@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.

>
>                 val = trailing_strtol(name);
>                 if (val > max) {
> --
> 2.17.1
>

Regards,
Simon

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] fdtdec: fdtdec_get_aliases_highest_id: skip aliases to disabled nodes
  2021-04-29 16:09 ` Simon Glass
@ 2021-04-29 16:48   ` Tim Harvey
  2021-06-11 16:32     ` Tim Harvey
  0 siblings, 1 reply; 9+ messages in thread
From: Tim Harvey @ 2021-04-29 16:48 UTC (permalink / raw)
  To: u-boot

On Thu, Apr 29, 2021 at 9:10 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tim,
>
> On Fri, 16 Apr 2021 at 14:30, Tim Harvey <tharvey@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@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.

In general it is just wrong to reserve id's for disabled devices.

Tim

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fdtdec: fdtdec_get_aliases_highest_id: skip aliases to disabled nodes
  2021-04-29 16:48   ` Tim Harvey
@ 2021-06-11 16:32     ` Tim Harvey
  2021-06-11 17:09       ` Sean Anderson
  0 siblings, 1 reply; 9+ messages in thread
From: Tim Harvey @ 2021-06-11 16:32 UTC (permalink / raw)
  To: Simon Glass; +Cc: U-Boot Mailing List

On Thu, Apr 29, 2021 at 9:48 AM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Thu, Apr 29, 2021 at 9:10 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Tim,
> >
> > On Fri, 16 Apr 2021 at 14:30, Tim Harvey <tharvey@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@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.
>
> In general it is just wrong to reserve id's for disabled devices.
>

Simon,

Do you have any additional feedback on this? It has not been picked up
yet and does cause issues on IMX boards using dm that do not use the
internal SoC's fec ethernet.

Perhaps there is a better way to resolve this?

Best regards,

Tim

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fdtdec: fdtdec_get_aliases_highest_id: skip aliases to disabled nodes
  2021-06-11 16:32     ` Tim Harvey
@ 2021-06-11 17:09       ` Sean Anderson
  2021-06-11 17:16         ` Tim Harvey
  0 siblings, 1 reply; 9+ messages in thread
From: Sean Anderson @ 2021-06-11 17:09 UTC (permalink / raw)
  To: Tim Harvey, Simon Glass; +Cc: U-Boot Mailing List



On 6/11/21 12:32 PM, Tim Harvey wrote:
 > On Thu, Apr 29, 2021 at 9:48 AM Tim Harvey <tharvey@gateworks.com> wrote:
 >>
 >> On Thu, Apr 29, 2021 at 9:10 AM Simon Glass <sjg@chromium.org> wrote:
 >>>
 >>> Hi Tim,
 >>>
 >>> On Fri, 16 Apr 2021 at 14:30, Tim Harvey <tharvey@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@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

 >>
 >> In general it is just wrong to reserve id's for disabled devices.
 >>
 >
 > Simon,
 >
 > Do you have any additional feedback on this? It has not been picked up
 > yet and does cause issues on IMX boards using dm that do not use the
 > internal SoC's fec ethernet.
 >
 > Perhaps there is a better way to resolve this?
 >
 > Best regards,
 >
 > Tim
 >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fdtdec: fdtdec_get_aliases_highest_id: skip aliases to disabled nodes
  2021-06-11 17:09       ` Sean Anderson
@ 2021-06-11 17:16         ` Tim Harvey
  2021-06-11 17:23           ` Sean Anderson
  0 siblings, 1 reply; 9+ messages in thread
From: Tim Harvey @ 2021-06-11 17:16 UTC (permalink / raw)
  To: Sean Anderson; +Cc: Simon Glass, U-Boot Mailing List

On Fri, Jun 11, 2021 at 10:09 AM Sean Anderson <sean.anderson@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@gateworks.com> wrote:
>  >>
>  >> On Thu, Apr 29, 2021 at 9:10 AM Simon Glass <sjg@chromium.org> wrote:
>  >>>
>  >>> Hi Tim,
>  >>>
>  >>> On Fri, 16 Apr 2021 at 14:30, Tim Harvey <tharvey@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@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.

Tim

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fdtdec: fdtdec_get_aliases_highest_id: skip aliases to disabled nodes
  2021-06-11 17:16         ` Tim Harvey
@ 2021-06-11 17:23           ` Sean Anderson
  2021-06-11 17:30             ` Tim Harvey
  0 siblings, 1 reply; 9+ messages in thread
From: Sean Anderson @ 2021-06-11 17:23 UTC (permalink / raw)
  To: Tim Harvey; +Cc: Simon Glass, U-Boot Mailing List



On 6/11/21 1:16 PM, Tim Harvey wrote:
 > On Fri, Jun 11, 2021 at 10:09 AM Sean Anderson <sean.anderson@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@gateworks.com> wrote:
 >>   >>
 >>   >> On Thu, Apr 29, 2021 at 9:10 AM Simon Glass <sjg@chromium.org> wrote:
 >>   >>>
 >>   >>> Hi Tim,
 >>   >>>
 >>   >>> On Fri, 16 Apr 2021 at 14:30, Tim Harvey <tharvey@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@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?

Do you know whether it would be possible to remove the alias at the same
time you mark the fec as disabled?

--Sean

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fdtdec: fdtdec_get_aliases_highest_id: skip aliases to disabled nodes
  2021-06-11 17:23           ` Sean Anderson
@ 2021-06-11 17:30             ` Tim Harvey
  2021-07-04 19:24               ` Simon Glass
  0 siblings, 1 reply; 9+ messages in thread
From: Tim Harvey @ 2021-06-11 17:30 UTC (permalink / raw)
  To: Sean Anderson; +Cc: Simon Glass, U-Boot Mailing List

On Fri, Jun 11, 2021 at 10:23 AM Sean Anderson <sean.anderson@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@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@gateworks.com> wrote:
>  >>   >>
>  >>   >> On Thu, Apr 29, 2021 at 9:10 AM Simon Glass <sjg@chromium.org> wrote:
>  >>   >>>
>  >>   >>> Hi Tim,
>  >>   >>>
>  >>   >>> On Fri, 16 Apr 2021 at 14:30, Tim Harvey <tharvey@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@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.

Tim

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] fdtdec: fdtdec_get_aliases_highest_id: skip aliases to disabled nodes
  2021-06-11 17:30             ` Tim Harvey
@ 2021-07-04 19:24               ` Simon Glass
  0 siblings, 0 replies; 9+ messages in thread
From: Simon Glass @ 2021-07-04 19:24 UTC (permalink / raw)
  To: Tim Harvey; +Cc: Sean Anderson, U-Boot Mailing List

Hi Tim,

On Fri, 11 Jun 2021 at 11:30, Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Fri, Jun 11, 2021 at 10:23 AM Sean Anderson <sean.anderson@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@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@gateworks.com> wrote:
> >  >>   >>
> >  >>   >> On Thu, Apr 29, 2021 at 9:10 AM Simon Glass <sjg@chromium.org> wrote:
> >  >>   >>>
> >  >>   >>> Hi Tim,
> >  >>   >>>
> >  >>   >>> On Fri, 16 Apr 2021 at 14:30, Tim Harvey <tharvey@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@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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-07-04 19:24 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-16 21:30 [PATCH] fdtdec: fdtdec_get_aliases_highest_id: skip aliases to disabled nodes Tim Harvey
2021-04-29 16:09 ` Simon Glass
2021-04-29 16:48   ` Tim Harvey
2021-06-11 16:32     ` Tim Harvey
2021-06-11 17:09       ` Sean Anderson
2021-06-11 17:16         ` Tim Harvey
2021-06-11 17:23           ` Sean Anderson
2021-06-11 17:30             ` Tim Harvey
2021-07-04 19:24               ` Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.