Dear Timur Tabi, In message <1274392909-16422-1-git-send-email-ti...@freescale.com> you wrote: > Add basic suport for the Freescale P1022DS reference board. > > Signed-off-by: Timur Tabi <ti...@freescale.com> > --- > > This patch requires the following two patches to be applied first: > > fsl/85xx: add clkdvdr and pmuxcr2 to global utilities structure definition > fsl: rename 'dma' to 'brdcfg1' in the ngPIXIS structure > > Makefile | 3 + > arch/powerpc/cpu/mpc8xxx/pci_cfg.c | 26 ++ > board/freescale/p1022ds/Makefile | 40 +++ > board/freescale/p1022ds/config.mk | 14 + > board/freescale/p1022ds/ddr.c | 108 +++++++ > board/freescale/p1022ds/law.c | 27 ++ > board/freescale/p1022ds/p1022ds.c | 369 ++++++++++++++++++++++++ > board/freescale/p1022ds/p1022ds_diu.c | 151 ++++++++++ > board/freescale/p1022ds/tlb.c | 76 +++++ > include/configs/P1022DS.h | 505 > +++++++++++++++++++++++++++++++++ > 10 files changed, 1319 insertions(+), 0 deletions(-) > create mode 100644 board/freescale/p1022ds/Makefile > create mode 100644 board/freescale/p1022ds/config.mk > create mode 100644 board/freescale/p1022ds/ddr.c > create mode 100644 board/freescale/p1022ds/law.c > create mode 100644 board/freescale/p1022ds/p1022ds.c > create mode 100644 board/freescale/p1022ds/p1022ds_diu.c > create mode 100644 board/freescale/p1022ds/tlb.c > create mode 100644 include/configs/P1022DS.h
Entries to MAKEALL and MAINTAINERS missing. > diff --git a/Makefile b/Makefile > index 1445e8b..583a576 100644 > --- a/Makefile > +++ b/Makefile > @@ -2493,6 +2493,9 @@ P2020DS_36BIT_config \ > P2020DS_config: unconfig > @$(MKCONFIG) -t $(@:_config=) P2020DS powerpc mpc85xx p2020ds freescale > > +P1022DS_config: unconfig > + @$(MKCONFIG) -t $(@:_config=) P1022DS powerpc mpc85xx p1022ds freescale > + > P1011RDB_config \ > P1011RDB_NAND_config \ > P1011RDB_SDCARD_config \ Please keep lists sorted / sort them. > diff --git a/board/freescale/p1022ds/ddr.c b/board/freescale/p1022ds/ddr.c > new file mode 100644 > index 0000000..c43c902 > --- /dev/null > +++ b/board/freescale/p1022ds/ddr.c ... > +void fsl_ddr_get_spd(ddr3_spd_eeprom_t *ctrl_dimms_spd, unsigned int > ctrl_num) > +{ > + int ret; > + > + /* The P1022 has only one DDR controller, and the board has only one > + DIMM slot. */ Incorrect multiline comment style. ... > +/* ranges for parameters: > + * wr_data_delay = 0-6 > + * clk adjust = 0-8 > + * cpo 2-0x1E (30) > + */ Incorrect multiline comment style. > +static const board_specific_parameters_t bsp[] = { > +/* memory controller 0 */ > +/* lo| hi| num| clk| cpo|wrdata|2T */ > +/* mhz| mhz|ranks|adjst| | delay| */ Incorrect multiline comment style. Please fix globally! > + { 0, 333, 1, 5, 31, 3, 0}, > + {334, 400, 1, 5, 31, 3, 0}, > + {401, 549, 1, 5, 31, 3, 0}, > + {550, 680, 1, 5, 31, 5, 0}, > + {681, 850, 1, 5, 31, 5, 0}, > + { 0, 333, 2, 5, 31, 3, 0}, > + {334, 400, 2, 5, 31, 3, 0}, > + {401, 549, 2, 5, 31, 3, 0}, > + {550, 680, 2, 5, 31, 5, 0}, > + {681, 850, 2, 5, 31, 5, 0}, Please use TABs for vertical alignment. > +void fsl_ddr_board_options(memctl_options_t *popts, dimm_params_t *pdimm, > + unsigned int ctrl_num) > +{ ... > + /* Per AN4039, enable ZQ calibration. */ > + popts->zq_en = 1; > + > + > + /* Drop one of these empty lines, please. > +int board_early_init_f(void) > +{ > + ccsr_gur_t *gur = (void *)CONFIG_SYS_MPC85xx_GUTS_ADDR; > + > + /* Set pmuxcr to allow both i2c1 and i2c2 */ > + setbits_be32(&gur->pmuxcr, 0x1000); > + in_be32(&gur->pmuxcr); What is this in_be32() needed for? Either add a comment why it's needed, or remove it. ... > +phys_size_t initdram(int board_type) > +{ > + phys_size_t dram_size = 0; > + > + puts("Initializing....\n"); > + > + dram_size = fsl_ddr_sdram(); > + dram_size = setup_ddr_tlbs(dram_size / 0x100000); > + dram_size *= 0x100000; > + > + puts(" DDR: "); > + return dram_size; How about using get_ram_size() for autosizing / testing? > + /* Enable the TFP410 Encoder (I2C address 0x38) > + */ Please use a symbolic name instead of the hard-wired constant. > +void pci_init_board(void) > +{ > + volatile ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR); > + struct fsl_pci_info pci_info[3]; > + u32 devdisr, pordevsr, io_sel; > + int first_free_busno = 0; > + int num = 0; > + > + int pcie_ep, pcie_configured; > + > + devdisr = in_be32(&gur->devdisr); > + pordevsr = in_be32(&gur->pordevsr); > + io_sel = (pordevsr & MPC85xx_PORDEVSR_IO_SEL) >> 18; > + > + debug (" pci_init_board: devdisr=%x, io_sel=%x\n", devdisr, io_sel); > + > + if (!(pordevsr & MPC85xx_PORDEVSR_SGMII2_DIS)) > + printf(" eTSEC2 is in sgmii mode.\n"); > + if (!(pordevsr & MPC85xx_PORDEVSR_SGMII3_DIS)) > + printf(" eTSEC3 is in sgmii mode.\n"); > + > + puts("\n"); Drop that. > +#ifdef CONFIG_PCIE2 > + pcie_configured = is_fsl_pci_cfg(LAW_TRGT_IF_PCIE_2, io_sel); > + > + if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE2)) { > + SET_STD_PCIE_INFO(pci_info[num], 2); > + pcie_ep = fsl_setup_hose(&pcie2_hose, pci_info[num].regs); > + printf(" PCIE2 connected to Slot 3 as %s (base addr %lx)\n", > + pcie_ep ? "Endpoint" : "Root Complex", > + pci_info[num].regs); > + first_free_busno = fsl_pci_init_port(&pci_info[num++], > + &pcie2_hose, first_free_busno); > + > + } else { > + printf(" PCIE2: disabled\n"); > + } > + puts("\n"); Ditto. > +#else > + setbits_be32(&gur->devdisr, MPC85xx_DEVDISR_PCIE2); /* disable */ > +#endif > + > +#ifdef CONFIG_PCIE3 > + pcie_configured = is_fsl_pci_cfg(LAW_TRGT_IF_PCIE_3, io_sel); > + > + if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE3)) { > + SET_STD_PCIE_INFO(pci_info[num], 3); > + pcie_ep = fsl_setup_hose(&pcie3_hose, pci_info[num].regs); > + printf(" PCIE3 connected to Slot 2 as %s (base addr %lx)\n", > + pcie_ep ? "Endpoint" : "Root Complex", > + pci_info[num].regs); > + first_free_busno = fsl_pci_init_port(&pci_info[num++], > + &pcie3_hose, first_free_busno); > + } else { > + printf(" PCIE3: disabled\n"); > + } > + puts("\n"); Ditto. > +#else > + setbits_be32(&gur->devdisr, MPC85xx_DEVDISR_PCIE3); /* disable */ > +#endif > + > +#ifdef CONFIG_PCIE1 > + pcie_configured = is_fsl_pci_cfg(LAW_TRGT_IF_PCIE_1, io_sel); > + > + if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE)) { > + SET_STD_PCIE_INFO(pci_info[num], 1); > + pcie_ep = fsl_setup_hose(&pcie1_hose, pci_info[num].regs); > + printf(" PCIE1 connected to Slot 1 as %s (base addr %lx)\n", > + pcie_ep ? "Endpoint" : "Root Complex", > + pci_info[num].regs); > + first_free_busno = fsl_pci_init_port(&pci_info[num++], > + &pcie1_hose, first_free_busno); > + } else { > + printf(" PCIE1: disabled\n"); > + } > + puts("\n"); Ditto. Hm... looks as if you were repeating the same code 3 times. Please make this a function. > +#ifdef CONFIG_GET_CLK_FROM_ICS307 This CONFIG_GET_CLK_FROM_ICS307 is documented? > +/* decode S[0-2] to Output Divider (OD) */ > +static u8 ics307_S_to_OD[] = { > + 10, 2, 8, 4, 5, 7, 3, 6 > +}; > + > +/* Calculate frequency being generated by ICS307-02 clock chip based upon > + * the control bytes being programmed into it. */ > +/* XXX: This function should probably go into a common library */ > +static unsigned long ics307_clk_freq(unsigned char cw0, unsigned char cw1, > + unsigned char cw2) > +{ > + const unsigned long InputFrequency = CONFIG_ICS307_REFCLK_HZ; > + unsigned long VDW = ((cw1 << 1) & 0x1FE) + ((cw2 >> 7) & 1); > + unsigned long RDW = cw2 & 0x7F; > + unsigned long OD = ics307_S_to_OD[cw0 & 0x7]; > + unsigned long freq; Please do not use any CamelCase or UPPER CASE identifiers. Please fix globally. > +#ifdef CONFIG_PCIE3 > + ft_fsl_pci_setup(blob, "pci2", &pcie3_hose); > +#endif > +#ifdef CONFIG_PCIE2 > + ft_fsl_pci_setup(blob, "pci0", &pcie2_hose); > +#endif > +#ifdef CONFIG_PCIE1 > + ft_fsl_pci_setup(blob, "pci1", &pcie1_hose); > +#endif Is this intentional? Looks weird to me... PCIE1 => pci1 => pcie1_hose PCIE2 => pci0 => pcie2_hose PCIE3 => pci2 => pcie3_hose Weird, weird... > +++ b/board/freescale/p1022ds/p1022ds_diu.c > @@ -0,0 +1,151 @@ This should probably go to drivers/video/ ? > + * Copyright 2007-2010 Freescale Semiconductor, Inc. > + * Authors: York Sun <york...@freescale.com> > + * Srikanth Srinivasan <srikanth.sriniva...@freescale.com> > + * Timur Tabi <ti...@freescale.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the Free > + * Software Foundation; either version 2 of the License, or (at your option) > + * any later version. > + */ > + > +#include <common.h> > +#include <command.h> > +#include <asm/io.h> > + > +#include "../common/ngpixis.h" > +#include "../common/fsl_diu_fb.h" > + > +#if defined(CONFIG_VIDEO) || defined(CONFIG_CFB_CONSOLE) > +#include <stdio_dev.h> > +#include <video_fb.h> > +#endif > + > +extern unsigned int FSL_Logo_BMP[]; > + > +static unsigned int xres, yres; > + > +void diu_set_pixel_clock(unsigned int pixclock) > +{ > + ccsr_gur_t *gur = (void *)CONFIG_SYS_MPC85xx_GUTS_ADDR; > + unsigned long speed_ccb, temp, pixval; > + > + speed_ccb = get_bus_freq(0); > + temp = 1000000000UL / pixclock; > + temp *= 1000; > + pixval = speed_ccb / temp; > + debug("DIU pixval = %lu\n", pixval); > + > + /* Modify PXCLK in GUTS CLKDVDR */ > + debug("DIU: Current value of CLKDVDR = 0x%08x\n", > in_be32(&gur->clkdvdr)); > + clrbits_be32(&gur->clkdvdr, 0xdfff0000); /* turn off clock */ > + setbits_be32(&gur->clkdvdr, 0x80000000 | ((pixval & 0x1F) << 16)) > + debug("DIU: Modified value of CLKDVDR = 0x%08x\n", > in_be32(&gur->clkdvdr)); > +} Hmmm... this looks pretty much similar to board/freescale/mpc8610hpcd/mpc8610hpcd_diu.c; I think you should merge these two into one driver. > +int p1022diu_init_show_bmp(cmd_tbl_t *cmdtp, int flag, int argc, char > *argv[]) > +{ > + unsigned int addr; ...while doing so, please clean up to use the standard bmp command instead. > +U_BOOT_CMD( > + diufb, CONFIG_SYS_MAXARGS, 1, p1022diu_init_show_bmp, > + "Init or Display BMP file", > + "init\n - initialize DIU\n" > + "addr\n - display bmp at address 'addr'" > +); This should go away. > diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h > new file mode 100644 > index 0000000..65a1265 > --- /dev/null > +++ b/include/configs/P1022DS.h ... > +#ifndef __ASSEMBLY__ > +extern unsigned long calculate_board_sys_clk(void); > +extern unsigned long calculate_board_ddr_clk(void); > +#endif Please move to appropriate header file. > +/* > + * Base addresses -- Note these are effective addresses where the > + * actual resources get mapped (not physical addresses) > + */ > +#define CONFIG_SYS_CCSRBAR_DEFAULT 0xff700000 /* CCSRBAR Default */ > +#define CONFIG_SYS_CCSRBAR 0xffe00000 /* relocated CCSRBAR */ > +#define CONFIG_SYS_CCSRBAR_PHYS 0xfffe00000ull /* physical > addr of CCSRBAR */ > +#define CONFIG_SYS_IMMR CONFIG_SYS_CCSRBAR /* PQII > uses CONFIG_SYS_IMMR */ Lines too long. Please fix globally. > +/* ECC will be enabled based on perf_mode environment variable */ > +//#define CONFIG_DDR_ECC No C++ comments. Please fix globally. > +/* video */ > +/* #define CONFIG_VIDEO */ Please do not add dead code. Please fix globally. > +#define CONFIG_BOOTDELAY 10 /* -1 disables auto-boot */ > +#undef CONFIG_BOOTARGS /* the boot command will set bootargs */ Do not undefine what is not defined anyway. > +#define CONFIG_EXTRA_ENV_SETTINGS \ > + "perf_mode=stable\0" \ > + "memctl_intlv_ctl=2\0" \ > + "netdev=eth0\0" \ > + "uboot=" MK_STR(CONFIG_UBOOTPATH) "\0" \ > + "tftpflash=tftpboot $loadaddr $uboot; " \ > + "protect off " MK_STR(TEXT_BASE) " +$filesize; " \ > + "erase " MK_STR(TEXT_BASE) " +$filesize; " \ > + "cp.b $loadaddr " MK_STR(TEXT_BASE) " $filesize; " \ > + "protect on " MK_STR(TEXT_BASE) " +$filesize; " \ > + "cmp.b $loadaddr " MK_STR(TEXT_BASE) " $filesize\0" \ Please indent by TABs only. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de It's hard to make a program foolproof because fools are so ingenious. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot