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