| Message ID | 1302748176-4324-2-git-send-email-sjg@chromium.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
Hi, 2011/4/14 Simon Glass <sjg@chromium.org>: > Built-in Ethernet adapters support setting the mac address by means of a > ethaddr environment variable for each interface (ethaddr, eth1addr, eth2addr). > > This adds similar support to the USB network side, using the names > usbethaddr, usbeth1addr, etc. They are kept separate since we don't want > a USB device taking the MAC address of a built-in device or vice versa. > > TEST=build, test on harmony, with setenv usbethaddr c0:c1:c0:13:0b:b8, bootp, tftp ... > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > board/davinci/common/misc.c | 2 +- > drivers/net/designware.c | 2 +- > drivers/usb/eth/usb_ether.c | 9 ++++- > include/net.h | 25 ++++++++++++++++- > net/eth.c | 65 ++++++++++++++++++++++++++---------------- 3 subsystems within 1 single patch (net + usb + davinci-boards). How are we gonna deal with it? Since the remainder of the series is USB tree related, I would suggest to include it in the u-boot-usb tree, but then I need the Ack of Ben and Sandeep. Kind regards, Remy
Dear Simon Glass, In message <1302748176-4324-2-git-send-email-sjg@chromium.org> you wrote: > Built-in Ethernet adapters support setting the mac address by means of a > ethaddr environment variable for each interface (ethaddr, eth1addr, eth2addr). > > This adds similar support to the USB network side, using the names > usbethaddr, usbeth1addr, etc. They are kept separate since we don't want > a USB device taking the MAC address of a built-in device or vice versa. > > TEST=build, test on harmony, with setenv usbethaddr c0:c1:c0:13:0b:b8, bootp, tftp ... > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > board/davinci/common/misc.c | 2 +- > drivers/net/designware.c | 2 +- > drivers/usb/eth/usb_ether.c | 9 ++++- > include/net.h | 25 ++++++++++++++++- > net/eth.c | 65 ++++++++++++++++++++++++++---------------- > 5 files changed, 73 insertions(+), 30 deletions(-) ... > diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c > index 7b55da3..6565ea5 100644 > --- a/drivers/usb/eth/usb_ether.c > +++ b/drivers/usb/eth/usb_ether.c > @@ -80,6 +80,7 @@ int is_eth_dev_on_usb_host(void) > */ > static void probe_valid_drivers(struct usb_device *dev) > { > + struct eth_device *eth; > int j; > > for (j = 0; prob_dev[j].probe && prob_dev[j].get_info; j++) { > @@ -88,9 +89,10 @@ static void probe_valid_drivers(struct usb_device *dev) > /* > * ok, it is a supported eth device. Get info and fill it in > */ > + eth = &usb_eth[usb_max_eth_dev].eth_dev; Index for eth is usb_max_eth_dev. > @@ -100,7 +102,10 @@ static void probe_valid_drivers(struct usb_device *dev) > * call since eth_current_changed (internally called) > * relies on it > */ > - eth_register(&usb_eth[usb_max_eth_dev - 1].eth_dev); > + eth_register(eth); You change the behaviour here. Please confirmt his is really intentional. > diff --git a/include/net.h b/include/net.h > index 95ef8ab..9256a46 100644 > --- a/include/net.h > +++ b/include/net.h > @@ -123,7 +123,18 @@ extern int eth_get_dev_index (void); /* get the device index */ > extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr); > extern int eth_getenv_enetaddr(char *name, uchar *enetaddr); > extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr); > -extern int eth_getenv_enetaddr_by_index(int index, uchar *enetaddr); > + > +/* > + * Get the hardware address for an ethernet interface . > + * Args: > + * base_name - base name for device (NULL for "eth") This is an atitifical decision for the API which is difficult to understand. It just makes the code and understanding it more difficult. Just pass "eth" when you mean it. > +int eth_write_hwaddr(struct eth_device *dev, const char *base_name, > + int eth_number) > +{ > + unsigned char env_enetaddr[6]; > + int ret = 0; > + > + eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr); Add error handling, or write (void)eth_getenv_enetaddr_by_index(...); to indicate that you intentionally ignore the return value. Best regards, Wolfgang Denk
Dear Remy Bohmer, In message <BANLkTinCWW+B2bWJpWFQQU_dki0D50XEeg@mail.gmail.com> you wrote: > > > board/davinci/common/misc.c | 2 +- > > drivers/net/designware.c | 2 +- > > drivers/usb/eth/usb_ether.c | 9 ++++- > > include/net.h | 25 ++++++++++++++++- > > net/eth.c | 65 +++++++++++++++= > +++++++++++---------------- > > 3 subsystems within 1 single patch (net + usb + davinci-boards). How > are we gonna deal with it? > Since the remainder of the series is USB tree related, I would suggest > to include it in the u-boot-usb tree, but then I need the Ack of Ben > and Sandeep. I'm standing in for the network custodian as long as we haven't found a better one. I think it will be indeed the best to pull this though your tree. But I just requested some more changes. Best regards, Wolfgang Denk
On Mon, Apr 18, 2011 at 12:04 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Simon Glass, > > In message <1302748176-4324-2-git-send-email-sjg@chromium.org> you wrote: >> @@ -88,9 +89,10 @@ static void probe_valid_drivers(struct usb_device *dev) >> /* >> * ok, it is a supported eth device. Get info and fill it in >> */ >> + eth = &usb_eth[usb_max_eth_dev].eth_dev; > > Index for eth is usb_max_eth_dev. > >> @@ -100,7 +102,10 @@ static void probe_valid_drivers(struct usb_device *dev) >> * call since eth_current_changed (internally called) >> * relies on it >> */ >> - eth_register(&usb_eth[usb_max_eth_dev - 1].eth_dev); >> + eth_register(eth); > > You change the behaviour here. Please confirmt his is really > intentional. Yes. Since I am using an 'eth' pointer I don't need to index the array again. The behaviour is the same as before. > >> diff --git a/include/net.h b/include/net.h >> @@ -123,7 +123,18 @@ extern int eth_get_dev_index (void); /* get the device index */ >> +/* >> + * Get the hardware address for an ethernet interface . >> + * Args: >> + * base_name - base name for device (NULL for "eth") > > This is an atitifical decision for the API which is difficult to > understand. It just makes the code and understanding it more > difficult. Just pass "eth" when you mean it. The intention was to avoid everyone having to pass the correct value - potential for error, etc. I could have created a #define, but decided on this. I have changed it as you request. > >> +int eth_write_hwaddr(struct eth_device *dev, const char *base_name, >> + int eth_number) >> +{ >> + unsigned char env_enetaddr[6]; >> + int ret = 0; >> + >> + eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr); > > Add error handling, or write > > (void)eth_getenv_enetaddr_by_index(...); > > to indicate that you intentionally ignore the return value. Done > > Best regards, > > Wolfgang Denk Regards, Simon
Dear Simon Glass, In message <BANLkTikGucjpun2RhS2T2Nyq4_KB9gK8zw@mail.gmail.com> you wrote: > > >> + eth = &usb_eth[usb_max_eth_dev].eth_dev; > > > > Index for eth is usb_max_eth_dev. > > > >> @@ -100,7 +102,10 @@ static void probe_valid_drivers(struct usb_device *> dev) > >> * call since eth_current_> changed (internally called) > >> * relies on it > >> */ > >> - eth_register(&usb_eth[usb_max_eth_dev - 1].eth_dev); > >> + eth_register(eth); > > > > You change the behaviour here. Please confirmt his is really > > intentional. > > Yes. Since I am using an 'eth' pointer I don't need to index the array > again. The behaviour is the same as before. No, it is not. Before, we were accessing entry N-1 here. Now we use entry N. usb_max_eth_dev != usb_max_eth_dev - 1 > >> + * base_name - base name for device (NULL for "eth") > > > > This is an atitifical decision for the API which is difficult to > > understand. It just makes the code and understanding it more > > difficult. Just pass "eth" when you mean it. > > The intention was to avoid everyone having to pass the correct value - > potential for error, etc. I could have created a #define, but decided > on this. Ummm... but having everyone to pass the correct value is actually a really good thing to have! Best regards, Wolfgang Denk
On Thu, Apr 21, 2011 at 4:38 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Simon Glass, > > In message <BANLkTikGucjpun2RhS2T2Nyq4_KB9gK8zw@mail.gmail.com> you wrote: >> >> >> + eth = &usb_eth[usb_max_eth_dev].eth_dev; >> > >> > Index for eth is usb_max_eth_dev. >> > >> >> @@ -100,7 +102,10 @@ static void probe_valid_drivers(struct usb_device *> dev) >> >> * call since eth_current_> changed (internally called) >> >> * relies on it >> >> */ >> >> - eth_register(&usb_eth[usb_max_eth_dev - 1].eth_dev); >> >> + eth_register(eth); >> > >> > You change the behaviour here. Please confirmt his is really >> > intentional. >> >> Yes. Since I am using an 'eth' pointer I don't need to index the array >> again. The behaviour is the same as before. > > No, it is not. Before, we were accessing entry N-1 here. Now we use > entry N. usb_max_eth_dev != usb_max_eth_dev - 1 Hi Wolfgang, Hmmm. If you see the usb_max_eth_dev++ below, it is increasing the index variable to keep eth_register() happy. I have kept that behaviour. So let's say usb_max_eth_dev is 3. The sequence is: - set eth to point to item 3 - increase usb_max_eth_dev to 4 as required by eth_register() - call eth_register() with item 3 (i.e. eth pointer hasn't changed) - call eth_write_hwaddr with item 3 Maybe look at the whole code with my patch applied? Let me know if I am missing something. + eth)) { /* found proper driver */ /* register with networking stack */ usb_max_eth_dev++; @@ -100,7 +102,10 @@ static void probe_valid_drivers(struct usb_device *dev) * call since eth_current_changed (internally called) * relies on it */ - eth_register(&usb_eth[usb_max_eth_dev - 1].eth_dev); + eth_register(eth); + if (eth_write_hwaddr(eth, "usbeth", + usb_max_eth_dev - 1)) + puts("Warning: failed to set MAC address\n"); break; } > >> >> + * base_name - base name for device (NULL for "eth") >> > >> > This is an atitifical decision for the API which is difficult to >> > understand. It just makes the code and understanding it more >> > difficult. Just pass "eth" when you mean it. >> >> The intention was to avoid everyone having to pass the correct value - >> potential for error, etc. I could have created a #define, but decided >> on this. > > Ummm... but having everyone to pass the correct value is actually a > really good thing to have! OK fair enough, it is in there now. Regards, Simon
On Thu, Apr 21, 2011 at 6:52 PM, Simon Glass <sjg@chromium.org> wrote: > On Thu, Apr 21, 2011 at 4:38 PM, Wolfgang Denk <wd@denx.de> wrote: > > Dear Simon Glass, > > > > In message <BANLkTikGucjpun2RhS2T2Nyq4_KB9gK8zw@mail.gmail.com> you > wrote: > >> > >> >> + eth = &usb_eth[usb_max_eth_dev].eth_dev; > >> > > >> > Index for eth is usb_max_eth_dev. > >> > > >> >> @@ -100,7 +102,10 @@ static void probe_valid_drivers(struct > usb_device *> dev) > >> >> * call since eth_current_> changed (internally > called) > >> >> * relies on it > >> >> */ > >> >> - eth_register(&usb_eth[usb_max_eth_dev - > 1].eth_dev); > >> >> + eth_register(eth); > >> > > >> > You change the behaviour here. Please confirmt his is really > >> > intentional. > >> > >> Yes. Since I am using an 'eth' pointer I don't need to index the array > >> again. The behaviour is the same as before. > > > > No, it is not. Before, we were accessing entry N-1 here. Now we use > > entry N. usb_max_eth_dev != usb_max_eth_dev - 1 > > Hi Wolfgang, > > Hmmm. If you see the usb_max_eth_dev++ below, it is increasing the > index variable to keep eth_register() happy. I have kept that > behaviour. > > So let's say usb_max_eth_dev is 3. The sequence is: > > - set eth to point to item 3 > - increase usb_max_eth_dev to 4 as required by eth_register() > - call eth_register() with item 3 (i.e. eth pointer hasn't changed) > - call eth_write_hwaddr with item 3 > > Maybe look at the whole code with my patch applied? > > Let me know if I am missing something. > > Hi Wolfgang, Please can you let me know where we stand with this patch? It is a bit opaque and if you don't mind me reaching a bit further I might be able to clean it up by changing eth_register(). Thanks, Simon
On Thursday, April 21, 2011 19:38:06 Wolfgang Denk wrote: > Simon Glass wrote: > > >> + * base_name - base name for device (NULL for "eth") > > > > > > This is an atitifical decision for the API which is difficult to > > > understand. It just makes the code and understanding it more > > > difficult. Just pass "eth" when you mean it. > > > > The intention was to avoid everyone having to pass the correct value - > > potential for error, etc. I could have created a #define, but decided > > on this. > > Ummm... but having everyone to pass the correct value is actually a > really good thing to have! i'm not sure about that. it just means it's easier for people to screw it up, or for duplicated code to propagate via copy & paste. the current API Simon has makes sense to me. -mike
diff --git a/board/davinci/common/misc.c b/board/davinci/common/misc.c index 2bfdf23..b2b980b 100644 --- a/board/davinci/common/misc.c +++ b/board/davinci/common/misc.c @@ -101,7 +101,7 @@ void davinci_sync_env_enetaddr(uint8_t *rom_enetaddr) { uint8_t env_enetaddr[6]; - eth_getenv_enetaddr_by_index(0, env_enetaddr); + eth_getenv_enetaddr_by_index(NULL, 0, env_enetaddr); if (!memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) { /* There is no MAC address in the environment, so we initialize * it from the value in the EEPROM. */ diff --git a/drivers/net/designware.c b/drivers/net/designware.c index 3f5eeb7..5e0f9e3 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -500,7 +500,7 @@ int designware_initialize(u32 id, ulong base_addr, u32 phy_addr) dev->iobase = (int)base_addr; dev->priv = priv; - eth_getenv_enetaddr_by_index(id, &dev->enetaddr[0]); + eth_getenv_enetaddr_by_index(NULL, id, &dev->enetaddr[0]); priv->dev = dev; priv->mac_regs_p = (struct eth_mac_regs *)base_addr; diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c index 7b55da3..6565ea5 100644 --- a/drivers/usb/eth/usb_ether.c +++ b/drivers/usb/eth/usb_ether.c @@ -80,6 +80,7 @@ int is_eth_dev_on_usb_host(void) */ static void probe_valid_drivers(struct usb_device *dev) { + struct eth_device *eth; int j; for (j = 0; prob_dev[j].probe && prob_dev[j].get_info; j++) { @@ -88,9 +89,10 @@ static void probe_valid_drivers(struct usb_device *dev) /* * ok, it is a supported eth device. Get info and fill it in */ + eth = &usb_eth[usb_max_eth_dev].eth_dev; if (prob_dev[j].get_info(dev, &usb_eth[usb_max_eth_dev], - &usb_eth[usb_max_eth_dev].eth_dev)) { + eth)) { /* found proper driver */ /* register with networking stack */ usb_max_eth_dev++; @@ -100,7 +102,10 @@ static void probe_valid_drivers(struct usb_device *dev) * call since eth_current_changed (internally called) * relies on it */ - eth_register(&usb_eth[usb_max_eth_dev - 1].eth_dev); + eth_register(eth); + if (eth_write_hwaddr(eth, "usbeth", + usb_max_eth_dev - 1)) + puts("Warning: failed to set MAC address\n"); break; } } diff --git a/include/net.h b/include/net.h index 95ef8ab..9256a46 100644 --- a/include/net.h +++ b/include/net.h @@ -123,7 +123,18 @@ extern int eth_get_dev_index (void); /* get the device index */ extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr); extern int eth_getenv_enetaddr(char *name, uchar *enetaddr); extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr); -extern int eth_getenv_enetaddr_by_index(int index, uchar *enetaddr); + +/* + * Get the hardware address for an ethernet interface . + * Args: + * base_name - base name for device (NULL for "eth") + * index - device index number (0 for first) + * enetaddr - returns 6 byte hardware address + * Returns: + * Return true if the address is valid. + */ +extern int eth_getenv_enetaddr_by_index(const char *base_name, int index, + uchar *enetaddr); extern int usb_eth_initialize(bd_t *bi); extern int eth_init(bd_t *bis); /* Initialize the device */ @@ -136,6 +147,18 @@ extern int eth_rx(void); /* Check for received packets */ extern void eth_halt(void); /* stop SCC */ extern char *eth_get_name(void); /* get name of current device */ +/* + * Set the hardware address for an ethernet interface based on 'eth%daddr' + * environment variable (or just 'ethaddr' if eth_number is 0). + * Args: + * base_name - base name for device (NULL for "eth") + * eth_number - value of %d (0 for first device of this type) + * Returns: + * 0 is success, non-zero is error status from driver. + */ +int eth_write_hwaddr(struct eth_device *dev, const char *base_name, + int eth_number); + #ifdef CONFIG_MCAST_TFTP int eth_mcast_join( IPaddr_t mcast_addr, u8 join); u32 ether_crc (size_t len, unsigned char const *p); diff --git a/net/eth.c b/net/eth.c index 3a7ff50..51ffbee 100644 --- a/net/eth.c +++ b/net/eth.c @@ -53,10 +53,13 @@ int eth_setenv_enetaddr(char *name, const uchar *enetaddr) return setenv(name, buf); } -int eth_getenv_enetaddr_by_index(int index, uchar *enetaddr) +int eth_getenv_enetaddr_by_index(const char *base_name, int index, + uchar *enetaddr) { char enetvar[32]; - sprintf(enetvar, index ? "eth%daddr" : "ethaddr", index); + sprintf(enetvar, index ? "%s%daddr" : "%saddr", + base_name ? base_name : "eth", + index); return eth_getenv_enetaddr(enetvar, enetaddr); } @@ -187,6 +190,37 @@ static void eth_current_changed(void) #endif } +int eth_write_hwaddr(struct eth_device *dev, const char *base_name, + int eth_number) +{ + unsigned char env_enetaddr[6]; + int ret = 0; + + eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr); + + if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) { + if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) && + memcmp(dev->enetaddr, env_enetaddr, 6)) { + printf("\nWarning: %s MAC addresses don't match:\n", + dev->name); + printf("Address in SROM is %pM\n", + dev->enetaddr); + printf("Address in environment is %pM\n", + env_enetaddr); + } + + memcpy(dev->enetaddr, env_enetaddr, 6); + } + + if (dev->write_hwaddr && + !eth_mac_skip(eth_number) && + is_valid_ether_addr(dev->enetaddr)) { + ret = dev->write_hwaddr(dev); + } + + return ret; +} + int eth_register(struct eth_device *dev) { struct eth_device *d; @@ -207,7 +241,6 @@ int eth_register(struct eth_device *dev) int eth_initialize(bd_t *bis) { - unsigned char env_enetaddr[6]; int eth_number = 0; eth_devices = NULL; @@ -258,27 +291,8 @@ int eth_initialize(bd_t *bis) if (strchr(dev->name, ' ')) puts("\nWarning: eth device name has a space!\n"); - eth_getenv_enetaddr_by_index(eth_number, env_enetaddr); - - if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) { - if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) && - memcmp(dev->enetaddr, env_enetaddr, 6)) - { - printf ("\nWarning: %s MAC addresses don't match:\n", - dev->name); - printf ("Address in SROM is %pM\n", - dev->enetaddr); - printf ("Address in environment is %pM\n", - env_enetaddr); - } - - memcpy(dev->enetaddr, env_enetaddr, 6); - } - if (dev->write_hwaddr && - !eth_mac_skip(eth_number) && - is_valid_ether_addr(dev->enetaddr)) { - dev->write_hwaddr(dev); - } + if (eth_write_hwaddr(dev, NULL, eth_number)) + puts("Warning: failed to set MAC address\n"); eth_number++; dev = dev->next; @@ -353,7 +367,8 @@ int eth_init(bd_t *bis) do { uchar env_enetaddr[6]; - if (eth_getenv_enetaddr_by_index(eth_number, env_enetaddr)) + if (eth_getenv_enetaddr_by_index(NULL, eth_number, + env_enetaddr)) memcpy(dev->enetaddr, env_enetaddr, 6); ++eth_number;
Built-in Ethernet adapters support setting the mac address by means of a ethaddr environment variable for each interface (ethaddr, eth1addr, eth2addr). This adds similar support to the USB network side, using the names usbethaddr, usbeth1addr, etc. They are kept separate since we don't want a USB device taking the MAC address of a built-in device or vice versa. TEST=build, test on harmony, with setenv usbethaddr c0:c1:c0:13:0b:b8, bootp, tftp ... Signed-off-by: Simon Glass <sjg@chromium.org> --- board/davinci/common/misc.c | 2 +- drivers/net/designware.c | 2 +- drivers/usb/eth/usb_ether.c | 9 ++++- include/net.h | 25 ++++++++++++++++- net/eth.c | 65 ++++++++++++++++++++++++++---------------- 5 files changed, 73 insertions(+), 30 deletions(-)