Hi, Sergei Shtylyov <sergei.shtyl...@cogentembedded.com> 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
signature.asc
Description: PGP signature
