usb: musb: only write CLRDATATOG when appropriate

PATCH New Mike Frysinger 3 participants 10 messages Aug 9, 2010 – Oct 13, 2010

Subsystems (2)

USB MUSB multipoint high speed dual-role controller

lore

Share & tools

lore

Mike Frysinger

author

2010-08-09 18:58 UTC #0 lore raw

PATCH 1/1 usb: musb: only write CLRDATATOG when appropriate

From: Bryan Wu <[email protected]>

This is a change similar to what is already in the Linux driver. We should only program the CLRDATATOG bit when the current mode indicates that it is needed.

Signed-off-by Bryan Wu <[email protected]>

Signed-off-by Cliff Cai <[email protected]>

Signed-off-by Mike Frysinger <[email protected]>

---
Note: can someone give this a spin on a non-Blackfin platform to make
sure this doesn't break things ?

 drivers/usb/musb/musb_hcd.c |   23 ++++++++++++++++-------
 1 files changed, 16 insertions(+), 7 deletions(-)

drivers/usb/musb/musb_hcd.c +16 -7

index dd2aa7f..dd66275 100644
@@ -144,19 +144,28 @@ static void write_toggle(struct usb_device *dev, u8 ep, u8 dir_out)
 	u16 csr;
 
 	if (dir_out) {
-		if (!toggle)
-			writew(MUSB_TXCSR_CLRDATATOG, &musbr->txcsr);
-		else {
-			csr = readw(&musbr->txcsr);
+		csr = readw(&musbr->txcsr);
+		if (!toggle) {
+			if (csr & MUSB_TXCSR_MODE)
+				csr = MUSB_TXCSR_CLRDATATOG;
+			else
+				csr = 0;
+			writew(csr, &musbr->txcsr);
+		} else {
 			csr |= MUSB_TXCSR_H_WR_DATATOGGLE;
 			writew(csr, &musbr->txcsr);
 			csr |= (toggle << MUSB_TXCSR_H_DATATOGGLE_SHIFT);
 			writew(csr, &musbr->txcsr);
 		}
 	} else {
-		if (!toggle)
-			writew(MUSB_RXCSR_CLRDATATOG, &musbr->rxcsr);
-		else {
+		if (!toggle) {
+			csr = readw(&musbr->txcsr);
+			if (csr & MUSB_TXCSR_MODE)
+				csr = MUSB_RXCSR_CLRDATATOG;
+			else
+				csr = 0;
+			writew(csr, &musbr->rxcsr);
+		} else {
 			csr = readw(&musbr->rxcsr);
 			csr |= MUSB_RXCSR_H_WR_DATATOGGLE;
 			writew(csr, &musbr->rxcsr);

Signature · 1 line

1.7.2

Mike Frysinger

author

2010-08-10 20:37 UTC #2 lore raw

PATCH 1/1 usb: musb: only write CLRDATATOG when appropriate

Quoted 13 lines · Sergei Shtylyov

Quoted 10 lines · Mike Frysinger
drivers/usb/musb/musb_hcd.c +0 -0
? ? ? ?} else {
- ? ? ? ? ? ? ? if (!toggle)
- ? ? ? ? ? ? ? ? ? ? ? writew(MUSB_RXCSR_CLRDATATOG, &musbr->rxcsr);
- ? ? ? ? ? ? ? else {
+ ? ? ? ? ? ? ? if (!toggle) {
+ ? ? ? ? ? ? ? ? ? ? ? csr = readw(&musbr->txcsr);
+ ? ? ? ? ? ? ? ? ? ? ? if (csr & MUSB_TXCSR_MODE)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? csr = MUSB_RXCSR_CLRDATATOG;
? Clearing RXCSR when FIFO is in TX mode?

unless i missed something, that is what Linux is doing.

musb_host.c:musb_rx_reinit()

        csr = musb_readw(ep->regs, MUSB_RXCSR);
        if (csr & MUSB_RXCSR_RXPKTRDY)
            WARNING("rx%d, packet/%d ready?\n", ep->epnum,
                musb_readw(ep->regs, MUSB_RXCOUNT));

        csr = musb_readw(ep->regs, MUSB_TXCSR);
        if (csr & MUSB_TXCSR_MODE)
            musb_h_flush_rxfifo(ep, MUSB_RXCSR_CLRDATATOG);
        else
            musb_h_flush_rxfifo(ep, 0);

although i see that i need to also extend the MUSB_TXCSR_MODE define for Blackfin musb ports in u-boot. but this shouldnt affect non-Blackfin musb ports. i'll send a sep patch for that. -mike

Remy Bohmer

2010-08-12 18:04 UTC #3 lore raw

PATCH 1/1 usb: musb: only write CLRDATATOG when appropriate

Hi,

2010/8/10 Mike Frysinger <[email protected]>:

Quoted 15 lines · Sergei Shtylyov

Quoted 13 lines
Quoted 10 lines · Mike Frysinger
drivers/usb/musb/musb_hcd.c +0 -0
? ? ? ?} else {
- ? ? ? ? ? ? ? if (!toggle)
- ? ? ? ? ? ? ? ? ? ? ? writew(MUSB_RXCSR_CLRDATATOG, &musbr->rxcsr);
- ? ? ? ? ? ? ? else {
+ ? ? ? ? ? ? ? if (!toggle) {
+ ? ? ? ? ? ? ? ? ? ? ? csr = readw(&musbr->txcsr);
+ ? ? ? ? ? ? ? ? ? ? ? if (csr & MUSB_TXCSR_MODE)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? csr = MUSB_RXCSR_CLRDATATOG;
? Clearing RXCSR when FIFO is in TX mode?

Quoted 1 line

unless i missed something, that is what Linux is doing.

Can Linux be wrong too?

Quoted 12 lines

musb_host.c:musb_rx_reinit()
? ? ? ?csr = musb_readw(ep->regs, MUSB_RXCSR);
? ? ? ?if (csr & MUSB_RXCSR_RXPKTRDY)
? ? ? ? ? ?WARNING("rx%d, packet/%d ready?\n", ep->epnum, ? ? ? ? ? ? ? ?musb_readw(ep->regs, MUSB_RXCOUNT));
? ? ? ?csr = musb_readw(ep->regs, MUSB_TXCSR);
? ? ? ?if (csr & MUSB_TXCSR_MODE)
? ? ? ? ? ?musb_h_flush_rxfifo(ep, MUSB_RXCSR_CLRDATATOG); ? ? ? ?else
? ? ? ? ? ?musb_h_flush_rxfifo(ep, 0);

Quoted 5 lines

although i see that i need to also extend the MUSB_TXCSR_MODE define for Blackfin musb ports in u-boot. ?but this shouldnt affect non-Blackfin musb ports. ?i'll send a sep patch for that. -mike

Sergei, do you agree that I apply this patch?

Kind regards,

Remy

Mike Frysinger

author

2010-08-12 19:43 UTC #4 lore raw

PATCH 1/1 usb: musb: only write CLRDATATOG when appropriate

Quoted 16 lines · Remy Bohmer

2010/8/10 Mike Frysinger:
Quoted 15 lines · Sergei Shtylyov
Quoted 13 lines
Quoted 10 lines · Mike Frysinger
drivers/usb/musb/musb_hcd.c +0 -0
? ? ? ?} else {
- ? ? ? ? ? ? ? if (!toggle)
- ? ? ? ? ? ? ? ? ? ? ? writew(MUSB_RXCSR_CLRDATATOG, &musbr->rxcsr);
- ? ? ? ? ? ? ? else {
+ ? ? ? ? ? ? ? if (!toggle) {
+ ? ? ? ? ? ? ? ? ? ? ? csr = readw(&musbr->txcsr);
+ ? ? ? ? ? ? ? ? ? ? ? if (csr & MUSB_TXCSR_MODE)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? csr = MUSB_RXCSR_CLRDATATOG;
? Clearing RXCSR when FIFO is in TX mode?

Quoted 3 lines

Quoted 1 line
unless i missed something, that is what Linux is doing.
Can Linux be wrong too?

anything is possible, but i dont believe people have complained about this working incorrectly on their platforms, else it'd be fixed by now i imagine
-mike

Sergei Shtylyov

2010-08-13 10:25 UTC #5 lore raw

PATCH 1/1 usb: musb: only write CLRDATATOG when appropriate

In reply to Remy Bohmer

Hello.

Quoted 13 lines · Remy Bohmer

Quoted 13 lines

drivers/usb/musb/musb_hcd.c static void write_toggle(struct usb_device *dev, u8 ep, u8 dir_out) · line 164

Quoted 10 lines · Mike Frysinger

drivers/usb/musb/musb_hcd.c +4 -3

       } else {
-               if (!toggle)
-                       writew(MUSB_RXCSR_CLRDATATOG, &musbr->rxcsr);
-               else {
+               if (!toggle) {
+                       csr = readw(&musbr->txcsr);
+                       if (csr & MUSB_TXCSR_MODE)
+                               csr = MUSB_RXCSR_CLRDATATOG;

Clearing RXCSR when FIFO is in TX mode?

I meant to say "clearing RX toggle".

Quoted 16 lines

Quoted 1 line

unless i missed something, that is what Linux is doing.

Can Linux be wrong too?

Quoted 11 lines

musb_host.c:musb_rx_reinit()

       csr = musb_readw(ep->regs, MUSB_RXCSR);
       if (csr & MUSB_RXCSR_RXPKTRDY)
           WARNING("rx%d, packet/%d ready?\n", ep->epnum,
               musb_readw(ep->regs, MUSB_RXCOUNT));

       csr = musb_readw(ep->regs, MUSB_TXCSR);
       if (csr & MUSB_TXCSR_MODE)
           musb_h_flush_rxfifo(ep, MUSB_RXCSR_CLRDATATOG);
       else
           musb_h_flush_rxfifo(ep, 0);

Quoted 6 lines

Quoted 4 lines
although i see that i need to also extend the MUSB_TXCSR_MODE define for Blackfin musb ports in u-boot. but this shouldnt affect non-Blackfin musb ports. i'll send a sep patch for that. -mike
Sergei, do you agree that I apply this patch?

Well, if it's based on the Linux code... although it still does look
wrong... I don't know. :-)

Quoted 3 lines

Kind regards,
Remy

WBR, Sergei

Mike Frysinger

author

2010-10-11 08:47 UTC #6 lore raw

PATCH 1/1 usb: musb: only write CLRDATATOG when appropriate

Quoted 5 lines · On Friday, August 13, 2010 06:25:05 Sergei Shtylyov wrote:

Quoted 1 line · Remy Bohmer
Sergei, do you agree that I apply this patch?
Well, if it's based on the Linux code... although it still does look
wrong... I don't know. :-)

do you have a board you could test on ? otherwise, it seems this patch has stalled and will stay that way ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101011/2463b17d/attachment.pgp

Remy Bohmer

2010-10-13 10:12 UTC #7 lore raw

PATCH 1/1 usb: musb: only write CLRDATATOG when appropriate

Hi,

2010/10/11 Mike Frysinger <[email protected]>:

Quoted 9 lines · On Friday, August 13, 2010 06:25:05 Sergei Shtylyov wrote:

Quoted 5 lines
Quoted 1 line · Remy Bohmer
Sergei, do you agree that I apply this patch?
? ? Well, if it's based on the Linux code... although it still does look wrong... I don't know. :-)
do you have a board you could test on ? ?otherwise, it seems this patch has stalled and will stay that way ...

No, Sorry, I do not have such a board...
But still, I applied it to u-boot-usb.

Thanks.

Remy

Remy Bohmer

2010-08-12 18:08 UTC #8 lore raw

PATCH 1/1 usb: musb: only write CLRDATATOG when appropriate

In reply to Mike Frysinger

Hi Mike,

2010/8/9 Mike Frysinger <[email protected]>:

Quoted 7 lines

From: Bryan Wu <[email protected]>
This is a change similar to what is already in the Linux driver. ?We should only program the CLRDATATOG bit when the current mode indicates that it is needed.
Signed-off-by Bryan Wu <[email protected]>

You use a email adress here that gets bounced due its non-existence... Can you replace it by a valid address?

Remy

Mike Frysinger

author

2010-08-12 19:42 UTC #9 lore raw

PATCH 1/1 usb: musb: only write CLRDATATOG when appropriate

Quoted 11 lines · Remy Bohmer

2010/8/9 Mike Frysinger:
Quoted 7 lines
From: Bryan Wu <[email protected]>
This is a change similar to what is already in the Linux driver. ?We should only program the CLRDATATOG bit when the current mode indicates that it is needed.
Signed-off-by Bryan Wu <[email protected]>
You use a email adress here that gets bounced due its non-existence... Can you replace it by a valid address?

the patch was originally written when he worked ADI (and that e-mail was valid). he has however moved on since.
-mike