| Message ID | 1325512756-15706-1-git-send-email-fransmeulenbroeks@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
> 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);
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
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);
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(-)