[U-Boot] [PATCH 3/4][v2] powerpc/85xx:Make debug exception vector accessible

[Prabhakar Kushwaha]

On Friday 23 March 2012 11:44 PM, Scott Wood wrote:
> On 03/23/2012 06:44 AM, Prabhakar Kushwaha wrote:
>> Hi Scott,
>>
>> On Friday 23 March 2012 01:13 AM, Scott Wood wrote:
>>> On 03/22/2012 12:52 AM, Prabhakar Kushwaha wrote:
>>>> Hi Scott,
>>>>
>>>>    Please find my reply in-lined
>>>>
>>>> On Thursday 22 March 2012 01:22 AM, Scott Wood wrote:
>>>>> On 03/20/2012 11:43 PM, Prabhakar Kushwaha wrote:
>>>>>> Debugging of e500 and e500v1 processer requires debug exception
>>>>>> vecter (IVPR +
>>>>>> IVOR15) to have valid and fetchable OP code.
>>>>>>
>>>>>> While executing in translated space (AS=1), whenever a debug
>>>>>> exception is
>>>>>> generated, the MSR[DS/IS] gets cleared i.e. AS=0 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 TLB
>>>>>> configuration to
>>>>>> have (IVOR + IVOR15) accessible from AS=0 also.
>>>>>>
>>>>>> Create a temporary TLB in AS0 to make sure debug exception verctor is
>>>>>> accessible on debug exception.
>>>>>>
>>>>>> Signed-off-by: Radu Lazarescu<radu.lazarescu at freescale.com>
>>>>>> Signed-off-by: Marius Grigoras<marius.grigoras at freescale.com>
>>>>>> Signed-off-by: Prabhakar Kushwaha<prabhakar at freescale.com>
>>>>> Can you document the flow of exactly what TLB entries are present at
>>>>> various points of the boot flow, for all the various configurations
>>>>> (NOR
>>>>> boot, NAND SPL, RAMBOOT, IFC versus non-IFC, etc).
>>>> Sure. May be separate patch will be send.
>>> Let's start with just an e-mail thoroughly describing your understanding
>>> of this.  It will provide necessary context for review.
>>>
>>> We can clean it up for permanent documentation once it's clear to
>>> everyone what's going on.
>> Sure. I will start this activity from now.
>> But i will suggest not to combine both patches. let debugger patch to go
>> ahead , if required i will send required patch later-on.
> My point is that I cannot fully follow what's going on here without
> spending a bunch of time looking at it, and I don't see anyone else
> stepping up to review this, so I'd like to see a write-up of what's
> going on so that I can properly review these patches.

Means i have to make the doc first :(
OK. Its my pleasure.

>>>>> In the ramboot case is this really supposed to be I+G?
>>>> I am not sure.  But same is done under label "create_init_ram_area" for
>>>> TLB entry 15.
>>>> what you suggest.
>>> I suggest as part of the request to document all of this, you figure out
>>> what should actually be mapped in each configuration.  The existing code
>>> might be wrong for some of them, but we shouldn't proceed ahead blindly
>>> and make an even bigger mess.
>>>
>> After internal discussion we can have following settings
>> for non-RAMBOOT(NOR boot) ==>  I + G
>> for RAMBOOT ==>  I, cache inhibited is required as L1 cache is used as
>> stack.
> Why does using L1 for a stack mean that the mapping must be cache
> inhibited?  Why do we even need to use L1 for a stack with ramboot?

it is done at label "_start_cont". L1 is set for Stack address as 
"switch_as" label.
am i wrong??


>>   I=0 it means the memory range is cacheable, so an access to an address
>> from that range could get the line in cache. If you are using the cache
>> as a memory zone(L1 as stack), it may overwrite some data in cache and
>> replace it with the last access.
> It will not do that -- when we use L1 (or part of it) for an early
> stack, we lock the cache lines.

Agree..

>>>>> Which path will NAND SPL go through (not the payload, but the SPL
>>>>> itself)?  That will be only a 4K window mapped, and guarded doesn't
>>>>> stop
>>>>> speculative instruction fetches, so we don't want to map more than is
>>>>> backed up by something.
>>>> NAND SPL go via !defined(CONFIG_SYS_RAMBOOT) path.
>>>>
>>>> i think  NAND_SPL  does not require temporary TLB  as NAND SPL even does
>>>> not have any interrupt vector.
>>> So there's no plan to support using breakpoints or single step during
>>> the SPL?  That's fine with me, but should be documented, and we should
>>> make sure this code does not run in that case.
>>>
>> Breakpoints will work as it is. No impact will be on debugging for NAND
>> SPL.
>>
>> Generally any debugger use some initial configuration file. Only problem
>> occurs
>> when  application modifies those configuration.
> Then why do we need to set MSR[DE] on entry, if the debugger can take
> care of it?
Yes. You are right. It is required only for SD/SPI boot not for 
NAND_SPL/NOR boot.
But to make MSR[DE] bit set general way (out of #define) throughout 
u-boot code. I did.

>>>>>> +    lis     r10,0xffc00000 at h
>>>>>> +    ori     r10,r10,0xffc00000 at l
>>>>> Don't waste instructions -- this could be in an SPL.  That ori is a
>>>>> no-op.
>>>> Please refer above response. May be this piece of code is not required
>>>> for NAND SPL
>>> Still, I'd like to know why you're writing 0xffc00000 to MAS7.  Only the
>>> low 4 bits of MAS7 are valid on current e500.
>>
>> The reason for using 0xffc00000 to support e6500 - since it supports
>> 40-bit physical addresses, the last 8 bits of MAS7 are defined.
> Regardless, you're setting the wrong end of MAS7.  It's the *lower*
> bits, not the upper bits, that are used.

oh..  got your point.

> And we should not be doing anything special for e6500 here.  e6500 does
> not need this, and e6500 platforms should not set
> CONFIG_SYS_PPC_E500_DEBUG_TLB.
Got it :)


>> But i am not sure whether e6500 will be part of mpc85xx or not.
> It will.

Thanks for confirmation.

>> So, I will use as
>> #ifdef CONFIG_ENABLE_36BIT_PHYS
>>      lis     r10,0x0000
>> #endif
for 36 bit physical address,
address should be 0x0_1107_f000 or 0x0_ffff_fffc for destination

  mas7 should be programmed. To make sure mas7 is 0.  I am setting it.
if not required i can remove.


--Prabhakar