Re: [U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver
Hi,
Thanks for the reply. This is my first patch to u-boot. Any advice is
appreciated. Comments in-line below.
- Original Message
From: Scott Wood scottw...@freescale.com
To: Chris Kiick cki...@att.net
Cc: u-boot@lists.denx.de
Sent: Wed, December 19, 2012 1:02:52 PM
Subject: Re: [U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat
driver
On 12/18/2012 05:27:00 PM, Chris Kiick wrote:
Allow boards to set their own ECC layouts and functions in NAND_PLAT_INIT
without being stomped on by nand_base.c intialization.
Signed-off-by: ckiick chris at kiicks.net
---
drivers/mtd/nand/nand_base.c |11 +++
drivers/mtd/nand/nand_plat.c |4 ++--
include/configs/qong.h |2 +-
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index a2d06be..614fc72 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3035,8 +3035,10 @@ int nand_scan_tail(struct mtd_info *mtd)
chip-ecc.mode = NAND_ECC_SOFT;
case NAND_ECC_SOFT:
-chip-ecc.calculate = nand_calculate_ecc;
-chip-ecc.correct = nand_correct_data;
+if (!chip-ecc.calculate)
+ chip-ecc.calculate = nand_calculate_ecc;
+ if (!chip-ecc.correct)
+ chip-ecc.correct = nand_correct_data;
chip-ecc.read_page = nand_read_page_swecc;
chip-ecc.read_subpage = nand_read_subpage;
chip-ecc.write_page = nand_write_page_swecc;
@@ -3044,9 +3046,10 @@ int nand_scan_tail(struct mtd_info *mtd)
chip-ecc.write_page_raw = nand_write_page_raw;
chip-ecc.read_oob = nand_read_oob_std;
chip-ecc.write_oob = nand_write_oob_std;
-if (!chip-ecc.size)
+ if (!chip-ecc.size) {
chip-ecc.size = 256;
- chip-ecc.bytes = 3;
+ chip-ecc.bytes = 3;
+ }
break;
case NAND_ECC_SOFT_BCH:
How is this part specific to nand plat?
OK, it's not, but if you are using nand plat, then you are forced to go through
this code path with no chance of making any changes after because of the way
board_nand_init() is written. I seem to see other nand drivers setting up ecc
layouts and then calling nand_scan_tail(), I'm not sure how they are getting
around this.
I reasoned that the change was safe for existing code because if something was
not setting its own ecc layout, it would still use the default, but it if was,
then it had to be after the call to nand_scan_tail() and that would override
whatever was set there anyway.
I'm not sure how specifying your own ECC functions fits with the purpose of
either NAND_ECC_SOFT or nand plat.
Well, NAND_ECC_SOFT is the only scheme that does fit with custom ECC
algorithms.
You have to pick one of the existing ECC schemes, and SOFT is the scheme that
allows you to do your own ECC, if you setup the layout, calculate and correct
parts. Looking at the code, that's what I thought it was for.
Is there another way to implement custom ECC algorithms, done in software?
As for the plat driver, it shouldn't care what ECC I'm using. It's just
running
the low-level byte-bang part of the driver for me, so I don't have to duplicate
the code. Isn't that its purpose? Do I have to re-write a driver interface
that
does the same thing as nand plat just because my ECC isn't the default?
If I'm going in the wrong direction, I'd appreciate advice on how it's supposed
to be done.
diff --git a/drivers/mtd/nand/nand_plat.c b/drivers/mtd/nand/nand_plat.c
index 37a0206..b3bda11 100644
--- a/drivers/mtd/nand/nand_plat.c
+++ b/drivers/mtd/nand/nand_plat.c
@@ -8,7 +8,7 @@
/* Your board must implement the following macros:
* NAND_PLAT_WRITE_CMD(chip, cmd)
* NAND_PLAT_WRITE_ADR(chip, cmd)
- * NAND_PLAT_INIT()
+ * NAND_PLAT_INIT(nand)
*
* It may also implement the following:
* NAND_PLAT_DEV_READY(chip)
@@ -53,7 +53,7 @@ int board_nand_init(struct nand_chip *nand)
#endif
#ifdef NAND_PLAT_INIT
- NAND_PLAT_INIT();
+NAND_PLAT_INIT(nand);
#endif
nand-cmd_ctrl = plat_cmd_ctrl;
diff --git a/include/configs/qong.h b/include/configs/qong.h
index d9bf201..077cbae 100644
--- a/include/configs/qong.h
+++ b/include/configs/qong.h
@@ -226,7 +226,7 @@ extern int qong_nand_rdy(void *chip);
#define CONFIG_NAND_PLAT
#define CONFIG_SYS_MAX_NAND_DEVICE 1
#define CONFIG_SYS_NAND_BASECS3_BASE
-#define NAND_PLAT_INIT() qong_nand_plat_init(nand)
+#define NAND_PLAT_INIT(nand) qong_nand_plat_init(nand)
#define QONG_NAND_CLE(chip) ((unsigned long)(chip)-IO_ADDR_W | (1
24))
#define QONG_NAND_ALE(chip) ((unsigned long)(chip)-IO_ADDR_W | (1 23))
This part looks unrelated.
It follows as a logical consequence of the other change
Re: [U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver
Hi,
Well, you are of course 100% correct. I went back and took out the nand plat
stuff, made my own driver and used NAND_ECC_HW mode. A few tweaks and it works
just great. No changes needed to nand base code.
The mode names are a bit misleading, perhaps someone could document them in
the README?
What do I do to withdraw the patch? Or does it just get bounced out of the
queue.
Thanks for the help,
--
Chris J. Kiick ch...@kiicks.net
- Original Message
From: Scott Wood scottw...@freescale.com
To: Chris Kiick cki...@att.net
Cc: u-boot@lists.denx.de
Sent: Wed, December 19, 2012 3:40:49 PM
Subject: Re: [U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat
driver
On 12/19/2012 03:16:19 PM, Chris Kiick wrote:
Hi,
Thanks for the reply. This is my first patch to u-boot. Any advice is
appreciated. Comments in-line below.
- Original Message
From: Scott Wood scottw...@freescale.com
To: Chris Kiick cki...@att.net
Cc: u-boot@lists.denx.de
Sent: Wed, December 19, 2012 1:02:52 PM
Subject: Re: [U-Boot] [PATCH] NAND: allow custom SW ECC when using nand
plat
driver
On 12/18/2012 05:27:00 PM, Chris Kiick wrote:
Allow boards to set their own ECC layouts and functions in
NAND_PLAT_INIT
without being stomped on by nand_base.c intialization.
Signed-off-by: ckiick chris at kiicks.net
---
drivers/mtd/nand/nand_base.c |11 +++
drivers/mtd/nand/nand_plat.c |4 ++--
include/configs/qong.h |2 +-
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c
b/drivers/mtd/nand/nand_base.c
index a2d06be..614fc72 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3035,8 +3035,10 @@ int nand_scan_tail(struct mtd_info *mtd)
chip-ecc.mode = NAND_ECC_SOFT;
case NAND_ECC_SOFT:
-chip-ecc.calculate = nand_calculate_ecc;
- chip-ecc.correct = nand_correct_data;
+ if (!chip-ecc.calculate)
+ chip-ecc.calculate = nand_calculate_ecc;
+ if (!chip-ecc.correct)
+ chip-ecc.correct = nand_correct_data;
chip-ecc.read_page = nand_read_page_swecc;
chip-ecc.read_subpage = nand_read_subpage;
chip-ecc.write_page = nand_write_page_swecc;
@@ -3044,9 +3046,10 @@ int nand_scan_tail(struct mtd_info *mtd)
chip-ecc.write_page_raw = nand_write_page_raw;
chip-ecc.read_oob = nand_read_oob_std;
chip-ecc.write_oob = nand_write_oob_std;
-if (!chip-ecc.size)
+ if (!chip-ecc.size) {
chip-ecc.size = 256;
- chip-ecc.bytes = 3;
+ chip-ecc.bytes = 3;
+ }
break;
case NAND_ECC_SOFT_BCH:
How is this part specific to nand plat?
OK, it's not, but if you are using nand plat, then you are forced to go
through
this code path with no chance of making any changes after because of the
way
board_nand_init() is written.
nand plat is meant to be a simple driver for simple hardware that follows a
certain pattern. If you have hardware that doesn't fit that, don't use nand
plat.
I seem to see other nand drivers setting up ecc
layouts and then calling nand_scan_tail(), I'm not sure how they are
getting
around this.
They don't use NAND_ECC_SOFT.
I reasoned that the change was safe for existing code because if something
was
not setting its own ecc layout, it would still use the default, but it if
was,
then it had to be after the call to nand_scan_tail() and that would
override
whatever was set there anyway.
It's not a matter of safety, but rather a sign that you're misusing things.
I'm not sure how specifying your own ECC functions fits with the
purpose
of
either NAND_ECC_SOFT or nand plat.
Well, NAND_ECC_SOFT is the only scheme that does fit with custom ECC
algorithms.
No, it's the opposite. NAND_ECC_SOFT is telling the generic code please do
ECC for me. NAND_ECC_HW is telling the generic code I want to do ECC
myself, usually because you have hardware that implements it. In your case,
it's because you're trying to maintain compatibility with something.
You have to pick one of the existing ECC schemes, and SOFT is the scheme
that
allows you to do your own ECC, if you setup the layout, calculate and
correct
parts. Looking at the code, that's what I thought it was for.
You just described NAND_ECC_HW. :-)
Is there another way to implement custom ECC algorithms, done in software?
As for the plat driver, it shouldn't care what ECC I'm using. It's just
running
the low-level byte-bang part of the driver for me, so I don't have to
duplicate
the code. Isn't that its
[U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver
Allow boards to set their own ECC layouts and functions in NAND_PLAT_INIT
without being stomped on by nand_base.c intialization.
Signed-off-by: ckiick chris at kiicks.net
---
drivers/mtd/nand/nand_base.c | 11 +++
drivers/mtd/nand/nand_plat.c |4 ++--
include/configs/qong.h |2 +-
3 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index a2d06be..614fc72 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3035,8 +3035,10 @@ int nand_scan_tail(struct mtd_info *mtd)
chip-ecc.mode = NAND_ECC_SOFT;
case NAND_ECC_SOFT:
-chip-ecc.calculate = nand_calculate_ecc;
-chip-ecc.correct = nand_correct_data;
+if (!chip-ecc.calculate)
+chip-ecc.calculate = nand_calculate_ecc;
+if (!chip-ecc.correct)
+chip-ecc.correct = nand_correct_data;
chip-ecc.read_page = nand_read_page_swecc;
chip-ecc.read_subpage = nand_read_subpage;
chip-ecc.write_page = nand_write_page_swecc;
@@ -3044,9 +3046,10 @@ int nand_scan_tail(struct mtd_info *mtd)
chip-ecc.write_page_raw = nand_write_page_raw;
chip-ecc.read_oob = nand_read_oob_std;
chip-ecc.write_oob = nand_write_oob_std;
-if (!chip-ecc.size)
+if (!chip-ecc.size) {
chip-ecc.size = 256;
-chip-ecc.bytes = 3;
+chip-ecc.bytes = 3;
+}
break;
case NAND_ECC_SOFT_BCH:
diff --git a/drivers/mtd/nand/nand_plat.c b/drivers/mtd/nand/nand_plat.c
index 37a0206..b3bda11 100644
--- a/drivers/mtd/nand/nand_plat.c
+++ b/drivers/mtd/nand/nand_plat.c
@@ -8,7 +8,7 @@
/* Your board must implement the following macros:
* NAND_PLAT_WRITE_CMD(chip, cmd)
* NAND_PLAT_WRITE_ADR(chip, cmd)
- * NAND_PLAT_INIT()
+ * NAND_PLAT_INIT(nand)
*
* It may also implement the following:
* NAND_PLAT_DEV_READY(chip)
@@ -53,7 +53,7 @@ int board_nand_init(struct nand_chip *nand)
#endif
#ifdef NAND_PLAT_INIT
-NAND_PLAT_INIT();
+NAND_PLAT_INIT(nand);
#endif
nand-cmd_ctrl = plat_cmd_ctrl;
diff --git a/include/configs/qong.h b/include/configs/qong.h
index d9bf201..077cbae 100644
--- a/include/configs/qong.h
+++ b/include/configs/qong.h
@@ -226,7 +226,7 @@ extern int qong_nand_rdy(void *chip);
#define CONFIG_NAND_PLAT
#define CONFIG_SYS_MAX_NAND_DEVICE 1
#define CONFIG_SYS_NAND_BASECS3_BASE
-#define NAND_PLAT_INIT() qong_nand_plat_init(nand)
+#define NAND_PLAT_INIT(nand) qong_nand_plat_init(nand)
#define QONG_NAND_CLE(chip) ((unsigned long)(chip)-IO_ADDR_W | (1 24))
#define QONG_NAND_ALE(chip) ((unsigned long)(chip)-IO_ADDR_W | (1 23))
--
1.7.9.5
--
Chris J. Kiick ch...@kiicks.net
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
