From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1755566AbdESKYs convert rfc822-to-8bit (ORCPT <rfc822;w@1wt.eu>);
        Fri, 19 May 2017 06:24:48 -0400
Received: from ozlabs.org ([103.22.144.67]:55543 "EHLO ozlabs.org"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752145AbdESKYr (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 19 May 2017 06:24:47 -0400
From: Michael Ellerman <mpe@ellerman.id.au>
To: Ivan Mikhaylov <ivan@de.ibm.com>,
        Alistair Popple <alistair@popple.id.au>,
        Matt Porter <mporter@kernel.crashing.org>
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Joel Stanley <joel@ozlabs.au.ibm.com>,
        Paul Mackerras <paulus@samba.org>
Subject: Re: [PATCH 4/4] arch/powerpc/44x/fsp2: wdt tcr update instead of whole rewrite
In-Reply-To: <20170515130754.59363-5-ivan@de.ibm.com>
References: <20170515130754.59363-5-ivan@de.ibm.com>
User-Agent: Notmuch/0.21 (https://notmuchmail.org)
Date: Fri, 19 May 2017 20:24:44 +1000
Message-ID: <87wp9d5ekz.fsf@concordia.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Ivan,

Ivan Mikhaylov <ivan@de.ibm.com> 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