Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1490942 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mtd: s3c2410: add device tree support Sergio Prado <sergio.prado@e-labworks.com> - 2016-09-25 19:50 +0200
    Re: [PATCH] mtd: s3c2410: add device tree support Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-09-25 20:10 +0200

#1490942 — Re: [PATCH] mtd: s3c2410: add device tree support

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?

> > +	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?

> > +	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?

Thanks!

-- 
Sergio Prado
Embedded Labworks
Office: +55 11 2628-3461
Mobile: +55 11 97123-3420

[toc] | [next] | [standalone]

#1490944

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web