Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16with generic iowrite(read)8/16(be)

[Grant Likely]

On Thu, Feb 7, 2013 at 5:22 PM, Alexey Brodkin
<Alexey.Brodkin@xxxxxxxxxxxx> wrote:
> On 02/07/2013 09:16 PM, Grant Likely wrote:
>>
>> On Thu, Feb 7, 2013 at 4:56 PM, Alexey Brodkin
>> <Alexey.Brodkin@xxxxxxxxxxxx> wrote:
>>>
>>> On 02/07/2013 08:44 PM, Grant Likely wrote:
>>>>
>>>> So, if I'm correct that means that for the data port (specifically
>>>> copying between RAM and the data port) using ioread16/iowrite16 on the
>>>> data port results in the correct behaviour. It also means that LE
>>>> support in the current driver is broken.
>>>
>>>
>>> That matches my earlier note when I wrote that for correct work on LE
>>> (note
>>> I'm on ARC, not PPC/MB) I needed to use "io{read|write}16" in
>>> "ace_data{in|out}_le16".
>>>
>>> With original "io{read|write}16be" instead data was corrupted.
>>
>>
>> In which case your bug-fix patch should drop the
>> ace_datain_le16/ace_dataout_le16 variants entirely and use the be16
>> ones for both (renaming appropriately).
>>
>> g.
>>
>> --
>> Grant Likely, B.Sc., P.Eng.
>> Secret Lab Technologies Ltd.
>>
>
> Sorry, do you mean to replace original lines:
> =======
>
> static void ace_datain_be16(struct ace_device *ace)
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *dst = ace->data_ptr;
>         while (i--)
>                 *dst++ = in_le16(ace->baseaddr + 0x40);
>         ace->data_ptr = dst;
>
> }
>
> static void ace_dataout_be16(struct ace_device *ace)
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *src = ace->data_ptr;
>         while (i--)
>                 ioread16(*src++, ace->baseaddr + 0x40);
>         ace->data_ptr = src;
> }
> =======
>
> with something like:
> =======
> static void ace_datain_16(struct ace_device *ace)
>
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *dst = ace->data_ptr;
>         while (i--)
>                 *dst++ = in_le16(ace->baseaddr + 0x40);
>         ace->data_ptr = dst;
> }
>
> static void ace_dataout_16(struct ace_device *ace)
>
> {
>         int i = ACE_FIFO_SIZE / 2;
>         u16 *src = ace->data_ptr;
>         while (i--)
>                 iowrite16(*src++, ace->baseaddr + 0x40);
>         ace->data_ptr = src;
> }

Ummm, I think you finger fumbled that because the above doesn't make sense.

I think that your original patch should be applied as-is first. It is
just a mechanical replacement of the ppc accessors with ioread/iowrite
variants. Nothing controversial there.

I was suggesting to use a second patch to drop
ace_datain_le16/ace_dataout_le16 and rename
ace_datain_be16/ace_dataout_be16 to ace_datain_16/ace_dataout_16.\,
and in that same patch you can switch the read loop to use
ioread16_rep/iowrite16_rep.

*However*... I think my first analysis is actually wrong for the BE
case. The current BE code works using ioread16() for the data port
which does a swap, but ioread16_rep() doesn't swap . I hadn't gone an
actually looked at how the _rep variants are implemented. (Thanks for
your help Ben). That means ioread16_rep will work fine for LE, but
we're still stuck with the slow loop on BE.

So, craft your bug fix for the LE case to use _rep variants for
ace_{datain,dataout}_le16() but don't change the BE support yet.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/