Patchwork [U-Boot,05/10,v3] spl, nand: add 4bit HW ecc oob first nand_read_page function

login
register
mail settings
Submitter Heiko Schocher
Date Oct. 6, 2011, 5:45 a.m.
Message ID <1317879924-18266-1-git-send-email-hs@denx.de>
Download mbox | patch
Permalink /patch/117950/
State Changes Requested
Headers show

Comments

Heiko Schocher - Oct. 6, 2011, 5:45 a.m.
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(-)
Tom Rini - Oct. 6, 2011, 4:05 p.m.
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?
Scott Wood - Oct. 10, 2011, 9:17 p.m.
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
Heiko Schocher - Oct. 11, 2011, 5:41 a.m.
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
Wolfgang Denk - Oct. 11, 2011, 5:49 a.m.
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
Scott Wood - Oct. 25, 2011, 3:24 p.m.
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
Heiko Schocher - Oct. 26, 2011, 6:16 a.m.
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
Scott Wood - Oct. 26, 2011, 4:58 p.m.
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
Heiko Schocher - Oct. 27, 2011, 5:23 a.m.
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
Scott Wood - Oct. 27, 2011, 6:14 p.m.
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
Heiko Schocher - Oct. 28, 2011, 6:05 a.m.
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
Scott Wood - Oct. 28, 2011, 4:03 p.m.
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

Patch

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)