Re: [PATCH v3 22/28] mtd: nand: pxa3xx: Introduce multiple page I/O support

[Ezequiel Garcia <ezequiel.garcia@free-electrons.com>]

On Mon, Nov 18, 2013 at 10:10:09AM -0800, Brian Norris wrote:
> Hi Ezequiel,
> 
> There's one question of yours that I think hasn't been addressed:
> 
> On Wed, Nov 06, 2013 at 08:32:11AM -0300, Ezequiel Garcia wrote:
> > Having followed (part) of the OMAP DT ECC discussion, I'm thinking:
> > wouldn't it be good to have an 'nand-ecc-strength' property in the DT?
> > 
> > It would match the ecc_strength_ds field. This way, the ECC mode
> > selection is left to the driver and not to the user.
> > On the other side, this can cause some severe incompatibilities
> > unless such driver *always* make the *same* selection.
> 
> I'm not quite so sure about the whole question of ECC in device tree.
> There seems to be 2 subtly different properties we might want to
> capture:
> 
> 1) What is the minimum ECC required for the flash?
> 
> 2) What is the exact ECC layout/strength/type used for this flash?
>    (i.e., what is the bootloader using?)
> 
> The first is quite generic: the property consists of a stength and step
> size. (But this is also duplicated in ONFI, and our full-ID device
> table.)
> 
> Still, I think a property for #1 could be useful, for those chips that
> are not discoverable. And if we (you?) add it, it should be done at the
> nand_base level, I think, so its binding is shared by all drivers.
> 
> I'm not quite sure how we identify the appropriate struct device_node
> for nand_base to use, though. Maybe we should force drivers to
> initialize a new mtd_info.of_node field? And then maybe this could also
> be integrated into the 'ofpart' parser, which currently requires drivers
> to pass a device_node via the mtd_part_parser_data struct?
> 
> Property #2 is very driver/hardware specific, and it may not be easy to
> capture this information properly using the same set of properties for
> all NAND drivers. For example, "BCH-8" is not the same on all systems;
> and even on the same system a software BCH-X could potentially be very
> different than (and incompatible with) a BCH-X as provided by hardware.
> And different hardware provides wildly different choices regarding ECC,
> so I'm not convinced that we could create a good generic binding for
> describing #2.
> 
> But I think a property like #2 is necessary for many platforms that need
> to eliminate the problem that you mention, where drivers must always
> make the same selection. Essentially, we're assuming bootloader/driver
> co-design, rather than properly communicating this information via a
> shared data structure like device tree.
> 
> Now, it's another question as to whether we need a property for both #1
> and #2. The latter would probably just override the former, but that
> doesn't mean that the former is unnecessary...
> 

I completely agree with all of the above, but I'm still a bit uncertain
about how useful implementing #1 would be.

As you say, encoding the specific (per-driver) ECC information in the
devicetree seems the safest way of dealing with that.

On the other side, I fail to clearly see a valid use case for reporting
the "minimum" ECC required strength in the devicetree.

If I'm not missing anything, then I'd say just implement #2, for each driver
that needs it. I know that pxa3xx-nand should have it to avoid future issues.
This item is on top of my NAND TODO list.

Thanks for following the discussion!
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com