[U-Boot] [PATCH v2] mpc86xx: set the DDR BATs after calculating true DDR size
Becky Bruce
beckyb at kernel.crashing.org
Tue Mar 30 19:02:04 CEST 2010
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 at 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 at freescale.com>
> Signed-off-by: Kumar Gala <galak at 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
More information about the U-Boot
mailing list