[U-Boot] [PATCH] usb: musb: only write CLRDATATOG when appropriate

[U-Boot] [PATCH] usb: musb: only write CLRDATATOG when appropriate

[Mike Frysinger]

From: Bryan Wu <bryan.wu@analog.com>

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 <bryan.wu@analog.com>
Signed-off-by: Cliff Cai <cliff.cai@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
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(-)

diff --git a/drivers/usb/musb/musb_hcd.c b/drivers/usb/musb/musb_hcd.c
index dd2aa7f..dd66275 100644
--- a/drivers/usb/musb/musb_hcd.c
+++ b/drivers/usb/musb/musb_hcd.c
@@ -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);
-- 
1.7.2

[U-Boot] [PATCH] usb: musb: only write CLRDATATOG when appropriate

[Sergei Shtylyov]

Hello.

Mike Frysinger wrote:

> From: Bryan Wu <bryan.wu@analog.com>

> 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 <bryan.wu@analog.com>
> Signed-off-by: Cliff Cai <cliff.cai@analog.com>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> 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(-)

> diff --git a/drivers/usb/musb/musb_hcd.c b/drivers/usb/musb/musb_hcd.c
> index dd2aa7f..dd66275 100644
> --- a/drivers/usb/musb/musb_hcd.c
> +++ b/drivers/usb/musb/musb_hcd.c
> @@ -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;

    Clearing RXCSR when FIFO is in TX mode?

> +			else
> +				csr = 0;
> +			writew(csr, &musbr->rxcsr);
> +		} else {
>  			csr = readw(&musbr->rxcsr);
>  			csr |= MUSB_RXCSR_H_WR_DATATOGGLE;
>  			writew(csr, &musbr->rxcsr);

WBR, Sergei

[U-Boot] [PATCH] usb: musb: only write CLRDATATOG when appropriate

[Mike Frysinger]

On Tue, Aug 10, 2010 at 6:26 AM, Sergei Shtylyov wrote:
> Mike Frysinger wrote:
>> --- a/drivers/usb/musb/musb_hcd.c
>> +++ b/drivers/usb/musb/musb_hcd.c
>> ? ? ? ?} 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

[U-Boot] [PATCH] usb: musb: only write CLRDATATOG when appropriate

[Remy Bohmer]

Hi,

2010/8/10 Mike Frysinger <vapier@gentoo.org>:
> On Tue, Aug 10, 2010 at 6:26 AM, Sergei Shtylyov wrote:
>> Mike Frysinger wrote:
>>> --- a/drivers/usb/musb/musb_hcd.c
>>> +++ b/drivers/usb/musb/musb_hcd.c
>>> ? ? ? ?} 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.

Can Linux be wrong too?

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

Sergei, do you agree that I apply this patch?

Kind regards,

Remy

[U-Boot] [PATCH] usb: musb: only write CLRDATATOG when appropriate

[Remy Bohmer]

Hi Mike,

2010/8/9 Mike Frysinger <vapier@gentoo.org>:
> From: Bryan Wu <bryan.wu@analog.com>
>
> 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 <bryan.wu@analog.com>

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

Remy

[U-Boot] [PATCH] usb: musb: only write CLRDATATOG when appropriate

[Mike Frysinger]

On Thu, Aug 12, 2010 at 2:08 PM, Remy Bohmer wrote:
> 2010/8/9 Mike Frysinger:
>> From: Bryan Wu <bryan.wu@analog.com>
>>
>> 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 <bryan.wu@analog.com>
>
> 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

[U-Boot] [PATCH] usb: musb: only write CLRDATATOG when appropriate

[Mike Frysinger]

On Thu, Aug 12, 2010 at 2:04 PM, Remy Bohmer wrote:
> 2010/8/10 Mike Frysinger:
>> On Tue, Aug 10, 2010 at 6:26 AM, Sergei Shtylyov wrote:
>>> Mike Frysinger wrote:
>>>> --- a/drivers/usb/musb/musb_hcd.c
>>>> +++ b/drivers/usb/musb/musb_hcd.c
>>>> ? ? ? ?} 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.
>
> 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

[U-Boot] [PATCH] usb: musb: only write CLRDATATOG when appropriate

[Sergei Shtylyov]

Hello.

Remy Bohmer wrote:

>>> Mike Frysinger wrote:
>>>> --- a/drivers/usb/musb/musb_hcd.c
>>>> +++ b/drivers/usb/musb/musb_hcd.c
>>>>        } 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".

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

> Can Linux be wrong too?

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

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

> Kind regards,

> Remy

WBR, Sergei

[U-Boot] [PATCH] usb: musb: only write CLRDATATOG when appropriate

[Mike Frysinger]

On Friday, August 13, 2010 06:25:05 Sergei Shtylyov wrote:
> Remy Bohmer wrote:
> > 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 

[U-Boot] [PATCH] usb: musb: only write CLRDATATOG when appropriate

[Remy Bohmer]

Hi,

2010/10/11 Mike Frysinger <vapier@gentoo.org>:
> On Friday, August 13, 2010 06:25:05 Sergei Shtylyov wrote:
>> Remy Bohmer wrote:
>> > 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