RE: [PATCH 0/3] An alternative to SPI NAND

Peter Pan 潘栋 (peterpandong)
2015-02-02T01:53:49Z

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