| Message ID | 1394647664-8258-2-git-send-email-b.brezillon.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Boris, On Wed, Mar 12, 2014 at 07:07:36PM +0100, Boris BREZILLON wrote: > 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 389b3c5..f3ff3a3 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -846,4 +846,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 Can you remove the unicode U+200E character? > + * Parameters) Please document the units for these fields here. It looks like you're using picoseconds. > + * > + */ > + Extra blank line. > +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 */ Brian
Hi Brian, On 30/04/2014 19:51, Brian Norris wrote: > Hi Boris, > > On Wed, Mar 12, 2014 at 07:07:36PM +0100, Boris BREZILLON wrote: >> + >> +/** >> + * 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 > Can you remove the unicode U+200E character? Sure > >> + * Parameters) > Please document the units for these fields here. It looks like you're > using picoseconds. I'll add field units (which are indeed picoseconds) to this comment. Best Regards, Boris
> > 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 389b3c5..f3ff3a3 100644 > > --- a/include/linux/mtd/nand.h > > +++ b/include/linux/mtd/nand.h [...] > > + * Parameters) > > Please document the units for these fields here. It looks like you're > using picoseconds. Can't we leave this open to interpretation? For instance, it's more convenient for our driver to handle these as nano second values. > > + * > > + */ > > + > > Extra blank line. > > > +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 tCSD_min; > > + u32 tDH_min; > > + u32 tDS_min; > > + u32 tFEAT_max; > > + u32 tIR_min; > > + u32 tITC_max; u32 tR_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 */
On 08/05/2014 16:29, Lee Jones wrote: >>> 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 389b3c5..f3ff3a3 100644 >>> --- a/include/linux/mtd/nand.h >>> +++ b/include/linux/mtd/nand.h > [...] > >>> + * Parameters) >> Please document the units for these fields here. It looks like you're >> using picoseconds. > Can't we leave this open to interpretation? For instance, it's more > convenient for our driver to handle these as nano second values. > >>> + * >>> + */ >>> + >> Extra blank line. >> >>> +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 tCSD_min; This is related to ddr2 timings, and this structure only define sdr related timings. >>> + u32 tDH_min; >>> + u32 tDS_min; >>> + u32 tFEAT_max; >>> + u32 tIR_min; >>> + u32 tITC_max; > u32 tR_max; Actually this one cannot be retrieved from the ONFI timing mode, you'll have to read bytes 137 and 138 in the parameter page. Hence I got rid of it in the first place. Then I considered adding all those missing timings in another structure (see [1]). I noticed you posted something similar on the MTD mailing list (and Lucas was waiting for the feature too). Could we work together to propose something that fulfills all our needs ? Here's what I need: - get detailled timings to be able to configure the sunxi NAND controller appropriately - a way to get these timings on non-ONFi NANDs (the proposed solution is exposing the closest supported ONFI timing mode in the NAND chip DT node, but I'm open to any new proposal). Best Regards, Boris [1] http://lists.infradead.org/pipermail/linux-mtd/2014-March/052525.html
On Thu, May 08, 2014 at 03:29:30PM +0100, Lee Jones wrote: > > > > Please document the units for these fields here. It looks like you're > > using picoseconds. > > Can't we leave this open to interpretation? For instance, it's more > convenient for our driver to handle these as nano second values. No, their values will be determined by the nand_base core, and we must have something consistent for drivers to rely on. However, I don't really have a hard preference on nanoseconds versus picoseconds. If we see that many of the values reach low-digit nanosecons, or fractional nanoseconds, it probably makes sense to have the higher resolution. > > > +struct nand_sdr_timings { ... > > > + u32 tCS_min; > > u32 tCSD_min; Is this a suggested addition, Lee? I agree with Boris that this looks like a DDR mode, which should not be covered here. > > > + u32 tDH_min; > > > + u32 tDS_min; > > > + u32 tFEAT_max; > > > + u32 tIR_min; > > > + u32 tITC_max; > > u32 tR_max; Same here, is this a suggested new field? If you need it, then we can follow up like Boris suggested, with a different method, since tR is not part of the electrical parameters of the timing mode. ... > > > +}; > > > + > > > #endif /* __LINUX_MTD_NAND_H */ Brian
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 389b3c5..f3ff3a3 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -846,4 +846,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 */
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(+)