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

[Stefano Babic <sbabic@denx.de>]

On 09/29/2011 08:32 AM, Helmut Raiger wrote:
> 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*,

Note: I understand now the misunderstanding. I want to pass a struct
weim_regs *, not 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,

This is ok

> but
> this might not be a problem with the CS registers. However passing the
> complete set of chip selects wouldn't work,

This is not what I meant. I want that the function change only one
chipselect, not all chipselects in one shot.

> 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).

No bitmap please...

> 
> What about:
> 
> void mxc_setup_weimcs(int cs, const struct mxc_weimcs *cs)
> {
> ...
> }

This is what I meant ! Only to check the names: mxc_weimcs is what I
described as weim_regs, right ? And this structure can be specified for
each SOC.

> 
> 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);

Yes, right

> }
> 
> This should still work for different SOCs (with different struct
> mxc_weimcs).

Exactly.

> CSCR_U et al. will be mx31 specific defines.

This is not a problem - other SOCc have or can have a different layout.
It is correct to define these macro into imx-regs.h, as you already did.

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