Re: [RFC PATCH 0/3] mtd: nand: provide a clear separation of chip and controller ops

[Brian Norris <computersforpeace@gmail.com>]

On Tue, Apr 28, 2015 at 06:02:41PM +0200, Boris Brezillon wrote:
> On Fri, 24 Apr 2015 19:22:54 -0700
> Brian Norris <computersforpeace@gmail.com> wrote:
> > > There surely is other pros and cons to this approach, and I'm pretty sure
> > > you will point plenty of them.
> > 
> > At the moment (and, though I have taken I while to respond, I have
> > thought about this issue occasionally over the last two months) I don't
> > actually see a lot of problems with your current proposal. The code is
> > pretty trivial at the moment.
> > 
> > The problem might only be that this does not yet go far enough. A lot of
> > the controller-specific stuff ends up being related to the nand_ecc_ctrl
> > struct. Those function pointers essentially end up being the bulk of
> > controller-specific function pointers, in some cases, but those may even
> > vary from nand_chip to nand_chip. I haven't figured out what best to do
> > with these yet.
> 
> Actually I was planning to make the same separation for nand_ecc_ctrl.
> The ops should be part of another structure (nand_ecc_ctrl seems like
> a good container), while instantiation information (like the selected
> ECC strength/step size and the oob layout) should be assigned to the
> NAND chip itself.

Sounds about right.

> > (BTW, this source of problem makes it hard to deal with on-die/built-in
> > ECC too; we might need to make use of the controller-specific *raw* read
> > functions while still handling the on-die ECC status info. *hint hint
> > Richard* *I'll comment on your on-die ECC patches soon, I hope*)
> 
> Hum, I'm not sure the raw functions are in cause here, but maybe we
> should provide read/write page functions (those ones should access the
> NAND in raw mode of course) in the nand_controller_ops.
> 
> This brings us to another aspect I'd like to rework: the way we're
> attaching an ECC implementation to a NAND chip.
> Currently, the nand_chip struct embeds a nand_ecc_ctrl struct which is
> then filled by NAND controller drivers.
> I'd like to make this field a pointer, so that NAND controllers can
> provide an ECC implementation which can then be overloaded by nand core
> if on-die ECC is available and explicitly requested.

Could work. Though, the "overloading" would probably involve wrapping
the controller callbacks, not completely replacing. It would look
loosely like:

 mtd_read()
 |_ ... chip->read_something()
        |_ if (on-die)
	      hwctrl->read_something_raw()
	   else
	      hwctrl->read_something()

> I might be wrong, but all the attempt to support on-die ECC I've seen
> so far are involving ECC implementation selection in each NAND
> controller driver.

Yeah, and that's one reason I haven't merged any of them. They are very
much tied to specific drivers (or classes of drivers). They haven't
provisioned for any way to seamlessly add on-die ECC support to drivers
by, e.g., disabling existing hardware ECC, or using existing "raw"
support to build support for on-die ECC.

In considering options here, I think we have a few things to plan for:
1. one controller may support many types of ECC (SW, HW)
2. one controller may support many strengths for a particular type
(1-bit, 8-bit, 24-bit, etc.)
3. we might want to override all of the above in order to use on-die ECC
4. we might want to opt out of on-die ECC, even on those that support it
(on Micron I think you can disable it? but on Toshiba you can't)
5. different ECC schemes for different partitions? Some have requested
being able to do a HW (?) 1-bit ECC on the bootloader partition, and a
stronger SW or HW ECC on the rest.

#1 and #2 have been handled OK with a variety of driver-specific coding
in between nand_scan_ident() and nand_scan_tail(). But it's a bit too
much work to make every driver add more logic there for #3 and #4. So if
we're going to do that kind of work, we should plan something that
allows a little more modularity / replaceability.

#5 is a lot less important, but I thought I'd throw it in there, in case
it could help guide the thought process for reworking this now.

Maybe some kind of ecc_ctrl() or ecc_select() callback provided (at
least partially) by the controller? It could handle multiplexing the
various ECC modes, so that nand_base can configure them directly. (Or
maybe I'm off-base a bit. I'll admit I haven't 100% though this last
suggestion completely, though I've imagined something like that for a
while.)

Brian