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

[Helmut Raiger]

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.