Hi,
On 11/05/2015 03:44 PM, Felipe Balbi wrote:
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 ?
Right now here have 3 problems:
1. tx fifo flush failed during device disconnect, it is harmless though;
2. WARN() causes console log flooding in the case which multi (100+) tx
urbs queued;
3. the 1ms delay causes disconnect delay in the case which multi tx urbs
queued;
We know if #1 was solved, #2 & #3 would not happen, but the handling of
#2 & #3 in the driver is not correct.
How about to address the #2 & #3 first, and leave #1 for another patch
since it would take some effort to find the root cause?
To solve #2 & #3,
1. change WARN() to dev_err() so that kernel log by default still shows
the issue but does not flood the console;
2. add inline comment that #1 needs revisit;
3. remove 1ms delay.
Please let me know if this is acceptable then I will send v3.
Regards,
-Bin.
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
