| Message ID | 20090923135156.GH14261@zod.rchland.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, 2009-09-23 at 09:51 -0400, Josh Boyer wrote: > Prior to the arch/ppc -> arch/powerpc transition, xmon had support for single > stepping on 4xx boards. The functionality was lost when arch/ppc was removed. > This patch restores single step support for 44x boards. > > Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com> > --- > arch/powerpc/xmon/xmon.c | 20 +++++++++++++++++++- > 1 files changed, 19 insertions(+), 1 deletions(-) > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > index c6f0a71..fe2ad71 100644 > --- a/arch/powerpc/xmon/xmon.c > +++ b/arch/powerpc/xmon/xmon.c > @@ -517,6 +517,15 @@ static int xmon_core(struct pt_regs *regs, int fromipi) > in_xmon = 0; > #endif > > +#ifdef CONFIG_4xx > + if ((regs->msr & (MSR_DE)) == (MSR_DE)) { Why not just if (regs->msr & MSR_DE) ? > + bp = at_breakpoint(regs->nip); > + if (bp != NULL) { > + regs->nip = (unsigned long) &bp->instr[0]; > + atomic_inc(&bp->ref_count); > + } > + } > +#else Any reason why that couldn't be in CONFIG_BOOKE ? > if ((regs->msr & (MSR_IR|MSR_PR|MSR_SF)) == (MSR_IR|MSR_SF)) { > bp = at_breakpoint(regs->nip); > if (bp != NULL) { > @@ -530,7 +539,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi) > } > } > } > - > +#endif > insert_cpu_bpts(); > > local_irq_restore(flags); > @@ -894,6 +903,14 @@ cmds(struct pt_regs *excp) > } > } > > +#ifdef CONFIG_44x Same comment about BOOKE intead of 44x > +static int do_step(struct pt_regs *regs) > +{ > + regs->msr |= MSR_DE; > + mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM); I'm not sure about setting IDM... Won't that be a problem if you have an external debugger connected ? > + return 1; > +} > +#else > /* > * Step a single instruction. > * Some instructions we emulate, others we execute with MSR_SE set. > @@ -924,6 +941,7 @@ static int do_step(struct pt_regs *regs) > regs->msr |= MSR_SE; > return 1; > } > +#endif Cheers, Ben. > static void bootcmds(void) > {
On Thu, Sep 24, 2009 at 07:34:34AM +1000, Benjamin Herrenschmidt wrote: >On Wed, 2009-09-23 at 09:51 -0400, Josh Boyer wrote: >> Prior to the arch/ppc -> arch/powerpc transition, xmon had support for single >> stepping on 4xx boards. The functionality was lost when arch/ppc was removed. >> This patch restores single step support for 44x boards. >> >> Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com> >> --- >> arch/powerpc/xmon/xmon.c | 20 +++++++++++++++++++- >> 1 files changed, 19 insertions(+), 1 deletions(-) >> >> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c >> index c6f0a71..fe2ad71 100644 >> --- a/arch/powerpc/xmon/xmon.c >> +++ b/arch/powerpc/xmon/xmon.c >> @@ -517,6 +517,15 @@ static int xmon_core(struct pt_regs *regs, int fromipi) >> in_xmon = 0; >> #endif >> >> +#ifdef CONFIG_4xx >> + if ((regs->msr & (MSR_DE)) == (MSR_DE)) { > >Why not just if (regs->msr & MSR_DE) ? Blind duplication of existing if case. Will fix. >> + bp = at_breakpoint(regs->nip); >> + if (bp != NULL) { >> + regs->nip = (unsigned long) &bp->instr[0]; >> + atomic_inc(&bp->ref_count); >> + } >> + } >> +#else > >Any reason why that couldn't be in CONFIG_BOOKE ? Off the top of my head, no. I haven't tested on 40x yet though. Will try and do that and revise. >> if ((regs->msr & (MSR_IR|MSR_PR|MSR_SF)) == (MSR_IR|MSR_SF)) { >> bp = at_breakpoint(regs->nip); >> if (bp != NULL) { >> @@ -530,7 +539,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi) >> } >> } >> } >> - >> +#endif >> insert_cpu_bpts(); >> >> local_irq_restore(flags); >> @@ -894,6 +903,14 @@ cmds(struct pt_regs *excp) >> } >> } >> >> +#ifdef CONFIG_44x > >Same comment about BOOKE intead of 44x Nod. >> +static int do_step(struct pt_regs *regs) >> +{ >> + regs->msr |= MSR_DE; >> + mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM); > >I'm not sure about setting IDM... Won't that be a problem if you have >an external debugger connected ? It could be. I have no external debugger, thus no way to check it. You don't get an exception without IDM set though, so it won't trap back into xmon like it should. This is how we did it in arch/ppc (which isn't always a great thing) as well. I don't see how you could get it working without IDM, unless you inserted a trap (aka breakpoint) every time. That seems sort of suboptimal when we have the IC event we can use. josh
On Wed, 2009-09-23 at 18:35 -0400, Josh Boyer wrote: > >Any reason why that couldn't be in CONFIG_BOOKE ? > > Off the top of my head, no. I haven't tested on 40x yet though. Will try > and do that and revise. Does 40x have CONFIG_BOOKE ? I was thinking more about FSL etc... but yeah, 40x is worth having a look too. > It could be. I have no external debugger, thus no way to check it. You don't > get an exception without IDM set though, so it won't trap back into xmon like > it should. This is how we did it in arch/ppc (which isn't always a great > thing) as well. Ok, let's leave it there for now. > I don't see how you could get it working without IDM, unless you inserted a > trap (aka breakpoint) every time. That seems sort of suboptimal when we have > the IC event we can use. The question is more whether IDM should be set once for all at boot (or not) but let's ignore that for now. Ben.
On Thu, Sep 24, 2009 at 07:34:34AM +1000, Benjamin Herrenschmidt wrote: > On Wed, 2009-09-23 at 09:51 -0400, Josh Boyer wrote: [snip] > > +static int do_step(struct pt_regs *regs) > > +{ > > + regs->msr |= MSR_DE; > > + mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM); > > I'm not sure about setting IDM... Won't that be a problem if you have > an external debugger connected ? Not in theory. It should be possible to set both IDM and EDM simultaneously.
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c index c6f0a71..fe2ad71 100644 --- a/arch/powerpc/xmon/xmon.c +++ b/arch/powerpc/xmon/xmon.c @@ -517,6 +517,15 @@ static int xmon_core(struct pt_regs *regs, int fromipi) in_xmon = 0; #endif +#ifdef CONFIG_4xx + if ((regs->msr & (MSR_DE)) == (MSR_DE)) { + bp = at_breakpoint(regs->nip); + if (bp != NULL) { + regs->nip = (unsigned long) &bp->instr[0]; + atomic_inc(&bp->ref_count); + } + } +#else if ((regs->msr & (MSR_IR|MSR_PR|MSR_SF)) == (MSR_IR|MSR_SF)) { bp = at_breakpoint(regs->nip); if (bp != NULL) { @@ -530,7 +539,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi) } } } - +#endif insert_cpu_bpts(); local_irq_restore(flags); @@ -894,6 +903,14 @@ cmds(struct pt_regs *excp) } } +#ifdef CONFIG_44x +static int do_step(struct pt_regs *regs) +{ + regs->msr |= MSR_DE; + mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_IC | DBCR0_IDM); + return 1; +} +#else /* * Step a single instruction. * Some instructions we emulate, others we execute with MSR_SE set. @@ -924,6 +941,7 @@ static int do_step(struct pt_regs *regs) regs->msr |= MSR_SE; return 1; } +#endif static void bootcmds(void) {
Prior to the arch/ppc -> arch/powerpc transition, xmon had support for single stepping on 4xx boards. The functionality was lost when arch/ppc was removed. This patch restores single step support for 44x boards. Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com> --- arch/powerpc/xmon/xmon.c | 20 +++++++++++++++++++- 1 files changed, 19 insertions(+), 1 deletions(-)