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

[Arnd Bergmann <arnd@arndb.de>]

On Friday 21 June 2013, Alexey Brodkin wrote:
> in(out)_8/in(out)_be16/in(out)_le16 are very powerpc/microblaze
> specific. To enable use of Xilinx System ACE driver build for other
> architectures (for example it's possible to use it on Xilinx ml-509
> board with ARC700 CPU in FPGA) we need to use generic implementation of
> accessors.
> 
> I posted this patch in January this year in general mailing list
> (linux-kernel@vger.kernel.org) and it got discussed. But with no
> conclusions/actions.
> 
> Now I'm sending it to arch list trying to resolve an issue above
> (dependency on another architecture - I'd like this driver to be
> arch-independent).
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> 
>  static void ace_datain_8(struct ace_device *ace)
> @@ -248,7 +248,7 @@ static void ace_datain_8(struct ace_device *ace)
>  	u8 *dst = ace->data_ptr;
>  	int i = ACE_FIFO_SIZE;
>  	while (i--)
> -		*dst++ = in_8(r++);
> +		*dst++ = ioread8(r++);
>  	ace->data_ptr = dst;
>  }

Why not just use ioread8_rep() to replace the loop?

> @@ -258,7 +258,7 @@ static void ace_dataout_8(struct ace_device *ace)
>  	u8 *src = ace->data_ptr;
>  	int i = ACE_FIFO_SIZE;
>  	while (i--)
> -		out_8(r++, *src++);
> +		iowrite8(*src++, r++);
>  	ace->data_ptr = src;
>  }

and iowrite_8

> @@ -285,7 +285,7 @@ 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);
> +		*dst++ = ioread16(ace->baseaddr + 0x40);
>  	ace->data_ptr = dst;
>  }
> @@ -294,19 +294,19 @@ static void ace_dataout_be16(struct ace_device *ace)
>  	int i = ACE_FIFO_SIZE / 2;
>  	u16 *src = ace->data_ptr;
>  	while (i--)
> -		out_le16(ace->baseaddr + 0x40, *src++);
> +		iowrite16(*src++, ace->baseaddr + 0x40);
>  	ace->data_ptr = src;
>  }

ioread16_rep/iowrite16_rep

>  static void ace_datain_le16(struct ace_device *ace)
> @@ -314,7 +314,7 @@ static void ace_datain_le16(struct ace_device *ace)
>  	int i = ACE_FIFO_SIZE / 2;
>  	u16 *dst = ace->data_ptr;
>  	while (i--)
> -		*dst++ = in_be16(ace->baseaddr + 0x40);
> +		*dst++ = ioread16be(ace->baseaddr + 0x40);
>  	ace->data_ptr = dst;
>  }
>  
> @@ -323,7 +323,7 @@ static void ace_dataout_le16(struct ace_device *ace)
>  	int i = ACE_FIFO_SIZE / 2;
>  	u16 *src = ace->data_ptr;
>  	while (i--)
> -		out_be16(ace->baseaddr + 0x40, *src++);
> +		iowrite16be(*src++, ace->baseaddr + 0x40);
>  	ace->data_ptr = src;
>  }

ioread16_rep/iowrite16_rep.

I guess the last two modifications are actually needed for correct
operation independent of CPU endianess. I don't what led to the
combination of big- and little-endian accessors, but my guess is
that it was trial-and-error together with cut-and-paste. The point
is that the single register access should match the endianess of
the device while the streaming access should match the endianess
of the CPU.

	Arnd