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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
