Dear Dirk,
In message <4929A58A.5060709 at googlemail.com> you wrote:
>> > I think repeating the sequence "base + offset(foo)" again and again
> > is not exactly nice or easy to read.
...
> >>- writel((a_add_high | a_add_low), SDRC_CS_CFG);
> >>+ writel((a_add_high | a_add_low), sdrc_base + OFFS(SDRC_CS_CFG));
...
> > Argh... that's awfully error prone. So you rely on havnig to deal
> > with 32 bit register accesses only, without any way of compiler
> > supported type checking?
...
> This style is what Jean-Christophe and Scott asked me to use [1]. I
> was asked to convert all OMAP3 read/write access to this style to get
> the patches accepted.
I don't see a request for this specific style in the message you
referenced.
Anyway - I put both Jean-Christophe and Scott on Cc:, and hereby ask
to discuss this again.
> Disussing this at IRC, we agreed on
>> -- cut --
> #define GPMC_BASE 0x6E000000
>> OFFS(x) ((x) >> 2)
>> #define GPMC_ECC_CONFIG 0x1F4
>> static uint32_t *gpmc_base = (uint32_t *)GPMC_BASE;
> ...
> writel(0x000, gpmc_base + OFFS(GPMC_ECC_CONFIG));
> -- cut --
But this is ugly and error prone, and I really do not want to continue
adding such code to U-Boot.
> style for 32-bit accesses. OFFS is used to be able to use offset
> macros in assembly, too, and to be able to directly use offset
> addresses from TRM not needing them to divide by 4 "manually" (error
> prone).
It may be true that the ARM referecne manuals just list register
offsets - PowerPC manuals do the same. But this is in no way a reason
to access the registers through a "base address + offset" style thing
without type checking.
> [1] http://lists.denx.de/pipermail/u-boot/2008-October/042483.html
> > Do you see a chance to convert the OMAP3 code to use data structures
> > instead of offset definitions to describe the processor registers and
> > peripherals?
>> Looking at the time I already spent into converting to the style used
> now, no, unfortunately, not. This was the last read/write converting
> patch, btw.
Well, if we do not clean this up now, more boards will get added
which all use this style, and we will never be able to find resources
to clean it up.
Instead of declaring lists of offsets like this:
#define GPMC_ECC_CONFIG 0x1F4
#define GPMC_ECC_CONTROL 0x1F8
#define GPMC_ECC_SIZE_CONFIG 0x1FC
#define GPMC_ECC1_RESULT 0x200
#define GPMC_ECC2_RESULT 0x204
#define GPMC_ECC3_RESULT 0x208
#define GPMC_ECC4_RESULT 0x20C
#define GPMC_ECC5_RESULT 0x210
#define GPMC_ECC6_RESULT 0x214
#define GPMC_ECC7_RESULT 0x218
#define GPMC_ECC8_RESULT 0x21C
#define GPMC_ECC9_RESULT 0x220
you should really use structures like this:
struct ecc_control {
u32 ecc_config;
u32 ecc_control;
u32 ecc_size_config;
u32 ecc1_result;
u32 ecc2_result;
u32 ecc3_result;
u32 ecc4_result;
u32 ecc5_result;
u32 ecc6_result;
u32 ecc7_result;
u32 ecc8_result;
u32 ecc9_result;
};
so that instead of
writel(0x000, gpmc_base + OFFS(GPMC_ECC_CONFIG));
you can write
writel(0x000, ptr->ecc_config);
or similar - and the compiler will actually perform type checking.
[Actually thius should be done for ALL such offset lists for all
systems that use such stuff, i. e. for probably all ARM systems.]
I am aware that this is a major style change, but it really seems
necessary to me.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Only a fool fights in a burning house.
-- Kank the Klingon, "Day of the Dove", stardate unknown