Re: [REV3] mtd: nand: Prepare for Micron on-die ECC controller support.

[Gerhard Sittig <gsi@denx.de>]

On Mon, 2014-03-31 at 14:58 -0600, David Mosberger wrote:
> 
> Gerhard,
> 
> On Mon, Mar 31, 2014 at 1:45 PM, Gerhard Sittig <gsi@denx.de> wrote:
> 
> > Isn't there the ONFI SETFEATURE request?  And don't you use this
> > very request to disable and enable chip internal ECC support, to
> > get the raw (uncorrected) bits after bitflips were detected?  So
> > I understand that there is support to enable this mode at will,
> > and we already have (or will have) code to do so.
> >
> > If we consider disabling on-die-ECC support when we find it
> > enabled and know it's not what the user wants to run, then the
> > next logical step might be to support enabling this feature if
> > the user wants it and the chip doesn't have it enabled at this
> > point in time.
> >
> > So I guess the buffers should get allocated as soon as the chip
> > supports on-die-ECC.  (While the allocation should only get
> > introduced in the phase where the raw-read and bitflip count gets
> > added.)
> 
> The ECC-mode of the driver never changes.
> Sure, what you describe could be implemented as an additional feature
> in the future (I don't have any plans to do so since it makes little sense to
> me, as no other ECC-mode uses the layout of the on-die-controller).

I'm sorry, it appears that I was unspecific or sloppy there.

The situation is that there are several phases in the setup:
There is the identification of the chip and its capabilities, an
optional adjustment of pre-set values according to the
information that was gathered, and then the "final" setup after
everything was evaluated.  This (in my mind) maps to the sequence
of nand_scan_ident(), nand_scan_tail(), and what might glue them
together.

So yes, after the ECC scheme was determined, it won't change for
the remainder of the driver's runtime.  Yet there is the code in
your submission that a pre-set software ECC would "turn into"
on-die-ECC if the chip was found in a specific state.  There has
been discussion that we might not blindly follow the chip's
current state, but involve a user's request and potentially
adjust what was found if it's different.

So please drop my thought of chaning ECC mode at runtime, after
the initial determination of which mode to use.  This really does
not apply.  The only change that might occur would be to
enable/disable the chip's feature if it differs from the user's
request, and this would happen before resource allocation in
nand_scan_tail().  At the very point of resource allocation, the
situation is stable.  Having determined the use of on-die-ECC
here at this point in time has involved all of the "supported",
"enabled", and "wanted" conditions.

I'm sorry for the confusion that I may have caused.


> To be honest, I'm a bit confused: on the one hand, you're asking me to
> simplify the patches into smaller pieces, on the other you seem to ask
> for new features.

This might look like a conflict only at first glance.  But I feel
that you can construct larger things with higher confidence if
you can inspect and verify individual pieces which you then snug
together.  If you know that the foundation is solid, you can put
more stuff on it with less worry.  Nothing in the kernel started
out at the complexity that's present now.  So no, splitting a
large patch into a series does not conflict with growing more
features within the resulting series.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de