Date:	Fri, 30 Jan 2015 00:57:25 +0000
From:	Peter Pan 潘栋 (peterpandong) 
	<peterpandong@...ron.com>
To:	Ezequiel Garcia <ezequiel.garcia@...tec.com>,
	Qi Wang 王起 (qiwang) <qiwang@...ron.com>,
	"Brian Norris" <computersforpeace@...il.com>
CC:	"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>
Subject: RE: [PATCH 0/3] An alternative to SPI NAND

> On 01/20/2015 11:11 PM, Qi Wang 王起 (qiwang) wrote:
> > On 01/20/2015 6:36 PM, Ezequiel Garcia wrote:
> >>
> >> On 01/12/2015 12:10 PM, Qi Wang 王起 (qiwang) wrote:
> >>> Hi Ezequiel,
> >>>
> >>> On 01/08/2015 11:27 AM, Ezequiel Garcia wrote:
> >>>>
> >>>> Hi Qi Wang,
> >>>>
> >>>> On 01/07/2015 11:45 PM, Qi Wang 王起 (qiwang) wrote:
> >>>>> Hi Brian,
> >>>>>
> >>>>> On Thu, Jan 08, 2015 at 9:03:24AM +0000, Brian Norris wrote:
> >>>>>>
> >>>>>> On Thu, Jan 08, 2015 at 12:47:24AM +0000, Peter Pan 潘栋
> (peterpandong)
> >>>>>> wrote:
> >>>>>>> Documentation/devicetree/bindings/mtd/spi-nand.txt |   22 +
> >>>>>>> drivers/mtd/Kconfig                                |    2 +
> >>>>>>> drivers/mtd/Makefile                               |    1 +
> >>>>>>> drivers/mtd/spi-nand/Kconfig                       |    7 +
> >>>>>>> drivers/mtd/spi-nand/Makefile                      |    3 +
> >>>>>>> drivers/mtd/spi-nand/spi-nand-base.c               | 2034
> >>>>>> ++++++++++++++++++++
> >>>>>>> drivers/mtd/spi-nand/spi-nand-bbt.c                | 1279
> >> ++++++++++++
> >>>>>>
> >>>>>> I can already tell by the diffstat that I don't like this. We
> probably
> >>>>>> don't need 3000 new lines of code for this, but we especially
> don't
> >> want
> >>>>>> to duplicate nand_bbt.c. It won't take a lot of work to augment
> >>>>>> nand_bbt.c to make it shareable. (I can whip that patch up if
> needed.)
> >>>>>
> >>>>> Yes, I agree with you, Nand_bbt.c do can be shared by Parallel
> NAND and
> >>>>> SPI NAND. Actually, we are working at this now. Will send patches
> to
> >> you
> >>>>> Once we finished it.
> >>>>>
> >>>>
> >>>> Thanks for the quick submission!
> >>>>
> >>>> However, Brian is right, this code duplication is a no go.
> >>>>
> >>>> Perhaps a more valid approach would be to first identify the code
> that
> >>>> needs to be shared in nand_bbt.c and nand_base.c, and export those
> >>>> symbols (or maybe do the required refactor).
> >>>
> >>> Yes, I agree Brian's suggestion in another mail.
> >>>
> >>> " The BBT code is something we definitely want to share, but it's
> >> actually
> >>> not very closely tied to nand_base.c, and it looks pretty easy to
> adapt
> >>> to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd
> just
> >>> need to parameterize a few relevant device details into a new
> nand_bbt
> >>> struct, rather than using struct nand_chip directly."
> >>>
> >>> To abstract a new nand_bbt struct instead of nand_chip to make SPI
> NAND
> >>> and parallel NAND can share nand_bbt.c file, I already begin to
> work on
> >>> this.
> >>>
> >>> For code shared in nand_base.c, I agree it would be better if we
> can find
> >>> a good method to share nand_base.c code between spi nand and
> parallel
> >> nand.
> >>> But frankly speaking, I'm not satisfied for the remap command
> method.
> >> This
> >>> method make code difficult to maintain when SPI NAND and Parallel
> NAND
> >>> evolve much differently in the future.
> >>>
> >>> Take some example,
> >>> If one new command (cache operation, multiple plane operation)
> >> implemented
> >>> in parallel NAND code, and is used in nand_read or nand_write, that
> will
> >>> cause maintainer to modify SPI NAND code to remap this new command,
> >> though
> >>> this modification probably could be slight. That means modification
> on
> >>> Parallel NAND flash need to consider SPI NAND as well.
> >>>
> >>> How do you think about this?
> >>>
> >>> For Peter Pan's patchset, if we do some modification to make
> nand_bbt.c
> >> to
> >>> make it shareable for Parallel and SPI NAND. The code line should
> be 2000.
> >>> I believe I can review this spi-nand-base.c to remove some
> redundant code
> >>> that may hundreds line. Is 1700 or 1800 code line is more
> acceptable?
> >>>
> >>> Let me know your opinions.
> >>>
> >>
> >> Sounds good.
> >>
> >> Do you still plan to maintain the spi-nand-base.c and spi-nand-
> device.c
> >> separation?
> >
> > Yes, still plan to maintain the spi-nand-base.c and spi-nand-device.c
> > separation. Abstract common code to spi-nand-base.c, and spi-nand-
> device.c is
> > used for realize the different function for different SPI NAND, such
> as ecc
> > layout, read ID etc.
> >
> 
> Any news about this? Is there anything I can do to help (reviewing,
> testing, coding...)?
> 
> Thanks!
> --
> Ezequiel

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. If put nand_bbt in nand_chip, we need to change the 
parameter of nand_chip->scan_bbt function from mtd_info to nand_bbt.
But nand_chip->scan_bbt is used in many place.
	drivers/mtd/nand/diskonchip.c:1372:     this->scan_bbt = nftl_scan_bbt;
	drivers/mtd/nand/diskonchip.c:1399:             this->scan_bbt = inftl_scan_bbt;
	drivers/mtd/nand/diskonchip.c:1405:             this->scan_bbt = nftl_scan_bbt;
	drivers/mtd/nand/diskonchip.c:1418:     this->scan_bbt = inftl_scan_bbt;
	drivers/mtd/nand/nand_base.c:2986:      if (!chip->scan_bbt)
	drivers/mtd/nand/nand_base.c:2987:              chip->scan_bbt = nand_default_bbt;
	drivers/mtd/nand/nand_base.c:4159:       * Initialize bitflip_threshold to its default prior scan_bbt() call.
	drivers/mtd/nand/nand_base.c:4160:       * scan_bbt() might invoke mtd_read(), thus bitflip_threshold must be
	drivers/mtd/nand/nand_base.c:4171:      return chip->scan_bbt(mtd);
	drivers/mtd/nand/gpmi-nand/gpmi-nand.c:1953:    chip->scan_bbt(mtd);
	drivers/mtd/nand/docg4.c:1083:   * first page of the block.  The default scan_bbt() in the nand
	drivers/mtd/nand/nandsim.c:2381:        if ((retval = chip->scan_bbt(nsmtd)) != 0)
	drivers/mtd/onenand/onenand_base.c:3976:        if (!this->scan_bbt)
	drivers/mtd/onenand/onenand_base.c:3977:                this->scan_bbt = onenand_default_bbt;
	drivers/mtd/onenand/onenand_base.c:4101:        ret = this->scan_bbt(mtd);
If put nand_bbt in mtd_info, we needn't to change the parameter of nand_chip->scan_bbt.
But only nand flash need bbt, I don't know whether it is proper to put nand_bbt structure
in mtd_info or not.

Besides, using nand_bbt struct will cause some elements(such as chip_size, page_size and so on)
in both nand_chip and nand_bbt struct.

Maybe there is another way but I don't know. So please feel free to talk about it.

Brain, could you give some suggestion ? We really need your help.

Powered by blists - more mailing lists