Hi, Sergei Shtylyov <sergei.shtyl...@cogentembedded.com> writes: > Hello. > > On 11/06/2015 12:06 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 > > Don't see where they say that...
it's subtle, but it's there:
" When the TxPktRdy bit is set, either manually or automatically, the
packet is deemed ready to be sent. The FIFONotEmpty bit in TxCSR is also
set.
When the packet has been successfully sent, both TxPktRdy and
FIFONotEmpty will be cleared and the appropriate Tx endpoint interrupt
generated (if enabled). The next packet can then be loaded into the
FIFO. "
And this is also easy to conclude when you realise that FIFONotEmpty is
set when "at least 1 packet has been loaded into the FIFO".
>> 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:
>
> Why? It should just take another loop iteration to clear the
> double-buffered FIFO, as far as my reading of the code goes...
right. Or we avoid the loop altogether :-) There's no reason for that
loop; We only need it now because this code doesn't work :-p
>> 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.
>
> If TxPktRdy is already set, what's the point of forcing it set on
> write?
Coming back to what you pointed at originally:
" [...] May be set simultaneously with TxPktRdy to abort the packet that
is currently being loaded into the FIFO. [...]"
It's not setting it back, it's telling MUSB which packet to flush. If
you go back to double buffering. Consider you already have one packet in
the FIFO and you're loading the second. TXPKTRDY will tell MUSB which of
the two you want to flush. (it might be that you either flush the one
currently being loaded or both, but that's not clear in the
documentation, so it would be have to be tested)
And here's another important note:
" 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 checking that TXPKTRDY is set before FIFOFLUSH is extra important and
we might actually want to add a WARN() (and a comment) about that too.
> If it gets cleared in-between (perhaps the packet was sent?), we'll get into
> the dubious situation with non data in FIFO (you've described), no?
no, we could be loading another packet in the FIFO and we would end up
corrupting that packet ;-)
--
balbi
signature.asc
Description: PGP signature
