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

Stefano Babic
2011-09-28T15:14:49Z

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