[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[s-paulraj at ti.com]

From: Sandeep Paulraj <s-paulraj@ti.com>

This patch adds 4 BIT ecc support in the DaVinci NAND
driver. Tested on both the DM355 and DM365.


Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
---
 drivers/mtd/nand/davinci_nand.c          |  290 +++++++++++++++++++++++++++++-
 include/asm-arm/arch-davinci/emif_defs.h |   10 +
 2 files changed, 298 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 7837a8e..6a53037 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -47,6 +47,16 @@
 #include <asm/arch/nand_defs.h>
 #include <asm/arch/emif_defs.h>
 
+/* Definitions for 4-bit hardware ECC */
+#define NAND_TIMEOUT			10240
+#define NAND_ECC_BUSY			0xC
+#define NAND_4BITECC_MASK		0x03FF03FF
+#define EMIF_NANDFSR_ECC_STATE_MASK  	0x00000F00
+#define ECC_STATE_NO_ERR		0x0
+#define ECC_STATE_TOO_MANY_ERRS		0x1
+#define ECC_STATE_ERR_CORR_COMP_P	0x2
+#define ECC_STATE_ERR_CORR_COMP_N	0x3
+
 static emif_registers *const emif_regs = (void *) DAVINCI_ASYNC_EMIF_CNTRL_BASE;
 
 static void nand_davinci_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
@@ -170,6 +180,274 @@ static int nand_davinci_correct_data(struct mtd_info *mtd, u_char *dat, u_char *
 }
 #endif /* CONFIG_SYS_NAND_HW_ECC */
 
+#ifdef CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST
+static struct nand_ecclayout nand_davinci_4bit_layout_oobfirst = {
+/*
+ * TI uses a different layout for 4K page deviecs. Since the
+ * eccpos filed can hold only a limited number of entries, adding
+ * support for 4K page will result in compilation warnings
+ * 4K Support will be added later
+ */
+#ifdef CONFIG_SYS_NAND_PAGE_2K
+	.eccbytes = 40,
+	.eccpos = {
+		24, 25, 26, 27, 28,
+		29, 30, 31, 32, 33, 34, 35, 36, 37, 38,
+		39, 40, 41, 42, 43, 44, 45, 46, 47, 48,
+		49, 50, 51, 52, 53, 54, 55, 56, 57, 58,
+		59, 60, 61, 62, 63,
+		},
+	.oobfree = {
+		{.offset = 2, .length = 22, },
+	},
+#endif
+};
+#endif
+
+static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode)
+{
+	u32 val;
+
+	switch (mode) {
+	case NAND_ECC_WRITE:
+	case NAND_ECC_READ:
+		/*
+		 * Start a new ECC calculation for reading or writing 512 bytes
+		 * of data.
+		 */
+		val = (emif_regs->NANDFCR & ~(3 << 4)) | (1 << 12);
+		emif_regs->NANDFCR = val;
+		break;
+	case NAND_ECC_READSYN:
+		val = emif_regs->NAND4BITECC1;
+		break;
+	default:
+		break;
+	}
+}
+
+static u32 nand_davinci_4bit_readecc(struct mtd_info *mtd, unsigned int ecc[4])
+{
+	ecc[0] = emif_regs->NAND4BITECC1 & NAND_4BITECC_MASK;
+	ecc[1] = emif_regs->NAND4BITECC2 & NAND_4BITECC_MASK;
+	ecc[2] = emif_regs->NAND4BITECC3 & NAND_4BITECC_MASK;
+	ecc[3] = emif_regs->NAND4BITECC4 & NAND_4BITECC_MASK;
+
+	return 0;
+}
+
+static int nand_davinci_4bit_calculate_ecc(struct mtd_info *mtd,
+					   const uint8_t *dat,
+					   uint8_t *ecc_code)
+{
+	unsigned int hw_4ecc[4] = { 0, 0, 0, 0 };
+	unsigned int const1 = 0, const2 = 0;
+	unsigned char count1 = 0;
+
+	/*
+	 * Since the NAND_HWECC_SYNDROME option is enabled, this routine is
+	 * only called just after the data and oob have been written.  The
+	 * ECC value calculated by the hardware ECC generator is available
+	 * for us to read.
+	 */
+	nand_davinci_4bit_readecc(mtd, hw_4ecc);
+
+	/*Convert 10 bit ecc value to 8 bit */
+	for (count1 = 0; count1 < 2; count1++) {
+		const2 = count1 * 5;
+		const1 = count1 * 2;
+
+		/* Take first 8 bits from val1 (count1=0) or val5 (count1=1) */
+		ecc_code[const2] = hw_4ecc[const1] & 0xFF;
+
+		/*
+		 * Take 2 bits as LSB bits from val1 (count1=0) or val5
+		 * (count1=1) and 6 bits from val2 (count1=0) or
+		 * val5 (count1=1)
+		 */
+		ecc_code[const2 + 1] =
+		    ((hw_4ecc[const1] >> 8) & 0x3) | ((hw_4ecc[const1] >> 14) &
+						      0xFC);
+
+		/*
+		 * Take 4 bits from val2 (count1=0) or val5 (count1=1) and
+		 * 4 bits from val3 (count1=0) or val6 (count1=1)
+		 */
+		ecc_code[const2 + 2] =
+		    ((hw_4ecc[const1] >> 22) & 0xF) |
+		    ((hw_4ecc[const1 + 1] << 4) & 0xF0);
+
+		/*
+		 * Take 6 bits from val3(count1=0) or val6 (count1=1) and
+		 * 2 bits from val4 (count1=0) or  val7 (count1=1)
+		 */
+		ecc_code[const2 + 3] =
+		    ((hw_4ecc[const1 + 1] >> 4) & 0x3F) |
+		    ((hw_4ecc[const1 + 1] >> 10) & 0xC0);
+
+		/* Take 8 bits from val4 (count1=0) or val7 (count1=1) */
+		ecc_code[const2 + 4] = (hw_4ecc[const1 + 1] >> 18) & 0xFF;
+	}
+	return 0;
+}
+
+static int nand_davinci_4bit_compare_ecc(struct mtd_info *mtd,
+					 uint8_t *read_ecc,
+					 uint8_t *page_data)
+{
+	struct nand_chip *this = mtd->priv;
+	unsigned short ecc_10bit[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+	int i;
+	unsigned int hw_4ecc[4] = { 0, 0, 0, 0 }, iserror = 0;
+	unsigned short *pspare = NULL, *pspare1 = NULL;
+	unsigned int numErrors, errorAddress, errorValue;
+	u32 val;
+
+	/*
+	 * Check for an ECC where all bytes are 0xFF.  If this is the case, we
+	 * will assume we are looking at an erased page and we should ignore
+	 * the ECC.
+	 */
+	for (i = 0; i < 10; i++) {
+		if (read_ecc[i] != 0xFF)
+			break;
+	}
+	if (i == 10)
+		return 0;
+
+	/* Convert 8 bit in to 10 bit */
+	pspare = (unsigned short *)&read_ecc[2];
+	pspare1 = (unsigned short *)&read_ecc[0];
+	/* Take 10 bits from 0th and 1st bytes */
+	ecc_10bit[0] = (*pspare1) & 0x3FF;	/* 10 */
+	/* Take 6 bits from 1st byte and 4 bits from 2nd byte */
+	ecc_10bit[1] = (((*pspare1) >> 10) & 0x3F)
+	    | (((pspare[0]) << 6) & 0x3C0);	/* 6 + 4 */
+	/* Take 4 bits form 2nd bytes and 6 bits from 3rd bytes */
+	ecc_10bit[2] = ((pspare[0]) >> 4) & 0x3FF;	/* 10 */
+	/*Take 2 bits from 3rd byte and 8 bits from 4th byte */
+	ecc_10bit[3] = (((pspare[0]) >> 14) & 0x3)
+	    | ((((pspare[1])) << 2) & 0x3FC);	/* 2 + 8 */
+	/* Take 8 bits from 5th byte and 2 bits from 6th byte */
+	ecc_10bit[4] = ((pspare[1]) >> 8)
+	    | ((((pspare[2])) << 8) & 0x300);	/* 8 + 2 */
+	/* Take 6 bits from 6th byte and 4 bits from 7th byte */
+	ecc_10bit[5] = (pspare[2] >> 2) & 0x3FF;	/* 10 */
+	/* Take 4 bits from 7th byte and 6 bits from 8th byte */
+	ecc_10bit[6] = (((pspare[2]) >> 12) & 0xF)
+	    | ((((pspare[3])) << 4) & 0x3F0);	/* 4 + 6 */
+	/*Take 2 bits from 8th byte and 8 bits from 9th byte */
+	ecc_10bit[7] = ((pspare[3]) >> 6) & 0x3FF;	/* 10 */
+
+	/*
+	 * Write the parity values in the NAND Flash 4-bit ECC Load register.
+	 * Write each parity value one at a time starting from 4bit_ecc_val8
+	 * to 4bit_ecc_val1.
+	 */
+	for (i = 7; i >= 0; i--)
+		emif_regs->NAND4BITECCLOAD = ecc_10bit[i];
+
+	/*
+	 * Perform a dummy read to the EMIF Revision Code and Status register.
+	 * This is required to ensure time for syndrome calculation after
+	 * writing the ECC values in previous step.
+	 */
+
+	val = emif_regs->NANDFSR;
+
+	/*
+	 * Read the syndrome from the NAND Flash 4-Bit ECC 1-4 registers.
+	 * A syndrome value of 0 means no bit errors. If the syndrome is
+	 * non-zero then go further otherwise return.
+	 */
+	nand_davinci_4bit_readecc(mtd, hw_4ecc);
+
+	if (hw_4ecc[0] == ECC_STATE_NO_ERR && hw_4ecc[1] == ECC_STATE_NO_ERR &&
+	    hw_4ecc[2] == ECC_STATE_NO_ERR && hw_4ecc[3] == ECC_STATE_NO_ERR)
+		return 0;
+
+	/*
+	 * Clear any previous address calculation by doing a dummy read of an
+	 * error address register.
+	 */
+	val = emif_regs->NANDERRADD1;
+
+	/*
+	 * Set the addr_calc_st bit(bit no 13) in the NAND Flash Control
+	 * register to 1.
+	 */
+	emif_regs->NANDFCR |= 1 << 13;
+
+	/*
+	 * Wait for the corr_state field (bits 8 to 11)in the
+	 * NAND Flash Status register to be equal to 0x0, 0x1, 0x2, or 0x3.
+	 */
+	i = NAND_TIMEOUT;
+	do {
+		val = emif_regs->NANDFSR;
+		val &= 0xc00;
+		i--;
+	} while ((i > 0) && val);
+
+	iserror = emif_regs->NANDFSR;
+	iserror &= EMIF_NANDFSR_ECC_STATE_MASK;
+	iserror = iserror >> 8;
+
+	/*
+	 * ECC_STATE_TOO_MANY_ERRS (0x1) means errors cannot be
+	 * corrected (five or more errors).  The number of errors
+	 * calculated (err_num field) differs from the number of errors
+	 * searched.  ECC_STATE_ERR_CORR_COMP_P (0x2) means error
+	 * correction complete (errors on bit 8 or 9).
+	 * ECC_STATE_ERR_CORR_COMP_N (0x3) means error correction
+	 * complete (error exists).
+	 */
+	for (i = 0; i < 100; i++)
+		udelay(this->chip_delay);
+
+	if (iserror == ECC_STATE_NO_ERR) {
+		val = emif_regs->NANDERRVAL1;
+		return 0;
+	} else if (iserror == ECC_STATE_TOO_MANY_ERRS) {
+		val = emif_regs->NANDERRVAL1;
+		return -1;
+	}
+
+	numErrors = ((emif_regs->NANDFSR >> 16) & 0x3) + 1;
+
+	/* Read the error address, error value and correct */
+	for (i = 0; i < numErrors; i++) {
+		if (i > 1) {
+			errorAddress =
+			    ((emif_regs->NANDERRADD2 >>
+			      (16 * (i & 1))) & 0x3FF);
+			errorAddress = ((512 + 7) - errorAddress);
+			errorValue =
+			    ((emif_regs->NANDERRVAL2 >>
+			      (16 * (i & 1))) & 0xFF);
+		} else {
+			errorAddress =
+			    ((emif_regs->NANDERRADD1 >>
+			      (16 * (i & 1))) & 0x3FF);
+			errorAddress = ((512 + 7) - errorAddress);
+			errorValue =
+			    ((emif_regs->NANDERRVAL1 >>
+			      (16 * (i & 1))) & 0xFF);
+		}
+		/* xor the corrupt data with error value */
+		if (errorAddress < 512)
+			page_data[errorAddress] ^= errorValue;
+	}
+
+	return numErrors;
+}
+
+static int nand_davinci_4bit_correct_data(struct mtd_info *mtd, uint8_t *dat,
+					  uint8_t *read_ecc, uint8_t *calc_ecc)
+{
+	return nand_davinci_4bit_compare_ecc(mtd, read_ecc, dat);
+}
+
 static int nand_davinci_dev_ready(struct mtd_info *mtd)
 {
 	return emif_regs->NANDFSR & 0x1;
@@ -215,7 +493,7 @@ void davinci_nand_init(struct nand_chip *nand)
 {
 	nand->chip_delay  = 0;
 #ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
-	nand->options	  = NAND_USE_FLASH_BBT;
+	nand->options	  |= NAND_USE_FLASH_BBT;
 #endif
 #ifdef CONFIG_SYS_NAND_HW_ECC
 	nand->ecc.mode = NAND_ECC_HW;
@@ -227,7 +505,15 @@ void davinci_nand_init(struct nand_chip *nand)
 #else
 	nand->ecc.mode = NAND_ECC_SOFT;
 #endif /* CONFIG_SYS_NAND_HW_ECC */
-
+#ifdef CONFIG_SYS_NAND_4BIT_HW_ECC_OOBFIRST
+	nand->ecc.mode = NAND_ECC_HW_OOB_FIRST;
+	nand->ecc.size = 512;
+	nand->ecc.bytes = 10;
+	nand->ecc.calculate = nand_davinci_4bit_calculate_ecc;
+	nand->ecc.correct = nand_davinci_4bit_correct_data;
+	nand->ecc.hwctl = nand_davinci_4bit_enable_hwecc;
+	nand->ecc.layout = &nand_davinci_4bit_layout_oobfirst;
+#endif
 	/* Set address of hardware control function */
 	nand->cmd_ctrl = nand_davinci_hwcontrol;
 
diff --git a/include/asm-arm/arch-davinci/emif_defs.h b/include/asm-arm/arch-davinci/emif_defs.h
index 646fc77..c91e30c 100644
--- a/include/asm-arm/arch-davinci/emif_defs.h
+++ b/include/asm-arm/arch-davinci/emif_defs.h
@@ -55,6 +55,16 @@ typedef struct {
 	dv_reg		NANDF2ECC;
 	dv_reg		NANDF3ECC;
 	dv_reg		NANDF4ECC;
+	u_int8_t	RSVD2[60];
+	dv_reg		NAND4BITECCLOAD;
+	dv_reg		NAND4BITECC1;
+	dv_reg		NAND4BITECC2;
+	dv_reg		NAND4BITECC3;
+	dv_reg		NAND4BITECC4;
+	dv_reg		NANDERRADD1;
+	dv_reg		NANDERRADD2;
+	dv_reg		NANDERRVAL1;
+	dv_reg		NANDERRVAL2;
 } emif_registers;
 
 typedef emif_registers	*emifregs;
-- 
1.6.0.4

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:45 Tue 11 Aug     , s-paulraj at ti.com wrote:
> From: Sandeep Paulraj <s-paulraj@ti.com>
> 
> This patch adds 4 BIT ecc support in the DaVinci NAND
> driver. Tested on both the DM355 and DM365.
> 
> 
> Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c          |  290 +++++++++++++++++++++++++++++-
>  include/asm-arm/arch-davinci/emif_defs.h |   10 +
>  2 files changed, 298 insertions(+), 2 deletions(-)
Scoot is this ok for you?

if yes I'd like to apply it with the board udpate to my next

Best Regards,
J.

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Wolfgang Denk]

Dear Jean-Christophe PLAGNIOL-VILLARD,

In message <20090817220313.GK23695@game.jcrosoft.org> you wrote:
> On 10:45 Tue 11 Aug     , s-paulraj at ti.com wrote:
> > From: Sandeep Paulraj <s-paulraj@ti.com>
> > 
> > This patch adds 4 BIT ecc support in the DaVinci NAND
> > driver. Tested on both the DM355 and DM365.
> > 
> > 
> > Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
> > ---
> >  drivers/mtd/nand/davinci_nand.c          |  290 +++++++++++++++++++++++++++++-
> >  include/asm-arm/arch-davinci/emif_defs.h |   10 +
> >  2 files changed, 298 insertions(+), 2 deletions(-)
> Scoot is this ok for you?

I guess you mean Scott here.

> if yes I'd like to apply it with the board udpate to my next

You probably want to add him on cc: if you want his opinion.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Don't put off for tomorrow what you can  do  today,  because  if  you
enjoy it today you can do it again tomorrow.

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Scott Wood]

On Tue, Aug 18, 2009 at 12:03:13AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 10:45 Tue 11 Aug     , s-paulraj at ti.com wrote:
> > From: Sandeep Paulraj <s-paulraj@ti.com>
> > 
> > This patch adds 4 BIT ecc support in the DaVinci NAND
> > driver. Tested on both the DM355 and DM365.
> > 
> > 
> > Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
> > ---
> >  drivers/mtd/nand/davinci_nand.c          |  290 +++++++++++++++++++++++++++++-
> >  include/asm-arm/arch-davinci/emif_defs.h |   10 +
> >  2 files changed, 298 insertions(+), 2 deletions(-)
> Scoot is this ok for you?
> 
> if yes I'd like to apply it with the board udpate to my next

It won't build without a change destined for my next branch
(NAND_ECC_HW_OOB_FIRST).

Does it depend on anything in your next that isn't in Wolfgang's next? 

-Scott

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Scott Wood]

On Tue, Aug 11, 2009 at 10:45:05AM -0400, s-paulraj at ti.com wrote:
> +static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode)
> +{
> +	u32 val;
> +
> +	switch (mode) {
> +	case NAND_ECC_WRITE:
> +	case NAND_ECC_READ:
> +		/*
> +		 * Start a new ECC calculation for reading or writing 512 bytes
> +		 * of data.
> +		 */
> +		val = (emif_regs->NANDFCR & ~(3 << 4)) | (1 << 12);
> +		emif_regs->NANDFCR = val;
> +		break;
> +	case NAND_ECC_READSYN:
> +		val = emif_regs->NAND4BITECC1;

Use I/O accessors.

> +	for (i = 0; i < 100; i++)
> +		udelay(this->chip_delay);

No explanation for the delay?  Is there any status register you can poll?

Is it truly 100 times the chip delay (even if that changes), or is it a
fixed delay that just happens to work out to that?

> +static int nand_davinci_4bit_correct_data(struct mtd_info *mtd, uint8_t *dat,
> +					  uint8_t *read_ecc, uint8_t *calc_ecc)
> +{
> +	return nand_davinci_4bit_compare_ecc(mtd, read_ecc, dat);
> +}

Why have a wrapper?  This seems to be the only place where compare_ecc is
used.

-Scott

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Paulraj, Sandeep]

> > +static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int
> mode)
> > +{
> > +	u32 val;
> > +
> > +	switch (mode) {
> > +	case NAND_ECC_WRITE:
> > +	case NAND_ECC_READ:
> > +		/*
> > +		 * Start a new ECC calculation for reading or writing 512
> bytes
> > +		 * of data.
> > +		 */
> > +		val = (emif_regs->NANDFCR & ~(3 << 4)) | (1 << 12);
> > +		emif_regs->NANDFCR = val;
> > +		break;
> > +	case NAND_ECC_READSYN:
> > +		val = emif_regs->NAND4BITECC1;
> 
> Use I/O accessors.
I could not understand this one. It is done similarly nand_davinci_enable_hwecc which is used for 1 BIT ECC.
NANDFCR is a control register and we have to write the appropriate valvue to it.
We similarly write to other register that are part of the IP in other sections of this driver.

> 
> > +	for (i = 0; i < 100; i++)
> > +		udelay(this->chip_delay);
> 
> No explanation for the delay?  Is there any status register you can poll?
> 
> Is it truly 100 times the chip delay (even if that changes), or is it a
> fixed delay that just happens to work out to that?
remnant of an old version of the driver which had bugs. I have got rid of this
> 
> > +static int nand_davinci_4bit_correct_data(struct mtd_info *mtd, uint8_t
> *dat,
> > +					  uint8_t *read_ecc, uint8_t *calc_ecc)
> > +{
> > +	return nand_davinci_4bit_compare_ecc(mtd, read_ecc, dat);
> > +}
> 
> Why have a wrapper?  This seems to be the only place where compare_ecc is
> used.
Yes its of no use, I'll remove it.
> 
> -Scott
I'll send in a patch with your comments addressed except the first one. I can resend the patch( again if required) once I know what you mean by your comment #1.

Thanks,
Sandeep

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Scott Wood]

On Tue, Aug 18, 2009 at 08:56:03AM -0500, Paulraj, Sandeep wrote:
> > > +	case NAND_ECC_READSYN:
> > > +		val = emif_regs->NAND4BITECC1;
> > 
> > Use I/O accessors.
> I could not understand this one. It is done similarly nand_davinci_enable_hwecc which is used for 1 BIT ECC.
> NANDFCR is a control register and we have to write the appropriate valvue to it.
> We similarly write to other register that are part of the IP in other sections of this driver.

Well, the comment applies to the rest of the driver too.  I won't NACK
because of it, but it's something to consider in the future.

> I'll send in a patch with your comments addressed except the first one. I can resend the patch( again if required) once I know what you mean by your comment #1.

What I mean is using accessors such as readl/writel -- though we're still
in a pretty sorry state with respect to what accessors we provide cross
architectures (you have to pick between native endian with no barriers,
or little endian with barriers).

-Scott

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Paulraj, Sandeep]

> -----Original Message-----
> From: Scott Wood [mailto:scottwood at freescale.com]
> Sent: Tuesday, August 18, 2009 11:12 AM
> To: Paulraj, Sandeep
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support
> 
> On Tue, Aug 18, 2009 at 08:56:03AM -0500, Paulraj, Sandeep wrote:
> > > > +	case NAND_ECC_READSYN:
> > > > +		val = emif_regs->NAND4BITECC1;
> > >
> > > Use I/O accessors.
> > I could not understand this one. It is done similarly
> nand_davinci_enable_hwecc which is used for 1 BIT ECC.
> > NANDFCR is a control register and we have to write the appropriate
> valvue to it.
> > We similarly write to other register that are part of the IP in other
> sections of this driver.
> 
> Well, the comment applies to the rest of the driver too.  I won't NACK
> because of it, but it's something to consider in the future.
> 
> > I'll send in a patch with your comments addressed except the first one.
> I can resend the patch( again if required) once I know what you mean by
> your comment #1.
> 
> What I mean is using accessors such as readl/writel -- though we're still
> in a pretty sorry state with respect to what accessors we provide cross
> architectures (you have to pick between native endian with no barriers,
> or little endian with barriers).

Scott, I am quite certain that I have received comments for other patches that I have submitted earlier where I have been specifically been asked to do things the way I am doing at the moment and not use readl/writel(because of the reason you mentioned above). 
> 
> -Scott

So should I assume that my updated patch has your ACK as you mentioned above that you want NACK it

Thanks,
Sandeep

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Scott Wood]

Paulraj, Sandeep wrote:
>> What I mean is using accessors such as readl/writel -- though we're still
>> in a pretty sorry state with respect to what accessors we provide cross
>> architectures (you have to pick between native endian with no barriers,
>> or little endian with barriers).
> 
> Scott, I am quite certain that I have received comments for other
> patches that I have submitted earlier where I have been specifically
> been asked to do things the way I am doing at the moment and not use
> readl/writel(because of the reason you mentioned above).

OK, I didn't see those comments.

> So should I assume that my updated patch has your ACK as you mentioned above that you want NACK it

It looks OK.

-Scott

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Scott Wood]

Paulraj, Sandeep wrote:
> And please correct me if I am wrong, but J-C can't actually pull these patches into his next branch correct?

Right, not yet.

> That is because it will require 2 of the NAND patches that I sent a week ago.
> 
> Thus you might have to accept the DM355 EVM config update patch as well which actually enables the NAND support.

I could do that with an ACK from JC if it does not depend on anything in 
his tree -- or I could send Wolfgang a -next pull request and then JC 
could sync with that.

-Scott

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Wolfgang Denk]

Dear Scott Wood,

In message <4A8ACBD0.6060309@freescale.com> you wrote:
> Paulraj, Sandeep wrote:
> > And please correct me if I am wrong, but J-C can't actually pull these patches into his next branch correct?
> 
> Right, not yet.
> 
> > That is because it will require 2 of the NAND patches that I sent a week ago.
> > 
> > Thus you might have to accept the DM355 EVM config update patch as well which actually enables the NAND support.
> 
> I could do that with an ACK from JC if it does not depend on anything in 
> his tree -- or I could send Wolfgang a -next pull request and then JC 
> could sync with that.

Jean-Christophe has acked this one - see
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/65792/focus=66144

But I'm concerned about direct register accesses. We really should
insist on using proper accessor calls.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Always leave room to add an explanation if it doesn't work out.

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Paulraj, Sandeep]

> Jean-Christophe has acked this one - see
> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/65792/focus=6614
He said he'll ACK if this patch is ACK'ed. He has not acked yet.
> 
> But I'm concerned about direct register accesses. We really should
> insist on using proper accessor calls.
I'll rework my driver based on your comments. 
> 
> Best regards,
> 
> Wolfgang Denk

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Wolfgang Denk]

Dear "Paulraj, Sandeep",

In message <0554BEF07D437848AF01B9C9B5F0BC5D7E99635F@dlee01.ent.ti.com> you wrote:
> 
> > What I mean is using accessors such as readl/writel -- though we're still
> > in a pretty sorry state with respect to what accessors we provide cross
> > architectures (you have to pick between native endian with no barriers,
> > or little endian with barriers).
> 
> Scott, I am quite certain that I have received comments for other patches that I have submitted earlier where I have been specifically been asked to do things the way I am doing at the moment and not use readl/writel(because of the reason you mentioned 
> above). 

Can you please use shorter lines? Something like a maximum lin leght
of 70 characters or so?  Thanks.

Well, this being a DaVinci driver, cross architectur poretability does
not really matter: this is ARM, and little-endian.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The bad reputation UNIX has gotten is totally undeserved, laid on by
people who don't understand, who have not gotten in there  and  tried
anything."          -- Jim Joyce, owner of Jim Joyce's UNIX Bookstore

[U-Boot] [PATCH] NAND: DaVinci: Adding 4 BIT ECC support

[Wolfgang Denk]

Dear Scott Wood,

In message <20090818151213.GB26119@b07421-ec1.am.freescale.net> you wrote:
> On Tue, Aug 18, 2009 at 08:56:03AM -0500, Paulraj, Sandeep wrote:
> > > > +	case NAND_ECC_READSYN:
> > > > +		val = emif_regs->NAND4BITECC1;
> > > 
> > > Use I/O accessors.
> > I could not understand this one. It is done similarly nand_davinci_enable_hwecc which is used for 1 BIT ECC.
> > NANDFCR is a control register and we have to write the appropriate valvue to it.
> > We similarly write to other register that are part of the IP in other sections of this driver.
> 
> Well, the comment applies to the rest of the driver too.  I won't NACK
> because of it, but it's something to consider in the future.

Well, I will NAK it, then.

We should really write clean code these days.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Some people march to the beat of a different drummer. And some people
tango!