Flex-OneNAND support
Subsystems (3)
PATCH 1/3 Flex-OneNAND support
This is a repost of the Flex-OneNAND devices support. Changes since the last post:
- Fix bug which caused 2X program in OneNAND to fail.
- Fix bug with eraseregions containing odd number of blocks.
- Add routine to check blocks are erased before changing boundary.
--- drivers/mtd/onenand/Kconfig | 62 +++ drivers/mtd/onenand/onenand_base.c | 827 ++++++++++++++++++++++++++++++++---- drivers/mtd/onenand/onenand_bbt.c | 13 +- drivers/mtd/onenand/onenand_sim.c | 76 +++- include/linux/mtd/onenand.h | 41 ++ include/linux/mtd/onenand_regs.h | 20 +- 6 files changed, 950 insertions(+), 89 deletions(-)
drivers/mtd/onenand/Kconfig +62 -0
@@ -71,4 +71,66 @@ config MTD_ONENAND_SIM The simulator may simulate various OneNAND flash chips for the OneNAND MTD layer. +config MTD_FLEXONENAND_BOUNDARY + bool "Flex-OneNAND Boundary Configuration" + depends on MTD_ONENAND + default n + help + Set SLC and MLC regions of Flex-OneNAND + +config MTD_FLEXONENAND_DIE0_BOUNDARY + int "Last SLC Block of Flex-OneNAND (min = 0, max = 1023)" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY + default "-1" + help + Configure Partition Information (PI) of Flex-OneNAND + + Entered value indicates index of last SLC block on Flex-OneNAND. + The remaining blocks are configured as MLC blocks. + + A value of -1 means that PI remains unchanged. + + This setting applies to : + - SDP Flex-OneNAND + - Die 1 of DDP Flex-OneNAND. + +config MTD_FLEXONENAND_DIE0_ISLOCKED + bool "Lock Boundary of Flex-OneNAND" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY + default n + help + Configure if Flex-OneNAND boundary should be locked. + Once locked, the boundary cannot be changed. + + This setting applies to : + - SDP Flex-OneNAND + - Die 1 of DDP Flex-OneNAND + +config MTD_FLEXONENAND_DDP_BOUNDARY + bool "Flex-OneNAND DDP Boundary Configuration" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY + default n + help + Set SLC and MLC regions of Die 2 of Flex-OneNAND DDP + +config MTD_FLEXONENAND_DIE1_BOUNDARY + int "Last SLC Block of Flex-OneNAND Die 2 (min = 0, max = 1023)" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY && MTD_FLEXONENAND_DDP_BOUNDARY + default "-1" + help + Configure Partition Information (PI) for Die 2 of DDP Flex-OneNAND. + + Entered value indicates index of last SLC block on Flex-OneNAND. + The remaining blocks are configured as MLC blocks. + + A value of -1 means that PI remains unchanged. + +config MTD_FLEXONENAND_DIE1_ISLOCKED + bool "Lock Boundary of Flex-OneNAND Die 2" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY && MTD_FLEXONENAND_DDP_BOUNDARY + default n + help + Configure if boundary for Die 2 of DDP Flex-OneNAND should be locked. + Once locked, the boundary cannot be changed. + endif # MTD_ONENAND
drivers/mtd/onenand/onenand_base.c +750 -77
1,228 lines not on this page · raw message
drivers/mtd/onenand/onenand_bbt.c +9 -4
@@ -60,6 +60,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descrstruct bbm_info *bbm = this->bbm;int i, j, numblocks, len, scanlen;int startblock; +unsigned slc =0 ;loff_t from;size_t readlen, ooblen;struct mtd_oob_ops ops;
@@ -76,7 +77,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr/* Note that numblocks is 2 * (real numblocks) here; * see i += 2 below as it makses shifting and masking less painful */ - numblocks = mtd->size >> (bbm->bbt_erase_shift -1 ); + numblocks = this->chipsize >> (bbm->bbt_erase_shift -1 ); startblock =0 ; from =0 ;
@@ -106,7 +107,11 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr } } i +=2 ; - from += (1 << bbm->bbt_erase_shift); +onenand_get_block (this, from, &slc); +if (slc) + from += (1 << bbm->bbt_erase_shift) >>1 ; +else + from += (1 << bbm->bbt_erase_shift); }return 0 ;
@@ -143,7 +148,7 @@ static int onenand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt)uint8_t res;/* Get block number * 2 */ - block = (int ) (offs >> (bbm->bbt_erase_shift -1 )); + block = (int ) (onenand_get_block (this, offs,NULL ) <<1 ); res = (bbm->bbt[block >>3 ] >> (block &0x06 )) &0x03 ; DEBUG(MTD_DEBUG_LEVEL2, "onenand_isbad_bbt: bbt info for offs 0x%08x: (block %d) 0x%02x\n",
@@ -178,7 +183,7 @@ int onenand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd)struct bbm_info *bbm = this->bbm;int len, ret =0 ; - len = mtd->size >> (this->erase_shift +2 ); + len = this->chipsize >> (this->erase_shift +2 );/* Allocate memory (2bit per block) and clear the memory bad block table */ bbm->bbt =kzalloc (len,GFP_KERNEL );if (!bbm->bbt) {
drivers/mtd/onenand/onenand_sim.c +70 -6
166 lines not on this page · raw message
include/linux/mtd/onenand.h +41 -0
82 lines not on this page · raw message
include/linux/mtd/onenand_regs.h +18 -2
@@ -67,6 +67,9 @@/* * Device ID Register F001h (R) */ +#define DEVICE_IS_FLEXONENAND (1 << 9) +#define FLEXONENAND_PI_MASK (0x3ff) +#define FLEXONENAND_PI_UNLOCK_SHIFT (14)#define ONENAND_DEVICE_DENSITY_MASK (0xf)#define ONENAND_DEVICE_DENSITY_SHIFT (4)#define ONENAND_DEVICE_IS_DDP (1 << 3)
@@ -84,6 +87,11 @@#define ONENAND_VERSION_PROCESS_SHIFT (8)/* +* Technology Register F006h (R) +*/ +#define ONENAND_TECHNOLOGY_IS_MLC (1 << 0) + +/* * Start Address 1 F100h (R/W) & Start Address 2 F101h (R/W) */ #define ONENAND_DDP_SHIFT (15)
@@ -93,7 +101,8 @@/* * Start Address 8 F107h (R/W) */ -#define ONENAND_FPA_MASK (0x3f) +/* Note: It's actually 0x3f in case of SLC */ +#define ONENAND_FPA_MASK (0x7f)#define ONENAND_FPA_SHIFT (2)#define ONENAND_FSA_MASK (0x03)
@@ -105,7 +114,8 @@#define ONENAND_BSA_BOOTRAM (0 << 2)#define ONENAND_BSA_DATARAM0 (2 << 2)#define ONENAND_BSA_DATARAM1 (3 << 2) -#define ONENAND_BSC_MASK (0x03) +/* Note: It's actually 0x03 in case of SLC */ +#define ONENAND_BSC_MASK (0x07)/* * Command Register F220h (R/W)
@@ -124,9 +134,13 @@#define ONENAND_CMD_RESET (0xF0)#define ONENAND_CMD_OTP_ACCESS (0x65)#define ONENAND_CMD_READID (0x90) +#define FLEXONENAND_CMD_PI_UPDATE (0x05) +#define FLEXONENAND_CMD_PI_ACCESS (0x66) +#define FLEXONENAND_CMD_RECOVER_LSB (0x05)/* NOTE: Those are not *REAL* commands */ #define ONENAND_CMD_BUFFERRAM (0x1978) +#define FLEXONENAND_CMD_READ_PI (0x1985)/* * System Configuration 1 Register F221h (R, R/W)
@@ -192,10 +206,12 @@#define ONENAND_ECC_1BIT_ALL (0x5555)#define ONENAND_ECC_2BIT (1 << 1)#define ONENAND_ECC_2BIT_ALL (0xAAAA) +#define FLEXONENAND_UNCORRECTABLE_ERROR (0x1010)/* * One-Time Programmable (OTP) */ +#define FLEXONENAND_OTP_LOCK_OFFSET (2048)#define ONENAND_OTP_LOCK_OFFSET (14)#endif /* __ONENAND_REG_H */
Signature · 1 line
1.5.3.3
Quoted 5 lines · On Tue, 03 Mar 2009 15:36:05 +0900 Rohit Hagargundgi <[email protected]>…
This is a repost of the Flex-OneNAND devices support. Changes since the last post:
- Fix bug which caused 2X program in OneNAND to fail.
- Fix bug with eraseregions containing odd number of blocks.
- Add routine to check blocks are erased before changing boundary.
"changes since last post" is not interesting information in the mainline git commit, so I habitually delete it.
After that, we end up without any useful changelog at all. Is that really optimal?
Quoted 68 lines
--- drivers/mtd/onenand/Kconfig | 62 +++ drivers/mtd/onenand/onenand_base.c | 827 ++++++++++++++++++++++++++++++++---- drivers/mtd/onenand/onenand_bbt.c | 13 +- drivers/mtd/onenand/onenand_sim.c | 76 +++- include/linux/mtd/onenand.h | 41 ++ include/linux/mtd/onenand_regs.h | 20 +- 6 files changed, 950 insertions(+), 89 deletions(-)drivers/mtd/onenand/Kconfig +51
-0@@ -71,4 +71,66 @@ config MTD_ONENAND_SIM The simulator may simulate various OneNAND flash chips for the OneNAND MTD layer. +config MTD_FLEXONENAND_BOUNDARY + bool "Flex-OneNAND Boundary Configuration" + depends on MTD_ONENAND + default n + help + Set SLC and MLC regions of Flex-OneNAND + +config MTD_FLEXONENAND_DIE0_BOUNDARY + int "Last SLC Block of Flex-OneNAND (min = 0, max = 1023)" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY + default "-1" + help + Configure Partition Information (PI) of Flex-OneNAND + + Entered value indicates index of last SLC block on Flex-OneNAND. + The remaining blocks are configured as MLC blocks. + + A value of -1 means that PI remains unchanged. + + This setting applies to : + - SDP Flex-OneNAND + - Die 1 of DDP Flex-OneNAND. + +config MTD_FLEXONENAND_DIE0_ISLOCKED + bool "Lock Boundary of Flex-OneNAND" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY + default n + help + Configure if Flex-OneNAND boundary should be locked. + Once locked, the boundary cannot be changed. + + This setting applies to : + - SDP Flex-OneNAND + - Die 1 of DDP Flex-OneNAND + +config MTD_FLEXONENAND_DDP_BOUNDARY + bool "Flex-OneNAND DDP Boundary Configuration" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY + default n + help + Set SLC and MLC regions of Die 2 of Flex-OneNAND DDP + +config MTD_FLEXONENAND_DIE1_BOUNDARY + int "Last SLC Block of Flex-OneNAND Die 2 (min = 0, max = 1023)" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY && MTD_FLEXONENAND_DDP_BOUNDARY + default "-1" + help + Configure Partition Information (PI) for Die 2 of DDP Flex-OneNAND. + + Entered value indicates index of last SLC block on Flex-OneNAND. + The remaining blocks are configured as MLC blocks.
drivers/mtd/onenand/Kconfig config MTD_ONENAND_SIM · line 134
Quoted 10 lines
drivers/mtd/onenand/Kconfig +10
-0+ + A value of -1 means that PI remains unchanged. + +config MTD_FLEXONENAND_DIE1_ISLOCKED + bool "Lock Boundary of Flex-OneNAND Die 2" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY && MTD_FLEXONENAND_DDP_BOUNDARY + default n + help + Configure if boundary for Die 2 of DDP Flex-OneNAND should be locked. + Once locked, the boundary cannot be changed.
This looks quite user-unfriendly to me. People will need to rebuild their kernel (or request a new one from their provider) just to alter some obscure MTD option.
It is about 100000000000 times better for people to be able to compile or distribute a single kernel image, and for the users of those kernels to be able to select options at boot-time or at runtime.
Can that be done here?
Quoted 8 lines
...
drivers/mtd/onenand/Kconfig +5
-0+static loff_t flexonenand_get_addr(struct onenand_chip *this, int block) +{ + loff_t ofs = 0; + int die = 0, boundary; +
drivers/mtd/onenand/Kconfig
Quoted 10 lines
drivers/mtd/onenand/Kconfig +10
-0+ if (ONENAND_IS_DDP(this) && block >= this->density_mask) { + block -= this->density_mask; + die = 1; + ofs = this->diesize[0]; + } + + boundary = this->boundary[die]; + ofs += block << (this->erase_shift - 1); + if (block > (boundary + 1)) + ofs += (block - boundary - 1) << (this->erase_shift - 1);
Both block' and boundary' have 32-bit types. Are you sure that the left-shift cannot overflow?
drivers/mtd/onenand/Kconfig
Quoted 7 lines
drivers/mtd/onenand/Kconfig +7
-0+ return ofs; +} + +inline loff_t onenand_get_addr(struct onenand_chip *this, int block) +{ + if (!FLEXONENAND(this)) + return block << this->erase_shift;
Ditto.
Quoted 15 lines
drivers/mtd/onenand/Kconfig +12
-0+ return flexonenand_get_addr(this, block); +} + +static inline int onenand_read_ecc(struct onenand_chip *this) +{ + int ecc, i, result = 0; + + if (!FLEXONENAND(this)) + return this->read_word(this->base + ONENAND_REG_ECC_STATUS); + + for (i = 0; i < 4; i++) { + ecc = this->read_word(this->base + ONENAND_REG_ECC_STATUS + i);
drivers/mtd/onenand/Kconfig
Quoted 10 lines
drivers/mtd/onenand/Kconfig +10
-0+ if (likely(!ecc)) + continue; + if (ecc & FLEXONENAND_UNCORRECTABLE_ERROR) + return ONENAND_ECC_2BIT_ALL; + else + result = ONENAND_ECC_1BIT_ALL; + } + + return result; +}
This patch does too much manual inlining. THis function in particular (which has two callsites) is waaaaaaaay too large to be inlined.
Quoted 30 lines
...
drivers/mtd/onenand/Kconfig +27
-0+static int onenand_mlc_read_ops_nolock(struct mtd_info *mtd, loff_t from, + struct mtd_oob_ops *ops) +{ + struct onenand_chip *this = mtd->priv; + struct mtd_ecc_stats stats; + size_t len = ops->len; + size_t ooblen = ops->ooblen; + u_char *buf = ops->datbuf; + u_char *oobbuf = ops->oobbuf; + int read = 0, column, thislen; + int oobread = 0, oobcolumn, thisooblen, oobsize; + int ret = 0; + int writesize = this->writesize; + + DEBUG(MTD_DEBUG_LEVEL3, "onenand_mlc_read_ops_nolock: from = 0x%08x, len = %i\n", (unsigned int) from, (int) len); + + if (ops->mode == MTD_OOB_AUTO) + oobsize = this->ecclayout->oobavail; + else + oobsize = mtd->oobsize; + + oobcolumn = from & (mtd->oobsize - 1); + + /* Do not allow reads past end of device */ + if (from + len > mtd->size) { + printk(KERN_ERR "onenand_mlc_read_ops_nolock: Attempt read beyond end of device\n"); + ops->retlen = 0;
drivers/mtd/onenand/Kconfig
Quoted 10 lines
drivers/mtd/onenand/Kconfig +10
-0+ ops->oobretlen = 0; + return -EINVAL; + } + + stats = mtd->ecc_stats; + + while (read < len) { + cond_resched(); + + thislen = min_t(int, writesize, len - read);
Use of min_t is often a sign that the types are wrong.
len' and read' should have type size_t. Probably other things should be size_t as well.
Quoted 43 lines
drivers/mtd/onenand/Kconfig +43
-0+ column = from & (writesize - 1); + if (column + thislen > writesize) + thislen = writesize - column; + + if (!onenand_check_bufferram(mtd, from)) { + this->command(mtd, ONENAND_CMD_READ, from, writesize); + + ret = this->wait(mtd, FL_READING); + if (unlikely(ret)) + ret = onenand_recover_lsb(mtd, from, ret); + onenand_update_bufferram(mtd, from, !ret); + if (ret == -EBADMSG) + ret = 0; + } + + this->read_bufferram(mtd, ONENAND_DATARAM, buf, column, thislen); + if (oobbuf) { + thisooblen = oobsize - oobcolumn; + thisooblen = min_t(int, thisooblen, ooblen - oobread); + + if (ops->mode == MTD_OOB_AUTO) + onenand_transfer_auto_oob(mtd, oobbuf, oobcolumn, thisooblen); + else + this->read_bufferram(mtd, ONENAND_SPARERAM, oobbuf, oobcolumn, thisooblen); + oobread += thisooblen; + oobbuf += thisooblen; + oobcolumn = 0; + } + + read += thislen; + if (read == len) + break; + + from += thislen; + buf += thislen; + } + + /* + * Return success, if no ECC failures, else -EBADMSG + * fs driver will take care of that, because + * retlen == desired len and result == -EBADMSG + */ + ops->retlen = read;
drivers/mtd/onenand/Kconfig
Quoted 10 lines
drivers/mtd/onenand/Kconfig +10
-0+ ops->oobretlen = oobread; + + if (ret) + return ret; + + if (mtd->ecc_stats.failed - stats.failed) + return -EBADMSG; + + return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0; +}
I wonder what the heck EUCLEAN was invented for and whether MTD's extensive use of it is appropriate.
include/linux/mtd/onenand.h line 23
Quoted 13 lines
...
include/linux/mtd/onenand.h +2
-0@@ -17,8 +17,32 @@#include <linux/mtd/onenand_regs.h> #include <linux/mtd/bbm.h> +#define MAX_DIES 2#define MAX_BUFFERRAM 2 +#if definedCONFIG_MTD_FLEXONENAND_DIE0_BOUNDARY
Plain old `#ifdef' is more common.
Quoted 17 lines
include/linux/mtd/onenand.h +17
-0+#define FLEXONENAND_DIE0_BOUNDARY CONFIG_MTD_FLEXONENAND_DIE0_BOUNDARY +#else +#define FLEXONENAND_DIE0_BOUNDARY -1 +#endif + +#if definedCONFIG_MTD_FLEXONENAND_DIE1_BOUNDARY +#define FLEXONENAND_DIE1_BOUNDARY CONFIG_MTD_FLEXONENAND_DIE1_BOUNDARY +#else +#define FLEXONENAND_DIE1_BOUNDARY -1 +#endif + +#if definedCONFIG_MTD_FLEXONENAND_DIE0_ISLOCKED +#define FLEXONENAND_DIE0_ISLOCKED 1 +#else +#define FLEXONENAND_DIE0_ISLOCKED 0 +#endif +#if definedCONFIG_MTD_FLEXONENAND_DIE1_ISLOCKED
include/linux/mtd/onenand.h
Quoted 10 lines
include/linux/mtd/onenand.h +7
-0+#define FLEXONENAND_DIE1_ISLOCKED 1 +#else +#define FLEXONENAND_DIE1_ISLOCKED 0 +#endif + +#define ONENAND_IS_MLC (this) \ + (this->technology & ONENAND_TECHNOLOGY_IS_MLC)
hm, I wonder why all these things were implemented as macros.
Quoted 3 lines
...
Quoted 12 lines · Andrew Morton
Quoted 12 lines
...
drivers/mtd/onenand/onenand_base.c +10
-0+static loff_t flexonenand_get_addr (struct onenand_chip *this,int block) +{ +loff_t ofs =0 ; +int die =0 , boundary; + +if (ONENAND_IS_DDP (this) && block >= this->density_mask) { + block -= this->density_mask; + die =1 ; + ofs = this->diesize[0 ]; + }
Quoted 8 lines
drivers/mtd/onenand/onenand_base.c static int onenand_buffer_address(int dataram1, int sectors, int count) · line 264
Quoted 5 lines
drivers/mtd/onenand/onenand_base.c +5
-0+ + boundary = this->boundary[die]; + ofs += block << (this->erase_shift -1 ); +if (block > (boundary +1 )) + ofs += (block - boundary -1 ) << (this->erase_shift -1 );Both
block' andboundary' have 32-bit types. Are you sure that the left-shift cannot overflow?
Only very recently has MTD supported sizes greater than 32 bits internally for any type of flash. The external APIs (ioctls) are still 32-bit based.
For this driver, supporting sizes over 32-bits is a separate issue - and may never be needed.
Quoted 5 lines
drivers/mtd/onenand/onenand_base.c static int onenand_transfer_auto_oob(struct mtd_info *mtd, uint8_t *buf, int col · line 1121
Quoted 2 lines
drivers/mtd/onenand/onenand_base.c +2
-0+return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN :0 ; +}I wonder what the heck EUCLEAN was invented for and whether MTD's extensive use of it is appropriate.
UBI uses it to detect bit-flips so that data can be moved before it can no longer be read. So it is pretty much essential for flash memories.
Quoted 22 lines · On Wed, 04 Mar 2009 13:41:59 +0200 Adrian Hunter <[email protected]>…
Quoted 12 lines · Andrew Morton
Quoted 12 lines
...
drivers/mtd/onenand/onenand_base.c +10
-0+static loff_t flexonenand_get_addr (struct onenand_chip *this,int block) +{ +loff_t ofs =0 ; +int die =0 , boundary; + +if (ONENAND_IS_DDP (this) && block >= this->density_mask) { + block -= this->density_mask; + die =1 ; + ofs = this->diesize[0 ]; + }Quoted 8 lines
drivers/mtd/onenand/onenand_base.c static int onenand_buffer_address(int dataram1, int sectors, int count) · line 264
Quoted 5 lines
drivers/mtd/onenand/onenand_base.c +5
-0+ + boundary = this->boundary[die]; + ofs += block << (this->erase_shift -1 ); +if (block > (boundary +1 )) + ofs += (block - boundary -1 ) << (this->erase_shift -1 );Both
block' andboundary' have 32-bit types. Are you sure that the left-shift cannot overflow?
Quoted 5 lines
Only very recently has MTD supported sizes greater than 32 bits internally for any type of flash. The external APIs (ioctls) are still 32-bit based.
For this driver, supporting sizes over 32-bits is a separate issue - and may never be needed.
So it doesn't support files >4G? What's the max device size (now and projected)?
Quoted 9 lines
Quoted 5 lines
drivers/mtd/onenand/onenand_base.c static int onenand_transfer_auto_oob(struct mtd_info *mtd, uint8_t *buf, int col · line 1121
Quoted 2 lines
drivers/mtd/onenand/onenand_base.c +2
-0+return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN :0 ; +}I wonder what the heck EUCLEAN was invented for and whether MTD's extensive use of it is appropriate.
UBI uses it to detect bit-flips so that data can be moved before it can no longer be read. So it is pretty much essential for flash memories.
That's not the point.
My point is: for what purpose was EUNCLEAN created by whoever created it and, given that, is its use by MTD appropriate? Because it does appear that this gets returned all the way to userspace sometimes.
Hi,
Quoted 24 lines · Andrew Morton
Quoted 22 lines · On Wed, 04 Mar 2009 13:41:59 +0200 Adrian Hunter <[email protected]>…
Quoted 20 lines · Andrew Morton
Quoted 7 lines
...
drivers/mtd/onenand/onenand_base.c +5
-0+static loff_t flexonenand_get_addr (struct onenand_chip *this,int block) +{ + loff_t ofs = 0; + int die = 0, boundary; +drivers/mtd/onenand/onenand_base.c static int onenand_buffer_address(int dataram1, int sectors, int count) · line 264
Quoted 10 lines
drivers/mtd/onenand/onenand_base.c +10
-0+ if (ONENAND_IS_DDP(this) && block >= this->density_mask) { + block -= this->density_mask; + die = 1; + ofs = this->diesize[0]; + } + + boundary = this->boundary[die]; + ofs += block << (this->erase_shift - 1); + if (block > (boundary + 1)) + ofs += (block - boundary - 1) << (this->erase_shift - 1);Both
block' andboundary' have 32-bit types. Are you sure that the left-shift cannot overflow?
Quoted 8 lines
Quoted 5 lines
Only very recently has MTD supported sizes greater than 32 bits internally for any type of flash. The external APIs (ioctls) are still 32-bit based.
For this driver, supporting sizes over 32-bits is a separate issue - and may never be needed.
So it doesn't support files >4G? What's the max device size (now and projected)?
Umm it's not simple question. basically Flex-OneNAND based on NAND flash technology. It means if NAND flash technology are advanced, it can also get more size. Currently max Flex-OneNAND has 16 Gib (2GiB) So it will be exceed the 4GiB soon. but I'm not sure when this device is opened. I think we know the issues currently and then if the device is opened, handle this one if required.
How do you think?
Thank you,
Kyungmin Park
Quoted 3 lines · On Thu, 5 Mar 2009 08:47:03 +0900 Kyungmin Park <[email protected]> wrote:
I think we know the issues currently and then if the device is opened, handle this one if required.
How do you think?
I think it would be prudent to fix the known problems now, rather than having to go through and re-debug them later on.
Bear in mind that people frequently copy old code when writing new code, so these things tend to propagate, creating more work later on.
Hi,
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Thu, 5 Mar 2009 08:47:03 +0900 Kyungmin Park <[email protected]> wrote:
I think we know the issues currently and then if the device is opened, handle this one if required.
How do you think?
Quoted 7 lines
I think it would be prudent to fix the known problems now, rather than having to go through and re-debug them later on.
Bear in mind that people frequently copy old code when writing new code, so these things tend to propagate, creating more work later on.
Agree,
To Amit,
Could you fix it and send a patch please.
Also can you reduce the left shift operation to resolve it permanently
Thank you,
Kyungmin Park
Hi Kyungmin,
----- Original Message -----
From: "Kyungmin Park" <[email protected]>
To: "Andrew Morton" <[email protected]>; "AMIT KUMARSHARMA" <[email protected]>
Cc: <[email protected]>; <[email protected]>; <[email protected]>; <[email protected]>
Sent: Thursday, March 05, 2009 9:13 AM
Subject: Re: [PATCH 1/3] [MTD] Flex-OneNAND support
Quoted 23 lines
Hi,
Quoted 12 lines · Andrew Morton
Quoted 4 lines · On Thu, 5 Mar 2009 08:47:03 +0900 Kyungmin Park <[email protected]> wrote:
I think we know the issues currently and then if the device is opened,
handle this one if required.
How do you think?I think it would be prudent to fix the known problems now, rather
than having to go through and re-debug them later on.Quoted 6 lines
Bear in mind that people frequently copy old code when writing
new code, so these things tend to propagate, creating more work
later on.
Quoted 7 lines
Agree,
To Amit,
Could you fix it and send a patch please.
Also can you reduce the left shift operation to resolve it permanently
sure i will send it ASAP.
Quoted 3 lines
Thank you,
Kyungmin Park
Quoted 8 lines · Andrew Morton
Quoted 7 lines
Quoted 4 lines
drivers/mtd/onenand/onenand_base.c static int onenand_transfer_auto_oob(struct mtd_info *mtd, uint8_t *buf, int col · line 1121
Quoted 2 lines
drivers/mtd/onenand/onenand_base.c +2
-0+return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN :0 ; +}I wonder what the heck EUCLEAN was invented for and whether MTD's extensive use of it is appropriate.
UBI uses it to detect bit-flips so that data can be moved before it can no longer be read. So it is pretty much essential for flash memories.
Quoted 5 lines
That's not the point.
My point is: for what purpose was EUNCLEAN created by whoever created it and, given that, is its use by MTD appropriate? Because it does appear that this gets returned all the way to userspace sometimes.
I have had a look at the use of EUCLEAN and found the following:
1. It does not seem to be defined in POSIX
2. It may have been used by unix variants to indicate an
attempt to mount a file system whose superblock was not marked as clean.
3. In Linux, it is used almost exclusively by MTD, with
one exception in smbfs, from which it is tempting to conclude that smbfs (at least) is using it incorrectly. There is one web page
http://www.wlug.org.nz/EUCLEAN where the author suggests ENOMEDIUM might be a better choice.
Can you point me to where EUCLEAN is returned to userspace? I took a quick look and couldn't see it.
Quoted 15 lines · Adrian Hunter
Quoted 3 lines
My point is: for what purpose was EUNCLEAN created by whoever created it and, given that, is its use by MTD appropriate? Because it does appear that this gets returned all the way to userspace sometimes.
I have had a look at the use of EUCLEAN and found the following:
1. It does not seem to be defined in POSIX
2. It may have been used by unix variants to indicate an
attempt to mount a file system whose superblock was not marked as clean.
3. In Linux, it is used almost exclusively by MTD, with
one exception in smbfs, from which it is tempting to conclude that smbfs (at least) is using it incorrectly. There is one web page
http://www.wlug.org.nz/EUCLEAN where the author suggests ENOMEDIUM might be a better choice.
Quoted 2 lines
Can you point me to where EUCLEAN is returned to userspace? I took a quick look and couldn't see it.
But for us, EUCLEAN looks like a good name :-) Probably this is why tglx choose this error code.
Signature · 2 lines
Best regards, Artem Bityutskiy (Битюцкий Артём)
Quoted 1 line · On Thu, 05 Mar 2009 15:38:50 +0200 Adrian Hunter <[email protected]>…
Can you point me to where EUCLEAN is returned to userspace?
err, nope. I was googling "euclean" and came across a report where someone was wondering why he got this errno. I can't find it now.
Hi,
Quoted 9 lines · Andrew Morton
Quoted 5 lines · On Tue, 03 Mar 2009 15:36:05 +0900 Rohit Hagargundgi <[email protected]>…
This is a repost of the Flex-OneNAND devices support. Changes since the last post:
- Fix bug which caused 2X program in OneNAND to fail.
- Fix bug with eraseregions containing odd number of blocks.
- Add routine to check blocks are erased before changing boundary.
Quoted 5 lines
"changes since last post" is not interesting information in the mainline git commit, so I habitually delete it.
After that, we end up without any useful changelog at all. Is that really optimal?
okay. I will change it.
Quoted 75 lines
Quoted 75 lines
--- drivers/mtd/onenand/Kconfig | 62 +++ drivers/mtd/onenand/onenand_base.c | 827 ++++++++++++++++++++++++++++++++---- drivers/mtd/onenand/onenand_bbt.c | 13 +- drivers/mtd/onenand/onenand_sim.c | 76 +++- include/linux/mtd/onenand.h | 41 ++ include/linux/mtd/onenand_regs.h | 20 +- 6 files changed, 950 insertions(+), 89 deletions(-)drivers/mtd/onenand/Kconfig +57
-0@@ -71,4 +71,66 @@ config MTD_ONENAND_SIM The simulator may simulate various OneNAND flash chips for the OneNAND MTD layer. +config MTD_FLEXONENAND_BOUNDARY + bool "Flex-OneNAND Boundary Configuration" + depends on MTD_ONENAND + default n + help + Set SLC and MLC regions of Flex-OneNAND + +config MTD_FLEXONENAND_DIE0_BOUNDARY + int "Last SLC Block of Flex-OneNAND (min = 0, max = 1023)" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY + default "-1" + help + Configure Partition Information (PI) of Flex-OneNAND + + Entered value indicates index of last SLC block on Flex-OneNAND. + The remaining blocks are configured as MLC blocks. + + A value of -1 means that PI remains unchanged. + + This setting applies to : + - SDP Flex-OneNAND + - Die 1 of DDP Flex-OneNAND. + +config MTD_FLEXONENAND_DIE0_ISLOCKED + bool "Lock Boundary of Flex-OneNAND" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY + default n + help + Configure if Flex-OneNAND boundary should be locked. + Once locked, the boundary cannot be changed. + + This setting applies to : + - SDP Flex-OneNAND + - Die 1 of DDP Flex-OneNAND + +config MTD_FLEXONENAND_DDP_BOUNDARY + bool "Flex-OneNAND DDP Boundary Configuration" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY + default n + help + Set SLC and MLC regions of Die 2 of Flex-OneNAND DDP + +config MTD_FLEXONENAND_DIE1_BOUNDARY + int "Last SLC Block of Flex-OneNAND Die 2 (min = 0, max = 1023)" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY && MTD_FLEXONENAND_DDP_BOUNDARY + default "-1" + help + Configure Partition Information (PI) for Die 2 of DDP Flex-OneNAND. + + Entered value indicates index of last SLC block on Flex-OneNAND. + The remaining blocks are configured as MLC blocks. + + A value of -1 means that PI remains unchanged. + +config MTD_FLEXONENAND_DIE1_ISLOCKED + bool "Lock Boundary of Flex-OneNAND Die 2" + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY && MTD_FLEXONENAND_DDP_BOUNDARY
Quoted 8 lines
drivers/mtd/onenand/Kconfig config MTD_ONENAND_SIM · line 134
Quoted 4 lines
drivers/mtd/onenand/Kconfig +4
-0+ default n + help + Configure if boundary for Die 2 of DDP Flex-OneNAND should be locked. + Once locked, the boundary cannot be changed.This looks quite user-unfriendly to me. People will need to rebuild their kernel (or request a new one from their provider) just to alter some obscure MTD option.
This option decides how much of the device becomes SLC NAND (faster,reliable - suitable for storing code) and how much becomes MLC NAND (slower, less reliable - suitable for storing user data).
Quoted 6 lines
It is about 100000000000 times better for people to be able to compile or distribute a single kernel image, and for the users of those kernels to be able to select options at boot-time or at runtime.
Can that be done here?
Runtime change is not needed and also damages user partitions. Boot-time change through kernel parameter sounds good. I will change it.
Quoted 13 lines
Quoted 13 lines
...
drivers/mtd/onenand/Kconfig +10
-0+static loff_t flexonenand_get_addr(struct onenand_chip *this, int block) +{ + loff_t ofs = 0; + int die = 0, boundary; + + if (ONENAND_IS_DDP(this) && block >= this->density_mask) { + block -= this->density_mask; + die = 1; + ofs = this->diesize[0]; + }
Quoted 8 lines
drivers/mtd/onenand/Kconfig
Quoted 5 lines
drivers/mtd/onenand/Kconfig +5
-0+ + boundary = this->boundary[die]; + ofs += block << (this->erase_shift - 1); + if (block > (boundary + 1)) + ofs += (block - boundary - 1) << (this->erase_shift - 1);Both
block' andboundary' have 32-bit types. Are you sure that the left-shift cannot overflow?
okay. fixed.
Quoted 10 lines
drivers/mtd/onenand/Kconfig
Quoted 8 lines
drivers/mtd/onenand/Kconfig +7
-0+ return ofs; +} + +inline loff_t onenand_get_addr(struct onenand_chip *this, int block) +{ + if (!FLEXONENAND(this)) + return block << this->erase_shift;Ditto.
okay. fixed.
Quoted 21 lines
Quoted 21 lines
drivers/mtd/onenand/Kconfig +17
-0+ return flexonenand_get_addr(this, block); +} + +static inline int onenand_read_ecc(struct onenand_chip *this) +{ + int ecc, i, result = 0; + + if (!FLEXONENAND(this)) + return this->read_word(this->base + ONENAND_REG_ECC_STATUS); + + for (i = 0; i < 4; i++) { + ecc = this->read_word(this->base + ONENAND_REG_ECC_STATUS + i); + if (likely(!ecc)) + continue; + if (ecc & FLEXONENAND_UNCORRECTABLE_ERROR) + return ONENAND_ECC_2BIT_ALL; + else
Quoted 8 lines
drivers/mtd/onenand/Kconfig
Quoted 5 lines
drivers/mtd/onenand/Kconfig +5
-0+ result = ONENAND_ECC_1BIT_ALL; + } + + return result; +}This patch does too much manual inlining. THis function in particular (which has two callsites) is waaaaaaaay too large to be inlined.
it is to avoid the function call overhead
as ecc gets checked for every read operation.
Quoted 43 lines
Quoted 30 lines
...
drivers/mtd/onenand/Kconfig +27
-0+static int onenand_mlc_read_ops_nolock(struct mtd_info *mtd, loff_t from, + struct mtd_oob_ops *ops) +{ + struct onenand_chip *this = mtd->priv; + struct mtd_ecc_stats stats; + size_t len = ops->len; + size_t ooblen = ops->ooblen; + u_char *buf = ops->datbuf; + u_char *oobbuf = ops->oobbuf; + int read = 0, column, thislen; + int oobread = 0, oobcolumn, thisooblen, oobsize; + int ret = 0; + int writesize = this->writesize; + + DEBUG(MTD_DEBUG_LEVEL3, "onenand_mlc_read_ops_nolock: from = 0x%08x, len = %i\n", (unsigned int) from, (int) len); + + if (ops->mode == MTD_OOB_AUTO) + oobsize = this->ecclayout->oobavail; + else + oobsize = mtd->oobsize; + + oobcolumn = from & (mtd->oobsize - 1); + + /* Do not allow reads past end of device */ + if (from + len > mtd->size) { + printk(KERN_ERR "onenand_mlc_read_ops_nolock: Attempt read beyond end of device\n"); + ops->retlen = 0;drivers/mtd/onenand/Kconfig
Quoted 10 lines
drivers/mtd/onenand/Kconfig +10
-0+ ops->oobretlen = 0; + return -EINVAL; + } + + stats = mtd->ecc_stats; + + while (read < len) { + cond_resched(); + + thislen = min_t(int, writesize, len - read);Use of
min_tis often a sign that the types are wrong.
Quoted 2 lines
len' andread' should have typesize_t. Probably other things should besize_tas well.
okay. a separate patch for this seems better.
Quoted 66 lines
Quoted 44 lines
drivers/mtd/onenand/Kconfig +43
-0+ column = from & (writesize - 1); + if (column + thislen > writesize) + thislen = writesize - column; + + if (!onenand_check_bufferram(mtd, from)) { + this->command(mtd, ONENAND_CMD_READ, from, writesize); + + ret = this->wait(mtd, FL_READING); + if (unlikely(ret)) + ret = onenand_recover_lsb(mtd, from, ret); + onenand_update_bufferram(mtd, from, !ret); + if (ret == -EBADMSG) + ret = 0; + } + + this->read_bufferram(mtd, ONENAND_DATARAM, buf, column, thislen); + if (oobbuf) { + thisooblen = oobsize - oobcolumn; + thisooblen = min_t(int, thisooblen, ooblen - oobread); + + if (ops->mode == MTD_OOB_AUTO) + onenand_transfer_auto_oob(mtd, oobbuf, oobcolumn, thisooblen); + else + this->read_bufferram(mtd, ONENAND_SPARERAM, oobbuf, oobcolumn, thisooblen); + oobread += thisooblen; + oobbuf += thisooblen; + oobcolumn = 0; + } + + read += thislen; + if (read == len) + break; + + from += thislen; + buf += thislen; + } + + /* + * Return success, if no ECC failures, else -EBADMSG + * fs driver will take care of that, because + * retlen == desired len and result == -EBADMSG + */ + ops->retlen = read;drivers/mtd/onenand/Kconfig
Quoted 10 lines
drivers/mtd/onenand/Kconfig +10
-0+ ops->oobretlen = oobread; + + if (ret) + return ret; + + if (mtd->ecc_stats.failed - stats.failed) + return -EBADMSG; + + return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0; +}I wonder what the heck EUCLEAN was invented for and whether MTD's extensive use of it is appropriate.
Quoted 8 lines
...
include/linux/mtd/onenand.h +0
-0@@ -17,8 +17,32 @@#include <linux/mtd/onenand_regs.h> #include <linux/mtd/bbm.h>
Quoted 6 lines
include/linux/mtd/onenand.h line 23
Quoted 4 lines
include/linux/mtd/onenand.h +2
-0+#define MAX_DIES 2#define MAX_BUFFERRAM 2 +#if definedCONFIG_MTD_FLEXONENAND_DIE0_BOUNDARY Plain old `#ifdef' is more common.
okay
Quoted 23 lines
Quoted 23 lines
include/linux/mtd/onenand.h +22
-0+#define FLEXONENAND_DIE0_BOUNDARY CONFIG_MTD_FLEXONENAND_DIE0_BOUNDARY +#else +#define FLEXONENAND_DIE0_BOUNDARY -1 +#endif + +#if definedCONFIG_MTD_FLEXONENAND_DIE1_BOUNDARY +#define FLEXONENAND_DIE1_BOUNDARY CONFIG_MTD_FLEXONENAND_DIE1_BOUNDARY +#else +#define FLEXONENAND_DIE1_BOUNDARY -1 +#endif + +#if definedCONFIG_MTD_FLEXONENAND_DIE0_ISLOCKED +#define FLEXONENAND_DIE0_ISLOCKED 1 +#else +#define FLEXONENAND_DIE0_ISLOCKED 0 +#endif +#if definedCONFIG_MTD_FLEXONENAND_DIE1_ISLOCKED +#define FLEXONENAND_DIE1_ISLOCKED 1 +#else +#define FLEXONENAND_DIE1_ISLOCKED 0 +#endif +
Quoted 9 lines
include/linux/mtd/onenand.h struct onenand_chip { · line 192
Quoted 4 lines
...
include/linux/mtd/onenand.h +2
-0+#define ONENAND_IS_MLC (this) \ + (this->technology & ONENAND_TECHNOLOGY_IS_MLC)hm, I wonder why all these things were implemented as macros.
Quoted 2 lines
...
Thanks,
Rohit