| Message ID | 1329296027-28471-1-git-send-email-prabhakar@freescale.com |
|---|---|
| State | Superseded |
| Delegated to: | Andy Fleming |
| Headers | show |
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
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
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
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
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
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
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
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
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
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.