[U-Boot] [PATCH V4 09/11] ARM/PPC: add a common way to access registers

[U-Boot] [PATCH V4 09/11] ARM/PPC: add a common way to access registers

[Stefano Babic]

Some Freescale's processors of different architectures
have the same peripherals (eSDHC controller in PowerPC
and i.MX51). This patch adds neutral functions to access
to the internal registers of the SOCs that can be used
by both architectures.

Signed-off-by: Stefano Babic <sbabic@denx.de>
---
 include/asm-arm/io.h |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/asm-ppc/io.h |   31 +++++++++++++++++++++++++++++--
 2 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/include/asm-arm/io.h b/include/asm-arm/io.h
index fec3a7e..e7d6d81 100644
--- a/include/asm-arm/io.h
+++ b/include/asm-arm/io.h
@@ -113,6 +113,53 @@ extern void __raw_readsl(unsigned int addr, void *data, int longlen);
 #define __raw_base_readl(base,off)	__arch_base_getl(base,off)
 
 /*
+ * Clear and set bits in one shot. These macros can be used to clear and
+ * set multiple bits in a register using a single call. These macros can
+ * also be used to set a multiple-bit bit pattern using a mask, by
+ * specifying the mask in the 'clear' parameter and the new bit pattern
+ * in the 'set' parameter.
+ */
+#define clrbits(type, addr, clear) \
+	write##type(__raw_read##type(addr) & ~(clear), (addr))
+
+#define setbits(type, addr, set) \
+	write##type(__raw_read##type(addr) | (set), (addr))
+
+#define clrsetbits(type, addr, clear, set) \
+	write##type((__raw_read##type(addr) & ~(clear)) | (set), (addr))
+
+/*
+ * The following macros are used to access memory mapped registers
+ * in drivers running on different architecture (ppc, arm).
+ * They calls the respective intern macros depending on the
+ * architecture endianess.  This allows to have a common way to
+ * access peripherals from the driver point of view
+ * even on systems with different endianess.
+ */
+#define iowrite(type,a,v)	write##type(v,a)
+#define ioread(type,a)		__raw_read##type(a)
+
+#define iowrite32(a,v)	iowrite(l,a,v)
+#define iowrite16(a,v)	iowrite(w,a,v)
+#define iowrite8(a,v)	iowrite(b,a,v)
+
+#define ioread32(a)	ioread(l,a)
+#define ioread16(a)	ioread(w,a)
+#define ioread8(a)	ioread(b,a)
+
+#define clrbits_reg32(addr, clear) clrbits(l, addr, clear)
+#define setbits_reg32(addr, set) setbits(l, addr, set)
+#define clrsetbits_reg32(addr, clear, set) clrsetbits(l, addr, clear, set)
+
+#define clrbits_reg16(addr, clear) clrbits(w, addr, clear)
+#define setbits_reg16(addr, set) setbits(w, addr, set)
+#define clrsetbits_reg16(addr, clear, set) clrsetbits(w, addr, clear, set)
+
+#define clrbits_reg8(addr, clear) clrbits(b, addr, clear)
+#define setbits_reg8(addr, set) setbits(b, addr, set)
+#define clrsetbits_reg8(addr, clear, set) clrsetbits(b, addr, clear, set)
+
+/*
  * Now, pick up the machine-defined IO definitions
  */
 #if 0	/* XXX###XXX */
diff --git a/include/asm-ppc/io.h b/include/asm-ppc/io.h
index 4ddad26..bf3fc56 100644
--- a/include/asm-ppc/io.h
+++ b/include/asm-ppc/io.h
@@ -242,13 +242,13 @@ extern inline void out_be32(volatile unsigned __iomem *addr, int val)
 	__asm__ __volatile__("sync; stw%U0%X0 %1,%0" : "=m" (*addr) : "r" (val));
 }
 
-/* Clear and set bits in one shot. These macros can be used to clear and
+/*
+ * Clear and set bits in one shot. These macros can be used to clear and
  * set multiple bits in a register using a single call. These macros can
  * also be used to set a multiple-bit bit pattern using a mask, by
  * specifying the mask in the 'clear' parameter and the new bit pattern
  * in the 'set' parameter.
  */
-
 #define clrbits(type, addr, clear) \
 	out_##type((addr), in_##type(addr) & ~(clear))
 
@@ -279,6 +279,33 @@ extern inline void out_be32(volatile unsigned __iomem *addr, int val)
 #define clrsetbits_8(addr, clear, set) clrsetbits(8, addr, clear, set)
 
 /*
+ * The following macros are used to access memory mapped registers
+ * in drivers running on different architecture (ppc, arm).
+ * They calls the respective intern macros depending on the
+ * architecture endianess.  This allows to have a common way to
+ * access peripherals from the driver point of view
+ * even on systems with different endianess.
+ */
+#define iowrite32(a,v)	out_be32(a,v)
+#define iowrite16(a,v)	out_be16(a,v)
+#define iowrite8(a,v)	out_be8(a,v)
+#define ioread32(a)	in_be32(a)
+#define ioread16(a)	in_be16(a)
+#define ioread8(a)	in_be8(a)
+
+#define clrbits_reg32(addr, clear) clrbits(be32, addr, clear)
+#define setbits_reg32(addr, set) setbits(be32, addr, set)
+#define clrsetbits_reg32(addr, clear, set) clrsetbits(be32, addr, clear, set)
+
+#define clrbits_reg16(addr, clear) clrbits(be16, addr, clear)
+#define setbits_reg16(addr, set) setbits(be16, addr, set)
+#define clrsetbits_reg16(addr, clear, set) clrsetbits(be16, addr, clear, set)
+
+#define clrbits_reg8(addr, clear) clrbits(be8, addr, clear)
+#define setbits_reg8(addr, set) setbits(be8, addr, set)
+#define clrsetbits_reg8(addr, clear, set) clrsetbits(be8, addr, clear, set)
+
+/*
  * Given a physical address and a length, return a virtual address
  * that can be used to access the memory range with the caching
  * properties specified by "flags".
-- 
1.6.3.3

[U-Boot] [PATCH V4 09/11] ARM/PPC: add a common way to access registers

[Wolfgang Denk]

Dear Stefano,

could you _please_ provide sufficient "References:" and "In-reply-to:"
headers with your messages, so threading is working? You are using
git-send-email so this is really simple - just enter the respective
message ID when it asks

	Message-ID to be used as In-Reply-To for the first email? 

Thanks.

In message <1264513404-5991-1-git-send-email-sbabic@denx.de> you wrote:
> Some Freescale's processors of different architectures
> have the same peripherals (eSDHC controller in PowerPC
> and i.MX51). This patch adds neutral functions to access
> to the internal registers of the SOCs that can be used
> by both architectures.
> 
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> ---
>  include/asm-arm/io.h |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/asm-ppc/io.h |   31 +++++++++++++++++++++++++++++--
>  2 files changed, 76 insertions(+), 2 deletions(-)

In my first comment I wrote:

| Please document these new macros.  Please be explicit about the
| behaviour of these macros on systems with different endianess.

This is still missing.

We need an entry in the README or even better some new
"doc/README.io_accessors" or similar, which lists the new macros and
their intended use and interface.

This file should also document the behaviour of these macros on big
endian and little endian machines.


For example, I doubt that the current implemntation in asm-arm/io.h
will work consistently both with big endian and little endian ARM
systems.


> diff --git a/include/asm-arm/io.h b/include/asm-arm/io.h
> index fec3a7e..e7d6d81 100644
> --- a/include/asm-arm/io.h
> +++ b/include/asm-arm/io.h
> @@ -113,6 +113,53 @@ extern void __raw_readsl(unsigned int addr, void *data, int longlen);
>  #define __raw_base_readl(base,off)	__arch_base_getl(base,off)
>  
>  /*
> + * Clear and set bits in one shot. These macros can be used to clear and
> + * set multiple bits in a register using a single call. These macros can
> + * also be used to set a multiple-bit bit pattern using a mask, by
> + * specifying the mask in the 'clear' parameter and the new bit pattern
> + * in the 'set' parameter.
> + */
> +#define clrbits(type, addr, clear) \
> +	write##type(__raw_read##type(addr) & ~(clear), (addr))
> +
> +#define setbits(type, addr, set) \
> +	write##type(__raw_read##type(addr) | (set), (addr))
> +
> +#define clrsetbits(type, addr, clear, set) \
> +	write##type((__raw_read##type(addr) & ~(clear)) | (set), (addr))

I think the definition of the clrbits/setbits/clrsetbits macros
should follow the existing examples in the Linux kernel, i. e. we
should have setbits64, setbits32, setbits16, and setbits8 and
respective for clrbits{64,32,16,8} and clrsetbits{64,32,16,8}.

> @@ -279,6 +279,33 @@ extern inline void out_be32(volatile unsigned __iomem *addr, int val)
>  #define clrsetbits_8(addr, clear, set) clrsetbits(8, addr, clear, set)
>  
>  /*
> + * The following macros are used to access memory mapped registers
> + * in drivers running on different architecture (ppc, arm).

This is NOT sufficient, and actually misleading. The issue does NOT
depend on Power versus ARM architecture, but on big endian versus
little endian systems / busses. You may even see the same problem
with ARM systems - there are big endian and little endian ARM confi-
gurations, and both should be working.


> + * They calls the respective intern macros depending on the
> + * architecture endianess.  This allows to have a common way to
> + * access peripherals from the driver point of view
> + * even on systems with different endianess.
> + */

Explanation of the behaviour regarding to endianess would be more
important than such implementation details.

> +#define clrbits_reg32(addr, clear) clrbits(be32, addr, clear)
> +#define setbits_reg32(addr, set) setbits(be32, addr, set)
> +#define clrsetbits_reg32(addr, clear, set) clrsetbits(be32, addr, clear, set)
> +
> +#define clrbits_reg16(addr, clear) clrbits(be16, addr, clear)
> +#define setbits_reg16(addr, set) setbits(be16, addr, set)
> +#define clrsetbits_reg16(addr, clear, set) clrsetbits(be16, addr, clear, set)
> +
> +#define clrbits_reg8(addr, clear) clrbits(be8, addr, clear)
> +#define setbits_reg8(addr, set) setbits(be8, addr, set)
> +#define clrsetbits_reg8(addr, clear, set) clrsetbits(be8, addr, clear, set)

Please drop the "reg" from the names (it's accessing memory mapped
I/O and not registers), and use setbits32() etc. instead, see above;
feel free to add macros as clrsetbits_be32() or clrsetbits_le32()
etc. as needed.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A supercomputer is a machine that runs an endless loop in 2 seconds.

[U-Boot] [PATCH V4 09/11] ARM/PPC: add a common way to access registers

[Stefano Babic]

Wolfgang Denk wrote:
> Dear Stefano,
> 
Hi Wolfgang,

> could you _please_ provide sufficient "References:" and "In-reply-to:"
> headers with your messages, so threading is working? You are using
> git-send-email so this is really simple - just enter the respective
> message ID when it asks
> 
> 	Message-ID to be used as In-Reply-To for the first email? 

Sorry for that.

> In my first comment I wrote:
> 
> | Please document these new macros.  Please be explicit about the
> | behaviour of these macros on systems with different endianess.
> 
> This is still missing.
> 
> We need an entry in the README or even better some new
> "doc/README.io_accessors" or similar, which lists the new macros and
> their intended use and interface.
> 
> This file should also document the behaviour of these macros on big
> endian and little endian machines.

I understand what you mean and I come now to a slightly different
solution as what you have proposed. Really my patches drop the endianess
(that is explicitely set in the actual fsl_esdhc driver) and generate
the confusion you reported. Probably, even if I add a better
documentation, it remains unclear which is the endianess used.

Another possibility will be to explicitely set the endianess with a
CONFIG_ when it is needed.
I found that in u-boot there is already a solution like this (for USB):

	CONFIG_EHCI_DESC_BIG_ENDIAN

My proposal will be:

In the accessors (io.h), I will add all requested macros for both
endianess (clrbits_le32,clrbits_be32, setbits_be32,...) for the arm
architecture as you proposed. It seems there is no need to change the
accessors for powerpc. I have to drop the "implicite" endianess I set
(for example, when I set clrsetbits32 as big endian for powerpc and
little endian for arm), source of confusion.

Probably I do not need in this case to write any additional
documentation because the endianess is explicitely set and IMHO is self
explained.

I will add something like CONFIG_FSL_ESDHC_BIG_ENDIAN or
CONFIG_FSL_ESDHC_LITTLE_ENDIAN in the driver I changed. In the related
header file for the driver, I can set driver specific macros that point
to the correct accessors, depending on the new CONFIG_ switch.
Then it should be clear which is the endianess used and it will be not
related to the processor architecture. What do you think about this ?

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V4 09/11] ARM/PPC: add a common way to access registers

[Wolfgang Denk]

Dear Stefano Babic,

In message <4B62C655.3090707@denx.de> you wrote:
>
> I will add something like CONFIG_FSL_ESDHC_BIG_ENDIAN or
> CONFIG_FSL_ESDHC_LITTLE_ENDIAN in the driver I changed. In the related
> header file for the driver, I can set driver specific macros that point
> to the correct accessors, depending on the new CONFIG_ switch.
> Then it should be clear which is the endianess used and it will be not
> related to the processor architecture. What do you think about this ?

I like this approach better than the precious one.

However, I had hoped that we could do without such manual configu-
ration like CONFIG_FSL_ESDHC_*_ENDIAN. I mean, we already know the
target byte order, and you know in which byte order you want to
access the data, or is this board dependent?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Anything that is worth doing at all is worth doing well.
                                       -- Philip Earl of Chesterfield

[U-Boot] [PATCH V4 09/11] ARM/PPC: add a common way to access registers

[Stefano Babic]

Wolfgang Denk wrote:

> I like this approach better than the precious one.
> 
> However, I had hoped that we could do without such manual configu-
> ration like CONFIG_FSL_ESDHC_*_ENDIAN. I mean, we already know the
> target byte order, and you know in which byte order you want to
> access the data, or is this board dependent?

It should be enough to include asm/byteorder.h. Based on __BYTE_ORDER, I
can select the correct accessors in the driver.

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V4 09/11] ARM/PPC: add a common way to access registers

[Wolfgang Denk]

Dear Stefano Babic,

In message <4B6A96B3.4060803@denx.de> you wrote:
> Wolfgang Denk wrote:
> 
> > I like this approach better than the precious one.
> > 
> > However, I had hoped that we could do without such manual configu-
> > ration like CONFIG_FSL_ESDHC_*_ENDIAN. I mean, we already know the
> > target byte order, and you know in which byte order you want to
> > access the data, or is this board dependent?
> 
> It should be enough to include asm/byteorder.h. Based on __BYTE_ORDER, I
> can select the correct accessors in the driver.

Excellent. Then let's do it this way.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A Perl script is correct if it's halfway readable and  gets  the  job
done before your boss fires you.
                       - L. Wall & R. L. Schwartz, _Programming Perl_

[U-Boot] [PATCH V4 09/11] ARM/PPC: add a common way to access registers

[Scott Wood]

Stefano Babic wrote:
> Wolfgang Denk wrote:
> 
>> I like this approach better than the precious one.
>>
>> However, I had hoped that we could do without such manual configu-
>> ration like CONFIG_FSL_ESDHC_*_ENDIAN. I mean, we already know the
>> target byte order, and you know in which byte order you want to
>> access the data, or is this board dependent?
> 
> It should be enough to include asm/byteorder.h. Based on __BYTE_ORDER, I
> can select the correct accessors in the driver.

How about adding additional I/O accessors that do native endian accesses instead 
of specified endian?

-Scott

[U-Boot] [PATCH V4 09/11] ARM/PPC: add a common way to access registers

[Stefano Babic]

Scott Wood wrote:

> How about adding additional I/O accessors that do native endian accesses
> instead of specified endian?

Hi Scott,

I confess I have thought to add this kind of abstraction, too. However,
this is probably overkilling for the purpose of this patch.

I think if we need another general way to manage endianess, it should be
better done in another patch. Scope of this patch is only to provide
common ways to access peripherals from SoCs sharing the same
controllers, but with different CPUs.

I do not know (at least, I have not founded), if there is already in
u-boot a driver shared between architectures with different endianess,
but as sure the Freescale's MX and PowerQuick processors have some
common parts and we should try to reuse code as Freescale reused gates.
 Or at least, try to do it....

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH V4 09/11] ARM/PPC: add a common way to access registers

[Scott Wood]

Stefano Babic wrote:
> Scott Wood wrote:
> 
>> How about adding additional I/O accessors that do native endian accesses
>> instead of specified endian?
> 
> Hi Scott,
> 
> I confess I have thought to add this kind of abstraction, too. However,
> this is probably overkilling for the purpose of this patch.
> 
> I think if we need another general way to manage endianess, it should be
> better done in another patch. 

Sure, it would be a separate patch -- but I suspect this driver isn't 
the only time we'll run into this.

-Scott