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

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".

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

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

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

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

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

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

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

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
participants (3)
-
Scott Wood
-
Stefano Babic
-
Wolfgang Denk