[U-Boot] [PATCH 1/3] arm: Add support for MB86R0x SoCs

[Matthias Weißer]

Am 22.04.2010 14:34, schrieb Wolfgang Denk:
 > Dear Matthias Weisser,
 >
 > In message<1271932257-14618-2-git-send-email-weisserm at arcor.de>  you 
wrote:
 >> This patch adds support for MB86R0x SoCs from Fujitsu
 >>
 >> Signed-off-by: Matthias Weisser<weisserm at arcor.de>
 > ...
 >> --- /dev/null
 >> +++ b/arch/arm/cpu/arm926ejs/mb86r0x/reset.c
 > ...
 >> +void reset_cpu(ulong ignored)
 >> +{
 >> +    writel(0x00000002, MB86R0x_CRG_PHYS_BASE + CRG_CRSR);
 >
 > Please define some symbolic name for the magic constant 0x00000002
 >
 > Also, we do not accept code based on a "base + offset" notation.
 > Please use C structures instead. [Please fix globally.]

I will fix this

 >> +#define TIMER_LOAD_VAL        0xffffffff
 >> +#define TIMER_BASE        MB86R0x_TIMER_PHYS_BASE
 >> +
 >> +#define TIMER_REG_LOAD        (TIMER_BASE + 0)
 >> +#define TIMER_REG_VALUE        (TIMER_BASE + 4)
 >> +#define TIMER_REG_CONTROL    (TIMER_BASE + 8)
 >
 > Create a proper C struct, please!

and this

 >> +/*
 >> + * Offset definitions for DRAM controller
 >> + */
 >> +#define DDR2C_DRIC        0x00
 >> +#define DDR2C_DRIC1        0x02
 >> +#define DDR2C_DRIC2        0x04
 >> +#define DDR2C_DRCA        0x06
 >> +#define DDR2C_DRCM        0x08
 >> +#define DDR2C_DRCST1        0x0a
 >> +#define DDR2C_DRCST2        0x0c
 >> +#define DDR2C_DRCR        0x0e
 >> +#define DDR2C_DRCF        0x20
 >> +#define DDR2C_DRASR        0x30
 >> +#define DDR2C_DRIMS        0x50
 >> +#define DDR2C_DROS        0x60
 >> +#define DDR2C_DRIBSLI        0x62
 >> +#define DDR2C_DRIBSODT1        0x64
 >> +#define DDR2C_DRIBSOCD        0x66
 >> +#define DDR2C_DRIBSOCD2        0x68
 >> +#define DDR2C_DROABA        0x70
 >> +#define DDR2C_DROBV        0x80
 >> +#define DDR2C_DROBS        0x84
 >> +#define DDR2C_DROBSR1        0x86
 >> +#define DDR2C_DROBSR2        0x88
 >> +#define DDR2C_DROBSR3        0x8a
 >> +#define DDR2C_DROBSR4        0x8c
 >> +#define DDR2C_DRIMR1        0x90
 >> +#define DDR2C_DRIMR2        0x92
 >> +#define DDR2C_DRIMR3        0x94
 >> +#define DDR2C_DRIMR4        0x96
 >> +#define DDR2C_DROISR1        0x98
 >> +#define DDR2C_DROISR2        0x9a
 >
 > C struct, please.
 >
 >> +/*
 >> + * Offset definitions Chip Control Module
 >> + */
 >> +#define CCNT_CCID        0x00
 >> +#define CCNT_CSRST        0x1c
 >> +#define CCNT_CIST        0x20
 >> +#define CCNT_CISTM        0x24
 >> +#define CCNT_CMUX_MD        0x30
 >> +#define CCNT_CDCRC        0xec
 >
 > Ditto.
 >
 >> +/*
 >> + * Offset definitions clock reset generator
 >> + */
 >> +#define CRG_CRPR        0x00
 >> +#define CRG_CRWR        0x08
 >> +#define CRG_CRSR        0x0c
 >> +#define CRG_CRDA        0x10
 >> +#define CRG_CRDB        0x14
 >> +#define CRG_CRHA        0x18
 >> +#define CRG_CRPA        0x1c
 >> +#define CRG_CRPB        0x20
 >> +#define CRG_CRHB        0x24
 >> +#define CRG_CRAM        0x28
 >
 > Ditto.

Well, the above three modules are used in assembler code only 
(lowlevel_init.S) and I didn't found a way to use C structs here. What 
would be the right approach in this case? Defining all these registers
as absolute addresses?

I have a also a couple of magic values in the mentioned .S file. Do I 
have to move them also to some symbolic constants?

Thanks for the quick review.

Regards,
Matthias