Re: [PATCH 4/4] arch/powerpc/44x/fsp2: wdt tcr update instead of whole rewrite

[Michael Ellerman]

Hi Ivan,

Ivan Mikhaylov <ivan@xxxxxxxxxx> writes:
> Prevent a kernel panic caused by unintentionally clearing TCR
> watchdog bits.  At this point in the kernel boot, the watchdog has
> already been enabled by u-boot.  The original code's attempt to
> write to the TCR register results in an inadvertent clearing of the
> watchdog configuration bits, causing the 476 to reset.
> Panic happens in case of error as silently reboot without any outputs
> on serial.

That sounds reasonable.

> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index 2b33cfa..f75e512 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -738,12 +738,28 @@ static int __init get_freq(char *name, int cells, unsigned long *val)
>  
>  static void start_cpu_decrementer(void)
>  {
> +	unsigned int tcr;
>  #if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
>  	/* Clear any pending timer interrupts */
>  	mtspr(SPRN_TSR, TSR_ENW | TSR_WIS | TSR_DIS | TSR_FIS);
>  
> +#ifdef CONFIG_FSP2
> +	/*
> +	 * Prevent a kernel panic caused by unintentionally clearing TCR
> +	 * watchdog bits.  At this point in the kernel boot, the watchdog has
> +	 * already been enabled by u-boot.  The original code's attempt to

Don't refer to "the original code", as it doesn't exist anymore now that
we've patched it. Just say something like ".. so we must not clear the
watchdog configuration bits".

> +	 * write to the TCR register results in an inadvertent clearing of the
> +	 * watchdog configuration bits, causing the 440 to reset.
> +	 */
> +	tcr = mfspr(SPRN_TCR);
> +	tcr &= TCR_WP_MASK; /* clear all bits except for TCR[WP] */
> +	tcr |= TCR_DIE; /* enable decrementer */
> +	mtspr(SPRN_TCR, tcr);
> +#else
>  	/* Enable decrementer interrupt */
>  	mtspr(SPRN_TCR, TCR_DIE);
> +#endif
> +
>  #endif /* defined(CONFIG_BOOKE) || defined(CONFIG_40x) */

That breaks the build for other platforms:

  arch/powerpc/kernel/time.c: In function âstart_cpu_decrementerâ:
  arch/powerpc/kernel/time.c:741:15: error: unused variable âtcrâ [-Werror=unused-variable]

You can just do something like:

  #if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
  	unsigned int tcr;

  #ifdef CONFIG_FSP2
  	tcr = mfspr(SPRN_TCR);
  	tcr &= TCR_WP_MASK; /* clear all bits except for TCR[WP] */
  #else
  	tcr = 0;
  #endif
  	tcr |= TCR_DIE; /* enable decrementer */
  	mtspr(SPRN_TCR, TCR_DIE);
  #endif /* defined(CONFIG_BOOKE) || defined(CONFIG_40x) */


Or you could possibly just always leave TCR[WP], is there any case where
it would be correct to clear that?

cheers