* [PATCH] powerpc/8xx: fix single_step debug
@ 2016-08-18 9:44 Christophe Leroy
2016-08-18 9:58 ` Gabriel Paubert
0 siblings, 1 reply; 5+ messages in thread
From: Christophe Leroy @ 2016-08-18 9:44 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, Scott Wood
Cc: linux-kernel, linuxppc-dev
SPRN_ICR must be read for clearing the internal freeze signal which
is asserted by the single step exception, otherwise the timebase and
decrementer remain freezed
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/reg_8xx.h | 1 +
arch/powerpc/kernel/traps.c | 8 ++++++++
2 files changed, 9 insertions(+)
diff --git a/arch/powerpc/include/asm/reg_8xx.h b/arch/powerpc/include/asm/reg_8xx.h
index feaf641..6dae71f 100644
--- a/arch/powerpc/include/asm/reg_8xx.h
+++ b/arch/powerpc/include/asm/reg_8xx.h
@@ -17,6 +17,7 @@
#define SPRN_DC_DAT 570 /* Read-only data register */
/* Misc Debug */
+#define SPRN_ICR 148
#define SPRN_DPDR 630
#define SPRN_MI_CAM 816
#define SPRN_MI_RAM0 817
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 2cb5892..0f1f0ce 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -400,8 +400,16 @@ static inline int check_io_access(struct pt_regs *regs)
#define REASON_TRAP 0x20000
#define single_stepping(regs) ((regs)->msr & MSR_SE)
+#ifdef CONFIG_PPC_8xx
+static inline void clear_single_step(struct pt_regs *regs)
+{
+ regs->msr &= ~MSR_SE;
+ mfspr(SPRN_ICR);
+}
+#else
#define clear_single_step(regs) ((regs)->msr &= ~MSR_SE)
#endif
+#endif
#if defined(CONFIG_4xx)
int machine_check_4xx(struct pt_regs *regs)
--
2.1.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] powerpc/8xx: fix single_step debug
2016-08-18 9:44 [PATCH] powerpc/8xx: fix single_step debug Christophe Leroy
@ 2016-08-18 9:58 ` Gabriel Paubert
2016-08-18 10:13 ` Christophe Leroy
0 siblings, 1 reply; 5+ messages in thread
From: Gabriel Paubert @ 2016-08-18 9:58 UTC (permalink / raw)
To: Christophe Leroy
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Scott Wood, linuxppc-dev, linux-kernel
On Thu, Aug 18, 2016 at 11:44:20AM +0200, Christophe Leroy wrote:
> SPRN_ICR must be read for clearing the internal freeze signal which
> is asserted by the single step exception, otherwise the timebase and
> decrementer remain freezed
Minor nit: s/freezed/frozen/
If the timebase and decrementer are frozen even for a few cycles, this
probably upsets timekeeping. I consider this a completely stupid design
decision, and maybe I'm not alone.
Gabriel
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/include/asm/reg_8xx.h | 1 +
> arch/powerpc/kernel/traps.c | 8 ++++++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/reg_8xx.h b/arch/powerpc/include/asm/reg_8xx.h
> index feaf641..6dae71f 100644
> --- a/arch/powerpc/include/asm/reg_8xx.h
> +++ b/arch/powerpc/include/asm/reg_8xx.h
> @@ -17,6 +17,7 @@
> #define SPRN_DC_DAT 570 /* Read-only data register */
>
> /* Misc Debug */
> +#define SPRN_ICR 148
> #define SPRN_DPDR 630
> #define SPRN_MI_CAM 816
> #define SPRN_MI_RAM0 817
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 2cb5892..0f1f0ce 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -400,8 +400,16 @@ static inline int check_io_access(struct pt_regs *regs)
> #define REASON_TRAP 0x20000
>
> #define single_stepping(regs) ((regs)->msr & MSR_SE)
> +#ifdef CONFIG_PPC_8xx
> +static inline void clear_single_step(struct pt_regs *regs)
> +{
> + regs->msr &= ~MSR_SE;
> + mfspr(SPRN_ICR);
> +}
> +#else
> #define clear_single_step(regs) ((regs)->msr &= ~MSR_SE)
> #endif
> +#endif
>
> #if defined(CONFIG_4xx)
> int machine_check_4xx(struct pt_regs *regs)
> --
> 2.1.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] powerpc/8xx: fix single_step debug
2016-08-18 9:58 ` Gabriel Paubert
@ 2016-08-18 10:13 ` Christophe Leroy
2016-08-18 10:16 ` Gabriel Paubert
0 siblings, 1 reply; 5+ messages in thread
From: Christophe Leroy @ 2016-08-18 10:13 UTC (permalink / raw)
To: Gabriel Paubert
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Scott Wood, linuxppc-dev, linux-kernel
Le 18/08/2016 à 11:58, Gabriel Paubert a écrit :
> On Thu, Aug 18, 2016 at 11:44:20AM +0200, Christophe Leroy wrote:
>> SPRN_ICR must be read for clearing the internal freeze signal which
>> is asserted by the single step exception, otherwise the timebase and
>> decrementer remain freezed
>
> Minor nit: s/freezed/frozen/
>
> If the timebase and decrementer are frozen even for a few cycles, this
> probably upsets timekeeping. I consider this a completely stupid design
> decision, and maybe I'm not alone.
>
> Gabriel
We could also unset TBF bit (TimeBase Freeze enable) in TBSCR register
(today it is set in arch/powerpc/platforms/8xx/m8xx_setup.c) but then it
would impact debug done with an external BDM system which expects the
decrementer and TB frozen when it freezes the execution.
Christophe
>
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/include/asm/reg_8xx.h | 1 +
>> arch/powerpc/kernel/traps.c | 8 ++++++++
>> 2 files changed, 9 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/reg_8xx.h b/arch/powerpc/include/asm/reg_8xx.h
>> index feaf641..6dae71f 100644
>> --- a/arch/powerpc/include/asm/reg_8xx.h
>> +++ b/arch/powerpc/include/asm/reg_8xx.h
>> @@ -17,6 +17,7 @@
>> #define SPRN_DC_DAT 570 /* Read-only data register */
>>
>> /* Misc Debug */
>> +#define SPRN_ICR 148
>> #define SPRN_DPDR 630
>> #define SPRN_MI_CAM 816
>> #define SPRN_MI_RAM0 817
>> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
>> index 2cb5892..0f1f0ce 100644
>> --- a/arch/powerpc/kernel/traps.c
>> +++ b/arch/powerpc/kernel/traps.c
>> @@ -400,8 +400,16 @@ static inline int check_io_access(struct pt_regs *regs)
>> #define REASON_TRAP 0x20000
>>
>> #define single_stepping(regs) ((regs)->msr & MSR_SE)
>> +#ifdef CONFIG_PPC_8xx
>> +static inline void clear_single_step(struct pt_regs *regs)
>> +{
>> + regs->msr &= ~MSR_SE;
>> + mfspr(SPRN_ICR);
>> +}
>> +#else
>> #define clear_single_step(regs) ((regs)->msr &= ~MSR_SE)
>> #endif
>> +#endif
>>
>> #if defined(CONFIG_4xx)
>> int machine_check_4xx(struct pt_regs *regs)
>> --
>> 2.1.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] powerpc/8xx: fix single_step debug
2016-08-18 10:13 ` Christophe Leroy
@ 2016-08-18 10:16 ` Gabriel Paubert
2016-08-18 11:38 ` Christophe Leroy
0 siblings, 1 reply; 5+ messages in thread
From: Gabriel Paubert @ 2016-08-18 10:16 UTC (permalink / raw)
To: Christophe Leroy
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Scott Wood, linuxppc-dev, linux-kernel
On Thu, Aug 18, 2016 at 12:13:21PM +0200, Christophe Leroy wrote:
>
>
> Le 18/08/2016 à 11:58, Gabriel Paubert a écrit :
> >On Thu, Aug 18, 2016 at 11:44:20AM +0200, Christophe Leroy wrote:
> >>SPRN_ICR must be read for clearing the internal freeze signal which
> >>is asserted by the single step exception, otherwise the timebase and
> >>decrementer remain freezed
> >
> >Minor nit: s/freezed/frozen/
> >
> >If the timebase and decrementer are frozen even for a few cycles, this
> >probably upsets timekeeping. I consider this a completely stupid design
> >decision, and maybe I'm not alone.
> >
> > Gabriel
>
> We could also unset TBF bit (TimeBase Freeze enable) in TBSCR
> register (today it is set in
> arch/powerpc/platforms/8xx/m8xx_setup.c) but then it would impact
> debug done with an external BDM system which expects the decrementer
> and TB frozen when it freezes the execution.
Ok, I believe that systematically setting it is a mistake, but then I'm
always a bit nervous about screwing up timekeeping (it certainly is
always a very bad idea when you are driving telescopes).
Gabriel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] powerpc/8xx: fix single_step debug
2016-08-18 10:16 ` Gabriel Paubert
@ 2016-08-18 11:38 ` Christophe Leroy
0 siblings, 0 replies; 5+ messages in thread
From: Christophe Leroy @ 2016-08-18 11:38 UTC (permalink / raw)
To: Gabriel Paubert
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Scott Wood, linuxppc-dev, linux-kernel
Le 18/08/2016 à 12:16, Gabriel Paubert a écrit :
> On Thu, Aug 18, 2016 at 12:13:21PM +0200, Christophe Leroy wrote:
>>
>>
>> Le 18/08/2016 à 11:58, Gabriel Paubert a écrit :
>>> On Thu, Aug 18, 2016 at 11:44:20AM +0200, Christophe Leroy wrote:
>>>> SPRN_ICR must be read for clearing the internal freeze signal which
>>>> is asserted by the single step exception, otherwise the timebase and
>>>> decrementer remain freezed
>>>
>>> Minor nit: s/freezed/frozen/
>>>
>>> If the timebase and decrementer are frozen even for a few cycles, this
>>> probably upsets timekeeping. I consider this a completely stupid design
>>> decision, and maybe I'm not alone.
>>>
>>> Gabriel
>>
>> We could also unset TBF bit (TimeBase Freeze enable) in TBSCR
>> register (today it is set in
>> arch/powerpc/platforms/8xx/m8xx_setup.c) but then it would impact
>> debug done with an external BDM system which expects the decrementer
>> and TB frozen when it freezes the execution.
>
> Ok, I believe that systematically setting it is a mistake, but then I'm
> always a bit nervous about screwing up timekeeping (it certainly is
> always a very bad idea when you are driving telescopes).
>
Indeed you are right, this should not happen. The issue is due to the
fact that the bootloader set the TRE bit in the DER register.
So the fix is to be done in the boot loader.
Christophe
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-08-18 11:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-18 9:44 [PATCH] powerpc/8xx: fix single_step debug Christophe Leroy
2016-08-18 9:58 ` Gabriel Paubert
2016-08-18 10:13 ` Christophe Leroy
2016-08-18 10:16 ` Gabriel Paubert
2016-08-18 11:38 ` Christophe Leroy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®