[U-Boot] [PATCH-OMAP3] OMAP3: Convert readl/writel and replace hardcoded values

[Wolfgang Denk]

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