Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Felipe Balbi]

Hi,

Sergei Shtylyov  writes:
> Hello.
>
> On 11/06/2015 12:44 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
>
>   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.
>
> That's exactly what we want, no? Note that the CPU cannot clear the 
> TxPktRdy bit, only MUSB itself can.
>
>> 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.
>
> This is not really different from the first case.

not for single buffering, not. If you use double buffering, it is very
different.

>>> 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.
>
> BTW, about MUSB docs. I have 

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Sergei Shtylyov]

Hello.

On 11/06/2015 12:44 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

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.


   That's exactly what we want, no? Note that the CPU cannot clear the 
TxPktRdy bit, only MUSB itself can.



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.


   This is not really different from the first case.


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.


   BTW, about MUSB docs. I have encountered the issue with the SendStall bit 
on more than one TI SoC (namely, DaVincis, OMAP-L1x, and AM35x). This bit 
seems to get clearead all by itself so that USB test 13 can never succed. Are 
you aware of this? AFAIR, I have tried to instrument this issue and couldn't 
detect a "rogue" register write that cleared the bit... This issue hadn't been 
documented anywhere (at least back then in 20008-2009)...


MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Bin Liu]

Hi,

On 11/05/2015 03:44 PM, Felipe Balbi wrote:


Hi,

Bin Liu  writes:

Hi,

On 11/05/2015 03:07 PM, Felipe Balbi wrote:


Hi again,

Felipe Balbi  writes:

Hi,

Sergei Shtylyov  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. 

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Felipe Balbi]

Hi,

Bin Liu  writes:
> 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

not dev_err(), try dev_WARN_ONCE(). I still want the verbosity of a
WARN(), but we can live with only one WARN().

> the issue but does not flood the console;
> 2. add inline comment that #1 needs revisit;
> 3. remove 1ms delay.

yeah, 1ms does sound like a lot.

> Please let me know if this is acceptable then I will send v3.

if you use dev_WARN_ONCE() I can accept. sure.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Sergei Shtylyov]

On 11/05/2015 10:54 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.



Thanks,
-Bin.



So I don't think this change is valid.


- It seems to be common that sometimes (if not always) musb is unable
   to flush tx fifo during urb dequeue. But it seems to be harmless,
   since the tx fifo flush is done again in musb_ep_program() when
   re-use the hw_ep.

   But the WARN() scares many end users, so change it to dev_dbg().

- applications could queue up many tx urbs, then the 1ms delay could
   causes minutes of delay in device disconnect. So remove it to get
   better user experience. The 1ms delay does not help the flushing
   anyway.

- cleanup the debug code - related to lastcsr.

Signed-off-by: Bin Liu 


[...]

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: musb: fix tx fifo flush handling

[Bin Liu]

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.

- It seems to be common that sometimes (if not always) musb is unable
  to flush tx fifo during urb dequeue. But it seems to be harmless,
  since the tx fifo flush is done again in musb_ep_program() when
  re-use the hw_ep.

  But the WARN() scares many end users, so change it to dev_dbg().

- applications could queue up many tx urbs, then the 1ms delay could
  causes minutes of delay in device disconnect. So remove it to get
  better user experience. The 1ms delay does not help the flushing
  anyway.

- cleanup the debug code - related to lastcsr.

Signed-off-by: Bin Liu 
---
 drivers/usb/musb/musb_host.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 883a9ad..5db24f2 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -112,22 +112,18 @@ static void musb_h_tx_flush_fifo(struct musb_hw_ep *ep)
struct musb *musb = ep->musb;
void __iomem*epio = ep->regs;
u16 csr;
-   u16 lastcsr = 0;
int retries = 1000;
 
csr = musb_readw(epio, MUSB_TXCSR);
-   while (csr & MUSB_TXCSR_FIFONOTEMPTY) {
-   if (csr != lastcsr)
-   dev_dbg(musb->controller, "Host TX FIFONOTEMPTY csr: 
%02x\n", csr);
-   lastcsr = csr;
-   csr |= MUSB_TXCSR_FLUSHFIFO | MUSB_TXCSR_TXPKTRDY;
+   while ((csr & MUSB_TXCSR_FIFONOTEMPTY) && (csr & MUSB_TXCSR_TXPKTRDY)) {
+   csr |= MUSB_TXCSR_FLUSHFIFO;
musb_writew(epio, MUSB_TXCSR, csr);
csr = musb_readw(epio, MUSB_TXCSR);
-   if (WARN(retries-- < 1,
-   "Could not flush host TX%d fifo: csr: %04x\n",
-   ep->epnum, csr))
+   if (retries-- < 1) {
+   dev_dbg(musb->controller, "Could not flush host TX%d 
fifo: csr: %04x\n",
+   ep->epnum, csr);
return;
-   mdelay(1);
+   }
}
 }
 
-- 
1.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Sergei Shtylyov]

On 11/05/2015 10:46 PM, Sergei Shtylyov 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 I don't think this change is valid.


- It seems to be common that sometimes (if not always) musb is unable
   to flush tx fifo during urb dequeue. But it seems to be harmless,
   since the tx fifo flush is done again in musb_ep_program() when
   re-use the hw_ep.

   But the WARN() scares many end users, so change it to dev_dbg().

- applications could queue up many tx urbs, then the 1ms delay could
   causes minutes of delay in device disconnect. So remove it to get
   better user experience. The 1ms delay does not help the flushing
   anyway.

- cleanup the debug code - related to lastcsr.

Signed-off-by: Bin Liu 
---
  drivers/usb/musb/musb_host.c | 16 ++--
  1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 883a9ad..5db24f2 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -112,22 +112,18 @@ static void musb_h_tx_flush_fifo(struct musb_hw_ep *ep)
  struct musb*musb = ep->musb;
  void __iomem*epio = ep->regs;
  u16csr;
-u16lastcsr = 0;
  intretries = 1000;

  csr = musb_readw(epio, MUSB_TXCSR);
-while (csr & MUSB_TXCSR_FIFONOTEMPTY) {
-if (csr != lastcsr)
-dev_dbg(musb->controller, "Host TX FIFONOTEMPTY csr: %02x\n",
csr);
-lastcsr = csr;
-csr |= MUSB_TXCSR_FLUSHFIFO | MUSB_TXCSR_TXPKTRDY;
+while ((csr & MUSB_TXCSR_FIFONOTEMPTY) && (csr & MUSB_TXCSR_TXPKTRDY)) {


 while (csr & (MUSB_TXCSR_FIFONOTEMPTY | MUSB_TXCSR_TXPKTRDY)) {


   Oops, I misread your check, it had &&, not ||.




+csr |= MUSB_TXCSR_FLUSHFIFO;



 No, this clearly contradicts the programmer's guide.


   Still I disagree with this change.

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Bin Liu]

Hi,

On 11/05/2015 01:46 PM, Sergei Shtylyov wrote:

Hello.

On 11/05/2015 09:53 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.


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.


Thanks,
-Bin.



So I don't think this change is valid.


- It seems to be common that sometimes (if not always) musb is unable
   to flush tx fifo during urb dequeue. But it seems to be harmless,
   since the tx fifo flush is done again in musb_ep_program() when
   re-use the hw_ep.

   But the WARN() scares many end users, so change it to dev_dbg().

- applications could queue up many tx urbs, then the 1ms delay could
   causes minutes of delay in device disconnect. So remove it to get
   better user experience. The 1ms delay does not help the flushing
   anyway.

- cleanup the debug code - related to lastcsr.

Signed-off-by: Bin Liu 
---
  drivers/usb/musb/musb_host.c | 16 ++--
  1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 883a9ad..5db24f2 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -112,22 +112,18 @@ static void musb_h_tx_flush_fifo(struct
musb_hw_ep *ep)
  struct musb*musb = ep->musb;
  void __iomem*epio = ep->regs;
  u16csr;
-u16lastcsr = 0;
  intretries = 1000;

  csr = musb_readw(epio, MUSB_TXCSR);
-while (csr & MUSB_TXCSR_FIFONOTEMPTY) {
-if (csr != lastcsr)
-dev_dbg(musb->controller, "Host TX FIFONOTEMPTY csr:
%02x\n", csr);
-lastcsr = csr;
-csr |= MUSB_TXCSR_FLUSHFIFO | MUSB_TXCSR_TXPKTRDY;
+while ((csr & MUSB_TXCSR_FIFONOTEMPTY) && (csr &
MUSB_TXCSR_TXPKTRDY)) {


 while (csr & (MUSB_TXCSR_FIFONOTEMPTY | MUSB_TXCSR_TXPKTRDY)) {


+csr |= MUSB_TXCSR_FLUSHFIFO;


 No, this clearly contradicts the programmer's guide.




--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Sergei Shtylyov]

Hello.

On 11/05/2015 09:53 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 I don't think this change is valid.


- It seems to be common that sometimes (if not always) musb is unable
   to flush tx fifo during urb dequeue. But it seems to be harmless,
   since the tx fifo flush is done again in musb_ep_program() when
   re-use the hw_ep.

   But the WARN() scares many end users, so change it to dev_dbg().

- applications could queue up many tx urbs, then the 1ms delay could
   causes minutes of delay in device disconnect. So remove it to get
   better user experience. The 1ms delay does not help the flushing
   anyway.

- cleanup the debug code - related to lastcsr.

Signed-off-by: Bin Liu 
---
  drivers/usb/musb/musb_host.c | 16 ++--
  1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 883a9ad..5db24f2 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -112,22 +112,18 @@ static void musb_h_tx_flush_fifo(struct musb_hw_ep *ep)
struct musb *musb = ep->musb;
void __iomem*epio = ep->regs;
u16 csr;
-   u16 lastcsr = 0;
int retries = 1000;

csr = musb_readw(epio, MUSB_TXCSR);
-   while (csr & MUSB_TXCSR_FIFONOTEMPTY) {
-   if (csr != lastcsr)
-   dev_dbg(musb->controller, "Host TX FIFONOTEMPTY csr: 
%02x\n", csr);
-   lastcsr = csr;
-   csr |= MUSB_TXCSR_FLUSHFIFO | MUSB_TXCSR_TXPKTRDY;
+   while ((csr & MUSB_TXCSR_FIFONOTEMPTY) && (csr & MUSB_TXCSR_TXPKTRDY)) {


while (csr & (MUSB_TXCSR_FIFONOTEMPTY | MUSB_TXCSR_TXPKTRDY)) {


+   csr |= MUSB_TXCSR_FLUSHFIFO;


No, this clearly contradicts the programmer's guide.



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Bin Liu]

Hi,

On 11/05/2015 02:04 PM, Sergei Shtylyov wrote:

On 11/05/2015 10:54 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.


However, the driver does 1000-times retry loop to probe the FIFONOTEMPTY 
bit. This is there in the very first version of the driver back in 2008. 
So I would think musb just behaves like that, rather than an errata. 
Also the WARN() was not there in the early version of the driver.


Please guide what we will do with this patch? Should I send V3 with the 
first change removed, or just drop this patch completely?


Thanks,
-Bin.




Thanks,
-Bin.



So I don't think this change is valid.


- It seems to be common that sometimes (if not always) musb is unable
   to flush tx fifo during urb dequeue. But it seems to be harmless,
   since the tx fifo flush is done again in musb_ep_program() when
   re-use the hw_ep.

   But the WARN() scares many end users, so change it to dev_dbg().

- applications could queue up many tx urbs, then the 1ms delay could
   causes minutes of delay in device disconnect. So remove it to get
   better user experience. The 1ms delay does not help the flushing
   anyway.

- cleanup the debug code - related to lastcsr.

Signed-off-by: Bin Liu 


[...]

MBR, Sergei


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Sergei Shtylyov]

On 11/05/2015 11:53 PM, Sergei Shtylyov 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. ;-)


However, the driver does 1000-times retry loop to probe the FIFONOTEMPTY bit.
This is there in the very first version of the driver back in 2008. So I would
think musb just behaves like that, rather than an errata. Also the WARN() was
not there in the early version of the driver.


Yes, but added shortly after, in 2008.


   Actually, the driver has always loudly complained in this case -- I've 
just rummaged in the commits.


MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Bin Liu]

On 11/05/2015 02:53 PM, Sergei Shtylyov wrote:

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. ;-)


Agreed, but my point was the driver doing the 1000 times retry must be 
for a reason.


But I guess the problem is nobody knows the reason...

Regards,
-Bin.




However, the driver does 1000-times retry loop to probe the
FIFONOTEMPTY bit.
This is there in the very first version of the driver back in 2008. So
I would
think musb just behaves like that, rather than an errata. Also the
WARN() was
not there in the early version of the driver.


Yes, but added shortly after, in 2008.


Please guide what we will do with this patch? Should I send V3 with
the first
change removed, or just drop this patch completely?


I'll let Felipe decide, it's his domain.


Thanks,
-Bin.


[...]

MBR, Sergei


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Sergei Shtylyov]

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...


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...




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?
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?



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.


   Agreed.

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Felipe Balbi]

Hi again,

Felipe Balbi  writes:
> Hi,
>
> Sergei Shtylyov  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


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Sergei Shtylyov]

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. ;-)


However, the driver does 1000-times retry loop to probe the FIFONOTEMPTY bit.
This is there in the very first version of the driver back in 2008. So I would
think musb just behaves like that, rather than an errata. Also the WARN() was
not there in the early version of the driver.


   Yes, but added shortly after, in 2008.


Please guide what we will do with this patch? Should I send V3 with the first
change removed, or just drop this patch completely?


   I'll let Felipe decide, it's his domain.


Thanks,
-Bin.


[...]

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Felipe Balbi]

Hi,

Sergei Shtylyov  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

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Bin Liu]

Hi,

On 11/05/2015 03:07 PM, Felipe Balbi wrote:


Hi again,

Felipe Balbi  writes:

Hi,

Sergei Shtylyov  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?



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.


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...

Regards,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Felipe Balbi]

Hi,

Bin Liu  writes:
> Hi,
>
> On 11/05/2015 03:07 PM, Felipe Balbi wrote:
>>
>> Hi again,
>>
>> Felipe Balbi  writes:
>>> Hi,
>>>
>>> Sergei Shtylyov  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, 

Re: [PATCH v2] usb: musb: fix tx fifo flush handling

[Felipe Balbi]

Hi,

Sergei Shtylyov  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