[U-Boot] [PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom
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; }
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@lists.denx.de/msg28329.html
Regards, Stefano Babic
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@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
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
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@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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
participants (5)
-
Detlev Zundel -
Heiko Schocher -
Mike Frysinger -
Stefano Babic -
Wolfgang Denk