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

[Scott Wood <scottwood@freescale.com>]

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@freescale.com>
>>>>> Signed-off-by: Marius Grigoras<marius.grigoras@freescale.com>
>>>>> Signed-off-by: Prabhakar Kushwaha<prabhakar@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.

>>>> 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?

>  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.

>>>> 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?

>>>>> +    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.

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.

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

It will.

> So, I will use as
> #ifdef CONFIG_ENABLE_36BIT_PHYS
>     lis     r10,0x0000
> #endif

Why?

-Scott