Skip to contentSkip to conversation

Omap2 updates for 2.6.29 merge window

PATCH In review Tony Lindgren 4 participants 12 messages Dec 5, 2008 – Dec 11, 2008

Subsystems (7)
lore
Share & tools
lore

Tony Lindgren

author maintainer
#0 lore raw

cover Omap2 updates for 2.6.29 merge window

Hi,

Here are some omap2 updates for review. Some of these patches also affect omap3 as the code is shared.

Regards,

Tony

---

David Brownell (1):
ARM: OMAP2: bard-h4: list those eeproms

Lauri Leukkunen (1):
ARM: OMAP2: Use omap_rev() instead of system_rev

Paul Walmsley (2):
ARM: OMAP2: skip unnecessary TLDR write during non-autoreload for gptimer
ARM: OMAP2: drop redundant pending write check for gptimer

Tony Lindgren (2):
ARM: OMAP2: Fix cpu detection
ARM: OMAP2: Prepare cpu detection for further improvements

 arch/arm/mach-omap1/id.c                        |   53 ++--
 arch/arm/mach-omap1/pm.c                        |    6 
 arch/arm/mach-omap2/board-h4.c                  |   14 +
 arch/arm/mach-omap2/clock34xx.c                 |    2 
 arch/arm/mach-omap2/id.c                        |  305 ++++++++++-------------
 arch/arm/plat-omap/dmtimer.c                    |   11 -
 arch/arm/plat-omap/include/mach/board-apollon.h |    4 
 arch/arm/plat-omap/include/mach/cpu.h           |   82 ++----
 arch/arm/plat-omap/sram.c                       |    3 
 9 files changed, 219 insertions(+), 261 deletions(-)
Signature · 1 line
Signature
author maintainer
#1 lore raw

PATCH 1/6 ARM: OMAP2: Prepare cpu detection for further improvements

Rename omap2_check_revision to omap24xx_check_revision. Then next patch will split if further and add omap34xx_check_revision.

---
 arch/arm/mach-omap2/id.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/id.c +6 -1
index bf45ff3..b6ee18c 100644
@@ -191,7 +191,7 @@ static void __init _set_omap_chip(void)
 
 }
 
-void __init omap2_check_revision(void)
+void __init omap24xx_check_revision(void)
 {
 	int i, j;
 	u32 idcode;
@@ -266,6 +266,11 @@ void __init omap2_check_revision(void)
 
 }
 
+void __init omap2_check_revision(void)
+{
+	omap24xx_check_revision();
+}
+
 void __init omap2_set_globals_tap(struct omap_globals *omap2_globals)
 {
 	class = omap2_globals->class;
author maintainer
#2 lore raw

PATCH 2/6 ARM: OMAP2: Fix cpu detection

In reply to Tony Lindgren

At some point omap2 changed the bits for GET_OMAP_CLASS, which broke 15xx detection on 730 as noticed by Russell King.

This patch fixes omap2 cpu detection to respect the original GET_OMAP_CLASS, and simplifies the detection for 34xx.

---
 arch/arm/mach-omap1/id.c              |    3 
 arch/arm/mach-omap2/clock34xx.c       |    2 
 arch/arm/mach-omap2/id.c              |  290 ++++++++++++++-------------------
 arch/arm/plat-omap/include/mach/cpu.h |   76 +++------
 4 files changed, 145 insertions(+), 226 deletions(-)
arch/arm/mach-omap1/id.c +0 -3
index 13083d7..d6110fe 100644
@@ -178,9 +178,6 @@ void __init omap_check_revision(void)
 	case 0x17:
 		system_rev |= 0x16;
 		break;
-	case 0x24:
-		system_rev |= 0x24;
-		break;
 	default:
 		printk("Unknown OMAP cpu type: 0x%02x\n", cpu_type);
 	}
arch/arm/mach-omap2/clock34xx.c +1 -1
index 084e110..3c2f665 100644
@@ -475,7 +475,7 @@ int __init omap2_clk_init(void)
 		 * Update this if there are further clock changes between ES2
 		 * and production parts
 		 */
-		if (is_sil_rev_equal_to(OMAP3430_REV_ES1_0)) {
+		if (system_rev == OMAP3430_REV_ES1_0) {
 			/* No 3430ES1-only rates exist, so no RATE_IN_3430ES1 */
 			cpu_clkflg |= CLOCK_IN_OMAP3430ES1;
 		} else {
arch/arm/mach-omap2/id.c +119 -171

339 lines not on this page · raw message

arch/arm/plat-omap/include/mach/cpu.h +25 -51

113 lines not on this page · raw message

author maintainer
#3 lore raw

PATCH 3/6 ARM: OMAP2: Use omap_rev() instead of system_rev

In reply to Tony Lindgren

From: Lauri Leukkunen <[email protected]>

system_rev is meant for board revision, this patch changes all relevant instances to use the new omap_rev() function liberating system_rev to be used with ATAG_REVISION as it has been designed.

---
 arch/arm/mach-omap1/id.c                        |   50 ++++++++++++++---------
 arch/arm/mach-omap1/pm.c                        |    6 ++-
 arch/arm/mach-omap2/clock34xx.c                 |    2 -
 arch/arm/mach-omap2/id.c                        |   34 ++++++++++------
 arch/arm/plat-omap/include/mach/board-apollon.h |    4 +-
 arch/arm/plat-omap/include/mach/cpu.h           |   10 ++---
 arch/arm/plat-omap/sram.c                       |    3 +
 7 files changed, 66 insertions(+), 43 deletions(-)
arch/arm/mach-omap1/id.c +30 -20
index d6110fe..89bb875 100644
@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <mach/cpu.h>
 
 #define OMAP_DIE_ID_0		0xfffe1800
 #define OMAP_DIE_ID_1		0xfffe1804
@@ -30,6 +31,8 @@ struct omap_id {
 	u32	type;		/* Cpu id bits [31:08], cpu class bits [07:00] */
 };
 
+static unsigned int omap_revision;
+
 /* Register values to detect the OMAP version */
 static struct omap_id omap_ids[] __initdata = {
 	{ .jtag_id = 0xb574, .die_rev = 0x2, .omap_id = 0x03310315, .type = 0x03100000},
@@ -53,6 +56,12 @@ static struct omap_id omap_ids[] __initdata = {
 	{ .jtag_id = 0xb5f7, .die_rev = 0x2, .omap_id = 0x03330100, .type = 0x17100000},
 };
 
+unsigned int omap_rev(void)
+{
+	return omap_revision;
+}
+EXPORT_SYMBOL(omap_rev);
+
 /*
  * Get OMAP type from PROD_ID.
  * 1710 has the PROD_ID in bits 15:00, not in 16:01 as documented in TRM.
@@ -121,17 +130,18 @@ void __init omap_check_revision(void)
 	omap_id = omap_readl(OMAP32_ID_0);
 
 #ifdef DEBUG
-	printk("OMAP_DIE_ID_0: 0x%08x\n", omap_readl(OMAP_DIE_ID_0));
-	printk("OMAP_DIE_ID_1: 0x%08x DIE_REV: %i\n",
+	printk(KERN_DEBUG "OMAP_DIE_ID_0: 0x%08x\n", omap_readl(OMAP_DIE_ID_0));
+	printk(KERN_DEBUG "OMAP_DIE_ID_1: 0x%08x DIE_REV: %i\n",
 		omap_readl(OMAP_DIE_ID_1),
 	       (omap_readl(OMAP_DIE_ID_1) >> 17) & 0xf);
-	printk("OMAP_PRODUCTION_ID_0: 0x%08x\n", omap_readl(OMAP_PRODUCTION_ID_0));
-	printk("OMAP_PRODUCTION_ID_1: 0x%08x JTAG_ID: 0x%04x\n",
+	printk(KERN_DEBUG "OMAP_PRODUCTION_ID_0: 0x%08x\n",
+		omap_readl(OMAP_PRODUCTION_ID_0));
+	printk(KERN_DEBUG "OMAP_PRODUCTION_ID_1: 0x%08x JTAG_ID: 0x%04x\n",
 		omap_readl(OMAP_PRODUCTION_ID_1),
 		omap_readl(OMAP_PRODUCTION_ID_1) & 0xffff);
-	printk("OMAP32_ID_0: 0x%08x\n", omap_readl(OMAP32_ID_0));
-	printk("OMAP32_ID_1: 0x%08x\n", omap_readl(OMAP32_ID_1));
-	printk("JTAG_ID: 0x%04x DIE_REV: %i\n", jtag_id, die_rev);
+	printk(KERN_DEBUG "OMAP32_ID_0: 0x%08x\n", omap_readl(OMAP32_ID_0));
+	printk(KERN_DEBUG "OMAP32_ID_1: 0x%08x\n", omap_readl(OMAP32_ID_1));
+	printk(KERN_DEBUG "JTAG_ID: 0x%04x DIE_REV: %i\n", jtag_id, die_rev);
 #endif
 
 	system_serial_high = omap_readl(OMAP_DIE_ID_0);
@@ -140,7 +150,7 @@ void __init omap_check_revision(void)
 	/* First check only the major version in a safe way */
 	for (i = 0; i < ARRAY_SIZE(omap_ids); i++) {
 		if (jtag_id == (omap_ids[i].jtag_id)) {
-			system_rev = omap_ids[i].type;
+			omap_revision = omap_ids[i].type;
 			break;
 		}
 	}
@@ -148,7 +158,7 @@ void __init omap_check_revision(void)
 	/* Check if we can find the die revision */
 	for (i = 0; i < ARRAY_SIZE(omap_ids); i++) {
 		if (jtag_id == omap_ids[i].jtag_id && die_rev == omap_ids[i].die_rev) {
-			system_rev = omap_ids[i].type;
+			omap_revision = omap_ids[i].type;
 			break;
 		}
 	}
@@ -158,35 +168,35 @@ void __init omap_check_revision(void)
 		if (jtag_id == omap_ids[i].jtag_id
 		    && die_rev == omap_ids[i].die_rev
 		    && omap_id == omap_ids[i].omap_id) {
-			system_rev = omap_ids[i].type;
+			omap_revision = omap_ids[i].type;
 			break;
 		}
 	}
 
 	/* Add the cpu class info (7xx, 15xx, 16xx, 24xx) */
-	cpu_type = system_rev >> 24;
+	cpu_type = omap_revision >> 24;
 
 	switch (cpu_type) {
 	case 0x07:
-		system_rev |= 0x07;
+		omap_revision |= 0x07;
 		break;
 	case 0x03:
 	case 0x15:
-		system_rev |= 0x15;
+		omap_revision |= 0x15;
 		break;
 	case 0x16:
 	case 0x17:
-		system_rev |= 0x16;
+		omap_revision |= 0x16;
 		break;
 	default:
-		printk("Unknown OMAP cpu type: 0x%02x\n", cpu_type);
+		printk(KERN_INFO "Unknown OMAP cpu type: 0x%02x\n", cpu_type);
 	}
 
-	printk("OMAP%04x", system_rev >> 16);
-	if ((system_rev >> 8) & 0xff)
-		printk("%x", (system_rev >> 8) & 0xff);
-	printk(" revision %i handled as %02xxx id: %08x%08x\n",
-	       die_rev, system_rev & 0xff, system_serial_low,
+	printk(KERN_INFO "OMAP%04x", omap_revision >> 16);
+	if ((omap_revision >> 8) & 0xff)
+		printk(KERN_INFO "%x", (omap_revision >> 8) & 0xff);
+	printk(KERN_INFO " revision %i handled as %02xxx id: %08x%08x\n",
+	       die_rev, omap_revision & 0xff, system_serial_low,
 	       system_serial_high);
 }
 
arch/arm/mach-omap1/pm.c +4 -2
index 770d256..9774c1f 100644
@@ -226,7 +226,8 @@ void omap_pm_suspend(void)
 {
 	unsigned long arg0 = 0, arg1 = 0;
 
-	printk("PM: OMAP%x is trying to enter deep sleep...\n", system_rev);
+	printk(KERN_INFO "PM: OMAP%x is trying to enter deep sleep...\n",
+		omap_rev());
 
 	omap_serial_wake_trigger(1);
 
@@ -421,7 +422,8 @@ void omap_pm_suspend(void)
 
 	omap_serial_wake_trigger(0);
 
-	printk("PM: OMAP%x is re-starting from deep sleep...\n", system_rev);
+	printk(KERN_INFO "PM: OMAP%x is re-starting from deep sleep...\n",
+		omap_rev());
 }
 
 #if defined(DEBUG) && defined(CONFIG_PROC_FS)
arch/arm/mach-omap2/clock34xx.c +1 -1
index 3c2f665..31bb701 100644
@@ -475,7 +475,7 @@ int __init omap2_clk_init(void)
 		 * Update this if there are further clock changes between ES2
 		 * and production parts
 		 */
-		if (system_rev == OMAP3430_REV_ES1_0) {
+		if (omap_rev() == OMAP3430_REV_ES1_0) {
 			/* No 3430ES1-only rates exist, so no RATE_IN_3430ES1 */
 			cpu_clkflg |= CLOCK_IN_OMAP3430ES1;
 		} else {
arch/arm/mach-omap2/id.c +21 -13
index d4e3a38..b0f8e7d 100644
@@ -23,6 +23,14 @@
 #include <mach/cpu.h>
 
 static struct omap_chip_id omap_chip;
+static unsigned int omap_revision;
+
+
+unsigned int omap_rev(void)
+{
+	return omap_revision;
+}
+EXPORT_SYMBOL(omap_rev);
 
 /**
  * omap_chip_is - test whether currently running OMAP matches a chip type
@@ -50,7 +58,7 @@ EXPORT_SYMBOL(omap_chip_is);
 struct omap_id {
 	u16	hawkeye;	/* Silicon type (Hawkeye id) */
 	u8	dev;		/* Device type from production_id reg */
-	u32	type;		/* Combined type id copied to system_rev */
+	u32	type;		/* Combined type id copied to omap_revision */
 };
 
 /* Register values to detect the OMAP version */
@@ -116,9 +124,9 @@ void __init omap24xx_check_revision(void)
 		j = i;
 	}
 
-	pr_info("OMAP%04x", system_rev >> 16);
-	if ((system_rev >> 8) & 0x0f)
-		pr_info("ES%x", (system_rev >> 12) & 0xf);
+	pr_info("OMAP%04x", omap_rev() >> 16);
+	if ((omap_rev() >> 8) & 0x0f)
+		pr_info("ES%x", (omap_rev() >> 12) & 0xf);
 	pr_info("\n");
 }
 
@@ -136,7 +144,7 @@ void __init omap34xx_check_revision(void)
 	 */
 	cpuid = read_cpuid(CPUID_ID);
 	if ((((cpuid >> 4) & 0xfff) == 0xc08) && ((cpuid & 0xf) == 0x0)) {
-		system_rev = OMAP3430_REV_ES1_0;
+		omap_revision = OMAP3430_REV_ES1_0;
 		goto out;
 	}
 
@@ -153,26 +161,26 @@ void __init omap34xx_check_revision(void)
 	if (hawkeye == 0xb7ae) {
 		switch (rev) {
 		case 0:
-			system_rev = OMAP3430_REV_ES2_0;
+			omap_revision = OMAP3430_REV_ES2_0;
 			rev_name = "ES2.0";
 			break;
 		case 2:
-			system_rev = OMAP3430_REV_ES2_1;
+			omap_revision = OMAP3430_REV_ES2_1;
 			rev_name = "ES2.1";
 			break;
 		case 3:
-			system_rev = OMAP3430_REV_ES3_0;
+			omap_revision = OMAP3430_REV_ES3_0;
 			rev_name = "ES3.0";
 			break;
 		default:
 			/* Use the latest known revision as default */
-			system_rev = OMAP3430_REV_ES3_0;
+			omap_revision = OMAP3430_REV_ES3_0;
 			rev_name = "Unknown revision\n";
 		}
 	}
 
 out:
-	pr_info("OMAP%04x %s\n", system_rev >> 16, rev_name);
+	pr_info("OMAP%04x %s\n", omap_rev() >> 16, rev_name);
 }
 
 /*
@@ -203,9 +211,9 @@ void __init omap2_check_revision(void)
 		omap_chip.oc |= CHIP_IS_OMAP2420;
 	} else if (cpu_is_omap343x()) {
 		omap_chip.oc = CHIP_IS_OMAP3430;
-		if (system_rev == OMAP3430_REV_ES1_0)
+		if (omap_rev() == OMAP3430_REV_ES1_0)
 			omap_chip.oc |= CHIP_IS_OMAP3430ES1;
-		else if (system_rev > OMAP3430_REV_ES1_0)
+		else if (omap_rev() > OMAP3430_REV_ES1_0)
 			omap_chip.oc |= CHIP_IS_OMAP3430ES2;
 	} else {
 		pr_err("Uninitialized omap_chip, please fix!\n");
@@ -221,7 +229,7 @@ void __init omap2_check_revision(void)
  */
 void __init omap2_set_globals_tap(struct omap_globals *omap2_globals)
 {
-	system_rev = omap2_globals->class;
+	omap_revision = omap2_globals->class;
 	tap_base = omap2_globals->tap;
 
 	if (cpu_is_omap34xx())
arch/arm/plat-omap/include/mach/board-apollon.h +3 -1
index 731c858..61bd5e8 100644
@@ -29,12 +29,14 @@
 #ifndef __ASM_ARCH_OMAP_APOLLON_H
 #define __ASM_ARCH_OMAP_APOLLON_H
 
+#include <mach/cpu.h>
+
 extern void apollon_mmc_init(void);
 
 static inline int apollon_plus(void)
 {
 	/* The apollon plus has IDCODE revision 5 */
-	return system_rev & 0xc0;
+	return omap_rev() & 0xc0;
 }
 
 /* Placeholder for APOLLON specific defines */
arch/arm/plat-omap/include/mach/cpu.h +5 -5
index 4325bbc..b2062f1 100644
@@ -34,12 +34,12 @@ struct omap_chip_id {
 #define OMAP_CHIP_INIT(x)	{ .oc = x }
 
 /*
- * system_rev bits:
+ * omap_rev bits:
  * CPU id bits	(0730, 1510, 1710, 2422...)	[31:16]
  * CPU revision	(See _REV_ defined in cpu.h)	[15:08]
  * CPU class bits (15xx, 16xx, 24xx, 34xx...)	[07:00]
  */
-extern unsigned int system_rev;
+unsigned int omap_rev(void);
 
 /*
  * Test if multicore OMAP support is needed
@@ -113,7 +113,7 @@ extern unsigned int system_rev;
  * cpu_is_omap243x():	True for OMAP2430
  * cpu_is_omap343x():	True for OMAP3430
  */
-#define GET_OMAP_CLASS	(system_rev & 0xff)
+#define GET_OMAP_CLASS	(omap_rev() & 0xff)
 
 #define IS_OMAP_CLASS(class, id)			\
 static inline int is_omap ##class (void)		\
@@ -121,7 +121,7 @@ static inline int is_omap ##class (void)		\
 	return (GET_OMAP_CLASS == (id)) ? 1 : 0;	\
 }
 
-#define GET_OMAP_SUBCLASS	((system_rev >> 20) & 0x0fff)
+#define GET_OMAP_SUBCLASS	((omap_rev() >> 20) & 0x0fff)
 
 #define IS_OMAP_SUBCLASS(subclass, id)			\
 static inline int is_omap ##subclass (void)		\
@@ -231,7 +231,7 @@ IS_OMAP_SUBCLASS(343x, 0x343)
  * cpu_is_omap2430():	True for OMAP2430
  * cpu_is_omap3430():	True for OMAP3430
  */
-#define GET_OMAP_TYPE	((system_rev >> 16) & 0xffff)
+#define GET_OMAP_TYPE	((omap_rev() >> 16) & 0xffff)
 
 #define IS_OMAP_TYPE(type, id)				\
 static inline int is_omap ##type (void)			\
arch/arm/plat-omap/sram.c +2 -1
index 9f9a921..e1556f8 100644
@@ -24,6 +24,7 @@
 
 #include <mach/sram.h>
 #include <mach/board.h>
+#include <mach/cpu.h>
 
 #include <mach/control.h>
 
@@ -87,7 +88,7 @@ static int is_sram_locked(void)
 	int type = 0;
 
 	if (cpu_is_omap242x())
-		type = system_rev & OMAP2_DEVICETYPE_MASK;
+		type = omap_rev() & OMAP2_DEVICETYPE_MASK;
 
 	if (type == GP_DEVICE) {
 		/* RAMFW: R/W access to all initiators for all qualifier sets */
author maintainer
#4 lore raw

PATCH 4/6 ARM: OMAP2: bard-h4: list those eeproms

In reply to Tony Lindgren

From: David Brownell <[email protected]>

Declare the two 1Kbit EEPROMs included in the H4 board stack. One is on the CPU card; the other is on the mainboard.

---
 arch/arm/mach-omap2/board-h4.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
arch/arm/mach-omap2/board-h4.c +14 -0
index 2fef2c8..7de0506 100644
@@ -19,6 +19,7 @@
 #include <linux/delay.h>
 #include <linux/workqueue.h>
 #include <linux/i2c.h>
+#include <linux/i2c/at24.h>
 #include <linux/input.h>
 #include <linux/err.h>
 #include <linux/clk.h>
@@ -392,11 +393,24 @@ static struct omap_board_config_kernel h4_config[] = {
 	{ OMAP_TAG_LCD,		&h4_lcd_config },
 };
 
+static struct at24_platform_data m24c01 = {
+	.byte_len	= SZ_1K / 8,
+	.page_size	= 16,
+};
+
 static struct i2c_board_info __initdata h4_i2c_board_info[] = {
 	{
 		I2C_BOARD_INFO("isp1301_omap", 0x2d),
 		.irq		= OMAP_GPIO_IRQ(125),
 	},
+	{	/* EEPROM on mainboard */
+		I2C_BOARD_INFO("24c01", 0x52),
+		.platform_data	= &m24c01,
+	},
+	{	/* EEPROM on cpu card */
+		I2C_BOARD_INFO("24c01", 0x57),
+		.platform_data	= &m24c01,
+	},
 };
 
 static void __init omap_h4_init(void)
author maintainer
#5 lore raw

PATCH 5/6 ARM: OMAP2: drop redundant pending write check for gptimer

In reply to Tony Lindgren

From: Paul Walmsley <[email protected]>

omap_dm_timer_write_reg() already waits for pending writes to complete, so the extra wait in omap_dm_timer_set_load() is superfluous.

---
 arch/arm/plat-omap/dmtimer.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)
arch/arm/plat-omap/dmtimer.c +0 -4
index 963c31c..595e3d5 100644
@@ -539,10 +539,6 @@ void omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload,
 	omap_dm_timer_write_reg(timer, OMAP_TIMER_CTRL_REG, l);
 	omap_dm_timer_write_reg(timer, OMAP_TIMER_LOAD_REG, load);
 
-	/* REVISIT: hw feature, ttgr overtaking tldr? */
-	while (readl(timer->io_base + (OMAP_TIMER_WRITE_PEND_REG & 0xff)))
-		cpu_relax();
-
 	omap_dm_timer_write_reg(timer, OMAP_TIMER_TRIGGER_REG, 0);
 }
 
author maintainer
#11 lore raw

PATCH 6/6 ARM: OMAP2: skip unnecessary TLDR write during non-autoreload for gptimer

In reply to Tony Lindgren

From: Paul Walmsley <[email protected]>

The GPTIMER TLDR register does not need to be written if the GPTIMER is not in autoreload mode. This is the usual case for dynamic tick-enabled kernels.

Simulation data indicate that skipping the read that occurs as part of the write should save at least 300-320 ns for each GPTIMER1 timer reprogram. (This assumes L4-Wakeup is at 19MHz and GPTIMER write posting is enabled.) Skipping the write itself probably won't have much impact since it should be posted on the OCP interconnect.

Tested on 2430SDP and 3430SDP.

---
 arch/arm/plat-omap/dmtimer.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
arch/arm/plat-omap/dmtimer.c +4 -3
index 595e3d5..e4f0ce0 100644
@@ -549,14 +549,15 @@ void omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload,
 	u32 l;
 
 	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
-	if (autoreload)
+	if (autoreload) {
 		l |= OMAP_TIMER_CTRL_AR;
-	else
+		omap_dm_timer_write_reg(timer, OMAP_TIMER_LOAD_REG, load);
+	} else {
 		l &= ~OMAP_TIMER_CTRL_AR;
+	}
 	l |= OMAP_TIMER_CTRL_ST;
 
 	omap_dm_timer_write_reg(timer, OMAP_TIMER_COUNTER_REG, load);
-	omap_dm_timer_write_reg(timer, OMAP_TIMER_LOAD_REG, load);
 	omap_dm_timer_write_reg(timer, OMAP_TIMER_CTRL_REG, l);
 }
 
#6 lore raw

Re: [PATCH 5/6] ARM: OMAP2: drop redundant pending write check for gptimer

In reply to Tony Lindgren
Quoted 2 lines · Tony Lindgren

omap_dm_timer_write_reg() already waits for pending writes to complete, so the extra wait in omap_dm_timer_set_load() is superfluous.

Is this right? I mean - do you have to wait for each individual write to each register to complete before writing the next (and different) register?

Given that the code waits for every write to complete, no wonder Richard Woodruff is complaining that the NOHZ code is poor (it isn't) - with three writes each waiting for the previous to complete- what was it, 78us each, to complete? That's 234us just to reprogram the timer hardware with the CPU spinning waiting for each of the posted writes to complete.

#7 lore raw

RE: [PATCH 5/6] ARM: OMAP2: drop redundant pending write check for gptimer

Quoted 8 lines · Tony Lindgren
Quoted 2 lines

omap_dm_timer_write_reg() already waits for pending writes to complete, so the extra wait in omap_dm_timer_set_load() is superfluous.

Is this right? I mean - do you have to wait for each individual write to each register to complete before writing the next (and different) register?

No, the effect is not as your guessing.

Each effected register has a posting bit in 3430 which is tracked (see TWPS register). You must be careful with reads or re-writes of the _same_ register.

Quoted 5 lines

Given that the code waits for every write to complete, no wonder Richard Woodruff is complaining that the NOHZ code is poor (it isn't) - with three writes each waiting for the previous to complete- what was it, 78us each, to complete? That's 234us just to reprogram the timer hardware with the CPU spinning waiting for each of the posted writes to complete.

In the above hack a while back I was wondering if 1 write was overtaking another causing some issue. That hack kept it working at that time and it doesn't affect functionality.

Yes, NOHZ is _poor_ today in respect to needless reprogramming. Code can be improved. I have sent Thomas a patch for the same which is in the MM tree for a while now.

Today on every interrupt entry (> 2.6.27) and exit (all versions) it can potentially reprogram the hardware with the _same_ event. If interrupts come in fast from a device (like MUSB) this will cause backing up at the posting during needless reprograms. You never get all the way out of the NOHZ code before another interrupt comes. This effect drops USB performance by 3x as shown by usbtest. A simple and correct tweak recovers that lost 3x in performance.

Regards,
Richard W.

#8 lore raw

Re: [PATCH 5/6] ARM: OMAP2: drop redundant pending write check for gptimer

Quoted 3 lines · Richard

Yes, NOHZ is _poor_ today in respect to needless reprogramming. Code can be improved. I have sent Thomas a patch for the same which is in the MM tree for a while now.

When I discussed this problem with Thomas, his reaction was rather negative due to there being conflicting requirements between this and ia64. And quite frankly I think that the OMAP behaviour is utter madness - taking as long as it does to update the timer registers means that no matter what you do, you _will_ be paying a penalty for that hardware.

Let's hope that later OMAP designs sanitise the timer hardware so it doesn't take stupid amounts of time to write to the registers, posted mode or not.

author maintainer
#9 lore raw

Re: [PATCH 5/6] ARM: OMAP2: drop redundant pending write check for gptimer

Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · Richard

Yes, NOHZ is _poor_ today in respect to needless reprogramming. Code can be improved. I have sent Thomas a patch for the same which is in the MM tree for a while now.

When I discussed this problem with Thomas, his reaction was rather negative due to there being conflicting requirements between this and ia64. And quite frankly I think that the OMAP behaviour is utter madness - taking as long as it does to update the timer registers means that no matter what you do, you _will_ be paying a penalty for that hardware.

Well still the timer code should be optimized for slower systems. The timer should not be reprogrammed during every device interrupt if the change is less than a jiffy. And going through the timer lists is not always needed, so Richard's patch tries to optimize that [1].

Quoted 3 lines

Let's hope that later OMAP designs sanitise the timer hardware so it doesn't take stupid amounts of time to write to the registers, posted mode or not.

Yeah. Richard has some profiling data showing you how much time is spent doing that, maybe post that again for reference? It's very interesting data.

Tony

[1] http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.28-rc2/2.6.28-rc2-mm1/broken-out/tick-schedc-suppress-needless-timer-reprogramming.patch

maintainer
#10 lore raw

Re: [PATCH 5/6] ARM: OMAP2: drop redundant pending write check for gptimer

In reply to Russell King - ARM Linux

Hello Thomas,

Quoted 8 lines · Russell King - ARM Linux
Quoted 3 lines · Richard

Yes, NOHZ is _poor_ today in respect to needless reprogramming. Code can be improved. I have sent Thomas a patch for the same which is in the MM tree for a while now.

When I discussed this problem with Thomas, his reaction was rather negative due to there being conflicting requirements between this and ia64.

Could you comment further on the problems with this patch from Richard Woodruff, linked at [1]? Looks like you had some comments which were not addressed, so, the patch at the bottom of this message reimplements Richard's original patch to try to take those into account. Does this updated patch address your concerns? Implementing something like this is a major win for OMAP and probably other platforms also.

...

1. http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.28-rc2/2.6.28-rc2-mm1/broken-out/tick-schedc-suppress-needless-timer-reprogramming.patch

- Paul

[PATCH] Suppress needless timer reprogramming (version 2)

<[email protected]>: patch reimplemented to respond to tglx comments from
http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.28-rc2/2.6.28-rc2-mm1/broken-out/tick-schedc-
This new patch has not been tested to determine whether it works as well as the original patch, will try to get Richard to confirm this.

...

From: "Woodruff, Richard" <[email protected]>

In my device I get many interrupts from a high speed USB device in a very short period of time. The system spends a lot of time reprogramming the hardware timer which is in a slower timing domain as compared to the CPU. This results in the CPU spending a huge amount of time waiting for the timer posting to be done. All of this reprogramming is useless as the wake up time has not changed.

As measured using ETM trace this drops my reprogramming penalty from almost 60% CPU load down to 15% during high interrupt rate. I can send traces to show this.

Suppress setting of duplicate timer event when timer already stopped. Timer programming can be very costly and can result in long cpu stall/wait times.

---
 kernel/time/tick-sched.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)
kernel/time/tick-sched.c +12 -2
index 342fc9c..0255cb1 100644
@@ -346,8 +346,18 @@ void tick_nohz_stop_sched_tick(int inidle)
 			/* Check, if the timer was already in the past */
 			if (hrtimer_active(&ts->sched_timer))
 				goto out;
-		} else if (!tick_program_event(expires, 0))
-				goto out;
+		} else if (ktime_equal(expires, dev->next_event)) {
+			/*
+			 * No point in reprogramming the timer, since
+			 * we'd be reprogramming it with the same
+			 * expiration.  [On some platforms (e.g., OMAP)
+			 * timer reprogramming is costly.]
+			 */
+			goto out;
+		} else if (!tick_program_event(expires, 0)) {
+			goto out;
+		}
+
 		/*
 		 * We are past the event already. So we crossed a
 		 * jiffie boundary. Update jiffies and raise the