[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 at 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 at 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.