[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Zach Sadecki]

This patch will also fix a problem with mx28 boards that are depending 
on the OCOTP bits to set the MAC address (like the Denx m28 board).  Now 
it simply fails with a "Warning: failed to set MAC address" if there's 
no environment variable instead of using the OCOTP bits like it should.  
This patch will fix that.

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Wolfgang Denk]

Dear Zach Sadecki,

In message <4F149CCB.6070307@itwatchdogs.com> you wrote:
> This patch will also fix a problem with mx28 boards that are depending 
> on the OCOTP bits to set the MAC address (like the Denx m28 board).  Now 
> it simply fails with a "Warning: failed to set MAC address" if there's 
> no environment variable instead of using the OCOTP bits like it should.  
> This patch will fix that.

But there _should_ always be a warning message if the environment
variable is missing.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Heavier than air flying machines are impossible.
                    -- Lord Kelvin, President, Royal Society, c. 1895

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Dirk Behme]

Dear Wolfgang and all,

On 17.01.2012 10:52, Wolfgang Denk wrote:
> Dear Zach Sadecki,
> 
> In message <4F149CCB.6070307@itwatchdogs.com> you wrote:
>> This patch will also fix a problem with mx28 boards that are depending 
>> on the OCOTP bits to set the MAC address (like the Denx m28 board).  Now 
>> it simply fails with a "Warning: failed to set MAC address" if there's 
>> no environment variable instead of using the OCOTP bits like it should.  
>> This patch will fix that.
> 
> But there _should_ always be a warning message if the environment
> variable is missing.

Let's see if I can summarize the issue:

If I understood correctly, we are talking about

http://git.denx.de/?p=u-boot.git;a=commitdiff;h=7616e7850804c7c69e0a22c179dfcba9e8f3f587

and there

int eth_initialize(bd_t *bis)
{
...
-  eth_getenv_enetaddr_by_index(eth_number, env_enetaddr);

versus

+int eth_write_hwaddr(...)
+{
...
+ if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr))
+   return -1;

I.e. the unconditional call of eth_getenv_enetaddr_by_index() was
replaced by a return -1 if the environment variable is missing
(resulting in "Warning: failed to set MAC address"? Btw: That's wrong?
It should be "Warning: failed to get MAC address from the environment"?)


Now, Eric's patch [1] (which needs an update of the commit message, 
sure, but was acked by the author of the original patch [2]) tries to 
re-introduce the original behavior:

+ eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);

So, what I understand for i.MXxx, Zach and Eric are talking about:

If the environment variable is missing, the system can use the OCOTP
bits to set the MAC address. If the return -1 is removed by using Eric's
patch.

And for overo:

 From Philip's mail, on an overo based product that uses the SMSC911x
ethernet chip, if the environment variable is missing, the MAC address
is set via EEPROM connected to the SMSC911x chip.


I.e. on these systems, with Eric's patch, we get a correct MAC address,
even if the environment variable is missing. This sounds like a good 
option (?).

The question is then: Do we want a "Warning: failed to set MAC address"
message, even if the MAC is set correctly? But not from the environment
variable, but somehow read from the HW?

I know you will correct what I missed in this summary ;)

Best regards

Dirk

[1] http://lists.denx.de/pipermail/u-boot/2011-August/098686.html

[2] http://lists.denx.de/pipermail/u-boot/2011-August/098690.html

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Eric Miao]

Ignore the return value of eth_getenv_enetaddr_by_index(), and if it
fails, fall back to use dev->enetaddr, which could be filled up by
the ethernet device driver.

Actually, this is the original behavior, and was later changed by
commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

Signed-off-by: Eric Miao <eric.miao@linaro.org>
---
 net/eth.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index a34fe59..c4fbe11 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -195,8 +195,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
 	unsigned char env_enetaddr[6];
 	int ret = 0;
 
-	if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr))
-		return -1;
+	eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
 
 	if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
 		if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&
-- 
1.7.4.1

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Simon Glass]

On Tue, Aug 16, 2011 at 10:33 PM, Eric Miao <eric.miao@linaro.org> wrote:
> Ignore the return value of eth_getenv_enetaddr_by_index(), and if it
> fails, fall back to use dev->enetaddr, which could be filled up by
> the ethernet device driver.
>
> Actually, this is the original behavior, and was later changed by
> commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.
>
> Signed-off-by: Eric Miao <eric.miao@linaro.org>

Hi Eric,

I found this in u-boot-usb although the commit hash I saw was 7616e7850.

> ---
> ?net/eth.c | ? ?3 +--
> ?1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/net/eth.c b/net/eth.c
> index a34fe59..c4fbe11 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -195,8 +195,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
> ? ? ? ?unsigned char env_enetaddr[6];
> ? ? ? ?int ret = 0;
>
> - ? ? ? if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr))
> - ? ? ? ? ? ? ? return -1;
> + ? ? ? eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
>
> ? ? ? ?if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
> ? ? ? ? ? ? ? ?if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&

Acked-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

> --
> 1.7.4.1
>
>

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Dirk Behme]

On 17.08.2011 08:22, Simon Glass wrote:
> On Tue, Aug 16, 2011 at 10:33 PM, Eric Miao <eric.miao@linaro.org> wrote:
>> Ignore the return value of eth_getenv_enetaddr_by_index(), and if it
>> fails, fall back to use dev->enetaddr, which could be filled up by
>> the ethernet device driver.
>>
>> Actually, this is the original behavior, and was later changed by
>> commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.
>>
>> Signed-off-by: Eric Miao <eric.miao@linaro.org>
> 
> Hi Eric,
> 
> I found this in u-boot-usb although the commit hash I saw was 7616e7850.
> 
>> ---
>>  net/eth.c |    3 +--
>>  1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/eth.c b/net/eth.c
>> index a34fe59..c4fbe11 100644
>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -195,8 +195,7 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
>>        unsigned char env_enetaddr[6];
>>        int ret = 0;
>>
>> -       if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr))
>> -               return -1;
>> +       eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
>>
>>        if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
>>                if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&
> 
> Acked-by: Simon Glass <sjg@chromium.org>

Ping.

Anybody likes to apply this? Or any new comments?

Many thanks and best regards

Dirk

P.S.: I add Wolfgang as he seems to have done the last commits to net/eth.c.

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Simon Glass]

On Wed, Jan 11, 2012 at 4:23 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> On 17.08.2011 08:22, Simon Glass wrote:
>>
>> On Tue, Aug 16, 2011 at 10:33 PM, Eric Miao <eric.miao@linaro.org> wrote:
>>>
>>> Ignore the return value of eth_getenv_enetaddr_by_index(), and if it
>>> fails, fall back to use dev->enetaddr, which could be filled up by
>>> the ethernet device driver.
>>>
>>> Actually, this is the original behavior, and was later changed by
>>> commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.
>>>
>>> Signed-off-by: Eric Miao <eric.miao@linaro.org>
>>
>>
>> Hi Eric,
>>
>> I found this in u-boot-usb although the commit hash I saw was 7616e7850.
>>
>>> ---
>>> ?net/eth.c | ? ?3 +--
>>> ?1 files changed, 1 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/eth.c b/net/eth.c
>>> index a34fe59..c4fbe11 100644
>>> --- a/net/eth.c
>>> +++ b/net/eth.c
>>> @@ -195,8 +195,7 @@ int eth_write_hwaddr(struct eth_device *dev, const
>>> char *base_name,
>>> ? ? ? unsigned char env_enetaddr[6];
>>> ? ? ? int ret = 0;
>>>
>>> - ? ? ? if (!eth_getenv_enetaddr_by_index(base_name, eth_number,
>>> env_enetaddr))
>>> - ? ? ? ? ? ? ? return -1;
>>> + ? ? ? eth_getenv_enetaddr_by_index(base_name, eth_number,
>>> env_enetaddr);
>>>
>>> ? ? ? if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
>>> ? ? ? ? ? ? ? if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&
>>
>>
>> Acked-by: Simon Glass <sjg@chromium.org>
>
>
> Ping.
>
> Anybody likes to apply this? Or any new comments?
>
> Many thanks and best regards
>
> Dirk
>
> P.S.: I add Wolfgang as he seems to have done the last commits to net/eth.c.

Maybe Stefan might pick it up through the staging tree?

Regards,
Simon

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Stefan Roese]

On Thursday 12 January 2012 06:12:19 Simon Glass wrote:
> On Wed, Jan 11, 2012 at 4:23 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> > On 17.08.2011 08:22, Simon Glass wrote:
> >> On Tue, Aug 16, 2011 at 10:33 PM, Eric Miao <eric.miao@linaro.org> wrote:
> >>> Ignore the return value of eth_getenv_enetaddr_by_index(), and if it
> >>> fails, fall back to use dev->enetaddr, which could be filled up by
> >>> the ethernet device driver.
> >>> 
> >>> Actually, this is the original behavior, and was later changed by
> >>> commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.
> >>> 
> >>> Signed-off-by: Eric Miao <eric.miao@linaro.org>
> >> 
> >> Hi Eric,
> >> 
> >> I found this in u-boot-usb although the commit hash I saw was 7616e7850.
> >> 
> >>> ---
> >>>  net/eth.c |    3 +--
> >>>  1 files changed, 1 insertions(+), 2 deletions(-)
> >>> 
> >>> diff --git a/net/eth.c b/net/eth.c
> >>> index a34fe59..c4fbe11 100644
> >>> --- a/net/eth.c
> >>> +++ b/net/eth.c
> >>> @@ -195,8 +195,7 @@ int eth_write_hwaddr(struct eth_device *dev, const
> >>> char *base_name,
> >>>       unsigned char env_enetaddr[6];
> >>>       int ret = 0;
> >>> 
> >>> -       if (!eth_getenv_enetaddr_by_index(base_name, eth_number,
> >>> env_enetaddr))
> >>> -               return -1;
> >>> +       eth_getenv_enetaddr_by_index(base_name, eth_number,
> >>> env_enetaddr);
> >>> 
> >>>       if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
> >>>               if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&
> >> 
> >> Acked-by: Simon Glass <sjg@chromium.org>
> > 
> > Ping.
> > 
> > Anybody likes to apply this? Or any new comments?
> > 
> > Many thanks and best regards
> > 
> > Dirk
> > 
> > P.S.: I add Wolfgang as he seems to have done the last commits to
> > net/eth.c.
> 
> Maybe Stefan might pick it up through the staging tree?

Okay, let's take a look...

Best regards,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Stefan Roese]

On Wednesday 17 August 2011 07:33:44 Eric Miao wrote:
> Ignore the return value of eth_getenv_enetaddr_by_index(), and if it
> fails, fall back to use dev->enetaddr, which could be filled up by
> the ethernet device driver.
> 
> Actually, this is the original behavior, and was later changed by
> commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

Applied to u-boot-staging/sr at denx.de. Thanks.
 
Best regards,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Wolfgang Denk]

Dear Eric Miao,

In message <1313559224-4627-1-git-send-email-eric.miao@linaro.org> you wrote:
> Ignore the return value of eth_getenv_enetaddr_by_index(), and if it
> fails, fall back to use dev->enetaddr, which could be filled up by
> the ethernet device driver.
> 
> Actually, this is the original behavior, and was later changed by
> commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

I'm not going to pull that patch.

Please explain which actual problem you're trying to fix.

Also, fix the commit ID - the one you list above does not exist in
mainline.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A direct quote from the Boss: "We passed over a lot of good people to
get the ones we hired."

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Eric Miao]

On Sat, Jan 14, 2012 at 3:19 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Eric Miao,
>
> In message <1313559224-4627-1-git-send-email-eric.miao@linaro.org> you wrote:
>> Ignore the return value of eth_getenv_enetaddr_by_index(), and if it
>> fails, fall back to use dev->enetaddr, which could be filled up by
>> the ethernet device driver.
>>
>> Actually, this is the original behavior, and was later changed by
>> commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.
>
> I'm not going to pull that patch.
>
> Please explain which actual problem you're trying to fix.

Hi Wolfgang,

The problem was with the current code, if there is no eth<n>addr
in the environment variables, eth_write_hwaddr() will fail immediately,
and it's going to use the default reset MAC address in the FEC controller
which is likely all 0.

To make it worse, the current kernel driver actually depends on
the MAC address registers being programmed correctly.

If traced back to the last commit to this file, which added this
eth_getenv_enetaddr_by_index(), that commit actually modified
the original behavior of this eth_write_hwaddr() function.

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Eric Miao]

On Mon, Jan 16, 2012 at 3:03 PM, Eric Miao <eric.miao@linaro.org> wrote:
> On Sat, Jan 14, 2012 at 3:19 AM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Eric Miao,
>>
>> In message <1313559224-4627-1-git-send-email-eric.miao@linaro.org> you wrote:
>>> Ignore the return value of eth_getenv_enetaddr_by_index(), and if it
>>> fails, fall back to use dev->enetaddr, which could be filled up by
>>> the ethernet device driver.
>>>
>>> Actually, this is the original behavior, and was later changed by
>>> commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.

Checked with git.denx.de/u-boot.git, the original commit hash is

7616e7850804c7c69e0a22c179dfcba9e8f3f587

Please note the behavior modification to eth_write_hwaddr() when
it's excerpted from the original eth_initialize().

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Philip Balister]

On 01/13/2012 02:19 PM, Wolfgang Denk wrote:
> Dear Eric Miao,
> 
> In message <1313559224-4627-1-git-send-email-eric.miao@linaro.org> you wrote:
>> Ignore the return value of eth_getenv_enetaddr_by_index(), and if it
>> fails, fall back to use dev->enetaddr, which could be filled up by
>> the ethernet device driver.
>>
>> Actually, this is the original behavior, and was later changed by
>> commit 48506a2cde2458fa1f8c5993afc98e5a4617e1d3.
> 
> I'm not going to pull that patch.
> 
> Please explain which actual problem you're trying to fix.
> 

For u-boot for an overo based product that uses the SMSC911x ethernet
chip (with MAC address set via EEPROM connected to the SMSC911x chip) I
have a local patch to prevent u-boot from printing:

Warning: failed to set MAC address

The MAC address is still OK and we basically removed the code that led
to the message to reduce customer confusion :)

Hopefully, the patch people are talking about here is a proper fix to
this problem.

Philip


> Also, fix the commit ID - the one you list above does not exist in
> mainline.
> 
> Best regards,
> 
> Wolfgang Denk
> 

[U-Boot] [PATCH] net/eth.c: fix eth_write_hwaddr() to use dev->enetaddr as fall back

[Wolfgang Denk]

Dear Philip Balister,

In message <4F142F52.508@balister.org> you wrote:
>
> > I'm not going to pull that patch.
> > 
> > Please explain which actual problem you're trying to fix.
> 
> For u-boot for an overo based product that uses the SMSC911x ethernet
> chip (with MAC address set via EEPROM connected to the SMSC911x chip) I
> have a local patch to prevent u-boot from printing:
> 
> Warning: failed to set MAC address

Something went wrong here.  Either you should not run the operation
that failed, or you should find out why it failed and fixe the cause
of that.

> The MAC address is still OK and we basically removed the code that led
> to the message to reduce customer confusion :)

Papering over errors or warnings is rarely a good thing.  We should
understand what happened and fix the original cause of the problem
instead.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I wrote my name at the top of the page. I wrote down  the  number  of
the  question  ``1''.  After much reflection I put a bracket round it
thus ``(1)''. But thereafter I could not think of anything  connected
with it that was either relevant or true.
                        - Sir Winston Churchill _My Early Life_ ch. 2