| Message ID | 20120930232723.GF30637@obsidianresearch.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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.
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
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(-)