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

Stefano Babic
2011-09-29T06:59:25Z

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