Messages in this thread | | | From | Grant Likely <> | Date | Fri, 8 Feb 2013 07:45:14 +0000 | Subject | Re: [PATCH] drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16 with generic iowrite(read)8/16(be) |
| |
On Thu, Feb 7, 2013 at 5:22 PM, Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote: > On 02/07/2013 09:16 PM, Grant Likely wrote: >> >> On Thu, Feb 7, 2013 at 4:56 PM, Alexey Brodkin >> <Alexey.Brodkin@synopsys.com> 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.
| |