| Message ID | 20090721025303.GA3693@july |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 07/21/2009 05:53 AM, Kyungmin Park wrote: > At bootloader, we don't need to read full page. It takes too long time. > Instead it only read pages required for boot. > > Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> > --- I do not understand the commit message. Could you please provide a better one which explains what the patch is really doing and why.
Hi On Tue, Jul 21, 2009 at 7:56 PM, Artem Bityutskiy<dedekind1@gmail.com> wrote: > Hi, > > On 07/21/2009 05:53 AM, Kyungmin Park wrote: >> >> At bootloader, we don't need to read full page. It takes too long time. >> Instead it only read pages required for boot. >> >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> >> --- > > I do not understand the commit message. Could you please provide a better > one which explains what the patch is really doing and why. Right, the page means all blocks. It doesn't scan all blocks at boot time. only some block contains kernel part are read and boot. The goal is simple, Avoid double scanning at boot time. One time for bootloader, Another is kernel. As you know at bootloader, only load kernel in normal case. it means we only read the kernel bad blocks and handle it. and jump to the kernel. that's all Thank you, Kyungmin Park
Hi, On Tue, Jul 21, 2009 at 8:05 PM, Kyungmin Park<kmpark@infradead.org> wrote: > Hi > > On Tue, Jul 21, 2009 at 7:56 PM, Artem Bityutskiy<dedekind1@gmail.com> wrote: >> Hi, >> >> On 07/21/2009 05:53 AM, Kyungmin Park wrote: >>> >>> At bootloader, we don't need to read full page. It takes too long time. >>> Instead it only read pages required for boot. >>> >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> >>> --- >> >> I do not understand the commit message. Could you please provide a better >> one which explains what the patch is really doing and why. > > Right, the page means all blocks. It doesn't scan all blocks at boot > time. only some block contains kernel part are read and boot. > > The goal is simple, Avoid double scanning at boot time. One time for > bootloader, Another is kernel. > As you know at bootloader, only load kernel in normal case. it means > we only read the kernel bad blocks and handle it. > and jump to the kernel. that's all > Experimental idea. How about to integrate the bbt scan and ubi scan? At ubi scan time. it scans all ubi blocks. In different from NAND, OneNAND read main page and spare page both. So if runtime badblock check enabled, read the page and check oob first and if it's valid. use main page. If not, set it's badblock. I'm not sure how much codes are modified. :) How to you think? Thank you, Kyungmin Park
On Wed, 2009-07-22 at 11:39 +0900, Kyungmin Park wrote: > Hi, > > On Tue, Jul 21, 2009 at 8:05 PM, Kyungmin Park<kmpark@infradead.org> wrote: > > Hi > > > > On Tue, Jul 21, 2009 at 7:56 PM, Artem Bityutskiy<dedekind1@gmail.com> wrote: > >> Hi, > >> > >> On 07/21/2009 05:53 AM, Kyungmin Park wrote: > >>> > >>> At bootloader, we don't need to read full page. It takes too long time. > >>> Instead it only read pages required for boot. > >>> > >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> > >>> --- > >> > >> I do not understand the commit message. Could you please provide a better > >> one which explains what the patch is really doing and why. > > > > Right, the page means all blocks. It doesn't scan all blocks at boot > > time. only some block contains kernel part are read and boot. > > > > The goal is simple, Avoid double scanning at boot time. One time for > > bootloader, Another is kernel. > > As you know at bootloader, only load kernel in normal case. it means > > we only read the kernel bad blocks and handle it. > > and jump to the kernel. that's all > > I understand the double-scanning issue. But I still do not understand how your patch is avoiding it? If the boot-loader has scanned the flash, it should pass the bad-block bitmap to the kernel. If you have on-flash BBT, then probably this is not a problem. > Experimental idea. > How about to integrate the bbt scan and ubi scan? It is possible, but isn't it better to just have on-flash BBT and forget about any scanning for bad blocks at all? We could go even further: If the boot-loader supports ubi, it could do the scanning and pass all UBI data to the kernel, in which case UBI would not need to do any scanning. > At ubi scan time. it scans all ubi blocks. In different from NAND, > OneNAND read main page and spare page both. > So if runtime badblock check enabled, read the page and check oob > first and if it's valid. use main page. If not, set it's badblock. > I'm not sure how much codes are modified. :) > > How to you think? It is possible, but I think the implementation will add more ugliness... Why not to just use on-flash BBT instead?
On Wed, Jul 22, 2009 at 3:19 PM, Artem Bityutskiy<dedekind1@gmail.com> wrote: > On Wed, 2009-07-22 at 11:39 +0900, Kyungmin Park wrote: >> Hi, >> >> On Tue, Jul 21, 2009 at 8:05 PM, Kyungmin Park<kmpark@infradead.org> wrote: >> > Hi >> > >> > On Tue, Jul 21, 2009 at 7:56 PM, Artem Bityutskiy<dedekind1@gmail.com> wrote: >> >> Hi, >> >> >> >> On 07/21/2009 05:53 AM, Kyungmin Park wrote: >> >>> >> >>> At bootloader, we don't need to read full page. It takes too long time. >> >>> Instead it only read pages required for boot. >> >>> >> >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> >> >>> --- >> >> >> >> I do not understand the commit message. Could you please provide a better >> >> one which explains what the patch is really doing and why. >> > >> > Right, the page means all blocks. It doesn't scan all blocks at boot >> > time. only some block contains kernel part are read and boot. >> > >> > The goal is simple, Avoid double scanning at boot time. One time for >> > bootloader, Another is kernel. >> > As you know at bootloader, only load kernel in normal case. it means >> > we only read the kernel bad blocks and handle it. >> > and jump to the kernel. that's all >> > > > I understand the double-scanning issue. But I still do not understand > how your patch is avoiding it? If the boot-loader has scanned the > flash, it should pass the bad-block bitmap to the kernel. If you have > on-flash BBT, then probably this is not a problem. It don't avoid the double-scanning issue. only partial scan at bootloader. In my environemt kernel has 2MiB partition and we only scan this partition at load kernel command. Exactly that it skip the bad block scanning except the kernel partition at bootloader As bootcmd load kernel, it first check that read block is bad, if not read this block, next check again next block, and read next block and so on. > >> Experimental idea. >> How about to integrate the bbt scan and ubi scan? > > It is possible, but isn't it better to just have on-flash BBT and > forget about any scanning for bad blocks at all? > > We could go even further: If the boot-loader supports ubi, it could do > the scanning and pass all UBI data to the kernel, in which case UBI > would not need to do any scanning. This is chicken and egg problem. actually the scan time is almost same either bootloader or kernel. My main idea integrate the scanning and ubi scan. In any case, we have to scan whole chip at least one. > >> At ubi scan time. it scans all ubi blocks. In different from NAND, >> OneNAND read main page and spare page both. >> So if runtime badblock check enabled, read the page and check oob >> first and if it's valid. use main page. If not, set it's badblock. >> I'm not sure how much codes are modified. :) >> >> How to you think? > > It is possible, but I think the implementation will add more ugliness... > Why not to just use on-flash BBT instead? > I don't have any idea about on-flash BBT. Thank you, Kyungmin Park
On 07/22/2009 10:11 AM, Kyungmin Park wrote: > It don't avoid the double-scanning issue. only partial scan at > bootloader. In my environemt kernel has 2MiB partition and we only > scan this partition at load kernel command. > Exactly that it skip the bad block scanning except the kernel > partition at bootloader > As bootcmd load kernel, it first check that read block is bad, if not > read this block, next check again next block, and read next block and > so on. So you have a small kernel partition, you store the kernel there. Your boot-loader reads the kernel partition and loads the kernel from there. And of course the boot-loader scans the kernel partition for bad block. And your boot-loader does not need to scan anything else. I understand this part. But what is next? Do you want to tell the kernel to avoid scanning the kernel partition because boot-loader has already scanned it? Sorry, I still cannot get the idea... > I don't have any idea about on-flash BBT. It is standard mechanism which exists in MTD from early tglx1 times. MTD can keep BBT on flash, AFAIR, in the last eraseblock. Look for the NAND_USE_FLASH_BBT constant. And this is the way to go if you want to optimize BBT scanning, IMO.
On Wed, Jul 22, 2009 at 4:28 PM, Artem Bityutskiy<dedekind1@gmail.com> wrote: > On 07/22/2009 10:11 AM, Kyungmin Park wrote: >> >> It don't avoid the double-scanning issue. only partial scan at >> bootloader. In my environemt kernel has 2MiB partition and we only >> scan this partition at load kernel command. >> Exactly that it skip the bad block scanning except the kernel >> partition at bootloader >> As bootcmd load kernel, it first check that read block is bad, if not >> read this block, next check again next block, and read next block and >> so on. > > So you have a small kernel partition, you store the kernel there. > Your boot-loader reads the kernel partition and loads the kernel > from there. And of course the boot-loader scans the kernel partition > for bad block. And your boot-loader does not need to scan anything > else. I understand this part. Right > > But what is next? Do you want to tell the kernel to avoid scanning the > kernel partition because boot-loader has already scanned it? Sorry, I > still cannot get the idea... Kernel part is same as before. Now I just optimize the boot loader part. >> I don't have any idea about on-flash BBT. > > It is standard mechanism which exists in MTD from early tglx1 times. > MTD can keep BBT on flash, AFAIR, in the last eraseblock. Look for the > NAND_USE_FLASH_BBT constant. And this is the way to go if you want to > optimize BBT scanning, IMO. Yes I know, I mean no plan to implement it now. Sorry Thank you, Kyungmin Park
Hi , > On Wed, 2009-07-22 at 11:39 +0900, Kyungmin Park wrote: >> Hi, >> >> On Tue, Jul 21, 2009 at 8:05 PM, Kyungmin >> Park<kmpark@infradead.org> wrote: >> > Hi >> > >> > On Tue, Jul 21, 2009 at 7:56 PM, Artem >> > Bityutskiy<dedekind1@gmail.com> wrote: >> >> Hi, >> >> >> >> On 07/21/2009 05:53 AM, Kyungmin Park wrote: >> >>> >> >>> At bootloader, we don't need to read full page. It >> >>> takes too long time. >> >>> Instead it only read pages required for boot. >> >>> >> >>> Signed-off-by: Kyungmin >> >>> Park<kyungmin.park@samsung.com> >> >>> --- >> >> >> >> I do not understand the commit message. Could you >> >> please provide a better >> >> one which explains what the patch is really doing and >> >> why. >> > >> > Right, the page means all blocks. It doesn't scan all >> > blocks at boot >> > time. only some block contains kernel part are read and >> > boot. >> > >> > The goal is simple, Avoid double scanning at boot time. >> > One time for >> > bootloader, Another is kernel. >> > As you know at bootloader, only load kernel in normal >> > case. it means >> > we only read the kernel bad blocks and handle it. >> > and jump to the kernel. that's all >> > > > I understand the double-scanning issue. But I still do not > understand > how your patch is avoiding it? If the boot-loader has > scanned the > flash, it should pass the bad-block bitmap to the kernel. > If you have > on-flash BBT, then probably this is not a problem. > >> Experimental idea. >> How about to integrate the bbt scan and ubi scan? > In my opinion integrating bbt scan and ubi scan is a valid idea. It will save our double scanning issue that is available in UBi at apresent. > It is possible, but isn't it better to just have on-flash > BBT and > forget about any scanning for bad blocks at all? > > We could go even further: If the boot-loader supports ubi, > it could do > the scanning and pass all UBI data to the kernel, in which > case UBI > would not need to do any scanning. > >> At ubi scan time. it scans all ubi blocks. In different >> from NAND, >> OneNAND read main page and spare page both. >> So if runtime badblock check enabled, read the page and >> check oob >> first and if it's valid. use main page. If not, set it's >> badblock. >> I'm not sure how much codes are modified. :) >> >> How to you think? > > It is possible, but I think the implementation will add > more ugliness... > Why not to just use on-flash BBT instead? implmentaing on flash bab block table will save our bbt scan time but flash bbt table may go huge bceuase you have to set full mapping table and in big device it may be to huge. > > -- > Best Regards, > Artem Bityutskiy (????? ????????) > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ >
On Wed, 2009-07-22 at 13:42 +0530, Amit Kumar Sharma wrote: > implmentaing on flash bab block table will save our bbt > scan time but flash bbt table may go huge bceuase you have > to set full > mapping table and in big device it may be to huge. If it is huge, then doing any even one scanning is crazy anyway.
On 07/21/2009 05:53 AM, Kyungmin Park wrote: > At bootloader, we don't need to read full page. It takes too long time. > Instead it only read pages required for boot. > > Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> I still did not get it, why you change boot-loader behavior by patching the kernel. Or you use the kernel as a bootloader? Also, the mtd-2.6.git tree does not havemtd/onenand/samsung.c Anyway, could you please re-send the patch with a nice description, for silly people like me :-)
On Sat, Jul 25, 2009 at 12:31 AM, Artem Bityutskiy<dedekind1@gmail.com> wrote: > On 07/21/2009 05:53 AM, Kyungmin Park wrote: >> >> At bootloader, we don't need to read full page. It takes too long time. >> Instead it only read pages required for boot. >> >> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> > > I still did not get it, why you change boot-loader behavior by patching > the kernel. Or you use the kernel as a bootloader? Since code consistency. I also send a same patch to u-boot. actually u-boot OneNAND code are almost same as kernel one. > > Also, the mtd-2.6.git tree does not havemtd/onenand/samsung.c Right, samsung.c is for s3c64xx and s5pc100 SoCs. I will send it after more tests. > > Anyway, could you please re-send the patch with a nice description, for > silly people like me :-) Sure. Thank you, Kyungmin Park
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c index 6e82909..f2b2d2d 100644 --- a/drivers/mtd/onenand/onenand_base.c +++ b/drivers/mtd/onenand/onenand_base.c @@ -9,7 +9,7 @@ * auto-placement support, read-while load support, various fixes * Copyright (C) Nokia Corporation, 2007 * - * Vishak G <vishak.g at samsung.com>, Rohit Hagargundgi <h.rohit at samsung.com> + * Vishak G <vishak.g@samsung.com>, Rohit Hagargundgi <h.rohit@samsung.com> * Flex-OneNAND support * Copyright (C) Samsung Electronics, 2008 * @@ -1477,6 +1477,7 @@ static int onenand_bbt_wait(struct mtd_info *mtd, int state) * @param ops oob operation description structure * * OneNAND read out-of-band data from the spare area for bbt scan + * Note that it operates without lock */ int onenand_bbt_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops) @@ -1498,9 +1499,6 @@ int onenand_bbt_read_oob(struct mtd_info *mtd, loff_t from, return ONENAND_BBT_READ_FATAL_ERROR; } - /* Grab the lock and see if the device is available */ - onenand_get_device(mtd, FL_READING); - column = from & (mtd->oobsize - 1); readcmd = ONENAND_IS_MLC(this) ? ONENAND_CMD_READ : ONENAND_CMD_READOOB; @@ -1537,9 +1535,6 @@ int onenand_bbt_read_oob(struct mtd_info *mtd, loff_t from, } } - /* Deselect and wake up anyone waiting on the device */ - onenand_release_device(mtd); - ops->oobretlen = read; return ret; } @@ -3408,7 +3403,7 @@ static void onenand_resume(struct mtd_info *mtd) if (this->state == FL_PM_SUSPENDED) onenand_release_device(mtd); else - printk(KERN_ERR "resume() called for the chip which is not" + printk(KERN_ERR "resume() called for the chip which is not " "in suspended state\n"); } diff --git a/drivers/mtd/onenand/onenand_bbt.c b/drivers/mtd/onenand/onenand_bbt.c index a91fcac..b8c0166 100644 --- a/drivers/mtd/onenand/onenand_bbt.c +++ b/drivers/mtd/onenand/onenand_bbt.c @@ -3,11 +3,17 @@ * * Bad Block Table support for the OneNAND driver * - * Copyright(c) 2005 Samsung Electronics + * Copyright(c) 2005-2009 Samsung Electronics * Kyungmin Park <kyungmin.park@samsung.com> * * Derived from nand_bbt.c * + * Legend in badblock table: + * 0x00 Normal + * 0x01 RESERVED + * 0x02 Used for runtime badblock check + * 0x03 Bad block (initial or runtime) + * * TODO: * Split BBT core and chip specific BBT. */ @@ -17,6 +23,10 @@ #include <linux/mtd/onenand.h> #include <linux/mtd/compatmac.h> +#define BBT_NORMAL_BITS 0x00 +#define BBT_RUNTIME_BADBLOCK_BITS 0x02 +#define BBT_BADBLOCK_BITS 0x03 + /** * check_short_pattern - [GENERIC] check if a pattern is in the buffer * @param buf the buffer to search @@ -28,7 +38,6 @@ * tables and good / bad block identifiers. Same as check_pattern, but * no optional empty check and the pattern is expected to start * at offset 0. - * */ static int check_short_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td) { @@ -44,6 +53,51 @@ static int check_short_pattern(uint8_t *buf, int len, int paglen, struct nand_bb } /** + * read_page_oob - [GENERIC] Read oob for runtime badblock check + * @param mtd MTD device structure + * @param from the length of buffer to search + * @param buf temporary buffer + * + * Read page oob at runtime badblock check + */ +static int read_page_oob(struct mtd_info *mtd, loff_t from, u_char *buf) +{ + struct onenand_chip *this = mtd->priv; + struct bbm_info *bbm = this->bbm; + struct nand_bbt_descr *bd = bbm->badblock_pattern; + struct mtd_oob_ops ops; + int ret, scanlen, block, j, res; + + scanlen = 0; + + ops.mode = MTD_OOB_PLACE; + ops.ooblen = 16; + ops.oobbuf = buf; + ops.len = ops.ooboffs = ops.retlen = ops.oobretlen = 0; + + /* Get block number * 2 */ + block = (int) (onenand_block(this, from) << 1); + + /* Set normal block first */ + res = BBT_NORMAL_BITS; + bbm->bbt[block >> 3] |= res << (block & 0x6); + + for (j = 0; j < 2; j++) { + ret = onenand_bbt_read_oob(mtd, from + j * mtd->writesize + bd->offs, &ops); + if (ret || check_short_pattern(&buf[j * scanlen], scanlen, mtd->writesize, bd)) { + res = BBT_BADBLOCK_BITS; + bbm->bbt[block >> 3] |= res << (block & 0x6); + printk(KERN_WARNING "Bad eraseblock %d at 0x%08x\n", + block >> 1, (unsigned int) from); + mtd->ecc_stats.badblocks++; + break; + } + } + + return res; +} + +/** * create_bbt - [GENERIC] Create a bad block table by scanning the device * @param mtd MTD device structure * @param buf temporary buffer @@ -99,7 +153,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr return -EIO; if (ret || check_short_pattern(&buf[j * scanlen], scanlen, mtd->writesize, bd)) { - bbm->bbt[i >> 3] |= 0x03 << (i & 0x6); + bbm->bbt[i >> 3] |= BBT_BADBLOCK_BITS << (i & 0x6); printk(KERN_WARNING "Bad eraseblock %d at 0x%08x\n", i >> 1, (unsigned int) from); mtd->ecc_stats.badblocks++; @@ -118,7 +172,6 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr return 0; } - /** * onenand_memory_bbt - [GENERIC] create a memory based bad block table * @param mtd MTD device structure @@ -127,7 +180,7 @@ static int create_bbt(struct mtd_info *mtd, uint8_t *buf, struct nand_bbt_descr * The function creates a memory based bbt by scanning the device * for manufacturer / software marked good / bad blocks */ -static inline int onenand_memory_bbt (struct mtd_info *mtd, struct nand_bbt_descr *bd) +static int onenand_memory_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd) { struct onenand_chip *this = mtd->priv; @@ -152,6 +205,12 @@ static int onenand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt) block = (int) (onenand_block(this, offs) << 1); res = (bbm->bbt[block >> 3] >> (block & 0x06)) & 0x03; + if (this->options & ONENAND_RUNTIME_BADBLOCK_CHECK) { + if (res == BBT_RUNTIME_BADBLOCK_BITS) + res = read_page_oob(mtd, offs, this->page_buf); + } + + DEBUG(MTD_DEBUG_LEVEL2, "onenand_isbad_bbt: bbt info for offs 0x%08x: (block %d) 0x%02x\n", (unsigned int) offs, block >> 1, res); @@ -201,6 +260,12 @@ int onenand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd) if (!bbm->isbad_bbt) bbm->isbad_bbt = onenand_isbad_bbt; + if (this->options & ONENAND_RUNTIME_BADBLOCK_CHECK) { + printk(KERN_INFO "Scanning device for bad blocks (skipped)\n"); + memset(bbm->bbt, 0xAA, len); + return 0; + } + /* Scan the device to build a memory based bad block table */ if ((ret = onenand_memory_bbt(mtd, bd))) { printk(KERN_ERR "onenand_scan_bbt: Can't scan flash and build the RAM-based BBT\n"); diff --git a/drivers/mtd/onenand/samsung.c b/drivers/mtd/onenand/samsung.c index 4b92abc..e4b8505 100644 --- a/drivers/mtd/onenand/samsung.c +++ b/drivers/mtd/onenand/samsung.c @@ -670,6 +670,9 @@ static int s3c_onenand_probe(struct platform_device *pdev) goto oob_buf_fail; } + /* Set runtime badblock check before onenand_scan() */ + this->options |= ONENAND_RUNTIME_BADBLOCK_CHECK; + if (onenand_scan(mtd, 1)) { err = -EFAULT; goto scan_failed; diff --git a/include/linux/mtd/onenand.h b/include/linux/mtd/onenand.h index 8ed8733..5697476 100644 --- a/include/linux/mtd/onenand.h +++ b/include/linux/mtd/onenand.h @@ -189,6 +189,7 @@ struct onenand_chip { #define ONENAND_HAS_UNLOCK_ALL (0x0002) #define ONENAND_HAS_2PLANE (0x0004) #define ONENAND_SKIP_UNLOCK_CHECK (0x0100) +#define ONENAND_RUNTIME_BADBLOCK_CHECK (0x0200) #define ONENAND_PAGEBUF_ALLOC (0x1000) #define ONENAND_OOBBUF_ALLOC (0x2000)
At bootloader, we don't need to read full page. It takes too long time. Instead it only read pages required for boot. Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> ---