Patchwork [U-Boot,v7,RESEND,5/5] usbeth: asix: Do a fast init if link already established

login
register
mail settings
Submitter Simon Glass
Date June 10, 2011, 3:04 p.m.
Message ID <1307718251-15947-6-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/99906/
State New, archived
Headers show

Comments

Simon Glass - June 10, 2011, 3:04 p.m.
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(-)
Mike Frysinger - June 10, 2011, 8:09 p.m.
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
Simon Glass - June 10, 2011, 10:22 p.m.
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
Mike Frysinger - June 11, 2011, 10:17 p.m.
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
Simon Glass - June 12, 2011, 12:01 a.m.
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

Patch

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 */