On Thu, 2013-08-29 at 06:10 -0500, Liu Shengzhou-B36685 wrote:

...

-----Original Message----- From: Wood Scott-B07421 Sent: Wednesday, August 14, 2013 8:35 AM To: Liu Shengzhou-B36685 Cc: u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH] powerpc/p1010rdb-pb: add support for p1010rdb-pb board

...

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.

As P1010RDB-PA will no longer be supported officially,

Why? Don't confuse Freescale supporting the board with U-Boot supporting the board.

but we still keep previous code for P1010RDB-PA. will add some description for P1010RDB-PA in commit message.

I don't see how this answers my question.

...

...

+uint pin_mux;

This is too generic for a global variable. Does it even need to be global?

Will rename to "static uint sd_ifc_mux", need to be global for invoking in several different functions.

If it's static, it's not global.

...

...

+#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?

previous code for debug, actually no longer needed, will remove cpld_show().

Make such cleanup a separate patch.

...

...

@@ -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?

for both PA and PB, this patch also tune for PA(though PA no longer be supported officially).

Why is it in this patch?

...

...

+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.

The patch supports two ways simultaneously:

1. mux command: for temporary use case in runtime for accessing IFC and SDHC without reboot, this way is very useful in practice and in some test cases.
2. hwconfig: for long-term use case.

This should be a separate patch from basic board support.

...

...

-#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?

No, this is for both PA and PB. old parameters were not the optimal, will add some description for P1010RDB-PA in commit message.

Separate patch.

-Scott