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
