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

[Prabhakar Kushwaha]

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.

>> diff --git a/arch/powerpc/cpu/mpc85xx/start.S b/arch/powerpc/cpu/mpc85xx/start.S
>> index 597151b..cef00ba 100644
>> --- a/arch/powerpc/cpu/mpc85xx/start.S
>> +++ b/arch/powerpc/cpu/mpc85xx/start.S
>> @@ -182,6 +182,66 @@ l2_disabled:
>>   	andi.	r1,r3,L1CSR0_DCE at l
>>   	beq	2b
>>
>> +#if defined(CONFIG_E500)&&  defined(CONFIG_SYS_PPC_E500_DEBUG_TLB)
>> +/*
>> + * TLB for debuggging in AS1
>> + * Create temporary TLB in AS0 to handle debug exception
>> + * As on debug exception MSR is cleared i.e. Address space is changed
>> + * to 0. A TLB (in AS0) is required to handle debug exception generated
>> + * in AS1.
>> + */
> s/TLB/TLB entry/g

Sure, i will update.

>> +
>> +	lis     r6,FSL_BOOKE_MAS0(1,
>> +			CONFIG_SYS_PPC_E500_DEBUG_TLB, 0)@h
>> +	ori     r6,r6,FSL_BOOKE_MAS0(1,
>> +			CONFIG_SYS_PPC_E500_DEBUG_TLB, 0)@l
>> +
>> +#if !defined(CONFIG_SYS_RAMBOOT)
>> +/*
>> + * TLB is created for IVPR + IVOR15 to map on valid OP code address
>> + * bacause flash's virtual address maps to 0xff800000 - 0xffffffff.
>> + * and this window is outside of 4K boot window.
>> + */
>> +	lis     r7,FSL_BOOKE_MAS1(1, 1, 0, 0, BOOKE_PAGESZ_4M)@h
>> +	ori     r7,r7,FSL_BOOKE_MAS1(1, 1, 0, 0, BOOKE_PAGESZ_4M)@l
>> +
>> +	lis     r8,FSL_BOOKE_MAS2(CONFIG_SYS_MONITOR_BASE&  0xffc00000,
>> +							(MAS2_I|MAS2_G))@h
>> +	ori     r8,r8,FSL_BOOKE_MAS2(CONFIG_SYS_MONITOR_BASE&  0xffc00000,
>> +							(MAS2_I|MAS2_G))@l
>> +
>> +	/* The 85xx has the default boot window 0xff800000 - 0xffffffff */
>> +	lis     r9,FSL_BOOKE_MAS3(0xffc00000, 0, (MAS3_SX|MAS3_SW|MAS3_SR))@h
>> +	ori     r9,r9,FSL_BOOKE_MAS3(0xffc00000, 0, (MAS3_SX|MAS3_SW|MAS3_SR))@l
>> +#else
>> +/*
>> + * TLB is created for IVPR + IVOR15 to map on valid OP code address
>> + * because "nexti" will resize TLB to 4K
>> + */
>> +	lis     r7,FSL_BOOKE_MAS1(1, 1, 0, 0, BOOKE_PAGESZ_256K)@h
>> +	ori     r7,r7,FSL_BOOKE_MAS1(1, 1, 0, 0, BOOKE_PAGESZ_256K)@l
>> +
>> +	lis     r8,FSL_BOOKE_MAS2(CONFIG_SYS_MONITOR_BASE, (MAS2_I|MAS2_G))@h
>> +	ori     r8,r8,FSL_BOOKE_MAS2(CONFIG_SYS_MONITOR_BASE,
>> +							(MAS2_I|MAS2_G))@l
>> +	lis     r9,FSL_BOOKE_MAS3(CONFIG_SYS_MONITOR_BASE, 0,
>> +						(MAS3_SX|MAS3_SW|MAS3_SR))@h
>> +	ori     r9,r9,FSL_BOOKE_MAS3(CONFIG_SYS_MONITOR_BASE, 0,
>> +						(MAS3_SX|MAS3_SW|MAS3_SR))@l
>> +#endif
> 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.

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

I will check this point

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

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


>> +	isync
>> +	msync
>> +	tlbwe
>> +#endif
> Need isync after the tlbwe.  I don't think we need the msync before it.

Ok.

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

--Prabhakar