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

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@boeing.com>
---
 arch/powerpc/cpu/mpc85xx/cpu.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

Dear Kyle Moffett,

In message <1298311199-18775-2-git-send-email-Kyle.D.Moffett@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@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.

> @@ -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?


Best regards,

Wolfgang Denk

On Feb 21, 2011, at 15:59, Wolfgang Denk wrote:
> In message <1298311199-18775-2-git-send-email-Kyle.D.Moffett@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@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