[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