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

[Scott Wood]

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.

> 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