[U-Boot] [PATCH 4/4] powerpc/p4080: Add support for the P4080DS board
Peter Tyser
ptyser at xes-inc.com
Wed Jul 21 18:31:19 CEST 2010
Hi Kumar,
That's great to see official 4080 board support! I had a few comments
below. I haven't dug into the 4080 much, so take them with a grain of
salt.
<snip>
> +#ifdef CONFIG_PHYS_64BIT
> + puts("36-bit Addressing\n");
> +#endif
Any chance CONFIG_ENABLE_36BIT_PHYS could be defined without
CONFIG_PHYS_64BIT, or vice-versa? Or any reason to not use
CONFIG_ENABLE_36BIT_PHYS above?
> + /* Display the actual SERDES reference clocks as configured by the
> + * dip switches on the board. Note that the SWx registers could
> + * technically be set to force the reference clocks to match the
> + * values that the SERDES expects (or vice versa). For now, however,
> + * we just display both values and hope the user notices when they
> + * don't match.
> + */
> + puts("SERDES Reference Clocks: ");
> + sw = in_8(&PIXIS_SW(3));
> + printf("Bank1=%uMHz ", (sw & 0x40) ? 125 : 100);
> + printf("Bank2=%sMHz ", (sw & 0x20) ? "156.25" : "125");
> + printf("Bank3=%sMHz\n", (sw & 0x10) ? "156.25" : "125");
Could you use serdes_clock_to_string() above? It'd make the values
above a little less magical and be portable to other boards that don't
use the PIXIS FPGA.
> +
> +#ifdef CONFIG_ECC_INIT_VIA_DDRCONTROLLER
> + /*
> + * Poll until memory is initialized. 512 Meg at 400MHz might hit this
> + * 200 times or so.
> + */
> + while ((ddr->sdram_cfg_2 & 16) != 0) {
> + debug("sdram_cfg2 = 0x%08x\n", ddr->sdram_cfg_2);
> + udelay(1000);
> + }
> + asm("sync; isync");
> + udelay(500);
> +
> + while ((ddr2->sdram_cfg_2 & 16) != 0) {
> + debug("sdram_cfg2 = 0x%08x\n", ddr2->sdram_cfg_2);
> + udelay(1000);
> + }
> + asm("sync; isync");
> + udelay(500);
> +#endif
Use in_be32()'s above? And change 16 to 0x10, or add a D_INIT define?
> +phys_size_t initdram(int board_type)
> +{
> + phys_size_t dram_size;
> +
> + puts("Initializing....\n");
> +
> +#ifdef CONFIG_DDR_SPD
> + dram_size = fsl_ddr_sdram();
> +#else
> + dram_size = fixed_sdram();
> +#endif
> + setup_ddr_tlbs(dram_size / 0x100000);
> +
> + puts(" DDR: ");
> + return dram_size;
> +}
Should the puts("DDR:") be removed? With it I'd think the output would
be "DRAM: DDR: (DDR3, 64-bit...)". Ie the DDR is a bit redundant
and the spaces a bit excessive.
> +#ifdef CONFIG_MP
> +void board_lmb_reserve(struct lmb *lmb)
> +{
> + cpu_mp_lmb_reserve(lmb);
> +}
> +#endif
It'd be nice to move the board_lmb_reserve() somewhere common at some
point since every board that has CONFIG_MP defined currently uses the
same chunk of code. Or maybe remove board_lmb_reserve() and have its
callees call cpu_mp_lmb_reserve().
> +#include <common.h>
> +#include <asm/mmu.h>
> +
> +struct fsl_e_tlb_entry tlb_table[] = {
> + /* TLB 0 - for temp stack in cache */
> + SET_TLB_ENTRY(0, CONFIG_SYS_INIT_RAM_ADDR, CONFIG_SYS_INIT_RAM_ADDR_PHYS,
> + MAS3_SX|MAS3_SW|MAS3_SR, 0,
> + 0, 0, BOOKE_PAGESZ_4K, 0),
> + SET_TLB_ENTRY(0, CONFIG_SYS_INIT_RAM_ADDR + 4 * 1024 , CONFIG_SYS_INIT_RAM_ADDR_PHYS + 4 * 1024,
> + MAS3_SX|MAS3_SW|MAS3_SR, 0,
> + 0, 0, BOOKE_PAGESZ_4K, 0),
> + SET_TLB_ENTRY(0, CONFIG_SYS_INIT_RAM_ADDR + 8 * 1024 , CONFIG_SYS_INIT_RAM_ADDR_PHYS + 8 * 1024,
> + MAS3_SX|MAS3_SW|MAS3_SR, 0,
> + 0, 0, BOOKE_PAGESZ_4K, 0),
> + SET_TLB_ENTRY(0, CONFIG_SYS_INIT_RAM_ADDR + 12 * 1024 , CONFIG_SYS_INIT_RAM_ADDR_PHYS + 12 * 1024,
> + MAS3_SX|MAS3_SW|MAS3_SR, 0,
> + 0, 0, BOOKE_PAGESZ_4K, 0),
> +
> + SET_TLB_ENTRY(0, PIXIS_BASE, PIXIS_BASE_PHYS,
> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> + 0, 0, BOOKE_PAGESZ_4K, 0),
> +
> + /* TLB 1 */
> + /* *I*** - Covers boot page */
> + SET_TLB_ENTRY(1, 0xfffff000, 0xfffff000,
> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> + 0, 0, BOOKE_PAGESZ_4K, 1),
> +
> + /* *I*G* - CCSRBAR */
> + SET_TLB_ENTRY(1, CONFIG_SYS_CCSRBAR, CONFIG_SYS_CCSRBAR_PHYS,
> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> + 0, 1, BOOKE_PAGESZ_16M, 1),
> +
> + /* *I*G* - Flash, localbus */
> + /* This will be changed to *I*G* after relocation to RAM. */
> + SET_TLB_ENTRY(1, CONFIG_SYS_FLASH_BASE, CONFIG_SYS_FLASH_BASE_PHYS,
> + MAS3_SX|MAS3_SR, MAS2_W|MAS2_G,
> + 0, 2, BOOKE_PAGESZ_256M, 1),
> +
> + /* *I*G* - PCI */
> + SET_TLB_ENTRY(1, CONFIG_SYS_PCIE1_MEM_VIRT, CONFIG_SYS_PCIE1_MEM_PHYS,
> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> + 0, 3, BOOKE_PAGESZ_1G, 1),
> +
> + /* *I*G* - PCI */
> + SET_TLB_ENTRY(1, CONFIG_SYS_PCIE1_MEM_VIRT + 0x40000000, CONFIG_SYS_PCIE1_MEM_PHYS + 0x40000000,
> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> + 0, 4, BOOKE_PAGESZ_256M, 1),
> +
> + SET_TLB_ENTRY(1, CONFIG_SYS_PCIE1_MEM_VIRT + 0x50000000, CONFIG_SYS_PCIE1_MEM_PHYS + 0x50000000,
> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> + 0, 5, BOOKE_PAGESZ_256M, 1),
> +
> + /* *I*G* - PCI I/O */
> + SET_TLB_ENTRY(1, CONFIG_SYS_PCIE1_IO_VIRT, CONFIG_SYS_PCIE1_IO_PHYS,
> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> + 0, 6, BOOKE_PAGESZ_256K, 1),
> +
> + /* Bman/Qman */
> + SET_TLB_ENTRY(1, CONFIG_SYS_BMAN_MEM_BASE, CONFIG_SYS_BMAN_MEM_PHYS,
> + MAS3_SX|MAS3_SW|MAS3_SR, 0,
> + 0, 9, BOOKE_PAGESZ_1M, 1),
> + SET_TLB_ENTRY(1, CONFIG_SYS_BMAN_MEM_BASE + 0x00100000, CONFIG_SYS_BMAN_MEM_PHYS + 0x00100000,
> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> + 0, 10, BOOKE_PAGESZ_1M, 1),
> + SET_TLB_ENTRY(1, CONFIG_SYS_QMAN_MEM_BASE, CONFIG_SYS_QMAN_MEM_PHYS,
> + MAS3_SX|MAS3_SW|MAS3_SR, 0,
> + 0, 11, BOOKE_PAGESZ_1M, 1),
> + SET_TLB_ENTRY(1, CONFIG_SYS_QMAN_MEM_BASE + 0x00100000, CONFIG_SYS_QMAN_MEM_PHYS + 0x00100000,
> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> + 0, 12, BOOKE_PAGESZ_1M, 1),
> +#ifdef CONFIG_SYS_DCSRBAR_PHYS
> + SET_TLB_ENTRY(1, CONFIG_SYS_DCSRBAR, CONFIG_SYS_DCSRBAR_PHYS,
> + MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
> + 0, 13, BOOKE_PAGESZ_4M, 1),
> +#endif
> +};
Lines > 80 chars. What happened to TLBs 7 and 8? It looks like you
configure LAWs for the SRIO interfaces, but not TLBs?
> +int num_tlb_entries = ARRAY_SIZE(tlb_table);
> diff --git a/boards.cfg b/boards.cfg
> index 5043833..8187b68 100644
> --- a/boards.cfg
> +++ b/boards.cfg
> @@ -340,6 +340,7 @@ MPC8540ADS powerpc mpc85xx mpc8540ads freescale
> MPC8544DS powerpc mpc85xx mpc8544ds freescale
> MPC8560ADS powerpc mpc85xx mpc8560ads freescale
> MPC8568MDS powerpc mpc85xx mpc8568mds freescale
> +P4080DS powerpc mpc85xx corenet_ds freescale
> XPEDITE5200 powerpc mpc85xx xpedite5200 xes
> XPEDITE5370 powerpc mpc85xx xpedite5370 xes
> P1022DS powerpc mpc85xx p1022ds freescale
How are these boards supposed to be sorted in general? It seems odd not
to have the P1022DS and P4080DS next to each other.
> +
> +/*
> + * P4080 DS board configuration file
> + *
> + */
Extra line above.
> diff --git a/include/configs/corenet_ds.h b/include/configs/corenet_ds.h
<snip>
> +/*
> + * P4080 DS board configuration file
> + *
> + */
corenet_ds.h above? And extra line. What's the connection between the
corenet_ds and P4080DS and their header files? I had assumed the
corenet_ds.h file would contain somewhat generic corenet defines that
could be shared between other corenet-based boards, but it seems to
contain many board-specific defines.
Best,
Peter
More information about the U-Boot
mailing list