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

[Kamil Debski]

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 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.

> +
> +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
N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå