[U-Boot] [PATCH] powerpc/mpc85xx: Enabling CPC conditionally based on hwconfig options

[Singh Sandeep-B37400]

Thanks for your comments.
Please find reply inline.

Regards,
Sandeep

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, June 13, 2013 10:09 PM
> To: Singh Sandeep-B37400
> Cc: u-boot at lists.denx.de; Singh Sandeep-B37400; afleming at gmail.com
> Subject: Re: [U-Boot] [PATCH] powerpc/mpc85xx: Enabling CPC conditionally
> based on hwconfig options
> 
> On 06/13/2013 01:59:51 AM, Sandeep Singh wrote:
> > If hwconfig does not contains "en_cpc" then by default all cpcs are
> > enabled If this config is defined then only those individual cpcs
> > which are defined in the subargument of "en_cpc" will be enabled e.g
> > en_cpc:cpc1,cpc2; (this will enable cpc1 and cpc2) or en_cpc:cpc2;
> > (this enables just cpc2)
> >
> > Signed-off-by: Sandeep Singh <Sandeep at freescale.com>
> > ---
> >  arch/powerpc/cpu/mpc85xx/cpu_init.c |   32
> > ++++++++++++++++++++++++++++++++
> >  1 files changed, 32 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init.c
> > b/arch/powerpc/cpu/mpc85xx/cpu_init.c
> > index 185e0d5..ea75ce5 100644
> > --- a/arch/powerpc/cpu/mpc85xx/cpu_init.c
> > +++ b/arch/powerpc/cpu/mpc85xx/cpu_init.c
> > @@ -159,11 +159,43 @@ void config_8560_ioports (volatile ccsr_cpm_t *
> > cpm)
> >  static void enable_cpc(void)
> >  {
> >  	int i;
> > +	int arglen;
> > +	int ret;
> >  	u32 size = 0;
> >
> >  	cpc_corenet_t *cpc = (cpc_corenet_t *)CONFIG_SYS_FSL_CPC_ADDR;
> > +	char buffer[HWCONFIG_BUFFER_SIZE];
> > +	char cpc_subarg[16];
> > +	bool have_hwconfig = 0;
> 
> = false
Ok
> 
> > +	const char *cpc_args = NULL;
> > +
> > +	/*
> > +	 * Extract hwconfig from environment since environment
> > +	 * is not setup properly yet
> > +	 */
> > +	ret = getenv_f("hwconfig", buffer, sizeof(buffer));
> > +	if (ret == -1) {
> > +		printf("Error getting hwconfig\n");
> > +		return;
> > +	}
> 
> It is not an error for hwconfig to be missing.  That just means that no
> options are set.
Right, will rectify
> 
> > +	/*
> > +	 * If "en_cpc" is not defined in hwconfig then by default all
> > +	 * cpcs are enable. If this config is defined then individual
> > +	 * cpcs which have to be enabled should also be defined.
> > +	 * e.g en_cpc:cpc1,cpc2;
> > +	 */
> > +	if (hwconfig_f("en_cpc", buffer))
> > +		have_hwconfig = 1;
> 
> = true
ok
> 
> have_hwconfig should be set based on the return of getenv_f.  Is there
> really any benefit to checking whether en_cpc is present, separately from
> the hwconfig_sub_f call?
This was done with the intension of providing greater configurability. When
en_cpc is defined then it's entirely up to user to decide which cpcs are to
be enabled. Hence we do following:

if_defined("en_cpc"){
	only_then
		check_for_cpc_options;
} else {
	enable_all_cpc;
}

> 
> -Scott