Dear tma...@apm.com, In message <1283390214-28255-1-git-send-email-tma...@apm.com> you wrote: > From: Tirumala Marri <tma...@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: w...@denx.de Due to lack of disk space, this fortune database has been discontinued. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot