GPMI-NAND: Wrong ECC size in driver

[Brian Norris]

On Wed, Jan 4, 2012 at 3:48 PM, Scott Wood <scottwood at freescale.com> wrote:
> On 01/04/2012 05:38 PM, Marek Vasut wrote:
>> Scott Wood wrote:
>>> On 01/04/2012 03:32 PM, Marek Vasut wrote:
>>>> Scott Wood wrote:
>>>>> Can we just get rid of NAND_CHIPOPTIONS_MSK and trust that drivers won't
>>>>> set options that aren't appropriate?  Possibly replace it with
>>>>> documentation about which options are for chips, which are for drivers,
>>>>> and which (such as NAND_NO_SUBPAGE_WRITE) can be set by either.
>>>>
>>>> Rather let's just adjust the mask?
>>>
>>> The way the mask is used means that any given option can only be chip or
>>> driver, not both.  Though, I don't see anywhere this option is set by a
>>> chip -- maybe we can just renumber it to be in the controller half.
>>
>> I suspect this was meant to allow controllers where one chip can do subpage-
>> write and the other can not.
>
> What I mean is I don't see any place where this is actually used --
> gpmi-nand is the only place I see this flag being set (though it belongs
> on at least elbc as well).

It's never actually set per-chip (i.e., it's not in the nand_ids table
or similar), so I don't see a problem with Scott's suggestion: "maybe
we can just renumber it to be in the controller half." But that may
not be the best way (see below)

>>> I still don't see a whole lot of value in the mask, though -- seems to
>>> be just causing problems, especially given that bits set by the "wrong"
>>> component are silently discarded.
>>
>> Yes. Patch is welcome to remove it and separate these two ;-)
>
> Do they need to be separated?  Just OR them with no mask.  If either the
> controller or the chip set the flag, then it's set, and subpage writes
> are disallowed.  Obviously you can only do this for certain types of
> options -- for others, just don't set the flag in an inappropriate context.

It looks like this might work as well. Now that I look at the code a
bit, it seems that there's at least one other option
(NAND_NO_AUTOINCR) which is sometimes set by the controller *and*
per-chip (in nand_ids.c) but the per-chip option will always be
silently masked out in nand_base.c:

  chip->options |= type->options & NAND_CHIPOPTIONS_MSK;

But it also seems that NAND_NO_AUTOINCR has no effect in the kernel
anyway... (seems like we should just kill the option, then, right? But
I digress...)

So there seems to be more than one issue related to a mostly
unnecessary mask, and unless there's a strong reason against it, I
like Scott's last suggestions a little better ("Just OR them with no
mask" and "just don't set the flag in an inappropriate context").

Brian