[U-Boot] net, kirkwood_egiga: init mac address before using network commands

[U-Boot] net, kirkwood_egiga: init mac address before using network commands

[Heiko Schocher]

initialize mac address with the value from "ethaddr", before
doing some network commands. This is not in line with u-boot
design principle "not to initalize not used devices", and
maybe should go away, if there is a solution for passing
the mac address to arm linux kernels.

Tested on the suen3 board.

Signed-off-by: Heiko Schocher <hs@denx.de>
---
posting this patch as a result of this discussion:

http://lists.denx.de/pipermail/u-boot/2010-March/069025.html

 drivers/net/kirkwood_egiga.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c
index 2ad7fea..e8b3777 100644
--- a/drivers/net/kirkwood_egiga.c
+++ b/drivers/net/kirkwood_egiga.c
@@ -678,7 +678,7 @@ int kirkwood_egiga_initialize(bd_t * bis)
 			return -1;
 		}

-		while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
+		if (!eth_getenv_enetaddr(s, dev->enetaddr)) {
 			/* Generate Random Private MAC addr if not set */
 			dev->enetaddr[0] = 0x02;
 			dev->enetaddr[1] = 0x50;
@@ -688,6 +688,7 @@ int kirkwood_egiga_initialize(bd_t * bis)
 			dev->enetaddr[5] = get_random_hex();
 			eth_setenv_enetaddr(s, dev->enetaddr);
 		}
+		port_uc_addr_set(dkwgbe->regs, dev->enetaddr);

 		dev->init = (void *)kwgbe_init;
 		dev->halt = (void *)kwgbe_halt;
-- 
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] net, kirkwood_egiga: init mac address before using network commands

[Mike Frysinger]

On Tuesday 30 March 2010 01:38:39 Heiko Schocher wrote:
> diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c
> index 2ad7fea..e8b3777 100644
> --- a/drivers/net/kirkwood_egiga.c
> +++ b/drivers/net/kirkwood_egiga.c
> @@ -678,7 +678,7 @@ int kirkwood_egiga_initialize(bd_t * bis)
>  			return -1;
>  		}
> 
> -		while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
> +		if (!eth_getenv_enetaddr(s, dev->enetaddr)) {

this too is broken (not just your change, but also the existing code).  please 
instead fix it to follow the documented MAC handling.  the initialize function 
should seed dev->enetaddr with the eeprom value only while the init function 
should take care of programming dev->enetaddr into the hardware's MAC 
registers.
-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/20100330/083884aa/attachment.pgp 

[U-Boot] net, kirkwood_egiga: init mac address before using network commands

[Simon Kagstrom]

On Tue, 30 Mar 2010 07:38:39 +0200
Heiko Schocher <hs@denx.de> wrote:

> initialize mac address with the value from "ethaddr", before
> doing some network commands. This is not in line with u-boot
> design principle "not to initalize not used devices", and
> maybe should go away, if there is a solution for passing
> the mac address to arm linux kernels.

I also tried this change half a year ago, but it got NACK:ed. The
discussion is here if you are interested:

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

// Simon

[U-Boot] net, kirkwood_egiga: init mac address before using network commands

[Detlev Zundel]

Hello Heiko,

> initialize mac address with the value from "ethaddr", before
> doing some network commands. This is not in line with u-boot
> design principle "not to initalize not used devices", and
> maybe should go away, if there is a solution for passing
> the mac address to arm linux kernels.
>
> Tested on the suen3 board.
>
> Signed-off-by: Heiko Schocher <hs@denx.de>

Acked-by: Detlev Zundel <dzu@denx.de>

My question about why doing this can negatively effect U-Boot still
stands.

I also actively request the U-Boot community to give feedback here -
after all, this _is_ a community project and fixing real problems is one
of the main tasks of a bootloader.

Cheers
  Detlev

-- 
Wenn ein Kopf und ein Buch zusammenstossen und es klingt hohl; ist
denn das allemal im Buche?
                               - Lichtenberg
--
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] net, kirkwood_egiga: init mac address before using network commands

[Simon Kagstrom]

On Tue, 30 Mar 2010 09:52:29 +0200
Detlev Zundel <dzu@denx.de> wrote:

> I also actively request the U-Boot community to give feedback here -
> after all, this _is_ a community project and fixing real problems is one
> of the main tasks of a bootloader.

Personally, I'd prefer using Heikos approach until Arm Linux has moved
to device trees. I know it's a deviation from how it's supposed to
work, but it also solves a real problem without introducing kludges
elsewhere.

I think most people (myself included) would just "solve" the problem by
carrying a private patch to setup the MAC address in U-boot anyway.

// Simon

[U-Boot] net, kirkwood_egiga: init mac address before using network commands

[Wolfgang Denk]

Dear Heiko Schocher,

In message <4BB18E5F.6060705@denx.de> you wrote:
> initialize mac address with the value from "ethaddr", before
> doing some network commands. This is not in line with u-boot
> design principle "not to initalize not used devices", and
> maybe should go away, if there is a solution for passing
> the mac address to arm linux kernels.
> 
> Tested on the suen3 board.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
> posting this patch as a result of this discussion:
> 
> http://lists.denx.de/pipermail/u-boot/2010-March/069025.html

Hm... it seems I misunderstood you there :-(

You wrote (here:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/76173/focus=76338):

| > 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 ...

At this point I thought that you were referring to the problem that
the fec_mxc.c  would under certain conditions ignore the "ethaddr"
environment setting.




>  		}
> 
> -		while (!eth_getenv_enetaddr(s, dev->enetaddr)) {
> +		if (!eth_getenv_enetaddr(s, dev->enetaddr)) {

I think this change is actually not related to the other modifi-
cations in this patch. Axctually, it is not even needed. OK, you can
call it paranoid and a waste of time to re-check the setting of the
environment variable after running setenv(), but then it's not a real
bug either. In any case it is unrelated and should not be mixed into
this commit.

>  			dev->enetaddr[0] = 0x02;
>  			dev->enetaddr[1] = 0x50;
> @@ -688,6 +688,7 @@ int kirkwood_egiga_initialize(bd_t * bis)
>  			dev->enetaddr[5] = get_random_hex();
>  			eth_setenv_enetaddr(s, dev->enetaddr);
>  		}
> +		port_uc_addr_set(dkwgbe->regs, dev->enetaddr);

This is another issue - this is not a bug fix.

I will leave it up to others (especially Ben) to comment on this part.

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 man will tell his bartender things he'll never tell his doctor.
	-- Dr. Phillip Boyce, "The Menagerie" ("The Cage"),
	   stardate unknown.

[U-Boot] net, kirkwood_egiga: init mac address before using network commands

[Wolfgang Denk]

Dear Simon Kagstrom,

In message <20100330100127.5f36495c@marrow.netinsight.se> you wrote:
> 
> > I also actively request the U-Boot community to give feedback here -
> > after all, this _is_ a community project and fixing real problems is one
> > of the main tasks of a bootloader.
> 
> Personally, I'd prefer using Heikos approach until Arm Linux has moved
> to device trees. I know it's a deviation from how it's supposed to
> work, but it also solves a real problem without introducing kludges
> elsewhere.

If we do not even raise issues with the current Linux code with the
Linux developers they will not even be aware that there are problems.
In the end, things will never change.  

It is always wrong to not even try to fix the root cause of problems.


> I think most people (myself included) would just "solve" the problem by
> carrying a private patch to setup the MAC address in U-boot anyway.

Interesting.  Why would you do this?  Why would you not rather fix the
Linux driver instead? [This is what I would do.]

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
"You can have my Unix system when you  pry  it  from  my  cold,  dead
fingers."                                                - Cal Keegan

[U-Boot] net, kirkwood_egiga: init mac address before using network commands

[Simon Kagstrom]

On Tue, 30 Mar 2010 10:26:59 +0200
Wolfgang Denk <wd@denx.de> wrote:

> > Personally, I'd prefer using Heikos approach until Arm Linux has moved
> > to device trees. I know it's a deviation from how it's supposed to
> > work, but it also solves a real problem without introducing kludges
> > elsewhere.
> 
> If we do not even raise issues with the current Linux code with the
> Linux developers they will not even be aware that there are problems.
> In the end, things will never change.  

I believe they are aware of this especially since many developers work
on both projects anyway. If I remember the discussion on ARM device
trees a year ago or so correct, this was one of the issues brought up
in support of the device trees (or it should have, anyway).

> > I think most people (myself included) would just "solve" the problem by
> > carrying a private patch to setup the MAC address in U-boot anyway.
> 
> Interesting.  Why would you do this?  Why would you not rather fix the
> Linux driver instead? [This is what I would do.]

Basically two reasons: First, it's a simpler fix in U-boot (a oneliner
for Kirkwood), and secondly because (as far as I understand, correct me
if I'm wrong), it lacks any well-defined protocol to transfer this
knowledge to the kernel driver.

I know mostly how it looks on the OpenRD board, where the MAC address
is stored in the U-boot environment. Easy to access in U-boot, but a
lot trickier from Linux. Sure, you could transfer it via a command-line
parameter or something, but personally I think this is uglier than
setting it up in U-boot anyway.

// Simon

[U-Boot] net, kirkwood_egiga: init mac address before using network commands

[Detlev Zundel]

Hi Simon,

>> Interesting.  Why would you do this?  Why would you not rather fix the
>> Linux driver instead? [This is what I would do.]
>
> Basically two reasons: First, it's a simpler fix in U-boot (a oneliner
> for Kirkwood), and secondly because (as far as I understand, correct me
> if I'm wrong), it lacks any well-defined protocol to transfer this
> knowledge to the kernel driver.

Yes, this is one reason, which will be fixed once the device tree is
available (for the boards using it, device tree on ARM in the
foreseeable future will only be optional, not mandatory).  But this is
not the only reason for me personally.

> I know mostly how it looks on the OpenRD board, where the MAC address
> is stored in the U-boot environment. Easy to access in U-boot, but a
> lot trickier from Linux. Sure, you could transfer it via a command-line
> parameter or something, but personally I think this is uglier than
> setting it up in U-boot anyway.

This is the other reason for me indeed.  For the current problem at
hand, we would need to extend the protocol, the protocol handlers and
the linux drivers in order to pass 6 bytes into Linux which then get
simply programmed into registers.  Why not regard the MAC registers as
the protocol to pass this information to the linux driver?  This already
exists and needs no further change whatsoever.

Moreover, linux drivers do not initialize _everything_ for the devices
they use, not even in PowerPC world.  Take for example the GPIOs on a
5200 board.  They are configured by this (admittedly inelegant) central
portconfig register.  The linux drivers accessing pins (which can be
either gpios or function pins) do not touch this portconfig but rely on
its correct setting in U-Boot.  So for the 5200 boards in U-Boot, we set
this portconfig in U-Boot even though we may never touch any of the
devices this "configuration" touches.  This is just one example I'm
personally aware of, I'm sure one can find many more like this.

Of course we can surely extend the device tree with yet more
information, but in my personal view it is also perfectly fine to
delegate some taks to the firmware.  We only have to be clear about what
tasks are to be done by firmware and what the operating system will do
by itself.

In the long run I think that if we extend all linux drivers to be
capable to initialize everything about their context, we have an
unmaintainable mess in the linux drivers.  I'd rather like to see linux
drivers implement the shared functionality and firmware to setup a
context of bare usability.

So the cut between what firmware can and/or should initialize is not
always as black and white for me personally but still an important
question to discuss.

Cheers
  Detlev

-- 
Music scenes ain't real life / They won't get rid of the bomb
Won't eliminate rape / Or bring down the banks / any kind of change
Takes more time and work / than changing channels on a TV set
                                        -- Jello Biafra
--
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