NAND: DaVinci: Adding 4 BIT ECC support
Subsystems (2)
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
@@ -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
@@ -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
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.
Signed-off-by Sandeep Paulraj <[email protected]>--- 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.
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
Signed-off-by Sandeep Paulraj <[email protected]>--- 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.
PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support
In reply to Jean-Christophe PLAGNIOL-VILLARDQuoted 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
Signed-off-by Sandeep Paulraj <[email protected]>--- 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
PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support
In reply to s-paulraj at ti.comQuoted 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
Paulraj
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, intmode)
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_eccis 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
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_hweccwhich 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
Paulraj
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 supportQuoted 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_hweccwhich 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
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]
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
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.
Paulraj
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
PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support
In reply to PaulrajDear "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
PATCH 1/1 NAND: DaVinci: Adding 4 BIT ECC support
In reply to Scott WoodDear 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_hweccwhich 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!