[U-Boot] [PATCH 1/4] Optimized nand_read_buf for kirkwood

[U-Boot] [PATCH 1/4] Optimized nand_read_buf for kirkwood

[Phil Sutter]

The basic idea is taken from the linux-kernel, but further optimized.

First align the buffer to 8 bytes, then use ldrd/strd to read and store
in 8 byte quantities, then do the final bytes.

Signed-off-by: Nico Erfurth <ne@erfurth.eu>
Cc: Prafulla Wadaskar <prafulla@marvell.com>
---
 drivers/mtd/nand/kirkwood_nand.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/kirkwood_nand.c b/drivers/mtd/nand/kirkwood_nand.c
index bdab5aa..e04a59f 100644
--- a/drivers/mtd/nand/kirkwood_nand.c
+++ b/drivers/mtd/nand/kirkwood_nand.c
@@ -38,6 +38,34 @@ struct kwnandf_registers {
 static struct kwnandf_registers *nf_reg =
 	(struct kwnandf_registers *)KW_NANDF_BASE;
 
+
+/* The basic idea is stolen from the linux kernel, but the inner loop is optimized a bit more */
+static void kw_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+
+	while (len && (unsigned long)buf & 7)
+	{
+		*buf++ = readb(chip->IO_ADDR_R);
+		len--;
+	};
+
+	asm volatile (
+		".LFlashLoop:\n"
+		"  subs\t%0, #8\n"
+		"  ldrpld\tr2, [%2]\n" // Read 2 words
+		"  strpld\tr2, [%1], #8\n" // Read 2 words
+		"  bpl\t.LFlashLoop\n" // This results in one additional loop if len%8 <> 0
+		"  addne\t%0, #8\n"
+		: "+&r" (len), "+&r" (buf)
+		: "r" (chip->IO_ADDR_R)
+		: "r2", "r3", "memory", "cc"
+	);
+
+	while (len--)
+		*buf++ = readb(chip->IO_ADDR_R);
+}
+
 /*
  * hardware specific access to control-lines/bits
  */
@@ -76,6 +104,7 @@ int board_nand_init(struct nand_chip *nand)
 	nand->options = NAND_COPYBACK | NAND_CACHEPRG | NAND_NO_PADDING;
 	nand->ecc.mode = NAND_ECC_SOFT;
 	nand->cmd_ctrl = kw_nand_hwcontrol;
+	nand->read_buf = kw_nand_read_buf;
 	nand->chip_delay = 40;
 	nand->select_chip = kw_nand_select_chip;
 	return 0;
-- 
1.7.3.4

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Phil Sutter]

Without this patch, when the currently chosen environment to be written
has bad blocks, saveenv fails completely. Instead, when there is
redundant environment fall back to the other copy. Environment reading
needs no adjustment, as the fallback logic for incomplete writes applies
to this case as well.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 common/env_nand.c |  100 ++++++++++++++++++++++------------------------------
 1 files changed, 42 insertions(+), 58 deletions(-)

diff --git a/common/env_nand.c b/common/env_nand.c
index 79e8033..895532b 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -168,72 +168,53 @@ int writeenv(size_t offset, u_char *buf)
 	return 0;
 }
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
-static unsigned char env_flags;
+typedef struct env_location {
+	const char *name;
+	nand_erase_options_t *erase_opts;
+	loff_t offset;
+} env_location_t;
 
-int saveenv(void)
+static int erase_and_write_env(env_location_t *location, u_char *env_new)
 {
-	env_t	env_new;
-	ssize_t	len;
-	char	*res;
-	int	ret = 0;
-	nand_erase_options_t nand_erase_options;
-
-	memset(&nand_erase_options, 0, sizeof(nand_erase_options));
-	nand_erase_options.length = CONFIG_ENV_RANGE;
+	int ret = 0;
 
-	if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE)
+	printf("Erasing %s...\n", location->name);
+	location->erase_opts->offset = location->offset;
+	if (nand_erase_opts(&nand_info[0], location->erase_opts))
 		return 1;
 
-	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL);
-	if (len < 0) {
-		error("Cannot export environment: errno = %d\n", errno);
-		return 1;
-	}
-	env_new.crc	= crc32(0, env_new.data, ENV_SIZE);
-	env_new.flags	= ++env_flags; /* increase the serial */
-
-	if (gd->env_valid == 1) {
-		puts("Erasing redundant NAND...\n");
-		nand_erase_options.offset = CONFIG_ENV_OFFSET_REDUND;
-		if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-			return 1;
-
-		puts("Writing to redundant NAND... ");
-		ret = writeenv(CONFIG_ENV_OFFSET_REDUND, (u_char *)&env_new);
-	} else {
-		puts("Erasing NAND...\n");
-		nand_erase_options.offset = CONFIG_ENV_OFFSET;
-		if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-			return 1;
-
-		puts("Writing to NAND... ");
-		ret = writeenv(CONFIG_ENV_OFFSET, (u_char *)&env_new);
-	}
-	if (ret) {
+	printf("Writing to %s... ", location->name);
+	if ((ret = writeenv(location->offset, env_new)))
 		puts("FAILED!\n");
-		return 1;
-	}
-
-	puts("done\n");
-
-	gd->env_valid = gd->env_valid == 2 ? 1 : 2;
 
 	return ret;
 }
-#else /* ! CONFIG_ENV_OFFSET_REDUND */
+
+static unsigned char env_flags;
+
 int saveenv(void)
 {
 	int	ret = 0;
 	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
 	ssize_t	len;
 	char	*res;
+	int	env_idx;
 	nand_erase_options_t nand_erase_options;
+	env_location_t location[] = {{
+		.name = "NAND",
+		.erase_opts = &nand_erase_options,
+		.offset = CONFIG_ENV_OFFSET,
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	}, {
+		.name = "redundant NAND",
+		.erase_opts = &nand_erase_options,
+		.offset = CONFIG_ENV_OFFSET_REDUND,
+#endif
+	}};
+
 
 	memset(&nand_erase_options, 0, sizeof(nand_erase_options));
 	nand_erase_options.length = CONFIG_ENV_RANGE;
-	nand_erase_options.offset = CONFIG_ENV_OFFSET;
 
 	if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE)
 		return 1;
@@ -244,22 +225,25 @@ int saveenv(void)
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;
 	}
-	env_new->crc = crc32(0, env_new->data, ENV_SIZE);
-
-	puts("Erasing Nand...\n");
-	if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-		return 1;
+	env_new->crc   = crc32(0, env_new->data, ENV_SIZE);
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	env_new->flags = ++env_flags; /* increase the serial */
+	env_idx = (gd->env_valid == 1);
+#endif
 
-	puts("Writing to Nand... ");
-	if (writeenv(CONFIG_ENV_OFFSET, (u_char *)env_new)) {
-		puts("FAILED!\n");
-		return 1;
-	}
+	ret = erase_and_write_env(&location[env_idx], (u_char *)env_new);
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	if (ret) {
+		env_idx = (env_idx + 1) & 1;
+		ret = erase_and_write_env(&location[env_idx],
+				(u_char *)env_new);
+	} else
+		gd->env_valid = gd->env_valid == 2 ? 1 : 2;
+#endif
 
 	puts("done\n");
 	return ret;
 }
-#endif /* CONFIG_ENV_OFFSET_REDUND */
 #endif /* CMD_SAVEENV */
 
 int readenv(size_t offset, u_char *buf)
-- 
1.7.3.4

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Scott Wood]

On 11/21/2012 06:59:19 AM, Phil Sutter wrote:
> Without this patch, when the currently chosen environment to be  
> written
> has bad blocks, saveenv fails completely. Instead, when there is
> redundant environment fall back to the other copy. Environment reading
> needs no adjustment, as the fallback logic for incomplete writes  
> applies
> to this case as well.
> 
> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>

Isn't this what CONFIG_ENV_RANGE is supposed to deal with?

Redundant environment is to deal with other problems such as a power  
failure during saveenv.  If you just fall back to the other copy,  
you're silently losing the redundancy.

-Scott

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Phil Sutter]

Hi,

On Tue, Nov 27, 2012 at 04:04:15PM -0600, Scott Wood wrote:
> On 11/21/2012 06:59:19 AM, Phil Sutter wrote:
> > Without this patch, when the currently chosen environment to be  
> > written
> > has bad blocks, saveenv fails completely. Instead, when there is
> > redundant environment fall back to the other copy. Environment reading
> > needs no adjustment, as the fallback logic for incomplete writes  
> > applies
> > to this case as well.
> > 
> > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> 
> Isn't this what CONFIG_ENV_RANGE is supposed to deal with?

Yes, that is more or less what is supposed to help for cases like this.
But given the fact that CONFIG_ENV_RANGE needs to span multiple erase
pages which in our case are 128k in size, this is quite a deal.
Especially since one needs to have multiple pages for both normal and
redundant environment to be really sure.

But, we already have a fixed hashmap in field, so using CONFIG_ENV_RANGE
is simply no option.

> Redundant environment is to deal with other problems such as a power  
> failure during saveenv.  If you just fall back to the other copy,  
> you're silently losing the redundancy.

The alternative to silently losing redundancy in case one of the blocks
in either normal or redundant env areas turns bad is to not being able
to save the environment at all anymore. I'd prefer dropping the
redundancy but still having a working system then. ;)

Best wishes and thanks for the review,

Phil Sutter
Software Engineer

-- 
Viprinet Europe GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:               +49 6721 49030-0
Direct line/Durchwahl:        +49 6721 49030-134
Fax:                          +49 6721 49030-109

phil.sutter at viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein, Germany
Commercial register/Handelsregister: Amtsgericht Mainz HRB44090
CEO/Gesch?ftsf?hrer: Simon Kissel

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Scott Wood]

On 11/28/2012 03:06:00 PM, Phil Sutter wrote:
> Hi,
> 
> On Tue, Nov 27, 2012 at 04:04:15PM -0600, Scott Wood wrote:
> > On 11/21/2012 06:59:19 AM, Phil Sutter wrote:
> > > Without this patch, when the currently chosen environment to be
> > > written
> > > has bad blocks, saveenv fails completely. Instead, when there is
> > > redundant environment fall back to the other copy. Environment  
> reading
> > > needs no adjustment, as the fallback logic for incomplete writes
> > > applies
> > > to this case as well.
> > >
> > > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> >
> > Isn't this what CONFIG_ENV_RANGE is supposed to deal with?
> 
> Yes, that is more or less what is supposed to help for cases like  
> this.
> But given the fact that CONFIG_ENV_RANGE needs to span multiple erase
> pages which in our case are 128k in size, this is quite a deal.
> Especially since one needs to have multiple pages for both normal and
> redundant environment to be really sure.

And *that* is what CONFIG_ENV_OFFSET_OOB is supposed to deal with. :-)

Though at the moment redundant environment is not supported with  
CONFIG_ENV_OFFSET_OOB.

> But, we already have a fixed hashmap in field, so using  
> CONFIG_ENV_RANGE
> is simply no option.

That's relevant to what you put in your product, but it shouldn't be  
the basis of how mainline U-Boot does things for all boards.

> > Redundant environment is to deal with other problems such as a power
> > failure during saveenv.  If you just fall back to the other copy,
> > you're silently losing the redundancy.
> 
> The alternative to silently losing redundancy in case one of the  
> blocks
> in either normal or redundant env areas turns bad is to not being able
> to save the environment at all anymore. I'd prefer dropping the
> redundancy but still having a working system then. ;)

Another alternative is to noisily lose redundancy.

-Scott

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Phil Sutter]

Scott,

On Thu, Dec 06, 2012 at 12:18:39PM -0600, Scott Wood wrote:
> On 11/28/2012 03:06:00 PM, Phil Sutter wrote:
> > Hi,
> > 
> > On Tue, Nov 27, 2012 at 04:04:15PM -0600, Scott Wood wrote:
> > > On 11/21/2012 06:59:19 AM, Phil Sutter wrote:
> > > > Without this patch, when the currently chosen environment to be
> > > > written
> > > > has bad blocks, saveenv fails completely. Instead, when there is
> > > > redundant environment fall back to the other copy. Environment  
> > reading
> > > > needs no adjustment, as the fallback logic for incomplete writes
> > > > applies
> > > > to this case as well.
> > > >
> > > > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> > >
> > > Isn't this what CONFIG_ENV_RANGE is supposed to deal with?
> > 
> > Yes, that is more or less what is supposed to help for cases like  
> > this.
> > But given the fact that CONFIG_ENV_RANGE needs to span multiple erase
> > pages which in our case are 128k in size, this is quite a deal.
> > Especially since one needs to have multiple pages for both normal and
> > redundant environment to be really sure.
> 
> And *that* is what CONFIG_ENV_OFFSET_OOB is supposed to deal with. :-)

Good to know, I already wondered what exactly this option is there for.

> Though at the moment redundant environment is not supported with  
> CONFIG_ENV_OFFSET_OOB.

I will have a look at it now, because that could be an elegant solution
for both of us I think.

> > But, we already have a fixed hashmap in field, so using  
> > CONFIG_ENV_RANGE
> > is simply no option.
> 
> That's relevant to what you put in your product, but it shouldn't be  
> the basis of how mainline U-Boot does things for all boards.

Yes, of course. That was merely me explaining myself, not so much of a
real point in matters of what should go mainline and what not.

> > > Redundant environment is to deal with other problems such as a power
> > > failure during saveenv.  If you just fall back to the other copy,
> > > you're silently losing the redundancy.
> > 
> > The alternative to silently losing redundancy in case one of the  
> > blocks
> > in either normal or redundant env areas turns bad is to not being able
> > to save the environment at all anymore. I'd prefer dropping the
> > redundancy but still having a working system then. ;)
> 
> Another alternative is to noisily lose redundancy.

Would that be an acceptable alternative from your point of view? Having
CONFIG_ENV_OFFSET_OOB in mind, there may be situations where all blocks
of either one of the redundant environments get bad (quite artificial, I
know). Losing redundancy in exchange for keeping env writeability could
still be an option then. What do you think?

Best wishes,

Phil Sutter
Software Engineer

-- 
Viprinet Europe GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:               +49 6721 49030-0
Direct line/Durchwahl:        +49 6721 49030-134
Fax:                          +49 6721 49030-109

phil.sutter at viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein, Germany
Commercial register/Handelsregister: Amtsgericht Mainz HRB44090
CEO/Gesch?ftsf?hrer: Simon Kissel

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Phil Sutter]

Scott,

On Fri, Dec 07, 2012 at 12:53:34PM +0100, Phil Sutter wrote:
> On Thu, Dec 06, 2012 at 12:18:39PM -0600, Scott Wood wrote:
> > On 11/28/2012 03:06:00 PM, Phil Sutter wrote:
> > > Hi,
> > > 
> > > On Tue, Nov 27, 2012 at 04:04:15PM -0600, Scott Wood wrote:
> > > > On 11/21/2012 06:59:19 AM, Phil Sutter wrote:
> > > > > Without this patch, when the currently chosen environment to be
> > > > > written
> > > > > has bad blocks, saveenv fails completely. Instead, when there is
> > > > > redundant environment fall back to the other copy. Environment  
> > > reading
> > > > > needs no adjustment, as the fallback logic for incomplete writes
> > > > > applies
> > > > > to this case as well.
> > > > >
> > > > > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> > > >
> > > > Isn't this what CONFIG_ENV_RANGE is supposed to deal with?
> > > 
> > > Yes, that is more or less what is supposed to help for cases like  
> > > this.
> > > But given the fact that CONFIG_ENV_RANGE needs to span multiple erase
> > > pages which in our case are 128k in size, this is quite a deal.
> > > Especially since one needs to have multiple pages for both normal and
> > > redundant environment to be really sure.
> > 
> > And *that* is what CONFIG_ENV_OFFSET_OOB is supposed to deal with. :-)
> 
> Good to know, I already wondered what exactly this option is there for.

Hmm. Does not look like CONFIG_ENV_OFFSET_OOB is used to select the
block(s) within the erase page to save the environment. Looking at
common/env_nand.c:318, the environment offset saved in the OOB seems to
be in erase page unit.

On the other hand, I could not find code that alters this setting based
on bad blocks found or whatever. This seems to simply be an alternative
to setting CONFIG_ENV_OFFSET at build-time. 

Best wishes,

Phil Sutter
Software Engineer

-- 
Viprinet Europe GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:               +49 6721 49030-0
Direct line/Durchwahl:        +49 6721 49030-134
Fax:                          +49 6721 49030-109

phil.sutter at viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein, Germany
Commercial register/Handelsregister: Amtsgericht Mainz HRB44090
CEO/Gesch?ftsf?hrer: Simon Kissel

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Scott Wood]

On 12/07/2012 10:58:53 AM, Phil Sutter wrote:
> Scott,
> 
> On Fri, Dec 07, 2012 at 12:53:34PM +0100, Phil Sutter wrote:
> > On Thu, Dec 06, 2012 at 12:18:39PM -0600, Scott Wood wrote:
> > > On 11/28/2012 03:06:00 PM, Phil Sutter wrote:
> > > > Hi,
> > > >
> > > > On Tue, Nov 27, 2012 at 04:04:15PM -0600, Scott Wood wrote:
> > > > > On 11/21/2012 06:59:19 AM, Phil Sutter wrote:
> > > > > > Without this patch, when the currently chosen environment  
> to be
> > > > > > written
> > > > > > has bad blocks, saveenv fails completely. Instead, when  
> there is
> > > > > > redundant environment fall back to the other copy.  
> Environment
> > > > reading
> > > > > > needs no adjustment, as the fallback logic for incomplete  
> writes
> > > > > > applies
> > > > > > to this case as well.
> > > > > >
> > > > > > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> > > > >
> > > > > Isn't this what CONFIG_ENV_RANGE is supposed to deal with?
> > > >
> > > > Yes, that is more or less what is supposed to help for cases  
> like
> > > > this.
> > > > But given the fact that CONFIG_ENV_RANGE needs to span multiple  
> erase
> > > > pages which in our case are 128k in size, this is quite a deal.
> > > > Especially since one needs to have multiple pages for both  
> normal and
> > > > redundant environment to be really sure.
> > >
> > > And *that* is what CONFIG_ENV_OFFSET_OOB is supposed to deal  
> with. :-)
> >
> > Good to know, I already wondered what exactly this option is there  
> for.
> 
> Hmm. Does not look like CONFIG_ENV_OFFSET_OOB is used to select the
> block(s) within the erase page to save the environment. Looking at
> common/env_nand.c:318, the environment offset saved in the OOB seems  
> to
> be in erase page unit.

I'm not sure what you mean by "block(s) within the erase page" --  
blocks are the unit of erasing, and of bad block marking.

The block to hold the environment is stored in the OOB of block zero,  
which is usually guaranteed to not be bad.

> On the other hand, I could not find code that alters this setting  
> based
> on bad blocks found or whatever. This seems to simply be an  
> alternative
> to setting CONFIG_ENV_OFFSET at build-time.

It is set by the "nand env.oob" command.  It is currently a manual  
process (or rather, automation is left to the user's board preparation  
process rather than being built into U-Boot), as U-Boot wouldn't know  
how to give back unused blocks to other purposes.

-Scott

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Phil Sutter]

On Fri, Dec 07, 2012 at 11:38:11AM -0600, Scott Wood wrote:
> On 12/07/2012 10:58:53 AM, Phil Sutter wrote:
> > Scott,
> > 
> > On Fri, Dec 07, 2012 at 12:53:34PM +0100, Phil Sutter wrote:
> > > On Thu, Dec 06, 2012 at 12:18:39PM -0600, Scott Wood wrote:
> > > > On 11/28/2012 03:06:00 PM, Phil Sutter wrote:
> > > > > Hi,
> > > > >
> > > > > On Tue, Nov 27, 2012 at 04:04:15PM -0600, Scott Wood wrote:
> > > > > > On 11/21/2012 06:59:19 AM, Phil Sutter wrote:
> > > > > > > Without this patch, when the currently chosen environment  
> > to be
> > > > > > > written
> > > > > > > has bad blocks, saveenv fails completely. Instead, when  
> > there is
> > > > > > > redundant environment fall back to the other copy.  
> > Environment
> > > > > reading
> > > > > > > needs no adjustment, as the fallback logic for incomplete  
> > writes
> > > > > > > applies
> > > > > > > to this case as well.
> > > > > > >
> > > > > > > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> > > > > >
> > > > > > Isn't this what CONFIG_ENV_RANGE is supposed to deal with?
> > > > >
> > > > > Yes, that is more or less what is supposed to help for cases  
> > like
> > > > > this.
> > > > > But given the fact that CONFIG_ENV_RANGE needs to span multiple  
> > erase
> > > > > pages which in our case are 128k in size, this is quite a deal.
> > > > > Especially since one needs to have multiple pages for both  
> > normal and
> > > > > redundant environment to be really sure.
> > > >
> > > > And *that* is what CONFIG_ENV_OFFSET_OOB is supposed to deal  
> > with. :-)
> > >
> > > Good to know, I already wondered what exactly this option is there  
> > for.
> > 
> > Hmm. Does not look like CONFIG_ENV_OFFSET_OOB is used to select the
> > block(s) within the erase page to save the environment. Looking at
> > common/env_nand.c:318, the environment offset saved in the OOB seems  
> > to
> > be in erase page unit.
> 
> I'm not sure what you mean by "block(s) within the erase page" --  
> blocks are the unit of erasing, and of bad block marking.

Not always, at least not with NAND flash. Erase pages are mostly bigger
than write pages (or "blocks"). In my case, flash consists of 0x800
bytes write pages and 0x2000 bytes erase pages.

> The block to hold the environment is stored in the OOB of block zero,  
> which is usually guaranteed to not be bad.

Erase or write block? Note that every write block has it's own OOB.

> > On the other hand, I could not find code that alters this setting  
> > based
> > on bad blocks found or whatever. This seems to simply be an  
> > alternative
> > to setting CONFIG_ENV_OFFSET at build-time.
> 
> It is set by the "nand env.oob" command.  It is currently a manual  
> process (or rather, automation is left to the user's board preparation  
> process rather than being built into U-Boot), as U-Boot wouldn't know  
> how to give back unused blocks to other purposes.

So that assumes that any block initially identified 'good' will ever
turn 'bad' later on?

Best wishes,

Phil Sutter
Software Engineer

-- 
Viprinet Europe GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:               +49 6721 49030-0
Direct line/Durchwahl:        +49 6721 49030-134
Fax:                          +49 6721 49030-109

phil.sutter at viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein, Germany
Commercial register/Handelsregister: Amtsgericht Mainz HRB44090
CEO/Gesch?ftsf?hrer: Simon Kissel

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Scott Wood]

On 12/10/2012 07:41:43 AM, Phil Sutter wrote:
> On Fri, Dec 07, 2012 at 11:38:11AM -0600, Scott Wood wrote:
> > On 12/07/2012 10:58:53 AM, Phil Sutter wrote:
> > > Hmm. Does not look like CONFIG_ENV_OFFSET_OOB is used to select  
> the
> > > block(s) within the erase page to save the environment. Looking at
> > > common/env_nand.c:318, the environment offset saved in the OOB  
> seems
> > > to
> > > be in erase page unit.
> >
> > I'm not sure what you mean by "block(s) within the erase page" --
> > blocks are the unit of erasing, and of bad block marking.
> 
> Not always, at least not with NAND flash. Erase pages are mostly  
> bigger
> than write pages (or "blocks"). In my case, flash consists of 0x800
> bytes write pages and 0x2000 bytes erase pages.

Erase blocks are larger than write pages, yes.  I've never heard erase  
blocks called "pages" or write pages called "blocks" -- but my main  
point is that the unit of erasing and the unit of badness are the same.

> > The block to hold the environment is stored in the OOB of block  
> zero,
> > which is usually guaranteed to not be bad.
> 
> Erase or write block? Note that every write block has it's own OOB.

"block" means "erase block".

Every write page has its own OOB, but it is erase blocks that are  
marked bad.  Typically the block can be marked bad in either the first  
or the second page of the erase block.

> > > On the other hand, I could not find code that alters this setting
> > > based
> > > on bad blocks found or whatever. This seems to simply be an
> > > alternative
> > > to setting CONFIG_ENV_OFFSET at build-time.
> >
> > It is set by the "nand env.oob" command.  It is currently a manual
> > process (or rather, automation is left to the user's board  
> preparation
> > process rather than being built into U-Boot), as U-Boot wouldn't  
> know
> > how to give back unused blocks to other purposes.
> 
> So that assumes that any block initially identified 'good' will ever
> turn 'bad' later on?

We don't currently have any mechanism for that to happen with the  
environment -- which could be another good reason to have real  
redundancy that doesn't get crippled from day one by having one copy  
land on a factory bad block.  Of course, that requires someone to  
implement support for redundant environment combined with  
CONFIG_ENV_OFFSET_OOB.

Maybe a better option is to implement support for storing the  
environment in ubi, although usually if your environment is in NAND  
that means your U-Boot image is in NAND, so you have the same problem  
there.  Maybe you could have an SPL that contains ubi support, that  
fits in the guaranteed-good first block.

Do you have any data on how often a block might go bad that wasn't  
factory-bad, to what extent reads versus writes matter, and whether  
there is anything special about block zero beyond not being factory-bad?

-Scott

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Phil Sutter]

On Tue, Dec 11, 2012 at 05:12:32PM -0600, Scott Wood wrote:
> On 12/10/2012 07:41:43 AM, Phil Sutter wrote:
> > On Fri, Dec 07, 2012 at 11:38:11AM -0600, Scott Wood wrote:
> > > On 12/07/2012 10:58:53 AM, Phil Sutter wrote:
> > > > Hmm. Does not look like CONFIG_ENV_OFFSET_OOB is used to select  
> > the
> > > > block(s) within the erase page to save the environment. Looking at
> > > > common/env_nand.c:318, the environment offset saved in the OOB  
> > seems
> > > > to
> > > > be in erase page unit.
> > >
> > > I'm not sure what you mean by "block(s) within the erase page" --
> > > blocks are the unit of erasing, and of bad block marking.
> > 
> > Not always, at least not with NAND flash. Erase pages are mostly  
> > bigger
> > than write pages (or "blocks"). In my case, flash consists of 0x800
> > bytes write pages and 0x2000 bytes erase pages.
> 
> Erase blocks are larger than write pages, yes.  I've never heard erase  
> blocks called "pages" or write pages called "blocks" -- but my main  
> point is that the unit of erasing and the unit of badness are the same.

Ah, OK. Please excuse my humble nomenclature, I never cared enough to
sort out what is called what. Of course, this is not the best basis for
a discussion about these things.

But getting back to the topic: The assumption of blocks getting bad, not
pages within a block means that for any kind of bad block prevention,
multiple blocks need to be used. Although I'm honestly speaking not
really sure why this needs to be like that. Maybe the bad page marking
would disappear when erasing the block it belongs to?

> > > The block to hold the environment is stored in the OOB of block  
> > zero,
> > > which is usually guaranteed to not be bad.
> > 
> > Erase or write block? Note that every write block has it's own OOB.
> 
> "block" means "erase block".
> 
> Every write page has its own OOB, but it is erase blocks that are  
> marked bad.  Typically the block can be marked bad in either the first  
> or the second page of the erase block.

Interesting. I had the impression of pages being marked bad and the
block's badness being taken from whether it contains bad pages. Probably
the 'nand markbad' command tricked me.

> > > > On the other hand, I could not find code that alters this setting
> > > > based
> > > > on bad blocks found or whatever. This seems to simply be an
> > > > alternative
> > > > to setting CONFIG_ENV_OFFSET at build-time.
> > >
> > > It is set by the "nand env.oob" command.  It is currently a manual
> > > process (or rather, automation is left to the user's board  
> > preparation
> > > process rather than being built into U-Boot), as U-Boot wouldn't  
> > know
> > > how to give back unused blocks to other purposes.
> > 
> > So that assumes that any block initially identified 'good' will ever
> > turn 'bad' later on?
> 
> We don't currently have any mechanism for that to happen with the  
> environment -- which could be another good reason to have real  
> redundancy that doesn't get crippled from day one by having one copy  
> land on a factory bad block.  Of course, that requires someone to  
> implement support for redundant environment combined with  
> CONFIG_ENV_OFFSET_OOB.

Well, as long as CONFIG_ENV_OFFSET_REDUND supported falling back to the
other copy in case of error there would be a working system in three of
four cases instead of only one.

> Maybe a better option is to implement support for storing the  
> environment in ubi, although usually if your environment is in NAND  
> that means your U-Boot image is in NAND, so you have the same problem  
> there.  Maybe you could have an SPL that contains ubi support, that  
> fits in the guaranteed-good first block.
> 
> Do you have any data on how often a block might go bad that wasn't  
> factory-bad, to what extent reads versus writes matter, and whether  
> there is anything special about block zero beyond not being factory-bad?

No, sadly not. I'd guess this information depends on what hardware being
used specifically. But I suppose block zero being prone to becoming
worn just like any other block, although it not being erased as often
should help a lot.

Assuming a certain number of erase cycles after each block is worn out
and given the fact that CONFIG_ENV_OFFSET_REDUND has always both blocks
written (unless power failure occurs), they would turn bad at the same
time and therefore rendering the environment useless with or without
fallback. :)

Best wishes, Phil

-- 
Viprinet Europe GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:               +49 6721 49030-0
Direct line/Durchwahl:        +49 6721 49030-134
Fax:                          +49 6721 49030-109

phil.sutter at viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein, Germany
Commercial register/Handelsregister: Amtsgericht Mainz HRB44090
CEO/Gesch?ftsf?hrer: Simon Kissel

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Scott Wood]

On 12/20/2012 03:28:39 PM, Phil Sutter wrote:
> On Tue, Dec 11, 2012 at 05:12:32PM -0600, Scott Wood wrote:
> > Erase blocks are larger than write pages, yes.  I've never heard  
> erase
> > blocks called "pages" or write pages called "blocks" -- but my main
> > point is that the unit of erasing and the unit of badness are the  
> same.
> 
> Ah, OK. Please excuse my humble nomenclature, I never cared enough to
> sort out what is called what. Of course, this is not the best basis  
> for
> a discussion about these things.
> 
> But getting back to the topic: The assumption of blocks getting bad,  
> not
> pages within a block means that for any kind of bad block prevention,
> multiple blocks need to be used. Although I'm honestly speaking not
> really sure why this needs to be like that. Maybe the bad page marking
> would disappear when erasing the block it belongs to?

Yes, it would disappear.  This is why erase operations skip bad blocks,  
unless the scrub option is uesd.

> > > > The block to hold the environment is stored in the OOB of block
> > > zero,
> > > > which is usually guaranteed to not be bad.
> > >
> > > Erase or write block? Note that every write block has it's own  
> OOB.
> >
> > "block" means "erase block".
> >
> > Every write page has its own OOB, but it is erase blocks that are
> > marked bad.  Typically the block can be marked bad in either the  
> first
> > or the second page of the erase block.
> 
> Interesting. I had the impression of pages being marked bad and the
> block's badness being taken from whether it contains bad pages.  
> Probably
> the 'nand markbad' command tricked me.

Do you mean the lack of error checking if you pass a non-block-aligned  
offset into "nand markbad"?

> > > So that assumes that any block initially identified 'good' will  
> ever
> > > turn 'bad' later on?
> >
> > We don't currently have any mechanism for that to happen with the
> > environment -- which could be another good reason to have real
> > redundancy that doesn't get crippled from day one by having one copy
> > land on a factory bad block.  Of course, that requires someone to
> > implement support for redundant environment combined with
> > CONFIG_ENV_OFFSET_OOB.
> 
> Well, as long as CONFIG_ENV_OFFSET_REDUND supported falling back to  
> the
> other copy in case of error there would be a working system in three  
> of
> four cases instead of only one.

I'm not sure what you mean here -- where do "three", "four", and "one"  
come from?

> > Maybe a better option is to implement support for storing the
> > environment in ubi, although usually if your environment is in NAND
> > that means your U-Boot image is in NAND, so you have the same  
> problem
> > there.  Maybe you could have an SPL that contains ubi support, that
> > fits in the guaranteed-good first block.
> >
> > Do you have any data on how often a block might go bad that wasn't
> > factory-bad, to what extent reads versus writes matter, and whether
> > there is anything special about block zero beyond not being  
> factory-bad?
> 
> No, sadly not. I'd guess this information depends on what hardware  
> being
> used specifically. But I suppose block zero being prone to becoming
> worn just like any other block, although it not being erased as often
> should help a lot.
> 
> Assuming a certain number of erase cycles after each block is worn out
> and given the fact that CONFIG_ENV_OFFSET_REDUND has always both  
> blocks
> written (unless power failure occurs), they would turn bad at the same
> time and therefore rendering the environment useless with or without
> fallback. :)

That depends on whether the specified number of erase cycles per block  
is a minimum for any block not marked factory-bad, or whether some  
fraction of non-factory-bad blocks may fail early.

-Scott

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Phil Sutter]

On Thu, Dec 20, 2012 at 03:41:37PM -0600, Scott Wood wrote:
> On 12/20/2012 03:28:39 PM, Phil Sutter wrote:
> > On Tue, Dec 11, 2012 at 05:12:32PM -0600, Scott Wood wrote:
> > > Erase blocks are larger than write pages, yes.  I've never heard  
> > erase
> > > blocks called "pages" or write pages called "blocks" -- but my main
> > > point is that the unit of erasing and the unit of badness are the  
> > same.
> > 
> > Ah, OK. Please excuse my humble nomenclature, I never cared enough to
> > sort out what is called what. Of course, this is not the best basis  
> > for
> > a discussion about these things.
> > 
> > But getting back to the topic: The assumption of blocks getting bad,  
> > not
> > pages within a block means that for any kind of bad block prevention,
> > multiple blocks need to be used. Although I'm honestly speaking not
> > really sure why this needs to be like that. Maybe the bad page marking
> > would disappear when erasing the block it belongs to?
> 
> Yes, it would disappear.  This is why erase operations skip bad blocks,  
> unless the scrub option is uesd.

Which is apparently preventing good pages in a block with a bad page
from being used, isn't it?

> > > > > The block to hold the environment is stored in the OOB of block
> > > > zero,
> > > > > which is usually guaranteed to not be bad.
> > > >
> > > > Erase or write block? Note that every write block has it's own  
> > OOB.
> > >
> > > "block" means "erase block".
> > >
> > > Every write page has its own OOB, but it is erase blocks that are
> > > marked bad.  Typically the block can be marked bad in either the  
> > first
> > > or the second page of the erase block.
> > 
> > Interesting. I had the impression of pages being marked bad and the
> > block's badness being taken from whether it contains bad pages.  
> > Probably
> > the 'nand markbad' command tricked me.
> 
> Do you mean the lack of error checking if you pass a non-block-aligned  
> offset into "nand markbad"?

I think the bigger "problem" is 'nand markbad' updating the bad block
table along the go. So no real bad block detection occurs as far as I
can tell.

> > > > So that assumes that any block initially identified 'good' will  
> > ever
> > > > turn 'bad' later on?
> > >
> > > We don't currently have any mechanism for that to happen with the
> > > environment -- which could be another good reason to have real
> > > redundancy that doesn't get crippled from day one by having one copy
> > > land on a factory bad block.  Of course, that requires someone to
> > > implement support for redundant environment combined with
> > > CONFIG_ENV_OFFSET_OOB.
> > 
> > Well, as long as CONFIG_ENV_OFFSET_REDUND supported falling back to  
> > the
> > other copy in case of error there would be a working system in three  
> > of
> > four cases instead of only one.
> 
> I'm not sure what you mean here -- where do "three", "four", and "one"  
> come from?

Just some quantitative approach: given the environment residing at block
A and it's redundant copy at block B, four situations may occur: both
blocks good, block A bad, block B bad or both blocks bad. Upstream would
fail in all cases but both blocks good. My patch would turn that into
failing only if both blocks bad. So working in three of four cases
instead of in only one of four.

> > > Maybe a better option is to implement support for storing the
> > > environment in ubi, although usually if your environment is in NAND
> > > that means your U-Boot image is in NAND, so you have the same  
> > problem
> > > there.  Maybe you could have an SPL that contains ubi support, that
> > > fits in the guaranteed-good first block.
> > >
> > > Do you have any data on how often a block might go bad that wasn't
> > > factory-bad, to what extent reads versus writes matter, and whether
> > > there is anything special about block zero beyond not being  
> > factory-bad?
> > 
> > No, sadly not. I'd guess this information depends on what hardware  
> > being
> > used specifically. But I suppose block zero being prone to becoming
> > worn just like any other block, although it not being erased as often
> > should help a lot.
> > 
> > Assuming a certain number of erase cycles after each block is worn out
> > and given the fact that CONFIG_ENV_OFFSET_REDUND has always both  
> > blocks
> > written (unless power failure occurs), they would turn bad at the same
> > time and therefore rendering the environment useless with or without
> > fallback. :)
> 
> That depends on whether the specified number of erase cycles per block  
> is a minimum for any block not marked factory-bad, or whether some  
> fraction of non-factory-bad blocks may fail early.

Sure. Also I'm not sure how "wear" happens, so if blocks get worse or
their probability of failure increases from erase to erase. Although the
later case would make it hard to guarantee a certain number of erase
cycles.

Best wishes, Phil

-- 
Viprinet Europe GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:               +49 6721 49030-0
Direct line/Durchwahl:        +49 6721 49030-134
Fax:                          +49 6721 49030-109

phil.sutter at viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein, Germany
Commercial register/Handelsregister: Amtsgericht Mainz HRB44090
CEO/Gesch?ftsf?hrer: Simon Kissel

[U-Boot] [PATCH 2/4] env_nand.c: support falling back to redundant env when writing

[Scott Wood]

On 12/21/2012 04:34:03 AM, Phil Sutter wrote:
> On Thu, Dec 20, 2012 at 03:41:37PM -0600, Scott Wood wrote:
> > On 12/20/2012 03:28:39 PM, Phil Sutter wrote:
> > > On Tue, Dec 11, 2012 at 05:12:32PM -0600, Scott Wood wrote:
> > > > Erase blocks are larger than write pages, yes.  I've never heard
> > > erase
> > > > blocks called "pages" or write pages called "blocks" -- but my  
> main
> > > > point is that the unit of erasing and the unit of badness are  
> the
> > > same.
> > >
> > > Ah, OK. Please excuse my humble nomenclature, I never cared  
> enough to
> > > sort out what is called what. Of course, this is not the best  
> basis
> > > for
> > > a discussion about these things.
> > >
> > > But getting back to the topic: The assumption of blocks getting  
> bad,
> > > not
> > > pages within a block means that for any kind of bad block  
> prevention,
> > > multiple blocks need to be used. Although I'm honestly speaking  
> not
> > > really sure why this needs to be like that. Maybe the bad page  
> marking
> > > would disappear when erasing the block it belongs to?
> >
> > Yes, it would disappear.  This is why erase operations skip bad  
> blocks,
> > unless the scrub option is uesd.
> 
> Which is apparently preventing good pages in a block with a bad page
> from being used, isn't it?

Right,  plus the knowledge of which pages within the block are bad  
simply isn't there.

> > > Interesting. I had the impression of pages being marked bad and  
> the
> > > block's badness being taken from whether it contains bad pages.
> > > Probably
> > > the 'nand markbad' command tricked me.
> >
> > Do you mean the lack of error checking if you pass a  
> non-block-aligned
> > offset into "nand markbad"?
> 
> I think the bigger "problem" is 'nand markbad' updating the bad block
> table along the go. So no real bad block detection occurs as far as I
> can tell.

I'm not sure what you mean here.

> > > Well, as long as CONFIG_ENV_OFFSET_REDUND supported falling back  
> to
> > > the
> > > other copy in case of error there would be a working system in  
> three
> > > of
> > > four cases instead of only one.
> >
> > I'm not sure what you mean here -- where do "three", "four", and  
> "one"
> > come from?
> 
> Just some quantitative approach: given the environment residing at  
> block
> A and it's redundant copy at block B, four situations may occur: both
> blocks good, block A bad, block B bad or both blocks bad. Upstream  
> would
> fail in all cases but both blocks good. My patch would turn that into
> failing only if both blocks bad. So working in three of four cases
> instead of in only one of four.

Those two cases that would suddenly be working would be lacking  
redundancy -- would you want to ship it like that?  If U-Boot is noisy  
about it, then such units can still have their NAND chips replaced  
before shipping.

-Scott

[U-Boot] [PATCH 3/4] env_nand.c: do warn only if really no valid environment could be loaded

[Phil Sutter]

The warning is misleading, since there is no equivalent success note
when reading the other copy succeeds.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 common/env_nand.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/common/env_nand.c b/common/env_nand.c
index 895532b..58ba558 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -315,6 +315,7 @@ int get_nand_env_oob(nand_info_t *nand, unsigned long *result)
 void env_relocate_spec(void)
 {
 #if !defined(ENV_IS_EMBEDDED)
+	int read1_fail = 0, read2_fail = 0;
 	int crc1_ok = 0, crc2_ok = 0;
 	env_t *ep, *tmp_env1, *tmp_env2;
 
@@ -326,11 +327,11 @@ void env_relocate_spec(void)
 		goto done;
 	}
 
-	if (readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1))
-		puts("No Valid Environment Area found\n");
+	read1_fail = readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1);
+	read2_fail = readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2);
 
-	if (readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2))
-		puts("No Valid Redundant Environment Area found\n");
+	if (read1_fail && read2_fail)
+		puts("No Valid Environment Area found\n");
 
 	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc;
 	crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc;
-- 
1.7.3.4

[U-Boot] [PATCH 3/4] env_nand.c: do warn only if really no valid environment could be loaded

[Scott Wood]

On 11/21/2012 06:59:20 AM, Phil Sutter wrote:
> The warning is misleading, since there is no equivalent success note
> when reading the other copy succeeds.
> 
> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> ---
>  common/env_nand.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/common/env_nand.c b/common/env_nand.c
> index 895532b..58ba558 100644
> --- a/common/env_nand.c
> +++ b/common/env_nand.c
> @@ -315,6 +315,7 @@ int get_nand_env_oob(nand_info_t *nand, unsigned  
> long *result)
>  void env_relocate_spec(void)
>  {
>  #if !defined(ENV_IS_EMBEDDED)
> +	int read1_fail = 0, read2_fail = 0;
>  	int crc1_ok = 0, crc2_ok = 0;
>  	env_t *ep, *tmp_env1, *tmp_env2;
> 
> @@ -326,11 +327,11 @@ void env_relocate_spec(void)
>  		goto done;
>  	}
> 
> -	if (readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1))
> -		puts("No Valid Environment Area found\n");
> +	read1_fail = readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1);
> +	read2_fail = readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *)  
> tmp_env2);

Why read the redundant environment if the the main one didn't fail?

-Scott

[U-Boot] [PATCH 3/4] env_nand.c: do warn only if really no valid environment could be loaded

[Scott Wood]

On 11/27/2012 04:06:03 PM, Scott Wood wrote:
> On 11/21/2012 06:59:20 AM, Phil Sutter wrote:
>> The warning is misleading, since there is no equivalent success note
>> when reading the other copy succeeds.
>> 
>> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
>> ---
>>  common/env_nand.c |    9 +++++----
>>  1 files changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/common/env_nand.c b/common/env_nand.c
>> index 895532b..58ba558 100644
>> --- a/common/env_nand.c
>> +++ b/common/env_nand.c
>> @@ -315,6 +315,7 @@ int get_nand_env_oob(nand_info_t *nand, unsigned  
>> long *result)
>>  void env_relocate_spec(void)
>>  {
>>  #if !defined(ENV_IS_EMBEDDED)
>> +	int read1_fail = 0, read2_fail = 0;
>>  	int crc1_ok = 0, crc2_ok = 0;
>>  	env_t *ep, *tmp_env1, *tmp_env2;
>> 
>> @@ -326,11 +327,11 @@ void env_relocate_spec(void)
>>  		goto done;
>>  	}
>> 
>> -	if (readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1))
>> -		puts("No Valid Environment Area found\n");
>> +	read1_fail = readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1);
>> +	read2_fail = readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *)  
>> tmp_env2);
> 
> Why read the redundant environment if the the main one didn't fail?

Never mind, misread the original code (the answer is we want to see  
which one is more recent).

-Scott

[U-Boot] [PATCH 3/4] env_nand.c: do warn only if really no valid environment could be loaded

[Scott Wood]

On 11/21/2012 06:59:20 AM, Phil Sutter wrote:
> The warning is misleading, since there is no equivalent success note
> when reading the other copy succeeds.
> 
> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> ---
>  common/env_nand.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/common/env_nand.c b/common/env_nand.c
> index 895532b..58ba558 100644
> --- a/common/env_nand.c
> +++ b/common/env_nand.c
> @@ -315,6 +315,7 @@ int get_nand_env_oob(nand_info_t *nand, unsigned  
> long *result)
>  void env_relocate_spec(void)
>  {
>  #if !defined(ENV_IS_EMBEDDED)
> +	int read1_fail = 0, read2_fail = 0;
>  	int crc1_ok = 0, crc2_ok = 0;
>  	env_t *ep, *tmp_env1, *tmp_env2;
> 
> @@ -326,11 +327,11 @@ void env_relocate_spec(void)
>  		goto done;
>  	}
> 
> -	if (readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1))
> -		puts("No Valid Environment Area found\n");
> +	read1_fail = readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1);
> +	read2_fail = readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *)  
> tmp_env2);
> 
> -	if (readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2))
> -		puts("No Valid Redundant Environment Area found\n");
> +	if (read1_fail && read2_fail)
> +		puts("No Valid Environment Area found\n");
> 
>  	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc;
>  	crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc;

We should still say something if one of the copies failed to load -- we  
just need to word it better (see env_flash.c for an example).

-Scott

[U-Boot] [PATCH 4/4] common/env_nand.c: calculate crc only when readenv was OK

[Phil Sutter]

Calculating the checksum of incompletely read data is useless.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 common/env_nand.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/env_nand.c b/common/env_nand.c
index 58ba558..038aa00 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -333,8 +333,8 @@ void env_relocate_spec(void)
 	if (read1_fail && read2_fail)
 		puts("No Valid Environment Area found\n");
 
-	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc;
-	crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc;
+	crc1_ok = !read1_fail && (crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc);
+	crc2_ok = !read2_fail && (crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc);
 
 	if (!crc1_ok && !crc2_ok) {
 		set_default_env("!bad CRC");
-- 
1.7.3.4

[U-Boot] [PATCH 1/4] Optimized nand_read_buf for kirkwood

[Prafulla Wadaskar]

> -----Original Message-----
> From: Phil Sutter [mailto:phil.sutter at viprinet.com]
> Sent: 21 November 2012 18:29
> To: u-boot at lists.denx.de
> Cc: Nico Erfurth; Prafulla Wadaskar
> Subject: [PATCH 1/4] Optimized nand_read_buf for kirkwood
> 
> The basic idea is taken from the linux-kernel, but further optimized.
> 
> First align the buffer to 8 bytes, then use ldrd/strd to read and
> store
> in 8 byte quantities, then do the final bytes.
> 
> Signed-off-by: Nico Erfurth <ne@erfurth.eu>
> Cc: Prafulla Wadaskar <prafulla@marvell.com>

Hi Phil,
Have you tested this patch? I don't see your signed-off or tested-by signature in this patch.

Regards...
Prafulla . . .

[U-Boot] [PATCH 1/4] Optimized nand_read_buf for kirkwood

[Phil Sutter]

Hello Prafulla,

On Sun, Nov 25, 2012 at 07:46:50PM -0800, Prafulla Wadaskar wrote:
> Have you tested this patch? I don't see your signed-off or tested-by signature in this patch.

Yes, I did. But just for functionality, no real spead measurement or so.
That's why I skipped the tested-by line. I just caught up on this,
please see the patch I send in reply to this mail.

Best wishes,

Phil Sutter
Software Engineer

-- 
Viprinet Europe GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:               +49 6721 49030-0
Direct line/Durchwahl:        +49 6721 49030-134
Fax:                          +49 6721 49030-109

phil.sutter at viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein, Germany
Commercial register/Handelsregister: Amtsgericht Mainz HRB44090
CEO/Gesch?ftsf?hrer: Simon Kissel

[U-Boot] [PATCH 1/4] Optimized nand_read_buf for kirkwood

[Phil Sutter]

The basic idea is taken from the linux-kernel, but further optimized.

First align the buffer to 8 bytes, then use ldrd/strd to read and store
in 8 byte quantities, then do the final bytes.

Tested using: 'date ; nand read.raw 0xE00000 0x0 0x10000 ; date'.
Without this patch, NAND read of 132MB took 49s (~2.69MB/s). With this
patch in place, reading the same amount of data was done in 27s
(~4.89MB/s). So read performance is increased by ~80%!

Signed-off-by: Nico Erfurth <ne@erfurth.eu>
Tested-by: Phil Sutter <phil.sutter@viprinet.com>
Cc: Prafulla Wadaskar <prafulla@marvell.com>
---
 drivers/mtd/nand/kirkwood_nand.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/kirkwood_nand.c b/drivers/mtd/nand/kirkwood_nand.c
index bdab5aa..e04a59f 100644
--- a/drivers/mtd/nand/kirkwood_nand.c
+++ b/drivers/mtd/nand/kirkwood_nand.c
@@ -38,6 +38,34 @@ struct kwnandf_registers {
 static struct kwnandf_registers *nf_reg =
 	(struct kwnandf_registers *)KW_NANDF_BASE;
 
+
+/* The basic idea is stolen from the linux kernel, but the inner loop is optimized a bit more */
+static void kw_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+
+	while (len && (unsigned long)buf & 7)
+	{
+		*buf++ = readb(chip->IO_ADDR_R);
+		len--;
+	};
+
+	asm volatile (
+		".LFlashLoop:\n"
+		"  subs\t%0, #8\n"
+		"  ldrpld\tr2, [%2]\n" // Read 2 words
+		"  strpld\tr2, [%1], #8\n" // Read 2 words
+		"  bpl\t.LFlashLoop\n" // This results in one additional loop if len%8 <> 0
+		"  addne\t%0, #8\n"
+		: "+&r" (len), "+&r" (buf)
+		: "r" (chip->IO_ADDR_R)
+		: "r2", "r3", "memory", "cc"
+	);
+
+	while (len--)
+		*buf++ = readb(chip->IO_ADDR_R);
+}
+
 /*
  * hardware specific access to control-lines/bits
  */
@@ -76,6 +104,7 @@ int board_nand_init(struct nand_chip *nand)
 	nand->options = NAND_COPYBACK | NAND_CACHEPRG | NAND_NO_PADDING;
 	nand->ecc.mode = NAND_ECC_SOFT;
 	nand->cmd_ctrl = kw_nand_hwcontrol;
+	nand->read_buf = kw_nand_read_buf;
 	nand->chip_delay = 40;
 	nand->select_chip = kw_nand_select_chip;
 	return 0;
-- 
1.7.3.4

[U-Boot] [PATCH 1/4] Optimized nand_read_buf for kirkwood

[Scott Wood]

On 11/26/2012 04:33:08 AM, Phil Sutter wrote:
> The basic idea is taken from the linux-kernel, but further optimized.
> 
> First align the buffer to 8 bytes, then use ldrd/strd to read and  
> store
> in 8 byte quantities, then do the final bytes.
> 
> Tested using: 'date ; nand read.raw 0xE00000 0x0 0x10000 ; date'.
> Without this patch, NAND read of 132MB took 49s (~2.69MB/s). With this
> patch in place, reading the same amount of data was done in 27s
> (~4.89MB/s). So read performance is increased by ~80%!
> 
> Signed-off-by: Nico Erfurth <ne@erfurth.eu>
> Tested-by: Phil Sutter <phil.sutter@viprinet.com>
> Cc: Prafulla Wadaskar <prafulla@marvell.com>
> ---
>  drivers/mtd/nand/kirkwood_nand.c |   29 +++++++++++++++++++++++++++++
>  1 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/nand/kirkwood_nand.c  
> b/drivers/mtd/nand/kirkwood_nand.c
> index bdab5aa..e04a59f 100644
> --- a/drivers/mtd/nand/kirkwood_nand.c
> +++ b/drivers/mtd/nand/kirkwood_nand.c
> @@ -38,6 +38,34 @@ struct kwnandf_registers {
>  static struct kwnandf_registers *nf_reg =
>  	(struct kwnandf_registers *)KW_NANDF_BASE;
> 
> +
> +/* The basic idea is stolen from the linux kernel, but the inner  
> loop is optimized a bit more */
> +static void kw_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int  
> len)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +
> +	while (len && (unsigned long)buf & 7)
> +	{

Brace goes on the previous line.

> +		*buf++ = readb(chip->IO_ADDR_R);
> +		len--;
> +	};
> +
> +	asm volatile (
> +		".LFlashLoop:\n"
> +		"  subs\t%0, #8\n"
> +		"  ldrpld\tr2, [%2]\n" // Read 2 words
> +		"  strpld\tr2, [%1], #8\n" // Read 2 words
> +		"  bpl\t.LFlashLoop\n" // This results in one  
> additional loop if len%8 <> 0
> +		"  addne\t%0, #8\n"
> +		: "+&r" (len), "+&r" (buf)
> +		: "r" (chip->IO_ADDR_R)
> +		: "r2", "r3", "memory", "cc"
> +	);

Use a real tab (or a space) rather than \t (which only helps  
readability in the asm output, rather than the C source that people  
actually look at).

Should probably use a numeric label to avoid any possibility of  
conflict.

Would this make more sense as a more generic optimized memcpy_fromio()  
or similar?

-Scott

[U-Boot] [PATCH 1/4] Optimized nand_read_buf for kirkwood

[Prafulla Wadaskar]

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de [mailto:u-boot-
> bounces at lists.denx.de] On Behalf Of Scott Wood
> Sent: 27 November 2012 05:09
> To: Phil Sutter
> Cc: u-boot at lists.denx.de; Nico Erfurth
> Subject: Re: [U-Boot] [PATCH 1/4] Optimized nand_read_buf for kirkwood
> 
> On 11/26/2012 04:33:08 AM, Phil Sutter wrote:
> > The basic idea is taken from the linux-kernel, but further
> optimized.
> >
> > First align the buffer to 8 bytes, then use ldrd/strd to read and
> > store
> > in 8 byte quantities, then do the final bytes.
> >
> > Tested using: 'date ; nand read.raw 0xE00000 0x0 0x10000 ; date'.
> > Without this patch, NAND read of 132MB took 49s (~2.69MB/s). With
> this
> > patch in place, reading the same amount of data was done in 27s
> > (~4.89MB/s). So read performance is increased by ~80%!
> >
> > Signed-off-by: Nico Erfurth <ne@erfurth.eu>
> > Tested-by: Phil Sutter <phil.sutter@viprinet.com>
> > Cc: Prafulla Wadaskar <prafulla@marvell.com>
> > ---
> >  drivers/mtd/nand/kirkwood_nand.c |   29
> +++++++++++++++++++++++++++++
> >  1 files changed, 29 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/kirkwood_nand.c
> > b/drivers/mtd/nand/kirkwood_nand.c
> > index bdab5aa..e04a59f 100644
> > --- a/drivers/mtd/nand/kirkwood_nand.c
> > +++ b/drivers/mtd/nand/kirkwood_nand.c
> > @@ -38,6 +38,34 @@ struct kwnandf_registers {
> >  static struct kwnandf_registers *nf_reg =
> >  	(struct kwnandf_registers *)KW_NANDF_BASE;
> >
> > +
> > +/* The basic idea is stolen from the linux kernel, but the inner
> > loop is optimized a bit more */
> > +static void kw_nand_read_buf(struct mtd_info *mtd, uint8_t *buf,
> int
> > len)
> > +{
> > +	struct nand_chip *chip = mtd->priv;
> > +
> > +	while (len && (unsigned long)buf & 7)
> > +	{
> 
> Brace goes on the previous line.
> 
> > +		*buf++ = readb(chip->IO_ADDR_R);
> > +		len--;
> > +	};
> > +
> > +	asm volatile (
> > +		".LFlashLoop:\n"
> > +		"  subs\t%0, #8\n"
> > +		"  ldrpld\tr2, [%2]\n" // Read 2 words
> > +		"  strpld\tr2, [%1], #8\n" // Read 2 words
> > +		"  bpl\t.LFlashLoop\n" // This results in one
> > additional loop if len%8 <> 0
> > +		"  addne\t%0, #8\n"
> > +		: "+&r" (len), "+&r" (buf)
> > +		: "r" (chip->IO_ADDR_R)
> > +		: "r2", "r3", "memory", "cc"
> > +	);
> 
> Use a real tab (or a space) rather than \t (which only helps
> readability in the asm output, rather than the C source that people
> actually look at).
> 
> Should probably use a numeric label to avoid any possibility of
> conflict.
> 
> Would this make more sense as a more generic optimized memcpy_fromio()
> or similar?

Hi Phil

For your next post of this patch, please do not forget to add version info and changlog to the patch.

Regards...
Prafulla . . .
 
> 
> -Scott
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 1/4] Optimized nand_read_buf for kirkwood

[Phil Sutter]

Hi Prafulla,

On Wed, Dec 19, 2012 at 10:44:01PM -0800, Prafulla Wadaskar wrote:
> For your next post of this patch, please do not forget to add version info and changlog to the patch.

Ah yes, indeed! Thanks a lot for the hint and sorry for the confusion
caused.

Best wishes, Phil

-- 
Viprinet Europe GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:               +49 6721 49030-0
Direct line/Durchwahl:        +49 6721 49030-134
Fax:                          +49 6721 49030-109

phil.sutter at viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein, Germany
Commercial register/Handelsregister: Amtsgericht Mainz HRB44090
CEO/Gesch?ftsf?hrer: Simon Kissel

[U-Boot] Version 2 of Kirkwood and env_nand improvements

[Phil Sutter]

Respin of my patch series for review. Only changes in patches 2 and 3:

env_nand.c: support falling back to redundant env when writing:
  Print status message for each of the redundant copies when writing. So
  when writing, we see which image is being written to. Also, when the first
  attempt fails, a second message follows so the user sees that writing was
  eventually successful.

env_nand.c: clarify log messages when env reading fails:
  Print a warning if one of the two redundant copies could not be read and an
  error if both failed to read. Message text and formatting chosen with
  common/env_flash.c as example.

[U-Boot] [PATCHv2 1/4] Optimized nand_read_buf for kirkwood (V3)

[Phil Sutter]

The basic idea is taken from the linux-kernel, but further optimized.

First align the buffer to 8 bytes, then use ldrd/strd to read and store
in 8 byte quantities, then do the final bytes.

Tested using: 'date ; nand read.raw 0xE00000 0x0 0x10000 ; date'.
Without this patch, NAND read of 132MB took 49s (~2.69MB/s). With this
patch in place, reading the same amount of data was done in 27s
(~4.89MB/s). So read performance is increased by ~80%!

Signed-off-by: Nico Erfurth <ne@erfurth.eu>
Tested-by: Phil Sutter <phil.sutter@viprinet.com>
Cc: Prafulla Wadaskar <prafulla@marvell.com>
---
 drivers/mtd/nand/kirkwood_nand.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/kirkwood_nand.c b/drivers/mtd/nand/kirkwood_nand.c
index bdab5aa..99e5f35 100644
--- a/drivers/mtd/nand/kirkwood_nand.c
+++ b/drivers/mtd/nand/kirkwood_nand.c
@@ -38,6 +38,37 @@ struct kwnandf_registers {
 static struct kwnandf_registers *nf_reg =
 	(struct kwnandf_registers *)KW_NANDF_BASE;
 
+
+/*
+ * The basic idea is stolen from the linux kernel, but the inner loop is
+ * optimized a bit more.
+ */
+static void kw_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+
+	while (len && (unsigned long)buf & 7) {
+		*buf++ = readb(chip->IO_ADDR_R);
+		len--;
+	};
+
+	/* This loop reads and writes 64bit per round. */
+	asm volatile (
+		"1:\n"
+		"  subs   %0, #8\n"
+		"  ldrpld r2, [%2]\n"
+		"  strpld r2, [%1], #8\n"
+		"  bhi    1b\n"
+		"  addne  %0, #8\n"
+		: "+&r" (len), "+&r" (buf)
+		: "r" (chip->IO_ADDR_R)
+		: "r2", "r3", "memory", "cc"
+	);
+
+	while (len--)
+		*buf++ = readb(chip->IO_ADDR_R);
+}
+
 /*
  * hardware specific access to control-lines/bits
  */
@@ -76,6 +107,7 @@ int board_nand_init(struct nand_chip *nand)
 	nand->options = NAND_COPYBACK | NAND_CACHEPRG | NAND_NO_PADDING;
 	nand->ecc.mode = NAND_ECC_SOFT;
 	nand->cmd_ctrl = kw_nand_hwcontrol;
+	nand->read_buf = kw_nand_read_buf;
 	nand->chip_delay = 40;
 	nand->select_chip = kw_nand_select_chip;
 	return 0;
-- 
1.7.3.4

[U-Boot] [PATCHv2 1/4] Optimized nand_read_buf for kirkwood (V3)

[Scott Wood]

On Thu, Feb 21, 2013 at 06:21:53PM +0100, Phil Sutter wrote:
> The basic idea is taken from the linux-kernel, but further optimized.
> 
> First align the buffer to 8 bytes, then use ldrd/strd to read and store
> in 8 byte quantities, then do the final bytes.
> 
> Tested using: 'date ; nand read.raw 0xE00000 0x0 0x10000 ; date'.
> Without this patch, NAND read of 132MB took 49s (~2.69MB/s). With this
> patch in place, reading the same amount of data was done in 27s
> (~4.89MB/s). So read performance is increased by ~80%!
> 
> Signed-off-by: Nico Erfurth <ne@erfurth.eu>
> Tested-by: Phil Sutter <phil.sutter@viprinet.com>
> Cc: Prafulla Wadaskar <prafulla@marvell.com>
> ---
>  drivers/mtd/nand/kirkwood_nand.c |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)

Which is it, v2 or v3?

Patch versioning goes inside the [], otherwise "git am" won't strip it
out of the patch subject (and Wolfgang doesn't like patch subjects being
edited while being applied).

> diff --git a/drivers/mtd/nand/kirkwood_nand.c b/drivers/mtd/nand/kirkwood_nand.c
> index bdab5aa..99e5f35 100644
> --- a/drivers/mtd/nand/kirkwood_nand.c
> +++ b/drivers/mtd/nand/kirkwood_nand.c
> @@ -38,6 +38,37 @@ struct kwnandf_registers {
>  static struct kwnandf_registers *nf_reg =
>  	(struct kwnandf_registers *)KW_NANDF_BASE;
>  
> +
> +/*
> + * The basic idea is stolen from the linux kernel, but the inner loop is
> + * optimized a bit more.
> + */
> +static void kw_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +
> +	while (len && (unsigned long)buf & 7) {
> +		*buf++ = readb(chip->IO_ADDR_R);
> +		len--;
> +	};
> +
> +	/* This loop reads and writes 64bit per round. */
> +	asm volatile (
> +		"1:\n"
> +		"  subs   %0, #8\n"
> +		"  ldrpld r2, [%2]\n"
> +		"  strpld r2, [%1], #8\n"
> +		"  bhi    1b\n"
> +		"  addne  %0, #8\n"
> +		: "+&r" (len), "+&r" (buf)
> +		: "r" (chip->IO_ADDR_R)
> +		: "r2", "r3", "memory", "cc"
> +	);
> +
> +	while (len--)
> +		*buf++ = readb(chip->IO_ADDR_R);
> +}

Can someone ACK this from the ARM side?

-Scott

[U-Boot] [PATCHv2 2/4] env_nand.c: support falling back to redundant env when writing

[Phil Sutter]

Without this patch, when the currently chosen environment to be written
has bad blocks, saveenv fails completely. Instead, when there is
redundant environment fall back to the other copy. Environment reading
needs no adjustment, as the fallback logic for incomplete writes applies
to this case as well.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 common/env_nand.c |  103 ++++++++++++++++++++++------------------------------
 1 files changed, 44 insertions(+), 59 deletions(-)

diff --git a/common/env_nand.c b/common/env_nand.c
index 22e72a2..c0c985c 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -168,72 +168,55 @@ int writeenv(size_t offset, u_char *buf)
 	return 0;
 }
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
-static unsigned char env_flags;
+typedef struct env_location {
+	const char *name;
+	nand_erase_options_t *erase_opts;
+	loff_t offset;
+} env_location_t;
 
-int saveenv(void)
+static int erase_and_write_env(env_location_t *location, u_char *env_new)
 {
-	env_t	env_new;
-	ssize_t	len;
-	char	*res;
-	int	ret = 0;
-	nand_erase_options_t nand_erase_options;
-
-	memset(&nand_erase_options, 0, sizeof(nand_erase_options));
-	nand_erase_options.length = CONFIG_ENV_RANGE;
+	int ret = 0;
 
-	if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE)
+	printf("Erasing %s...\n", location->name);
+	location->erase_opts->offset = location->offset;
+	if (nand_erase_opts(&nand_info[0], location->erase_opts))
 		return 1;
 
-	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
-	if (len < 0) {
-		error("Cannot export environment: errno = %d\n", errno);
-		return 1;
-	}
-	env_new.crc	= crc32(0, env_new.data, ENV_SIZE);
-	env_new.flags	= ++env_flags; /* increase the serial */
-
-	if (gd->env_valid == 1) {
-		puts("Erasing redundant NAND...\n");
-		nand_erase_options.offset = CONFIG_ENV_OFFSET_REDUND;
-		if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-			return 1;
-
-		puts("Writing to redundant NAND... ");
-		ret = writeenv(CONFIG_ENV_OFFSET_REDUND, (u_char *)&env_new);
-	} else {
-		puts("Erasing NAND...\n");
-		nand_erase_options.offset = CONFIG_ENV_OFFSET;
-		if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-			return 1;
-
-		puts("Writing to NAND... ");
-		ret = writeenv(CONFIG_ENV_OFFSET, (u_char *)&env_new);
-	}
-	if (ret) {
+	printf("Writing to %s... ", location->name);
+	if ((ret = writeenv(location->offset, env_new)))
 		puts("FAILED!\n");
-		return 1;
-	}
-
-	puts("done\n");
-
-	gd->env_valid = gd->env_valid == 2 ? 1 : 2;
+	else
+		puts("OK\n");
 
 	return ret;
 }
-#else /* ! CONFIG_ENV_OFFSET_REDUND */
+
+static unsigned char env_flags;
+
 int saveenv(void)
 {
 	int	ret = 0;
 	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
 	ssize_t	len;
 	char	*res;
+	int	env_idx;
 	nand_erase_options_t nand_erase_options;
+	env_location_t location[] = {{
+		.name = "NAND",
+		.erase_opts = &nand_erase_options,
+		.offset = CONFIG_ENV_OFFSET,
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	}, {
+		.name = "redundant NAND",
+		.erase_opts = &nand_erase_options,
+		.offset = CONFIG_ENV_OFFSET_REDUND,
+#endif
+	}};
+
 
 	memset(&nand_erase_options, 0, sizeof(nand_erase_options));
 	nand_erase_options.length = CONFIG_ENV_RANGE;
-	nand_erase_options.offset = CONFIG_ENV_OFFSET;
 
 	if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE)
 		return 1;
@@ -244,22 +227,24 @@ int saveenv(void)
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;
 	}
-	env_new->crc = crc32(0, env_new->data, ENV_SIZE);
-
-	puts("Erasing Nand...\n");
-	if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-		return 1;
+	env_new->crc   = crc32(0, env_new->data, ENV_SIZE);
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	env_new->flags = ++env_flags; /* increase the serial */
+	env_idx = (gd->env_valid == 1);
+#endif
 
-	puts("Writing to Nand... ");
-	if (writeenv(CONFIG_ENV_OFFSET, (u_char *)env_new)) {
-		puts("FAILED!\n");
-		return 1;
-	}
+	ret = erase_and_write_env(&location[env_idx], (u_char *)env_new);
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	if (ret) {
+		env_idx = (env_idx + 1) & 1;
+		ret = erase_and_write_env(&location[env_idx],
+				(u_char *)env_new);
+	} else
+		gd->env_valid = gd->env_valid == 2 ? 1 : 2;
+#endif
 
-	puts("done\n");
 	return ret;
 }
-#endif /* CONFIG_ENV_OFFSET_REDUND */
 #endif /* CMD_SAVEENV */
 
 int readenv(size_t offset, u_char *buf)
-- 
1.7.3.4

[U-Boot] [PATCHv2 2/4] env_nand.c: support falling back to redundant env when writing

[Scott Wood]

On Thu, Feb 21, 2013 at 06:21:54PM +0100, Phil Sutter wrote:
> Without this patch, when the currently chosen environment to be written
> has bad blocks, saveenv fails completely. Instead, when there is
> redundant environment fall back to the other copy. Environment reading
> needs no adjustment, as the fallback logic for incomplete writes applies
> to this case as well.
> 
> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> ---
>  common/env_nand.c |  103 ++++++++++++++++++++++------------------------------
>  1 files changed, 44 insertions(+), 59 deletions(-)
> 
> diff --git a/common/env_nand.c b/common/env_nand.c
> index 22e72a2..c0c985c 100644
> --- a/common/env_nand.c
> +++ b/common/env_nand.c
> @@ -168,72 +168,55 @@ int writeenv(size_t offset, u_char *buf)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_ENV_OFFSET_REDUND
> -static unsigned char env_flags;
> +typedef struct env_location {
> +	const char *name;
> +	nand_erase_options_t *erase_opts;
> +	loff_t offset;
> +} env_location_t;

WARNING: do not add new typedefs
#286: FILE: common/env_nand.c:171:
+typedef struct env_location {

> +	printf("Writing to %s... ", location->name);
> +	if ((ret = writeenv(location->offset, env_new)))
>  		puts("FAILED!\n");
> -		return 1;
> -	}

ERROR: do not use assignment in if condition
#339: FILE: common/env_nand.c:187:
+	if ((ret = writeenv(location->offset, env_new)))


> -
> -	puts("done\n");
> -
> -	gd->env_valid = gd->env_valid == 2 ? 1 : 2;
> +	else
> +		puts("OK\n");
>  
>  	return ret;
>  }
> -#else /* ! CONFIG_ENV_OFFSET_REDUND */
> +
> +static unsigned char env_flags;
> +
>  int saveenv(void)
>  {
>  	int	ret = 0;
>  	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
>  	ssize_t	len;
>  	char	*res;
> +	int	env_idx;
>  	nand_erase_options_t nand_erase_options;
> +	env_location_t location[] = {{
> +		.name = "NAND",
> +		.erase_opts = &nand_erase_options,
> +		.offset = CONFIG_ENV_OFFSET,
> +#ifdef CONFIG_ENV_OFFSET_REDUND
> +	}, {
> +		.name = "redundant NAND",
> +		.erase_opts = &nand_erase_options,
> +		.offset = CONFIG_ENV_OFFSET_REDUND,
> +#endif
> +	}};
> +

ERROR: space required after that close brace '}'
#374: FILE: common/env_nand.c:215:
+	}};

...or rather, it should be like this:

	static const struct env_location location[] = {
		{
			.name = "NAND";
			...
		},
#ifdef CONFIG_ENV_OFFSET_REDUND
		{
			.name = "redundant NAND",
			...
		},
#endif
	};

Note that without the "static" GCC will emit code to dynamically
initialize the array, which will be larger.

> +#ifdef CONFIG_ENV_OFFSET_REDUND
> +	if (ret) {
> +		env_idx = (env_idx + 1) & 1;
> +		ret = erase_and_write_env(&location[env_idx],
> +				(u_char *)env_new);
> +	} else
> +		gd->env_valid = gd->env_valid == 2 ? 1 : 2;

Braces around both halves of the if/else if one side has them.

-Scott

[U-Boot] [PATCHv2 3/4] env_nand.c: clarify log messages when env reading fails

[Phil Sutter]

The single message is misleading, since there is no equivalent success
note when reading the other copy succeeds. Instead, warn if one of the
redundant copies could not be loaded and emphasise on the error when
reading both fails.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 common/env_nand.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/common/env_nand.c b/common/env_nand.c
index c0c985c..60a87ec 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -316,6 +316,7 @@ int get_nand_env_oob(nand_info_t *nand, unsigned long *result)
 void env_relocate_spec(void)
 {
 #if !defined(ENV_IS_EMBEDDED)
+	int read1_fail = 0, read2_fail = 0;
 	int crc1_ok = 0, crc2_ok = 0;
 	env_t *ep, *tmp_env1, *tmp_env2;
 
@@ -327,11 +328,14 @@ void env_relocate_spec(void)
 		goto done;
 	}
 
-	if (readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1))
-		puts("No Valid Environment Area found\n");
+	read1_fail = readenv(CONFIG_ENV_OFFSET, (u_char *) tmp_env1);
+	read2_fail = readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2);
 
-	if (readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2))
-		puts("No Valid Redundant Environment Area found\n");
+	if (read1_fail && read2_fail)
+		puts("*** Error - No Valid Environment Area found\n");
+	else if (read1_fail || read2_fail)
+		puts("*** Warning - some problems detected "
+		     "reading environment; recovered successfully\n");
 
 	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc;
 	crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc;
-- 
1.7.3.4

[U-Boot] [PATCHv2 3/4] env_nand.c: clarify log messages when env reading fails

[Scott Wood]

On Thu, Feb 21, 2013 at 06:21:55PM +0100, Phil Sutter wrote:
> The single message is misleading, since there is no equivalent success
> note when reading the other copy succeeds. Instead, warn if one of the
> redundant copies could not be loaded and emphasise on the error when
> reading both fails.
> 
> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> ---
>  common/env_nand.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)

Applied to u-boot-nand-flash.

> -	if (readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2))
> -		puts("No Valid Redundant Environment Area found\n");
> +	if (read1_fail && read2_fail)
> +		puts("*** Error - No Valid Environment Area found\n");
> +	else if (read1_fail || read2_fail)
> +		puts("*** Warning - some problems detected "
> +		     "reading environment; recovered successfully\n");
>  
>  	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc;
>  	crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc;

We should also give a message if one of the CRCs is bad, though that's
an existing problem.

-Scott

[U-Boot] [PATCHv2 3/4] env_nand.c: clarify log messages when env reading fails

[Phil Sutter]

Scott,

On Fri, Feb 22, 2013 at 07:59:41PM -0600, Scott Wood wrote:
> On Thu, Feb 21, 2013 at 06:21:55PM +0100, Phil Sutter wrote:
> > The single message is misleading, since there is no equivalent success
> > note when reading the other copy succeeds. Instead, warn if one of the
> > redundant copies could not be loaded and emphasise on the error when
> > reading both fails.
> > 
> > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> > ---
> >  common/env_nand.c |   12 ++++++++----
> >  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> Applied to u-boot-nand-flash.
> 
> > -	if (readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2))
> > -		puts("No Valid Redundant Environment Area found\n");
> > +	if (read1_fail && read2_fail)
> > +		puts("*** Error - No Valid Environment Area found\n");
> > +	else if (read1_fail || read2_fail)
> > +		puts("*** Warning - some problems detected "
> > +		     "reading environment; recovered successfully\n");
> >  
> >  	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc;
> >  	crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc;
> 
> We should also give a message if one of the CRCs is bad, though that's
> an existing problem.

Yes, that would be nice. While writing this, I also had the idea of
introducing some macros for unified message output, like so:

| #define __print(level, ...) {
|	printf("*** %s - ", level);
|	printf(__VA_ARGS__);
|	printf("\n");
| }
| #define perror(...) __print("Error", __VA_ARGS__)
| #define pwarn(...) __print("Warning", __VA_ARGS__)
| ...

What do you think? That would require diligently touching a lot of
source files, of course.

Best wishes,

Phil Sutter
Software Engineer

-- 
Viprinet - Never be offline again!

************************************************
Viprinet auf der CeBIT 2013 - 5.-9. M?rz:
Besuchen Sie uns in Halle 13, Stand D27!
Alle gezeigten Produkte im Livebetrieb,
zahlreiche Beispielapplikationen. Gerne
schicken wir Ihnen kostenlose Eintrittskarten.
http://www.viprinet.com/de/cebit-2013
************************************************
Viprinet at CeBIT 2013, March 5 to 9
in Hannover, Germany. Come and visit us
at Hall 13, Booth D27! All exhibits shown
live, many sample applications. We?ll be
happy to send you free admission tickets.
http://www.viprinet.com/en/cebit-2013
************************************************

Viprinet Europe GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:               +49 6721 49030-0
Direct line/Durchwahl:        +49 6721 49030-134
Fax:                          +49 6721 49030-109

phil.sutter at viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein, Germany
Commercial register/Handelsregister: Amtsgericht Mainz HRB44090
CEO/Gesch?ftsf?hrer: Simon Kissel

[U-Boot] [PATCHv2 3/4] env_nand.c: clarify log messages when env reading fails

[Scott Wood]

On 02/25/2013 03:39:12 AM, Phil Sutter wrote:
> Scott,
> 
> On Fri, Feb 22, 2013 at 07:59:41PM -0600, Scott Wood wrote:
> > We should also give a message if one of the CRCs is bad, though  
> that's
> > an existing problem.
> 
> Yes, that would be nice. While writing this, I also had the idea of
> introducing some macros for unified message output, like so:
> 
> | #define __print(level, ...) {
> |	printf("*** %s - ", level);
> |	printf(__VA_ARGS__);
> |	printf("\n");
> | }
> | #define perror(...) __print("Error", __VA_ARGS__)
> | #define pwarn(...) __print("Warning", __VA_ARGS__)
> | ...
> 
> What do you think? That would require diligently touching a lot of
> source files, of course.

This suggestion belongs in its own thread with an appropriate subject,  
but if we do anything like this we should use pr_warn(), pr_err(), etc.  
since we share a bunch of code with Linux -- although output from that  
code may be made worse since it's not expecting the prefixes...

-Scott

[U-Boot] [PATCHv2 4/4] common/env_nand.c: calculate crc only when readenv was OK

[Phil Sutter]

Calculating the checksum of incompletely read data is useless.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 common/env_nand.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/env_nand.c b/common/env_nand.c
index 60a87ec..0e1d17a 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -337,8 +337,8 @@ void env_relocate_spec(void)
 		puts("*** Warning - some problems detected "
 		     "reading environment; recovered successfully\n");
 
-	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc;
-	crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc;
+	crc1_ok = !read1_fail && (crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc);
+	crc2_ok = !read2_fail && (crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc);
 
 	if (!crc1_ok && !crc2_ok) {
 		set_default_env("!bad CRC");
-- 
1.7.3.4

[U-Boot] [PATCHv2 4/4] common/env_nand.c: calculate crc only when readenv was OK

[Scott Wood]

On Thu, Feb 21, 2013 at 06:21:56PM +0100, Phil Sutter wrote:
> Calculating the checksum of incompletely read data is useless.
> 
> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> ---
>  common/env_nand.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/common/env_nand.c b/common/env_nand.c
> index 60a87ec..0e1d17a 100644
> --- a/common/env_nand.c
> +++ b/common/env_nand.c
> @@ -337,8 +337,8 @@ void env_relocate_spec(void)
>  		puts("*** Warning - some problems detected "
>  		     "reading environment; recovered successfully\n");
>  
> -	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc;
> -	crc2_ok = crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc;
> +	crc1_ok = !read1_fail && (crc32(0, tmp_env1->data, ENV_SIZE) == tmp_env1->crc);
> +	crc2_ok = !read2_fail && (crc32(0, tmp_env2->data, ENV_SIZE) == tmp_env2->crc);

Lines over 80 columns -- fixed and applied to u-boot-nand-flash.

-Scott

[U-Boot] Version 3 of Kirkwood and env_nand improvements

[Phil Sutter]

Respin of the remaining two of my patch series for review.  First patch
is identical but removed the versioning from the title as suggested.
Second patch cleaned up so checkpatch.pl does not complain anymore.
Kudos to Scott for pointing out the warnings and his further comments. 

Greetings, Phil

[U-Boot] [PATCH v3 1/2] Optimized nand_read_buf for kirkwood

[Phil Sutter]

The basic idea is taken from the linux-kernel, but further optimized.

First align the buffer to 8 bytes, then use ldrd/strd to read and store
in 8 byte quantities, then do the final bytes.

Tested using: 'date ; nand read.raw 0xE00000 0x0 0x10000 ; date'.
Without this patch, NAND read of 132MB took 49s (~2.69MB/s). With this
patch in place, reading the same amount of data was done in 27s
(~4.89MB/s). So read performance is increased by ~80%!

Signed-off-by: Nico Erfurth <ne@erfurth.eu>
Tested-by: Phil Sutter <phil.sutter@viprinet.com>
Cc: Prafulla Wadaskar <prafulla@marvell.com>
---
 drivers/mtd/nand/kirkwood_nand.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/mtd/nand/kirkwood_nand.c b/drivers/mtd/nand/kirkwood_nand.c
index 0a99a10..85ea5d2 100644
--- a/drivers/mtd/nand/kirkwood_nand.c
+++ b/drivers/mtd/nand/kirkwood_nand.c
@@ -38,6 +38,37 @@ struct kwnandf_registers {
 static struct kwnandf_registers *nf_reg =
 	(struct kwnandf_registers *)KW_NANDF_BASE;
 
+
+/*
+ * The basic idea is stolen from the linux kernel, but the inner loop is
+ * optimized a bit more.
+ */
+static void kw_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+
+	while (len && (unsigned long)buf & 7) {
+		*buf++ = readb(chip->IO_ADDR_R);
+		len--;
+	};
+
+	/* This loop reads and writes 64bit per round. */
+	asm volatile (
+		"1:\n"
+		"  subs   %0, #8\n"
+		"  ldrpld r2, [%2]\n"
+		"  strpld r2, [%1], #8\n"
+		"  bhi    1b\n"
+		"  addne  %0, #8\n"
+		: "+&r" (len), "+&r" (buf)
+		: "r" (chip->IO_ADDR_R)
+		: "r2", "r3", "memory", "cc"
+	);
+
+	while (len--)
+		*buf++ = readb(chip->IO_ADDR_R);
+}
+
 /*
  * hardware specific access to control-lines/bits
  */
@@ -80,6 +111,7 @@ int board_nand_init(struct nand_chip *nand)
 	nand->ecc.mode = NAND_ECC_SOFT;
 #endif
 	nand->cmd_ctrl = kw_nand_hwcontrol;
+	nand->read_buf = kw_nand_read_buf;
 	nand->chip_delay = 40;
 	nand->select_chip = kw_nand_select_chip;
 	return 0;
-- 
1.8.1.5

[U-Boot] [PATCH v3 1/2] Optimized nand_read_buf for kirkwood

[Albert ARIBAUD]

Hi Phil,

On Wed, 26 Jun 2013 20:25:25 +0200, Phil Sutter
<phil.sutter@viprinet.com> wrote:

> The basic idea is taken from the linux-kernel, but further optimized.
> 
> First align the buffer to 8 bytes, then use ldrd/strd to read and store
> in 8 byte quantities, then do the final bytes.
> 
> Tested using: 'date ; nand read.raw 0xE00000 0x0 0x10000 ; date'.
> Without this patch, NAND read of 132MB took 49s (~2.69MB/s). With this
> patch in place, reading the same amount of data was done in 27s
> (~4.89MB/s). So read performance is increased by ~80%!
> 
> Signed-off-by: Nico Erfurth <ne@erfurth.eu>
> Tested-by: Phil Sutter <phil.sutter@viprinet.com>
> Cc: Prafulla Wadaskar <prafulla@marvell.com>
> ---

Patch history missing.

>  drivers/mtd/nand/kirkwood_nand.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/mtd/nand/kirkwood_nand.c b/drivers/mtd/nand/kirkwood_nand.c
> index 0a99a10..85ea5d2 100644
> --- a/drivers/mtd/nand/kirkwood_nand.c
> +++ b/drivers/mtd/nand/kirkwood_nand.c
> @@ -38,6 +38,37 @@ struct kwnandf_registers {
>  static struct kwnandf_registers *nf_reg =
>  	(struct kwnandf_registers *)KW_NANDF_BASE;
>  
> +
> +/*
> + * The basic idea is stolen from the linux kernel, but the inner loop is
> + * optimized a bit more.
> + */
> +static void kw_nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +
> +	while (len && (unsigned long)buf & 7) {
> +		*buf++ = readb(chip->IO_ADDR_R);
> +		len--;
> +	};
> +
> +	/* This loop reads and writes 64bit per round. */
> +	asm volatile (
> +		"1:\n"
> +		"  subs   %0, #8\n"
> +		"  ldrpld r2, [%2]\n"
> +		"  strpld r2, [%1], #8\n"
> +		"  bhi    1b\n"
> +		"  addne  %0, #8\n"
> +		: "+&r" (len), "+&r" (buf)
> +		: "r" (chip->IO_ADDR_R)
> +		: "r2", "r3", "memory", "cc"
> +	);

Are assembler instructions *really* required? IOW, can you not get
enough performance simply with a cleverly written C loop?

> +	while (len--)
> +		*buf++ = readb(chip->IO_ADDR_R);
> +}
> +
>  /*
>   * hardware specific access to control-lines/bits
>   */
> @@ -80,6 +111,7 @@ int board_nand_init(struct nand_chip *nand)
>  	nand->ecc.mode = NAND_ECC_SOFT;
>  #endif
>  	nand->cmd_ctrl = kw_nand_hwcontrol;
> +	nand->read_buf = kw_nand_read_buf;
>  	nand->chip_delay = 40;
>  	nand->select_chip = kw_nand_select_chip;
>  	return 0;


Amicalement,
-- 
Albert.

[U-Boot] [U-Boot,v3,1/2] Optimized nand_read_buf for kirkwood

[Scott Wood]

On Wed, Jun 26, 2013 at 08:25:25PM +0200, Phil Sutter wrote:
> The basic idea is taken from the linux-kernel, but further optimized.
> 
> First align the buffer to 8 bytes, then use ldrd/strd to read and store
> in 8 byte quantities, then do the final bytes.
> 
> Tested using: 'date ; nand read.raw 0xE00000 0x0 0x10000 ; date'.
> Without this patch, NAND read of 132MB took 49s (~2.69MB/s). With this
> patch in place, reading the same amount of data was done in 27s
> (~4.89MB/s). So read performance is increased by ~80%!
> 
> Signed-off-by: Nico Erfurth <ne@erfurth.eu>
> Tested-by: Phil Sutter <phil.sutter@viprinet.com>
> Cc: Prafulla Wadaskar <prafulla@marvell.com>

Missing your signoff, and if Nico was the main author then there should
be a From: line indicating that.

-Scott

[U-Boot] [PATCH v3 2/2] env_nand.c: support falling back to redundant env when writing

[Phil Sutter]

Without this patch, when the currently chosen environment to be written
has bad blocks, saveenv fails completely. Instead, when there is
redundant environment fall back to the other copy. Environment reading
needs no adjustment, as the fallback logic for incomplete writes applies
to this case as well.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 common/env_nand.c | 105 ++++++++++++++++++++++++------------------------------
 1 file changed, 46 insertions(+), 59 deletions(-)

diff --git a/common/env_nand.c b/common/env_nand.c
index b745822..06af5ce 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -168,72 +168,56 @@ int writeenv(size_t offset, u_char *buf)
 	return 0;
 }
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
-static unsigned char env_flags;
+struct env_location {
+	const char *name;
+	nand_erase_options_t *erase_opts;
+	loff_t offset;
+};
 
-int saveenv(void)
+static int erase_and_write_env(struct env_location *location, u_char *env_new)
 {
-	env_t	env_new;
-	ssize_t	len;
-	char	*res;
-	int	ret = 0;
-	nand_erase_options_t nand_erase_options;
-
-	memset(&nand_erase_options, 0, sizeof(nand_erase_options));
-	nand_erase_options.length = CONFIG_ENV_RANGE;
-
-	if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE)
-		return 1;
+	int ret = 0;
 
-	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
-	if (len < 0) {
-		error("Cannot export environment: errno = %d\n", errno);
+	printf("Erasing %s...\n", location->name);
+	location->erase_opts->offset = location->offset;
+	if (nand_erase_opts(&nand_info[0], location->erase_opts))
 		return 1;
-	}
-	env_new.crc	= crc32(0, env_new.data, ENV_SIZE);
-	env_new.flags	= ++env_flags; /* increase the serial */
 
-	if (gd->env_valid == 1) {
-		puts("Erasing redundant NAND...\n");
-		nand_erase_options.offset = CONFIG_ENV_OFFSET_REDUND;
-		if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-			return 1;
-
-		puts("Writing to redundant NAND... ");
-		ret = writeenv(CONFIG_ENV_OFFSET_REDUND, (u_char *)&env_new);
-	} else {
-		puts("Erasing NAND...\n");
-		nand_erase_options.offset = CONFIG_ENV_OFFSET;
-		if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-			return 1;
-
-		puts("Writing to NAND... ");
-		ret = writeenv(CONFIG_ENV_OFFSET, (u_char *)&env_new);
-	}
-	if (ret) {
-		puts("FAILED!\n");
-		return 1;
-	}
-
-	puts("done\n");
-
-	gd->env_valid = gd->env_valid == 2 ? 1 : 2;
+	printf("Writing to %s... ", location->name);
+	ret = writeenv(location->offset, env_new);
+	puts(ret ? "FAILED!\n" : "OK\n");
 
 	return ret;
 }
-#else /* ! CONFIG_ENV_OFFSET_REDUND */
+
+static unsigned char env_flags;
+
 int saveenv(void)
 {
 	int	ret = 0;
 	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
 	ssize_t	len;
 	char	*res;
+	int	env_idx;
 	nand_erase_options_t nand_erase_options;
+	static const struct env_location location[] = {
+		{
+			.name = "NAND",
+			.erase_opts = &nand_erase_options,
+			.offset = CONFIG_ENV_OFFSET,
+		},
+#ifdef CONFIG_ENV_OFFSET_REDUND
+		{
+			.name = "redundant NAND",
+			.erase_opts = &nand_erase_options,
+			.offset = CONFIG_ENV_OFFSET_REDUND,
+		},
+#endif
+	};
+
 
 	memset(&nand_erase_options, 0, sizeof(nand_erase_options));
 	nand_erase_options.length = CONFIG_ENV_RANGE;
-	nand_erase_options.offset = CONFIG_ENV_OFFSET;
 
 	if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE)
 		return 1;
@@ -244,22 +228,25 @@ int saveenv(void)
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;
 	}
-	env_new->crc = crc32(0, env_new->data, ENV_SIZE);
-
-	puts("Erasing Nand...\n");
-	if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-		return 1;
+	env_new->crc   = crc32(0, env_new->data, ENV_SIZE);
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	env_new->flags = ++env_flags; /* increase the serial */
+	env_idx = (gd->env_valid == 1);
+#endif
 
-	puts("Writing to Nand... ");
-	if (writeenv(CONFIG_ENV_OFFSET, (u_char *)env_new)) {
-		puts("FAILED!\n");
-		return 1;
+	ret = erase_and_write_env(&location[env_idx], (u_char *)env_new);
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	if (ret) {
+		env_idx = (env_idx + 1) & 1;
+		ret = erase_and_write_env(&location[env_idx],
+				(u_char *)env_new);
+	} else {
+		gd->env_valid = gd->env_valid == 2 ? 1 : 2;
 	}
+#endif
 
-	puts("done\n");
 	return ret;
 }
-#endif /* CONFIG_ENV_OFFSET_REDUND */
 #endif /* CMD_SAVEENV */
 
 int readenv(size_t offset, u_char *buf)
-- 
1.8.1.5

[U-Boot] [PATCH v3 2/2] env_nand.c: support falling back to redundant env when writing

[Scott Wood]

On 06/26/2013 01:25:26 PM, Phil Sutter wrote:
> Without this patch, when the currently chosen environment to be  
> written
> has bad blocks, saveenv fails completely. Instead, when there is
> redundant environment fall back to the other copy. Environment reading
> needs no adjustment, as the fallback logic for incomplete writes  
> applies
> to this case as well.
> 
> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> ---
>  common/env_nand.c | 105  
> ++++++++++++++++++++++++------------------------------
>  1 file changed, 46 insertions(+), 59 deletions(-)

Missing description of changes since v2

> -#else /* ! CONFIG_ENV_OFFSET_REDUND */
> +
> +static unsigned char env_flags;

env_nand.c:193:22: warning: 'env_flags' defined but not used
[-Wunused-variable]

(when CONFIG_ENV_OFFSET_REDUND is not defined)

>  int saveenv(void)
>  {
>  	int	ret = 0;
>  	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
>  	ssize_t	len;
>  	char	*res;
> +	int	env_idx;
>  	nand_erase_options_t nand_erase_options;
> +	static const struct env_location location[] = {
> +		{
> +			.name = "NAND",
> +			.erase_opts = &nand_erase_options,
> +			.offset = CONFIG_ENV_OFFSET,
> +		},
> +#ifdef CONFIG_ENV_OFFSET_REDUND
> +		{
> +			.name = "redundant NAND",
> +			.erase_opts = &nand_erase_options,
> +			.offset = CONFIG_ENV_OFFSET_REDUND,
> +		},
> +#endif
> +	};
> +

env_nand.c:206:4: error: initializer element is not constant
env_nand.c:206:4: error: (near initialization for  
'location[0].erase_opts')

You could make nand_erase_options static, or you could use code to  
assign
that field.

Is this code untested, or did you accidentally send an old version?

> -	puts("Writing to Nand... ");
> -	if (writeenv(CONFIG_ENV_OFFSET, (u_char *)env_new)) {
> -		puts("FAILED!\n");
> -		return 1;
> +	ret = erase_and_write_env(&location[env_idx], (u_char  
> *)env_new);

env_nand.c:237:2: warning: passing argument 1 of 'erase_and_write_env'
discards 'const' qualifier from pointer target type [enabled by default]
env_nand.c:177:12: note: expected 'struct env_location *' but argument  
is
of type 'const struct env_location *'

> +#ifdef CONFIG_ENV_OFFSET_REDUND
> +	if (ret) {
> +		env_idx = (env_idx + 1) & 1;
> +		ret = erase_and_write_env(&location[env_idx],
> +				(u_char *)env_new);

Can you print a message here specifically saying that redundancy has  
been
lost?  I realize that the previous erase_and_write_env will have printed
"FAILED", but it'd be nice to be explicit about the consequences.

-Scott

[U-Boot] [PATCH v3 2/2] env_nand.c: support falling back to redundant env when writing

[Phil Sutter]

Hi,

On Wed, Jul 17, 2013 at 05:25:31PM -0500, Scott Wood wrote:
> On 06/26/2013 01:25:26 PM, Phil Sutter wrote:
> > Without this patch, when the currently chosen environment to be  
> > written
> > has bad blocks, saveenv fails completely. Instead, when there is
> > redundant environment fall back to the other copy. Environment reading
> > needs no adjustment, as the fallback logic for incomplete writes  
> > applies
> > to this case as well.
> > 
> > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
> > ---
> >  common/env_nand.c | 105  
> > ++++++++++++++++++++++++------------------------------
> >  1 file changed, 46 insertions(+), 59 deletions(-)
> 
> Missing description of changes since v2
> 
> > -#else /* ! CONFIG_ENV_OFFSET_REDUND */
> > +
> > +static unsigned char env_flags;
> 
> env_nand.c:193:22: warning: 'env_flags' defined but not used
> [-Wunused-variable]
> 
> (when CONFIG_ENV_OFFSET_REDUND is not defined)
> 
> >  int saveenv(void)
> >  {
> >  	int	ret = 0;
> >  	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
> >  	ssize_t	len;
> >  	char	*res;
> > +	int	env_idx;
> >  	nand_erase_options_t nand_erase_options;
> > +	static const struct env_location location[] = {
> > +		{
> > +			.name = "NAND",
> > +			.erase_opts = &nand_erase_options,
> > +			.offset = CONFIG_ENV_OFFSET,
> > +		},
> > +#ifdef CONFIG_ENV_OFFSET_REDUND
> > +		{
> > +			.name = "redundant NAND",
> > +			.erase_opts = &nand_erase_options,
> > +			.offset = CONFIG_ENV_OFFSET_REDUND,
> > +		},
> > +#endif
> > +	};
> > +
> 
> env_nand.c:206:4: error: initializer element is not constant
> env_nand.c:206:4: error: (near initialization for  
> 'location[0].erase_opts')
> 
> You could make nand_erase_options static, or you could use code to  
> assign
> that field.
> 
> Is this code untested, or did you accidentally send an old version?

Yes, indeed. My apologies for not having tested this, seems like a
combination of too much belief in your builtin parser and failure of
mine.

Corrected version which incorporates your suggestions and tries to solve
the issue above in a cleaner way follows. And yes, this time it has even
been tested.

Greetings, Phil

-- 
Viprinet Europe GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:               +49 6721 49030-0
Direct line/Durchwahl:        +49 6721 49030-134
Fax:                          +49 6721 49030-109

phil.sutter at viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein, Germany
Commercial register/Handelsregister: Amtsgericht Mainz HRB44090
CEO/Gesch?ftsf?hrer: Simon Kissel

[U-Boot] [PATCH] env_nand.c: support falling back to redundant env when writing

[Phil Sutter]

Without this patch, when the currently chosen environment to be written
has bad blocks, saveenv fails completely. Instead, when there is
redundant environment fall back to the other copy. Environment reading
needs no adjustment, as the fallback logic for incomplete writes applies
to this case as well.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
Changes since V1:
- Print status message for each of the redundant copies when writing, so
  the error message for any first failed attempt does not confuse that
  much.

Changes since V2:
- Checkpatch-compliance:
  - do not typedef struct env_location
  - avoid assignments in conditionals
  - some minor formatting corrections
- make location array static const

Changes since V3:
- constify field 'erase_opts' in struct env_location, no need to assign
  to it at run-time
- drop unnecessary field 'offset' in struct env_location
- function 'erase_and_write_env' must take passed struct env_location as
  const
- declare a global var 'env_flags' only if CONFIG_ENV_OFFSET_REDUND is
  defined
- warn if first attempt to write env failed
---
 common/env_nand.c | 116 +++++++++++++++++++++++++-----------------------------
 1 file changed, 54 insertions(+), 62 deletions(-)

diff --git a/common/env_nand.c b/common/env_nand.c
index b745822..db7da37 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -168,72 +168,57 @@ int writeenv(size_t offset, u_char *buf)
 	return 0;
 }
 
-#ifdef CONFIG_ENV_OFFSET_REDUND
-static unsigned char env_flags;
+struct env_location {
+	const char *name;
+	const nand_erase_options_t erase_opts;
+};
 
-int saveenv(void)
+static int erase_and_write_env(const struct env_location *location,
+		u_char *env_new)
 {
-	env_t	env_new;
-	ssize_t	len;
-	char	*res;
-	int	ret = 0;
-	nand_erase_options_t nand_erase_options;
+	int ret = 0;
 
-	memset(&nand_erase_options, 0, sizeof(nand_erase_options));
-	nand_erase_options.length = CONFIG_ENV_RANGE;
-
-	if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE)
-		return 1;
-
-	res = (char *)&env_new.data;
-	len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
-	if (len < 0) {
-		error("Cannot export environment: errno = %d\n", errno);
-		return 1;
-	}
-	env_new.crc	= crc32(0, env_new.data, ENV_SIZE);
-	env_new.flags	= ++env_flags; /* increase the serial */
-
-	if (gd->env_valid == 1) {
-		puts("Erasing redundant NAND...\n");
-		nand_erase_options.offset = CONFIG_ENV_OFFSET_REDUND;
-		if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-			return 1;
-
-		puts("Writing to redundant NAND... ");
-		ret = writeenv(CONFIG_ENV_OFFSET_REDUND, (u_char *)&env_new);
-	} else {
-		puts("Erasing NAND...\n");
-		nand_erase_options.offset = CONFIG_ENV_OFFSET;
-		if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-			return 1;
-
-		puts("Writing to NAND... ");
-		ret = writeenv(CONFIG_ENV_OFFSET, (u_char *)&env_new);
-	}
-	if (ret) {
-		puts("FAILED!\n");
+	printf("Erasing %s...\n", location->name);
+	if (nand_erase_opts(&nand_info[0], &location->erase_opts))
 		return 1;
-	}
-
-	puts("done\n");
 
-	gd->env_valid = gd->env_valid == 2 ? 1 : 2;
+	printf("Writing to %s... ", location->name);
+	ret = writeenv(location->erase_opts.offset, env_new);
+	puts(ret ? "FAILED!\n" : "OK\n");
 
 	return ret;
 }
-#else /* ! CONFIG_ENV_OFFSET_REDUND */
+
+#ifdef CONFIG_ENV_OFFSET_REDUND
+static unsigned char env_flags;
+#endif
+
 int saveenv(void)
 {
 	int	ret = 0;
 	ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1);
 	ssize_t	len;
 	char	*res;
-	nand_erase_options_t nand_erase_options;
+	int	env_idx = 0;
+	static const struct env_location location[] = {
+		{
+			.name = "NAND",
+			.erase_opts = {
+				.length = CONFIG_ENV_RANGE,
+				.offset = CONFIG_ENV_OFFSET,
+			},
+		},
+#ifdef CONFIG_ENV_OFFSET_REDUND
+		{
+			.name = "redundant NAND",
+			.erase_opts = {
+				.length = CONFIG_ENV_RANGE,
+				.offset = CONFIG_ENV_OFFSET_REDUND,
+			},
+		},
+#endif
+	};
 
-	memset(&nand_erase_options, 0, sizeof(nand_erase_options));
-	nand_erase_options.length = CONFIG_ENV_RANGE;
-	nand_erase_options.offset = CONFIG_ENV_OFFSET;
 
 	if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE)
 		return 1;
@@ -244,22 +229,29 @@ int saveenv(void)
 		error("Cannot export environment: errno = %d\n", errno);
 		return 1;
 	}
-	env_new->crc = crc32(0, env_new->data, ENV_SIZE);
-
-	puts("Erasing Nand...\n");
-	if (nand_erase_opts(&nand_info[0], &nand_erase_options))
-		return 1;
+	env_new->crc   = crc32(0, env_new->data, ENV_SIZE);
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	env_new->flags = ++env_flags; /* increase the serial */
+	env_idx = (gd->env_valid == 1);
+#endif
 
-	puts("Writing to Nand... ");
-	if (writeenv(CONFIG_ENV_OFFSET, (u_char *)env_new)) {
-		puts("FAILED!\n");
-		return 1;
+	ret = erase_and_write_env(&location[env_idx], (u_char *)env_new);
+#ifdef CONFIG_ENV_OFFSET_REDUND
+	if (!ret) {
+		/* preset other copy for next write */
+		gd->env_valid = gd->env_valid == 2 ? 1 : 2;
+		return ret;
 	}
 
-	puts("done\n");
+	env_idx = (env_idx + 1) & 1;
+	ret = erase_and_write_env(&location[env_idx], (u_char *)env_new);
+	if (!ret)
+		printf("Warning: primary env write failed,"
+				" redundancy is lost!\n");
+#endif
+
 	return ret;
 }
-#endif /* CONFIG_ENV_OFFSET_REDUND */
 #endif /* CMD_SAVEENV */
 
 int readenv(size_t offset, u_char *buf)
-- 
1.8.1.5

[U-Boot] [PATCH] env_nand.c: support falling back to redundant env when writing

[Phil Sutter]

On Fri, Jul 19, 2013 at 12:20:26PM +0200, Phil Sutter wrote:
Subject: [U-Boot] [PATCH] env_nand.c: support falling back to redundant
                   ~~~~~

Heck, PATCHv4 that is.

-- 
Viprinet Europe GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:               +49 6721 49030-0
Direct line/Durchwahl:        +49 6721 49030-134
Fax:                          +49 6721 49030-109

phil.sutter at viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein, Germany
Commercial register/Handelsregister: Amtsgericht Mainz HRB44090
CEO/Gesch?ftsf?hrer: Simon Kissel

[U-Boot] env_nand.c: support falling back to redundant env when writing

[Scott Wood]

On Fri, Jul 19, 2013 at 12:20:26PM +0200, Phil Sutter wrote:
> Without this patch, when the currently chosen environment to be written
> has bad blocks, saveenv fails completely. Instead, when there is
> redundant environment fall back to the other copy. Environment reading
> needs no adjustment, as the fallback logic for incomplete writes applies
> to this case as well.
> 
> Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>

Applied to u-boot-nand-flash

-Scott