Re: [U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
On Wednesday 02 June 2010 21:57:42 Heiko Schocher wrote: Actual fec_mxc.c driver is *not* correct, because if in eeprom is a correct mac, it *always* programms this in the mac address registers from the chip! This is not OK, and must be fixed! i agree 100% 2. Read from environment in net/eth.c after initialize() 3. Give priority to the value in the environment if a conflict 4. Program hardware in the device's init() function. If somebody wants to subvert the 'design philosophy', the right way is to call eth_dev-init() in board code. Maybe this list should go in a doc? the 1. - 4. is already in the documents ive mentioned multiple times, but they arent short to the point like Ben has summarized, so that would probably be good to add as a summary and/or intro to one of them. Ben's suggestion on how to subvert things by forcibly calling eth_dev- init() sits best in my book for people insisting on throwing in a hack today. it could even be done today in the board-specific board_eth_init() function by calling eth_init() after all the NICs have been registered. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
thanks Heiko for the followups -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
Hi Ben, Hold on a second. This patch is wrong. As Mike has pointed out, the net library already gets the MAC address from the environment. The correct flow is: 1. Read from hardware in initialize() function 2. Read from environment in net/eth.c after initialize() 3. Give priority to the value in the environment if a conflict 4. Program hardware in the device's init() function. If somebody wants to subvert the 'design philosophy', the right way is to call eth_dev-init() in board code. This would mean that for the real problem of a missing mac address, the device then is initialized completely adding the autonegotation timeout to every bootup sequence, correct? If it is, then it doesn't solve my problem in an acceptable way. On the other hand a different route occured to Wolfgang and me in a lengthy discussion. This will need a little broader interpretation of the design guidelines, but as I still cannot see any downside to this and it will also fix some inconsistent behaviour _that we currently have_ (setenv ethaddr ..., do not do any network transfer and boot linux), I want to follow this route. I will try to implement this in form of a patch in order to keep the discussion close to the effects on the code base. Cheers Detlev -- #!/usr/bin/perl $c=print\\#\!\/usr\/bin\/perl\ \\\$c\=\.quotemeta\(\$c\)\.\;\\n\$c;\; print#!/usr/bin/perl\n\$c=\.quotemeta($c).\;\n$c;; -- 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: d...@denx.de ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
Hi Detlev, On Wed, Mar 31, 2010 at 6:44 AM, Detlev Zundel d...@denx.de wrote: Hi Ben, Hold on a second. This patch is wrong. As Mike has pointed out, the net library already gets the MAC address from the environment. The correct flow is: 1. Read from hardware in initialize() function 2. Read from environment in net/eth.c after initialize() 3. Give priority to the value in the environment if a conflict 4. Program hardware in the device's init() function. If somebody wants to subvert the 'design philosophy', the right way is to call eth_dev-init() in board code. This would mean that for the real problem of a missing mac address, the device then is initialized completely adding the autonegotation timeout to every bootup sequence, correct? My suggestion here is a crude hack, and definitely does more than needed. It has the advantage of having narrow scope (is limited to the board in question). If it is, then it doesn't solve my problem in an acceptable way. On the other hand a different route occured to Wolfgang and me in a lengthy discussion. This will need a little broader interpretation of the design guidelines, but as I still cannot see any downside to this and it will also fix some inconsistent behaviour _that we currently have_ (setenv ethaddr ..., do not do any network transfer and boot linux), I want to follow this route. I will try to implement this in form of a patch in order to keep the discussion close to the effects on the code base. I'm looking forward to seeing what you come up with. I personally don't have a problem with adding the few ns to boot-up time that programming an SOC's MAC would take, but dislike the piece-meal approach that's been done so far. Cheers Detlev regards, Ben ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
Hi Ben, Hi Detlev, On Wed, Mar 31, 2010 at 6:44 AM, Detlev Zundel d...@denx.de wrote: Hi Ben, Hold on a second. This patch is wrong. As Mike has pointed out, the net library already gets the MAC address from the environment. The correct flow is: 1. Read from hardware in initialize() function 2. Read from environment in net/eth.c after initialize() 3. Give priority to the value in the environment if a conflict 4. Program hardware in the device's init() function. If somebody wants to subvert the 'design philosophy', the right way is to call eth_dev-init() in board code. This would mean that for the real problem of a missing mac address, the device then is initialized completely adding the autonegotation timeout to every bootup sequence, correct? My suggestion here is a crude hack, and definitely does more than needed. It has the advantage of having narrow scope (is limited to the board in question). This specific problem in the meantime hit me on multiple arm boards with different network interfaces so I think it has a broader audience than a single board. If it is, then it doesn't solve my problem in an acceptable way. On the other hand a different route occured to Wolfgang and me in a lengthy discussion. This will need a little broader interpretation of the design guidelines, but as I still cannot see any downside to this and it will also fix some inconsistent behaviour _that we currently have_ (setenv ethaddr ..., do not do any network transfer and boot linux), I want to follow this route. I will try to implement this in form of a patch in order to keep the discussion close to the effects on the code base. I'm looking forward to seeing what you come up with. I personally don't have a problem with adding the few ns to boot-up time that programming an SOC's MAC would take, but dislike the piece-meal approach that's been done so far. I fully agree. Previously I was under the impression that we already have a fast initialization (probe) and a full initialization (init) of the network interfaces, where programming the mac would (on a first stab) fit into the probe part (and some drivers obviously do/did this). In the meantime it seems like it is a broader problem of keeping ethaddr and friends in sync with the real hardware. Although this is something I personally always took for granted, it currently is most of the time but not always correct. If we solve the latter problem in a nice way, the initial problem will simply disappear (or so I hope) ;) Cheers Detlev -- You live and learn -- 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: d...@denx.de ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
On Wednesday 31 March 2010 10:46:25 Detlev Zundel wrote: I fully agree. Previously I was under the impression that we already have a fast initialization (probe) and a full initialization (init) of the network interfaces, where programming the mac would (on a first stab) fit into the probe part (and some drivers obviously do/did this). In the meantime it seems like it is a broader problem of keeping ethaddr and friends in sync with the real hardware. Although this is something I personally always took for granted, it currently is most of the time but not always correct. If we solve the latter problem in a nice way, the initial problem will simply disappear (or so I hope) ;) the latter problem is already solved in the general case. the first initialize path gets the hw mac address and gives it to the common layers. common layers take care of keeping the structures in sync. when the slow init path gets called to actually bring up the hardware, the mac has already been synced to the structures. correctly written devices never have to care about anything other than dev-enetaddr and should not be programming the MAC with the MAC address in the fast initialize path. really what you want is a new eth entry point like eth_write_mac() that goes through the common layers and programs the MAC with the MAC address. this isnt a problem with the current net framework. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
Hi Mike, On 3/31/2010 12:59 PM, Mike Frysinger wrote: On Wednesday 31 March 2010 10:46:25 Detlev Zundel wrote: I fully agree. Previously I was under the impression that we already have a fast initialization (probe) and a full initialization (init) of the network interfaces, where programming the mac would (on a first stab) fit into the probe part (and some drivers obviously do/did this). In the meantime it seems like it is a broader problem of keeping ethaddr and friends in sync with the real hardware. Although this is something I personally always took for granted, it currently is most of the time but not always correct. If we solve the latter problem in a nice way, the initial problem will simply disappear (or so I hope) ;) the latter problem is already solved in the general case. the first initialize path gets the hw mac address and gives it to the common layers. common layers take care of keeping the structures in sync. when the slow init path gets called to actually bring up the hardware, the mac has already been synced to the structures. correctly written devices never have to care about anything other than dev-enetaddr and should not be programming the MAC with the MAC address in the fast initialize path. really what you want is a new eth entry point like eth_write_mac() that goes through the common layers and programs the MAC with the MAC address. this isnt a problem with the current net framework. Detlev and I were just talking off-list about exactly that. I guess 'great minds think alike', as my grandma used to say. -mike regards, Ben ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
Hi Wolfgang, On 3/30/2010 1:34 PM, Wolfgang Denk wrote: Dear Heiko Schocher, In message4bb238e9.7060...@denx.de you wrote: if ethaddr is not setup in the environment, fill the device enetaddr with the contents of the eeprom, and only the device enetaddr, not the mac address registers! Tested on the magnesium board. Signed-off-by: Heiko Schocherh...@denx.de --- - changes since v1 posted here: http://lists.denx.de/pipermail/u-boot/2010-March/069192.html - splitted in two patches as Wolfgang suggested Thanks. Note that it would also have been an excellent idea to put the responsible custodian on Cc: drivers/net/fec_mxc.c |9 + 1 files changed, 5 insertions(+), 4 deletions(-) Applied, thanks. Ben, this is (as far as I see it) an undisputed bug fix, so I'm pulling this patch (and only this one from this series of 4) directly. Hope this is ok with you. Sorry for not wading into this conversation earlier. I have issues with this driver, in particular that it isn't truly a 'MULTI' driver. This came to my attention when I noticed that Heiko's changes reference the 'ethaddr' environment variable. Hopefully somebody will fix it properly. In any case, as you say, this fixes a real bug and doesn't make things worse. Good enough, I guess. Best regards, Wolfgang Denk thanks, Ben ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
On Tuesday 30 March 2010 16:34:00 Wolfgang Denk wrote: Ben, this is (as far as I see it) an undisputed bug fix, so I'm pulling this patch (and only this one from this series of 4) directly. Hope this is ok with you. are people just ignoring my e-mails ? ive already pointed out multiple times why this is wrong and not what the current net standard is doing. -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
Wolfgang, On 3/30/2010 1:34 PM, Wolfgang Denk wrote: Dear Heiko Schocher, In message4bb238e9.7060...@denx.de you wrote: if ethaddr is not setup in the environment, fill the device enetaddr with the contents of the eeprom, and only the device enetaddr, not the mac address registers! Tested on the magnesium board. Signed-off-by: Heiko Schocherh...@denx.de --- - changes since v1 posted here: http://lists.denx.de/pipermail/u-boot/2010-March/069192.html - splitted in two patches as Wolfgang suggested Thanks. Note that it would also have been an excellent idea to put the responsible custodian on Cc: drivers/net/fec_mxc.c |9 + 1 files changed, 5 insertions(+), 4 deletions(-) Applied, thanks. Ben, this is (as far as I see it) an undisputed bug fix, so I'm pulling this patch (and only this one from this series of 4) directly. Hope this is ok with you. Hold on a second. This patch is wrong. As Mike has pointed out, the net library already gets the MAC address from the environment. The correct flow is: 1. Read from hardware in initialize() function 2. Read from environment in net/eth.c after initialize() 3. Give priority to the value in the environment if a conflict 4. Program hardware in the device's init() function. If somebody wants to subvert the 'design philosophy', the right way is to call eth_dev-init() in board code. regards, Ben ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
Hello Ben,
Ben Warren wrote:
Wolfgang,
On 3/30/2010 1:34 PM, Wolfgang Denk wrote:
Dear Heiko Schocher,
In message4bb238e9.7060...@denx.de you wrote:
if ethaddr is not setup in the environment, fill the device
enetaddr with the contents of the eeprom, and only
the device enetaddr, not the mac address registers!
Tested on the magnesium board.
Signed-off-by: Heiko Schocherh...@denx.de
---
- changes since v1 posted here:
http://lists.denx.de/pipermail/u-boot/2010-March/069192.html
- splitted in two patches as Wolfgang suggested
Thanks. Note that it would also have been an excellent idea to put
the responsible custodian on Cc:
drivers/net/fec_mxc.c |9 +
1 files changed, 5 insertions(+), 4 deletions(-)
Applied, thanks.
Ben, this is (as far as I see it) an undisputed bug fix, so I'm
pulling this patch (and only this one from this series of 4)
directly. Hope this is ok with you.
Hold on a second. This patch is wrong. As Mike has pointed out, the
Now I got lost ... I think the critical part of my patch v1 is splitted
out to 2/2 v2 ... or?
net library already gets the MAC address from the environment. The
correct flow is:
1. Read from hardware in initialize() function
Ah, Ok, so that is the right way? If so, then I should remove
+ if (!eth_getenv_enetaddr(ethaddr, ethaddr)) {
from my patch, and then it should be OK, right?
Actual fec_mxc.c driver is *not* correct, because if in eeprom
is a correct mac, it *always* programms this in the mac address
registers from the chip!
This is not OK, and must be fixed!
2. Read from environment in net/eth.c after initialize()
3. Give priority to the value in the environment if a conflict
4. Program hardware in the device's init() function.
If somebody wants to subvert the 'design philosophy', the right way is
to call eth_dev-init() in board code.
Maybe this list should go in a doc?
bye
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
if ethaddr is not setup in the environment, fill the device
enetaddr with the contents of the eeprom, and only
the device enetaddr, not the mac address registers!
Tested on the magnesium board.
Signed-off-by: Heiko Schocher h...@denx.de
---
- changes since v1 posted here:
http://lists.denx.de/pipermail/u-boot/2010-March/069192.html
- splitted in two patches as Wolfgang suggested
drivers/net/fec_mxc.c |9 +
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
index 5af9cdb..98ff64b 100644
--- a/drivers/net/fec_mxc.c
+++ b/drivers/net/fec_mxc.c
@@ -749,10 +749,11 @@ 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)) {
+ if (fec_get_hwaddr(edev, ethaddr) == 0) {
+ printf(got MAC address from EEPROM: %pM\n, ethaddr);
+ memcpy(edev-enetaddr, ethaddr, 6);
+ }
}
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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] net, fec_mxc: only setup the device enetaddr with eeprom value, if ethaddr is not setup
Dear Heiko Schocher, In message 4bb238e9.7060...@denx.de you wrote: if ethaddr is not setup in the environment, fill the device enetaddr with the contents of the eeprom, and only the device enetaddr, not the mac address registers! Tested on the magnesium board. Signed-off-by: Heiko Schocher h...@denx.de --- - changes since v1 posted here: http://lists.denx.de/pipermail/u-boot/2010-March/069192.html - splitted in two patches as Wolfgang suggested Thanks. Note that it would also have been an excellent idea to put the responsible custodian on Cc: drivers/net/fec_mxc.c |9 + 1 files changed, 5 insertions(+), 4 deletions(-) Applied, thanks. Ben, this is (as far as I see it) an undisputed bug fix, so I'm pulling this patch (and only this one from this series of 4) directly. Hope this is ok with 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: w...@denx.de Our management frequently gets lost in thought. That's because it's unfamiliar territory. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
