[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 hs@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;
Dear Heiko Schocher,
In message 4BB238E9.7060609@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 hs@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
Hi Wolfgang,
On 3/30/2010 1:34 PM, Wolfgang Denk wrote:
Dear Heiko Schocher,
In message4BB238E9.7060609@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 Schocherhs@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
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
Wolfgang,
On 3/30/2010 1:34 PM, Wolfgang Denk wrote:
Dear Heiko Schocher,
In message4BB238E9.7060609@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 Schocherhs@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
Hello Ben,
Ben Warren wrote:
Wolfgang,
On 3/30/2010 1:34 PM, Wolfgang Denk wrote:
Dear Heiko Schocher,
In message4BB238E9.7060609@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 Schocherhs@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:
- 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!
- Read from environment in net/eth.c after initialize()
- Give priority to the value in the environment if a conflict
- 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
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%
- Read from environment in net/eth.c after initialize()
- Give priority to the value in the environment if a conflict
- 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
only fill the device enetaddr with the contents of the eeprom, not program it in mac address registers
Signed-off-by: Heiko Schocher hs@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
- changes since v2 posted here:
http://lists.denx.de/pipermail/u-boot/2010-March/069244.html
- add comment from Ben and Mike: - not to look in content from "ethaddr" - only read the mac from eeprom to ethdevices enetaddr and do not program it in mac address registers
drivers/net/fec_mxc.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 5af9cdb..8c4ade5 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -752,7 +752,6 @@ static int fec_probe(bd_t *bd) 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); }
return 0;
Hi Heiko,
Much better. Thanks, and sorry again for not responding sooner.
Wolfgang - please apply this to the current release.
On Tue, Mar 30, 2010 at 11:34 PM, Heiko Schocher hs@denx.de wrote:
only fill the device enetaddr with the contents of the eeprom, not program it in mac address registers
Signed-off-by: Heiko Schocher hs@denx.de
Acked-by: Ben Warren biggerbadderben@gmail.com
---
- changes since v1 posted here:
http://lists.denx.de/pipermail/u-boot/2010-March/069192.html
splitted in two patches as Wolfgang suggested
changes since v2 posted here:
http://lists.denx.de/pipermail/u-boot/2010-March/069244.html
- add comment from Ben and Mike:
- not to look in content from "ethaddr"
- only read the mac from eeprom to ethdevices enetaddr and do not program it in mac address registers
drivers/net/fec_mxc.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 5af9cdb..8c4ade5 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -752,7 +752,6 @@ static int fec_probe(bd_t *bd) 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); } 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
Dear Ben Warren,
In message w2if8328f7c1003302350o154a5e96k25d7f8674657820@mail.gmail.com you wrote:
Wolfgang - please apply this to the current release.
On Tue, Mar 30, 2010 at 11:34 PM, Heiko Schocher hs@denx.de wrote:
only fill the device enetaddr with the contents of the eeprom, not program it in mac address registers
Signed-off-by: Heiko Schocher hs@denx.de
Acked-by: Ben Warren biggerbadderben@gmail.com
Done, thanks.
Best regards,
Wolfgang Denk
Dear Heiko Schocher,
In message 4BB2ED06.3010907@denx.de you wrote:
only fill the device enetaddr with the contents of the eeprom, not program it in mac address registers
Signed-off-by: Heiko Schocher hs@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
changes since v2 posted here:
http://lists.denx.de/pipermail/u-boot/2010-March/069244.html
- add comment from Ben and Mike:
- not to look in content from "ethaddr"
- only read the mac from eeprom to ethdevices enetaddr and do not program it in mac address registers
drivers/net/fec_mxc.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
Thanks. Applied after reverting v2.
Best regards,
Wolfgang Denk
Hi All,
I'm not sure if this is the place to ask this, please redirect if it isn't.
I'm looking into using a BDI3000 with a P2020RDB evaluation kit we've already purchased from Denx, but there is some confusion over how the board hardware handles the power on boot configuration signals to the processor. The addenda to the P2020RDB manuals provided by Freescale seem to indicate that certain boot functions that are supposed to be configurable using dipswitches on the board do not work as they should, notably certain clock speeds and the SPI and eSDHC boot configurations, which are the two options I am keen to exercise before moving on to prototyping.
Is a BDI3000, (using bdiGDB?), able to override the signals on these pins using JTAG functionality? Essentially, what I would like is some way of finding out if the P2020 is obtaining the correct PLL ratio and boot ROM location from the signals, and more importantly, override any erroneus signals that the hardware on the platform (P2020RDB and later, the prototype) might be providing (or failing to provide) it. (without using an oscilloscope and a soldering iron that is).
Regards,
Can Aydin
as this seems unclear, document how the flow of setting up the mac address is correct.
Signed-off-by: Heiko Schocher hs@denx.de --- doc/README.enetaddr | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/doc/README.enetaddr b/doc/README.enetaddr index 1d75aa3..9edb6cd 100644 --- a/doc/README.enetaddr +++ b/doc/README.enetaddr @@ -28,6 +28,18 @@ Here are the places where MAC addresses might be stored: purpose of passing this information to an OS kernel we are about to boot
+----------- + correct flow of setting up the mac address (summarized) +----------- + +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. + ------- Usage -------
Dear Heiko Schocher,
In message 4BB2ED0B.6030406@denx.de you wrote:
as this seems unclear, document how the flow of setting up the mac address is correct.
Signed-off-by: Heiko Schocher hs@denx.de
doc/README.enetaddr | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
Thanks. Applied (as discussed internally) with slight changes, adding Mike Frysinger's input.
Best regards,
Wolfgang Denk
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:
- Read from hardware in initialize() function
- Read from environment in net/eth.c after initialize()
- Give priority to the value in the environment if a conflict
- 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
Hi Detlev,
On Wed, Mar 31, 2010 at 6:44 AM, Detlev Zundel dzu@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:
- Read from hardware in initialize() function
- Read from environment in net/eth.c after initialize()
- Give priority to the value in the environment if a conflict
- 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
Hi Ben,
Hi Detlev,
On Wed, Mar 31, 2010 at 6:44 AM, Detlev Zundel dzu@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
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
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
thanks Heiko for the followups -mike
participants (6)
-
Ben Warren -
Can Aydin -
Detlev Zundel -
Heiko Schocher -
Mike Frysinger -
Wolfgang Denk