Re: [PATCH] mtd: s3c2410: add device tree support

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Sergio,

On Sun, 25 Sep 2016 14:42:57 -0300
Sergio Prado <sergio.prado@e-labworks.com> wrote:

> Hi Boris,
> 
> > > +Optional properties:
> > > +- samsung,tacls : time for active CLE/ALE to nWE/nOE
> > > +- samsung,twrph0 : active time for nWE/nOE
> > > +- samsung,twrph1 : time for release CLE/ALE from nWE/nOE inactive  
> > 
> > Can you try to extract these information from the nand_sdr_timings
> > struct instead of passing it in your DT?  
> 
> I've tested and the nand chip I'm working on is not ONFI or JEDEC
> compatible, so looks like it is not possible to extract the timing
> information from nand_sdr_timings. Am I right?

There's an default_onfi_timing_mode in the nand_flash_dev struct. You
can define a full-id for your NAND chip in the nand-ids table if you
want. Otherwise, this means you'll have to use ONFI timing mode 0
(which should work for all NANDs).

Note that we've recently introduced a generic interface [1] to let NAND
controllers configure the timings, and let the core select the best
timings based on the information it has (ONFI, JEDEC or the nand-ids
table).

> 
> > > +	samsung,ignore_unset_ecc;  
> > 
> > Just discovered this ->ignore_unset_ecc property, and I don't
> > understand why it's here for...
> > Either you don't want to have ECC, and in this case you should set
> > NAND_ECC_NONE, or you want to have ECC calculated, and in this case,
> > the only valid situation where ECC bytes are 0xff is when the page is
> > erased.
> > 
> > If I'm right, please fix the driver instead of adding this DT property.
> > If I'm wrong, could you explain in more detail when you have ECC bytes
> > set to 0xff?  
> 
> I think you are right but I am not an expert on flash devices and the
> MTD sub-system. The commit message of this code says "If a block's ecc
> field is all 0xff, then ignore the ECC correction. This is for systems
> where some of the blocks, such as the initial cramfs are written without
> ECC and need to be loaded on start.". Does it make sense?

Well, no, it does not make any sense. What could have a sense is
enabling ECC on some partitions, but not on others, but that's not
supported right now (actually, I tried to add support for that once,
but it was not accepted).

Do you really need that? I mean, is the platform you're trying to
convert to DT using this property in its platform_data objects?

I'd recommend to drop this property until we figure what it's really
used for.

> 
> > > +	for_each_available_child_of_node(np, child) {
> > > +
> > > +		sets->name = (char *)child->name;
> > > +		sets->of_node = child;
> > > +		sets->nr_chips = 1;
> > > +
> > > +		if (!of_property_match_string(child, "nand-ecc-mode", "none"))
> > > +			sets->disable_ecc = 1;
> > > +
> > > +		if (of_get_property(child, "nand-on-flash-bbt", NULL))
> > > +			sets->flash_bbt = 1;
> > > +  
> > 
> > These properties are automatically extracted in nand_scan_ident(), why
> > do you need to parse them twice?
> >   
> 
> You are right, there is no need to parse them twice. But taking a look
> at the code I found a problem. Right now enabling hardware ECC is done
> at compile time by enabling CONFIG_MTD_NAND_S3C2410_HWECC in the
> menuconfig. Looks like this config option should be removed so we can
> select ECC mode using nand-ecc-mode property in the device tree. But
> this would break current boards that are not using a device tree. So it
> would be necessary to add a ecc_mode field in the platform_data of these
> boards and set them all to the default ECC mode (NAND_ECC_SOFT). Is this
> the right approach?

It's the right approach, indeed.

Thanks,

Boris

[1]http://www.spinics.net/lists/arm-kernel/msg532007.html