[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Kumar Gala]

From: Timur Tabi <timur@freescale.com>

After determining how much DDR is actually in the system, set DBAT0 and
IBAT0 accordingly.  This ensures that the CPU won't attempt to access
(via speculation) addresses outside of actual memory.

On 86xx systems, DBAT0 and IBAT0 (the BATs for DDR) are initialized to 2GB
and kept that way.  If the system has less than 2GB of memory (typical for
an MPC8610 HPCD), the CPU may attempt to access this memory during
speculation.  The zlib code is notorious for generating such memory reads,
and indeed on the MPC8610, uncompressing the Linux kernel causes a machine
check (without this patch).

Signed-off-by: Timur Tabi <timur@freescale.com>
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
* Fixed TO_BATU_BL to handle non-power of two
* Fixed write_bat to use computed 'bl' not always 2G size
* removed comment that wasn't need anymore

 board/freescale/mpc8610hpcd/mpc8610hpcd.c |    2 +
 board/freescale/mpc8641hpcn/mpc8641hpcn.c |    2 +
 cpu/mpc86xx/cpu.c                         |   30 +++++++++++++++++++++++++++++
 cpu/mpc86xx/cpu_init.c                    |    4 +++
 include/asm-ppc/mmu.h                     |    6 ++++-
 include/configs/MPC8610HPCD.h             |    6 +---
 include/configs/MPC8641HPCN.h             |    4 +--
 include/mpc86xx.h                         |    2 +
 8 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/board/freescale/mpc8610hpcd/mpc8610hpcd.c b/board/freescale/mpc8610hpcd/mpc8610hpcd.c
index 784a2ed..ab5f800 100644
--- a/board/freescale/mpc8610hpcd/mpc8610hpcd.c
+++ b/board/freescale/mpc8610hpcd/mpc8610hpcd.c
@@ -127,6 +127,8 @@ initdram(int board_type)
 	dram_size = fixed_sdram();
 #endif
 
+	setup_ddr_bat(dram_size);
+
 	puts(" DDR: ");
 	return dram_size;
 }
diff --git a/board/freescale/mpc8641hpcn/mpc8641hpcn.c b/board/freescale/mpc8641hpcn/mpc8641hpcn.c
index c521527..443c9fd 100644
--- a/board/freescale/mpc8641hpcn/mpc8641hpcn.c
+++ b/board/freescale/mpc8641hpcn/mpc8641hpcn.c
@@ -74,6 +74,8 @@ initdram(int board_type)
 	dram_size = fixed_sdram();
 #endif
 
+	setup_ddr_bat(dram_size);
+
 	puts("    DDR: ");
 	return dram_size;
 }
diff --git a/cpu/mpc86xx/cpu.c b/cpu/mpc86xx/cpu.c
index f7e012d..ac171f5 100644
--- a/cpu/mpc86xx/cpu.c
+++ b/cpu/mpc86xx/cpu.c
@@ -197,3 +197,33 @@ void mpc86xx_reginfo(void)
 	printf("\tBR7\t0x%08X\tOR7\t0x%08X \n", in_be32(&lbc->br7), in_be32(&lbc->or7));
 
 }
+
+/*
+ * Set the DDR BATs to reflect the actual size of DDR.
+ *
+ * dram_size is the actual size of DDR, in bytes
+ *
+ * Note: we assume that CONFIG_MAX_MEM_MAPPED is 2G or smaller as we only
+ * are using a single BAT to cover DDR.  
+ *
+ * If this is not true, (e.g. CONFIG_MAX_MEM_MAPPED is 2GB but HID0_XBSEN
+ * is not defined) then we might have a situation where U-Boot will attempt
+ * to relocated itself outside of the region mapped by DBAT0.
+ * This will cause a machine check.
+ *
+ */
+void setup_ddr_bat(phys_addr_t dram_size)
+{
+	unsigned long batu, bl;
+
+	bl = TO_BATU_BL(min(dram_size, CONFIG_MAX_MEM_MAPPED));
+
+	if (BATU_SIZE(bl) != dram_size) {
+		u64 sz = (u64)dram_size - BATU_SIZE(bl);
+		print_size(sz, " left unmapped\n");
+	}
+
+	batu = bl | BATU_VS | BATU_VP;
+	write_bat(DBAT0, batu, CONFIG_SYS_DBAT0L);
+	write_bat(IBAT0, batu, CONFIG_SYS_IBAT0L);
+}
diff --git a/cpu/mpc86xx/cpu_init.c b/cpu/mpc86xx/cpu_init.c
index 5a78a9c..b4f047d 100644
--- a/cpu/mpc86xx/cpu_init.c
+++ b/cpu/mpc86xx/cpu_init.c
@@ -138,8 +138,12 @@ int cpu_init_r(void)
 /* Set up BAT registers */
 void setup_bats(void)
 {
+#if defined(CONFIG_SYS_DBAT0U) && defined(CONFIG_SYS_DBAT0L)
 	write_bat(DBAT0, CONFIG_SYS_DBAT0U, CONFIG_SYS_DBAT0L);
+#endif
+#if defined(CONFIG_SYS_IBAT0U) && defined(CONFIG_SYS_IBAT0L)
 	write_bat(IBAT0, CONFIG_SYS_IBAT0U, CONFIG_SYS_IBAT0L);
+#endif
 	write_bat(DBAT1, CONFIG_SYS_DBAT1U, CONFIG_SYS_DBAT1L);
 	write_bat(IBAT1, CONFIG_SYS_IBAT1U, CONFIG_SYS_IBAT1L);
 	write_bat(DBAT2, CONFIG_SYS_DBAT2U, CONFIG_SYS_DBAT2L);
diff --git a/include/asm-ppc/mmu.h b/include/asm-ppc/mmu.h
index fd10249..ce7f081 100644
--- a/include/asm-ppc/mmu.h
+++ b/include/asm-ppc/mmu.h
@@ -213,7 +213,11 @@ extern void print_bats(void);
 #define BATL_PADDR(x) ((phys_addr_t)((x & 0xfffe0000)		\
 				     | ((x & 0x0e00ULL) << 24)	\
 				     | ((x & 0x04ULL) << 30)))
-#define BATU_SIZE(x) (1UL << (fls((x & BATU_BL_MAX) >> 2) + 17))
+#define BATU_SIZE(x) (1ULL << (fls((x & BATU_BL_MAX) >> 2) + 17))
+
+/* bytes into BATU_BL */
+#define TO_BATU_BL(x) \
+	(u32)((((1ull << __ilog2_u64((u64)x)) / (128 * 1024)) - 1) * 4)
 
 /* Used to set up SDR1 register */
 #define HASH_TABLE_SIZE_64K	0x00010000
diff --git a/include/configs/MPC8610HPCD.h b/include/configs/MPC8610HPCD.h
index 1d2d659..fed441e 100644
--- a/include/configs/MPC8610HPCD.h
+++ b/include/configs/MPC8610HPCD.h
@@ -341,10 +341,8 @@
  * BAT0		2G	Cacheable, non-guarded
  * 0x0000_0000	2G	DDR
  */
-#define CONFIG_SYS_DBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE)
-#define CONFIG_SYS_DBAT0U	(BATU_BL_2G | BATU_VS | BATU_VP)
-#define CONFIG_SYS_IBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE )
-#define CONFIG_SYS_IBAT0U	CONFIG_SYS_DBAT0U
+#define CONFIG_SYS_DBAT0L	(BATL_PP_RW)
+#define CONFIG_SYS_IBAT0L	(BATL_PP_RW)
 
 /*
  * BAT1		1G	Cache-inhibited, guarded
diff --git a/include/configs/MPC8641HPCN.h b/include/configs/MPC8641HPCN.h
index 12a8f60..94e4d24 100644
--- a/include/configs/MPC8641HPCN.h
+++ b/include/configs/MPC8641HPCN.h
@@ -482,9 +482,7 @@ extern unsigned long get_board_sys_clk(unsigned long dummy);
  * BAT0		DDR
  */
 #define CONFIG_SYS_DBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE)
-#define CONFIG_SYS_DBAT0U	(BATU_BL_2G | BATU_VS | BATU_VP)
-#define CONFIG_SYS_IBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE )
-#define CONFIG_SYS_IBAT0U	CONFIG_SYS_DBAT0U
+#define CONFIG_SYS_IBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE)
 
 /*
  * BAT1		LBC (PIXIS/CF)
diff --git a/include/mpc86xx.h b/include/mpc86xx.h
index c6f30f9..eb85d60 100644
--- a/include/mpc86xx.h
+++ b/include/mpc86xx.h
@@ -83,5 +83,7 @@ static __inline__ unsigned long get_l2cr (void)
    return l2cr_val;
 }
 
+void setup_ddr_bat(phys_addr_t dram_size);
+
 #endif  /* _ASMLANGUAGE */
 #endif	/* __MPC86xx_H__ */
-- 
1.6.0.6

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Timur Tabi]

Kumar Gala wrote:
> +void setup_ddr_bat(phys_addr_t dram_size)
> +{
> +	unsigned long batu, bl;
> +
> +	bl = TO_BATU_BL(min(dram_size, CONFIG_MAX_MEM_MAPPED));
> +
> +	if (BATU_SIZE(bl) != dram_size) {
> +		u64 sz = (u64)dram_size - BATU_SIZE(bl);
> +		print_size(sz, " left unmapped\n");
> +	}

We still have the problem that, on a 1.5GB system, U-Boot will think that there are 1.5GB of DDR, but the BAT will be set to 1GB.  When U-Boot tries to relocate itself, it will machine check.

We need a way to tell U-Boot that we only have 1GB of DDR, but still have it tell Linux that we have 1.5GB of DDR.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Kumar Gala]

On Mar 30, 2010, at 9:37 AM, Timur Tabi wrote:

> Kumar Gala wrote:
>> +void setup_ddr_bat(phys_addr_t dram_size)
>> +{
>> +	unsigned long batu, bl;
>> +
>> +	bl = TO_BATU_BL(min(dram_size, CONFIG_MAX_MEM_MAPPED));
>> +
>> +	if (BATU_SIZE(bl) != dram_size) {
>> +		u64 sz = (u64)dram_size - BATU_SIZE(bl);
>> +		print_size(sz, " left unmapped\n");
>> +	}
> 
> We still have the problem that, on a 1.5GB system, U-Boot will think that there are 1.5GB of DDR, but the BAT will be set to 1GB.  When U-Boot tries to relocate itself, it will machine check.
> 
> We need a way to tell U-Boot that we only have 1GB of DDR, but still have it tell Linux that we have 1.5GB of DDR.

is this situation you have today? (or just a concern?)

- k

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Timur Tabi]

Kumar Gala wrote:

>> We still have the problem that, on a 1.5GB system, U-Boot will think that there are 1.5GB of DDR, but the BAT will be set to 1GB.  When U-Boot tries to relocate itself, it will machine check.
>>
>> We need a way to tell U-Boot that we only have 1GB of DDR, but still have it tell Linux that we have 1.5GB of DDR.
> 
> is this situation you have today? (or just a concern?)

Just a concern.

Technically, we have the same problem on e500, except on e500 we use multiple TLBs to map the memory.  In theory, we need at most one TLB/BAT per DIMM slot, which means we would need four BATS on the MPC8641 HPCN.

Do we want to tell customers that U-Boot/Linux only supports power-of-two sizes of DDR?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Kumar Gala]

On Mar 30, 2010, at 10:21 AM, Timur Tabi wrote:

> Kumar Gala wrote:
> 
>>> We still have the problem that, on a 1.5GB system, U-Boot will think that there are 1.5GB of DDR, but the BAT will be set to 1GB.  When U-Boot tries to relocate itself, it will machine check.
>>> 
>>> We need a way to tell U-Boot that we only have 1GB of DDR, but still have it tell Linux that we have 1.5GB of DDR.
>> 
>> is this situation you have today? (or just a concern?)
> 
> Just a concern.
> 
> Technically, we have the same problem on e500, except on e500 we use multiple TLBs to map the memory.  In theory, we need at most one TLB/BAT per DIMM slot, which means we would need four BATS on the MPC8641 HPCN.
> 
> Do we want to tell customers that U-Boot/Linux only supports power-of-two sizes of DDR?

We can make this work by updating how get_effective_memsize() works in lib_ppc/board.c.  We just need to make it aware of the amount of mapped memory.

The simplest thing might be to just add in a gd->mem_map_size to deal with the issue on both 86xx/44x/85xx

- k

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Timur Tabi]

Kumar Gala wrote:
> We can make this work by updating how get_effective_memsize() works in lib_ppc/board.c.  We just need to make it aware of the amount of mapped memory.

Is this something you want handled in this patch?

> The simplest thing might be to just add in a gd->mem_map_size to deal with the issue on both 86xx/44x/85xx

I don't know about 44x, but like I said, I don't think it's a problem in 85xx, because we use up to 8 TLBs to map DDR, which is more than enough to cover all memory size possibilities.


-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Kumar Gala]

On Mar 30, 2010, at 10:32 AM, Timur Tabi wrote:

> Kumar Gala wrote:
>> We can make this work by updating how get_effective_memsize() works in lib_ppc/board.c.  We just need to make it aware of the amount of mapped memory.
> 
> Is this something you want handled in this patch?

Not really.  I was trying to get this patch in a state that we could get it into v2010.03.  That change is a bit more intrusive.

So if 8610 works for you I'll add the patch into my tree for Wolfgang to pull.

- k

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Timur Tabi]

Kumar Gala wrote:

> Not really.  I was trying to get this patch in a state that we could get it into v2010.03.  That change is a bit more intrusive.

Isn't it already too late for 2010.03?

> So if 8610 works for you I'll add the patch into my tree for Wolfgang to pull.

I'll test it and let you know.  But please add a comment to the changelog and/or source code that it will limit DDR for U-Boot and Linux to the next lowest power of two.  

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Becky Bruce]

Did I miss something here? Why did you change the coherence attribute  
in the BAT?  Please add an explanation to the comment, assuming you  
have a good reason for this (I'm not awake enough to think about this  
right now :)

Otherwise, a couple of minor nits below.

On Mar 30, 2010, at 9:25 AM, Kumar Gala wrote:

> From: Timur Tabi <timur@freescale.com>
>
> After determining how much DDR is actually in the system, set DBAT0  
> and
> IBAT0 accordingly.  This ensures that the CPU won't attempt to access
> (via speculation) addresses outside of actual memory.
>
> On 86xx systems, DBAT0 and IBAT0 (the BATs for DDR) are initialized  
> to 2GB
> and kept that way.  If the system has less than 2GB of memory  
> (typical for
> an MPC8610 HPCD), the CPU may attempt to access this memory during
> speculation.  The zlib code is notorious for generating such memory  
> reads,
> and indeed on the MPC8610, uncompressing the Linux kernel causes a  
> machine
> check (without this patch).
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
> * Fixed TO_BATU_BL to handle non-power of two
> * Fixed write_bat to use computed 'bl' not always 2G size
> * removed comment that wasn't need anymore
>
> board/freescale/mpc8610hpcd/mpc8610hpcd.c |    2 +
> board/freescale/mpc8641hpcn/mpc8641hpcn.c |    2 +
> cpu/mpc86xx/cpu.c                         |   30 ++++++++++++++++++++ 
> +++++++++
> cpu/mpc86xx/cpu_init.c                    |    4 +++
> include/asm-ppc/mmu.h                     |    6 ++++-
> include/configs/MPC8610HPCD.h             |    6 +---
> include/configs/MPC8641HPCN.h             |    4 +--
> include/mpc86xx.h                         |    2 +
> 8 files changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/board/freescale/mpc8610hpcd/mpc8610hpcd.c b/board/ 
> freescale/mpc8610hpcd/mpc8610hpcd.c
> index 784a2ed..ab5f800 100644
> --- a/board/freescale/mpc8610hpcd/mpc8610hpcd.c
> +++ b/board/freescale/mpc8610hpcd/mpc8610hpcd.c
> @@ -127,6 +127,8 @@ initdram(int board_type)
> 	dram_size = fixed_sdram();
> #endif
>
> +	setup_ddr_bat(dram_size);
> +
> 	puts(" DDR: ");
> 	return dram_size;
> }
> diff --git a/board/freescale/mpc8641hpcn/mpc8641hpcn.c b/board/ 
> freescale/mpc8641hpcn/mpc8641hpcn.c
> index c521527..443c9fd 100644
> --- a/board/freescale/mpc8641hpcn/mpc8641hpcn.c
> +++ b/board/freescale/mpc8641hpcn/mpc8641hpcn.c
> @@ -74,6 +74,8 @@ initdram(int board_type)
> 	dram_size = fixed_sdram();
> #endif
>
> +	setup_ddr_bat(dram_size);
> +
> 	puts("    DDR: ");
> 	return dram_size;
> }
> diff --git a/cpu/mpc86xx/cpu.c b/cpu/mpc86xx/cpu.c
> index f7e012d..ac171f5 100644
> --- a/cpu/mpc86xx/cpu.c
> +++ b/cpu/mpc86xx/cpu.c
> @@ -197,3 +197,33 @@ void mpc86xx_reginfo(void)
> 	printf("\tBR7\t0x%08X\tOR7\t0x%08X \n", in_be32(&lbc->br7),  
> in_be32(&lbc->or7));
>
> }
> +
> +/*
> + * Set the DDR BATs to reflect the actual size of DDR.
> + *
> + * dram_size is the actual size of DDR, in bytes
> + *
> + * Note: we assume that CONFIG_MAX_MEM_MAPPED is 2G or smaller as  
> we only
> + * are using a single BAT to cover DDR.
> + *
> + * If this is not true, (e.g. CONFIG_MAX_MEM_MAPPED is 2GB but  
> HID0_XBSEN
> + * is not defined) then we might have a situation where U-Boot will  
> attempt
> + * to relocated itself outside of the region mapped by DBAT0.
> + * This will cause a machine check.

At the very least, fix this comment to mention the problem with not  
being able to map all the RAM as well, if you're going to leave it  
that way.  Can you test a board with a strange amount of RAM (1.5GB,  
or something), and see what happens with this patch?  I really don't  
like leaving things this way.

> + *
> + */
> +void setup_ddr_bat(phys_addr_t dram_size)
> +{
> +	unsigned long batu, bl;
> +
> +	bl = TO_BATU_BL(min(dram_size, CONFIG_MAX_MEM_MAPPED));
> +
> +	if (BATU_SIZE(bl) != dram_size) {
> +		u64 sz = (u64)dram_size - BATU_SIZE(bl);
> +		print_size(sz, " left unmapped\n");
> +	}
> +
> +	batu = bl | BATU_VS | BATU_VP;
> +	write_bat(DBAT0, batu, CONFIG_SYS_DBAT0L);
> +	write_bat(IBAT0, batu, CONFIG_SYS_IBAT0L);
> +}
> diff --git a/cpu/mpc86xx/cpu_init.c b/cpu/mpc86xx/cpu_init.c
> index 5a78a9c..b4f047d 100644
> --- a/cpu/mpc86xx/cpu_init.c
> +++ b/cpu/mpc86xx/cpu_init.c
> @@ -138,8 +138,12 @@ int cpu_init_r(void)
> /* Set up BAT registers */
> void setup_bats(void)
> {
> +#if defined(CONFIG_SYS_DBAT0U) && defined(CONFIG_SYS_DBAT0L)
> 	write_bat(DBAT0, CONFIG_SYS_DBAT0U, CONFIG_SYS_DBAT0L);
> +#endif
> +#if defined(CONFIG_SYS_IBAT0U) && defined(CONFIG_SYS_IBAT0L)
> 	write_bat(IBAT0, CONFIG_SYS_IBAT0U, CONFIG_SYS_IBAT0L);
> +#endif
> 	write_bat(DBAT1, CONFIG_SYS_DBAT1U, CONFIG_SYS_DBAT1L);
> 	write_bat(IBAT1, CONFIG_SYS_IBAT1U, CONFIG_SYS_IBAT1L);
> 	write_bat(DBAT2, CONFIG_SYS_DBAT2U, CONFIG_SYS_DBAT2L);
> diff --git a/include/asm-ppc/mmu.h b/include/asm-ppc/mmu.h
> index fd10249..ce7f081 100644
> --- a/include/asm-ppc/mmu.h
> +++ b/include/asm-ppc/mmu.h
> @@ -213,7 +213,11 @@ extern void print_bats(void);
> #define BATL_PADDR(x) ((phys_addr_t)((x & 0xfffe0000)		\
> 				     | ((x & 0x0e00ULL) << 24)	\
> 				     | ((x & 0x04ULL) << 30)))
> -#define BATU_SIZE(x) (1UL << (fls((x & BATU_BL_MAX) >> 2) + 17))
> +#define BATU_SIZE(x) (1ULL << (fls((x & BATU_BL_MAX) >> 2) + 17))
> +
> +/* bytes into BATU_BL */
> +#define TO_BATU_BL(x) \
> +	(u32)((((1ull << __ilog2_u64((u64)x)) / (128 * 1024)) - 1) * 4)

It's a nit, but can we change the *4 to << 2 ?  I know most modern  
compilers should optimize this, but I think it makes the code easier  
to read and is logically more sensical, and if you've got to change  
the patch, anyway, we might as well clean this up.

-B


>
> /* Used to set up SDR1 register */
> #define HASH_TABLE_SIZE_64K	0x00010000
> diff --git a/include/configs/MPC8610HPCD.h b/include/configs/ 
> MPC8610HPCD.h
> index 1d2d659..fed441e 100644
> --- a/include/configs/MPC8610HPCD.h
> +++ b/include/configs/MPC8610HPCD.h
> @@ -341,10 +341,8 @@
>  * BAT0		2G	Cacheable, non-guarded
>  * 0x0000_0000	2G	DDR
>  */
> -#define CONFIG_SYS_DBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE)
> -#define CONFIG_SYS_DBAT0U	(BATU_BL_2G | BATU_VS | BATU_VP)
> -#define CONFIG_SYS_IBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE )
> -#define CONFIG_SYS_IBAT0U	CONFIG_SYS_DBAT0U
> +#define CONFIG_SYS_DBAT0L	(BATL_PP_RW)
> +#define CONFIG_SYS_IBAT0L	(BATL_PP_RW)
>
> /*
>  * BAT1		1G	Cache-inhibited, guarded
> diff --git a/include/configs/MPC8641HPCN.h b/include/configs/ 
> MPC8641HPCN.h
> index 12a8f60..94e4d24 100644
> --- a/include/configs/MPC8641HPCN.h
> +++ b/include/configs/MPC8641HPCN.h
> @@ -482,9 +482,7 @@ extern unsigned long get_board_sys_clk(unsigned  
> long dummy);
>  * BAT0		DDR
>  */
> #define CONFIG_SYS_DBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE)
> -#define CONFIG_SYS_DBAT0U	(BATU_BL_2G | BATU_VS | BATU_VP)
> -#define CONFIG_SYS_IBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE )
> -#define CONFIG_SYS_IBAT0U	CONFIG_SYS_DBAT0U
> +#define CONFIG_SYS_IBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE)
>
> /*
>  * BAT1		LBC (PIXIS/CF)
> diff --git a/include/mpc86xx.h b/include/mpc86xx.h
> index c6f30f9..eb85d60 100644
> --- a/include/mpc86xx.h
> +++ b/include/mpc86xx.h
> @@ -83,5 +83,7 @@ static __inline__ unsigned long get_l2cr (void)
>    return l2cr_val;
> }
>
> +void setup_ddr_bat(phys_addr_t dram_size);
> +
> #endif  /* _ASMLANGUAGE */
> #endif	/* __MPC86xx_H__ */
> -- 
> 1.6.0.6
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Becky Bruce]

On Mar 30, 2010, at 12:02 PM, Becky Bruce wrote:

> Did I miss something here? Why did you change the coherence attribute
> in the BAT?  Please add an explanation to the comment, assuming you
> have a good reason for this (I'm not awake enough to think about this
> right now :)
>

Ah, my brain comes online - I see you only did this for 8610.  Carry  
on :)

-B

> Otherwise, a couple of minor nits below.
>
> On Mar 30, 2010, at 9:25 AM, Kumar Gala wrote:
>
>> From: Timur Tabi <timur@freescale.com>
>>
>> After determining how much DDR is actually in the system, set DBAT0
>> and
>> IBAT0 accordingly.  This ensures that the CPU won't attempt to access
>> (via speculation) addresses outside of actual memory.
>>
>> On 86xx systems, DBAT0 and IBAT0 (the BATs for DDR) are initialized
>> to 2GB
>> and kept that way.  If the system has less than 2GB of memory
>> (typical for
>> an MPC8610 HPCD), the CPU may attempt to access this memory during
>> speculation.  The zlib code is notorious for generating such memory
>> reads,
>> and indeed on the MPC8610, uncompressing the Linux kernel causes a
>> machine
>> check (without this patch).
>>
>> Signed-off-by: Timur Tabi <timur@freescale.com>
>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>> ---
>> * Fixed TO_BATU_BL to handle non-power of two
>> * Fixed write_bat to use computed 'bl' not always 2G size
>> * removed comment that wasn't need anymore
>>
>> board/freescale/mpc8610hpcd/mpc8610hpcd.c |    2 +
>> board/freescale/mpc8641hpcn/mpc8641hpcn.c |    2 +
>> cpu/mpc86xx/cpu.c                         |   30 ++++++++++++++++++++
>> +++++++++
>> cpu/mpc86xx/cpu_init.c                    |    4 +++
>> include/asm-ppc/mmu.h                     |    6 ++++-
>> include/configs/MPC8610HPCD.h             |    6 +---
>> include/configs/MPC8641HPCN.h             |    4 +--
>> include/mpc86xx.h                         |    2 +
>> 8 files changed, 48 insertions(+), 8 deletions(-)
>>
>> diff --git a/board/freescale/mpc8610hpcd/mpc8610hpcd.c b/board/
>> freescale/mpc8610hpcd/mpc8610hpcd.c
>> index 784a2ed..ab5f800 100644
>> --- a/board/freescale/mpc8610hpcd/mpc8610hpcd.c
>> +++ b/board/freescale/mpc8610hpcd/mpc8610hpcd.c
>> @@ -127,6 +127,8 @@ initdram(int board_type)
>> 	dram_size = fixed_sdram();
>> #endif
>>
>> +	setup_ddr_bat(dram_size);
>> +
>> 	puts(" DDR: ");
>> 	return dram_size;
>> }
>> diff --git a/board/freescale/mpc8641hpcn/mpc8641hpcn.c b/board/
>> freescale/mpc8641hpcn/mpc8641hpcn.c
>> index c521527..443c9fd 100644
>> --- a/board/freescale/mpc8641hpcn/mpc8641hpcn.c
>> +++ b/board/freescale/mpc8641hpcn/mpc8641hpcn.c
>> @@ -74,6 +74,8 @@ initdram(int board_type)
>> 	dram_size = fixed_sdram();
>> #endif
>>
>> +	setup_ddr_bat(dram_size);
>> +
>> 	puts("    DDR: ");
>> 	return dram_size;
>> }
>> diff --git a/cpu/mpc86xx/cpu.c b/cpu/mpc86xx/cpu.c
>> index f7e012d..ac171f5 100644
>> --- a/cpu/mpc86xx/cpu.c
>> +++ b/cpu/mpc86xx/cpu.c
>> @@ -197,3 +197,33 @@ void mpc86xx_reginfo(void)
>> 	printf("\tBR7\t0x%08X\tOR7\t0x%08X \n", in_be32(&lbc->br7),
>> in_be32(&lbc->or7));
>>
>> }
>> +
>> +/*
>> + * Set the DDR BATs to reflect the actual size of DDR.
>> + *
>> + * dram_size is the actual size of DDR, in bytes
>> + *
>> + * Note: we assume that CONFIG_MAX_MEM_MAPPED is 2G or smaller as
>> we only
>> + * are using a single BAT to cover DDR.
>> + *
>> + * If this is not true, (e.g. CONFIG_MAX_MEM_MAPPED is 2GB but
>> HID0_XBSEN
>> + * is not defined) then we might have a situation where U-Boot will
>> attempt
>> + * to relocated itself outside of the region mapped by DBAT0.
>> + * This will cause a machine check.
>
> At the very least, fix this comment to mention the problem with not
> being able to map all the RAM as well, if you're going to leave it
> that way.  Can you test a board with a strange amount of RAM (1.5GB,
> or something), and see what happens with this patch?  I really don't
> like leaving things this way.
>
>> + *
>> + */
>> +void setup_ddr_bat(phys_addr_t dram_size)
>> +{
>> +	unsigned long batu, bl;
>> +
>> +	bl = TO_BATU_BL(min(dram_size, CONFIG_MAX_MEM_MAPPED));
>> +
>> +	if (BATU_SIZE(bl) != dram_size) {
>> +		u64 sz = (u64)dram_size - BATU_SIZE(bl);
>> +		print_size(sz, " left unmapped\n");
>> +	}
>> +
>> +	batu = bl | BATU_VS | BATU_VP;
>> +	write_bat(DBAT0, batu, CONFIG_SYS_DBAT0L);
>> +	write_bat(IBAT0, batu, CONFIG_SYS_IBAT0L);
>> +}
>> diff --git a/cpu/mpc86xx/cpu_init.c b/cpu/mpc86xx/cpu_init.c
>> index 5a78a9c..b4f047d 100644
>> --- a/cpu/mpc86xx/cpu_init.c
>> +++ b/cpu/mpc86xx/cpu_init.c
>> @@ -138,8 +138,12 @@ int cpu_init_r(void)
>> /* Set up BAT registers */
>> void setup_bats(void)
>> {
>> +#if defined(CONFIG_SYS_DBAT0U) && defined(CONFIG_SYS_DBAT0L)
>> 	write_bat(DBAT0, CONFIG_SYS_DBAT0U, CONFIG_SYS_DBAT0L);
>> +#endif
>> +#if defined(CONFIG_SYS_IBAT0U) && defined(CONFIG_SYS_IBAT0L)
>> 	write_bat(IBAT0, CONFIG_SYS_IBAT0U, CONFIG_SYS_IBAT0L);
>> +#endif
>> 	write_bat(DBAT1, CONFIG_SYS_DBAT1U, CONFIG_SYS_DBAT1L);
>> 	write_bat(IBAT1, CONFIG_SYS_IBAT1U, CONFIG_SYS_IBAT1L);
>> 	write_bat(DBAT2, CONFIG_SYS_DBAT2U, CONFIG_SYS_DBAT2L);
>> diff --git a/include/asm-ppc/mmu.h b/include/asm-ppc/mmu.h
>> index fd10249..ce7f081 100644
>> --- a/include/asm-ppc/mmu.h
>> +++ b/include/asm-ppc/mmu.h
>> @@ -213,7 +213,11 @@ extern void print_bats(void);
>> #define BATL_PADDR(x) ((phys_addr_t)((x & 0xfffe0000)		\
>> 				     | ((x & 0x0e00ULL) << 24)	\
>> 				     | ((x & 0x04ULL) << 30)))
>> -#define BATU_SIZE(x) (1UL << (fls((x & BATU_BL_MAX) >> 2) + 17))
>> +#define BATU_SIZE(x) (1ULL << (fls((x & BATU_BL_MAX) >> 2) + 17))
>> +
>> +/* bytes into BATU_BL */
>> +#define TO_BATU_BL(x) \
>> +	(u32)((((1ull << __ilog2_u64((u64)x)) / (128 * 1024)) - 1) * 4)
>
> It's a nit, but can we change the *4 to << 2 ?  I know most modern
> compilers should optimize this, but I think it makes the code easier
> to read and is logically more sensical, and if you've got to change
> the patch, anyway, we might as well clean this up.
>
> -B
>
>
>>
>> /* Used to set up SDR1 register */
>> #define HASH_TABLE_SIZE_64K	0x00010000
>> diff --git a/include/configs/MPC8610HPCD.h b/include/configs/
>> MPC8610HPCD.h
>> index 1d2d659..fed441e 100644
>> --- a/include/configs/MPC8610HPCD.h
>> +++ b/include/configs/MPC8610HPCD.h
>> @@ -341,10 +341,8 @@
>> * BAT0		2G	Cacheable, non-guarded
>> * 0x0000_0000	2G	DDR
>> */
>> -#define CONFIG_SYS_DBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE)
>> -#define CONFIG_SYS_DBAT0U	(BATU_BL_2G | BATU_VS | BATU_VP)
>> -#define CONFIG_SYS_IBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE )
>> -#define CONFIG_SYS_IBAT0U	CONFIG_SYS_DBAT0U
>> +#define CONFIG_SYS_DBAT0L	(BATL_PP_RW)
>> +#define CONFIG_SYS_IBAT0L	(BATL_PP_RW)
>>
>> /*
>> * BAT1		1G	Cache-inhibited, guarded
>> diff --git a/include/configs/MPC8641HPCN.h b/include/configs/
>> MPC8641HPCN.h
>> index 12a8f60..94e4d24 100644
>> --- a/include/configs/MPC8641HPCN.h
>> +++ b/include/configs/MPC8641HPCN.h
>> @@ -482,9 +482,7 @@ extern unsigned long get_board_sys_clk(unsigned
>> long dummy);
>> * BAT0		DDR
>> */
>> #define CONFIG_SYS_DBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE)
>> -#define CONFIG_SYS_DBAT0U	(BATU_BL_2G | BATU_VS | BATU_VP)
>> -#define CONFIG_SYS_IBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE )
>> -#define CONFIG_SYS_IBAT0U	CONFIG_SYS_DBAT0U
>> +#define CONFIG_SYS_IBAT0L	(BATL_PP_RW | BATL_MEMCOHERENCE)
>>
>> /*
>> * BAT1		LBC (PIXIS/CF)
>> diff --git a/include/mpc86xx.h b/include/mpc86xx.h
>> index c6f30f9..eb85d60 100644
>> --- a/include/mpc86xx.h
>> +++ b/include/mpc86xx.h
>> @@ -83,5 +83,7 @@ static __inline__ unsigned long get_l2cr (void)
>>   return l2cr_val;
>> }
>>
>> +void setup_ddr_bat(phys_addr_t dram_size);
>> +
>> #endif  /* _ASMLANGUAGE */
>> #endif	/* __MPC86xx_H__ */
>> -- 
>> 1.6.0.6
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Becky Bruce]

On Mar 30, 2010, at 10:33 AM, Kumar Gala wrote:

>
> On Mar 30, 2010, at 10:32 AM, Timur Tabi wrote:
>
>> Kumar Gala wrote:
>>> We can make this work by updating how get_effective_memsize()  
>>> works in lib_ppc/board.c.  We just need to make it aware of the  
>>> amount of mapped memory.
>>
>> Is this something you want handled in this patch?
>
> Not really.  I was trying to get this patch in a state that we could  
> get it into v2010.03.  That change is a bit more intrusive.

More intrusive, but needs to be done soon.  Having u-boot thinking it  
has more ram than it does is fraught with peril.....  We shouldn't  
leave this as it is, or it's going to get swept under the rug until it  
bites us.

-B

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Timur Tabi]

Becky Bruce wrote:

> At the very least, fix this comment to mention the problem with not  
> being able to map all the RAM as well, if you're going to leave it  
> that way. 

Isn't that what the comment already says?  "region mapped by DBAT0" == "map all the RAM"?

> Can you test a board with a strange amount of RAM (1.5GB,  
> or something), and see what happens with this patch?  I really don't  
> like leaving things this way.

I'm pretty sure that U-Boot will machine check during relocation.  That's what the comment says.  I haven't tried it though.  I could probably set up bullwinkle later this week with 1.5GB and try it.

>> +#define TO_BATU_BL(x) \
>> +	(u32)((((1ull << __ilog2_u64((u64)x)) / (128 * 1024)) - 1) * 4)
> 
> It's a nit, but can we change the *4 to << 2 ?  I know most modern  
> compilers should optimize this, but I think it makes the code easier  
> to read and is logically more sensical, and if you've got to change  
> the patch, anyway, we might as well clean this up.

I used a spreadsheet to help me figure out the algorithm, so I wouldn't say that << 2 is more sensical, but I don't really care either way.  I don't see how it improves the readability, though.

-- 
Timur Tabi
Linux kernel developer@Freescale

[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size

[Timur Tabi]

On Tue, Mar 30, 2010 at 12:02 PM, Becky Bruce
<beckyb@kernel.crashing.org> wrote:

> Can you test a board with a strange amount of RAM (1.5GB,
> or something), and see what happens with this patch? ?I really don't
> like leaving things this way.

I put three 512MB sticks into bullwinkle, and got this:

U-Boot 2010.03-rc1-00006-g249f62d (Mar 30 2010 - 16:08:44)

CPU:   8641D, Version: 2.0, (0x80900120)
Core:  E600 Core 0, Version: 2.2, (0x80040202)
Clock Configuration:
       CPU:1500 MHz, MPX:500  MHz
       DDR:250  MHz (500 MT/s data rate), LBC:31.250 MHz
L1:    D-cache 32 KB enabled
       I-cache 32 KB enabled
L2:    512 KB enabled
Board: MPC8641HPCN, Sys ID: 0x10, Sys Ver: 0x10, FPGA Ver: 0x22, vBank: 1
I2C:   ready
DRAM:  512 MB left unmapped
    DDR:  1.1 GB (DDR2, 64-bit, CL=4, ECC off)

and here it hangs.

So not only does it hang, but it appears there's a bug in print_size().

-- 
Timur Tabi
Linux kernel developer at Freescale