[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Heiko Schocher]

doc/README.enetaddr prescribes, to use a mac address stored in
environment, before using a mac address stored in some special
place. This patch fixes this for the fec_mxc driver.

Tested on the magnesium board.

Signed-off-by: Heiko Schocher <hs@denx.de>
---
 drivers/net/fec_mxc.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
index 5af9cdb..b5245ec 100644
--- a/drivers/net/fec_mxc.c
+++ b/drivers/net/fec_mxc.c
@@ -749,11 +749,18 @@ static int fec_probe(bd_t *bd)

 	eth_register(edev);

-	if (fec_get_hwaddr(edev, ethaddr) == 0) {
-		printf("got MAC address from EEPROM: %pM\n", ethaddr);
-		memcpy(edev->enetaddr, ethaddr, 6);
-		fec_set_hwaddr(edev);
+	if (!eth_getenv_enetaddr("ethaddr", ethaddr)) {
+		/* "ethaddr" is not set in the environment */
+		if (fec_get_hwaddr(edev, ethaddr) == 0) {
+			printf("got MAC address from EEPROM: %pM\n", ethaddr);
+			eth_setenv_enetaddr("ethaddr", ethaddr);
+		} else {
+			printf ("no MAC found\n");
+			return -1;
+		}
 	}
+	memcpy(edev->enetaddr, ethaddr, 6);
+	fec_set_hwaddr(edev);

 	return 0;
 }
-- 
1.6.2.5

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Stefano Babic]

Heiko Schocher wrote:

Hi Heiko,

> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
>  drivers/net/fec_mxc.c |   15 +++++++++++----
>  1 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index 5af9cdb..b5245ec 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -749,11 +749,18 @@ static int fec_probe(bd_t *bd)
> 
>  	eth_register(edev);
> 
> -	if (fec_get_hwaddr(edev, ethaddr) == 0) {
> -		printf("got MAC address from EEPROM: %pM\n", ethaddr);
> -		memcpy(edev->enetaddr, ethaddr, 6);
> -		fec_set_hwaddr(edev);
> +	if (!eth_getenv_enetaddr("ethaddr", ethaddr)) {
> +		/* "ethaddr" is not set in the environment */
> +		if (fec_get_hwaddr(edev, ethaddr) == 0) {
> +			printf("got MAC address from EEPROM: %pM\n", ethaddr);
> +			eth_setenv_enetaddr("ethaddr", ethaddr);
> +		} else {
> +			printf ("no MAC found\n");
> +			return -1;
> +		}
>  	}
> +	memcpy(edev->enetaddr, ethaddr, 6);
> +	fec_set_hwaddr(edev);
> 
>  	return 0;
>  }

As I understood Ben's comment on my last patch, the driver must not
touch any environment variable, that is it must not call any
getenv/setenv function.

See http://www.mail-archive.com/u-boot at lists.denx.de/msg28329.html

Regards,
Stefano Babic

-- 
=====================================================================
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, fec_mxc: use mac address stored in env before looking in eeprom

[Detlev Zundel]

Hi Stefano,

> Heiko Schocher wrote:
>
> Hi Heiko,
>
>> 
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>>  drivers/net/fec_mxc.c |   15 +++++++++++----
>>  1 files changed, 11 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
>> index 5af9cdb..b5245ec 100644
>> --- a/drivers/net/fec_mxc.c
>> +++ b/drivers/net/fec_mxc.c
>> @@ -749,11 +749,18 @@ static int fec_probe(bd_t *bd)
>> 
>>  	eth_register(edev);
>> 
>> -	if (fec_get_hwaddr(edev, ethaddr) == 0) {
>> -		printf("got MAC address from EEPROM: %pM\n", ethaddr);
>> -		memcpy(edev->enetaddr, ethaddr, 6);
>> -		fec_set_hwaddr(edev);
>> +	if (!eth_getenv_enetaddr("ethaddr", ethaddr)) {
>> +		/* "ethaddr" is not set in the environment */
>> +		if (fec_get_hwaddr(edev, ethaddr) == 0) {
>> +			printf("got MAC address from EEPROM: %pM\n", ethaddr);
>> +			eth_setenv_enetaddr("ethaddr", ethaddr);
>> +		} else {
>> +			printf ("no MAC found\n");
>> +			return -1;
>> +		}
>>  	}
>> +	memcpy(edev->enetaddr, ethaddr, 6);
>> +	fec_set_hwaddr(edev);
>> 
>>  	return 0;
>>  }
>
> As I understood Ben's comment on my last patch, the driver must not
> touch any environment variable, that is it must not call any
> getenv/setenv function.
>
> See http://www.mail-archive.com/u-boot at lists.denx.de/msg28329.html

Hm, maybe this needs to be discussed again.

What Heiko _really_ fixes is that the board has a problem with linux
when no network transfer was done in U-Boot.  Looking at the code, this
is quite obvious as this special driver does not program any mac address
into the controller when no EEPROM mac is found.

No one could argue that Linux should initialize that, but given our
failed attempts to fix such things in the ARM linux tree, I looked for
another solution.  

Looking into our network code again I found doc/README.drivers.eth.  It
is explicitely allowed in this document to program a mac address into
the controller.  Now on the other hand if we have "ethaddr" override any
other settings, I fail to see how we should do this without using a
getenv call.

So maybe Ben _will_ allow this construct after all.

Cheers
  Detlev

-- 
Basically,  Barnes & Noble separates things by how old they are  -- current
stuff is "Fiction", stuff from 20 years ago is "Literature", stuff from 100
years ago is "Classics", stuff from 400 years ago is "Shakespeare" [..] and
stuff from 2000 years ago is "History".
                     -- James "Kibo" Parry in <kibo-1207032212000001@10.0.1.2>
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Heiko Schocher]

Hello stefano,

Stefano Babic wrote:
> Heiko Schocher wrote:
> 
> Hi Heiko,
> 
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>>  drivers/net/fec_mxc.c |   15 +++++++++++----
>>  1 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
>> index 5af9cdb..b5245ec 100644
>> --- a/drivers/net/fec_mxc.c
>> +++ b/drivers/net/fec_mxc.c
>> @@ -749,11 +749,18 @@ static int fec_probe(bd_t *bd)
>>
>>  	eth_register(edev);
>>
>> -	if (fec_get_hwaddr(edev, ethaddr) == 0) {
>> -		printf("got MAC address from EEPROM: %pM\n", ethaddr);
>> -		memcpy(edev->enetaddr, ethaddr, 6);
>> -		fec_set_hwaddr(edev);
>> +	if (!eth_getenv_enetaddr("ethaddr", ethaddr)) {
>> +		/* "ethaddr" is not set in the environment */
>> +		if (fec_get_hwaddr(edev, ethaddr) == 0) {
>> +			printf("got MAC address from EEPROM: %pM\n", ethaddr);
>> +			eth_setenv_enetaddr("ethaddr", ethaddr);
>> +		} else {
>> +			printf ("no MAC found\n");
>> +			return -1;
>> +		}
>>  	}
>> +	memcpy(edev->enetaddr, ethaddr, 6);
>> +	fec_set_hwaddr(edev);
>>
>>  	return 0;
>>  }
> 
> As I understood Ben's comment on my last patch, the driver must not
> touch any environment variable, that is it must not call any
> getenv/setenv function.
> 
> See http://www.mail-archive.com/u-boot at lists.denx.de/msg28329.html

Huh, didn;t see your patch ... sorry!

Hmm.. but if u-boot didn;t use this interface, linux never get
the mac address stored in ethaddr ... so, if using in u-boot
network, linux see/uses the mac address from environment, if
not using network in u-boot, linux uses the mac address stored
in eeprom ...

and if no valid mac address is in eeprom, linux has no valid
mac address, if u-boot don;t uses the interface ...

If I look in the code drivers/net/fec_mxc.c fec_get_hwaddr():

if defined CONFIG_MX51 this function _always_ returns -1, so
if not using this interface, on MX51 based plattforms, the
mac address is never initialized ...

How works this on such plattforms?

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Mike Frysinger]

On Wednesday 24 March 2010 07:56:28 Heiko Schocher wrote:
> doc/README.enetaddr prescribes, to use a mac address stored in
> environment, before using a mac address stored in some special
> place.

you're reading things incorrectly, or you didnt read the whole thing.  the 
fec_mxc driver is correct today.  its initialize function seeds eth_device-
>enetaddr with the hardware storage.  it is up to the common networking code 
to sync the environment to eth_device before calling the driver init function 
(which it does).  then the driver always uses eth_device->enetaddr.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100324/617e0e43/attachment.pgp 

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Mike Frysinger]

On Wednesday 24 March 2010 08:52:26 Detlev Zundel wrote:
> What Heiko _really_ fixes is that the board has a problem with linux
> when no network transfer was done in U-Boot.  Looking at the code, this
> is quite obvious as this special driver does not program any mac address
> into the controller when no EEPROM mac is found.

which is correct

> No one could argue that Linux should initialize that, but given our
> failed attempts to fix such things in the ARM linux tree, I looked for
> another solution.

read the FAQ and/or talk to Wolfgang

> Looking into our network code again I found doc/README.drivers.eth.  It
> is explicitely allowed in this document to program a mac address into
> the controller.

i dont think you read the document correctly.  it says program the driver MAC 
address in the init function using eth_device->enetaddr, not the environment, 
and not the initialize function.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100324/39dceb97/attachment.pgp 

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Heiko Schocher]

Hello Mike,

Mike Frysinger wrote:
> On Wednesday 24 March 2010 07:56:28 Heiko Schocher wrote:
>> doc/README.enetaddr prescribes, to use a mac address stored in
>> environment, before using a mac address stored in some special
>> place.
> 
> you're reading things incorrectly, or you didnt read the whole thing.  the 
> fec_mxc driver is correct today.  its initialize function seeds eth_device-
>> enetaddr with the hardware storage.  it is up to the common networking code 
> to sync the environment to eth_device before calling the driver init function 
> (which it does).  then the driver always uses eth_device->enetaddr.

Ok, but this driver initialize in fec_probe() (which gets called
through fecmxc_initialize() on uboot startup) always the fec mac
address registers with eeprom mac address. So, if uboot not uses
network interface, linux uses this mac address (and if no valid
mac address is in eeprom, linux has no valid mac address). But,
if uboot uses network, before booting linux, linux gets the
network address stored in ethaddr, because this driver sets
again in fec_init the mac address (at this point the content
from ethaddr) -> 2 different mac addresses used on this
plattform ... I think this could not be correct, and should be
fixed ...

So, if not changing something in the driver, how is this intended
to solve? (I know, it would be great to have a standard to pass
the mac address to linux on arm based plattforms, but as Detlev
wrote, such attempts failed ...)

I don;t want to make this through board specific code ...

While writing this, and realizing that this behaviour is a feature,
maybe this problem occur on other network drivers on arm plattforms
too ... hah, found one:

drivers/net/kirkwood_egiga.c

did it in the same way as drivers/net/fec_mxc.c ... Ok, it is
in line with uboot standard, but arm plattforms which booting
linux without doing network trafic under uboot tend to have
different mac addresses ...

This should be solved!

(Actual I don;t know, if arm linux prescribes something about
how to setup the mac address before it ...)

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Wolfgang Denk]

Dear Heiko,

In message <4BAB0357.60309@denx.de> you wrote:
> 
> Ok, but this driver initialize in fec_probe() (which gets called
> through fecmxc_initialize() on uboot startup) always the fec mac
> address registers with eeprom mac address. So, if uboot not uses

The question to me is why fec_probe() is called at all, even in cases
where U-Boot does not use any network related commands. This is IMO
wrong. Eventually somebody comes up with patches to fix this?

> network interface, linux uses this mac address (and if no valid
> mac address is in eeprom, linux has no valid mac address). But,
> if uboot uses network, before booting linux, linux gets the
> network address stored in ethaddr, because this driver sets
> again in fec_init the mac address (at this point the content
> from ethaddr) -> 2 different mac addresses used on this
> plattform ... I think this could not be correct, and should be
> fixed ...

Thisis wrong in any case, and indeed should fixed. Is the bigger, more
strategical fix above is not performed now (for whatever reasons),then
this issue should be fixed anyway.

> So, if not changing something in the driver, how is this intended
> to solve? (I know, it would be great to have a standard to pass
> the mac address to linux on arm based plattforms, but as Detlev
> wrote, such attempts failed ...)

Well, they failed temporarily. A long-term solution is to use the
device tree; a RFC for the "ARM Boot standard for passing device tree
blob" has just been posted yesterday, see
http://thread.gmane.org/gmane.linux.drivers.devicetree/1938

> drivers/net/kirkwood_egiga.c
> 
> did it in the same way as drivers/net/fec_mxc.c ... Ok, it is
> in line with uboot standard, but arm plattforms which booting
> linux without doing network trafic under uboot tend to have
> different mac addresses ...
> 
> This should be solved!

Please include in your fix.

> (Actual I don;t know, if arm linux prescribes something about
> how to setup the mac address before it ...)

The PTBs for the ARM Linux kernel recommend to use an initramfs file
system and use user space tools to set the MAC address. I don't know
if such an approach makes any sense to you :-(

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
Research is what I'm doing when I don't know what I'm doing.
                                                 -- Wernher von Braun

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Detlev Zundel]

Hi Wolfgang,

>> Ok, but this driver initialize in fec_probe() (which gets called
>> through fecmxc_initialize() on uboot startup) always the fec mac
>> address registers with eeprom mac address. So, if uboot not uses
>
> The question to me is why fec_probe() is called at all, even in cases
> where U-Boot does not use any network related commands. This is IMO
> wrong. Eventually somebody comes up with patches to fix this?

As I said before, this is documented in doc/README.drivers.eth.  If you
want to change this, then maybe one should first fix the documentation
to know what is actually wanted.  I certainly have lost track of what is
wanted and what is sensible.

Cheers
  Detlev

-- 
Programming X-Windows is like trying to find the square root of pi
using roman numerals.
                                          -- The UNIX Haters Handbook
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Wolfgang Denk]

Dear Detlev Zundel,

In message <m2tys4k0c8.fsf@ohwell.denx.de> you wrote:
> 
> > The question to me is why fec_probe() is called at all, even in cases
> > where U-Boot does not use any network related commands. This is IMO
> > wrong. Eventually somebody comes up with patches to fix this?
> 
> As I said before, this is documented in doc/README.drivers.eth.  If you
> want to change this, then maybe one should first fix the documentation

I think the documentation should be changed when the code gets
changed, not sooner nor later.

> to know what is actually wanted.  I certainly have lost track of what is
> wanted and what is sensible.

We do not want to touch interfaces that we don't use. If we don't use
Ethernet in U-Boot, there is no need to probe or initialize it.

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
"Love your country but never trust its government."
- from a hand-painted road sign in central Pennsylvania

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Mike Frysinger]

On Thursday 25 March 2010 05:40:01 Wolfgang Denk wrote:
> Heiko wrote:
> > Ok, but this driver initialize in fec_probe() (which gets called
> > through fecmxc_initialize() on uboot startup) always the fec mac
> > address registers with eeprom mac address. So, if uboot not uses
> 
> The question to me is why fec_probe() is called at all, even in cases
> where U-Boot does not use any network related commands. This is IMO
> wrong. Eventually somebody comes up with patches to fix this?

the current fec_mxc driver has squashed the initialize and probe steps.  it 
should have half the things removed from probe and put into initialize and 
then the call to probe moved to the init step.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100325/eafcb2b9/attachment.pgp 

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Mike Frysinger]

On Thursday 25 March 2010 09:16:55 Detlev Zundel wrote:
> >> Ok, but this driver initialize in fec_probe() (which gets called
> >> through fecmxc_initialize() on uboot startup) always the fec mac
> >> address registers with eeprom mac address. So, if uboot not uses
> > 
> > The question to me is why fec_probe() is called at all, even in cases
> > where U-Boot does not use any network related commands. This is IMO
> > wrong. Eventually somebody comes up with patches to fix this?
> 
> As I said before, this is documented in doc/README.drivers.eth.  If you
> want to change this, then maybe one should first fix the documentation
> to know what is actually wanted.  I certainly have lost track of what is
> wanted and what is sensible.

and as i said before, i think you're mistaken.  please highlight the *exact* 
areas of the doc you think permits this behavior.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100325/9b037f11/attachment.pgp 

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Wolfgang Denk]

Dear Mike Frysinger,

In message <201003251338.18817.vapier@gentoo.org> you wrote:
>
> > The question to me is why fec_probe() is called at all, even in cases
> > where U-Boot does not use any network related commands. This is IMO
> > wrong. Eventually somebody comes up with patches to fix this?
> 
> the current fec_mxc driver has squashed the initialize and probe steps.  it
> should have half the things removed from probe and put into initialize and 
> then the call to probe moved to the init step.

OK - but why should we always probe?

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
Sometimes a feeling is all we humans have to go on.
	-- Kirk, "A Taste of Armageddon", stardate 3193.9

[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom

[Mike Frysinger]

On Thursday 25 March 2010 15:11:10 Wolfgang Denk wrote:
> Mike Frysinger wrote:
> > > The question to me is why fec_probe() is called at all, even in cases
> > > where U-Boot does not use any network related commands. This is IMO
> > > wrong. Eventually somebody comes up with patches to fix this?
> > 
> > the current fec_mxc driver has squashed the initialize and probe steps. 
> > it should have half the things removed from probe and put into
> > initialize and then the call to probe moved to the init step.
> 
> OK - but why should we always probe?

i merely mean that the structure set up and enetaddr seeding should be split 
out into the initialize function.  as for the contents of probe vs init and 
how much the driver should "probe" the hw, i leave that up to the fec_mxc 
maintainer -- i have no vested interest in this driver beyond the common eth 
interface.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100325/9c8fb9f4/attachment.pgp 

[U-Boot] Design principles (was: Re: [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom)

[Detlev Zundel]

Hi Wolfgang,

> Dear Detlev Zundel,
>
> In message <m2tys4k0c8.fsf@ohwell.denx.de> you wrote:
>> 
>> > The question to me is why fec_probe() is called at all, even in cases
>> > where U-Boot does not use any network related commands. This is IMO
>> > wrong. Eventually somebody comes up with patches to fix this?
>> 
>> As I said before, this is documented in doc/README.drivers.eth.  If you
>> want to change this, then maybe one should first fix the documentation
>
> I think the documentation should be changed when the code gets
> changed, not sooner nor later.

I'd much rather write up what I want to see in code than to document the
mess we have in some drivers, but as you keep saying - YMMV.

Actually the existance of a design document shows me that this thought
is not so wild as it sounds.

>> to know what is actually wanted.  I certainly have lost track of what is
>> wanted and what is sensible.
>
> We do not want to touch interfaces that we don't use. If we don't use
> Ethernet in U-Boot, there is no need to probe or initialize it.

Ok, I still don't get it.  Is this "we do not touch interfaces" a truth
not to be questioned or is it the essence of other goals we want to
achieve summed up in a nice fashion?

Because what I still fail to see is how writing 6 bytes into an SRAM
area is "touching a device".  Yet the absence of this code means that
there is no working solution at all to the problem at hand as you may
well know.

So can you please enlighten me why as to how initializing SRAM is
"touching a device" - or why exactly you oppose this so strict.

Thanks
  Detlev

-- 
1. What is the best thing about Unix?
A: The community.
2. What is the worst thing about Unix?
A: That there are so many communities.         (Rob Pike)
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] Design principles (was: Re: [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom)

[Wolfgang Denk]

Dear Detlev,

In message <m2hbo3jx28.fsf_-_@ohwell.denx.de> you wrote:
> 
> > We do not want to touch interfaces that we don't use. If we don't use
> > Ethernet in U-Boot, there is no need to probe or initialize it.
> 
> Ok, I still don't get it.  Is this "we do not touch interfaces" a truth
> not to be questioned or is it the essence of other goals we want to
> achieve summed up in a nice fashion?

You know pretty well that this a design principle which is intended to
keep U-Boot small, fast, and flexible.

To me it makes little sense to initialize for example a network port
(and wait hundrets of millisecods, or even seconds) for the link to
come up and (auto-) negotiation to complete, when we do not use the
network in U-Boot ourself, and when he have no knowledge if Linux
will use the network interface (and if it does, it will usually
re-initialize the PHY, thus waiting again). The same goes for the
initialization of USB, IDE, or similar interfaces.

I am well aware that not all code is working like this. Board ports
that origin from earlier versions of U-Boot (or even from PPCBoot)
behave differently.  This is a lesson we learned over time.

But now it's written down.

I am not sure at the moment if you question this design principle in
general, or if you accept it as base for the ongoing discussion.
Please say so, if you disagree here, so we can try to find a
compromize to be used as base for this discussion.

> Because what I still fail to see is how writing 6 bytes into an SRAM
> area is "touching a device".  Yet the absence of this code means that
> there is no working solution at all to the problem at hand as you may
> well know.
> 
> So can you please enlighten me why as to how initializing SRAM is
> "touching a device" - or why exactly you oppose this so strict.

I cannot understand how you might think that writing some data to
registers or internal RAM of a device could be NOT considered as
"touching" this device. You modify the state of this device, ergo you
are touching it. Or not?


I guess what you really want to tell us is that this initializing is
not such a bad thing - it is quick and does not add any real delay to
the boot process, and it solves a problem that otherwise needs to be
solved elsewhere (in the Linux Ethernet driver, or the Linux boot
API), where more effort is needed.

If you re-read my previous postings in this thread you might even see
that I tend to take a pretty pragmatic position here and support  the
suggested fix for the exiting (obviously incorrect) behaviour.


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
Where people stand is not as important as which way they face.
        - Terry Pratchett & Stephen Briggs, _The Discworld Companion_

[U-Boot] Design principles

[Detlev Zundel]

Hi Wolfgang,

>> > We do not want to touch interfaces that we don't use. If we don't use
>> > Ethernet in U-Boot, there is no need to probe or initialize it.
>> 
>> Ok, I still don't get it.  Is this "we do not touch interfaces" a truth
>> not to be questioned or is it the essence of other goals we want to
>> achieve summed up in a nice fashion?
>
> You know pretty well that this a design principle which is intended to
> keep U-Boot small, fast, and flexible.

Exactly, it is the essence of these ultimate goals and not a credo in
itself.

> To me it makes little sense to initialize for example a network port
> (and wait hundrets of millisecods, or even seconds) for the link to
> come up and (auto-) negotiation to complete, when we do not use the
> network in U-Boot ourself, and when he have no knowledge if Linux
> will use the network interface (and if it does, it will usually
> re-initialize the PHY, thus waiting again). The same goes for the
> initialization of USB, IDE, or similar interfaces.

Full ACK and I really _never_ questioned this.

> I am well aware that not all code is working like this. Board ports
> that origin from earlier versions of U-Boot (or even from PPCBoot)
> behave differently.  This is a lesson we learned over time.
>
> But now it's written down.

And this is a good thing in full accordance with my previous statements.

> I am not sure at the moment if you question this design principle in
> general, or if you accept it as base for the ongoing discussion.
> Please say so, if you disagree here, so we can try to find a
> compromize to be used as base for this discussion.

I agree to the design principle but for the value it brings and not for
the words it is formulated in.

>> Because what I still fail to see is how writing 6 bytes into an SRAM
>> area is "touching a device".  Yet the absence of this code means that
>> there is no working solution at all to the problem at hand as you may
>> well know.
>> 
>> So can you please enlighten me why as to how initializing SRAM is
>> "touching a device" - or why exactly you oppose this so strict.
>
> I cannot understand how you might think that writing some data to
> registers or internal RAM of a device could be NOT considered as
> "touching" this device. You modify the state of this device, ergo you
> are touching it. Or not?

This is getting philosophical, really and I can find good arguments for
both views: If you consider the mac sram cells to be part of the state
of the device, you _do_ change the state.  On the other hand, the
ethernet controller ip block very likely has no idea whatsoever if those
sram was touched, so in this view you _do not_ change the state.

So if this is getting philosophical, why not ask the question "under
what circumstances could this writing into sram have negative
effects?".  I for myself cannot find any point to raise here.

> I guess what you really want to tell us is that this initializing is
> not such a bad thing - it is quick and does not add any real delay to
> the boot process, and it solves a problem that otherwise needs to be
> solved elsewhere (in the Linux Ethernet driver, or the Linux boot
> API), where more effort is needed.

Yes, it solves a problem.  Also it is a static initialization which I
feel is something the bootloader should do.  U-Boot knows ethaddr, so
again I see no negative, only positive consequences of doing such a
thing

> If you re-read my previous postings in this thread you might even see
> that I tend to take a pretty pragmatic position here and support  the
> suggested fix for the exiting (obviously incorrect) behaviour.

So it seems I did not understand your "please include in your fix"
statement.  Can you tell me exactly what you meant by this?

Cheers
  Detlev

-- 
A change in language can transform our appreciation of the cosmos
                       -- Benjamin Lee Whorf
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] Design principles

[Wolfgang Denk]

Dear Detlev,

In message <m2sk7ngv95.fsf@ohwell.denx.de> you wrote:
> 
> >> > We do not want to touch interfaces that we don't use. If we don't use
...


...
> Full ACK and I really _never_ questioned this.

Good.

> > I cannot understand how you might think that writing some data to
> > registers or internal RAM of a device could be NOT considered as
> > "touching" this device. You modify the state of this device, ergo you
> > are touching it. Or not?
> 
> This is getting philosophical, really and I can find good arguments for
> both views: If you consider the mac sram cells to be part of the state
> of the device, you _do_ change the state.  On the other hand, the
> ethernet controller ip block very likely has no idea whatsoever if those
> sram was touched, so in this view you _do not_ change the state.

This argument takes you from bad to worse. If you consider this SRAM
as a separate device (not related to the Ethernet), then the
"Initialize devices only when they are needed within U-Boot" rule
still means that this SRAM device shall not be touched, as we don't
need or use it in U-Boot (unless running some network command).


> So if this is getting philosophical, why not ask the question "under
> what circumstances could this writing into sram have negative
> effects?".  I for myself cannot find any point to raise here.

With this approach we can probably partially initialize a number of
other devices as well.

But does it make sense?  Shall we start to accept code that will
perform the "fast" part of initialization for a random collection of
devices?  Leaving devices in a "half-initialized" state?

As I see it, the core problem is a gap loophole in the (ARM) Linux
kernel interface. If we had - for example - device tree support for
ARM, or something like a MAC ATAG, all this discussion would be void.

Do you agree?

So why cannot you simply concede that we are knowingly violating our
own rules and take the line of least resistance?


> Yes, it solves a problem.  Also it is a static initialization which I
> feel is something the bootloader should do.  U-Boot knows ethaddr, so
> again I see no negative, only positive consequences of doing such a
> thing

It is a violation of the design rule, still. We don't make any
difference there between "static" or "dynamic", "fast" or "slow"
initializations.  I think it is good that we don't.

> > If you re-read my previous postings in this thread you might even see
> > that I tend to take a pretty pragmatic position here and support  the
> > suggested fix for the exiting (obviously incorrect) behaviour.
> 
> So it seems I did not understand your "please include in your fix"
> statement.  Can you tell me exactly what you meant by this?


I was referring to this part of Heikos mail:

> While writing this, and realizing that this behaviour is a feature,
> maybe this problem occur on other network drivers on arm plattforms
> too ... hah, found one:
> 
> drivers/net/kirkwood_egiga.c
> 
> did it in the same way as drivers/net/fec_mxc.c ... Ok, it is
> in line with uboot standard, but arm plattforms which booting
> linux without doing network trafic under uboot tend to have
> different mac addresses ...
> 
> This should be solved!

The "Please include in your fix" means that the same bug fix as
implemented for the fec_mxc driver should also be applied to the
kirkwood_egiga driver - I see no sense in knowing the same bug is in
two files and fixing it in just one.


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
Never ascribe to malice that which can  adequately  be  explained  by
stupidity.

[U-Boot] Design principles

[Detlev Zundel]

Hi Wolfgang,

>> > I cannot understand how you might think that writing some data to
>> > registers or internal RAM of a device could be NOT considered as
>> > "touching" this device. You modify the state of this device, ergo you
>> > are touching it. Or not?
>> 
>> This is getting philosophical, really and I can find good arguments for
>> both views: If you consider the mac sram cells to be part of the state
>> of the device, you _do_ change the state.  On the other hand, the
>> ethernet controller ip block very likely has no idea whatsoever if those
>> sram was touched, so in this view you _do not_ change the state.
>
> This argument takes you from bad to worse. If you consider this SRAM
> as a separate device (not related to the Ethernet), then the
> "Initialize devices only when they are needed within U-Boot" rule
> still means that this SRAM device shall not be touched, as we don't
> need or use it in U-Boot (unless running some network command).

You are regarding 6 bytes of SRAM as a device _only so you can
mechanically apply a sentence to it_ which may loose its meaning in the
process?  You loose me down this route, sorry.

There was this quote on this mailing list previously so maybe it is time
to repost it:

A foolish consistency is the hobgoblin of little minds.
                                   -- Ralph Waldo Emerson             

>> So if this is getting philosophical, why not ask the question "under
>> what circumstances could this writing into sram have negative
>> effects?".  I for myself cannot find any point to raise here.
>
> With this approach we can probably partially initialize a number of
> other devices as well.

This is only if you accept that writing 6 bytes of SRAM is "partly
initializing a device", which I do not follow, sorry.

> But does it make sense?  Shall we start to accept code that will
> perform the "fast" part of initialization for a random collection of
> devices?  Leaving devices in a "half-initialized" state?

Again, I do not agree on the "half-initialized state".

> As I see it, the core problem is a gap loophole in the (ARM) Linux
> kernel interface. If we had - for example - device tree support for
> ARM, or something like a MAC ATAG, all this discussion would be void.
>
> Do you agree?

Not quite.  I know that this topic crept up on ARM, but actually it was
also the netdev mailing list that turned down the attempts to allow for
mac addresses to be passed to the network drivers.  So this is not
bounded to ARM.  

I would rather restate it in the light of "what initializations are to
be done by the firmware and what initializations are to be done by the
linux drivers".

Now this is a controversial thing, for sure, but it is all the more
worthy of real reflection instead of blindly applying rules.

> So why cannot you simply concede that we are knowingly violating our
> own rules and take the line of least resistance?

Why should I?  I still do not believe that following a sentence blindly
is one of our rules, sorry.  If this is what you think, then say so
clearly and we can let the discussion rest as then there is no room for
discussion in the first place.

I also tried for quite a length to explain why I also consider this not
a "least resistance" way - no matter how often you repeat it.

>> Yes, it solves a problem.  Also it is a static initialization which I
>> feel is something the bootloader should do.  U-Boot knows ethaddr, so
>> again I see no negative, only positive consequences of doing such a
>> thing
>
> It is a violation of the design rule, still. We don't make any
> difference there between "static" or "dynamic", "fast" or "slow"
> initializations.  I think it is good that we don't.

For myself I disagree.  I do not consider switching off thought to be a
good thing.

>> > If you re-read my previous postings in this thread you might even see
>> > that I tend to take a pretty pragmatic position here and support  the
>> > suggested fix for the exiting (obviously incorrect) behaviour.
>> 
>> So it seems I did not understand your "please include in your fix"
>> statement.  Can you tell me exactly what you meant by this?
>
>
> I was referring to this part of Heikos mail:
>
>> While writing this, and realizing that this behaviour is a feature,
>> maybe this problem occur on other network drivers on arm plattforms
>> too ... hah, found one:
>> 
>> drivers/net/kirkwood_egiga.c
>> 
>> did it in the same way as drivers/net/fec_mxc.c ... Ok, it is
>> in line with uboot standard, but arm plattforms which booting
>> linux without doing network trafic under uboot tend to have
>> different mac addresses ...
>> 
>> This should be solved!
>
> The "Please include in your fix" means that the same bug fix as
> implemented for the fec_mxc driver should also be applied to the
> kirkwood_egiga driver - I see no sense in knowing the same bug is in
> two files and fixing it in just one.

We are drifing apart even more.  I do not say that I grasp every part of
the kirkwood_egiga driver, but for me it seems like it only reads
the ethaddr environment addresses which I fail to see why it does that.
What I cannot see is that it programs the mac address in any way.  So
what exactly is the bug you are referring to here and how do you "support
a pragmatic fix"?

Cheers
  Detlev

-- 
LISP has survived for 21 years because it is an approximate local
optimum in the space of programming languages.
                                           -- John McCarthy (1980)
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] Design principles

[Wolfgang Denk]

Dear Detlev Zundel,

In message <m27hozgrrn.fsf@ohwell.denx.de> you wrote:
> 
> > I was referring to this part of Heikos mail:
> >
> >> While writing this, and realizing that this behaviour is a feature,
> >> maybe this problem occur on other network drivers on arm plattforms
> >> too ... hah, found one:
> >> 
> >> drivers/net/kirkwood_egiga.c
> >> 
> >> did it in the same way as drivers/net/fec_mxc.c ... Ok, it is
> >> in line with uboot standard, but arm plattforms which booting
> >> linux without doing network trafic under uboot tend to have
> >> different mac addresses ...
> >> 
> >> This should be solved!
> >
> > The "Please include in your fix" means that the same bug fix as
> > implemented for the fec_mxc driver should also be applied to the
> > kirkwood_egiga driver - I see no sense in knowing the same bug is in
> > two files and fixing it in just one.
> 
> We are drifing apart even more.  I do not say that I grasp every part of
> the kirkwood_egiga driver, but for me it seems like it only reads
> the ethaddr environment addresses which I fail to see why it does that.

I think it does so to initialize the envrionment with a (semi-) random
private MAC address if none is set.

> What I cannot see is that it programs the mac address in any way.  So
> what exactly is the bug you are referring to here and how do you "support
> a pragmatic fix"?

I did not look at that driver until now. It was Heiko who said that
"this problem occur on other network drivers" and lists
kirkwood_egiga as an example.


My request was to apply the fix that Heiko submitted for fec_mxc.c to
all other drivers as well that have the sam problem (as far as we
know about these, of course). So if kirkwood_egiga is clean (in this
respect), there is no need to change it.

I only want to make sure that "this problem occur on other network
drivers" does not slip through, but that any such drivers we know
about gt fixed as well.

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
In general, if you think something isn't in Perl, try it out, because
it usually is :-) - Larry Wall in <1991Jul31.174523.9447@netlabs.com>

[U-Boot] Design principles

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> Dear Detlev Zundel,
> 
> In message <m27hozgrrn.fsf@ohwell.denx.de> you wrote:
[...]
>> We are drifing apart even more.  I do not say that I grasp every part of
>> the kirkwood_egiga driver, but for me it seems like it only reads
>> the ethaddr environment addresses which I fail to see why it does that.
> 
> I think it does so to initialize the envrionment with a (semi-) random
> private MAC address if none is set.

Yep, exactly.

>> What I cannot see is that it programs the mac address in any way.  So
>> what exactly is the bug you are referring to here and how do you "support
>> a pragmatic fix"?
> 
> I did not look at that driver until now. It was Heiko who said that
> "this problem occur on other network drivers" and lists
> kirkwood_egiga as an example.
> 
> 
> My request was to apply the fix that Heiko submitted for fec_mxc.c to
> all other drivers as well that have the sam problem (as far as we

I can make such an attempt, if It gets accepted here?

> know about these, of course). So if kirkwood_egiga is clean (in this
> respect), there is no need to change it.

It ends in the same problem, as the fec_mxc.c driver has ...

If not running network commands in u-boot, the mac is not setup
which results in the problem with booting an arm Linux Kernel.

If running network commands in u-boot, the mac is setup properly
with the content from ethaddr ...

So, what to do? Accept the arm solution with first booting
an intrd and setup the mac with userspace tools, or waiting
for "arm Linux with DTS", or fixing this in u-boot now(maybe with
a comment that this is not in line with u-boot design goals,
and should go away, if arm linux is fixed (fixed is maybe not
the right word ... ))?

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] Design principles

[Wolfgang Denk]

Dear Heiko,

in message <4BADB479.6050604@denx.de> you wrote:
> 
> > My request was to apply the fix that Heiko submitted for fec_mxc.c to
> > all other drivers as well that have the sam problem (as far as we
> 
> I can make such an attempt, if It gets accepted here?

As far as I am concerned, I already said so.

> > know about these, of course). So if kirkwood_egiga is clean (in this
> > respect), there is no need to change it.
> 
> It ends in the same problem, as the fec_mxc.c driver has ...
> 
> If not running network commands in u-boot, the mac is not setup
> which results in the problem with booting an arm Linux Kernel.
> 
> If running network commands in u-boot, the mac is setup properly
> with the content from ethaddr ...

Right.

> So, what to do? Accept the arm solution with first booting
> an intrd and setup the mac with userspace tools, or waiting
> for "arm Linux with DTS", or fixing this in u-boot now(maybe with
> a comment that this is not in line with u-boot design goals,
> and should go away, if arm linux is fixed (fixed is maybe not
> the right word ... ))?

It makes obviously no sense to embedded systems to boot a ramdisk
just to set the MAC address so we can then boot for example with root
file system over NFS.  I feel that people suggesting this must be (at
least in this respekt) so narrow minded that they coold look through
a keyhole with both eyes.

It definitely is a good idea to support all activities to bring
device-tree support for ARM, as this will solve a number of other
issues as well.


In U-Boot, we cannot change the whole world. We can strive to get
clean interfaces to the Linux kernel, and object against adding new
code to U-Boot that violates the design principles.  On the other
hand, no review is perfect, and there are many places in the U-Boot
code that are far from perfect.  And if these imperfect pieces of code
contain obvious bugs (as is the case here), it makes perfect sense at
least to fix these bugs as long it is not possible yet (for whatever
reasons) to replace the whole code with a Perfect Implementation (TM).

So please go on and (re-) submit your (extended) patch.

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
Be wiser than other people if you can, but do not tell them so.
                                       -- Philip Earl of Chesterfield