Patchwork [RFC,v2,04/14] mtd: nand: define struct nand_timings

login
register
mail settings
Submitter Boris BREZILLON
Date Jan. 29, 2014, 2:34 p.m.
Message ID <1391006064-28890-5-git-send-email-b.brezillon.dev@gmail.com>
Download mbox | patch
Permalink /patch/315069/
State New
Headers show

Comments

Boris BREZILLON - Jan. 29, 2014, 2:34 p.m.
Define a struct containing the standard NAND timings as described in NAND
datasheets.

Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
---
 include/linux/mtd/nand.h |   49 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
Boris BREZILLON - March 10, 2014, 1:44 p.m.
Hello,

Le 29/01/2014 15:34, Boris BREZILLON a écrit :
> Define a struct containing the standard NAND timings as described in NAND
> datasheets.
>
> Signed-off-by: Boris BREZILLON <b.brezillon.dev@gmail.com>
> ---
>   include/linux/mtd/nand.h |   49 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 49 insertions(+)
>
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 9e6c8f9..67f0829 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -805,4 +805,53 @@ static inline bool nand_is_slc(struct nand_chip *chip)
>   {
>   	return chip->bits_per_cell == 1;
>   }
> +
> +/**
> + * struct nand_sdr_timings - SDR NAND chip timings
> + *
> + * This struct defines the timing requirements of a SDR NAND chip.
> + * These informations can be found in every NAND datasheets and the timings
> + * meaning are described in the ONFI specifications:
> + * www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf‎ (chapter 4.15 Timing
> + * Parameters)
> + *
> + */
> +
> +struct nand_sdr_timings {
> +	u32 tALH_min;
> +	u32 tADL_min;
> +	u32 tALS_min;
> +	u32 tAR_min;
> +	u32 tCEA_max;
> +	u32 tCEH_min;
> +	u32 tCH_min;
> +	u32 tCHZ_max;
> +	u32 tCLH_min;
> +	u32 tCLR_min;
> +	u32 tCLS_min;
> +	u32 tCOH_min;
> +	u32 tCS_min;
> +	u32 tDH_min;
> +	u32 tDS_min;
> +	u32 tFEAT_max;
> +	u32 tIR_min;
> +	u32 tITC_max;
> +	u32 tRC_min;
> +	u32 tREA_max;
> +	u32 tREH_min;
> +	u32 tRHOH_min;
> +	u32 tRHW_min;
> +	u32 tRHZ_max;
> +	u32 tRLOH_min;
> +	u32 tRP_min;
> +	u32 tRR_min;
> +	u64 tRST_max;
> +	u32 tWB_max;
> +	u32 tWC_min;
> +	u32 tWH_min;
> +	u32 tWHR_min;
> +	u32 tWP_min;
> +	u32 tWW_min;
> +};

Some timings are missing here (see Table 55 in the ONFI spec):

-tR
-tBERS
-tCCS
-tPLEBSY
-...

I see at least 3 of those timings that could be useful (for the moment) :
- tR: this one should be used to fill the chip_delay field
- tPROG and tBERS: could be used within nand_wait to choose the timeo
   value appropriately.

The problem is that these timings cannot be deduced from the ONFI timing 
mode
but should rather be extracted from other ONFI parameters or from 
specific DT
properties (hence why I didn't add them to the nand_sdr_timings struct 
int the
first place).

Should I add these fields to the nand_sdr_timings struct and change the way
onfi_async_timing_mode_to_sdr_timings works (fill a nand timing struct 
passed
as an argument instead of returning a const pointer) ?
Or should I create a new struct for these timings ?
In the latter case how should I name it ?

Best Regards,

Boris

> +
>   #endif /* __LINUX_MTD_NAND_H */
Jason Gunthorpe - March 11, 2014, 6:55 p.m.
On Mon, Mar 10, 2014 at 02:44:04PM +0100, Boris BREZILLON wrote:

> Some timings are missing here (see Table 55 in the ONFI spec):

Right..

The 'mode' covers only the raw electrical parameters needed to
exchange commands, other timings cover the commands
themselves. Notably the timing mode does not alter those parameters.

To me it seems tidy to keep the 'mode' timings contained in their own
struct and find other homes for the other parameters.

> -tR
> -tBERS
> -tCCS
> -tPLEBSY
> -...
> 
> I see at least 3 of those timings that could be useful (for the moment) :
> - tR: this one should be used to fill the chip_delay field
> - tPROG and tBERS: could be used within nand_wait to choose the timeo
>   value appropriately.

IIRC these timing values are really only necessary if the controller
does not support the READY/BUSY input, in that case drivers typically
seem to use 'chip_delay' which is the maximum possible command
execution time (a sleep long enough to guarentee that READY/BUSY is
de-asserted).

> Or should I create a new struct for these timings ?
> In the latter case how should I name it ?

struct onfi_command_timings ?

Regards,
Jason
Boris BREZILLON - March 12, 2014, 4:46 p.m.
Le 11/03/2014 19:55, Jason Gunthorpe a écrit :
> On Mon, Mar 10, 2014 at 02:44:04PM +0100, Boris BREZILLON wrote:
>
>> Some timings are missing here (see Table 55 in the ONFI spec):
> Right..
>
> The 'mode' covers only the raw electrical parameters needed to
> exchange commands, other timings cover the commands
> themselves. Notably the timing mode does not alter those parameters.
>
> To me it seems tidy to keep the 'mode' timings contained in their own
> struct and find other homes for the other parameters.
>
>> -tR
>> -tBERS
>> -tCCS
>> -tPLEBSY
>> -...
>>
>> I see at least 3 of those timings that could be useful (for the moment) :
>> - tR: this one should be used to fill the chip_delay field
>> - tPROG and tBERS: could be used within nand_wait to choose the timeo
>>    value appropriately.
> IIRC these timing values are really only necessary if the controller
> does not support the READY/BUSY input, in that case drivers typically
> seem to use 'chip_delay' which is the maximum possible command
> execution time (a sleep long enough to guarentee that READY/BUSY is
> de-asserted).
You're right about tR (or chip_delay): it's only used when there are no 
R/B pin.
I experienced it when I tried the RB_NONE case in the sunxi driver: the 
default
chip_delay set by the NAND core code was too small to fit the NAND chip 
requirements.

Anyway, I really think the chip_delay field should be set according to 
NAND chip
characteristics not harcoded in NAND controller driver code (as 
currently done).

tPROG and tBERS, would be used in nand_wait function and do not depend 
on the R/B pin.
These are just used as timeouts.

>
>> Or should I create a new struct for these timings ?
>> In the latter case how should I name it ?
> struct onfi_command_timings ?

I'm not a big fan of this name. I think timing structs should not 
contain onfi in their
names, because these timings are also available on non ONFI chips.

Moreover, these timings are also part of the SDR NAND timings, they are 
just retrieved
using another method when interfacing with an ONFI chip.

Maybe we should change the nand_sdr_timings struct name too.

What do you think ?

Best Regards,

Boris

>
> Regards,
> Jason
Jason Gunthorpe - March 12, 2014, 7:07 p.m.
On Wed, Mar 12, 2014 at 05:46:53PM +0100, Boris BREZILLON wrote:

> >>I see at least 3 of those timings that could be useful (for the moment) :
> >>- tR: this one should be used to fill the chip_delay field
> >>- tPROG and tBERS: could be used within nand_wait to choose the timeo
> >>   value appropriately.
> >IIRC these timing values are really only necessary if the controller
> >does not support the READY/BUSY input, in that case drivers typically
> >seem to use 'chip_delay' which is the maximum possible command
> >execution time (a sleep long enough to guarentee that READY/BUSY is
> >de-asserted).

> You're right about tR (or chip_delay): it's only used when there are
> no R/B pin.  I experienced it when I tried the RB_NONE case in the
> sunxi driver: the default chip_delay set by the NAND core code was
> too small to fit the NAND chip requirements.
> 
> Anyway, I really think the chip_delay field should be set according
> to NAND chip characteristics not harcoded in NAND controller driver
> code (as currently done).

Drivers these days are often taking this value from the DT node
property 'chip-delay'. I think this would be nice to have in common
code too...

> tPROG and tBERS, would be used in nand_wait function and do not
> depend on the R/B pin.  These are just used as timeouts.

tPROG/tBERS have that special mode where R/B remains asserted but you
can still issue a status read to the chip to check on the command, so
the timeout required here is just a big number to detect failed NAND
controllers, it isn't really too important to have an exact value..

> >>Or should I create a new struct for these timings ?
> >>In the latter case how should I name it ?
> >struct onfi_command_timings ?
> 
> I'm not a big fan of this name. I think timing structs should not
> contain onfi in their names, because these timings are also
> available on non ONFI chips.

Explicitly defering to the ONFI spec makes it clear what the
definition of the timing parameter actually is.

If JEDEC has a different model then drivers will need to configure
their interfaces a little differently.

So we might end up with a jedec_sdr_timings too :|

> What do you think ?

I'd focus on getting the bus timings working before tackling too much
more ...

Jason
M. Warner Losh - March 12, 2014, 8:18 p.m.
On Mar 12, 2014, at 1:07 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:

> On Wed, Mar 12, 2014 at 05:46:53PM +0100, Boris BREZILLON wrote:
>> I'm not a big fan of this name. I think timing structs should not
>> contain onfi in their names, because these timings are also
>> available on non ONFI chips.
> 
> Explicitly defering to the ONFI spec makes it clear what the
> definition of the timing parameter actually is.

This is good, since ONFI is a very public spec.

> If JEDEC has a different model then drivers will need to configure
> their interfaces a little differently.

JEDEC’s spec is just a public as ONFI’s, but in the past at least it has been difficult
to get without purchase.

> So we might end up with a jedec_sdr_timings too :|

And onfi_ddr_timings and jedec_ddr_timings since those timing modes are appearing
on shipping parts.

Warner

Patch

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 9e6c8f9..67f0829 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -805,4 +805,53 @@  static inline bool nand_is_slc(struct nand_chip *chip)
 {
 	return chip->bits_per_cell == 1;
 }
+
+/**
+ * struct nand_sdr_timings - SDR NAND chip timings
+ *
+ * This struct defines the timing requirements of a SDR NAND chip.
+ * These informations can be found in every NAND datasheets and the timings
+ * meaning are described in the ONFI specifications:
+ * www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf‎ (chapter 4.15 Timing
+ * Parameters)
+ *
+ */
+
+struct nand_sdr_timings {
+	u32 tALH_min;
+	u32 tADL_min;
+	u32 tALS_min;
+	u32 tAR_min;
+	u32 tCEA_max;
+	u32 tCEH_min;
+	u32 tCH_min;
+	u32 tCHZ_max;
+	u32 tCLH_min;
+	u32 tCLR_min;
+	u32 tCLS_min;
+	u32 tCOH_min;
+	u32 tCS_min;
+	u32 tDH_min;
+	u32 tDS_min;
+	u32 tFEAT_max;
+	u32 tIR_min;
+	u32 tITC_max;
+	u32 tRC_min;
+	u32 tREA_max;
+	u32 tREH_min;
+	u32 tRHOH_min;
+	u32 tRHW_min;
+	u32 tRHZ_max;
+	u32 tRLOH_min;
+	u32 tRP_min;
+	u32 tRR_min;
+	u64 tRST_max;
+	u32 tWB_max;
+	u32 tWC_min;
+	u32 tWH_min;
+	u32 tWHR_min;
+	u32 tWP_min;
+	u32 tWW_min;
+};
+
 #endif /* __LINUX_MTD_NAND_H */