[U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver
Chris Kiick
ckiick at att.net
Thu Dec 20 16:05:49 CET 2012
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 at freescale.com>
> To: Chris Kiick <ckiick at 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 at freescale.com>
> > > To: Chris Kiick <ckiick at 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 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 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
>
More information about the U-Boot
mailing list