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

login
register
mail settings
Submitter Sandeep Singh
Date June 13, 2013, 6:59 a.m.
Message ID <1371106791-27581-1-git-send-email-Sandeep@freescale.com>
Download mbox | patch
Permalink /patch/250986/
State Changes Requested
Delegated to: Andy Fleming
Headers show

Comments

Sandeep Singh - June 13, 2013, 6:59 a.m.
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(-)
Scott Wood - June 13, 2013, 4:39 p.m.
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

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

> +	/*
> +	 * 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

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?

-Scott
Sandeep Singh - June 14, 2013, 5:26 a.m.
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@lists.denx.de; Singh Sandeep-B37400; afleming@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
Sandeep Singh - July 2, 2013, 10:01 a.m.
Sorry somehow I missed this earlier. Please find my reply inline.

Regards,
Sandeep


> -----Original Message-----
> From: Wood Scott-B07421
> Sent: 15 June 2013 01:09
> To: Singh Sandeep-B37400
> Cc: Wood Scott-B07421; u-boot@lists.denx.de; afleming@gmail.com
> Subject: Re: [U-Boot] [PATCH] powerpc/mpc85xx: Enabling CPC conditionally
> based on hwconfig options
> 
> On 06/14/2013 12:26:18 AM, Singh Sandeep-B37400 wrote:
> > 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;
> > }
> 
> OK, I see.
> 
> What is the use case for enabling one cpc but not another?  What is the
> use case for disabling cpc at all?  The answer should go in the commit
> message.

In B4860, Starcore owns one CPC and second CPC is owned by PowerPC. So we need to enable only one CPC at u-boot.

> 
> -Scott

Patch

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;
+	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;
+	}
+
+	/*
+	 * 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;
 
 	for (i = 0; i < CONFIG_SYS_NUM_CPC; i++, cpc++) {
+		sprintf(cpc_subarg, "cpc%u", i + 1);
+		if (have_hwconfig) {
+			cpc_args = hwconfig_sub_f("en_cpc", cpc_subarg, buffer);
+			if (cpc_args == NULL)
+				continue;
+		}
+
 		u32 cpccfg0 = in_be32(&cpc->cpccfg0);
 		size += CPC_CFG0_SZ_K(cpccfg0);
 #if defined(CONFIG_RAMBOOT_PBL) || defined(CONFIG_SECURE_HKAREA_CPC)