On Mon, 28 Mar 2016 16:09:44 +0800 Peter Pan <peterpans...@gmail.com> wrote:
> Hi Boris, > > Firstly, thanks a lot for taking time to review my patches. > > On Fri, Mar 25, 2016 at 4:35 PM, Boris Brezillon > <boris.brezil...@free-electrons.com> wrote: > > Hi Peter, > > > > On Mon, 14 Mar 2016 02:47:55 +0000 > > Peter Pan <peterpans...@gmail.com> wrote: > > > >> From: Brian Norris <computersforpe...@gmail.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 <computersforpe...@gmail.com> > >> [Peter: 1. correct comment style > >> 2. introduce struct nand_bbt_ops and nand_chip_layout_info] > >> Signed-off-by: Peter Pan <peterpand...@micron.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. > > Fix this in v4 > > > >> + int numchips; > > > > I would rename it numdies, or ndies. numchips implies you're having > > several chips, which is not the case. > > Fix this in v4 > > > >> + u64 chipsize; > > > > Ditto, s/chipsize/diesize/ > > Fix this in v4 > > > >> + int chip_shift; > > > > Ditto. > > Fix this in v4 > > > >> + int bbt_erase_shift; > > > > Hm, this is not related to the memory organization. I'd prefer moving > > this one directly in > > Yes, I also realize bbt_erase_shift is not proper. How about just rename it > to erase_block_shift or block_shift ? eraseblock_shift sounds good as long as bbt_erase_shift == eraseblock_shift is always true. > > > > >> + 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 ;-). > > Acctually your idea is quite good. Actually, struct nand_chip_layout_info > shouldn't be in nand_bbt.h. It should be in nand.h or nand_base.h and embedded > in struct nand_chip (or struct nand_deivce as your said). Yes, I didn't comment on that since I don't want to create a nand_base.h header file. The idea is to rename nand.h into rawnand.h and then create a nand.h file containing all interface-independent stuff (like memory organization info). > The reason I did't do this is I feel it will be too involved. I need > to change almost > all files under mtd/nand/, which generates a larger patch set. Yes, I know, that's why I'm not asking that right now. But that would be great if we could prepare things for this move... -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com