| Message ID | 1389190924-26226-4-git-send-email-b.brezillon@overkiz.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jan 8, 2014 at 8:21 AM, Boris BREZILLON <b.brezillon@overkiz.com> wrote: > Add a function to retrieve NAND timings from a given DT node. > > Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com> > --- > drivers/of/of_mtd.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/of_mtd.h | 9 +++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c > index a27ec94..52e07fd 100644 > --- a/drivers/of/of_mtd.c > +++ b/drivers/of/of_mtd.c > @@ -83,3 +83,50 @@ bool of_get_nand_on_flash_bbt(struct device_node *np) > return of_property_read_bool(np, "nand-on-flash-bbt"); > } > EXPORT_SYMBOL_GPL(of_get_nand_on_flash_bbt); > + > +/** > + * of_get_nand_timings - Get nand timings for the given device_node > + * @np: Pointer to the given device_node > + * > + * return 0 on success errno other wise > + */ > +int of_get_nand_timings(struct device_node *np, struct nand_timings *timings) > +{ > + memset(timings, 0, sizeof(*timings)); > + > + of_property_read_u32(np, "tCLS-min", &timings->tCLS_min); > + of_property_read_u32(np, "tCLH-min", &timings->tCLH_min); > + of_property_read_u32(np, "tCS-min", &timings->tCS_min); > + of_property_read_u32(np, "tCH-min", &timings->tCH_min); > + of_property_read_u32(np, "tWP-min", &timings->tWP_min); > + of_property_read_u32(np, "tALS-min", &timings->tALS_min); > + of_property_read_u32(np, "tALH-min", &timings->tALH_min); > + of_property_read_u32(np, "tDS-min", &timings->tDS_min); > + of_property_read_u32(np, "tDH-min", &timings->tDH_min); > + of_property_read_u32(np, "tWC-min", &timings->tWC_min); > + of_property_read_u32(np, "tWH-min", &timings->tWH_min); > + of_property_read_u32(np, "tR-max", &timings->tR_max); > + of_property_read_u32(np, "tAR-min", &timings->tAR_min); > + of_property_read_u32(np, "tCLR-min", &timings->tCLR_min); > + of_property_read_u32(np, "tRR-min", &timings->tRR_min); > + of_property_read_u32(np, "tRP-min", &timings->tRP_min); > + of_property_read_u32(np, "tWB-max", &timings->tWB_max); > + of_property_read_u32(np, "tRC-min", &timings->tRC_min); > + of_property_read_u32(np, "tREA-max", &timings->tREA_max); > + of_property_read_u32(np, "tRHZ-max", &timings->tRHZ_max); > + of_property_read_u32(np, "tCHZ-max", &timings->tCHZ_max); > + of_property_read_u32(np, "tRHOH-min", &timings->tRHOH_min); > + of_property_read_u32(np, "tRLOH-min", &timings->tRLOH_min); > + of_property_read_u32(np, "tCOH-min", &timings->tCOH_min); > + of_property_read_u32(np, "tREH-min", &timings->tREH_min); > + of_property_read_u32(np, "tWHR-min", &timings->tWHR_min); > + of_property_read_u32(np, "tRHW-min", &timings->tRHW_min); > + of_property_read_u32(np, "tIR-min", &timings->tIR_min); > + of_property_read_u32(np, "tCR-min", &timings->tCR_min); > + of_property_read_u32(np, "tADL-min", &timings->tADL_min); > + of_property_read_u32(np, "tRST-max", &timings->tRST_max); > + of_property_read_u32(np, "tWW-min", &timings->tWW_min); These all need to be documented. These apply to which compatible strings? They should also be suffixed with the unit the property is in (i.e. -ms, -us, -ns). Rob
On 08/01/2014 17:30, Rob Herring wrote: > On Wed, Jan 8, 2014 at 8:21 AM, Boris BREZILLON <b.brezillon@overkiz.com> wrote: >> Add a function to retrieve NAND timings from a given DT node. >> >> Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com> >> --- >> drivers/of/of_mtd.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/of_mtd.h | 9 +++++++++ >> 2 files changed, 56 insertions(+) >> >> diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c >> index a27ec94..52e07fd 100644 >> --- a/drivers/of/of_mtd.c >> +++ b/drivers/of/of_mtd.c >> @@ -83,3 +83,50 @@ bool of_get_nand_on_flash_bbt(struct device_node *np) >> return of_property_read_bool(np, "nand-on-flash-bbt"); >> } >> EXPORT_SYMBOL_GPL(of_get_nand_on_flash_bbt); >> + >> +/** >> + * of_get_nand_timings - Get nand timings for the given device_node >> + * @np: Pointer to the given device_node >> + * >> + * return 0 on success errno other wise >> + */ >> +int of_get_nand_timings(struct device_node *np, struct nand_timings *timings) >> +{ >> + memset(timings, 0, sizeof(*timings)); >> + >> + of_property_read_u32(np, "tCLS-min", &timings->tCLS_min); >> + of_property_read_u32(np, "tCLH-min", &timings->tCLH_min); >> + of_property_read_u32(np, "tCS-min", &timings->tCS_min); >> + of_property_read_u32(np, "tCH-min", &timings->tCH_min); >> + of_property_read_u32(np, "tWP-min", &timings->tWP_min); >> + of_property_read_u32(np, "tALS-min", &timings->tALS_min); >> + of_property_read_u32(np, "tALH-min", &timings->tALH_min); >> + of_property_read_u32(np, "tDS-min", &timings->tDS_min); >> + of_property_read_u32(np, "tDH-min", &timings->tDH_min); >> + of_property_read_u32(np, "tWC-min", &timings->tWC_min); >> + of_property_read_u32(np, "tWH-min", &timings->tWH_min); >> + of_property_read_u32(np, "tR-max", &timings->tR_max); >> + of_property_read_u32(np, "tAR-min", &timings->tAR_min); >> + of_property_read_u32(np, "tCLR-min", &timings->tCLR_min); >> + of_property_read_u32(np, "tRR-min", &timings->tRR_min); >> + of_property_read_u32(np, "tRP-min", &timings->tRP_min); >> + of_property_read_u32(np, "tWB-max", &timings->tWB_max); >> + of_property_read_u32(np, "tRC-min", &timings->tRC_min); >> + of_property_read_u32(np, "tREA-max", &timings->tREA_max); >> + of_property_read_u32(np, "tRHZ-max", &timings->tRHZ_max); >> + of_property_read_u32(np, "tCHZ-max", &timings->tCHZ_max); >> + of_property_read_u32(np, "tRHOH-min", &timings->tRHOH_min); >> + of_property_read_u32(np, "tRLOH-min", &timings->tRLOH_min); >> + of_property_read_u32(np, "tCOH-min", &timings->tCOH_min); >> + of_property_read_u32(np, "tREH-min", &timings->tREH_min); >> + of_property_read_u32(np, "tWHR-min", &timings->tWHR_min); >> + of_property_read_u32(np, "tRHW-min", &timings->tRHW_min); >> + of_property_read_u32(np, "tIR-min", &timings->tIR_min); >> + of_property_read_u32(np, "tCR-min", &timings->tCR_min); >> + of_property_read_u32(np, "tADL-min", &timings->tADL_min); >> + of_property_read_u32(np, "tRST-max", &timings->tRST_max); >> + of_property_read_u32(np, "tWW-min", &timings->tWW_min); > These all need to be documented. They're document in PATCH 4/9 (but the description might be a bit light). > These apply to which compatible > strings? Actually this could apply to any nand chips. The NAND Flash Controller driver is then responsible for converting these values to use it. > They should also be suffixed with the unit the property is in > (i.e. -ms, -us, -ns). Sure, I'll add the unit (which is -ns BTW). Thanks. Best Regards, Boris > > Rob
On Wed, Jan 08, 2014 at 03:21:58PM +0100, Boris BREZILLON wrote: > +int of_get_nand_timings(struct device_node *np, struct nand_timings *timings) > +{ > + memset(timings, 0, sizeof(*timings)); > + of_property_read_u32(np, "tCLS-min", &timings->tCLS_min); > + of_property_read_u32(np, "tCLH-min", &timings->tCLH_min); > + of_property_read_u32(np, "tCS-min", &timings->tCS_min); [..] A while ago when discussing another controller it was pointed out these values are all auto-probable directly from the NAND via a ONFI defined GET FEATURE @0x01 query, and adding these timings to the DT was NAK'd.. Basically you set the interface to the slowest ONFI timing mode, do the GET FEATURE to the NAND chip and then increase the interface speed to the highest mutually supported ONFI mode. Is there some reason you need to encode this in the DT? Jason
Hello Jason, Le 08/01/2014 19:34, Jason Gunthorpe a écrit : > On Wed, Jan 08, 2014 at 03:21:58PM +0100, Boris BREZILLON wrote: > >> +int of_get_nand_timings(struct device_node *np, struct nand_timings *timings) >> +{ >> + memset(timings, 0, sizeof(*timings)); >> + of_property_read_u32(np, "tCLS-min", &timings->tCLS_min); >> + of_property_read_u32(np, "tCLH-min", &timings->tCLH_min); >> + of_property_read_u32(np, "tCS-min", &timings->tCS_min); > [..] > > A while ago when discussing another controller it was pointed out > these values are all auto-probable directly from the NAND via a ONFI > defined GET FEATURE @0x01 query, and adding these timings to the DT > was NAK'd.. > > Basically you set the interface to the slowest ONFI timing mode, do > the GET FEATURE to the NAND chip and then increase the interface speed > to the highest mutually supported ONFI mode. > Is there some reason you need to encode this in the DT? What if the NAND does not support the ONFI interface (and this is exactly the case for the NAND available on the cubietruck board: H27UCG8T2ATR). But I'm glag to hear about this ONFI feature :). I'll add a function to retrieve these timings if the NAND support the ONFI interface. Best Regards, Boris > > Jason
On Wed, Jan 08, 2014 at 08:00:02PM +0100, boris brezillon wrote: > Hello Jason, > > Le 08/01/2014 19:34, Jason Gunthorpe a ?crit : > >On Wed, Jan 08, 2014 at 03:21:58PM +0100, Boris BREZILLON wrote: > > > >>+int of_get_nand_timings(struct device_node *np, struct nand_timings *timings) > >>+{ > >>+ memset(timings, 0, sizeof(*timings)); > >>+ of_property_read_u32(np, "tCLS-min", &timings->tCLS_min); > >>+ of_property_read_u32(np, "tCLH-min", &timings->tCLH_min); > >>+ of_property_read_u32(np, "tCS-min", &timings->tCS_min); > >[..] > > > >A while ago when discussing another controller it was pointed out > >these values are all auto-probable directly from the NAND via a ONFI > >defined GET FEATURE @0x01 query, and adding these timings to the DT > >was NAK'd.. > > > >Basically you set the interface to the slowest ONFI timing mode, do > >the GET FEATURE to the NAND chip and then increase the interface speed > >to the highest mutually supported ONFI mode. > > >Is there some reason you need to encode this in the DT? > > What if the NAND does not support the ONFI interface (and this is > exactly the case for the NAND available on the cubietruck board: > H27UCG8T2ATR). Sounds like a good reason to me! You might want to check if you can boil down the DT timings from the huge list to just an ONFI mode number.. I'd echo Rob's comments, the property needs to include the units in the name, and I strongly recommend picoseconds for these values. Also, you might want to check that the ONFI names for these parameters are used, not a vendor specific name to try and avoid confusion. Jason
On 08/01/2014 20:13, Jason Gunthorpe wrote: > On Wed, Jan 08, 2014 at 08:00:02PM +0100, boris brezillon wrote: >> Hello Jason, >> >> Le 08/01/2014 19:34, Jason Gunthorpe a ?crit : >>> On Wed, Jan 08, 2014 at 03:21:58PM +0100, Boris BREZILLON wrote: >>> >>>> +int of_get_nand_timings(struct device_node *np, struct nand_timings *timings) >>>> +{ >>>> + memset(timings, 0, sizeof(*timings)); >>>> + of_property_read_u32(np, "tCLS-min", &timings->tCLS_min); >>>> + of_property_read_u32(np, "tCLH-min", &timings->tCLH_min); >>>> + of_property_read_u32(np, "tCS-min", &timings->tCS_min); >>> [..] >>> >>> A while ago when discussing another controller it was pointed out >>> these values are all auto-probable directly from the NAND via a ONFI >>> defined GET FEATURE @0x01 query, and adding these timings to the DT >>> was NAK'd.. >>> >>> Basically you set the interface to the slowest ONFI timing mode, do >>> the GET FEATURE to the NAND chip and then increase the interface speed >>> to the highest mutually supported ONFI mode. >>> Is there some reason you need to encode this in the DT? >> What if the NAND does not support the ONFI interface (and this is >> exactly the case for the NAND available on the cubietruck board: >> H27UCG8T2ATR). > Sounds like a good reason to me! > > You might want to check if you can boil down the DT timings from the > huge list to just an ONFI mode number.. Sure, but the sunxi driver needs at least 19 of them... > > I'd echo Rob's comments, the property needs to include the units > in the name, and I strongly recommend picoseconds for these > values. Agreed, picosecond is a more future-proof unit. > > Also, you might want to check that the ONFI names for these parameters > are used, not a vendor specific name to try and avoid confusion. I'll check it. Thanks. Best Regards, Boris > > Jason
On Thu, Jan 09, 2014 at 09:36:18AM +0100, boris brezillon wrote: > >You might want to check if you can boil down the DT timings from the > >huge list to just an ONFI mode number.. > > Sure, but the sunxi driver needs at least 19 of them... So does mvebu's NAND driver.. What I ment was you could have a onfi,nand-timing-mode = 0 in the DT. Each of the modes defines all ~19 parameters, higher modes are faster. Pick a mode value that fits all the parameters of the connected non-ONFI flash. This would be instead of defining each parameter individually.. Provide some helpers to convert from a onfi mode number to all the onfi defined timing parameters so that drivers can configure the HW.. Jason
Hello Jason, On 09/01/2014 18:35, Jason Gunthorpe wrote: > On Thu, Jan 09, 2014 at 09:36:18AM +0100, boris brezillon wrote: > >>> You might want to check if you can boil down the DT timings from the >>> huge list to just an ONFI mode number.. >> Sure, but the sunxi driver needs at least 19 of them... > So does mvebu's NAND driver.. > > What I ment was you could have a > > onfi,nand-timing-mode = 0 > > in the DT. Each of the modes defines all ~19 parameters, higher modes > are faster. > > Pick a mode value that fits all the parameters of the connected > non-ONFI flash. > > This would be instead of defining each parameter > individually.. Provide some helpers to convert from a onfi mode number > to all the onfi defined timing parameters so that drivers can > configure the HW.. Are you suggesting we should provide a function that converts these modes into a nand_timings struct, or just use the timing modes and let the NAND controller drivers configure its IP accordingly ? I found the ONFI timing tables in this document: www.*onfi*.org/~/media/*ONFI*/specs/*onfi*_3_1_spec.pdf (chapter 4.16). I suppose my nand_timings struct should use the names described page 110-111 (at least if we decide to use nand_timings and not nand_timing_modes), right ? Best Regards, Boris > > Jason
On 15/01/2014 16:09, boris brezillon wrote: > Hello Jason, > > On 09/01/2014 18:35, Jason Gunthorpe wrote: >> On Thu, Jan 09, 2014 at 09:36:18AM +0100, boris brezillon wrote: >> >>>> You might want to check if you can boil down the DT timings from the >>>> huge list to just an ONFI mode number.. >>> Sure, but the sunxi driver needs at least 19 of them... >> So does mvebu's NAND driver.. >> >> What I ment was you could have a >> >> onfi,nand-timing-mode = 0 >> >> in the DT. Each of the modes defines all ~19 parameters, higher modes >> are faster. >> >> Pick a mode value that fits all the parameters of the connected >> non-ONFI flash. >> >> This would be instead of defining each parameter >> individually.. Provide some helpers to convert from a onfi mode number >> to all the onfi defined timing parameters so that drivers can >> configure the HW.. > > Are you suggesting we should provide a function that converts these > modes into a nand_timings struct, or just use the timing modes and > let the NAND controller drivers configure its IP accordingly ? > > I found the ONFI timing tables in this document: > > www.*onfi*.org/~/media/*ONFI*/specs/*onfi*_3_1_spec.pdf (chapter 4.16). > > I suppose my nand_timings struct should use the names described > page 110-111 (at least if we decide to use nand_timings and not > nand_timing_modes), right ? After taking a closer look at this document, the only parameter available in my nand_timings struct that is not defined in the standard is tR_max (data transfer from cell to register). And the ONFI standard defines 4 more timings: - tCEA_max - tCEH_min - tFEAT_max - tITC_max > > Best Regards, > > Boris > >> >> Jason >
On Wed, Jan 15, 2014 at 06:03:01PM +0100, boris brezillon wrote: > >>Pick a mode value that fits all the parameters of the connected > >>non-ONFI flash. > >> > >>This would be instead of defining each parameter > >>individually.. Provide some helpers to convert from a onfi mode number > >>to all the onfi defined timing parameters so that drivers can > >>configure the HW.. > > > >Are you suggesting we should provide a function that converts these > >modes into a nand_timings struct, or just use the timing modes and > >let the NAND controller drivers configure its IP accordingly ? Either seems reasonable to me, but passing the ONFI mode directly from the NAND core to the driver seems a little safer.. The NAND core can provide a helper function to xlate the mode number to the timing struct to help drivers that need broken out timing. > >I found the ONFI timing tables in this document: > > > >www.*onfi*.org/~/media/*ONFI*/specs/*onfi*_3_1_spec.pdf (chapter 4.16). > > > >I suppose my nand_timings struct should use the names described > >page 110-111 (at least if we decide to use nand_timings and not > >nand_timing_modes), right ? Yah, I think follow the standard. The standard has timing diagrams that show what all these parameters actually are. > After taking a closer look at this document, the only parameter > available in my nand_timings struct that is not defined in the > standard is tR_max (data transfer from cell to register). Maybe it can be derived from the other parameters? The ONFI values seemed pretty comprehensive to me. I think the mvebu driver was similar, not all of the ONFI values were used and some translation was needed. Jason
On Tue, 21 Jan 2014 15:57:40 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Wed, Jan 15, 2014 at 06:03:01PM +0100, boris brezillon wrote: > > > >>Pick a mode value that fits all the parameters of the connected > > >>non-ONFI flash. > > >> > > >>This would be instead of defining each parameter > > >>individually.. Provide some helpers to convert from a onfi mode number > > >>to all the onfi defined timing parameters so that drivers can > > >>configure the HW.. > > > > > >Are you suggesting we should provide a function that converts these > > >modes into a nand_timings struct, or just use the timing modes and > > >let the NAND controller drivers configure its IP accordingly ? > > Either seems reasonable to me, but passing the ONFI mode directly from > the NAND core to the driver seems a little safer.. I agree here. There are a lot of parameters being defined. If it can be boiled down to an ONFI mode that will make for a much more robust binding. Far fewer things to get wrong. g.
diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c index a27ec94..52e07fd 100644 --- a/drivers/of/of_mtd.c +++ b/drivers/of/of_mtd.c @@ -83,3 +83,50 @@ bool of_get_nand_on_flash_bbt(struct device_node *np) return of_property_read_bool(np, "nand-on-flash-bbt"); } EXPORT_SYMBOL_GPL(of_get_nand_on_flash_bbt); + +/** + * of_get_nand_timings - Get nand timings for the given device_node + * @np: Pointer to the given device_node + * + * return 0 on success errno other wise + */ +int of_get_nand_timings(struct device_node *np, struct nand_timings *timings) +{ + memset(timings, 0, sizeof(*timings)); + + of_property_read_u32(np, "tCLS-min", &timings->tCLS_min); + of_property_read_u32(np, "tCLH-min", &timings->tCLH_min); + of_property_read_u32(np, "tCS-min", &timings->tCS_min); + of_property_read_u32(np, "tCH-min", &timings->tCH_min); + of_property_read_u32(np, "tWP-min", &timings->tWP_min); + of_property_read_u32(np, "tALS-min", &timings->tALS_min); + of_property_read_u32(np, "tALH-min", &timings->tALH_min); + of_property_read_u32(np, "tDS-min", &timings->tDS_min); + of_property_read_u32(np, "tDH-min", &timings->tDH_min); + of_property_read_u32(np, "tWC-min", &timings->tWC_min); + of_property_read_u32(np, "tWH-min", &timings->tWH_min); + of_property_read_u32(np, "tR-max", &timings->tR_max); + of_property_read_u32(np, "tAR-min", &timings->tAR_min); + of_property_read_u32(np, "tCLR-min", &timings->tCLR_min); + of_property_read_u32(np, "tRR-min", &timings->tRR_min); + of_property_read_u32(np, "tRP-min", &timings->tRP_min); + of_property_read_u32(np, "tWB-max", &timings->tWB_max); + of_property_read_u32(np, "tRC-min", &timings->tRC_min); + of_property_read_u32(np, "tREA-max", &timings->tREA_max); + of_property_read_u32(np, "tRHZ-max", &timings->tRHZ_max); + of_property_read_u32(np, "tCHZ-max", &timings->tCHZ_max); + of_property_read_u32(np, "tRHOH-min", &timings->tRHOH_min); + of_property_read_u32(np, "tRLOH-min", &timings->tRLOH_min); + of_property_read_u32(np, "tCOH-min", &timings->tCOH_min); + of_property_read_u32(np, "tREH-min", &timings->tREH_min); + of_property_read_u32(np, "tWHR-min", &timings->tWHR_min); + of_property_read_u32(np, "tRHW-min", &timings->tRHW_min); + of_property_read_u32(np, "tIR-min", &timings->tIR_min); + of_property_read_u32(np, "tCR-min", &timings->tCR_min); + of_property_read_u32(np, "tADL-min", &timings->tADL_min); + of_property_read_u32(np, "tRST-max", &timings->tRST_max); + of_property_read_u32(np, "tWW-min", &timings->tWW_min); + + return 0; +} +EXPORT_SYMBOL_GPL(of_get_nand_timings); diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h index 6f10e93..ecedb5f 100644 --- a/include/linux/of_mtd.h +++ b/include/linux/of_mtd.h @@ -9,12 +9,15 @@ #ifndef __LINUX_OF_MTD_H #define __LINUX_OF_NET_H +#include <linux/mtd/nand.h> + #ifdef CONFIG_OF_MTD #include <linux/of.h> int of_get_nand_ecc_mode(struct device_node *np); int of_get_nand_bus_width(struct device_node *np); bool of_get_nand_on_flash_bbt(struct device_node *np); +int of_get_nand_timings(struct device_node *np, struct nand_timings *timings); #else /* CONFIG_OF_MTD */ @@ -33,6 +36,12 @@ static inline bool of_get_nand_on_flash_bbt(struct device_node *np) return false; } +static inline int of_get_nand_timings(struct device_node *np, + struct nand_timings *timings) +{ + return -ENOSYS; +} + #endif /* CONFIG_OF_MTD */ #endif /* __LINUX_OF_MTD_H */
Add a function to retrieve NAND timings from a given DT node. Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com> --- drivers/of/of_mtd.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/of_mtd.h | 9 +++++++++ 2 files changed, 56 insertions(+)