[U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver

[U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver

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

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_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))
-- 
1.7.9.5



 --
Chris J. Kiick chris@kiicks.net

[U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver

[Scott Wood]

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

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

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

This part looks unrelated.

-Scott

[U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver

[Chris Kiick]

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 <scottwood@freescale.com>
> To: Chris Kiick <ckiick@att.net>
> 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
> 
> 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 @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_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))
> 
> 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. They originally stuffed it into 
u-boot 1.1.2 with a crude hack that pretty much ignored the interface and 
changed the base code, so it would never be compatible with anything else.  Now 
I'm trying to move up to a better version of u-boot, and I'm stuck with the 
legacy ECC. Eventually it would be nice to get the board code added to u-boot, 
so I'm trying to pave the way with some more generic changes that it depends on. 
And avoid adding anything to the base code that would be dead code on any other 
board. This is the minimal change to u-boot that would still allow me to support 
this board.

Anyway, let me know what you think. I'm open to any suggestions.

Thanks,

> -Scott
>
--
Chris J. Kiick chris at kiicks.net

[U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver

[Scott Wood]

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 <scottwood@freescale.com>
> > To: Chris Kiick <ckiick@att.net>
> > 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
> >
> > 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 @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 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?

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.

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

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.

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

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

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

VAR?

> 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

[U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver

[Scott Wood]

On 12/19/2012 03:40:03 PM, Scott Wood wrote:
>> 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.

Wait, just layout, not algorithm?  How is that a problem with either  
NAND_ECC_SOFT or nand plat?

-Scott

[U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver

[Chris Kiick]

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 chris at kiicks.net



----- Original Message ----
> From: Scott Wood <scottwood@freescale.com>
> To: Chris Kiick <ckiick@att.net>
> 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
> 
> 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 <scottwood@freescale.com>
> >  > To: Chris Kiick <ckiick@att.net>
> > > 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
> >  >
> > > 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 @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 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?
> 
> 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.
> 
> > 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_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))
> > >
> > > 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.
> 
> 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.
> 
> > 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.
> 
> Exactly.  I have no problem with that change, it  just should be a separate 
>patch.
> 
> > 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)
> 
> VAR?
> 
> > 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
> 

[U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver

[Scott Wood]

On 12/20/2012 09:05:49 AM, Chris Kiick wrote:
> 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?

Patches welcome. :-)

>   What do I do to withdraw the patch? Or does it just get bounced out  
> of the
> queue.

An e-mail reply is fine (which you just did).

-Scott