[U-Boot] [PATCH] ftsmc020: enhance for features and asm support.

[Macpaul Lin]

Hi Wolfgang

2011/3/31 Wolfgang Denk <wd at denx.de>:
> Dear Macpaul Lin,
>
> We should probably split architecture and/or board specific additions
> like these into separate files in the respectice architecture / board
> directories.  Eventually we add make targets for these, then; for now
> it's probably sufficient to add some #include to lib/asm-offsets.c
>
> Otherwise lib/asm-offsets.c will quickly become an unreadable mess.
>
> Otherwise this looks OK with me.

There are lots of register offset is widely been used both in
lowlevel_init and C environment.
If we really want assembly offset of these registers is generated from
structures,
lots of includes files with OFFSET() marco to generate asm-offsets
will be required.
For example:
in "arch/arm/include/asm/arch-at91/at91sam9_smc.h", there exist the
similar problem as ftsmc020.

#ifdef __ASSEMBLY__

#ifndef AT91_SMC_BASE
#define AT91_SMC_BASE   AT91_SMC0_BASE
#endif

#define AT91_ASM_SMC_SETUP0     AT91_SMC_BASE
#define AT91_ASM_SMC_PULSE0     (AT91_SMC_BASE + 0x04)
#define AT91_ASM_SMC_CYCLE0     (AT91_SMC_BASE + 0x08)
#define AT91_ASM_SMC_MODE0      (AT91_SMC_BASE + 0x0C)

#else

typedef struct  at91_cs {
        u32     setup;          /* 0x00 SMC Setup Register */
        u32     pulse;          /* 0x04 SMC Pulse Register */
        u32     cycle;          /* 0x08 SMC Cycle Register */
        u32     mode;           /* 0x0C SMC Mode Register */
} at91_cs_t;

typedef struct  at91_smc {
        at91_cs_t       cs[8];
} at91_smc_t;

#endif /*  __ASSEMBLY__ */

Thus we may need to write some code like OFFSET(AT91_ASM_SMC_SETUP0,
at91_cs, setup);
some where in the included file then generate the code like.
#define AT91_ASM_SMC_SETUP0 (0) /* offsetof(struct at91_cs, setup) */

Even like this, we cannot directly use AT91_ASM_SMC_SETUP0 like the
code above in lowlevel_init.c.
If there is a code wrote as writel(0x1,  AT91_ASM_SMC_SETUP0);
originally must be rewrote as
writel(0x1,  AT91_SMC_BASE + AT91_ASM_SMC_SETUP0);

Hence we really need some scalable rework for the both a split
arch/board make-asm-offset directories.

I'm not sure if my thought of this scenario correct?

> Keep in mind that I dislike this manual unrolling of the nested
> structs. It may work in your code, but it is ugly and doesn't scale.
> Also, it does not allow any kind of looping over the entries which
> might be needed here and there.  I strongly recommend to get rid of
> these nested declarations.

Ok, since the original structure was commit by other people, I'm still
trying to rewrite the original C code.

>> #define FTSMC020_PAD4 (48) /* offsetof(struct ftsmc020, pad[4]) */
>> #define FTSMC020_PAD5 (52) /* offsetof(struct ftsmc020, pad[5]) */
>> #define FTSMC020_PAD6 (56) /* offsetof(struct ftsmc020, pad[6]) */
>> #define FTSMC020_PAD7 (60) /* offsetof(struct ftsmc020, pad[7]) */
>> #define FTSMC020_SSR (64) /* offsetof(struct ftsmc020, ssr) */
>>
>> However, this looks weird. It doesn't look like the other automated
>> generated code.
>
> What exactly looks weird?  And what "other automated generated code"
> do you mean?

I meant, should we make a script that could auto generate asm-offset
like a translation from
struct ftsmc020 {
    unsigned bank0_cr;
    unsigned bank0_tpr;
    ...
}
into
#define FTSMC020_BANK0_CR 0x00;
or
#define FTSMC020_BANK0_TPR (4);

without manually reworte the structure in the way as
OFFSET(FTSMC020_BANK0_CR, ftsmc020, bank0_cr);
and create a new specific header file for make-asm-offset?

>> Could I move the generated code into ftsmc020.h?
>
> No - what for?  This is automatically generated code, that gets used
> somewhere. No human eye is supposed to have to read it.

You are right, the comments of FTSMC020_BANK0_TPR (4) is really bad
for human reading.
However, when people implementing lowlevel_init or other assembly
files, they still need such kind of reference
to write *.S files before the asm-offset has been generated by make.

Thanks.

-- 
Best regards,
Macpaul Lin