Patchwork [U-Boot] fw_env: use vars from the board config

login
register
mail settings
Submitter Frans Meulenbroeks
Date Jan. 2, 2012, 1:59 p.m.
Message ID <1325512756-15706-1-git-send-email-fransmeulenbroeks@gmail.com>
Download mbox | patch
Permalink /patch/133843/
State Changes Requested
Headers show

Comments

Frans Meulenbroeks - Jan. 2, 2012, 1:59 p.m.
it is quite odd that fw_printenv/fw_setenv does not
use the settings from include/configs but instead
redefines things.

This patch uses the variables from the config file
The edit in fw_env.c is only needed to resolve a name clash

Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>

---

Note: this is more intended to get some feedback.
(also to see if I am on the right track)
I did test the changes locally.

(and yes, I know there are some more things that could be cleaned up).
---
 tools/env/fw_env.c |   20 ++++++++++----------
 tools/env/fw_env.h |   36 ++++++++++++++++--------------------
 2 files changed, 26 insertions(+), 30 deletions(-)
Marek Vasut - Jan. 2, 2012, 3:11 p.m.
> it is quite odd that fw_printenv/fw_setenv does not
> use the settings from include/configs but instead
> redefines things.
> 
> This patch uses the variables from the config file
> The edit in fw_env.c is only needed to resolve a name clash
> 
> Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
> 
> ---
> 
> Note: this is more intended to get some feedback.
> (also to see if I am on the right track)
> I did test the changes locally.
> 
> (and yes, I know there are some more things that could be cleaned up).
> ---
>  tools/env/fw_env.c |   20 ++++++++++----------
>  tools/env/fw_env.h |   36 ++++++++++++++++--------------------
>  2 files changed, 26 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> index 996682e..6597fbf 100644
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -79,7 +79,7 @@ static int dev_current;
>  #define ENVSECTORS(i) envdevices[(i)].env_sectors
>  #define DEVTYPE(i)    envdevices[(i)].mtd_type
> 
> -#define CONFIG_ENV_SIZE ENVSIZE(dev_current)
> +#define CFG_ENV_SIZE ENVSIZE(dev_current)

NAK, don't change it to CFG_... for no reason! Why did you change it ? Just use 
ENVSIZE(dev_current) instead.

M

> 
>  #define ENV_SIZE      getenvsize()
> 
> @@ -216,7 +216,7 @@ static int get_config (char *);
>  #endif
>  static inline ulong getenvsize (void)
>  {
> -	ulong rc = CONFIG_ENV_SIZE - sizeof (long);
> +	ulong rc = CFG_ENV_SIZE - sizeof (long);
> 
>  	if (HaveRedundEnv)
>  		rc -= sizeof (char);
> @@ -425,7 +425,7 @@ int fw_env_write(char *name, char *value)
>  		++env;
>  	/*
>  	 * Overflow when:
> -	 * "name" + "=" + "val" +"\0\0"  > CONFIG_ENV_SIZE - (env-environment)
> +	 * "name" + "=" + "val" +"\0\0"  > CFG_ENV_SIZE - (env-environment)
>  	 */
>  	len = strlen (name) + 2;
>  	/* add '=' for first arg, ' ' for all others */
> @@ -941,7 +941,7 @@ static int flash_write (int fd_current, int fd_target,
> int dev_target) DEVOFFSET (dev_target), DEVNAME (dev_target));
>  #endif
>  	rc = flash_write_buf (dev_target, fd_target, environment.image,
> -			      CONFIG_ENV_SIZE, DEVOFFSET (dev_target),
> +			      CFG_ENV_SIZE, DEVOFFSET (dev_target),
>  			      DEVTYPE(dev_target));
>  	if (rc < 0)
>  		return rc;
> @@ -980,10 +980,10 @@ static int flash_read (int fd)
> 
>  	DEVTYPE(dev_current) = mtdinfo.type;
> 
> -	rc = flash_read_buf (dev_current, fd, environment.image, 
CONFIG_ENV_SIZE,
> +	rc = flash_read_buf (dev_current, fd, environment.image, CFG_ENV_SIZE,
>  			     DEVOFFSET (dev_current), mtdinfo.type);
> 
> -	return (rc != CONFIG_ENV_SIZE) ? -1 : 0;
> +	return (rc != CFG_ENV_SIZE) ? -1 : 0;
>  }
> 
>  static int flash_io (int mode)
> @@ -1080,11 +1080,11 @@ int fw_env_open(void)
>  	if (parse_config ())		/* should fill envdevices */
>  		return -1;
> 
> -	addr0 = calloc (1, CONFIG_ENV_SIZE);
> +	addr0 = calloc (1, CFG_ENV_SIZE);
>  	if (addr0 == NULL) {
>  		fprintf (stderr,
>  			"Not enough memory for environment (%ld bytes)\n",
> -			CONFIG_ENV_SIZE);
> +			CFG_ENV_SIZE);
>  		return -1;
>  	}
> 
> @@ -1119,11 +1119,11 @@ int fw_env_open(void)
>  		flag0 = *environment.flags;
> 
>  		dev_current = 1;
> -		addr1 = calloc (1, CONFIG_ENV_SIZE);
> +		addr1 = calloc (1, CFG_ENV_SIZE);
>  		if (addr1 == NULL) {
>  			fprintf (stderr,
>  				"Not enough memory for environment (%ld 
bytes)\n",
> -				CONFIG_ENV_SIZE);
> +				CFG_ENV_SIZE);
>  			return -1;
>  		}
>  		redundant = addr1;
> diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h
> index 2dcb373..8fcc5bd 100644
> --- a/tools/env/fw_env.h
> +++ b/tools/env/fw_env.h
> @@ -24,30 +24,26 @@
>  /*
>   * To build the utility with the run-time configuration
>   * uncomment the next line.
> - * See included "fw_env.config" sample file
> + * See included "fw_env.config" sample file (TRAB board)
>   * for notes on configuration.
>   */
> -#define CONFIG_FILE     "/etc/fw_env.config"
> +//#define CONFIG_FILE     "/etc/fw_env.config"
> 
> -#define HAVE_REDUND /* For systems with 2 env sectors */
> -#define DEVICE1_NAME      "/dev/mtd1"
> -#define DEVICE2_NAME      "/dev/mtd2"
> -#define DEVICE1_OFFSET    0x0000
> -#define ENV1_SIZE         0x4000
> -#define DEVICE1_ESIZE     0x4000
> -#define DEVICE1_ENVSECTORS     2
> -#define DEVICE2_OFFSET    0x0000
> -#define ENV2_SIZE         0x4000
> -#define DEVICE2_ESIZE     0x4000
> -#define DEVICE2_ENVSECTORS     2
> +#include "config.h"
> 
> -#define CONFIG_BAUDRATE		115200
> -#define CONFIG_BOOTDELAY	5	/* autoboot after 5 seconds	*/
> -#define CONFIG_BOOTCOMMAND							
\
> -	"bootp; "								
\
> -	"setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} "	
\
> -	"ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; "	
\
> -	"bootm"
> +#define DEVICE1_NAME      "/dev/mtd0"
> +#define DEVICE2_NAME      "/dev/mtd0"
> +#define DEVICE1_OFFSET    CONFIG_ENV_ADDR_REDUND
> +#define ENV1_SIZE         CONFIG_ENV_SIZE
> +#define DEVICE1_ESIZE     CONFIG_ENV_SECT_SIZE
> +#define DEVICE1_ENVSECTORS     1
> +#ifdef CONFIG_ENV_ADDR_REDUND
> +#define HAVE_REDUND /* For systems with 2 env sectors */
> +#define DEVICE2_OFFSET    CONFIG_ENV_ADDR_REDUND
> +#define ENV2_SIZE         CONFIG_ENV_SIZE_REDUND
> +#define DEVICE2_ESIZE     CONFIG_ENV_SECT_SIZE
> +#define DEVICE2_ENVSECTORS     1
> +#endif
> 
>  extern int   fw_printenv(int argc, char *argv[]);
>  extern char *fw_getenv  (char *name);
Marek Vasut - Jan. 2, 2012, 4:38 p.m.
Cc u-boot ML please.

> 2012/1/2 Marek Vasut <marek.vasut@gmail.com>
> 
> > > it is quite odd that fw_printenv/fw_setenv does not
> > > use the settings from include/configs but instead
> > > redefines things.
> > > 
> > > This patch uses the variables from the config file
> > > The edit in fw_env.c is only needed to resolve a name clash
> > > 
> > > Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
> > > 
> > > ---
> > > 
> > > Note: this is more intended to get some feedback.
> > > (also to see if I am on the right track)
> > > I did test the changes locally.
> > > 
> > > (and yes, I know there are some more things that could be cleaned up).
> > > ---
> > > 
> > >  tools/env/fw_env.c |   20 ++++++++++----------
> > >  tools/env/fw_env.h |   36 ++++++++++++++++--------------------
> > >  2 files changed, 26 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> > > index 996682e..6597fbf 100644
> > > --- a/tools/env/fw_env.c
> > > +++ b/tools/env/fw_env.c
> > > @@ -79,7 +79,7 @@ static int dev_current;
> > > 
> > >  #define ENVSECTORS(i) envdevices[(i)].env_sectors
> > >  #define DEVTYPE(i)    envdevices[(i)].mtd_type
> > > 
> > > -#define CONFIG_ENV_SIZE ENVSIZE(dev_current)
> > > +#define CFG_ENV_SIZE ENVSIZE(dev_current)
> > 
> > NAK, don't change it to CFG_... for no reason! Why did you change it ?
> > Just use
> > ENVSIZE(dev_current) instead.
> 
> I agree with that.
> As I wrote in the comment of the patch, this was mainly to get some
> feedback that I am on the right track.
> I've seen a number of places where the code of fw_env.c could be improved,
> but opted for minimal change for now, as for now I am mostly solicitating
> feedback on the changes in fw_env.h
> (and the actual reason for the change is that CONFIG_ENV_SIZE is defined
> here, but also in config.h, resulting in a naming config, a quick rename
> was the simplest way forward for now)

Let's see what the others think
> 
> And actually I feel that lines like:
> #define ENV1_SIZE         CONFIG_ENV_SIZE
> in fw_env.h are somewhat pointless and it would be better to eliminate
> ENV1_SIZE completely.
> 
> There are more cases like that. I'm happy to spent time on this, but only
> if it is felt to be useful and has any chance on being accepted.
> 
> Best regards, Frans

M

Patch

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 996682e..6597fbf 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -79,7 +79,7 @@  static int dev_current;
 #define ENVSECTORS(i) envdevices[(i)].env_sectors
 #define DEVTYPE(i)    envdevices[(i)].mtd_type
 
-#define CONFIG_ENV_SIZE ENVSIZE(dev_current)
+#define CFG_ENV_SIZE ENVSIZE(dev_current)
 
 #define ENV_SIZE      getenvsize()
 
@@ -216,7 +216,7 @@  static int get_config (char *);
 #endif
 static inline ulong getenvsize (void)
 {
-	ulong rc = CONFIG_ENV_SIZE - sizeof (long);
+	ulong rc = CFG_ENV_SIZE - sizeof (long);
 
 	if (HaveRedundEnv)
 		rc -= sizeof (char);
@@ -425,7 +425,7 @@  int fw_env_write(char *name, char *value)
 		++env;
 	/*
 	 * Overflow when:
-	 * "name" + "=" + "val" +"\0\0"  > CONFIG_ENV_SIZE - (env-environment)
+	 * "name" + "=" + "val" +"\0\0"  > CFG_ENV_SIZE - (env-environment)
 	 */
 	len = strlen (name) + 2;
 	/* add '=' for first arg, ' ' for all others */
@@ -941,7 +941,7 @@  static int flash_write (int fd_current, int fd_target, int dev_target)
 		DEVOFFSET (dev_target), DEVNAME (dev_target));
 #endif
 	rc = flash_write_buf (dev_target, fd_target, environment.image,
-			      CONFIG_ENV_SIZE, DEVOFFSET (dev_target),
+			      CFG_ENV_SIZE, DEVOFFSET (dev_target),
 			      DEVTYPE(dev_target));
 	if (rc < 0)
 		return rc;
@@ -980,10 +980,10 @@  static int flash_read (int fd)
 
 	DEVTYPE(dev_current) = mtdinfo.type;
 
-	rc = flash_read_buf (dev_current, fd, environment.image, CONFIG_ENV_SIZE,
+	rc = flash_read_buf (dev_current, fd, environment.image, CFG_ENV_SIZE,
 			     DEVOFFSET (dev_current), mtdinfo.type);
 
-	return (rc != CONFIG_ENV_SIZE) ? -1 : 0;
+	return (rc != CFG_ENV_SIZE) ? -1 : 0;
 }
 
 static int flash_io (int mode)
@@ -1080,11 +1080,11 @@  int fw_env_open(void)
 	if (parse_config ())		/* should fill envdevices */
 		return -1;
 
-	addr0 = calloc (1, CONFIG_ENV_SIZE);
+	addr0 = calloc (1, CFG_ENV_SIZE);
 	if (addr0 == NULL) {
 		fprintf (stderr,
 			"Not enough memory for environment (%ld bytes)\n",
-			CONFIG_ENV_SIZE);
+			CFG_ENV_SIZE);
 		return -1;
 	}
 
@@ -1119,11 +1119,11 @@  int fw_env_open(void)
 		flag0 = *environment.flags;
 
 		dev_current = 1;
-		addr1 = calloc (1, CONFIG_ENV_SIZE);
+		addr1 = calloc (1, CFG_ENV_SIZE);
 		if (addr1 == NULL) {
 			fprintf (stderr,
 				"Not enough memory for environment (%ld bytes)\n",
-				CONFIG_ENV_SIZE);
+				CFG_ENV_SIZE);
 			return -1;
 		}
 		redundant = addr1;
diff --git a/tools/env/fw_env.h b/tools/env/fw_env.h
index 2dcb373..8fcc5bd 100644
--- a/tools/env/fw_env.h
+++ b/tools/env/fw_env.h
@@ -24,30 +24,26 @@ 
 /*
  * To build the utility with the run-time configuration
  * uncomment the next line.
- * See included "fw_env.config" sample file
+ * See included "fw_env.config" sample file (TRAB board)
  * for notes on configuration.
  */
-#define CONFIG_FILE     "/etc/fw_env.config"
+//#define CONFIG_FILE     "/etc/fw_env.config"
 
-#define HAVE_REDUND /* For systems with 2 env sectors */
-#define DEVICE1_NAME      "/dev/mtd1"
-#define DEVICE2_NAME      "/dev/mtd2"
-#define DEVICE1_OFFSET    0x0000
-#define ENV1_SIZE         0x4000
-#define DEVICE1_ESIZE     0x4000
-#define DEVICE1_ENVSECTORS     2
-#define DEVICE2_OFFSET    0x0000
-#define ENV2_SIZE         0x4000
-#define DEVICE2_ESIZE     0x4000
-#define DEVICE2_ENVSECTORS     2
+#include "config.h"
 
-#define CONFIG_BAUDRATE		115200
-#define CONFIG_BOOTDELAY	5	/* autoboot after 5 seconds	*/
-#define CONFIG_BOOTCOMMAND							\
-	"bootp; "								\
-	"setenv bootargs root=/dev/nfs nfsroot=${serverip}:${rootpath} "	\
-	"ip=${ipaddr}:${serverip}:${gatewayip}:${netmask}:${hostname}::off; "	\
-	"bootm"
+#define DEVICE1_NAME      "/dev/mtd0"
+#define DEVICE2_NAME      "/dev/mtd0"
+#define DEVICE1_OFFSET    CONFIG_ENV_ADDR_REDUND
+#define ENV1_SIZE         CONFIG_ENV_SIZE
+#define DEVICE1_ESIZE     CONFIG_ENV_SECT_SIZE
+#define DEVICE1_ENVSECTORS     1
+#ifdef CONFIG_ENV_ADDR_REDUND
+#define HAVE_REDUND /* For systems with 2 env sectors */
+#define DEVICE2_OFFSET    CONFIG_ENV_ADDR_REDUND
+#define ENV2_SIZE         CONFIG_ENV_SIZE_REDUND
+#define DEVICE2_ESIZE     CONFIG_ENV_SECT_SIZE
+#define DEVICE2_ENVSECTORS     1
+#endif
 
 extern int   fw_printenv(int argc, char *argv[]);
 extern char *fw_getenv  (char *name);