On Wed, 2013-09-11 at 00:58 -0500, Liu Shengzhou-B36685 wrote:

Scott, please review new version http://patchwork.ozlabs.org/patch/273539/

...

-----Original Message----- From: Wood Scott-B07421 Sent: Wednesday, September 04, 2013 11:57 PM To: Liu Shengzhou-B36685 Cc: Wood Scott-B07421; u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH] powerpc/p1010rdb-pb: add support for p1010rdb-pb board

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.

Ever PMO team said to remove the code/document of old P1010RDB-PA.

I don't know who PMO is, but they don't dictate what a community project such as U-Boot does. If there is a good argument to make for dropping PA (such as talking about how limited the distribution of the boards has been so far, though I was not under the impression that it was particularly limited), then make that argument. Otherwise, we're keeping PA support and it's irrelevant that some people in Freescale don't care about it any more (except for York, of course).

But I prefer to adhere to reserve code support for old board.

Then what's up with statements like, "we will no longer product and maintain old board, the change can be regarded as for new 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.

I meant new version of this patch would be updated with subject and commit description to cover all the changes of both new and old boards instead of separating patch.

No, please keep PB support separate from anything intended to change how PA works.

...

...

...

...

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

Use 'static' to limits it as global just in the scope of this module.

My point is that the term "global" usually means program-wide rather than file-scope, especially when talking about namespacing.

...

...

...

...

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

new version of this patch has covered all the changes. I don't think it's really necessary as a separate patch.

I disagree and will not review it in this state (the point is to make it easier to review -- both now and during future code archaeology). If York is happy with it like this, then that's between the two of you...

...

...

...

...

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

Yes, better as a separate patch, but it's not really necessary to split it, we will no longer product and maintain old board, the change can be regarded as for new board.

Huh? I don't see what this has to do with PA versus PB at all. It's not even really about p1010rdb at all. It's a new feature for controlling p1010 pin muxing.

-Scott