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 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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot