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

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

[Prabhakar Kushwaha]

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>
---
 Based upon git://git.denx.de/u-boot.git branch master

 Changes for v2: 
	- Put Temporary TLB creation under #define

  Tested on
  - SoC having E500 Family processor (P1010RDB, BSC9131RDB)
  - SoC having E500MC Family processor (P4080DS, P3041DS)

 arch/powerpc/cpu/mpc85xx/cpu_init_early.c |   32 +++++++++++++++-
 arch/powerpc/cpu/mpc85xx/start.S          |   60 +++++++++++++++++++++++++++++
 arch/powerpc/include/asm/config_mpc85xx.h |    3 +-
 3 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init_early.c b/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
index 091af7c..d0b15a4 100644
--- a/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
+++ b/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
@@ -1,5 +1,5 @@
 /*
- * Copyright 2009-2011 Freescale Semiconductor, Inc
+ * Copyright 2009-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
@@ -53,6 +53,36 @@ void setup_ifc(void)
 
 	asm volatile("isync;msync;tlbwe;isync");
 
+#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.
+ *
+ * TLB is created for IVPR + IVOR15 to map on valid OP code address
+ * bacause flash's physical address is going to change as
+ * CONFIG_SYS_FLASH_BASE_PHYS.
+ */
+	_mas0 = MAS0_TLBSEL(1) |
+			MAS0_ESEL(CONFIG_SYS_PPC_E500_DEBUG_TLB);
+	_mas1 = MAS1_VALID | MAS1_TID(0) | MAS1_IPROT |
+			MAS1_TSIZE(BOOKE_PAGESZ_4M);
+	_mas2 = FSL_BOOKE_MAS2(CONFIG_SYS_TEXT_BASE, MAS2_I|MAS2_G);
+	_mas3 = FSL_BOOKE_MAS3(flash_phys, 0, MAS3_SW|MAS3_SR|MAS3_SX);
+	_mas7 = FSL_BOOKE_MAS7(flash_phys);
+
+	mtspr(MAS0, _mas0);
+	mtspr(MAS1, _mas1);
+	mtspr(MAS2, _mas2);
+	mtspr(MAS3, _mas3);
+	mtspr(MAS7, _mas7);
+
+	asm volatile("isync;msync;tlbwe;isync");
+#endif
+
+	/* Change flash's physical address */
 	out_be32(&(ifc_regs->cspr_cs[0].cspr), CONFIG_SYS_CSPR0);
 	out_be32(&(ifc_regs->csor_cs[0].csor), CONFIG_SYS_CSOR0);
 	out_be32(&(ifc_regs->amask_cs[0].amask), CONFIG_SYS_AMASK0);
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.
+ */
+
+	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
+	lis     r10,0xffc00000 at h
+	ori     r10,r10,0xffc00000 at l
+
+	mtspr   MAS0,r6
+	mtspr   MAS1,r7
+	mtspr   MAS2,r8
+	mtspr   MAS3,r9
+	mtspr   MAS7,r10
+	isync
+	msync
+	tlbwe
+#endif
+
 /*
  * Ne need to setup interrupt vector for NAND SPL
  * because NAND SPL never compiles it.
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
-- 
1.7.5.4

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

[Scott Wood]

On 03/20/2012 11:43 PM, Prabhakar Kushwaha wrote:
> diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init_early.c b/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
> index 091af7c..d0b15a4 100644
> --- a/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
> +++ b/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright 2009-2011 Freescale Semiconductor, Inc
> + * Copyright 2009-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
> @@ -53,6 +53,36 @@ void setup_ifc(void)
>  
>  	asm volatile("isync;msync;tlbwe;isync");
>  
> +#if defined(CONFIG_E500) && defined(CONFIG_SYS_PPC_E500_DEBUG_TLB)

There is no need to check for CONFIG_E500 anywhere in
arch/powerpc/cpu/mpc85xx.  mpc85xx implies CONFIG_E500.

I don't see anywhere in this patchset where you set
CONFIG_SYS_PPC_E500_DEBUG_TLB on any actual board.  It should be set for
all e500v1/v2, not just so that debug works but so that this code
remains tested.

-Scott

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

[Prabhakar Kushwaha]

Hi Scott,

On Wednesday 21 March 2012 10:04 PM, Scott Wood wrote:
> On 03/20/2012 11:43 PM, Prabhakar Kushwaha wrote:
>> diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init_early.c b/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
>> index 091af7c..d0b15a4 100644
>> --- a/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
>> +++ b/arch/powerpc/cpu/mpc85xx/cpu_init_early.c
>> @@ -1,5 +1,5 @@
>>   /*
>> - * Copyright 2009-2011 Freescale Semiconductor, Inc
>> + * Copyright 2009-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
>> @@ -53,6 +53,36 @@ void setup_ifc(void)
>>
>>   	asm volatile("isync;msync;tlbwe;isync");
>>
>> +#if defined(CONFIG_E500)&&  defined(CONFIG_SYS_PPC_E500_DEBUG_TLB)
> There is no need to check for CONFIG_E500 anywhere in
> arch/powerpc/cpu/mpc85xx.  mpc85xx implies CONFIG_E500.

Ok. I will re-spin all the patches without CONFIG_E500 condition check.

> I don't see anywhere in this patchset where you set
> CONFIG_SYS_PPC_E500_DEBUG_TLB on any actual board.

This CONFIG_SYS_PPC_E500_DEBUG_TLB is defined in  
arch/powerpc/include/asm/config_mpc85xx.h.
i defined for P1010 (in this patch). For rest of e500 v1, v2 processor 
family new patch-set will be sent once this base patch is accepted.

>   It should be set for
> all e500v1/v2, not just so that debug works but so that this code
> remains tested.

Yes. I agree with you.
It will be automatically enabled when  CONFIG_SYS_PPC_E500_DEBUG_TLB is 
defined per SoC(e500 v1/v2). As of now only for P1010 it is enabled.

--Prabhakar

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

[Scott Wood]

On 03/21/2012 12:04 PM, Prabhakar Kushwaha wrote:
> Hi Scott,
> 
> On Wednesday 21 March 2012 10:04 PM, Scott Wood wrote:
>> I don't see anywhere in this patchset where you set
>> CONFIG_SYS_PPC_E500_DEBUG_TLB on any actual board.
> 
> This CONFIG_SYS_PPC_E500_DEBUG_TLB is defined in 
> arch/powerpc/include/asm/config_mpc85xx.h.
> i defined for P1010 (in this patch). For rest of e500 v1, v2 processor
> family new patch-set will be sent once this base patch is accepted.

Ah sorry, missed that. :-P

-Scott

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

[Scott Wood]

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

> 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

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

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.

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

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

> +	isync
> +	msync
> +	tlbwe
> +#endif

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

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

-Scott

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

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

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

[Scott Wood]

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.

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

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

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

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

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

The decision to actually make use of the TLB entry would be a different
symbol, if you don't want it to be unconditional (relative to the
surrounding code).

-Scott

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

[Timur Tabi]

Scott Wood wrote:
>> > 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?

On e500v1 parts, CONFIG_SYS_CCSRBAR_PHYS_HIGH will always be 0, so it will
only write 0 to MAS7.  Is that still considered a bad thing?  The tlbwe
will ignore MAS7.

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Scott Wood]

On 03/22/2012 02:51 PM, Timur Tabi wrote:
> Scott Wood wrote:
>>>> 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?
> 
> On e500v1 parts, CONFIG_SYS_CCSRBAR_PHYS_HIGH will always be 0, so it will
> only write 0 to MAS7.  Is that still considered a bad thing?  The tlbwe
> will ignore MAS7.

Either it's needed, or we should get rid of CONFIG_ENABLE_36BIT_PHYS
entirely.  Either way, we should test the results on e500v1 hardware.

-Scott

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

[Timur Tabi]

Scott Wood wrote:
> Either it's needed, or we should get rid of CONFIG_ENABLE_36BIT_PHYS
> entirely.  Either way, we should test the results on e500v1 hardware.

That macro conditionally enables support for MAS7:

#if defined(CONFIG_ENABLE_36BIT_PHYS)
	ori	r0,r0,HID0_ENMAS7 at l	/* Enable MAS7 */
#endif

So I don't think we can get rid of it, otherwise I suspect an e500v1
operating system will not work on an e500v2 part.

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Scott Wood]

On 03/22/2012 02:56 PM, Timur Tabi wrote:
> Scott Wood wrote:
>> Either it's needed, or we should get rid of CONFIG_ENABLE_36BIT_PHYS
>> entirely.  Either way, we should test the results on e500v1 hardware.
> 
> That macro conditionally enables support for MAS7:
> 
> #if defined(CONFIG_ENABLE_36BIT_PHYS)
> 	ori	r0,r0,HID0_ENMAS7 at l	/* Enable MAS7 */
> #endif
> 
> So I don't think we can get rid of it, otherwise I suspect an e500v1
> operating system will not work on an e500v2 part.

Fine, keep it for that (though I'd argue that if that were the concern,
we should have been clearing it before we enter the OS, and the Os can
set it if it wants).  But what about in write_tlb, or tlb.c?

There's obviously an existing attempt to avoid touching MAS7 if
CONFIG_ENABLE_36BIT_PHYS is not set.  Either that's unnecessary and we
should remove it, or we should continue adhering to it.

-Scott

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


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

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

[Scott Wood]

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

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

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

[Scott Wood]

On 03/23/2012 09:24 PM, Prabhakar Kushwaha wrote:
> On Friday 23 March 2012 11:44 PM, Scott Wood wrote:
>> On 03/23/2012 06:44 AM, Prabhakar Kushwaha wrote:
>>> 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 don't doubt that we do use the L1 for stack unconditionally -- just
that in the ramboot case we could probably get away without it if it
were a problem.  I don't think it's a problem, though, because it
doesn't conflict with creating cacheable mappings.

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

I'm not saying it should be conditional -- just trying to understand
when it's needed at all, and what we can expect the debugger to have
already done.

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

CONFIG_ENABLE_36BIT_PHYS should be protecting the MAS7 mtspr, not the lis.

-Scott