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

Singh Sandeep-B37400
2013-06-14T05:26:18Z

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