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

Wolfgang Denk
2010-09-02T11:55:35Z

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.