| Message ID | 1317939802-19695-1-git-send-email-michael@walle.cc |
|---|---|
| State | Changes Requested |
| Headers | show |
> -----Original Message----- > From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] > On Behalf Of Michael Walle > Sent: Friday, October 07, 2011 3:53 AM > To: u-boot@lists.denx.de > Subject: [U-Boot] [PATCH] mvgbe: fix network device indices > > Don't assume that the MAC address of egiga0 rsp. egiga1 is ethaddr rsp. > eth1addr. If there is only a egiga1 device, u-boot will enumerate it as > device 0 and therefore the MAC address is set with the environmen > varibale > ethaddr. So if there is only one eth port available, the associated environment variable should be ethaddr, this is understood. Is there any issue with this? Then I have anther question in my mind- Why not we name eth0addr for environment variable associated with egiga0 to maintain consistency with naming and used ports? Regards.. Prafulla . .
Am Fr, 7.10.2011, 10:26 schrieb Prafulla Wadaskar: >> -----Original Message----- >> From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] >> On Behalf Of Michael Walle >> Sent: Friday, October 07, 2011 3:53 AM >> To: u-boot@lists.denx.de >> Subject: [U-Boot] [PATCH] mvgbe: fix network device indices >> >> Don't assume that the MAC address of egiga0 rsp. egiga1 is ethaddr rsp. >> eth1addr. If there is only a egiga1 device, u-boot will enumerate it as >> device 0 and therefore the MAC address is set with the environmen >> varibale >> ethaddr. > > So if there is only one eth port available, the associated environment > variable should be ethaddr, this is understood. > Is there any issue with this? Are you asking me or the list? (if the question was for me, remember that there might be _only_ the egiga1 device.) > Then I have anther question in my mind- > Why not we name eth0addr for environment variable associated with egiga0 > to maintain consistency with naming and used ports? If i get it right, there is another problem with setting the environment variable from the driver itself. eth_register() and eth_init() using an own index to enumerate the devices. Eg. if an e1000 is registered before the the associated environment variable would be eth1addr for the first mvgbe device and eth2addr for the second. This patch only fixes the case where no other network device is registered. (The current behaviour just works with egiga0 being the only or the first and egiga1 being the second device).
On Thursday 06 October 2011 18:23:22 Michael Walle wrote: > --- a/net/eth.c > +++ b/net/eth.c > > +int eth_setenv_enetaddr_by_index(const char *base_name, int index, > + const uchar *enetaddr) > +{ > + char enetvar[32]; > + sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); > + return eth_setenv_enetaddr(enetvar, enetaddr); > +} please unify the duplicate logic coming from eth_getenv_enetaddr_by_index in a new local static function -mike
Am Freitag 07 Oktober 2011, 12:48:40 schrieb Michael Walle: > Am Fr, 7.10.2011, 10:26 schrieb Prafulla Wadaskar: > >> -----Original Message----- > >> From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] > >> On Behalf Of Michael Walle > >> Sent: Friday, October 07, 2011 3:53 AM > >> To: u-boot@lists.denx.de > >> Subject: [U-Boot] [PATCH] mvgbe: fix network device indices > >> > >> Don't assume that the MAC address of egiga0 rsp. egiga1 is ethaddr rsp. > >> eth1addr. If there is only a egiga1 device, u-boot will enumerate it as > >> device 0 and therefore the MAC address is set with the environmen > >> varibale > >> ethaddr. > > > > So if there is only one eth port available, the associated environment > > variable should be ethaddr, this is understood. > > Is there any issue with this? > > Are you asking me or the list? (if the question was for me, remember that > there might be _only_ the egiga1 device.) > > > Then I have anther question in my mind- > > Why not we name eth0addr for environment variable associated with egiga0 > > to maintain consistency with naming and used ports? > > If i get it right, there is another problem with setting the environment > variable from the driver itself. > > eth_register() and eth_init() using an own index to enumerate the devices. > Eg. if an e1000 is registered before the the associated environment > variable would be eth1addr for the first mvgbe device and eth2addr for the > second. > > This patch only fixes the case where no other network device is > registered. (The current behaviour just works with egiga0 being the only > or the first and egiga1 being the second device). ping :)
> -----Original Message----- > From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] > On Behalf Of Michael Walle > Sent: Friday, October 07, 2011 3:53 AM > To: u-boot@lists.denx.de > Subject: [U-Boot] [PATCH] mvgbe: fix network device indices > > Don't assume that the MAC address of egiga0 rsp. egiga1 is ethaddr rsp. > eth1addr. If there is only a egiga1 device, u-boot will enumerate it as > device 0 and therefore the MAC address is set with the environment > varibale > ethaddr. Hi If I understood it correctly, In current implementation, if only egiga1 device is enabled on the board, then it will assign MAC address associated with environment variable "ethaddr". This patch will make environment variable "egiga1" available in such case. Right? If so, then this is not the case with only mvgbe, it is applicable for all network drivers. I think this is okay, are there any issues following this standard structure at your end? Regards.. Prafulla . . .
Am Freitag 21 Oktober 2011, 10:09:15 schrieben Sie: > > -----Original Message----- > > From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] > > On Behalf Of Michael Walle > > Sent: Friday, October 07, 2011 3:53 AM > > To: u-boot@lists.denx.de > > Subject: [U-Boot] [PATCH] mvgbe: fix network device indices > > > > Don't assume that the MAC address of egiga0 rsp. egiga1 is ethaddr rsp. > > eth1addr. If there is only a egiga1 device, u-boot will enumerate it as > > device 0 and therefore the MAC address is set with the environment > > varibale > > ethaddr. > > Hi > > If I understood it correctly, > In current implementation, if only egiga1 device is enabled on the board, > then it will assign MAC address associated with environment variable > "ethaddr". yes but the current mvgbe driver will check eth1addr and set it to a random value if not set. > This patch will make environment variable "egiga1" available in such case. > Right? mh? This patch will use the same enumeration as the net/eth.c code in eth_initialize(). At least if there is no other ethernet driver than mvgbe. So ethaddr is set to a random value instead of eth1addr, which eth_initialize() then use the set the mac address. > If so, then this is not the case with only mvgbe, it is applicable for all > network drivers. yeah other drivers also set eth(N)addr sometimes, which suffers from the same problem. maybe a driver could provide some callback to initialize a macaddress or eth_register returns the device index which in turn could be used to get the proper ethNaddr environment variable.
> -----Original Message----- > From: Michael Walle [mailto:michael@walle.cc] > Sent: Wednesday, October 26, 2011 2:41 AM > To: Prafulla Wadaskar > Cc: u-boot@lists.denx.de; Mike Frysinger > Subject: Re: [U-Boot] [PATCH] mvgbe: fix network device indices > > Am Freitag 21 Oktober 2011, 10:09:15 schrieben Sie: > > > -----Original Message----- > > > From: u-boot-bounces@lists.denx.de [mailto:u-boot- > bounces@lists.denx.de] > > > On Behalf Of Michael Walle > > > Sent: Friday, October 07, 2011 3:53 AM > > > To: u-boot@lists.denx.de > > > Subject: [U-Boot] [PATCH] mvgbe: fix network device indices > > > > > > Don't assume that the MAC address of egiga0 rsp. egiga1 is > ethaddr rsp. > > > eth1addr. If there is only a egiga1 device, u-boot will > enumerate it as > > > device 0 and therefore the MAC address is set with the > environment > > > varibale > > > ethaddr. > > > > Hi > > > > If I understood it correctly, > > In current implementation, if only egiga1 device is enabled > on the board, > > then it will assign MAC address associated with environment > variable > > "ethaddr". > yes but the current mvgbe driver will check eth1addr and set it > to a random > value if not set. Yes, but this may be avoided by defining CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION as done for edminiv2 board. > > > This patch will make environment variable "egiga1" available > in such case. > > Right? > mh? This patch will use the same enumeration as the net/eth.c > code in > eth_initialize(). At least if there is no other ethernet driver > than mvgbe. > So ethaddr is set to a random value instead of eth1addr, which > eth_initialize() then use the set the mac address. > > > If so, then this is not the case with only mvgbe, it is > applicable for all > > network drivers. > yeah other drivers also set eth(N)addr sometimes, which suffers > from the same > problem. maybe a driver could provide some callback to > initialize a macaddress > or eth_register returns the device index which in turn could be > used to get > the proper ethNaddr environment variable. I think right place to provide solution for this problem is net/eth.c. The suggested change is logical but doing this will affect all u-boot users. May be some more opinion on this will be helpful. Regards.. Prafulla . . . > > -- > Michael
Am Do, 27.10.2011, 11:12 schrieb Prafulla Wadaskar: >> > If I understood it correctly, >> > In current implementation, if only egiga1 device is enabled >> on the board, >> > then it will assign MAC address associated with environment >> variable >> > "ethaddr". >> yes but the current mvgbe driver will check eth1addr and set it >> to a random >> value if not set. > > Yes, but this may be avoided by defining > CONFIG_SKIP_LOCAL_MAC_RANDOMIZATION as done for edminiv2 board. Ok, but that is not my problem ;) And even if this macro is set, the wrong environment variable will still be set (in this case not a randomized mac address, but :00:00:<devnum> suffix). >> > This patch will make environment variable "egiga1" available >> in such case. >> > Right? >> mh? This patch will use the same enumeration as the net/eth.c >> code in >> eth_initialize(). At least if there is no other ethernet driver >> than mvgbe. >> So ethaddr is set to a random value instead of eth1addr, which >> eth_initialize() then use the set the mac address. >> >> > If so, then this is not the case with only mvgbe, it is >> applicable for all >> > network drivers. >> yeah other drivers also set eth(N)addr sometimes, which suffers >> from the same >> problem. maybe a driver could provide some callback to >> initialize a macaddress >> or eth_register returns the device index which in turn could be >> used to get >> the proper ethNaddr environment variable. > > I think right place to provide solution for this problem is net/eth.c. > The suggested change is logical but doing this will affect all u-boot > users. May be some more opinion on this will be helpful. ok, so the second option still applies, that is eth_register() returns a device index. the current eth_register always returns zero, in fact all drivers in the uboot source don't check the return code. so here we could return the device index. @wolfgang: any opinion on that? or any other idea how to pass the ethernet device number to a network driver?
This patchset introduces a new element "index" within the eth_device structure, which is populated by eth_register(). The second patch uses this element to get the right name of the environment variable for the ethernet hardware address.
diff --git a/drivers/net/mvgbe.c b/drivers/net/mvgbe.c index c701f43..738e8d3 100644 --- a/drivers/net/mvgbe.c +++ b/drivers/net/mvgbe.c @@ -645,7 +645,7 @@ int mvgbe_initialize(bd_t *bis) struct mvgbe_device *dmvgbe; struct eth_device *dev; int devnum; - char *s; + int eth_idx = 0; u8 used_ports[MAX_MVGBE_DEVS] = CONFIG_MVGBE_PORTS; for (devnum = 0; devnum < MAX_MVGBE_DEVS; devnum++) { @@ -700,16 +700,13 @@ error1: /* must be less than NAMESIZE (16) */ sprintf(dev->name, "egiga%d", devnum); - /* Extract the MAC address from the environment */ switch (devnum) { case 0: dmvgbe->regs = (void *)MVGBE0_BASE; - s = "ethaddr"; break; #if defined(MVGBE1_BASE) case 1: dmvgbe->regs = (void *)MVGBE1_BASE; - s = "eth1addr"; break; #endif default: /* this should never happen */ @@ -718,7 +715,9 @@ error1: return -1; } - while (!eth_getenv_enetaddr(s, dev->enetaddr)) { + /* Extract the MAC address from the environment */ + while (!eth_getenv_enetaddr_by_index("eth", eth_idx, + dev->enetaddr)) { /* Generate Private MAC addr if not set */ dev->enetaddr[0] = 0x02; dev->enetaddr[1] = 0x50; @@ -734,7 +733,7 @@ error1: dev->enetaddr[4] = get_random_hex(); dev->enetaddr[5] = get_random_hex(); #endif - eth_setenv_enetaddr(s, dev->enetaddr); + eth_setenv_enetaddr_by_index("eth", eth_idx, dev->enetaddr); } dev->init = (void *)mvgbe_init; @@ -745,6 +744,8 @@ error1: eth_register(dev); + eth_idx++; + #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) miiphy_register(dev->name, smi_reg_read, smi_reg_write); /* Set phy address of the port */ diff --git a/include/net.h b/include/net.h index d5d37b6..d378cd2 100644 --- a/include/net.h +++ b/include/net.h @@ -103,6 +103,18 @@ extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr); extern int eth_getenv_enetaddr_by_index(const char *base_name, int index, uchar *enetaddr); +/* + * Set the hardware address for an ethernet interface. + * Args: + * base_name - base name for device (normally "eth") + * index - device index number (0 for first) + * enetaddr - returns 6 byte hardware address + * Returns: + * Return true if the environment varibable was set successfully. + */ +extern int eth_setenv_enetaddr_by_index(const char *base_name, int index, + const uchar *enetaddr); + extern int usb_eth_initialize(bd_t *bi); extern int eth_init(bd_t *bis); /* Initialize the device */ extern int eth_send(volatile void *packet, int length); /* Send a packet */ diff --git a/net/eth.c b/net/eth.c index 4280d6d..a8f68fc 100644 --- a/net/eth.c +++ b/net/eth.c @@ -62,6 +62,14 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index, return eth_getenv_enetaddr(enetvar, enetaddr); } +int eth_setenv_enetaddr_by_index(const char *base_name, int index, + const uchar *enetaddr) +{ + char enetvar[32]; + sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index); + return eth_setenv_enetaddr(enetvar, enetaddr); +} + static int eth_mac_skip(int index) { char enetvar[15];
Don't assume that the MAC address of egiga0 rsp. egiga1 is ethaddr rsp. eth1addr. If there is only a egiga1 device, u-boot will enumerate it as device 0 and therefore the MAC address is set with the environmen varibale ethaddr. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/net/mvgbe.c | 13 +++++++------ include/net.h | 12 ++++++++++++ net/eth.c | 8 ++++++++ 3 files changed, 27 insertions(+), 6 deletions(-)