[PATCH 0/3] Prepare for GPMC driver conversion

[PATCH 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. Cleans up OneNAND platform code a little
3. Handles additional timings in Kernel

This series is based on 3.5-rc1 & made on top of
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69501.html
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69881.html

Regards
Afzal

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

 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         |   24 ++++++---------
 arch/arm/mach-omap2/gpmc.c                 |    6 ++++
 arch/arm/plat-omap/include/plat/gpmc.h     |    6 ++++
 14 files changed, 77 insertions(+), 99 deletions(-)

-- 
1.7.10.2

[PATCH 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>
---
 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 1/3] ARM: OMAP2+: nand: unify init functions

[Jon Hunter]

Hi Afzal,

On 06/11/2012 09:01 AM, Afzal Mohammed wrote:
> 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.

[1] also confirms the omap3-touchbook uses CS0.

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

Which boards have been tested with this change?

Otherwise, looks good.

Reviewed-by: Jon Hunter <jon-hunter@ti.com>

Cheers
Jon

[1] http://www.alwaysinnovating.com/wiki/index.php/Booting

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

[Mohammed, Afzal]

Hi Jon,

On Mon, Jun 11, 2012 at 21:13:45, Hunter, Jon wrote:

> Which boards have been tested with this change?

Beagle board

> Reviewed-by: Jon Hunter <jon-hunter@ti.com>

Thanks

Regards
Afzal

[PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion

[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
expect to work properly unless GPMC is setup, this has been
removed. Two instances of doing the same has been clubbed
into one as onenand driver always calls indirectly
"omap2_onenand_set_sync_mode", and has placed such that
configuring onenand for async read/write would happen once
GPMC is setup for async mode (which happens even for sync
mode, before configuring to sync mode).

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 arch/arm/mach-omap2/gpmc-onenand.c |   24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index 71d7c07..fd4c48d 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -38,10 +38,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 int omap2_onenand_set_async_mode(int cs)
 {
 	struct gpmc_timings t;
-	u32 reg;
 	int err;
 
 	const int t_cer = 15;
@@ -55,11 +54,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;
@@ -95,10 +89,6 @@ static int omap2_onenand_set_async_mode(int cs, void __iomem *onenand_base)
 	if (err)
 		return err;
 
-	/* 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;
 }
@@ -191,19 +181,21 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
 	u32 reg;
 	bool clk_dep = false;
 
+	/* 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);
+
 	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);
+		return 0;
 
 	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;
 	}
@@ -421,6 +413,8 @@ void __init gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
 	gpmc_onenand_resource.end = gpmc_onenand_resource.start +
 							ONENAND_IO_SIZE - 1;
 
+	omap2_onenand_set_async_mode(gpmc_onenand_data->cs);
+
 	if (platform_device_register(&gpmc_onenand_device) < 0) {
 		pr_err("%s: Unable to register OneNAND device\n", __func__);
 		gpmc_cs_free(gpmc_onenand_data->cs);
-- 
1.7.10.2

[PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion

[Jon Hunter]

Hi Afzal,

On 06/11/2012 09:01 AM, Afzal Mohammed wrote:
> 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
> expect to work properly unless GPMC is setup, this has been
> removed. 

So I think I see what you are saying, but the above is not 100% clear. I
think that what you are saying is that omap2_onenand_set_async_mode() is
attempting to configure the onenand registers before configuring the
gpmc interface and hence this is not correct.

> Two instances of doing the same has been clubbed
> into one as onenand driver always calls indirectly
> "omap2_onenand_set_sync_mode", and has placed such that
> configuring onenand for async read/write would happen once
> GPMC is setup for async mode (which happens even for sync
> mode, before configuring to sync mode).

It may be clearer to say that _set_sync_mode is always called during
onenand setup and this will call _set_async_mode. So instead of calling
_set_async_mode from _set_sync_mode, just call it directly during the
gpmc_onenand_init().

> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> ---
>  arch/arm/mach-omap2/gpmc-onenand.c |   24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> index 71d7c07..fd4c48d 100644
> --- a/arch/arm/mach-omap2/gpmc-onenand.c
> +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> @@ -38,10 +38,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 int omap2_onenand_set_async_mode(int cs)
>  {
>  	struct gpmc_timings t;
> -	u32 reg;
>  	int err;
>  
>  	const int t_cer = 15;
> @@ -55,11 +54,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;
> @@ -95,10 +89,6 @@ static int omap2_onenand_set_async_mode(int cs, void __iomem *onenand_base)
>  	if (err)
>  		return err;
>  
> -	/* 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);

I agree with getting rid of the first instance at the beginning of
_set_async_mode, but why get rid of the above one? Are you assuming that
by default it is in async mode? Could be nice to keep it to be explicit.

>  	return 0;
>  }
> @@ -191,19 +181,21 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
>  	u32 reg;
>  	bool clk_dep = false;
>  
> +	/* 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);
> +

Should we only set these after we have checked the flags and depending
on which flags are set?

>  	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);
> +		return 0;
>  
>  	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;
>  	}
> @@ -421,6 +413,8 @@ void __init gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
>  	gpmc_onenand_resource.end = gpmc_onenand_resource.start +
>  							ONENAND_IO_SIZE - 1;
>  
> +	omap2_onenand_set_async_mode(gpmc_onenand_data->cs);
> +

What about putting this in the gpmc_onenand_setup() function before the
call to _set_sync_mode? Seems nice to have these together.

Cheers
Jon

[PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion

[Mohammed, Afzal]

Hi Jon,

On Tue, Jun 12, 2012 at 00:06:30, Hunter, Jon wrote:

> I agree with getting rid of the first instance at the beginning of
> _set_async_mode, but why get rid of the above one? Are you assuming that
> by default it is in async mode? Could be nice to keep it to be explicit.

Second one is achieved below

> > @@ -191,19 +181,21 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
> >  	u32 reg;
> >  	bool clk_dep = false;
> >  
> > +	/* 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);
> > +
> 
> Should we only set these after we have checked the flags and depending
> on which flags are set?

Even for sync mode, setting async mode initially is required

> > @@ -421,6 +413,8 @@ void __init gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
> >  	gpmc_onenand_resource.end = gpmc_onenand_resource.start +
> >  							ONENAND_IO_SIZE - 1;
> >  
> > +	omap2_onenand_set_async_mode(gpmc_onenand_data->cs);
> > +
> 
> What about putting this in the gpmc_onenand_setup() function before the
> call to _set_sync_mode? Seems nice to have these together.

Intention of this patch is to setup GPMC async mode before driver starts
its job so that reconfiguring GPMC needs to be to be done only once (this
helps in avoiding issues when it has to work with gpmc driver).

With the existing code, set_async was done as part of set_sync, hence
requiring GPMC to be configured twice after driver takes control, with
your suggestion too, GPMC would have to be configured twice.

Regards
Afzal

[PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion

[Jon Hunter]

On 06/12/2012 01:16 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Tue, Jun 12, 2012 at 00:06:30, Hunter, Jon wrote:
> 
>> I agree with getting rid of the first instance at the beginning of
>> _set_async_mode, but why get rid of the above one? Are you assuming that
>> by default it is in async mode? Could be nice to keep it to be explicit.
> 
> Second one is achieved below

Hmmm ... it seems odd to put the commands for setting async in the set
sync. I see what you are doing but surely someone should call set async
and the set sync.

>>> @@ -191,19 +181,21 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
>>>  	u32 reg;
>>>  	bool clk_dep = false;
>>>  
>>> +	/* 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);
>>> +
>>
>> Should we only set these after we have checked the flags and depending
>> on which flags are set?
> 
> Even for sync mode, setting async mode initially is required
> 
>>> @@ -421,6 +413,8 @@ void __init gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
>>>  	gpmc_onenand_resource.end = gpmc_onenand_resource.start +
>>>  							ONENAND_IO_SIZE - 1;
>>>  
>>> +	omap2_onenand_set_async_mode(gpmc_onenand_data->cs);
>>> +
>>
>> What about putting this in the gpmc_onenand_setup() function before the
>> call to _set_sync_mode? Seems nice to have these together.
> 
> Intention of this patch is to setup GPMC async mode before driver starts
> its job so that reconfiguring GPMC needs to be to be done only once (this
> helps in avoiding issues when it has to work with gpmc driver).
> 
> With the existing code, set_async was done as part of set_sync, hence
> requiring GPMC to be configured twice after driver takes control, with
> your suggestion too, GPMC would have to be configured twice.

I am just suggesting that you place the call to set_async_mode in the
gpmc_onenand_setup() instead of the gpmc_onenand_init() and remove the
calls from set_sync (like you have done). So I don't see that these
would configure the GPMC twice.

Jon

[PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion

[Mohammed, Afzal]

Hi Jon,

On Tue, Jun 12, 2012 at 23:00:48, Hunter, Jon wrote:

> On 06/12/2012 01:16 AM, Mohammed, Afzal wrote:
> > With the existing code, set_async was done as part of set_sync, hence
> > requiring GPMC to be configured twice after driver takes control, with
> > your suggestion too, GPMC would have to be configured twice.
> 
> I am just suggesting that you place the call to set_async_mode in the
> gpmc_onenand_setup() instead of the gpmc_onenand_init() and remove the
> calls from set_sync (like you have done). So I don't see that these
> would configure the GPMC twice.

As gpmc_onenand_setup is a callback by onenand driver, we would have
lost the opportunity to configure onenand before driver is probed.
This would cause requirement of double GPMC configuring and we lost
the opportunity to configure GPMC before driver is probed.
And the first step for onenand configuration is always to set it
to async mode (with the way it is done now), so it seems reasonable
to rely on normal GPMC configuration for async & then do reconfigure
for sync.

Regards
Afzal

[PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion

[Jon Hunter]

Hi Afzal,

On 06/13/2012 12:03 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Tue, Jun 12, 2012 at 23:00:48, Hunter, Jon wrote:
> 
>> On 06/12/2012 01:16 AM, Mohammed, Afzal wrote:
>>> With the existing code, set_async was done as part of set_sync, hence
>>> requiring GPMC to be configured twice after driver takes control, with
>>> your suggestion too, GPMC would have to be configured twice.
>>
>> I am just suggesting that you place the call to set_async_mode in the
>> gpmc_onenand_setup() instead of the gpmc_onenand_init() and remove the
>> calls from set_sync (like you have done). So I don't see that these
>> would configure the GPMC twice.
> 
> As gpmc_onenand_setup is a callback by onenand driver, we would have
> lost the opportunity to configure onenand before driver is probed.

Is that a problem? Looks like it is called early in the probe and so I
would hope no one is attempting to access the onenand itself before the
probe has completed.

> This would cause requirement of double GPMC configuring and we lost
> the opportunity to configure GPMC before driver is probed.

I am not convinced we need to. Furthermore with your change you do not
actually set async mode in the onenand until _set_sync() is called.

> And the first step for onenand configuration is always to set it
> to async mode (with the way it is done now), so it seems reasonable
> to rely on normal GPMC configuration for async & then do reconfigure
> for sync.

Yes but as far as I can see, it seems that this is the intent of the
onenand_setup() function to perform the necessary initialisation.

Have you tested onenand? Do you have a board with onenand?

Cheers
Jon

[PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion

[Mohammed, Afzal]

Hi Jon,

On Wed, Jun 13, 2012 at 22:08:47, Hunter, Jon wrote:
> On 06/13/2012 12:03 AM, Mohammed, Afzal wrote:

> > As gpmc_onenand_setup is a callback by onenand driver, we would have
> > lost the opportunity to configure onenand before driver is probed.
> 
> Is that a problem? Looks like it is called early in the probe and so I
> would hope no one is attempting to access the onenand itself before the
> probe has completed.

During gpmc driver probe, it will configure all the connected peripherals,
if configuration details are not present at that point of time, gpmc driver
will cry out saying that configuration & timings has not been configured,
(please see holler if no configuration patch). And I do not see any reason
why gpmc driver should not be fed with async mode configuration initially,
as it has to be done always.

> 
> > This would cause requirement of double GPMC configuring and we lost
> > the opportunity to configure GPMC before driver is probed.
> 
> I am not convinced we need to. Furthermore with your change you do not
> actually set async mode in the onenand until _set_sync() is called.

Yes, setting async mode in onenand is done in set_sync function, and it is
always called by onenand driver indirectly.

Seems if setting async mode in onenand is taken out of set_sync & placed
it before set_sync invocation in gpmc_onenand_setup, intention will be
clear, right ? (even though sequence wise same thing is happening now)


> 
> > And the first step for onenand configuration is always to set it
> > to async mode (with the way it is done now), so it seems reasonable
> > to rely on normal GPMC configuration for async & then do reconfigure
> > for sync.
> 
> Yes but as far as I can see, it seems that this is the intent of the
> onenand_setup() function to perform the necessary initialisation.

I believe doing it in gpmc_onenand_init is better, due to the reasons
mentioned above as well as because function name will correspond to what
it is doing, i.e. initialization

> 
> Have you tested onenand? Do you have a board with onenand?

Yes, on omap3evm, with help of local patches (as mainline doesn't have
those). It was mentioned in cover letter of gpmc driver conversion series

Regards
Afzal

[PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion

[Jon Hunter]

Hi Afzal,

On 06/14/2012 12:40 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Wed, Jun 13, 2012 at 22:08:47, Hunter, Jon wrote:
>> On 06/13/2012 12:03 AM, Mohammed, Afzal wrote:
> 
>>> As gpmc_onenand_setup is a callback by onenand driver, we would have
>>> lost the opportunity to configure onenand before driver is probed.
>>
>> Is that a problem? Looks like it is called early in the probe and so I
>> would hope no one is attempting to access the onenand itself before the
>> probe has completed.
> 
> During gpmc driver probe, it will configure all the connected peripherals,
> if configuration details are not present at that point of time, gpmc driver
> will cry out saying that configuration & timings has not been configured,
> (please see holler if no configuration patch). 

Sorry, I am not sure if I am missing something here, but isn't the
chip-select requested during the gpmc probe? If so then we should not be
programming the gpmc registers at all until the chip-select has been
allocated. Hence, after the probe seems more appropriate.

> And I do not see any reason
> why gpmc driver should not be fed with async mode configuration initially,
> as it has to be done always.

It is more of where you are doing it. I am not against putting in async
mode to begin with.

>>
>>> This would cause requirement of double GPMC configuring and we lost
>>> the opportunity to configure GPMC before driver is probed.
>>
>> I am not convinced we need to. Furthermore with your change you do not
>> actually set async mode in the onenand until _set_sync() is called.
> 
> Yes, setting async mode in onenand is done in set_sync function, and it is
> always called by onenand driver indirectly.
> 
> Seems if setting async mode in onenand is taken out of set_sync & placed
> it before set_sync invocation in gpmc_onenand_setup, intention will be
> clear, right ? (even though sequence wise same thing is happening now)

Exactly.

>>
>>> And the first step for onenand configuration is always to set it
>>> to async mode (with the way it is done now), so it seems reasonable
>>> to rely on normal GPMC configuration for async & then do reconfigure
>>> for sync.
>>
>> Yes but as far as I can see, it seems that this is the intent of the
>> onenand_setup() function to perform the necessary initialisation.
> 
> I believe doing it in gpmc_onenand_init is better, due to the reasons
> mentioned above as well as because function name will correspond to what
> it is doing, i.e. initialization

If the chip-select is allocating during the probe, then I don't agree.

>> Have you tested onenand? Do you have a board with onenand?
> 
> Yes, on omap3evm, with help of local patches (as mainline doesn't have
> those). It was mentioned in cover letter of gpmc driver conversion series

Great. Sorry but with 20+ patch spread across 3 series it is not always
easy to find the details. So ideally it should be mentioned in this
cover letter too.

Thanks
Jon

[PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion

[Mohammed, Afzal]

Hi Jon,

On Thu, Jun 14, 2012 at 23:23:48, Hunter, Jon wrote:
> On 06/14/2012 12:40 AM, Mohammed, Afzal wrote:

> > During gpmc driver probe, it will configure all the connected peripherals,
> > if configuration details are not present at that point of time, gpmc driver
> > will cry out saying that configuration & timings has not been configured,
> > (please see holler if no configuration patch). 
> 
> Sorry, I am not sure if I am missing something here, but isn't the
> chip-select requested during the gpmc probe? If so then we should not be
> programming the gpmc registers at all until the chip-select has been
> allocated. Hence, after the probe seems more appropriate.

This patch by itself has no much meaning other than preparing for driver
conversion so that driver conversion series would be simpler, if you read
it with gpmc driver conversion series and adapting peripherals to gpmc
driver series, hopefully you can make out what I am trying to express

with gpmc driver, first chip select request happens, then programming
gpmc registers happen, but note that both happen in probe

> >> I am not convinced we need to. Furthermore with your change you do not
> >> actually set async mode in the onenand until _set_sync() is called.
> > 
> > Yes, setting async mode in onenand is done in set_sync function, and it is
> > always called by onenand driver indirectly.
> > 
> > Seems if setting async mode in onenand is taken out of set_sync & placed
> > it before set_sync invocation in gpmc_onenand_setup, intention will be
> > clear, right ? (even though sequence wise same thing is happening now)
> 
> Exactly.

Ok, v2 will have this change

> >> Yes but as far as I can see, it seems that this is the intent of the
> >> onenand_setup() function to perform the necessary initialisation.
> > 
> > I believe doing it in gpmc_onenand_init is better, due to the reasons
> > mentioned above as well as because function name will correspond to what
> > it is doing, i.e. initialization
> 
> If the chip-select is allocating during the probe, then I don't agree.

I believe you were referring to gpmc driver probe, not onenand probe,
the changes has been done such that both old and new driver interface
can make use of most of existing code.

And only old interface will call gpmc_onenand_init, and with that gpmc
driver interface is not coming into picture, while with new gpmc driver
interface, function used would be different one as in [1]

> > Yes, on omap3evm, with help of local patches (as mainline doesn't have
> > those). It was mentioned in cover letter of gpmc driver conversion series
> 
> Great. Sorry but with 20+ patch spread across 3 series it is not always
> easy to find the details. So ideally it should be mentioned in this
> cover letter too.

I am sorry for that

Regards
Afzal

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

[PATCH 3/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.

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 arch/arm/mach-omap2/gpmc.c             |    6 ++++++
 arch/arm/plat-omap/include/plat/gpmc.h |    6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 578fd4c..517953f 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -313,6 +313,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);
diff --git a/arch/arm/plat-omap/include/plat/gpmc.h b/arch/arm/plat-omap/include/plat/gpmc.h
index 2e6e259..802fb22 100644
--- a/arch/arm/plat-omap/include/plat/gpmc.h
+++ b/arch/arm/plat-omap/include/plat/gpmc.h
@@ -128,6 +128,12 @@ 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 */
-- 
1.7.10.2

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

[Jon Hunter]

Hi Afzal,

On 06/11/2012 09:02 AM, Afzal Mohammed wrote:
> 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.

What boards have been tested with this change?

Tony is going to want to know what we have tested with such changes to
avoid any regressions.

> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> ---
>  arch/arm/mach-omap2/gpmc.c             |    6 ++++++
>  arch/arm/plat-omap/include/plat/gpmc.h |    6 ++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 578fd4c..517953f 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -313,6 +313,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);
> diff --git a/arch/arm/plat-omap/include/plat/gpmc.h b/arch/arm/plat-omap/include/plat/gpmc.h
> index 2e6e259..802fb22 100644
> --- a/arch/arm/plat-omap/include/plat/gpmc.h
> +++ b/arch/arm/plat-omap/include/plat/gpmc.h
> @@ -128,6 +128,12 @@ 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 */

In general, I agree with this, but if we apply this today, it seems that
we *may* be clearing these fields if they have been configured by the
bootloader and hence this could introduce a regression (potentially).

So we ever need to test boards that this change impacts or at least
verify that this change would not impact these boards because these
fields have not been configured.

Cheers
Jon

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

[Mohammed, Afzal]

Hi Jon,

On Tue, Jun 12, 2012 at 00:19:35, Hunter, Jon wrote:
 
> What boards have been tested with this change?

Beagle board, after applying all 5 series of patches, without all
patch series it can't be tested for beagle board as it depended on
bootloader, not this API

> > +	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 */
> 
> In general, I agree with this, but if we apply this today, it seems that
> we *may* be clearing these fields if they have been configured by the
> bootloader and hence this could introduce a regression (potentially).
> 
> So we ever need to test boards that this change impacts or at least
> verify that this change would not impact these boards because these
> fields have not been configured.

Yes, it is the exactly the reason this patch has been splitted out
of gpmc driver conversion series. To find out whether enhancing
existing API cause any regression and if so, then it can be easily
root caused and would not be confused with driver conversion.

This was the only change required w.r.t old interface, need to make
sure that this won't causes breakage on any of the boards (the boards
which were unknowingly depending on bootloader for these settings).
I have only beagle board & omap3evm (both would not get affected
by this change with existing code) and others can't be validated.
Once this is in lo tree or mainline, this change can be validated
for different boards.


Regards
Afzal

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

[Jon Hunter]

On 06/12/2012 01:37 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Tue, Jun 12, 2012 at 00:19:35, Hunter, Jon wrote:
>  
>> What boards have been tested with this change?
> 
> Beagle board, after applying all 5 series of patches, without all
> patch series it can't be tested for beagle board as it depended on
> bootloader, not this API
> 
>>> +	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 */
>>
>> In general, I agree with this, but if we apply this today, it seems that
>> we *may* be clearing these fields if they have been configured by the
>> bootloader and hence this could introduce a regression (potentially).
>>
>> So we ever need to test boards that this change impacts or at least
>> verify that this change would not impact these boards because these
>> fields have not been configured.
> 
> Yes, it is the exactly the reason this patch has been splitted out
> of gpmc driver conversion series. To find out whether enhancing
> existing API cause any regression and if so, then it can be easily
> root caused and would not be confused with driver conversion.
> 
> This was the only change required w.r.t old interface, need to make
> sure that this won't causes breakage on any of the boards (the boards
> which were unknowingly depending on bootloader for these settings).
> I have only beagle board & omap3evm (both would not get affected
> by this change with existing code) and others can't be validated.
> Once this is in lo tree or mainline, this change can be validated
> for different boards.

Should you at least warn, if you are going to over-write a value?

Jon

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

[Mohammed, Afzal]

Hi Jon,

On Tue, Jun 12, 2012 at 23:06:53, Hunter, Jon wrote:

> Should you at least warn, if you are going to over-write a value?

Yes, that would be better except for too much logging, if Tony
prefers that way I will add. 

Regards
Afzal

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

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120612 22:00]:
> Hi Jon,
> 
> On Tue, Jun 12, 2012 at 23:06:53, Hunter, Jon wrote:
> 
> > Should you at least warn, if you are going to over-write a value?
> 
> Yes, that would be better except for too much logging, if Tony
> prefers that way I will add. 

If there's a chance it causing file system corruption, we should
test it carefully on the boards before applying. If that's done,
then there's probably no need for warnings. It's safer to disable
NAND for untested boards if there's a chance of breaking the timings.

Tony

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [120613 04:36]:
> * Mohammed, Afzal <afzal@ti.com> [120612 22:00]:
> > Hi Jon,
> > 
> > On Tue, Jun 12, 2012 at 23:06:53, Hunter, Jon wrote:
> > 
> > > Should you at least warn, if you are going to over-write a value?
> > 
> > Yes, that would be better except for too much logging, if Tony
> > prefers that way I will add. 
> 
> If there's a chance it causing file system corruption, we should
> test it carefully on the boards before applying. If that's done,
> then there's probably no need for warnings. It's safer to disable
> NAND for untested boards if there's a chance of breaking the timings.

Actually this patch breaks at least DMA on tusb6010 on n8x0. That's
a MUSB hardware in a wrapper connected to GPMC that's very picky with
the timings.

Got any hints what should be done with the cycle2cycle stuff for
tusb6010?

Tony

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

[Mohammed, Afzal]

Hi Tony,

On Wed, Jun 13, 2012 at 17:24:45, Tony Lindgren wrote:

> > If there's a chance it causing file system corruption, we should
> > test it carefully on the boards before applying. If that's done,
> > then there's probably no need for warnings. It's safer to disable
> > NAND for untested boards if there's a chance of breaking the timings.
> 
> Actually this patch breaks at least DMA on tusb6010 on n8x0. That's
> a MUSB hardware in a wrapper connected to GPMC that's very picky with
> the timings.
> 
> Got any hints what should be done with the cycle2cycle stuff for
> tusb6010?

Not as of now, let me try to find out.

Regards
Afzal

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

[Mohammed, Afzal]

Hi Tony,

On Wed, Jun 13, 2012 at 17:28:33, Mohammed, Afzal wrote:
> On Wed, Jun 13, 2012 at 17:24:45, Tony Lindgren wrote:
> 
> > > If there's a chance it causing file system corruption, we should
> > > test it carefully on the boards before applying. If that's done,
> > > then there's probably no need for warnings. It's safer to disable
> > > NAND for untested boards if there's a chance of breaking the timings.
> > 
> > Actually this patch breaks at least DMA on tusb6010 on n8x0. That's
> > a MUSB hardware in a wrapper connected to GPMC that's very picky with
> > the timings.
> > 
> > Got any hints what should be done with the cycle2cycle stuff for
> > tusb6010?
> 
> Not as of now, let me try to find out.

Probably with the below patch [1], we can get values set by bootloader &
calculate back value to entered in Kernel, of course that may not work
if tusb6010 works with different gpmc i/p frequency.

Meanwhile, let me try to get datasheet & find proper values.
 
Regards
Afzal

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index eec011a..bc75e6c 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -336,11 +336,15 @@ int gpmc_cs_calc_divider(int cs, unsigned int sync_clk)
        return div;
 }

+static void gpmc_print_cs_timings(int cs);
+
 int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
 {
        int div;
        u32 l;

+       gpmc_print_cs_timings(cs);
+
        div = gpmc_cs_calc_divider(cs, t->sync_clk);
        if (div < 0)
                return -1;

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

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120614 03:15]:
> Hi Tony,
> 
> On Wed, Jun 13, 2012 at 17:28:33, Mohammed, Afzal wrote:
> > On Wed, Jun 13, 2012 at 17:24:45, Tony Lindgren wrote:
> > 
> > > > If there's a chance it causing file system corruption, we should
> > > > test it carefully on the boards before applying. If that's done,
> > > > then there's probably no need for warnings. It's safer to disable
> > > > NAND for untested boards if there's a chance of breaking the timings.
> > > 
> > > Actually this patch breaks at least DMA on tusb6010 on n8x0. That's
> > > a MUSB hardware in a wrapper connected to GPMC that's very picky with
> > > the timings.
> > > 
> > > Got any hints what should be done with the cycle2cycle stuff for
> > > tusb6010?
> > 
> > Not as of now, let me try to find out.
> 
> Probably with the below patch [1], we can get values set by bootloader &
> calculate back value to entered in Kernel, of course that may not work
> if tusb6010 works with different gpmc i/p frequency.

Well I took a look at the values, and it seems the only difference is the
static GPMC_CONFIG1_CLKACTIVATIONTIME(1) that your patch now overwrites 0.

Tony

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

[Mohammed, Afzal]

Hi Tony,

On Thu, Jun 14, 2012 at 15:49:02, Tony Lindgren wrote:

> Well I took a look at the values, and it seems the only difference is the
> static GPMC_CONFIG1_CLKACTIVATIONTIME(1) that your patch now overwrites 0.

It seems change below should be part of $subject.

Please let me know your comments

Regards
Afzal

diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index fd4c48d..938896c 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -321,6 +321,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;
@@ -354,7 +356,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/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

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

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120614 03:43]:
> Hi Tony,
> 
> On Thu, Jun 14, 2012 at 15:49:02, Tony Lindgren wrote:
> 
> > Well I took a look at the values, and it seems the only difference is the
> > static GPMC_CONFIG1_CLKACTIVATIONTIME(1) that your patch now overwrites 0.
> 
> It seems change below should be part of $subject.
> 
> Please let me know your comments

Well I could not get this to apply either on top of the $subject nor all your
patches for some reason, but I manually applied the tusb6010 part with the
following change..

> --- 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);
> +

..this should be just 1 as it's one tick, not ns.

>         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

And that makes tusb6010 work as earlier with your patches.

For onenand I'm getting the following error:

omap2-onenand omap2-onenand: Cannot request GPMC CS 

Regards,

Tony

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

[Mohammed, Afzal]

Hi Tony,

On Thu, Jun 14, 2012 at 17:19:06, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120614 03:43]:

> > It seems change below should be part of $subject.
> For onenand I'm getting the following error:
> 
> omap2-onenand omap2-onenand: Cannot request GPMC CS 

Without this change (not referring to 3/3, but diff that
was sent in previous mail), was not this error present

Regards
Afzal

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

[Mohammed, Afzal]

Hi Tony,

On Thu, Jun 14, 2012 at 17:19:06, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120614 03:43]:

> For onenand I'm getting the following error:
> 
> omap2-onenand omap2-onenand: Cannot request GPMC CS 

I am a bit confused with this message, for onenand, we only have,

pr_err("%s: Cannot request GPMC CS\n", __func__);

dev_err is used only for nand

Regards
Afzal

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

[Mohammed, Afzal]

Hi Tony,

On Thu, Jun 14, 2012 at 17:39:41, Mohammed, Afzal wrote:
> On Thu, Jun 14, 2012 at 17:19:06, Tony Lindgren wrote:

> > For onenand I'm getting the following error:
> > 
> > omap2-onenand omap2-onenand: Cannot request GPMC CS 
> 
> I am a bit confused with this message, for onenand, we only have,
> 
> pr_err("%s: Cannot request GPMC CS\n", __func__);
> 
> dev_err is used only for nand

This error can happen if my patches are not present,

dev_err for onenand driver was removed by,

[mtd: onenand: omap2: obtain memory from resource],

seems, mtd patches are not in your present branch

Regards
Afzal

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

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120614 05:25]:
> Hi Tony,
> 
> On Thu, Jun 14, 2012 at 17:39:41, Mohammed, Afzal wrote:
> > On Thu, Jun 14, 2012 at 17:19:06, Tony Lindgren wrote:
> 
> > > For onenand I'm getting the following error:
> > > 
> > > omap2-onenand omap2-onenand: Cannot request GPMC CS 
> > 
> > I am a bit confused with this message, for onenand, we only have,
> > 
> > pr_err("%s: Cannot request GPMC CS\n", __func__);
> > 
> > dev_err is used only for nand
> 
> This error can happen if my patches are not present,
> 
> dev_err for onenand driver was removed by,
> 
> [mtd: onenand: omap2: obtain memory from resource],
> 
> seems, mtd patches are not in your present branch

OK, I'll try with those.

Tony

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

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120614 03:43]:
> Hi Tony,
> 
> On Thu, Jun 14, 2012 at 15:49:02, Tony Lindgren wrote:
> 
> > Well I took a look at the values, and it seems the only difference is the
> > static GPMC_CONFIG1_CLKACTIVATIONTIME(1) that your patch now overwrites 0.
> 
> It seems change below should be part of $subject.
> 
> Please let me know your comments
> 
> Regards
> Afzal
> 
> diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> index fd4c48d..938896c 100644
> --- a/arch/arm/mach-omap2/gpmc-onenand.c
> +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> @@ -321,6 +321,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;
> +

This too should be fclk_offset, not fclk_offset_ns.

Tony

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

[Mohammed, Afzal]

Hi Tony,

On Thu, Jun 14, 2012 at 17:22:08, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120614 03:43]:
> > +       t.clk_activation = fclk_offset_ns;
> > +
> 
> This too should be fclk_offset, not fclk_offset_ns.

As gpmc_cs_set_timing convert it to ticks from ns,
shouldn't we put it in ns ? 

Regards
Afzal

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

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120614 05:00]:
> Hi Tony,
> 
> On Thu, Jun 14, 2012 at 17:22:08, Tony Lindgren wrote:
> > * Mohammed, Afzal <afzal@ti.com> [120614 03:43]:
> > > +       t.clk_activation = fclk_offset_ns;
> > > +
> > 
> > This too should be fclk_offset, not fclk_offset_ns.
> 
> As gpmc_cs_set_timing convert it to ticks from ns,
> shouldn't we put it in ns ? 

Hmm I see, something's wrong though.. Some of these we really
want to specify as ticks instead of ns. Your patch was using
1 tick value as 1 ns value, which won't work. When I changed
it back to what I thought was ticks, it just happened to work
probably because of rounding. That's probably the reason why
some of these have been set directly with gpmc_cs_write_reg
as that sets tick values directly.

It seems that we need to still allow both ns and tick values.

Regards,

Tony

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

[Mohammed, Afzal]

Hi Tony,

On Thu, Jun 14, 2012 at 17:59:31, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120614 05:00]:
> > On Thu, Jun 14, 2012 at 17:22:08, Tony Lindgren wrote:
> > > * Mohammed, Afzal <afzal@ti.com> [120614 03:43]:
> > > > +       t.clk_activation = fclk_offset_ns;
> > > > +
> > > 
> > > This too should be fclk_offset, not fclk_offset_ns.
> > 
> > As gpmc_cs_set_timing convert it to ticks from ns,
> > shouldn't we put it in ns ? 
> 
> Hmm I see, something's wrong though.. Some of these we really
> want to specify as ticks instead of ns. Your patch was using
> 1 tick value as 1 ns value, which won't work. When I changed
> it back to what I thought was ticks, it just happened to work
> probably because of rounding. That's probably the reason why
> some of these have been set directly with gpmc_cs_write_reg
> as that sets tick values directly.

For onenand,

fclk_offset = gpmc_ns_to_ticks(fclk_offset_ns);

By using directly fclk_offset_ns, we transfer job of doing
the same to gpmc_cs_set_timings (which does gpmc_ns_to_ticks
over fclk_offset_ns & then do << 25)

Hence passing fclk_offset_ns in effect should be same as,

GPMC_CONFIG1_CLKACTIVATIONTIME(fclk_offset)

If onenand is not working with it, then there is something that
is invisible here.

Were you able to get it working with the changed value for
usb or onenand ?

In case possible, can you check enabling DEBUG and see what
timings are logged

Regards
Afzal

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

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120614 05:57]:
> Hi Tony,
> 
> On Thu, Jun 14, 2012 at 17:59:31, Tony Lindgren wrote:
> > * Mohammed, Afzal <afzal@ti.com> [120614 05:00]:
> > > On Thu, Jun 14, 2012 at 17:22:08, Tony Lindgren wrote:
> > > > * Mohammed, Afzal <afzal@ti.com> [120614 03:43]:
> > > > > +       t.clk_activation = fclk_offset_ns;
> > > > > +
> > > > 
> > > > This too should be fclk_offset, not fclk_offset_ns.
> > > 
> > > As gpmc_cs_set_timing convert it to ticks from ns,
> > > shouldn't we put it in ns ? 
> > 
> > Hmm I see, something's wrong though.. Some of these we really
> > want to specify as ticks instead of ns. Your patch was using
> > 1 tick value as 1 ns value, which won't work. When I changed
> > it back to what I thought was ticks, it just happened to work
> > probably because of rounding. That's probably the reason why
> > some of these have been set directly with gpmc_cs_write_reg
> > as that sets tick values directly.
> 
> For onenand,
> 
> fclk_offset = gpmc_ns_to_ticks(fclk_offset_ns);
> 
> By using directly fclk_offset_ns, we transfer job of doing
> the same to gpmc_cs_set_timings (which does gpmc_ns_to_ticks
> over fclk_offset_ns & then do << 25)
> 
> Hence passing fclk_offset_ns in effect should be same as,
> 
> GPMC_CONFIG1_CLKACTIVATIONTIME(fclk_offset)
> 
> If onenand is not working with it, then there is something that
> is invisible here.

Yup, you're right, for onenand it should be in ns.
 
> Were you able to get it working with the changed value for
> usb or onenand ?

Yes seems to work after applying the two patches that I somehow
skipped and with fclk_offset_ns.
 
> In case possible, can you check enabling DEBUG and see what
> timings are logged

Sounds like the tusb6010 non-ns tick value is the only remaining
issue.

Regards,

Tony

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

[Mohammed, Afzal]

Hi Tony,

On Thu, Jun 14, 2012 at 22:23:37, Tony Lindgren wrote:
 
> Sounds like the tusb6010 non-ns tick value is the only remaining
> issue.

t.clk_activation = gpmc_ticks_to_ns(1);

was used so that gpmc_cs_set_timings would do gpmc_ns_to_ticks over
it & hence effectively 1 tick would get programmed in the register
for clock activation.

A value of 1 for clk_activation also should work due to rounding
effect.

But I am unable to find reason for failure upon using
gpmc_ticks_to_ns(1), which seems to me right thing to be used.
Let me try to invoke tusb6010 functions in beagle board,
observe timings so that at least I will get an idea
what is going on here (even though it is guaranteed to crash)

Regards
Afzal

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

[Mohammed, Afzal]

Hi Tony,

On Fri, Jun 15, 2012 at 11:12:46, Mohammed, Afzal wrote:

> But I am unable to find reason for failure upon using
> gpmc_ticks_to_ns(1), which seems to me right thing to be used.
> Let me try to invoke tusb6010 functions in beagle board,
> observe timings so that at least I will get an idea
> what is going on here (even though it is guaranteed to crash)

Checked simulating on beagle board, I am at total loss to
understand why using gpmc_ticks_to_ns(1) has failed for n8x0

clk_activation timings with both values as follows,
 
[1] With t.clk_activation = gpmc_ticks_to_ns(1);

GPMC CS4: clk_activation:   1 ticks,   6 ns (was   0 ticks)   6 ns

[2] With t.clk_activation = 1;

GPMC CS4: clk_activation:   1 ticks,   6 ns (was   0 ticks)   1 ns

Last field show in ns the time we are trying to set,
and for both cases, 1 ticks are being programmed in register.

Regards
Afzal 

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

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120614 23:20]:
> Hi Tony,
> 
> On Fri, Jun 15, 2012 at 11:12:46, Mohammed, Afzal wrote:
> 
> > But I am unable to find reason for failure upon using
> > gpmc_ticks_to_ns(1), which seems to me right thing to be used.
> > Let me try to invoke tusb6010 functions in beagle board,
> > observe timings so that at least I will get an idea
> > what is going on here (even though it is guaranteed to crash)
> 
> Checked simulating on beagle board, I am at total loss to
> understand why using gpmc_ticks_to_ns(1) has failed for n8x0
> 
> clk_activation timings with both values as follows,
>  
> [1] With t.clk_activation = gpmc_ticks_to_ns(1);
> 
> GPMC CS4: clk_activation:   1 ticks,   6 ns (was   0 ticks)   6 ns
> 
> [2] With t.clk_activation = 1;
> 
> GPMC CS4: clk_activation:   1 ticks,   6 ns (was   0 ticks)   1 ns
> 
> Last field show in ns the time we are trying to set,
> and for both cases, 1 ticks are being programmed in register.

Yes tired it again it is working correctly. I must have messed up
something yesterday when manually patching the clk_activation, maybe
I put the clk_activation value into async timings instead as I was
seeing the tick value set to 0 for the sync mode.

So looks OK to me, n800 tusb6010 and onenand behave as earlier,
also onenand on n900 seems to get detected as earlier.

Regards,

Tony

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

[Mohammed, Afzal]

Hi Tony,

On Fri, Jun 15, 2012 at 16:15:43, Tony Lindgren wrote:

> something yesterday when manually patching the clk_activation, maybe
> I put the clk_activation value into async timings instead as I was
> seeing the tick value set to 0 for the sync mode.

I too thought like that initially, but when you told that putting
value of 1 tick made it work, I thought it is some different issue

Regards
Afzal

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

[Mohammed, Afzal]

Hi Tony,

On Wed, Jun 13, 2012 at 17:02:17, Tony Lindgren wrote:
> * Mohammed, Afzal <afzal@ti.com> [120612 22:00]:
> > Yes, that would be better except for too much logging, if Tony
> > prefers that way I will add. 
> 
> If there's a chance it causing file system corruption, we should
> test it carefully on the boards before applying. If that's done,
> then there's probably no need for warnings. It's safer to disable
> NAND for untested boards if there's a chance of breaking the timings.

By disabling NAND, I understand that you are suggesting to remove
nand initialization done in board file, right ?

And boards that can be tested here are omap3evm & beagleboard,
both of which can't be tested for this change.

Or should additional timings be achieved without affecting old
interface, but that it seems would necessitate more code
duplication.

Regards
Afzal

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

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120613 05:43]:
> Hi Tony,
> 
> On Wed, Jun 13, 2012 at 17:02:17, Tony Lindgren wrote:
> > * Mohammed, Afzal <afzal@ti.com> [120612 22:00]:
> > > Yes, that would be better except for too much logging, if Tony
> > > prefers that way I will add. 
> > 
> > If there's a chance it causing file system corruption, we should
> > test it carefully on the boards before applying. If that's done,
> > then there's probably no need for warnings. It's safer to disable
> > NAND for untested boards if there's a chance of breaking the timings.
> 
> By disabling NAND, I understand that you are suggesting to remove
> nand initialization done in board file, right ?

Yes, but before we do that, let's fix things first for cases that we
can test, like tusb6010 DMA.
 
> And boards that can be tested here are omap3evm & beagleboard,
> both of which can't be tested for this change.
> 
> Or should additional timings be achieved without affecting old
> interface, but that it seems would necessitate more code
> duplication.

We should just keep the same timings as before, with values
added for the newly added registers.

Regards,

Tony

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

[Mohammed, Afzal]

Hi Tony,

On Wed, Jun 13, 2012 at 18:12:15, Tony Lindgren wrote:

> > Or should additional timings be achieved without affecting old
> > interface, but that it seems would necessitate more code
> > duplication.
> 
> We should just keep the same timings as before, with values
> added for the newly added registers.

What I understood from above is to get values bootloader used to
set for those new timings & put in Kernel, right ?

Regards
Afzal

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

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120613 07:09]:
> Hi Tony,
> 
> On Wed, Jun 13, 2012 at 18:12:15, Tony Lindgren wrote:
> 
> > > Or should additional timings be achieved without affecting old
> > > interface, but that it seems would necessitate more code
> > > duplication.
> > 
> > We should just keep the same timings as before, with values
> > added for the newly added registers.
> 
> What I understood from above is to get values bootloader used to
> set for those new timings & put in Kernel, right ?

Yeah and eventually to the .dts files.

Tony

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

[Tony Lindgren]

* Afzal Mohammed <afzal@ti.com> [120611 07:21]:
> 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.
> 
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> ---
>  arch/arm/mach-omap2/gpmc.c             |    6 ++++++
>  arch/arm/plat-omap/include/plat/gpmc.h |    6 ++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 578fd4c..517953f 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -313,6 +313,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);
> +

Thinking about this, the CONFIG1 bits have been set with
gpmc_cs_write_reg because these are part of the static configuration
and do not need to be dynamically calculated as they are tick based.
For example, tusb6010 sets GPMC_CONFIG1_CLKACTIVATIONTIME(1) during init.

Writing these over and over again during DVFS does not make sense, they
should be only initialized once.

Regards,

Tony

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

[Mohammed, Afzal]

Hi Tony,

On Thu, Jun 14, 2012 at 14:59:57, Tony Lindgren wrote:
> * Afzal Mohammed <afzal@ti.com> [120611 07:21]:

> > +	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);
> > +
> 
> Thinking about this, the CONFIG1 bits have been set with
> gpmc_cs_write_reg because these are part of the static configuration
> and do not need to be dynamically calculated as they are tick based.
> For example, tusb6010 sets GPMC_CONFIG1_CLKACTIVATIONTIME(1) during init.

But aren't we deciding number of ticks based on clock period ?
If we take case of onenand, based on the knowledge of clock period,
number of ticks are calculated.

And similarly to decide cycle2cycledelay, busturnaround, we decide number
of ticks based on peripheral datasheet timings & gpmc clock, hence
shouldn't it also be dynamically calculated similar to timings that were
existing earlier.

Regards
Afzal

> 
> Writing these over and over again during DVFS does not make sense, they
> should be only initialized once.
> 
> Regards,
> 
> Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120614 02:46]:
> Hi Tony,
> 
> On Thu, Jun 14, 2012 at 14:59:57, Tony Lindgren wrote:
> > * Afzal Mohammed <afzal@ti.com> [120611 07:21]:
> 
> > > +	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);
> > > +
> > 
> > Thinking about this, the CONFIG1 bits have been set with
> > gpmc_cs_write_reg because these are part of the static configuration
> > and do not need to be dynamically calculated as they are tick based.
> > For example, tusb6010 sets GPMC_CONFIG1_CLKACTIVATIONTIME(1) during init.
> 
> But aren't we deciding number of ticks based on clock period ?
> If we take case of onenand, based on the knowledge of clock period,
> number of ticks are calculated.
> 
> And similarly to decide cycle2cycledelay, busturnaround, we decide number
> of ticks based on peripheral datasheet timings & gpmc clock, hence
> shouldn't it also be dynamically calculated similar to timings that were
> existing earlier.

Hmm indeed onenand is setting that dynamically. OK, let's try your
approach and make sure we patch in the missing values so we don't
overwrite the values set with gpmc_cs_write_reg.

Regards,

Tony

[PATCH 0/3] Prepare for GPMC driver conversion

[Mohammed, Afzal]

Hi Tony,

On Mon, Jun 11, 2012 at 19:31:01, 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. Cleans up OneNAND platform code a little
> 3. Handles additional timings in Kernel
> 
> This series is based on 3.5-rc1 & made on top of
> http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69501.html
> http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69881.html


Please let me know your comments on this series.

gpmc driver conversion series [1] is dependent on this.

Regards
Afzal

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

[PATCH 0/3] Prepare for GPMC driver conversion

[Tony Lindgren]

* Mohammed, Afzal <afzal@ti.com> [120612 03:32]:
> Hi Tony,
> 
> On Mon, Jun 11, 2012 at 19:31:01, 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. Cleans up OneNAND platform code a little
> > 3. Handles additional timings in Kernel
> > 
> > This series is based on 3.5-rc1 & made on top of
> > http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69501.html
> > http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69881.html
> 
> 
> Please let me know your comments on this series.
> 
> gpmc driver conversion series [1] is dependent on this.

Looks good to me, made one comment regarding testing and maybe disabling
NAND for untested boards if timings change. Are Jon's comments all handled?

Tony
 
> Regards
> Afzal
> 
> [1] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69897.html
> 

[PATCH 0/3] Prepare for GPMC driver conversion

[Mohammed, Afzal]

Hi Tony,

On Wed, Jun 13, 2012 at 17:03:59, Tony Lindgren wrote:

> NAND for untested boards if timings change. Are Jon's comments all handled?

I have explained the justification, why those changes were done so,
waiting for his comments.

Regards
Afzal

[PATCH 0/3] Prepare for GPMC driver conversion

[Jon Hunter]

Hi Tony, Afzal,

On 06/13/2012 07:40 AM, Mohammed, Afzal wrote:
> Hi Tony,
> 
> On Wed, Jun 13, 2012 at 17:03:59, Tony Lindgren wrote:
> 
>> NAND for untested boards if timings change. Are Jon's comments all handled?
> 
> I have explained the justification, why those changes were done so,
> waiting for his comments.

My only real concern right now is for onenand.

Afzal, let me know if you have been able to test. I have a omap2430 with
onenand I could try.

Cheers
Jon

[PATCH 0/3] Prepare for GPMC driver conversion

[Mohammed, Afzal]

Hi Jon,

On Wed, Jun 13, 2012 at 22:16:57, Hunter, Jon wrote:

> Afzal, let me know if you have been able to test. I have a omap2430 with
> onenand I could try.

Yes, this has been tested with omap3evm. Let me try if I can get another
onenand board to test.

If you can test, that would be really helpful.

Regards
Afzal