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