On 09/28/2011 05:14 PM, Stefano Babic wrote >> 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 ? >> I took it from the kernel where it is done that way and didn't ask why. I'll move it.
>> 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. I used the writel register access to assure correct memory barriers, but this might not be a problem with the CS registers. However passing the complete set of chip selects wouldn't work, as only a few will be setup in the function, while others aren't touched (we could pass a bitmap to select which ones should be set, but this seems flamboyant). What about: void mxc_setup_weimcs(int cs, const struct mxc_weimcs *cs) { ... } void some_board_init_func(void) { /* CS5: CPLD incl. network controller */ static const struct mxc_weimcs cs5 = { /* sp wp bcd bcs psz pme sync dol cnc wsc ew wws edc */ CSCR_U(0, 0, 0, 0, 0, 0, 0, 0, 3, 24, 0, 4, 3), /* oea oen ebwa ebwn csa ebc dsz csn psr cre wrap csen */ CSCR_L(2, 2, 2, 5, 2, 0, 5, 2, 0, 0, 0, 1), /* ebra ebrn rwa rwn mum lah lbn lba dww dct wwu age cnc2 fce*/ CSCR_A(2, 2, 2, 2, 0, 0, 2, 2, 0, 0, 0, 0, 0, 0) }; mxc_setup_weimcs(5, &cs5); } This should still work for different SOCs (with different struct mxc_weimcs). CSCR_U et al. will be mx31 specific defines. Helmut -- Scanned by MailScanner. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot