[PATCH 0/4] ARM OMAP2+ GPMC: various fixes and bus children

[PATCH 0/4] ARM OMAP2+ GPMC: various fixes and bus children

[Robert ABEL]

I found a few more bugs in the GPMC driver. The first three patches should be self-explanatory:

*debug output was unaligned --> align it
*GPMCFCLKDIVIDER is used in all use cases --> always program it
*WAITMONITORINGTIME was computed in FCLK ticks instead of CLK ticks --> compute in CLK ticks

The last patch adds bus children to the GPMC. Currently, the GPMC driver
only supports direct children, which means one child with unique settings per CS region.

This patch adds explicit code inside the GPMC driver to probe for busses and
instantiate their children. (As opposed to adding the GPMC to of_default_bus_match_table
so it would be matched when the DT is first populated.)

GPMC properties apply to busses as well as direct children and map to whole CS regions.

Example:

&gpmc {
	ranges = </* CS0 */ 0 /* offset (broken) */ 0x0000 /* OCP address */ 0x1000000 /* size */ 0x1000000>;

	my_bus: nor@0,0x0000 { /* CS0; offset 0x0000 */

		compatible = "simple-bus";
		#address-cells = <1>;
		#size-cells = <1>;
		ranges = </* address */ 0x0000 /* parent address */ 0 0x0000 /* size */ 0x1000000>;
		reg = </* parent address */0 0x0000 /* size */ 0x1000000>;
		bank-width = <2>; /* GPMC_DEVWIDTH_16BIT */

		gpmc,sync-read;
		gpmc,sync-write;
		gpmc,sync-clk-ps = ...;
		... etc. ...
		reg-io-width = <2>;
		
		my_subdev0: subdev@0x0000 { /* address 0x0000 */

			compatible = "my,driver0";
			reg = </* parent address */ 0x0000 /* size */ 0x0800000>;

		};
		... etc. ...
		my_subdevN: subdev@0x8000 { /* address 0x8000 */

			compatible = "my,driverN";
			reg = </* parent address */ 0x0000 /* size */ 0x0800000>;

		};

	};

};


Robert ABEL (4):
  ARM OMAP2+ GPMC: fix debug output alignment
  ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER
  ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
  ARM OMAP2+ GPMC: add bus children

 arch/arm/mach-omap2/gpmc.c | 105 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 81 insertions(+), 24 deletions(-)

-- 
2.3.0

[PATCH 1/4] ARM OMAP2+ GPMC: fix debug output alignment

[Robert ABEL]

GPMC debug output is aligned to 10 characters for field names.
However, some fields have bigger names, screwing up the alignment.
Consequently, alignment was changed to longest field name (17 chars) for now.

Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
---
 arch/arm/mach-omap2/gpmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 1ab6bc0..5c3639c 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -296,7 +296,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 	nr_bits = end_bit - st_bit + 1;
 	if (ticks >= 1 << nr_bits) {
 #ifdef DEBUG
-		printk(KERN_INFO "GPMC CS%d: %-10s* %3d ns, %3d ticks >= %d\n",
+		printk(KERN_INFO "GPMC CS%d: %-17s* %3d ns, %3d ticks >= %d\n",
 				cs, name, time, ticks, 1 << nr_bits);
 #endif
 		return -1;
@@ -306,7 +306,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 	l = gpmc_cs_read_reg(cs, reg);
 #ifdef DEBUG
 	printk(KERN_INFO
-		"GPMC CS%d: %-10s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
+		"GPMC CS%d: %-17s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
 	       cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
 			(l >> st_bit) & mask, time);
 #endif
-- 
2.3.0

[PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert ABEL]

GPMC uses GPMCFCLKDIVIDER during synchronous as well as asynchronous accesses
in conjunction with WAITMONITORINGTIME. Thus, it's wrong to only program it for
synchronous accesses. Remove the conditional.

Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
---
 arch/arm/mach-omap2/gpmc.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5c3639c..bae4a20 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -382,19 +382,14 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
 	if (gpmc_capability & GPMC_HAS_WR_ACCESS)
 		GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
 
-	/* caller is expected to have initialized CONFIG1 to cover
-	 * at least sync vs async
-	 */
 	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
-	if (l & (GPMC_CONFIG1_READTYPE_SYNC | GPMC_CONFIG1_WRITETYPE_SYNC)) {
 #ifdef DEBUG
-		printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
-				cs, (div * gpmc_get_fclk_period()) / 1000, div);
+	printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
+	       cs, (div * gpmc_get_fclk_period()) / 1000, div);
 #endif
-		l &= ~0x03;
-		l |= (div - 1);
-		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
-	}
+	l &= ~0x03;
+	l |= (div - 1);
+	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
 
 	gpmc_cs_bool_timings(cs, &t->bool_timings);
 
-- 
2.3.0

[PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Robert ABEL]

WAITMONITORINGTIME is expressed in GPMC_CLK cycles (even for asynchronous accesses).
However the driver currently converts them to GPMC_FCLK cycles, thus waitmonitoringtime in dt
had to be predivided by divider as a workaround. This patch fixes the issue by reading the
current GPMCFCLKDIVIDER and adjusting the divisor for WAITMONITORINGTIME accordingly.

Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
---
 arch/arm/mach-omap2/gpmc.c | 78 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 64 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index bae4a20..4fa5ff1 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -113,6 +113,9 @@
  */
 #define	GPMC_NR_IRQ		2
 
+#define GPMC_FCLK       0
+#define GPMC_CLK        1
+
 struct gpmc_client_irq	{
 	unsigned		irq;
 	u32			bitmask;
@@ -208,16 +211,55 @@ static unsigned long gpmc_get_fclk_period(void)
 	return rate;
 }
 
-static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
+/**
+ * gpmc_get_clk_period - get period of selected clk in ps
+ * @cs     : affected chip select
+ * @clk_sel: either one of GPMC_CLK or GPMC_FCLK
+ *
+ * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
+ * prior to calling this function with GPMC_CLK.
+ * 
+ */
+static unsigned long gpmc_get_clk_period(int cs, unsigned int clk_sel)
+{
+
+	unsigned long tick_ps = gpmc_get_fclk_period();
+	u32 l;
+	int div;
+
+	switch (clk_sel) {
+	case GPMC_CLK:
+		/* get current clk divider */
+		l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
+		div = (l & 0x03) + 1;
+		/* get GPMC_CLK period */
+		tick_ps *= div;
+		break;
+	case GPMC_FCLK:
+		/* FALL-THROUGH */
+	default:
+		break;
+	}
+
+	return tick_ps;
+
+}
+
+static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, unsigned int clk_sel)
 {
 	unsigned long tick_ps;
 
 	/* Calculate in picosecs to yield more exact results */
-	tick_ps = gpmc_get_fclk_period();
+	tick_ps = gpmc_get_clk_period(cs, clk_sel);
 
 	return (time_ns * 1000 + tick_ps - 1) / tick_ps;
 }
 
+static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
+{
+	return gpmc_ns_to_clk_ticks(time_ns, 0, GPMC_FCLK);
+}
+
 static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
 {
 	unsigned long tick_ps;
@@ -280,10 +322,10 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
 
 #ifdef DEBUG
 static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
-			       int time, const char *name)
+			       int time, unsigned int clk_sel, const char *name)
 #else
 static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
-			       int time)
+			       int time, unsigned int clk_sel)
 #endif
 {
 	u32 l;
@@ -292,7 +334,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 	if (time == 0)
 		ticks = 0;
 	else
-		ticks = gpmc_ns_to_ticks(time);
+		ticks = gpmc_ns_to_clk_ticks(time, cs, clk_sel);
 	nr_bits = end_bit - st_bit + 1;
 	if (ticks >= 1 << nr_bits) {
 #ifdef DEBUG
@@ -307,7 +349,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 #ifdef DEBUG
 	printk(KERN_INFO
 		"GPMC CS%d: %-17s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
-	       cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
+	       cs, name, ticks, gpmc_get_clk_period(cs, clk_sel) * ticks / 1000,
 			(l >> st_bit) & mask, time);
 #endif
 	l &= ~(mask << st_bit);
@@ -320,12 +362,19 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 #ifdef DEBUG
 #define GPMC_SET_ONE(reg, st, end, field) \
 	if (set_gpmc_timing_reg(cs, (reg), (st), (end),		\
-			t->field, #field) < 0)			\
+			t->field, GPMC_FCLK, #field) < 0)			\
 		return -1
+#define GPMC_SET_ONE_C(reg, st, end, field, clk_sel) \
+	if (set_gpmc_timing_reg(cs, (reg), (st), (end),     \
+            t->field, clk_sel, #field) < 0)             \
+        return -1
 #else
 #define GPMC_SET_ONE(reg, st, end, field) \
-	if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field) < 0) \
+	if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field, GPMC_FCLK) < 0) \
 		return -1
+#define GPMC_SET_ONE_C(reg, st, end, field, clk_sel) \
+	if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field, clk_sel) < 0)   \
+        return -1
 #endif
 
 int gpmc_calc_divider(unsigned int sync_clk)
@@ -374,22 +423,23 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
 	GPMC_SET_ONE(GPMC_CS_CONFIG6, 0, 3, bus_turnaround);
 	GPMC_SET_ONE(GPMC_CS_CONFIG6, 8, 11, cycle2cycle_delay);
 
-	GPMC_SET_ONE(GPMC_CS_CONFIG1, 18, 19, wait_monitoring);
-	GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
-
 	if (gpmc_capability & GPMC_HAS_WR_DATA_MUX_BUS)
 		GPMC_SET_ONE(GPMC_CS_CONFIG6, 16, 19, wr_data_mux_bus);
 	if (gpmc_capability & GPMC_HAS_WR_ACCESS)
 		GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
 
 	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
+	l &= ~0x03;
+	l |= (div - 1);
+	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
+
+	GPMC_SET_ONE_C(GPMC_CS_CONFIG1, 18, 19, wait_monitoring, GPMC_CLK);
+	GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
+
 #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
-	l &= ~0x03;
-	l |= (div - 1);
-	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
 
 	gpmc_cs_bool_timings(cs, &t->bool_timings);
 
-- 
2.3.0

[PATCH 4/4] ARM OMAP2+ GPMC: add bus children

[Robert ABEL]

This patch adds support for spawning busses as children of the GPMC.

Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
---
 arch/arm/mach-omap2/gpmc.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 4fa5ff1..c6c8543 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -27,6 +27,7 @@
 #include <linux/of_address.h>
 #include <linux/of_mtd.h>
 #include <linux/of_device.h>
+#include <linux/of_platform.h>
 #include <linux/mtd/nand.h>
 #include <linux/pm_runtime.h>
 
@@ -1589,9 +1590,20 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 	gpmc_cs_set_timings(cs, &gpmc_t);
 
 no_timings:
-	if (of_platform_device_create(child, NULL, &pdev->dev))
+
+	if (of_match_node(of_default_bus_match_table, child)) {
+
+		/* ignore return code, because 0 is ambiguous */
+		of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev);
 		return 0;
 
+	} else {
+
+		if (of_platform_device_create(child, NULL, &pdev->dev))
+			return 0;
+
+	}
+
 	dev_err(&pdev->dev, "failed to create gpmc child %s\n", child->name);
 	ret = -ENODEV;
 
-- 
2.3.0

Re: [PATCH 4/4] ARM OMAP2+ GPMC: add bus children

[Roger Quadros]

On 16/02/15 17:49, Robert ABEL wrote:
> This patch adds support for spawning busses as children of the GPMC.
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  arch/arm/mach-omap2/gpmc.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 4fa5ff1..c6c8543 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -27,6 +27,7 @@
>  #include <linux/of_address.h>
>  #include <linux/of_mtd.h>
>  #include <linux/of_device.h>
> +#include <linux/of_platform.h>
>  #include <linux/mtd/nand.h>
>  #include <linux/pm_runtime.h>
>  
> @@ -1589,9 +1590,20 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  	gpmc_cs_set_timings(cs, &gpmc_t);
>  
>  no_timings:
> -	if (of_platform_device_create(child, NULL, &pdev->dev))
> +
> +	if (of_match_node(of_default_bus_match_table, child)) {
> +
> +		/* ignore return code, because 0 is ambiguous */
> +		of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev);
>  		return 0;
>  
> +	} else {
> +
> +		if (of_platform_device_create(child, NULL, &pdev->dev))
> +			return 0;
> +
> +	}
> +

Can we simply use only
of_platform_populate(child, NULL, NULL, &pdev->dev)

That way we get rid of the if..else and let OF core take care of
creating buses or devices.

>  	dev_err(&pdev->dev, "failed to create gpmc child %s\n", child->name);
>  	ret = -ENODEV;
>  
> 

cheers,
-roger

Re: [PATCH 4/4] ARM OMAP2+ GPMC: add bus children

[Roger Quadros]

On 16/02/15 17:49, Robert ABEL wrote:
> This patch adds support for spawning busses as children of the GPMC.
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  arch/arm/mach-omap2/gpmc.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 4fa5ff1..c6c8543 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -27,6 +27,7 @@
>  #include <linux/of_address.h>
>  #include <linux/of_mtd.h>
>  #include <linux/of_device.h>
> +#include <linux/of_platform.h>
>  #include <linux/mtd/nand.h>
>  #include <linux/pm_runtime.h>
>  
> @@ -1589,9 +1590,20 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  	gpmc_cs_set_timings(cs, &gpmc_t);
>  
>  no_timings:
> -	if (of_platform_device_create(child, NULL, &pdev->dev))
> +
> +	if (of_match_node(of_default_bus_match_table, child)) {
> +
> +		/* ignore return code, because 0 is ambiguous */
> +		of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev);
>  		return 0;
>  
> +	} else {
> +
> +		if (of_platform_device_create(child, NULL, &pdev->dev))
> +			return 0;
> +
> +	}
> +

Can we simply use only
of_platform_populate(child, NULL, NULL, &pdev->dev)

That way we get rid of the if..else and let OF core take care of
creating buses or devices.

>  	dev_err(&pdev->dev, "failed to create gpmc child %s\n", child->name);
>  	ret = -ENODEV;
>  
> 

cheers,
-roger

Re: [PATCH 4/4] ARM OMAP2+ GPMC: add bus children

[Robert Abel]

Hi Roger,

On Tue, Feb 17, 2015 at 10:41 AM, Roger Quadros <rogerq@ti.com> wrote:>
> Can we simply use only
> of_platform_populate(child, NULL, NULL, &pdev->dev)
>
> That way we get rid of the if..else and let OF core take care of
> creating buses or devices.

Surely, you mean

>of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev);

? If matches is NULL, we won't match any busses. Or did you mean to
just replace the second conditional?

I'll try that and see it works.

Regards,

Robert

Re: [PATCH 4/4] ARM OMAP2+ GPMC: add bus children

[Roger Quadros]

On 17/02/15 15:57, Robert Abel wrote:
> Hi Roger,
> 
> On Tue, Feb 17, 2015 at 10:41 AM, Roger Quadros <rogerq@ti.com> wrote:>
>> Can we simply use only
>> of_platform_populate(child, NULL, NULL, &pdev->dev)
>>
>> That way we get rid of the if..else and let OF core take care of
>> creating buses or devices.
> 
> Surely, you mean
> 
>> of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev);
> 
> ? If matches is NULL, we won't match any busses. Or did you mean to
> just replace the second conditional?

I was wondering if just one call to of_platform_populate() is sufficient.
so you probably need "of_default_bus_match_table" for matches.

> 
> I'll try that and see it works.
> 

cheers,
-roger

Re: [PATCH 4/4] ARM OMAP2+ GPMC: add bus children

[Roger Quadros]

On 17/02/15 15:57, Robert Abel wrote:
> Hi Roger,
> 
> On Tue, Feb 17, 2015 at 10:41 AM, Roger Quadros <rogerq@ti.com> wrote:>
>> Can we simply use only
>> of_platform_populate(child, NULL, NULL, &pdev->dev)
>>
>> That way we get rid of the if..else and let OF core take care of
>> creating buses or devices.
> 
> Surely, you mean
> 
>> of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev);
> 
> ? If matches is NULL, we won't match any busses. Or did you mean to
> just replace the second conditional?

I was wondering if just one call to of_platform_populate() is sufficient.
so you probably need "of_default_bus_match_table" for matches.

> 
> I'll try that and see it works.
> 

cheers,
-roger

Re: [PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Roger Quadros]

Robert,

On 16/02/15 17:49, Robert ABEL wrote:
> WAITMONITORINGTIME is expressed in GPMC_CLK cycles (even for asynchronous accesses).
> However the driver currently converts them to GPMC_FCLK cycles, thus waitmonitoringtime in dt
> had to be predivided by divider as a workaround. This patch fixes the issue by reading the

Can you please point out which DT had used this pre-divided workaround? We will have to
fix those then.

> current GPMCFCLKDIVIDER and adjusting the divisor for WAITMONITORINGTIME accordingly.
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  arch/arm/mach-omap2/gpmc.c | 78 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 64 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c

Can you please rebase your patches on top of v3.19?
gpmc.c has been moved to drivers/memory/omap-gpmc.c

> index bae4a20..4fa5ff1 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -113,6 +113,9 @@
>   */
>  #define	GPMC_NR_IRQ		2
>  
> +#define GPMC_FCLK       0
> +#define GPMC_CLK        1
> +

enum gpmc_clksel {
	GPMC_CLKSEL_FCLK,
	GPMC_CLKSEL_CLK,
};

So all the below occurrences of "unsigned int clk_sel" become "enum gpmc_clksel".

>  struct gpmc_client_irq	{
>  	unsigned		irq;
>  	u32			bitmask;
> @@ -208,16 +211,55 @@ static unsigned long gpmc_get_fclk_period(void)
>  	return rate;
>  }
>  
> -static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
> +/**
> + * gpmc_get_clk_period - get period of selected clk in ps
> + * @cs     : affected chip select
> + * @clk_sel: either one of GPMC_CLK or GPMC_FCLK
> + *
> + * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
> + * prior to calling this function with GPMC_CLK.
> + * 
> + */
> +static unsigned long gpmc_get_clk_period(int cs, unsigned int clk_sel)
> +{
> +
> +	unsigned long tick_ps = gpmc_get_fclk_period();
> +	u32 l;
> +	int div;
> +
> +	switch (clk_sel) {
> +	case GPMC_CLK:
> +		/* get current clk divider */
> +		l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> +		div = (l & 0x03) + 1;
> +		/* get GPMC_CLK period */
> +		tick_ps *= div;
> +		break;
> +	case GPMC_FCLK:
> +		/* FALL-THROUGH */
> +	default:
> +		break;
> +	}
> +
> +	return tick_ps;
> +
> +}
> +
> +static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, unsigned int clk_sel)
>  {
>  	unsigned long tick_ps;
>  
>  	/* Calculate in picosecs to yield more exact results */
> -	tick_ps = gpmc_get_fclk_period();
> +	tick_ps = gpmc_get_clk_period(cs, clk_sel);
>  
>  	return (time_ns * 1000 + tick_ps - 1) / tick_ps;
>  }
>  
> +static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
> +{
> +	return gpmc_ns_to_clk_ticks(time_ns, 0, GPMC_FCLK);

This function should have been unchanged since we're dealing with GPMC_FCLK
and it was using gpmc_get_fclk_period().

> +}
> +
>  static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
>  {
>  	unsigned long tick_ps;
> @@ -280,10 +322,10 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>  
>  #ifdef DEBUG
>  static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> -			       int time, const char *name)
> +			       int time, unsigned int clk_sel, const char *name)
>  #else
>  static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> -			       int time)
> +			       int time, unsigned int clk_sel)
>  #endif
>  {
>  	u32 l;
> @@ -292,7 +334,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  	if (time == 0)
>  		ticks = 0;
>  	else
> -		ticks = gpmc_ns_to_ticks(time);
> +		ticks = gpmc_ns_to_clk_ticks(time, cs, clk_sel);
>  	nr_bits = end_bit - st_bit + 1;
>  	if (ticks >= 1 << nr_bits) {
>  #ifdef DEBUG
> @@ -307,7 +349,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  #ifdef DEBUG
>  	printk(KERN_INFO
>  		"GPMC CS%d: %-17s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
> -	       cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
> +	       cs, name, ticks, gpmc_get_clk_period(cs, clk_sel) * ticks / 1000,
>  			(l >> st_bit) & mask, time);
>  #endif
>  	l &= ~(mask << st_bit);
> @@ -320,12 +362,19 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  #ifdef DEBUG
>  #define GPMC_SET_ONE(reg, st, end, field) \
>  	if (set_gpmc_timing_reg(cs, (reg), (st), (end),		\
> -			t->field, #field) < 0)			\
> +			t->field, GPMC_FCLK, #field) < 0)			\
>  		return -1
> +#define GPMC_SET_ONE_C(reg, st, end, field, clk_sel) \

Let's call this GPMC_SET_ONE_CLKSEL()

> +	if (set_gpmc_timing_reg(cs, (reg), (st), (end),     \
> +            t->field, clk_sel, #field) < 0)             \
> +        return -1
>  #else
>  #define GPMC_SET_ONE(reg, st, end, field) \
> -	if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field) < 0) \
> +	if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field, GPMC_FCLK) < 0) \
>  		return -1
> +#define GPMC_SET_ONE_C(reg, st, end, field, clk_sel) \
> +	if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field, clk_sel) < 0)   \
> +        return -1
>  #endif
>  
>  int gpmc_calc_divider(unsigned int sync_clk)
> @@ -374,22 +423,23 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
>  	GPMC_SET_ONE(GPMC_CS_CONFIG6, 0, 3, bus_turnaround);
>  	GPMC_SET_ONE(GPMC_CS_CONFIG6, 8, 11, cycle2cycle_delay);
>  
> -	GPMC_SET_ONE(GPMC_CS_CONFIG1, 18, 19, wait_monitoring);
> -	GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
> -
>  	if (gpmc_capability & GPMC_HAS_WR_DATA_MUX_BUS)
>  		GPMC_SET_ONE(GPMC_CS_CONFIG6, 16, 19, wr_data_mux_bus);
>  	if (gpmc_capability & GPMC_HAS_WR_ACCESS)
>  		GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
>  
>  	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> +	l &= ~0x03;
> +	l |= (div - 1);
> +	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
> +
> +	GPMC_SET_ONE_C(GPMC_CS_CONFIG1, 18, 19, wait_monitoring, GPMC_CLK);
> +	GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
> +
>  #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
> -	l &= ~0x03;
> -	l |= (div - 1);
> -	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
>  
>  	gpmc_cs_bool_timings(cs, &t->bool_timings);
>  
> 

You will also need to correct gpmc_cs_show_timings() to show the
correct timing for "wait-monitoring-ns" based on GPMC_CLK.

cheers,
-roger

Re: [PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Roger Quadros]

Robert,

On 16/02/15 17:49, Robert ABEL wrote:
> WAITMONITORINGTIME is expressed in GPMC_CLK cycles (even for asynchronous accesses).
> However the driver currently converts them to GPMC_FCLK cycles, thus waitmonitoringtime in dt
> had to be predivided by divider as a workaround. This patch fixes the issue by reading the

Can you please point out which DT had used this pre-divided workaround? We will have to
fix those then.

> current GPMCFCLKDIVIDER and adjusting the divisor for WAITMONITORINGTIME accordingly.
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  arch/arm/mach-omap2/gpmc.c | 78 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 64 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c

Can you please rebase your patches on top of v3.19?
gpmc.c has been moved to drivers/memory/omap-gpmc.c

> index bae4a20..4fa5ff1 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -113,6 +113,9 @@
>   */
>  #define	GPMC_NR_IRQ		2
>  
> +#define GPMC_FCLK       0
> +#define GPMC_CLK        1
> +

enum gpmc_clksel {
	GPMC_CLKSEL_FCLK,
	GPMC_CLKSEL_CLK,
};

So all the below occurrences of "unsigned int clk_sel" become "enum gpmc_clksel".

>  struct gpmc_client_irq	{
>  	unsigned		irq;
>  	u32			bitmask;
> @@ -208,16 +211,55 @@ static unsigned long gpmc_get_fclk_period(void)
>  	return rate;
>  }
>  
> -static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
> +/**
> + * gpmc_get_clk_period - get period of selected clk in ps
> + * @cs     : affected chip select
> + * @clk_sel: either one of GPMC_CLK or GPMC_FCLK
> + *
> + * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
> + * prior to calling this function with GPMC_CLK.
> + * 
> + */
> +static unsigned long gpmc_get_clk_period(int cs, unsigned int clk_sel)
> +{
> +
> +	unsigned long tick_ps = gpmc_get_fclk_period();
> +	u32 l;
> +	int div;
> +
> +	switch (clk_sel) {
> +	case GPMC_CLK:
> +		/* get current clk divider */
> +		l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> +		div = (l & 0x03) + 1;
> +		/* get GPMC_CLK period */
> +		tick_ps *= div;
> +		break;
> +	case GPMC_FCLK:
> +		/* FALL-THROUGH */
> +	default:
> +		break;
> +	}
> +
> +	return tick_ps;
> +
> +}
> +
> +static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, unsigned int clk_sel)
>  {
>  	unsigned long tick_ps;
>  
>  	/* Calculate in picosecs to yield more exact results */
> -	tick_ps = gpmc_get_fclk_period();
> +	tick_ps = gpmc_get_clk_period(cs, clk_sel);
>  
>  	return (time_ns * 1000 + tick_ps - 1) / tick_ps;
>  }
>  
> +static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
> +{
> +	return gpmc_ns_to_clk_ticks(time_ns, 0, GPMC_FCLK);

This function should have been unchanged since we're dealing with GPMC_FCLK
and it was using gpmc_get_fclk_period().

> +}
> +
>  static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
>  {
>  	unsigned long tick_ps;
> @@ -280,10 +322,10 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>  
>  #ifdef DEBUG
>  static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> -			       int time, const char *name)
> +			       int time, unsigned int clk_sel, const char *name)
>  #else
>  static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> -			       int time)
> +			       int time, unsigned int clk_sel)
>  #endif
>  {
>  	u32 l;
> @@ -292,7 +334,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  	if (time == 0)
>  		ticks = 0;
>  	else
> -		ticks = gpmc_ns_to_ticks(time);
> +		ticks = gpmc_ns_to_clk_ticks(time, cs, clk_sel);
>  	nr_bits = end_bit - st_bit + 1;
>  	if (ticks >= 1 << nr_bits) {
>  #ifdef DEBUG
> @@ -307,7 +349,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  #ifdef DEBUG
>  	printk(KERN_INFO
>  		"GPMC CS%d: %-17s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
> -	       cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
> +	       cs, name, ticks, gpmc_get_clk_period(cs, clk_sel) * ticks / 1000,
>  			(l >> st_bit) & mask, time);
>  #endif
>  	l &= ~(mask << st_bit);
> @@ -320,12 +362,19 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  #ifdef DEBUG
>  #define GPMC_SET_ONE(reg, st, end, field) \
>  	if (set_gpmc_timing_reg(cs, (reg), (st), (end),		\
> -			t->field, #field) < 0)			\
> +			t->field, GPMC_FCLK, #field) < 0)			\
>  		return -1
> +#define GPMC_SET_ONE_C(reg, st, end, field, clk_sel) \

Let's call this GPMC_SET_ONE_CLKSEL()

> +	if (set_gpmc_timing_reg(cs, (reg), (st), (end),     \
> +            t->field, clk_sel, #field) < 0)             \
> +        return -1
>  #else
>  #define GPMC_SET_ONE(reg, st, end, field) \
> -	if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field) < 0) \
> +	if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field, GPMC_FCLK) < 0) \
>  		return -1
> +#define GPMC_SET_ONE_C(reg, st, end, field, clk_sel) \
> +	if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field, clk_sel) < 0)   \
> +        return -1
>  #endif
>  
>  int gpmc_calc_divider(unsigned int sync_clk)
> @@ -374,22 +423,23 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
>  	GPMC_SET_ONE(GPMC_CS_CONFIG6, 0, 3, bus_turnaround);
>  	GPMC_SET_ONE(GPMC_CS_CONFIG6, 8, 11, cycle2cycle_delay);
>  
> -	GPMC_SET_ONE(GPMC_CS_CONFIG1, 18, 19, wait_monitoring);
> -	GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
> -
>  	if (gpmc_capability & GPMC_HAS_WR_DATA_MUX_BUS)
>  		GPMC_SET_ONE(GPMC_CS_CONFIG6, 16, 19, wr_data_mux_bus);
>  	if (gpmc_capability & GPMC_HAS_WR_ACCESS)
>  		GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
>  
>  	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> +	l &= ~0x03;
> +	l |= (div - 1);
> +	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
> +
> +	GPMC_SET_ONE_C(GPMC_CS_CONFIG1, 18, 19, wait_monitoring, GPMC_CLK);
> +	GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
> +
>  #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
> -	l &= ~0x03;
> -	l |= (div - 1);
> -	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
>  
>  	gpmc_cs_bool_timings(cs, &t->bool_timings);
>  
> 

You will also need to correct gpmc_cs_show_timings() to show the
correct timing for "wait-monitoring-ns" based on GPMC_CLK.

cheers,
-roger

Re: [PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Robert Abel]

Hi Roger,

On Tue, Feb 17, 2015 at 10:27 AM, Roger Quadros <rogerq@ti.com> wrote:
>
> Can you please point out which DT had used this pre-divided workaround? We will have to
> fix those then.

I didn't check. A cursory glance reveals all DTS in arch/arm/boot/dts
to use a value of 0.

>
> Can you please rebase your patches on top of v3.19?
> gpmc.c has been moved to drivers/memory/omap-gpmc.c


Will do.

>
> So all the below occurrences of "unsigned int clk_sel" become "enum gpmc_clksel".

Yes, I should have opted for a cleaner solution from the start.

> > +static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, unsigned int clk_sel)
> >  {
> >       unsigned long tick_ps;
> >
> >       /* Calculate in picosecs to yield more exact results */
> > -     tick_ps = gpmc_get_fclk_period();
> > +     tick_ps = gpmc_get_clk_period(cs, clk_sel);
> >
> >       return (time_ns * 1000 + tick_ps - 1) / tick_ps;
> >  }
> >
> > +static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
> > +{
> > +     return gpmc_ns_to_clk_ticks(time_ns, 0, GPMC_FCLK);
>
> This function should have been unchanged since we're dealing with GPMC_FCLK
> and it was using gpmc_get_fclk_period().


Which function? gpmc_ns_to_ticks? It still uses FCLK. I merely did not
want to have the picosecond formula in two places. I don't see a good
reason to do so now either...

>
> Let's call this GPMC_SET_ONE_CLKSEL()


Will do.


>
> You will also need to correct gpmc_cs_show_timings() to show the
> correct timing for "wait-monitoring-ns" based on GPMC_CLK.


I was working off 3.14, checked if it worked for 3.17, so both didn't
have gpmc_cs_show_timings. I'll take a look.

Regards,

Robert

Re: [PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Roger Quadros]

On 17/02/15 15:48, Robert Abel wrote:
> Hi Roger,
> 
> On Tue, Feb 17, 2015 at 10:27 AM, Roger Quadros <rogerq@ti.com> wrote:
>>
>> Can you please point out which DT had used this pre-divided workaround? We will have to
>> fix those then.
> 
> I didn't check. A cursory glance reveals all DTS in arch/arm/boot/dts
> to use a value of 0.
> 
>>
>> Can you please rebase your patches on top of v3.19?
>> gpmc.c has been moved to drivers/memory/omap-gpmc.c
> 
> 
> Will do.
> 
>>
>> So all the below occurrences of "unsigned int clk_sel" become "enum gpmc_clksel".
> 
> Yes, I should have opted for a cleaner solution from the start.
> 
>>> +static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, unsigned int clk_sel)
>>>  {
>>>       unsigned long tick_ps;
>>>
>>>       /* Calculate in picosecs to yield more exact results */
>>> -     tick_ps = gpmc_get_fclk_period();
>>> +     tick_ps = gpmc_get_clk_period(cs, clk_sel);
>>>
>>>       return (time_ns * 1000 + tick_ps - 1) / tick_ps;
>>>  }
>>>
>>> +static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
>>> +{
>>> +     return gpmc_ns_to_clk_ticks(time_ns, 0, GPMC_FCLK);
>>
>> This function should have been unchanged since we're dealing with GPMC_FCLK
>> and it was using gpmc_get_fclk_period().
> 
> 
> Which function? gpmc_ns_to_ticks? It still uses FCLK. I merely did not
> want to have the picosecond formula in two places. I don't see a good
> reason to do so now either...

Agree with you. I missed to see earlier that it was getting rid of the
picosecond formula.

> 
>>
>> Let's call this GPMC_SET_ONE_CLKSEL()
> 
> 
> Will do.
> 
> 
>>
>> You will also need to correct gpmc_cs_show_timings() to show the
>> correct timing for "wait-monitoring-ns" based on GPMC_CLK.
> 
> 
> I was working off 3.14, checked if it worked for 3.17, so both didn't
> have gpmc_cs_show_timings. I'll take a look.
> 
Thanks.

cheers,
-roger

Re: [PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Roger Quadros]

On 17/02/15 15:48, Robert Abel wrote:
> Hi Roger,
> 
> On Tue, Feb 17, 2015 at 10:27 AM, Roger Quadros <rogerq@ti.com> wrote:
>>
>> Can you please point out which DT had used this pre-divided workaround? We will have to
>> fix those then.
> 
> I didn't check. A cursory glance reveals all DTS in arch/arm/boot/dts
> to use a value of 0.
> 
>>
>> Can you please rebase your patches on top of v3.19?
>> gpmc.c has been moved to drivers/memory/omap-gpmc.c
> 
> 
> Will do.
> 
>>
>> So all the below occurrences of "unsigned int clk_sel" become "enum gpmc_clksel".
> 
> Yes, I should have opted for a cleaner solution from the start.
> 
>>> +static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, unsigned int clk_sel)
>>>  {
>>>       unsigned long tick_ps;
>>>
>>>       /* Calculate in picosecs to yield more exact results */
>>> -     tick_ps = gpmc_get_fclk_period();
>>> +     tick_ps = gpmc_get_clk_period(cs, clk_sel);
>>>
>>>       return (time_ns * 1000 + tick_ps - 1) / tick_ps;
>>>  }
>>>
>>> +static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
>>> +{
>>> +     return gpmc_ns_to_clk_ticks(time_ns, 0, GPMC_FCLK);
>>
>> This function should have been unchanged since we're dealing with GPMC_FCLK
>> and it was using gpmc_get_fclk_period().
> 
> 
> Which function? gpmc_ns_to_ticks? It still uses FCLK. I merely did not
> want to have the picosecond formula in two places. I don't see a good
> reason to do so now either...

Agree with you. I missed to see earlier that it was getting rid of the
picosecond formula.

> 
>>
>> Let's call this GPMC_SET_ONE_CLKSEL()
> 
> 
> Will do.
> 
> 
>>
>> You will also need to correct gpmc_cs_show_timings() to show the
>> correct timing for "wait-monitoring-ns" based on GPMC_CLK.
> 
> 
> I was working off 3.14, checked if it worked for 3.17, so both didn't
> have gpmc_cs_show_timings. I'll take a look.
> 
Thanks.

cheers,
-roger

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Tony Lindgren]

* Robert ABEL <rabel@cit-ec.uni-bielefeld.de> [150216 07:52]:
> GPMC uses GPMCFCLKDIVIDER during synchronous as well as asynchronous accesses
> in conjunction with WAITMONITORINGTIME. Thus, it's wrong to only program it for
> synchronous accesses. Remove the conditional.

Do you have some test case that gets fixed by this? Or does this
fix some regression?

The reason why I'm asking is that AFAIK we've had async access
working all along?

Reagrds,

Tony


 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  arch/arm/mach-omap2/gpmc.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 5c3639c..bae4a20 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -382,19 +382,14 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
>  	if (gpmc_capability & GPMC_HAS_WR_ACCESS)
>  		GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
>  
> -	/* caller is expected to have initialized CONFIG1 to cover
> -	 * at least sync vs async
> -	 */
>  	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> -	if (l & (GPMC_CONFIG1_READTYPE_SYNC | GPMC_CONFIG1_WRITETYPE_SYNC)) {
>  #ifdef DEBUG
> -		printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
> -				cs, (div * gpmc_get_fclk_period()) / 1000, div);
> +	printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
> +	       cs, (div * gpmc_get_fclk_period()) / 1000, div);
>  #endif
> -		l &= ~0x03;
> -		l |= (div - 1);
> -		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
> -	}
> +	l &= ~0x03;
> +	l |= (div - 1);
> +	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
>  
>  	gpmc_cs_bool_timings(cs, &t->bool_timings);
>  
> -- 
> 2.3.0
> 

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert Abel]

Hi Tony,

On 16 Feb 2015 18:10, Tony Lindgren wrote:
> * Robert ABEL <rabel@cit-ec.uni-bielefeld.de> [150216 07:52]:
>> GPMC uses GPMCFCLKDIVIDER during synchronous as well as asynchronous accesses
>> in conjunction with WAITMONITORINGTIME. Thus, it's wrong to only program it for
>> synchronous accesses. Remove the conditional.
> Do you have some test case that gets fixed by this? Or does this
> fix some regression?
>
> The reason why I'm asking is that AFAIK we've had async access
> working all along?
Maybe to clarify: This is only affects asynchronous accesses with 
WAITREADMONITORING and/or WAITWRITEMONITORING enabled. Regular 
asynchronous accesses without WAIT pin monitoring were never affected to 
begin with and are not affected by this change.

I don't have an off-hand test case and seeing as the current 
implementation is faulty, it's probable nobody actively uses this 
feature. Cf. OMAP35xx TRM (SPRUF98X) pp. 1103. Basically, 
WAITMONITORINGTIME extends the number of cycles from the end of WAIT 
asserted to when the data is provided/sampled and the read/write access 
(should) take place. While this behavior is esoteric, it's not a reason 
not to fix this oversight.

Regards,

Robert

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Roger Quadros]

Hi Robert,

On 16/02/15 17:48, Robert ABEL wrote:
> GPMC uses GPMCFCLKDIVIDER during synchronous as well as asynchronous accesses
> in conjunction with WAITMONITORINGTIME. Thus, it's wrong to only program it for
> synchronous accesses. Remove the conditional.

Can you use the following wording from TRM instead?

as per am335x TRM (spruh73i.pdf), section 7.1.3.3.8.3.2

The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
even though the access is defined as asynchronous, and no GPMC_CLK clock
is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
for the GPMC clock, so it must be programmed to define the
correct WAITMONITORINGTIME delay.

> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  arch/arm/mach-omap2/gpmc.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 5c3639c..bae4a20 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -382,19 +382,14 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
>  	if (gpmc_capability & GPMC_HAS_WR_ACCESS)
>  		GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
>  
> -	/* caller is expected to have initialized CONFIG1 to cover
> -	 * at least sync vs async
> -	 */
>  	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> -	if (l & (GPMC_CONFIG1_READTYPE_SYNC | GPMC_CONFIG1_WRITETYPE_SYNC)) {
>  #ifdef DEBUG
> -		printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
> -				cs, (div * gpmc_get_fclk_period()) / 1000, div);
> +	printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
> +	       cs, (div * gpmc_get_fclk_period()) / 1000, div);
>  #endif
> -		l &= ~0x03;
> -		l |= (div - 1);
> -		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
> -	}
> +	l &= ~0x03;
> +	l |= (div - 1);
> +	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);

Instead of this can we explicitly set the GPMC_CLK divider to 1 and hence
corresponding divider bits to 0 in the asynchronous case?
This is because the previously calculated "div" depends on synchronous clock which
might not be properly initialized for asynchronous devices.

AFAIK t->sync_clk is always 0 for asynchronous devices and gpmc_calc_divider(0)
will return 1 and your patch will work but still we shouldn't depend on sync_clk for
asynchronous devices so let's set this explicitly.

You can add a note like this
"For asynchronous devices we explicitly set GPMC_CLK to be equal to GPMC_FCLK.
Even though GPMC_CLK pin is not used by the asynchronous device we need it
for WAITMONITORINGTIME configuration in case wait-pin monitoring is used."

>  
>  	gpmc_cs_bool_timings(cs, &t->bool_timings);
>  
> 

cheers,
-roger

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Roger Quadros]

Hi Robert,

On 16/02/15 17:48, Robert ABEL wrote:
> GPMC uses GPMCFCLKDIVIDER during synchronous as well as asynchronous accesses
> in conjunction with WAITMONITORINGTIME. Thus, it's wrong to only program it for
> synchronous accesses. Remove the conditional.

Can you use the following wording from TRM instead?

as per am335x TRM (spruh73i.pdf), section 7.1.3.3.8.3.2

The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
even though the access is defined as asynchronous, and no GPMC_CLK clock
is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
for the GPMC clock, so it must be programmed to define the
correct WAITMONITORINGTIME delay.

> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  arch/arm/mach-omap2/gpmc.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 5c3639c..bae4a20 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -382,19 +382,14 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
>  	if (gpmc_capability & GPMC_HAS_WR_ACCESS)
>  		GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
>  
> -	/* caller is expected to have initialized CONFIG1 to cover
> -	 * at least sync vs async
> -	 */
>  	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> -	if (l & (GPMC_CONFIG1_READTYPE_SYNC | GPMC_CONFIG1_WRITETYPE_SYNC)) {
>  #ifdef DEBUG
> -		printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
> -				cs, (div * gpmc_get_fclk_period()) / 1000, div);
> +	printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
> +	       cs, (div * gpmc_get_fclk_period()) / 1000, div);
>  #endif
> -		l &= ~0x03;
> -		l |= (div - 1);
> -		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
> -	}
> +	l &= ~0x03;
> +	l |= (div - 1);
> +	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);

Instead of this can we explicitly set the GPMC_CLK divider to 1 and hence
corresponding divider bits to 0 in the asynchronous case?
This is because the previously calculated "div" depends on synchronous clock which
might not be properly initialized for asynchronous devices.

AFAIK t->sync_clk is always 0 for asynchronous devices and gpmc_calc_divider(0)
will return 1 and your patch will work but still we shouldn't depend on sync_clk for
asynchronous devices so let's set this explicitly.

You can add a note like this
"For asynchronous devices we explicitly set GPMC_CLK to be equal to GPMC_FCLK.
Even though GPMC_CLK pin is not used by the asynchronous device we need it
for WAITMONITORINGTIME configuration in case wait-pin monitoring is used."

>  
>  	gpmc_cs_bool_timings(cs, &t->bool_timings);
>  
> 

cheers,
-roger

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert Abel]

Hi Roger,

On Tue, Feb 17, 2015 at 9:12 AM, Roger Quadros <rogerq@ti.com> wrote:
>
> Can you use the following wording from TRM instead?
>
> as per am335x TRM (spruh73i.pdf), section 7.1.3.3.8.3.2
>
> The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
> even though the access is defined as asynchronous, and no GPMC_CLK clock
> is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
> for the GPMC clock, so it must be programmed to define the
> correct WAITMONITORINGTIME delay.


Verbatim? Sure can. Gonna do that with a rebase to 3.19, I guess.

>
> Instead of this can we explicitly set the GPMC_CLK divider to 1 and hence
> corresponding divider bits to 0 in the asynchronous case?
> This is because the previously calculated "div" depends on synchronous clock which
> might not be properly initialized for asynchronous devices.


No, we shouldn't. If WAITREADMONITORING and/or WAITWRITEMONITORING is
enabled, sync_clk must be set in order to use WAITMONITORINGTIME
correctly. If it's not explicitly set, it's set to 0, which yields div
1 anyways.
The reason being that a fixed divider of 1 will limit a user's ability
to prolong the #WAIT-deassert --> *access delay for no good reason. If
working with a slow device, this will inconvenience users.

>
> AFAIK t->sync_clk is always 0 for asynchronous devices and gpmc_calc_divider(0)
> will return 1 and your patch will work but still we shouldn't depend on sync_clk for
> asynchronous devices so let's set this explicitly.


See above. Hardware depends on divider, divider is set by sync_clk, so
we shouldn't limit what a user can program in DT.
Having said that, I'm not aware that sync_clk is always 0 for async
devices. The code always parses it and sets the appropriate field in
gpmc_t, which is passed to gpmc_cs_set_timings.
Now, there is generally a lack of checking for optional/required DT
properties, so I didn't add extra checking.

Regards,

Robert

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Roger Quadros]

On 17/02/15 15:34, Robert Abel wrote:
> Hi Roger,
> 
> On Tue, Feb 17, 2015 at 9:12 AM, Roger Quadros <rogerq@ti.com <mailto:rogerq@ti.com>> wrote:
> 
>     Can you use the following wording from TRM instead?
> 
>     as per am335x TRM (spruh73i.pdf), section 7.1.3.3.8.3.2
> 
>     The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
>     even though the access is defined as asynchronous, and no GPMC_CLK clock
>     is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
>     for the GPMC clock, so it must be programmed to define the
>     correct WAITMONITORINGTIME delay.
> 
> 
> Verbatim? Sure can. Gonna do that with a rebase to 3.19, I guess.
>  
> 
>     Instead of this can we explicitly set the GPMC_CLK divider to 1 and hence
>     corresponding divider bits to 0 in the asynchronous case?
>     This is because the previously calculated "div" depends on synchronous clock which
>     might not be properly initialized for asynchronous devices.
> 
> 
> No, we shouldn't. If WAITREADMONITORING and/or WAITWRITEMONITORING is enabled, sync_clk must be set in order to use WAITMONITORINGTIME correctly. If it's not explicitly set, it's set to 0, which yields div 1 anyways.
> The reason being that a fixed divider of 1 will limit a user's ability to prolong the #WAIT-deassert --> *access delay for no good reason. If working with a slow device, this will inconvenience users.

nobody stops the DT binding from specifying a large enough "gpmc,wait-monitoring-ns" value.
The driver must use that to scale the GPMC_CLK if it doesn't fit in the GPMC_FCLK.
This feature can come separately though. So for now I was suggesting to set the divisor to 1.

What I'm stressing on is that there shouldn't be any dependency on "gpmc,sync-clk-ps" for
asynchronous devices. It also becomes easier to specify the wait-monitoring-ns as we don't need
to cross reference with "sync-clk-ps".

>  
> 
>     AFAIK t->sync_clk is always 0 for asynchronous devices and gpmc_calc_divider(0)
>     will return 1 and your patch will work but still we shouldn't depend on sync_clk for
>     asynchronous devices so let's set this explicitly.
> 
>  
> See above. Hardware depends on divider, divider is set by sync_clk, so we shouldn't limit what a user can program in DT.
> Having said that, I'm not aware that sync_clk is always 0 for async devices. The code always parses it and sets the appropriate field in gpmc_t, which is passed to gpmc_cs_set_timings.
> Now, there is generally a lack of checking for optional/required DT properties, so I didn't add extra checking.

AFAIK "gpmc,sync-clk-ps" is not specified for asynchronous devices so it defaults to 0
in the driver.

cheers,
-roger

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Roger Quadros]

On 17/02/15 15:34, Robert Abel wrote:
> Hi Roger,
> 
> On Tue, Feb 17, 2015 at 9:12 AM, Roger Quadros <rogerq@ti.com <mailto:rogerq@ti.com>> wrote:
> 
>     Can you use the following wording from TRM instead?
> 
>     as per am335x TRM (spruh73i.pdf), section 7.1.3.3.8.3.2
> 
>     The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
>     even though the access is defined as asynchronous, and no GPMC_CLK clock
>     is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
>     for the GPMC clock, so it must be programmed to define the
>     correct WAITMONITORINGTIME delay.
> 
> 
> Verbatim? Sure can. Gonna do that with a rebase to 3.19, I guess.
>  
> 
>     Instead of this can we explicitly set the GPMC_CLK divider to 1 and hence
>     corresponding divider bits to 0 in the asynchronous case?
>     This is because the previously calculated "div" depends on synchronous clock which
>     might not be properly initialized for asynchronous devices.
> 
> 
> No, we shouldn't. If WAITREADMONITORING and/or WAITWRITEMONITORING is enabled, sync_clk must be set in order to use WAITMONITORINGTIME correctly. If it's not explicitly set, it's set to 0, which yields div 1 anyways.
> The reason being that a fixed divider of 1 will limit a user's ability to prolong the #WAIT-deassert --> *access delay for no good reason. If working with a slow device, this will inconvenience users.

nobody stops the DT binding from specifying a large enough "gpmc,wait-monitoring-ns" value.
The driver must use that to scale the GPMC_CLK if it doesn't fit in the GPMC_FCLK.
This feature can come separately though. So for now I was suggesting to set the divisor to 1.

What I'm stressing on is that there shouldn't be any dependency on "gpmc,sync-clk-ps" for
asynchronous devices. It also becomes easier to specify the wait-monitoring-ns as we don't need
to cross reference with "sync-clk-ps".

>  
> 
>     AFAIK t->sync_clk is always 0 for asynchronous devices and gpmc_calc_divider(0)
>     will return 1 and your patch will work but still we shouldn't depend on sync_clk for
>     asynchronous devices so let's set this explicitly.
> 
>  
> See above. Hardware depends on divider, divider is set by sync_clk, so we shouldn't limit what a user can program in DT.
> Having said that, I'm not aware that sync_clk is always 0 for async devices. The code always parses it and sets the appropriate field in gpmc_t, which is passed to gpmc_cs_set_timings.
> Now, there is generally a lack of checking for optional/required DT properties, so I didn't add extra checking.

AFAIK "gpmc,sync-clk-ps" is not specified for asynchronous devices so it defaults to 0
in the driver.

cheers,
-roger

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert Abel]

Hi Roger,

On Tue, Feb 17, 2015 at 2:52 PM, Roger Quadros <rogerq@ti.com> wrote:
> nobody stops the DT binding from specifying a large enough "gpmc,wait-monitoring-ns" value.
> The driver must use that to scale the GPMC_CLK if it doesn't fit in the GPMC_FCLK.
> This feature can come separately though. So for now I was suggesting to set the divisor to 1.
> [...]
> AFAIK "gpmc,sync-clk-ps" is not specified for asynchronous devices so it defaults to 0
> in the driver.

As you have rightly pointed out, sync-clk-ps defaults to 0, i.e.
divider 1. My solution would work for people /now/ with different
gpmc,wait-monitoring-ns requirements. Of course, in general you're
right, the driver could compute that on its own. However, this
influences sampling behavior of the GPMC, which is somewhat strange
anyway. Since I lack a proper test setup and time to experiment with
the GPMC, I'd compromise on leaving sync-clk-ps default to 0, divider
defaults to 1. If somebody feels up to implementing driver-side
GPMC_CLK scaling, they might as well nix the dependency at that point
in time. Right now, keeping the dependency seems more useful to users
than killing it right away.

> What I'm stressing on is that there shouldn't be any dependency on "gpmc,sync-clk-ps" for
> asynchronous devices. It also becomes easier to specify the wait-monitoring-ns as we don't need
> to cross reference with "sync-clk-ps".

As an aside: There shouldn't be a dependency on the FCLK for
synchronous accesses either. The GPMC driver is in a somewhat terrible
state that synchronous protocols have to specify in ns, which get
scaled by the startup FCLK period... So this wrongful dependency
doesn't make my top ten, especially since it right now would fit a use
case.

Regards,

Robert

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Roger Quadros]

On 17/02/15 16:06, Robert Abel wrote:
> Hi Roger,
> 
> On Tue, Feb 17, 2015 at 2:52 PM, Roger Quadros <rogerq@ti.com> wrote:
>> nobody stops the DT binding from specifying a large enough "gpmc,wait-monitoring-ns" value.
>> The driver must use that to scale the GPMC_CLK if it doesn't fit in the GPMC_FCLK.
>> This feature can come separately though. So for now I was suggesting to set the divisor to 1.
>> [...]
>> AFAIK "gpmc,sync-clk-ps" is not specified for asynchronous devices so it defaults to 0
>> in the driver.
> 
> As you have rightly pointed out, sync-clk-ps defaults to 0, i.e.
> divider 1. My solution would work for people /now/ with different
> gpmc,wait-monitoring-ns requirements. Of course, in general you're
> right, the driver could compute that on its own. However, this
> influences sampling behavior of the GPMC, which is somewhat strange
> anyway. Since I lack a proper test setup and time to experiment with
> the GPMC, I'd compromise on leaving sync-clk-ps default to 0, divider
> defaults to 1. If somebody feels up to implementing driver-side
> GPMC_CLK scaling, they might as well nix the dependency at that point
> in time. Right now, keeping the dependency seems more useful to users
> than killing it right away.

one more thing to note is that just specifying sync-clk-ps in DT is not enough for
asynchronous devices.

The driver doesn't set gpmc_t->sync_clk if "gpmc,sync-read" or "gpmc,sync-write"
was not set in the DT, which would be the case for asynchronous devices.

> 
>> What I'm stressing on is that there shouldn't be any dependency on "gpmc,sync-clk-ps" for
>> asynchronous devices. It also becomes easier to specify the wait-monitoring-ns as we don't need
>> to cross reference with "sync-clk-ps".
> 
> As an aside: There shouldn't be a dependency on the FCLK for
> synchronous accesses either. The GPMC driver is in a somewhat terrible
> state that synchronous protocols have to specify in ns, which get
> scaled by the startup FCLK period... So this wrongful dependency
> doesn't make my top ten, especially since it right now would fit a use
> case.

What is your proposal to make things better? And what is your use case that doesn't
work with existing setup?

cheers,
-roger

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Roger Quadros]

On 17/02/15 16:06, Robert Abel wrote:
> Hi Roger,
> 
> On Tue, Feb 17, 2015 at 2:52 PM, Roger Quadros <rogerq@ti.com> wrote:
>> nobody stops the DT binding from specifying a large enough "gpmc,wait-monitoring-ns" value.
>> The driver must use that to scale the GPMC_CLK if it doesn't fit in the GPMC_FCLK.
>> This feature can come separately though. So for now I was suggesting to set the divisor to 1.
>> [...]
>> AFAIK "gpmc,sync-clk-ps" is not specified for asynchronous devices so it defaults to 0
>> in the driver.
> 
> As you have rightly pointed out, sync-clk-ps defaults to 0, i.e.
> divider 1. My solution would work for people /now/ with different
> gpmc,wait-monitoring-ns requirements. Of course, in general you're
> right, the driver could compute that on its own. However, this
> influences sampling behavior of the GPMC, which is somewhat strange
> anyway. Since I lack a proper test setup and time to experiment with
> the GPMC, I'd compromise on leaving sync-clk-ps default to 0, divider
> defaults to 1. If somebody feels up to implementing driver-side
> GPMC_CLK scaling, they might as well nix the dependency at that point
> in time. Right now, keeping the dependency seems more useful to users
> than killing it right away.

one more thing to note is that just specifying sync-clk-ps in DT is not enough for
asynchronous devices.

The driver doesn't set gpmc_t->sync_clk if "gpmc,sync-read" or "gpmc,sync-write"
was not set in the DT, which would be the case for asynchronous devices.

> 
>> What I'm stressing on is that there shouldn't be any dependency on "gpmc,sync-clk-ps" for
>> asynchronous devices. It also becomes easier to specify the wait-monitoring-ns as we don't need
>> to cross reference with "sync-clk-ps".
> 
> As an aside: There shouldn't be a dependency on the FCLK for
> synchronous accesses either. The GPMC driver is in a somewhat terrible
> state that synchronous protocols have to specify in ns, which get
> scaled by the startup FCLK period... So this wrongful dependency
> doesn't make my top ten, especially since it right now would fit a use
> case.

What is your proposal to make things better? And what is your use case that doesn't
work with existing setup?

cheers,
-roger

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert Abel]

On Tue, Feb 17, 2015 at 3:25 PM, Roger Quadros <rogerq@ti.com> wrote:
> one more thing to note is that just specifying sync-clk-ps in DT is not enough for
> asynchronous devices.
>
> The driver doesn't set gpmc_t->sync_clk if "gpmc,sync-read" or "gpmc,sync-write"
> was not set in the DT, which would be the case for asynchronous devices.

You're mistaken. It's always parsed, no matter what. See [1]. But I
did implement your suggestion of computing the divider in the mean
time. I'm going to send the patches rebased to 3.19 tomorrow, after I
tested them.

> What is your proposal to make things better? And what is your use case that doesn't
> work with existing setup?

Well, the current setup was obviously inspired by an asynchronous-only
use-case, where all the timings are in seconds and get converted
on-the-fly. Of course, currently, there is no support to re-compute
them once the gpmc_fclk changes frequency, but I guess that's what the
TODO about the clock framework is about?

Anyway, my synchronous use-case is inherently... synchronous.
Synchronous protocols shouldn't be specified in ns at all. They should
directly be specified in clock ticks. This is also advantageous, as
changes in gpmc_fclk don't need to propagate to the registers.

My main grief with the current setup is its dependency on the
*unknown* gpmc_fclk period at startup when the DT is processed and
GPMC code is called. For instance, my private DT currently operates on
the assumption of 166 Mhz L3 clk (and therefore gpmc_fclk), so all my
timings are in multiples of 6 ns. Should now a colleague of mine think
it would be a splendid idea to change this frequency by means of
bootloader options [to save power, to process data even faster, etc],
everything would break, because the L3 might have to be clocked at 100
MHz (due to divider limits along the clock tree for example) thus
making my gpmc_fclk period 10ns. Now all my synchronous timings are
wrong, because all DT values round to different clock ticks.

One solution would obviously be very verbose: Split synchronous and
asynchronous timings completely. Have a time-ns and a time-tck (where
time is waitmonitoring, or readaccess etc) setting for different use
cases. This would work for truly asynchronous (read _and_ write
asynchronous) as well as truly synchronous (read _and_ write
synchronous) setups.

This 'simple' model breaks for parts where one form of access is
synchronous while the other is asynchronous, e.g. Spansion WS/NS/VS
parts, see [2] pg. 10. There's no easy solution for mixed timings,
because some timing parameters are shared between synchronous and
asynchronous accesses (WAITMONITORINGTIME, CSONTIME, ADVONTIME,
WRDATAONADMUXBUS etc.). One obvious solution is to try to slow the
synchronous side down until asynchronous timings can be met, but that
would make for very slow accesses in most cases.

For instance, I originally started out thinking the GPMC would run at
100 MHz externally -- because it's the max frequency rating -- only to
be surprised when the clock looked weird on the GPMC_CLK pin, because
it was at 166 MHz. I'm not sure how to completely remedy this, but
specifying timings in ns in DT and then depending on the correct board
frequency is kind of shady... :(

Regards,

Robert

[1]: http://lxr.free-electrons.com/source/drivers/memory/omap-gpmc.c#L1509
[2]: http://processors.wiki.ti.com/images/5/56/SpansionNOR-AM35x.pdf ;
("WS/NS/VS can be used for synchronous burst read / asynchronous
single write")

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Tony Lindgren]

* Robert Abel <rabel@cit-ec.uni-bielefeld.de> [150223 13:42]:
> On Tue, Feb 17, 2015 at 3:25 PM, Roger Quadros <rogerq@ti.com> wrote:
> > one more thing to note is that just specifying sync-clk-ps in DT is not enough for
> > asynchronous devices.
> >
> > The driver doesn't set gpmc_t->sync_clk if "gpmc,sync-read" or "gpmc,sync-write"
> > was not set in the DT, which would be the case for asynchronous devices.
> 
> You're mistaken. It's always parsed, no matter what. See [1]. But I
> did implement your suggestion of computing the divider in the mean
> time. I'm going to send the patches rebased to 3.19 tomorrow, after I
> tested them.
> 
> > What is your proposal to make things better? And what is your use case that doesn't
> > work with existing setup?
> 
> Well, the current setup was obviously inspired by an asynchronous-only
> use-case, where all the timings are in seconds and get converted
> on-the-fly. Of course, currently, there is no support to re-compute
> them once the gpmc_fclk changes frequency, but I guess that's what the
> TODO about the clock framework is about?
> 
> Anyway, my synchronous use-case is inherently... synchronous.
> Synchronous protocols shouldn't be specified in ns at all. They should
> directly be specified in clock ticks. This is also advantageous, as
> changes in gpmc_fclk don't need to propagate to the registers.
> 
> My main grief with the current setup is its dependency on the
> *unknown* gpmc_fclk period at startup when the DT is processed and
> GPMC code is called. For instance, my private DT currently operates on
> the assumption of 166 Mhz L3 clk (and therefore gpmc_fclk), so all my
> timings are in multiples of 6 ns. Should now a colleague of mine think
> it would be a splendid idea to change this frequency by means of
> bootloader options [to save power, to process data even faster, etc],
> everything would break, because the L3 might have to be clocked at 100
> MHz (due to divider limits along the clock tree for example) thus
> making my gpmc_fclk period 10ns. Now all my synchronous timings are
> wrong, because all DT values round to different clock ticks.
> 
> One solution would obviously be very verbose: Split synchronous and
> asynchronous timings completely. Have a time-ns and a time-tck (where
> time is waitmonitoring, or readaccess etc) setting for different use
> cases. This would work for truly asynchronous (read _and_ write
> asynchronous) as well as truly synchronous (read _and_ write
> synchronous) setups.
> 
> This 'simple' model breaks for parts where one form of access is
> synchronous while the other is asynchronous, e.g. Spansion WS/NS/VS
> parts, see [2] pg. 10. There's no easy solution for mixed timings,
> because some timing parameters are shared between synchronous and
> asynchronous accesses (WAITMONITORINGTIME, CSONTIME, ADVONTIME,
> WRDATAONADMUXBUS etc.). One obvious solution is to try to slow the
> synchronous side down until asynchronous timings can be met, but that
> would make for very slow accesses in most cases.
> 
> For instance, I originally started out thinking the GPMC would run at
> 100 MHz externally -- because it's the max frequency rating -- only to
> be surprised when the clock looked weird on the GPMC_CLK pin, because
> it was at 166 MHz. I'm not sure how to completely remedy this, but
> specifying timings in ns in DT and then depending on the correct board
> frequency is kind of shady... :(

There have been few gpmc devices that were configured to work with
changing L3 frequencies. For the related retime function, maybe take
a look at the code removed for tusb_set_sync_mode() in commit
47acde167260. Just specifying the ticks is not enough there either..

Sorry I don't have a solution to how the timings should be specified,
but both timings and ticks are needed it seems.

Regards,

Tony
 
> [1]: http://lxr.free-electrons.com/source/drivers/memory/omap-gpmc.c#L1509
> [2]: http://processors.wiki.ti.com/images/5/56/SpansionNOR-AM35x.pdf ;
> ("WS/NS/VS can be used for synchronous burst read / asynchronous
> single write")

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Roger Quadros]

On 23/02/15 23:38, Robert Abel wrote:
> On Tue, Feb 17, 2015 at 3:25 PM, Roger Quadros <rogerq@ti.com> wrote:
>> one more thing to note is that just specifying sync-clk-ps in DT is not enough for
>> asynchronous devices.
>>
>> The driver doesn't set gpmc_t->sync_clk if "gpmc,sync-read" or "gpmc,sync-write"
>> was not set in the DT, which would be the case for asynchronous devices.
> 
> You're mistaken. It's always parsed, no matter what. See [1]. But I

You are right.

> did implement your suggestion of computing the divider in the mean
> time. I'm going to send the patches rebased to 3.19 tomorrow, after I
> tested them.
> 
>> What is your proposal to make things better? And what is your use case that doesn't
>> work with existing setup?
> 
> Well, the current setup was obviously inspired by an asynchronous-only
> use-case, where all the timings are in seconds and get converted
> on-the-fly. Of course, currently, there is no support to re-compute
> them once the gpmc_fclk changes frequency, but I guess that's what the
> TODO about the clock framework is about?

Yes.
> 
> Anyway, my synchronous use-case is inherently... synchronous.
> Synchronous protocols shouldn't be specified in ns at all. They should
> directly be specified in clock ticks. This is also advantageous, as
> changes in gpmc_fclk don't need to propagate to the registers.
> 
I haven't looked much at synchronous memories except oneNAND which has both
asynchronous and synchronous modes. In the datasheet, synchronous timings are
specified in seconds and change depending on device speed grade.
So seems to be the case with the Spansion NOR flash you mentioned.

IMO the DT entries should directly match the unit specified
in the memory device datasheet. Translations are best left to sw.

> My main grief with the current setup is its dependency on the
> *unknown* gpmc_fclk period at startup when the DT is processed and
> GPMC code is called. For instance, my private DT currently operates on
> the assumption of 166 Mhz L3 clk (and therefore gpmc_fclk), so all my
> timings are in multiples of 6 ns. Should now a colleague of mine think
> it would be a splendid idea to change this frequency by means of
> bootloader options [to save power, to process data even faster, etc],
> everything would break, because the L3 might have to be clocked at 100
> MHz (due to divider limits along the clock tree for example) thus
> making my gpmc_fclk period 10ns. Now all my synchronous timings are
> wrong, because all DT values round to different clock ticks.

gpmc_fclk is L3 bus clock and is not configurable by the driver
but preset at boot time or might be scaled by cpufreq driver.
GPMC_CLK is configurable to some extent by the divisor.

If some parameters of newer devices are specified in CLK ticks instead of seconds
then I agree that we need to add new DT bindings to specify them.
Can you specify which parameters are specified in CLKs in the device you are using.

> 
> One solution would obviously be very verbose: Split synchronous and
> asynchronous timings completely. Have a time-ns and a time-tck (where
> time is waitmonitoring, or readaccess etc) setting for different use
> cases. This would work for truly asynchronous (read _and_ write
> asynchronous) as well as truly synchronous (read _and_ write
> synchronous) setups.
> 
> This 'simple' model breaks for parts where one form of access is
> synchronous while the other is asynchronous, e.g. Spansion WS/NS/VS
> parts, see [2] pg. 10. There's no easy solution for mixed timings,
> because some timing parameters are shared between synchronous and
> asynchronous accesses (WAITMONITORINGTIME, CSONTIME, ADVONTIME,
> WRDATAONADMUXBUS etc.). One obvious solution is to try to slow the
> synchronous side down until asynchronous timings can be met, but that
> would make for very slow accesses in most cases.

There was a hackish way this was made to work with OneNAND devices.
It still needs a major cleanup.

The oneNAND driver starts up with asynchronous timings and then calculates
the synchronous timings on the fly based on device speed grade.
http://lxr.free-electrons.com/source/arch/arm/mach-omap2/gpmc-onenand.c#L345

> 
> For instance, I originally started out thinking the GPMC would run at
> 100 MHz externally -- because it's the max frequency rating -- only to
> be surprised when the clock looked weird on the GPMC_CLK pin, because
> it was at 166 MHz. I'm not sure how to completely remedy this, but
> specifying timings in ns in DT and then depending on the correct board
> frequency is kind of shady... :(

Right. I get the picture now. Let's address the issues one by one.

- for deterministic gpmc_fclk, gpmc,sync-clk-ps allows you to specify the
external GPMC_CLK period. 
GPMC driver doesn't have control of gpmc_fclk frequency but it
should try to set GPMC_CLK as close as possible to the specified DT
gpmc,sync-clk-ps by using the fclk divider.

- Identify what parameters are better specified in GPMC_CLK ticks instead
of seconds and provide DT bindings for them

- If gpmc_fclk changes due to whatever reasons, the driver must be notified
and it must reset GPMC_CLK to match the DT value and configure affected timings.

cheers,
-roger

Re: [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Roger Quadros]

On 23/02/15 23:38, Robert Abel wrote:
> On Tue, Feb 17, 2015 at 3:25 PM, Roger Quadros <rogerq@ti.com> wrote:
>> one more thing to note is that just specifying sync-clk-ps in DT is not enough for
>> asynchronous devices.
>>
>> The driver doesn't set gpmc_t->sync_clk if "gpmc,sync-read" or "gpmc,sync-write"
>> was not set in the DT, which would be the case for asynchronous devices.
> 
> You're mistaken. It's always parsed, no matter what. See [1]. But I

You are right.

> did implement your suggestion of computing the divider in the mean
> time. I'm going to send the patches rebased to 3.19 tomorrow, after I
> tested them.
> 
>> What is your proposal to make things better? And what is your use case that doesn't
>> work with existing setup?
> 
> Well, the current setup was obviously inspired by an asynchronous-only
> use-case, where all the timings are in seconds and get converted
> on-the-fly. Of course, currently, there is no support to re-compute
> them once the gpmc_fclk changes frequency, but I guess that's what the
> TODO about the clock framework is about?

Yes.
> 
> Anyway, my synchronous use-case is inherently... synchronous.
> Synchronous protocols shouldn't be specified in ns at all. They should
> directly be specified in clock ticks. This is also advantageous, as
> changes in gpmc_fclk don't need to propagate to the registers.
> 
I haven't looked much at synchronous memories except oneNAND which has both
asynchronous and synchronous modes. In the datasheet, synchronous timings are
specified in seconds and change depending on device speed grade.
So seems to be the case with the Spansion NOR flash you mentioned.

IMO the DT entries should directly match the unit specified
in the memory device datasheet. Translations are best left to sw.

> My main grief with the current setup is its dependency on the
> *unknown* gpmc_fclk period at startup when the DT is processed and
> GPMC code is called. For instance, my private DT currently operates on
> the assumption of 166 Mhz L3 clk (and therefore gpmc_fclk), so all my
> timings are in multiples of 6 ns. Should now a colleague of mine think
> it would be a splendid idea to change this frequency by means of
> bootloader options [to save power, to process data even faster, etc],
> everything would break, because the L3 might have to be clocked at 100
> MHz (due to divider limits along the clock tree for example) thus
> making my gpmc_fclk period 10ns. Now all my synchronous timings are
> wrong, because all DT values round to different clock ticks.

gpmc_fclk is L3 bus clock and is not configurable by the driver
but preset at boot time or might be scaled by cpufreq driver.
GPMC_CLK is configurable to some extent by the divisor.

If some parameters of newer devices are specified in CLK ticks instead of seconds
then I agree that we need to add new DT bindings to specify them.
Can you specify which parameters are specified in CLKs in the device you are using.

> 
> One solution would obviously be very verbose: Split synchronous and
> asynchronous timings completely. Have a time-ns and a time-tck (where
> time is waitmonitoring, or readaccess etc) setting for different use
> cases. This would work for truly asynchronous (read _and_ write
> asynchronous) as well as truly synchronous (read _and_ write
> synchronous) setups.
> 
> This 'simple' model breaks for parts where one form of access is
> synchronous while the other is asynchronous, e.g. Spansion WS/NS/VS
> parts, see [2] pg. 10. There's no easy solution for mixed timings,
> because some timing parameters are shared between synchronous and
> asynchronous accesses (WAITMONITORINGTIME, CSONTIME, ADVONTIME,
> WRDATAONADMUXBUS etc.). One obvious solution is to try to slow the
> synchronous side down until asynchronous timings can be met, but that
> would make for very slow accesses in most cases.

There was a hackish way this was made to work with OneNAND devices.
It still needs a major cleanup.

The oneNAND driver starts up with asynchronous timings and then calculates
the synchronous timings on the fly based on device speed grade.
http://lxr.free-electrons.com/source/arch/arm/mach-omap2/gpmc-onenand.c#L345

> 
> For instance, I originally started out thinking the GPMC would run at
> 100 MHz externally -- because it's the max frequency rating -- only to
> be surprised when the clock looked weird on the GPMC_CLK pin, because
> it was at 166 MHz. I'm not sure how to completely remedy this, but
> specifying timings in ns in DT and then depending on the correct board
> frequency is kind of shady... :(

Right. I get the picture now. Let's address the issues one by one.

- for deterministic gpmc_fclk, gpmc,sync-clk-ps allows you to specify the
external GPMC_CLK period. 
GPMC driver doesn't have control of gpmc_fclk frequency but it
should try to set GPMC_CLK as close as possible to the specified DT
gpmc,sync-clk-ps by using the fclk divider.

- Identify what parameters are better specified in GPMC_CLK ticks instead
of seconds and provide DT bindings for them

- If gpmc_fclk changes due to whatever reasons, the driver must be notified
and it must reset GPMC_CLK to match the DT value and configure affected timings.

cheers,
-roger