[PATCH v2 0/3] omap: gpmc-nand: add ability to keep timings defined by the bootloader

[PATCH v2 0/3] omap: gpmc-nand: add ability to keep timings defined by the bootloader

[Mike Rapoport]

These patches add ability to keep GPMC timings configured by the
bootloader. The platforms have to either define NAND GPMC timinings
explicitly via gpmc_t field of omap_nand_platform_data or set
keep_timings flag to allow detection of current timing configuration
and its subsequent use in omap2_nand_gpmc_retime.

v2 changes:
   - remove debug leftovers
   - make timing rounding depend on .keep_timing flag and introduce
    omap2_nand_gpmc_round_timings to facilitate this change

Mike Rapoport (3):
  omap: gpmc: add gpmc_cs_get_timings
  omap: gpmc-nand: introduce omap2_nand_gpmc_round_timings helper
  omap: gpmc-nand: add ability to keep timings defined by the
    bootloader

 arch/arm/mach-omap2/gpmc-nand.c        |   73 +++++++++++++++++++------------
 arch/arm/mach-omap2/gpmc.c             |   76 ++++++++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/gpmc.h |    1 +
 arch/arm/plat-omap/include/plat/nand.h |    1 +
 4 files changed, 123 insertions(+), 28 deletions(-)

[PATCH v2 1/3] omap: gpmc: add gpmc_cs_get_timings

[Mike Rapoport]

Add gpmc_cs_get_timings counterpart of gpmc_cs_set_timings and
convinience macros to read particular timing configuration fields

Signed-off-by: Mike Rapoport <mike@compulab.co.il>
---
 arch/arm/mach-omap2/gpmc.c             |   76 ++++++++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/gpmc.h |    1 +
 2 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5bc3ca0..527a0da 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -163,6 +163,36 @@ unsigned int gpmc_round_ns_to_ticks(unsigned int time_ns)
 }
 
 #ifdef DEBUG
+static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
+			       const char *name)
+#else
+static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit)
+#endif
+{
+	u32 l;
+	int ticks, mask, nr_bits, time;
+
+	nr_bits = end_bit - st_bit + 1;
+	mask = ((1 << nr_bits) - 1);
+
+	l = gpmc_cs_read_reg(cs, reg);
+	ticks = (l >> st_bit) & mask;
+
+	if (ticks == 0)
+		time = 0;
+	else
+		time = gpmc_ticks_to_ns(ticks);
+
+#ifdef DEBUG
+	printk(KERN_INFO
+		"GPMC CS%d: %-10s: %3d ticks, %3d ns\n",
+	       cs, name, ticks, time);
+#endif
+
+	return time;
+}
+
+#ifdef DEBUG
 static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 			       int time, const char *name)
 #else
@@ -206,10 +236,14 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 	if (set_gpmc_timing_reg(cs, (reg), (st), (end),		\
 			t->field, #field) < 0)			\
 		return -1
+#define GPMC_GET_ONE(reg, st, end, field) \
+	t->field = get_gpmc_timing_reg(cs, (reg), (st), (end), #field)
 #else
 #define GPMC_SET_ONE(reg, st, end, field) \
 	if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field) < 0) \
 		return -1
+#define GPMC_GET_ONE(reg, st, end, field) \
+	t->field = get_gpmc_timing_reg(cs, (reg), (st), (end))
 #endif
 
 int gpmc_cs_calc_divider(int cs, unsigned int sync_clk)
@@ -227,6 +261,48 @@ int gpmc_cs_calc_divider(int cs, unsigned int sync_clk)
 	return div;
 }
 
+void gpmc_cs_get_timings(int cs, struct gpmc_timings *t)
+{
+	int div;
+	u32 l;
+
+	GPMC_GET_ONE(GPMC_CS_CONFIG2,  0,  3, cs_on);
+	GPMC_GET_ONE(GPMC_CS_CONFIG2,  8, 12, cs_rd_off);
+	GPMC_GET_ONE(GPMC_CS_CONFIG2, 16, 20, cs_wr_off);
+
+	GPMC_GET_ONE(GPMC_CS_CONFIG3,  0,  3, adv_on);
+	GPMC_GET_ONE(GPMC_CS_CONFIG3,  8, 12, adv_rd_off);
+	GPMC_GET_ONE(GPMC_CS_CONFIG3, 16, 20, adv_wr_off);
+
+	GPMC_GET_ONE(GPMC_CS_CONFIG4,  0,  3, oe_on);
+	GPMC_GET_ONE(GPMC_CS_CONFIG4,  8, 12, oe_off);
+	GPMC_GET_ONE(GPMC_CS_CONFIG4, 16, 19, we_on);
+	GPMC_GET_ONE(GPMC_CS_CONFIG4, 24, 28, we_off);
+
+	GPMC_GET_ONE(GPMC_CS_CONFIG5,  0,  4, rd_cycle);
+	GPMC_GET_ONE(GPMC_CS_CONFIG5,  8, 12, wr_cycle);
+	GPMC_GET_ONE(GPMC_CS_CONFIG5, 16, 20, access);
+
+	GPMC_GET_ONE(GPMC_CS_CONFIG5, 24, 27, page_burst_access);
+
+	if (cpu_is_omap34xx()) {
+		GPMC_GET_ONE(GPMC_CS_CONFIG6, 16, 19, wr_data_mux_bus);
+		GPMC_GET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
+	}
+
+	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
+	if (l & (GPMC_CONFIG1_READTYPE_SYNC | GPMC_CONFIG1_WRITETYPE_SYNC)) {
+		div = (l & 0x03) + 1;
+#ifdef DEBUG
+		printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
+				cs, (div * gpmc_get_fclk_period()) / 1000, div);
+#endif
+		t->sync_clk = (div * gpmc_get_fclk_period()) / 1000;
+	} else {
+		t->sync_clk = 0;
+	}
+}
+
 int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
 {
 	int div;
diff --git a/arch/arm/plat-omap/include/plat/gpmc.h b/arch/arm/plat-omap/include/plat/gpmc.h
index 145838a..5c345f1 100644
--- a/arch/arm/plat-omap/include/plat/gpmc.h
+++ b/arch/arm/plat-omap/include/plat/gpmc.h
@@ -102,6 +102,7 @@ extern void gpmc_cs_write_reg(int cs, int idx, u32 val);
 extern u32 gpmc_cs_read_reg(int cs, int idx);
 extern int gpmc_cs_calc_divider(int cs, unsigned int sync_clk);
 extern int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t);
+extern void gpmc_cs_get_timings(int cs, struct gpmc_timings *t);
 extern int gpmc_cs_request(int cs, unsigned long size, unsigned long *base);
 extern void gpmc_cs_free(int cs);
 extern int gpmc_cs_set_reserved(int cs, int reserved);
-- 
1.6.6.2

[PATCH v2 2/3] omap: gpmc-nand: introduce omap2_nand_gpmc_round_timings helper

[Mike Rapoport]

Signed-off-by: Mike Rapoport <mike@compulab.co.il>
---
 arch/arm/mach-omap2/gpmc-nand.c |   56 +++++++++++++++++++++------------------
 1 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index e57fb29..9434c80 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -34,6 +34,35 @@ static struct platform_device gpmc_nand_device = {
 	.resource	= &gpmc_nand_resource,
 };
 
+static void omap2_nand_gpmc_round_timings(struct gpmc_timings *src,
+					  struct gpmc_timings *dst)
+{
+	dst->sync_clk = gpmc_round_ns_to_ticks(src->sync_clk);
+	dst->cs_on = gpmc_round_ns_to_ticks(src->cs_on);
+	dst->adv_on = gpmc_round_ns_to_ticks(src->adv_on);
+
+	/* Read */
+	dst->adv_rd_off = gpmc_round_ns_to_ticks(src->adv_rd_off);
+	dst->oe_on  = dst->adv_on;
+	dst->access = gpmc_round_ns_to_ticks(src->access);
+	dst->oe_off = gpmc_round_ns_to_ticks(src->oe_off);
+	dst->cs_rd_off = gpmc_round_ns_to_ticks(src->cs_rd_off);
+	dst->rd_cycle  = gpmc_round_ns_to_ticks(src->rd_cycle);
+
+	/* Write */
+	dst->adv_wr_off = gpmc_round_ns_to_ticks(src->adv_wr_off);
+	dst->we_on  = dst->oe_on;
+	if (cpu_is_omap34xx()) {
+		dst->wr_data_mux_bus =	gpmc_round_ns_to_ticks(
+				src->wr_data_mux_bus);
+		dst->wr_access = gpmc_round_ns_to_ticks(
+				src->wr_access);
+	}
+	dst->we_off = gpmc_round_ns_to_ticks(src->we_off);
+	dst->cs_wr_off = gpmc_round_ns_to_ticks(src->cs_wr_off);
+	dst->wr_cycle  = gpmc_round_ns_to_ticks(src->wr_cycle);
+}
+
 static int omap2_nand_gpmc_retime(void)
 {
 	struct gpmc_timings t;
@@ -43,32 +72,7 @@ static int omap2_nand_gpmc_retime(void)
 		return 0;
 
 	memset(&t, 0, sizeof(t));
-	t.sync_clk = gpmc_round_ns_to_ticks(gpmc_nand_data->gpmc_t->sync_clk);
-	t.cs_on = gpmc_round_ns_to_ticks(gpmc_nand_data->gpmc_t->cs_on);
-	t.adv_on = gpmc_round_ns_to_ticks(gpmc_nand_data->gpmc_t->adv_on);
-
-	/* Read */
-	t.adv_rd_off = gpmc_round_ns_to_ticks(
-				gpmc_nand_data->gpmc_t->adv_rd_off);
-	t.oe_on  = t.adv_on;
-	t.access = gpmc_round_ns_to_ticks(gpmc_nand_data->gpmc_t->access);
-	t.oe_off = gpmc_round_ns_to_ticks(gpmc_nand_data->gpmc_t->oe_off);
-	t.cs_rd_off = gpmc_round_ns_to_ticks(gpmc_nand_data->gpmc_t->cs_rd_off);
-	t.rd_cycle  = gpmc_round_ns_to_ticks(gpmc_nand_data->gpmc_t->rd_cycle);
-
-	/* Write */
-	t.adv_wr_off = gpmc_round_ns_to_ticks(
-				gpmc_nand_data->gpmc_t->adv_wr_off);
-	t.we_on  = t.oe_on;
-	if (cpu_is_omap34xx()) {
-	    t.wr_data_mux_bus =	gpmc_round_ns_to_ticks(
-				gpmc_nand_data->gpmc_t->wr_data_mux_bus);
-	    t.wr_access = gpmc_round_ns_to_ticks(
-				gpmc_nand_data->gpmc_t->wr_access);
-	}
-	t.we_off = gpmc_round_ns_to_ticks(gpmc_nand_data->gpmc_t->we_off);
-	t.cs_wr_off = gpmc_round_ns_to_ticks(gpmc_nand_data->gpmc_t->cs_wr_off);
-	t.wr_cycle  = gpmc_round_ns_to_ticks(gpmc_nand_data->gpmc_t->wr_cycle);
+	omap2_nand_gpmc_round_timings(gpmc_nand_data->gpmc_t, &t);
 
 	/* Configure GPMC */
 	gpmc_cs_write_reg(gpmc_nand_data->cs, GPMC_CS_CONFIG1,
-- 
1.6.6.2

[PATCH v2 3/3] omap: gpmc-nand: add ability to keep timings defined by the bootloader

[Mike Rapoport]

Signed-off-by: Mike Rapoport <mike@compulab.co.il>
---
 arch/arm/mach-omap2/gpmc-nand.c        |   21 +++++++++++++++++----
 arch/arm/plat-omap/include/plat/nand.h |    1 +
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index 9434c80..96f0c7f 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -22,6 +22,7 @@
 #define WR_RD_PIN_MONITORING	0x00600000
 
 static struct omap_nand_platform_data *gpmc_nand_data;
+static struct gpmc_timings gpmc_default_timings;
 
 static struct resource gpmc_nand_resource = {
 	.flags		= IORESOURCE_MEM,
@@ -65,21 +66,28 @@ static void omap2_nand_gpmc_round_timings(struct gpmc_timings *src,
 
 static int omap2_nand_gpmc_retime(void)
 {
+	struct device *dev = &gpmc_nand_device.dev;
+	struct gpmc_timings *gpmc_t = gpmc_nand_data->gpmc_t;
 	struct gpmc_timings t;
 	int err;
 
-	if (!gpmc_nand_data->gpmc_t)
+	if (!gpmc_t) {
+		dev_warn(dev, "No timings provided, skipping retime\n");
 		return 0;
+	}
 
-	memset(&t, 0, sizeof(t));
-	omap2_nand_gpmc_round_timings(gpmc_nand_data->gpmc_t, &t);
+	if (!gpmc_nand_data->keep_timings) {
+		memset(&t, 0, sizeof(t));
+		omap2_nand_gpmc_round_timings(gpmc_nand_data->gpmc_t, &t);
+		gpmc_t = &t;
+	}
 
 	/* Configure GPMC */
 	gpmc_cs_write_reg(gpmc_nand_data->cs, GPMC_CS_CONFIG1,
 			GPMC_CONFIG1_DEVICESIZE(gpmc_nand_data->devsize) |
 			GPMC_CONFIG1_DEVICETYPE_NAND);
 
-	err = gpmc_cs_set_timings(gpmc_nand_data->cs, &t);
+	err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t);
 	if (err)
 		return err;
 
@@ -116,6 +124,11 @@ int __init gpmc_nand_init(struct omap_nand_platform_data *_nand_data)
 		return err;
 	}
 
+	if (gpmc_nand_data->keep_timings) {
+		gpmc_cs_get_timings(gpmc_nand_data->cs, &gpmc_default_timings);
+		gpmc_nand_data->gpmc_t = &gpmc_default_timings;
+	}
+
 	err = gpmc_nand_setup();
 	if (err < 0) {
 		dev_err(dev, "NAND platform setup failed: %d\n", err);
diff --git a/arch/arm/plat-omap/include/plat/nand.h b/arch/arm/plat-omap/include/plat/nand.h
index f8efd54..0f727ea 100644
--- a/arch/arm/plat-omap/include/plat/nand.h
+++ b/arch/arm/plat-omap/include/plat/nand.h
@@ -24,6 +24,7 @@ struct omap_nand_platform_data {
 	void __iomem		*gpmc_cs_baseaddr;
 	void __iomem		*gpmc_baseaddr;
 	int			devsize;
+	bool			keep_timings;
 };
 
 /* size (4 KiB) for IO mapping */
-- 
1.6.6.2

Re: [PATCH v2 3/3] omap: gpmc-nand: add ability to keep timings defined by the bootloader

[Tony Lindgren]

* Mike Rapoport <mike@compulab.co.il> [100429 01:44]:
> Signed-off-by: Mike Rapoport <mike@compulab.co.il>

Please add a proper description to all the patches.

> --- a/arch/arm/mach-omap2/gpmc-nand.c
> +++ b/arch/arm/mach-omap2/gpmc-nand.c
> @@ -116,6 +124,11 @@ int __init gpmc_nand_init(struct omap_nand_platform_data *_nand_data)
>  		return err;
>  	}
>  
> +	if (gpmc_nand_data->keep_timings) {
> +		gpmc_cs_get_timings(gpmc_nand_data->cs, &gpmc_default_timings);
> +		gpmc_nand_data->gpmc_t = &gpmc_default_timings;
> +	}
> +
>  	err = gpmc_nand_setup();
>  	if (err < 0) {
>  		dev_err(dev, "NAND platform setup failed: %d\n", err);

Hmm, so you're setting the timings based on the bootloader values?

I' think the problem with that is that chances are that it still won't
work for other L3 frequencies because of rounding errors.

With gpmc_cs_get_timings() you're already using tick rounded timings,
so you won't get the required accuracy out of those for the other
L3 frequencies.

So maybe just not do anything, and print a warning on gpmc L3 changes
if the timings are not set?

Regards,

Tony

Re: [PATCH v2 3/3] omap: gpmc-nand: add ability to keep timings defined by the bootloader

[Mike Rapoport]

On Mon, May 3, 2010 at 9:24 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Mike Rapoport <mike@compulab.co.il> [100429 01:44]:
>> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>
> Please add a proper description to all the patches.
>
>> --- a/arch/arm/mach-omap2/gpmc-nand.c
>> +++ b/arch/arm/mach-omap2/gpmc-nand.c
>> @@ -116,6 +124,11 @@ int __init gpmc_nand_init(struct omap_nand_platform_data *_nand_data)
>>               return err;
>>       }
>>
>> +     if (gpmc_nand_data->keep_timings) {
>> +             gpmc_cs_get_timings(gpmc_nand_data->cs, &gpmc_default_timings);
>> +             gpmc_nand_data->gpmc_t = &gpmc_default_timings;
>> +     }
>> +
>>       err = gpmc_nand_setup();
>>       if (err < 0) {
>>               dev_err(dev, "NAND platform setup failed: %d\n", err);
>
> Hmm, so you're setting the timings based on the bootloader values?
>
> I' think the problem with that is that chances are that it still won't
> work for other L3 frequencies because of rounding errors.
>
> With gpmc_cs_get_timings() you're already using tick rounded timings,
> so you won't get the required accuracy out of those for the other
> L3 frequencies.

Agree. But even if the timings are in nanoseconds there are rounding
errors, and there are still chances that L3 frequency change will
break NAND
So it comes down to what provides better tolerance, the explicit NAND
timings in nanosecs or (rounded) timings in ticks derived from
bootloader settings...

> So maybe just not do anything, and print a warning on gpmc L3 changes
> if the timings are not set?

I don't quite understand where exactly this warning should go. I
haven't found any treatment of L3 frequency changes in gpmc related
code neither in linux-omap nor in linux-omap-pm...

> Regards,
>
> Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
	Sincerely Yours,
		Mike.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] omap: gpmc-nand: add ability to keep timings defined by the bootloader

[Tony Lindgren]

* Mike Rapoport <mike.rapoport@gmail.com> [100503 13:28]:
> On Mon, May 3, 2010 at 9:24 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Mike Rapoport <mike@compulab.co.il> [100429 01:44]:
> >> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
> >
> > Please add a proper description to all the patches.
> >
> >> --- a/arch/arm/mach-omap2/gpmc-nand.c
> >> +++ b/arch/arm/mach-omap2/gpmc-nand.c
> >> @@ -116,6 +124,11 @@ int __init gpmc_nand_init(struct omap_nand_platform_data *_nand_data)
> >>               return err;
> >>       }
> >>
> >> +     if (gpmc_nand_data->keep_timings) {
> >> +             gpmc_cs_get_timings(gpmc_nand_data->cs, &gpmc_default_timings);
> >> +             gpmc_nand_data->gpmc_t = &gpmc_default_timings;
> >> +     }
> >> +
> >>       err = gpmc_nand_setup();
> >>       if (err < 0) {
> >>               dev_err(dev, "NAND platform setup failed: %d\n", err);
> >
> > Hmm, so you're setting the timings based on the bootloader values?
> >
> > I' think the problem with that is that chances are that it still won't
> > work for other L3 frequencies because of rounding errors.
> >
> > With gpmc_cs_get_timings() you're already using tick rounded timings,
> > so you won't get the required accuracy out of those for the other
> > L3 frequencies.
> 
> Agree. But even if the timings are in nanoseconds there are rounding
> errors, and there are still chances that L3 frequency change will
> break NAND
> So it comes down to what provides better tolerance, the explicit NAND
> timings in nanosecs or (rounded) timings in ticks derived from
> bootloader settings...

My experience is that you can get the nanosec timings from the device
datasheet(s) and that just should work for any L3 frequency. My
experience is also that trying to do it the other way around won't work
because of rounding errors. Trying to produce nanosecond values out
of the tick values just is not accurate enough.

That's why gpmc-onenand.c and usb-tusb6010.c timings are done the way
they are, and they're known to work at various L3 frequencies.

> > So maybe just not do anything, and print a warning on gpmc L3 changes
> > if the timings are not set?
> 
> I don't quite understand where exactly this warning should go. I
> haven't found any treatment of L3 frequency changes in gpmc related
> code neither in linux-omap nor in linux-omap-pm...

Ah, right. There's currently nothing doing that.. That would have to
be done based on cpufreq notifiers (or clock notifiers). But we don't
have any of that at least in the mainline yet. Hmm I don't even think
we currently scale the L3 for cpufreq.. Right now the best way to test
would be by booting at different L3 frequencies.

Anyways, my point is that setting gpmc_default_timings based on the
bootloader after doing the gpmc_cs_get_timings is most likely unsafe
for other L3 frequencies.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] omap: gpmc-nand: add ability to keep timings defined by the bootloader

[Mike Rapoport]

Tony Lindgren wrote:
> * Mike Rapoport <mike.rapoport@gmail.com> [100503 13:28]:

>> So it comes down to what provides better tolerance, the explicit NAND
>> timings in nanosecs or (rounded) timings in ticks derived from
>> bootloader settings...
> 
> My experience is that you can get the nanosec timings from the device
> datasheet(s) and that just should work for any L3 frequency.

And what about boards that can have different NAND flash chips 
assembled? What datasheet should be used to get the nanosecs? Note, that 
detecting NAND ID in the bootloader and adjusting timings appropriately 
is not that big deal, and doing it in the kernel seems to me really 
impractical.

> My experience is also that trying to do it the other way around won't work
> because of rounding errors. Trying to produce nanosecond values out
> of the tick values just is not accurate enough.

I'm still not convinced. Similar approach worked for me with several 
devices attached to sort of GPMC controllers on different SoC. There 
always was a way to set timings once in the bootloader and then detect 
the timings in the kernel and update them during cpu-freq transitions...

> That's why gpmc-onenand.c and usb-tusb6010.c timings are done the way
> they are, and they're known to work at various L3 frequencies.

I'm not really familiar with OneNAND, but looking at gpmc-onenand.c I
see hardcoded timings. Moreover, the nanosecs values seem to get 
adjusted for different L3 frequencies.
So, for NAND it would mean that platform would have to supply several 
timing sets for different L3 freqs?

>>> So maybe just not do anything, and print a warning on gpmc L3 changes
>>> if the timings are not set?
>> I don't quite understand where exactly this warning should go. I
>> haven't found any treatment of L3 frequency changes in gpmc related
>> code neither in linux-omap nor in linux-omap-pm...
> 
> Ah, right. There's currently nothing doing that.. That would have to
> be done based on cpufreq notifiers (or clock notifiers). But we don't
> have any of that at least in the mainline yet. Hmm I don't even think
> we currently scale the L3 for cpufreq.. Right now the best way to test
> would be by booting at different L3 frequencies.
> 
> Anyways, my point is that setting gpmc_default_timings based on the
> bootloader after doing the gpmc_cs_get_timings is most likely unsafe
> for other L3 frequencies.
> 
> Regards,
> 
> Tony


-- 
Sincerely yours,
Mike.

Re: [PATCH v2 3/3] omap: gpmc-nand: add ability to keep timings defined by the bootloader

[Tony Lindgren]

* Mike Rapoport <mike@compulab.co.il> [100504 06:19]:
> Tony Lindgren wrote:
> >* Mike Rapoport <mike.rapoport@gmail.com> [100503 13:28]:
> 
> >>So it comes down to what provides better tolerance, the explicit NAND
> >>timings in nanosecs or (rounded) timings in ticks derived from
> >>bootloader settings...
> >
> >My experience is that you can get the nanosec timings from the device
> >datasheet(s) and that just should work for any L3 frequency.
> 
> And what about boards that can have different NAND flash chips
> assembled? What datasheet should be used to get the nanosecs? Note,
> that detecting NAND ID in the bootloader and adjusting timings
> appropriately is not that big deal, and doing it in the kernel seems
> to me really impractical.

Hmm, if there are different variations of the same board, the best way
is to pass ATAG_REVISION from the bootloader. Then you can set the
selected NAND platform data based on the revision.
 
> >My experience is also that trying to do it the other way around won't work
> >because of rounding errors. Trying to produce nanosecond values out
> >of the tick values just is not accurate enough.
> 
> I'm still not convinced. Similar approach worked for me with several
> devices attached to sort of GPMC controllers on different SoC. There
> always was a way to set timings once in the bootloader and then
> detect the timings in the kernel and update them during cpu-freq
> transitions...

Not based on my experience with GPMC and L3.. When converting from GPMC
ticks to nanosecond timings, you're losing accuracy so things won't scale
the right way if you scale L3 frequency.

> >That's why gpmc-onenand.c and usb-tusb6010.c timings are done the way
> >they are, and they're known to work at various L3 frequencies.
> 
> I'm not really familiar with OneNAND, but looking at gpmc-onenand.c I
> see hardcoded timings. Moreover, the nanosecs values seem to get
> adjusted for different L3 frequencies.

Yeah, you need to look at the L3 frequency to calculate the timings.

> So, for NAND it would mean that platform would have to supply
> several timing sets for different L3 freqs?

Not needed if the GPMC tick timings are calculated based on the
device datasheet nanosecond timings and omap L3 frequency. Then it
scales for all L3 frequencies.

In any case, we still need to also allow using hardcoded bootloader
timings for people who don't care about the L3 scaling. But there
are other more serious things badly broken with the whole gpmc-nand
and drivers/mtd/nand/omap2.c, I'll send another email about that
shortly.

Regards,

Tony