[U-Boot] [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 --- 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;
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)
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
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
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
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
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
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
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
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
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
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
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
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
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;
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
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
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
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");
-----Original Message----- From: Phil Sutter [mailto:phil.sutter@viprinet.com] Sent: 21 November 2012 18:29 To: u-boot@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 . . .
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
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;
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 oneadditional 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
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot- bounces@lists.denx.de] On Behalf Of Scott Wood Sent: 27 November 2012 05:09 To: Phil Sutter Cc: u-boot@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 oneadditional 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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
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
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.
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;
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
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)
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
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;
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
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
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
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");
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
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
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;
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,
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
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)
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
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
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)
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.
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
participants (4)
-
Albert ARIBAUD -
Phil Sutter -
Prafulla Wadaskar -
Scott Wood