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

Helmut Raiger
2011-09-29T06:32:23Z

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.