[U-Boot] [PATCH] arm/pxa-devel: fix and cleanup of pxa_mem_setup macro

[Marek Vasut]

Dne Po 17. května 2010 08:28:22 Mikhail Kshevetskiy napsal(a):
> * strict following to section 6.4.10 of Intel PXA27xx Developer's Manual.
> * use r7 to store CONFIG_SYS_MDREFR_VAL as r6 is used in pxa_wait_ticks
> 
> WARNING: This macro do not assume the values for K0DB4, KxDB2, KxFREE
>          and APD bits of CONFIG_SYS_MDREFR_VAL as it was done early
>          on many pxa platforms. All pxa developers that plan to use
>          this macro should check the validity of their MDREFR values.

Exactly, I'd rather see the developers fix their configs instead of putting here 
some weird goo. But I might be wrong ... see below please.
> 
> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy at gmail.com>
> ---
>  arch/arm/include/asm/arch-pxa/macro.h |   81
> +++++++++++++++++++++++---------- 1 files changed, 56 insertions(+), 25
> deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-pxa/macro.h
> b/arch/arm/include/asm/arch-pxa/macro.h index 1f1759b..26d7a8d 100644
> --- a/arch/arm/include/asm/arch-pxa/macro.h
> +++ b/arch/arm/include/asm/arch-pxa/macro.h
> @@ -102,7 +102,11 @@
>  /*
>   * This macro sets up the Memory controller of the PXA2xx CPU
>   *
> - * Clobbered regs: r3, r4, r5
> + * Clobbered regs: r3, r4, r5, r6, r7
> + *
> + * See section 6.4.10 of Intel PXA2xx Processor Developer's Manual
> + *  
> http://www.marvell.com/products/processors/applications/pxa_family/pxa_27x
> _dev_man.pdf + *  
> http://www.marvell.com/products/processors/applications/pxa_family/PXA3xx_
> Developers_Manual.zip */

Remove this, PXA3xx won't use this macro.

>  .macro	pxa_mem_setup
>  	/* This comes handy when setting MDREFR */
> @@ -149,27 +153,38 @@
>  	 */
> 
>  	/*
> -	 * Before accessing MDREFR we need a valid DRI field, so we set
> -	 * this to power on defaults + DRI field.
> +	 * Before accessing MDREFR we need a valid DRI field.
> +	 * Also we must properly configure MDREFR[K0DB2] and MDREFR[K0DB4].
> +	 * Optionaly we can set MDREFR[KxFREE] bits.
> +	 * So we set MDREFR to power on defaults + (DRI, K0DB2, K0DB4, KxFREE)
> +	 * fields from the config.
> +	 *
> +	 * WARNING: K0DB2 and K0DB4 bits are usually set, while KxFREE bits
> +	 *          are usually unser.
>  	 */
>  	ldr	r5, [r3, #MDREFR_OFFSET]
> -	bic	r5, r5, #0x0ff
> -	bic	r5, r5, #0xf00	/* MDREFR user config with zeroed DRI */
> -
> -	ldr	r4, =CONFIG_SYS_MDREFR_VAL
> -	mov	r6, r4
> -	lsl	r4, #20
> -	lsr	r4, #20		/* Get a valid DRI field */
> +	ldr	r4, =( 0xFFF | MDREFR_K0DB4 | MDREFR_K0DB2 )
> +	orr	r4, #( MDREFR_K0FREE | MDREFR_K1FREE | MDREFR_K2FREE )
> +	bic	r5, r5, r4	/* clear DRI, K0DB2, K0DB4, KxFREE fields */
> 
> -	orr	r5, r5, r4	/* MDREFR user config with correct DRI */

You can just wipe all the bits away, can't you ?

> +	/*
> +	 * r3 is busy with MEMC_BASE,
> +	 * r4, r5, r6 used in pxa_wait_ticks and other places,
> +	 * so use r7 to store user specified MDREFR_VAL
> +	 */
> +	ldr	r7, =CONFIG_SYS_MDREFR_VAL
> +	and	r4, r7, r4
> +	orr	r5, r5, r4	/* use custom DRI, K0DB2, K0DB4, KxFREE */

And replace it with user config (if the user config is done correctly of course. 
If the user config is incorrect, that's what should be fixed.)
> 
>  	orr	r5, #MDREFR_K0RUN
>  	orr	r5, #MDREFR_SLFRSH
>  	bic	r5, #MDREFR_APD
>  	bic	r5, #MDREFR_E1PIN
> +	bic	r5, #MDREFR_K1RUN
> +	bic	r5, #MDREFR_K2RUN

Then just fine-tune the bits according 6.4.10 in PXA270 manual.

btw. these bic's are unneeded according to the manual.

Ok, please try explaining why is this solution better from what was here? I'm 
kind of missing it, sorry.

> 
>  	str	r5, [r3, #MDREFR_OFFSET]
> -	ldr	r4, [r3, #MDREFR_OFFSET]
> +	ldr	r5, [r3, #MDREFR_OFFSET]
> 
>  	/*
>  	 * 5) Initialize Synchronous Static Memory (Flash/Peripherals)
> @@ -184,16 +199,29 @@
>  	write32rb	(MEMC_BASE + SXCNFG_OFFSET), CONFIG_SYS_SXCNFG_VAL
> 
>  	/*
> -	 * 6) Initialize SDRAM
> +	 * 6) Initialize SDRAM,
> +	 *    also we must properly set MDREFR[K1DB2] and MDREFR[K2DB2]
> +	 *
> +	 *    WARNING: K1DB2 and K2DB2 bits are usually set
>  	 */
> 
> -	bic	r6, #MDREFR_SLFRSH
> -	str	r6, [r3, #MDREFR_OFFSET]
> -	ldr	r4, [r3, #MDREFR_OFFSET]
> +	and	r4, r7, #( MDREFR_K1DB2 | MDREFR_K2DB2 )
> +	ldr	r5, [r3, #MDREFR_OFFSET]
> +	bic	r5, #( MDREFR_K1DB2 | MDREFR_K2DB2 )
> +	orr	r5, r5, r4
> 
> -	orr	r6, #MDREFR_E1PIN
> -	str	r6, [r3, #MDREFR_OFFSET]
> -	ldr	r4, [r3, #MDREFR_OFFSET]
> +	orr	r5, #MDREFR_K1RUN
> +	orr	r5, #MDREFR_K2RUN

No, don't enable this unconditionally.

> +	str	r5, [r3, #MDREFR_OFFSET]
> +	ldr	r5, [r3, #MDREFR_OFFSET]
> +
> +	bic	r5, #MDREFR_SLFRSH
> +	str	r5, [r3, #MDREFR_OFFSET]
> +	ldr	r5, [r3, #MDREFR_OFFSET]
> +
> +	orr	r5, #MDREFR_E1PIN
> +	str	r5, [r3, #MDREFR_OFFSET]
> +	ldr	r5, [r3, #MDREFR_OFFSET]
> 
>  	/*
>  	 * 7) Write MDCNFG with MDCNFG:DEx deasserted (set to 0), to configure
> @@ -246,14 +274,17 @@
>  	ldr     r4, [r3, #MDMRS_OFFSET]
> 
>  	/*
> -	 * 11) Enable APD
> +	 * 11) Optionaly enable MDREFR[APD]
> +	 *
> +	 *     WARNING: APD bit is usually set.
>  	 */
> 
> -	ldr	r4, [r3, #MDREFR_OFFSET]
> -	and	r6, r6, #MDREFR_APD
> -	orr	r4, r4, r6
> -	str	r4, [r3, #MDREFR_OFFSET]
> -	ldr	r4, [r3, #MDREFR_OFFSET]
> +	and	r7, #MDREFR_APD
> +	ldr	r5, [r3, #MDREFR_OFFSET]
> +	bic	r5, #MDREFR_APD
> +	orr	r5, r5, r7
> +	str	r5, [r3, #MDREFR_OFFSET]
> +	ldr	r5, [r3, #MDREFR_OFFSET]

Why are you adding complexity here ?

>  .endm
> 
>  /*

Good work so far, cheers!