[U-Boot] [PATCH 2/4] powerpc/85xx:Fix MSR[DE] bit in MSR to support debugger
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@h - ori r6,r6,MSR_IS|MSR_DS@l + lis r6,MSR_IS|MSR_DS|MSR_DE@h + ori r6,r6,MSR_IS|MSR_DS|MSR_DE@l lis r7,switch_as@h ori r7,r7,switch_as@l
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@h
- ori r6,r6,MSR_IS|MSR_DS@l
- lis r6,MSR_IS|MSR_DS|MSR_DE@h
- ori r6,r6,MSR_IS|MSR_DS|MSR_DE@l
And why don;t we need such an #ifdef here?
Also, CONFIG_E500_V1_V2 is undocumented.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Tuesday 06 March 2012 08:10 PM, Wolfgang Denk wrote:
Dear Prabhakar Kushwaha,
In message1329296042-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@h
- ori r6,r6,MSR_IS|MSR_DS@l
- lis r6,MSR_IS|MSR_DS|MSR_DE@h
- ori r6,r6,MSR_IS|MSR_DS|MSR_DE@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
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@h
- ori r6,r6,MSR_IS|MSR_DS@l
- lis r6,MSR_IS|MSR_DS|MSR_DE@h
- ori r6,r6,MSR_IS|MSR_DS|MSR_DE@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
Hi Wolfgang,
Sorry for delayed response.
On Wednesday 07 March 2012 05:33 PM, Wolfgang Denk wrote:
Dear Prabhakar Kushwaha,
In message4F56DD59.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@h
- ori r6,r6,MSR_IS|MSR_DS@l
- lis r6,MSR_IS|MSR_DS|MSR_DE@h
- ori r6,r6,MSR_IS|MSR_DS|MSR_DE@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
Hi Wolfgang,
On Tuesday 06 March 2012 08:10 PM, Wolfgang Denk wrote:
Dear Prabhakar Kushwaha,
In message1329296042-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@h
- ori r6,r6,MSR_IS|MSR_DS@l
- lis r6,MSR_IS|MSR_DS|MSR_DE@h
- ori r6,r6,MSR_IS|MSR_DS|MSR_DE@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
participants (2)
-
Prabhakar Kushwaha -
Wolfgang Denk