Date:	Mon, 2 Feb 2015 01:53:49 +0000
From:	Peter Pan 潘栋 (peterpandong) 
	<peterpandong@...ron.com>
To:	Brian Norris <computersforpeace@...il.com>,
	Ezequiel Garcia <ezequiel.garcia@...tec.com>
CC:	Qi Wang 王起 (qiwang) <qiwang@...ron.com>,
	"dwmw2@...radead.org" <dwmw2@...radead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	Frank Liu 刘群 (frankliu) 
	<frankliu@...ron.com>,
	Melanie Zhang 张燕 (melaniezhang) 
	<melaniezhang@...ron.com>,
	James Hartley <james.hartley@...tec.com>,
	"Andrew Bresticker" <abrestic@...omium.org>
Subject: RE: [PATCH 0/3] An alternative to SPI NAND

On Sat, Jan 31, 2015 at 15:02:29AM -0300, Brian Norris wrote:
> 
> On Fri, Jan 30, 2015 at 08:47:29AM -0300, Ezequiel Garcia wrote:
> > On 01/29/2015 09:57 PM, Peter Pan 潘栋 (peterpandong) wrote:
> > [..]
> > >
> > > Currently, we are working on sharing the bbt code. I think your and
> Brain's suggestion
> > > will be very helpful.
> > >
> > > There are two options. We can put struct nand_bbt pointer in either
> nand_chip
> > > or mtd_info structure.
> >
> > I'm sorry, you lost me here. Do you mean struct nand_bbt_descr ?
> 
> I'm assuming he's suggesting a new struct that he's calling nand_bbt.
> 
> > > If put nand_bbt in nand_chip, we need to change the
> >
> > I thought the plan was NOT to base spi-nand on nand, so you can't put
> > this in nand_chip, can you?
> 
> You could. It's just a matter of who does the pointer chasing. (It's
> just important that nand_bbt.c doesn't *assume* that it has nand_chip
> as
> a parent.)
> 
> If struct nand_bbt is embedded directly in struct mtd_info, then
> nand_bbt.c could implement functions such that its users can just do
> this:
> 
> 	mtd->_block_isbad = nand_bbt_blockisbad;
> 
> But if you don't (and instead embed it in a custom location like
> nand_chip), we'd have to write wrappers that call the nand_bbt_*
> functions (like nand_base does today). e.g.:
> 
> 	mtd->_block_isbad = spi_nand_blockisbad;
> 
> int spi_nand_block_isbad(struct mtd_info *mtd, loff_t offs)
> {
> 	struct spi_nand_chip *chip = mtd->priv;
> 	struct nand_bbt *bbt = chip->bbt;
> 
> 	return nand_bbt_blockisbad(bbt, offs);
> }
> 
> Anyway, I think it's worth laying out exactly where the pieces are
> going
> to fit here. I reckon we need the following:
> 
>  1. a short list of parameters that nand_bbt needs from the NAND
>  implementation (either nand_base or spi-nand-*)
> 
>  2. a list of parameters that need to be kept around for nand_bbt
>  bookkeeping / handling
> 
>  3. an init function (nand_bbt_init()?) that takes #1 and computes #2
> 
>  3(a) a corresponding release function
> 
>  4. a set of functions that, given only a few obvious parameters (e.g.,
>  block offsets) and a struct that holds #2, handle all BBT needs (e.g.,
>  bad block lookup, bad block scanning, marking new bad blocks)
> 
> Since I see #1 and #2 overlapping quite a bit, we might combine them,
> where several parameters are expected to be set by the nand_bbt user
> (either nand_base.c or nand_bbt.c), and a few are considered private to
> nand_bbt.
> 
> 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);
> 
> 	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() */
> 
> 	int bbt_erase_shift;
> 	int page_shift;
> 
> 	u8 *bbt;
> };
> 
> #3 might simply look like:
> 
> int nand_bbt_init(struct nand_bbt *bbt);
> void nand_bbt_release(struct nand_bbt *bbt);
> 
> #4 might look like the following APIs for nand_bbt.c (not too different
> than we have now):
> 
> int nand_bbt_init(struct nand_bbt *bbt); /* init + scan? */
> 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);
> 
> (The above allows for nand_bbt to be embedded anywhere; we could modify
> this API to be drop-in replacements for the mtd_info callbacks if we
> embed struct nand_bbt in mtd_info.)
> 
> > Also: After looking at the nand_bbt.c file, I'm wondering how
> promising
> > is to separate it from NAND. It seems the BBT code is quite attached
> to
> > NAND!
> 
> There will be quite a bit of churn, but I don't think that it is too
> strongly attached.
> 
> > Are you planning to do this in just one patch? Maybe it's better to
> > start simple and prepare small patches that gradually make the
> > nand_base.c and nand_bbt.c files less dependent.
> 
> Yeah, a series of patches would be best. And maybe start smaller.
> 
> If the nand_bbt stuff really slows someone down, we might want to just
> agree on an API similar to the above and then work on the rest of the
> SPI NAND details, assuming someone (e.g., me) writes the implementation
> (and transitions other drivers to do this).
> 
> Unless someone else thinks they have an excellent handle on the above,
> I'll take a stab at doing this. I actually have a few patches that have
> hung around a while (with little incentive to finish them) that do
> parts
> of what I'm suggesting above.
> 

I'm glad to hear that there are already some patches about your suggestion
above. I know you are quite busy. Maybe I can do some help if you need.

Peter

Powered by blists - more mailing lists