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

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

drivers/mtd/nand/nand_base.c +4 -2

index a2d06be..614fc72 100644
@@ -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;

drivers/mtd/nand/nand_plat.c +2 -2

index 37a0206..b3bda11 100644
@@ -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;

include/configs/qong.h +1 -1

index d9bf201..077cbae 100644
@@ -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_BASE    CS3_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))

From: Scott Wood <[email protected]>
To: Chris Kiick <[email protected]>
Cc: u-boot at 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

Quoted 39 lines · Chris Kiick

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 @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(-)

drivers/mtd/nand/nand_base.c +7 -3

index a2d06be..614fc72 100644
@@ -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;
 

Quoted 1 line

case NAND_ECC_SOFT_BCH:

How is this part specific to nand plat?

I'm not sure how specifying your own ECC functions fits with the purpose of either NAND_ECC_SOFT or nand plat.

Quoted 33 lines

drivers/mtd/nand/nand_plat.c +1 -1

index  37a0206..b3bda11 100644
@@ -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

drivers/mtd/nand/nand_plat.c +1 -1

-     NAND_PLAT_INIT();
+    NAND_PLAT_INIT(nand);
  #endif
 
     nand->cmd_ctrl =  plat_cmd_ctrl;

include/configs/qong.h +0 -0

index d9bf201..077cbae 100644

@@ -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_BASE CS3_BASE
-#define NAND_PLAT_INIT() qong_nand_plat_init(nand)
+#define NAND_PLAT_INIT(nand) qong_nand_plat_init(nand)

Quoted 1 line

#define QONG_NAND_CLE(chip) ((unsigned long)(chip)->IO_ADDR_W | (1 <<

Quoted 1 line

#define QONG_NAND_ALE(chip) ((unsigned long)(chip)->IO_ADDR_W | (1 << 23))

This part looks unrelated.

-Scott

Hi,
Thanks for the reply. This is my first patch to u-boot. Any advice
is
appreciated. Comments in-line below.

----- Original Message ----

Quoted 5 lines

From: Scott Wood <[email protected]>
To: Chris Kiick <[email protected]>
Cc: u-boot at 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

Quoted 4 lines

driver

Quoted 1 line · Chris Kiick

Allow boards to set their own ECC layouts and functions in

NAND_PLAT_INIT

Quoted 10 lines

Review

Quoted 10 lines

without being stomped on by nand_base.c intialization.

Signed-off-by ckiick <chris @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(-)

(quoted diff) +0 -0

b/drivers/mtd/nand/nand_base.c

Quoted 28 lines

Quoted 28 lines

index a2d06be..614fc72 100644

drivers/mtd/nand/nand_base.c +7 -3

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

Quoted 3 lines

Quoted 1 line

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.

Quoted 1 line

I'm not sure how specifying your own ECC functions fits with the

purpose of

Quoted 1 line

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.

Quoted 1 line

drivers/mtd/nand/nand_plat.c

Quoted 1 line

drivers/mtd/nand/nand_plat.c renamed from +0 -0

b/drivers/mtd/nand/nand_plat.c

Quoted 32 lines

Quoted 32 lines

index 37a0206..b3bda11 100644

drivers/mtd/nand/nand_plat.c +1 -1

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

drivers/mtd/nand/nand_plat.c +1 -1

-     NAND_PLAT_INIT();
+    NAND_PLAT_INIT(nand);
  #endif
 
     nand->cmd_ctrl =  plat_cmd_ctrl;

include/configs/qong.h +0 -0

index d9bf201..077cbae 100644

@@ -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_BASE CS3_BASE
-#define NAND_PLAT_INIT() qong_nand_plat_init(nand)
+#define NAND_PLAT_INIT(nand) qong_nand_plat_init(nand)

Quoted 1 line

Quoted 1 line

#define QONG_NAND_CLE(chip) ((unsigned long)(chip)->IO_ADDR_W |

(1 <<
24))

Quoted 1 line

Quoted 1 line

#define QONG_NAND_ALE(chip) ((unsigned long)(chip)->IO_ADDR_W |

(1 << 23))

Quoted 2 lines

This part looks unrelated.

It follows as a logical consequence of the other change. If NAND_PLAT_INIT
function is going to make changes to the nand chip structure (which it would
have to do to setup the ECC), then it should be an explicit parameter.

And if
that macro is changed, then all the boards that use it have to follow. Which in
this case is just the qong board, which was assuming it could do that anyway.
I'm really not sure why NAND_PLAT_INIT() doesn't have that parameter already.

So the reason for all this is... I have this board. I'm sure you hear that a
lot. The VAR for the device decided (who knows why)

to use this extended ECC algorithm instead of the hardware supported one.

Quoted 4 lines

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.

From: Scott Wood <[email protected]>
To: Chris Kiick <[email protected]>
Cc: u-boot at 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

Quoted 5 lines · Chris Kiick

Hi,
Thanks for the reply. This is my first patch to u-boot. Any advice is
appreciated. Comments in-line below.

Quoted 7 lines

----- Original Message ----

Quoted 5 lines

From: Scott Wood <[email protected]>
To: Chris Kiick <[email protected]>
Cc: u-boot at 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

Quoted 4 lines

Quoted 4 lines

driver

Quoted 1 line · Chris Kiick

Allow boards to set their own ECC layouts and functions in

NAND_PLAT_INIT

Quoted 10 lines

Quoted 10 lines

drivers/mtd/nand/nand_base.c

Quoted 10 lines

without being stomped on by nand_base.c intialization.

Signed-off-by ckiick <chris @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(-)

drivers/mtd/nand/nand_base.c renamed from +0 -0

b/drivers/mtd/nand/nand_base.c

Quoted 28 lines

Quoted 28 lines

Quoted 28 lines

index a2d06be..614fc72 100644

drivers/mtd/nand/nand_base.c +4 -2

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

drivers/mtd/nand/nand_base.c +3 -1

+         if (!chip->ecc.size) {
              chip->ecc.size =  256;
-         chip->ecc.bytes =  3;
+              chip->ecc.bytes = 3;
+          }
         break;

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 1 line

this code path with no chance of making any changes after because of the

Quoted 1 line

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.

Quoted 2 lines

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

Quoted 1 line

around this.

They don't use NAND_ECC_SOFT.

Quoted 1 line

I reasoned that the change was safe for existing code because if something

was

Quoted 1 line

not setting its own ecc layout, it would still use the default, but it if

was,

Quoted 1 line

then it had to be after the call to nand_scan_tail() and that would

Quoted 1 line

whatever was set there anyway.

It's not a matter of safety, but rather a sign that you're misusing things.

Quoted 1 line

Quoted 1 line

I'm not sure how specifying your own ECC functions fits with the purpose

of

Quoted 2 lines

Quoted 1 line

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.

Quoted 1 line

You have to pick one of the existing ECC schemes, and SOFT is the scheme

that

Quoted 1 line

allows you to do your own ECC, if you setup the layout, calculate and

correct

Quoted 1 line

parts. Looking at the code, that's what I thought it was for.

You just described NAND_ECC_HW. :-)

Quoted 3 lines

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

Quoted 1 line

the low-level byte-bang part of the driver for me, so I don't have to

duplicate

Quoted 1 line

the code. Isn't that its purpose? Do I have to re-write a driver interface

that

Quoted 1 line

does the same thing as nand plat just because my ECC isn't the default?

There is very little code in the nand plat driver. I would not be too worried about duplicating that, if the result is more straightforward.

The fact that there's still only one board using it (but three ifdef symbols!) makes me wonder if nand plat was a bad idea in general.

Quoted 1 line

If I'm going in the wrong direction, I'd appreciate advice on how it's

supposed

Quoted 3 lines

to be done.

Quoted 1 line

Quoted 1 line

diff --git a/drivers/mtd/nand/nand_plat.c

b/drivers/mtd/nand/nand_plat.c

Quoted 33 lines

Quoted 33 lines

Quoted 33 lines

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

drivers/mtd/nand/nand_base.c +1 -0

+     NAND_PLAT_INIT(nand);
  #endif
 
     nand->cmd_ctrl =  plat_cmd_ctrl;

include/configs/qong.h +0 -0

index d9bf201..077cbae 100644

@@ -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_BASE CS3_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

Quoted 2 lines

24))

Quoted 1 line

Quoted 1 line

  #define QONG_NAND_ALE(chip) ((unsigned  long)(chip)->IO_ADDR_W | (1  << 

23))

Quoted 3 lines

Quoted 2 lines

This part looks unrelated.

It follows as a logical consequence of the other change. If

Quoted 1 line

function is going to make changes to the nand chip structure (which it

Quoted 1 line

have to do to setup the ECC), then it should be an explicit parameter.

But the one existing platform that uses nand plat already accesses the nand chip structure. Converting it from an implicit local variable reference to an explicit parameter is an improvement, but it's not a new concern for your board.

Quoted 2 lines

And if
that macro is changed, then all the boards that use it have to follow. Which

in

Quoted 1 line

this case is just the qong board, which was assuming it could do that

anyway.

Quoted 1 line

I'm really not sure why NAND_PLAT_INIT() doesn't have that parameter

already.

Exactly. I have no problem with that change, it just should be a separate patch.

Quoted 1 line

So the reason for all this is... I have this board. I'm sure you hear that

a

Quoted 1 line

lot. The VAR for the device decided (who knows why)

VAR?

Quoted 1 line

to use this extended ECC algorithm instead of the hardware supported one.

So you have hardware ECC, and you're ignoring it? Maybe this should be an optional compatibility mode rather than the way all new users will have their NAND operate?

Or is the hardware ECC insufficiently strong to deal with the NAND chip you're using (e.g. NAND chip requires 4-bit correction but controller implements 1-bit correction)? NAND_ECC_SOFT_BCH is an option if you need 4-bit correction (or more).

-Scott

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.