Dear tmarri@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)
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