Re: [PATCH] usb: musb: Fix fifo reads for dm816x with musb_dsps

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

On 03/19/2015 08:55 PM, Tony Lindgren wrote:

>>>> --- a/drivers/usb/musb/musb_dsps.c
>>>> +++ b/drivers/usb/musb/musb_dsps.c
>>>> @@ -655,6 +655,36 @@ static int dsps_musb_reset(struct musb *musb)
>>>>       return !session_restart;
>>>>   }
>>>>
>>>> +/* Similar to am35x, dm81xx support only 32-bit read operation */
>>>> +static void dsps_read_fifo32(struct musb_hw_ep *hw_ep, u16 len, u8 *dst)
>>>> +{
>>>> +    void __iomem *fifo = hw_ep->fifo;
>>>> +    u32        val;
>>>> +    int        i;
>>>> +
>>>> +    /* Read for 32bit-aligned destination address */
>>>> +    if (likely((0x03 & (unsigned long)dst) == 0) && len >= 4) {
>>>> +        readsl(fifo, dst, len >> 2);
>>>> +        dst += len & ~0x03;
>>>> +        len &= 0x03;
>>>> +    }
>>>> +    /*
>>>> +     * Now read the remaining 1 to 3 byte or complete length if
>>>> +     * unaligned address.
>>>> +     */

>>>    This comment seems misplaced, it belongs before the next *if*.

>>>> +    if (len > 4) {
>>>> +        for (i = 0; i < (len >> 2); i++) {
>>>> +            *(u32 *)dst = musb_readl(fifo, 0);
>>>> +            dst += 4;
>>>> +        }

>>>     Not sure how this is different to using readsl().

>>     Ah, the default implementation of musb_readl() uses __raw_readl().
>> So you'd probably want to keep this loop, not readsl() call.

> Not sure I follow you here..

    I just wrongly thought readsl() uses readl() internally. readl() is 
supposed to swap bytes when needed (BE case), while __raw_readl() is not.

> Also include/asm-generic/io.h readsl()
> uses __raw_readl()?

    Looking at the arch/arm/include/asm/io.h, readsl() is equivalent to 
__raw_readsl() too. Forgot about this "asymmetry".

> It seems things work with what I posted, so a readsl() loop, then
> just read the remaining 1 to 3 bytes.

    In LE mode, there would have been no difference anyway.

> Regards,

> Tony

WBR, Sergei