From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Babic Date: Wed, 28 Sep 2011 17:14:49 +0200 Subject: [U-Boot] [PATCH] mx31: provide readable WEIM CS accessor In-Reply-To: <1317214100-1379-2-git-send-email-helmut.raiger@hale.at> References: <20110922140833.CF0B6140796D@gemini.denx.de> <1317214100-1379-1-git-send-email-helmut.raiger@hale.at> <1317214100-1379-2-git-send-email-helmut.raiger@hale.at> Message-ID: <4E8339E9.3080204@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.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 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 > #include > > /* 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 =====================================================================