Hi,
Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx> writes:
> Hello.
>
> On 11/06/2015 12:44 AM, Felipe Balbi wrote:
>
>>>>>>>>>>> Here are a few changes in musb_h_tx_flush_fifo().
>>>>>>>>>>>
>>>>>>>>>>> - Refering to 2ccc6d30a (usb: musb: fix bit mask for CSR in
>>>>>>>>>>> musb_h_tx_flush_fifo()), the datasheet says that
>>>>>>>>>>> MUSB_TXCSR_FLUSHFIFO
>>>>>>>>>>> is only valid when MUSB_TXCSR_TXPKTRDY is set as well. It means
>>>>>>>>>>> MUSB_TXCSR_TXPKTRDY should be checked, not set.
>>>>>>>>>>
>>>>>>>>>> Hum, my copy of the MUSBHDRC programmer's guide says about
>>>>>>>>>> TXCSR.FlushFIFO in host mode:
>>>>>>>>>>
>>>>>>>>>> <<
>>>>>>>>>> The CPU writes a 1 to this bit to flush the latest packet from the
>>>>>>>>>> endpoint Tx FIFO. The FIFO pointer is reset, the TxPktRdy bit (below) is
>>>>>>>>>> cleared and an interrupt is generated. May be set simultaneously with
>>>>>>>>>> TxPktRdy to abort the packet that is currently being loaded into the
>>>>>>>>>> FIFO. Note: FlushFIFO should only be used when TxPktRdy is set. At other
>>>>>>>>>> times, it may cause data to be corrupted. Also note that, if the FIFO is
>>>>>>>>>> double-buffered, FlushFIFO may need to be set twice to completely clear
>>>>>>>>>> the FIFO.
>>>>>>>>>> >>
>>>>>>>>>
>>>>>>>>> So how to interpret this message? FLUSHFIFO and TXPKTRDY should be set
>>>>>>>>> at the
>>>>>>>>> same time? If so, I will drop this change in V3.
>>>>>>>>
>>>>>>>> Yes, I think it's rather clear in this respect.
>>>>>>>>
>>>>>>>>> In either case, musb is unable to flush the tx fifo in urb dequque for
>>>>>>>>> device
>>>>>>>>> disconnect, but harmless, so the next two changes are important to
>>>>>>>>> give better
>>>>>>>>> user experience.
>>>>>>>>
>>>>>>>> Hm, looks like some errata... and hiding this from users while
>>>>>>>> there's no workaround doesn't seem a good policy.
>>>>>>>
>>>>>>> Well, based on the programmer's guide, the driver should set the flushFIFO
>>>>>>> bit, and wait for the interrupt.
>>>>>>
>>>>>> That's oversimplified, consider the double-buffered FIFO. ;-)
>>>>>
>>>>> Seems like we have some misinterpretation here and IMO Bin's patch is
>>>>> almost complete. The way I read MUSB's docs, it tells me three different
>>>>> things:
>>>>>
>>>>> 1. FIFONOTEMPTY is valid *only* when TXPKTRDY is also set
>>>>>
>>>>> Bin's new while condition solves this part:
>>>>>
>>>>> + while ((csr & MUSB_TXCSR_FIFONOTEMPTY) && (csr & MUSB_TXCSR_TXPKTRDY)) {
>>>>>
>>>>> 2. If we have Double Buffering, we need to set MUSB_TXCSR_FLUSHFIFO
>>>>> twice.
>>>>>
>>>>> This does not seem to be taken care of, but it would be
>>>>> something below:
>>>>>
>>>>> csr |= MUSB_TXCSR_FLUSH_FIFO;
>>>>> musb_writew(epio, MUSB_TXCSR, csr);
>>>>>
>>>>> if (ep->double_buffering_enabled)
>>>>> musb_write(epio, MUSB_TXCSR, csr);
>>>>>
>>>>> 3. We can set TXPKTRDY *together* with MUSB_TXCSR_FLUSH_FIFO to cancel
>>>>> the packet which is being loaded in the fifo right now.
>>>>>
>>>>> This seems to be a regression with current patch, however I
>>>>> don't think current code is perfect either. It unconditionally
>>>>> sets FLUSH_FIFO and TXPKTRDY without caring for the consequences
>>>>> of doing that. What happens if we set both of them but there's
>>>>> no packet being currently loaded into the FIFO ?
>>>>>
>>>>> To minimize changes, I think Bin just needs to keep the original csr
>>>>> unconditionally setting both FLUSHFIFO and TXPKTRDY alone while also
>>>>> making sure to check that TKPKTRDY is set when checking FIFONOTEMPTY.
>
>>> I am not sure this is any different, since TXPKTRDY is checked, setting
>>> it again along with FLUSHFIFO does not change TXPKTRDY bit, isn't it?
>>
>> you're missing one detail:
>>
>> If you set only FLUSHFIFO, TXPKTRDY is cleared and the packet *already*
>> in the FIFO gets flushed.
>
> That's exactly what we want, no? Note that the CPU cannot clear the
> TxPktRdy bit, only MUSB itself can.
>
>> If you set them both together, you're telling MUSB that you want to
>> flush a packet which is *still* being loaded into the FIFO. After that
>> TXPKTRDY will be cleared by MUSB.
>
> This is not really different from the first case.
not for single buffering, not. If you use double buffering, it is very
different.
>>> I have a few different test cases, as long as a tx urb is queued, the
>>> WARN() will be triggered when dequque called by device disconnect, that
>>> is why I concluded that currently musb is never able to flush tx fifo.
>>>
>>> If we are doing it wrong, I am not what the problem is...
>>
>> And that's where debugging comes in, right ? MUSB's docs are not the
>> best out there and they haven't been updated for 8 years at least. There
>> will be points where it's lacking and we just have to deal with it and
>> figure out what the IP is actually doing.
>
> BTW, about MUSB docs. I have encountered the issue with the SendStall bit
> on more than one TI SoC (namely, DaVincis, OMAP-L1x, and AM35x). This bit
> seems to get clearead all by itself so that USB test 13 can never succed. Are
> you aware of this? AFAIR, I have tried to instrument this issue and couldn't
> detect a "rogue" register write that cleared the bit... This issue hadn't been
> documented anywhere (at least back then in 20008-2009)...
no, I was not aware of this. Thanks for notifying.
--
balbi
Attachment:
signature.asc
Description: PGP signature
