diff mbox series

[U-Boot] net: eth-uclass: Remove warning about ROM MAC address

Message ID 20191010230048.10935-1-smoch@web.de
State Superseded
Delegated to: Tom Rini
Headers show
Series [U-Boot] net: eth-uclass: Remove warning about ROM MAC address | expand

Commit Message

Sören Moch Oct. 10, 2019, 11 p.m. UTC 
Using a MAC address from ROM storage is the normal case for most
ethernet hardware. Why should we warn about this?

Signed-off-by: Soeren Moch <smoch@web.de>
---
Cc: Joe Hershberger <joe.hershberger@ni.com>
Cc: u-boot@lists.denx.de
---
 net/eth-uclass.c | 2 --
 1 file changed, 2 deletions(-)

--
2.17.1

Comments

Joe Hershberger Oct. 11, 2019, 3:06 a.m. UTC | #1 
Hi Soeren,

On Thu, Oct 10, 2019 at 6:01 PM Soeren Moch <smoch@web.de> wrote:
>
> Using a MAC address from ROM storage is the normal case for most
> ethernet hardware. Why should we warn about this?

Most hardware that U-Boot runs on is an SoC and the boards rarely have
a ROM associated with the Ethernet MAC. Usually the storage for the
ethaddr is the U-Boot environment itself. That's the reason it warns.

Cheers,
-Joe

> Signed-off-by: Soeren Moch <smoch@web.de>
> ---
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: u-boot@lists.denx.de
> ---
>  net/eth-uclass.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 3bd98b01ad..8b29de37bb 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -538,8 +538,6 @@ static int eth_post_probe(struct udevice *dev)
>                 memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>         } else if (is_valid_ethaddr(pdata->enetaddr)) {
>                 eth_env_set_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
> -               printf("\nWarning: %s using MAC address from ROM\n",
> -                      dev->name);
>         } else if (is_zero_ethaddr(pdata->enetaddr) ||
>                    !is_valid_ethaddr(pdata->enetaddr)) {
>  #ifdef CONFIG_NET_RANDOM_ETHADDR
> --
> 2.17.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Sören Moch Oct. 11, 2019, 8:07 a.m. UTC | #2 
Hi Joe,

On 11.10.19 05:06, Joe Hershberger wrote:
> Hi Soeren,
>
> On Thu, Oct 10, 2019 at 6:01 PM Soeren Moch <smoch@web.de> wrote:
>> Using a MAC address from ROM storage is the normal case for most
>> ethernet hardware. Why should we warn about this?
> Most hardware that U-Boot runs on is an SoC and the boards rarely have
> a ROM associated with the Ethernet MAC. Usually the storage for the
> ethaddr is the U-Boot environment itself. That's the reason it warns.
Hm, I just converted the tbs2910 board to DM_ETH and now see this
misleading warning. This board is based on a i.MX6Q SoC (ARMv7), the
ethernet MAC is stored in fuses on this SoC.
There is absolutely nothing wrong in using the ROM MAC address. For
systems without ROM storage we use the environment to set the MAC, also
fine. Also OK if both addresses are set identically.
We only should warn the user if addresses are set inconsistently or not
set at all.

Regards,
Soeren
>
> Cheers,
> -Joe
>
>> Signed-off-by: Soeren Moch <smoch@web.de>
>> ---
>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>> Cc: u-boot@lists.denx.de
>> ---
>>  net/eth-uclass.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
>> index 3bd98b01ad..8b29de37bb 100644
>> --- a/net/eth-uclass.c
>> +++ b/net/eth-uclass.c
>> @@ -538,8 +538,6 @@ static int eth_post_probe(struct udevice *dev)
>>                 memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>>         } else if (is_valid_ethaddr(pdata->enetaddr)) {
>>                 eth_env_set_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
>> -               printf("\nWarning: %s using MAC address from ROM\n",
>> -                      dev->name);
>>         } else if (is_zero_ethaddr(pdata->enetaddr) ||
>>                    !is_valid_ethaddr(pdata->enetaddr)) {
>>  #ifdef CONFIG_NET_RANDOM_ETHADDR
>> --
>> 2.17.1
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
Fabio Estevam Dec. 10, 2019, 9:42 a.m. UTC | #3 
Hi Soeren,

On Thu, Oct 10, 2019 at 8:01 PM Soeren Moch <smoch@web.de> wrote:
>
> Using a MAC address from ROM storage is the normal case for most
> ethernet hardware. Why should we warn about this?
>
> Signed-off-by: Soeren Moch <smoch@web.de>
> ---
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: u-boot@lists.denx.de
> ---
>  net/eth-uclass.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 3bd98b01ad..8b29de37bb 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -538,8 +538,6 @@ static int eth_post_probe(struct udevice *dev)
>                 memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>         } else if (is_valid_ethaddr(pdata->enetaddr)) {
>                 eth_env_set_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
> -               printf("\nWarning: %s using MAC address from ROM\n",
> -                      dev->name);

I also find this warning misleading:

Reviewed-by: Fabio Estevam <festevam@gmail.com>
Fabio Estevam Dec. 10, 2019, 9:47 a.m. UTC | #4 
Hi Joe,

On Fri, Oct 11, 2019 at 12:06 AM Joe Hershberger <joe.hershberger@ni.com> wrote:
>
> Hi Soeren,
>
> On Thu, Oct 10, 2019 at 6:01 PM Soeren Moch <smoch@web.de> wrote:
> >
> > Using a MAC address from ROM storage is the normal case for most
> > ethernet hardware. Why should we warn about this?
>
> Most hardware that U-Boot runs on is an SoC and the boards rarely have
> a ROM associated with the Ethernet MAC. Usually the storage for the
> ethaddr is the U-Boot environment itself. That's the reason it warns.

On i.MX SoCs the MAC usually is stored in fuses via On-Chip OTP
Controller (OCOTP).

Should we at least change the message to debug() level instead?

As Soeren says there is nothing wrong with the fact that the MAC
addresses are retrieved from fuses and there is nothing the user can
do about this warning message.

IMHO it is causing pure noise.

Thanks
Frieder Schrempf Dec. 11, 2019, 11:54 a.m. UTC | #5 
On 11.10.19 01:00, Soeren Moch wrote:
> Using a MAC address from ROM storage is the normal case for most
> ethernet hardware. Why should we warn about this?
> 
> Signed-off-by: Soeren Moch <smoch@web.de>

Some months ago I came up with the very same patch and I currently have 
it in my local fork with the description "Reading the MAC address from 
ROM is often a standard use case and should not produce a warning".

Therefore I'm supporting Soeren's and Fabio's point of view here and I'm 
in favor of merging this patch or if preferred, change the printf() to 
debug().

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

> ---
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: u-boot@lists.denx.de
> ---
>   net/eth-uclass.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 3bd98b01ad..8b29de37bb 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -538,8 +538,6 @@ static int eth_post_probe(struct udevice *dev)
>   		memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
>   	} else if (is_valid_ethaddr(pdata->enetaddr)) {
>   		eth_env_set_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
> -		printf("\nWarning: %s using MAC address from ROM\n",
> -		       dev->name);
>   	} else if (is_zero_ethaddr(pdata->enetaddr) ||
>   		   !is_valid_ethaddr(pdata->enetaddr)) {
>   #ifdef CONFIG_NET_RANDOM_ETHADDR
> --
> 2.17.1
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>
Fabio Estevam Jan. 6, 2020, 11:32 p.m. UTC | #6 
Hi Joe,

On Wed, Dec 11, 2019 at 8:54 AM Schrempf Frieder
<frieder.schrempf@kontron.de> wrote:
>
> On 11.10.19 01:00, Soeren Moch wrote:
> > Using a MAC address from ROM storage is the normal case for most
> > ethernet hardware. Why should we warn about this?
> >
> > Signed-off-by: Soeren Moch <smoch@web.de>
>
> Some months ago I came up with the very same patch and I currently have
> it in my local fork with the description "Reading the MAC address from
> ROM is often a standard use case and should not produce a warning".
>
> Therefore I'm supporting Soeren's and Fabio's point of view here and I'm
> in favor of merging this patch or if preferred, change the printf() to
> debug().
>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Any feedback, please?
Fabio Estevam Jan. 28, 2020, 4:33 p.m. UTC | #7 
Hi Joe,

Ping

On Mon, Jan 6, 2020 at 8:32 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Joe,
>
> On Wed, Dec 11, 2019 at 8:54 AM Schrempf Frieder
> <frieder.schrempf@kontron.de> wrote:
> >
> > On 11.10.19 01:00, Soeren Moch wrote:
> > > Using a MAC address from ROM storage is the normal case for most
> > > ethernet hardware. Why should we warn about this?
> > >
> > > Signed-off-by: Soeren Moch <smoch@web.de>
> >
> > Some months ago I came up with the very same patch and I currently have
> > it in my local fork with the description "Reading the MAC address from
> > ROM is often a standard use case and should not produce a warning".
> >
> > Therefore I'm supporting Soeren's and Fabio's point of view here and I'm
> > in favor of merging this patch or if preferred, change the printf() to
> > debug().
> >
> > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> Any feedback, please?
Joe Hershberger Feb. 9, 2020, 5:26 p.m. UTC | #8 
On Tue, Jan 28, 2020 at 10:33 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Joe,
>
> Ping
>
> On Mon, Jan 6, 2020 at 8:32 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Joe,
> >
> > On Wed, Dec 11, 2019 at 8:54 AM Schrempf Frieder
> > <frieder.schrempf@kontron.de> wrote:
> > >
> > > On 11.10.19 01:00, Soeren Moch wrote:
> > > > Using a MAC address from ROM storage is the normal case for most
> > > > ethernet hardware. Why should we warn about this?
> > > >
> > > > Signed-off-by: Soeren Moch <smoch@web.de>
> > >
> > > Some months ago I came up with the very same patch and I currently have
> > > it in my local fork with the description "Reading the MAC address from
> > > ROM is often a standard use case and should not produce a warning".
> > >
> > > Therefore I'm supporting Soeren's and Fabio's point of view here and I'm
> > > in favor of merging this patch or if preferred, change the printf() to
> > > debug().
> > >
> > > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> >
> > Any feedback, please?

Traditionally the env was the source of truth for the MAC, so using
the ROM was a fall-back if the env didn't have one. But times change,
I guess. I'll pull this into my next PR.

Acked-by: Joe Hershberger <joe.hershberger@ni.com>
diff mbox series

Patch

diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index 3bd98b01ad..8b29de37bb 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -538,8 +538,6 @@  static int eth_post_probe(struct udevice *dev)
 		memcpy(pdata->enetaddr, env_enetaddr, ARP_HLEN);
 	} else if (is_valid_ethaddr(pdata->enetaddr)) {
 		eth_env_set_enetaddr_by_index("eth", dev->seq, pdata->enetaddr);
-		printf("\nWarning: %s using MAC address from ROM\n",
-		       dev->name);
 	} else if (is_zero_ethaddr(pdata->enetaddr) ||
 		   !is_valid_ethaddr(pdata->enetaddr)) {
 #ifdef CONFIG_NET_RANDOM_ETHADDR