Hi again,
Felipe Balbi <balbi@xxxxxx> writes:
> 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.
Also, leave the WARN() alone. We don't want FIFOFLUSH to ever fail. If
it does, we're doing it wrong.
--
balbi
Attachment:
signature.asc
Description: PGP signature
