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

[Prabhakar Kushwaha]

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.


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

  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.


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

>>>> +    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.
But i am not sure whether e6500 will be part of mpc85xx or not.

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


>>>> +    mtspr   MAS0,r6
>>>> +    mtspr   MAS1,r7
>>>> +    mtspr   MAS2,r8
>>>> +    mtspr   MAS3,r9
>>>> +    mtspr   MAS7,r10
>>> Why are you writing 0xffc00000 into MAS7?
>>>
>>> Access to MAS7 needs to be conditional on CONFIG_ENABLE_36BIT_PHYS
>>> (misnamed, should be something like CONFIG_SYS_PPC_HAS_MAS7).
>> i will put this code under #define CONFIG_ENABLE_36BIT_PHYS
>>
>> For your kind information : in start.S, label label
>> "create_ccsr_new_tlb", "create_ccsr_old_tlb" uses  MAS7  without
>> CONFIG_ENABLE_36BIT_PHYS  #define.
>> It should be fixed ??
> Yes, it should be fixed.  That was a fairly recent change and perhaps
> e500v1 has not been tested since then -- Timur, could you look at this?

i will use CONFIG_ENABLE_36BIT_PHYS defines.

>>>> diff --git a/arch/powerpc/include/asm/config_mpc85xx.h
>>>> b/arch/powerpc/include/asm/config_mpc85xx.h
>>>> index 8654625..268c56e 100644
>>>> --- a/arch/powerpc/include/asm/config_mpc85xx.h
>>>> +++ b/arch/powerpc/include/asm/config_mpc85xx.h
>>>> @@ -1,5 +1,5 @@
>>>>    /*
>>>> - * Copyright 2011 Freescale Semiconductor, Inc.
>>>> + * Copyright 2011-2012 Freescale Semiconductor, Inc.
>>>>     *
>>>>     * This program is free software; you can redistribute it and/or
>>>>     * modify it under the terms of the GNU General Public License as
>>>> @@ -107,6 +107,7 @@
>>>>    #define CONFIG_MAX_CPUS            1
>>>>    #define CONFIG_FSL_SDHC_V2_3
>>>>    #define CONFIG_SYS_FSL_NUM_LAWS        12
>>>> +#define CONFIG_SYS_PPC_E500_DEBUG_TLB    3
>>>>    #define CONFIG_TSECV2
>>>>    #define CONFIG_SYS_FSL_SEC_COMPAT    4
>>>>    #define CONFIG_FSL_SATA_V2
>>> It would be nice if we could have one place where all the fixed TLB
>>> entries are assigned.
>>>
>> Am i supposed to send separate patch having TLB entry defined for all
>> e500 v1/v2 SoC ?
> That's a different issue, I just mean having one central place where
> each hardcoded TLB entry gets a symbolic name, as well as a #define for
> where programmatic TLB entry allocation can begin.

Yes. I agree with you. May be i can take this activity also.

--Prabhakar