[U-Boot] [PATCH] NAND: allow custom SW ECC when using nand plat driver
Scott Wood
scottwood at freescale.com
Wed Dec 19 22:40:03 CET 2012
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