| Message ID | 1317879924-18266-1-git-send-email-hs@denx.de |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, Oct 5, 2011 at 10:45 PM, Heiko Schocher <hs@denx.de> wrote: > similiar to commit dc7cd8e59ba077f3b4c1a4557c9cd86a31b9ab1f, only > adapted for the new spl framework. > > Signed-off-by: Heiko Schocher <hs@denx.de> > Cc: Scott Wood <scottwood@freescale.com> > Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> > Cc: Sandeep Paulraj <s-paulraj@ti.com> > > --- > changes for v3: > - add comment from Scott Wood: > as BSS is cleared, no need for intializing vars with 0 > remove this. Scott and I were talking about this on IRC (since I'm adding in a BCH8 SPL platform) and isn't the only difference just where the OOB is? So the config switch should be changed to reflect that since this code would work with any ECC size and OOB first, yes?
On 10/06/2011 12:45 AM, Heiko Schocher wrote: > similiar to commit dc7cd8e59ba077f3b4c1a4557c9cd86a31b9ab1f, only > adapted for the new spl framework. > > Signed-off-by: Heiko Schocher <hs@denx.de> > Cc: Scott Wood <scottwood@freescale.com> > Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> > Cc: Sandeep Paulraj <s-paulraj@ti.com> > > --- > changes for v3: > - add comment from Scott Wood: > as BSS is cleared, no need for intializing vars with 0 > remove this. > > drivers/mtd/nand/nand_spl_simple.c | 43 +++++++++++++++++++++++++++++++++++- > 1 files changed, 42 insertions(+), 1 deletions(-) > > diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c > index 71491d4..622a9b5 100644 > --- a/drivers/mtd/nand/nand_spl_simple.c > +++ b/drivers/mtd/nand/nand_spl_simple.c > @@ -140,6 +140,47 @@ static int nand_is_bad_block(int block) > return 0; > } > > +#if defined(CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST) Change the name to not be 4BIT-specific, and add documentation for this config option. -Scott
Hello Scott, Scott Wood wrote: > On 10/06/2011 12:45 AM, Heiko Schocher wrote: >> similiar to commit dc7cd8e59ba077f3b4c1a4557c9cd86a31b9ab1f, only >> adapted for the new spl framework. >> >> Signed-off-by: Heiko Schocher <hs@denx.de> >> Cc: Scott Wood <scottwood@freescale.com> >> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> >> Cc: Sandeep Paulraj <s-paulraj@ti.com> >> >> --- >> changes for v3: >> - add comment from Scott Wood: >> as BSS is cleared, no need for intializing vars with 0 >> remove this. >> >> drivers/mtd/nand/nand_spl_simple.c | 43 +++++++++++++++++++++++++++++++++++- >> 1 files changed, 42 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c >> index 71491d4..622a9b5 100644 >> --- a/drivers/mtd/nand/nand_spl_simple.c >> +++ b/drivers/mtd/nand/nand_spl_simple.c >> @@ -140,6 +140,47 @@ static int nand_is_bad_block(int block) >> return 0; >> } >> >> +#if defined(CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST) > > Change the name to not be 4BIT-specific, and add documentation for this > config option. Hmm.. this is no new config option, it enables on davinci socs the 4Bit NAND HW ECC generation ... and I need this in spl code too ... Ok, you are right, there is no documentation of this config option. Should I add this documentation in this patch or in a seperate patch? bye, Heiko
Dear Heiko Schocher, In message <4E93D6FF.9020408@denx.de> you wrote: > > Hmm.. this is no new config option, it enables on davinci socs the > 4Bit NAND HW ECC generation ... and I need this in spl code too ... > Ok, you are right, there is no documentation of this config option. > Should I add this documentation in this patch or in a seperate patch? Separate, please. Thanks. Best regards, Wolfgang Denk
On 10/11/2011 12:41 AM, Heiko Schocher wrote: > Hello Scott, > > Scott Wood wrote: >> On 10/06/2011 12:45 AM, Heiko Schocher wrote: >>> similiar to commit dc7cd8e59ba077f3b4c1a4557c9cd86a31b9ab1f, only >>> adapted for the new spl framework. >>> >>> Signed-off-by: Heiko Schocher <hs@denx.de> >>> Cc: Scott Wood <scottwood@freescale.com> >>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> >>> Cc: Sandeep Paulraj <s-paulraj@ti.com> >>> >>> --- >>> changes for v3: >>> - add comment from Scott Wood: >>> as BSS is cleared, no need for intializing vars with 0 >>> remove this. >>> >>> drivers/mtd/nand/nand_spl_simple.c | 43 +++++++++++++++++++++++++++++++++++- >>> 1 files changed, 42 insertions(+), 1 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c >>> index 71491d4..622a9b5 100644 >>> --- a/drivers/mtd/nand/nand_spl_simple.c >>> +++ b/drivers/mtd/nand/nand_spl_simple.c >>> @@ -140,6 +140,47 @@ static int nand_is_bad_block(int block) >>> return 0; >>> } >>> >>> +#if defined(CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST) >> >> Change the name to not be 4BIT-specific, and add documentation for this >> config option. > > Hmm.. this is no new config option, it enables on davinci socs the > 4Bit NAND HW ECC generation ... and I need this in spl code too ... It's not new, but it is misnamed (there's nothing 4-bit specific in the #ifdeffed code), and we now have someone that wants this for 8-bit ECC. -Scott
Hello Scott, Scott Wood wrote: > On 10/11/2011 12:41 AM, Heiko Schocher wrote: >> Hello Scott, >> >> Scott Wood wrote: >>> On 10/06/2011 12:45 AM, Heiko Schocher wrote: >>>> similiar to commit dc7cd8e59ba077f3b4c1a4557c9cd86a31b9ab1f, only >>>> adapted for the new spl framework. >>>> >>>> Signed-off-by: Heiko Schocher <hs@denx.de> >>>> Cc: Scott Wood <scottwood@freescale.com> >>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> >>>> Cc: Sandeep Paulraj <s-paulraj@ti.com> >>>> >>>> --- >>>> changes for v3: >>>> - add comment from Scott Wood: >>>> as BSS is cleared, no need for intializing vars with 0 >>>> remove this. >>>> >>>> drivers/mtd/nand/nand_spl_simple.c | 43 +++++++++++++++++++++++++++++++++++- >>>> 1 files changed, 42 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c >>>> index 71491d4..622a9b5 100644 >>>> --- a/drivers/mtd/nand/nand_spl_simple.c >>>> +++ b/drivers/mtd/nand/nand_spl_simple.c >>>> @@ -140,6 +140,47 @@ static int nand_is_bad_block(int block) >>>> return 0; >>>> } >>>> >>>> +#if defined(CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST) >>> Change the name to not be 4BIT-specific, and add documentation for this >>> config option. >> Hmm.. this is no new config option, it enables on davinci socs the >> 4Bit NAND HW ECC generation ... and I need this in spl code too ... > > It's not new, but it is misnamed (there's nothing 4-bit specific in the > #ifdeffed code), and we now have someone that wants this for 8-bit ECC. look at my v4 series of this patchset: http://patchwork.ozlabs.org/patch/121295/ I renamed it to: CONFIG_SYS_NAND_HW_ECC_OOBFIRST In drivers/mtd/nand/davinci_nand.c CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST is used, and there are 4bit specific functions, so this define is also valid, just we need in drivers/mtd/nand/nand_spl_simple.c a more common name, right? bye, Heiko
On 10/26/2011 01:16 AM, Heiko Schocher wrote: > Hello Scott, > > Scott Wood wrote: >> It's not new, but it is misnamed (there's nothing 4-bit specific in the >> #ifdeffed code), and we now have someone that wants this for 8-bit ECC. > > look at my v4 series of this patchset: > > http://patchwork.ozlabs.org/patch/121295/ > > I renamed it to: CONFIG_SYS_NAND_HW_ECC_OOBFIRST OK. I just got back from vacation, so I'm still going through an e-mail backlog. > In drivers/mtd/nand/davinci_nand.c CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST > is used, and there are 4bit specific functions, so this define is > also valid, just we need in drivers/mtd/nand/nand_spl_simple.c a more > common name, right? Right. Ideally, though, that would either be CONFIG_SYS_DAVINCI_... or #if defined(CONFIG_SYS_NAND_HW_ECC_OOBFIRST) && \ defined(CONFIG_SYS_NAND_HW_ECC_4BIT) -Scott
Hello Scott, Scott Wood wrote: > On 10/26/2011 01:16 AM, Heiko Schocher wrote: >> Hello Scott, >> >> Scott Wood wrote: >>> It's not new, but it is misnamed (there's nothing 4-bit specific in the >>> #ifdeffed code), and we now have someone that wants this for 8-bit ECC. >> look at my v4 series of this patchset: >> >> http://patchwork.ozlabs.org/patch/121295/ >> >> I renamed it to: CONFIG_SYS_NAND_HW_ECC_OOBFIRST > > OK. I just got back from vacation, so I'm still going through an e-mail > backlog. No problem, hope you had a nice vacation? >> In drivers/mtd/nand/davinci_nand.c CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST >> is used, and there are 4bit specific functions, so this define is >> also valid, just we need in drivers/mtd/nand/nand_spl_simple.c a more >> common name, right? > > Right. Ideally, though, that would either be CONFIG_SYS_DAVINCI_... or > #if defined(CONFIG_SYS_NAND_HW_ECC_OOBFIRST) && \ > defined(CONFIG_SYS_NAND_HW_ECC_4BIT) Hmm.. I thought you meant, this is not davinci nor 4bit specific? Or do you mean to rename the CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST define in drivers/mtd/nand/davinci_nand.c? Sandeep, what do you think? bye, Heiko
On 10/27/2011 12:23 AM, Heiko Schocher wrote: >>> In drivers/mtd/nand/davinci_nand.c CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST >>> is used, and there are 4bit specific functions, so this define is >>> also valid, just we need in drivers/mtd/nand/nand_spl_simple.c a more >>> common name, right? >> >> Right. Ideally, though, that would either be CONFIG_SYS_DAVINCI_... or >> #if defined(CONFIG_SYS_NAND_HW_ECC_OOBFIRST) && \ >> defined(CONFIG_SYS_NAND_HW_ECC_4BIT) > > Hmm.. I thought you meant, this is not davinci nor 4bit specific? > Or do you mean to rename the CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST > define in drivers/mtd/nand/davinci_nand.c? I just mean that "4bit" and "oobfirst" are independent things, so I'd rather not have something that combines the two as part of the configuration of the general NAND interface in the absence of a requirement to enumerate all possibilities. OTOH, the structure of driver's private config can be whatever makes the most sense for that driver, but should be named in a way that it's obviously driver-specific. -Scott
Hello Scott, Scott Wood wrote: > On 10/27/2011 12:23 AM, Heiko Schocher wrote: >>>> In drivers/mtd/nand/davinci_nand.c CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST >>>> is used, and there are 4bit specific functions, so this define is >>>> also valid, just we need in drivers/mtd/nand/nand_spl_simple.c a more >>>> common name, right? >>> Right. Ideally, though, that would either be CONFIG_SYS_DAVINCI_... or >>> #if defined(CONFIG_SYS_NAND_HW_ECC_OOBFIRST) && \ >>> defined(CONFIG_SYS_NAND_HW_ECC_4BIT) >> Hmm.. I thought you meant, this is not davinci nor 4bit specific? >> Or do you mean to rename the CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST >> define in drivers/mtd/nand/davinci_nand.c? > > I just mean that "4bit" and "oobfirst" are independent things, so I'd > rather not have something that combines the two as part of the > configuration of the general NAND interface in the absence of a > requirement to enumerate all possibilities. OTOH, the structure of > driver's private config can be whatever makes the most sense for that > driver, but should be named in a way that it's obviously driver-specific. So my v4 version of this patch is ok? see: http://patchwork.ozlabs.org/patch/121295/ Maybe we should rename the CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST in drivers/mtd/nand/davinci_nand.c to CONFIG_SYS_DAVINCI_NAND_HW_4BIT in a seperate patch? Sandeep? bye, Heiko
On 10/28/2011 01:05 AM, Heiko Schocher wrote: > Hello Scott, > > Scott Wood wrote: >> On 10/27/2011 12:23 AM, Heiko Schocher wrote: >>>>> In drivers/mtd/nand/davinci_nand.c CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST >>>>> is used, and there are 4bit specific functions, so this define is >>>>> also valid, just we need in drivers/mtd/nand/nand_spl_simple.c a more >>>>> common name, right? >>>> Right. Ideally, though, that would either be CONFIG_SYS_DAVINCI_... or >>>> #if defined(CONFIG_SYS_NAND_HW_ECC_OOBFIRST) && \ >>>> defined(CONFIG_SYS_NAND_HW_ECC_4BIT) >>> Hmm.. I thought you meant, this is not davinci nor 4bit specific? >>> Or do you mean to rename the CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST >>> define in drivers/mtd/nand/davinci_nand.c? >> >> I just mean that "4bit" and "oobfirst" are independent things, so I'd >> rather not have something that combines the two as part of the >> configuration of the general NAND interface in the absence of a >> requirement to enumerate all possibilities. OTOH, the structure of >> driver's private config can be whatever makes the most sense for that >> driver, but should be named in a way that it's obviously driver-specific. > > So my v4 version of this patch is ok? see: > http://patchwork.ozlabs.org/patch/121295/ Yes, the patch looks good -- ACK sent. > Maybe we should rename the CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST > in drivers/mtd/nand/davinci_nand.c to CONFIG_SYS_DAVINCI_NAND_HW_4BIT > in a seperate patch? Sure, that'd be good. -Scott
diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c index 71491d4..622a9b5 100644 --- a/drivers/mtd/nand/nand_spl_simple.c +++ b/drivers/mtd/nand/nand_spl_simple.c @@ -140,6 +140,47 @@ static int nand_is_bad_block(int block) return 0; } +#if defined(CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST) +static int nand_read_page(int block, int page, uchar *dst) +{ + struct nand_chip *this = mtd.priv; + u_char *ecc_calc; + u_char *ecc_code; + u_char *oob_data; + int i; + int eccsize = CONFIG_SYS_NAND_ECCSIZE; + int eccbytes = CONFIG_SYS_NAND_ECCBYTES; + int eccsteps = CONFIG_SYS_NAND_ECCSTEPS; + uint8_t *p = dst; + int stat; + + /* + * No malloc available for now, just use some temporary locations + * in SDRAM + */ + ecc_calc = (u_char *)(CONFIG_SYS_SDRAM_BASE + 0x10000); + ecc_code = ecc_calc + 0x100; + oob_data = ecc_calc + 0x200; + + nand_command(block, page, 0, NAND_CMD_READOOB); + this->read_buf(&mtd, oob_data, CONFIG_SYS_NAND_OOBSIZE); + nand_command(block, page, 0, NAND_CMD_READ0); + + /* Pick the ECC bytes out of the oob data */ + for (i = 0; i < CONFIG_SYS_NAND_ECCTOTAL; i++) + ecc_code[i] = oob_data[nand_ecc_pos[i]]; + + + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { + this->ecc.hwctl(&mtd, NAND_ECC_READ); + this->read_buf(&mtd, p, eccsize); + this->ecc.calculate(&mtd, p, &ecc_calc[i]); + stat = this->ecc.correct(&mtd, p, &ecc_code[i], &ecc_calc[i]); + } + + return 0; +} +#else static int nand_read_page(int block, int page, void *dst) { struct nand_chip *this = mtd.priv; @@ -186,6 +227,7 @@ static int nand_read_page(int block, int page, void *dst) return 0; } +#endif int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst) { @@ -230,7 +272,6 @@ void nand_init(void) mtd.priv = &nand_chip; nand_chip.IO_ADDR_R = nand_chip.IO_ADDR_W = (void __iomem *)CONFIG_SYS_NAND_BASE; - nand_chip.options = 0; board_nand_init(&nand_chip); if (nand_chip.select_chip)
similiar to commit dc7cd8e59ba077f3b4c1a4557c9cd86a31b9ab1f, only adapted for the new spl framework. Signed-off-by: Heiko Schocher <hs@denx.de> Cc: Scott Wood <scottwood@freescale.com> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net> Cc: Sandeep Paulraj <s-paulraj@ti.com> --- changes for v3: - add comment from Scott Wood: as BSS is cleared, no need for intializing vars with 0 remove this. drivers/mtd/nand/nand_spl_simple.c | 43 +++++++++++++++++++++++++++++++++++- 1 files changed, 42 insertions(+), 1 deletions(-)