[U-Boot] [PATCH] powerpc/p1010rdb-pb: add support for p1010rdb-pb board

Scott Wood scottwood at freescale.com
Wed Aug 14 02:35:15 CEST 2013


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.Liu at 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





More information about the U-Boot mailing list