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

Tirumala Marri
2010-09-03T01:48:44Z

Mr Wolfgang,
> Hm... is this APM82? Official press releases refer to this as
> APM821xx? Or ist it APM82xxx?
[Marri] You are correct APM821XX is right name.

>
> If you don't decide to print "64" here, then please pull all the
> APM82???
> code into a separate #ifdef block.
[Marri] Sure will do.

>
> > -#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?
[Marri] No, It is same as other SoCs.

>
> > 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.

[Marri] I will define CONFIG_SYS_OCM_SIZE . But for now this is only added

Bluestone.h. I need to remove "#if defined(CONFIG_APM821XX) I will have
to
Add CONFIG_SYS_OCM_SIZE to all the board config files which are based on
460Ex and 460GT.
I think that would be completely different patch.

>
>
> > 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?
>
[Marri] I will work on this .

Regards,
Marri