[PATCH v2 0/3] make Xilinx System ACE driver arch-independent

[PATCH v2 0/3] make Xilinx System ACE driver arch-independent

[Alexey Brodkin]

In v2 I put together 3 patches:
1. Blind replace of PPC/Microblaze-specific accessors with generic ones.
2. Fix functions that access data via dedicated register in SystemACE controller
so now CPU accesses data with native endianess.
3. Remove dependency on PPC/Microblaze - i.e. make driver arch-independent.

Alexey Brodkin (3):
  drivers/block/xsysace - replace in(out)_8/in(out)_be16/in(out)_le16
    with generic iowrite(read)8/16(be)
  drivers/block/xsysace - use "_rep" accessors with CPU endianess for
    data
  drivers/block - make Xilinx SystemACE driver arch-independent

 drivers/block/Kconfig   |    1 -
 drivers/block/xsysace.c |   74 ++++++++++++++---------------------------------
 2 files changed, 21 insertions(+), 54 deletions(-)


Cc: Vineet Gupta <vgupta@synopsys.com>
Cc: Mischa Jonker <mjonker@synopsys.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
-- 
1.7.10.4

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

[Alexey Brodkin]

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.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>

Cc: Vineet Gupta <vgupta@synopsys.com>
Cc: Mischa Jonker <mjonker@synopsys.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/block/xsysace.c |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
index 1f38643..64fd3c0 100644
--- a/drivers/block/xsysace.c
+++ b/drivers/block/xsysace.c
@@ -232,14 +232,14 @@ struct ace_reg_ops {
 static u16 ace_in_8(struct ace_device *ace, int reg)
 {
 	void __iomem *r = ace->baseaddr + reg;
-	return in_8(r) | (in_8(r + 1) << 8);
+	return ioread8(r) | (ioread8(r + 1) << 8);
 }
 
 static void ace_out_8(struct ace_device *ace, int reg, u16 val)
 {
 	void __iomem *r = ace->baseaddr + reg;
-	out_8(r, val);
-	out_8(r + 1, val >> 8);
+	iowrite8(val, r);
+	iowrite8(val >> 8, r + 1);
 }
 
 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;
 }
 
@@ -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;
 }
 
@@ -272,12 +272,12 @@ static struct ace_reg_ops ace_reg_8_ops = {
 /* 16 bit big endian bus attachment */
 static u16 ace_in_be16(struct ace_device *ace, int reg)
 {
-	return in_be16(ace->baseaddr + reg);
+	return ioread16be(ace->baseaddr + reg);
 }
 
 static void ace_out_be16(struct ace_device *ace, int reg, u16 val)
 {
-	out_be16(ace->baseaddr + reg, val);
+	iowrite16be(val, ace->baseaddr + reg);
 }
 
 static void ace_datain_be16(struct ace_device *ace)
@@ -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;
 }
 
 /* 16 bit little endian bus attachment */
 static u16 ace_in_le16(struct ace_device *ace, int reg)
 {
-	return in_le16(ace->baseaddr + reg);
+	return ioread16(ace->baseaddr + reg);
 }
 
 static void ace_out_le16(struct ace_device *ace, int reg, u16 val)
 {
-	out_le16(ace->baseaddr + reg, val);
+	iowrite16(val, ace->baseaddr + reg);
 }
 
 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;
 }
 
-- 
1.7.10.4

[PATCH v2 2/3] drivers/block/xsysace - use "_rep" accessors with CPU endianess for data

[Alexey Brodkin]

Initially different data accessors were used for LE abd BE CPUs:
"ioread16" in "ace_datain_be16" and "ioread16be" in "ace_datain_le16".
The same with writes.

While it worked in some cases (for example on BE PPC) it didn't work in
others (LE ARC).

Mentioned functions access data (by 16-bit chunks) from storage (i.e.
CompactFlash card) via DATABUFREG of Xilinx SystemACE CF controller.
And to interpret data properly CPU needs to access data in DATABUFREG
with native endianess.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>

Cc: Vineet Gupta <vgupta@synopsys.com>
Cc: Mischa Jonker <mjonker@synopsys.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/block/xsysace.c |   60 +++++++++++------------------------------------
 1 file changed, 14 insertions(+), 46 deletions(-)

diff --git a/drivers/block/xsysace.c b/drivers/block/xsysace.c
index 64fd3c0..af973aa 100644
--- a/drivers/block/xsysace.c
+++ b/drivers/block/xsysace.c
@@ -244,22 +244,14 @@ static void ace_out_8(struct ace_device *ace, int reg, u16 val)
 
 static void ace_datain_8(struct ace_device *ace)
 {
-	void __iomem *r = ace->baseaddr + 0x40;
-	u8 *dst = ace->data_ptr;
-	int i = ACE_FIFO_SIZE;
-	while (i--)
-		*dst++ = ioread8(r++);
-	ace->data_ptr = dst;
+	ioread8_rep(ace->baseaddr + 0x40, ace->data_ptr, ACE_FIFO_SIZE);
+	ace->data_ptr += ACE_FIFO_SIZE;
 }
 
 static void ace_dataout_8(struct ace_device *ace)
 {
-	void __iomem *r = ace->baseaddr + 0x40;
-	u8 *src = ace->data_ptr;
-	int i = ACE_FIFO_SIZE;
-	while (i--)
-		iowrite8(*src++, r++);
-	ace->data_ptr = src;
+	iowrite8_rep(ace->baseaddr + 0x40, ace->data_ptr, ACE_FIFO_SIZE);
+	ace->data_ptr += ACE_FIFO_SIZE;
 }
 
 static struct ace_reg_ops ace_reg_8_ops = {
@@ -280,22 +272,16 @@ static void ace_out_be16(struct ace_device *ace, int reg, u16 val)
 	iowrite16be(val, ace->baseaddr + reg);
 }
 
-static void ace_datain_be16(struct ace_device *ace)
+static void ace_datain_16(struct ace_device *ace)
 {
-	int i = ACE_FIFO_SIZE / 2;
-	u16 *dst = ace->data_ptr;
-	while (i--)
-		*dst++ = ioread16(ace->baseaddr + 0x40);
-	ace->data_ptr = dst;
+	ioread16_rep(ace->baseaddr + 0x40, ace->data_ptr, ACE_FIFO_SIZE / 2);
+	ace->data_ptr += ACE_FIFO_SIZE;
 }
 
-static void ace_dataout_be16(struct ace_device *ace)
+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;
+	iowrite16_rep(ace->baseaddr + 0x40, ace->data_ptr, ACE_FIFO_SIZE / 2);
+	ace->data_ptr += ACE_FIFO_SIZE;
 }
 
 /* 16 bit little endian bus attachment */
@@ -309,36 +295,18 @@ static void ace_out_le16(struct ace_device *ace, int reg, u16 val)
 	iowrite16(val, ace->baseaddr + reg);
 }
 
-static void ace_datain_le16(struct ace_device *ace)
-{
-	int i = ACE_FIFO_SIZE / 2;
-	u16 *dst = ace->data_ptr;
-	while (i--)
-		*dst++ = ioread16be(ace->baseaddr + 0x40);
-	ace->data_ptr = dst;
-}
-
-static void ace_dataout_le16(struct ace_device *ace)
-{
-	int i = ACE_FIFO_SIZE / 2;
-	u16 *src = ace->data_ptr;
-	while (i--)
-		iowrite16be(*src++, ace->baseaddr + 0x40);
-	ace->data_ptr = src;
-}
-
 static struct ace_reg_ops ace_reg_be16_ops = {
 	.in = ace_in_be16,
 	.out = ace_out_be16,
-	.datain = ace_datain_be16,
-	.dataout = ace_dataout_be16,
+	.datain = ace_datain_16,
+	.dataout = ace_dataout_16,
 };
 
 static struct ace_reg_ops ace_reg_le16_ops = {
 	.in = ace_in_le16,
 	.out = ace_out_le16,
-	.datain = ace_datain_le16,
-	.dataout = ace_dataout_le16,
+	.datain = ace_datain_16,
+	.dataout = ace_dataout_16,
 };
 
 static inline u16 ace_in(struct ace_device *ace, int reg)
-- 
1.7.10.4

[PATCH v2 3/3] drivers/block - make Xilinx SystemACE driver arch-independent

[Alexey Brodkin]

With generic accessors Xilinx SystemACE driver is no longer limited to
PPC/Microblaze architecures. It might be built and used on any
architecture now.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>

Cc: Vineet Gupta <vgupta@synopsys.com>
Cc: Mischa Jonker <mjonker@synopsys.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/block/Kconfig |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index b81ddfe..746d2f7 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -460,7 +460,6 @@ source "drivers/s390/block/Kconfig"
 
 config XILINX_SYSACE
 	tristate "Xilinx SystemACE support"
-	depends on 4xx || MICROBLAZE
 	help
 	  Include support for the Xilinx SystemACE CompactFlash interface
 
-- 
1.7.10.4

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

[Arnd Bergmann]

On Monday 24 June 2013 12:26:02 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.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> 
> Cc: Vineet Gupta <vgupta@synopsys.com>
> Cc: Mischa Jonker <mjonker@synopsys.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Michal Simek <monstr@monstr.eu>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH v2 2/3] drivers/block/xsysace - use "_rep" accessors with CPU endianess for data

[Arnd Bergmann]

On Monday 24 June 2013 12:26:03 Alexey Brodkin wrote:
> Initially different data accessors were used for LE abd BE CPUs:
> "ioread16" in "ace_datain_be16" and "ioread16be" in "ace_datain_le16".
> The same with writes.
> 
> While it worked in some cases (for example on BE PPC) it didn't work in
> others (LE ARC).
> 
> Mentioned functions access data (by 16-bit chunks) from storage (i.e.
> CompactFlash card) via DATABUFREG of Xilinx SystemACE CF controller.
> And to interpret data properly CPU needs to access data in DATABUFREG
> with native endianess.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> 
> Cc: Vineet Gupta <vgupta@synopsys.com>
> Cc: Mischa Jonker <mjonker@synopsys.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Michal Simek <monstr@monstr.eu>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH v2 0/3] make Xilinx System ACE driver arch-independent

[Alexey Brodkin]

On 06/24/2013 12:26 PM, Alexey Brodkin wrote:
> In v2 I put together 3 patches:
> 1. Blind replace of PPC/Microblaze-specific accessors with generic ones.
> 2. Fix functions that access data via dedicated register in SystemACE controller
> so now CPU accesses data with native endianess.
> 3. Remove dependency on PPC/Microblaze - i.e. make driver arch-independent.

Any comments on this patch-set?

-Alexey

Re: [PATCH v2 2/3] drivers/block/xsysace - use "_rep" accessors with CPU endianess for data

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1419 bytes --]

On 06/24/2013 10:26 AM, Alexey Brodkin wrote:
> Initially different data accessors were used for LE abd BE CPUs:
> "ioread16" in "ace_datain_be16" and "ioread16be" in "ace_datain_le16".
> The same with writes.
> 
> While it worked in some cases (for example on BE PPC) it didn't work in
> others (LE ARC).

I am not sure about this. It seems to me that what you need to do
is swapped wires in your hw design to use the same configuration
as is used on ppc and microblaze for data access.


> Mentioned functions access data (by 16-bit chunks) from storage (i.e.
> CompactFlash card) via DATABUFREG of Xilinx SystemACE CF controller.
> And to interpret data properly CPU needs to access data in DATABUFREG
> with native endianess.

I have had a lot of discussions about using native endianess.
This driver supports endian detection on register side
but not on data side.
Is this soft IP? If yes then just swapped wires on bus and use
standard configuration.

Grant is driver owner and he has to decide if this is acceptable
or not.
I can test it on microblaze hw.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2 2/3] drivers/block/xsysace - use "_rep" accessors with CPU endianess for data

[Alexey Brodkin]

On 06/25/2013 09:58 AM, Michal Simek wrote:
> On 06/24/2013 10:26 AM, Alexey Brodkin wrote:
>> Initially different data accessors were used for LE abd BE CPUs:
>> "ioread16" in "ace_datain_be16" and "ioread16be" in "ace_datain_le16".
>> The same with writes.
>>
>> While it worked in some cases (for example on BE PPC) it didn't work in
>> others (LE ARC).
>
> I am not sure about this. It seems to me that what you need to do
> is swapped wires in your hw design to use the same configuration
> as is used on ppc and microblaze for data access.
 >
>> Mentioned functions access data (by 16-bit chunks) from storage (i.e.
>> CompactFlash card) via DATABUFREG of Xilinx SystemACE CF controller.
>> And to interpret data properly CPU needs to access data in DATABUFREG
>> with native endianess.
>
> I have had a lot of discussions about using native endianess.
> This driver supports endian detection on register side
> but not on data side.
> Is this soft IP? If yes then just swapped wires on bus and use
> standard configuration.

I don't think there's a wiring problem.
For starters "Xilinx SystemACE CF controller" (at least the one I'm 
dealing with on "Xilinx ML-509" board) is a real hardware IC (with part 
number XCCACE-TQ144I).

And what about your HW? Is your SystemACE controller is a soft-IP?

As described in corresponding datasheet 
(http://www.xilinx.com/support/documentation/data_sheets/ds080.pdf) 
CF-card is connected to this IC directly - so CPU itself doesn't have 
any connection to CF.

CPU only can access (read/write) SystemACE's registers and by these actions:
1. Config SystemACE or read its configuration and status (registers 
0x00-0x1d).
2. Read/write data from/to CF-card (register 0x40).

And as long as configuration/status registers access is proven to work I 
expect access to data via just reads/writes from/to another same 
register should work as well.

> Grant is driver owner and he has to decide if this is acceptable
> or not.

As far as I may see from latest MAINTAINERS file grant is no longer 
maintains it. So who may take any decision now? Arnd?

> I can test it on microblaze hw.

Would be very nice and helpful if you give it a shot on Microblaze HW.
BTW what is an endianess of this HW? Only BE or both BE/LE?

Regards,
Alexey

Re: [PATCH v2 2/3] drivers/block/xsysace - use "_rep" accessors with CPU endianess for data

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 3989 bytes --]

On 06/25/2013 08:32 AM, Alexey Brodkin wrote:
> On 06/25/2013 09:58 AM, Michal Simek wrote:
>> On 06/24/2013 10:26 AM, Alexey Brodkin wrote:
>>> Initially different data accessors were used for LE abd BE CPUs:
>>> "ioread16" in "ace_datain_be16" and "ioread16be" in "ace_datain_le16".
>>> The same with writes.
>>>
>>> While it worked in some cases (for example on BE PPC) it didn't work in
>>> others (LE ARC).
>>
>> I am not sure about this. It seems to me that what you need to do
>> is swapped wires in your hw design to use the same configuration
>> as is used on ppc and microblaze for data access.
>  >
>>> Mentioned functions access data (by 16-bit chunks) from storage (i.e.
>>> CompactFlash card) via DATABUFREG of Xilinx SystemACE CF controller.
>>> And to interpret data properly CPU needs to access data in DATABUFREG
>>> with native endianess.
>>
>> I have had a lot of discussions about using native endianess.
>> This driver supports endian detection on register side
>> but not on data side.
>> Is this soft IP? If yes then just swapped wires on bus and use
>> standard configuration.
> 
> I don't think there's a wiring problem.
> For starters "Xilinx SystemACE CF controller" (at least the one I'm 
> dealing with on "Xilinx ML-509" board) is a real hardware IC (with part 
> number XCCACE-TQ144I).
> 
> And what about your HW? Is your SystemACE controller is a soft-IP?

bus-> sysace soft IP -> connetion to the real chip(MPD below) -> chip -> CF

In xilinx mhs file there are MPD pins which are data pins
and they are wired like this for 16bit mode
PORT fpga_0_SysACE_CompactFlash_SysACE_MPD_pin = fpga_0_SysACE_CompactFlash_SysACE_MPD_pin, DIR = IO, VEC = [15:0]
and like this for 8bit mode.
PORT SysACE_MPD = SysACE_MPD, DIR = IO, VEC = [7:0]

I am not sure about configuration but if you have something similar
what xilinx has then you should be simple able to change data pins
and this will reflect linux driver.

what configuration do you use?

> 
> As described in corresponding datasheet 
> (http://www.xilinx.com/support/documentation/data_sheets/ds080.pdf) 
> CF-card is connected to this IC directly - so CPU itself doesn't have 
> any connection to CF.

I am not talking about connection between system ace chip and CF.
xilinx configuration is done as I have described above where
we can setup data lines order. I haven't done any experiment with it
but seems to me straight forward to do it.

> CPU only can access (read/write) SystemACE's registers and by these actions:
> 1. Config SystemACE or read its configuration and status (registers 
> 0x00-0x1d).
> 2. Read/write data from/to CF-card (register 0x40).
> 
> And as long as configuration/status registers access is proven to work I 
> expect access to data via just reads/writes from/to another same 
> register should work as well.
>
>> Grant is driver owner and he has to decide if this is acceptable
>> or not.
> 
> As far as I may see from latest MAINTAINERS file grant is no longer 
> maintains it. So who may take any decision now? Arnd?

I should probably assign it to my me as the part of my microblaze
responsibility. And xilinx virtex as part of my work for xilinx.

MAINTAINERS: Update Grant's email address and maintainership
sha1: 19624236cce1b33a5d43895a92e3a9d438dc36e2


>> I can test it on microblaze hw.
> 
> Would be very nice and helpful if you give it a shot on Microblaze HW.
> BTW what is an endianess of this HW? Only BE or both BE/LE?

8bit BE/LE 16bit BE. I have one 16bit LE configuration too
but before I do testing on real HW let's clear your connection.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2 2/3] drivers/block/xsysace - use "_rep" accessors with CPU endianess for data

[Alexey Brodkin]

On 06/26/2013 01:31 PM, Michal Simek wrote:
> On 06/25/2013 08:32 AM, Alexey Brodkin wrote:
>> On 06/25/2013 09:58 AM, Michal Simek wrote:
>>> On 06/24/2013 10:26 AM, Alexey Brodkin wrote:
>>>> Initially different data accessors were used for LE abd BE CPUs:
>>>> "ioread16" in "ace_datain_be16" and "ioread16be" in "ace_datain_le16".
>>>> The same with writes.
>>>>
>>>> While it worked in some cases (for example on BE PPC) it didn't work in
>>>> others (LE ARC).
>>>
>>> I am not sure about this. It seems to me that what you need to do
>>> is swapped wires in your hw design to use the same configuration
>>> as is used on ppc and microblaze for data access.
>>   >
>>>> Mentioned functions access data (by 16-bit chunks) from storage (i.e.
>>>> CompactFlash card) via DATABUFREG of Xilinx SystemACE CF controller.
>>>> And to interpret data properly CPU needs to access data in DATABUFREG
>>>> with native endianess.
>>>
>>> I have had a lot of discussions about using native endianess.
>>> This driver supports endian detection on register side
>>> but not on data side.
>>> Is this soft IP? If yes then just swapped wires on bus and use
>>> standard configuration.
>>
>> I don't think there's a wiring problem.
>> For starters "Xilinx SystemACE CF controller" (at least the one I'm
>> dealing with on "Xilinx ML-509" board) is a real hardware IC (with part
>> number XCCACE-TQ144I).
>>
>> And what about your HW? Is your SystemACE controller is a soft-IP?
>
> bus-> sysace soft IP -> connetion to the real chip(MPD below) -> chip -> CF
>
> In xilinx mhs file there are MPD pins which are data pins
> and they are wired like this for 16bit mode
> PORT fpga_0_SysACE_CompactFlash_SysACE_MPD_pin = fpga_0_SysACE_CompactFlash_SysACE_MPD_pin, DIR = IO, VEC = [15:0]
> and like this for 8bit mode.
> PORT SysACE_MPD = SysACE_MPD, DIR = IO, VEC = [7:0]
>
> I am not sure about configuration but if you have something similar
> what xilinx has then you should be simple able to change data pins
> and this will reflect linux driver.
>
> what configuration do you use?

Well, we have our own BVCI-to-MPU bridge that maps SystemACE interface 
in CPU memory space (i.e. memory-mapped registers). This bridge also 
implements data read/write from/to SyatemACE FIFO. And frankly I cannot 
say how data is transferred to/from this FIFO.

I will need to look into bridge's Verilog to get all specifics.
but I'm not an expert in HW/Verilog so not sure how useful this 
excursion might be.

So no good clarification on this yet, sorry.

>> As described in corresponding datasheet
>> (http://www.xilinx.com/support/documentation/data_sheets/ds080.pdf)
>> CF-card is connected to this IC directly - so CPU itself doesn't have
>> any connection to CF.
>
> I am not talking about connection between system ace chip and CF.
> xilinx configuration is done as I have described above where
> we can setup data lines order. I haven't done any experiment with it
> but seems to me straight forward to do it.
>
>> CPU only can access (read/write) SystemACE's registers and by these actions:
>> 1. Config SystemACE or read its configuration and status (registers
>> 0x00-0x1d).
>> 2. Read/write data from/to CF-card (register 0x40).
>>
>> And as long as configuration/status registers access is proven to work I
>> expect access to data via just reads/writes from/to another same
>> register should work as well.
>>
>>> Grant is driver owner and he has to decide if this is acceptable
>>> or not.
>>
>> As far as I may see from latest MAINTAINERS file grant is no longer
>> maintains it. So who may take any decision now? Arnd?
>
> I should probably assign it to my me as the part of my microblaze
> responsibility. And xilinx virtex as part of my work for xilinx.
>
> MAINTAINERS: Update Grant's email address and maintainership
> sha1: 19624236cce1b33a5d43895a92e3a9d438dc36e2
>
>
>>> I can test it on microblaze hw.
>>
>> Would be very nice and helpful if you give it a shot on Microblaze HW.
>> BTW what is an endianess of this HW? Only BE or both BE/LE?
>
> 8bit BE/LE 16bit BE. I have one 16bit LE configuration too
> but before I do testing on real HW let's clear your connection.

If this won't take much time I will appreciate if you try proposed patch 
at least on 1 pair of LE/BE hw.

If it doesn't work then data accessors might need to be implemented the 
same way they were initially done. And in this case my v1 patch may make 
sense (replace accessors with generic versions).

Regards,
Alexey