[U-Boot] [PATCH] mx25: Clean up lowlevel_init

Wed Aug 15 08:27:38 CEST 2012

Dear Benoît

Am 14.08.2012 23:25, schrieb Benoît Thébaudeau:
> Clean up mx25 lowlevel_init:
>   - Add comments.
>   - Do not use write32 repeatedly with the same value in order no to increase
>     code size.
>   - Make register values configurable.
>   - Use defined values instead of literal constants.
>   - Use defined macros instead of duplicating code.
>
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau at advansee.com>
> Cc: Stefano Babic <sbabic at denx.de>
> ---
>   .../arch/arm/include/asm/arch-mx25/macro.h         |   57 +++++++++++++-------
>   .../board/karo/tx25/lowlevel_init.S                |   37 +++----------
>   .../board/syteco/zmx25/lowlevel_init.S             |    6 +++
>   3 files changed, 51 insertions(+), 49 deletions(-)
>
> diff --git u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
> index 3b694da..9550ade 100644
> --- u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx25/macro.h
> +++ u-boot-4d3c95f/arch/arm/include/asm/arch-mx25/macro.h
> @@ -32,32 +32,49 @@
>
>   #include <asm/arch/imx-regs.h>
>   #include <generated/asm-offsets.h>
> +#include <asm/macro.h>
>
> +/*
> + * AIPS setup - Only setup MPROTx registers.
> + * The PACR default values are good.
> + */
>   .macro init_aips
> -	write32	IMX_AIPS1_BASE + AIPS_MPR_0_7, 0x77777777
> -	write32	IMX_AIPS1_BASE + AIPS_MPR_8_15, 0x77777777
> -	write32	IMX_AIPS2_BASE + AIPS_MPR_0_7, 0x77777777
> -	write32	IMX_AIPS2_BASE + AIPS_MPR_8_15, 0x77777777
> +	ldr	r0, =IMX_AIPS1_BASE
> +	ldr	r1, =AIPS_MPR_CONFIG
> +	str	r1, [r0, #AIPS_MPR_0_7]
> +	str	r1, [r0, #AIPS_MPR_8_15]
> +	ldr	r2, =IMX_AIPS2_BASE
> +	str	r1, [r2, #AIPS_MPR_0_7]
> +	str	r1, [r2, #AIPS_MPR_8_15]
>   .endm
>
> +/* MAX (Multi-Layer AHB Crossbar Switch) setup */
>   .macro init_max
> -	write32	IMX_MAX_BASE + MAX_MPR0, 0x43210
> -	write32	IMX_MAX_BASE + MAX_MPR1, 0x43210
> -	write32	IMX_MAX_BASE + MAX_MPR2, 0x43210
> -	write32	IMX_MAX_BASE + MAX_MPR3, 0x43210
> -	write32	IMX_MAX_BASE + MAX_MPR4, 0x43210
> -
> -	write32	IMX_MAX_BASE + MAX_SGPCR0, 0x10
> -	write32	IMX_MAX_BASE + MAX_SGPCR1, 0x10
> -	write32	IMX_MAX_BASE + MAX_SGPCR2, 0x10
> -	write32	IMX_MAX_BASE + MAX_SGPCR3, 0x10
> -	write32	IMX_MAX_BASE + MAX_SGPCR4, 0x10
> +	ldr	r0, =IMX_MAX_BASE
> +	ldr	r1, =MAX_MPR_CONFIG
> +	str	r1, [r0, #MAX_MPR0]	/* for S0 */
> +	str	r1, [r0, #MAX_MPR1]	/* for S1 */
> +	str	r1, [r0, #MAX_MPR2]	/* for S2 */
> +	str	r1, [r0, #MAX_MPR3]	/* for S3 */
> +	str	r1, [r0, #MAX_MPR4]	/* for S4 */
> +	ldr	r1, =MAX_SGPCR_CONFIG
> +	str	r1, [r0, #MAX_SGPCR0]	/* for S0 */
> +	str	r1, [r0, #MAX_SGPCR1]	/* for S1 */
> +	str	r1, [r0, #MAX_SGPCR2]	/* for S2 */
> +	str	r1, [r0, #MAX_SGPCR3]	/* for S3 */
> +	str	r1, [r0, #MAX_SGPCR4]	/* for S4 */
> +	ldr	r1, =MAX_MGPCR_CONFIG
> +	str	r1, [r0, #MAX_MGPCR0]	/* for M0 */
> +	str	r1, [r0, #MAX_MGPCR1]	/* for M1 */
> +	str	r1, [r0, #MAX_MGPCR2]	/* for M2 */
> +	str	r1, [r0, #MAX_MGPCR3]	/* for M3 */
> +	str	r1, [r0, #MAX_MGPCR4]	/* for M4 */
> +.endm
>
> -	write32	IMX_MAX_BASE + MAX_MGPCR0, 0x0
> -	write32	IMX_MAX_BASE + MAX_MGPCR1, 0x0
> -	write32	IMX_MAX_BASE + MAX_MGPCR2, 0x0
> -	write32	IMX_MAX_BASE + MAX_MGPCR3, 0x0
> -	write32	IMX_MAX_BASE + MAX_MGPCR4, 0x0
> +/* M3IF setup */
> +.macro init_m3if
> +	/* M3IF Control Register (M3IFCTL) */
> +	write32	IMX_M3IF_CTRL_BASE, M3IFCTL_CONFIG
>   .endm
>
>   #endif /* __ASSEMBLY__ */
> diff --git u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
> index 823df10..cdd9bda 100644
> --- u-boot-4d3c95f.orig/board/karo/tx25/lowlevel_init.S
> +++ u-boot-4d3c95f/board/karo/tx25/lowlevel_init.S
> @@ -23,36 +23,13 @@
>
>   #include <asm/macro.h>
>
> -.macro init_aips
> -	write32	0x43f00000, 0x77777777
> -	write32	0x43f00004, 0x77777777
> -	write32	0x43f00000, 0x77777777
> -	write32	0x53f00004, 0x77777777
> -.endm
> +#define AIPS_MPR_CONFIG		0x77777777
> +#define MAX_MPR_CONFIG		0x43210
> +#define MAX_SGPCR_CONFIG	0x10
> +#define MAX_MGPCR_CONFIG	0x0
> +#define M3IFCTL_CONFIG		0x1

I think it would be nicer if the init_max macro can get these values as 
parameters and not as define which has to be there before including the 
header file. You could even use default arguments so that the macro can 
be used without any argument in the default case. What do you think?

> -.macro init_max
> -	write32	0x43f04000, 0x43210
> -	write32	0x43f04100, 0x43210
> -	write32	0x43f04200, 0x43210
> -	write32	0x43f04300, 0x43210
> -	write32	0x43f04400, 0x43210
> -
> -	write32	0x43f04010, 0x10
> -	write32	0x43f04110, 0x10
> -	write32	0x43f04210, 0x10
> -	write32	0x43f04310, 0x10
> -	write32	0x43f04410, 0x10
> -
> -	write32	0x43f04800, 0x0
> -	write32	0x43f04900, 0x0
> -	write32	0x43f04a00, 0x0
> -	write32	0x43f04b00, 0x0
> -	write32	0x43f04c00, 0x0
> -.endm
> -
> -.macro init_m3if
> -	write32 0xb8003000, 0x1
> -.endm
> +#include <asm/arch/macro.h>
>
>   .macro init_clocks
>   	/*
> @@ -64,6 +41,8 @@
>   	 * 0x00600000 makes CLKO parent clk the USB clk
>   	 */
>   	write32	0x53f80064, 0x45600000
> +
> +	/* CCTL: ARM = 399 MHz, AHB = 133 MHz */
>   	write32	0x53f80008, 0x20034000
>
>   	/*
> diff --git u-boot-4d3c95f.orig/board/syteco/zmx25/lowlevel_init.S u-boot-4d3c95f/board/syteco/zmx25/lowlevel_init.S
> index 1002674..575755c 100644
> --- u-boot-4d3c95f.orig/board/syteco/zmx25/lowlevel_init.S
> +++ u-boot-4d3c95f/board/syteco/zmx25/lowlevel_init.S
> @@ -25,6 +25,12 @@
>    */
>
>   #include <asm/macro.h>
> +
> +#define AIPS_MPR_CONFIG		0x77777777
> +#define MAX_MPR_CONFIG		0x43210
> +#define MAX_SGPCR_CONFIG	0x10
> +#define MAX_MGPCR_CONFIG	0x0
> +
>   #include <asm/arch/macro.h>
>   #include <asm/arch/imx-regs.h>
>   #include <generated/asm-offsets.h>

I didn't build and run this but from a functional point it looks good.

BTW:
It would be nice if you put the maintainer of a changed file on CC. I 
didn't check the ML all the time and maybe missed this change.

Regards
Matthias