[RFC] SystemACE driver - abstract register ops

[RFC] SystemACE driver - abstract register ops

[John Williams]

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

Grant,

Thanks for your work on the SystemACE driver - I'll be porting/merging 
this across to MicroBlaze very shortly.

Given that SysACE can be hooked up in any number of ways, bit widths, 
endians, PPC/Microblaze, can you please offer your comments on the 
attached patch?

It introduce a private ace_reg_ops structure, with various member 
functions for the different kinds of accesses to the HW.

This patch should not change the functionality of your original driver 
at all, it's just groundwork for what's to come.

I recognise that it adds indirection into the various access paths, and 
potentially a little bloat.  Whether this is better than #if 
1...#else...#endif is debatable.

Similar issues will arise for most (all?) of the Xilinx drivers that we 
will share between PPC and MicroBlaze.  Hopefully we can converge on a 
nice consistent and clean way of handling these dual arch drivers.

Cheers,

John

[-- Attachment #2: xsysace-regops.patch --]
[-- Type: text/plain, Size: 5430 bytes --]

Index: linux-2.6.x-petalogix/drivers/block/xsysace.c
===================================================================
--- linux-2.6.x-petalogix/drivers/block/xsysace.c	(revision 2628)
+++ linux-2.6.x-petalogix/drivers/block/xsysace.c	(working copy)
@@ -73,6 +73,11 @@
  *    interrupt, then the kernel timer will expire and the driver can
  *    continue where it left off.
  *
+ *  SystemACE can be wired up in different endian orders and data widths.
+ *  It works on both PPC and MicroBlaze architectures.  For this reason,
+ *  an ace_reg_ops structure is used that abstracts away low level
+ *  endian/width/arch access to the HW registers.
+
  * To Do:
  *    - Add FPGA configuration control interface.
  *    - Request major number from lanana
@@ -161,32 +166,120 @@
 /* ---------------------------------------------------------------------
  * Low level register access
  */
+struct reg_ops {
+	u8 (*read8)(void *addr);
+	u16 (*read16)(void *addr);
+	u32 (*read32)(void *addr);
+	u32 (*readdata)(void *addr);
 
-/* register access macros */
+	void (*write16)(void *addr, u16 val);
+	void (*write32)(void *addr, u32 val);
+	void (*writedata)(void *addr, u32 val);
+};
+
+#define ace_reg_read8(ace, reg) (ace->ops->read8(ace->baseaddr + reg))
+#define ace_reg_read16(ace, reg) (ace->ops->read16(ace->baseaddr + reg))
+#define ace_reg_readdata(ace, reg) (ace->ops->readdata(ace->baseaddr + reg))
+#define ace_reg_read32(ace, reg) (ace->ops->read32(ace->baseaddr+reg))
+#define ace_reg_write16(ace, reg, val) (ace->ops->write16(ace->baseaddr+reg, val))
+#define ace_reg_writedata(ace, reg, val) (ace->ops->writedata(ace->baseaddr + reg, val))
+#define ace_reg_write32(ace, reg, val) (ace->ops->write32(ace->baseaddr+reg, val))
+
+/* register access functions */
 #if 1 /* Little endian 16-bit regs */
-#define ace_reg_read8(ace, reg) in_8(ace->baseaddr + reg)
-#define ace_reg_read16(ace, reg) in_le16(ace->baseaddr + reg)
-#define ace_reg_readdata(ace, reg) in_be16(ace->baseaddr + reg)
-#define ace_reg_read32(ace, reg) ((in_le16(ace->baseaddr + reg+2) << 16) | \
-                                  (in_le16(ace->baseaddr + reg)))
-#define ace_reg_write16(ace, reg, val) out_le16(ace->baseaddr + reg, val)
-#define ace_reg_writedata(ace, reg, val) out_be16(ace->baseaddr + reg, val)
-#define ace_reg_write32(ace, reg, val) { \
-                           out_le16(ace->baseaddr + reg+2, (val) >> 16); \
-                           out_le16(ace->baseaddr + reg, val); \
-                       }
+static u8 ace_le16_read8(void *addr)
+{
+	return in_8(addr);
+}
+
+static u16 ace_le16_read16(void *addr)
+{
+	return in_le16(addr);
+}
+
+static u32 ace_le16_read32(void *addr)
+{
+	return ((in_le16(addr+2) << 16) | (in_le16(addr)));
+}
+
+static u32 ace_le16_readdata(void *addr)
+{
+	return in_be16(addr);
+}
+
+static void ace_le16_write16(void *addr, u16 val)
+{
+	out_le16(addr, val);
+}
+
+static void ace_le16_write32(void *addr, u32 val)
+{
+	out_le16(addr+2,(val) >> 16); \
+	out_le16(addr, val);
+}
+
+static void ace_le16_writedata(void *addr, u32 val)
+{
+	out_be16(addr, val);
+}
+
+static struct reg_ops ace_ops = {
+	.read8	=	ace_le16_read8,
+	.read16 = 	ace_le16_read16,
+	.read32 = 	ace_le16_read32,
+	.readdata = 	ace_le16_readdata,
+	.write16 = 	ace_le16_write16,
+	.write32 = 	ace_le16_write32,
+	.writedata = 	ace_le16_writedata
+} ;
+
 #else /* Big endian 16-bit regs */
-#define ace_reg_read8(ace, reg) in_8(ace->baseaddr + reg)
-#define ace_reg_read16(ace, reg) in_be16(ace->baseaddr + reg)
-#define ace_reg_readdata(ace, reg) in_le16(ace->baseaddr + reg)
-#define ace_reg_read32(ace, reg) ((in_be16(ace->baseaddr + reg+2) << 16) | \
-                                  (in_be16(ace->baseaddr + reg)))
-#define ace_reg_write16(ace, reg, val) out_be16(ace->baseaddr + reg, val)
-#define ace_reg_writedata(ace, reg, val) out_le16(ace->baseaddr + reg, val)
-#define ace_reg_write32(ace, reg, val) { \
-                           out_be16(ace->baseaddr + reg+2, (val) >> 16); \
-                           out_be16(ace->baseaddr + reg, val); \
-                       }
+static u8 ace_be16_read8(void *addr)
+{
+	return in_8(addr);
+}
+
+static u16 ace_be16_read16(void *addr)
+{
+	return in_be16(addr);
+}
+
+static u32 ace_be16_read32(void *addr)
+{
+	return ((in_be16(addr+2) << 16) | (in_be16(addr)));
+}
+
+static u32 ace_be16_readdata(void *addr)
+{
+	return in_le16(addr);
+}
+
+static void ace_be16_write16(void *addr, u16 val)
+{
+	out_be16(addr, val);
+}
+
+static void ace_be16_write32(void *addr, u32 val)
+{
+	out_be16(addr+2,(val) >> 16); \
+	out_be16(addr, val);
+}
+
+static void ace_be16_writedata(void *addr, u32 val)
+{
+	out_le16(addr, val);
+}
+
+static struct reg_ops ace_ops = {
+	.read8	=	ace_be16_read8,
+	.read16 = 	ace_be16_read16,
+	.read32 = 	ace_be16_read32,
+	.readdata = 	ace_be16_readdata,
+	.write16 = 	ace_be16_write16,
+	.write32 = 	ace_be16_write32,
+	.writedata = 	ace_be16_writedata
+} ;
+
 #endif
 
 struct ace_device {
@@ -222,6 +315,9 @@
 	int bus_width;  /* 0 := 8 bit; 1 := 16 bit */
 	int lock_count;
 
+	/* Register access ops */
+	struct reg_ops *ops;
+
 	/* Block device data structures */
 	spinlock_t lock;
 	struct device *dev;
@@ -995,6 +1091,8 @@
 	ace->id = dev->id;
 	ace->irq = NO_IRQ;
 
+	ace->ops=&ace_ops;
+
 	for (i = 0; i < dev->num_resources; i++) {
 		if (dev->resource[i].flags & IORESOURCE_MEM)
 			ace->physaddr = dev->resource[i].start;

Re: [RFC] SystemACE driver - abstract register ops

[Grant Likely]

On 4/27/07, John Williams <jwilliams@itee.uq.edu.au> wrote:
> Grant,
>
> Thanks for your work on the SystemACE driver - I'll be porting/merging
> this across to MicroBlaze very shortly.
>
> Given that SysACE can be hooked up in any number of ways, bit widths,
> endians, PPC/Microblaze, can you please offer your comments on the
> attached patch?

HAHAHAHAHAHAHA!  Guess what I just finished writing before receiving your email.

I've got something very similar that I'm testing now, but thanks for
the patch.  I feel better that someone else has the same opinion on
how to hook up the bus.

Cheers,
g.

>
> It introduce a private ace_reg_ops structure, with various member
> functions for the different kinds of accesses to the HW.
>
> This patch should not change the functionality of your original driver
> at all, it's just groundwork for what's to come.
>
> I recognise that it adds indirection into the various access paths, and
> potentially a little bloat.  Whether this is better than #if
> 1...#else...#endif is debatable.
>
> Similar issues will arise for most (all?) of the Xilinx drivers that we
> will share between PPC and MicroBlaze.  Hopefully we can converge on a
> nice consistent and clean way of handling these dual arch drivers.
>
> Cheers,
>
> John
>
> Index: linux-2.6.x-petalogix/drivers/block/xsysace.c
> ===================================================================
> --- linux-2.6.x-petalogix/drivers/block/xsysace.c       (revision 2628)
> +++ linux-2.6.x-petalogix/drivers/block/xsysace.c       (working copy)
> @@ -73,6 +73,11 @@
>   *    interrupt, then the kernel timer will expire and the driver can
>   *    continue where it left off.
>   *
> + *  SystemACE can be wired up in different endian orders and data widths.
> + *  It works on both PPC and MicroBlaze architectures.  For this reason,
> + *  an ace_reg_ops structure is used that abstracts away low level
> + *  endian/width/arch access to the HW registers.
> +
>   * To Do:
>   *    - Add FPGA configuration control interface.
>   *    - Request major number from lanana
> @@ -161,32 +166,120 @@
>  /* ---------------------------------------------------------------------
>   * Low level register access
>   */
> +struct reg_ops {
> +       u8 (*read8)(void *addr);
> +       u16 (*read16)(void *addr);
> +       u32 (*read32)(void *addr);
> +       u32 (*readdata)(void *addr);
>
> -/* register access macros */
> +       void (*write16)(void *addr, u16 val);
> +       void (*write32)(void *addr, u32 val);
> +       void (*writedata)(void *addr, u32 val);
> +};
> +
> +#define ace_reg_read8(ace, reg) (ace->ops->read8(ace->baseaddr + reg))
> +#define ace_reg_read16(ace, reg) (ace->ops->read16(ace->baseaddr + reg))
> +#define ace_reg_readdata(ace, reg) (ace->ops->readdata(ace->baseaddr + reg))
> +#define ace_reg_read32(ace, reg) (ace->ops->read32(ace->baseaddr+reg))
> +#define ace_reg_write16(ace, reg, val) (ace->ops->write16(ace->baseaddr+reg, val))
> +#define ace_reg_writedata(ace, reg, val) (ace->ops->writedata(ace->baseaddr + reg, val))
> +#define ace_reg_write32(ace, reg, val) (ace->ops->write32(ace->baseaddr+reg, val))
> +
> +/* register access functions */
>  #if 1 /* Little endian 16-bit regs */
> -#define ace_reg_read8(ace, reg) in_8(ace->baseaddr + reg)
> -#define ace_reg_read16(ace, reg) in_le16(ace->baseaddr + reg)
> -#define ace_reg_readdata(ace, reg) in_be16(ace->baseaddr + reg)
> -#define ace_reg_read32(ace, reg) ((in_le16(ace->baseaddr + reg+2) << 16) | \
> -                                  (in_le16(ace->baseaddr + reg)))
> -#define ace_reg_write16(ace, reg, val) out_le16(ace->baseaddr + reg, val)
> -#define ace_reg_writedata(ace, reg, val) out_be16(ace->baseaddr + reg, val)
> -#define ace_reg_write32(ace, reg, val) { \
> -                           out_le16(ace->baseaddr + reg+2, (val) >> 16); \
> -                           out_le16(ace->baseaddr + reg, val); \
> -                       }
> +static u8 ace_le16_read8(void *addr)
> +{
> +       return in_8(addr);
> +}
> +
> +static u16 ace_le16_read16(void *addr)
> +{
> +       return in_le16(addr);
> +}
> +
> +static u32 ace_le16_read32(void *addr)
> +{
> +       return ((in_le16(addr+2) << 16) | (in_le16(addr)));
> +}
> +
> +static u32 ace_le16_readdata(void *addr)
> +{
> +       return in_be16(addr);
> +}
> +
> +static void ace_le16_write16(void *addr, u16 val)
> +{
> +       out_le16(addr, val);
> +}
> +
> +static void ace_le16_write32(void *addr, u32 val)
> +{
> +       out_le16(addr+2,(val) >> 16); \
> +       out_le16(addr, val);
> +}
> +
> +static void ace_le16_writedata(void *addr, u32 val)
> +{
> +       out_be16(addr, val);
> +}
> +
> +static struct reg_ops ace_ops = {
> +       .read8  =       ace_le16_read8,
> +       .read16 =       ace_le16_read16,
> +       .read32 =       ace_le16_read32,
> +       .readdata =     ace_le16_readdata,
> +       .write16 =      ace_le16_write16,
> +       .write32 =      ace_le16_write32,
> +       .writedata =    ace_le16_writedata
> +} ;
> +
>  #else /* Big endian 16-bit regs */
> -#define ace_reg_read8(ace, reg) in_8(ace->baseaddr + reg)
> -#define ace_reg_read16(ace, reg) in_be16(ace->baseaddr + reg)
> -#define ace_reg_readdata(ace, reg) in_le16(ace->baseaddr + reg)
> -#define ace_reg_read32(ace, reg) ((in_be16(ace->baseaddr + reg+2) << 16) | \
> -                                  (in_be16(ace->baseaddr + reg)))
> -#define ace_reg_write16(ace, reg, val) out_be16(ace->baseaddr + reg, val)
> -#define ace_reg_writedata(ace, reg, val) out_le16(ace->baseaddr + reg, val)
> -#define ace_reg_write32(ace, reg, val) { \
> -                           out_be16(ace->baseaddr + reg+2, (val) >> 16); \
> -                           out_be16(ace->baseaddr + reg, val); \
> -                       }
> +static u8 ace_be16_read8(void *addr)
> +{
> +       return in_8(addr);
> +}
> +
> +static u16 ace_be16_read16(void *addr)
> +{
> +       return in_be16(addr);
> +}
> +
> +static u32 ace_be16_read32(void *addr)
> +{
> +       return ((in_be16(addr+2) << 16) | (in_be16(addr)));
> +}
> +
> +static u32 ace_be16_readdata(void *addr)
> +{
> +       return in_le16(addr);
> +}
> +
> +static void ace_be16_write16(void *addr, u16 val)
> +{
> +       out_be16(addr, val);
> +}
> +
> +static void ace_be16_write32(void *addr, u32 val)
> +{
> +       out_be16(addr+2,(val) >> 16); \
> +       out_be16(addr, val);
> +}
> +
> +static void ace_be16_writedata(void *addr, u32 val)
> +{
> +       out_le16(addr, val);
> +}
> +
> +static struct reg_ops ace_ops = {
> +       .read8  =       ace_be16_read8,
> +       .read16 =       ace_be16_read16,
> +       .read32 =       ace_be16_read32,
> +       .readdata =     ace_be16_readdata,
> +       .write16 =      ace_be16_write16,
> +       .write32 =      ace_be16_write32,
> +       .writedata =    ace_be16_writedata
> +} ;
> +
>  #endif
>
>  struct ace_device {
> @@ -222,6 +315,9 @@
>         int bus_width;  /* 0 := 8 bit; 1 := 16 bit */
>         int lock_count;
>
> +       /* Register access ops */
> +       struct reg_ops *ops;
> +
>         /* Block device data structures */
>         spinlock_t lock;
>         struct device *dev;
> @@ -995,6 +1091,8 @@
>         ace->id = dev->id;
>         ace->irq = NO_IRQ;
>
> +       ace->ops=&ace_ops;
> +
>         for (i = 0; i < dev->num_resources; i++) {
>                 if (dev->resource[i].flags & IORESOURCE_MEM)
>                         ace->physaddr = dev->resource[i].start;
>
>


-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [RFC] SystemACE driver - abstract register ops

[Grant Likely]

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

On 4/27/07, John Williams <jwilliams@itee.uq.edu.au> wrote:
> Grant,
>
> Thanks for your work on the SystemACE driver - I'll be porting/merging
> this across to MicroBlaze very shortly.

Very cool; I hope it works well.

> Given that SysACE can be hooked up in any number of ways, bit widths,
> endians, PPC/Microblaze, can you please offer your comments on the
> attached patch?

Okay, after getting over my initial mirth about working on the *exact*
same thing and finishing it at the *exact* time that you sent me your
patch, I think I'm ready to make useful comments.  :-)
>
> It introduce a private ace_reg_ops structure, with various member
> functions for the different kinds of accesses to the HW.
>
> This patch should not change the functionality of your original driver
> at all, it's just groundwork for what's to come.
>
> I recognise that it adds indirection into the various access paths, and
> potentially a little bloat.  Whether this is better than #if
> 1...#else...#endif is debatable.

I'm not to concerned with the added redirection.  On my 405 designs, I
find that bus overhead has a far greater impact than any of the
processing paths in the driver, so this shouldn't be a problem.
Besides, when we finally move to arch/powerpc, it will become very
feasable to have a single kernel image that will boot on multiple
ppc405 FPGA configurations; just change the device tree passed in.

> Similar issues will arise for most (all?) of the Xilinx drivers that we
> will share between PPC and MicroBlaze.  Hopefully we can converge on a
> nice consistent and clean way of handling these dual arch drivers.

I agree 100%

For your reading pleasure, I've attached the bus attachment changes
that I've made in my tree.  I hope to get this driver accepted into
mainline during the 2.6.22 merge window; so please get any comments
you have back to me ASAP.

Cheers,
g.

[-- Attachment #2: 0001-SYSACE-Make-bus-binding-selectable-at-runtime.patch --]
[-- Type: application/x-patch, Size: 10652 bytes --]

Re: [RFC] SystemACE driver - abstract register ops

[Stefan Roese]

On Friday 27 April 2007 09:31, Grant Likely wrote:
> For your reading pleasure, I've attached the bus attachment changes
> that I've made in my tree.  I hope to get this driver accepted into
> mainline during the 2.6.22 merge window; so please get any comments
> you have back to me ASAP.

Works fine on my Katmai 440SPe system with SysACE connected in 16bit 
big-endian mode. Thanks.

Acked-by Stefan Roese <sr@denx.de>

Best regards,
Stefan

Re: [RFC] SystemACE driver - abstract register ops

[Wolfgang Reissnegger]

Hi All,

I wanted to say that I am VERY excited about all the great work that is 
happening here. As some of you know I am currently working on setting up 
a Linux kernel git tree that contains a multitude of Xilinx drivers such 
as IIC, SPI, GPIO, SYSACE, EMAC, EMACLITE, UARTLITE versions, FLASH as 
well support for the MicroBlaze processor.
In addition I pulled Grant's virtex-dev branch (with TEMAC and SYSACE). 
I will integrate those as soon as I'm done with the other work. I am 
already eying the FrameBuffer driver from Andrei.

The end goal is to have a kernel tree that contains ALL Xilinx drivers 
and the multitude of drivers that are being created/updated and 
contributed here. Having such a kernel available will simplify the 
process of bringing up a new system. The hope is that contributors will 
adopt the idea of having a "Xilinx" kernel and will start using that 
tree as a "base reference".

At this point the Xilinx kernel branch is based on version v2.6.20 with 
the uc0 patch applied, as well as the various LynuxWorks patches that 
support MicroBlaze and add the mentioned drivers. The LynuxWorks code 
has been cleaned up to not contain the proprietary hardcoded sections to 
make it more acceptable to the Open Source community. I'm sure there's 
more work to be done, but it's a start.

Once the git server is up and running we will be adding new drivers they 
become available and integrate patches and updates along the way.

Unfortunately, I spend a lot of my time now working the politics of 
getting a git server up and running and convincing management that this 
actually _is_ a good idea. It's slow going and there's a lot of meetings 
and discussions going on. But I'm still hopeful. As a backup I'm trying 
to get a kernel.org/git account so I can publish the tree there.

I will post news here as they unfold. In the meantime it would be very 
interesting to hear ideas and suggestions from you. Concerns that people 
have. Pitfalls to look out for etc.

Thanks,
    Wolfgang



Grant Likely wrote:
> On 4/27/07, John Williams <jwilliams@itee.uq.edu.au> wrote:
>> Grant,
>>
>> Thanks for your work on the SystemACE driver - I'll be porting/merging
>> this across to MicroBlaze very shortly.
> 
> Very cool; I hope it works well.
> 
>> Given that SysACE can be hooked up in any number of ways, bit widths,
>> endians, PPC/Microblaze, can you please offer your comments on the
>> attached patch?
> 
> Okay, after getting over my initial mirth about working on the *exact*
> same thing and finishing it at the *exact* time that you sent me your
> patch, I think I'm ready to make useful comments.  :-)
>>
>> It introduce a private ace_reg_ops structure, with various member
>> functions for the different kinds of accesses to the HW.
>>
>> This patch should not change the functionality of your original driver
>> at all, it's just groundwork for what's to come.
>>
>> I recognise that it adds indirection into the various access paths, and
>> potentially a little bloat.  Whether this is better than #if
>> 1...#else...#endif is debatable.
> 
> I'm not to concerned with the added redirection.  On my 405 designs, I
> find that bus overhead has a far greater impact than any of the
> processing paths in the driver, so this shouldn't be a problem.
> Besides, when we finally move to arch/powerpc, it will become very
> feasable to have a single kernel image that will boot on multiple
> ppc405 FPGA configurations; just change the device tree passed in.
> 
>> Similar issues will arise for most (all?) of the Xilinx drivers that we
>> will share between PPC and MicroBlaze.  Hopefully we can converge on a
>> nice consistent and clean way of handling these dual arch drivers.
> 
> I agree 100%
> 
> For your reading pleasure, I've attached the bus attachment changes
> that I've made in my tree.  I hope to get this driver accepted into
> mainline during the 2.6.22 merge window; so please get any comments
> you have back to me ASAP.
> 
> Cheers,
> g.
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Linuxppc-embedded mailing list
> Linuxppc-embedded@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-embedded

Re: [RFC] SystemACE driver - abstract register ops

[Andrei Konovalov]

Hi Grant,

Grant Likely wrote:
> For your reading pleasure, I've attached the bus attachment changes
> that I've made in my tree.  I hope to get this driver accepted into
> mainline during the 2.6.22 merge window; so please get any comments
> you have back to me ASAP.
> 
> Cheers,
> g.

ML300 + IBM microdrive.
8-bit, __BIG_ENDIAN
(ace->bus_width = 0;)

Have noticed a misprint (see below).
Correcting it seems to make the device geometry to be recognized OK, but then:

[    3.798449] xsysace xsa: Xilinx SystemACE revision 1.0.12
[    3.891228] xsysace xsa: capacity: 2104704 sectors
[    3.973193]  xsa:<4>xsysace xsa: kicking stalled fsm; state=3 task=1 iter=2 dc=0
[    6.103446] xsysace xsa: kicking stalled fsm; state=3 task=1 iter=2 dc=0
[    7.207342] xsysace xsa: kicking stalled fsm; state=3 task=1 iter=2 dc=0
[    8.311230] xsysace xsa: kicking stalled fsm; state=3 task=1 iter=2 dc=0
[    9.411113] xsysace xsa: kicking stalled fsm; state=3 task=1 iter=2 dc=0
[   10.511016] xsysace xsa: kicking stalled fsm; state=3 task=1 iter=2 dc=0

Will have a deeper look tomorrow.
And try CF card too.
At the moment not sure if this the driver issue.

Thanks,
Andrei


Index: linux-2.6.20/drivers/block/xsysace.c
===================================================================
--- linux-2.6.20.orig/drivers/block/xsysace.c
+++ linux-2.6.20/drivers/block/xsysace.c
@@ -240,9 +240,9 @@ static void ace_identin_8(struct ace_dev
  	int i = ACE_FIFO_SIZE/2;
  	while (i--)
  #if defined(__BIG_ENDIAN)
-		*ace->data_ptr = (in_8(r)) | (in_8(r+1)<<8);
+		*ace->data_ptr++ = (in_8(r)) | (in_8(r+1)<<8);
  #else
-		*ace->data_ptr = (in_8(r)<<8) | (in_8(r+1));
+		*ace->data_ptr++ = (in_8(r)<<8) | (in_8(r+1));
  #endif
  	ace->data_count--;
  }
@@ -253,9 +253,9 @@ static void ace_datain_8(struct ace_devi
  	int i = ACE_FIFO_SIZE/2;
  	while (i--)
  #if defined(__BIG_ENDIAN)
-		*ace->data_ptr = (in_8(r)<<8) | (in_8(r+1));
+		*ace->data_ptr++ = (in_8(r)<<8) | (in_8(r+1));
  #else
-		*ace->data_ptr = (in_8(r)) | (in_8(r+1)<<8);
+		*ace->data_ptr++ = (in_8(r)) | (in_8(r+1)<<8);
  #endif
  }

Re: [RFC] SystemACE driver - abstract register ops

[Grant Likely]

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

On 4/27/07, Andrei Konovalov <akonovalov@ru.mvista.com> wrote:
> [   10.511016] xsysace xsa: kicking stalled fsm; state=3 task=1 iter=2 dc=0
>
> Will have a deeper look tomorrow.
> And try CF card too.
> At the moment not sure if this the driver issue.

Oops, I messed it up; try the attached change (I had left in a bogus
data_count decrement).  Thanks for catching the ptr increment bug.

g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

[-- Attachment #2: 0001-SYSACE-Bug-fixes-to-8-bit-binding.patch --]
[-- Type: application/x-patch, Size: 1307 bytes --]

Re: [RFC] SystemACE driver - abstract register ops

[John Williams]

Grant,

Grant Likely wrote:
> On 4/27/07, John Williams <jwilliams@itee.uq.edu.au> wrote:
> 
>> Thanks for your work on the SystemACE driver - I'll be porting/merging
>> this across to MicroBlaze very shortly.
> 
> Very cool; I hope it works well.

Indeed it does - your latest patchset version of the systemACE driver 
"just works" on MicroBlaze 2.6.20

I have only tested the 16-bit buswidth (standard ML401 reference design).

> For your reading pleasure, I've attached the bus attachment changes
> that I've made in my tree.  I hope to get this driver accepted into
> mainline during the 2.6.22 merge window; so please get any comments
> you have back to me ASAP.

Acked-by: John Williams <jwilliams@itee.uq.edu.au>

John

Re: [RFC] SystemACE driver - abstract register ops

[Grant Likely]

On 4/30/07, John Williams <jwilliams@itee.uq.edu.au> wrote:
> Grant,
>
> Grant Likely wrote:
> > Very cool; I hope it works well.
>
> Indeed it does - your latest patchset version of the systemACE driver
> "just works" on MicroBlaze 2.6.20
>
> Acked-by: John Williams <jwilliams@itee.uq.edu.au>

Wonderful, thanks John.  I've got a few last little details that I'm
going to try and get sorted out in the next 24 hours or so, and then
I'll post the driver to the lkml.  If things go really well, it might
even get picked up for 2.6.22.  I'll make sure to CC you when I send
it.

Cheers,
g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [RFC] SystemACE driver - abstract register ops

[Peter Korsgaard]

>>>>> "WR" == Wolfgang Reissnegger <wolfgang.reissnegger@xilinx.com> writes:

Hi,

WR> The end goal is to have a kernel tree that contains ALL Xilinx
WR> drivers and the multitude of drivers that are being
WR> created/updated and contributed here. Having such a kernel
WR> available will simplify the process of bringing up a new
WR> system. The hope is that contributors will adopt the idea of
WR> having a "Xilinx" kernel and will start using that tree as a "base
WR> reference".

But there already is such a tree - It's called 'mainline'. I can see
some advantage in having a Xilinx tree as a staging area, but it
really HAS to be a temporary thing and stuff needs to be pushed to
mainline.

WR> I will post news here as they unfold. In the meantime it would be
WR> very interesting to hear ideas and suggestions from you. Concerns
WR> that people have. Pitfalls to look out for etc.

Hereby my 2 cents.

-- 
Bye, Peter Korsgaard

Re: [RFC] SystemACE driver - abstract register ops

[Wolfgang Reissnegger]

Hi Peter,

thanks for the feedback.

Yes, I agree. Having the code in mainline would be most preferable. 
However, there is always a delay between when people submit patches and 
when code actually does get into mainline. Also, it's not always 
guaranteed that the code is actually accepted into mainline. In the 
meantime there are a multitude of patches floating around which IMHO can 
be quite confusing.

The purpose of the "Xilinx git tree" is to provide a central place where 
those patches can be accumulated, including PPC and MicroBlaze. Also, as 
Xilinx is creating new IP cores, drivers for those cores will be 
directly added to that tree, eliminating the EDK->Linux horrors.

So, yes, I think it's partly a staging area for code that should make it 
into mainline, but it's also a repository for code that is not accepted 
into mainline (yet), or new, or otherwise useful to the community.

Wolfgang

Peter Korsgaard wrote:
>>>>>> "WR" == Wolfgang Reissnegger <wolfgang.reissnegger@xilinx.com> writes:
> 
> Hi,
> 
> WR> The end goal is to have a kernel tree that contains ALL Xilinx
> WR> drivers and the multitude of drivers that are being
> WR> created/updated and contributed here. Having such a kernel
> WR> available will simplify the process of bringing up a new
> WR> system. The hope is that contributors will adopt the idea of
> WR> having a "Xilinx" kernel and will start using that tree as a "base
> WR> reference".
> 
> But there already is such a tree - It's called 'mainline'. I can see
> some advantage in having a Xilinx tree as a staging area, but it
> really HAS to be a temporary thing and stuff needs to be pushed to
> mainline.
> 
> WR> I will post news here as they unfold. In the meantime it would be
> WR> very interesting to hear ideas and suggestions from you. Concerns
> WR> that people have. Pitfalls to look out for etc.
> 
> Hereby my 2 cents.
>