Re: UDC support for MIPS/AU1200 and Geode/CS5536

Re: UDC support for MIPS/AU1200 and Geode/CS5536

[Jordan Crouse]

On 09/01/06 11:03 -0700, Jordan Crouse wrote:

> From the "two-birds-one-stone" department, I am pleased to present USB UDC
> support for both the MIPS Au1200 SoC and the Geode CS5535 south bridge.  
                                                     ^^^^^^
Of course, I meant the CS5536 - this *won't* work for the CS5535.

Jordan

-- 
Jordan Crouse
Senior Linux Engineer
AMD - Personal Connectivity Solutions Group
<www.amd.com/embeddedprocessors>

Re: [linux-usb-devel] [PATCH] UDC support for MIPS/AU1200 and Geode/CS5536

[Oliver Neukum]

Am Montag, 9. Januar 2006 19:03 schrieb Jordan Crouse:
> >From the "two-birds-one-stone" department, I am pleased to present USB UDC
> support for both the MIPS Au1200 SoC and the Geode CS5535 south bridge.  
> Also, coming soon (in the next few days), OTG, which has been removed from
> the usb_host patch, and put into its own patch (as per David's comments).
> 
> This patch is against current linux-mips git, but it should apply for Linus's
> tree as well.
> 
> Regards,
> Jordan
> 
+        VDBG("udc_read_bytes(): %d bytes\n", bytes);
+
+        /* dwords first */
+        for (i = 0; i < bytes / UDC_DWORD_BYTES; i++) {
+               *((u32*) (buf + (i<<2))) = readl(dev->rxfifo); 
+        }

Is there any reason you don't increment by 4?

	Regards
		Oliver

Re: [linux-usb-devel] [PATCH] UDC support for MIPS/AU1200 and Geode/CS5536

[Thomas Dahlmann]

Oliver Neukum wrote:

>Am Montag, 9. Januar 2006 19:03 schrieb Jordan Crouse:
>  
>
>>>From the "two-birds-one-stone" department, I am pleased to present USB UDC
>>support for both the MIPS Au1200 SoC and the Geode CS5535 south bridge.  
>>Also, coming soon (in the next few days), OTG, which has been removed from
>>the usb_host patch, and put into its own patch (as per David's comments).
>>
>>This patch is against current linux-mips git, but it should apply for Linus's
>>tree as well.
>>
>>Regards,
>>Jordan
>>
>>    
>>
>+        VDBG("udc_read_bytes(): %d bytes\n", bytes);
>+
>+        /* dwords first */
>+        for (i = 0; i < bytes / UDC_DWORD_BYTES; i++) {
>+               *((u32*) (buf + (i<<2))) = readl(dev->rxfifo); 
>+        }
>
>Is there any reason you don't increment by 4?
>
>	Regards
>		Oliver
>
>
>
>  
>
The loop is for reading dwords only, so "i < bytes / UDC_DWORD_BYTES" cuts
off remaining 1,2 or 3 bytes which are handled by the next loop.
But you are right, incrementing by 4 may look better,  as

        for (i = 0; i < bytes - bytes % UDC_DWORD_BYTES; i+=4) {
               *((u32*) (buf + i)) = readl(dev->rxfifo); 
        }


Thanks,
Thomas

Re: [linux-usb-devel] [PATCH] UDC support for MIPS/AU1200 and Geode/CS5536

[David Vrabel]

Thomas Dahlmann wrote:
> 
> The loop is for reading dwords only, so "i < bytes / UDC_DWORD_BYTES" cuts
> off remaining 1,2 or 3 bytes which are handled by the next loop.
> But you are right, incrementing by 4 may look better,  as
> 
>        for (i = 0; i < bytes - bytes % UDC_DWORD_BYTES; i+=4) {

    for (i = 0; i <= bytes - UDC_DWORD_BYTES; i += 4) {

?

David Vrabel

Re: [linux-usb-devel] [PATCH] UDC support for MIPS/AU1200 and Geode/CS5536

[Oliver Neukum]

Am Dienstag, 10. Januar 2006 12:02 schrieb Thomas Dahlmann:
> 
> Oliver Neukum wrote:
> 
> >Am Montag, 9. Januar 2006 19:03 schrieb Jordan Crouse:
> >  
> >
> >>>From the "two-birds-one-stone" department, I am pleased to present USB UDC
> >>support for both the MIPS Au1200 SoC and the Geode CS5535 south bridge.  
> >>Also, coming soon (in the next few days), OTG, which has been removed from
> >>the usb_host patch, and put into its own patch (as per David's comments).
> >>
> >>This patch is against current linux-mips git, but it should apply for Linus's
> >>tree as well.
> >>
> >>Regards,
> >>Jordan
> >>
> >>    
> >>
> >+        VDBG("udc_read_bytes(): %d bytes\n", bytes);
> >+
> >+        /* dwords first */
> >+        for (i = 0; i < bytes / UDC_DWORD_BYTES; i++) {
> >+               *((u32*) (buf + (i<<2))) = readl(dev->rxfifo); 
> >+        }
> >
> >Is there any reason you don't increment by 4?
> >
> >	Regards
> >		Oliver
> >
> >
> >
> >  
> >
> The loop is for reading dwords only, so "i < bytes / UDC_DWORD_BYTES" cuts
> off remaining 1,2 or 3 bytes which are handled by the next loop.
> But you are right, incrementing by 4 may look better,  as
> 
>         for (i = 0; i < bytes - bytes % UDC_DWORD_BYTES; i+=4) {
>                *((u32*) (buf + i)) = readl(dev->rxfifo); 
>         }

Not only will it look better, but it'll save you a shift operation.
You might even compute start and finish values before the loop and
save an addition in the body.

	Regards
		Oliver

Re: [linux-usb-devel] [PATCH] UDC support for MIPS/AU1200 and Geode/CS5536

[Thomas Dahlmann]

Oliver Neukum wrote:

>Am Dienstag, 10. Januar 2006 12:02 schrieb Thomas Dahlmann:
>  
>
>>Oliver Neukum wrote:
>>
>>    
>>
>>>Am Montag, 9. Januar 2006 19:03 schrieb Jordan Crouse:
>>> 
>>>
>>>      
>>>
>>>>>From the "two-birds-one-stone" department, I am pleased to present USB UDC
>>>>support for both the MIPS Au1200 SoC and the Geode CS5535 south bridge.  
>>>>Also, coming soon (in the next few days), OTG, which has been removed from
>>>>the usb_host patch, and put into its own patch (as per David's comments).
>>>>
>>>>This patch is against current linux-mips git, but it should apply for Linus's
>>>>tree as well.
>>>>
>>>>Regards,
>>>>Jordan
>>>>
>>>>   
>>>>
>>>>        
>>>>
>>>+        VDBG("udc_read_bytes(): %d bytes\n", bytes);
>>>+
>>>+        /* dwords first */
>>>+        for (i = 0; i < bytes / UDC_DWORD_BYTES; i++) {
>>>+               *((u32*) (buf + (i<<2))) = readl(dev->rxfifo); 
>>>+        }
>>>
>>>Is there any reason you don't increment by 4?
>>>
>>>	Regards
>>>		Oliver
>>>
>>>
>>>
>>> 
>>>
>>>      
>>>
>>The loop is for reading dwords only, so "i < bytes / UDC_DWORD_BYTES" cuts
>>off remaining 1,2 or 3 bytes which are handled by the next loop.
>>But you are right, incrementing by 4 may look better,  as
>>
>>        for (i = 0; i < bytes - bytes % UDC_DWORD_BYTES; i+=4) {
>>               *((u32*) (buf + i)) = readl(dev->rxfifo); 
>>        }
>>    
>>
>
>Not only will it look better, but it'll save you a shift operation.
>You might even compute start and finish values before the loop and
>save an addition in the body.
>
>	Regards
>		Oliver
>
>
>
>  
>
I have changed this for the next patch release. Thanks for the input !

Regards,
Thomas