Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751975AbbFZFxD (ORCPT ); Fri, 26 Jun 2015 01:53:03 -0400 Received: from mailout.micron.com ([137.201.242.129]:16720 "EHLO mailout.micron.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751558AbbFZFwz (ORCPT ); Fri, 26 Jun 2015 01:52:55 -0400 From: =?utf-8?B?UGV0ZXIgUGFuIOa9mOagiyAocGV0ZXJwYW5kb25nKQ==?= To: Kamil Debski , "dwmw2@infradead.org" , Brian Norris , "fransklaver@gmail.com" , "wsa@the-dreams.de" , "zajec5@gmail.com" , "boris.brezillon@free-electrons.com" , "baruch@tkos.co.il" , "ezequiel.garcia@free-electrons.com" , "kdasu.kdev@gmail.com" , "rogerq@ti.com" , "asierra@xes-inc.com" , bpqw , "Ionela Voinescu" CC: "linux-mtd@lists.infradead.org" , "linux-kernel@vger.kernel.org" , =?utf-8?B?RnJhbmsgTGl1IOWImOe+pCAoZnJhbmtsaXUp?= Subject: RE: [PATCH 1/1] mtd: nand_bbt: separate struct nand_chip from nand_bbt.c Thread-Topic: [PATCH 1/1] mtd: nand_bbt: separate struct nand_chip from nand_bbt.c Thread-Index: AdCO369Kn44//ROMQ26ouDqH5B3pagAAQ31wCBgSDxAAJFjTsA== Date: Fri, 26 Jun 2015 05:51:53 +0000 Message-ID: <87F60714EC601C4C83DFF1D2E3D390A0326875CC@NTXXIAMBX02.xacn.micron.com> References: <87F60714EC601C4C83DFF1D2E3D390A027AF036F@NTXXIAMBX02.xacn.micron.com> <616819EF2EBFD445AEF36EFF651655B6AB9E83@hhmail02.hh.imgtec.org> In-Reply-To: <616819EF2EBFD445AEF36EFF651655B6AB9E83@hhmail02.hh.imgtec.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.167.84.5] X-TM-AS-Product-Ver: SMEX-11.0.0.4179-8.000.1202-21634.005 X-TM-AS-Result: No--20.243300-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No x-mt-checkinternalsenderrule: True Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id t5Q5r8dI027724 Content-Length: 9898 Lines: 283 Hi Kamil, First of all, thanks for your great contribution. On Jun 25, 2015 at 20:44 PM, Kamil Debski wrote: > > Hi Peter, > > Thank you for work on this. I have applied this patch to the latest > kernel and it had some minor merge conflicts that needed to be resolved. > But apart from this it compiled and worked in my setup. > > My testing procedure was following: > 1) I applied your patch to the NAND core > 2) I applied old SPI NAND patches by Ezequiel Garcia. His patches use > the NAND core as a bridge between the SPI NAND core and MTD core. Hence > it was possible to test changes in the NAND core. > 3) The kernel compiled and booted. I then run nandtest and it finished > successfully. > > I know that you have prepared a patch that adds the SPI NAND core and > that uses common BBT. I think that it is a good idea to send it along > with a v2 of this patch rebased to the 4.1 or the next branch of Brain > Norris. I would really like to test it :) I will send the new SPI NAND patch and v2 BBT patch out soon. Your effort will be a great help! > > I have read the patch code and I have this one minor comment. Please > find it below. > > Best wishes, > Kamil Debski > > From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On > Behalf > Of Peter Pan ?? (peterpandong) > Sent: 15 May 2015 08:32 > > > Currently nand_bbt.c is tied with struct nand_chip, and it makes > other > > NAND family chips hard to use nand_bbt.c. Maybe it's the reason why > > onenand has own bbt(onenand_bbt.c). > > > > Parameterize a few relevant device detail information into a new > > nand_bbt struct, and set some hooks for chip specified part. Allocate > > and initialize struct nand_bbt in nand_base.c. > > > > Most of the patch is borrowed from Brian Norris > > . > > http://git.infradead.org/users/norris/linux- > > mtd.git/shortlog/refs/heads/nand-bbt > > > > Signed-off-by: Peter Pan > > Signed-off-by: Brian Norris > > Tested-by: Kamil Debski > > > --- > > drivers/mtd/nand/docg4.c | 8 +- > > drivers/mtd/nand/nand_base.c | 145 +++++++++++- > > drivers/mtd/nand/nand_bbt.c | 518 +++++++++++++++++---------------- > ----- > > ----- > > include/linux/mtd/bbm.h | 96 +------- > > include/linux/mtd/nand.h | 11 +- > > include/linux/mtd/nand_bbt.h | 160 +++++++++++++ > > 6 files changed, 516 insertions(+), 422 deletions(-) > > create mode 100644 include/linux/mtd/nand_bbt.h > > > > [ snip] > > > diff --git a/include/linux/mtd/nand_bbt.h > b/include/linux/mtd/nand_bbt.h > > new file mode 100644 > > index 0000000..03ee0eb > > --- /dev/null > > +++ b/include/linux/mtd/nand_bbt.h > > @@ -0,0 +1,160 @@ > > +/* > > + * NAND family Bad Block Table support > > + * > > + * Copyright © 2015 Broadcom Corporation > > + * Brian Norris > > + * > > + * This program is free software; you can redistribute it and/or > modify > > + * it under the terms of the GNU General Public License as published > by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + */ > > +#ifndef __LINUX_MTD_NAND_BBT_H > > +#define __LINUX_MTD_NAND_BBT_H > > + > > +struct mtd_info; > > + > > +/* The maximum number of NAND chips in an array */ > > +#define NAND_MAX_CHIPS 8 > > + > > +/** > > + * struct nand_bbt_descr - bad block table descriptor > > + * @options: options for this descriptor > > + * @pages: the page(s) where we find the bbt, used with option > > BBT_ABSPAGE > > + * when bbt is searched, then we store the found bbts pages > > here. > > + * Its an array and supports up to 8 chips now > > + * @offs: offset of the pattern in the oob area of the page > > + * @veroffs: offset of the bbt version counter in the oob are of > the page > > + * @version: version read from the bbt page during scan > > + * @len: length of the pattern, if 0 no pattern check is performed > > + * @maxblocks: maximum number of blocks to search for a bbt. This > > number of > > + * blocks is reserved at the end of the device where the > tables > > are > > + * written. > > + * @reserved_block_code: if non-0, this pattern denotes a reserved > (rather > > than > > + * bad) block in the stored bbt > > + * @pattern: pattern to identify bad block table or factory marked > good / > > + * bad blocks, can be NULL, if len = 0 > > + * > > + * Descriptor for the bad block table marker and the descriptor for > the > > + * pattern which identifies good and bad blocks. The assumption is > made > > + * that the pattern and the version count are always located in the > oob area > > + * of the first block. > > + */ > > +struct nand_bbt_descr { > > + int options; > > + int pages[NAND_MAX_CHIPS]; > > + int offs; > > + int veroffs; > > + uint8_t version[NAND_MAX_CHIPS]; > > + int len; > > + int maxblocks; > > + int reserved_block_code; > > + uint8_t *pattern; > > +}; > > + > > +/* Options for the bad block table descriptors */ > > + > > +/* The number of bits used per block in the bbt on the device */ > > +#define NAND_BBT_NRBITS_MSK 0x0000000F > > +#define NAND_BBT_1BIT 0x00000001 > > +#define NAND_BBT_2BIT 0x00000002 > > +#define NAND_BBT_4BIT 0x00000004 > > +#define NAND_BBT_8BIT 0x00000008 > > +/* The bad block table is in the last good block of the device */ > > +#define NAND_BBT_LASTBLOCK 0x00000010 > > +/* The bbt is at the given page, else we must scan for the bbt */ > > +#define NAND_BBT_ABSPAGE 0x00000020 > > +/* bbt is stored per chip on multichip devices */ > > +#define NAND_BBT_PERCHIP 0x00000080 > > +/* bbt has a version counter at offset veroffs */ > > +#define NAND_BBT_VERSION 0x00000100 > > +/* Create a bbt if none exists */ > > +#define NAND_BBT_CREATE 0x00000200 > > +/* > > + * Create an empty BBT with no vendor information. Vendor's > information > > may be > > + * unavailable, for example, if the NAND controller has a different > data and > > OOB > > + * layout or if this information is already purged. Must be used in > > conjunction > > + * with NAND_BBT_CREATE. > > + */ > > +#define NAND_BBT_CREATE_EMPTY 0x00000400 > > +/* Write bbt if necessary */ > > +#define NAND_BBT_WRITE 0x00002000 > > +/* Read and write back block contents when writing bbt */ > > +#define NAND_BBT_SAVECONTENT 0x00004000 > > +/* Search good / bad pattern on the first and the second page */ > > +#define NAND_BBT_SCAN2NDPAGE 0x00008000 > > +/* Search good / bad pattern on the last page of the eraseblock */ > > +#define NAND_BBT_SCANLASTPAGE 0x00010000 > > +/* > > + * Use a flash based bad block table. By default, OOB identifier is > saved in > > + * OOB area. This option is passed to the default bad block table > function. > > + */ > > +#define NAND_BBT_USE_FLASH 0x00020000 > > +/* > > + * Do not store flash based bad block table marker in the OOB area; > store it > > + * in-band. > > + */ > > +#define NAND_BBT_NO_OOB 0x00040000 > > +/* > > + * Do not write new bad block markers to OOB; useful, e.g., when ECC > > covers > > + * entire spare area. Must be used with NAND_BBT_USE_FLASH. > > + */ > > +#define NAND_BBT_NO_OOB_BBM 0x00080000 > > + > > +/* The maximum number of blocks to scan for a bbt */ > > +#define NAND_BBT_SCAN_MAXBLOCKS 4 > > + > > +struct nand_bbt { > > + struct mtd_info *mtd; > > + > > + /* > > + * This is important to abstract out of nand_bbt.c and provide > > + * separately in nand_base.c and spi-nand-base.c -- it's sort of > > + * duplicated in nand_block_bad() (nand_base) and > > + * scan_block_fast() (nand_bbt) right now > > + * > > + * Note that this also means nand_chip.badblock_pattern should > > + * be removed from nand_bbt.c > > + */ > > + int (*is_bad_bbm)(struct mtd_info *mtd, loff_t ofs); > > + > > + /* Only required if the driver wants to attempt to program new > > + * bad block markers that imitate the factory-marked BBMs */ > > + int (*mark_bad_bbm)(struct mtd_info *mtd, loff_t ofs); > > + > > + /* Erase a block, bypassing reserved checks */ > > + int (*erase)(struct mtd_info *mtd, loff_t ofs); > > + > > + unsigned int bbt_options; > > + > > + /* Only required for NAND_BBT_PERCHIP */ > > + int numchips; > > + > > + /* Discourage new customer usages here; suggest usage of the > > + * relevant NAND_BBT_* options instead */ > > + struct nand_bbt_descr *bbt_td; > > + struct nand_bbt_descr *bbt_md; > > + > > + /* The rest are filled in by nand_bbt_init() */ > > + > > + u64 chipsize; > > + int chip_shift; > > + int bbt_erase_shift; > > + int page_shift; > > + > > + u8 *bbt; > > +}; > > Here the comments and description are in the structure definition. When > you look above at struct nand_bbt_descr then description of all fields > is above the structure definition. It would be good to keep a > consistent style of comments. I think the style of struct > nand_bbt_descr is more appropriate. > I will consider your suggestion on the v2 patch. Thanks for your suggestion. > > + > > +int nand_bbt_init(struct nand_bbt *bbt); > > +void nand_bbt_release(struct nand_bbt *bbt); > > +int nand_bbt_markbad(struct nand_bbt *bbt, loff_t offs); > > +int nand_bbt_isreserved(struct nand_bbt *bbt, loff_t offs); > > +int nand_bbt_isbad(struct nand_bbt *bbt, loff_t offs); > > + > > +#endif /* __LINUX_MTD_NAND_BBT_H */ > > -- > > 1.9.1 > > Best wishes, > Kamil Debski Thanks, Peter Pan ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?