[U-Boot] [PATCH] PPC4xx: PLB4 Arbiter and Memory Queue Optimizations

[Stefan Roese]

Hi Prodyut,

On Monday 11 August 2008, Prodyut Hazarika wrote:
> Please put your views on the replies below:
> > OK. But on which 4xx boards did you test this change? And could you
>
> please remove the duplicated code from the "other", non-AMCC boards too
> with your > next patch version (DU440, PMC440, korat, lwmon5, pcs440ep).
> I added the other board maintainers to CC.
>
> I have tested on Canyonlands, Glacier, Yosemite and Kilauea boards.

OK, thanks.

> I 
> will make the changes for the other non-AMCC boards and resubmit. I
> cannot test on non-AMCC boards, though I don't think this patch can
> cause an issue.

Of course you can't test this on non-AMCC boards. Thanks for further cleaning 
up.

> >> -	mtdcr(SDRAM_PLBADDULL, 0x00000000); /* MQ0_BAUL */
> >> -	mtdcr(SDRAM_PLBADDUHB, 0x00000008); /* MQ0_BAUH */
> >> +	{
> >
> > I don't particularly like this extra "{" level without need. Please
>
> remove it.
>
> The { is kept to avoid warnings. Without the "{", I would need to define
> the local variable near
> The beginning of the function. That would cause warning (Unused
> variable) for non 440EX/GT families.
> Please advice whether you want this warning to come up, if I remove the
> "{"

I knew why you did it this way (have been there before), but your "solution" 
is not the preferred one. As Wolfgang already explained the variable 
declarations should go to the beginning of the function and if warnings are 
seen on other platforms, then the declaration needs an #ifdef too.

> > How about 440EPx/GRx and it's problem with the PLB4A0_ACR[WRP].
> Currently this gets cleared in a board specific file because of the MAL
> burst
> > problems in Linux when its set. Shouldn't we configure it correctly in
> this place for all 440EPx/GRx and remove it from the board specific
> code?
>
> This patch is getting bigger and bigger. I will put that cleanup in a
> separate patch, since it for a different set of cleanups.

OK. I just came to my mind after looking at the arbiter defines and their 
usage. We just should not forget to clean this up soon.

> >> +#define plb1_bearl                (PLB_ARBITER_BASE+ 0x0C)
> >> +#define plb1_bearh                (PLB_ARBITER_BASE+ 0x0D)
> >
> > I would really prefer to have uppercase defines here. I know that this
> is not your "invention" but still we should probably clean this up now,
> that
> > we are busy with these PLB arbiter defines. So could you please change
> those defines to uppercase? Thanks.
>
> I will change all to uppercase and resubmit.

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================