| Message ID | 1307718251-15947-6-git-send-email-sjg@chromium.org |
|---|---|
| State | New, archived |
| Headers | show |
On Friday, June 10, 2011 11:04:11 Simon Glass wrote: > The Asix driver takes the link down during init() and then brings it back > up. This commit changes this so that if a link has already been > established successfully we simply check that the link is still good. > > This reduces the delay between successive network commands. i'm not sure about this. i think this is a general issue to all net devices and should be resolved there rather than making diff net drivers behave differently based on how the maintainer felt like going. i also vaguely recall there being discussions on this topic recently. -mike
On Fri, Jun 10, 2011 at 1:09 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Friday, June 10, 2011 11:04:11 Simon Glass wrote: >> The Asix driver takes the link down during init() and then brings it back >> up. This commit changes this so that if a link has already been >> established successfully we simply check that the link is still good. >> >> This reduces the delay between successive network commands. > > i'm not sure about this. i think this is a general issue to all net devices > and should be resolved there rather than making diff net drivers behave > differently based on how the maintainer felt like going. > > i also vaguely recall there being discussions on this topic recently. > -mike > Hi Mike, OK perhaps my understanding is incorrect. I saw discussion about the SMSC driver and whether half should do something there. I changed it to stop packet reception. In my view it is correct for it to stop packet reception but keep the link up. Bringing the link down and up takes a few seconds, which adds delay between bootp and tftp. We don't want that. Regards, Simon
On Friday, June 10, 2011 18:22:06 Simon Glass wrote: > On Fri, Jun 10, 2011 at 1:09 PM, Mike Frysinger <vapier@gentoo.org> wrote: > > On Friday, June 10, 2011 11:04:11 Simon Glass wrote: > >> The Asix driver takes the link down during init() and then brings it > >> back up. This commit changes this so that if a link has already been > >> established successfully we simply check that the link is still good. > >> > >> This reduces the delay between successive network commands. > > > > i'm not sure about this. i think this is a general issue to all net > > devices and should be resolved there rather than making diff net drivers > > behave differently based on how the maintainer felt like going. > > > > i also vaguely recall there being discussions on this topic recently. > > OK perhaps my understanding is incorrect. > > I saw discussion about the SMSC driver and whether half should do > something there. I changed it to stop packet reception. > > In my view it is correct for it to stop packet reception but keep the > link up. Bringing the link down and up takes a few seconds, which adds > delay between bootp and tftp. We don't want that. yes, but i'm saying that this needs to be decided at a higher level and then documented rather than changing diff drivers to behave differently. i'm not against making changes here to make things operate more smoothly, but i am against undocumented fragmentation, especially when the networking layer in u- boot is already a confusing mine field for people. the current behavior is for the device to be completely taken down at halt() and completely brought up at init() time. you're proposing something else, so let's do it as a proper proposal and not a single driver change. -mike
On Sat, Jun 11, 2011 at 3:17 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Friday, June 10, 2011 18:22:06 Simon Glass wrote: >> On Fri, Jun 10, 2011 at 1:09 PM, Mike Frysinger <vapier@gentoo.org> wrote: >> > On Friday, June 10, 2011 11:04:11 Simon Glass wrote: >> >> The Asix driver takes the link down during init() and then brings it >> >> back up. This commit changes this so that if a link has already been >> >> established successfully we simply check that the link is still good. >> >> >> >> This reduces the delay between successive network commands. >> > >> > i'm not sure about this. i think this is a general issue to all net >> > devices and should be resolved there rather than making diff net drivers >> > behave differently based on how the maintainer felt like going. >> > >> > i also vaguely recall there being discussions on this topic recently. >> >> OK perhaps my understanding is incorrect. >> >> I saw discussion about the SMSC driver and whether half should do >> something there. I changed it to stop packet reception. >> >> In my view it is correct for it to stop packet reception but keep the >> link up. Bringing the link down and up takes a few seconds, which adds >> delay between bootp and tftp. We don't want that. > > yes, but i'm saying that this needs to be decided at a higher level and then > documented rather than changing diff drivers to behave differently. i'm not > against making changes here to make things operate more smoothly, but i am > against undocumented fragmentation, especially when the networking layer in u- > boot is already a confusing mine field for people. > > the current behavior is for the device to be completely taken down at halt() > and completely brought up at init() time. you're proposing something else, so > let's do it as a proper proposal and not a single driver change. > -mike > Hi Mike, Yes I do understand. The Atmel macb driver sees that a link is already up, doesn't force a renegotiate and just turns off packet transfer in the halt() method. That was the behavior I was modelling. It might be reasonable to declare that halt may keep the link up if it wants to, and that there is then no way to bring the link down. Or perhaps we could add two new methods: - start = bring up the link if not already up, and get ready for transmission - stop = stop transmission/reception but don't bring down the link These could be skipped if not defined for each driver: - start can simply be skipped, on the assumption that init has set up the link - stop, if missing, then U-Boot must call halt() instead, then init next time around There may be some desire for an explicit command to change the state of an interface, like 'eth stop n' or 'eth halt n'. If this suits I am happy to create a patch for it, or something along these lines, so long as it doesn't turn into a timer-scale epic. Note that USB is a little different here. WIth a directly-attached ethernet, power is generally applied by the hardware on boot, and the PHY will often pick up a link before U-Boot comes to call the Ethernet init() routine. This works well and I don't want to change it. For USB unfortunately everything is off until power is applied to the adapter via the normal USB approach, complete with 500ms delays, etc. We can't get around this, but certainly don't want to be removing and reapplying power to the adapter between each network operation. It takes ages. I even consider it desirable to provide an option to pre-init any USB ethernet adapters found, while scanning the rest of the USB bus. Anyway I will drop this patch from the series and carry it locally for now. I await your comments. Regards, Simon
diff --git a/drivers/usb/eth/asix.c b/drivers/usb/eth/asix.c index 9b012e4..18e5457 100644 --- a/drivers/usb/eth/asix.c +++ b/drivers/usb/eth/asix.c @@ -307,20 +307,12 @@ static int mii_nway_restart(struct ueth_data *dev) return r; } -/* - * Asix callbacks - */ -static int asix_init(struct eth_device *eth, bd_t *bd) +static int full_init(struct eth_device *eth) { + struct ueth_data *dev = (struct ueth_data *)eth->priv; int embd_phy; unsigned char buf[ETH_ALEN]; u16 rx_ctl; - struct ueth_data *dev = (struct ueth_data *)eth->priv; - int timeout = 0; -#define TIMEOUT_RESOLUTION 50 /* ms */ - int link_detected; - - debug("** %s()\n", __func__); if (asix_write_gpio(dev, AX_GPIO_RSE | AX_GPIO_GPO_2 | AX_GPIO_GPO2EN, 5) < 0) @@ -395,6 +387,25 @@ static int asix_init(struct eth_device *eth, bd_t *bd) if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL) < 0) goto out_err; + return 0; +out_err: + return -1; +} + +/* + * Asix callbacks + */ +static int asix_init(struct eth_device *eth, bd_t *bd) +{ + struct ueth_data *dev = (struct ueth_data *)eth->priv; + int timeout = 0; +#define TIMEOUT_RESOLUTION 50 /* ms */ + int link_detected; + + debug("** %s()\n", __func__); + + if (!dev->has_been_running && full_init(eth)) + return -1; do { link_detected = asix_mdio_read(dev, dev->phy_id, MII_BMSR) & @@ -411,12 +422,11 @@ static int asix_init(struct eth_device *eth, bd_t *bd) printf("done.\n"); } else { printf("unable to connect.\n"); - goto out_err; + return -1; } + dev->has_been_running = 1; return 0; -out_err: - return -1; } static int asix_send(struct eth_device *eth, volatile void *packet, int length) diff --git a/include/usb_ether.h b/include/usb_ether.h index a7fb26b..73b085d 100644 --- a/include/usb_ether.h +++ b/include/usb_ether.h @@ -37,8 +37,9 @@ struct ueth_data { /* eth info */ - struct eth_device eth_dev; /* used with eth_register */ - int phy_id; /* mii phy id */ + struct eth_device eth_dev; /* used with eth_register */ + int phy_id; /* mii phy id */ + int has_been_running; /* 1 if we have had a link up */ /* usb info */ struct usb_device *pusb_dev; /* this usb_device */
The Asix driver takes the link down during init() and then brings it back up. This commit changes this so that if a link has already been established successfully we simply check that the link is still good. This reduces the delay between successive network commands. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/usb/eth/asix.c | 36 +++++++++++++++++++++++------------- include/usb_ether.h | 5 +++-- 2 files changed, 26 insertions(+), 15 deletions(-)