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

Prabhakar Kushwaha
2012-03-24T02:24:22Z

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

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