Hi,
Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx> writes:
> On 11/05/2015 11:34 PM, Bin Liu 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 would also add a very verbose and descriptive comment on that
particular location so we never forget about these MUSB oddities next
time someone's looking at this.
--
balbi
Attachment:
signature.asc
Description: PGP signature
