[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <9c9fda240809220011w49c0875q804f836550ff3476@mail.gmail.com>
Date: Mon, 22 Sep 2008 16:11:31 +0900
From: "Kyungmin Park" <kmpark@...radead.org>
To: moorthy.apg@...sung.com
Cc: linux-mtd@...ts.infradead.org,
"Kyungmin Park" <kyungmin.park@...sung.com>,
lkml <linux-kernel@...r.kernel.org>
Subject: Re: [ANNOUNCE] [PATCH] [MTD] Flex-OneNAND MTD Driver available.
Hi,
It's another fusion device intergrage SLC & MLC NAND into just one
chip. So I added cc LKML.
It looks good to me except some unclean variable & syntactic usages.
Please clarify it and give more descriptions.
Thank you,
Kyungmin Park
On Fri, Sep 19, 2008 at 9:31 PM, AYYANARPONNUSAMY GANGHEYAMOORTHY
<moorthy.apg@...sung.com> wrote:
> Hi All,
> We are happy to release Flex-OneNAND MTD to open source community.
>
> Brief description of Flex-OneNAND :
> Samsung Flex-OneNAND is a highly reliable embedded memory targeted
> for both consumer electronics, and next generation mobile phone market.
> With Samsung's accumulated NAND flash technologies over the last decade,
> Samsung designs an ideal single memory chip based on NAND architecture
> integrating SRAM buffers and logic interface. Samsung Flex-OneNAND takes
> both advantages from high-speed data read function of NOR flash and the
> advanced data storage function of NAND flash.
>
> Flex-OneNAND combines SLC and MLC technologies into a single device.
> SLC area provides increased reliability and speed, suitable for storing
> code and data, such as bootloader, kernel and root file system.
> MLC area provides high density and is best used for storing user data.
> Users can configure the size of SLC and MLC regions.
>
> Refer (http://www.samsung.com/global/business/semiconductor/products/fusionmemory/Products_FlexOneNAND.html)
>
> Existing MTD does not allow for erase regions with odd number
> of blocks(partitions turn read only). So it is better to set
> the die boundary to odd values. Die boundary setting can be
> done in include/mtd/onenand.h
>
> A partition, theoretically, can span across SLC and MLC regions.
> There is no apparent advantage with it however. Also, such a
> partition will have MLC erase size, with two SLC blocks erased at
> a time during operation. If the second block goes bad, we end up
> marking the first block as bad. So, again, it is not advisable
> to create partitions across erase regions.
>
> Apart from this patch, some more posts for Flex-OneNAND support are
> 1. JFFS2 [PATCH 1/1] [MTD] [JFFS2] MLC NAND support
> [PATCH 1/2] [MTD] [JFFS2] MLC NAND support
>
> 2. MTD-UTILS [PATCH] [MTD-UTILS] Add support for 4KB page flash devices
>
> 3. UBOOT [U-Boot] [PATCH] FlexOneNAND support in U-Boot
> (Posted in http://lists.denx.de/mailman/listinfo/u-boot)
>
> Signed-off-by: Vishak G <vishak.g@...sung.com>
> Signed-off-by: Rohit Hagargundgi <h.rohit@...sung.com>
> ---
> diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
> index 926cf3a..42c19cb 100644
> --- a/drivers/mtd/onenand/onenand_base.c
> +++ b/drivers/mtd/onenand/onenand_base.c
> @@ -9,6 +9,10 @@
> * auto-placement support, read-while load support, various fixes
> * Copyright (C) Nokia Corporation, 2007
> *
> + * Vishak G <vishak.g@...sung.com>, Rohit Hagargundgi <h.rohit@...sung.com>
> + * Flex-OneNAND support
> + * Copyright (C) Samsung Electronics, 2008
> + *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> * published by the Free Software Foundation.
> @@ -27,6 +31,37 @@
>
> #include <asm/io.h>
>
> +static int boundary[] = {
> + FLEXONENAND_DIE0_BOUNDARY,
> + FLEXONENAND_DIE1_BOUNDARY,
> +};
> +
> +static int lock[] = {
> + FLEXONENAND_DIE0_ISLOCKED,
> + FLEXONENAND_DIE1_ISLOCKED,
> +};
Is it really needed? and is it changed at runtime? If not please add 'const'.
> +
> +/**
> + * onenand_oob_128 - oob info for Flex-Onenand with 4KB page
> + */
> +static struct nand_ecclayout onenand_oob_128 = {
> + .eccbytes = 80,
> + .eccpos = {
> + 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
> + 22, 23, 24, 25, 26, 27, 28, 29, 30, 31,
> + 38, 39, 40, 41, 42, 43, 44, 45, 46, 47,
> + 54, 55, 56, 57, 58, 59, 60, 61, 62, 63,
> + 70, 71, 72, 73, 74, 75, 76, 77, 78, 79,
> + 86, 87, 88, 89, 90, 91, 92, 93, 94, 95,
> + 102, 103, 104, 105, 106, 107, 108, 109, 110, 111,
> + 118, 119, 120, 121, 122, 123, 124, 125, 126, 127
> + },
> + .oobfree = {
> + {2, 4}, {16, 6}, {32, 6}, {48, 6},
> + {64, 6}, {80, 6}, {96, 6}, {112, 6}
> + }
> +};
Even though we use 2nd Bad block Information (BI) we leave it. It uses
provided spare area. As you know, it's enough.
> if (page != -1) {
> /* Now we use page size operation */
> - int sectors = 4, count = 4;
> + int sectors = 0, count = 0;
> int dataram;
The '0' means all sectors.
>
> +/**
> + * flexonenand_unlock_all - [Flex-OneNAND Interface] unlock all blocks
> + * @param mtd MTD device structure
> + *
> + * Unlock all blocks
> + */
> +static int flexonenand_unlock_all(struct mtd_info *mtd)
> +{
> + struct onenand_chip *this = mtd->priv;
> + size_t len = mtd->erasesize;
> +
> + if (mtd->numeraseregions > 1)
> + len >>= 1;
> +
> + onenand_do_lock_cmd(mtd, 0, len, ONENAND_CMD_UNLOCK_ALL);
> + if (ONENAND_IS_DDP(this))
> + onenand_do_lock_cmd(mtd, this->diesize[0], len,
> + ONENAND_CMD_UNLOCK_ALL);
> + onenand_check_lock_status(this);
> + return 0;
> +}
Does it need to define unlock all for Flex-OneNAND?
I think it's same between OneNAND and Flex-OneNAND. Is it some chip
specific problem?
> +
> #ifdef CONFIG_MTD_ONENAND_OTP
>
> - ret = onenand_write_oob_nolock(mtd, from, &ops);
> + ret = FLEXONENAND(this) ?
> + onenand_write_ops_nolock(mtd, (mtd->writesize * 49), &ops)
> + : onenand_write_oob_nolock(mtd, from, &ops);
Please describe why we multiply the '49' as mentioned below.
> @@ -2428,25 +2680,32 @@
> size_t len)
> {
> struct onenand_chip *this = mtd->priv;
> - u_char *oob_buf = this->oob_buf;
> + u_char *oob_buf = FLEXONENAND(this) ? this->page_buf : this->oob_buf;
How about the define 'buf' instead of 'oob_buf'.
> size_t retlen;
> int ret;
>
> - memset(oob_buf, 0xff, mtd->oobsize);
> + memset(oob_buf, 0xff, FLEXONENAND(this) ? this->writesize
> + : mtd->oobsize);
> /*
> * Note: OTP lock operation
> * OTP block : 0xXXFC
> * 1st block : 0xXXF3 (If chip support)
> * Both : 0xXXF0 (If chip support)
> */
> - oob_buf[ONENAND_OTP_LOCK_OFFSET] = 0xFC;
> + if (FLEXONENAND(this))
> + oob_buf[FLEXONENAND_OTP_LOCK_OFFSET] = 0xFC;
> + else
> + oob_buf[ONENAND_OTP_LOCK_OFFSET] = 0xFC;
>
> /*
> * Write lock mark to 8th word of sector0 of page0 of the spare0.
> * We write 16 bytes spare area instead of 2 bytes.
> + * For Flex-OneNAND, we write lock mark to 1st word of sector 4 of
> + * main area of page 49.
> */
> +
> from = 0;
> - len = 16;
> + len = FLEXONENAND(this) ? mtd->writesize : 16;
>
> ret = onenand_otp_walk(mtd, from, len, &retlen, oob_buf, do_otp_lock, MTD_OTP_USER);
>
> @@ -2495,6 +2754,14 @@
> break;
> }
>
>
> ddp = device & ONENAND_DEVICE_IS_DDP;
> density = onenand_get_density(device);
> - printk(KERN_INFO "%sOneNAND%s %dMB %sV 16-bit (0x%02x)\n",
> - demuxed ? "" : "Muxed ",
> + flexonenand = device & DEVICE_IS_FLEXONENAND;
> + printk(KERN_INFO "%s%sOneNAND%s %dMB %sV 16-bit (0x%02x)\n",
> + flexonenand ? "Flex-" : "",
> + demuxed ? "" : "Mux",
Please display pin type and Flex such as, Muxed {Flex-}OneNAND
> ddp ? "(DDP)" : "",
> (16 << density),
> vcc ? "2.65/3.3" : "1.8",
> device);
> - printk(KERN_INFO "OneNAND version = 0x%04x\n", version);
> + printk(KERN_INFO "%sOneNAND version = 0x%04x\n",
> + flexonenand ? "Flex-" : "", version);
Does it need to display Flex here?
> }
>
> static const struct onenand_manufacturers onenand_manuf_ids[] = {
> @@ -2558,6 +2828,181 @@
> }
>
> /**
> +* flexonenand_get_boundary - Reads the SLC boundary
> +* @param onenand_info - onenand info structure
> +**/
> +static int flexonenand_get_boundary(struct mtd_info *mtd)
> +{
> + struct onenand_chip *this = mtd->priv;
> + unsigned die, bdry;
> + int ret, syscfg, locked;
> +
> + /* Disable ECC */
> + syscfg = this->read_word(this->base + ONENAND_REG_SYS_CFG1);
> + this->write_word((syscfg | 0x0100), this->base + ONENAND_REG_SYS_CFG1);
> +
> + for (die = 0; die < this->dies; die++) {
> + this->command(mtd, FLEXONENAND_CMD_PI_ACCESS, die, 0);
> + this->wait(mtd, FL_SYNCING);
> +
> + this->command(mtd, ONENAND_CMD_READ, die, 0);
> + ret = this->wait(mtd, FL_READING);
> +
> + bdry = this->read_word(this->base + ONENAND_DATARAM);
> + locked = bdry >> FLEXONENAND_PI_UNLOCK_SHIFT;
> + locked = (locked == 0x3) ? 0 : 1;
> + this->boundary[die] = bdry & FLEXONENAND_PI_MASK;
> + this->boundary_locked[die] = locked;
> + this->command(mtd, ONENAND_CMD_RESET, 0, 0);
> + ret = this->wait(mtd, FL_RESETING);
> +
> + printk(KERN_INFO "Die %d boundary: %d%s\n", die,
> + this->boundary[die], locked ? "(Locked)" : "(Unlocked)");
> + }
> +
> + /* Enable ECC */
> + this->write_word(syscfg, this->base + ONENAND_REG_SYS_CFG1);
> + return 0;
> +}
> +
> +/**
> + * get_flexonenand_size - Fill up fields in onenand_chip
> + * boundary[], diesize[], chipsize,
> + * boundary_locked[]
> + * @param mtd - MTD device structure
> + */
> +void get_flexonenand_size(struct mtd_info *mtd)
Please make it 'static'.
> +{
> + struct onenand_chip *this = mtd->priv;
> /* Pages per a block are always 64 in OneNAND */
> mtd->erasesize = mtd->writesize << 6;
> -
> + mtd->erasesize <<= FLEXONENAND(this) ? 1 : 0;
Please add description Flex-OneNAND has always 128 pages per a block.
> this->erase_shift = ffs(mtd->erasesize) - 1;
> this->page_shift = ffs(mtd->writesize) - 1;
> this->page_mask = (1 << (this->erase_shift - this->page_shift)) - 1;
> @@ -2632,7 +3092,20 @@
>
> /* REVIST: Multichip handling */
>
> - mtd->size = this->chipsize;
> + if (FLEXONENAND(this)) {
> + unsigned die;
> +
> + get_flexonenand_size(mtd);
> +
> + /* Change the device boundaries if required */
> + for (die = 0; die < this->dies; die++)
> + if ((!this->boundary_locked[die]) &&
> + (boundary[die] >= 0) &&
> + (boundary[die] != this->boundary[die]))
> + flexonenand_set_boundary(mtd, die,
> + boundary[die], lock[die]);
> + } else
> + mtd->size = this->chipsize;
>
> /* Check OneNAND features */
> onenand_check_features(mtd);
> @@ -2749,6 +3222,10 @@
> * Allow subpage writes up to oobsize.
> */
> switch (mtd->oobsize) {
> + case 128:
> + this->ecclayout = &onenand_oob_128;
> + mtd->subpage_sft = 0;
> + break;
> case 64:
> this->ecclayout = &onenand_oob_64;
> mtd->subpage_sft = 2;
> @@ -2768,6 +3245,10 @@
> break;
> }
>
> + /* Don't allow the sub-page write in MLC */
> + if (ONENAND_IS_MLC(this))
> + mtd->subpage_sft = 0;
> +
> this->subpagesize = mtd->writesize >> mtd->subpage_sft;
>
> /*
> @@ -2812,7 +3293,8 @@
> mtd->owner = THIS_MODULE;
>
> /* Unlock whole block */
> - onenand_unlock_all(mtd);
> + FLEXONENAND(this) ? flexonenand_unlock_all(mtd)
> + : onenand_unlock_all(mtd);
>
> return this->scan_bbt(mtd);
> }
> @@ -2843,6 +3325,8 @@
> kfree(this->page_buf);
> if (this->options & ONENAND_OOBBUF_ALLOC)
> kfree(this->oob_buf);
> + if (FLEXONENAND(this))
> + kfree(mtd->eraseregions);
> }
>
> EXPORT_SYMBOL_GPL(onenand_scan);
> diff --git a/drivers/mtd/onenand/onenand_bbt.c b/drivers/mtd/onenand/onenand_bbt.c
> --- a/drivers/mtd/onenand/onenand_bbt.c
> +++ b/drivers/mtd/onenand/onenand_bbt.c
> @@ -60,6 +60,7 @@
> struct bbm_info *bbm = this->bbm;
> int i, j, numblocks, len, scanlen;
> int startblock;
> + unsigned slc;
> loff_t from;
> size_t readlen, ooblen;
> struct mtd_oob_ops ops;
Initialize the slc as '0'.
> @@ -76,7 +77,7 @@
> /* 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,13 @@
> }
> }
> i += 2;
> - from += (1 << bbm->bbt_erase_shift);
> + if (FLEXONENAND(this)) {
> + onenand_get_block(mtd, from, &slc);
> + from += (1 << bbm->bbt_erase_shift) >> 1;
> + if (!slc)
> + from += (1 << bbm->bbt_erase_shift) >> 1;
> + } else
> + from += (1 << bbm->bbt_erase_shift);
> }
>
> return 0;
> @@ -143,7 +150,7 @@
> uint8_t res;
>
> /* Get block number * 2 */
> - block = (int) (offs >> (bbm->bbt_erase_shift - 1));
> + block = (int) (onenand_get_block(mtd, 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 +185,7 @@
> 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) {
> diff --git a/include/linux/mtd/onenand.h b/include/linux/mtd/onenand.h
> --- a/include/linux/mtd/onenand.h
> +++ b/include/linux/mtd/onenand.h
> @@ -17,8 +17,24 @@
> #include <linux/mtd/onenand_regs.h>
> #include <linux/mtd/bbm.h>
>
> +#define MAX_DIES 2
> #define MAX_BUFFERRAM 2
>
> +/**
> + * FlexOneNAND device boundary setting
> + * Setting -1 will not change the boundary
> + */
> +#define FLEXONENAND_DIE0_BOUNDARY -1
> +#define FLEXONENAND_DIE1_BOUNDARY -1
> +
> +/**
> + * Setting value 1 locks the boundary
> + * WARNING : Once locked, the boundary cannot be changed.
> + * Use with care.
> + */
> +#define FLEXONENAND_DIE0_ISLOCKED 0
> +#define FLEXONENAND_DIE1_ISLOCKED 0
> +
> /* Scan and identify a OneNAND device */
> extern int onenand_scan(struct mtd_info *mtd, int max_chips);
> /* Free resources held by the OneNAND device */
> @@ -51,6 +67,11 @@
> /**
> * struct onenand_chip - OneNAND Private Flash Chip Data
> * @base: [BOARDSPECIFIC] address to access OneNAND
> + * @dies: [INTERN][FLEX-ONENAND] number of dies on chip
> + * @boundary: [INTERN][FLEX-ONENAND] Boundary of the dies
> + * @boundary_locked: [INTERN][FLEX-ONENAND] TRUE indicates die boundary
> + * is locked and cannot be changed
> + * @diesize: [INTERN][FLEX-ONENAND] Size of the dies
> * @chipsize: [INTERN] the size of one chip for multichip arrays
> * @device_id: [INTERN] device ID
> * @density_mask: chip density, used for DDP devices
> @@ -92,9 +113,14 @@
> */
> struct onenand_chip {
> void __iomem *base;
> + unsigned dies;
> + unsigned boundary[MAX_DIES];
> + unsigned int boundary_locked[MAX_DIES];
> + unsigned int diesize[MAX_DIES];
> unsigned int chipsize;
> unsigned int device_id;
> unsigned int version_id;
> + unsigned int technology;
> unsigned int density_mask;
> unsigned int options;
>
> @@ -145,6 +171,8 @@
> #define ONENAND_SET_BUFFERRAM0(this) (this->bufferram_index = 0)
> #define ONENAND_SET_BUFFERRAM1(this) (this->bufferram_index = 1)
>
> +#define FLEXONENAND(this) \
> + (this->device_id & DEVICE_IS_FLEXONENAND)
> #define ONENAND_GET_SYS_CFG1(this) \
> (this->read_word(this->base + ONENAND_REG_SYS_CFG1))
> #define ONENAND_SET_SYS_CFG1(v, this) \
> @@ -153,6 +181,9 @@
> #define ONENAND_IS_DDP(this) \
> (this->device_id & ONENAND_DEVICE_IS_DDP)
>
> +#define ONENAND_IS_MLC(this) \
> + (this->technology & ONENAND_TECHNOLOGY_IS_MLC)
> +
> #ifdef CONFIG_MTD_ONENAND_2X_PROGRAM
> #define ONENAND_IS_2PLANE(this) \
> (this->options & ONENAND_HAS_2PLANE)
> @@ -189,5 +220,7 @@
>
> int onenand_bbt_read_oob(struct mtd_info *mtd, loff_t from,
> struct mtd_oob_ops *ops);
> +unsigned onenand_get_block(struct mtd_info *mtd, loff_t addr,
> + unsigned *isblkslc);
>
> #endif /* __LINUX_MTD_ONENAND_H */
> diff --git a/include/linux/mtd/onenand_regs.h b/include/linux/mtd/onenand_regs.h
> --- a/include/linux/mtd/onenand_regs.h
> +++ b/include/linux/mtd/onenand_regs.h
> @@ -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,6 +134,9 @@
> #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)
> @@ -190,10 +203,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 */
> diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
> --- a/include/mtd/mtd-abi.h
> +++ b/include/mtd/mtd-abi.h
> @@ -102,7 +102,11 @@
> uint32_t useecc;
> uint32_t eccbytes;
> uint32_t oobfree[8][2];
> +#ifdef CONFIG_MTD_ONENAND
> + uint32_t eccpos[128];
> +#else
> uint32_t eccpos[32];
> +#endif
> };
>
> struct nand_oobfree {
> @@ -117,7 +121,11 @@
> */
> struct nand_ecclayout {
> uint32_t eccbytes;
> +#ifdef CONFIG_MTD_ONENAND
> + uint32_t eccpos[128];
> +#else
> uint32_t eccpos[64];
> +#endif
> uint32_t oobavail;
> struct nand_oobfree oobfree[MTD_MAX_OOBFREE_ENTRIES];
> };
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists