* [U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send
@ 2010-08-11 21:40 Stefano Babic
2010-08-11 22:28 ` Remy Bohmer
0 siblings, 1 reply; 5+ messages in thread
From: Stefano Babic @ 2010-08-11 21:40 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send
2010-08-11 21:40 [U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send Stefano Babic
@ 2010-08-11 22:28 ` Remy Bohmer
2010-08-12 8:44 ` Stefano Babic
0 siblings, 1 reply; 5+ messages in thread
From: Remy Bohmer @ 2010-08-11 22:28 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send
2010-08-11 22:28 ` Remy Bohmer
@ 2010-08-12 8:44 ` Stefano Babic
2010-08-12 9:52 ` Remy Bohmer
0 siblings, 1 reply; 5+ messages in thread
From: Stefano Babic @ 2010-08-12 8:44 UTC (permalink / raw)
To: u-boot
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
=====================================================================
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send
2010-08-12 8:44 ` Stefano Babic
@ 2010-08-12 9:52 ` Remy Bohmer
2010-08-12 12:33 ` Stefano Babic
0 siblings, 1 reply; 5+ messages in thread
From: Remy Bohmer @ 2010-08-12 9:52 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send
2010-08-12 9:52 ` Remy Bohmer
@ 2010-08-12 12:33 ` Stefano Babic
0 siblings, 0 replies; 5+ messages in thread
From: Stefano Babic @ 2010-08-12 12:33 UTC (permalink / raw)
To: u-boot
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
=====================================================================
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-08-12 12:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-11 21:40 [U-Boot] [PATCH] USB-CDC: called handle_interrupts inside usb_eth_send Stefano Babic
2010-08-11 22:28 ` Remy Bohmer
2010-08-12 8:44 ` Stefano Babic
2010-08-12 9:52 ` Remy Bohmer
2010-08-12 12:33 ` Stefano Babic
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.