Re: [U-Boot] [U-Boot, U-boot, 1/2] common: cmd_nand: add nand ecclayout command
On 06/20/2014 04:00 AM, Scott Wood wrote:
On Fri, May 16, 2014 at 09:26:36PM +0300, Khoronzhuk, Ivan wrote:
From: WingMan Kwok w-kw...@ti.com
This commit adds a nand ecclayout command that allows the ecclayout of
the current nand device to be changed during run time. This feature is
useful when using u-boot to write something to nand flash that will be
read by other applications, such as ROM bootloader, that expects a
different ECC layout. In that case, change the current nand device
ecclayout using the nand ecclayout set command before writing the
data to nand flash.
Signed-off-by: WingMan Kwok w-kw...@ti.com
Signed-off-by: Ivan Khoronzhuk ivan.khoronz...@ti.com
---
common/cmd_nand.c | 115 +-
include/nand.h| 10 +
2 files changed, 124 insertions(+), 1 deletion(-)
diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 04ab0f1..7cbe6fc 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -462,6 +462,53 @@ static void adjust_size_for_badblocks(loff_t *size, loff_t
offset, int dev)
}
}
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+static void nand_print_ecclayout_info(struct nand_ecclayout *p)
+{
+ int i;
+ struct nand_oobfree *oobfree;
+
+ if (!p)
+ return;
+
+ printf( num ecc bytes: %d\n, p-eccbytes);
+ puts( ecc pos:\n);
+
+ for (i = 0; i p-eccbytes; i++) {
+ if (i !(i % 9))
+ printf(\n);
+
+ printf(%2d , p-eccpos[i]);
+ }
Why 9?
It's to print a new line on every 9th character position.
I'll add a comment.
+ puts(\n oobfree:\n);
+ puts(offset length\n);
+
+ oobfree = p-oobfree;
+ for (i = 0; oobfree-length i MTD_MAX_OOBFREE_ENTRIES_LARGE; i++) {
+ printf( %2d%2d\n, oobfree-offset, oobfree-length);
+ oobfree++;
+ }
+}
+
+static void nand_print_device_ecclayout(int dev)
+{
+ int idx;
+ nand_info_t *nand = nand_info[dev];
+ struct nand_chip *chip = nand-priv;
+
+ idx = board_nand_ecclayout_get_idx(chip, chip-ecc.layout);
+
+ if (idx 0) {
+ puts(no ecc layout\n);
+ return;
+ }
+
+ printf(\necc layout %d:\n, idx);
+ nand_print_ecclayout_info(chip-ecc.layout);
+}
+#endif
+
static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
{
int i, ret = 0;
@@ -506,8 +553,12 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])
putc('\n');
if (dev 0 || dev = CONFIG_SYS_MAX_NAND_DEVICE)
puts(no devices available\n);
- else
+ else {
nand_print_and_set_info(dev);
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+ nand_print_device_ecclayout(dev);
+#endif
+ }
return 0;
}
Braces are needed on both sides of if/else if used on one side.
Yes.
It would be better to provide an inline no-op stub when
CONFIG_CMD_NAND_ECCLAYOUT, than ifdeffing at the call site.
Yes. It's better.
Does it really make sense to dump this information here, when the user
could use the ecclayout command instead?
Yes. Seems it's redundant. I'll better delete it.
@@ -836,6 +887,60 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])
}
#endif
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+ if (strcmp(cmd, ecclayout) == 0) {
+ int idx;
+ struct nand_ecclayout *p;
+ nand_info_t *nand = nand_info[dev];
+ struct nand_chip *chip = nand-priv;
+
+ if (argc 3) {
+ puts(Current device ecclayout:\n);
+ nand_print_device_ecclayout(dev);
+ return 0;
+ }
+
+ if (!strcmp(argv[2], set)) {
+ if (argc 4)
+ return 1;
+
+ idx = (int)simple_strtoul(argv[3], NULL, 10);
+ if (!board_nand_ecclayout_set(chip, idx)) {
+ p = chip-ecc.layout;
+ p-oobavail = 0;
+ for (i = 0; p-oobfree[i].length
+ i ARRAY_SIZE(p-oobfree); i++)
+ p-oobavail += p-oobfree[i].length;
+
+ nand-oobavail = p-oobavail;
Shouldn't board_nand_ecclayout_set() take care of this? Possibly via
common helper functions, but not here.
Yes, It should be moved to board_nand_ecclayout_set()
+ } else {
+ printf(Setting current device
+ to ecc layout %d FAILED!\n, idx);
Don't wrap
Re: [U-Boot] [U-Boot, U-boot, 1/2] common: cmd_nand: add nand ecclayout command
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -462,6 +462,53 @@ static void adjust_size_for_badblocks(loff_t *size,
+ for (i = 0; i p-eccbytes; i++) {
+ if (i !(i % 9))
+ printf(\n);
+
+ printf(%2d , p-eccpos[i]);
+ }
Why 9?
It's to print a new line on every 9th character position.
I'll add a comment.
OK, Scott, breath... I got this one. It'll be OK...
Ivan,
I am confident Scott understood that a newline would
be generated every ninth-character. We all get that. I think
what Scott was asking was why the value 9 was chosen?
Why not 10? Or 8? Or 145? Was it to fit some arbitrary
line length or screen size? Would it make more sense to
use something familiar like a base 10 or half of base-16?
Thanks,
jdl
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [U-Boot, U-boot, 1/2] common: cmd_nand: add nand ecclayout command
On Fri, 2014-06-20 at 09:10 -0500, Jon Loeliger wrote:
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -462,6 +462,53 @@ static void adjust_size_for_badblocks(loff_t *size,
+ for (i = 0; i p-eccbytes; i++) {
+ if (i !(i % 9))
+ printf(\n);
+
+ printf(%2d , p-eccpos[i]);
+ }
Why 9?
It's to print a new line on every 9th character position.
I'll add a comment.
OK, Scott, breath... I got this one. It'll be OK...
Ivan,
I am confident Scott understood that a newline would
be generated every ninth-character. We all get that. I think
what Scott was asking was why the value 9 was chosen?
Why not 10? Or 8? Or 145? Was it to fit some arbitrary
line length or screen size? Would it make more sense to
use something familiar like a base 10 or half of base-16?
More specifically, it neither avoids a division (as a power of two
would) nor does it seem to match the ecc size of the layouts used by the
davinci driver (which is the only user of this so far), nor is it
anywhere near 80 columns.
Also, why is the field width two characters, when ecc positions can
exceed 100?
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [U-Boot, U-boot, 1/2] common: cmd_nand: add nand ecclayout command
On Fri, 2014-06-20 at 16:29 +0300, Ivan Khoronzhuk wrote:
On 06/20/2014 04:00 AM, Scott Wood wrote:
On Fri, May 16, 2014 at 09:26:36PM +0300, Khoronzhuk, Ivan wrote:
+ } else {
+ printf(Setting current device
+ to ecc layout %d FAILED!\n, idx);
Don't wrap user-visible strings (checkpatch should have warned you about
this).
For user's eyes it's not wrapped.
That's not the point. Wrapping it in the source means the user can't
grep for the string. Checkpatch should have issued a warning about
this. This is an exception to the 80 column rule.
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [U-Boot, U-boot, 1/2] common: cmd_nand: add nand ecclayout command
On 06/20/2014 07:31 PM, Scott Wood wrote:
On Fri, 2014-06-20 at 16:29 +0300, Ivan Khoronzhuk wrote:
On 06/20/2014 04:00 AM, Scott Wood wrote:
On Fri, May 16, 2014 at 09:26:36PM +0300, Khoronzhuk, Ivan wrote:
+ } else {
+ printf(Setting current device
+ to ecc layout %d FAILED!\n, idx);
Don't wrap user-visible strings (checkpatch should have warned you about
this).
For user's eyes it's not wrapped.
That's not the point. Wrapping it in the source means the user can't
grep for the string. Checkpatch should have issued a warning about
this. This is an exception to the 80 column rule.
-Scott
Ok.
--
Regards,
Ivan Khoronzhuk
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [U-Boot, U-boot, 1/2] common: cmd_nand: add nand ecclayout command
On 06/20/2014 07:03 PM, Scott Wood wrote:
On Fri, 2014-06-20 at 09:10 -0500, Jon Loeliger wrote:
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -462,6 +462,53 @@ static void adjust_size_for_badblocks(loff_t *size,
+ for (i = 0; i p-eccbytes; i++) {
+ if (i !(i % 9))
+ printf(\n);
+
+ printf(%2d , p-eccpos[i]);
+ }
Why 9?
It's to print a new line on every 9th character position.
I'll add a comment.
OK, Scott, breath... I got this one. It'll be OK...
Ivan,
I am confident Scott understood that a newline would
be generated every ninth-character. We all get that. I think
what Scott was asking was why the value 9 was chosen?
Why not 10? Or 8? Or 145? Was it to fit some arbitrary
line length or screen size? Would it make more sense to
use something familiar like a base 10 or half of base-16?
More specifically, it neither avoids a division (as a power of two
would) nor does it seem to match the ecc size of the layouts used by the
davinci driver (which is the only user of this so far), nor is it
anywhere near 80 columns.
Also, why is the field width two characters, when ecc positions can
exceed 100?
-Scott
Ok, I'll try to correct as you proposed. I dislike it also ...
Thanks.
--
Regards,
Ivan Khoronzhuk
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [U-Boot, U-boot, 1/2] common: cmd_nand: add nand ecclayout command
On Fri, May 16, 2014 at 09:26:36PM +0300, Khoronzhuk, Ivan wrote:
From: WingMan Kwok w-kw...@ti.com
This commit adds a nand ecclayout command that allows the ecclayout of
the current nand device to be changed during run time. This feature is
useful when using u-boot to write something to nand flash that will be
read by other applications, such as ROM bootloader, that expects a
different ECC layout. In that case, change the current nand device
ecclayout using the nand ecclayout set command before writing the
data to nand flash.
Signed-off-by: WingMan Kwok w-kw...@ti.com
Signed-off-by: Ivan Khoronzhuk ivan.khoronz...@ti.com
---
common/cmd_nand.c | 115 +-
include/nand.h| 10 +
2 files changed, 124 insertions(+), 1 deletion(-)
diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 04ab0f1..7cbe6fc 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -462,6 +462,53 @@ static void adjust_size_for_badblocks(loff_t *size,
loff_t offset, int dev)
}
}
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+static void nand_print_ecclayout_info(struct nand_ecclayout *p)
+{
+ int i;
+ struct nand_oobfree *oobfree;
+
+ if (!p)
+ return;
+
+ printf( num ecc bytes: %d\n, p-eccbytes);
+ puts( ecc pos:\n);
+
+ for (i = 0; i p-eccbytes; i++) {
+ if (i !(i % 9))
+ printf(\n);
+
+ printf(%2d , p-eccpos[i]);
+ }
Why 9?
+ puts(\n oobfree:\n);
+ puts(offset length\n);
+
+ oobfree = p-oobfree;
+ for (i = 0; oobfree-length i MTD_MAX_OOBFREE_ENTRIES_LARGE; i++) {
+ printf( %2d%2d\n, oobfree-offset, oobfree-length);
+ oobfree++;
+ }
+}
+
+static void nand_print_device_ecclayout(int dev)
+{
+ int idx;
+ nand_info_t *nand = nand_info[dev];
+ struct nand_chip *chip = nand-priv;
+
+ idx = board_nand_ecclayout_get_idx(chip, chip-ecc.layout);
+
+ if (idx 0) {
+ puts(no ecc layout\n);
+ return;
+ }
+
+ printf(\necc layout %d:\n, idx);
+ nand_print_ecclayout_info(chip-ecc.layout);
+}
+#endif
+
static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
{
int i, ret = 0;
@@ -506,8 +553,12 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])
putc('\n');
if (dev 0 || dev = CONFIG_SYS_MAX_NAND_DEVICE)
puts(no devices available\n);
- else
+ else {
nand_print_and_set_info(dev);
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+ nand_print_device_ecclayout(dev);
+#endif
+ }
return 0;
}
Braces are needed on both sides of if/else if used on one side.
It would be better to provide an inline no-op stub when
CONFIG_CMD_NAND_ECCLAYOUT, than ifdeffing at the call site.
Does it really make sense to dump this information here, when the user
could use the ecclayout command instead?
@@ -836,6 +887,60 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])
}
#endif
+#ifdef CONFIG_CMD_NAND_ECCLAYOUT
+ if (strcmp(cmd, ecclayout) == 0) {
+ int idx;
+ struct nand_ecclayout *p;
+ nand_info_t *nand = nand_info[dev];
+ struct nand_chip *chip = nand-priv;
+
+ if (argc 3) {
+ puts(Current device ecclayout:\n);
+ nand_print_device_ecclayout(dev);
+ return 0;
+ }
+
+ if (!strcmp(argv[2], set)) {
+ if (argc 4)
+ return 1;
+
+ idx = (int)simple_strtoul(argv[3], NULL, 10);
+ if (!board_nand_ecclayout_set(chip, idx)) {
+ p = chip-ecc.layout;
+ p-oobavail = 0;
+ for (i = 0; p-oobfree[i].length
+ i ARRAY_SIZE(p-oobfree); i++)
+ p-oobavail += p-oobfree[i].length;
+
+ nand-oobavail = p-oobavail;
Shouldn't board_nand_ecclayout_set() take care of this? Possibly via
common helper functions, but not here.
+ } else {
+ printf(Setting current device
+ to ecc layout %d FAILED!\n, idx);
Don't wrap user-visible strings (checkpatch should have warned you about
this).
+ }
+
+ return 0;
+ }
+
+ if (strcmp(argv[2], all) != 0)
+ return 1;
+
+ /* show all available ecc layouts */
+
