Patchwork PPC: Enable the Watchdog vector for 405

login
register
mail settings
Submitter Jason Gunthorpe
Date Sept. 30, 2012, 11:27 p.m.
Message ID <20120930232723.GF30637@obsidianresearch.com>
Download mbox | patch
Permalink /patch/188205/
State Superseded
Headers show

Comments

Jason Gunthorpe - Sept. 30, 2012, 11:27 p.m.
Move the body of the PIT exception out of line to make room.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 arch/powerpc/kernel/head_40x.S |   26 +++++++++++++-------------
 arch/powerpc/kernel/traps.c    |    2 +-
 2 files changed, 14 insertions(+), 14 deletions(-)
Josh Boyer - Oct. 1, 2012, 12:16 p.m.
On Sun, Sep 30, 2012 at 7:27 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> Move the body of the PIT exception out of line to make room.

What boards did you test this on?  What driver are you using for the
watchdog?

> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  arch/powerpc/kernel/head_40x.S |   26 +++++++++++++-------------
>  arch/powerpc/kernel/traps.c    |    2 +-
>  2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/kernel/head_40x.S b/arch/powerpc/kernel/head_40x.S
> index 4989661..7edd7b1 100644
> --- a/arch/powerpc/kernel/head_40x.S
> +++ b/arch/powerpc/kernel/head_40x.S
> @@ -431,29 +431,19 @@ label:
>
>  /* 0x1000 - Programmable Interval Timer (PIT) Exception */
>         START_EXCEPTION(0x1000, Decrementer)
> -       NORMAL_EXCEPTION_PROLOG
> -       lis     r0,TSR_PIS@h
> -       mtspr   SPRN_TSR,r0             /* Clear the PIT exception */
> -       addi    r3,r1,STACK_FRAME_OVERHEAD
> -       EXC_XFER_LITE(0x1000, timer_interrupt)
> +       b pit_longer
>
> -#if 0
>  /* NOTE:
> - * FIT and WDT handlers are not implemented yet.
> + * FIT handler is not implemented yet.
>   */
>
>  /* 0x1010 - Fixed Interval Timer (FIT) Exception
>  */
> -       STND_EXCEPTION(0x1010,  FITException,           unknown_exception)
> +//     STND_EXCEPTION(0x1010,  FITException,           unknown_exception)

Please just move the #endif for the #if 0 up instead of putting a C++
style comment here.

>  /* 0x1020 - Watchdog Timer (WDT) Exception
>  */
> -#ifdef CONFIG_BOOKE_WDT
>         CRITICAL_EXCEPTION(0x1020, WDTException, WatchdogException)
> -#else
> -       CRITICAL_EXCEPTION(0x1020, WDTException, unknown_exception)
> -#endif
> -#endif

Please leave this wrapped in CONFIG_BOOKE_WDT.  I don't agree with
unconditionally enabling this for every 405 chip out there.

>  /* 0x1100 - Data TLB Miss Exception
>   * As the name implies, translation is not in the MMU, so search the
> @@ -738,6 +728,16 @@ label:
>                 (MSR_KERNEL & ~(MSR_ME|MSR_DE|MSR_CE)), \
>                 NOCOPY, crit_transfer_to_handler, ret_from_crit_exc)
>
> +       /* Programmable Interval Timer (PIT) Exception. The PIT runs into
> +          the space reserved for other exceptions, so we branch down
> +          to here. */
> +pit_longer:
> +       NORMAL_EXCEPTION_PROLOG
> +       lis     r0,TSR_PIS@h
> +       mtspr   SPRN_TSR,r0             /* Clear the PIT exception */
> +       addi    r3,r1,STACK_FRAME_OVERHEAD
> +       EXC_XFER_LITE(0x1000, timer_interrupt)
> +
>  /*
>   * The other Data TLB exceptions bail out to this point
>   * if they can't resolve the lightweight TLB fault.
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index ae0843f..0701ec1 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -1514,7 +1514,7 @@ void unrecoverable_exception(struct pt_regs *regs)
>         die("Unrecoverable exception", regs, SIGABRT);
>  }
>
> -#ifdef CONFIG_BOOKE_WDT
> +#if defined(CONFIG_BOOKE_WDT) | defined(CONFIG_40x)

Pretty sure you meant || here?  Thought if you just enable the existing
config option, I don't think you'd need to edit this file at all.

josh
Jason Gunthorpe - Oct. 1, 2012, 4:25 p.m.
On Mon, Oct 01, 2012 at 08:16:29AM -0400, Josh Boyer wrote:
> On Sun, Sep 30, 2012 at 7:27 PM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
> > Move the body of the PIT exception out of line to make room.
> 
> What boards did you test this on?  What driver are you using for the
> watchdog?

Tested on a 405F6 core (Xilinx's variant), the board is custom, and
the control for the watchdog SPRs was bundled into a watchdog driver
for the board's watchdog controller.

> >  /* 0x1010 - Fixed Interval Timer (FIT) Exception
> >  */
> > -       STND_EXCEPTION(0x1010,  FITException,           unknown_exception)
> > +//     STND_EXCEPTION(0x1010,  FITException,           unknown_exception)
> 
> Please just move the #endif for the #if 0 up instead of putting a C++
> style comment here.

Sure
 
> >  /* 0x1020 - Watchdog Timer (WDT) Exception
> >  */
> > -#ifdef CONFIG_BOOKE_WDT
> >         CRITICAL_EXCEPTION(0x1020, WDTException, WatchdogException)
> > -#else
> > -       CRITICAL_EXCEPTION(0x1020, WDTException, unknown_exception)
> > -#endif
> > -#endif
> 
> Please leave this wrapped in CONFIG_BOOKE_WDT.  I don't agree with
> unconditionally enabling this for every 405 chip out there.

What are you concerned with? If some core varient does not put a
watchdog there, then you still get a panic from the default watchdog
exception handler..

> > -#ifdef CONFIG_BOOKE_WDT
> > +#if defined(CONFIG_BOOKE_WDT) | defined(CONFIG_40x)
> 
> Pretty sure you meant || here?  Thought if you just enable the existing
> config option, I don't think you'd need to edit this file at all.

Yes, I didn't want to use BOOKE_WDT because I have not tested that
driver, nor do I want that driver included in my kernel.. I think the
watchdog driver in use should be orthogonal to having the exception
wired in?

Jason
Josh Boyer - Oct. 1, 2012, 5:32 p.m.
On Mon, Oct 1, 2012 at 12:25 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Mon, Oct 01, 2012 at 08:16:29AM -0400, Josh Boyer wrote:
>> On Sun, Sep 30, 2012 at 7:27 PM, Jason Gunthorpe
>> <jgunthorpe@obsidianresearch.com> wrote:
>> > Move the body of the PIT exception out of line to make room.
>>
>> What boards did you test this on?  What driver are you using for the
>> watchdog?
>
> Tested on a 405F6 core (Xilinx's variant), the board is custom, and
> the control for the watchdog SPRs was bundled into a watchdog driver
> for the board's watchdog controller.
>
>> >  /* 0x1020 - Watchdog Timer (WDT) Exception
>> >  */
>> > -#ifdef CONFIG_BOOKE_WDT
>> >         CRITICAL_EXCEPTION(0x1020, WDTException, WatchdogException)
>> > -#else
>> > -       CRITICAL_EXCEPTION(0x1020, WDTException, unknown_exception)
>> > -#endif
>> > -#endif
>>
>> Please leave this wrapped in CONFIG_BOOKE_WDT.  I don't agree with
>> unconditionally enabling this for every 405 chip out there.
>
> What are you concerned with? If some core varient does not put a
> watchdog there, then you still get a panic from the default watchdog
> exception handler..

I'm concerned with the fact that you've moved PIT and now enabled
something that's been enabled for years.  There's no need to do it like
that.

>> > -#ifdef CONFIG_BOOKE_WDT
>> > +#if defined(CONFIG_BOOKE_WDT) | defined(CONFIG_40x)
>>
>> Pretty sure you meant || here?  Thought if you just enable the existing
>> config option, I don't think you'd need to edit this file at all.
>
> Yes, I didn't want to use BOOKE_WDT because I have not tested that
> driver, nor do I want that driver included in my kernel.. I think the
> watchdog driver in use should be orthogonal to having the exception
> wired in?

And it certainly can be.  Just make the driver a module and don't
install it or load it.  The #ifdef will still evaluate to true.

josh
Jason Gunthorpe - Oct. 1, 2012, 5:48 p.m.
On Mon, Oct 01, 2012 at 01:32:47PM -0400, Josh Boyer wrote:
> On Mon, Oct 1, 2012 at 12:25 PM, Jason Gunthorpe

> >> Please leave this wrapped in CONFIG_BOOKE_WDT.  I don't agree with
> >> unconditionally enabling this for every 405 chip out there.
> >
> > What are you concerned with? If some core varient does not put a
> > watchdog there, then you still get a panic from the default watchdog
> > exception handler..
> 
> I'm concerned with the fact that you've moved PIT and now enabled
> something that's been enabled for years.  There's no need to do it like
> that.

Well, just moving the ifdef still keeps the PIT change, and either the
vector is never called and it is harmless to add the new entry point,
or CPUs have been randomly calling into DTLBMiss for years, which
seems worth discovering.

FWIW, this patch has been carried in our tree since about 2.6.14,
mind you we only use two 405 varients.

> > Yes, I didn't want to use BOOKE_WDT because I have not tested that
> > driver, nor do I want that driver included in my kernel.. I think the
> > watchdog driver in use should be orthogonal to having the exception
> > wired in?
> 
> And it certainly can be.  Just make the driver a module and don't
> install it or load it.  The #ifdef will still evaluate to true.

Well, we use non-modular kernels, but I can certainly patch the driver
out.

If I resend using BOOKE_WDT will you take it?

Thanks,
Jason
Benjamin Herrenschmidt - Oct. 4, 2012, 11:05 a.m.
On Sun, 2012-09-30 at 17:27 -0600, Jason Gunthorpe wrote:
> diff --git a/arch/powerpc/kernel/head_40x.S b/arch/powerpc/kernel/head_40x.S
> index 4989661..7edd7b1 100644
> --- a/arch/powerpc/kernel/head_40x.S
> +++ b/arch/powerpc/kernel/head_40x.S
> @@ -431,29 +431,19 @@ label:
>  
>  /* 0x1000 - Programmable Interval Timer (PIT) Exception */
>  	START_EXCEPTION(0x1000, Decrementer)
> -	NORMAL_EXCEPTION_PROLOG
> -	lis	r0,TSR_PIS@h
> -	mtspr	SPRN_TSR,r0		/* Clear the PIT exception */
> -	addi	r3,r1,STACK_FRAME_OVERHEAD
> -	EXC_XFER_LITE(0x1000, timer_interrupt)
> +	b pit_longer

Looks like you indeed have no choice but move it down, though I dislike
the label name :-)

Look at how we do a similar thing in exceptions-64.S, we basically just
don't use START_EXCEPTION at that location. We put a .=xxxx and a
branch, and use the real exception name at the target label.

Or just name it "pit_exception" if you want to keep things simple. I
just don't like "pit_longer" :-)
 
> -#if 0
>  /* NOTE:
> - * FIT and WDT handlers are not implemented yet.
> + * FIT handler is not implemented yet.
>   */

Any reason to comment that out ? Better off also branching out of line
to a stub similar to the PIT one that then calls unknown_exception. That
way if it triggers by accident, you'll get a clean trace.

>  /* 0x1010 - Fixed Interval Timer (FIT) Exception
>  */
> -	STND_EXCEPTION(0x1010,	FITException,		unknown_exception)
> +//	STND_EXCEPTION(0x1010,	FITException,		unknown_exception)
>  
>  /* 0x1020 - Watchdog Timer (WDT) Exception
>  */
> -#ifdef CONFIG_BOOKE_WDT
>  	CRITICAL_EXCEPTION(0x1020, WDTException, WatchdogException)
> -#else
> -	CRITICAL_EXCEPTION(0x1020, WDTException, unknown_exception)
> -#endif
> -#endif

Move it out of line too please. When a given vector "slot" gets crowded,
I prefer moving everything in it out of line to keep things consistent.
 
>  /* 0x1100 - Data TLB Miss Exception
>   * As the name implies, translation is not in the MMU, so search the
> @@ -738,6 +728,16 @@ label:
>  		(MSR_KERNEL & ~(MSR_ME|MSR_DE|MSR_CE)), \
>  		NOCOPY, crit_transfer_to_handler, ret_from_crit_exc)
>  
> +	/* Programmable Interval Timer (PIT) Exception. The PIT runs into
> +	   the space reserved for other exceptions, so we branch down
> +	   to here. */
> +pit_longer:
> +	NORMAL_EXCEPTION_PROLOG
> +	lis	r0,TSR_PIS@h
> +	mtspr	SPRN_TSR,r0		/* Clear the PIT exception */
> +	addi	r3,r1,STACK_FRAME_OVERHEAD
> +	EXC_XFER_LITE(0x1000, timer_interrupt)
> +
>  /*
>   * The other Data TLB exceptions bail out to this point
>   * if they can't resolve the lightweight TLB fault.
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index ae0843f..0701ec1 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -1514,7 +1514,7 @@ void unrecoverable_exception(struct pt_regs *regs)
>  	die("Unrecoverable exception", regs, SIGABRT);
>  }
>  
> -#ifdef CONFIG_BOOKE_WDT
> +#if defined(CONFIG_BOOKE_WDT) | defined(CONFIG_40x)
>  /*
>   * Default handler for a Watchdog exception,
>   * spins until a reboot occurs

Cheers,
Ben.

Patch

diff --git a/arch/powerpc/kernel/head_40x.S b/arch/powerpc/kernel/head_40x.S
index 4989661..7edd7b1 100644
--- a/arch/powerpc/kernel/head_40x.S
+++ b/arch/powerpc/kernel/head_40x.S
@@ -431,29 +431,19 @@  label:
 
 /* 0x1000 - Programmable Interval Timer (PIT) Exception */
 	START_EXCEPTION(0x1000, Decrementer)
-	NORMAL_EXCEPTION_PROLOG
-	lis	r0,TSR_PIS@h
-	mtspr	SPRN_TSR,r0		/* Clear the PIT exception */
-	addi	r3,r1,STACK_FRAME_OVERHEAD
-	EXC_XFER_LITE(0x1000, timer_interrupt)
+	b pit_longer
 
-#if 0
 /* NOTE:
- * FIT and WDT handlers are not implemented yet.
+ * FIT handler is not implemented yet.
  */
 
 /* 0x1010 - Fixed Interval Timer (FIT) Exception
 */
-	STND_EXCEPTION(0x1010,	FITException,		unknown_exception)
+//	STND_EXCEPTION(0x1010,	FITException,		unknown_exception)
 
 /* 0x1020 - Watchdog Timer (WDT) Exception
 */
-#ifdef CONFIG_BOOKE_WDT
 	CRITICAL_EXCEPTION(0x1020, WDTException, WatchdogException)
-#else
-	CRITICAL_EXCEPTION(0x1020, WDTException, unknown_exception)
-#endif
-#endif
 
 /* 0x1100 - Data TLB Miss Exception
  * As the name implies, translation is not in the MMU, so search the
@@ -738,6 +728,16 @@  label:
 		(MSR_KERNEL & ~(MSR_ME|MSR_DE|MSR_CE)), \
 		NOCOPY, crit_transfer_to_handler, ret_from_crit_exc)
 
+	/* Programmable Interval Timer (PIT) Exception. The PIT runs into
+	   the space reserved for other exceptions, so we branch down
+	   to here. */
+pit_longer:
+	NORMAL_EXCEPTION_PROLOG
+	lis	r0,TSR_PIS@h
+	mtspr	SPRN_TSR,r0		/* Clear the PIT exception */
+	addi	r3,r1,STACK_FRAME_OVERHEAD
+	EXC_XFER_LITE(0x1000, timer_interrupt)
+
 /*
  * The other Data TLB exceptions bail out to this point
  * if they can't resolve the lightweight TLB fault.
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index ae0843f..0701ec1 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1514,7 +1514,7 @@  void unrecoverable_exception(struct pt_regs *regs)
 	die("Unrecoverable exception", regs, SIGABRT);
 }
 
-#ifdef CONFIG_BOOKE_WDT
+#if defined(CONFIG_BOOKE_WDT) | defined(CONFIG_40x)
 /*
  * Default handler for a Watchdog exception,
  * spins until a reboot occurs