* [U-Boot] [PATCH 2/4] powerpc/85xx:Fix MSR[DE] bit in MSR to support debugger
@ 2012-02-15 8:54 Prabhakar Kushwaha
2012-03-06 14:40 ` Wolfgang Denk
0 siblings, 1 reply; 6+ messages in thread
From: Prabhakar Kushwaha @ 2012-02-15 8:54 UTC (permalink / raw)
To: u-boot
Debugger's ability to debug an application is constrained by the
architecture's debug IP / run-control solution that may impose certain
requirements for the application itself.
Similarly, when referring to the e500 and e500v2 architecture, there are two
basic rules any application has to respect in order to allow full debugging
support:
1. Keep MSR[DE] bit set
2. Have a valid opcode that can be fetched from the debug exception
vector [IVPR|IVOR15].
Here MSR = Machine State register
This patch makes sure of point "1" and make MSR[DE] bit is set uniformaly
across the different execution in address space.
Signed-off-by: Radu Lazarescu <radu.lazarescu@freescale.com>
Signed-off-by: Prabhakar Kushwaha <prabhakar@freescale.com>
---
Applies on http://git.denx.de/u-boot.git branch master
arch/powerpc/cpu/mpc85xx/start.S | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/cpu/mpc85xx/start.S b/arch/powerpc/cpu/mpc85xx/start.S
index 7bfa2d5..09111e6 100644
--- a/arch/powerpc/cpu/mpc85xx/start.S
+++ b/arch/powerpc/cpu/mpc85xx/start.S
@@ -82,6 +82,11 @@
.globl _start_e500
_start_e500:
+#if defined(CONFIG_E500_V1_V2)
+/* Enable debug exception */
+ li r1,MSR_DE
+ mtmsr r1
+#endif
#if defined(CONFIG_SECURE_BOOT) && defined(CONFIG_E500MC)
/* ISBC uses L2 as stack.
@@ -729,8 +734,8 @@ create_init_ram_area:
msync
tlbwe
- lis r6,MSR_IS|MSR_DS at h
- ori r6,r6,MSR_IS|MSR_DS at l
+ lis r6,MSR_IS|MSR_DS|MSR_DE at h
+ ori r6,r6,MSR_IS|MSR_DS|MSR_DE at l
lis r7,switch_as at h
ori r7,r7,switch_as at l
--
1.7.5.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 2/4] powerpc/85xx:Fix MSR[DE] bit in MSR to support debugger
2012-02-15 8:54 [U-Boot] [PATCH 2/4] powerpc/85xx:Fix MSR[DE] bit in MSR to support debugger Prabhakar Kushwaha
@ 2012-03-06 14:40 ` Wolfgang Denk
2012-03-07 4:00 ` Prabhakar Kushwaha
2012-03-07 4:13 ` Prabhakar Kushwaha
0 siblings, 2 replies; 6+ messages in thread
From: Wolfgang Denk @ 2012-03-06 14:40 UTC (permalink / raw)
To: u-boot
Dear Prabhakar Kushwaha,
In message <1329296042-28506-1-git-send-email-prabhakar@freescale.com> you wrote:
> Debugger's ability to debug an application is constrained by the
> architecture's debug IP / run-control solution that may impose certain
> requirements for the application itself.
> Similarly, when referring to the e500 and e500v2 architecture, there are two
> basic rules any application has to respect in order to allow full debugging
> support:
> 1. Keep MSR[DE] bit set
> 2. Have a valid opcode that can be fetched from the debug exception
> vector [IVPR|IVOR15].
> Here MSR = Machine State register
>
> This patch makes sure of point "1" and make MSR[DE] bit is set uniformaly
> across the different execution in address space.
Does it make sense to repeast the whole README here?
> diff --git a/arch/powerpc/cpu/mpc85xx/start.S b/arch/powerpc/cpu/mpc85xx/start.S
> index 7bfa2d5..09111e6 100644
> --- a/arch/powerpc/cpu/mpc85xx/start.S
> +++ b/arch/powerpc/cpu/mpc85xx/start.S
> @@ -82,6 +82,11 @@
> .globl _start_e500
>
> _start_e500:
> +#if defined(CONFIG_E500_V1_V2)
> +/* Enable debug exception */
> + li r1,MSR_DE
> + mtmsr r1
> +#endif
Why is this #ifdef needed here?
> #if defined(CONFIG_SECURE_BOOT) && defined(CONFIG_E500MC)
> /* ISBC uses L2 as stack.
> @@ -729,8 +734,8 @@ create_init_ram_area:
> msync
> tlbwe
>
> - lis r6,MSR_IS|MSR_DS at h
> - ori r6,r6,MSR_IS|MSR_DS at l
> + lis r6,MSR_IS|MSR_DS|MSR_DE at h
> + ori r6,r6,MSR_IS|MSR_DS|MSR_DE at l
And why don;t we need such an #ifdef here?
Also, CONFIG_E500_V1_V2 is undocumented.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"(The Chief Programmer) personally defines the functional and
performance specifications, designs the program, codes it, tests it,
and writes its documentation... He needs great talent, ten years
experience and considerable systems and applications knowledge,
whether in applied mathematics, business data handling, or whatever."
- Fred P. Brooks, _The Mythical Man Month_
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 2/4] powerpc/85xx:Fix MSR[DE] bit in MSR to support debugger
2012-03-06 14:40 ` Wolfgang Denk
@ 2012-03-07 4:00 ` Prabhakar Kushwaha
2012-03-07 12:03 ` Wolfgang Denk
2012-03-07 4:13 ` Prabhakar Kushwaha
1 sibling, 1 reply; 6+ messages in thread
From: Prabhakar Kushwaha @ 2012-03-07 4:00 UTC (permalink / raw)
To: u-boot
Hi Wolfgang,
On Tuesday 06 March 2012 08:10 PM, Wolfgang Denk wrote:
> Dear Prabhakar Kushwaha,
>
> In message<1329296042-28506-1-git-send-email-prabhakar@freescale.com> you wrote:
>> Debugger's ability to debug an application is constrained by the
>> architecture's debug IP / run-control solution that may impose certain
>> requirements for the application itself.
>> Similarly, when referring to the e500 and e500v2 architecture, there are two
>> basic rules any application has to respect in order to allow full debugging
>> support:
>> 1. Keep MSR[DE] bit set
>> 2. Have a valid opcode that can be fetched from the debug exception
>> vector [IVPR|IVOR15].
>> Here MSR = Machine State register
>>
>> This patch makes sure of point "1" and make MSR[DE] bit is set uniformaly
>> across the different execution in address space.
> Does it make sense to repeast the whole README here?
I agree. it should not be.
But as i sent a series of patch, this change require some explanation.
I will change the description.
>> diff --git a/arch/powerpc/cpu/mpc85xx/start.S b/arch/powerpc/cpu/mpc85xx/start.S
>> index 7bfa2d5..09111e6 100644
>> --- a/arch/powerpc/cpu/mpc85xx/start.S
>> +++ b/arch/powerpc/cpu/mpc85xx/start.S
>> @@ -82,6 +82,11 @@
>> .globl _start_e500
>>
>> _start_e500:
>> +#if defined(CONFIG_E500_V1_V2)
>> +/* Enable debug exception */
>> + li r1,MSR_DE
>> + mtmsr r1
>> +#endif
> Why is this #ifdef needed here?
This #ifdef is to make sure MSR_DE bit is set before u-boot's actual
code start executing.
it is for overcoming one of the requirement of debugging i.e. Keep
MSR_DE bit set.
>> #if defined(CONFIG_SECURE_BOOT)&& defined(CONFIG_E500MC)
>> /* ISBC uses L2 as stack.
>> @@ -729,8 +734,8 @@ create_init_ram_area:
>> msync
>> tlbwe
>>
>> - lis r6,MSR_IS|MSR_DS at h
>> - ori r6,r6,MSR_IS|MSR_DS at l
>> + lis r6,MSR_IS|MSR_DS|MSR_DE at h
>> + ori r6,r6,MSR_IS|MSR_DS|MSR_DE at l
> And why don;t we need such an #ifdef here?
I found similar piece of code in start.S without any #define at label
_start_cont. That's why i made it like this.
if required i will put it under #define.
> Also, CONFIG_E500_V1_V2 is undocumented.
Definition of this #define is part of documentation. I will add comment
before using it.
--Prabhakar
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 2/4] powerpc/85xx:Fix MSR[DE] bit in MSR to support debugger
2012-03-06 14:40 ` Wolfgang Denk
2012-03-07 4:00 ` Prabhakar Kushwaha
@ 2012-03-07 4:13 ` Prabhakar Kushwaha
1 sibling, 0 replies; 6+ messages in thread
From: Prabhakar Kushwaha @ 2012-03-07 4:13 UTC (permalink / raw)
To: u-boot
Hi Wolfgang,
On Tuesday 06 March 2012 08:10 PM, Wolfgang Denk wrote:
> Dear Prabhakar Kushwaha,
>
> In message<1329296042-28506-1-git-send-email-prabhakar@freescale.com> you wrote:
>> Debugger's ability to debug an application is constrained by the
>> architecture's debug IP / run-control solution that may impose certain
>> requirements for the application itself.
>> Similarly, when referring to the e500 and e500v2 architecture, there are two
>> basic rules any application has to respect in order to allow full debugging
>> support:
>> 1. Keep MSR[DE] bit set
>> 2. Have a valid opcode that can be fetched from the debug exception
>> vector [IVPR|IVOR15].
>> Here MSR = Machine State register
>>
>> This patch makes sure of point "1" and make MSR[DE] bit is set uniformaly
>> across the different execution in address space.
> Does it make sense to repeast the whole README here?
I agree. it should not be.
But as i sent a series of patch, this change require some explanation.
I will change the description.
>> diff --git a/arch/powerpc/cpu/mpc85xx/start.S b/arch/powerpc/cpu/mpc85xx/start.S
>> index 7bfa2d5..09111e6 100644
>> --- a/arch/powerpc/cpu/mpc85xx/start.S
>> +++ b/arch/powerpc/cpu/mpc85xx/start.S
>> @@ -82,6 +82,11 @@
>> .globl _start_e500
>>
>> _start_e500:
>> +#if defined(CONFIG_E500_V1_V2)
>> +/* Enable debug exception */
>> + li r1,MSR_DE
>> + mtmsr r1
>> +#endif
> Why is this #ifdef needed here?
This #ifdef is to make sure MSR_DE bit is set before u-boot's actual
code start executing.
it is for overcoming one of the requirement of debugging i.e. Keep
MSR_DE bit set.
>> #if defined(CONFIG_SECURE_BOOT)&& defined(CONFIG_E500MC)
>> /* ISBC uses L2 as stack.
>> @@ -729,8 +734,8 @@ create_init_ram_area:
>> msync
>> tlbwe
>>
>> - lis r6,MSR_IS|MSR_DS at h
>> - ori r6,r6,MSR_IS|MSR_DS at l
>> + lis r6,MSR_IS|MSR_DS|MSR_DE at h
>> + ori r6,r6,MSR_IS|MSR_DS|MSR_DE at l
> And why don;t we need such an #ifdef here?
I found similar piece of code in start.S without any #define at label
_start_cont. That's why i made it like this.
if required i will put it under #define.
> Also, CONFIG_E500_V1_V2 is undocumented.
Definition of this #define is part of documentation. I will add comment
before using it.
--Prabhakar
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 2/4] powerpc/85xx:Fix MSR[DE] bit in MSR to support debugger
2012-03-07 4:00 ` Prabhakar Kushwaha
@ 2012-03-07 12:03 ` Wolfgang Denk
2012-03-13 5:24 ` Prabhakar Kushwaha
0 siblings, 1 reply; 6+ messages in thread
From: Wolfgang Denk @ 2012-03-07 12:03 UTC (permalink / raw)
To: u-boot
Dear Prabhakar Kushwaha,
In message <4F56DD59.1080304@freescale.com> you wrote:
>
> >> --- a/arch/powerpc/cpu/mpc85xx/start.S
> >> +++ b/arch/powerpc/cpu/mpc85xx/start.S
> >> @@ -82,6 +82,11 @@
> >> .globl _start_e500
> >>
> >> _start_e500:
> >> +#if defined(CONFIG_E500_V1_V2)
> >> +/* Enable debug exception */
> >> + li r1,MSR_DE
> >> + mtmsr r1
> >> +#endif
> > Why is this #ifdef needed here?
>
> This #ifdef is to make sure MSR_DE bit is set before u-boot's actual
> code start executing.
> it is for overcoming one of the requirement of debugging i.e. Keep
> MSR_DE bit set.
Sorry, this makes no sense to me. The #ifdef does not change the
order of execution, it just enables or disables the code. Are there
any situations where omitting this code is required, or where it makes
sense?
> >> @@ -729,8 +734,8 @@ create_init_ram_area:
> >> msync
> >> tlbwe
> >>
> >> - lis r6,MSR_IS|MSR_DS at h
> >> - ori r6,r6,MSR_IS|MSR_DS at l
> >> + lis r6,MSR_IS|MSR_DS|MSR_DE at h
> >> + ori r6,r6,MSR_IS|MSR_DS|MSR_DE at l
> > And why don;t we need such an #ifdef here?
>
> I found similar piece of code in start.S without any #define at label
> _start_cont. That's why i made it like this.
> if required i will put it under #define.
Please handle in a consistent way. See above.
> > Also, CONFIG_E500_V1_V2 is undocumented.
>
> Definition of this #define is part of documentation. I will add comment
> before using it.
CONFIG_ options must be explained in the README.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I like your game but we have to change the rules."
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH 2/4] powerpc/85xx:Fix MSR[DE] bit in MSR to support debugger
2012-03-07 12:03 ` Wolfgang Denk
@ 2012-03-13 5:24 ` Prabhakar Kushwaha
0 siblings, 0 replies; 6+ messages in thread
From: Prabhakar Kushwaha @ 2012-03-13 5:24 UTC (permalink / raw)
To: u-boot
Hi Wolfgang,
Sorry for delayed response.
On Wednesday 07 March 2012 05:33 PM, Wolfgang Denk wrote:
> Dear Prabhakar Kushwaha,
>
> In message<4F56DD59.1080304@freescale.com> you wrote:
>>>> --- a/arch/powerpc/cpu/mpc85xx/start.S
>>>> +++ b/arch/powerpc/cpu/mpc85xx/start.S
>>>> @@ -82,6 +82,11 @@
>>>> .globl _start_e500
>>>>
>>>> _start_e500:
>>>> +#if defined(CONFIG_E500_V1_V2)
>>>> +/* Enable debug exception */
>>>> + li r1,MSR_DE
>>>> + mtmsr r1
>>>> +#endif
>>> Why is this #ifdef needed here?
>> This #ifdef is to make sure MSR_DE bit is set before u-boot's actual
>> code start executing.
>> it is for overcoming one of the requirement of debugging i.e. Keep
>> MSR_DE bit set.
> Sorry, this makes no sense to me. The #ifdef does not change the
> order of execution, it just enables or disables the code. Are there
> any situations where omitting this code is required, or where it makes
> sense?
OK.
after having internal discussion about different processor support. we
can make this code permanently enabled.
>>>> @@ -729,8 +734,8 @@ create_init_ram_area:
>>>> msync
>>>> tlbwe
>>>>
>>>> - lis r6,MSR_IS|MSR_DS at h
>>>> - ori r6,r6,MSR_IS|MSR_DS at l
>>>> + lis r6,MSR_IS|MSR_DS|MSR_DE at h
>>>> + ori r6,r6,MSR_IS|MSR_DS|MSR_DE at l
>>> And why don;t we need such an #ifdef here?
>> I found similar piece of code in start.S without any #define at label
>> _start_cont. That's why i made it like this.
>> if required i will put it under #define.
> Please handle in a consistent way. See above.
Sure,
I believe we should not have any #define for this piece of code to make
it consistent across the file.
>>> Also, CONFIG_E500_V1_V2 is undocumented.
>> Definition of this #define is part of documentation. I will add comment
>> before using it.
> CONFIG_ options must be explained in the README.
I will take care of this
--Prabhakar
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-13 5:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-15 8:54 [U-Boot] [PATCH 2/4] powerpc/85xx:Fix MSR[DE] bit in MSR to support debugger Prabhakar Kushwaha
2012-03-06 14:40 ` Wolfgang Denk
2012-03-07 4:00 ` Prabhakar Kushwaha
2012-03-07 12:03 ` Wolfgang Denk
2012-03-13 5:24 ` Prabhakar Kushwaha
2012-03-07 4:13 ` Prabhakar Kushwaha
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox