[U-Boot] [PATCH 4/4] powerpc/p4080: Add support for the P4080DS board

[Kumar Gala]

On Jul 21, 2010, at 11:31 AM, Peter Tyser wrote:

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

CONFIG_ENABLE_36BIT_PHYS is just to enable the capability on e500v2 cores.  It probably should be renamed at this point.

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

Dont see the value to convert doubly that way.

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

Going to drop the non-SPD code for now.

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

This is consistent w/every other board port we have right now.

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

Going to leave this alone for now.  Its an orthogonal cleanup to this board port.

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

Correct, we dont ever access SRIO space in u-boot and dont have the VA address space left to map it anywhere.

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

Will fix that, should be sorted by name.  I think P1022DS got out of order.

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

corenet_ds is for corenet based platform DS boards.  This will include P3041 and P5020 DS boards in the future.  Its not about corenet platforms which is a different direction.

- k