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 Date: Fri, 19 May 2017 20:24:44 +1000 [thread overview] Message-ID: <87wp9d5ekz.fsf@concordia.ellerman.id.au> (raw) In-Reply-To: <20170515130754.59363-5-ivan@de.ibm.com> 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
WARNING: multiple messages have this Message-ID (diff)
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 Date: Fri, 19 May 2017 20:24:44 +1000 [thread overview] Message-ID: <87wp9d5ekz.fsf@concordia.ellerman.id.au> (raw) In-Reply-To: <20170515130754.59363-5-ivan@de.ibm.com> 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, u= nsigned long *val) >=20=20 > 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); >=20=20 > +#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 =3D mfspr(SPRN_TCR); > + tcr &=3D TCR_WP_MASK; /* clear all bits except for TCR[WP] */ > + tcr |=3D 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 =E2=80=98start_cpu_decrementer=E2= =80=99: arch/powerpc/kernel/time.c:741:15: error: unused variable =E2=80=98tcr=E2= =80=99 [-Werror=3Dunused-variable] You can just do something like: #if defined(CONFIG_BOOKE) || defined(CONFIG_40x) unsigned int tcr; #ifdef CONFIG_FSP2 tcr =3D mfspr(SPRN_TCR); tcr &=3D TCR_WP_MASK; /* clear all bits except for TCR[WP] */ #else tcr =3D 0; #endif tcr |=3D 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
next prev parent reply other threads:[~2017-05-19 10:24 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-05-15 13:07 [PATCH 4/4] arch/powerpc/44x/fsp2: wdt tcr update instead of whole rewrite Ivan Mikhaylov 2017-05-19 10:24 ` Michael Ellerman [this message] 2017-05-19 10:24 ` Michael Ellerman 2017-05-19 15:47 ` Ivan Mikhaylov 2017-05-19 15:47 ` Ivan Mikhaylov 2017-05-23 4:09 ` Michael Ellerman 2017-05-26 10:37 ` Michael Ellerman 2017-05-30 9:11 ` [4/4] " Michael Ellerman 2017-05-19 10:30 ` [PATCH 4/4] " Benjamin Herrenschmidt
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87wp9d5ekz.fsf@concordia.ellerman.id.au \ --to=mpe@ellerman.id.au \ --cc=alistair@popple.id.au \ --cc=benh@kernel.crashing.org \ --cc=ivan@de.ibm.com \ --cc=joel@ozlabs.au.ibm.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mporter@kernel.crashing.org \ --cc=paulus@samba.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.