Re: [U-Boot] [PATCH] NAND: Support dynamic location of enviromnent (CONFIG_ENV_OFFSET_OOB)
On Mon, May 17, 2010 at 05:04:30PM -0400, Ben Gardiner wrote:
diff --git a/common/cmd_dynenv.c b/common/cmd_dynenv.c
new file mode 100644
index 000..5167875
--- /dev/null
+++ b/common/cmd_dynenv.c
@@ -0,0 +1,112 @@
+/*
+ * (C) Copyright 2006-2007 OpenMoko, Inc.
+ * Author: Harald Welte lafo...@openmoko.org
+ * (C) Copyright 2008 Harald Welte lafo...@openmoko.org
Is this correct and up-to-date?
+unsigned long env_offset;
This is a pretty generic name for something NAND-specific -- as is dynenv.
Maybe this should be a nand subcommand? Putting it in cmd_nand.c would also
eliminate the need to export arg_off_size().
+ if(mtd-oobavail CONFIG_ENV_OFFSET_SIZE){
Please put a space after if and before the opening brace (in fact, there's
no need for the braces at all on this one-liner).
+ printf(Insufficient available OOB bytes: %d OOB bytes
available but %d required for dynenv support\n,mtd-oobavail,8);
Keep lines under 80 columns (both in source code and in output), and put
spaces after commas.
+ }
+
+ oob_buf = malloc(mtd-oobsize);
+ if(!oob_buf)
+ return -ENOMEM;
Let the user know it didn't work?
You only really need 8 bytes, why not just put it on the stack? Likewise in
get_dynenv().
+ ops.datbuf = NULL;
+ ops.mode = MTD_OOB_AUTO;
+ ops.ooboffs = 0;
+ ops.ooblen = CONFIG_ENV_OFFSET_SIZE;
+ ops.oobbuf = (void *) oob_buf;
+
+ ret = mtd-read_oob(mtd, CONFIG_ENV_OFFSET_SIZE, ops);
+ oob_buf[0] = ENV_OOB_MARKER;
+
+ if (!ret) {
+ if(addr ~oob_buf[1]) {
+ printf(ERROR: erase OOB block 0 to
+ write this value\n);
You cannot erase OOB without erasing the entire block, so this message
is a little confusing.
Do you really expect to make use of the ability to set a new address
without erasing, if it only clears bits?
+ ret = mtd-write_oob(mtd, CONFIG_ENV_OFFSET_SIZE, ops);
+ if (!ret)
+ CONFIG_ENV_OFFSET = addr;
+ else {
+ printf(Error writing OOB block 0\n);
+ goto fail;
+ }
If you put braces on one side of the else, put them on both.
+
+ free(oob_buf);
+ } else
+ goto usage;
Likewise.
Is there anything you can do on dynenv set so that the user won't have to
reboot after setting the dynenv to be able to saveenv into the new
environment?
+U_BOOT_CMD(dynenv, 4, 1, do_dynenv,
+ dynenv - dynamically placed (NAND) environment,
+ set off- set enviromnent offset\n
+ dynenv get - get environment offset);
\ No newline at end of file
Please put a newline at the end of the file.
diff --git a/common/cmd_nand.h b/common/cmd_nand.h
new file mode 100644
index 000..023ed4f
--- /dev/null
+++ b/common/cmd_nand.h
@@ -0,0 +1,33 @@
+/*
+ * cmd_nand.h - Convenience functions
+ *
+ * (C) Copyright 2006-2007 OpenMoko, Inc.
+ * Author: Werner Almesberger wer...@openmoko.org
Is this really the right copyright/authorship for this file?
+#ifndef CMD_NAND_H
+#define CMD_NAND_H
+
+#include nand.h
+
+
+/* common/cmd_nand.c */
+int nand_arg_off_size(int argc, char *argv[], nand_info_t *nand, ulong *off,
+ ulong *size, int net);
Just put it in nand.h.
+ if(!ret) {
+ if(oob_buf[0] == ENV_OOB_MARKER) {
+ *result = oob_buf[1];
Should probably encode the environment location as a block number, rather
than as a byte, for flashes larger than 4GiB (there are other places in the
environment handling where this won't work, but let's not add more).
+ }
+ else {
} else {
#ifdef CONFIG_ENV_OFFSET_REDUND
void env_relocate_spec (void)
{
@@ -357,12 +398,23 @@ void env_relocate_spec (void)
#if !defined(ENV_IS_EMBEDDED)
int ret;
+#if defined(CONFIG_ENV_OFFSET_OOB)
+ ret = get_dynenv(CONFIG_ENV_OFFSET);
Taking the address of a macro looks really weird. This will only work if
the macro is defined as env_offset, so why not just use env_offset?
ret = readenv(CONFIG_ENV_OFFSET, (u_char *) env_ptr);
if (ret)
return use_default();
if (crc32(0, env_ptr-data, ENV_SIZE) != env_ptr-crc)
return use_default();
+
#endif /* ! ENV_IS_EMBEDDED */
Unrelated whitespace change, please leave out.
#endif /* CONFIG_ENV_OFFSET_REDUND */
diff --git a/include/environment.h b/include/environment.h
index b9924fd..03b6c92 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -74,15 +74,24 @@
#endif /* CONFIG_ENV_IS_IN_FLASH */
#if defined(CONFIG_ENV_IS_IN_NAND)
-# ifndef CONFIG_ENV_OFFSET
-# error Need to define CONFIG_ENV_OFFSET when using CONFIG_ENV_IS_IN_NAND
-# endif
+# if
Re: [U-Boot] [PATCH] NAND: Support dynamic location of enviromnent (CONFIG_ENV_OFFSET_OOB)
Thank you for the thorough review, Scott.
On Wed, May 26, 2010 at 6:58 PM, Scott Wood scottw...@freescale.com wrote:
On Mon, May 17, 2010 at 05:04:30PM -0400, Ben Gardiner wrote:
diff --git a/common/cmd_dynenv.c b/common/cmd_dynenv.c
new file mode 100644
index 000..5167875
--- /dev/null
+++ b/common/cmd_dynenv.c
@@ -0,0 +1,112 @@
+/*
+ * (C) Copyright 2006-2007 OpenMoko, Inc.
+ * Author: Harald Welte lafo...@openmoko.org
+ * (C) Copyright 2008 Harald Welte lafo...@openmoko.org
Is this correct and up-to-date?
The code was taken from Harald's patches he submitted to the mailing
list. I didn't do much to them except to re-factor the 'get'
functionality. Should I have added myself / my organization to the
copyright list? I figured my changes were not enough to warrant the
additional attribution of copyright.
+unsigned long env_offset;
This is a pretty generic name for something NAND-specific -- as is dynenv.
Maybe this should be a nand subcommand? Putting it in cmd_nand.c would also
eliminate the need to export arg_off_size().
I'm glad you brought this up. I was wondering if the command might be
better suited under the nand command -- I agree that it would
eliminate the export of arg_off_size and all the renames, which would
be very nice. Would 'nand dynenv' do? How about 'nand env.oob' ?
+ if(mtd-oobavail CONFIG_ENV_OFFSET_SIZE){
Please put a space after if and before the opening brace (in fact, there's
no need for the braces at all on this one-liner).
will do
+ printf(Insufficient available OOB bytes: %d OOB bytes
available but %d required for dynenv support\n,mtd-oobavail,8);
Keep lines under 80 columns (both in source code and in output), and put
spaces after commas.
sorry about that, I should know better :)
+ }
+
+ oob_buf = malloc(mtd-oobsize);
+ if(!oob_buf)
+ return -ENOMEM;
Let the user know it didn't work?
You only really need 8 bytes, why not just put it on the stack? Likewise in
get_dynenv().
good ideas
+ ops.datbuf = NULL;
+ ops.mode = MTD_OOB_AUTO;
+ ops.ooboffs = 0;
+ ops.ooblen = CONFIG_ENV_OFFSET_SIZE;
+ ops.oobbuf = (void *) oob_buf;
+
+ ret = mtd-read_oob(mtd, CONFIG_ENV_OFFSET_SIZE, ops);
+ oob_buf[0] = ENV_OOB_MARKER;
+
+ if (!ret) {
+ if(addr ~oob_buf[1]) {
+ printf(ERROR: erase OOB block 0 to
+ write this value\n);
You cannot erase OOB without erasing the entire block, so this message
is a little confusing.
Do you really expect to make use of the ability to set a new address
without erasing, if it only clears bits?
No, I suppose not. The main use-case seems to be 'dynenv set' on a
freshly erased nand when using u-boot for programming. OTOH I have
noticed that (one of) the nand erase utility that comes with the TI
OMAP L138 EVM erases the NAND w/o erasing the OOB; so it was useful
during testing to know if I was able to write the correct value to the
OOB. I could replace this with a read-back check to accomplish the
same task -- this will also make 'set' commit the new offset 'live'
(see below)
+ ret = mtd-write_oob(mtd, CONFIG_ENV_OFFSET_SIZE, ops);
+ if (!ret)
+ CONFIG_ENV_OFFSET = addr;
+ else {
+ printf(Error writing OOB block 0\n);
+ goto fail;
+ }
If you put braces on one side of the else, put them on both.
+
+ free(oob_buf);
+ } else
+ goto usage;
Likewise.
will do
Is there anything you can do on dynenv set so that the user won't have to
reboot after setting the dynenv to be able to saveenv into the new
environment?
Good catch. Yes I think this could also be handled by replacing the
OOB bit-check with a call and check of the 'dynenv get' command after
set. In this way the global variable that has been swapped inplace of
CONFIG_ENV_OFFSET will have the value of the newly set dynamic
environment.
+U_BOOT_CMD(dynenv, 4, 1, do_dynenv,
+ dynenv - dynamically placed (NAND) environment,
+ set off - set enviromnent offset\n
+ dynenv get - get environment offset);
\ No newline at end of file
Please put a newline at the end of the file.
sorry I really should have caught that one.
diff --git a/common/cmd_nand.h b/common/cmd_nand.h
new file mode 100644
index 000..023ed4f
--- /dev/null
+++ b/common/cmd_nand.h
@@ -0,0 +1,33 @@
+/*
+ * cmd_nand.h - Convenience functions
+ *
+ * (C) Copyright 2006-2007 OpenMoko, Inc.
+ * Author: Werner Almesberger wer...@openmoko.org
Is this really the right copyright/authorship for this file?
see answer above; but this file will be removed in the next version
since I will stick the dynenv command under the nand
