[PATCH 0/8 v2] ARM OMAP2+ GPMC: fixes and bus children

[PATCH 0/8 v2] ARM OMAP2+ GPMC: fixes and bus children

[Robert ABEL]

These are the changes I proposed in two separate patchsets
#([1], [2]) rebased to 3.19 as well as new changes for little bugs
I noticed while preparing this patchset.

1. DEBUG was undefined in source code --> remove offending lines
2. add capability to have busses as children of the GPMC and multiple
   devices on a bus. See [2] for an example DTS syntax.
3. debug output was unaligned --> align it
4. output for copy-pasting to DTS had erroneous timing outputs and
   made it hard to copy-paste --> remove timing values, add comments
   as DTS comments.
5. WAITMONITORINGTIME is expressed as GPMC_CLK cycles for all accesses.
   GPMCFCLKDIVIDER is used as a divider, so it must always be programmed.
6. GPMCFCLKDIVIDER is calculated according to WAITMONITORINGTIME for
   asynchronous accesses inside the driver --> asynchronous accesses now
   completely decoupled from gpmc,sync-clk-ps.
7. WAITMONITORINGTIME was being programmed/shown in GPMC_FCLK cycles instead
   of GPMC_CLK cycles --> add clock domain information where necessary.
8. Calculated values for WAITMONITORINGTIME and CLKACTIVATIONTIME that were
   outside the defined range would not raise an error.
   DEVICESIZE, ATTACHEDDEVICEPAGELENGTH, WAITMONITORINGTIME and 
   CLKACTIVATIONTIME would not be marked as incorrect on DTS output.
   --> Fix all of these.

[1]: https://lkml.org/lkml/2015/2/12/495
[2]: https://lkml.org/lkml/2015/2/16/337

Robert ABEL (9):
  ARM OMAP2+ GPMC: don't undef DEBUG
  ARM OMAP2+ GPMC: add bus children
  ARM OMAP2+ GPMC: fix debug output alignment
  ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS
  ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER
  ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME
  ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
  ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

 arch/arm/mach-omap2/gpmc-nand.c    |  17 +--
 arch/arm/mach-omap2/gpmc-onenand.c |   4 +-
 drivers/memory/Makefile            |   2 +
 drivers/memory/omap-gpmc.c         | 287 +++++++++++++++++++++++++++++--------
 include/linux/omap-gpmc.h          |   2 +-
 5 files changed, 240 insertions(+), 72 deletions(-)

-- 
2.3.0

[PATCH 1/8 v2] ARM OMAP2+ GPMC: don't undef DEBUG

[Robert ABEL]

OMAP2+ GPMC driver undefines DEBUG, which makes it unnecessarily
hard to turn DEBUG on. Remove the offending lines.

Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
---
 drivers/memory/omap-gpmc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 24696f5..5cabac8 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -12,8 +12,6 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
-#undef DEBUG
-
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
-- 
2.3.0

[PATCH 3/8 v2] 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>
---
 drivers/memory/omap-gpmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 5cabac8..78b78a64 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-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/omap-gpmc.h>
 #include <linux/mtd/nand.h>
 #include <linux/pm_runtime.h>
@@ -1800,7 +1801,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 	gpmc_cs_enable_mem(cs);
 
 no_timings:
-	if (of_platform_device_create(child, NULL, &pdev->dev))
+	if (!of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev))
 		return 0;
 
 	dev_err(&pdev->dev, "failed to create gpmc child %s\n", child->name);
-- 
2.3.0

[PATCH 4/8 v2] 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>
---
 drivers/memory/omap-gpmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 78b78a64..b5c8305 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -482,7 +482,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 5/8 v2] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS

[Robert ABEL]

DTS output was formatted to require additional work when copy-pasting into DTS.
Nano-second timings were removed, because they were not a confidence interval nor
an indication what timing values would result in the same #ticks

Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
---
 drivers/memory/omap-gpmc.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index b5c8305..ff1a1e7 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -337,32 +337,45 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
 }
 
 #ifdef DEBUG
+/**
+ * get_gpmc_timing_reg - read a timing parameter and print DTS settings for it.
+ * @cs      Chip Select Region
+ * @reg     GPMC_CS_CONFIGn register offset.
+ * @st_bit  Start Bit
+ * @end_bit End Bit. Must be >= @st_bit.
+ * @name    DTS node name, w/o "gpmc,"
+ * @raw     Raw Format Option.
+ *          raw format:  gpmc,name = <value>
+ *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
+ * @noval   Parameter values equal to 0 are not printed.
+ * @shift   Parameter value left shifts @shift, which is then printed instead of value.
+ *
+ */
 static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 			       bool raw, bool noval, int shift,
 			       const char *name)
 {
 	u32 l;
-	int nr_bits, max_value, mask;
+	int nr_bits;
+	int mask;
 
 	l = gpmc_cs_read_reg(cs, reg);
 	nr_bits = end_bit - st_bit + 1;
-	max_value = (1 << nr_bits) - 1;
-	mask = max_value << st_bit;
-	l = (l & mask) >> st_bit;
+	mask = (1 << nr_bits) - 1;;
+	l = (l >> st_bit) & mask;
 	if (shift)
 		l = (shift << l);
 	if (noval && (l == 0))
 		return 0;
 	if (!raw) {
-		unsigned int time_ns_min, time_ns, time_ns_max;
+		/* DTS tick format for timings in ns */
+		unsigned int time_ns;
 
-		time_ns_min = gpmc_ticks_to_ns(l ? l - 1 : 0);
 		time_ns = gpmc_ticks_to_ns(l);
-		time_ns_max = gpmc_ticks_to_ns(l + 1 > max_value ?
-					       max_value : l + 1);
-		pr_info("gpmc,%s = <%u> (%u - %u ns, %i ticks)\n",
-			name, time_ns, time_ns_min, time_ns_max, l);
+		pr_info("gpmc,%s = <%u> /* %i ticks */\n",
+			name, time_ns, l);
 	} else {
+		/* raw format */
 		pr_info("gpmc,%s = <%u>\n", name, l);
 	}
 
-- 
2.3.0

[PATCH 6/8 v2] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Robert ABEL]

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>
---
 drivers/memory/omap-gpmc.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index ff1a1e7..a6abaf0 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -566,19 +566,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);
 	gpmc_cs_show_timings(cs, "after gpmc_cs_set_timings");
-- 
2.3.0

[PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Robert ABEL]

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.

Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
truly asynchronous accesses, i.e. both read and write asynchronous.

Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
---
 arch/arm/mach-omap2/gpmc-nand.c    | 17 ++++-----
 arch/arm/mach-omap2/gpmc-onenand.c |  4 +--
 drivers/memory/omap-gpmc.c         | 74 ++++++++++++++++++++++++++++++++++----
 include/linux/omap-gpmc.h          |  2 +-
 4 files changed, 80 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index d5951b1..e863a59 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -96,14 +96,6 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
 	gpmc_nand_res[1].start = gpmc_get_client_irq(GPMC_IRQ_FIFOEVENTENABLE);
 	gpmc_nand_res[2].start = gpmc_get_client_irq(GPMC_IRQ_COUNT_EVENT);
 
-	if (gpmc_t) {
-		err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t);
-		if (err < 0) {
-			pr_err("omap2-gpmc: Unable to set gpmc timings: %d\n", err);
-			return err;
-		}
-	}
-
 	memset(&s, 0, sizeof(struct gpmc_settings));
 	if (gpmc_nand_data->of_node)
 		gpmc_read_settings_dt(gpmc_nand_data->of_node, &s);
@@ -111,6 +103,15 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
 		gpmc_set_legacy(gpmc_nand_data, &s);
 
 	s.device_nand = true;
+
+	if (gpmc_t) {
+		err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t, &s);
+		if (err < 0) {
+			pr_err("omap2-gpmc: Unable to set gpmc timings: %d\n", err);
+			return err;
+		}
+	}
+
 	err = gpmc_cs_program_settings(gpmc_nand_data->cs, &s);
 	if (err < 0)
 		goto out_free_cs;
diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index 53d197e..f899e77 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -293,7 +293,7 @@ static int omap2_onenand_setup_async(void __iomem *onenand_base)
 	if (ret < 0)
 		return ret;
 
-	ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t);
+	ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t, &onenand_async);
 	if (ret < 0)
 		return ret;
 
@@ -331,7 +331,7 @@ static int omap2_onenand_setup_sync(void __iomem *onenand_base, int *freq_ptr)
 	if (ret < 0)
 		return ret;
 
-	ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t);
+	ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t, &onenand_sync);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index a6abaf0..9cf8d21 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -139,6 +139,8 @@
 #define GPMC_CONFIG1_WAIT_READ_MON      (1 << 22)
 #define GPMC_CONFIG1_WAIT_WRITE_MON     (1 << 21)
 #define GPMC_CONFIG1_WAIT_MON_IIME(val) ((val & 3) << 18)
+/** WAITMONITORINGTIME Max Ticks */
+#define GPMC_CONFIG1_WAIT_MON_TIME_MAX  2
 #define GPMC_CONFIG1_WAIT_PIN_SEL(val)  ((val & 3) << 16)
 #define GPMC_CONFIG1_DEVICESIZE(val)    ((val & 3) << 12)
 #define GPMC_CONFIG1_DEVICESIZE_16      GPMC_CONFIG1_DEVICESIZE(1)
@@ -511,13 +513,41 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 			t->field, #field) < 0)			\
 		return -1
 
+/**
+ * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
+ * @wait_monitoring WAITMONITORINGTIME in ns.
+ * @return          -1 on failure to scale, else proper divider > 0.
+ * @note            WAITMONITORINGTIME will be _at least_ as long as desired.
+ *                  i.e. read timings should be kept            -> don't sample bus too early
+ *                  while write timings should work out as well -> data is longer on bus
+ */
+static int gpmc_calc_waitmonitoring_divider(unsigned int wait_monitoring)
+{
+
+	int div = gpmc_ns_to_ticks(wait_monitoring);
+
+	div += GPMC_CONFIG1_WAIT_MON_TIME_MAX - 1;
+	div /= GPMC_CONFIG1_WAIT_MON_TIME_MAX;
+
+	if (div > 4)
+		return -1;
+	if (div <= 0)
+		div = 1;
+
+	return div;
+
+}
+
+/**
+ * gpmc_calc_divider - calculate GPMC_FCLK divider for sync_clk GPMC_CLK period.
+ * @sync_clk GPMC_CLK period in ps.
+ * @return   Returns at least 1 if GPMC_FCLK can be divided to GPMC_CLK.
+ *           Else, returns -1.
+ */
 int gpmc_calc_divider(unsigned int sync_clk)
 {
-	int div;
-	u32 l;
+	int div = gpmc_ps_to_ticks(sync_clk);
 
-	l = sync_clk + (gpmc_get_fclk_period() - 1);
-	div = l / gpmc_get_fclk_period();
 	if (div > 4)
 		return -1;
 	if (div <= 0)
@@ -526,7 +556,13 @@ int gpmc_calc_divider(unsigned int sync_clk)
 	return div;
 }
 
-int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
+/**
+ * gpmc_cs_set_timings - program timing parameters for Chip Select Region.
+ * @cs   Chip Select Region.
+ * @t    GPMC timing parameters.
+ * @s    GPMC timing settings.
+ */
+int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_settings *s)
 {
 	int div;
 	u32 l;
@@ -536,6 +572,32 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
 	if (div < 0)
 		return div;
 
+	/* 
+	 * See if we need to change the divider for waitmonitoringtime.
+	 * 
+	 * If DT contains sync-clk-ps for truly asynchronous accesses,
+	 * ignore it as long as waitmonitoringtime is used.
+	 * 
+	 * This protects mixed synchronous and asynchronous devices,
+	 * i.e. must not change div to scale async waitmonitoringtime or
+	 * sync accesses would be broken.
+	 * 
+	 * We raise an error later if waitmonitoringtime does not fit.
+	 */
+	if (!s->sync_read && !s->sync_write &&
+	    (s->wait_on_read || s->wait_on_write)
+	   ) {
+
+		div = gpmc_calc_waitmonitoring_divider(t->wait_monitoring);
+		if (div < 0) {
+			pr_err("%s: waitmonitoringtime %3d ns too large for greatest gpmcfclkdivider.\n",
+			       __func__,
+			       t->wait_monitoring
+			       );
+			return -1;
+		}
+	}
+
 	GPMC_SET_ONE(GPMC_CS_CONFIG2,  0,  3, cs_on);
 	GPMC_SET_ONE(GPMC_CS_CONFIG2,  8, 12, cs_rd_off);
 	GPMC_SET_ONE(GPMC_CS_CONFIG2, 16, 20, cs_wr_off);
@@ -1793,7 +1855,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 	if (ret < 0)
 		goto err;
 
-	ret = gpmc_cs_set_timings(cs, &gpmc_t);
+	ret = gpmc_cs_set_timings(cs, &gpmc_t, &gpmc_s);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to set gpmc timings for: %s\n",
 			child->name);
diff --git a/include/linux/omap-gpmc.h b/include/linux/omap-gpmc.h
index c2080ee..9301437 100644
--- a/include/linux/omap-gpmc.h
+++ b/include/linux/omap-gpmc.h
@@ -163,7 +163,7 @@ extern unsigned int gpmc_ticks_to_ns(unsigned int ticks);
 
 extern void gpmc_cs_write_reg(int cs, int idx, u32 val);
 extern int gpmc_calc_divider(unsigned int sync_clk);
-extern int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t);
+extern int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_settings *s);
 extern int gpmc_cs_program_settings(int cs, struct gpmc_settings *p);
 extern int gpmc_cs_request(int cs, unsigned long size, unsigned long *base);
 extern void gpmc_cs_free(int cs);
-- 
2.3.0

[PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Robert ABEL]

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.

This patch correctly computes WAITMONITORINGTIME in GPMC_CLK cycles instead of GPMC_FCLK cycles,
both during programming (gpmc_cs_set_timings) and during retrieval (gpmc_cs_show_timings).

Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
---
 drivers/memory/omap-gpmc.c | 125 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 99 insertions(+), 26 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 9cf8d21..6591991 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -170,6 +170,11 @@
  */
 #define	GPMC_NR_IRQ		2
 
+enum gpmc_clk_domain {
+	GPMC_CD_FCLK,
+	GPMC_CD_CLK
+};
+
 struct gpmc_cs_data {
 	const char *name;
 
@@ -268,16 +273,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 clock domain in ps
+ * @cs Chip Select Region.
+ * @cd Clock Domain.
+ *
+ * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
+ * prior to calling this function with GPMC_CD_CLK.
+ * 
+ */
+static unsigned long gpmc_get_clk_period(int cs, enum gpmc_clk_domain cd)
+{
+
+	unsigned long tick_ps = gpmc_get_fclk_period();
+	u32 l;
+	int div;
+
+	switch (cd) {
+	case GPMC_CD_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_CD_FCLK:
+		/* FALL-THROUGH */
+	default:
+		break;
+	}
+
+	return tick_ps;
+
+}
+
+static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, enum gpmc_clk_domain cd)
 {
 	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, cd);
 
 	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, /* any CS */ 0, GPMC_CD_FCLK);
+}
+
 static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
 {
 	unsigned long tick_ps;
@@ -288,9 +332,14 @@ static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
 	return (time_ps + tick_ps - 1) / tick_ps;
 }
 
+unsigned int gpmc_clk_ticks_to_ns(unsigned ticks, int cs, enum gpmc_clk_domain cd)
+{
+	return ticks * gpmc_get_clk_period(cs, cd) / 1000;
+}
+
 unsigned int gpmc_ticks_to_ns(unsigned int ticks)
 {
-	return ticks * gpmc_get_fclk_period() / 1000;
+	return gpmc_clk_ticks_to_ns(ticks, /* any CS */ 0, GPMC_CD_FCLK);
 }
 
 static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
@@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
  * @st_bit  Start Bit
  * @end_bit End Bit. Must be >= @st_bit.
  * @name    DTS node name, w/o "gpmc,"
+ * @cd      Clock Domain of timing parameter.
+ * @shift   Parameter value left shifts @shift, which is then printed instead of value.
  * @raw     Raw Format Option.
  *          raw format:  gpmc,name = <value>
  *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
  * @noval   Parameter values equal to 0 are not printed.
- * @shift   Parameter value left shifts @shift, which is then printed instead of value.
  *
  */
-static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
-			       bool raw, bool noval, int shift,
-			       const char *name)
+static int get_gpmc_timing_reg(
+	/* timing specifiers */
+	int cs, int reg, int st_bit, int end_bit,
+	const char *name, const enum gpmc_clk_domain cd,
+	/* value transform */
+	int shift,
+	/* format specifiers */
+	bool raw, bool noval)
 {
 	u32 l;
 	int nr_bits;
@@ -373,7 +428,7 @@ static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 		/* DTS tick format for timings in ns */
 		unsigned int time_ns;
 
-		time_ns = gpmc_ticks_to_ns(l);
+		time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
 		pr_info("gpmc,%s = <%u> /* %i ticks */\n",
 			name, time_ns, l);
 	} else {
@@ -388,13 +443,15 @@ static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 	pr_info("cs%i %s: 0x%08x\n", cs, #config, \
 		gpmc_cs_read_reg(cs, config))
 #define GPMC_GET_RAW(reg, st, end, field) \
-	get_gpmc_timing_reg(cs, (reg), (st), (end), 1, 0, 0, field)
+	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 0)
 #define GPMC_GET_RAW_BOOL(reg, st, end, field) \
-	get_gpmc_timing_reg(cs, (reg), (st), (end), 1, 1, 0, field)
+	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 1)
 #define GPMC_GET_RAW_SHIFT(reg, st, end, shift, field) \
-	get_gpmc_timing_reg(cs, (reg), (st), (end), 1, 1, (shift), field)
+	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, (shift), 1, 1)
 #define GPMC_GET_TICKS(reg, st, end, field) \
-	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, 0, 0, field)
+	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 0, 0)
+#define GPMC_GET_TICKS_CD(reg, st, end, field, cd) \
+	get_gpmc_timing_reg(cs, (reg), (st), (end), field, (cd), 0, 0, 0)
 
 static void gpmc_show_regs(int cs, const char *desc)
 {
@@ -462,7 +519,7 @@ static void gpmc_cs_show_timings(int cs, const char *desc)
 	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 0, 3, "bus-turnaround-ns");
 	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 8, 11, "cycle2cycle-delay-ns");
 
-	GPMC_GET_TICKS(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns");
+	GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns", GPMC_CD_CLK);
 	GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, "clk-activation-ns");
 
 	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 16, 19, "wr-data-mux-bus-ns");
@@ -474,8 +531,20 @@ static inline void gpmc_cs_show_timings(int cs, const char *desc)
 }
 #endif
 
+/**
+ * set_gpmc_timing_reg - set a single timing parameter for Chip Select Region.
+ * @cs      Chip Select Region.
+ * @reg     GPMC_CS_CONFIGn register offset.
+ * @st_bit  Start Bit
+ * @end_bit End Bit. Must be >= @st_bit.
+ * @time    Timing parameter in ns.
+ * @cd      Timing parameter clock domain.
+ * @name    Timing parameter name.
+ * @note    Caller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER
+ *          prior to calling this function with @cd equal to GPMC_CD_CLK.
+ */
 static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
-			       int time, const char *name)
+			       int time, enum gpmc_clk_domain cd, const char *name)
 {
 	u32 l;
 	int ticks, mask, nr_bits;
@@ -483,12 +552,12 @@ 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, cd);
 	nr_bits = end_bit - st_bit + 1;
 	mask = (1 << nr_bits) - 1;
 
 	if (ticks > mask) {
-		pr_err("%s: GPMC error! CS%d: %s: %d ns, %d ticks > %d\n",
+		pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",
 		       __func__, cs, name, time, ticks, mask);
 
 		return -1;
@@ -498,7 +567,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, cd) * ticks / 1000,
 			(l >> st_bit) & mask, time);
 #endif
 	l &= ~(mask << st_bit);
@@ -508,11 +577,14 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 	return 0;
 }
 
-#define GPMC_SET_ONE(reg, st, end, field) \
-	if (set_gpmc_timing_reg(cs, (reg), (st), (end),		\
-			t->field, #field) < 0)			\
+#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
+	if (set_gpmc_timing_reg(cs, (reg), (st), (end), \
+	    t->field, (cd), #field) < 0)                \
 		return -1
 
+#define GPMC_SET_ONE(reg, st, end, field) \
+	GPMC_SET_ONE_CD(reg, st, end, field, GPMC_CD_FCLK)
+
 /**
  * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
  * @wait_monitoring WAITMONITORINGTIME in ns.
@@ -620,22 +692,23 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_
 	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_CD(GPMC_CS_CONFIG1, 18, 19, wait_monitoring, GPMC_CD_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);
 	gpmc_cs_show_timings(cs, "after gpmc_cs_set_timings");
-- 
2.3.0

[PATCH 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

[Robert ABEL]

GPMC_CONFIG1_i parameters CLKACTIVATIONTIME and WAITMONITORINGTIME
have reserved values.
Raise an error if calculated timings try to program reserved values.

GPMC_CONFIG1_i ATTCHEDDEVICEPAGELENGTH and DEVICESIZE were already checked
when parsing the DT.

Explicitly comment invalid values on gpmc_cs_show_timings for
-CLKACTIVATIONTIME
-WAITMONITORINGTIME
-DEVICESIZE
-ATTACHEDDEVICEPAGELENGTH

Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
---
 drivers/memory/omap-gpmc.c | 69 ++++++++++++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 23 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 6591991..cc2e6d0 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -135,6 +135,8 @@
 #define GPMC_CONFIG1_WRITETYPE_ASYNC    (0 << 27)
 #define GPMC_CONFIG1_WRITETYPE_SYNC     (1 << 27)
 #define GPMC_CONFIG1_CLKACTIVATIONTIME(val) ((val & 3) << 25)
+/** CLKACTIVATIONTIME Max Ticks */
+#define GPMC_CONFIG1_CLKACTIVATIONTIME_MAX 2
 #define GPMC_CONFIG1_PAGE_LEN(val)      ((val & 3) << 23)
 #define GPMC_CONFIG1_WAIT_READ_MON      (1 << 22)
 #define GPMC_CONFIG1_WAIT_WRITE_MON     (1 << 21)
@@ -144,6 +146,8 @@
 #define GPMC_CONFIG1_WAIT_PIN_SEL(val)  ((val & 3) << 16)
 #define GPMC_CONFIG1_DEVICESIZE(val)    ((val & 3) << 12)
 #define GPMC_CONFIG1_DEVICESIZE_16      GPMC_CONFIG1_DEVICESIZE(1)
+/** DEVICESIZE Max Value */
+#define GPMC_CONFIG1_DEVICESIZE_MAX     GPMC_CONFIG1_DEVICESIZE_16
 #define GPMC_CONFIG1_DEVICETYPE(val)    ((val & 3) << 10)
 #define GPMC_CONFIG1_DEVICETYPE_NOR     GPMC_CONFIG1_DEVICETYPE(0)
 #define GPMC_CONFIG1_MUXTYPE(val)       ((val & 3) << 8)
@@ -394,18 +398,21 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
  * @reg     GPMC_CS_CONFIGn register offset.
  * @st_bit  Start Bit
  * @end_bit End Bit. Must be >= @st_bit.
+ * @max     Maximum parameter value (after optional @shift).
+ *          If 0, maximum is as high as @st_bit and @end_bit allow.
  * @name    DTS node name, w/o "gpmc,"
  * @cd      Clock Domain of timing parameter.
  * @shift   Parameter value left shifts @shift, which is then printed instead of value.
  * @raw     Raw Format Option.
  *          raw format:  gpmc,name = <value>
  *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
+ *          When @max is exceeded, "invalid" is printed inside comment.
  * @noval   Parameter values equal to 0 are not printed.
  *
  */
 static int get_gpmc_timing_reg(
 	/* timing specifiers */
-	int cs, int reg, int st_bit, int end_bit,
+	int cs, int reg, int st_bit, int end_bit, int max,
 	const char *name, const enum gpmc_clk_domain cd,
 	/* value transform */
 	int shift,
@@ -415,11 +422,15 @@ static int get_gpmc_timing_reg(
 	u32 l;
 	int nr_bits;
 	int mask;
+	bool invalid;
 
 	l = gpmc_cs_read_reg(cs, reg);
 	nr_bits = end_bit - st_bit + 1;
 	mask = (1 << nr_bits) - 1;;
 	l = (l >> st_bit) & mask;
+	if (!max)
+		max = mask;
+	invalid = l > max;
 	if (shift)
 		l = (shift << l);
 	if (noval && (l == 0))
@@ -429,11 +440,11 @@ static int get_gpmc_timing_reg(
 		unsigned int time_ns;
 
 		time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
-		pr_info("gpmc,%s = <%u> /* %i ticks */\n",
-			name, time_ns, l);
+		pr_info("gpmc,%s = <%u> /* %i ticks %s*/\n",
+			name, time_ns, l, invalid ? "; invalid " : "");
 	} else {
 		/* raw format */
-		pr_info("gpmc,%s = <%u>\n", name, l);
+		pr_info("gpmc,%s = <%u>%s\n", name, l, invalid ? " /* invalid */" : "");
 	}
 
 	return l;
@@ -443,15 +454,19 @@ static int get_gpmc_timing_reg(
 	pr_info("cs%i %s: 0x%08x\n", cs, #config, \
 		gpmc_cs_read_reg(cs, config))
 #define GPMC_GET_RAW(reg, st, end, field) \
-	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 0)
+	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 1, 0)
+#define GPMC_GET_RAW_MAX(reg, st, end, max, field) \
+	get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, 0, 1, 0)
 #define GPMC_GET_RAW_BOOL(reg, st, end, field) \
-	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 1)
-#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, field) \
-	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, (shift), 1, 1)
+	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 1, 1)
+#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \
+	get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, (shift), 1, 1)
 #define GPMC_GET_TICKS(reg, st, end, field) \
-	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 0, 0)
+	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 0, 0)
 #define GPMC_GET_TICKS_CD(reg, st, end, field, cd) \
-	get_gpmc_timing_reg(cs, (reg), (st), (end), field, (cd), 0, 0, 0)
+	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, (cd), 0, 0, 0)
+#define GPMC_GET_TICKS_CD_MAX(reg, st, end, max, field, cd) \
+	get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, (cd), 0, 0, 0)
 
 static void gpmc_show_regs(int cs, const char *desc)
 {
@@ -475,11 +490,11 @@ static void gpmc_cs_show_timings(int cs, const char *desc)
 	pr_info("gpmc cs%i access configuration:\n", cs);
 	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1,  4,  4, "time-para-granularity");
 	GPMC_GET_RAW(GPMC_CS_CONFIG1,  8,  9, "mux-add-data");
-	GPMC_GET_RAW(GPMC_CS_CONFIG1, 12, 13, "device-width");
+	GPMC_GET_RAW_MAX(GPMC_CS_CONFIG1, 12, 13, GPMC_CONFIG1_DEVICESIZE_MAX, "device-width");
 	GPMC_GET_RAW(GPMC_CS_CONFIG1, 16, 17, "wait-pin");
 	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 21, 21, "wait-on-write");
 	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 22, 22, "wait-on-read");
-	GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, "burst-length");
+	GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, 2, "burst-length");
 	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 27, 27, "sync-write");
 	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 28, 28, "burst-write");
 	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 29, 29, "gpmc,sync-read");
@@ -519,8 +534,8 @@ static void gpmc_cs_show_timings(int cs, const char *desc)
 	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 0, 3, "bus-turnaround-ns");
 	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 8, 11, "cycle2cycle-delay-ns");
 
-	GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns", GPMC_CD_CLK);
-	GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, "clk-activation-ns");
+	GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, "wait-monitoring-ns", GPMC_CD_CLK);
+	GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 25, 26, GPMC_CONFIG1_CLKACTIVATIONTIME_MAX, "clk-activation-ns", GPMC_CD_FCLK);
 
 	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 16, 19, "wr-data-mux-bus-ns");
 	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 24, 28, "wr-access-ns");
@@ -537,13 +552,15 @@ static inline void gpmc_cs_show_timings(int cs, const char *desc)
  * @reg     GPMC_CS_CONFIGn register offset.
  * @st_bit  Start Bit
  * @end_bit End Bit. Must be >= @st_bit.
+ * @max     Maximum parameter value.
+ *          If 0, maximum is as high as @st_bit and @end_bit allow.
  * @time    Timing parameter in ns.
  * @cd      Timing parameter clock domain.
  * @name    Timing parameter name.
  * @note    Caller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER
  *          prior to calling this function with @cd equal to GPMC_CD_CLK.
  */
-static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
+static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit, int max,
 			       int time, enum gpmc_clk_domain cd, const char *name)
 {
 	u32 l;
@@ -556,9 +573,12 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 	nr_bits = end_bit - st_bit + 1;
 	mask = (1 << nr_bits) - 1;
 
-	if (ticks > mask) {
+	if (!max)
+		max = mask;
+
+	if (ticks > max) {
 		pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",
-		       __func__, cs, name, time, ticks, mask);
+		       __func__, cs, name, time, ticks, max);
 
 		return -1;
 	}
@@ -577,13 +597,16 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 	return 0;
 }
 
-#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
-	if (set_gpmc_timing_reg(cs, (reg), (st), (end), \
-	    t->field, (cd), #field) < 0)                \
+#define GPMC_SET_ONE_CD_MAX(reg, st, end, max, field, cd)  \
+	if (set_gpmc_timing_reg(cs, (reg), (st), (end), (max), \
+	    t->field, (cd), #field) < 0)                       \
 		return -1
 
+#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
+	GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK)
+
 #define GPMC_SET_ONE(reg, st, end, field) \
-	GPMC_SET_ONE_CD(reg, st, end, field, GPMC_CD_FCLK)
+	GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK)
 
 /**
  * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
@@ -702,8 +725,8 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_
 	l |= (div - 1);
 	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
 
-	GPMC_SET_ONE_CD(GPMC_CS_CONFIG1, 18, 19, wait_monitoring, GPMC_CD_CLK);
-	GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
+	GPMC_SET_ONE_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, wait_monitoring, GPMC_CD_CLK);
+	GPMC_SET_ONE_CD_MAX(GPMC_CS_CONFIG1, 25, 26, GPMC_CONFIG1_CLKACTIVATIONTIME_MAX, clk_activation, GPMC_CD_FCLK);
 
 #ifdef DEBUG
 	printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
-- 
2.3.0

Re: [PATCH 0/8 v2] ARM OMAP2+ GPMC: fixes and bus children

[Robert Abel]

On Tue, Feb 24, 2015 at 9:05 PM, Robert ABEL
<rabel@cit-ec.uni-bielefeld.de> wrote:
>
> These are the changes I proposed in two separate patchsets
> #([1], [2]) rebased to 3.19 as well as new changes for little bugs
> I noticed while preparing this patchset.

It seems my m4d s3d sk177z failed me. Disregard the missing Patch 2,
the numbering is off, but the contents are correct.

Regards,

Robert

Re: [PATCH 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

[Roger Quadros]

Robert,

On 24/02/15 22:05, Robert ABEL wrote:
> GPMC_CONFIG1_i parameters CLKACTIVATIONTIME and WAITMONITORINGTIME
> have reserved values.
> Raise an error if calculated timings try to program reserved values.
> 
> GPMC_CONFIG1_i ATTCHEDDEVICEPAGELENGTH and DEVICESIZE were already checked

typo ATTCHEDDEVICEPAGELENGTH->ATTACHEDDEVICEPAGELENGTH

> when parsing the DT.
> 
> Explicitly comment invalid values on gpmc_cs_show_timings for
> -CLKACTIVATIONTIME
> -WAITMONITORINGTIME
> -DEVICESIZE
> -ATTACHEDDEVICEPAGELENGTH
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  drivers/memory/omap-gpmc.c | 69 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 46 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 6591991..cc2e6d0 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -135,6 +135,8 @@
>  #define GPMC_CONFIG1_WRITETYPE_ASYNC    (0 << 27)
>  #define GPMC_CONFIG1_WRITETYPE_SYNC     (1 << 27)
>  #define GPMC_CONFIG1_CLKACTIVATIONTIME(val) ((val & 3) << 25)
> +/** CLKACTIVATIONTIME Max Ticks */
> +#define GPMC_CONFIG1_CLKACTIVATIONTIME_MAX 2
>  #define GPMC_CONFIG1_PAGE_LEN(val)      ((val & 3) << 23)
>  #define GPMC_CONFIG1_WAIT_READ_MON      (1 << 22)
>  #define GPMC_CONFIG1_WAIT_WRITE_MON     (1 << 21)
> @@ -144,6 +146,8 @@
>  #define GPMC_CONFIG1_WAIT_PIN_SEL(val)  ((val & 3) << 16)
>  #define GPMC_CONFIG1_DEVICESIZE(val)    ((val & 3) << 12)
>  #define GPMC_CONFIG1_DEVICESIZE_16      GPMC_CONFIG1_DEVICESIZE(1)
> +/** DEVICESIZE Max Value */
> +#define GPMC_CONFIG1_DEVICESIZE_MAX     GPMC_CONFIG1_DEVICESIZE_16

Shouldn't this be 1 instead? I'm hoping max value is without the shift
based on GPMC_CONFIG1_CLKACTIVATIONTIME_MAX.

>  #define GPMC_CONFIG1_DEVICETYPE(val)    ((val & 3) << 10)
>  #define GPMC_CONFIG1_DEVICETYPE_NOR     GPMC_CONFIG1_DEVICETYPE(0)
>  #define GPMC_CONFIG1_MUXTYPE(val)       ((val & 3) << 8)
> @@ -394,18 +398,21 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>   * @reg     GPMC_CS_CONFIGn register offset.
>   * @st_bit  Start Bit
>   * @end_bit End Bit. Must be >= @st_bit.
> + * @max     Maximum parameter value (after optional @shift).

max should be absolute value, without the shift.

> + *          If 0, maximum is as high as @st_bit and @end_bit allow.
>   * @name    DTS node name, w/o "gpmc,"
>   * @cd      Clock Domain of timing parameter.
>   * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>   * @raw     Raw Format Option.
>   *          raw format:  gpmc,name = <value>
>   *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
> + *          When @max is exceeded, "invalid" is printed inside comment.
>   * @noval   Parameter values equal to 0 are not printed.
>   *
>   */
>  static int get_gpmc_timing_reg(
>  	/* timing specifiers */
> -	int cs, int reg, int st_bit, int end_bit,
> +	int cs, int reg, int st_bit, int end_bit, int max,
>  	const char *name, const enum gpmc_clk_domain cd,
>  	/* value transform */
>  	int shift,
> @@ -415,11 +422,15 @@ static int get_gpmc_timing_reg(
>  	u32 l;
>  	int nr_bits;
>  	int mask;
> +	bool invalid;
>  
>  	l = gpmc_cs_read_reg(cs, reg);
>  	nr_bits = end_bit - st_bit + 1;
>  	mask = (1 << nr_bits) - 1;;
>  	l = (l >> st_bit) & mask;
> +	if (!max)
> +		max = mask;
> +	invalid = l > max;
>  	if (shift)
>  		l = (shift << l);
>  	if (noval && (l == 0))
> @@ -429,11 +440,11 @@ static int get_gpmc_timing_reg(
>  		unsigned int time_ns;
>  
>  		time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
> -		pr_info("gpmc,%s = <%u> /* %i ticks */\n",
> -			name, time_ns, l);
> +		pr_info("gpmc,%s = <%u> /* %i ticks %s*/\n",
> +			name, time_ns, l, invalid ? "; invalid " : "");
>  	} else {
>  		/* raw format */
> -		pr_info("gpmc,%s = <%u>\n", name, l);
> +		pr_info("gpmc,%s = <%u>%s\n", name, l, invalid ? " /* invalid */" : "");
>  	}
>  
>  	return l;
> @@ -443,15 +454,19 @@ static int get_gpmc_timing_reg(
>  	pr_info("cs%i %s: 0x%08x\n", cs, #config, \
>  		gpmc_cs_read_reg(cs, config))
>  #define GPMC_GET_RAW(reg, st, end, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 0)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 1, 0)
> +#define GPMC_GET_RAW_MAX(reg, st, end, max, field) \
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, 0, 1, 0)
>  #define GPMC_GET_RAW_BOOL(reg, st, end, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 1)
> -#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, (shift), 1, 1)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 1, 1)
> +#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, (shift), 1, 1)

Is it better to rename this to GPMC_GET_RAW_SHIFT_MAX() as it takes the max parameter.

>  #define GPMC_GET_TICKS(reg, st, end, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 0, 0)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 0, 0)
>  #define GPMC_GET_TICKS_CD(reg, st, end, field, cd) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), field, (cd), 0, 0, 0)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, (cd), 0, 0, 0)
> +#define GPMC_GET_TICKS_CD_MAX(reg, st, end, max, field, cd) \
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, (cd), 0, 0, 0)
>  
>  static void gpmc_show_regs(int cs, const char *desc)
>  {
> @@ -475,11 +490,11 @@ static void gpmc_cs_show_timings(int cs, const char *desc)
>  	pr_info("gpmc cs%i access configuration:\n", cs);
>  	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1,  4,  4, "time-para-granularity");
>  	GPMC_GET_RAW(GPMC_CS_CONFIG1,  8,  9, "mux-add-data");
> -	GPMC_GET_RAW(GPMC_CS_CONFIG1, 12, 13, "device-width");
> +	GPMC_GET_RAW_MAX(GPMC_CS_CONFIG1, 12, 13, GPMC_CONFIG1_DEVICESIZE_MAX, "device-width");
>  	GPMC_GET_RAW(GPMC_CS_CONFIG1, 16, 17, "wait-pin");
>  	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 21, 21, "wait-on-write");
>  	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 22, 22, "wait-on-read");
> -	GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, "burst-length");
> +	GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, 2, "burst-length");

want to define GPMC_CONFIG1_BURSTLENGTH_MAX?

>  	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 27, 27, "sync-write");
>  	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 28, 28, "burst-write");
>  	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 29, 29, "gpmc,sync-read");
> @@ -519,8 +534,8 @@ static void gpmc_cs_show_timings(int cs, const char *desc)
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 0, 3, "bus-turnaround-ns");
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 8, 11, "cycle2cycle-delay-ns");
>  
> -	GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns", GPMC_CD_CLK);
> -	GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, "clk-activation-ns");
> +	GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, "wait-monitoring-ns", GPMC_CD_CLK);

use GPMC_CONFIG1_WAITMONTIME_MAX to have consistent naming.

> +	GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 25, 26, GPMC_CONFIG1_CLKACTIVATIONTIME_MAX, "clk-activation-ns", GPMC_CD_FCLK);
>  
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 16, 19, "wr-data-mux-bus-ns");
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 24, 28, "wr-access-ns");
> @@ -537,13 +552,15 @@ static inline void gpmc_cs_show_timings(int cs, const char *desc)
>   * @reg     GPMC_CS_CONFIGn register offset.
>   * @st_bit  Start Bit
>   * @end_bit End Bit. Must be >= @st_bit.
> + * @max     Maximum parameter value.
> + *          If 0, maximum is as high as @st_bit and @end_bit allow.
>   * @time    Timing parameter in ns.
>   * @cd      Timing parameter clock domain.
>   * @name    Timing parameter name.
>   * @note    Caller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER
>   *          prior to calling this function with @cd equal to GPMC_CD_CLK.
>   */
> -static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> +static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit, int max,
>  			       int time, enum gpmc_clk_domain cd, const char *name)
>  {
>  	u32 l;
> @@ -556,9 +573,12 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  	nr_bits = end_bit - st_bit + 1;
>  	mask = (1 << nr_bits) - 1;
>  
> -	if (ticks > mask) {
> +	if (!max)
> +		max = mask;
> +
> +	if (ticks > max) {
>  		pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",
> -		       __func__, cs, name, time, ticks, mask);
> +		       __func__, cs, name, time, ticks, max);
>  
>  		return -1;
>  	}
> @@ -577,13 +597,16 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  	return 0;
>  }
>  
> -#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
> -	if (set_gpmc_timing_reg(cs, (reg), (st), (end), \
> -	    t->field, (cd), #field) < 0)                \
> +#define GPMC_SET_ONE_CD_MAX(reg, st, end, max, field, cd)  \
> +	if (set_gpmc_timing_reg(cs, (reg), (st), (end), (max), \
> +	    t->field, (cd), #field) < 0)                       \
>  		return -1
>  
> +#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
> +	GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK)

last parameter should be (cd) instead of GPMC_CD_FCLK.

> +
>  #define GPMC_SET_ONE(reg, st, end, field) \
> -	GPMC_SET_ONE_CD(reg, st, end, field, GPMC_CD_FCLK)
> +	GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK)
>  
>  /**
>   * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
> @@ -702,8 +725,8 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_
>  	l |= (div - 1);
>  	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
>  
> -	GPMC_SET_ONE_CD(GPMC_CS_CONFIG1, 18, 19, wait_monitoring, GPMC_CD_CLK);
> -	GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
> +	GPMC_SET_ONE_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, wait_monitoring, GPMC_CD_CLK);
> +	GPMC_SET_ONE_CD_MAX(GPMC_CS_CONFIG1, 25, 26, GPMC_CONFIG1_CLKACTIVATIONTIME_MAX, clk_activation, GPMC_CD_FCLK);
>  
>  #ifdef DEBUG
>  	printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
> 

cheers,
-roger

Re: [PATCH 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

[Roger Quadros]

Robert,

On 24/02/15 22:05, Robert ABEL wrote:
> GPMC_CONFIG1_i parameters CLKACTIVATIONTIME and WAITMONITORINGTIME
> have reserved values.
> Raise an error if calculated timings try to program reserved values.
> 
> GPMC_CONFIG1_i ATTCHEDDEVICEPAGELENGTH and DEVICESIZE were already checked

typo ATTCHEDDEVICEPAGELENGTH->ATTACHEDDEVICEPAGELENGTH

> when parsing the DT.
> 
> Explicitly comment invalid values on gpmc_cs_show_timings for
> -CLKACTIVATIONTIME
> -WAITMONITORINGTIME
> -DEVICESIZE
> -ATTACHEDDEVICEPAGELENGTH
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  drivers/memory/omap-gpmc.c | 69 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 46 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 6591991..cc2e6d0 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -135,6 +135,8 @@
>  #define GPMC_CONFIG1_WRITETYPE_ASYNC    (0 << 27)
>  #define GPMC_CONFIG1_WRITETYPE_SYNC     (1 << 27)
>  #define GPMC_CONFIG1_CLKACTIVATIONTIME(val) ((val & 3) << 25)
> +/** CLKACTIVATIONTIME Max Ticks */
> +#define GPMC_CONFIG1_CLKACTIVATIONTIME_MAX 2
>  #define GPMC_CONFIG1_PAGE_LEN(val)      ((val & 3) << 23)
>  #define GPMC_CONFIG1_WAIT_READ_MON      (1 << 22)
>  #define GPMC_CONFIG1_WAIT_WRITE_MON     (1 << 21)
> @@ -144,6 +146,8 @@
>  #define GPMC_CONFIG1_WAIT_PIN_SEL(val)  ((val & 3) << 16)
>  #define GPMC_CONFIG1_DEVICESIZE(val)    ((val & 3) << 12)
>  #define GPMC_CONFIG1_DEVICESIZE_16      GPMC_CONFIG1_DEVICESIZE(1)
> +/** DEVICESIZE Max Value */
> +#define GPMC_CONFIG1_DEVICESIZE_MAX     GPMC_CONFIG1_DEVICESIZE_16

Shouldn't this be 1 instead? I'm hoping max value is without the shift
based on GPMC_CONFIG1_CLKACTIVATIONTIME_MAX.

>  #define GPMC_CONFIG1_DEVICETYPE(val)    ((val & 3) << 10)
>  #define GPMC_CONFIG1_DEVICETYPE_NOR     GPMC_CONFIG1_DEVICETYPE(0)
>  #define GPMC_CONFIG1_MUXTYPE(val)       ((val & 3) << 8)
> @@ -394,18 +398,21 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>   * @reg     GPMC_CS_CONFIGn register offset.
>   * @st_bit  Start Bit
>   * @end_bit End Bit. Must be >= @st_bit.
> + * @max     Maximum parameter value (after optional @shift).

max should be absolute value, without the shift.

> + *          If 0, maximum is as high as @st_bit and @end_bit allow.
>   * @name    DTS node name, w/o "gpmc,"
>   * @cd      Clock Domain of timing parameter.
>   * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>   * @raw     Raw Format Option.
>   *          raw format:  gpmc,name = <value>
>   *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
> + *          When @max is exceeded, "invalid" is printed inside comment.
>   * @noval   Parameter values equal to 0 are not printed.
>   *
>   */
>  static int get_gpmc_timing_reg(
>  	/* timing specifiers */
> -	int cs, int reg, int st_bit, int end_bit,
> +	int cs, int reg, int st_bit, int end_bit, int max,
>  	const char *name, const enum gpmc_clk_domain cd,
>  	/* value transform */
>  	int shift,
> @@ -415,11 +422,15 @@ static int get_gpmc_timing_reg(
>  	u32 l;
>  	int nr_bits;
>  	int mask;
> +	bool invalid;
>  
>  	l = gpmc_cs_read_reg(cs, reg);
>  	nr_bits = end_bit - st_bit + 1;
>  	mask = (1 << nr_bits) - 1;;
>  	l = (l >> st_bit) & mask;
> +	if (!max)
> +		max = mask;
> +	invalid = l > max;
>  	if (shift)
>  		l = (shift << l);
>  	if (noval && (l == 0))
> @@ -429,11 +440,11 @@ static int get_gpmc_timing_reg(
>  		unsigned int time_ns;
>  
>  		time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
> -		pr_info("gpmc,%s = <%u> /* %i ticks */\n",
> -			name, time_ns, l);
> +		pr_info("gpmc,%s = <%u> /* %i ticks %s*/\n",
> +			name, time_ns, l, invalid ? "; invalid " : "");
>  	} else {
>  		/* raw format */
> -		pr_info("gpmc,%s = <%u>\n", name, l);
> +		pr_info("gpmc,%s = <%u>%s\n", name, l, invalid ? " /* invalid */" : "");
>  	}
>  
>  	return l;
> @@ -443,15 +454,19 @@ static int get_gpmc_timing_reg(
>  	pr_info("cs%i %s: 0x%08x\n", cs, #config, \
>  		gpmc_cs_read_reg(cs, config))
>  #define GPMC_GET_RAW(reg, st, end, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 0)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 1, 0)
> +#define GPMC_GET_RAW_MAX(reg, st, end, max, field) \
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, 0, 1, 0)
>  #define GPMC_GET_RAW_BOOL(reg, st, end, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 1)
> -#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, (shift), 1, 1)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 1, 1)
> +#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, (shift), 1, 1)

Is it better to rename this to GPMC_GET_RAW_SHIFT_MAX() as it takes the max parameter.

>  #define GPMC_GET_TICKS(reg, st, end, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 0, 0)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 0, 0)
>  #define GPMC_GET_TICKS_CD(reg, st, end, field, cd) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), field, (cd), 0, 0, 0)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, (cd), 0, 0, 0)
> +#define GPMC_GET_TICKS_CD_MAX(reg, st, end, max, field, cd) \
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, (cd), 0, 0, 0)
>  
>  static void gpmc_show_regs(int cs, const char *desc)
>  {
> @@ -475,11 +490,11 @@ static void gpmc_cs_show_timings(int cs, const char *desc)
>  	pr_info("gpmc cs%i access configuration:\n", cs);
>  	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1,  4,  4, "time-para-granularity");
>  	GPMC_GET_RAW(GPMC_CS_CONFIG1,  8,  9, "mux-add-data");
> -	GPMC_GET_RAW(GPMC_CS_CONFIG1, 12, 13, "device-width");
> +	GPMC_GET_RAW_MAX(GPMC_CS_CONFIG1, 12, 13, GPMC_CONFIG1_DEVICESIZE_MAX, "device-width");
>  	GPMC_GET_RAW(GPMC_CS_CONFIG1, 16, 17, "wait-pin");
>  	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 21, 21, "wait-on-write");
>  	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 22, 22, "wait-on-read");
> -	GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, "burst-length");
> +	GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, 2, "burst-length");

want to define GPMC_CONFIG1_BURSTLENGTH_MAX?

>  	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 27, 27, "sync-write");
>  	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 28, 28, "burst-write");
>  	GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 29, 29, "gpmc,sync-read");
> @@ -519,8 +534,8 @@ static void gpmc_cs_show_timings(int cs, const char *desc)
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 0, 3, "bus-turnaround-ns");
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 8, 11, "cycle2cycle-delay-ns");
>  
> -	GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns", GPMC_CD_CLK);
> -	GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, "clk-activation-ns");
> +	GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, "wait-monitoring-ns", GPMC_CD_CLK);

use GPMC_CONFIG1_WAITMONTIME_MAX to have consistent naming.

> +	GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 25, 26, GPMC_CONFIG1_CLKACTIVATIONTIME_MAX, "clk-activation-ns", GPMC_CD_FCLK);
>  
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 16, 19, "wr-data-mux-bus-ns");
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 24, 28, "wr-access-ns");
> @@ -537,13 +552,15 @@ static inline void gpmc_cs_show_timings(int cs, const char *desc)
>   * @reg     GPMC_CS_CONFIGn register offset.
>   * @st_bit  Start Bit
>   * @end_bit End Bit. Must be >= @st_bit.
> + * @max     Maximum parameter value.
> + *          If 0, maximum is as high as @st_bit and @end_bit allow.
>   * @time    Timing parameter in ns.
>   * @cd      Timing parameter clock domain.
>   * @name    Timing parameter name.
>   * @note    Caller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER
>   *          prior to calling this function with @cd equal to GPMC_CD_CLK.
>   */
> -static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> +static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit, int max,
>  			       int time, enum gpmc_clk_domain cd, const char *name)
>  {
>  	u32 l;
> @@ -556,9 +573,12 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  	nr_bits = end_bit - st_bit + 1;
>  	mask = (1 << nr_bits) - 1;
>  
> -	if (ticks > mask) {
> +	if (!max)
> +		max = mask;
> +
> +	if (ticks > max) {
>  		pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",
> -		       __func__, cs, name, time, ticks, mask);
> +		       __func__, cs, name, time, ticks, max);
>  
>  		return -1;
>  	}
> @@ -577,13 +597,16 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  	return 0;
>  }
>  
> -#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
> -	if (set_gpmc_timing_reg(cs, (reg), (st), (end), \
> -	    t->field, (cd), #field) < 0)                \
> +#define GPMC_SET_ONE_CD_MAX(reg, st, end, max, field, cd)  \
> +	if (set_gpmc_timing_reg(cs, (reg), (st), (end), (max), \
> +	    t->field, (cd), #field) < 0)                       \
>  		return -1
>  
> +#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
> +	GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK)

last parameter should be (cd) instead of GPMC_CD_FCLK.

> +
>  #define GPMC_SET_ONE(reg, st, end, field) \
> -	GPMC_SET_ONE_CD(reg, st, end, field, GPMC_CD_FCLK)
> +	GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK)
>  
>  /**
>   * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
> @@ -702,8 +725,8 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_
>  	l |= (div - 1);
>  	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
>  
> -	GPMC_SET_ONE_CD(GPMC_CS_CONFIG1, 18, 19, wait_monitoring, GPMC_CD_CLK);
> -	GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
> +	GPMC_SET_ONE_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, wait_monitoring, GPMC_CD_CLK);
> +	GPMC_SET_ONE_CD_MAX(GPMC_CS_CONFIG1, 25, 26, GPMC_CONFIG1_CLKACTIVATIONTIME_MAX, clk_activation, GPMC_CD_FCLK);
>  
>  #ifdef DEBUG
>  	printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
> 

cheers,
-roger

Re: [PATCH 1/8 v2] ARM OMAP2+ GPMC: don't undef DEBUG

[Roger Quadros]

On 24/02/15 22:05, Robert ABEL wrote:
> OMAP2+ GPMC driver undefines DEBUG, which makes it unnecessarily
> hard to turn DEBUG on. Remove the offending lines.
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

> ---
>  drivers/memory/omap-gpmc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 24696f5..5cabac8 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -12,8 +12,6 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> -#undef DEBUG
> -
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> 

Re: [PATCH 1/8 v2] ARM OMAP2+ GPMC: don't undef DEBUG

[Roger Quadros]

On 24/02/15 22:05, Robert ABEL wrote:
> OMAP2+ GPMC driver undefines DEBUG, which makes it unnecessarily
> hard to turn DEBUG on. Remove the offending lines.
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

> ---
>  drivers/memory/omap-gpmc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 24696f5..5cabac8 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -12,8 +12,6 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> -#undef DEBUG
> -
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
> 

Re: [PATCH 3/8 v2] ARM OMAP2+ GPMC: add bus children

[Roger Quadros]

Robert,

On 24/02/15 22:05, 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>
> ---
>  drivers/memory/omap-gpmc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 5cabac8..78b78a64 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-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/omap-gpmc.h>
>  #include <linux/mtd/nand.h>
>  #include <linux/pm_runtime.h>
> @@ -1800,7 +1801,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  	gpmc_cs_enable_mem(cs);
>  
>  no_timings:
> -	if (of_platform_device_create(child, NULL, &pdev->dev))
> +	if (!of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev))
>  		return 0;

This creates platform devices for the children of child, but what about platform
device for the child itself?

e.g. This fails to create the NOR device and instead starts creating children for
NOR partitions instead.

What bus device are you using. Shouldn't the bus driver for that bus be responsible
for spawning children of its bus?

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

cheers,
-roger

Re: [PATCH 3/8 v2] ARM OMAP2+ GPMC: add bus children

[Roger Quadros]

Robert,

On 24/02/15 22:05, 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>
> ---
>  drivers/memory/omap-gpmc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 5cabac8..78b78a64 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-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/omap-gpmc.h>
>  #include <linux/mtd/nand.h>
>  #include <linux/pm_runtime.h>
> @@ -1800,7 +1801,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  	gpmc_cs_enable_mem(cs);
>  
>  no_timings:
> -	if (of_platform_device_create(child, NULL, &pdev->dev))
> +	if (!of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev))
>  		return 0;

This creates platform devices for the children of child, but what about platform
device for the child itself?

e.g. This fails to create the NOR device and instead starts creating children for
NOR partitions instead.

What bus device are you using. Shouldn't the bus driver for that bus be responsible
for spawning children of its bus?

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

cheers,
-roger

Re: [PATCH 4/8 v2] ARM OMAP2+ GPMC: fix debug output alignment

[Roger Quadros]

On 24/02/15 22:05, Robert ABEL wrote:
> 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>

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

> ---
>  drivers/memory/omap-gpmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 78b78a64..b5c8305 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -482,7 +482,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
> 

Re: [PATCH 4/8 v2] ARM OMAP2+ GPMC: fix debug output alignment

[Roger Quadros]

On 24/02/15 22:05, Robert ABEL wrote:
> 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>

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

> ---
>  drivers/memory/omap-gpmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 78b78a64..b5c8305 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -482,7 +482,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
> 

Re: [PATCH 5/8 v2] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS

[Roger Quadros]

On 24/02/15 22:05, Robert ABEL wrote:
> DTS output was formatted to require additional work when copy-pasting into DTS.
> Nano-second timings were removed, because they were not a confidence interval nor
> an indication what timing values would result in the same #ticks

If they were not is it possible to dump min,max timings that will result in
the same ticks?

Otherwise patch is fine.

cheers,
-roger

> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  drivers/memory/omap-gpmc.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index b5c8305..ff1a1e7 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -337,32 +337,45 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>  }
>  
>  #ifdef DEBUG
> +/**
> + * get_gpmc_timing_reg - read a timing parameter and print DTS settings for it.
> + * @cs      Chip Select Region
> + * @reg     GPMC_CS_CONFIGn register offset.
> + * @st_bit  Start Bit
> + * @end_bit End Bit. Must be >= @st_bit.
> + * @name    DTS node name, w/o "gpmc,"
> + * @raw     Raw Format Option.
> + *          raw format:  gpmc,name = <value>
> + *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
> + * @noval   Parameter values equal to 0 are not printed.
> + * @shift   Parameter value left shifts @shift, which is then printed instead of value.
> + *
> + */
>  static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  			       bool raw, bool noval, int shift,
>  			       const char *name)
>  {
>  	u32 l;
> -	int nr_bits, max_value, mask;
> +	int nr_bits;
> +	int mask;
>  
>  	l = gpmc_cs_read_reg(cs, reg);
>  	nr_bits = end_bit - st_bit + 1;
> -	max_value = (1 << nr_bits) - 1;
> -	mask = max_value << st_bit;
> -	l = (l & mask) >> st_bit;
> +	mask = (1 << nr_bits) - 1;;
> +	l = (l >> st_bit) & mask;
>  	if (shift)
>  		l = (shift << l);
>  	if (noval && (l == 0))
>  		return 0;
>  	if (!raw) {
> -		unsigned int time_ns_min, time_ns, time_ns_max;
> +		/* DTS tick format for timings in ns */
> +		unsigned int time_ns;
>  
> -		time_ns_min = gpmc_ticks_to_ns(l ? l - 1 : 0);
>  		time_ns = gpmc_ticks_to_ns(l);
> -		time_ns_max = gpmc_ticks_to_ns(l + 1 > max_value ?
> -					       max_value : l + 1);
> -		pr_info("gpmc,%s = <%u> (%u - %u ns, %i ticks)\n",
> -			name, time_ns, time_ns_min, time_ns_max, l);
> +		pr_info("gpmc,%s = <%u> /* %i ticks */\n",
> +			name, time_ns, l);
>  	} else {
> +		/* raw format */
>  		pr_info("gpmc,%s = <%u>\n", name, l);
>  	}
>  
> 

Re: [PATCH 5/8 v2] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS

[Roger Quadros]

On 24/02/15 22:05, Robert ABEL wrote:
> DTS output was formatted to require additional work when copy-pasting into DTS.
> Nano-second timings were removed, because they were not a confidence interval nor
> an indication what timing values would result in the same #ticks

If they were not is it possible to dump min,max timings that will result in
the same ticks?

Otherwise patch is fine.

cheers,
-roger

> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  drivers/memory/omap-gpmc.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index b5c8305..ff1a1e7 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -337,32 +337,45 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>  }
>  
>  #ifdef DEBUG
> +/**
> + * get_gpmc_timing_reg - read a timing parameter and print DTS settings for it.
> + * @cs      Chip Select Region
> + * @reg     GPMC_CS_CONFIGn register offset.
> + * @st_bit  Start Bit
> + * @end_bit End Bit. Must be >= @st_bit.
> + * @name    DTS node name, w/o "gpmc,"
> + * @raw     Raw Format Option.
> + *          raw format:  gpmc,name = <value>
> + *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
> + * @noval   Parameter values equal to 0 are not printed.
> + * @shift   Parameter value left shifts @shift, which is then printed instead of value.
> + *
> + */
>  static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  			       bool raw, bool noval, int shift,
>  			       const char *name)
>  {
>  	u32 l;
> -	int nr_bits, max_value, mask;
> +	int nr_bits;
> +	int mask;
>  
>  	l = gpmc_cs_read_reg(cs, reg);
>  	nr_bits = end_bit - st_bit + 1;
> -	max_value = (1 << nr_bits) - 1;
> -	mask = max_value << st_bit;
> -	l = (l & mask) >> st_bit;
> +	mask = (1 << nr_bits) - 1;;
> +	l = (l >> st_bit) & mask;
>  	if (shift)
>  		l = (shift << l);
>  	if (noval && (l == 0))
>  		return 0;
>  	if (!raw) {
> -		unsigned int time_ns_min, time_ns, time_ns_max;
> +		/* DTS tick format for timings in ns */
> +		unsigned int time_ns;
>  
> -		time_ns_min = gpmc_ticks_to_ns(l ? l - 1 : 0);
>  		time_ns = gpmc_ticks_to_ns(l);
> -		time_ns_max = gpmc_ticks_to_ns(l + 1 > max_value ?
> -					       max_value : l + 1);
> -		pr_info("gpmc,%s = <%u> (%u - %u ns, %i ticks)\n",
> -			name, time_ns, time_ns_min, time_ns_max, l);
> +		pr_info("gpmc,%s = <%u> /* %i ticks */\n",
> +			name, time_ns, l);
>  	} else {
> +		/* raw format */
>  		pr_info("gpmc,%s = <%u>\n", name, l);
>  	}
>  
> 

Re: [PATCH 6/8 v2] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Roger Quadros]

On 24/02/15 22:05, Robert ABEL wrote:
> 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>

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

> ---
>  drivers/memory/omap-gpmc.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index ff1a1e7..a6abaf0 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -566,19 +566,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);
>  	gpmc_cs_show_timings(cs, "after gpmc_cs_set_timings");
> 

Re: [PATCH 6/8 v2] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER

[Roger Quadros]

On 24/02/15 22:05, Robert ABEL wrote:
> 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>

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

> ---
>  drivers/memory/omap-gpmc.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index ff1a1e7..a6abaf0 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -566,19 +566,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);
>  	gpmc_cs_show_timings(cs, "after gpmc_cs_set_timings");
> 

Re: [PATCH 3/8 v2] ARM OMAP2+ GPMC: add bus children

[Robert Abel]

Hi Roger,

On 25 Feb 2015 13:02, Roger Quadros wrote:
> This creates platform devices for the children of child, but what 
> about platform device for the child itself?
It seems my first try in the other patch set wasn't so wrong after all. 
Maybe unconditionally call

of_platform_device_create(child,...) and
of_platform_populate(child, ...)?

> Shouldn't the bus driver for that bus be responsible for spawning children of its bus?
Well, a bus driver doesn't actually do much work here. The standard buses are just there for offset and so on.
And that's my use case. Have one CS region with fixed settings and multiple devices in it.

Creating code that in effect replicates what simple-bus does isn't worthwhile. Basically, it would replicate half the code that
of_platform_populate goes through anyway without good reason.

If complex bus-behavior is needed -- more than single-bus offset shifts --, a complex bus driver can always be written.
Since it wouldn't match of_default_match_table, it itself would be responsible for creating children.

I think I'll go with my first patch for this one, maybe call of_platform_decide_create unconditionally to check whether buses are
available.

Regards,

Robert

Re: [PATCH 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

[Robert Abel]

Hi Roger,

On 25 Feb 2015 11:44, Roger Quadros wrote:
> typo ATTCHEDDEVICEPAGELENGTH->ATTACHEDDEVICEPAGELENGTH 
Yep.
>> +/** DEVICESIZE Max Value */
>> +#define GPMC_CONFIG1_DEVICESIZE_MAX     GPMC_CONFIG1_DEVICESIZE_16
> Shouldn't this be 1 instead? I'm hoping max value is without the shift
> based on GPMC_CONFIG1_CLKACTIVATIONTIME_MAX.
Yes, it should. It's a remnant from the change below...
>> + * @max     Maximum parameter value (after optional @shift).
> max should be absolute value, without the shift.
Yes, forgot to change that.
>> +#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \
>> +	get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, (shift), 1, 1)
> Is it better to rename this to GPMC_GET_RAW_SHIFT_MAX() as it takes the max parameter.
Ok.
>> +	GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, 2, "burst-length");
> want to define GPMC_CONFIG1_BURSTLENGTH_MAX?
Yes, for consistency. Originally, i was not going to create defines for 
limits used only in one place, as opposed to WAITMONITORINGTIME and 
CLKACTIVATIONTIME.
>> -	GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns", GPMC_CD_CLK);
>> -	GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, "clk-activation-ns");
>> +	GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, "wait-monitoring-ns", GPMC_CD_CLK);
> use GPMC_CONFIG1_WAITMONTIME_MAX to have consistent naming.
I used the naming the other define had. Saw no point in changing them. 
They go mostly unused anyway.
Might as well change it.

>> +#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
>> +	GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK)
> last parameter should be (cd) instead of GPMC_CD_FCLK.
Ah, copy-paste, there you are again. Luckily this define goes unused, 
because all GPMC_SET_ONE_CD became GPMC_SET_ONE_CD_MAX anyway.
So I'll delete that. Quite a keen eye, though ;)

Regards,

Robert

Re: [PATCH 5/8 v2] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS

[Tony Lindgren]

* Roger Quadros <rogerq@ti.com> [150225 05:28]:
> On 24/02/15 22:05, Robert ABEL wrote:
> > DTS output was formatted to require additional work when copy-pasting into DTS.
> > Nano-second timings were removed, because they were not a confidence interval nor
> > an indication what timing values would result in the same #ticks
> 
> If they were not is it possible to dump min,max timings that will result in
> the same ticks?

Yeah my plan was o display the nanosecond range resulting in the
same tick value. That makes it a bit easier to compare the timings
to the data sheet. So maybe show both tick and ns range?

Ideally the timings would be from the device data sheet with
the latency added for components that are on the line in
addition to the device, such as level shifters and FPGA.

Regards,

Tony

Re: [PATCH 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

[Roger Quadros]

Robert,

On 25/02/15 17:17, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 11:44, Roger Quadros wrote:
>> typo ATTCHEDDEVICEPAGELENGTH->ATTACHEDDEVICEPAGELENGTH 
> Yep.
>>> +/** DEVICESIZE Max Value */
>>> +#define GPMC_CONFIG1_DEVICESIZE_MAX     GPMC_CONFIG1_DEVICESIZE_16
>> Shouldn't this be 1 instead? I'm hoping max value is without the shift
>> based on GPMC_CONFIG1_CLKACTIVATIONTIME_MAX.
> Yes, it should. It's a remnant from the change below...
>>> + * @max     Maximum parameter value (after optional @shift).
>> max should be absolute value, without the shift.
> Yes, forgot to change that.
>>> +#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \
>>> +    get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, (shift), 1, 1)
>> Is it better to rename this to GPMC_GET_RAW_SHIFT_MAX() as it takes the max parameter.
> Ok.
>>> +    GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, 2, "burst-length");
>> want to define GPMC_CONFIG1_BURSTLENGTH_MAX?
> Yes, for consistency. Originally, i was not going to create defines for limits used only in one place, as opposed to WAITMONITORINGTIME and CLKACTIVATIONTIME.
>>> -    GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns", GPMC_CD_CLK);
>>> -    GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, "clk-activation-ns");
>>> +    GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, "wait-monitoring-ns", GPMC_CD_CLK);
>> use GPMC_CONFIG1_WAITMONTIME_MAX to have consistent naming.
> I used the naming the other define had. Saw no point in changing them. They go mostly unused anyway.
> Might as well change it.

Not much of an issue. I leave it upto you.

cheers,
-roger
> 
>>> +#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
>>> +    GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK)
>> last parameter should be (cd) instead of GPMC_CD_FCLK.
> Ah, copy-paste, there you are again. Luckily this define goes unused, because all GPMC_SET_ONE_CD became GPMC_SET_ONE_CD_MAX anyway.
> So I'll delete that. Quite a keen eye, though ;)
> 
> Regards,
> 
> Robert

Re: [PATCH 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters

[Roger Quadros]

Robert,

On 25/02/15 17:17, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 11:44, Roger Quadros wrote:
>> typo ATTCHEDDEVICEPAGELENGTH->ATTACHEDDEVICEPAGELENGTH 
> Yep.
>>> +/** DEVICESIZE Max Value */
>>> +#define GPMC_CONFIG1_DEVICESIZE_MAX     GPMC_CONFIG1_DEVICESIZE_16
>> Shouldn't this be 1 instead? I'm hoping max value is without the shift
>> based on GPMC_CONFIG1_CLKACTIVATIONTIME_MAX.
> Yes, it should. It's a remnant from the change below...
>>> + * @max     Maximum parameter value (after optional @shift).
>> max should be absolute value, without the shift.
> Yes, forgot to change that.
>>> +#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \
>>> +    get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, (shift), 1, 1)
>> Is it better to rename this to GPMC_GET_RAW_SHIFT_MAX() as it takes the max parameter.
> Ok.
>>> +    GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, 2, "burst-length");
>> want to define GPMC_CONFIG1_BURSTLENGTH_MAX?
> Yes, for consistency. Originally, i was not going to create defines for limits used only in one place, as opposed to WAITMONITORINGTIME and CLKACTIVATIONTIME.
>>> -    GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns", GPMC_CD_CLK);
>>> -    GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, "clk-activation-ns");
>>> +    GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, "wait-monitoring-ns", GPMC_CD_CLK);
>> use GPMC_CONFIG1_WAITMONTIME_MAX to have consistent naming.
> I used the naming the other define had. Saw no point in changing them. They go mostly unused anyway.
> Might as well change it.

Not much of an issue. I leave it upto you.

cheers,
-roger
> 
>>> +#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
>>> +    GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK)
>> last parameter should be (cd) instead of GPMC_CD_FCLK.
> Ah, copy-paste, there you are again. Luckily this define goes unused, because all GPMC_SET_ONE_CD became GPMC_SET_ONE_CD_MAX anyway.
> So I'll delete that. Quite a keen eye, though ;)
> 
> Regards,
> 
> Robert

Re: [PATCH 3/8 v2] ARM OMAP2+ GPMC: add bus children

[Roger Quadros]

Robert,

On 25/02/15 17:06, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 13:02, Roger Quadros wrote:
>> This creates platform devices for the children of child, but what about platform device for the child itself?
> It seems my first try in the other patch set wasn't so wrong after all. Maybe unconditionally call
> 
> of_platform_device_create(child,...) and
> of_platform_populate(child, ...)?
> 
>> Shouldn't the bus driver for that bus be responsible for spawning children of its bus?
> Well, a bus driver doesn't actually do much work here. The standard buses are just there for offset and so on.
> And that's my use case. Have one CS region with fixed settings and multiple devices in it.
> 
> Creating code that in effect replicates what simple-bus does isn't worthwhile. Basically, it would replicate half the code that
> of_platform_populate goes through anyway without good reason.
> 
> If complex bus-behavior is needed -- more than single-bus offset shifts --, a complex bus driver can always be written.
> Since it wouldn't match of_default_match_table, it itself would be responsible for creating children.
> 
> I think I'll go with my first patch for this one, maybe call of_platform_decide_create unconditionally to check whether buses are
> available.

OK. Would be interesting to see how unconditional call to of_platform_decide_create() behaves
for your case.

cheers,
-roger

Re: [PATCH 3/8 v2] ARM OMAP2+ GPMC: add bus children

[Roger Quadros]

Robert,

On 25/02/15 17:06, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 13:02, Roger Quadros wrote:
>> This creates platform devices for the children of child, but what about platform device for the child itself?
> It seems my first try in the other patch set wasn't so wrong after all. Maybe unconditionally call
> 
> of_platform_device_create(child,...) and
> of_platform_populate(child, ...)?
> 
>> Shouldn't the bus driver for that bus be responsible for spawning children of its bus?
> Well, a bus driver doesn't actually do much work here. The standard buses are just there for offset and so on.
> And that's my use case. Have one CS region with fixed settings and multiple devices in it.
> 
> Creating code that in effect replicates what simple-bus does isn't worthwhile. Basically, it would replicate half the code that
> of_platform_populate goes through anyway without good reason.
> 
> If complex bus-behavior is needed -- more than single-bus offset shifts --, a complex bus driver can always be written.
> Since it wouldn't match of_default_match_table, it itself would be responsible for creating children.
> 
> I think I'll go with my first patch for this one, maybe call of_platform_decide_create unconditionally to check whether buses are
> available.

OK. Would be interesting to see how unconditional call to of_platform_decide_create() behaves
for your case.

cheers,
-roger

Re: [PATCH 3/8 v2] ARM OMAP2+ GPMC: add bus children

[Robert Abel]

Hi Roger,

On 25 Feb 2015 17:18, Roger Quadros wrote:
> OK. Would be interesting to see how unconditional call to of_platform_decide_create() behaves
> for your case.
I'm not able to test today, so results will be in tomorrow. If that 
doesn't work, I'll just resubmit my first patch with the conditional.

I'll be posting a v3 of this entire patch series tomorrow after testing. 
I think I should leave your ACKs out for you to re-ACK?

Regards,

Robert

Re: [PATCH 5/8 v2] ARM OMAP2+ GPMC: change get_gpmc_timing_reg output for DTS

[Robert Abel]

Hi Tony,

On 25 Feb 2015 16:23, Tony Lindgren wrote:
> * Roger Quadros <rogerq@ti.com> [150225 05:28]:
>> On 24/02/15 22:05, Robert ABEL wrote:
>>> DTS output was formatted to require additional work when copy-pasting into DTS.
>>> Nano-second timings were removed, because they were not a confidence interval nor
>>> an indication what timing values would result in the same #ticks
>> If they were not is it possible to dump min,max timings that will result in
>> the same ticks?
> Yeah my plan was o display the nanosecond range resulting in the
> same tick value. That makes it a bit easier to compare the timings
> to the data sheet. So maybe show both tick and ns range?
Ah, so my hunch was right. I reimplemented that correctly now.
Basically, your code just went [ticks-1, ticks+1] in ns. But times are 
always ceiling(ns / gpmc_[f]clk), i.e. (ticks - 1, ticks] in ns.

Some other patches change as well because of this change, so a v3 is in 
order =].

Regards,

Robert

Re: [PATCH 3/8 v2] ARM OMAP2+ GPMC: add bus children

[Roger Quadros]

On 25/02/15 18:23, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 17:18, Roger Quadros wrote:
>> OK. Would be interesting to see how unconditional call to of_platform_decide_create() behaves
>> for your case.
> I'm not able to test today, so results will be in tomorrow. If that doesn't work, I'll just resubmit my first patch with the conditional.

Tomorrow is fine.

> 
> I'll be posting a v3 of this entire patch series tomorrow after testing. I think I should leave your ACKs out for you to re-ACK?

If you are not changing the patches I've acked you can keep my ACK.

cheers,
-roger

Re: [PATCH 3/8 v2] ARM OMAP2+ GPMC: add bus children

[Roger Quadros]

On 25/02/15 18:23, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 17:18, Roger Quadros wrote:
>> OK. Would be interesting to see how unconditional call to of_platform_decide_create() behaves
>> for your case.
> I'm not able to test today, so results will be in tomorrow. If that doesn't work, I'll just resubmit my first patch with the conditional.

Tomorrow is fine.

> 
> I'll be posting a v3 of this entire patch series tomorrow after testing. I think I should leave your ACKs out for you to re-ACK?

If you are not changing the patches I've acked you can keep my ACK.

cheers,
-roger

Re: [PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Roger Quadros]

On 24/02/15 22:05, Robert ABEL wrote:
> 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.
> 
> Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
> truly asynchronous accesses, i.e. both read and write asynchronous.
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  arch/arm/mach-omap2/gpmc-nand.c    | 17 ++++-----
>  arch/arm/mach-omap2/gpmc-onenand.c |  4 +--
>  drivers/memory/omap-gpmc.c         | 74 ++++++++++++++++++++++++++++++++++----
>  include/linux/omap-gpmc.h          |  2 +-
>  4 files changed, 80 insertions(+), 17 deletions(-)
> 

./scripts/checkpatch.pl detects some styling errors.

> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
> index d5951b1..e863a59 100644
> --- a/arch/arm/mach-omap2/gpmc-nand.c
> +++ b/arch/arm/mach-omap2/gpmc-nand.c
> @@ -96,14 +96,6 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
>  	gpmc_nand_res[1].start = gpmc_get_client_irq(GPMC_IRQ_FIFOEVENTENABLE);
>  	gpmc_nand_res[2].start = gpmc_get_client_irq(GPMC_IRQ_COUNT_EVENT);
>  
> -	if (gpmc_t) {
> -		err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t);
> -		if (err < 0) {
> -			pr_err("omap2-gpmc: Unable to set gpmc timings: %d\n", err);
> -			return err;
> -		}
> -	}
> -
>  	memset(&s, 0, sizeof(struct gpmc_settings));
>  	if (gpmc_nand_data->of_node)
>  		gpmc_read_settings_dt(gpmc_nand_data->of_node, &s);
> @@ -111,6 +103,15 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
>  		gpmc_set_legacy(gpmc_nand_data, &s);
>  
>  	s.device_nand = true;
> +
> +	if (gpmc_t) {
> +		err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t, &s);
> +		if (err < 0) {
> +			pr_err("omap2-gpmc: Unable to set gpmc timings: %d\n", err);
> +			return err;
> +		}
> +	}
> +
>  	err = gpmc_cs_program_settings(gpmc_nand_data->cs, &s);
>  	if (err < 0)
>  		goto out_free_cs;
> diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> index 53d197e..f899e77 100644
> --- a/arch/arm/mach-omap2/gpmc-onenand.c
> +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> @@ -293,7 +293,7 @@ static int omap2_onenand_setup_async(void __iomem *onenand_base)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t);
> +	ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t, &onenand_async);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -331,7 +331,7 @@ static int omap2_onenand_setup_sync(void __iomem *onenand_base, int *freq_ptr)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t);
> +	ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t, &onenand_sync);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index a6abaf0..9cf8d21 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -139,6 +139,8 @@
>  #define GPMC_CONFIG1_WAIT_READ_MON      (1 << 22)
>  #define GPMC_CONFIG1_WAIT_WRITE_MON     (1 << 21)
>  #define GPMC_CONFIG1_WAIT_MON_IIME(val) ((val & 3) << 18)

Not caused by your patch but we can fix the typo

GPMC_CONFIG1_WAIT_MON_IIME -> GPMC_CONFIG1_WAIT_MON_TIME

> +/** WAITMONITORINGTIME Max Ticks */
> +#define GPMC_CONFIG1_WAIT_MON_TIME_MAX  2
>  #define GPMC_CONFIG1_WAIT_PIN_SEL(val)  ((val & 3) << 16)
>  #define GPMC_CONFIG1_DEVICESIZE(val)    ((val & 3) << 12)
>  #define GPMC_CONFIG1_DEVICESIZE_16      GPMC_CONFIG1_DEVICESIZE(1)
> @@ -511,13 +513,41 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  			t->field, #field) < 0)			\
>  		return -1
>  
> +/**
> + * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
> + * @wait_monitoring WAITMONITORINGTIME in ns.
> + * @return          -1 on failure to scale, else proper divider > 0.
> + * @note            WAITMONITORINGTIME will be _at least_ as long as desired.
> + *                  i.e. read timings should be kept            -> don't sample bus too early
> + *                  while write timings should work out as well -> data is longer on bus
> + */
> +static int gpmc_calc_waitmonitoring_divider(unsigned int wait_monitoring)
> +{
> +
> +	int div = gpmc_ns_to_ticks(wait_monitoring);
> +
> +	div += GPMC_CONFIG1_WAIT_MON_TIME_MAX - 1;
> +	div /= GPMC_CONFIG1_WAIT_MON_TIME_MAX;

Sorry I didn't understand how this works. :P
>From the TRM,

waitmonitoringtime_ns = waitmonitoring_ticks x (gpmc_clk_div + 1) x gpmc_fclk_ns

> +
> +	if (div > 4)
> +		return -1;
> +	if (div <= 0)
> +		div = 1;
> +
> +	return div;
> +
> +}
> +
> +/**
> + * gpmc_calc_divider - calculate GPMC_FCLK divider for sync_clk GPMC_CLK period.
> + * @sync_clk GPMC_CLK period in ps.
> + * @return   Returns at least 1 if GPMC_FCLK can be divided to GPMC_CLK.
> + *           Else, returns -1.
> + */
>  int gpmc_calc_divider(unsigned int sync_clk)
>  {
> -	int div;
> -	u32 l;
> +	int div = gpmc_ps_to_ticks(sync_clk);
>  
> -	l = sync_clk + (gpmc_get_fclk_period() - 1);
> -	div = l / gpmc_get_fclk_period();
>  	if (div > 4)
>  		return -1;
>  	if (div <= 0)
> @@ -526,7 +556,13 @@ int gpmc_calc_divider(unsigned int sync_clk)
>  	return div;
>  }
>  
> -int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
> +/**
> + * gpmc_cs_set_timings - program timing parameters for Chip Select Region.
> + * @cs   Chip Select Region.
> + * @t    GPMC timing parameters.
> + * @s    GPMC timing settings.
> + */
> +int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_settings *s)
>  {
>  	int div;
>  	u32 l;
> @@ -536,6 +572,32 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
>  	if (div < 0)
>  		return div;
>  
> +	/* 
> +	 * See if we need to change the divider for waitmonitoringtime.
> +	 * 
> +	 * If DT contains sync-clk-ps for truly asynchronous accesses,
> +	 * ignore it as long as waitmonitoringtime is used.
> +	 * 

The comment in the $subject was more easier to understand for me than the above

"Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
truly asynchronous accesses, i.e. both read and write asynchronous."

> +	 * This protects mixed synchronous and asynchronous devices,
> +	 * i.e. must not change div to scale async waitmonitoringtime or
> +	 * sync accesses would be broken.
> +	 * 
> +	 * We raise an error later if waitmonitoringtime does not fit.
> +	 */
> +	if (!s->sync_read && !s->sync_write &&
> +	    (s->wait_on_read || s->wait_on_write)
> +	   ) {
> +
> +		div = gpmc_calc_waitmonitoring_divider(t->wait_monitoring);
> +		if (div < 0) {
> +			pr_err("%s: waitmonitoringtime %3d ns too large for greatest gpmcfclkdivider.\n",
> +			       __func__,
> +			       t->wait_monitoring
> +			       );
> +			return -1;
> +		}
> +	}
> +
>  	GPMC_SET_ONE(GPMC_CS_CONFIG2,  0,  3, cs_on);
>  	GPMC_SET_ONE(GPMC_CS_CONFIG2,  8, 12, cs_rd_off);
>  	GPMC_SET_ONE(GPMC_CS_CONFIG2, 16, 20, cs_wr_off);
> @@ -1793,7 +1855,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  	if (ret < 0)
>  		goto err;
>  
> -	ret = gpmc_cs_set_timings(cs, &gpmc_t);
> +	ret = gpmc_cs_set_timings(cs, &gpmc_t, &gpmc_s);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to set gpmc timings for: %s\n",
>  			child->name);
> diff --git a/include/linux/omap-gpmc.h b/include/linux/omap-gpmc.h
> index c2080ee..9301437 100644
> --- a/include/linux/omap-gpmc.h
> +++ b/include/linux/omap-gpmc.h
> @@ -163,7 +163,7 @@ extern unsigned int gpmc_ticks_to_ns(unsigned int ticks);
>  
>  extern void gpmc_cs_write_reg(int cs, int idx, u32 val);
>  extern int gpmc_calc_divider(unsigned int sync_clk);
> -extern int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t);
> +extern int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_settings *s);
>  extern int gpmc_cs_program_settings(int cs, struct gpmc_settings *p);
>  extern int gpmc_cs_request(int cs, unsigned long size, unsigned long *base);
>  extern void gpmc_cs_free(int cs);
> 

cheers,
-roger

Re: [PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Roger Quadros]

On 24/02/15 22:05, Robert ABEL wrote:
> 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.
> 
> Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
> truly asynchronous accesses, i.e. both read and write asynchronous.
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  arch/arm/mach-omap2/gpmc-nand.c    | 17 ++++-----
>  arch/arm/mach-omap2/gpmc-onenand.c |  4 +--
>  drivers/memory/omap-gpmc.c         | 74 ++++++++++++++++++++++++++++++++++----
>  include/linux/omap-gpmc.h          |  2 +-
>  4 files changed, 80 insertions(+), 17 deletions(-)
> 

./scripts/checkpatch.pl detects some styling errors.

> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
> index d5951b1..e863a59 100644
> --- a/arch/arm/mach-omap2/gpmc-nand.c
> +++ b/arch/arm/mach-omap2/gpmc-nand.c
> @@ -96,14 +96,6 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
>  	gpmc_nand_res[1].start = gpmc_get_client_irq(GPMC_IRQ_FIFOEVENTENABLE);
>  	gpmc_nand_res[2].start = gpmc_get_client_irq(GPMC_IRQ_COUNT_EVENT);
>  
> -	if (gpmc_t) {
> -		err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t);
> -		if (err < 0) {
> -			pr_err("omap2-gpmc: Unable to set gpmc timings: %d\n", err);
> -			return err;
> -		}
> -	}
> -
>  	memset(&s, 0, sizeof(struct gpmc_settings));
>  	if (gpmc_nand_data->of_node)
>  		gpmc_read_settings_dt(gpmc_nand_data->of_node, &s);
> @@ -111,6 +103,15 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
>  		gpmc_set_legacy(gpmc_nand_data, &s);
>  
>  	s.device_nand = true;
> +
> +	if (gpmc_t) {
> +		err = gpmc_cs_set_timings(gpmc_nand_data->cs, gpmc_t, &s);
> +		if (err < 0) {
> +			pr_err("omap2-gpmc: Unable to set gpmc timings: %d\n", err);
> +			return err;
> +		}
> +	}
> +
>  	err = gpmc_cs_program_settings(gpmc_nand_data->cs, &s);
>  	if (err < 0)
>  		goto out_free_cs;
> diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> index 53d197e..f899e77 100644
> --- a/arch/arm/mach-omap2/gpmc-onenand.c
> +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> @@ -293,7 +293,7 @@ static int omap2_onenand_setup_async(void __iomem *onenand_base)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t);
> +	ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t, &onenand_async);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -331,7 +331,7 @@ static int omap2_onenand_setup_sync(void __iomem *onenand_base, int *freq_ptr)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t);
> +	ret = gpmc_cs_set_timings(gpmc_onenand_data->cs, &t, &onenand_sync);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index a6abaf0..9cf8d21 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -139,6 +139,8 @@
>  #define GPMC_CONFIG1_WAIT_READ_MON      (1 << 22)
>  #define GPMC_CONFIG1_WAIT_WRITE_MON     (1 << 21)
>  #define GPMC_CONFIG1_WAIT_MON_IIME(val) ((val & 3) << 18)

Not caused by your patch but we can fix the typo

GPMC_CONFIG1_WAIT_MON_IIME -> GPMC_CONFIG1_WAIT_MON_TIME

> +/** WAITMONITORINGTIME Max Ticks */
> +#define GPMC_CONFIG1_WAIT_MON_TIME_MAX  2
>  #define GPMC_CONFIG1_WAIT_PIN_SEL(val)  ((val & 3) << 16)
>  #define GPMC_CONFIG1_DEVICESIZE(val)    ((val & 3) << 12)
>  #define GPMC_CONFIG1_DEVICESIZE_16      GPMC_CONFIG1_DEVICESIZE(1)
> @@ -511,13 +513,41 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  			t->field, #field) < 0)			\
>  		return -1
>  
> +/**
> + * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
> + * @wait_monitoring WAITMONITORINGTIME in ns.
> + * @return          -1 on failure to scale, else proper divider > 0.
> + * @note            WAITMONITORINGTIME will be _at least_ as long as desired.
> + *                  i.e. read timings should be kept            -> don't sample bus too early
> + *                  while write timings should work out as well -> data is longer on bus
> + */
> +static int gpmc_calc_waitmonitoring_divider(unsigned int wait_monitoring)
> +{
> +
> +	int div = gpmc_ns_to_ticks(wait_monitoring);
> +
> +	div += GPMC_CONFIG1_WAIT_MON_TIME_MAX - 1;
> +	div /= GPMC_CONFIG1_WAIT_MON_TIME_MAX;

Sorry I didn't understand how this works. :P
>From the TRM,

waitmonitoringtime_ns = waitmonitoring_ticks x (gpmc_clk_div + 1) x gpmc_fclk_ns

> +
> +	if (div > 4)
> +		return -1;
> +	if (div <= 0)
> +		div = 1;
> +
> +	return div;
> +
> +}
> +
> +/**
> + * gpmc_calc_divider - calculate GPMC_FCLK divider for sync_clk GPMC_CLK period.
> + * @sync_clk GPMC_CLK period in ps.
> + * @return   Returns at least 1 if GPMC_FCLK can be divided to GPMC_CLK.
> + *           Else, returns -1.
> + */
>  int gpmc_calc_divider(unsigned int sync_clk)
>  {
> -	int div;
> -	u32 l;
> +	int div = gpmc_ps_to_ticks(sync_clk);
>  
> -	l = sync_clk + (gpmc_get_fclk_period() - 1);
> -	div = l / gpmc_get_fclk_period();
>  	if (div > 4)
>  		return -1;
>  	if (div <= 0)
> @@ -526,7 +556,13 @@ int gpmc_calc_divider(unsigned int sync_clk)
>  	return div;
>  }
>  
> -int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
> +/**
> + * gpmc_cs_set_timings - program timing parameters for Chip Select Region.
> + * @cs   Chip Select Region.
> + * @t    GPMC timing parameters.
> + * @s    GPMC timing settings.
> + */
> +int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_settings *s)
>  {
>  	int div;
>  	u32 l;
> @@ -536,6 +572,32 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
>  	if (div < 0)
>  		return div;
>  
> +	/* 
> +	 * See if we need to change the divider for waitmonitoringtime.
> +	 * 
> +	 * If DT contains sync-clk-ps for truly asynchronous accesses,
> +	 * ignore it as long as waitmonitoringtime is used.
> +	 * 

The comment in the $subject was more easier to understand for me than the above

"Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
truly asynchronous accesses, i.e. both read and write asynchronous."

> +	 * This protects mixed synchronous and asynchronous devices,
> +	 * i.e. must not change div to scale async waitmonitoringtime or
> +	 * sync accesses would be broken.
> +	 * 
> +	 * We raise an error later if waitmonitoringtime does not fit.
> +	 */
> +	if (!s->sync_read && !s->sync_write &&
> +	    (s->wait_on_read || s->wait_on_write)
> +	   ) {
> +
> +		div = gpmc_calc_waitmonitoring_divider(t->wait_monitoring);
> +		if (div < 0) {
> +			pr_err("%s: waitmonitoringtime %3d ns too large for greatest gpmcfclkdivider.\n",
> +			       __func__,
> +			       t->wait_monitoring
> +			       );
> +			return -1;
> +		}
> +	}
> +
>  	GPMC_SET_ONE(GPMC_CS_CONFIG2,  0,  3, cs_on);
>  	GPMC_SET_ONE(GPMC_CS_CONFIG2,  8, 12, cs_rd_off);
>  	GPMC_SET_ONE(GPMC_CS_CONFIG2, 16, 20, cs_wr_off);
> @@ -1793,7 +1855,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  	if (ret < 0)
>  		goto err;
>  
> -	ret = gpmc_cs_set_timings(cs, &gpmc_t);
> +	ret = gpmc_cs_set_timings(cs, &gpmc_t, &gpmc_s);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to set gpmc timings for: %s\n",
>  			child->name);
> diff --git a/include/linux/omap-gpmc.h b/include/linux/omap-gpmc.h
> index c2080ee..9301437 100644
> --- a/include/linux/omap-gpmc.h
> +++ b/include/linux/omap-gpmc.h
> @@ -163,7 +163,7 @@ extern unsigned int gpmc_ticks_to_ns(unsigned int ticks);
>  
>  extern void gpmc_cs_write_reg(int cs, int idx, u32 val);
>  extern int gpmc_calc_divider(unsigned int sync_clk);
> -extern int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t);
> +extern int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_settings *s);
>  extern int gpmc_cs_program_settings(int cs, struct gpmc_settings *p);
>  extern int gpmc_cs_request(int cs, unsigned long size, unsigned long *base);
>  extern void gpmc_cs_free(int cs);
> 

cheers,
-roger

Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Roger Quadros]

Robert,

On 24/02/15 22:05, Robert ABEL wrote:
> 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.
> 
> This patch correctly computes WAITMONITORINGTIME in GPMC_CLK cycles instead of GPMC_FCLK cycles,
> both during programming (gpmc_cs_set_timings) and during retrieval (gpmc_cs_show_timings).
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  drivers/memory/omap-gpmc.c | 125 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 99 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 9cf8d21..6591991 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -170,6 +170,11 @@
>   */
>  #define	GPMC_NR_IRQ		2
>  
> +enum gpmc_clk_domain {
> +	GPMC_CD_FCLK,
> +	GPMC_CD_CLK
> +};
> +
>  struct gpmc_cs_data {
>  	const char *name;
>  
> @@ -268,16 +273,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 clock domain in ps
> + * @cs Chip Select Region.
> + * @cd Clock Domain.
> + *
> + * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
> + * prior to calling this function with GPMC_CD_CLK.
> + * 
> + */
> +static unsigned long gpmc_get_clk_period(int cs, enum gpmc_clk_domain cd)
> +{
> +
> +	unsigned long tick_ps = gpmc_get_fclk_period();
> +	u32 l;
> +	int div;
> +
> +	switch (cd) {
> +	case GPMC_CD_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_CD_FCLK:
> +		/* FALL-THROUGH */
> +	default:
> +		break;
> +	}
> +
> +	return tick_ps;
> +
> +}
> +
> +static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, enum gpmc_clk_domain cd)
>  {
>  	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, cd);
>  
>  	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, /* any CS */ 0, GPMC_CD_FCLK);
> +}
> +
>  static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
>  {
>  	unsigned long tick_ps;
> @@ -288,9 +332,14 @@ static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
>  	return (time_ps + tick_ps - 1) / tick_ps;
>  }
>  
> +unsigned int gpmc_clk_ticks_to_ns(unsigned ticks, int cs, enum gpmc_clk_domain cd)
> +{
> +	return ticks * gpmc_get_clk_period(cs, cd) / 1000;
> +}
> +
>  unsigned int gpmc_ticks_to_ns(unsigned int ticks)
>  {
> -	return ticks * gpmc_get_fclk_period() / 1000;
> +	return gpmc_clk_ticks_to_ns(ticks, /* any CS */ 0, GPMC_CD_FCLK);
>  }
>  
>  static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
> @@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>   * @st_bit  Start Bit
>   * @end_bit End Bit. Must be >= @st_bit.
>   * @name    DTS node name, w/o "gpmc,"
> + * @cd      Clock Domain of timing parameter.
> + * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>   * @raw     Raw Format Option.
>   *          raw format:  gpmc,name = <value>
>   *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
>   * @noval   Parameter values equal to 0 are not printed.
> - * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>   *
>   */
> -static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> -			       bool raw, bool noval, int shift,
> -			       const char *name)
> +static int get_gpmc_timing_reg(
> +	/* timing specifiers */
> +	int cs, int reg, int st_bit, int end_bit,
> +	const char *name, const enum gpmc_clk_domain cd,
> +	/* value transform */
> +	int shift,
> +	/* format specifiers */
> +	bool raw, bool noval)

now that you are rearranging the parameters, "name" parameter should probably be
at the same position (or last) in get_gpmc_timing_reg() and set_gpmc_timing_reg()?
Also clock domain (cd) position could be matched if possible.

>  {
>  	u32 l;
>  	int nr_bits;
> @@ -373,7 +428,7 @@ static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  		/* DTS tick format for timings in ns */
>  		unsigned int time_ns;
>  
> -		time_ns = gpmc_ticks_to_ns(l);
> +		time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
>  		pr_info("gpmc,%s = <%u> /* %i ticks */\n",
>  			name, time_ns, l);
>  	} else {
> @@ -388,13 +443,15 @@ static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  	pr_info("cs%i %s: 0x%08x\n", cs, #config, \
>  		gpmc_cs_read_reg(cs, config))
>  #define GPMC_GET_RAW(reg, st, end, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), 1, 0, 0, field)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 0)
>  #define GPMC_GET_RAW_BOOL(reg, st, end, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), 1, 1, 0, field)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 1)
>  #define GPMC_GET_RAW_SHIFT(reg, st, end, shift, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), 1, 1, (shift), field)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, (shift), 1, 1)
>  #define GPMC_GET_TICKS(reg, st, end, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, 0, 0, field)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 0, 0)
> +#define GPMC_GET_TICKS_CD(reg, st, end, field, cd) \
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), field, (cd), 0, 0, 0)
>  
>  static void gpmc_show_regs(int cs, const char *desc)
>  {
> @@ -462,7 +519,7 @@ static void gpmc_cs_show_timings(int cs, const char *desc)
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 0, 3, "bus-turnaround-ns");
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 8, 11, "cycle2cycle-delay-ns");
>  
> -	GPMC_GET_TICKS(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns");
> +	GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns", GPMC_CD_CLK);
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, "clk-activation-ns");
>  
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 16, 19, "wr-data-mux-bus-ns");
> @@ -474,8 +531,20 @@ static inline void gpmc_cs_show_timings(int cs, const char *desc)
>  }
>  #endif
>  
> +/**
> + * set_gpmc_timing_reg - set a single timing parameter for Chip Select Region.
> + * @cs      Chip Select Region.
> + * @reg     GPMC_CS_CONFIGn register offset.
> + * @st_bit  Start Bit
> + * @end_bit End Bit. Must be >= @st_bit.
> + * @time    Timing parameter in ns.
> + * @cd      Timing parameter clock domain.
> + * @name    Timing parameter name.
> + * @note    Caller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER

@note is not a parameter.

> + *          prior to calling this function with @cd equal to GPMC_CD_CLK.
> + */
>  static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> -			       int time, const char *name)
> +			       int time, enum gpmc_clk_domain cd, const char *name)
>  {
>  	u32 l;
>  	int ticks, mask, nr_bits;
> @@ -483,12 +552,12 @@ 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, cd);
>  	nr_bits = end_bit - st_bit + 1;
>  	mask = (1 << nr_bits) - 1;
>  
>  	if (ticks > mask) {
> -		pr_err("%s: GPMC error! CS%d: %s: %d ns, %d ticks > %d\n",
> +		pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",

any reason for removing the "error!" string?

>  		       __func__, cs, name, time, ticks, mask);
>  
>  		return -1;
> @@ -498,7 +567,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, cd) * ticks / 1000,
>  			(l >> st_bit) & mask, time);
>  #endif
>  	l &= ~(mask << st_bit);
> @@ -508,11 +577,14 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  	return 0;
>  }
>  
> -#define GPMC_SET_ONE(reg, st, end, field) \
> -	if (set_gpmc_timing_reg(cs, (reg), (st), (end),		\
> -			t->field, #field) < 0)			\
> +#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
> +	if (set_gpmc_timing_reg(cs, (reg), (st), (end), \
> +	    t->field, (cd), #field) < 0)                \
>  		return -1
>  
> +#define GPMC_SET_ONE(reg, st, end, field) \
> +	GPMC_SET_ONE_CD(reg, st, end, field, GPMC_CD_FCLK)
> +
>  /**
>   * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
>   * @wait_monitoring WAITMONITORINGTIME in ns.
> @@ -620,22 +692,23 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_
>  	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_CD(GPMC_CS_CONFIG1, 18, 19, wait_monitoring, GPMC_CD_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);
>  	gpmc_cs_show_timings(cs, "after gpmc_cs_set_timings");
> 

cheers,
-roger

Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Roger Quadros]

Robert,

On 24/02/15 22:05, Robert ABEL wrote:
> 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.
> 
> This patch correctly computes WAITMONITORINGTIME in GPMC_CLK cycles instead of GPMC_FCLK cycles,
> both during programming (gpmc_cs_set_timings) and during retrieval (gpmc_cs_show_timings).
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  drivers/memory/omap-gpmc.c | 125 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 99 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 9cf8d21..6591991 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -170,6 +170,11 @@
>   */
>  #define	GPMC_NR_IRQ		2
>  
> +enum gpmc_clk_domain {
> +	GPMC_CD_FCLK,
> +	GPMC_CD_CLK
> +};
> +
>  struct gpmc_cs_data {
>  	const char *name;
>  
> @@ -268,16 +273,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 clock domain in ps
> + * @cs Chip Select Region.
> + * @cd Clock Domain.
> + *
> + * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
> + * prior to calling this function with GPMC_CD_CLK.
> + * 
> + */
> +static unsigned long gpmc_get_clk_period(int cs, enum gpmc_clk_domain cd)
> +{
> +
> +	unsigned long tick_ps = gpmc_get_fclk_period();
> +	u32 l;
> +	int div;
> +
> +	switch (cd) {
> +	case GPMC_CD_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_CD_FCLK:
> +		/* FALL-THROUGH */
> +	default:
> +		break;
> +	}
> +
> +	return tick_ps;
> +
> +}
> +
> +static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, enum gpmc_clk_domain cd)
>  {
>  	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, cd);
>  
>  	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, /* any CS */ 0, GPMC_CD_FCLK);
> +}
> +
>  static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
>  {
>  	unsigned long tick_ps;
> @@ -288,9 +332,14 @@ static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
>  	return (time_ps + tick_ps - 1) / tick_ps;
>  }
>  
> +unsigned int gpmc_clk_ticks_to_ns(unsigned ticks, int cs, enum gpmc_clk_domain cd)
> +{
> +	return ticks * gpmc_get_clk_period(cs, cd) / 1000;
> +}
> +
>  unsigned int gpmc_ticks_to_ns(unsigned int ticks)
>  {
> -	return ticks * gpmc_get_fclk_period() / 1000;
> +	return gpmc_clk_ticks_to_ns(ticks, /* any CS */ 0, GPMC_CD_FCLK);
>  }
>  
>  static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
> @@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>   * @st_bit  Start Bit
>   * @end_bit End Bit. Must be >= @st_bit.
>   * @name    DTS node name, w/o "gpmc,"
> + * @cd      Clock Domain of timing parameter.
> + * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>   * @raw     Raw Format Option.
>   *          raw format:  gpmc,name = <value>
>   *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
>   * @noval   Parameter values equal to 0 are not printed.
> - * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>   *
>   */
> -static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> -			       bool raw, bool noval, int shift,
> -			       const char *name)
> +static int get_gpmc_timing_reg(
> +	/* timing specifiers */
> +	int cs, int reg, int st_bit, int end_bit,
> +	const char *name, const enum gpmc_clk_domain cd,
> +	/* value transform */
> +	int shift,
> +	/* format specifiers */
> +	bool raw, bool noval)

now that you are rearranging the parameters, "name" parameter should probably be
at the same position (or last) in get_gpmc_timing_reg() and set_gpmc_timing_reg()?
Also clock domain (cd) position could be matched if possible.

>  {
>  	u32 l;
>  	int nr_bits;
> @@ -373,7 +428,7 @@ static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  		/* DTS tick format for timings in ns */
>  		unsigned int time_ns;
>  
> -		time_ns = gpmc_ticks_to_ns(l);
> +		time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
>  		pr_info("gpmc,%s = <%u> /* %i ticks */\n",
>  			name, time_ns, l);
>  	} else {
> @@ -388,13 +443,15 @@ static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  	pr_info("cs%i %s: 0x%08x\n", cs, #config, \
>  		gpmc_cs_read_reg(cs, config))
>  #define GPMC_GET_RAW(reg, st, end, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), 1, 0, 0, field)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 0)
>  #define GPMC_GET_RAW_BOOL(reg, st, end, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), 1, 1, 0, field)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1, 1)
>  #define GPMC_GET_RAW_SHIFT(reg, st, end, shift, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), 1, 1, (shift), field)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, (shift), 1, 1)
>  #define GPMC_GET_TICKS(reg, st, end, field) \
> -	get_gpmc_timing_reg(cs, (reg), (st), (end), 0, 0, 0, field)
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 0, 0)
> +#define GPMC_GET_TICKS_CD(reg, st, end, field, cd) \
> +	get_gpmc_timing_reg(cs, (reg), (st), (end), field, (cd), 0, 0, 0)
>  
>  static void gpmc_show_regs(int cs, const char *desc)
>  {
> @@ -462,7 +519,7 @@ static void gpmc_cs_show_timings(int cs, const char *desc)
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 0, 3, "bus-turnaround-ns");
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 8, 11, "cycle2cycle-delay-ns");
>  
> -	GPMC_GET_TICKS(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns");
> +	GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns", GPMC_CD_CLK);
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, "clk-activation-ns");
>  
>  	GPMC_GET_TICKS(GPMC_CS_CONFIG6, 16, 19, "wr-data-mux-bus-ns");
> @@ -474,8 +531,20 @@ static inline void gpmc_cs_show_timings(int cs, const char *desc)
>  }
>  #endif
>  
> +/**
> + * set_gpmc_timing_reg - set a single timing parameter for Chip Select Region.
> + * @cs      Chip Select Region.
> + * @reg     GPMC_CS_CONFIGn register offset.
> + * @st_bit  Start Bit
> + * @end_bit End Bit. Must be >= @st_bit.
> + * @time    Timing parameter in ns.
> + * @cd      Timing parameter clock domain.
> + * @name    Timing parameter name.
> + * @note    Caller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER

@note is not a parameter.

> + *          prior to calling this function with @cd equal to GPMC_CD_CLK.
> + */
>  static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> -			       int time, const char *name)
> +			       int time, enum gpmc_clk_domain cd, const char *name)
>  {
>  	u32 l;
>  	int ticks, mask, nr_bits;
> @@ -483,12 +552,12 @@ 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, cd);
>  	nr_bits = end_bit - st_bit + 1;
>  	mask = (1 << nr_bits) - 1;
>  
>  	if (ticks > mask) {
> -		pr_err("%s: GPMC error! CS%d: %s: %d ns, %d ticks > %d\n",
> +		pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",

any reason for removing the "error!" string?

>  		       __func__, cs, name, time, ticks, mask);
>  
>  		return -1;
> @@ -498,7 +567,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, cd) * ticks / 1000,
>  			(l >> st_bit) & mask, time);
>  #endif
>  	l &= ~(mask << st_bit);
> @@ -508,11 +577,14 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  	return 0;
>  }
>  
> -#define GPMC_SET_ONE(reg, st, end, field) \
> -	if (set_gpmc_timing_reg(cs, (reg), (st), (end),		\
> -			t->field, #field) < 0)			\
> +#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \
> +	if (set_gpmc_timing_reg(cs, (reg), (st), (end), \
> +	    t->field, (cd), #field) < 0)                \
>  		return -1
>  
> +#define GPMC_SET_ONE(reg, st, end, field) \
> +	GPMC_SET_ONE_CD(reg, st, end, field, GPMC_CD_FCLK)
> +
>  /**
>   * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
>   * @wait_monitoring WAITMONITORINGTIME in ns.
> @@ -620,22 +692,23 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_
>  	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_CD(GPMC_CS_CONFIG1, 18, 19, wait_monitoring, GPMC_CD_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);
>  	gpmc_cs_show_timings(cs, "after gpmc_cs_set_timings");
> 

cheers,
-roger

Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Robert Abel]

Hi Roger,

On 25 Feb 2015 17:58, Roger Quadros wrote:
>> static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
>> @@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>>    * @st_bit  Start Bit
>>    * @end_bit End Bit. Must be >= @st_bit.
>>    * @name    DTS node name, w/o "gpmc,"
>> + * @cd      Clock Domain of timing parameter.
>> + * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>>    * @raw     Raw Format Option.
>>    *          raw format:  gpmc,name = <value>
>>    *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
>>    * @noval   Parameter values equal to 0 are not printed.
>> - * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>>    *
>>    */
>> -static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>> -			       bool raw, bool noval, int shift,
>> -			       const char *name)
>> +static int get_gpmc_timing_reg(
>> +	/* timing specifiers */
>> +	int cs, int reg, int st_bit, int end_bit,
>> +	const char *name, const enum gpmc_clk_domain cd,
>> +	/* value transform */
>> +	int shift,
>> +	/* format specifiers */
>> +	bool raw, bool noval)
> now that you are rearranging the parameters, "name" parameter should probably be
> at the same position (or last) in get_gpmc_timing_reg() and set_gpmc_timing_reg()?
> Also clock domain (cd) position could be matched if possible.
I rearranged them primarily, because I wanted to group the specifiers 
according to function, because I found it unnatural to add clock domain 
to the end, when it's "more important" than the format specifiers.
set_gpmc_timing_reg are fine in that regard as it doesn't have format 
specifiers.
>> +/**
>> + * set_gpmc_timing_reg - set a single timing parameter for Chip Select Region.
>> + * @cs      Chip Select Region.
>> + * @reg     GPMC_CS_CONFIGn register offset.
>> + * @st_bit  Start Bit
>> + * @end_bit End Bit. Must be >= @st_bit.
>> + * @time    Timing parameter in ns.
>> + * @cd      Timing parameter clock domain.
>> + * @name    Timing parameter name.
>> + * @note    Caller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER
> @note is not a parameter.
Well no, note's a note. This is a doxygen-style comment, so tools should 
put a note in the created documentation. Doxygen will put a box with 
yellow background, for instance.
>> -		pr_err("%s: GPMC error! CS%d: %s: %d ns, %d ticks > %d\n",
>> +		pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",
> any reason for removing the "error!" string?
It's already pr_err, the "error!" in-between "GPMC CS%d" made it hard to 
read and there's a WARN after that statement in all cases, because a 
child _must_ fail if a timing parameter constraint is broken.

Regards,

Robert

Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Roger Quadros]

Robert,

On 25/02/15 19:07, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 17:58, Roger Quadros wrote:
>>> static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
>>> @@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>>>    * @st_bit  Start Bit
>>>    * @end_bit End Bit. Must be >= @st_bit.
>>>    * @name    DTS node name, w/o "gpmc,"
>>> + * @cd      Clock Domain of timing parameter.
>>> + * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>>>    * @raw     Raw Format Option.
>>>    *          raw format:  gpmc,name = <value>
>>>    *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
>>>    * @noval   Parameter values equal to 0 are not printed.
>>> - * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>>>    *
>>>    */
>>> -static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>>> -                   bool raw, bool noval, int shift,
>>> -                   const char *name)
>>> +static int get_gpmc_timing_reg(
>>> +    /* timing specifiers */
>>> +    int cs, int reg, int st_bit, int end_bit,
>>> +    const char *name, const enum gpmc_clk_domain cd,
>>> +    /* value transform */
>>> +    int shift,
>>> +    /* format specifiers */
>>> +    bool raw, bool noval)
>> now that you are rearranging the parameters, "name" parameter should probably be
>> at the same position (or last) in get_gpmc_timing_reg() and set_gpmc_timing_reg()?
>> Also clock domain (cd) position could be matched if possible.
> I rearranged them primarily, because I wanted to group the specifiers according to function, because I found it unnatural to add clock domain to the end, when it's "more important" than the format specifiers.
> set_gpmc_timing_reg are fine in that regard as it doesn't have format specifiers.

OK.

>>> +/**
>>> + * set_gpmc_timing_reg - set a single timing parameter for Chip Select Region.
>>> + * @cs      Chip Select Region.
>>> + * @reg     GPMC_CS_CONFIGn register offset.
>>> + * @st_bit  Start Bit
>>> + * @end_bit End Bit. Must be >= @st_bit.
>>> + * @time    Timing parameter in ns.
>>> + * @cd      Timing parameter clock domain.
>>> + * @name    Timing parameter name.
>>> + * @note    Caller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER
>> @note is not a parameter.
> Well no, note's a note. This is a doxygen-style comment, so tools should put a note in the created documentation. Doxygen will put a box with yellow background, for instance.

Oh ok.

>>> -        pr_err("%s: GPMC error! CS%d: %s: %d ns, %d ticks > %d\n",
>>> +        pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",
>> any reason for removing the "error!" string?
> It's already pr_err, the "error!" in-between "GPMC CS%d" made it hard to read and there's a WARN after that statement in all cases, because a child _must_ fail if a timing parameter constraint is broken.

How will the user know by looking at the kernel log that it was really an error?
We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary to
clearly show an Error message.

You can probably reword it like "%s: Error!! GPMC CS %d..."

cheers,
-roger

Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Roger Quadros]

Robert,

On 25/02/15 19:07, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 17:58, Roger Quadros wrote:
>>> static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
>>> @@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>>>    * @st_bit  Start Bit
>>>    * @end_bit End Bit. Must be >= @st_bit.
>>>    * @name    DTS node name, w/o "gpmc,"
>>> + * @cd      Clock Domain of timing parameter.
>>> + * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>>>    * @raw     Raw Format Option.
>>>    *          raw format:  gpmc,name = <value>
>>>    *          tick format: gpmc,name = <value> /&zwj;* x ticks *&zwj;/
>>>    * @noval   Parameter values equal to 0 are not printed.
>>> - * @shift   Parameter value left shifts @shift, which is then printed instead of value.
>>>    *
>>>    */
>>> -static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>>> -                   bool raw, bool noval, int shift,
>>> -                   const char *name)
>>> +static int get_gpmc_timing_reg(
>>> +    /* timing specifiers */
>>> +    int cs, int reg, int st_bit, int end_bit,
>>> +    const char *name, const enum gpmc_clk_domain cd,
>>> +    /* value transform */
>>> +    int shift,
>>> +    /* format specifiers */
>>> +    bool raw, bool noval)
>> now that you are rearranging the parameters, "name" parameter should probably be
>> at the same position (or last) in get_gpmc_timing_reg() and set_gpmc_timing_reg()?
>> Also clock domain (cd) position could be matched if possible.
> I rearranged them primarily, because I wanted to group the specifiers according to function, because I found it unnatural to add clock domain to the end, when it's "more important" than the format specifiers.
> set_gpmc_timing_reg are fine in that regard as it doesn't have format specifiers.

OK.

>>> +/**
>>> + * set_gpmc_timing_reg - set a single timing parameter for Chip Select Region.
>>> + * @cs      Chip Select Region.
>>> + * @reg     GPMC_CS_CONFIGn register offset.
>>> + * @st_bit  Start Bit
>>> + * @end_bit End Bit. Must be >= @st_bit.
>>> + * @time    Timing parameter in ns.
>>> + * @cd      Timing parameter clock domain.
>>> + * @name    Timing parameter name.
>>> + * @note    Caller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER
>> @note is not a parameter.
> Well no, note's a note. This is a doxygen-style comment, so tools should put a note in the created documentation. Doxygen will put a box with yellow background, for instance.

Oh ok.

>>> -        pr_err("%s: GPMC error! CS%d: %s: %d ns, %d ticks > %d\n",
>>> +        pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",
>> any reason for removing the "error!" string?
> It's already pr_err, the "error!" in-between "GPMC CS%d" made it hard to read and there's a WARN after that statement in all cases, because a child _must_ fail if a timing parameter constraint is broken.

How will the user know by looking at the kernel log that it was really an error?
We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary to
clearly show an Error message.

You can probably reword it like "%s: Error!! GPMC CS %d..."

cheers,
-roger

Re: [PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Robert Abel]

Hi Roger,

On 25 Feb 2015 17:33, Roger Quadros wrote:
> ./scripts/checkpatch.pl detects some styling errors.
Well, there's like a million lines over 80 characters. I'm also a 
heathen and don't use an 80 character terminal either.
I'll fix the more serious issues checkpatch finds, but some styling 
errors are even from code already in the driver.

>>   #define GPMC_CONFIG1_WAIT_MON_IIME(val) ((val & 3) << 18)
> Not caused by your patch but we can fix the typo
>
> GPMC_CONFIG1_WAIT_MON_IIME -> GPMC_CONFIG1_WAIT_MON_TIME
I'd rather remove than fix. None of these defines is in active use anymore.
>> +/**
>> + * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
>> + * @wait_monitoring WAITMONITORINGTIME in ns.
>> + * @return          -1 on failure to scale, else proper divider > 0.
>> + * @note            WAITMONITORINGTIME will be _at least_ as long as desired.
>> + *                  i.e. read timings should be kept            -> don't sample bus too early
>> + *                  while write timings should work out as well -> data is longer on bus
>> + */
>> +static int gpmc_calc_waitmonitoring_divider(unsigned int wait_monitoring)
>> +{
>> +
>> +	int div = gpmc_ns_to_ticks(wait_monitoring);
>> +
>> +	div += GPMC_CONFIG1_WAIT_MON_TIME_MAX - 1;
>> +	div /= GPMC_CONFIG1_WAIT_MON_TIME_MAX;
> Sorry I didn't understand how this works. :P
>  From the TRM,
>
> waitmonitoringtime_ns = waitmonitoring_ticks x (gpmc_clk_div + 1) x gpmc_fclk_ns
Using that formula:
gpmc_clk_div + 1 = ceil(ceil(waitmonitoringtime_ns / gpmc_fclk_ns) / 
waitmonitoring_ticks).

The reason I use GPMC_CONFIG1_WAIT_MON_TIME_MAX directly, is because 
every other pair is identical:

We have the following cases:
a) WAITMONITORINGTIME = 0, so div doesn't matter (we set div to 1) or
b) WAITMONITORINGTIME = 1, with div = 1 or
c) WAITMONITORINGTIME = 2, with div = n

WAITMONITORINGTIME is never 1 with div = n and div /= 1, because that's 
the same as WAITMONITORINGTIME = 2 with div = n - 1.
Cases a) and b) are caught using the fact that div = 0 after the division.
Case c) is handled by assuming that WAITMONITORINGTIME will always be 2 
(i.e. GPMC_CONFIG1_WAIT_MON_TIME_MAX) for all divs /= 0 at that point.

Took me a while to realize that this works, too.

>> +	/*
>> +	 * See if we need to change the divider for waitmonitoringtime.
>> +	 *
>> +	 * If DT contains sync-clk-ps for truly asynchronous accesses,
>> +	 * ignore it as long as waitmonitoringtime is used.
>> +	 *
> The comment in the $subject was more easier to understand for me than the above
>
> "Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
> truly asynchronous accesses, i.e. both read and write asynchronous."
Well, the comment in $subject was a general description. Here, we don't 
touch div when
a) any access is synchronous
b) all accesses are asynchronous, but WAITMONITORINGTIME is not used, 
i.e. WAITREADMONITORING and WAITWRITEMONITORING are both not set.

So, if WAITMONITORINGTIME is not used, or any access is synchronous, we 
program the sync-clk-ps anyways, because the GPMC won't rely on it for 
any timing.
So the comment is OK, IMHO.

Regards,

Robert

Re: [PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Roger Quadros]

Robert,

On 25/02/15 18:33, Roger Quadros wrote:
> On 24/02/15 22:05, Robert ABEL wrote:
>> 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.
>>
>> Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
>> truly asynchronous accesses, i.e. both read and write asynchronous.
>>
>> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
>> ---
>>  arch/arm/mach-omap2/gpmc-nand.c    | 17 ++++-----
>>  arch/arm/mach-omap2/gpmc-onenand.c |  4 +--
>>  drivers/memory/omap-gpmc.c         | 74 ++++++++++++++++++++++++++++++++++----
>>  include/linux/omap-gpmc.h          |  2 +-
>>  4 files changed, 80 insertions(+), 17 deletions(-)

Need to patch mach-omap2/usb-tusb6010.c as well. else we get

arch/arm/mach-omap2/usb-tusb6010.c: In function ‘tusb_set_async_mode’:
arch/arm/mach-omap2/usb-tusb6010.c:74:2: error: too few arguments to function ‘gpmc_cs_set_timings’
In file included from arch/arm/mach-omap2/gpmc.h:14:0,
                 from arch/arm/mach-omap2/usb-tusb6010.c:23:
include/linux/omap-gpmc.h:166:12: note: declared here
arch/arm/mach-omap2/usb-tusb6010.c: In function ‘tusb_set_sync_mode’:
arch/arm/mach-omap2/usb-tusb6010.c:101:2: error: too few arguments to function ‘gpmc_cs_set_timings’
In file included from arch/arm/mach-omap2/gpmc.h:14:0,
                 from arch/arm/mach-omap2/usb-tusb6010.c:23:
include/linux/omap-gpmc.h:166:12: note: declared here

cheers,
-roger

Re: [PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Roger Quadros]

Robert,

On 25/02/15 18:33, Roger Quadros wrote:
> On 24/02/15 22:05, Robert ABEL wrote:
>> 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.
>>
>> Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
>> truly asynchronous accesses, i.e. both read and write asynchronous.
>>
>> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
>> ---
>>  arch/arm/mach-omap2/gpmc-nand.c    | 17 ++++-----
>>  arch/arm/mach-omap2/gpmc-onenand.c |  4 +--
>>  drivers/memory/omap-gpmc.c         | 74 ++++++++++++++++++++++++++++++++++----
>>  include/linux/omap-gpmc.h          |  2 +-
>>  4 files changed, 80 insertions(+), 17 deletions(-)

Need to patch mach-omap2/usb-tusb6010.c as well. else we get

arch/arm/mach-omap2/usb-tusb6010.c: In function ‘tusb_set_async_mode’:
arch/arm/mach-omap2/usb-tusb6010.c:74:2: error: too few arguments to function ‘gpmc_cs_set_timings’
In file included from arch/arm/mach-omap2/gpmc.h:14:0,
                 from arch/arm/mach-omap2/usb-tusb6010.c:23:
include/linux/omap-gpmc.h:166:12: note: declared here
arch/arm/mach-omap2/usb-tusb6010.c: In function ‘tusb_set_sync_mode’:
arch/arm/mach-omap2/usb-tusb6010.c:101:2: error: too few arguments to function ‘gpmc_cs_set_timings’
In file included from arch/arm/mach-omap2/gpmc.h:14:0,
                 from arch/arm/mach-omap2/usb-tusb6010.c:23:
include/linux/omap-gpmc.h:166:12: note: declared here

cheers,
-roger

Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Robert Abel]

Hi Roger,

On 25 Feb 2015 18:17, Roger Quadros wrote:
> How will the user know by looking at the kernel log that it was really an error?
> We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary to
> clearly show an Error message.
>
> You can probably reword it like "%s: Error!! GPMC CS %d..."

I'll put "error" in there. But just for the record, it's this messaged 
followed by a WARN that explains "failed to add child %s".
So I'd expect the user to put A and B together ;)

Robert

Re: [PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Robert Abel]

On 25 Feb 2015 18:20, Roger Quadros wrote:
> Need to patch mach-omap2/usb-tusb6010.c as well. else we get
>
> arch/arm/mach-omap2/usb-tusb6010.c: In function ‘tusb_set_async_mode’:
> arch/arm/mach-omap2/usb-tusb6010.c:74:2: error: too few arguments to function ‘gpmc_cs_set_timings’
> In file included from arch/arm/mach-omap2/gpmc.h:14:0,
>                   from arch/arm/mach-omap2/usb-tusb6010.c:23:
> include/linux/omap-gpmc.h:166:12: note: declared here
> arch/arm/mach-omap2/usb-tusb6010.c: In function ‘tusb_set_sync_mode’:
> arch/arm/mach-omap2/usb-tusb6010.c:101:2: error: too few arguments to function ‘gpmc_cs_set_timings’
> In file included from arch/arm/mach-omap2/gpmc.h:14:0,
>                   from arch/arm/mach-omap2/usb-tusb6010.c:23:
> include/linux/omap-gpmc.h:166:12: note: declared here
Argh... Another one of those exported function calls :|

Robert

Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Roger Quadros]

On 25/02/15 19:22, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 18:17, Roger Quadros wrote:
>> How will the user know by looking at the kernel log that it was really an error?
>> We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary to
>> clearly show an Error message.
>>
>> You can probably reword it like "%s: Error!! GPMC CS %d..."
> 
> I'll put "error" in there. But just for the record, it's this messaged followed by a WARN that explains "failed to add child %s".
> So I'd expect the user to put A and B together ;)

Sorry, my bad. We are in fact returning -1 in GPMC_SET_ONE().
So no need to add the "Error" string.

cheers,
-roger

Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug

[Roger Quadros]

On 25/02/15 19:22, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 18:17, Roger Quadros wrote:
>> How will the user know by looking at the kernel log that it was really an error?
>> We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary to
>> clearly show an Error message.
>>
>> You can probably reword it like "%s: Error!! GPMC CS %d..."
> 
> I'll put "error" in there. But just for the record, it's this messaged followed by a WARN that explains "failed to add child %s".
> So I'd expect the user to put A and B together ;)

Sorry, my bad. We are in fact returning -1 in GPMC_SET_ONE().
So no need to add the "Error" string.

cheers,
-roger

Re: [PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Roger Quadros]

Robert,

On 25/02/15 19:20, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 17:33, Roger Quadros wrote:
>> ./scripts/checkpatch.pl detects some styling errors.
> Well, there's like a million lines over 80 characters. I'm also a heathen and don't use an 80 character terminal either.
> I'll fix the more serious issues checkpatch finds, but some styling errors are even from code already in the driver.

neither do I but let's fix whatever is possible to fix
in the new code.

> 
>>>   #define GPMC_CONFIG1_WAIT_MON_IIME(val) ((val & 3) << 18)
>> Not caused by your patch but we can fix the typo
>>
>> GPMC_CONFIG1_WAIT_MON_IIME -> GPMC_CONFIG1_WAIT_MON_TIME
> I'd rather remove than fix. None of these defines is in active use anymore.

OK with me.

>>> +/**
>>> + * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
>>> + * @wait_monitoring WAITMONITORINGTIME in ns.
>>> + * @return          -1 on failure to scale, else proper divider > 0.
>>> + * @note            WAITMONITORINGTIME will be _at least_ as long as desired.
>>> + *                  i.e. read timings should be kept            -> don't sample bus too early
>>> + *                  while write timings should work out as well -> data is longer on bus
>>> + */
>>> +static int gpmc_calc_waitmonitoring_divider(unsigned int wait_monitoring)
>>> +{
>>> +
>>> +    int div = gpmc_ns_to_ticks(wait_monitoring);
>>> +
>>> +    div += GPMC_CONFIG1_WAIT_MON_TIME_MAX - 1;
>>> +    div /= GPMC_CONFIG1_WAIT_MON_TIME_MAX;
>> Sorry I didn't understand how this works. :P
>>  From the TRM,
>>
>> waitmonitoringtime_ns = waitmonitoring_ticks x (gpmc_clk_div + 1) x gpmc_fclk_ns
> Using that formula:
> gpmc_clk_div + 1 = ceil(ceil(waitmonitoringtime_ns / gpmc_fclk_ns) / waitmonitoring_ticks).

Right, we need to return div = gpmc_div + 1 to take care of the (div - 1) done by the caller before
programming div.
I wouldn't mind if we update gpmc_calc_divider() and this function to return the actual div value
that needs to be programmed in the register. It is upto you.

How about mentioning the above formula in a comment for benefit of future readers?
you could also mention that reducing the formula to ticks we get.

gpmc_div + 1 = ceil(wanted_waitmonitoring_ticks/waitmonitoring_ticks)

> 
> The reason I use GPMC_CONFIG1_WAIT_MON_TIME_MAX directly, is because every other pair is identical:
> 
> We have the following cases:
> a) WAITMONITORINGTIME = 0, so div doesn't matter (we set div to 1) or
> b) WAITMONITORINGTIME = 1, with div = 1 or
> c) WAITMONITORINGTIME = 2, with div = n
> 
> WAITMONITORINGTIME is never 1 with div = n and div /= 1, because that's the same as WAITMONITORINGTIME = 2 with div = n - 1.
> Cases a) and b) are caught using the fact that div = 0 after the division.
> Case c) is handled by assuming that WAITMONITORINGTIME will always be 2 (i.e. GPMC_CONFIG1_WAIT_MON_TIME_MAX) for all divs /= 0 at that point.
> 
> Took me a while to realize that this works, too.
> 
>>> +    /*
>>> +     * See if we need to change the divider for waitmonitoringtime.
>>> +     *
>>> +     * If DT contains sync-clk-ps for truly asynchronous accesses,
>>> +     * ignore it as long as waitmonitoringtime is used.
>>> +     *
>> The comment in the $subject was more easier to understand for me than the above
>>
>> "Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
>> truly asynchronous accesses, i.e. both read and write asynchronous."
> Well, the comment in $subject was a general description. Here, we don't touch div when
> a) any access is synchronous
> b) all accesses are asynchronous, but WAITMONITORINGTIME is not used, i.e. WAITREADMONITORING and WAITWRITEMONITORING are both not set.
> 
> So, if WAITMONITORINGTIME is not used, or any access is synchronous, we program the sync-clk-ps anyways, because the GPMC won't rely on it for any timing.
> So the comment is OK, IMHO.

OK with me.

cheers,
-roger

Re: [PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Roger Quadros]

Robert,

On 25/02/15 19:20, Robert Abel wrote:
> Hi Roger,
> 
> On 25 Feb 2015 17:33, Roger Quadros wrote:
>> ./scripts/checkpatch.pl detects some styling errors.
> Well, there's like a million lines over 80 characters. I'm also a heathen and don't use an 80 character terminal either.
> I'll fix the more serious issues checkpatch finds, but some styling errors are even from code already in the driver.

neither do I but let's fix whatever is possible to fix
in the new code.

> 
>>>   #define GPMC_CONFIG1_WAIT_MON_IIME(val) ((val & 3) << 18)
>> Not caused by your patch but we can fix the typo
>>
>> GPMC_CONFIG1_WAIT_MON_IIME -> GPMC_CONFIG1_WAIT_MON_TIME
> I'd rather remove than fix. None of these defines is in active use anymore.

OK with me.

>>> +/**
>>> + * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
>>> + * @wait_monitoring WAITMONITORINGTIME in ns.
>>> + * @return          -1 on failure to scale, else proper divider > 0.
>>> + * @note            WAITMONITORINGTIME will be _at least_ as long as desired.
>>> + *                  i.e. read timings should be kept            -> don't sample bus too early
>>> + *                  while write timings should work out as well -> data is longer on bus
>>> + */
>>> +static int gpmc_calc_waitmonitoring_divider(unsigned int wait_monitoring)
>>> +{
>>> +
>>> +    int div = gpmc_ns_to_ticks(wait_monitoring);
>>> +
>>> +    div += GPMC_CONFIG1_WAIT_MON_TIME_MAX - 1;
>>> +    div /= GPMC_CONFIG1_WAIT_MON_TIME_MAX;
>> Sorry I didn't understand how this works. :P
>>  From the TRM,
>>
>> waitmonitoringtime_ns = waitmonitoring_ticks x (gpmc_clk_div + 1) x gpmc_fclk_ns
> Using that formula:
> gpmc_clk_div + 1 = ceil(ceil(waitmonitoringtime_ns / gpmc_fclk_ns) / waitmonitoring_ticks).

Right, we need to return div = gpmc_div + 1 to take care of the (div - 1) done by the caller before
programming div.
I wouldn't mind if we update gpmc_calc_divider() and this function to return the actual div value
that needs to be programmed in the register. It is upto you.

How about mentioning the above formula in a comment for benefit of future readers?
you could also mention that reducing the formula to ticks we get.

gpmc_div + 1 = ceil(wanted_waitmonitoring_ticks/waitmonitoring_ticks)

> 
> The reason I use GPMC_CONFIG1_WAIT_MON_TIME_MAX directly, is because every other pair is identical:
> 
> We have the following cases:
> a) WAITMONITORINGTIME = 0, so div doesn't matter (we set div to 1) or
> b) WAITMONITORINGTIME = 1, with div = 1 or
> c) WAITMONITORINGTIME = 2, with div = n
> 
> WAITMONITORINGTIME is never 1 with div = n and div /= 1, because that's the same as WAITMONITORINGTIME = 2 with div = n - 1.
> Cases a) and b) are caught using the fact that div = 0 after the division.
> Case c) is handled by assuming that WAITMONITORINGTIME will always be 2 (i.e. GPMC_CONFIG1_WAIT_MON_TIME_MAX) for all divs /= 0 at that point.
> 
> Took me a while to realize that this works, too.
> 
>>> +    /*
>>> +     * See if we need to change the divider for waitmonitoringtime.
>>> +     *
>>> +     * If DT contains sync-clk-ps for truly asynchronous accesses,
>>> +     * ignore it as long as waitmonitoringtime is used.
>>> +     *
>> The comment in the $subject was more easier to understand for me than the above
>>
>> "Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
>> truly asynchronous accesses, i.e. both read and write asynchronous."
> Well, the comment in $subject was a general description. Here, we don't touch div when
> a) any access is synchronous
> b) all accesses are asynchronous, but WAITMONITORINGTIME is not used, i.e. WAITREADMONITORING and WAITWRITEMONITORING are both not set.
> 
> So, if WAITMONITORINGTIME is not used, or any access is synchronous, we program the sync-clk-ps anyways, because the GPMC won't rely on it for any timing.
> So the comment is OK, IMHO.

OK with me.

cheers,
-roger