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

[Tirumala Marri <tmarri@apm.com>]

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