On Tue, 2013-08-13 at 16:43 +0800, Shengzhou Liu wrote:
> Add support for freescale P1010RDB-PB board.
>
> Signed-off-by: Shengzhou Liu <shengzhou....@freescale.com>
> ---
> board/freescale/p1010rdb/law.c | 2 -
> board/freescale/p1010rdb/p1010rdb.c | 302
> ++++++++++++++++++++++++++++++++---
> board/freescale/p1010rdb/tlb.c | 4 -
> boards.cfg | 21 +++
> include/configs/P1010RDB.h | 111 ++++++++++---
> 5 files changed, 383 insertions(+), 57 deletions(-)
>
> diff --git a/board/freescale/p1010rdb/law.c b/board/freescale/p1010rdb/law.c
> index 0045127..ed41a05 100644
> --- a/board/freescale/p1010rdb/law.c
> +++ b/board/freescale/p1010rdb/law.c
> @@ -9,11 +9,9 @@
> #include <asm/mmu.h>
>
> struct law_entry law_table[] = {
> -#ifndef CONFIG_SDCARD
> SET_LAW(CONFIG_SYS_FLASH_BASE_PHYS, LAW_SIZE_32M, LAW_TRGT_IF_IFC),
> SET_LAW(CONFIG_SYS_CPLD_BASE_PHYS, LAW_SIZE_128K, LAW_TRGT_IF_IFC),
> SET_LAW(CONFIG_SYS_NAND_BASE_PHYS, LAW_SIZE_1M, LAW_TRGT_IF_IFC),
> -#endif
> };
If this is applicable to the current board as well (is that
P1010RDB-PA?), then it isn't related to adding PB support and thus
belongs in a separate patch.
> +uint pin_mux;
This is too generic for a global variable. Does it even need to be
global?
> -#ifndef CONFIG_SDCARD
> struct cpld_data {
> u8 cpld_ver; /* cpld revision */
> +#if defined(CONFIG_P1010RDB)
> u8 pcba_ver; /* pcb revision number */
> u8 twindie_ddr3;
> u8 res1[6];
> @@ -51,18 +69,16 @@ struct cpld_data {
> u8 por1; /* POR Options */
> u8 por2; /* POR Options */
> u8 por3; /* POR Options */
> +#elif defined(CONFIG_P1010RDB_PB)
> + u8 rom_loc;
> +#endif
> };
CONFIG_P1010RDB should be defined if CONFIG_P1010RDB_PB is defined.
Define a new CONFIG_P1010RDB_PA (if that's the appropriate name) for
things that are specifically for the older revision.
> +#if defined(CONFIG_P1010RDB) && defined(DEBUG)
> void cpld_show(void)
> {
> struct cpld_data *cpld_data = (void *)(CONFIG_SYS_CPLD_BASE);
>
> - printf("CPLD: V%x.%x PCBA: V%x.0\n",
> - in_8(&cpld_data->cpld_ver) & 0xF0,
> - in_8(&cpld_data->cpld_ver) & 0x0F,
> - in_8(&cpld_data->pcba_ver) & 0x0F);
Why are you removing this? Where is cpld_show() called?
> @@ -246,6 +446,16 @@ void fdt_del_sdhc(void *blob)
> }
> }
>
> +void fdt_del_ifc(void *blob)
> +{
> + int nodeoff = 0;
> +
> + while ((nodeoff = fdt_node_offset_by_compatible(blob, 0,
> + "fsl,ifc")) >= 0) {
> + fdt_del_node(blob, nodeoff);
> + }
> +}
Is this PB-specific? If no, why is it in this patch? If not, why isn't
the caller guarded by the PB ifdef?
> +static int pin_mux_cmd(cmd_tbl_t *cmdtp, int flag, int argc,
> + char * const argv[])
> +{
> + if (argc < 2)
> + return CMD_RET_USAGE;
> + if (strcmp(argv[1], "ifc") == 0)
> + config_board_mux(MUX_TYPE_IFC);
> + else if (strcmp(argv[1], "sdhc") == 0)
> + config_board_mux(MUX_TYPE_SDHC);
> + else
> + return CMD_RET_USAGE;
> + return 0;
> +}
> +
> +U_BOOT_CMD(
> + mux, 2, 0, pin_mux_cmd,
> + "configure multiplexing pin for IFC/SDHC bus in runtime",
> + "bus_type (e.g. mux sdhc)"
> +);
Are you sure this is a good idea? What happens to the drivers using
said hardware at the time? Granted they should be idle when not running
a command that actively uses them, but still... Usually we use hwconfig
for this sort of thing.
> @@ -203,25 +207,24 @@ extern unsigned long get_sdram_size(void);
> #define CONFIG_SYS_DDR_INIT_ADDR 0x00000000
> #define CONFIG_SYS_DDR_INIT_EXT_ADDR 0x00000000
> #define CONFIG_SYS_DDR_MODE_CONTROL 0x00000000
> -
> #define CONFIG_SYS_DDR_ZQ_CONTROL 0x89080600
> #define CONFIG_SYS_DDR_SR_CNTR 0x00000000
> #define CONFIG_SYS_DDR_RCW_1 0x00000000
> #define CONFIG_SYS_DDR_RCW_2 0x00000000
> -#define CONFIG_SYS_DDR_CONTROL 0x470C0000 /* Type = DDR3
> */
> -#define CONFIG_SYS_DDR_CONTROL_2 0x04401010
> +#define CONFIG_SYS_DDR_CONTROL 0xc70c0008 /* Type = DDR3
> */
> +#define CONFIG_SYS_DDR_CONTROL_2 0x24401000
> #define CONFIG_SYS_DDR_TIMING_4 0x00000001
> -#define CONFIG_SYS_DDR_TIMING_5 0x03402400
> +#define CONFIG_SYS_DDR_TIMING_5 0x02401400
>
> -#define CONFIG_SYS_DDR_TIMING_3_800 0x00020000
> -#define CONFIG_SYS_DDR_TIMING_0_800 0x00330004
> -#define CONFIG_SYS_DDR_TIMING_1_800 0x6f6B4644
> +#define CONFIG_SYS_DDR_TIMING_3_800 0x00030000
> +#define CONFIG_SYS_DDR_TIMING_0_800 0x00110104
> +#define CONFIG_SYS_DDR_TIMING_1_800 0x6f6b8644
> #define CONFIG_SYS_DDR_TIMING_2_800 0x0FA888CF
> #define CONFIG_SYS_DDR_CLK_CTRL_800 0x03000000
> -#define CONFIG_SYS_DDR_MODE_1_800 0x40461520
> -#define CONFIG_SYS_DDR_MODE_2_800 0x8000c000
> +#define CONFIG_SYS_DDR_MODE_1_800 0x00441420
> +#define CONFIG_SYS_DDR_MODE_2_800 0x00000000
> #define CONFIG_SYS_DDR_INTERVAL_800 0x0C300100
> -#define CONFIG_SYS_DDR_WRLVL_CONTROL_800 0x8655A608
> +#define CONFIG_SYS_DDR_WRLVL_CONTROL_800 0x8675f608
Shouldn't this be ifdeffed by the board revision?
> +#if defined(CONFIG_P1010RDB)
> +#define CONFIG_BOOTMODE \
> + "boot_bank0=i2c dev 0; i2c mw 18 1 f1; i2c mw 18 3 f0; \
> +mw.b ffb00011 0; mw.b ffb00009 0; reset\0 \
> + boot_bank1=i2c dev 0; i2c mw 18 1 f1; i2c mw 18 3 f0; \
> +mw.b ffb00011 0; mw.b ffb00009 1; reset\0 \
> + boot_nand=i2c dev 0; i2c mw 18 1 f9; i2c mw 18 3 f0; \
> +mw.b ffb00011 0; mw.b ffb00017 1; reset\0"
> +#elif defined(CONFIG_P1010RDB_PB)
> +#define CONFIG_BOOTMODE \
> + "boot_bank0=i2c dev 0; i2c mw 18 1 fe; i2c mw 18 3 0; i2c mw 19 1 2; \
> +i2c mw 19 3 e1; reset\0 \
> + boot_bank1=i2c dev 0; i2c mw 18 1 fe; i2c mw 18 3 0; i2c mw 19 1 12; \
> +i2c mw 19 3 e1; reset\0 \
> + boot_nand=i2c dev 0; i2c mw 18 1 fc; i2c mw 18 3 0; i2c mw 19 1 8; \
> +i2c mw 19 3 f7; reset\0 \
> + boot_spi=i2c dev 0; i2c mw 18 1 fa; i2c mw 18 3 0; i2c mw 19 1 0; \
> +i2c mw 19 3 f7; reset\0 \
> + boot_sd=i2c dev 0; i2c mw 18 1 f8; i2c mw 18 3 0; i2c mw 19 1 4; \
> +i2c mw 19 3 f3; reset\0"
> +#endif
Don't put newlines in strings. Do it like this:
#define CONFIG_BOOTMODE \
"bootb_bank0=i2c dev 0; i2c mw 18 1 fe; i2c mw 18 3 0; " \
"i2c mw 19 1 2; i2c mw 19 3 e1; reset\0" \
...
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
