[U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send

[U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send

[Stefano Babic]

The patch removes an endless loop  in the usb_eth_send
if the tx_complete is not called before going
in the loop. The driver interrupt routine is called
allowing the driver to check if the TX is completed.

Signed-off-by: Stefano Babic <sbabic@denx.de>
---
 drivers/usb/gadget/ether.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
index 1481d76..9eb43d7 100644
--- a/drivers/usb/gadget/ether.c
+++ b/drivers/usb/gadget/ether.c
@@ -1808,6 +1808,8 @@ static int usb_eth_send(struct eth_device* netdev, volatile void* packet, int le
 	struct usb_request	*req = NULL;
 
 	struct eth_dev *dev = &l_ethdev;
+	unsigned long ts;
+	unsigned long timeout = USB_CONNECT_TIMEOUT;
 	dprintf("%s:...\n",__func__);
 
 	req = dev->tx_req;
@@ -1837,9 +1839,15 @@ static int usb_eth_send(struct eth_device* netdev, volatile void* packet, int le
 
 	if (!retval)
 		dprintf("%s: packet queued\n",__func__);
+	ts = get_timer(0);
+	packet_sent=0;
 	while(!packet_sent)
 	{
-		packet_sent=0;
+		if (get_timer(ts) > timeout) {
+			printf ("timeout sending packets to usb ethernet\n");
+			return -1;
+		}
+		usb_gadget_handle_interrupts();
 	}
 
 	return 0;
-- 
1.6.3.3

[U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send

[Remy Bohmer]

Hi Stefano,

2010/8/11 Stefano Babic <sbabic@denx.de>:
> The patch removes an endless loop ?in the usb_eth_send
> if the tx_complete is not called before going
> in the loop. The driver interrupt routine is called
> allowing the driver to check if the TX is completed.
>
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> ---
> ?drivers/usb/gadget/ether.c | ? 10 +++++++++-
> ?1 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
> index 1481d76..9eb43d7 100644
> --- a/drivers/usb/gadget/ether.c
> +++ b/drivers/usb/gadget/ether.c
> + ? ? ? ts = get_timer(0);

> + ? ? ? packet_sent=0;
This line breaks the at91sam9261 code. Can you please fix this?

Rest of the patches do not seem to break anything and look okay to me.
Thanks.

Kind regards,

Remy

[U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send

[Stefano Babic]

Remy Bohmer wrote:
> Hi Stefano,
> 

Hi Remy,

>> +       packet_sent=0;
> This line breaks the at91sam9261 code. Can you please fix this?

This means to me that tx_complete() is not called for at91sam9261, as it
supposed to be (or better, as I supposed..).

I see that packet_sent is reset only in the eth_init() function. Does it
mean that usb_eth_send() does not wait for tx completion in the case of
 at91sam9261 ?

I supposed (and I manage in this way..) that packet_sent is used as flag
to detect when a single packet is really transmitted, and in this way is
used when I call usb_handle_interrupts(). The callback stored into the
USB request is tx_complete() that is the one responsible to set the
flag. So the simple mechanism is that usb_eth_send clear the flag and
wait until tx_complete sets it. It seems now to me that on at91sam9261
usb_eth_send() waits only for the first packet, and then it does not
wait anymore. How can we be sure that the packet is really transmitted ?

Should I protect the line with some AT91 CONFIG or am I missing something ?

Best regards,
Stefano


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send

[Remy Bohmer]

Hi Stefano,

2010/8/12 Stefano Babic <sbabic@denx.de>:
> Remy Bohmer wrote:
>> Hi Stefano,
>>
>
> Hi Remy,
>
>>> + ? ? ? packet_sent=0;
>> This line breaks the at91sam9261 code. Can you please fix this?
>
> This means to me that tx_complete() is not called for at91sam9261, as it
> supposed to be (or better, as I supposed..).
>
> I see that packet_sent is reset only in the eth_init() function. Does it
> mean that usb_eth_send() does not wait for tx completion in the case of
> ?at91sam9261 ?

Looking at this I agree that this packet_sent flag should be reset on
every usb_eth_send().

> I supposed (and I manage in this way..) that packet_sent is used as flag
> to detect when a single packet is really transmitted, and in this way is
> used when I call usb_handle_interrupts(). The callback stored into the
> USB request is tx_complete() that is the one responsible to set the
> flag. So the simple mechanism is that usb_eth_send clear the flag and
> wait until tx_complete sets it. It seems now to me that on at91sam9261
> usb_eth_send() waits only for the first packet, and then it does not
> wait anymore. How can we be sure that the packet is really transmitted ?
>
> Should I protect the line with some AT91 CONFIG or am I missing something ?

No, it should be AT91 specific...

Looking at the flow on at91 we see that this happens:
ether.c:usb_eth_send()
	gadget.h:usb_ep_queue()
		at91_udc.c:at91_ep_queue()
			at91_udc.c:write_fifo()
				at91_udc.c:done()
					req->req.complete()
						tx_complete()
							packet_send = 1

The packet is transmitted in the context of the usb_ep_queue()
routine, not in the context of the interrupt handler.
So, the timeout should incorporate usb_ep_queue also.

I think this is better:
       ts = get_timer(0);
       packet_sent=0;
       retval = usb_ep_queue (dev->in_ep, req, GFP_ATOMIC);

compared to:
       ts = get_timer(0);
       packet_sent=0;
       while(!packet_sent)

But I must mention that I have not yet tested it though...

Kind regards,

Remy

[U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send

[Stefano Babic]

Remy Bohmer wrote:

> Looking at this I agree that this packet_sent flag should be reset on
> every usb_eth_send().

Ok

>> Should I protect the line with some AT91 CONFIG or am I missing something ?
> 
> No, it should be AT91 specific...
> 
> Looking at the flow on at91 we see that this happens:
> ether.c:usb_eth_send()
> 	gadget.h:usb_ep_queue()
> 		at91_udc.c:at91_ep_queue()
> 			at91_udc.c:write_fifo()
> 				at91_udc.c:done()
> 					req->req.complete()
> 						tx_complete()
> 							packet_send = 1
> 

Ok, understood.

> The packet is transmitted in the context of the usb_ep_queue()
> routine, not in the context of the interrupt handler.
> So, the timeout should incorporate usb_ep_queue also.
> 
> I think this is better:
>        ts = get_timer(0);
>        packet_sent=0;
>        retval = usb_ep_queue (dev->in_ep, req, GFP_ATOMIC);
> 
> compared to:
>        ts = get_timer(0);
>        packet_sent=0;
>        while(!packet_sent)
> 

Yes, agree, it should be enough to call usb_ep_queue after clearing the
flag as you suggest.

> But I must mention that I have not yet tested it though...

It does not matter. I will repost the whole patch series with the
comments we get up now. After that we can do some more tests to check if
I break something...

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================