From mboxrd@z Thu Jan  1 00:00:00 1970
From: Prabhakar Kushwaha <prabhakar@freescale.com>
Date: Tue, 13 Mar 2012 10:54:51 +0530
Subject: [U-Boot] [PATCH 2/4] powerpc/85xx:Fix MSR[DE] bit in MSR to
 support debugger
In-Reply-To: <20120307120336.A9C93202C7F@gemini.denx.de>
References: <1329296042-28506-1-git-send-email-prabhakar@freescale.com>
	<20120306144050.6CDFF202D7D@gemini.denx.de>
	<4F56DD59.1080304@freescale.com>
	<20120307120336.A9C93202C7F@gemini.denx.de>
Message-ID: <4F5EDA23.6000109@freescale.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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