Hi,
Bin Liu <b-liu@xxxxxx> writes:
> Hi,
>
> On 11/05/2015 03:07 PM, Felipe Balbi wrote:
>>
>> 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 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.
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.
>>> 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.
>>
>
> Well, I have tried the changes you mentioned above, but the WARN() was
> still triggered for dequeue all the tx urbs while disconnect the
> device.
then we need to figure out why isn't the patcket being flushed. Are you
using double buffering ? Is a packet being loaded in the FIFO right now?
Does it change if you set *only* FLUSHFIFO but *not* TXKPKTRDY ?
> 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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
