RE: [PATCH 1/1] mtd: nand_bbt: separate struct nand_chip from nand_bbt.c

[Peter Pan ææ (peterpandong)]

Hi Kamil,

First of all, thanks for your great contribution.

On Jun 25, 2015 at 20:44 PM, Kamil Debski<Kamil.Debski@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx] 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
> > <computersforpeace@xxxxxxxxx>.
> > http://git.infradead.org/users/norris/linux-
> > mtd.git/shortlog/refs/heads/nand-bbt
> >
> > Signed-off-by: Peter Pan <peterpandong@xxxxxxxxxx>
> > Signed-off-by: Brian Norris <computersforpeace@xxxxxxxxx>
> 
> Tested-by: Kamil Debski <kamil.debski@xxxxxxxxxx>
> 
> > ---
> >  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 <computersforpeace@xxxxxxxxx>
> > + *
> > + * 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‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå