Date:	Fri, 25 Mar 2016 09:35:19 +0100
From:	Boris Brezillon <boris.brezillon@...e-electrons.com>
To:	Peter Pan <peterpansjtu@...il.com>
Cc:	computersforpeace@...il.com, dwmw2@...radead.org,
	linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org,
	karlzhang@...ron.com, beanhuo@...ron.com, xuejiancheng@...wei.com,
	Peter Pan <peterpandong@...ron.com>
Subject: Re: [PATCH 02/11] mtd: nand_bbt: introduce BBT related data
 structure

Hi Peter,

On Mon, 14 Mar 2016 02:47:55 +0000
Peter Pan <peterpansjtu@...il.com> wrote:

> From: Brian Norris <computersforpeace@...il.com>
> 
> 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).
> 
> Separate struct nand_chip from BBT code can make current BBT shareable.
> We create struct nand_bbt to take place of nand_chip in nand_bbt.c
> 
> Below is mtd folder structure we want:
> 	drivers/mtd/nand/<all-nand-core-code>
> 	drivers/mtd/nand/raw/<raw-nand-controller-drivers>
> 	drivers/mtd/nand/spi/<spi-nand-code>
> 	drivers/mtd/nand/onenand/<onenand-code>
> 	drivers/mtd/nand/chips/<manufacturer-spcific-code>
> 
> Of course, nand_bbt.c should be part of <all-nand-core-code>.
> 
> We put every chip layout related information BBT needed into struct
> nand_chip_layout_info.
> 	@numchips:	number of physical chips, required for NAND_BBT_PERCHIP
> 	@chipsize:	the size of one chip for multichip arrays
> 	@chip_shift:	number of address bits in one chip
> 	@bbt_erase_shift:	number of address bits in a bbt entry
> 	@page_shift:	number of address bits in a page
> 
> We defined a struct nand_bbt_ops for BBT ops. Struct
> 	@is_bad_bbm:	check if a block is factory bad block
> 	@erase:	erase block bypassing resvered checks
> 
> Struct nand_bbt includes all BBT information:
> 	@mtd:	pointer to MTD device structure
> 	@bbt_options:	bad block specific options. All options used
> 			here must come from nand_bbt.h.
> 	@bbt_ops:	struct nand_bbt_ops pointer.
> 	@info:		struct nand_chip_layout_info pointer.
> 	@bbt_td:	bad block table descriptor for flash lookup.
> 	@bbt_md:	bad block table mirror descriptor
> 	@bbt:		bad block table pointer
> 
> Signed-off-by: Brian Norris <computersforpeace@...il.com>
> [Peter: 1. correct comment style
> 	2. introduce struct nand_bbt_ops and nand_chip_layout_info]
> Signed-off-by: Peter Pan <peterpandong@...ron.com>
> ---
>  include/linux/mtd/nand_bbt.h | 67 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/include/linux/mtd/nand_bbt.h b/include/linux/mtd/nand_bbt.h
> index 5a65230..cfb22c8 100644
> --- a/include/linux/mtd/nand_bbt.h
> +++ b/include/linux/mtd/nand_bbt.h
> @@ -18,6 +18,8 @@
>  #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
>  
> @@ -115,4 +117,69 @@ struct nand_bbt_descr {
>  /* The maximum number of blocks to scan for a bbt */
>  #define NAND_BBT_SCAN_MAXBLOCKS	4
>  
> +struct nand_bbt;
> +
> +/**
> + * struct nand_bbt_ops - bad block table operations
> + * @is_bad_bbm:	check if a block is factory bad block
> + * @erase:	erase block bypassing resvered checks
> + */
> +struct nand_bbt_ops {
> +	/*
> +	 * 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);
> +
> +	/* Erase a block, bypassing reserved checks */
> +	int (*erase)(struct mtd_info *mtd, loff_t ofs);
> +};
> +
> +/**
> + * struct nand_chip_layout_info - strucure contains all chip layout
> + * information that BBT needed.
> + * @numchips:	number of physical chips, required for NAND_BBT_PERCHIP
> + * @chipsize:	the size of one chip for multichip arrays
> + * @chip_shift:	number of address bits in one chip
> + * @bbt_erase_shift:	number of address bits in a bbt entry
> + * @page_shift:	number of address bits in a page
> + */
> +struct nand_chip_layout_info {

I know I'm the one who suggested this name, but NAND datasheet seems to
call it "memory organization", so maybe we should rename this struct
nand_memory_organization.

> +	int numchips;

I would rename it numdies, or ndies. numchips implies you're having
several chips, which is not the case.

> +	u64 chipsize;

Ditto, s/chipsize/diesize/

> +	int chip_shift;

Ditto.

> +	int bbt_erase_shift;

Hm, this is not related to the memory organization. I'd prefer moving
this one directly in 

> +	int page_shift;
> +};

The structure should probably contain other info like (oob size, pages
per block, blocks per die, ...)
I know some of those information are redundant with mtd_info content,
but it would be clearer to have everything in a common place.

Also, I'd recommend using helpers to access memory organization info.
For example nand_get_die_size(mtd), nand_get_page_size(mtd), ...

On a more general note, as already said, I'd like to see more
generalization across NAND based devices, no matter the interface
they're using.
Doing that implies forcing all NAND based devices to inherit from a
common class. Something like

struct nand_device {
	struct mtd_info mtd;
	struct nand_memory_organization memorg;
	/* ... */
};

/* rawnand_device <-> nand_chip */
struct rawnand_device {
	struct nand_device base;
	/* raw NAND specific fields */
}

struct spinand_device {
	struct nand_device base;
	/* SPI NAND specific fields */
};

struct onenand_device {
	struct nand_device base;
	/* OneNAND specific fields */
};

With this design, nand_bbt and nand_bbt_ops could use the generic
nand_device instead of directly using the mtd instance.

Anyway, that's just a long term goal, and I wanted to share my
ideas. I guess your plan is to add support for SPI nand devices, so
keep this in mind ;-).

> +
> +/**
> + * struct nand_bbt - bad block table structure
> + * @mtd:	pointer to MTD device structure
> + * @bbt_options:	bad block specific options. All options used
> + *			here must come from nand_bbt.h.
> + * @bbt_ops:	struct nand_bbt_ops pointer.
> + * @info:	struct nand_chip_layout_info pointer.
> + * @bbt_td:	bad block table descriptor for flash lookup.
> + * @bbt_md:	bad block table mirror descriptor
> + * @bbt:	bad block table pointer
> + */
> +struct nand_bbt {
> +	struct mtd_info *mtd;
> +	unsigned int bbt_options;
> +	const struct nand_bbt_ops *bbt_ops;
> +	struct nand_chip_layout_info *info;
> +	/*
> +	 * Discourage new custom usages here; suggest usage of the
> +	 * relevant NAND_BBT_* options instead
> +	 */
> +	struct nand_bbt_descr *bbt_td;
> +	struct nand_bbt_descr *bbt_md;
> +	u8 *bbt;
> +};
> +
>  #endif	/* __LINUX_MTD_NAND_BBT_H */



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Powered by blists - more mailing lists