Patchwork [U-Boot,1/4] doc: Add documentation for mpc85xx debugger support

login
register
mail settings
Submitter Prabhakar Kushwaha
Date Feb. 15, 2012, 8:53 a.m.
Message ID <1329296027-28471-1-git-send-email-prabhakar@freescale.com>
Download mbox | patch
Permalink /patch/141277/
State Superseded
Delegated to: Andy Fleming
Headers show

Comments

Prabhakar Kushwaha - Feb. 15, 2012, 8:53 a.m.
This describes requirement of e500 and e500v2 processor to support any
debugger. it also provide an insight of switch used and defined.

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

 doc/README.mpc85xx_debugger |   44 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)
 create mode 100644 doc/README.mpc85xx_debugger
Wolfgang Denk - March 6, 2012, 2:39 p.m.
Dear Prabhakar Kushwaha,

In message <1329296027-28471-1-git-send-email-prabhakar@freescale.com> you wrote:
> This describes requirement of e500 and e500v2 processor to support any
> debugger. it also provide an insight of switch used and defined.
> 
> 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
> 
>  doc/README.mpc85xx_debugger |   44 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 44 insertions(+), 0 deletions(-)
>  create mode 100644 doc/README.mpc85xx_debugger

This document contains a number of typos and othe rerrors, mixed with
inconsitent indentation (some lines use TABs, others use spaces).

I suggest you clean it up and have it reviewed by a native speaker
before resubmitting.

Regarding the split into 4 separate patches: this makes no sense to
me.  For example, in this patch you reference new (and undocumented
config options like CONFIG_E500_V1_V2) without ever using them.

Please squash patches.

Best regards,

Wolfgang Denk
Prabhakar Kushwaha - March 7, 2012, 3:32 a.m.
Hi Wolfgang,
  Thanks for reviewing this patch.
  Please find my response in-lined

On Tuesday 06 March 2012 08:09 PM, Wolfgang Denk wrote:
> Dear Prabhakar Kushwaha,
>
> In message<1329296027-28471-1-git-send-email-prabhakar@freescale.com>  you wrote:
>> This describes requirement of e500 and e500v2 processor to support any
>> debugger. it also provide an insight of switch used and defined.
>>
>> 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
>>
>>   doc/README.mpc85xx_debugger |   44 +++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 44 insertions(+), 0 deletions(-)
>>   create mode 100644 doc/README.mpc85xx_debugger
> This document contains a number of typos and othe rerrors, mixed with
> inconsitent indentation (some lines use TABs, others use spaces).
>
> I suggest you clean it up and have it reviewed by a native speaker
> before resubmitting.

I will take care of your review-comments and will have first review at 
native level before floating it again.

> Regarding the split into 4 separate patches: this makes no sense to
> me.  For example, in this patch you reference new (and undocumented
> config options like CONFIG_E500_V1_V2) without ever using them.
>
> Please squash patches.
>
My objective was to have separate patch for every logical piece. That's 
why I sent a series of patch.
I will request you to let them as series for better understanding of others.

May be in each patch before use of any #define (related to debugger), I 
will write a comment.
Please suggest.

Regarding CONFIG_E500_V1_V2, Its description is also part of this patch 
or is it not cleared ?

--Prabhakar
Wolfgang Denk - March 7, 2012, 6:24 a.m.
Dear Kushwaha Prabhakar-B32579,

In message <071A08F2C6A57E4E94D980ECA553F874575244@039-SN1MPN1-005.039d.mgd.msft.net> you wrote:
>
> Regarding CONFIG_E500_V1_V2, Its description is also part of this
> patch or is it not cleared ?

First, documentation of CONFIG_ options belongs into the central
README, so we have it all in a single place.

Second, "Enables code taking care of above mentioned rule" is not
really helpful to understand what it's actually doing.

The name of the cvariable suggests that this defiens a E500 core
based system, but it does not even contain a slight hint that it has
something to do with debugging.

Also, what's the "V1_V2" ? Are there also other systems (say, e500 v3
cores), and are this not affected? We already have CONFIG_E500 and
CONFIG_E500MC so CONFIG_E500_V1_V2 appears to belong to this group,
but if I understand your intentions it does something completely
unrelated.

All this is highly confusing.

Best regards,

Wolfgang Denk
Prabhakar Kushwaha - March 7, 2012, 8:50 a.m.
Hi Wolfgang,

On Wednesday 07 March 2012 11:54 AM, Wolfgang Denk wrote:
>
> In message<071A08F2C6A57E4E94D980ECA553F874575244@039-SN1MPN1-005.039d.mgd.msft.net>  you wrote:
>> Regarding CONFIG_E500_V1_V2, Its description is also part of this
>> patch or is it not cleared ?
> First, documentation of CONFIG_ options belongs into the central
> README, so we have it all in a single place.

I will take care in next version.

> Second, "Enables code taking care of above mentioned rule" is not
> really helpful to understand what it's actually doing.

I will add more description

> The name of the cvariable suggests that this defiens a E500 core
> based system, but it does not even contain a slight hint that it has
> something to do with debugging.

Yes i agree. From #define no one can get hint of debugging.  It was 
intended.

This #define is created to overcome restriction of e500 v1 and v2 family 
processor. We can have this #define permanently enabled. That's why i 
did not create any CONFIG_ having DBG name.

Unfortunately this is a restriction for debugging.


> Also, what's the "V1_V2" ? Are there also other systems (say, e500 v3
> cores), and are this not affected? We already have CONFIG_E500 and
> CONFIG_E500MC so CONFIG_E500_V1_V2 appears to belong to this group,
> but if I understand your intentions it does something completely
> unrelated.
V1_V2 is used because it applied to e500v1 and e500v2 not e500mc 
processor. So CONFIG_E500MC cant be used. Also I cant use CONFIG_E500 as 
it refer the entire e500 family which includes e500mc.

Thinking over lot of confusion over #define i should use 
CONFIG_E500_V1_V2_DBG.
Please guide me in having correct #define.

Regards,
Prabhakar
Wolfgang Denk - March 7, 2012, 12:30 p.m.
Dear Prabhakar Kushwaha,

In message <4F572159.9020303@freescale.com> you wrote:
> 
> > Also, what's the "V1_V2" ? Are there also other systems (say, e500 v3
> > cores), and are this not affected? We already have CONFIG_E500 and
> > CONFIG_E500MC so CONFIG_E500_V1_V2 appears to belong to this group,
> > but if I understand your intentions it does something completely
> > unrelated.
> V1_V2 is used because it applied to e500v1 and e500v2 not e500mc 
> processor. So CONFIG_E500MC cant be used. Also I cant use CONFIG_E500 as 
> it refer the entire e500 family which includes e500mc.

Hm... I am not sure if CONFIG_E500 was supposed to include
CONFIG_E500MC; it's nowhere documented.  Let's assume it is.

What happens if you enable this code on a E500MC system?


Best regards,

Wolfgang Denk
Prabhakar Kushwaha - March 13, 2012, 7:14 a.m.
Hi Wolfgang,

On Wednesday 07 March 2012 06:00 PM, Wolfgang Denk wrote:
> Dear Prabhakar Kushwaha,
>
> In message<4F572159.9020303@freescale.com>  you wrote:
>>> Also, what's the "V1_V2" ? Are there also other systems (say, e500 v3
>>> cores), and are this not affected? We already have CONFIG_E500 and
>>> CONFIG_E500MC so CONFIG_E500_V1_V2 appears to belong to this group,
>>> but if I understand your intentions it does something completely
>>> unrelated.
>> V1_V2 is used because it applied to e500v1 and e500v2 not e500mc
>> processor. So CONFIG_E500MC cant be used. Also I cant use CONFIG_E500 as
>> it refer the entire e500 family which includes e500mc.
> Hm... I am not sure if CONFIG_E500 was supposed to include
> CONFIG_E500MC; it's nowhere documented.  Let's assume it is.
>
> What happens if you enable this code on a E500MC system?
>

Debug  restrictions are not valid for e500mc system.

At first sight it should not hurt e500mc execution (other than some 
seemingly unnecessary steps). However i will check this point.

Regards,
Prabhakar
Prabhakar Kushwaha - March 14, 2012, 9:35 a.m.
Hi Wolfgang,

On Tuesday 13 March 2012 12:44 PM, Prabhakar Kushwaha wrote:
> Hi Wolfgang,
>
> On Wednesday 07 March 2012 06:00 PM, Wolfgang Denk wrote:
>> Dear Prabhakar Kushwaha,
>>
>> In message<4F572159.9020303@freescale.com>  you wrote:
>>>> Also, what's the "V1_V2" ? Are there also other systems (say, e500 v3
>>>> cores), and are this not affected? We already have CONFIG_E500 and
>>>> CONFIG_E500MC so CONFIG_E500_V1_V2 appears to belong to this group,
>>>> but if I understand your intentions it does something completely
>>>> unrelated.
>>> V1_V2 is used because it applied to e500v1 and e500v2 not e500mc
>>> processor. So CONFIG_E500MC cant be used. Also I cant use 
>>> CONFIG_E500 as
>>> it refer the entire e500 family which includes e500mc.
>> Hm... I am not sure if CONFIG_E500 was supposed to include
>> CONFIG_E500MC; it's nowhere documented.  Let's assume it is.
>>
>> What happens if you enable this code on a E500MC system?
>>
>
> Debug  restrictions are not valid for e500mc system.
>
> At first sight it should not hurt e500mc execution (other than some 
> seemingly unnecessary steps). However i will check this point.
>
We tried by enabling CONFIG_E500_V2_V2 for E500MC with u-boot patches. 
It boots fine and debugging can be done.

So, we can use CONFIG_E500 #define instead of CONFIG_E500_V2_V2 i.e. 
debugging will always be enabled. One have to define 
CONFIG_DEBUGGER_TEMP_TLB  for debugging in AS1 ( Part of patch 
"powerpc/85xx:Update NOR code base to support debugger" )

CONFIG_DEBUGGER_TEMP_TLB can also be used for placing code which can 
only be required during debugging (specially code of temporary TLB 
creation)

Please suggest.

Regards,
Prabhakar
Scott Wood - March 14, 2012, 7:30 p.m.
On 03/14/2012 04:35 AM, Prabhakar Kushwaha wrote:
> Hi Wolfgang,
> 
> On Tuesday 13 March 2012 12:44 PM, Prabhakar Kushwaha wrote:
>> Hi Wolfgang,
>>
>> On Wednesday 07 March 2012 06:00 PM, Wolfgang Denk wrote:
>>> Dear Prabhakar Kushwaha,
>>>
>>> In message<4F572159.9020303@freescale.com>  you wrote:
>>>>> Also, what's the "V1_V2" ? Are there also other systems (say, e500 v3
>>>>> cores), and are this not affected? We already have CONFIG_E500 and
>>>>> CONFIG_E500MC so CONFIG_E500_V1_V2 appears to belong to this group,
>>>>> but if I understand your intentions it does something completely
>>>>> unrelated.
>>>> V1_V2 is used because it applied to e500v1 and e500v2 not e500mc
>>>> processor. So CONFIG_E500MC cant be used. Also I cant use
>>>> CONFIG_E500 as
>>>> it refer the entire e500 family which includes e500mc.
>>> Hm... I am not sure if CONFIG_E500 was supposed to include
>>> CONFIG_E500MC; it's nowhere documented.  Let's assume it is.
>>>
>>> What happens if you enable this code on a E500MC system?
>>>
>>
>> Debug  restrictions are not valid for e500mc system.
>>
>> At first sight it should not hurt e500mc execution (other than some
>> seemingly unnecessary steps). However i will check this point.
>>
> We tried by enabling CONFIG_E500_V2_V2 for E500MC with u-boot patches.
> It boots fine and debugging can be done.

Be sure to mention in comments that the hack is only really needed for
v1/v2.

> So, we can use CONFIG_E500 #define instead of CONFIG_E500_V2_V2 i.e.
> debugging will always be enabled. One have to define
> CONFIG_DEBUGGER_TEMP_TLB  for debugging in AS1 ( Part of patch
> "powerpc/85xx:Update NOR code base to support debugger" )

CONFIG_SYS_PPC_E500_DEBUG_TLB

> CONFIG_DEBUGGER_TEMP_TLB can also be used for placing code which can
> only be required during debugging (specially code of temporary TLB
> creation)

Is there something specific you had in mind, other than the use that is
already present in this patchset?

-Scott
Prabhakar Kushwaha - March 15, 2012, 3:51 a.m.
Hi Scott,

On Thursday 15 March 2012 01:00 AM, Scott Wood wrote:
> On 03/14/2012 04:35 AM, Prabhakar Kushwaha wrote:
>> Hi Wolfgang,
>>
>> On Tuesday 13 March 2012 12:44 PM, Prabhakar Kushwaha wrote:
>>> Hi Wolfgang,
>>>
>>> On Wednesday 07 March 2012 06:00 PM, Wolfgang Denk wrote:
>>>> Dear Prabhakar Kushwaha,
>>>>
>>>> In message<4F572159.9020303@freescale.com>   you wrote:
>>>>>> Also, what's the "V1_V2" ? Are there also other systems (say, e500 v3
>>>>>> cores), and are this not affected? We already have CONFIG_E500 and
>>>>>> CONFIG_E500MC so CONFIG_E500_V1_V2 appears to belong to this group,
>>>>>> but if I understand your intentions it does something completely
>>>>>> unrelated.
>>>>> V1_V2 is used because it applied to e500v1 and e500v2 not e500mc
>>>>> processor. So CONFIG_E500MC cant be used. Also I cant use
>>>>> CONFIG_E500 as
>>>>> it refer the entire e500 family which includes e500mc.
>>>> Hm... I am not sure if CONFIG_E500 was supposed to include
>>>> CONFIG_E500MC; it's nowhere documented.  Let's assume it is.
>>>>
>>>> What happens if you enable this code on a E500MC system?
>>>>
>>> Debug  restrictions are not valid for e500mc system.
>>>
>>> At first sight it should not hurt e500mc execution (other than some
>>> seemingly unnecessary steps). However i will check this point.
>>>
>> We tried by enabling CONFIG_E500_V2_V2 for E500MC with u-boot patches.
>> It boots fine and debugging can be done.
> Be sure to mention in comments that the hack is only really needed for
> v1/v2.

I will clearly mention in doc

>> So, we can use CONFIG_E500 #define instead of CONFIG_E500_V2_V2 i.e.
>> debugging will always be enabled. One have to define
>> CONFIG_DEBUGGER_TEMP_TLB  for debugging in AS1 ( Part of patch
>> "powerpc/85xx:Update NOR code base to support debugger" )
> CONFIG_SYS_PPC_E500_DEBUG_TLB

OK

>> CONFIG_DEBUGGER_TEMP_TLB can also be used for placing code which can
>> only be required during debugging (specially code of temporary TLB
>> creation)
> Is there something specific you had in mind, other than the use that is
> already present in this patchset?

There is no specific use case in my mind other than the patch-set.

Actually, Wolfgang is having concern about code size increase because of 
"temporary TLB creation" in start.S for debugging.  That's why i am 
planning to use this #define for "temporary TLB creation"

--Prabhakar

Patch

diff --git a/doc/README.mpc85xx_debugger b/doc/README.mpc85xx_debugger
new file mode 100644
index 0000000..b35d710
--- /dev/null
+++ b/doc/README.mpc85xx_debugger
@@ -0,0 +1,44 @@ 
+Overview
+--------
+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].
+Where:
+	MSR  : Machine State register
+	IVPR : Interrupt Vector Prefix Register
+	IVOR : Interrupt Vector Offset Register
+
+Depending upon above 2 points there various place in powerpc/mpc85xx/ code
+which break the rules:
+	- MSR[DE] is not se
+        - Changing the context with a rfi instruction, but omitting to preserve
+	the [DE] bit in SRR1
+	- Changing IVPR/IVOR15 to a new location, before that location is
+	  un-accessible
+        - Changing IVPR/IVOR15 to a location that does not have a valid opcode
+	at [IVOR|IVOR15]
+	- While executing in translated space (AS=1), whenever a debug
+	  exception is generated, the MSR[DS/IS] gets cleared and the processor
+	  tries to fetch an instruction from the debug exception vector
+	  (IVPR|IVOR15); since now we are in AS=0, the application needs to
+	  ensure the proper configuration to have IVOR|IVOR15 accessible from
+	  AS=0 also
+
+Config Switches:
+----------------
+CONFIG_E500_V1_V2		Enables code taking care of above mentioned rule.
+CONFIG_DEBUGGER_TEMP_TLB	Define temporary TLB number.
+				It will be used to create temporary TLB for AS0
+				during execution in AS1. The TLB entry will be
+				created for debug exception vector.
+				Becuase on debug exception MSR[DS/IS] get
+				cleared i.e.  execution space is shifted back
+				to AS0 and a TLB is required to have debug
+				exception vecor accessible.