[PATCH 0/2] mtd: nand: support ONFI timings mode retrieval for non-ONFI NANDs

[PATCH 0/2] mtd: nand: support ONFI timings mode retrieval for non-ONFI NANDs

[Boris BREZILLON]

Hi,

Following the series adding ONFI timings mode support, here is a series
adding support for timings mode retrieval on non-ONFI NANDs.

It just adds a new field to the nand_chip and nand_flash_dev struct so
that anyone can define its chip requirements in the nand_ids table.

The 2nd patch serves as an example and adds an entry for the Hynix
H27UCG8T2ATR-BC NAND chip used on the Cubietruck board.

Best Regards,

Boris

Boris BREZILLON (2):
  mtd: nand: support ONFI timing mode retrieval for non-ONFI NANDs
  mtd: nand: add Hynix's H27UCG8T2ATR-BC to nand_ids table

 drivers/mtd/nand/nand_base.c | 1 +
 drivers/mtd/nand/nand_ids.c  | 4 ++++
 include/linux/mtd/nand.h     | 7 +++++++
 3 files changed, 12 insertions(+)

-- 
1.8.3.2

[PATCH 1/2] mtd: nand: support ONFI timings mode retrieval for non-ONFI NANDs

[Boris BREZILLON]

Add an onfi_timing_mode_ds field to nand_chip and nand_flash_dev in order
to support NAND timings definition for non-ONFI NAND.

NAND that support better timings mode than the default one (timings mode 0)
have to define a new entry in the nand_ids table.

The timings mode should be deduced from timings description from the
datasheet and the ONFI specification
(www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf, chapter 4.15
"Timing Parameters").
You should choose the closest mode that fit the timings requirements of
your NAND chip.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c | 1 +
 include/linux/mtd/nand.h     | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index d8cdf06..c952c21 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3576,6 +3576,7 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->options |= type->options;
 		chip->ecc_strength_ds = NAND_ECC_STRENGTH(type);
 		chip->ecc_step_ds = NAND_ECC_STEP(type);
+		chip->onfi_timing_mode_ds = type->onfi_timing_mode_ds;
 
 		*busw = type->options & NAND_BUSWIDTH_16;
 
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 3083c53..435c005 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -587,6 +587,7 @@ struct nand_buffers {
  * @ecc_step_ds:	[INTERN] ECC step required by the @ecc_strength_ds,
  *                      also from the datasheet. It is the recommended ECC step
  *			size, if known; if unknown, set to zero.
+ * @onfi_timing_mode_ds:[INTERN] ONFI timing mode deduced from datasheet.
  * @numchips:		[INTERN] number of physical chips
  * @chipsize:		[INTERN] the size of one chip for multichip arrays
  * @pagemask:		[INTERN] page number mask = number of (pages / chip) - 1
@@ -671,6 +672,7 @@ struct nand_chip {
 	uint8_t bits_per_cell;
 	uint16_t ecc_strength_ds;
 	uint16_t ecc_step_ds;
+	int onfi_timing_mode_ds;
 	int badblockpos;
 	int badblockbits;
 
@@ -772,6 +774,10 @@ struct nand_chip {
  *               @ecc_step_ds in nand_chip{}, also from the datasheet.
  *               For example, the "4bit ECC for each 512Byte" can be set with
  *               NAND_ECC_INFO(4, 512).
+ * @onfi_timing_mode_ds: the ONFI timing mode supported by this NAND chip. This
+ *                       should be deduced from timings described in the
+ *                       datasheet.
+ *
  */
 struct nand_flash_dev {
 	char *name;
@@ -792,6 +798,7 @@ struct nand_flash_dev {
 		uint16_t strength_ds;
 		uint16_t step_ds;
 	} ecc;
+	int onfi_timing_mode_ds;
 };
 
 /**
-- 
1.8.3.2

[PATCH 2/2] mtd: nand: add Hynix's H27UCG8T2ATR-BC to nand_ids table

[Boris BREZILLON]

Add the full description of the Hynix H27UCG8T2ATR-BC NAND chip in the
nand_ids table so that we can later use the NAND ECC infos and ONFI timings
mode in controller drivers.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_ids.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
index 3d7c89f..19d4168 100644
--- a/drivers/mtd/nand/nand_ids.c
+++ b/drivers/mtd/nand/nand_ids.c
@@ -46,6 +46,10 @@ struct nand_flash_dev nand_flash_ids[] = {
 	{"SDTNRGAMA 64G 3.3V 8-bit",
 		{ .id = {0x45, 0xde, 0x94, 0x93, 0x76, 0x50} },
 		  SZ_16K, SZ_8K, SZ_4M, 0, 6, 1280, NAND_ECC_INFO(40, SZ_1K) },
+	{"H27UCG8T2ATR-BC 64G 3.3V 8-bit",
+		{ .id = {0xad, 0xde, 0x94, 0xda, 0x74, 0xc4} },
+		  SZ_8K, SZ_8K, SZ_4M, 0, 6, 640, NAND_ECC_INFO(40, SZ_1K),
+		  4 },
 
 	LEGACY_ID_NAND("NAND 4MiB 5V 8-bit",   0x6B, 4, SZ_8K, SP_OPTIONS),
 	LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE3, 4, SZ_8K, SP_OPTIONS),
-- 
1.8.3.2

Re: [PATCH 2/2] mtd: nand: add Hynix's H27UCG8T2ATR-BC to nand_ids table

[Boris BREZILLON]

On Mon, 28 Jul 2014 11:16:52 +0200
Boris BREZILLON <boris.brezillon@free-electrons.com> wrote:

> Add the full description of the Hynix H27UCG8T2ATR-BC NAND chip in the
> nand_ids table so that we can later use the NAND ECC infos and ONFI timings
> mode in controller drivers.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  drivers/mtd/nand/nand_ids.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
> index 3d7c89f..19d4168 100644
> --- a/drivers/mtd/nand/nand_ids.c
> +++ b/drivers/mtd/nand/nand_ids.c
> @@ -46,6 +46,10 @@ struct nand_flash_dev nand_flash_ids[] = {
>  	{"SDTNRGAMA 64G 3.3V 8-bit",
>  		{ .id = {0x45, 0xde, 0x94, 0x93, 0x76, 0x50} },
>  		  SZ_16K, SZ_8K, SZ_4M, 0, 6, 1280, NAND_ECC_INFO(40, SZ_1K) },
> +	{"H27UCG8T2ATR-BC 64G 3.3V 8-bit",
> +		{ .id = {0xad, 0xde, 0x94, 0xda, 0x74, 0xc4} },
> +		  SZ_8K, SZ_8K, SZ_4M, 0, 6, 640, NAND_ECC_INFO(40, SZ_1K),

There is a big mistake here ------^

It should be SZ_2M not SZ_4M, I'll fix that.

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/2] mtd: nand: support ONFI timings mode retrieval for non-ONFI NANDs

[Brian Norris]

Since you were asking about this series, I have a comment:

On Mon, Jul 28, 2014 at 11:16:51AM +0200, Boris BREZILLON wrote:
> Add an onfi_timing_mode_ds field to nand_chip and nand_flash_dev in order
> to support NAND timings definition for non-ONFI NAND.
> 
> NAND that support better timings mode than the default one (timings mode 0)
> have to define a new entry in the nand_ids table.
> 
> The timings mode should be deduced from timings description from the
> datasheet and the ONFI specification
> (www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf, chapter 4.15
> "Timing Parameters").
> You should choose the closest mode that fit the timings requirements of
> your NAND chip.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  drivers/mtd/nand/nand_base.c | 1 +
>  include/linux/mtd/nand.h     | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index d8cdf06..c952c21 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3576,6 +3576,7 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
>  		chip->options |= type->options;
>  		chip->ecc_strength_ds = NAND_ECC_STRENGTH(type);
>  		chip->ecc_step_ds = NAND_ECC_STEP(type);
> +		chip->onfi_timing_mode_ds = type->onfi_timing_mode_ds;
>  
>  		*busw = type->options & NAND_BUSWIDTH_16;
>  
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 3083c53..435c005 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -587,6 +587,7 @@ struct nand_buffers {
>   * @ecc_step_ds:	[INTERN] ECC step required by the @ecc_strength_ds,
>   *                      also from the datasheet. It is the recommended ECC step
>   *			size, if known; if unknown, set to zero.
> + * @onfi_timing_mode_ds:[INTERN] ONFI timing mode deduced from datasheet.

Might want at least one space between '[INTERN]' and 'ONFI'?

>   * @numchips:		[INTERN] number of physical chips
>   * @chipsize:		[INTERN] the size of one chip for multichip arrays
>   * @pagemask:		[INTERN] page number mask = number of (pages / chip) - 1
> @@ -671,6 +672,7 @@ struct nand_chip {
>  	uint8_t bits_per_cell;
>  	uint16_t ecc_strength_ds;
>  	uint16_t ecc_step_ds;
> +	int onfi_timing_mode_ds;

I'm not sure if we'll just want a field specific to non-ONFI chips,
faking an ONFI timing mode; can we make this into a more generally
useful field, that represents something consistent across all NAND
types? I was thinking a "highest mode supported", but that actually
isn't great, since for true ONFI modes (except mode 0) require a SET
FEATURES command to change to a higher mode.

Maybe just something like onfi_timing_mode_default, which we could then
use as mode 0 for ONFI chips.

Ideally, we could centralize as much of this as possible, so that a NAND
driver only has to know the mechanics of "how do I translate an ONFI
mode to a timing configuration for my NAND controller," instead of any
details of how to choose from one of many supported ONFI modes. i.e., it
could implement something like the following hook:

	int (*set_timing_mode)(struct nand_sdr_timings *timing);

But anyway, that's out of the scope of this series, and it may be harder
than I make it sound. I just wanted to throw that out there.

>  	int badblockpos;
>  	int badblockbits;
>  
> @@ -772,6 +774,10 @@ struct nand_chip {
>   *               @ecc_step_ds in nand_chip{}, also from the datasheet.
>   *               For example, the "4bit ECC for each 512Byte" can be set with
>   *               NAND_ECC_INFO(4, 512).
> + * @onfi_timing_mode_ds: the ONFI timing mode supported by this NAND chip. This
> + *                       should be deduced from timings described in the
> + *                       datasheet.
> + *
>   */
>  struct nand_flash_dev {
>  	char *name;
> @@ -792,6 +798,7 @@ struct nand_flash_dev {
>  		uint16_t strength_ds;
>  		uint16_t step_ds;
>  	} ecc;
> +	int onfi_timing_mode_ds;
>  };
>  
>  /**

Brian

Re: [PATCH 1/2] mtd: nand: support ONFI timings mode retrieval for non-ONFI NANDs

[Boris BREZILLON]

On Fri, 19 Sep 2014 21:46:07 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> Since you were asking about this series, I have a comment:
> 
> On Mon, Jul 28, 2014 at 11:16:51AM +0200, Boris BREZILLON wrote:
> > Add an onfi_timing_mode_ds field to nand_chip and nand_flash_dev in order
> > to support NAND timings definition for non-ONFI NAND.
> > 
> > NAND that support better timings mode than the default one (timings mode 0)
> > have to define a new entry in the nand_ids table.
> > 
> > The timings mode should be deduced from timings description from the
> > datasheet and the ONFI specification
> > (www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf, chapter 4.15
> > "Timing Parameters").
> > You should choose the closest mode that fit the timings requirements of
> > your NAND chip.
> > 
> > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/mtd/nand/nand_base.c | 1 +
> >  include/linux/mtd/nand.h     | 7 +++++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index d8cdf06..c952c21 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -3576,6 +3576,7 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
> >  		chip->options |= type->options;
> >  		chip->ecc_strength_ds = NAND_ECC_STRENGTH(type);
> >  		chip->ecc_step_ds = NAND_ECC_STEP(type);
> > +		chip->onfi_timing_mode_ds = type->onfi_timing_mode_ds;
> >  
> >  		*busw = type->options & NAND_BUSWIDTH_16;
> >  
> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > index 3083c53..435c005 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h
> > @@ -587,6 +587,7 @@ struct nand_buffers {
> >   * @ecc_step_ds:	[INTERN] ECC step required by the @ecc_strength_ds,
> >   *                      also from the datasheet. It is the recommended ECC step
> >   *			size, if known; if unknown, set to zero.
> > + * @onfi_timing_mode_ds:[INTERN] ONFI timing mode deduced from datasheet.
> 
> Might want at least one space between '[INTERN]' and 'ONFI'?

You mean between the colon and '[INTERN]', right ?

> 
> >   * @numchips:		[INTERN] number of physical chips
> >   * @chipsize:		[INTERN] the size of one chip for multichip arrays
> >   * @pagemask:		[INTERN] page number mask = number of (pages / chip) - 1
> > @@ -671,6 +672,7 @@ struct nand_chip {
> >  	uint8_t bits_per_cell;
> >  	uint16_t ecc_strength_ds;
> >  	uint16_t ecc_step_ds;
> > +	int onfi_timing_mode_ds;
> 
> I'm not sure if we'll just want a field specific to non-ONFI chips,
> faking an ONFI timing mode; can we make this into a more generally
> useful field, that represents something consistent across all NAND
> types? I was thinking a "highest mode supported", but that actually
> isn't great, since for true ONFI modes (except mode 0) require a SET
> FEATURES command to change to a higher mode.
> 
> Maybe just something like onfi_timing_mode_default, which we could then
> use as mode 0 for ONFI chips.
> 
> Ideally, we could centralize as much of this as possible, so that a NAND
> driver only has to know the mechanics of "how do I translate an ONFI
> mode to a timing configuration for my NAND controller,"

I guess you meant "how do I translate a NAND timing config given by
nand_sdr_timings to my specific NAND controller config", right ?

> instead of any
> details of how to choose from one of many supported ONFI modes. i.e., it
> could implement something like the following hook:
> 
> 	int (*set_timing_mode)(struct nand_sdr_timings *timing);

I'm not opposed to this solution (I actually think this is how it
should be done :-), and, IIRC, Jason proposed to do the same kind of
things a while ago), but this means the conversion would be done by the
NAND FW and passed to the NAND driver, and this implies reworking the
drivers already directly using ONFI modes to configure their NAND
timings to make use nand_sdr_timings instead.

Moreover, I think we should define a union to handle several kind of
timings (ddr NAND are coming :-P):

enum nand_timings_type /* or nand_bus_type or something else ;-) */ {
	SDR_NAND,
	DDR_NAND,
}

struct nand_timings {
	enum nand_timings_type type;
	union {
		struct nand_sdr_timings sdr;
		struct nand_ddr_timings ddr;
	} timings;
};

But this is another story ;-).

> 
> But anyway, that's out of the scope of this series, and it may be harder
> than I make it sound. I just wanted to throw that out there.

If you don't mind I'd like to get something simple first: just renaming
onfi_timing_mode_ds is fine, but adding the whole conversion stuff to
the core implies much more changes. And I can work on a more
integrated solution as a second step.

Let me know what you think. 

Best Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/2] mtd: nand: support ONFI timings mode retrieval for non-ONFI NANDs

[Brian Norris]

On Sat, Sep 20, 2014 at 01:29:43PM +0200, Boris BREZILLON wrote:
> On Fri, 19 Sep 2014 21:46:07 -0700
> Brian Norris <computersforpeace@gmail.com> wrote:
> 
> > Since you were asking about this series, I have a comment:
> > 
> > On Mon, Jul 28, 2014 at 11:16:51AM +0200, Boris BREZILLON wrote:
> > > Add an onfi_timing_mode_ds field to nand_chip and nand_flash_dev in order
> > > to support NAND timings definition for non-ONFI NAND.
> > > 
> > > NAND that support better timings mode than the default one (timings mode 0)
> > > have to define a new entry in the nand_ids table.
> > > 
> > > The timings mode should be deduced from timings description from the
> > > datasheet and the ONFI specification
> > > (www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf, chapter 4.15
> > > "Timing Parameters").
> > > You should choose the closest mode that fit the timings requirements of
> > > your NAND chip.
> > > 
> > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > > ---
> > >  drivers/mtd/nand/nand_base.c | 1 +
> > >  include/linux/mtd/nand.h     | 7 +++++++
> > >  2 files changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > > index d8cdf06..c952c21 100644
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -3576,6 +3576,7 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
> > >  		chip->options |= type->options;
> > >  		chip->ecc_strength_ds = NAND_ECC_STRENGTH(type);
> > >  		chip->ecc_step_ds = NAND_ECC_STEP(type);
> > > +		chip->onfi_timing_mode_ds = type->onfi_timing_mode_ds;
> > >  
> > >  		*busw = type->options & NAND_BUSWIDTH_16;
> > >  
> > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > > index 3083c53..435c005 100644
> > > --- a/include/linux/mtd/nand.h
> > > +++ b/include/linux/mtd/nand.h
> > > @@ -587,6 +587,7 @@ struct nand_buffers {
> > >   * @ecc_step_ds:	[INTERN] ECC step required by the @ecc_strength_ds,
> > >   *                      also from the datasheet. It is the recommended ECC step
> > >   *			size, if known; if unknown, set to zero.
> > > + * @onfi_timing_mode_ds:[INTERN] ONFI timing mode deduced from datasheet.
> > 
> > Might want at least one space between '[INTERN]' and 'ONFI'?
> 
> You mean between the colon and '[INTERN]', right ?

Yep! My mistake.

> > 
> > >   * @numchips:		[INTERN] number of physical chips
> > >   * @chipsize:		[INTERN] the size of one chip for multichip arrays
> > >   * @pagemask:		[INTERN] page number mask = number of (pages / chip) - 1
> > > @@ -671,6 +672,7 @@ struct nand_chip {
> > >  	uint8_t bits_per_cell;
> > >  	uint16_t ecc_strength_ds;
> > >  	uint16_t ecc_step_ds;
> > > +	int onfi_timing_mode_ds;
> > 
> > I'm not sure if we'll just want a field specific to non-ONFI chips,
> > faking an ONFI timing mode; can we make this into a more generally
> > useful field, that represents something consistent across all NAND
> > types? I was thinking a "highest mode supported", but that actually
> > isn't great, since for true ONFI modes (except mode 0) require a SET
> > FEATURES command to change to a higher mode.
> > 
> > Maybe just something like onfi_timing_mode_default, which we could then
> > use as mode 0 for ONFI chips.
> > 
> > Ideally, we could centralize as much of this as possible, so that a NAND
> > driver only has to know the mechanics of "how do I translate an ONFI
> > mode to a timing configuration for my NAND controller,"
> 
> I guess you meant "how do I translate a NAND timing config given by
> nand_sdr_timings to my specific NAND controller config", right ?

Yes.

> > instead of any
> > details of how to choose from one of many supported ONFI modes. i.e., it
> > could implement something like the following hook:
> > 
> > 	int (*set_timing_mode)(struct nand_sdr_timings *timing);
> 
> I'm not opposed to this solution (I actually think this is how it
> should be done :-), and, IIRC, Jason proposed to do the same kind of
> things a while ago), but this means the conversion would be done by the
> NAND FW and passed to the NAND driver, and this implies reworking the
> drivers already directly using ONFI modes to configure their NAND
> timings to make use nand_sdr_timings instead.

I think we can safely ignore most of how drivers already configured
timings (I see at least denali.c with its own custom boot parameter),
and try to work out a solution that can be used by more than one driver
eventually. Conversion may or may not happen for some drivers, so we
should plan to have some sort of opt-in or opt-out ability.

> Moreover, I think we should define a union to handle several kind of
> timings (ddr NAND are coming :-P):
> 
> enum nand_timings_type /* or nand_bus_type or something else ;-) */ {
> 	SDR_NAND,
> 	DDR_NAND,
> }
> 
> struct nand_timings {
> 	enum nand_timings_type type;
> 	union {
> 		struct nand_sdr_timings sdr;
> 		struct nand_ddr_timings ddr;
> 	} timings;
> };

Perhaps. Although it's not immediately clear to me why we would need to
represent them in the same struct; I'm pretty sure we will have some
controllers that can handle one but not the other, so we might just have
two independent interfaces, and no need for the enum.

But anyway, I'm not 100% clear on how this would look, until one of us
provides a patch set, and we can work from there.

> But this is another story ;-).

Yes, I think so.

> > 
> > But anyway, that's out of the scope of this series, and it may be harder
> > than I make it sound. I just wanted to throw that out there.
> 
> If you don't mind I'd like to get something simple first: just renaming
> onfi_timing_mode_ds is fine, but adding the whole conversion stuff to
> the core implies much more changes. And I can work on a more
> integrated solution as a second step.

Sounds good. I really didn't expect much modification to this patch set,
but since you needed to fix the second patch, I though I'd give you my
nitpicks too.

One last random thought: if we ever integrate the SET_FEATURES commands
to set timing modes (async or synchronous ONFI modes) in the core code,
I think we'll need to ensure that we resend the SET_FEATURES command
after any NAND_CMD_RESET, as well as after any resume from suspend
(nand_resume()?).

Brian