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

[Dirk Behme]

Dear Wolfgang,

Wolfgang Denk wrote:
> Dear Dirk,
> 
> In message <1227445293-23887-1-git-send-email-dirk.behme at googlemail.com> you wrote:
> 
>>Convert readl/writel to base + offset style. Replace hardcoded values with
>>macros.
> 
> 
> I think repeating the sequence "base + offset(foo)" again  and  again
> is not exactly nice or easy to read.

But it's what the patch does ;) Anyway, if I remember correctly this 
is why you asked me to re-send OMAP3 patch series to mailing list 
again once all the review comments are fixed and not directly git pull 
from OMAP3 branch. To get rid of all these "fix here, clean up there" 
comments.

>>-	writel((a_add_high | a_add_low), SDRC_CS_CFG);
>>+	writel((a_add_high | a_add_low), sdrc_base + OFFS(SDRC_CS_CFG));
> 
> 
> I find this new code is even less readable.
> 
> At this point I was tempted to compain that you should not re-invent
> the offsetof() macro when I realized that the actual definition of
> OFFS is
> 
> 	#define OFFS(x)    ((x) >> 2)
> 
> 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?
> 
> That's awful.

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.

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

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

[1] http://lists.denx.de/pipermail/u-boot/2008-October/042483.html

> I am aware that many areas of ARM code use such a  horrible  program-
> ming  syle  of  defining only register offsets instead of proper data
> structures like for example PowerPC has been doing for ages.
> 
> 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.

Best regards

Dirk

Btw.: Did you notice how e.g. Nomadik patch [2] does it?

-- cut --
#define NOMADIK_GPIO1_BASE	0x101E5000
...
writel(0xC37800F0, NOMADIK_GPIO1_BASE + 0x20);
-- cut --

Having this patch applied in this format I slightly wonder

a) why I have to convert all hardcoded values to macros

b) why I have to convert all writex/readx

Style used by Nomadik patch is the same OMAP3 initially used and that 
wasn't accepted...

[2] http://lists.denx.de/pipermail/u-boot/2008-November/043651.html