Optimized nand_read_buf for kirkwood

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 <[email protected]>

Quoted 8 lines · 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.

Quoted 1 line

Signed-off-by Phil Sutter <[email protected]>

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.

Hi,

Quoted 5 lines · Scott Wood

Quoted 4 lines · 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

Quoted 6 lines

Quoted 5 lines

needs no adjustment, as the fallback logic for incomplete writes applies
to this case as well.

Signed-off-by Phil Sutter <[email protected]>

Quoted 1 line

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.

Quoted 3 lines

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. ;)

Quoted 17 lines · Phil Sutter

Hi,

Quoted 5 lines · Scott Wood

Quoted 4 lines · 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

Quoted 7 lines

Quoted 5 lines

needs no adjustment, as the fallback logic for incomplete writes applies
to this case as well.

Signed-off-by Phil Sutter <[email protected]>

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

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 9 lines

Quoted 3 lines

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.

Quoted 25 lines · Scott Wood

Quoted 23 lines · Phil Sutter

Hi,

Quoted 5 lines · Scott Wood

Quoted 4 lines · 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

Quoted 7 lines

Quoted 5 lines

needs no adjustment, as the fallback logic for incomplete writes applies
to this case as well.

Signed-off-by Phil Sutter <[email protected]>

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.

Quoted 1 line

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.

Scott,

Quoted 7 lines · Phil Sutter

Quoted 6 lines · Scott Wood

Quoted 5 lines · Phil Sutter

Hi,

Quoted 2 lines · Scott Wood

Quoted 1 line · Phil Sutter

Without this patch, when the currently chosen environment

to be

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

written
has bad blocks, saveenv fails completely. Instead, when

there is

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

redundant environment fall back to the other copy.

Environment

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

reading

Quoted 1 line

Quoted 1 line

needs no adjustment, as the fallback logic for incomplete

writes

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

applies
to this case as well.

Signed-off-by Phil Sutter <[email protected]>

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

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

this.
But given the fact that CONFIG_ENV_RANGE needs to span multiple

erase

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

redundant environment to be really sure.

And *that* is what CONFIG_ENV_OFFSET_OOB is supposed to deal

with. :-)

Quoted 1 line

Quoted 1 line

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.

Quoted 42 lines · Phil Sutter

Scott,

Quoted 7 lines · Phil Sutter

Quoted 6 lines · Scott Wood

Quoted 5 lines · Phil Sutter

Hi,

Quoted 2 lines · Scott Wood

Quoted 1 line · Phil Sutter

Without this patch, when the currently chosen environment

to be

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

written
has bad blocks, saveenv fails completely. Instead, when

there is

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

redundant environment fall back to the other copy.

Environment

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

reading

Quoted 1 line

Quoted 1 line

needs no adjustment, as the fallback logic for incomplete

writes

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

applies
to this case as well.

Signed-off-by Phil Sutter <[email protected]>

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

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

this.
But given the fact that CONFIG_ENV_RANGE needs to span multiple

erase

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

redundant environment to be really sure.

And *that* is what CONFIG_ENV_OFFSET_OOB is supposed to deal

with. :-)

Quoted 2 lines

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

for.

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 2 lines · Scott Wood

Quoted 1 line · Phil Sutter

Hmm. Does not look like CONFIG_ENV_OFFSET_OOB is used to select

the

Quoted 2 lines

Quoted 2 lines

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

Quoted 3 lines

Quoted 2 lines

to
be in erase page unit.

Quoted 2 lines

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.

Quoted 1 line

The block to hold the environment is stored in the OOB of block

zero,

Quoted 1 line

which is usually guaranteed to not be bad.

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

Quoted 8 lines

Quoted 5 lines

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

Quoted 1 line

process rather than being built into U-Boot), as U-Boot wouldn't

know

Quoted 1 line

how to give back unused blocks to other purposes.

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

Quoted 13 lines · Phil Sutter

Quoted 2 lines · Scott Wood

Quoted 1 line · Phil Sutter

Hmm. Does not look like CONFIG_ENV_OFFSET_OOB is used to select

the

Quoted 2 lines

Quoted 2 lines

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

Quoted 1 line

The block to hold the environment is stored in the OOB of block

zero,

Quoted 1 line

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.

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

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

Quoted 7 lines

preparation

Quoted 1 line

process rather than being built into U-Boot), as U-Boot wouldn't

know

Quoted 1 line

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?

Quoted 1 line · Scott Wood

Erase blocks are larger than write pages, yes. I've never heard

erase

Quoted 2 lines

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?

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

The block to hold the environment is stored in the OOB of block

zero,

Quoted 1 line

which is usually guaranteed to not be bad.

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

OOB.

Quoted 5 lines

"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

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

So that assumes that any block initially identified 'good' will

ever

Quoted 8 lines

Quoted 1 line

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.

Quoted 3 lines

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

Quoted 6 lines

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. :)

Quoted 12 lines · Phil Sutter

Quoted 1 line · Scott Wood

Erase blocks are larger than write pages, yes. I've never heard

erase

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

The block to hold the environment is stored in the OOB of block

zero,

Quoted 1 line

which is usually guaranteed to not be bad.

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

OOB.

Quoted 5 lines

"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

Quoted 1 line

or the second page of the erase block.

Quoted 4 lines

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"?

Quoted 11 lines

Quoted 1 line

Quoted 1 line

So that assumes that any block initially identified 'good' will

ever

Quoted 8 lines

Quoted 1 line

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.

Quoted 5 lines

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?

Quoted 18 lines

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 5 lines · Scott Wood

Quoted 4 lines · Phil Sutter

Quoted 1 line · Scott Wood

Erase blocks are larger than write pages, yes. I've never heard

erase

Quoted 1 line

blocks called "pages" or write pages called "blocks" -- but my

main

Quoted 1 line

Quoted 1 line

Quoted 1 line

point is that the unit of erasing and the unit of badness are

the

Quoted 3 lines

Quoted 3 lines

same.

Ah, OK. Please excuse my humble nomenclature, I never cared

enough to

Quoted 1 line

Quoted 1 line

sort out what is called what. Of course, this is not the best

basis

Quoted 4 lines

Quoted 4 lines

for
a discussion about these things.

But getting back to the topic: The assumption of blocks getting

bad,

Quoted 2 lines

Quoted 2 lines

not
pages within a block means that for any kind of bad block

prevention,

Quoted 1 line

Quoted 1 line

multiple blocks need to be used. Although I'm honestly speaking

not

Quoted 1 line

Quoted 1 line

really sure why this needs to be like that. Maybe the bad page

marking

Quoted 3 lines

Quoted 1 line

would disappear when erasing the block it belongs to?

Yes, it would disappear. This is why erase operations skip bad

blocks,

Quoted 1 line

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?

Quoted 1 line

Quoted 1 line

Interesting. I had the impression of pages being marked bad and

the

Quoted 4 lines

Quoted 3 lines

block's badness being taken from whether it contains bad pages. Probably
the 'nand markbad' command tricked me.

Quoted 1 line

Do you mean the lack of error checking if you pass a

non-block-aligned

Quoted 1 line

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.

Quoted 1 line

Quoted 1 line

Well, as long as CONFIG_ENV_OFFSET_REDUND supported falling back

to

Quoted 2 lines

Quoted 2 lines

the
other copy in case of error there would be a working system in

three

Quoted 4 lines

Quoted 2 lines

of
four cases instead of only one.

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

"one"

Quoted 1 line

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.

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

Signed-off-by Phil Sutter <[email protected]>

---
 common/env_nand.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

common/env_nand.c +1 -0

index 895532b..58ba558 100644
@@ -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;

Quoted 25 lines · Phil Sutter

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

Signed-off-by Phil Sutter <[email protected]>

---
 common/env_nand.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

common/env_nand.c +1 -0

index 895532b..58ba558 100644
@@ -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;
 	}
 

common/env_nand.c void env_relocate_spec(void)

Quoted 5 lines

common/env_nand.c +2 -2

-	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?

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

Signed-off-by Phil Sutter <[email protected]>

---
 common/env_nand.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

common/env_nand.c +5 -4

index 895532b..58ba558 100644
@@ -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;

-----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 <[email protected]>

Cc Prafulla Wadaskar <[email protected]>

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

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 <[email protected]>

Tested-by Phil Sutter <[email protected]>

Cc Prafulla Wadaskar <[email protected]>

---
 drivers/mtd/nand/kirkwood_nand.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

drivers/mtd/nand/kirkwood_nand.c +2 -0

index bdab5aa..e04a59f 100644
@@ -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)

drivers/mtd/nand/kirkwood_nand.c +10 -0

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

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

Quoted 1 line · Phil Sutter

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

optimized.

Quoted 7 lines

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

Quoted 8 lines

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 <[email protected]>

Tested-by Phil Sutter <[email protected]>

Cc Prafulla Wadaskar <[email protected]>

---
 drivers/mtd/nand/kirkwood_nand.c |   29

+++++++++++++++++++++++++++++

Quoted 13 lines

 1 files changed, 29 insertions(+), 0 deletions(-)

drivers/mtd/nand/kirkwood_nand.c +2 -0

index bdab5aa..e04a59f 100644
@@ -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

Quoted 2 lines

loop is optimized a bit more */
+static void kw_nand_read_buf(struct mtd_info *mtd, uint8_t *buf,

int

drivers/mtd/nand/kirkwood_nand.c struct kwnandf_registers { · line 48

Quoted 6 lines

len)

drivers/mtd/nand/kirkwood_nand.c +5 -0

+{
+	struct nand_chip *chip = mtd->priv;
+
+	while (len && (unsigned long)buf & 7)
+	{

Brace goes on the previous line.

Quoted 11 lines

drivers/mtd/nand/kirkwood_nand.c +10 -0

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

drivers/mtd/nand/kirkwood_nand.c struct kwnandf_registers { · line 63

Quoted 5 lines

drivers/mtd/nand/kirkwood_nand.c +5 -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 mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

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 <[email protected]>

Tested-by Phil Sutter <[email protected]>

Cc Prafulla Wadaskar <[email protected]>

---
 drivers/mtd/nand/kirkwood_nand.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

drivers/mtd/nand/kirkwood_nand.c +20 -0

index bdab5aa..99e5f35 100644
@@ -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"

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 <[email protected]>

---
 common/env_nand.c |  103 ++++++++++++++++++++++------------------------------
 1 files changed, 44 insertions(+), 59 deletions(-)

common/env_nand.c +0 -0

index 22e72a2..c0c985c 100644

common/env_nand.c +8 -5

-
-	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",

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 <[email protected]>

---
 common/env_nand.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

Quoted 9 lines · 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 <[email protected]>

---
 common/env_nand.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

Applied to u-boot-nand-flash.

Quoted 8 lines

common/env_nand.c +5 -2

-	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");

common/env_nand.c void env_relocate_spec(void) · line 341

Quoted 2 lines

common/env_nand.c +0 -0

 	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,

Quoted 1 line · Scott Wood

We should also give a message if one of the CRCs is bad, though

that's

Quoted 1 line

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.

Calculating the checksum of incompletely read data is useless.

Signed-off-by Phil Sutter <[email protected]>

---
 common/env_nand.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

common/env_nand.c +0 -0

index 60a87ec..0e1d17a 100644

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 <[email protected]>

Tested-by Phil Sutter <[email protected]>

Cc Prafulla Wadaskar <[email protected]>

---

 drivers/mtd/nand/kirkwood_nand.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

drivers/mtd/nand/kirkwood_nand.c +16 -0

index 0a99a10..85ea5d2 100644
@@ -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 (

drivers/mtd/nand/kirkwood_nand.c +4 -0

+	while (len--)
+		*buf++ = readb(chip->IO_ADDR_R);
+}
+
 /*
  * hardware specific access to control-lines/bits
  */

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 <[email protected]>

Tested-by Phil Sutter <[email protected]>

Cc Prafulla Wadaskar <[email protected]>

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 <[email protected]>

---
 common/env_nand.c | 105  
++++++++++++++++++++++++------------------------------
 1 file changed, 46 insertions(+), 59 deletions(-)

int saveenv(void)
{

common/env_nand.c +6 -0

 	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,

common/env_nand.c +1 -4

-	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);

Quoted 8 lines · 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.

Quoted 5 lines

Signed-off-by Phil Sutter <[email protected]>

---
 common/env_nand.c | 105  
++++++++++++++++++++++++------------------------------
 1 file changed, 46 insertions(+), 59 deletions(-)

Missing description of changes since v2

common/env_nand.c int writeenv(size_t offset, u_char *buf) · line 193

Quoted 3 lines

common/env_nand.c +2 -1

-#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)

Quoted 13 lines

int saveenv(void)
{

common/env_nand.c +6 -0

 	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,

common/env_nand.c int writeenv(size_t offset, u_char *buf) · line 217

Quoted 10 lines

common/env_nand.c +10 -0

+		},
+#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?

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 <[email protected]>