[U-Boot] [PATCH] mx31: provide readable WEIM CS accessor

[Stefano Babic <sbabic@denx.de>]

On 09/28/2011 02:48 PM, Helmut Raiger wrote:
> Some macros are added to support the setup for i.MX31 WEIM
> chip selects. As a compromise between verbosity and readability
> an ASCII-art'ish bit comment is used instead of bitfields.
> All i.MX31 boards have been patched to use this approach using a helper
> program to verify the changes.
> 
> Signed-off-by: Helmut Raiger <helmut.raiger@hale.at>

Hi Helmut,

> ---
>  arch/arm/include/asm/arch-mx31/imx-regs.h   |   38 ++++++++++++-
>  board/davedenx/qong/qong.c                  |   79 +++++++++------------------
>  board/freescale/mx31ads/mx31ads.c           |   13 +++--
>  board/freescale/mx31pdk/mx31pdk.c           |   11 +++-
>  board/imx31_phycore/imx31_phycore.c         |   36 +++++++++---
>  board/logicpd/imx31_litekit/imx31_litekit.c |   24 ++++++--
>  6 files changed, 123 insertions(+), 78 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-mx31/imx-regs.h b/arch/arm/include/asm/arch-mx31/imx-regs.h
> index 2064870..d535830 100644
> --- a/arch/arm/include/asm/arch-mx31/imx-regs.h
> +++ b/arch/arm/include/asm/arch-mx31/imx-regs.h
> @@ -25,6 +25,7 @@
>  #define __ASM_ARCH_MX31_IMX_REGS_H
>  
>  #if !(defined(__KERNEL_STRICT_NAMES) || defined(__ASSEMBLY__))
> +#include <asm/io.h>
>  #include <asm/types.h>
>  
>  /* Clock control module registers */
> @@ -534,10 +535,41 @@ enum iomux_pins {
>  #define ESDCTL_BL(x)			((x) << 7)
>  #define ESDCTL_PRCT(x)			((x) << 0)
>  
> +/* 13 fields of the upper CS control register */
> +#define CSCR_U(sp, wp, bcd, bcs, psz, pme, sync, dol, \
> +		cnc, wsc, ew, wws, edc) \
> +	((sp) << 31 | (wp) << 30 | (bcd) << 28 | (psz) << 22 | (pme) << 21 |\
> +	 (sync) << 20 | (dol) << 16 | (cnc) << 14 | (wsc) << 8 | (ew) << 7 |\
> +	 (wws) << 4 | (edc) << 0)
> +/* 12 fields of the lower CS control register */
> +#define CSCR_L(oea, oen, ebwa, ebwn, \
> +		csa, ebc, dsz, csn, psr, cre, wrap, csen) \
> +	((oea) << 28 | (oen) << 24 | (ebwa) << 20 | (ebwn) << 16 |\
> +	 (csa) << 12 | (ebc) << 11 | (dsz) << 8 | (csn) << 4 |\
> +	 (psr) << 3 | (cre) << 2 | (wrap) << 1 | (csen) << 0)
> +/* 14 fields of the additional CS control register */
> +#define CSCR_A(ebra, ebrn, rwa, rwn, mum, lah, lbn, lba, dww, dct, \
> +		wwu, age, cnc2, fce) \
> +	((ebra) << 28 | (ebrn) << 24 | (rwa) << 20 | (rwn) << 16 |\
> +	 (mum) << 15 | (lah) << 13 | (lbn) << 10 | (lba) << 8 |\
> +	 (dww) << 6 | (dct) << 4 | (wwu) << 3 |\
> +	 (age) << 2 | (cnc2) << 1 | (fce) << 0)
> +
>  #define WEIM_BASE	0xb8002000
> -#define CSCR_U(x)	(WEIM_BASE + (x) * 0x10)
> -#define CSCR_L(x)	(WEIM_BASE + 4 + (x) * 0x10)
> -#define CSCR_A(x)	(WEIM_BASE + 8 + (x) * 0x10)
> +#define WEIM_CSCR_U(x)	(WEIM_BASE + (x) * 0x10)
> +#define WEIM_CSCR_L(x)	(WEIM_BASE + 4 + (x) * 0x10)
> +#define WEIM_CSCR_A(x)	(WEIM_BASE + 8 + (x) * 0x10)
> +
> +#ifndef __ASSEMBLER__
> +static inline void mx31_setup_weimcs(int cs,

Is there a reason to embed this function in imx-regs.h ? Why not in
./arch/arm/cpu/arm1136/mx31/generic.c, where I think this function
belongs ?

We are trying to get consistency among the several i.MX SOCs. For this
reason, a general function should not have a specific SOC prefix.
You introduce now a new accessor to set up the WEIM registers. We have
not yet such as function, but we can have then for other SOCs, too.
Rename your function as mxc_setup_weimcs(), and when an accessor will be
supplied for MX5 (or MX*), the same name must be used.

> +		unsigned int upper, unsigned int lower, unsigned int add)
> +{
> +	writel(upper, WEIM_CSCR_U(cs));
> +	writel(lower, WEIM_CSCR_L(cs));
> +	writel(add, WEIM_CSCR_A(cs));
> +}

You are using offests to access registers. Why not to set a structure as:

struct weim_regs {
	u32 upper;
	u32 lower;
	u32 adder;
	u32 reserved;
}

and then :

struct weim {
	struct weim_regs cs[6];
};

...or something like that.

Passing the register values to the function makes the accessor too
striclty bound to the mx31. But if you pass a struct weim*, that is void
mxc_setup_weimcs(struct weim *), we can have the same accessor (with a
different implementation, of course) for the other SOCs, too. I can
imagine we can have MX5 (at the moment I see only the mx53ard) using the
same way to set up the WEIM interface.

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
=====================================================================