[U-Boot] net, kirkwood_egiga: init mac address before using network commands
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;
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
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@lists.denx.de/msg16994.html
// Simon
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
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
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
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
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
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
participants (5)
-
Detlev Zundel -
Heiko Schocher -
Mike Frysinger -
Simon Kagstrom -
Wolfgang Denk