Skip to contentSkip to conversation

NAND: DaVinci: Adding 4 BIT ECC support

PATCH Superseded s-paulraj at ti.com 5 participants 14 messages Aug 11, 2009 – Aug 18, 2009 1 message not in the archive

Subsystems (2)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

From: Sandeep Paulraj <[email protected]>

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

---
 drivers/mtd/nand/davinci_nand.c          |  290 +++++++++++++++++++++++++++++-
 include/asm-arm/arch-davinci/emif_defs.h |   10 +
 2 files changed, 298 insertions(+), 2 deletions(-)
drivers/mtd/nand/davinci_nand.c +288 -2
index 7837a8e..6a53037 100644
@@ -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;
 
include/asm-arm/arch-davinci/emif_defs.h +10 -0
index 646fc77..c91e30c 100644
@@ -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;
Signature · 1 line
1.6.0.4
#1 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

Quoted 11 lines · s-paulraj at ti.com

From: Sandeep Paulraj <[email protected]>

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

---
 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.

#2 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

Dear Jean-Christophe PLAGNIOL-VILLARD,

Quoted 7 lines · In message <[email protected]> you wrote:
Quoted 6 lines · s-paulraj at ti.com

From: Sandeep Paulraj <[email protected]>

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

Quoted 6 lines
Quoted 5 lines
---
 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.

Quoted 1 line

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

Signature · 5 lines
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.
#3 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

In reply to Jean-Christophe PLAGNIOL-VILLARD
Quoted 7 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 6 lines · s-paulraj at ti.com

From: Sandeep Paulraj <[email protected]>

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

Quoted 8 lines
Quoted 5 lines
---
 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

#4 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

In reply to s-paulraj at ti.com
Quoted 6 lines · s-paulraj at ti.com
drivers/mtd/nand/davinci_nand.c +6 -0
+static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int mode)
+{
+	u32 val;
+
+	switch (mode) {
+	case NAND_ECC_WRITE:

drivers/mtd/nand/davinci_nand.c static int nand_davinci_correct_data(struct mtd_info *mtd, u_char *dat, u_char * · line 222

Quoted 10 lines
drivers/mtd/nand/davinci_nand.c +10 -0
+	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.

drivers/mtd/nand/davinci_nand.c static int nand_davinci_correct_data(struct mtd_info *mtd, u_char *dat, u_char * · line 406

Quoted 2 lines
drivers/mtd/nand/davinci_nand.c +2 -0
+	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?

drivers/mtd/nand/davinci_nand.c static int nand_davinci_correct_data(struct mtd_info *mtd, u_char *dat, u_char * · line 449

Quoted 5 lines
drivers/mtd/nand/davinci_nand.c +5 -0
+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

#5 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

Quoted 12 lines
Quoted 2 lines

+static void nand_davinci_4bit_enable_hwecc(struct mtd_info *mtd, int

mode)

drivers/mtd/nand/davinci_nand.c static int nand_davinci_correct_data(struct mtd_info *mtd, u_char *dat, u_char *

Quoted 8 lines
drivers/mtd/nand/davinci_nand.c +8 -0
+{
+	u32 val;
+
+	switch (mode) {
+	case NAND_ECC_WRITE:
+	case NAND_ECC_READ:
+		/*
+		 * Start a new ECC calculation for reading or writing 512

bytes

Quoted 9 lines

drivers/mtd/nand/davinci_nand.c static int nand_davinci_correct_data(struct mtd_info *mtd, u_char *dat, u_char * · line 222

Quoted 7 lines
drivers/mtd/nand/davinci_nand.c +7 -0
+		 * 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.

Quoted 8 lines

drivers/mtd/nand/davinci_nand.c static int nand_davinci_correct_data(struct mtd_info *mtd, u_char *dat, u_char * · line 406

Quoted 3 lines
drivers/mtd/nand/davinci_nand.c +2 -0
+	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

Quoted 10 lines
Quoted 2 lines

+static int nand_davinci_4bit_correct_data(struct mtd_info *mtd, uint8_t

*dat,

drivers/mtd/nand/davinci_nand.c static int nand_davinci_correct_data(struct mtd_info *mtd, u_char *dat, u_char * · line 449

Quoted 4 lines
drivers/mtd/nand/davinci_nand.c +4 -0
+					  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.

Quoted 2 lines

-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

#6 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

Quoted 7 lines · Sandeep
Quoted 4 lines

drivers/mtd/nand/davinci_nand.c static int nand_davinci_correct_data(struct mtd_info *mtd, u_char *dat, u_char * · line 222

Quoted 2 lines
drivers/mtd/nand/davinci_nand.c +2 -0
+	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.

Quoted 1 line

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

#7 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

Quoted 24 lines

-----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

Quoted 5 lines · Sandeep
Quoted 4 lines

drivers/mtd/nand/davinci_nand.c static int nand_davinci_correct_data(struct mtd_info *mtd, u_char *dat, u_char * · line 222

Quoted 2 lines
drivers/mtd/nand/davinci_nand.c +2 -0
+	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.

Quoted 1 line

NANDFCR is a control register and we have to write the appropriate

valvue to it.

Quoted 1 line

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.

Quoted 8 lines
Quoted 1 line

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).

Quoted 2 lines

-Scott

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

Thanks,
Sandeep

#8 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

Quoted 9 lines · Sandeep
Quoted 4 lines

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.

Quoted 1 line

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

It looks OK.

-Scott

A message that is not in the archive (sent off-list or not yet received): [email protected]

#10 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

Quoted 1 line · Sandeep

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.

Quoted 3 lines

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

#11 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

Dear Scott Wood,

Quoted 5 lines · In message <[email protected]> you wrote:
Quoted 1 line · Sandeep

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.

Quoted 7 lines
Quoted 3 lines

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

Signature · 4 lines
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.
#12 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

Best regards,

Wolfgang Denk

#13 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

In reply to Paulraj

Dear "Paulraj, Sandeep",

Quoted 8 lines · In message <[email protected]> you…
Quoted 5 lines

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

Signature · 6 lines
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
#14 lore raw

PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support

In reply to Scott Wood

Dear Scott Wood,

Quoted 11 lines · In message <[email protected]> you wrote:
Quoted 7 lines · Sandeep
Quoted 4 lines

drivers/mtd/nand/davinci_nand.c static int nand_davinci_correct_data(struct mtd_info *mtd, u_char *dat, u_char * · line 222

Quoted 2 lines
drivers/mtd/nand/davinci_nand.c +2 -0
+	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

Signature · 5 lines
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!