From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Tyser Date: Wed, 21 Jul 2010 11:31:19 -0500 Subject: [U-Boot] [PATCH 4/4] powerpc/p4080: Add support for the P4080DS board In-Reply-To: <1279232142-16322-4-git-send-email-galak@kernel.crashing.org> References: <1279232142-16322-1-git-send-email-galak@kernel.crashing.org> <1279232142-16322-2-git-send-email-galak@kernel.crashing.org> <1279232142-16322-3-git-send-email-galak@kernel.crashing.org> <1279232142-16322-4-git-send-email-galak@kernel.crashing.org> Message-ID: <1279729879.7232.8656.camel@petert> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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. > +#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 > +#include > + > +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 > +/* > + * 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