[U-Boot] [PATCH v2 1/2] APM82xxx: Add CPU and other peripheral support

[Wolfgang Denk <wd@denx.de>]

Dear tmarri at apm.com,

In message <1283390214-28255-1-git-send-email-tmarri@apm.com> you wrote:
> From: Tirumala Marri <tmarri@apm.com>
> 
> APM82XXX is a new line of SoCs which are derivatives of
> PPC44X family of processors.
> 
> This patch adds support of CPU, cache,
> tlb, 32k ocm, bootstraps, PLB AHB bus, DDR and
> Some common register definitions.
...
>  #if defined(CONFIG_XILINX_440)
>  	puts("IBM PowerPC 4");
> +#elif defined(CONFIG_APM82XXX)
> +	puts("APM PowerPC APM82");

Hm... is this APM82? Official press releases refer to this as
APM821xx? Or ist it APM82xxx?

On the other hand, this seems to be a 464 core, so should we not print
464 here?

>  #else
>  	puts("AMCC PowerPC 4");
>  #endif
> @@ -316,7 +334,7 @@ int checkcpu (void)
>  #if defined(CONFIG_440)
>  #if defined(CONFIG_460EX) || defined(CONFIG_460GT)
>  	puts("60");
> -#else
> +#elif !defined(CONFIG_APM82XXX)
>  	puts("40");

This gets confusing.

If you don't decide to print "64" here, then please pull all the APM82???
code into a separate #ifdef block.

> --- a/arch/powerpc/cpu/ppc4xx/cpu_init.c
> +++ b/arch/powerpc/cpu/ppc4xx/cpu_init.c
> @@ -222,13 +222,15 @@ void reconfigure_pll(u32 new_cpu_freq)
>  void
>  cpu_init_f (void)
>  {
> -#if defined(CONFIG_WATCHDOG) || defined(CONFIG_440GX) || defined(CONFIG_460EX)
> +#if defined(CONFIG_WATCHDOG) || defined(CONFIG_440GX) || \
> +	defined(CONFIG_460EX)

Why do you modify this line?  Coding style fixes should go separate.

> -#if (defined(CONFIG_405EP) || defined (CONFIG_405EX)) && !defined(CONFIG_SYS_4xx_GPIO_TABLE)
> +#if (defined(CONFIG_405EP) || defined(CONFIG_405EX)) && \
> +	!defined(CONFIG_SYS_4xx_GPIO_TABLE) && !defined(CONFIG_APM82XXX)

Please keep list sorted.

> +#if defined(CONFIG_APM82XXX)
> +
> +/*
> + * Specific for APM82XXX
> + * Change:
> + *	PLL registers reflect the current PLL setting of the chip.
> + *	So unlike previous implementation that reads bootstrap
> + *	registers to determine system clocking information, this
> + *	implementation directly extracts the information from
> + *	current PLL registers values.
> + */

Please remove references to "previous implementation" which is unknown
here anyway.


> -#elif defined(CONFIG_440SPE) || defined(CONFIG_460EX) || defined(CONFIG_460GT)
> +#elif defined(CONFIG_440SPE) || defined(CONFIG_460EX) || \
> +      defined(CONFIG_460GT) || defined(CONFIG_APM82XXX)
>  	lis	r1,0x0000		/* BAS = X_0000_0000 */
>  	ori	r1,r1,0x0984		/* first 64k */

Should the size be adjusted here, too?

>  	mtdcr	ISRAM0_SB0CR,r1
> @@ -752,7 +754,11 @@ _start:
>  	mtdcr	ISRAM1_PMEG,r1
>  
>  	lis	r1,0x0004		/* BAS = 4_0004_0000 */
> +#if defined(CONFIG_APM82XXX) /* APM82XXX only has 32KB of OCM */
> +	ori     r1,r1,0x0784            /* 32k */
> +#else
>  	ori	r1,r1,0x0984		/* 64k */

Please get rid of these magic numbers and add a #define for a symbolic
constant so the code becomes readable, and the selection can be done
on a per-CPU base, i. e. without needing #ifdef's here.


> diff --git a/include/ppc440.h b/include/ppc440.h
> index c807dda..b5c1832 100644
> --- a/include/ppc440.h
> +++ b/include/ppc440.h

Hm... you are adding a number of large blocks of code, all #ifdef'ed.

This is ugly, difficult to read and difficult to maintain.

Why not adding a new file  include/apm82xxx.h  instead?


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
Due to lack of disk space, this fortune database has been discontinued.