[Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Ming Lei]

Hi All,

In another thread, Sergei pointed out there is a ZLP issue in musb_g_tx:

Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
> On 15-09-2010 14:05, Felipe Balbi wrote:
>
> >> I didn't say it was duplicate for DMA, just too late.
>
> > how come ? we need to send ZLP before giving back the request.
>
>     Well, look at the code ionce again. We need to send ZLP *after*
> request->actual == request->length, but as the check is inserted
> after the ZLP send, ZLP *may* be sent once the first DMA completes,
> not the last.
>
> WBR, Sergei

balbi also confirmed that is is really a problem.

I also have two related questions below for the problem:

1),  why is the check for "is_dma" needed here?

      if (is_dma || request->actual == request->length) {
           ....
      }

2), why is a zlp needed in the case below?

      #ifdef CONFIG_USB_INVENTRA_DMA
                  || (is_dma && (!dma->desired_mode ||
                     (request->actual & (musb_ep->packet_sz - 1))))
     #endif

Seems no request->zero is set to ask for zlp explicitly in
the case above.

IMO, it is not difficult to give a good fix for the ZLP problem
if the two questions are clear.

-- 
Lei Ming

Re: [Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Felipe Balbi]

Hi,

On Wed, Sep 15, 2010 at 05:53:10AM -0500, Ming Lei wrote:
>1),  why is the check for "is_dma" needed here?
>
>      if (is_dma || request->actual == request->length) {
>           ....
>      }

if you programmed dma to request->length (and assuming it worked just
fine) mode1 will only interrupt you when the entire request has been
sent.

>2), why is a zlp needed in the case below?
>
>      #ifdef CONFIG_USB_INVENTRA_DMA
>                  || (is_dma && (!dma->desired_mode ||
>                     (request->actual & (musb_ep->packet_sz - 1))))
>     #endif

in that case, it's not a zlp, it's short packet. Inventra will *NOT*
transfer short packets, you need to set txpktrdy by hand to get it
transfered.

>IMO, it is not difficult to give a good fix for the ZLP problem
>if the two questions are clear.

true, but some re-work needs to be done.

-- 
balbi

Re: [Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Ming Lei]

2010/9/15 Felipe Balbi <balbi@ti.com>:
> Hi,
>
> On Wed, Sep 15, 2010 at 05:53:10AM -0500, Ming Lei wrote:
>>
>> 1),  why is the check for "is_dma" needed here?
>>
>>     if (is_dma || request->actual == request->length) {
>>          ....
>>     }
>
> if you programmed dma to request->length (and assuming it worked just
> fine) mode1 will only interrupt you when the entire request has been
> sent.

If so, once the dma interrupt comes, will request->actual be same
with request->length in musb_g_tx?  And if it is true, could we remove the
check for 'is_dma'?

>
>> 2), why is a zlp needed in the case below?
>>
>>     #ifdef CONFIG_USB_INVENTRA_DMA
>>                 || (is_dma && (!dma->desired_mode ||
>>                    (request->actual & (musb_ep->packet_sz - 1))))
>>    #endif
>
> in that case, it's not a zlp, it's short packet. Inventra will *NOT*
> transfer short packets, you need to set txpktrdy by hand to get it
> transfered.

I see now.

>
>> IMO, it is not difficult to give a good fix for the ZLP problem
>> if the two questions are clear.
>
> true, but some re-work needs to be done.

Thanks for your reply.


-- 
Lei Ming

Re: [Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Felipe Balbi]

Hi,

On Wed, Sep 15, 2010 at 06:02:22AM -0500, Ming Lei wrote:
>If so, once the dma interrupt comes, will request->actual be same
>with request->length in musb_g_tx?  And if it is true, could we remove the
>check for 'is_dma'?

see that is_dma is set to true by just checking if dma in enabled in
txcsr, it might be that dma didn't complete everything and you need to
write txpktrdy by hand to send last short packet. So to remove that you
would need to re-work a bit more code.

You need to know when this is a dma IRQ or an endpoint IRQ.

-- 
balbi

Re: [Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Ming Lei]

2010/9/15 Felipe Balbi <balbi@ti.com>:

>
>> IMO, it is not difficult to give a good fix for the ZLP problem
>> if the two questions are clear.
>
> true, but some re-work needs to be done.

I suggest we do the fix for ZLP issue in a new patch against the
current patch set to avoid the rework. More importantly, it is one
another fix, we should insist on the rule: do one fix in one commit,
which is very helpful to do version management(regression test,
bisect...).

-- 
Lei Ming

Re: [Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Sergei Shtylyov]

Ming Lei wrote:
> Hi All,

> In another thread, Sergei pointed out there is a ZLP issue in musb_g_tx:

> Sergei Shtylyov <sshtylyov@mvista.com> wrote:
>> Hello.

>> On 15-09-2010 14:05, Felipe Balbi wrote:

>>>> I didn't say it was duplicate for DMA, just too late.

>>> how come ? we need to send ZLP before giving back the request.

>>     Well, look at the code ionce again. We need to send ZLP *after*
>> request->actual == request->length, but as the check is inserted
>> after the ZLP send, ZLP *may* be sent once the first DMA completes,
>> not the last.

>> WBR, Sergei

> balbi also confirmed that is is really a problem.

> I also have two related questions below for the problem:

> 1),  why is the check for "is_dma" needed here?

>       if (is_dma || request->actual == request->length) {
>            ....
>       }

    I'm not sure -- it seems erratic.

> 2), why is a zlp needed in the case below?
> 
>       #ifdef CONFIG_USB_INVENTRA_DMA
>                   || (is_dma && (!dma->desired_mode ||
>                      (request->actual & (musb_ep->packet_sz - 1))))
>      #endif

> Seems no request->zero is set to ask for zlp explicitly in
> the case above.

    This is not for ZLP -- this is here to set TXPktRdy for the last short 
packet in the Inventra DMA mode 0 that doesn't set TXPktRdy in such case.

> IMO, it is not difficult to give a good fix for the ZLP problem
> if the two questions are clear.

WBR, Sergei

Re: [Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Sergei Shtylyov]

Hello.

Felipe Balbi wrote:

> On Wed, Sep 15, 2010 at 06:02:22AM -0500, Ming Lei wrote:
>> If so, once the dma interrupt comes, will request->actual be same
>> with request->length in musb_g_tx?  And if it is true, could we remove 
>> the
>> check for 'is_dma'?

> see that is_dma is set to true by just checking if dma in enabled in
> txcsr, it might be that dma didn't complete everything and you need to
> write txpktrdy by hand to send last short packet. So to remove that you
> would need to re-work a bit more code.

    I don't see what to rework. The last short packet should still satisfy 
(request->actual == request->length) condition, no?

> You need to know when this is a dma IRQ or an endpoint IRQ.

    We know that -- but why check it there, before (request->actual == 
request->length)?

WBR, Sergei

Re: [Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Felipe Balbi]

Hi,

On Wed, Sep 15, 2010 at 08:10:15AM -0500, Sergei Shtylyov wrote:
>    I don't see what to rework. The last short packet should still satisfy
>(request->actual == request->length) condition, no?

of course not, it's short not zero. so the last short packet can be
anything from 1 to 511 bytes.

-- 
balbi

Re: [Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Gadiyar, Anand]

On Thu, Sep 16, 2010 at 11:35 AM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Wed, Sep 15, 2010 at 08:10:15AM -0500, Sergei Shtylyov wrote:
>>
>>   I don't see what to rework. The last short packet should still satisfy
>> (request->actual == request->length) condition, no?
>
> of course not, it's short not zero. so the last short packet can be
> anything from 1 to 511 bytes.
>

If it's TX, both condtions should automatically be true, right?

- Anand

Re: [Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Felipe Balbi]

On Thu, Sep 16, 2010 at 01:15:32AM -0500, Gadiyar, Anand wrote:
>On Thu, Sep 16, 2010 at 11:35 AM, Felipe Balbi <balbi@ti.com> wrote:
>> Hi,
>>
>> On Wed, Sep 15, 2010 at 08:10:15AM -0500, Sergei Shtylyov wrote:
>>>
>>>   I don't see what to rework. The last short packet should still satisfy
>>> (request->actual == request->length) condition, no?
>>
>> of course not, it's short not zero. so the last short packet can be
>> anything from 1 to 511 bytes.
>>
>
>If it's TX, both condtions should automatically be true, right?

even in mode1 ? I have to revist my docs, but afaict mode1 won't
transmit last short packet, no matter if it's tx or rx. Could be wrong,
though.

-- 
balbi

Re: [Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Ming Lei]

2010/9/16 Felipe Balbi <balbi@ti.com>:

>> If it's TX, both condtions should automatically be true, right?
>
> even in mode1 ? I have to revist my docs, but afaict mode1 won't
> transmit last short packet, no matter if it's tx or rx. Could be wrong,
> though.

Seems short packet is always sent using mode 0, see txstate.

-- 
Lei Ming

Re: [Discussion] USB: musb-gadget: how to fix ZLP issue in musb_g_tx

[Sergei Shtylyov]

Hello.

On 16-09-2010 10:05, Felipe Balbi wrote:

>> I don't see what to rework. The last short packet should still satisfy
>> (request->actual == request->length) condition, no?

> of course not, it's short not zero. so the last short packet can be
> anything from 1 to 511 bytes.

    Sigh. Where have I said anything different? What I meant is that the last 
short packet is already counted in request->actual, otherwise the condition 
(request->actual & (musb_ep->packet_sz - 1)) wouldn't work.

WBR, Sergei