[PATCH v5 0/3] Prepare for GPMC driver conversion

[PATCH v5 0/3] Prepare for GPMC driver conversion

[Afzal Mohammed]

Hi,

Objective of this series is to make things easy for GPMC driver
conversion series by separating out more things from driver
conversion series.

This series,
1. Unifies NAND platform initialization functions
2. Prepares OneNAND platform code for gpmc driver migration
3. Handles additional timings in Kernel

This series is based on 3.5-rc1 & made on top of
[PATCH v2 00/10] Prepare for GPMC driver conversion (w.r.t MTD)
{http://www.mail-archive.com/linux-omap at vger.kernel.org/msg70096.html}

These changes has been tested with omap3evm & beagle board. Relevant
GPMC peripherals that got tested by this were NAND (beagle) and
OneNAND (using local patches for omap3evm)

Regards
Afzal

v5:
1. Use flags for sync_read/write, hv, vhf
v4:
1. Reorganize OneNAND set_sync/async functions in a better way
v3:
1. Refactor OneNAND set_sync/async functions to separate out timing
 and configurations
2. Handle bool type timings too
3. Swap patches 2 & 3 due to dependency of OneNAND change on  newly
 added bool type timings
v2:
1. Make use of timing api for setting clock activation time, and
 remove direct writing to register for clock activation. Peripherals
 making use of it were tusb6010 & onenand
2. Move ensuring that async mode in OneNAND has been setup from
 set_sync to setup function, improve commit message


Afzal Mohammed (3):
  ARM: OMAP2+: nand: unify init functions
  ARM: OMAP2+: gpmc: handle additional timings
  ARM: OMAP2+: onenand: prepare for gpmc driver migration

 arch/arm/mach-omap2/board-devkit8000.c     |    8 +-
 arch/arm/mach-omap2/board-flash.c          |   45 ++++---
 arch/arm/mach-omap2/board-flash.h          |    6 +-
 arch/arm/mach-omap2/board-igep0020.c       |    2 +-
 arch/arm/mach-omap2/board-ldp.c            |    4 +-
 arch/arm/mach-omap2/board-omap3beagle.c    |    8 +-
 arch/arm/mach-omap2/board-omap3touchbook.c |    8 +-
 arch/arm/mach-omap2/board-overo.c          |    7 +-
 arch/arm/mach-omap2/board-zoom.c           |    5 +-
 arch/arm/mach-omap2/common-board-devices.c |   46 -------
 arch/arm/mach-omap2/common-board-devices.h |    1 -
 arch/arm/mach-omap2/gpmc-onenand.c         |  200 ++++++++++++++++------------
 arch/arm/mach-omap2/gpmc.c                 |   45 +++++++
 arch/arm/mach-omap2/usb-tusb6010.c         |    3 +-
 arch/arm/plat-omap/include/plat/gpmc.h     |   19 +++
 15 files changed, 235 insertions(+), 172 deletions(-)

-- 
1.7.10.2

[PATCH v5 1/3] ARM: OMAP2+: nand: unify init functions

[Afzal Mohammed]

Helper function for updating nand platform data has been
added the capability to take timing structure arguement.
Usage of omap_nand_flash_init() has been replaced by modifed
one, omap_nand_flash_init was doing things similar to
board_nand_init except that NAND CS# were being acquired
based on bootloader setting. As CS# is hardwired for a given
board, acquiring gpmc CS# has been removed, and updated with
the value on board.

NAND CS# used in beagle board was found to be CS0.
Thomas Weber <thomas.weber.linux@googlemail.com> reported
that value of devkit8000 to be CS0. Overo board was found
to be using CS0 based on u-boot, while google grep says
omap3touchbook too has CS0.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
Reviewed-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/board-devkit8000.c     |    8 +++--
 arch/arm/mach-omap2/board-flash.c          |   45 ++++++++++++++-------------
 arch/arm/mach-omap2/board-flash.h          |    6 ++--
 arch/arm/mach-omap2/board-igep0020.c       |    2 +-
 arch/arm/mach-omap2/board-ldp.c            |    4 +--
 arch/arm/mach-omap2/board-omap3beagle.c    |    8 +++--
 arch/arm/mach-omap2/board-omap3touchbook.c |    8 +++--
 arch/arm/mach-omap2/board-overo.c          |    7 +++--
 arch/arm/mach-omap2/board-zoom.c           |    5 +--
 arch/arm/mach-omap2/common-board-devices.c |   46 ----------------------------
 arch/arm/mach-omap2/common-board-devices.h |    1 -
 11 files changed, 56 insertions(+), 84 deletions(-)

diff --git a/arch/arm/mach-omap2/board-devkit8000.c b/arch/arm/mach-omap2/board-devkit8000.c
index 6567c1c..6ee429a 100644
--- a/arch/arm/mach-omap2/board-devkit8000.c
+++ b/arch/arm/mach-omap2/board-devkit8000.c
@@ -59,8 +59,11 @@
 
 #include "mux.h"
 #include "hsmmc.h"
+#include "board-flash.h"
 #include "common-board-devices.h"
 
+#define	NAND_CS			0
+
 #define OMAP_DM9000_GPIO_IRQ	25
 #define OMAP3_DEVKIT_TS_GPIO	27
 
@@ -628,8 +631,9 @@ static void __init devkit8000_init(void)
 
 	usb_musb_init(NULL);
 	usbhs_init(&usbhs_bdata);
-	omap_nand_flash_init(NAND_BUSWIDTH_16, devkit8000_nand_partitions,
-			     ARRAY_SIZE(devkit8000_nand_partitions));
+	board_nand_init(devkit8000_nand_partitions,
+		ARRAY_SIZE(devkit8000_nand_partitions), NAND_CS,
+		NAND_BUSWIDTH_16, NULL);
 
 	/* Ensure SDRC pins are mux'd for self-refresh */
 	omap_mux_init_signal("sdrc_cke0", OMAP_PIN_OUTPUT);
diff --git a/arch/arm/mach-omap2/board-flash.c b/arch/arm/mach-omap2/board-flash.c
index 70a81f9..0ee820b 100644
--- a/arch/arm/mach-omap2/board-flash.c
+++ b/arch/arm/mach-omap2/board-flash.c
@@ -108,41 +108,41 @@ __init board_onenand_init(struct mtd_partition *nor_parts, u8 nr_parts, u8 cs)
 		defined(CONFIG_MTD_NAND_OMAP2_MODULE)
 
 /* Note that all values in this struct are in nanoseconds */
-static struct gpmc_timings nand_timings = {
+struct gpmc_timings nand_default_timings[1] = {
+	{
+		.sync_clk = 0,
 
-	.sync_clk = 0,
+		.cs_on = 0,
+		.cs_rd_off = 36,
+		.cs_wr_off = 36,
 
-	.cs_on = 0,
-	.cs_rd_off = 36,
-	.cs_wr_off = 36,
+		.adv_on = 6,
+		.adv_rd_off = 24,
+		.adv_wr_off = 36,
 
-	.adv_on = 6,
-	.adv_rd_off = 24,
-	.adv_wr_off = 36,
+		.we_off = 30,
+		.oe_off = 48,
 
-	.we_off = 30,
-	.oe_off = 48,
+		.access = 54,
+		.rd_cycle = 72,
+		.wr_cycle = 72,
 
-	.access = 54,
-	.rd_cycle = 72,
-	.wr_cycle = 72,
-
-	.wr_access = 30,
-	.wr_data_mux_bus = 0,
+		.wr_access = 30,
+		.wr_data_mux_bus = 0,
+	},
 };
 
-static struct omap_nand_platform_data board_nand_data = {
-	.gpmc_t		= &nand_timings,
-};
+static struct omap_nand_platform_data board_nand_data;
 
 void
-__init board_nand_init(struct mtd_partition *nand_parts,
-			u8 nr_parts, u8 cs, int nand_type)
+__init board_nand_init(struct mtd_partition *nand_parts, u8 nr_parts, u8 cs,
+				int nand_type, struct gpmc_timings *gpmc_t)
 {
 	board_nand_data.cs		= cs;
 	board_nand_data.parts		= nand_parts;
 	board_nand_data.nr_parts	= nr_parts;
 	board_nand_data.devsize		= nand_type;
+	board_nand_data.gpmc_t		= gpmc_t;
 
 	board_nand_data.ecc_opt = OMAP_ECC_HAMMING_CODE_DEFAULT;
 	board_nand_data.gpmc_irq = OMAP_GPMC_IRQ_BASE + cs;
@@ -243,5 +243,6 @@ void __init board_flash_init(struct flash_partitions partition_info[],
 		pr_err("NAND: Unable to find configuration in GPMC\n");
 	else
 		board_nand_init(partition_info[2].parts,
-			partition_info[2].nr_parts, nandcs, nand_type);
+			partition_info[2].nr_parts, nandcs,
+			nand_type, nand_default_timings);
 }
diff --git a/arch/arm/mach-omap2/board-flash.h b/arch/arm/mach-omap2/board-flash.h
index c44b70d..a3aa5fc 100644
--- a/arch/arm/mach-omap2/board-flash.h
+++ b/arch/arm/mach-omap2/board-flash.h
@@ -40,12 +40,14 @@ static inline void board_flash_init(struct flash_partitions part[],
 #if defined(CONFIG_MTD_NAND_OMAP2) || \
 		defined(CONFIG_MTD_NAND_OMAP2_MODULE)
 extern void board_nand_init(struct mtd_partition *nand_parts,
-					u8 nr_parts, u8 cs, int nand_type);
+		u8 nr_parts, u8 cs, int nand_type, struct gpmc_timings *gpmc_t);
+extern struct gpmc_timings nand_default_timings[];
 #else
 static inline void board_nand_init(struct mtd_partition *nand_parts,
-					u8 nr_parts, u8 cs, int nand_type)
+		u8 nr_parts, u8 cs, int nand_type, struct gpmc_timings *gpmc_t)
 {
 }
+#define	nand_default_timings	NULL
 #endif
 
 #if defined(CONFIG_MTD_ONENAND_OMAP2) || \
diff --git a/arch/arm/mach-omap2/board-igep0020.c b/arch/arm/mach-omap2/board-igep0020.c
index 7491529..40d2d5d 100644
--- a/arch/arm/mach-omap2/board-igep0020.c
+++ b/arch/arm/mach-omap2/board-igep0020.c
@@ -175,7 +175,7 @@ static void __init igep_flash_init(void)
 		pr_info("IGEP: initializing NAND memory device\n");
 		board_nand_init(igep_flash_partitions,
 				ARRAY_SIZE(igep_flash_partitions),
-				0, NAND_BUSWIDTH_16);
+				0, NAND_BUSWIDTH_16, nand_default_timings);
 	} else if (mux == IGEP_SYSBOOT_ONENAND) {
 		pr_info("IGEP: initializing OneNAND memory device\n");
 		board_onenand_init(igep_flash_partitions,
diff --git a/arch/arm/mach-omap2/board-ldp.c b/arch/arm/mach-omap2/board-ldp.c
index ef9e829..5d8fa99 100644
--- a/arch/arm/mach-omap2/board-ldp.c
+++ b/arch/arm/mach-omap2/board-ldp.c
@@ -427,8 +427,8 @@ static void __init omap_ldp_init(void)
 	omap_serial_init();
 	omap_sdrc_init(NULL, NULL);
 	usb_musb_init(NULL);
-	board_nand_init(ldp_nand_partitions,
-		ARRAY_SIZE(ldp_nand_partitions), ZOOM_NAND_CS, 0);
+	board_nand_init(ldp_nand_partitions, ARRAY_SIZE(ldp_nand_partitions),
+				ZOOM_NAND_CS, 0, nand_default_timings);
 
 	omap_hsmmc_init(mmc);
 	ldp_display_init();
diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index 79c6909..5aa8f28 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -51,8 +51,11 @@
 #include "mux.h"
 #include "hsmmc.h"
 #include "pm.h"
+#include "board-flash.h"
 #include "common-board-devices.h"
 
+#define	NAND_CS	0
+
 /*
  * OMAP3 Beagle revision
  * Run time detection of Beagle revision is done by reading GPIO.
@@ -521,8 +524,9 @@ static void __init omap3_beagle_init(void)
 
 	usb_musb_init(NULL);
 	usbhs_init(&usbhs_bdata);
-	omap_nand_flash_init(NAND_BUSWIDTH_16, omap3beagle_nand_partitions,
-			     ARRAY_SIZE(omap3beagle_nand_partitions));
+	board_nand_init(omap3beagle_nand_partitions,
+		ARRAY_SIZE(omap3beagle_nand_partitions), NAND_CS,
+		NAND_BUSWIDTH_16, NULL);
 
 	/* Ensure msecure is mux'd to be able to set the RTC. */
 	omap_mux_init_signal("sys_drm_msecure", OMAP_PIN_OFF_OUTPUT_HIGH);
diff --git a/arch/arm/mach-omap2/board-omap3touchbook.c b/arch/arm/mach-omap2/board-omap3touchbook.c
index 485d14d..75e07f6 100644
--- a/arch/arm/mach-omap2/board-omap3touchbook.c
+++ b/arch/arm/mach-omap2/board-omap3touchbook.c
@@ -52,6 +52,7 @@
 
 #include "mux.h"
 #include "hsmmc.h"
+#include "board-flash.h"
 #include "common-board-devices.h"
 
 #include <asm/setup.h>
@@ -61,6 +62,8 @@
 #define TB_BL_PWM_TIMER		9
 #define TB_KILL_POWER_GPIO	168
 
+#define	NAND_CS			0
+
 static unsigned long touchbook_revision;
 
 static struct mtd_partition omap3touchbook_nand_partitions[] = {
@@ -370,8 +373,9 @@ static void __init omap3_touchbook_init(void)
 	omap_ads7846_init(4, OMAP3_TS_GPIO, 310, &ads7846_pdata);
 	usb_musb_init(NULL);
 	usbhs_init(&usbhs_bdata);
-	omap_nand_flash_init(NAND_BUSWIDTH_16, omap3touchbook_nand_partitions,
-			     ARRAY_SIZE(omap3touchbook_nand_partitions));
+	board_nand_init(omap3touchbook_nand_partitions,
+		ARRAY_SIZE(omap3touchbook_nand_partitions), NAND_CS,
+		NAND_BUSWIDTH_16, NULL);
 
 	/* Ensure SDRC pins are mux'd for self-refresh */
 	omap_mux_init_signal("sdrc_cke0", OMAP_PIN_OUTPUT);
diff --git a/arch/arm/mach-omap2/board-overo.c b/arch/arm/mach-omap2/board-overo.c
index 8fa2fc3..39f2994 100644
--- a/arch/arm/mach-omap2/board-overo.c
+++ b/arch/arm/mach-omap2/board-overo.c
@@ -57,8 +57,11 @@
 #include "mux.h"
 #include "sdram-micron-mt46h32m32lf-6.h"
 #include "hsmmc.h"
+#include "board-flash.h"
 #include "common-board-devices.h"
 
+#define	NAND_CS			0
+
 #define OVERO_GPIO_BT_XGATE	15
 #define OVERO_GPIO_W2W_NRESET	16
 #define OVERO_GPIO_PENDOWN	114
@@ -500,8 +503,8 @@ static void __init overo_init(void)
 	omap_serial_init();
 	omap_sdrc_init(mt46h32m32lf6_sdrc_params,
 				  mt46h32m32lf6_sdrc_params);
-	omap_nand_flash_init(0, overo_nand_partitions,
-			     ARRAY_SIZE(overo_nand_partitions));
+	board_nand_init(overo_nand_partitions,
+		ARRAY_SIZE(overo_nand_partitions), NAND_CS, 0, NULL);
 	usb_musb_init(NULL);
 	usbhs_init(&usbhs_bdata);
 	overo_spi_init();
diff --git a/arch/arm/mach-omap2/board-zoom.c b/arch/arm/mach-omap2/board-zoom.c
index 4e7e561..a04dd85 100644
--- a/arch/arm/mach-omap2/board-zoom.c
+++ b/arch/arm/mach-omap2/board-zoom.c
@@ -114,8 +114,9 @@ static void __init omap_zoom_init(void)
 		usbhs_init(&usbhs_bdata);
 	}
 
-	board_nand_init(zoom_nand_partitions, ARRAY_SIZE(zoom_nand_partitions),
-						ZOOM_NAND_CS, NAND_BUSWIDTH_16);
+	board_nand_init(zoom_nand_partitions,
+			ARRAY_SIZE(zoom_nand_partitions), ZOOM_NAND_CS,
+			NAND_BUSWIDTH_16, nand_default_timings);
 	zoom_debugboard_init();
 	zoom_peripherals_init();
 
diff --git a/arch/arm/mach-omap2/common-board-devices.c b/arch/arm/mach-omap2/common-board-devices.c
index 1706ebc..0fd35a4 100644
--- a/arch/arm/mach-omap2/common-board-devices.c
+++ b/arch/arm/mach-omap2/common-board-devices.c
@@ -93,49 +93,3 @@ void __init omap_ads7846_init(int bus_num, int gpio_pendown, int gpio_debounce,
 {
 }
 #endif
-
-#if defined(CONFIG_MTD_NAND_OMAP2) || defined(CONFIG_MTD_NAND_OMAP2_MODULE)
-static struct omap_nand_platform_data nand_data;
-
-void __init omap_nand_flash_init(int options, struct mtd_partition *parts,
-				 int nr_parts)
-{
-	u8 cs = 0;
-	u8 nandcs = GPMC_CS_NUM + 1;
-
-	/* find out the chip-select on which NAND exists */
-	while (cs < GPMC_CS_NUM) {
-		u32 ret = 0;
-		ret = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
-
-		if ((ret & 0xC00) == 0x800) {
-			printk(KERN_INFO "Found NAND on CS%d\n", cs);
-			if (nandcs > GPMC_CS_NUM)
-				nandcs = cs;
-		}
-		cs++;
-	}
-
-	if (nandcs > GPMC_CS_NUM) {
-		printk(KERN_INFO "NAND: Unable to find configuration "
-				 "in GPMC\n ");
-		return;
-	}
-
-	if (nandcs < GPMC_CS_NUM) {
-		nand_data.cs = nandcs;
-		nand_data.parts = parts;
-		nand_data.nr_parts = nr_parts;
-		nand_data.devsize = options;
-
-		printk(KERN_INFO "Registering NAND on CS%d\n", nandcs);
-		if (gpmc_nand_init(&nand_data) < 0)
-			printk(KERN_ERR "Unable to register NAND device\n");
-	}
-}
-#else
-void __init omap_nand_flash_init(int options, struct mtd_partition *parts,
-				 int nr_parts)
-{
-}
-#endif
diff --git a/arch/arm/mach-omap2/common-board-devices.h b/arch/arm/mach-omap2/common-board-devices.h
index a0b4a428..72bb41b 100644
--- a/arch/arm/mach-omap2/common-board-devices.h
+++ b/arch/arm/mach-omap2/common-board-devices.h
@@ -10,6 +10,5 @@ struct ads7846_platform_data;
 
 void omap_ads7846_init(int bus_num, int gpio_pendown, int gpio_debounce,
 		       struct ads7846_platform_data *board_pdata);
-void omap_nand_flash_init(int opts, struct mtd_partition *parts, int n_parts);
 
 #endif /* __OMAP_COMMON_BOARD_DEVICES__ */
-- 
1.7.10.2

[PATCH v5 2/3] ARM: OMAP2+: gpmc: handle additional timings

[Afzal Mohammed]

Configure busturnaround, cycle2cycledelay, waitmonitoringtime,
clkactivationtime in gpmc_cs_set_timings(). This is done so
that boards can configure these parameters of gpmc in Kernel
instead of relying on bootloader. Also configure bool type
timings like extradelay.

This needed change to two existing users that were configuring
clk activation time by directly writing to registers. Thanks to
Tony for making me aware of the issue & being kind enough to
test this change.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---

v3:
Handle bool type timings too
v2:
Make use of timing api for setting clock activation time, and
 remove direct writing to register for clock activation. Peripherals
 making use of it were tusb6010 & onenand

 arch/arm/mach-omap2/gpmc-onenand.c     |    3 ++-
 arch/arm/mach-omap2/gpmc.c             |   45 ++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/usb-tusb6010.c     |    3 ++-
 arch/arm/plat-omap/include/plat/gpmc.h |   19 ++++++++++++++
 4 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index 71d7c07..8863e0a 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -329,6 +329,8 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
 	t.rd_cycle = gpmc_ticks_to_ns(fclk_offset + (latency + 1) * div +
 		     ticks_cez);
 
+	t.clk_activation = fclk_offset_ns;
+
 	/* Write */
 	if (sync_write) {
 		t.adv_wr_off = t.adv_rd_off;
@@ -362,7 +364,6 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
 			  (sync_read ? GPMC_CONFIG1_READTYPE_SYNC : 0) |
 			  (sync_write ? GPMC_CONFIG1_WRITEMULTIPLE_SUPP : 0) |
 			  (sync_write ? GPMC_CONFIG1_WRITETYPE_SYNC : 0) |
-			  GPMC_CONFIG1_CLKACTIVATIONTIME(fclk_offset) |
 			  GPMC_CONFIG1_PAGE_LEN(2) |
 			  (cpu_is_omap34xx() ? 0 :
 				(GPMC_CONFIG1_WAIT_READ_MON |
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 578fd4c..8b0978f 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -64,6 +64,13 @@
 #define GPMC_ECC_CTRL_ECCREG8		0x008
 #define GPMC_ECC_CTRL_ECCREG9		0x009
 
+#define	GPMC_CONFIG2_CSEXTRADELAY		BIT(7)
+#define	GPMC_CONFIG3_ADVEXTRADELAY		BIT(7)
+#define	GPMC_CONFIG4_OEEXTRADELAY		BIT(7)
+#define	GPMC_CONFIG4_WEEXTRADELAY		BIT(23)
+#define	GPMC_CONFIG6_CYCLE2CYCLEDIFFCSEN	BIT(6)
+#define	GPMC_CONFIG6_CYCLE2CYCLESAMECSEN	BIT(7)
+
 #define GPMC_CS0_OFFSET		0x60
 #define GPMC_CS_SIZE		0x30
 
@@ -220,6 +227,36 @@ unsigned int gpmc_round_ns_to_ticks(unsigned int time_ns)
 	return ticks * gpmc_get_fclk_period() / 1000;
 }
 
+static inline void gpmc_cs_modify_reg(int cs, int reg, u32 mask, bool value)
+{
+	u32 l;
+
+	l = gpmc_cs_read_reg(cs, reg);
+	if (value)
+		l |= mask;
+	else
+		l &= ~mask;
+	gpmc_cs_write_reg(cs, reg, l);
+}
+
+static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
+{
+	gpmc_cs_modify_reg(cs, GPMC_CS_CONFIG1,
+		GPMC_CONFIG1_TIME_PARA_GRAN, p->time_para_granularity);
+	gpmc_cs_modify_reg(cs, GPMC_CS_CONFIG2,
+		GPMC_CONFIG2_CSEXTRADELAY, p->cs_extra_delay);
+	gpmc_cs_modify_reg(cs, GPMC_CS_CONFIG3,
+		GPMC_CONFIG3_ADVEXTRADELAY, p->adv_extra_delay);
+	gpmc_cs_modify_reg(cs, GPMC_CS_CONFIG4,
+		GPMC_CONFIG4_OEEXTRADELAY, p->oe_extra_delay);
+	gpmc_cs_modify_reg(cs, GPMC_CS_CONFIG4,
+		GPMC_CONFIG4_OEEXTRADELAY, p->we_extra_delay);
+	gpmc_cs_modify_reg(cs, GPMC_CS_CONFIG6,
+		GPMC_CONFIG6_CYCLE2CYCLESAMECSEN, p->cycle2cyclesamecsen);
+	gpmc_cs_modify_reg(cs, GPMC_CS_CONFIG6,
+		GPMC_CONFIG6_CYCLE2CYCLEDIFFCSEN, p->cycle2cyclediffcsen);
+}
+
 #ifdef DEBUG
 static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 			       int time, const char *name)
@@ -313,6 +350,12 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
 
 	GPMC_SET_ONE(GPMC_CS_CONFIG5, 24, 27, page_burst_access);
 
+	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 (cpu_is_omap34xx()) {
 		GPMC_SET_ONE(GPMC_CS_CONFIG6, 16, 19, wr_data_mux_bus);
 		GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
@@ -332,6 +375,8 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
 		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
 	}
 
+	gpmc_cs_bool_timings(cs, &t->bool_timings);
+
 	return 0;
 }
 
diff --git a/arch/arm/mach-omap2/usb-tusb6010.c b/arch/arm/mach-omap2/usb-tusb6010.c
index db84a46..5c98755 100644
--- a/arch/arm/mach-omap2/usb-tusb6010.c
+++ b/arch/arm/mach-omap2/usb-tusb6010.c
@@ -174,6 +174,8 @@ static int tusb_set_sync_mode(unsigned sysclk_ps, unsigned fclk_ps)
 	tmp = t.cs_wr_off * 1000 + 7000 /* t_scsn_rdy_z */;
 	t.wr_cycle = next_clk(t.cs_wr_off, tmp, fclk_ps);
 
+	t.clk_activation = gpmc_ticks_to_ns(1);
+
 	return gpmc_cs_set_timings(sync_cs, &t);
 }
 
@@ -283,7 +285,6 @@ tusb6010_setup_interface(struct musb_hdrc_platform_data *data,
 			| GPMC_CONFIG1_READTYPE_SYNC
 			| GPMC_CONFIG1_WRITEMULTIPLE_SUPP
 			| GPMC_CONFIG1_WRITETYPE_SYNC
-			| GPMC_CONFIG1_CLKACTIVATIONTIME(1)
 			| GPMC_CONFIG1_PAGE_LEN(2)
 			| GPMC_CONFIG1_WAIT_READ_MON
 			| GPMC_CONFIG1_WAIT_WRITE_MON
diff --git a/arch/arm/plat-omap/include/plat/gpmc.h b/arch/arm/plat-omap/include/plat/gpmc.h
index 2e6e259..b7c9ea6 100644
--- a/arch/arm/plat-omap/include/plat/gpmc.h
+++ b/arch/arm/plat-omap/include/plat/gpmc.h
@@ -96,6 +96,17 @@ enum omap_ecc {
 	OMAP_ECC_BCH8_CODE_HW, /* 8-bit BCH ecc code */
 };
 
+/* bool type time settings */
+struct gpmc_bool_timings {
+	bool cycle2cyclediffcsen;
+	bool cycle2cyclesamecsen;
+	bool we_extra_delay;
+	bool oe_extra_delay;
+	bool adv_extra_delay;
+	bool cs_extra_delay;
+	bool time_para_granularity;
+};
+
 /*
  * Note that all values in this struct are in nanoseconds except sync_clk
  * (which is in picoseconds), while the register values are in gpmc_fck cycles.
@@ -128,9 +139,17 @@ struct gpmc_timings {
 	u16 rd_cycle;		/* Total read cycle time */
 	u16 wr_cycle;		/* Total write cycle time */
 
+	u16 bus_turnaround;
+	u16 cycle2cycle_delay;
+
+	u16 wait_monitoring;
+	u16 clk_activation;
+
 	/* The following are only on OMAP3430 */
 	u16 wr_access;		/* WRACCESSTIME */
 	u16 wr_data_mux_bus;	/* WRDATAONADMUXBUS */
+
+	struct gpmc_bool_timings bool_timings;
 };
 
 struct gpmc_nand_regs {
-- 
1.7.10.2

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Afzal Mohammed]

Reorganize gpmc-onenand initialization so that changes
required for gpmc driver migration can be made smooth.

Ensuring sync read/write are disabled in onenand cannot
be expected to work properly unless GPMC is setup, this
has been removed.

Refactor set_async_mode & set_sync_mode functions to
separate out timing calculation & actual configuration
(GPMC & OneNAND side).

Thanks to Jon for his suggestions.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
Reviewed-by: Jon Hunter <jon-hunter@ti.com>
---

v5:
Use flags for sync_read/write, hv, vhf
v4:
Reorganize set_sync/async functions in a better way
v3:
Refactor set_sync/async functions to separate out timing and
 configurations
v2:
Move ensuring that async mode in OneNAND has been setup from
 set_sync to setup function, improve commit message

 arch/arm/mach-omap2/gpmc-onenand.c |  197 ++++++++++++++++++++----------------
 1 file changed, 111 insertions(+), 86 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index 8863e0a..c8a9487 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -15,6 +15,7 @@
 #include <linux/platform_device.h>
 #include <linux/mtd/onenand_regs.h>
 #include <linux/io.h>
+#include <linux/err.h>
 
 #include <asm/mach/flash.h>
 
@@ -25,6 +26,14 @@
 
 #define	ONENAND_IO_SIZE	SZ_128K
 
+#define	ONENAND_FLAG_SYNCREAD	(1 << 0)
+#define	ONENAND_FLAG_SYNCWRITE	(1 << 1)
+#define	ONENAND_FLAG_HF		(1 << 2)
+#define	ONENAND_FLAG_VHF	(1 << 3)
+
+static unsigned onenand_flags;
+static unsigned latency;
+
 static struct omap_onenand_platform_data *gpmc_onenand_data;
 
 static struct resource gpmc_onenand_resource = {
@@ -38,11 +47,9 @@ static struct platform_device gpmc_onenand_device = {
 	.resource	= &gpmc_onenand_resource,
 };
 
-static int omap2_onenand_set_async_mode(int cs, void __iomem *onenand_base)
+static struct gpmc_timings omap2_onenand_calc_async_timings(void)
 {
 	struct gpmc_timings t;
-	u32 reg;
-	int err;
 
 	const int t_cer = 15;
 	const int t_avdp = 12;
@@ -55,11 +62,6 @@ static int omap2_onenand_set_async_mode(int cs, void __iomem *onenand_base)
 	const int t_wpl = 40;
 	const int t_wph = 30;
 
-	/* Ensure sync read and sync write are disabled */
-	reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
-	reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
-	writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
-
 	memset(&t, 0, sizeof(t));
 	t.sync_clk = 0;
 	t.cs_on = 0;
@@ -86,25 +88,30 @@ static int omap2_onenand_set_async_mode(int cs, void __iomem *onenand_base)
 	t.cs_wr_off = t.we_off + gpmc_round_ns_to_ticks(t_wph);
 	t.wr_cycle  = t.cs_wr_off + gpmc_round_ns_to_ticks(t_cez);
 
+	return t;
+}
+
+static int gpmc_set_async_mode(int cs, struct gpmc_timings *t)
+{
 	/* Configure GPMC for asynchronous read */
 	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1,
 			  GPMC_CONFIG1_DEVICESIZE_16 |
 			  GPMC_CONFIG1_MUXADDDATA);
 
-	err = gpmc_cs_set_timings(cs, &t);
-	if (err)
-		return err;
+	return gpmc_cs_set_timings(cs, t);
+}
+
+static void omap2_onenand_set_async_mode(void __iomem *onenand_base)
+{
+	u32 reg;
 
 	/* Ensure sync read and sync write are disabled */
 	reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
 	reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
 	writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
-
-	return 0;
 }
 
-static void set_onenand_cfg(void __iomem *onenand_base, int latency,
-				int sync_read, int sync_write, int hf, int vhf)
+static void set_onenand_cfg(void __iomem *onenand_base)
 {
 	u32 reg;
 
@@ -112,19 +119,19 @@ static void set_onenand_cfg(void __iomem *onenand_base, int latency,
 	reg &= ~((0x7 << ONENAND_SYS_CFG1_BRL_SHIFT) | (0x7 << 9));
 	reg |=	(latency << ONENAND_SYS_CFG1_BRL_SHIFT) |
 		ONENAND_SYS_CFG1_BL_16;
-	if (sync_read)
+	if (onenand_flags & ONENAND_FLAG_SYNCREAD)
 		reg |= ONENAND_SYS_CFG1_SYNC_READ;
 	else
 		reg &= ~ONENAND_SYS_CFG1_SYNC_READ;
-	if (sync_write)
+	if (onenand_flags & ONENAND_FLAG_SYNCWRITE)
 		reg |= ONENAND_SYS_CFG1_SYNC_WRITE;
 	else
 		reg &= ~ONENAND_SYS_CFG1_SYNC_WRITE;
-	if (hf)
+	if (onenand_flags & ONENAND_FLAG_HF)
 		reg |= ONENAND_SYS_CFG1_HF;
 	else
 		reg &= ~ONENAND_SYS_CFG1_HF;
-	if (vhf)
+	if (onenand_flags & ONENAND_FLAG_VHF)
 		reg |= ONENAND_SYS_CFG1_VHF;
 	else
 		reg &= ~ONENAND_SYS_CFG1_VHF;
@@ -172,9 +179,9 @@ static int omap2_onenand_get_freq(struct omap_onenand_platform_data *cfg,
 	return freq;
 }
 
-static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
-					void __iomem *onenand_base,
-					int *freq_ptr)
+static struct gpmc_timings
+omap2_onenand_calc_sync_timings(struct omap_onenand_platform_data *cfg,
+						int freq, bool clk_dep)
 {
 	struct gpmc_timings t;
 	const int t_cer  = 15;
@@ -184,29 +191,14 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
 	const int t_wpl  = 40;
 	const int t_wph  = 30;
 	int min_gpmc_clk_period, t_ces, t_avds, t_avdh, t_ach, t_aavdh, t_rdyo;
-	int div, fclk_offset_ns, fclk_offset, gpmc_clk_ns, latency;
-	int first_time = 0, hf = 0, vhf = 0, sync_read = 0, sync_write = 0;
-	int err, ticks_cez;
-	int cs = cfg->cs, freq = *freq_ptr;
-	u32 reg;
-	bool clk_dep = false;
-
-	if (cfg->flags & ONENAND_SYNC_READ) {
-		sync_read = 1;
-	} else if (cfg->flags & ONENAND_SYNC_READWRITE) {
-		sync_read = 1;
-		sync_write = 1;
-	} else
-		return omap2_onenand_set_async_mode(cs, onenand_base);
+	int div, fclk_offset_ns, fclk_offset, gpmc_clk_ns;
+	int ticks_cez;
+	int cs = cfg->cs;
 
-	if (!freq) {
-		/* Very first call freq is not known */
-		err = omap2_onenand_set_async_mode(cs, onenand_base);
-		if (err)
-			return err;
-		freq = omap2_onenand_get_freq(cfg, onenand_base, &clk_dep);
-		first_time = 1;
-	}
+	if (cfg->flags & ONENAND_SYNC_READ)
+		onenand_flags = ONENAND_FLAG_SYNCREAD;
+	else if (cfg->flags & ONENAND_SYNC_READWRITE)
+		onenand_flags = ONENAND_FLAG_SYNCREAD | ONENAND_FLAG_SYNCWRITE;
 
 	switch (freq) {
 	case 104:
@@ -244,19 +236,23 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
 		t_ach   = 9;
 		t_aavdh = 7;
 		t_rdyo  = 15;
-		sync_write = 0;
+		onenand_flags &= ~ONENAND_FLAG_SYNCWRITE;
 		break;
 	}
 
 	div = gpmc_cs_calc_divider(cs, min_gpmc_clk_period);
 	gpmc_clk_ns = gpmc_ticks_to_ns(div);
 	if (gpmc_clk_ns < 15) /* >66Mhz */
-		hf = 1;
+		onenand_flags |= ONENAND_FLAG_HF;
+	else
+		onenand_flags &= ~ONENAND_FLAG_HF;
 	if (gpmc_clk_ns < 12) /* >83Mhz */
-		vhf = 1;
-	if (vhf)
+		onenand_flags |= ONENAND_FLAG_VHF;
+	else
+		onenand_flags &= ~ONENAND_FLAG_VHF;
+	if (onenand_flags & ONENAND_FLAG_VHF)
 		latency = 8;
-	else if (hf)
+	else if (onenand_flags & ONENAND_FLAG_HF)
 		latency = 6;
 	else if (gpmc_clk_ns >= 25) /* 40 MHz*/
 		latency = 3;
@@ -279,36 +275,16 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
 		}
 	}
 
-	if (first_time)
-		set_onenand_cfg(onenand_base, latency,
-					sync_read, sync_write, hf, vhf);
+	/* Set synchronous read timings */
+	memset(&t, 0, sizeof(t));
 
 	if (div == 1) {
-		reg = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG2);
-		reg |= (1 << 7);
-		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG2, reg);
-		reg = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG3);
-		reg |= (1 << 7);
-		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG3, reg);
-		reg = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG4);
-		reg |= (1 << 7);
-		reg |= (1 << 23);
-		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG4, reg);
-	} else {
-		reg = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG2);
-		reg &= ~(1 << 7);
-		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG2, reg);
-		reg = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG3);
-		reg &= ~(1 << 7);
-		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG3, reg);
-		reg = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG4);
-		reg &= ~(1 << 7);
-		reg &= ~(1 << 23);
-		gpmc_cs_write_reg(cs, GPMC_CS_CONFIG4, reg);
+		t.bool_timings.cs_extra_delay = true;
+		t.bool_timings.adv_extra_delay = true;
+		t.bool_timings.oe_extra_delay = true;
+		t.bool_timings.we_extra_delay = true;
 	}
 
-	/* Set synchronous read timings */
-	memset(&t, 0, sizeof(t));
 	t.sync_clk = min_gpmc_clk_period;
 	t.cs_on = 0;
 	t.adv_on = 0;
@@ -332,7 +308,7 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
 	t.clk_activation = fclk_offset_ns;
 
 	/* Write */
-	if (sync_write) {
+	if (onenand_flags & ONENAND_FLAG_SYNCWRITE) {
 		t.adv_wr_off = t.adv_rd_off;
 		t.we_on  = 0;
 		t.we_off = t.cs_rd_off;
@@ -357,6 +333,14 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
 		}
 	}
 
+	return t;
+}
+
+static int gpmc_set_sync_mode(int cs, struct gpmc_timings *t)
+{
+	unsigned sync_read = onenand_flags & ONENAND_FLAG_SYNCREAD;
+	unsigned sync_write = onenand_flags & ONENAND_FLAG_SYNCWRITE;
+
 	/* Configure GPMC for synchronous read */
 	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1,
 			  GPMC_CONFIG1_WRAPBURST_SUPP |
@@ -372,11 +356,45 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
 			  GPMC_CONFIG1_DEVICETYPE_NOR |
 			  GPMC_CONFIG1_MUXADDDATA);
 
-	err = gpmc_cs_set_timings(cs, &t);
-	if (err)
-		return err;
+	return gpmc_cs_set_timings(cs, t);
+}
+
+static int omap2_onenand_setup_async(void __iomem *onenand_base)
+{
+	struct gpmc_timings t;
+	int ret;
+
+	t = omap2_onenand_calc_async_timings();
+
+	ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);
+	if (IS_ERR_VALUE(ret))
+		return ret;
+
+	omap2_onenand_set_async_mode(onenand_base);
+
+	return 0;
+}
+
+static int omap2_onenand_setup_sync(void __iomem *onenand_base, int *freq_ptr)
+{
+	int ret, freq = *freq_ptr;
+	struct gpmc_timings t;
+	bool clk_dep = false;
+
+	if (!freq) {
+		/* Very first call freq is not known */
+		freq = omap2_onenand_get_freq(gpmc_onenand_data,
+						onenand_base, &clk_dep);
+		set_onenand_cfg(onenand_base);
+	}
+
+	t = omap2_onenand_calc_sync_timings(gpmc_onenand_data, freq, clk_dep);
 
-	set_onenand_cfg(onenand_base, latency, sync_read, sync_write, hf, vhf);
+	ret = gpmc_set_sync_mode(gpmc_onenand_data->cs, &t);
+	if (IS_ERR_VALUE(ret))
+		return ret;
+
+	set_onenand_cfg(onenand_base);
 
 	*freq_ptr = freq;
 
@@ -386,15 +404,22 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
 static int gpmc_onenand_setup(void __iomem *onenand_base, int *freq_ptr)
 {
 	struct device *dev = &gpmc_onenand_device.dev;
+	unsigned l = ONENAND_SYNC_READ | ONENAND_SYNC_READWRITE;
+	int ret;
 
-	/* Set sync timings in GPMC */
-	if (omap2_onenand_set_sync_mode(gpmc_onenand_data, onenand_base,
-			freq_ptr) < 0) {
-		dev_err(dev, "Unable to set synchronous mode\n");
-		return -EINVAL;
+	ret = omap2_onenand_setup_async(onenand_base);
+	if (ret) {
+		dev_err(dev, "unable to set to async mode\n");
+		return ret;
 	}
 
-	return 0;
+	if (!(gpmc_onenand_data->flags & l))
+		return 0;
+
+	ret = omap2_onenand_setup_sync(onenand_base, freq_ptr);
+	if (ret)
+		dev_err(dev, "unable to set to sync mode\n");
+	return ret;
 }
 
 void __init gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
-- 
1.7.10.2

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Tony Lindgren]

* Afzal Mohammed <afzal@ti.com> [120626 23:53]:
> Reorganize gpmc-onenand initialization so that changes
> required for gpmc driver migration can be made smooth.
> 
> Ensuring sync read/write are disabled in onenand cannot
> be expected to work properly unless GPMC is setup, this
> has been removed.
> 
> Refactor set_async_mode & set_sync_mode functions to
> separate out timing calculation & actual configuration
> (GPMC & OneNAND side).

The last patch in this series causes onenand not to show
up on my n900. I believe the problem has been there earlier
too, but I just did not notice it.

Without the last patch in this series I get:

[    1.650207] OneNAND driver initializing
[    1.655487] omap2-onenand omap2-onenand: initializing on CS0, phys base 0x04000000, virtual base d0880000, freq 83 MHz
[    1.666931] OneNAND Manufacturer: Numonyx (0x20)
[    1.671905] Muxed OneNAND 256MB 1.8V 16-bit (0x40)
[    1.676971] OneNAND version = 0x0031
[    1.918762] Creating 6 MTD partitions on "omap2-onenand":

And with this patch applied I get:

[    1.634643] OneNAND driver initializing
[    1.640075] omap2-onenand omap2-onenand: initializing on CS0, phys base 0x04000000, virtual base d0880000, freq 54 MHz
[    1.651519] OneNAND Manufacturer: Unknown (0xa0)

Any ideas?

Tony

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony,

On Wed, Jun 27, 2012 at 20:28:45, Tony Lindgren wrote:

> The last patch in this series causes onenand not to show
> up on my n900. I believe the problem has been there earlier
> too, but I just did not notice it.

Sorry for the delayed response, could reach workplace a short
while ago only

Could the diff [1] be tried and check whether it resolves the issue,

Regards
Afzal

[1]
diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index c8a9487..bbae674 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -364,6 +364,8 @@ static int omap2_onenand_setup_async(void __iomem *onenand_base)
        struct gpmc_timings t;
        int ret;

+       omap2_onenand_set_async_mode(onenand_base);
+
        t = omap2_onenand_calc_async_timings();

        ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120628 02:36]:
> Hi Tony,
> 
> On Wed, Jun 27, 2012 at 20:28:45, Tony Lindgren wrote:
> 
> > The last patch in this series causes onenand not to show
> > up on my n900. I believe the problem has been there earlier
> > too, but I just did not notice it.
> 
> Sorry for the delayed response, could reach workplace a short
> while ago only
> 
> Could the diff [1] be tried and check whether it resolves the issue,
> 
> Regards
> Afzal
> 
> [1]
> diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> index c8a9487..bbae674 100644
> --- a/arch/arm/mach-omap2/gpmc-onenand.c
> +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> @@ -364,6 +364,8 @@ static int omap2_onenand_setup_async(void __iomem *onenand_base)
>         struct gpmc_timings t;
>         int ret;
> 
> +       omap2_onenand_set_async_mode(onenand_base);
> +
>         t = omap2_onenand_calc_async_timings();
> 
>         ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);

Yes that seems to do the trick, thanks! I can fold that into the
breaking patch when applying.

Tony

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony,

On Thu, Jun 28, 2012 at 18:02:07, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120628 02:36]:

> Yes that seems to do the trick, thanks! I can fold that into the
> breaking patch when applying.

Relieved, thanks

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony,

On Thu, Jun 28, 2012 at 18:02:07, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120628 02:36]:

> > diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> > index c8a9487..bbae674 100644
> > --- a/arch/arm/mach-omap2/gpmc-onenand.c
> > +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> > @@ -364,6 +364,8 @@ static int omap2_onenand_setup_async(void __iomem *onenand_base)
> >         struct gpmc_timings t;
> >         int ret;
> > 
> > +       omap2_onenand_set_async_mode(onenand_base);
> > +
> >         t = omap2_onenand_calc_async_timings();
> > 
> >         ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);
> 
> Yes that seems to do the trick, thanks! I can fold that into the
> breaking patch when applying.


Also following needs to be removed from commit message,

"Ensuring sync read/write are disabled in onenand cannot
be expected to work properly unless GPMC is setup, this
has been removed."

With the above diff we are in effect disabling sync mode in
onenand before GPMC has been setup in the Kernel

If you want, I can sent a new series too.

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony,

On Thu, Jun 28, 2012 at 18:14:30, Mohammed, Afzal wrote:
> On Thu, Jun 28, 2012 at 18:02:07, Tony Lindgren wrote:
> > * Mohammed, Afzal <afzal@ti.com> [120628 02:36]:

> > > diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> > > index c8a9487..bbae674 100644
> > > --- a/arch/arm/mach-omap2/gpmc-onenand.c
> > > +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> > > @@ -364,6 +364,8 @@ static int omap2_onenand_setup_async(void __iomem *onenand_base)
> > >         struct gpmc_timings t;
> > >         int ret;
> > > 
> > > +       omap2_onenand_set_async_mode(onenand_base);
> > > +
> > >         t = omap2_onenand_calc_async_timings();
> > > 
> > >         ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);
> > 
> > Yes that seems to do the trick, thanks! I can fold that into the
> > breaking patch when applying.
> 
> 
> Also following needs to be removed from commit message,
> 
> "Ensuring sync read/write are disabled in onenand cannot
> be expected to work properly unless GPMC is setup, this
> has been removed."
> 
> With the above diff we are in effect disabling sync mode in
> onenand before GPMC has been setup in the Kernel
> 
> If you want, I can sent a new series too.

Can you please take this series along with the above changes
folded in $subject patch. I & Jon concurred on this as a
solution in this same thread (along with preventing
hwmod reset, which is taken care in hwmod patch).

Please let me know whether you prefer to have a separate series
posted inclusive of the above changes.

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Jon Hunter]

Hi Tony, Afzal,

On 06/28/2012 07:32 AM, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120628 02:36]:
>> Hi Tony,
>>
>> On Wed, Jun 27, 2012 at 20:28:45, Tony Lindgren wrote:
>>
>>> The last patch in this series causes onenand not to show
>>> up on my n900. I believe the problem has been there earlier
>>> too, but I just did not notice it.
>>
>> Sorry for the delayed response, could reach workplace a short
>> while ago only
>>
>> Could the diff [1] be tried and check whether it resolves the issue,
>>
>> Regards
>> Afzal
>>
>> [1]
>> diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
>> index c8a9487..bbae674 100644
>> --- a/arch/arm/mach-omap2/gpmc-onenand.c
>> +++ b/arch/arm/mach-omap2/gpmc-onenand.c
>> @@ -364,6 +364,8 @@ static int omap2_onenand_setup_async(void __iomem *onenand_base)
>>         struct gpmc_timings t;
>>         int ret;
>>
>> +       omap2_onenand_set_async_mode(onenand_base);
>> +
>>         t = omap2_onenand_calc_async_timings();
>>
>>         ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);
> 
> Yes that seems to do the trick, thanks! I can fold that into the
> breaking patch when applying.

I am not sure what to make of this. Testing Afzal's this series along with the other
gpmc-prep series [1], onenand is working fine on my 3430sdp and I see ...

[    2.792510] OneNAND driver initializing
[    2.797576] omap2-onenand omap2-onenand: initializing on CS2, phys base 0x20000000, virtual base c88c0000, freq 0 MHz
[    2.808929] OneNAND Manufacturer: Samsung (0xec)
[    2.808990] Muxed OneNAND 256MB 1.8V 16-bit (0x40)
[    2.814208] OneNAND version = 0x002c

The above change seems to imply that Tony's n900 is dependent on the bootloader settings
and not those being set by the kernel. Ideally, we should not need to set the async mode
in the onenand before we set the onenand timings in the gpmc (per Afzal's changelog
comment). This appears backwards.

The other thing to note is that the 3430sdp has samsung onenand where as the n900 has
Numonyx. The gpmc-onenand.c only has one set of settings that it is using for all
devices. However, it would appear that at least the async settings are not working for
the Numonyx. Therefore, may be we need to get a dump of Tony's n900 settings and make
sure the right settings are being used for the appropriate board. 

These onenand settings are really killing us. I don't want us to have to spend alot
of time re-calculating this stuff but the way it has been written to begin with is not
driver friendly. I really wonder if we need to have some sort of callback for the 
onenand timings from the driver. It is ugly, but the alternative is that someone needs
to sit down and re-calculate all the timings again to get them into a driver friendly
format. Furthermore, it seems that onenand is no longer available from the likes of
samsung and numonyx (micron) so it is hard to justify re-calculating everything again.
I am not even sure if we have all the datasheets!

Let me know your thoughts.

Cheers
Jon

[1] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg70096.html

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Jon Hunter]

Hi Tony, Afzal,

On 06/28/2012 11:43 AM, Jon Hunter wrote:
> Hi Tony, Afzal,
> 
> On 06/28/2012 07:32 AM, Tony Lindgren wrote:
>> * Mohammed, Afzal <afzal@ti.com> [120628 02:36]:
>>> Hi Tony,
>>>
>>> On Wed, Jun 27, 2012 at 20:28:45, Tony Lindgren wrote:
>>>
>>>> The last patch in this series causes onenand not to show
>>>> up on my n900. I believe the problem has been there earlier
>>>> too, but I just did not notice it.
>>>
>>> Sorry for the delayed response, could reach workplace a short
>>> while ago only
>>>
>>> Could the diff [1] be tried and check whether it resolves the issue,
>>>
>>> Regards
>>> Afzal
>>>
>>> [1]
>>> diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
>>> index c8a9487..bbae674 100644
>>> --- a/arch/arm/mach-omap2/gpmc-onenand.c
>>> +++ b/arch/arm/mach-omap2/gpmc-onenand.c
>>> @@ -364,6 +364,8 @@ static int omap2_onenand_setup_async(void __iomem *onenand_base)
>>>         struct gpmc_timings t;
>>>         int ret;
>>>
>>> +       omap2_onenand_set_async_mode(onenand_base);
>>> +
>>>         t = omap2_onenand_calc_async_timings();
>>>
>>>         ret = gpmc_set_async_mode(gpmc_onenand_data->cs, &t);
>>
>> Yes that seems to do the trick, thanks! I can fold that into the
>> breaking patch when applying.
> 
> I am not sure what to make of this. Testing Afzal's this series along with the other
> gpmc-prep series [1], onenand is working fine on my 3430sdp and I see ...
> 
> [    2.792510] OneNAND driver initializing
> [    2.797576] omap2-onenand omap2-onenand: initializing on CS2, phys base 0x20000000, virtual base c88c0000, freq 0 MHz

I realised that the above print showing 0 MHz is another clue as to why
the above change is needed for the n900. It is printing 0 MHz because
the OMAP3430 SDP does not support sync read or write. The frequency is
only queried during the configuration of the sync mode timings and not
the async. I had just submitted a 2 patch series to fix this so that is
displays the correct frequency for all boards [1]. If you agree with my
changes may be we can include them in Afzal's series.

However, more importantly with my fix, I now see that the frequency
supporting by the OneNAND on the SDP is 66MHz. On Tony's n900 it shows
83MHz. The timings for async mode are hard-coded in the gpmc-onenand.c
and it does not have different timings for different devices and
different frequencies. Hence, this is probably why the async timings in
the gpmc-onenand.c do not work for the n900.

The problem is that unlike nand, where it is clear which boards were
dependent on the bootloader timings, for onenand it really is not 100%
clear. Some boards support ONENAND_SYNC_READWRITE and so these are not
using the async timings. However, the n8x0 appears only to use sync
read. Does the n900 use the same settings as the n8x0?

Tony, have you tried using any of the mtd kernel tests to verify OneNAND
read/write is working on your n900? For example ...

# insmod mtd_pagetest.ko dev=<mtd-part-num>

_NOTE_ that above test erases the OneNAND! ;-)

Cheers
Jon

[1] http://marc.info/?l=linux-omap&m=134090910321284&w=2

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [120628 12:05]:
> 
> Tony, have you tried using any of the mtd kernel tests to verify OneNAND
> read/write is working on your n900? For example ...
> 
> # insmod mtd_pagetest.ko dev=<mtd-part-num>
> 
> _NOTE_ that above test erases the OneNAND! ;-)

Ehh not thanks.

Tony

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony, Jon,

On Thu, Jun 28, 2012 at 22:13:37, Hunter, Jon wrote:
> On 06/28/2012 07:32 AM, Tony Lindgren wrote:
> > * Mohammed, Afzal <afzal@ti.com> [120628 02:36]:
> >> On Wed, Jun 27, 2012 at 20:28:45, Tony Lindgren wrote:

> >>> The last patch in this series causes onenand not to show
> >>> up on my n900. I believe the problem has been there earlier
> >>> too, but I just did not notice it.

> > Yes that seems to do the trick, thanks! I can fold that into the
> > breaking patch when applying.
> 
> I am not sure what to make of this. Testing Afzal's this series along with the other
> gpmc-prep series [1], onenand is working fine on my 3430sdp and I see ...
> 
> [    2.792510] OneNAND driver initializing
> [    2.797576] omap2-onenand omap2-onenand: initializing on CS2, phys base 0x20000000, virtual base c88c0000, freq 0 MHz
> [    2.808929] OneNAND Manufacturer: Samsung (0xec)
> [    2.808990] Muxed OneNAND 256MB 1.8V 16-bit (0x40)
> [    2.814208] OneNAND version = 0x002c
> 
> The above change seems to imply that Tony's n900 is dependent on the bootloader settings
> and not those being set by the kernel. Ideally, we should not need to set the async mode

In one of the Samsumg onenand datasheet, it was mentioned,
(seems Numonyx datasheet is under NDA)

[A] "If device is accessed synchronously, while set to asynchronous
read mode, it is possible to read out first data without problems"

It seems even though above may not imply the below, chances are that
below would be true

[B] "If device is accessed asynchronously, while set to synchronous
read mode, it will not be possible to read out data"

And B seems logical(to me); if onenand is set to synchronous mode,
it would expect clocks to transfer data which may not happen with
asynchronous operation from gpmc.

Here the problem with Tony's n900 (expecting it to use one of mach-id
in n8x0, hence flag sync read) board may be that bootloader (could not
find uboot handling these machines) before handing over control to
Kernel, sets it to synchronous mode. And then if we apply B, we can
expect what we are seeing here.

Assuming above is a explanation to what Tony is observing on n900,
perhaps, resetting onenand may help Kernel work even without the
proposed diff. There is a gpio pin field used in n8x0, if that is
connected to reset of onenand, may be it can achieve what we
want without the diff.

> Numonyx. The gpmc-onenand.c only has one set of settings that it is using for all
> devices. However, it would appear that at least the async settings are not working for
> the Numonyx. Therefore, may be we need to get a dump of Tony's n900 settings and make

I have a different opinion, even with the existing code, with the default
timings for onenand, Numonyx is working in async mode, reason being that
frequency is being obtained with read operation executed in async mode
(later based on this frequency, code calculates sync timings & then set
to sync mode)


This change indeed has brought to our notice one instance where
Kernel can't handle gpmc by itself, may be resetting onenand is
a solution, as it seems bootloader is leaving it in sync mode.

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi,

On Fri, Jun 29, 2012 at 11:45:49, Mohammed, Afzal wrote:

> Kernel can't handle gpmc by itself, may be resetting onenand is
> a solution, as it seems bootloader is leaving it in sync mode.

To add, the above solution is made under the assumption that onenand
after reset will be in async mode (based on Samsung datsheet)

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Jon Hunter]

Hi Afzal,

On 06/29/2012 01:15 AM, Mohammed, Afzal wrote:
> Hi Tony, Jon,
> 
> On Thu, Jun 28, 2012 at 22:13:37, Hunter, Jon wrote:
>> On 06/28/2012 07:32 AM, Tony Lindgren wrote:
>>> * Mohammed, Afzal <afzal@ti.com> [120628 02:36]:
>>>> On Wed, Jun 27, 2012 at 20:28:45, Tony Lindgren wrote:
> 
>>>>> The last patch in this series causes onenand not to show
>>>>> up on my n900. I believe the problem has been there earlier
>>>>> too, but I just did not notice it.
> 
>>> Yes that seems to do the trick, thanks! I can fold that into the
>>> breaking patch when applying.
>>
>> I am not sure what to make of this. Testing Afzal's this series along with the other
>> gpmc-prep series [1], onenand is working fine on my 3430sdp and I see ...
>>
>> [    2.792510] OneNAND driver initializing
>> [    2.797576] omap2-onenand omap2-onenand: initializing on CS2, phys base 0x20000000, virtual base c88c0000, freq 0 MHz
>> [    2.808929] OneNAND Manufacturer: Samsung (0xec)
>> [    2.808990] Muxed OneNAND 256MB 1.8V 16-bit (0x40)
>> [    2.814208] OneNAND version = 0x002c
>>
>> The above change seems to imply that Tony's n900 is dependent on the bootloader settings
>> and not those being set by the kernel. Ideally, we should not need to set the async mode
> 
> In one of the Samsumg onenand datasheet, it was mentioned,
> (seems Numonyx datasheet is under NDA)
> 
> [A] "If device is accessed synchronously, while set to asynchronous
> read mode, it is possible to read out first data without problems"
> 
> It seems even though above may not imply the below, chances are that
> below would be true
> 
> [B] "If device is accessed asynchronously, while set to synchronous
> read mode, it will not be possible to read out data"
> 
> And B seems logical(to me); if onenand is set to synchronous mode,
> it would expect clocks to transfer data which may not happen with
> asynchronous operation from gpmc.
> 
> Here the problem with Tony's n900 (expecting it to use one of mach-id
> in n8x0, hence flag sync read) board may be that bootloader (could not
> find uboot handling these machines) before handing over control to
> Kernel, sets it to synchronous mode. And then if we apply B, we can
> expect what we are seeing here.

Ok, that is a good point. However, there is also the possibility that
the OneNAND is in async mode and after changing the gpmc async timings
writes are no longer working. Although, it that was the case then maybe
we would never be able to switch to sync mode again. So may be it is
more likely that it is in sync mode. A dump of the gpmc registers should
tell us.

> Assuming above is a explanation to what Tony is observing on n900,
> perhaps, resetting onenand may help Kernel work even without the
> proposed diff. There is a gpio pin field used in n8x0, if that is
> connected to reset of onenand, may be it can achieve what we
> want without the diff.
> 
>> Numonyx. The gpmc-onenand.c only has one set of settings that it is using for all
>> devices. However, it would appear that at least the async settings are not working for
>> the Numonyx. Therefore, may be we need to get a dump of Tony's n900 settings and make
> 
> I have a different opinion, even with the existing code, with the default
> timings for onenand, Numonyx is working in async mode, reason being that
> frequency is being obtained with read operation executed in async mode
> (later based on this frequency, code calculates sync timings & then set
> to sync mode)

I don't follow "frequency is being obtained with read operation executed
in async mode". Can you give a code reference?

Frequency of the OneNAND is not important for async mode (as only sync
mode uses a clock), only the gpmc clock frequency is important for
calculating the timings. For async mode the OneNAND timings are static.

For configuring the async timings the code does not distinguish between
different OneNAND devices and therefore, assumes that the async timings
are the same for all devices. I cannot believe that a Samsung and
Numonyx device would have identical async timings. However, if the
timings are not optimal there is a chance they could work for all devices.

> This change indeed has brought to our notice one instance where
> Kernel can't handle gpmc by itself, may be resetting onenand is
> a solution, as it seems bootloader is leaving it in sync mode.

It would be an interesting test.

Cheers
Jon

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Jon,

On Fri, Jun 29, 2012 at 19:45:37, Hunter, Jon wrote:
> On 06/29/2012 01:15 AM, Mohammed, Afzal wrote:

> > I have a different opinion, even with the existing code, with the default
> > timings for onenand, Numonyx is working in async mode, reason being that
> > frequency is being obtained with read operation executed in async mode
> > (later based on this frequency, code calculates sync timings & then set
> > to sync mode)
> 
> I don't follow "frequency is being obtained with read operation executed
> in async mode". Can you give a code reference?

In the present code, for sync mode, omap2_onenand_set_sync_mode() invokes
omap2_onenand_set_async_mode() initially, to set onenand & gpmc to async mode,
then invokes omap2_onenand_get_freq() to get frequency, here finding out
frequency from onenand is done by reading relevant onenand register
in async mode.

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [120628 09:48]:
> 
> [    2.792510] OneNAND driver initializing
> [    2.797576] omap2-onenand omap2-onenand: initializing on CS2, phys base 0x20000000, virtual base c88c0000, freq 0 MHz
> [    2.808929] OneNAND Manufacturer: Samsung (0xec)
> [    2.808990] Muxed OneNAND 256MB 1.8V 16-bit (0x40)
> [    2.814208] OneNAND version = 0x002c
> 
> The above change seems to imply that Tony's n900 is dependent on the bootloader settings
> and not those being set by the kernel. Ideally, we should not need to set the async mode
> in the onenand before we set the onenand timings in the gpmc (per Afzal's changelog
> comment). This appears backwards.

That should not be the case, I'm more likely to believe in Afzal's explanation.
 
> The other thing to note is that the 3430sdp has samsung onenand where as the n900 has
> Numonyx. The gpmc-onenand.c only has one set of settings that it is using for all
> devices. However, it would appear that at least the async settings are not working for
> the Numonyx. Therefore, may be we need to get a dump of Tony's n900 settings and make
> sure the right settings are being used for the appropriate board. 

Hmm I would assume the n900 onenand settings are the most tested settings we have.
And AFAIK have also been tested with L3 frequency scaling. So let's assume they're
mostly right.
 
> These onenand settings are really killing us. I don't want us to have to spend alot
> of time re-calculating this stuff but the way it has been written to begin with is not
> driver friendly. I really wonder if we need to have some sort of callback for the 
> onenand timings from the driver. It is ugly, but the alternative is that someone needs
> to sit down and re-calculate all the timings again to get them into a driver friendly
> format. Furthermore, it seems that onenand is no longer available from the likes of
> samsung and numonyx (micron) so it is hard to justify re-calculating everything again.
> I am not even sure if we have all the datasheets!
> 
> Let me know your thoughts.

I don't think we should spend much time working on the recalculations. Let's rather
use these known working cases as examples. If they don't work, it's likely that
adding any new devices won't work either.

For the timings, we should allow specifying either cycles or time values in the
data struct. And always just use the cycle value directly if specified, and
othewise use the time value. We may be able to limit the registers where we need
to allow either cycle or time value depending on the device futher.

In general, I doubt that we can come up with better calculations. The existing
code pretty well already follows the device spec timings. And using cycle values
for some registers is the right thing to do according to the connected device
specs no matter what the frequency is. In those cases converting from time values
to cycles does not make sense.

Regards,

Tony

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony,

On Mon, Jul 02, 2012 at 12:06:51, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [120628 09:48]:

> > The above change seems to imply that Tony's n900 is dependent on the bootloader settings
> > and not those being set by the kernel. Ideally, we should not need to set the async mode
> > in the onenand before we set the onenand timings in the gpmc (per Afzal's changelog
> > comment). This appears backwards.
> 
> That should not be the case, I'm more likely to believe in Afzal's explanation.

Not sure whether you are fine with fixing up this patch with added diff

Assuming inferences so far is not wrong, right now this patch with the added diff
would be perfectly fine.

Problem would happen when we are at a stage to do gpmc reset using hwmod [seems
miles to go before I sleep {or read gpmc hwmod reset} ;)]. If bootloader left
onenand configured in sync mode, to switch onenand to async mode, first configuring
gpmc to sync mode would be required & for that we need frequency information
from onenand and to get that information from onenand, gpmc has to be configured
for sync mode and to configure gpmc to sync mode ....

Seems like chicken and egg problem.

N900, I believe is board-rx51, could not get proper schematic for onenand
connections, nor do I have the Numonyx datsheet, or the board, hence not
sure whether resetting onenand may resolve the issue. But if you feel that
it is the right solution & it would resolve the issue, and if you can let
me know gpio connected (if any) to onenand reset or else some pointers to
achieving it, I can try making a patch to do onenand reset

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Jon Hunter]

On 07/02/2012 04:43 AM, Mohammed, Afzal wrote:
> Hi Tony,
> 
> On Mon, Jul 02, 2012 at 12:06:51, Tony Lindgren wrote:
>> * Jon Hunter <jon-hunter@ti.com> [120628 09:48]:
> 
>>> The above change seems to imply that Tony's n900 is dependent on the bootloader settings
>>> and not those being set by the kernel. Ideally, we should not need to set the async mode
>>> in the onenand before we set the onenand timings in the gpmc (per Afzal's changelog
>>> comment). This appears backwards.
>>
>> That should not be the case, I'm more likely to believe in Afzal's explanation.
> 
> Not sure whether you are fine with fixing up this patch with added diff
> 
> Assuming inferences so far is not wrong, right now this patch with the added diff
> would be perfectly fine.
> 
> Problem would happen when we are at a stage to do gpmc reset using hwmod [seems
> miles to go before I sleep {or read gpmc hwmod reset} ;)]. If bootloader left
> onenand configured in sync mode, to switch onenand to async mode, first configuring
> gpmc to sync mode would be required & for that we need frequency information
> from onenand and to get that information from onenand, gpmc has to be configured
> for sync mode and to configure gpmc to sync mode ....

You are concerned about hwmod performing a reset of the gpmc during
boot? We should be able to use the HWMOD_INIT_NO_RESET flag to prevent
this. Would that work?

Cheers
Jon

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Jon,

On Mon, Jul 02, 2012 at 22:59:03, Hunter, Jon wrote:
> On 07/02/2012 04:43 AM, Mohammed, Afzal wrote:

> > Not sure whether you are fine with fixing up this patch with added diff
> > 
> > Assuming inferences so far is not wrong, right now this patch with the added diff
> > would be perfectly fine.
> > 
> > Problem would happen when we are at a stage to do gpmc reset using hwmod [seems
> > miles to go before I sleep {or read gpmc hwmod reset} ;)]. If bootloader left
> > onenand configured in sync mode, to switch onenand to async mode, first configuring
> > gpmc to sync mode would be required & for that we need frequency information
> > from onenand and to get that information from onenand, gpmc has to be configured
> > for sync mode and to configure gpmc to sync mode ....
> 
> You are concerned about hwmod performing a reset of the gpmc during
> boot? We should be able to use the HWMOD_INIT_NO_RESET flag to prevent
> this. Would that work?

Yes that will work in the short term, the reason I am trying to avoid no
reset flag in the long term is to prevent this board support being broken,
please refer Paul's requirement [1] in accepting gpmc hwmod patch

Regards
Afzal

[1] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69041.html

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Jon Hunter]

On 07/02/2012 11:35 PM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Mon, Jul 02, 2012 at 22:59:03, Hunter, Jon wrote:
>> On 07/02/2012 04:43 AM, Mohammed, Afzal wrote:
> 
>>> Not sure whether you are fine with fixing up this patch with added diff
>>>
>>> Assuming inferences so far is not wrong, right now this patch with the added diff
>>> would be perfectly fine.
>>>
>>> Problem would happen when we are at a stage to do gpmc reset using hwmod [seems
>>> miles to go before I sleep {or read gpmc hwmod reset} ;)]. If bootloader left
>>> onenand configured in sync mode, to switch onenand to async mode, first configuring
>>> gpmc to sync mode would be required & for that we need frequency information
>>> from onenand and to get that information from onenand, gpmc has to be configured
>>> for sync mode and to configure gpmc to sync mode ....
>>
>> You are concerned about hwmod performing a reset of the gpmc during
>> boot? We should be able to use the HWMOD_INIT_NO_RESET flag to prevent
>> this. Would that work?
> 
> Yes that will work in the short term, the reason I am trying to avoid no
> reset flag in the long term is to prevent this board support being broken,
> please refer Paul's requirement [1] in accepting gpmc hwmod patch

Ok, thanks for the reminder.

So we have 2 options here ...

1. Use the HWMOD_INIT_NO_RESET for now and your updated version of this
patch
2. See if there is a gpio available to control the OneNAND reset on the
n900.

Do you agree? Any other options?

Cheers
Jon

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Jon, Tony,

On Tue, Jul 03, 2012 at 20:40:03, Hunter, Jon wrote:

> So we have 2 options here ...
> 
> 1. Use the HWMOD_INIT_NO_RESET for now and your updated version of this
> patch
> 2. See if there is a gpio available to control the OneNAND reset on the
> n900.
> 
> Do you agree? Any other options?

Yes agree, I am fine either way, unable to see any other option

HWMOD_INIT_NO_RESET is any way required to start driver conversion
as both new & old interface needs to be supported till all machines
are converted to use the new driver interface as well as at least
till all machines are made independent of bootloader w.r.t GPMC

As of now I feel we should go with option 1

If we need to go with option 2, (assuming OneNAND reset is solution
to the problem) all machines that is configured for sync mode
presently (n8x0, rx51, rm680) has to be made intelligent to do
OneNAND reset (provided gpio is connected to reset). And for all
these machines including N900 (seems rx51), information on gpio pin
connected to reset (if at all connected) and the way to do reset is
required, which I am so far unable to get. Once this information
could be obtained, I can make the patch and then need help from
someone who has the board to test it

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Jon Hunter]

On 07/02/2012 01:36 AM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [120628 09:48]:
>>
>> [    2.792510] OneNAND driver initializing
>> [    2.797576] omap2-onenand omap2-onenand: initializing on CS2, phys base 0x20000000, virtual base c88c0000, freq 0 MHz
>> [    2.808929] OneNAND Manufacturer: Samsung (0xec)
>> [    2.808990] Muxed OneNAND 256MB 1.8V 16-bit (0x40)
>> [    2.814208] OneNAND version = 0x002c
>>
>> The above change seems to imply that Tony's n900 is dependent on the bootloader settings
>> and not those being set by the kernel. Ideally, we should not need to set the async mode
>> in the onenand before we set the onenand timings in the gpmc (per Afzal's changelog
>> comment). This appears backwards.
> 
> That should not be the case, I'm more likely to believe in Afzal's explanation.

Right I agree with Afzal's explanation now too.

>> The other thing to note is that the 3430sdp has samsung onenand where as the n900 has
>> Numonyx. The gpmc-onenand.c only has one set of settings that it is using for all
>> devices. However, it would appear that at least the async settings are not working for
>> the Numonyx. Therefore, may be we need to get a dump of Tony's n900 settings and make
>> sure the right settings are being used for the appropriate board. 
> 
> Hmm I would assume the n900 onenand settings are the most tested settings we have.
> And AFAIK have also been tested with L3 frequency scaling. So let's assume they're
> mostly right.
>  
>> These onenand settings are really killing us. I don't want us to have to spend alot
>> of time re-calculating this stuff but the way it has been written to begin with is not
>> driver friendly. I really wonder if we need to have some sort of callback for the 
>> onenand timings from the driver. It is ugly, but the alternative is that someone needs
>> to sit down and re-calculate all the timings again to get them into a driver friendly
>> format. Furthermore, it seems that onenand is no longer available from the likes of
>> samsung and numonyx (micron) so it is hard to justify re-calculating everything again.
>> I am not even sure if we have all the datasheets!
>>
>> Let me know your thoughts.
> 
> I don't think we should spend much time working on the recalculations. Let's rather
> use these known working cases as examples. If they don't work, it's likely that
> adding any new devices won't work either.
> 
> For the timings, we should allow specifying either cycles or time values in the
> data struct. And always just use the cycle value directly if specified, and
> othewise use the time value. We may be able to limit the registers where we need
> to allow either cycle or time value depending on the device futher.
> 
> In general, I doubt that we can come up with better calculations. The existing
> code pretty well already follows the device spec timings. And using cycle values
> for some registers is the right thing to do according to the connected device
> specs no matter what the frequency is. In those cases converting from time values
> to cycles does not make sense.

Ok agree, but the problem here is how to provide the timings to the
driver. The onenand code is doing a lot of rounding based upon the gpmc
clock before it presents the timings (in nano-seconds) to the gpmc
function to calculate the final timings and program the gpmc
chip-select. So therefore I think that we have the following options ...

1. The simplest is to continue using a global variable for storing the
gpmc f-clk handle and have the OneNAND timings calculated prior to
probing the gpmc driver.

2. Provide some sort of "retime" callback that the gpmc driver can call
at probe time to calculate the timings.

Cheers
Jon

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [120702 10:30]:
> 
> On 07/02/2012 01:36 AM, Tony Lindgren wrote:
> > 
> > In general, I doubt that we can come up with better calculations. The existing
> > code pretty well already follows the device spec timings. And using cycle values
> > for some registers is the right thing to do according to the connected device
> > specs no matter what the frequency is. In those cases converting from time values
> > to cycles does not make sense.
> 
> Ok agree, but the problem here is how to provide the timings to the
> driver. The onenand code is doing a lot of rounding based upon the gpmc
> clock before it presents the timings (in nano-seconds) to the gpmc
> function to calculate the final timings and program the gpmc
> chip-select. So therefore I think that we have the following options ...
> 
> 1. The simplest is to continue using a global variable for storing the
> gpmc f-clk handle and have the OneNAND timings calculated prior to
> probing the gpmc driver.

Well we should not expose gpmc fck handle to the drivers..
 
> 2. Provide some sort of "retime" callback that the gpmc driver can call
> at probe time to calculate the timings.

Yes how about the gpmc using driver code registers itself with the gpmc code
and also registers it's retime function with the gpmc? That way the gpmc fck
stays inside the gpmc code, and the driver specific retime function should
be able to do the calculation based on driver clocks. The retime function
needs to have also a pointer to driver private data for it's clocks etc.

It seems this retime function may need to be called by the gpmc code when
L3 changes, and the driver code if the driver is switching between runtime
and idle clocks like tusb6010 for example does.

Regards,

Tony

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Jon Hunter]

On 07/03/2012 03:17 AM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [120702 10:30]:
>>
>> On 07/02/2012 01:36 AM, Tony Lindgren wrote:
>>>
>>> In general, I doubt that we can come up with better calculations. The existing
>>> code pretty well already follows the device spec timings. And using cycle values
>>> for some registers is the right thing to do according to the connected device
>>> specs no matter what the frequency is. In those cases converting from time values
>>> to cycles does not make sense.
>>
>> Ok agree, but the problem here is how to provide the timings to the
>> driver. The onenand code is doing a lot of rounding based upon the gpmc
>> clock before it presents the timings (in nano-seconds) to the gpmc
>> function to calculate the final timings and program the gpmc
>> chip-select. So therefore I think that we have the following options ...
>>
>> 1. The simplest is to continue using a global variable for storing the
>> gpmc f-clk handle and have the OneNAND timings calculated prior to
>> probing the gpmc driver.
> 
> Well we should not expose gpmc fck handle to the drivers..

Yes, I was hoping we could avoid continuing to do this.

>> 2. Provide some sort of "retime" callback that the gpmc driver can call
>> at probe time to calculate the timings.
> 
> Yes how about the gpmc using driver code registers itself with the gpmc code
> and also registers it's retime function with the gpmc? That way the gpmc fck
> stays inside the gpmc code, and the driver specific retime function should
> be able to do the calculation based on driver clocks. The retime function
> needs to have also a pointer to driver private data for it's clocks etc.
> 
> It seems this retime function may need to be called by the gpmc code when
> L3 changes, and the driver code if the driver is switching between runtime
> and idle clocks like tusb6010 for example does.

Seems fine to me.

Cheers
Jon

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony,

On Tue, Jul 03, 2012 at 13:47:47, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [120702 10:30]:

> > 2. Provide some sort of "retime" callback that the gpmc driver can call
> > at probe time to calculate the timings.
> 
> Yes how about the gpmc using driver code registers itself with the gpmc code
> and also registers it's retime function with the gpmc? That way the gpmc fck
> stays inside the gpmc code, and the driver specific retime function should
> be able to do the calculation based on driver clocks. The retime function
> needs to have also a pointer to driver private data for it's clocks etc.

Sorry, not sure whether I follow you properly, based on what has been
understood, will try to rephrase,

All the present gpmc timing calculation done in arch/arm/mach-omap2/gpmc-*
to be moved to gpmc driver. And all the peripheral drivers using gpmc, i.e
like smsc911x, tusb6010 needs to register their retime function with gpmc
driver. And gpmc driver will invoke these registered retime function, when
clock frequency changes.

But wouldn't it need changes in the existing drivers like smsc911x that are
used by multiple architectures with gpmc specific registration (put under
a macro ?). We will be having gpmc driver code that contains knowledge
about peripheral timing calculation, seems there is no way out of this.
Peripheral agnostic gpmc code may not happen it seems

In that case gpmc driver probe would have to be relieved of the task of
setting up gpmc timings as we have to wait till peripheral drivers
register their callback, right ?, seems in that case no timing information
needs (or can be) to be passed from DT

> It seems this retime function may need to be called by the gpmc code when
> L3 changes, and the driver code if the driver is switching between runtime
> and idle clocks like tusb6010 for example does.

I believe you are referring to tusb6010_platform_retime(), other than during
initial setup, i.e. in tusb6010_setup_interface(), it is not invoked.
tusb6010_platform_retime() is an exported symbol, unless I am missing
something it is not invoked other than during initial setup. Did find this
function in tusb6010.c, it is commented out, may be this was present earlier ?

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120704 00:05]:
> Hi Tony,
> 
> On Tue, Jul 03, 2012 at 13:47:47, Tony Lindgren wrote:
> > * Jon Hunter <jon-hunter@ti.com> [120702 10:30]:
> 
> > > 2. Provide some sort of "retime" callback that the gpmc driver can call
> > > at probe time to calculate the timings.
> > 
> > Yes how about the gpmc using driver code registers itself with the gpmc code
> > and also registers it's retime function with the gpmc? That way the gpmc fck
> > stays inside the gpmc code, and the driver specific retime function should
> > be able to do the calculation based on driver clocks. The retime function
> > needs to have also a pointer to driver private data for it's clocks etc.
> 
> Sorry, not sure whether I follow you properly, based on what has been
> understood, will try to rephrase,
> 
> All the present gpmc timing calculation done in arch/arm/mach-omap2/gpmc-*
> to be moved to gpmc driver. And all the peripheral drivers using gpmc, i.e
> like smsc911x, tusb6010 needs to register their retime function with gpmc
> driver. And gpmc driver will invoke these registered retime function, when
> clock frequency changes.
> 
> But wouldn't it need changes in the existing drivers like smsc911x that are
> used by multiple architectures with gpmc specific registration (put under
> a macro ?). We will be having gpmc driver code that contains knowledge
> about peripheral timing calculation, seems there is no way out of this.
> Peripheral agnostic gpmc code may not happen it seems

It depends. For some drivers scaling both gpmc clock and the device clock
can happen, like with tusb6010 for example. But the smsc911x does not know
about the clocks.. So to additional changes to the driver would be required
to if device clocks need scaling. But for now, we should be able to do it
at gpmc level with the retime function, or just disable DFS for those clocks
if not supported.

The ideal solution in the long run would be to have gpmc scaling frequency
as the bus and device scaling frequency using cpufreq/devicefreq whatever
notifiers.
 
> In that case gpmc driver probe would have to be relieved of the task of
> setting up gpmc timings as we have to wait till peripheral drivers
> register their callback, right ?, seems in that case no timing information
> needs (or can be) to be passed from DT

Well we should pass all the gpmc timing information from DT. And then the
driver also still needs to register it's retime function with gpmc.

> > It seems this retime function may need to be called by the gpmc code when
> > L3 changes, and the driver code if the driver is switching between runtime
> > and idle clocks like tusb6010 for example does.
> 
> I believe you are referring to tusb6010_platform_retime(), other than during
> initial setup, i.e. in tusb6010_setup_interface(), it is not invoked.
> tusb6010_platform_retime() is an exported symbol, unless I am missing
> something it is not invoked other than during initial setup. Did find this
> function in tusb6010.c, it is commented out, may be this was present earlier ?

Hmm yes looks like it's commented out.. But in theory the retime function
should be called between idle clock and runtime clock. And also for DFS,
so it's something we should be considered for proper gpmc support.

Regards,

Tony

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony,

On Wed, Jul 04, 2012 at 13:21:59, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120704 00:05]:
> > On Tue, Jul 03, 2012 at 13:47:47, Tony Lindgren wrote:

> > > Yes how about the gpmc using driver code registers itself with the gpmc code
> > > and also registers it's retime function with the gpmc? That way the gpmc fck
> > > stays inside the gpmc code, and the driver specific retime function should
> > > be able to do the calculation based on driver clocks. The retime function
> > > needs to have also a pointer to driver private data for it's clocks etc.
> > 
> > Sorry, not sure whether I follow you properly, based on what has been
> > understood, will try to rephrase,
> > 
> > All the present gpmc timing calculation done in arch/arm/mach-omap2/gpmc-*
> > to be moved to gpmc driver. And all the peripheral drivers using gpmc, i.e
> > like smsc911x, tusb6010 needs to register their retime function with gpmc
> > driver. And gpmc driver will invoke these registered retime function, when
> > clock frequency changes.
> > 
> > But wouldn't it need changes in the existing drivers like smsc911x that are
> > used by multiple architectures with gpmc specific registration (put under
> > a macro ?). We will be having gpmc driver code that contains knowledge
> > about peripheral timing calculation, seems there is no way out of this.
> > Peripheral agnostic gpmc code may not happen it seems
> 
> It depends. For some drivers scaling both gpmc clock and the device clock
> can happen, like with tusb6010 for example. But the smsc911x does not know
> about the clocks.. So to additional changes to the driver would be required
> to if device clocks need scaling. But for now, we should be able to do it
> at gpmc level with the retime function, or just disable DFS for those clocks
> if not supported.

I have a doubt whether we are talking about the same thing, presently
our main issue is in eliminating the necessity of peripheral specific
functions like gpmc_onenand_init, tusb_setup_interface (which calls
tusb6010_platform_retime), etc., which calculate gpmc timings by
processing peripheral specific timings over gpmc clock period and
these processing were required before gpmc driver probe gets invoked
as gpmc driver needed timings which gpmc can understand to configure
gpmc.

During boot time, gpmc driver needs to know timings in terms of gpmc
parameters to configure gpmc, what I unable to understand from your
proposal is how gpmc driver knows which retime function to invoke (so
that gpmc timings can be calculated based on the type of peripheral)
to calculate gpmc timings. Or do you want a DT field specifying type
of peripheral connected and so that at probe time the relevant retime
can be invoked ?

Initially I thought you were suggesting that all peripheral drivers
connected to gpmc should register their retime function (where
function will be an exported symbol - part of gpmc code) and
invoke the respective retime as part of peripheral driver probe,
hence relieve gpmc driver of doing it, even though retime would be
present as part of gpmc code. But seems that is not what you were
suggesting.

By "we should be able to do it at gpmc level", I am unable to
understand what you are suggesting.

> > In that case gpmc driver probe would have to be relieved of the task of
> > setting up gpmc timings as we have to wait till peripheral drivers
> > register their callback, right ?, seems in that case no timing information
> > needs (or can be) to be passed from DT
> 
> Well we should pass all the gpmc timing information from DT. And then the
> driver also still needs to register it's retime function with gpmc.

So timing information that would be passed from DT should be for
exact gpmc timings like cs_on, cs_off etc., right ?, in that case
should we manually calculate (like as now done by Kernel in
gpmc-onenand.c etc) it by having the knowledge of connected
peripheral & gpmc frequency at boot time and update it in DT ?, as
we can't apply retime on it as we don't know the connected
peripheral in gpmc driver. Or do you want another field through DT
to decide retime that is to be used, then I think passing timing
from DT would not be needed

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120705 03:29]:
> 
> I have a doubt whether we are talking about the same thing, presently
> our main issue is in eliminating the necessity of peripheral specific
> functions like gpmc_onenand_init, tusb_setup_interface (which calls
> tusb6010_platform_retime), etc., which calculate gpmc timings by
> processing peripheral specific timings over gpmc clock period and
> these processing were required before gpmc driver probe gets invoked
> as gpmc driver needed timings which gpmc can understand to configure
> gpmc.

Right. The issue is that both the gpmc clock and the peripheral clock
may change, and both cause the need to reprogram the gpmc timings.
 
> During boot time, gpmc driver needs to know timings in terms of gpmc
> parameters to configure gpmc, what I unable to understand from your
> proposal is how gpmc driver knows which retime function to invoke (so
> that gpmc timings can be calculated based on the type of peripheral)
> to calculate gpmc timings. Or do you want a DT field specifying type
> of peripheral connected and so that at probe time the relevant retime
> can be invoked ?

The peripheral init code should register it's timings with gpmc,
and also register a retime function for the gpmc code to use. Then
the timings can eventually come from DT.
 
> Initially I thought you were suggesting that all peripheral drivers
> connected to gpmc should register their retime function (where
> function will be an exported symbol - part of gpmc code) and
> invoke the respective retime as part of peripheral driver probe,
> hence relieve gpmc driver of doing it, even though retime would be
> present as part of gpmc code. But seems that is not what you were
> suggesting.

Yes that's what I was suggesting.
 
> By "we should be able to do it at gpmc level", I am unable to
> understand what you are suggesting.

Right, gpmc should be able to handle things alone with the registered
retime function for smsc911x, where the driver does not even know about
the bus. With DT, the platform init code gpmc-smsc911x.c should become
a driver that registers with gpmc and provides the retime function.

For some drivers, like  tusb6010, also the peripheral clock affects the
timings. So both gpmc and the driver may need to be able to call the
retime function.
 
> > > In that case gpmc driver probe would have to be relieved of the task of
> > > setting up gpmc timings as we have to wait till peripheral drivers
> > > register their callback, right ?, seems in that case no timing information
> > > needs (or can be) to be passed from DT
> > 
> > Well we should pass all the gpmc timing information from DT. And then the
> > driver also still needs to register it's retime function with gpmc.
> 
> So timing information that would be passed from DT should be for
> exact gpmc timings like cs_on, cs_off etc., right ?, in that case
> should we manually calculate (like as now done by Kernel in
> gpmc-onenand.c etc) it by having the knowledge of connected
> peripheral & gpmc frequency at boot time and update it in DT ?, as
> we can't apply retime on it as we don't know the connected
> peripheral in gpmc driver. Or do you want another field through DT
> to decide retime that is to be used, then I think passing timing
> from DT would not be needed

The timings values should be passed to gpmc from DT. We need to
be able to pass both cycle and time based values. If no cycle based
value is passed, the time based value should be used. This is because
some peripheral timings can be cycle based, while others can be time
based.

The peripheral driver can register it's retime function with gpmc and
get a cookie back that allows getting the DT provided timings from gpmc.
And after that the initial timings can be set.

Regards,

Tony

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony,

On Thu, Jul 05, 2012 at 16:25:35, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120705 03:29]:

> > Initially I thought you were suggesting that all peripheral drivers
> > connected to gpmc should register their retime function (where

> Yes that's what I was suggesting.

To understand you better, peripheral drivers above refers to
driver created out of present gpmc platform init code like
gpmc-smsc911x.c. You were not referring to
drivers/net/ethernet/smsc/smsc911x.c, right ?

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120705 05:03]:
> Hi Tony,
> 
> On Thu, Jul 05, 2012 at 16:25:35, Tony Lindgren wrote:
> > * Mohammed, Afzal <afzal@ti.com> [120705 03:29]:
> 
> > > Initially I thought you were suggesting that all peripheral drivers
> > > connected to gpmc should register their retime function (where
> 
> > Yes that's what I was suggesting.
> 
> To understand you better, peripheral drivers above refers to
> driver created out of present gpmc platform init code like
> gpmc-smsc911x.c. You were not referring to
> drivers/net/ethernet/smsc/smsc911x.c, right ?

Yes arch/arm/mach-omap2/gpmc-smsc911x.c for now, but eventually
that too should probably be just a regular driver. Just how it
will work together with drivers/net/ethernet/smsc/smsc911x.c is
still a bit open though :)

Tony

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony,

On Thu, Jul 05, 2012 at 16:25:35, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120705 03:29]:

> > I have a doubt whether we are talking about the same thing, presently
> > our main issue is in eliminating the necessity of peripheral specific
> > functions like gpmc_onenand_init, tusb_setup_interface (which calls
> > tusb6010_platform_retime), etc., which calculate gpmc timings by
> > processing peripheral specific timings over gpmc clock period and
> > these processing were required before gpmc driver probe gets invoked
> > as gpmc driver needed timings which gpmc can understand to configure
> > gpmc.
> 
> Right. The issue is that both the gpmc clock and the peripheral clock
> may change, and both cause the need to reprogram the gpmc timings.

Presently bigger issue that I am facing w.r.t driver conversion is the
requirement of peripheral specific gpmc timing calculation before probe.
I believe currently in mainline runtime gpmc clock changes are not
handled

> > By "we should be able to do it at gpmc level", I am unable to
> > understand what you are suggesting.
> 
> Right, gpmc should be able to handle things alone with the registered
> retime function for smsc911x, where the driver does not even know about
> the bus. With DT, the platform init code gpmc-smsc911x.c should become
> a driver that registers with gpmc and provides the retime function.

So then we would be having two devices & drivers to represent gpmc
peripheral like smsc911x, one existing ethernet driver and other one
for handling gpmc timings, right ?. And with DT, so we need two nodes
to represent a gpmc peripheral ?

> > So timing information that would be passed from DT should be for
> > exact gpmc timings like cs_on, cs_off etc., right ?, in that case
> > should we manually calculate (like as now done by Kernel in
> > gpmc-onenand.c etc) it by having the knowledge of connected
> > peripheral & gpmc frequency at boot time and update it in DT ?, as
> > we can't apply retime on it as we don't know the connected
> > peripheral in gpmc driver. Or do you want another field through DT
> > to decide retime that is to be used, then I think passing timing
> > from DT would not be needed
> 
> The timings values should be passed to gpmc from DT. We need to
> be able to pass both cycle and time based values. If no cycle based
> value is passed, the time based value should be used. This is because
> some peripheral timings can be cycle based, while others can be time
> based.

If we can describe gpmc timings purely based on time and cycles field
for all the peripherals, can we not remove all the retime functions like
timing calculation done in gpmc-onenand.c ?
 
> The peripheral driver can register it's retime function with gpmc and
> get a cookie back that allows getting the DT provided timings from gpmc.
> And after that the initial timings can be set.

If timings peripheral timings can be fully contained in driver, should
we try to pass the same timings translated in terms of gpmc timings
through DT ?, and how do we get equivalent gpmc timings to update DT,
manually calculate similar to platform init code ?, or I misunderstood
you

Sorry to trouble you with more questions, I wanted to understand the way
you want to deal with the issue.

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120705 07:56]:
> Hi Tony,
> 
> On Thu, Jul 05, 2012 at 16:25:35, Tony Lindgren wrote:
> > * Mohammed, Afzal <afzal@ti.com> [120705 03:29]:
> 
> > > I have a doubt whether we are talking about the same thing, presently
> > > our main issue is in eliminating the necessity of peripheral specific
> > > functions like gpmc_onenand_init, tusb_setup_interface (which calls
> > > tusb6010_platform_retime), etc., which calculate gpmc timings by
> > > processing peripheral specific timings over gpmc clock period and
> > > these processing were required before gpmc driver probe gets invoked
> > > as gpmc driver needed timings which gpmc can understand to configure
> > > gpmc.
> > 
> > Right. The issue is that both the gpmc clock and the peripheral clock
> > may change, and both cause the need to reprogram the gpmc timings.
> 
> Presently bigger issue that I am facing w.r.t driver conversion is the
> requirement of peripheral specific gpmc timing calculation before probe.
> I believe currently in mainline runtime gpmc clock changes are not
> handled

Hmm I don't follow, why can't the timings be set during the peripheral
specific driver probe?
 
> > > By "we should be able to do it at gpmc level", I am unable to
> > > understand what you are suggesting.
> > 
> > Right, gpmc should be able to handle things alone with the registered
> > retime function for smsc911x, where the driver does not even know about
> > the bus. With DT, the platform init code gpmc-smsc911x.c should become
> > a driver that registers with gpmc and provides the retime function.
> 
> So then we would be having two devices & drivers to represent gpmc
> peripheral like smsc911x, one existing ethernet driver and other one
> for handling gpmc timings, right ?. And with DT, so we need two nodes
> to represent a gpmc peripheral ?

Well ideally we'd have some kind of bus glue setup eventually so we'll
have just one device for smsc911x.. But like I said, that part is a
bit open still. At least I don't have any clear solution in mind for
how to do the bus specific wrapper drivers.
 
> > > So timing information that would be passed from DT should be for
> > > exact gpmc timings like cs_on, cs_off etc., right ?, in that case
> > > should we manually calculate (like as now done by Kernel in
> > > gpmc-onenand.c etc) it by having the knowledge of connected
> > > peripheral & gpmc frequency at boot time and update it in DT ?, as
> > > we can't apply retime on it as we don't know the connected
> > > peripheral in gpmc driver. Or do you want another field through DT
> > > to decide retime that is to be used, then I think passing timing
> > > from DT would not be needed
> > 
> > The timings values should be passed to gpmc from DT. We need to
> > be able to pass both cycle and time based values. If no cycle based
> > value is passed, the time based value should be used. This is because
> > some peripheral timings can be cycle based, while others can be time
> > based.
> 
> If we can describe gpmc timings purely based on time and cycles field
> for all the peripherals, can we not remove all the retime functions like
> timing calculation done in gpmc-onenand.c ?

No that's probably not enough because the time and cycles for a peripheral
may need to be different if the peripheral clock rate changes.
  
> > The peripheral driver can register it's retime function with gpmc and
> > get a cookie back that allows getting the DT provided timings from gpmc.
> > And after that the initial timings can be set.
> 
> If timings peripheral timings can be fully contained in driver, should
> we try to pass the same timings translated in terms of gpmc timings
> through DT ?, and how do we get equivalent gpmc timings to update DT,
> manually calculate similar to platform init code ?, or I misunderstood
> you
> 
> Sorry to trouble you with more questions, I wanted to understand the way
> you want to deal with the issue.

Well yes the timings should be passed via devicetree in a gpmc specific
format. But the peripheral specific retime function still needs to be
also registered for peripherals that need it.

Regards,

Tony

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony,

Could not respond you earlier as was sick

On Fri, Jul 06, 2012 at 17:35:33, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120705 07:56]:
> > On Thu, Jul 05, 2012 at 16:25:35, Tony Lindgren wrote:

> > Presently bigger issue that I am facing w.r.t driver conversion is the
> > requirement of peripheral specific gpmc timing calculation before probe.
> > I believe currently in mainline runtime gpmc clock changes are not
> > handled
> 
> Hmm I don't follow, why can't the timings be set during the peripheral
> specific driver probe?

My viewpoint while making the above comment was with gpmc driver
series that was posted, in that it was required to do peripheral specific
timings calculation before probe. With your suggested peripheral specific
driver for the purpose of handling retime, it is not a problem, a
solution on how to integrate it has to be found though.

> > > The peripheral driver can register it's retime function with gpmc and
> > > get a cookie back that allows getting the DT provided timings from gpmc.
> > > And after that the initial timings can be set.
> > 
> > If timings peripheral timings can be fully contained in driver, should
> > we try to pass the same timings translated in terms of gpmc timings
> > through DT ?, and how do we get equivalent gpmc timings to update DT,
> > manually calculate similar to platform init code ?, or I misunderstood
> > you

> Well yes the timings should be passed via devicetree in a gpmc specific

I assume with the above, you were referring to peripherals that does not
have retime function.

> format. But the peripheral specific retime function still needs to be
> also registered for peripherals that need it.

For the peripherals requiring retime, we cannot (as otherwise whatever
retime does would have to be manually done based on the knowledge of
boot time gpmc clock period to calculate gpmc timings to be fed to DT)
pass gpmc timings via device tree, right ?

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120709 23:24]:
> Hi Tony,
> 
> Could not respond you earlier as was sick
> 
> On Fri, Jul 06, 2012 at 17:35:33, Tony Lindgren wrote:
> > * Mohammed, Afzal <afzal@ti.com> [120705 07:56]:
> > > On Thu, Jul 05, 2012 at 16:25:35, Tony Lindgren wrote:
> 
> > > Presently bigger issue that I am facing w.r.t driver conversion is the
> > > requirement of peripheral specific gpmc timing calculation before probe.
> > > I believe currently in mainline runtime gpmc clock changes are not
> > > handled
> > 
> > Hmm I don't follow, why can't the timings be set during the peripheral
> > specific driver probe?
> 
> My viewpoint while making the above comment was with gpmc driver
> series that was posted, in that it was required to do peripheral specific
> timings calculation before probe. With your suggested peripheral specific
> driver for the purpose of handling retime, it is not a problem, a
> solution on how to integrate it has to be found though.

The connected peripheral probe should be able to set the gpmc timings
just fine before registering with the gpmc.
 
> > > > The peripheral driver can register it's retime function with gpmc and
> > > > get a cookie back that allows getting the DT provided timings from gpmc.
> > > > And after that the initial timings can be set.
> > > 
> > > If timings peripheral timings can be fully contained in driver, should
> > > we try to pass the same timings translated in terms of gpmc timings
> > > through DT ?, and how do we get equivalent gpmc timings to update DT,
> > > manually calculate similar to platform init code ?, or I misunderstood
> > > you
> 
> > Well yes the timings should be passed via devicetree in a gpmc specific
> 
> I assume with the above, you were referring to peripherals that does not
> have retime function.

It can still have a retime function, it just needs to be registered during
the probe of the connected peripheral as we can't pass that from device tree.
 
> > format. But the peripheral specific retime function still needs to be
> > also registered for peripherals that need it.
> 
> For the peripherals requiring retime, we cannot (as otherwise whatever
> retime does would have to be manually done based on the knowledge of
> boot time gpmc clock period to calculate gpmc timings to be fed to DT)
> pass gpmc timings via device tree, right ?

We can still do it when the connected peripheral probe registers with
gpmc.

Regards,

Tony

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony,

On Tue, Jul 10, 2012 at 15:15:38, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120709 23:24]:
> > On Fri, Jul 06, 2012 at 17:35:33, Tony Lindgren wrote:

> > > format. But the peripheral specific retime function still needs to be
> > > also registered for peripherals that need it.
> > 
> > For the peripherals requiring retime, we cannot (as otherwise whatever
> > retime does would have to be manually done based on the knowledge of
> > boot time gpmc clock period to calculate gpmc timings to be fed to DT)
> > pass gpmc timings via device tree, right ?
> 
> We can still do it when the connected peripheral probe registers with
> gpmc.

We can, but would it be feasible practically ?, gpmc timings to update in
DT for a such a peripheral (requiring retime) can be found out only by
manual calculation similar to the way done in retime function (based on
peripheral's timings and boot time gpmc clock period), correct ?, Also
wouldn't this make it necessary to know gpmc clock period at boot time
for properly updating gpmc timing entry in DT ?

And in this case, we are going to register retime function, so instead of
relying on DT to provide gpmc timings for such a peripheral, won't it
be better to make use of retime that is already registered ?

 
Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120710 03:09]:
> Hi Tony,
> 
> On Tue, Jul 10, 2012 at 15:15:38, Tony Lindgren wrote:
> > * Mohammed, Afzal <afzal@ti.com> [120709 23:24]:
> > > On Fri, Jul 06, 2012 at 17:35:33, Tony Lindgren wrote:
> 
> > > > format. But the peripheral specific retime function still needs to be
> > > > also registered for peripherals that need it.
> > > 
> > > For the peripherals requiring retime, we cannot (as otherwise whatever
> > > retime does would have to be manually done based on the knowledge of
> > > boot time gpmc clock period to calculate gpmc timings to be fed to DT)
> > > pass gpmc timings via device tree, right ?
> > 
> > We can still do it when the connected peripheral probe registers with
> > gpmc.
> 
> We can, but would it be feasible practically ?, gpmc timings to update in
> DT for a such a peripheral (requiring retime) can be found out only by
> manual calculation similar to the way done in retime function (based on
> peripheral's timings and boot time gpmc clock period), correct ?, Also
> wouldn't this make it necessary to know gpmc clock period at boot time
> for properly updating gpmc timing entry in DT ?

The gpmc clock period can be returned to the connected peripheral when
it's registering. Well basically we can call the retime function upon
registering and pass the gpmc clock period.
 
> And in this case, we are going to register retime function, so instead of
> relying on DT to provide gpmc timings for such a peripheral, won't it
> be better to make use of retime that is already registered ?

No we need to pass the timings from device tree as they may be different
for similar boards. For example, different level shifters used on
similar boards may affect the timings, although the retime function
can be the same.

Regards,

Tony

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony,

On Tue, Jul 10, 2012 at 18:47:34, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120710 03:09]:
> > On Tue, Jul 10, 2012 at 15:15:38, Tony Lindgren wrote:
> > > * Mohammed, Afzal <afzal@ti.com> [120709 23:24]:

> > > > For the peripherals requiring retime, we cannot (as otherwise whatever
> > > > retime does would have to be manually done based on the knowledge of
> > > > boot time gpmc clock period to calculate gpmc timings to be fed to DT)
> > > > pass gpmc timings via device tree, right ?
> > > 
> > > We can still do it when the connected peripheral probe registers with
> > > gpmc.

Were you actually referring to updating kernel view of device tree nodes, and
not the device tree file (not sure whether it is really possible) ?

> > We can, but would it be feasible practically ?, gpmc timings to update in
> > DT for a such a peripheral (requiring retime) can be found out only by
> > manual calculation similar to the way done in retime function (based on
> > peripheral's timings and boot time gpmc clock period), correct ?, Also
> > wouldn't this make it necessary to know gpmc clock period at boot time
> > for properly updating gpmc timing entry in DT ?
> 
> The gpmc clock period can be returned to the connected peripheral when
> it's registering. Well basically we can call the retime function upon
> registering and pass the gpmc clock period.

Won't this lead to the necessity of particular driver load order problem ?,
As per the above, to return gpmc clock period to the connected peripheral,
we need to ensure that gpmc driver is probed before peripheral driver.
And in that case how can gpmc driver rely on DT as gpmc timings for the
peripheral requiring retime would not yet be available as peripheral
driver is not yet probed, seems like a circular dependency.

>  
> > And in this case, we are going to register retime function, so instead of
> > relying on DT to provide gpmc timings for such a peripheral, won't it
> > be better to make use of retime that is already registered ?
> 
> No we need to pass the timings from device tree as they may be different
> for similar boards. For example, different level shifters used on
> similar boards may affect the timings, although the retime function
> can be the same.

Unless I am missing something, could not see this scenario taken care
in the existing retime functions, did see one comment for smc91x, but
seems in that case Kernel doesn't do any timing configuration and
leave timings as configured by bootloader

Regards
Afzal

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Jon Hunter]

Hi Afzal,

On 07/10/2012 08:47 AM, Mohammed, Afzal wrote:
> Hi Tony,
> 
> On Tue, Jul 10, 2012 at 18:47:34, Tony Lindgren wrote:
>> * Mohammed, Afzal <afzal@ti.com> [120710 03:09]:
>>> On Tue, Jul 10, 2012 at 15:15:38, Tony Lindgren wrote:
>>>> * Mohammed, Afzal <afzal@ti.com> [120709 23:24]:
> 
>>>>> For the peripherals requiring retime, we cannot (as otherwise whatever
>>>>> retime does would have to be manually done based on the knowledge of
>>>>> boot time gpmc clock period to calculate gpmc timings to be fed to DT)
>>>>> pass gpmc timings via device tree, right ?
>>>>
>>>> We can still do it when the connected peripheral probe registers with
>>>> gpmc.
> 
> Were you actually referring to updating kernel view of device tree nodes, and
> not the device tree file (not sure whether it is really possible) ?

I believe that Tony is suggesting performing the retime at probe time.
In the case of OneNand you would simply call the
onenand_set_async_mode/sync_mode from within the probe as a retime
function.

>>> We can, but would it be feasible practically ?, gpmc timings to update in
>>> DT for a such a peripheral (requiring retime) can be found out only by
>>> manual calculation similar to the way done in retime function (based on
>>> peripheral's timings and boot time gpmc clock period), correct ?, Also
>>> wouldn't this make it necessary to know gpmc clock period at boot time
>>> for properly updating gpmc timing entry in DT ?
>>
>> The gpmc clock period can be returned to the connected peripheral when
>> it's registering. Well basically we can call the retime function upon
>> registering and pass the gpmc clock period.
> 
> Won't this lead to the necessity of particular driver load order problem ?,
> As per the above, to return gpmc clock period to the connected peripheral,
> we need to ensure that gpmc driver is probed before peripheral driver.

I think it is more like when probing the gpmc, the retime function for
the connected peripheral is called passing the gpmc clock freq.

> And in that case how can gpmc driver rely on DT as gpmc timings for the
> peripheral requiring retime would not yet be available as peripheral
> driver is not yet probed, seems like a circular dependency.

The DT node should simply have the information required by the retime
function or gpmc timings themselves if available. In the case of OneNAND
async mode you have a bunch of constants such as ...

const int t_cer  = 15;
const int t_avdp = 12;
const int t_cez  = 20; /* max of t_cez, t_oez */
const int t_ds   = 30;
const int t_wpl  = 40;
const int t_wph  = 30

These can be stored in the DT and then translated to gpmc timings at
runtime. DT should only store static timing or clock information known
at compile time.

>>> And in this case, we are going to register retime function, so instead of
>>> relying on DT to provide gpmc timings for such a peripheral, won't it
>>> be better to make use of retime that is already registered ?
>>
>> No we need to pass the timings from device tree as they may be different
>> for similar boards. For example, different level shifters used on
>> similar boards may affect the timings, although the retime function
>> can be the same.
> 
> Unless I am missing something, could not see this scenario taken care
> in the existing retime functions, did see one comment for smc91x, but
> seems in that case Kernel doesn't do any timing configuration and
> leave timings as configured by bootloader

I think that we just need to adapt the current functions that our
calculating the timings so that ...

1. We can call from them within the gpmc probe to setup the timings
versus having the peripherals program the gpmc priory to probe.
2. Any static timing information needed by the retime function is part
of the platform data passed to the gpmc probe and therefore can also be
read from DT.

>From a high-level I think that the goal should be ...

gpmc_probe
	--> request CS
	--> calls retime function to calculate gpmc timing (optional)
	--> configures CS
	--> registers peripheral device

Cheers
Jon

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [120710 10:20]:
> Hi Afzal,
> 
> On 07/10/2012 08:47 AM, Mohammed, Afzal wrote:
> > Hi Tony,
> > 
> > On Tue, Jul 10, 2012 at 18:47:34, Tony Lindgren wrote:
> >> * Mohammed, Afzal <afzal@ti.com> [120710 03:09]:
> >>> On Tue, Jul 10, 2012 at 15:15:38, Tony Lindgren wrote:
> >>>> * Mohammed, Afzal <afzal@ti.com> [120709 23:24]:
> > 
> >>>>> For the peripherals requiring retime, we cannot (as otherwise whatever
> >>>>> retime does would have to be manually done based on the knowledge of
> >>>>> boot time gpmc clock period to calculate gpmc timings to be fed to DT)
> >>>>> pass gpmc timings via device tree, right ?
> >>>>
> >>>> We can still do it when the connected peripheral probe registers with
> >>>> gpmc.
> > 
> > Were you actually referring to updating kernel view of device tree nodes, and
> > not the device tree file (not sure whether it is really possible) ?
> 
> I believe that Tony is suggesting performing the retime at probe time.
> In the case of OneNand you would simply call the
> onenand_set_async_mode/sync_mode from within the probe as a retime
> function.

Yes there's no need to do it earlier.
 
> >>> We can, but would it be feasible practically ?, gpmc timings to update in
> >>> DT for a such a peripheral (requiring retime) can be found out only by
> >>> manual calculation similar to the way done in retime function (based on
> >>> peripheral's timings and boot time gpmc clock period), correct ?, Also
> >>> wouldn't this make it necessary to know gpmc clock period at boot time
> >>> for properly updating gpmc timing entry in DT ?
> >>
> >> The gpmc clock period can be returned to the connected peripheral when
> >> it's registering. Well basically we can call the retime function upon
> >> registering and pass the gpmc clock period.
> > 
> > Won't this lead to the necessity of particular driver load order problem ?,
> > As per the above, to return gpmc clock period to the connected peripheral,
> > we need to ensure that gpmc driver is probed before peripheral driver.
> 
> I think it is more like when probing the gpmc, the retime function for
> the connected peripheral is called passing the gpmc clock freq.

Right, there's no need to do any of that at gpmc probe time. And the
module dependencies take care of the load ordrer as gpmc_cs_request()
is exported.
 
> > And in that case how can gpmc driver rely on DT as gpmc timings for the
> > peripheral requiring retime would not yet be available as peripheral
> > driver is not yet probed, seems like a circular dependency.
> 
> The DT node should simply have the information required by the retime
> function or gpmc timings themselves if available. In the case of OneNAND
> async mode you have a bunch of constants such as ...
> 
> const int t_cer  = 15;
> const int t_avdp = 12;
> const int t_cez  = 20; /* max of t_cez, t_oez */
> const int t_ds   = 30;
> const int t_wpl  = 40;
> const int t_wph  = 30
> 
> These can be stored in the DT and then translated to gpmc timings at
> runtime. DT should only store static timing or clock information known
> at compile time.

Yup. And the format of the timing data in DT should be standardized so
the only differences for each connected peripheral is the retime function.
 
> >>> And in this case, we are going to register retime function, so instead of
> >>> relying on DT to provide gpmc timings for such a peripheral, won't it
> >>> be better to make use of retime that is already registered ?
> >>
> >> No we need to pass the timings from device tree as they may be different
> >> for similar boards. For example, different level shifters used on
> >> similar boards may affect the timings, although the retime function
> >> can be the same.
> > 
> > Unless I am missing something, could not see this scenario taken care
> > in the existing retime functions, did see one comment for smc91x, but
> > seems in that case Kernel doesn't do any timing configuration and
> > leave timings as configured by bootloader
> 
> I think that we just need to adapt the current functions that our
> calculating the timings so that ...
> 
> 1. We can call from them within the gpmc probe to setup the timings
> versus having the peripherals program the gpmc priory to probe.
> 2. Any static timing information needed by the retime function is part
> of the platform data passed to the gpmc probe and therefore can also be
> read from DT.

Yup, and also:

3. Disable frequency scaling for L3 if no retime function is specified

In that case we may have a generic default function that just sets the
boot time values and disables the L3 scaling.
 
> From a high-level I think that the goal should be ...
> 
> gpmc_probe
> 	--> request CS
> 	--> calls retime function to calculate gpmc timing (optional)
> 	--> configures CS
> 	--> registers peripheral device

Yes with few additions.. Connected peripheral probe requests CS from
gpmc with the optional retime function pointer passed as a parameter.
After gpmc code has determined the CS is available, it calls the optional
retime function before returning back to the connected peripheral probe.

So how about the following with a bit more details:

gpmc_probe
	--> just sets up gpmc resources then idles itself

connected peripheral probe
	--> calls gpmc_cs_request() with gpmc timings from DT and an
	    optional retime function as a parameter
	--> gpmc_cs_request() allocates the CS
	--> gpmc_cs_request() calls the optional retime function and
	    if not specified, just sets the DT timings and disables
	    L3 DFS
	--> gpmc_cs_request() returns to connected peripheral probe

Regards,

Tony

[PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration

[Mohammed, Afzal]

Hi Tony, Jon,

Thanks for your explanations, ideas & suggestions.

Let me try to come up with a solution based on these.

Regards
Afzal


On Wed, Jul 11, 2012 at 12:17:25, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [120710 10:20]:
> > Hi Afzal,
> > 
> > On 07/10/2012 08:47 AM, Mohammed, Afzal wrote:
> > > Hi Tony,
> > > 
> > > On Tue, Jul 10, 2012 at 18:47:34, Tony Lindgren wrote:
> > >> * Mohammed, Afzal <afzal@ti.com> [120710 03:09]:
> > >>> On Tue, Jul 10, 2012 at 15:15:38, Tony Lindgren wrote:
> > >>>> * Mohammed, Afzal <afzal@ti.com> [120709 23:24]:
> > > 
> > >>>>> For the peripherals requiring retime, we cannot (as otherwise whatever
> > >>>>> retime does would have to be manually done based on the knowledge of
> > >>>>> boot time gpmc clock period to calculate gpmc timings to be fed to DT)
> > >>>>> pass gpmc timings via device tree, right ?
> > >>>>
> > >>>> We can still do it when the connected peripheral probe registers with
> > >>>> gpmc.
> > > 
> > > Were you actually referring to updating kernel view of device tree nodes, and
> > > not the device tree file (not sure whether it is really possible) ?
> > 
> > I believe that Tony is suggesting performing the retime at probe time.
> > In the case of OneNand you would simply call the
> > onenand_set_async_mode/sync_mode from within the probe as a retime
> > function.
> 
> Yes there's no need to do it earlier.
>  
> > >>> We can, but would it be feasible practically ?, gpmc timings to update in
> > >>> DT for a such a peripheral (requiring retime) can be found out only by
> > >>> manual calculation similar to the way done in retime function (based on
> > >>> peripheral's timings and boot time gpmc clock period), correct ?, Also
> > >>> wouldn't this make it necessary to know gpmc clock period at boot time
> > >>> for properly updating gpmc timing entry in DT ?
> > >>
> > >> The gpmc clock period can be returned to the connected peripheral when
> > >> it's registering. Well basically we can call the retime function upon
> > >> registering and pass the gpmc clock period.
> > > 
> > > Won't this lead to the necessity of particular driver load order problem ?,
> > > As per the above, to return gpmc clock period to the connected peripheral,
> > > we need to ensure that gpmc driver is probed before peripheral driver.
> > 
> > I think it is more like when probing the gpmc, the retime function for
> > the connected peripheral is called passing the gpmc clock freq.
> 
> Right, there's no need to do any of that at gpmc probe time. And the
> module dependencies take care of the load ordrer as gpmc_cs_request()
> is exported.
>  
> > > And in that case how can gpmc driver rely on DT as gpmc timings for the
> > > peripheral requiring retime would not yet be available as peripheral
> > > driver is not yet probed, seems like a circular dependency.
> > 
> > The DT node should simply have the information required by the retime
> > function or gpmc timings themselves if available. In the case of OneNAND
> > async mode you have a bunch of constants such as ...
> > 
> > const int t_cer  = 15;
> > const int t_avdp = 12;
> > const int t_cez  = 20; /* max of t_cez, t_oez */
> > const int t_ds   = 30;
> > const int t_wpl  = 40;
> > const int t_wph  = 30
> > 
> > These can be stored in the DT and then translated to gpmc timings at
> > runtime. DT should only store static timing or clock information known
> > at compile time.
> 
> Yup. And the format of the timing data in DT should be standardized so
> the only differences for each connected peripheral is the retime function.
>  
> > >>> And in this case, we are going to register retime function, so instead of
> > >>> relying on DT to provide gpmc timings for such a peripheral, won't it
> > >>> be better to make use of retime that is already registered ?
> > >>
> > >> No we need to pass the timings from device tree as they may be different
> > >> for similar boards. For example, different level shifters used on
> > >> similar boards may affect the timings, although the retime function
> > >> can be the same.
> > > 
> > > Unless I am missing something, could not see this scenario taken care
> > > in the existing retime functions, did see one comment for smc91x, but
> > > seems in that case Kernel doesn't do any timing configuration and
> > > leave timings as configured by bootloader
> > 
> > I think that we just need to adapt the current functions that our
> > calculating the timings so that ...
> > 
> > 1. We can call from them within the gpmc probe to setup the timings
> > versus having the peripherals program the gpmc priory to probe.
> > 2. Any static timing information needed by the retime function is part
> > of the platform data passed to the gpmc probe and therefore can also be
> > read from DT.
> 
> Yup, and also:
> 
> 3. Disable frequency scaling for L3 if no retime function is specified
> 
> In that case we may have a generic default function that just sets the
> boot time values and disables the L3 scaling.
>  
> > From a high-level I think that the goal should be ...
> > 
> > gpmc_probe
> > 	--> request CS
> > 	--> calls retime function to calculate gpmc timing (optional)
> > 	--> configures CS
> > 	--> registers peripheral device
> 
> Yes with few additions.. Connected peripheral probe requests CS from
> gpmc with the optional retime function pointer passed as a parameter.
> After gpmc code has determined the CS is available, it calls the optional
> retime function before returning back to the connected peripheral probe.
> 
> So how about the following with a bit more details:
> 
> gpmc_probe
> 	--> just sets up gpmc resources then idles itself
> 
> connected peripheral probe
> 	--> calls gpmc_cs_request() with gpmc timings from DT and an
> 	    optional retime function as a parameter
> 	--> gpmc_cs_request() allocates the CS
> 	--> gpmc_cs_request() calls the optional retime function and
> 	    if not specified, just sets the DT timings and disables
> 	    L3 DFS
> 	--> gpmc_cs_request() returns to connected peripheral probe
> 
> Regards,
> 
> Tony
> 

gpmc generic retime function (subject was RE: [PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration)

[Mohammed, Afzal]

Hi Tony, Jon,

On Wed, Jul 11, 2012 at 12:17:25, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [120710 10:20]:

> > The DT node should simply have the information required by the retime
> > function or gpmc timings themselves if available. In the case of OneNAND

> > These can be stored in the DT and then translated to gpmc timings at
> > runtime. DT should only store static timing or clock information known

> Yup. And the format of the timing data in DT should be standardized so
> the only differences for each connected peripheral is the retime function.

If we are able to achieve a generic retime function applicable to all
peripherals then we don't need wrapper layer for retime handling or two
linux devices and drivers (one the existing and the other to handle retime)
to represent a single physical gpmc peripheral device (for DT conversion).
Then handling core frequency scaling and DT conversion would be easier.
We were trying to create such a retime function that would be generic so
as to handle different types of gpmc peripherals.

And we have been able to create such a function. Below is an implementation
that has been made for handling asynchronous timings. It has been tested for
OneNAND & SMSC on OMAP3EVM (rev G & C) with [1-4]. OneNAND was tested using
[5] (OMAP3EVM OneNAND works in async mode) & SMSC using [6] (mainline does
not have a timing calculation for smsc911x)

It was difficult to squeeze tusb6010 timing calculation into generic timing
calculation, hence a boolean "tusb" has been used. This is what I could
achieve based on existing retime for tusb6010 and for lack of tusb6010
timing specifications.

----8<-----------------------------------------------------------------------

/* Device timings in picoseconds */
struct gpmc_device_timings {
        u32     cs_setup;       /* CS setup time */
        u32     adv_setup;      /* ADV setup time */
        u32     adv_rd_off;     /* ADV read off time */
        u32     adv_add_hold;   /* address hold time */
        u32     oe_setup;       /* OE setup time */
        u32     adv_access;     /* access time from ADV assertion */
        u32     rd_access;      /* read access time */
        u32     oe_access;      /* access time from OE assertion */
        u32     cs_access;      /* access time from CS asertion */
        u32     rd_cycle;       /* read cycle time */
        u32     cs_highz;       /* CS deassertion to high Z */
        u32     oe_highz;       /* OE deassertion to high Z */
        u32     adv_wr_off;     /* ADV write off time */
        u32     we_setup;       /* WE setup time */
        u32     wr_pulse;       /* write assertion time */
        u32     wr_data_setup;  /* data setup time from write assertion */
        u32     wr_high;        /* write deassertion time */
        u32     we_highz;       /* WE deassertion to high Z */
        u32     wr_cycle;       /* write cycle time */

        bool    mux;            /* address & data muxed */
        bool    tusb;           /* peripheral is tusb6010 */
};

struct gpmc_timings gpmc_calc_timings(struct gpmc_device_timings *dev_t)
{
        struct gpmc_timings gpmc_t;
        bool mux = dev_t->mux;
        bool tusb = dev_t->tusb;
        u32 temp;

        memset(&gpmc_t, 0, sizeof(gpmc_t));

        /* cs_on */
        gpmc_t.cs_on = gpmc_round_ns_to_ticks(dev_t->cs_setup / 1000);

        /* adv_on */
        temp = dev_t->adv_setup;
        if (tusb)
                temp = max_t(u32,
                        (gpmc_t.cs_on + gpmc_ticks_to_ns(1)) * 1000, temp);
        gpmc_t.adv_on = gpmc_round_ns_to_ticks(temp / 1000);

        /* adv_rd_off */
        temp = dev_t->adv_rd_off;
        if (tusb)
                temp = max_t(u32,
                        (gpmc_t.adv_on + gpmc_ticks_to_ns(1)) * 1000, temp);
        gpmc_t.adv_rd_off = gpmc_round_ns_to_ticks(temp / 1000);

        /* oe_on */
        if (mux)
                temp = gpmc_t.adv_rd_off * 1000 + dev_t->adv_add_hold;
        else
                temp = dev_t->oe_setup;
        if (tusb)
                temp = max_t(u32,
                        (gpmc_t.adv_rd_off + gpmc_ticks_to_ns(1)) * 1000, temp);
        gpmc_t.oe_on = gpmc_round_ns_to_ticks(temp / 1000);

        /* access */
        temp = max_t(u32, dev_t->rd_access,
                                gpmc_t.oe_on * 1000 + dev_t->oe_access);
        temp = max_t(u32, temp,
                                gpmc_t.cs_on * 1000 + dev_t->cs_access);
        temp = max_t(u32, temp,
                                gpmc_t.adv_on * 1000 + dev_t->adv_access);
        if (tusb) {
                temp = max_t(u32, temp,
                                (gpmc_t.oe_on + gpmc_ticks_to_ns(1)) * 1000);
                temp = max_t(u32, temp, gpmc_t.oe_on * 1000 + 300);
        }
        gpmc_t.access = gpmc_round_ns_to_ticks(temp / 1000);

        gpmc_t.oe_off = gpmc_t.access + gpmc_ticks_to_ns(1);
        gpmc_t.cs_rd_off = gpmc_t.oe_off;

        /* rd_cycle */
        temp = max_t(u32, dev_t->rd_cycle,
                        gpmc_t.cs_rd_off * 1000 + dev_t->cs_highz);
        temp = max_t(u32, temp,
                        gpmc_t.oe_off * 1000 + dev_t->oe_highz);
        if (tusb) {
                temp = max_t(u32, temp,
                        (gpmc_t.cs_rd_off + gpmc_ticks_to_ns(1)) * 1000);
                temp = max_t(u32, temp, gpmc_t.cs_rd_off * 1000 + 7000);
        }
        gpmc_t.rd_cycle = gpmc_round_ns_to_ticks(temp / 1000);

        /* adv_wr_off */
        temp = dev_t->adv_wr_off;
        if (tusb)
                temp = max_t(u32,
                        (gpmc_t.adv_on + gpmc_ticks_to_ns(1)) * 1000, temp);
        gpmc_t.adv_wr_off = gpmc_round_ns_to_ticks(temp / 1000);

        /* we_on */
        if (mux)
                temp = gpmc_t.adv_wr_off * 1000 + dev_t->adv_add_hold;
        else
                temp = dev_t->we_setup;
        if (tusb)
                temp = max_t(u32,
                        (gpmc_t.adv_wr_off + gpmc_ticks_to_ns(1)) * 1000, temp);
        gpmc_t.we_on = gpmc_round_ns_to_ticks(temp / 1000);

        gpmc_t.wr_data_mux_bus = gpmc_t.we_on;

        /* we_off */
        temp = max_t(u32, gpmc_t.we_on * 1000 + dev_t->wr_pulse,
                        gpmc_t.we_on * 1000 + dev_t->wr_data_setup);
        if (tusb) {
                temp = max_t(u32, temp,
                                (gpmc_t.we_on + gpmc_ticks_to_ns(1)) * 1000);
                temp = max_t(u32, temp, gpmc_t.we_on * 1000 + 300);
        }
        gpmc_t.we_off = gpmc_round_ns_to_ticks(temp / 1000);

        gpmc_t.cs_wr_off = gpmc_round_ns_to_ticks((gpmc_t.we_off * 1000 +
                                dev_t->wr_high) / 1000);

        /* wr_cycle */
        temp = max_t(u32, dev_t->wr_cycle,
                        gpmc_t.cs_wr_off * 1000 + dev_t->cs_highz);
        temp = max_t(u32, temp,
                        gpmc_t.we_off * 1000 + dev_t->we_highz);
        if (tusb) {
                temp = max_t(u32, temp,
                        (gpmc_t.cs_wr_off + gpmc_ticks_to_ns(1)) * 1000);
                temp = max_t(u32, temp, gpmc_t.cs_wr_off * 1000 + 7000);
        }
        gpmc_t.wr_cycle = gpmc_round_ns_to_ticks(temp / 1000);

        return gpmc_t;
}

> > From a high-level I think that the goal should be ...
> > 
> > gpmc_probe
> > 	--> request CS
> > 	--> calls retime function to calculate gpmc timing (optional)
> > 	--> configures CS
> > 	--> registers peripheral device
> 
> Yes with few additions.. Connected peripheral probe requests CS from
> gpmc with the optional retime function pointer passed as a parameter.
> After gpmc code has determined the CS is available, it calls the optional
> retime function before returning back to the connected peripheral probe.
> 
> So how about the following with a bit more details:
> 
> gpmc_probe
> 	--> just sets up gpmc resources then idles itself
> 
> connected peripheral probe
> 	--> calls gpmc_cs_request() with gpmc timings from DT and an
> 	    optional retime function as a parameter
> 	--> gpmc_cs_request() allocates the CS
> 	--> gpmc_cs_request() calls the optional retime function and
> 	    if not specified, just sets the DT timings and disables
> 	    L3 DFS
> 	--> gpmc_cs_request() returns to connected peripheral probe

Once we are able to use the generic retime function as mentioned above,
instead of requiring additional driver for each peripheral that has
retime and its effect on complicating DT conversion,  we can have the
gpmc driver organization similar to the one that was posted
earlier[4].

With DT, we can pass peripheral timings that has been generalized as in 
gpmc_device_timings structure. In addition to timings, we would need
to have fields for specifying whether multiplexed or not ("mux" in
struct gpmc_device_timings), asynchronous or synchronous.

With the generic retime function, driver could do as follows,

gpmc_probe
  -> setup_device
    -> setup_cs
      ->setup_cs_config_timing
		if (gpmc_timings)
			gpmc_cs_set_timings(gpmc_t);
		else if (gpmc_device_timings) {
			gpmc_t = gpmc_calc_timings();
			gpmc_cs_set_timings(cs, &gpmc_t);
		}


In the above, addition to already posted driver series [4] would
be the "else if" portions.

For core DFS, we could register a notifier that would invoke,
	gpmc_t = gpmc_calc_timings(); /* defined above */
	gpmc_cs_set-timings(cs, &gpmc_t);
enabling us to have the gpmc timings adjusted.


Please let me know your comments on the above. Currently I am working
on achieving generalized timing for synchronous operations too so
that gpmc timings can be calculated at runtime using a single generic
retime function for all kinds of operations like mux, sync/async etc.

Regards
Afzal

[1] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg70096.html
[2] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg71027.html
[3] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69881.html
[4] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg70753.html

[5]
diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index bbae674..fab21a1 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -50,6 +50,7 @@ static struct platform_device gpmc_onenand_device = {
 static struct gpmc_timings omap2_onenand_calc_async_timings(void)
 {
        struct gpmc_timings t;
+       struct gpmc_device_timings dev_t;
 
        const int t_cer = 15;
        const int t_avdp = 12;
@@ -63,6 +64,24 @@ static struct gpmc_timings omap2_onenand_calc_async_timings(void)
        const int t_wph = 30;
 
        memset(&t, 0, sizeof(t));
+
+       memset(&dev_t, 0, sizeof(dev_t));
+       dev_t.cs_setup = 0;
+       dev_t.adv_setup = 0;
+       dev_t.adv_rd_off = 15 * 1000;
+       dev_t.adv_add_hold = 7 * 1000;
+       dev_t.adv_access = 76 * 1000;
+       dev_t.oe_access = 20 * 1000;
+       dev_t.cs_access = 76 * 1000;
+       dev_t.cs_highz = 20 * 1000;
+       dev_t.adv_wr_off = 15 * 1000;
+       dev_t.wr_pulse = 40 * 1000;
+       dev_t.wr_high = 30 * 1000;
+
+       dev_t.mux = true;
+
+       return gpmc_calc_timings(&dev_t);
+
        t.sync_clk = 0;
        t.cs_on = 0;
        t.adv_on = 0;

[6]

diff --git a/arch/arm/mach-omap2/gpmc-smsc911x.c b/arch/arm/mach-omap2/gpmc-smsc911x.c
index b6c77be..53c7958 100644
--- a/arch/arm/mach-omap2/gpmc-smsc911x.c
+++ b/arch/arm/mach-omap2/gpmc-smsc911x.c
@@ -39,6 +39,50 @@ static struct smsc911x_platform_config gpmc_smsc911x_config = {
        .irq_type       = SMSC911X_IRQ_TYPE_OPEN_DRAIN,
 };
 
+static void gpmc_smsc911x_timing(struct omap_smsc911x_platform_data *gpmc_cfg)
+{
+       struct gpmc_timings t;
+       /* SMSC 9220 timings */
+       unsigned tcycle_r = 165;
+       unsigned tcsl_r = 32;
+       unsigned tcsh_r = 133;
+       unsigned tcsdv_r = 30;
+       unsigned tdoff_r = 9;
+       unsigned tcycle_w = 165;
+       unsigned tcsl_w = 32;
+       unsigned tcsh_w = 133;
+       unsigned tdsu_w = 7;
+       struct gpmc_device_timings dev_t;
+
+       memset(&t, sizeof(t), 0);
+
+       memset(&dev_t, sizeof(dev_t), 0);
+       dev_t.rd_access = tcsdv_r * 1000;
+       dev_t.rd_cycle = tcycle_r * 1000;
+       dev_t.wr_data_setup = tdsu_w * 1000;
+       dev_t.wr_pulse = tcsl_w * 1000;
+       dev_t.wr_cycle = tcycle_w * 1000;
+
+       t = gpmc_calc_timings(&dev_t);
+       gpmc_cs_set_timings(gpmc_cfg->cs, &t);
+       return;
+
+       t.cs_on = 0;
+       t.oe_on = 0;
+       t.access = tcsdv_r;
+       t.oe_off = max_t(unsigned, t.access + gpmc_ticks_to_ns(1), tcsl_r);
+       t.cs_rd_off = t.oe_off;
+       t.rd_cycle = tcsl_r + max(tcsh_r, tdoff_r);
+       t.rd_cycle = max_t(unsigned, tcycle_r, t.rd_cycle);
+
+       t.we_on = 0;
+       t.we_off = max(tcsl_w, tdsu_w);
+       t.cs_wr_off = t.we_off;
+       t.wr_cycle = max_t(unsigned, t.we_off + gpmc_ticks_to_ns(1), tcycle_w);
+
+       gpmc_cs_set_timings(gpmc_cfg->cs, &t);
+}
+
/*
  * Initialize smsc911x device connected to the GPMC. Note that we
  * assume that pin multiplexing is done in the board-*.c file,
@@ -81,6 +125,8 @@ void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *gpmc_cfg)
 
        gpmc_smsc911x_config.flags = gpmc_cfg->flags ? : SMSC911X_USE_16BIT;
 
+       gpmc_smsc911x_timing(gpmc_cfg);
+
        pdev = platform_device_register_resndata(NULL, "smsc911x", gpmc_cfg->id,
                 gpmc_smsc911x_resources, ARRAY_SIZE(gpmc_smsc911x_resources),
                 &gpmc_smsc911x_config, sizeof(gpmc_smsc911x_config));

gpmc generic retime function (subject was RE: [PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration)

[Jon Hunter]

Hi Afzal,

Sorry for the delay, I have been out of the office.

On 08/06/2012 08:38 AM, Mohammed, Afzal wrote:
> Hi Tony, Jon,
> 
> On Wed, Jul 11, 2012 at 12:17:25, Tony Lindgren wrote:
>> * Jon Hunter <jon-hunter@ti.com> [120710 10:20]:
> 
>>> The DT node should simply have the information required by the retime
>>> function or gpmc timings themselves if available. In the case of OneNAND
> 
>>> These can be stored in the DT and then translated to gpmc timings at
>>> runtime. DT should only store static timing or clock information known
> 
>> Yup. And the format of the timing data in DT should be standardized so
>> the only differences for each connected peripheral is the retime function.
> 
> If we are able to achieve a generic retime function applicable to all
> peripherals then we don't need wrapper layer for retime handling or two
> linux devices and drivers (one the existing and the other to handle retime)
> to represent a single physical gpmc peripheral device (for DT conversion).
> Then handling core frequency scaling and DT conversion would be easier.
> We were trying to create such a retime function that would be generic so
> as to handle different types of gpmc peripherals.

Sounds like a much better approach!

> And we have been able to create such a function. Below is an implementation
> that has been made for handling asynchronous timings. It has been tested for
> OneNAND & SMSC on OMAP3EVM (rev G & C) with [1-4]. OneNAND was tested using
> [5] (OMAP3EVM OneNAND works in async mode) & SMSC using [6] (mainline does
> not have a timing calculation for smsc911x)

Are you able to verify that the timing calculated by this function are
identical? May be some more details on exactly how you tested this would
be good.

> It was difficult to squeeze tusb6010 timing calculation into generic timing
> calculation, hence a boolean "tusb" has been used. This is what I could
> achieve based on existing retime for tusb6010 and for lack of tusb6010
> timing specifications.
> 
> ----8<-----------------------------------------------------------------------
> 
> /* Device timings in picoseconds */
> struct gpmc_device_timings {
>         u32     cs_setup;       /* CS setup time */
>         u32     adv_setup;      /* ADV setup time */
>         u32     adv_rd_off;     /* ADV read off time */
>         u32     adv_add_hold;   /* address hold time */
>         u32     oe_setup;       /* OE setup time */
>         u32     adv_access;     /* access time from ADV assertion */
>         u32     rd_access;      /* read access time */
>         u32     oe_access;      /* access time from OE assertion */
>         u32     cs_access;      /* access time from CS asertion */
>         u32     rd_cycle;       /* read cycle time */
>         u32     cs_highz;       /* CS deassertion to high Z */
>         u32     oe_highz;       /* OE deassertion to high Z */
>         u32     adv_wr_off;     /* ADV write off time */
>         u32     we_setup;       /* WE setup time */
>         u32     wr_pulse;       /* write assertion time */
>         u32     wr_data_setup;  /* data setup time from write assertion */
>         u32     wr_high;        /* write deassertion time */
>         u32     we_highz;       /* WE deassertion to high Z */
>         u32     wr_cycle;       /* write cycle time */
> 
>         bool    mux;            /* address & data muxed */
>         bool    tusb;           /* peripheral is tusb6010 */

Do you think that there is any value in making the tusb member a "u32
dev_type" and then set it too GPMC_DEVICE_TUSB then this could be used
for other devices in the future too if needed?

> };
> 
> struct gpmc_timings gpmc_calc_timings(struct gpmc_device_timings *dev_t)
> {
>         struct gpmc_timings gpmc_t;
>         bool mux = dev_t->mux;
>         bool tusb = dev_t->tusb;
>         u32 temp;
> 
>         memset(&gpmc_t, 0, sizeof(gpmc_t));
> 
>         /* cs_on */
>         gpmc_t.cs_on = gpmc_round_ns_to_ticks(dev_t->cs_setup / 1000);
> 
>         /* adv_on */
>         temp = dev_t->adv_setup;
>         if (tusb)
>                 temp = max_t(u32,
>                         (gpmc_t.cs_on + gpmc_ticks_to_ns(1)) * 1000, temp);
>         gpmc_t.adv_on = gpmc_round_ns_to_ticks(temp / 1000);

Would it be possible to create a sub-function called
gpmc_calc_timings_tusb() and put all these "if (tusb)" statements in
there? Or maybe a generic function called gpmc_calc_timings_prepare().

For the above case could have ...

void gpmc_calc_timings_prepare(struct gpmc_device_timings *dev_t)
{
	if (dev_t->tusb) {
		dev_t->adv_on = max_t(u32,
		(gpmc_t.cs_on + gpmc_ticks_to_ns(1)) * 1000,
		dev_t->adv_setup);
		...
	} else {
		dev_t->adv_on = dev_t->adv_setup;
		...
	}
}

And then in the gpmc_calc_timings() you would just have ...

         gpmc_t.adv_on = gpmc_round_ns_to_ticks(dev_t->adv_on / 1000);

>         /* adv_rd_off */
>         temp = dev_t->adv_rd_off;
>         if (tusb)
>                 temp = max_t(u32,
>                         (gpmc_t.adv_on + gpmc_ticks_to_ns(1)) * 1000, temp);
>         gpmc_t.adv_rd_off = gpmc_round_ns_to_ticks(temp / 1000);
> 
>         /* oe_on */
>         if (mux)
>                 temp = gpmc_t.adv_rd_off * 1000 + dev_t->adv_add_hold;
>         else
>                 temp = dev_t->oe_setup;
>         if (tusb)
>                 temp = max_t(u32,
>                         (gpmc_t.adv_rd_off + gpmc_ticks_to_ns(1)) * 1000, temp);
>         gpmc_t.oe_on = gpmc_round_ns_to_ticks(temp / 1000);
> 
>         /* access */
>         temp = max_t(u32, dev_t->rd_access,
>                                 gpmc_t.oe_on * 1000 + dev_t->oe_access);
>         temp = max_t(u32, temp,
>                                 gpmc_t.cs_on * 1000 + dev_t->cs_access);
>         temp = max_t(u32, temp,
>                                 gpmc_t.adv_on * 1000 + dev_t->adv_access);
>         if (tusb) {
>                 temp = max_t(u32, temp,
>                                 (gpmc_t.oe_on + gpmc_ticks_to_ns(1)) * 1000);
>                 temp = max_t(u32, temp, gpmc_t.oe_on * 1000 + 300);
>         }
>         gpmc_t.access = gpmc_round_ns_to_ticks(temp / 1000);
> 
>         gpmc_t.oe_off = gpmc_t.access + gpmc_ticks_to_ns(1);
>         gpmc_t.cs_rd_off = gpmc_t.oe_off;
> 
>         /* rd_cycle */
>         temp = max_t(u32, dev_t->rd_cycle,
>                         gpmc_t.cs_rd_off * 1000 + dev_t->cs_highz);
>         temp = max_t(u32, temp,
>                         gpmc_t.oe_off * 1000 + dev_t->oe_highz);
>         if (tusb) {
>                 temp = max_t(u32, temp,
>                         (gpmc_t.cs_rd_off + gpmc_ticks_to_ns(1)) * 1000);
>                 temp = max_t(u32, temp, gpmc_t.cs_rd_off * 1000 + 7000);
>         }
>         gpmc_t.rd_cycle = gpmc_round_ns_to_ticks(temp / 1000);
> 
>         /* adv_wr_off */
>         temp = dev_t->adv_wr_off;
>         if (tusb)
>                 temp = max_t(u32,
>                         (gpmc_t.adv_on + gpmc_ticks_to_ns(1)) * 1000, temp);
>         gpmc_t.adv_wr_off = gpmc_round_ns_to_ticks(temp / 1000);
> 
>         /* we_on */
>         if (mux)
>                 temp = gpmc_t.adv_wr_off * 1000 + dev_t->adv_add_hold;
>         else
>                 temp = dev_t->we_setup;
>         if (tusb)
>                 temp = max_t(u32,
>                         (gpmc_t.adv_wr_off + gpmc_ticks_to_ns(1)) * 1000, temp);
>         gpmc_t.we_on = gpmc_round_ns_to_ticks(temp / 1000);
> 
>         gpmc_t.wr_data_mux_bus = gpmc_t.we_on;
> 
>         /* we_off */
>         temp = max_t(u32, gpmc_t.we_on * 1000 + dev_t->wr_pulse,
>                         gpmc_t.we_on * 1000 + dev_t->wr_data_setup);
>         if (tusb) {
>                 temp = max_t(u32, temp,
>                                 (gpmc_t.we_on + gpmc_ticks_to_ns(1)) * 1000);
>                 temp = max_t(u32, temp, gpmc_t.we_on * 1000 + 300);
>         }
>         gpmc_t.we_off = gpmc_round_ns_to_ticks(temp / 1000);
> 
>         gpmc_t.cs_wr_off = gpmc_round_ns_to_ticks((gpmc_t.we_off * 1000 +
>                                 dev_t->wr_high) / 1000);
> 
>         /* wr_cycle */
>         temp = max_t(u32, dev_t->wr_cycle,
>                         gpmc_t.cs_wr_off * 1000 + dev_t->cs_highz);
>         temp = max_t(u32, temp,
>                         gpmc_t.we_off * 1000 + dev_t->we_highz);
>         if (tusb) {
>                 temp = max_t(u32, temp,
>                         (gpmc_t.cs_wr_off + gpmc_ticks_to_ns(1)) * 1000);
>                 temp = max_t(u32, temp, gpmc_t.cs_wr_off * 1000 + 7000);
>         }
>         gpmc_t.wr_cycle = gpmc_round_ns_to_ticks(temp / 1000);
> 
>         return gpmc_t;
> }
> 
>>> From a high-level I think that the goal should be ...
>>>
>>> gpmc_probe
>>> 	--> request CS
>>> 	--> calls retime function to calculate gpmc timing (optional)
>>> 	--> configures CS
>>> 	--> registers peripheral device
>>
>> Yes with few additions.. Connected peripheral probe requests CS from
>> gpmc with the optional retime function pointer passed as a parameter.
>> After gpmc code has determined the CS is available, it calls the optional
>> retime function before returning back to the connected peripheral probe.
>>
>> So how about the following with a bit more details:
>>
>> gpmc_probe
>> 	--> just sets up gpmc resources then idles itself
>>
>> connected peripheral probe
>> 	--> calls gpmc_cs_request() with gpmc timings from DT and an
>> 	    optional retime function as a parameter
>> 	--> gpmc_cs_request() allocates the CS
>> 	--> gpmc_cs_request() calls the optional retime function and
>> 	    if not specified, just sets the DT timings and disables
>> 	    L3 DFS
>> 	--> gpmc_cs_request() returns to connected peripheral probe
> 
> Once we are able to use the generic retime function as mentioned above,
> instead of requiring additional driver for each peripheral that has
> retime and its effect on complicating DT conversion,  we can have the
> gpmc driver organization similar to the one that was posted
> earlier[4].
> 
> With DT, we can pass peripheral timings that has been generalized as in 
> gpmc_device_timings structure. In addition to timings, we would need
> to have fields for specifying whether multiplexed or not ("mux" in
> struct gpmc_device_timings), asynchronous or synchronous.
> 
> With the generic retime function, driver could do as follows,
> 
> gpmc_probe
>   -> setup_device
>     -> setup_cs
>       ->setup_cs_config_timing
> 		if (gpmc_timings)
> 			gpmc_cs_set_timings(gpmc_t);
> 		else if (gpmc_device_timings) {
> 			gpmc_t = gpmc_calc_timings();
> 			gpmc_cs_set_timings(cs, &gpmc_t);
> 		}
> 

I will let Tony comment on how he would like the above to work.

> In the above, addition to already posted driver series [4] would
> be the "else if" portions.
> 
> For core DFS, we could register a notifier that would invoke,
> 	gpmc_t = gpmc_calc_timings(); /* defined above */
> 	gpmc_cs_set-timings(cs, &gpmc_t);
> enabling us to have the gpmc timings adjusted.
> 
> 
> Please let me know your comments on the above. Currently I am working
> on achieving generalized timing for synchronous operations too so
> that gpmc timings can be calculated at runtime using a single generic
> retime function for all kinds of operations like mux, sync/async etc.

Great!

Cheers
Jon

gpmc generic retime function (subject was RE: [PATCH v5 3/3] ARM: OMAP2+: onenand: prepare for gpmc driver migration)

[Mohammed, Afzal]

Hi Jon,

On Fri, Aug 17, 2012 at 20:32:34, Hunter, Jon wrote:

> > And we have been able to create such a function. Below is an implementation
> > that has been made for handling asynchronous timings. It has been tested for
> > OneNAND & SMSC on OMAP3EVM (rev G & C) with [1-4]. OneNAND was tested using
> > [5] (OMAP3EVM OneNAND works in async mode) & SMSC using [6] (mainline does
> > not have a timing calculation for smsc911x)
> 
> Are you able to verify that the timing calculated by this function are
> identical? May be some more details on exactly how you tested this would
> be good.

Yes, it was verified.

A new driver preparation series has been posted,
	http://marc.info/?l=linux-omap&m=134554573104116&w=2
that includes generic timing calculation method.

The new series mentions how timing values were validated. There are a
couple of timing parameters that would vary as mentioned in the above
mentioned mail, but these I don't expect to create problems as this is 
more inline with gpmc peripheral timings. And both of the these has
been verified that it would not create problem with peripheral
functionality. One was tested directly (we_on related for OneNAND) and
other indirectly (adv_rd(wr)_off on SMSC 9220 for SMSC 91C96).

Reason for doing so was that quirks required to handle these specific
cases could be avoided, otherwise new peripheral timing data would be
required and it would be difficult to achieve DT bindings (when DT
happens) for these kind of fixup timings.

But if this causes any problems (which I don't expect), then we will
have to fallback to the quirks that I wanted to avoid.

> Do you think that there is any value in making the tusb member a "u32
> dev_type" and then set it too GPMC_DEVICE_TUSB then this could be used
> for other devices in the future too if needed?

> Would it be possible to create a sub-function called
> gpmc_calc_timings_tusb() and put all these "if (tusb)" statements in
> there? Or maybe a generic function called gpmc_calc_timings_prepare().

Usage of a "tusb" check was something that I really wanted to avoid as
that was making generic timing calculation function peripheral Gnostic.
With the newly posted series, "tusb" field has been removed.

Regards
Afzal

[PATCH v5 0/3] Prepare for GPMC driver conversion

[Mohammed, Afzal]

Hi Tony,

On Wed, Jun 27, 2012 at 12:03:07, Mohammed, Afzal wrote:

> Objective of this series is to make things easy for GPMC driver
> conversion series by separating out more things from driver
> conversion series.
> 
> This series,
> 1. Unifies NAND platform initialization functions
> 2. Prepares OneNAND platform code for gpmc driver migration
> 3. Handles additional timings in Kernel
> 
> This series is based on 3.5-rc1 & made on top of
> [PATCH v2 00/10] Prepare for GPMC driver conversion (w.r.t MTD)
> {http://www.mail-archive.com/linux-omap at vger.kernel.org/msg70096.html}
> 
> These changes has been tested with omap3evm & beagle board. Relevant
> GPMC peripherals that got tested by this were NAND (beagle) and
> OneNAND (using local patches for omap3evm)

All of Jon's comments in this series has been taken care and has
Reviewed-by from him on 2 patches, can you please take this series

Regards
Afzal