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

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

[Robert ABEL]

These are the changes I proposed in three separate patchsets
#([1], [2], [3]) 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 --> correct 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
[3]: https://lkml.org/lkml/2015/2/24/609

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 +-
 arch/arm/mach-omap2/usb-tusb6010.c |   4 +-
 drivers/memory/Makefile            |   2 +
 drivers/memory/omap-gpmc.c         | 313 +++++++++++++++++++++++++++++--------
 include/linux/omap-gpmc.h          |   2 +-
 6 files changed, 265 insertions(+), 77 deletions(-)

-- 
2.3.0

[PATCH 1/8] 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 2/8] ARM OMAP2+ GPMC: add bus children

[Robert ABEL]

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

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

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 5cabac8..74a8c52 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,8 +1801,20 @@ 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))
-		return 0;
+
+	/* create platform device, NULL on error or when disabled */
+	if (!of_platform_device_create(child, NULL, &pdev->dev))
+		goto err_child_fail;
+
+	/* is child a common bus? */
+	if (of_match_node(of_default_bus_match_table, child))
+		/* create children and other common bus children */
+		if (of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev))
+			goto err_child_fail;
+
+	return 0;
+
+err_child_fail:
 
 	dev_err(&pdev->dev, "failed to create gpmc child %s\n", child->name);
 	ret = -ENODEV;
-- 
2.3.0

[PATCH 4/8] 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 74a8c52..dbb6753 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 4/8] 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 replaced with interval of values that produce the same
number of clock ticks.

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

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index dbb6753..9340e7a 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -337,32 +337,49 @@ 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 ns -- y ns]; x ticks *&zwj;/
+ *          Where (x ns -- y ns] is the half-open interval from x ns to y ns that
+ *          result in the same tick value.
+ * @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;
+		unsigned int time_ns_min;
 
 		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> /* (%u ns - %u ns]; %i ticks */\n",
+			name, time_ns, time_ns_min, time_ns, l);
 	} else {
+		/* raw format */
 		pr_info("gpmc,%s = <%u>\n", name, l);
 	}
 
-- 
2.3.0

[PATCH 5/8] 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 | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 9340e7a..4139f0d 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -498,7 +498,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
+	pr_info(
 		"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);
@@ -570,19 +570,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);
+	pr_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 6/8] 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
pure 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 +-
 arch/arm/mach-omap2/usb-tusb6010.c |  4 +-
 drivers/memory/omap-gpmc.c         | 82 ++++++++++++++++++++++++++++++++++----
 include/linux/omap-gpmc.h          |  2 +-
 5 files changed, 89 insertions(+), 20 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/arch/arm/mach-omap2/usb-tusb6010.c b/arch/arm/mach-omap2/usb-tusb6010.c
index 8333400..e554d9e 100644
--- a/arch/arm/mach-omap2/usb-tusb6010.c
+++ b/arch/arm/mach-omap2/usb-tusb6010.c
@@ -71,7 +71,7 @@ static int tusb_set_async_mode(unsigned sysclk_ps)
 
 	gpmc_calc_timings(&t, &tusb_async, &dev_t);
 
-	return gpmc_cs_set_timings(async_cs, &t);
+	return gpmc_cs_set_timings(async_cs, &t, &tusb_async);
 }
 
 static int tusb_set_sync_mode(unsigned sysclk_ps)
@@ -98,7 +98,7 @@ static int tusb_set_sync_mode(unsigned sysclk_ps)
 
 	gpmc_calc_timings(&t, &tusb_sync, &dev_t);
 
-	return gpmc_cs_set_timings(sync_cs, &t);
+	return gpmc_cs_set_timings(sync_cs, &t, &tusb_sync);
 }
 
 /* tusb driver calls this when it changes the chip's clocking */
diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 4139f0d..d71ea05 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -138,7 +138,9 @@
 #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)
-#define GPMC_CONFIG1_WAIT_MON_IIME(val) ((val & 3) << 18)
+#define GPMC_CONFIG1_WAIT_MON_TIME(val) ((val & 3) << 18)
+/** WAITMONITORINGTIME Max Ticks */
+#define GPMC_CONFIG1_WAITMONITORINGTIME_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)
@@ -515,13 +517,46 @@ 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
+ *                  Formula:
+ *                  gpmc_clk_div + 1 = ceil(ceil(waitmonitoringtime_ns / gpmc_fclk_ns)
+ *                                     / waitmonitoring_ticks)
+ *                  WAITMONITORINGTIME resulting in 0 or 1 tick with div = 1 are caught by
+ *                  div <= 0 check.
+ */
+static int gpmc_calc_waitmonitoring_divider(unsigned int wait_monitoring)
+{
+
+	int div = gpmc_ns_to_ticks(wait_monitoring);
+
+	div += GPMC_CONFIG1_WAITMONITORINGTIME_MAX - 1;
+	div /= GPMC_CONFIG1_WAITMONITORINGTIME_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)
@@ -530,7 +565,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;
@@ -540,6 +581,33 @@ 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.
+	 *
+	 * Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
+	 * pure asynchronous accesses, i.e. both read and write asynchronous.
+	 * However, only do so if WAITMONITORINGTIME is actually used, i.e.
+	 * either WAITREADMONITORING or WAITWRITEMONITORING is set.
+	 *
+	 * This statement must not change div to scale async WAITMONITORINGTIME to
+	 * protect mixed synchronous and asynchronous accesses.
+	 *
+	 * 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);
@@ -1797,7 +1865,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 7/8] 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 | 126 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 99 insertions(+), 27 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index d71ea05..400b0a6 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,54 @@ 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 +331,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,18 +394,24 @@ 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 ns -- y ns]; x ticks *&zwj;/
  *          Where (x ns -- y ns] is the half-open interval from x ns to y ns that
  *          result in the same tick value.
  * @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;
@@ -376,8 +430,8 @@ static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 		unsigned int time_ns;
 		unsigned int time_ns_min;
 
-		time_ns_min = gpmc_ticks_to_ns(l ? l - 1 : 0);
-		time_ns = gpmc_ticks_to_ns(l);
+		time_ns_min = gpmc_clk_ticks_to_ns(l ? l - 1 : 0, cs, cd);
+		time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
 		pr_info("gpmc,%s = <%u> /* (%u ns - %u ns]; %i ticks */\n",
 			name, time_ns, time_ns_min, time_ns, l);
 	} else {
@@ -392,13 +446,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)
 {
@@ -466,7 +522,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");
@@ -478,8 +534,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;
@@ -487,12 +555,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;
@@ -502,7 +570,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 #ifdef DEBUG
 	pr_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);
@@ -512,11 +580,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.
@@ -630,22 +701,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
 	pr_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 8/8] 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 ATTACHEDDEVICEPAGELENGTH 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 | 68 ++++++++++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 23 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 400b0a6..7e5300d 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -135,7 +135,11 @@
 #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)
+/** ATTACHEDDEVICEPAGELENGTH Max Value */
+#define GPMC_CONFIG1_ATTACHEDDEVICEPAGELENGTH_MAX 2
 #define GPMC_CONFIG1_WAIT_READ_MON      (1 << 22)
 #define GPMC_CONFIG1_WAIT_WRITE_MON     (1 << 21)
 #define GPMC_CONFIG1_WAIT_MON_TIME(val) ((val & 3) << 18)
@@ -144,6 +148,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     1
 #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)
@@ -393,6 +399,8 @@ 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 (before 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.
@@ -401,12 +409,13 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
  *          tick format: gpmc,name = <value> /&zwj;*(x ns -- y ns]; x ticks *&zwj;/
  *          Where (x ns -- y ns] is the half-open interval from x ns to y ns that
  *          result in the same tick value.
+ *          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,
@@ -416,11 +425,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))
@@ -432,11 +445,11 @@ static int get_gpmc_timing_reg(
 
 		time_ns_min = gpmc_clk_ticks_to_ns(l ? l - 1 : 0, cs, cd);
 		time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
-		pr_info("gpmc,%s = <%u> /* (%u ns - %u ns]; %i ticks */\n",
-			name, time_ns, time_ns_min, time_ns, l);
+		pr_info("gpmc,%s = <%u> /* (%u ns - %u ns]; %i ticks %s*/\n",
+			name, time_ns, time_ns_min, 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;
@@ -446,15 +459,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_MAX(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)
 {
@@ -478,11 +495,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_MAX(GPMC_CS_CONFIG1, 23, 24, 4, GPMC_CONFIG1_ATTACHEDDEVICEPAGELENGTH_MAX, "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");
@@ -522,8 +539,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_WAITMONITORINGTIME_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");
@@ -540,13 +557,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;
@@ -559,9 +578,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;
 	}
@@ -580,13 +602,13 @@ 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(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
@@ -711,8 +733,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_WAITMONITORINGTIME_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
 	pr_info("GPMC CS%d CLK period is %lu ns (div %d)\n",
-- 
2.3.0

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

[Sergei Shtylyov]

Hello.

On 02/26/2015 05:45 PM, Robert ABEL wrote:

> DTS output was formatted to require additional work when copy-pasting into DTS.
> Nano-second timings were replaced with interval of values that produce the same
> number of clock ticks.

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

> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index dbb6753..9340e7a 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -337,32 +337,49 @@ 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

    Documentation/kernel-doc-nano-HOWTO.txt requires colons after the 
parameter names, doesn't it?

> + * @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 ns -- y ns]; x ticks *&zwj;/
> + *          Where (x ns -- y ns] is the half-open interval from x ns to y ns that
> + *          result in the same tick value.
> + * @noval   Parameter values equal to 0 are not printed.
> + * @shift   Parameter value left shifts @shift, which is then printed instead of value.
> + *

    You should also describe the meaning of the function's result in a 
"Return:" section.

> + */
>   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;

    BIT(nr_bits) - 1, perhaps?

WBR, Sergei

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

[Roger Quadros]

Hi Robert,

On 26/02/15 16:45, Robert ABEL wrote:
> This patch adds support for spawning buses as children of the GPMC.
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  drivers/memory/omap-gpmc.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 5cabac8..74a8c52 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,8 +1801,20 @@ 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))
> -		return 0;
> +
> +	/* create platform device, NULL on error or when disabled */
> +	if (!of_platform_device_create(child, NULL, &pdev->dev))
> +		goto err_child_fail;
> +
> +	/* is child a common bus? */
> +	if (of_match_node(of_default_bus_match_table, child))
> +		/* create children and other common bus children */
> +		if (of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev))
> +			goto err_child_fail;

this would print "failed to create gpmc child" but we have already created
the gpmc child in the first of_platform_device_create() call.
A more appropriate message would be "failed to populate all children of child->name"

Also do you want to return failure?
it will result in of_node_put() of the child and another print message
about "probing gpmc child %s failed" in gpmc_probe_dt().

IMO if the GPMC node's child was created fine then we shouldn't return error.

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

cheers,
-roger

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

[Roger Quadros]

Hi Robert,

On 26/02/15 16:45, Robert ABEL wrote:
> This patch adds support for spawning buses as children of the GPMC.
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  drivers/memory/omap-gpmc.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 5cabac8..74a8c52 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,8 +1801,20 @@ 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))
> -		return 0;
> +
> +	/* create platform device, NULL on error or when disabled */
> +	if (!of_platform_device_create(child, NULL, &pdev->dev))
> +		goto err_child_fail;
> +
> +	/* is child a common bus? */
> +	if (of_match_node(of_default_bus_match_table, child))
> +		/* create children and other common bus children */
> +		if (of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev))
> +			goto err_child_fail;

this would print "failed to create gpmc child" but we have already created
the gpmc child in the first of_platform_device_create() call.
A more appropriate message would be "failed to populate all children of child->name"

Also do you want to return failure?
it will result in of_node_put() of the child and another print message
about "probing gpmc child %s failed" in gpmc_probe_dt().

IMO if the GPMC node's child was created fine then we shouldn't return error.

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

cheers,
-roger

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

[Roger Quadros]

Robert,

On 26/02/15 16:45, Robert ABEL wrote:
> DTS output was formatted to require additional work when copy-pasting into DTS.
> Nano-second timings were replaced with interval of values that produce the same
> number of clock ticks.
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  drivers/memory/omap-gpmc.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index dbb6753..9340e7a 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -337,32 +337,49 @@ 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 ns -- y ns]; x ticks *&zwj;/
> + *          Where (x ns -- y ns] is the half-open interval from x ns to y ns that
> + *          result in the same tick value.
> + * @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;
> +		unsigned int time_ns_min;
>  
>  		time_ns_min = gpmc_ticks_to_ns(l ? l - 1 : 0);

should be
		time_ns_min = l ? gpmc_ticks_to_ns(l - 1) + 1 : 0;

		+ 1ns since we don't want to fall into the previous tick bracket.
		for l == 0 we have t_min as 0. no need to pass it through gpmc_ticks_to_ns() or add 1 ns.

>  		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> /* (%u ns - %u ns]; %i ticks */\n",
> +			name, time_ns, time_ns_min, time_ns, l);
>  	} else {
> +		/* raw format */
>  		pr_info("gpmc,%s = <%u>\n", name, l);
>  	}
>  
> 
cheers,
-roger

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

[Roger Quadros]

Robert,

On 26/02/15 16:45, Robert ABEL wrote:
> DTS output was formatted to require additional work when copy-pasting into DTS.
> Nano-second timings were replaced with interval of values that produce the same
> number of clock ticks.
> 
> Signed-off-by: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>
> ---
>  drivers/memory/omap-gpmc.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index dbb6753..9340e7a 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -337,32 +337,49 @@ 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 ns -- y ns]; x ticks *&zwj;/
> + *          Where (x ns -- y ns] is the half-open interval from x ns to y ns that
> + *          result in the same tick value.
> + * @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;
> +		unsigned int time_ns_min;
>  
>  		time_ns_min = gpmc_ticks_to_ns(l ? l - 1 : 0);

should be
		time_ns_min = l ? gpmc_ticks_to_ns(l - 1) + 1 : 0;

		+ 1ns since we don't want to fall into the previous tick bracket.
		for l == 0 we have t_min as 0. no need to pass it through gpmc_ticks_to_ns() or add 1 ns.

>  		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> /* (%u ns - %u ns]; %i ticks */\n",
> +			name, time_ns, time_ns_min, time_ns, l);
>  	} else {
> +		/* raw format */
>  		pr_info("gpmc,%s = <%u>\n", name, l);
>  	}
>  
> 
cheers,
-roger

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

[Robert Abel]

Hi Roger,

On Fri, Feb 27, 2015 at 11:43 AM, Roger Quadros <rogerq@ti.com> wrote:
>>               time_ns_min = gpmc_ticks_to_ns(l ? l - 1 : 0);
>
> should be
>                 time_ns_min = l ? gpmc_ticks_to_ns(l - 1) + 1 : 0;
That's a micro-optimization.
>
>                 + 1ns since we don't want to fall into the previous tick bracket.
>                 for l == 0 we have t_min as 0. no need to pass it through gpmc_ticks_to_ns() or add 1 ns.
That's why the invervals are half-open. I can make them closed, no problem.

Regards,

Robert

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

[Robert Abel]

On Thu, Feb 26, 2015 at 4:06 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
>    Documentation/kernel-doc-nano-HOWTO.txt requires colons after the
> parameter names, doesn't it?

Jesus Christ, you guys are killing me...
I've already spent way more time on this patch series than I intended
to anyway...

>> +       mask = (1 << nr_bits) - 1;
>
>
>    BIT(nr_bits) - 1, perhaps?

Not happening... BIT macro obscures what's actually going on.

Regards,

Robert

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

[Sergei Shtylyov]

Hello.

On 2/27/2015 5:17 PM, Robert Abel wrote:

>>     Documentation/kernel-doc-nano-HOWTO.txt requires colons after the
>> parameter names, doesn't it?

> Jesus Christ, you guys are killing me...
> I've already spent way more time on this patch series than I intended
> to anyway...

    That's what you get when pushing your stuff upstream. You're not alone 
here. :-)

>>> +       mask = (1 << nr_bits) - 1;

>>     BIT(nr_bits) - 1, perhaps?

> Not happening... BIT macro obscures what's actually going on.

    I did search for a macro that does ((1 << n) - 1) but didn't found it, 
unfortunately...

> Regards,
> Robert

WBR, Sergei

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

[Robert Abel]

Hi Roger,

On Fri, Feb 27, 2015 at 11:24 AM, Roger Quadros <rogerq@ti.com> wrote:
>> +     /* is child a common bus? */
>> +     if (of_match_node(of_default_bus_match_table, child))
>> +             /* create children and other common bus children */
>> +             if (of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev))
>> +                     goto err_child_fail;
>
> this would print "failed to create gpmc child" but we have already created
> the gpmc child in the first of_platform_device_create() call.
> A more appropriate message would be "failed to populate all children of child->name"
>
> Also do you want to return failure?
> it will result in of_node_put() of the child and another print message
> about "probing gpmc child %s failed" in gpmc_probe_dt().
>
> IMO if the GPMC node's child was created fine then we shouldn't return error.

As of_platform_populate _always_ return 0 no matter what, the only way
to reach that message is if probing the child failed.
As I cannot see into the future when of_platform_populate might
actually be changed to return meaningful codes, we shouldn't try to
foresee what the actual problem might be today either. This is a
battle for another day.

Regards,

Robert

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

[Tony Lindgren]

* Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> [150227 06:27]:
> Hello.
> 
> On 2/27/2015 5:17 PM, Robert Abel wrote:
> 
> >>    Documentation/kernel-doc-nano-HOWTO.txt requires colons after the
> >>parameter names, doesn't it?
> 
> >Jesus Christ, you guys are killing me...
> >I've already spent way more time on this patch series than I intended
> >to anyway...
> 
>    That's what you get when pushing your stuff upstream. You're not alone
> here. :-)

And thanks for the effort. Best to fix up things for good, that will
save headaches later on for everybody. Hmm did you file all the forms
for a change request in the kernel?  Just kidding :)

Tony

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

[Roger Quadros]

Hi Robert,

On 27/02/15 17:08, Robert Abel wrote:
> Hi Roger,
> 
> On Fri, Feb 27, 2015 at 11:24 AM, Roger Quadros <rogerq@ti.com> wrote:
>>> +     /* is child a common bus? */
>>> +     if (of_match_node(of_default_bus_match_table, child))
>>> +             /* create children and other common bus children */
>>> +             if (of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev))
>>> +                     goto err_child_fail;
>>
>> this would print "failed to create gpmc child" but we have already created
>> the gpmc child in the first of_platform_device_create() call.
>> A more appropriate message would be "failed to populate all children of child->name"
>>
>> Also do you want to return failure?
>> it will result in of_node_put() of the child and another print message
>> about "probing gpmc child %s failed" in gpmc_probe_dt().
>>
>> IMO if the GPMC node's child was created fine then we shouldn't return error.
> 
> As of_platform_populate _always_ return 0 no matter what, the only way
> to reach that message is if probing the child failed.

GPMCs child is already probed. It is the child's child we are talking about
in of_platform_populate.

> As I cannot see into the future when of_platform_populate might
> actually be changed to return meaningful codes, we shouldn't try to
> foresee what the actual problem might be today either. This is a
> battle for another day.
> 

If that is the case then I'd rather not check for return value of of_platform_populate().
Failure in populating GPMC child's children is already out of scope of GPMC driver.

cheers,
-roger

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

[Roger Quadros]

Hi Robert,

On 27/02/15 17:08, Robert Abel wrote:
> Hi Roger,
> 
> On Fri, Feb 27, 2015 at 11:24 AM, Roger Quadros <rogerq@ti.com> wrote:
>>> +     /* is child a common bus? */
>>> +     if (of_match_node(of_default_bus_match_table, child))
>>> +             /* create children and other common bus children */
>>> +             if (of_platform_populate(child, of_default_bus_match_table, NULL, &pdev->dev))
>>> +                     goto err_child_fail;
>>
>> this would print "failed to create gpmc child" but we have already created
>> the gpmc child in the first of_platform_device_create() call.
>> A more appropriate message would be "failed to populate all children of child->name"
>>
>> Also do you want to return failure?
>> it will result in of_node_put() of the child and another print message
>> about "probing gpmc child %s failed" in gpmc_probe_dt().
>>
>> IMO if the GPMC node's child was created fine then we shouldn't return error.
> 
> As of_platform_populate _always_ return 0 no matter what, the only way
> to reach that message is if probing the child failed.

GPMCs child is already probed. It is the child's child we are talking about
in of_platform_populate.

> As I cannot see into the future when of_platform_populate might
> actually be changed to return meaningful codes, we shouldn't try to
> foresee what the actual problem might be today either. This is a
> battle for another day.
> 

If that is the case then I'd rather not check for return value of of_platform_populate().
Failure in populating GPMC child's children is already out of scope of GPMC driver.

cheers,
-roger

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

[Robert Abel]

Hi Roger,

On Tue, Mar 3, 2015 at 11:09 AM, Roger Quadros <rogerq@ti.com> wrote:
> If that is the case then I'd rather not check for return value of of_platform_populate().
> Failure in populating GPMC child's children is already out of scope of GPMC driver.

Well, I'd rather leave it in for now. If something *does* break in the
future, the user will at least get a message about it.

Regards,

Robert

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

[Roger Quadros]

Robert,

On 26/02/15 16:45, Robert ABEL wrote:
> These are the changes I proposed in three separate patchsets
> #([1], [2], [3]) 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 --> correct 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
> [3]: https://lkml.org/lkml/2015/2/24/609

I'm OK with this version.

Tony, after you ACK these I will queue them for v4.1.

cheers,
-roger

> 
> 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 +-
>  arch/arm/mach-omap2/usb-tusb6010.c |   4 +-
>  drivers/memory/Makefile            |   2 +
>  drivers/memory/omap-gpmc.c         | 313 +++++++++++++++++++++++++++++--------
>  include/linux/omap-gpmc.h          |   2 +-
>  6 files changed, 265 insertions(+), 77 deletions(-)
> 

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

[Roger Quadros]

Robert,

On 26/02/15 16:45, Robert ABEL wrote:
> These are the changes I proposed in three separate patchsets
> #([1], [2], [3]) 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 --> correct 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
> [3]: https://lkml.org/lkml/2015/2/24/609

I'm OK with this version.

Tony, after you ACK these I will queue them for v4.1.

cheers,
-roger

> 
> 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 +-
>  arch/arm/mach-omap2/usb-tusb6010.c |   4 +-
>  drivers/memory/Makefile            |   2 +
>  drivers/memory/omap-gpmc.c         | 313 +++++++++++++++++++++++++++++--------
>  include/linux/omap-gpmc.h          |   2 +-
>  6 files changed, 265 insertions(+), 77 deletions(-)
> 

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

[Roger Quadros]

Robert,

On 26/02/15 16:45, Robert ABEL wrote:
> These are the changes I proposed in three separate patchsets
> #([1], [2], [3]) 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 --> correct 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
> [3]: https://lkml.org/lkml/2015/2/24/609

I'm OK with this version.

Tony, after you ACK these I will queue them for v4.1.

cheers,
-roger

> 
> 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 +-
>  arch/arm/mach-omap2/usb-tusb6010.c |   4 +-
>  drivers/memory/Makefile            |   2 +
>  drivers/memory/omap-gpmc.c         | 313 +++++++++++++++++++++++++++++--------
>  include/linux/omap-gpmc.h          |   2 +-
>  6 files changed, 265 insertions(+), 77 deletions(-)
> 

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

[Roger Quadros]

Robert,

On 26/02/15 16:45, Robert ABEL wrote:
> These are the changes I proposed in three separate patchsets
> #([1], [2], [3]) 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 --> correct 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
> [3]: https://lkml.org/lkml/2015/2/24/609

I'm OK with this version.

Tony, after you ACK these I will queue them for v4.1.

cheers,
-roger

> 
> 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 +-
>  arch/arm/mach-omap2/usb-tusb6010.c |   4 +-
>  drivers/memory/Makefile            |   2 +
>  drivers/memory/omap-gpmc.c         | 313 +++++++++++++++++++++++++++++--------
>  include/linux/omap-gpmc.h          |   2 +-
>  6 files changed, 265 insertions(+), 77 deletions(-)
> 

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

[Robert Abel]

Hi Roger,

On Tue, Mar 3, 2015 at 1:55 PM, Roger Quadros <rogerq@ti.com> wrote:
> I'm OK with this version.
>
> Tony, after you ACK these I will queue them for v4.1.

Please use v4 of my patches. The DTS output has been changed and the
comments have their colon.

Regards,

Robert

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

[Tony Lindgren]

* Robert Abel <rabel@cit-ec.uni-bielefeld.de> [150303 05:03]:
> Hi Roger,
> 
> On Tue, Mar 3, 2015 at 1:55 PM, Roger Quadros <rogerq@ti.com> wrote:
> > I'm OK with this version.
> >
> > Tony, after you ACK these I will queue them for v4.1.
> 
> Please use v4 of my patches. The DTS output has been changed and the
> comments have their colon.

Acked them in v4 thread, looks good to me.

Tony