[U-Boot] [PATCH 1/7] mpc85xx: Support a board-specific processor reset routines

[Moffett, Kyle D]

On Feb 21, 2011, at 15:59, Wolfgang Denk wrote:
> In message <1298311199-18775-2-git-send-email-Kyle.D.Moffett at boeing.com> you wrote:
>> Some board models (such as the submitted P2020-based HWW-1U-1A hardware)
>> need specialized code to run when a reset is requested to ensure proper
>> synchronization with other hardware.
>> 
>> In order to facilitate such board ports, we add a board_reset_r()
>> routine which is called from the do_reset() command function instead of
>> directly poking at the MPC85xx "RSTCR" (reset control) register.
>> 
>> Signed-off-by: Kyle Moffett <Kyle.D.Moffett at boeing.com>
>> ---
>> arch/powerpc/cpu/mpc85xx/cpu.c |    7 +++++--
>> 1 files changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/powerpc/cpu/mpc85xx/cpu.c b/arch/powerpc/cpu/mpc85xx/cpu.c
>> index 1aad2ba..dbc662f 100644
>> --- a/arch/powerpc/cpu/mpc85xx/cpu.c
>> +++ b/arch/powerpc/cpu/mpc85xx/cpu.c
>> @@ -204,8 +204,10 @@ int checkcpu (void)
>> 
>> int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> {
>> -/* Everything after the first generation of PQ3 parts has RSTCR */
>> -#if defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
>> +#if defined(CONFIG_BOARD_RESET_R)
>> +	extern void board_reset_r(void);
>> +	board_reset_r();
>> +#elif defined(CONFIG_MPC8540) || defined(CONFIG_MPC8541) || \
>>     defined(CONFIG_MPC8555) || defined(CONFIG_MPC8560)
>> 	unsigned long val, msr;
> 
> Please implement without #ifdef's using a weak function - see
> arch/powerpc/cpu/mpc86xx/cpu.c for an example.  Actually you might
> want to facto this out into common code.
> 
> Side note: don't use ever "extern" function declarations in the code;
> use proper prototype declarations in some header file instead.

Hmm, ok.  I thought that looked kind of funny but I copied it from somewhere else in U-Boot so I figured it was fine.

I'll see if I can't come up with something a little more generic, otherwise I'll use the weak function approach.


>> @@ -221,6 +223,7 @@ int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> 	val |= 0x70000000;
>> 	mtspr(DBCR0,val);
>> #else
>> +	/* Everything after the first generation of PQ3 parts has RSTCR */
>> 	volatile ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
> 
> Note that this declaration is now basicy in the middle of code, which
> is ugly at best.
> 
> What is the "volatile" needed for?

It was always in the middle of code, I just moved the comment.  I also didn't add the volatile, and I have no idea why anybody thought it was needed in the first place.

I'll fix it in v4.  Thanks!

Cheers,
Kyle Moffett