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

[Scott Wood]

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