* [U-Boot] omap_gpmc: Do not default to HAM1 when SW ECC is selected @ 2015-02-18 2:58 Adam YH Lee 2015-02-18 14:14 ` Andreas Bießmann 0 siblings, 1 reply; 6+ messages in thread From: Adam YH Lee @ 2015-02-18 2:58 UTC (permalink / raw) To: u-boot The ECC scheme selection algorithm in OMAP GPMC appears to be left untested when BCH8 handling code was added. Running 'nandecc sw' defaults to HAM1 even if the board is using another scheme (ex. OMAP_ECC_BCH8_CODE_HW_DETECTION_SW on OMAP3). This results in unrecoverable ECC errors when reading data. This commit fixes the behavior by checking for CONFIG_BCH and using the scheme defined by CONFIG_NAND_OMAP_ECCSCHEME in the board configuration file. This has been tested on Gumstix Overo (OMAP3). Signed-off-by: Adam YH Lee <adam.yh.lee@gmail.com> --- drivers/mtd/nand/omap_gpmc.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c index fc64f48..5daf932 100644 --- a/drivers/mtd/nand/omap_gpmc.c +++ b/drivers/mtd/nand/omap_gpmc.c @@ -901,8 +901,13 @@ int __maybe_unused omap_nand_switch_ecc(uint32_t hardware, uint32_t eccstrength) return -EINVAL; } } else { + #ifdef CONFIG_BCH + err = omap_select_ecc_scheme(nand, CONFIG_NAND_OMAP_ECCSCHEME, + mtd->writesize, mtd->oobsize); + #else err = omap_select_ecc_scheme(nand, OMAP_ECC_HAM1_CODE_SW, mtd->writesize, mtd->oobsize); + #endif } /* Update NAND handling after ECC mode switch */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] omap_gpmc: Do not default to HAM1 when SW ECC is selected 2015-02-18 2:58 [U-Boot] omap_gpmc: Do not default to HAM1 when SW ECC is selected Adam YH Lee @ 2015-02-18 14:14 ` Andreas Bießmann 2015-02-18 17:33 ` Adam Lee 2015-02-18 19:25 ` [U-Boot] [PATCH] omap: gpmc: 'nandecc sw' can use HAM1 or BCH8 Ash Charles 0 siblings, 2 replies; 6+ messages in thread From: Andreas Bießmann @ 2015-02-18 14:14 UTC (permalink / raw) To: u-boot Hi Adam, On 02/18/2015 03:58 AM, Adam YH Lee wrote: > The ECC scheme selection algorithm in OMAP GPMC appears to be left untested when > BCH8 handling code was added. Running 'nandecc sw' defaults to HAM1 even if > the board is using another scheme (ex. OMAP_ECC_BCH8_CODE_HW_DETECTION_SW on > OMAP3). This results in unrecoverable ECC errors when reading data. This commit > fixes the behavior by checking for CONFIG_BCH and using the scheme defined by > CONFIG_NAND_OMAP_ECCSCHEME in the board configuration file. > > This has been tested on Gumstix Overo (OMAP3). > > Signed-off-by: Adam YH Lee <adam.yh.lee@gmail.com> > --- > drivers/mtd/nand/omap_gpmc.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c > index fc64f48..5daf932 100644 > --- a/drivers/mtd/nand/omap_gpmc.c > +++ b/drivers/mtd/nand/omap_gpmc.c > @@ -901,8 +901,13 @@ int __maybe_unused omap_nand_switch_ecc(uint32_t hardware, uint32_t eccstrength) > return -EINVAL; > } > } else { > + #ifdef CONFIG_BCH > + err = omap_select_ecc_scheme(nand, CONFIG_NAND_OMAP_ECCSCHEME, > + mtd->writesize, mtd->oobsize); > + #else > err = omap_select_ecc_scheme(nand, OMAP_ECC_HAM1_CODE_SW, > mtd->writesize, mtd->oobsize); Couldn't we just use the CONFIG_NAND_OMAP_ECCSCHEME instead of OMAP_ECC_HAM1_CODE_SW here and omit the CONFIG_BCH define? On the other hand I think the whole function omap_nand_switch_ecc() is wrong. AFAIR it should only be used on omap3 aka am35xx/dm37xx devices witch the nandecc command. These SoC do not have a ELM. Therefore I decided to say BCH8 on those devices is always 'HW ECC' when introduced BCH8 on omap3 first. Nowadays it is the so called BCH8_CODE_HW_DETECTION_SW. Coming from this mindset the right solution is to use some detection if the ELM is supported or not and switch in the HW part of omap_nand_switch_ecc() between OMAP_ECC_BCH8_CODE_HW and OMAP_ECC_BCH8_CODE_HW_DETECTION_SW. Best regards Andreas Bie?mann ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] omap_gpmc: Do not default to HAM1 when SW ECC is selected 2015-02-18 14:14 ` Andreas Bießmann @ 2015-02-18 17:33 ` Adam Lee 2015-02-18 19:04 ` Adam Lee 2015-02-18 19:25 ` [U-Boot] [PATCH] omap: gpmc: 'nandecc sw' can use HAM1 or BCH8 Ash Charles 1 sibling, 1 reply; 6+ messages in thread From: Adam Lee @ 2015-02-18 17:33 UTC (permalink / raw) To: u-boot Hi Andreas, for OMAP3 and AM35xx boards, it would have been ok omitting the CONFIG_BCH check and simply use CONFIG_NAND_OMAP_ECCSCHEME. Those boards use the ecc scheme config already. However I just wasn't 100% sure if I could rely on this config for all TI OMAP/AM based boards. I know OMAP3 and AM35xx board configs already have CONFIG_NAND_OMAP_ECCSCHEME. Should I check for other OMAP and AM series? The original ecc detection function seems to be written with an assumption that config is nonexistent - hence defaulting to HAM1. That said, I agree that the whole omap_nand_switch_ecc() could be improved. However, to me, the confusion arised from the fact that OMAP3 can do BCH8 hw ECC calculation but needs software to do the correction. Hence my patch changes the software part of the omap_nand_switch_ecc(), and not the hardware part. Just to clarify, what you are saying is that I should leave the software part as it was (defaulting to HAM1), and in the hardware part I should check for ELM support and choose a BCH8 scheme accordingly, regardless of what's defined by CONFIG_NAND_OMAP_ECCSCHEME? In other words, I will run 'nandecc hw' to enable BCH8? Let me know, Thanks, Adam On Wed, Feb 18, 2015 at 6:14 AM, Andreas Bie?mann < andreas.devel@googlemail.com> wrote: > Hi Adam, > > On 02/18/2015 03:58 AM, Adam YH Lee wrote: > > The ECC scheme selection algorithm in OMAP GPMC appears to be left > untested when > > BCH8 handling code was added. Running 'nandecc sw' defaults to HAM1 even > if > > the board is using another scheme (ex. > OMAP_ECC_BCH8_CODE_HW_DETECTION_SW on > > OMAP3). This results in unrecoverable ECC errors when reading data. This > commit > > fixes the behavior by checking for CONFIG_BCH and using the scheme > defined by > > CONFIG_NAND_OMAP_ECCSCHEME in the board configuration file. > > > > This has been tested on Gumstix Overo (OMAP3). > > > > Signed-off-by: Adam YH Lee <adam.yh.lee@gmail.com> > > --- > > drivers/mtd/nand/omap_gpmc.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c > > index fc64f48..5daf932 100644 > > --- a/drivers/mtd/nand/omap_gpmc.c > > +++ b/drivers/mtd/nand/omap_gpmc.c > > @@ -901,8 +901,13 @@ int __maybe_unused omap_nand_switch_ecc(uint32_t > hardware, uint32_t eccstrength) > > return -EINVAL; > > } > > } else { > > + #ifdef CONFIG_BCH > > + err = omap_select_ecc_scheme(nand, > CONFIG_NAND_OMAP_ECCSCHEME, > > + mtd->writesize, mtd->oobsize); > > + #else > > err = omap_select_ecc_scheme(nand, OMAP_ECC_HAM1_CODE_SW, > > mtd->writesize, mtd->oobsize); > > Couldn't we just use the CONFIG_NAND_OMAP_ECCSCHEME instead of > OMAP_ECC_HAM1_CODE_SW here and omit the CONFIG_BCH define? > > On the other hand I think the whole function omap_nand_switch_ecc() is > wrong. AFAIR it should only be used on omap3 aka am35xx/dm37xx devices > witch the nandecc command. > These SoC do not have a ELM. Therefore I decided to say BCH8 on those > devices is always 'HW ECC' when introduced BCH8 on omap3 first. Nowadays > it is the so called BCH8_CODE_HW_DETECTION_SW. Coming from this mindset > the right solution is to use some detection if the ELM is supported or > not and switch in the HW part of omap_nand_switch_ecc() between > OMAP_ECC_BCH8_CODE_HW and OMAP_ECC_BCH8_CODE_HW_DETECTION_SW. > > Best regards > > Andreas Bie?mann > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] omap_gpmc: Do not default to HAM1 when SW ECC is selected 2015-02-18 17:33 ` Adam Lee @ 2015-02-18 19:04 ` Adam Lee 0 siblings, 0 replies; 6+ messages in thread From: Adam Lee @ 2015-02-18 19:04 UTC (permalink / raw) To: u-boot Had a conversation with Ash @ Gumstix and he pointed out relying on CONFIG_NAND_OMAP_ECCSCHEME could be dangerous as it could be anything other than the two SW ECC schemes available for OMAP3. Also it looks like making a selection between OMAP_ECC_BCH8_CODE_HW and OMAP_ECC_BCH8_CODE_HW_DETECTION_SW may not make sense as the latter clearly requires CONFIG_BCH, which enables "software" library for BCH. On Wed, Feb 18, 2015 at 9:33 AM, Adam Lee <adam.yh.lee@gmail.com> wrote: > Hi Andreas, for OMAP3 and AM35xx boards, it would have been ok omitting > the > CONFIG_BCH check and simply use CONFIG_NAND_OMAP_ECCSCHEME. > Those boards use the ecc scheme config already. However I just wasn't 100% > sure if I could rely on this config for all TI OMAP/AM based boards. I > know OMAP3 > and AM35xx board configs already have CONFIG_NAND_OMAP_ECCSCHEME. > Should I check for other OMAP and AM series? The original ecc detection > function > seems to be written with an assumption that config is nonexistent - hence > defaulting > to HAM1. > > That said, I agree that the whole omap_nand_switch_ecc() could be improved. > However, to me, the confusion arised from the fact that OMAP3 can do BCH8 > hw ECC > calculation but needs software to do the correction. Hence my patch > changes the > software part of the omap_nand_switch_ecc(), and not the hardware part. > > Just to clarify, what you are saying is that I should leave the software > part as it was (defaulting > to HAM1), and in the hardware part I should check for ELM support and > choose a BCH8 > scheme accordingly, regardless of what's defined by > CONFIG_NAND_OMAP_ECCSCHEME? > > In other words, I will run 'nandecc hw' to enable BCH8? > > Let me know, > > Thanks, > > Adam > > > > On Wed, Feb 18, 2015 at 6:14 AM, Andreas Bie?mann < > andreas.devel at googlemail.com> wrote: > >> Hi Adam, >> >> On 02/18/2015 03:58 AM, Adam YH Lee wrote: >> > The ECC scheme selection algorithm in OMAP GPMC appears to be left >> untested when >> > BCH8 handling code was added. Running 'nandecc sw' defaults to HAM1 >> even if >> > the board is using another scheme (ex. >> OMAP_ECC_BCH8_CODE_HW_DETECTION_SW on >> > OMAP3). This results in unrecoverable ECC errors when reading data. >> This commit >> > fixes the behavior by checking for CONFIG_BCH and using the scheme >> defined by >> > CONFIG_NAND_OMAP_ECCSCHEME in the board configuration file. >> > >> > This has been tested on Gumstix Overo (OMAP3). >> > >> > Signed-off-by: Adam YH Lee <adam.yh.lee@gmail.com> >> > --- >> > drivers/mtd/nand/omap_gpmc.c | 5 +++++ >> > 1 file changed, 5 insertions(+) >> > >> > diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c >> > index fc64f48..5daf932 100644 >> > --- a/drivers/mtd/nand/omap_gpmc.c >> > +++ b/drivers/mtd/nand/omap_gpmc.c >> > @@ -901,8 +901,13 @@ int __maybe_unused omap_nand_switch_ecc(uint32_t >> hardware, uint32_t eccstrength) >> > return -EINVAL; >> > } >> > } else { >> > + #ifdef CONFIG_BCH >> > + err = omap_select_ecc_scheme(nand, >> CONFIG_NAND_OMAP_ECCSCHEME, >> > + mtd->writesize, mtd->oobsize); >> > + #else >> > err = omap_select_ecc_scheme(nand, OMAP_ECC_HAM1_CODE_SW, >> > mtd->writesize, mtd->oobsize); >> >> Couldn't we just use the CONFIG_NAND_OMAP_ECCSCHEME instead of >> OMAP_ECC_HAM1_CODE_SW here and omit the CONFIG_BCH define? >> >> On the other hand I think the whole function omap_nand_switch_ecc() is >> wrong. AFAIR it should only be used on omap3 aka am35xx/dm37xx devices >> witch the nandecc command. >> These SoC do not have a ELM. Therefore I decided to say BCH8 on those >> devices is always 'HW ECC' when introduced BCH8 on omap3 first. Nowadays >> it is the so called BCH8_CODE_HW_DETECTION_SW. Coming from this mindset >> the right solution is to use some detection if the ELM is supported or >> not and switch in the HW part of omap_nand_switch_ecc() between >> OMAP_ECC_BCH8_CODE_HW and OMAP_ECC_BCH8_CODE_HW_DETECTION_SW. >> >> Best regards >> >> Andreas Bie?mann >> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] omap: gpmc: 'nandecc sw' can use HAM1 or BCH8 2015-02-18 14:14 ` Andreas Bießmann 2015-02-18 17:33 ` Adam Lee @ 2015-02-18 19:25 ` Ash Charles 2015-03-06 15:47 ` [U-Boot] " Tom Rini 1 sibling, 1 reply; 6+ messages in thread From: Ash Charles @ 2015-02-18 19:25 UTC (permalink / raw) To: u-boot The 'nandecc sw' command selects a software-based error correction algorithm. By default, this is OMAP_ECC_HAM1_CODE_SW but some platforms use OMAP_ECC_BCH8_CODE_HW_DETECTION_SW as their software-based correction algorithm. Allow a user to be specific e.g. # nandecc sw <hamming|bch8> where 'hamming' is still the default. Note: we don't just use CONFIG_NAND_OMAP_ECCSCHEME as it might be set to a hardware-based ECC scheme---a little strange when the user has requested 'sw' ECC. Signed-off-by: Ash Charles <ashcharles@gmail.com> --- arch/arm/cpu/armv7/omap3/board.c | 11 ++++++++++- drivers/mtd/nand/omap_gpmc.c | 12 +++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/arch/arm/cpu/armv7/omap3/board.c b/arch/arm/cpu/armv7/omap3/board.c index 667e77f..b4e29ab 100644 --- a/arch/arm/cpu/armv7/omap3/board.c +++ b/arch/arm/cpu/armv7/omap3/board.c @@ -327,7 +327,16 @@ static int do_switch_ecc(cmd_tbl_t * cmdtp, int flag, int argc, char * const arg goto usage; } } else if (strncmp(argv[1], "sw", 2) == 0) { - omap_nand_switch_ecc(0, 0); + if (argc == 2) { + omap_nand_switch_ecc(0, 1); + } else { + if (strncmp(argv[2], "hamming", 7) == 0) + omap_nand_switch_ecc(0, 1); + else if (strncmp(argv[2], "bch8", 4) == 0) + omap_nand_switch_ecc(0, 8); + else + goto usage; + } } else { goto usage; } diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c index db1599e..d6a28e6 100644 --- a/drivers/mtd/nand/omap_gpmc.c +++ b/drivers/mtd/nand/omap_gpmc.c @@ -794,8 +794,18 @@ int __maybe_unused omap_nand_switch_ecc(uint32_t hardware, uint32_t eccstrength) return -EINVAL; } } else { - err = omap_select_ecc_scheme(nand, OMAP_ECC_HAM1_CODE_SW, + if (eccstrength == 1) { + err = omap_select_ecc_scheme(nand, + OMAP_ECC_HAM1_CODE_SW, + mtd->writesize, mtd->oobsize); + } else if (eccstrength == 8) { + err = omap_select_ecc_scheme(nand, + OMAP_ECC_BCH8_CODE_HW_DETECTION_SW, mtd->writesize, mtd->oobsize); + } else { + printf("nand: error: unsupported ECC scheme\n"); + return -EINVAL; + } } /* Update NAND handling after ECC mode switch */ -- 2.1.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] omap: gpmc: 'nandecc sw' can use HAM1 or BCH8 2015-02-18 19:25 ` [U-Boot] [PATCH] omap: gpmc: 'nandecc sw' can use HAM1 or BCH8 Ash Charles @ 2015-03-06 15:47 ` Tom Rini 0 siblings, 0 replies; 6+ messages in thread From: Tom Rini @ 2015-03-06 15:47 UTC (permalink / raw) To: u-boot On Wed, Feb 18, 2015 at 11:25:11AM -0800, Ash Charles wrote: > The 'nandecc sw' command selects a software-based error correction > algorithm. By default, this is OMAP_ECC_HAM1_CODE_SW but some > platforms use OMAP_ECC_BCH8_CODE_HW_DETECTION_SW as their > software-based correction algorithm. Allow a user to be specific e.g. > # nandecc sw <hamming|bch8> > where 'hamming' is still the default. > > Note: we don't just use CONFIG_NAND_OMAP_ECCSCHEME as it might be set > to a hardware-based ECC scheme---a little strange when the user > has requested 'sw' ECC. > > Signed-off-by: Ash Charles <ashcharles@gmail.com> Applied to u-boot/master, thanks! -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150306/b5e2f10d/attachment.sig> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-03-06 15:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-02-18 2:58 [U-Boot] omap_gpmc: Do not default to HAM1 when SW ECC is selected Adam YH Lee 2015-02-18 14:14 ` Andreas Bießmann 2015-02-18 17:33 ` Adam Lee 2015-02-18 19:04 ` Adam Lee 2015-02-18 19:25 ` [U-Boot] [PATCH] omap: gpmc: 'nandecc sw' can use HAM1 or BCH8 Ash Charles 2015-03-06 15:47 ` [U-Boot] " Tom Rini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox