Consolidation: move SMP local timers to driver/clocksource

arch/arm/kernel/time.c +6 -0

@@ -147,6 +148,19 @@ static int __init timer_init_syscore_ops(void)
 
 device_initcall(timer_init_syscore_ops);
 
+void (* __initdata arm_late_time_init)(void);
+
+static void __init __arm_late_time_init(void)
+{
+       if (arm_late_time_init)
+               arm_late_time_init();

Quoted 15 lines · Marc Zyngier

arch/arm/kernel/time.c +11 -0

@@ -147,6 +148,19 @@ static int __init timer_init_syscore_ops(void)
 
 device_initcall(timer_init_syscore_ops);
 
+void (* __initdata arm_late_time_init)(void);
+
+static void __init __arm_late_time_init(void)
+{
+       if (arm_late_time_init)
+               arm_late_time_init();
+
+#ifdef CONFIG_LOCAL_TIMER_DEVICES
+       early_platform_driver_register_all("localtimer");
+       early_platform_driver_probe("localtimer", 1, 0);
+#endif

arch/arm/kernel/time.c static int __init timer_init_syscore_ops(void) · line 166

Quoted 5 lines

arch/arm/kernel/time.c +2 -0

+}
+
 void __init time_init(void)
 {
        system_timer = machine_desc->timer;

Instead of having two separate global pointers, should we make the ARM specific callback a machine_desc callback?

Quoted 3 lines

Instead of having two separate global pointers, should we make the ARM specific callback a machine_desc callback?

Probably not. I only see this arm_late_time_init pointer as a temporary measure. The only user (shmobile) uses this hook to install its own early timers, and there is a patch floating around to move that code to time.c. Unless we have other uses for such a callback, of course.

Convert the Tegra platforms to use the new arm_smp_twd driver. Add the platform device and register it as early platform device.

arch/arm/mach-tegra/timer.c +0 -0

index 9035042..4e1ecd0 100644

Marc Zyngier wrote at Thursday, June 16, 2011 1:07 PM:

Quoted 2 lines

Convert the Tegra platforms to use the new arm_smp_twd driver. Add the platform device and register it as early platform device.

...

Quoted 4 lines

arch/arm/mach-tegra/timer.c +0 -0

index 9035042..4e1ecd0 100644

...

arch/arm/mach-tegra/timer.c static struct irqaction tegra_timer_irq = { · line 204

Quoted 3 lines

arch/arm/mach-tegra/timer.c +3 -0

+static struct platform_device *tegra_early_devices[] = {
+	&tegra_twd_device,
+};

In the cases where the timer device is registered within a timer-specific file, as here, and hence is likely the only device to be registered, does it make sense to elide the platform_device array above, and ...

...

arch/arm/mach-tegra/timer.c static void __init tegra_init_timer(void) · line 266

Quoted 7 lines

arch/arm/mach-tegra/timer.c +2 -0

@@ -241,6 +261,8 @@ static void __init tegra_init_timer(void)
 	tegra_clockevent.cpumask = cpu_all_mask;
 	tegra_clockevent.irq = tegra_timer_irq.irq;
 	clockevents_register_device(&tegra_clockevent);
+	early_platform_add_devices(tegra_early_devices,
+				   ARRAY_SIZE(tegra_early_devices));
 }

... and just refer to the single device directly here:

early_platform_add_devices(&tegra_twd_device, 1);

?

Convert the OMAP4 platforms to use the new arm_smp_twd driver. Add the platform device and register it as early platform device.

Tested on a Pandaboard.

Cc Tony Lindgren<[email protected]>

Cc Santosh Shilimkar<[email protected]>

Signed-off-by Marc Zyngier<[email protected]>

---
  arch/arm/mach-omap2/Kconfig     |    3 +-
  arch/arm/mach-omap2/Makefile    |    1 -
  arch/arm/mach-omap2/timer-gp.c  |   40 ++++++++++++++++++++++++++++++++------
  arch/arm/mach-omap2/timer-mpu.c |   40 ---------------------------------------
  4 files changed, 35 insertions(+), 49 deletions(-)

delete mode 100644 arch/arm/mach-omap2/timer-mpu.c

arch/arm/mach-omap2/Kconfig +2 -1

index 19d5891..805d7b9 100644
@@ -44,7 +44,8 @@ config ARCH_OMAP4
  	depends on ARCH_OMAP2PLUS
  	select CPU_V7
  	select ARM_GIC
-	select LOCAL_TIMERS if SMP
+	select LOCAL_TIMER_DEVICES if SMP
+	select ARM_SMP_TWD
  	select PL310_ERRATA_588369
  	select PL310_ERRATA_727915
  	select ARM_ERRATA_720789

arch/arm/mach-omap2/Makefile +0 -1

index b148077..f48c989 100644
@@ -22,7 +22,6 @@ obj-$(CONFIG_TWL4030_CORE) += omap_twl.o
 
  # SMP support ONLY available for OMAP4
  obj-$(CONFIG_SMP)			+= omap-smp.o omap-headsmp.o
-obj-$(CONFIG_LOCAL_TIMERS)		+= timer-mpu.o
  obj-$(CONFIG_HOTPLUG_CPU)		+= omap-hotplug.o
  obj-$(CONFIG_ARCH_OMAP4)		+= omap44xx-smc.o omap4-common.o
 

arch/arm/mach-omap2/timer-gp.c +8 -1

index 3b9cf85..f2f0db6 100644
@@ -35,10 +35,10 @@
  #include<linux/irq.h>
  #include<linux/clocksource.h>
  #include<linux/clockchips.h>
+#include<linux/platform_device.h>
 
  #include<asm/mach/time.h>
  #include<plat/dmtimer.h>
-#include<asm/localtimer.h>
  #include<asm/sched_clock.h>
  #include<plat/common.h>
  #include<plat/omap_hwmod.h>

@@ -247,18 +247,44 @@ static void __init omap2_gp_clocksource_init(void)
  }
  #endif
 
+#ifdef CONFIG_ARM_SMP_TWD
+static struct resource omap2_gp_twd_resources[] = {
+	{
+		.start	= OMAP44XX_LOCAL_TWD_BASE,
+		.end	= OMAP44XX_LOCAL_TWD_BASE + 0x10,
+		.flags	= IORESOURCE_MEM,
+	},

arch/arm/mach-omap2/timer-gp.c +9 -6

+	.resource	= omap2_gp_twd_resources,
+	.num_resources	= ARRAY_SIZE(omap2_gp_twd_resources),
+};
+
+static struct platform_device *omap2_gp_early_devices[] = {
+	&omap2_gp_twd_device,
+};
+#endif
+
  static void __init omap2_gp_timer_init(void)
  {
-#ifdef CONFIG_LOCAL_TIMERS
-	if (cpu_is_omap44xx()) {
-		twd_base = ioremap(OMAP44XX_LOCAL_TWD_BASE, SZ_256);
-		BUG_ON(!twd_base);
-	}
-#endif
  	omap_dm_timer_init();
 

arch/arm/mach-omap2/timer-gp.c +0 -0

  struct sys_timer omap_timer = {

arch/arm/mach-omap2/timer-mpu.c deleted +0 -30

deleted file mode 100644
index 1fd5ca5..0000000
@@ -1,40 +0,0 @@
-/*
- * The MPU local timer source file. In OMAP4, both cortex-a9 cores have
- * own timer in it's MPU domain. These timers will be driving the
- * linux kernel SMP tick framework when active. These timers are not
- * part of the wake up domain.
- *
- * Copyright (C) 2009 Texas Instruments, Inc.
- *
- * Author:
- *      Santosh Shilimkar<[email protected]>
- *
- * This file is based on arm realview smp platform file.
- * Copyright (C) 2002 ARM Ltd.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-#include<linux/init.h>
-#include<linux/smp.h>
-#include<linux/clockchips.h>
-#include<asm/irq.h>
-#include<asm/smp_twd.h>
-#include<asm/localtimer.h>
-#include<asm/hardware/gic.h>
-
-/*
- * Setup the local clock events for a CPU.
- */
-int __cpuinit local_timer_setup(struct clock_event_device *evt)

Quoted 72 lines · Marc Zyngier

Convert the OMAP4 platforms to use the new arm_smp_twd driver. Add the platform device and register it as early platform device.

Tested on a Pandaboard.

Cc Tony Lindgren<[email protected]>

Cc Santosh Shilimkar<[email protected]>

Signed-off-by Marc Zyngier<[email protected]>

---
  arch/arm/mach-omap2/Kconfig     |    3 +-
  arch/arm/mach-omap2/Makefile    |    1 -
  arch/arm/mach-omap2/timer-gp.c  |   40 ++++++++++++++++++++++++++++++++------
  arch/arm/mach-omap2/timer-mpu.c |   40 ---------------------------------------
  4 files changed, 35 insertions(+), 49 deletions(-)

delete mode 100644 arch/arm/mach-omap2/timer-mpu.c

arch/arm/mach-omap2/Kconfig +2 -1

index 19d5891..805d7b9 100644
@@ -44,7 +44,8 @@ config ARCH_OMAP4
  	depends on ARCH_OMAP2PLUS
  	select CPU_V7
  	select ARM_GIC
-	select LOCAL_TIMERS if SMP
+	select LOCAL_TIMER_DEVICES if SMP
+	select ARM_SMP_TWD
  	select PL310_ERRATA_588369
  	select PL310_ERRATA_727915
  	select ARM_ERRATA_720789

arch/arm/mach-omap2/Makefile +0 -1

index b148077..f48c989 100644
@@ -22,7 +22,6 @@ obj-$(CONFIG_TWL4030_CORE) += omap_twl.o
 
  # SMP support ONLY available for OMAP4
  obj-$(CONFIG_SMP)			+= omap-smp.o omap-headsmp.o
-obj-$(CONFIG_LOCAL_TIMERS)		+= timer-mpu.o
  obj-$(CONFIG_HOTPLUG_CPU)		+= omap-hotplug.o
  obj-$(CONFIG_ARCH_OMAP4)		+= omap44xx-smc.o omap4-common.o
 

arch/arm/mach-omap2/timer-gp.c +11 -1

index 3b9cf85..f2f0db6 100644
@@ -35,10 +35,10 @@
  #include<linux/irq.h>
  #include<linux/clocksource.h>
  #include<linux/clockchips.h>
+#include<linux/platform_device.h>
 
  #include<asm/mach/time.h>
  #include<plat/dmtimer.h>
-#include<asm/localtimer.h>
  #include<asm/sched_clock.h>
  #include<plat/common.h>
  #include<plat/omap_hwmod.h>

@@ -247,18 +247,44 @@ static void __init omap2_gp_clocksource_init(void)
  }
  #endif
 
+#ifdef CONFIG_ARM_SMP_TWD
+static struct resource omap2_gp_twd_resources[] = {
+	{
+		.start	= OMAP44XX_LOCAL_TWD_BASE,
+		.end	= OMAP44XX_LOCAL_TWD_BASE + 0x10,
+		.flags	= IORESOURCE_MEM,
+	},
+	{
+		.start	= OMAP44XX_IRQ_LOCALTIMER,
+		.end	= OMAP44XX_IRQ_LOCALTIMER,

arch/arm/mach-omap2/timer-gp.c static void __init omap2_gp_clocksource_init(void) · line 266

Quoted 7 lines

arch/arm/mach-omap2/timer-gp.c +7 -0

+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device omap2_gp_twd_device = {
+	.name		= "arm_smp_twd",
+	.id		= -1,

Why is the id set to -1 ?

Quoted 19 lines

arch/arm/mach-omap2/timer-gp.c +9 -6

+	.resource	= omap2_gp_twd_resources,
+	.num_resources	= ARRAY_SIZE(omap2_gp_twd_resources),
+};
+
+static struct platform_device *omap2_gp_early_devices[] = {
+	&omap2_gp_twd_device,
+};
+#endif
+
  static void __init omap2_gp_timer_init(void)
  {
-#ifdef CONFIG_LOCAL_TIMERS
-	if (cpu_is_omap44xx()) {
-		twd_base = ioremap(OMAP44XX_LOCAL_TWD_BASE, SZ_256);
-		BUG_ON(!twd_base);
-	}
-#endif
  	omap_dm_timer_init();
 

arch/arm/mach-omap2/timer-gp.c static void __init omap2_gp_clocksource_init(void) · line 289

Quoted 10 lines

arch/arm/mach-omap2/timer-gp.c +6 -0

  	omap2_gp_clockevent_init();
  	omap2_gp_clocksource_init();
+#ifdef CONFIG_ARM_SMP_TWD
+	/* Local timers are not supprted on OMAP4430 ES1.0 */
+	if (omap_rev() != OMAP4430_REV_ES1_0)
+		early_platform_add_devices(omap2_gp_early_devices,
+					   ARRAY_SIZE(omap2_gp_early_devices));
+#endif
  }
 

These changes looks good.

Quoted 37 lines

arch/arm/mach-omap2/timer-gp.c +0 -0

  struct sys_timer omap_timer = {

arch/arm/mach-omap2/timer-mpu.c deleted +0 -30

deleted file mode 100644
index 1fd5ca5..0000000
@@ -1,40 +0,0 @@
-/*
- * The MPU local timer source file. In OMAP4, both cortex-a9 cores have
- * own timer in it's MPU domain. These timers will be driving the
- * linux kernel SMP tick framework when active. These timers are not
- * part of the wake up domain.
- *
- * Copyright (C) 2009 Texas Instruments, Inc.
- *
- * Author:
- *      Santosh Shilimkar<[email protected]>
- *
- * This file is based on arm realview smp platform file.
- * Copyright (C) 2002 ARM Ltd.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-#include<linux/init.h>
-#include<linux/smp.h>
-#include<linux/clockchips.h>
-#include<asm/irq.h>
-#include<asm/smp_twd.h>
-#include<asm/localtimer.h>
-#include<asm/hardware/gic.h>
-
-/*
- * Setup the local clock events for a CPU.
- */
-int __cpuinit local_timer_setup(struct clock_event_device *evt)

arch/arm/mach-omap2/timer-mpu.c line 40

Quoted 10 lines

arch/arm/mach-omap2/timer-mpu.c deleted +0 -10

-{
-	/* Local timers are not supprted on OMAP4430 ES1.0 */
-	if (omap_rev() == OMAP4430_REV_ES1_0)
-		return -ENXIO;
-
-	evt->irq = gic_ppi_to_vppi(OMAP44XX_IRQ_LOCALTIMER);
-	twd_timer_setup(evt);
-	return 0;
-}
-

And this one is even better. -40 lines from arch/arm/*omap*/

I am fine with all changes except the id value. With that clarified,

Reviewedd-by Santosh Shilimkar <[email protected]>

Hi Santosh,

Quoted 73 lines · Santosh Shilimkar

Quoted 72 lines · Marc Zyngier

Convert the OMAP4 platforms to use the new arm_smp_twd driver. Add the platform device and register it as early platform device.

Tested on a Pandaboard.

Cc Tony Lindgren<[email protected]>

Cc Santosh Shilimkar<[email protected]>

Signed-off-by Marc Zyngier<[email protected]>

---
   arch/arm/mach-omap2/Kconfig     |    3 +-
   arch/arm/mach-omap2/Makefile    |    1 -
   arch/arm/mach-omap2/timer-gp.c  |   40 ++++++++++++++++++++++++++++++++------
   arch/arm/mach-omap2/timer-mpu.c |   40 ---------------------------------------
   4 files changed, 35 insertions(+), 49 deletions(-)

delete mode 100644 arch/arm/mach-omap2/timer-mpu.c

arch/arm/mach-omap2/Kconfig +2 -1

index 19d5891..805d7b9 100644
@@ -44,7 +44,8 @@ config ARCH_OMAP4
   	depends on ARCH_OMAP2PLUS
   	select CPU_V7
   	select ARM_GIC
-	select LOCAL_TIMERS if SMP
+	select LOCAL_TIMER_DEVICES if SMP
+	select ARM_SMP_TWD
   	select PL310_ERRATA_588369
   	select PL310_ERRATA_727915
   	select ARM_ERRATA_720789

arch/arm/mach-omap2/Makefile +0 -1

index b148077..f48c989 100644
@@ -22,7 +22,6 @@ obj-$(CONFIG_TWL4030_CORE) += omap_twl.o
 
   # SMP support ONLY available for OMAP4
   obj-$(CONFIG_SMP)			+= omap-smp.o omap-headsmp.o
-obj-$(CONFIG_LOCAL_TIMERS)		+= timer-mpu.o
   obj-$(CONFIG_HOTPLUG_CPU)		+= omap-hotplug.o
   obj-$(CONFIG_ARCH_OMAP4)		+= omap44xx-smc.o omap4-common.o
 

arch/arm/mach-omap2/timer-gp.c +11 -1

index 3b9cf85..f2f0db6 100644
@@ -35,10 +35,10 @@
   #include<linux/irq.h>
   #include<linux/clocksource.h>
   #include<linux/clockchips.h>
+#include<linux/platform_device.h>
 
   #include<asm/mach/time.h>
   #include<plat/dmtimer.h>
-#include<asm/localtimer.h>
   #include<asm/sched_clock.h>
   #include<plat/common.h>
   #include<plat/omap_hwmod.h>

@@ -247,18 +247,44 @@ static void __init omap2_gp_clocksource_init(void)
   }
   #endif
 
+#ifdef CONFIG_ARM_SMP_TWD
+static struct resource omap2_gp_twd_resources[] = {
+	{
+		.start	= OMAP44XX_LOCAL_TWD_BASE,
+		.end	= OMAP44XX_LOCAL_TWD_BASE + 0x10,
+		.flags	= IORESOURCE_MEM,
+	},
+	{
+		.start	= OMAP44XX_IRQ_LOCALTIMER,
+		.end	= OMAP44XX_IRQ_LOCALTIMER,

Quoted 8 lines

arch/arm/mach-omap2/timer-gp.c static void __init omap2_gp_clocksource_init(void) · line 266

Quoted 7 lines

arch/arm/mach-omap2/timer-gp.c +7 -0

+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct platform_device omap2_gp_twd_device = {
+	.name		= "arm_smp_twd",
+	.id		= -1,

Why is the id set to -1 ?

This is the normal way of indicating that there is only one such device in the system. The device ends being called "arm_smp_twd" instead of "arm_smp_twd.0". See Documentation/driver-model/platform.txt for details.

Add a "new" driver to driver/clocksource to support the Samsung MCT hardware. This is basically the local timer part of arch/arm/mach-exynos4/mct.c, turned into a platform driver, and with a CPU notifier being used to start/stop timers on secondary cores.

Cc Kukjin Kim <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Quoted 7 lines · Marc Zyngier

Add a "new" driver to driver/clocksource to support the Samsung MCT hardware. This is basically the local timer part of arch/arm/mach-exynos4/mct.c, turned into a platform driver, and with a CPU notifier being used to start/stop timers on secondary cores.

Quoted 2 lines

Cc Kukjin Kim <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

My feeling is that it would be nicer to do this as an atomic change together with the removal of the existing code, rather than two patches.

As the life expectancy of asm/smp_twd.h seems rather limited, move the few defines concerning mpcore_wdt to the driver itself.

Change the dependency to ARM_SMP_TWD as well.

Cc Wim Van Sebroeck <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Hi Marc,

Quoted 3 lines

As the life expectancy of asm/smp_twd.h seems rather limited, move the few defines concerning mpcore_wdt to the driver itself.

Quoted 4 lines

Change the dependency to ARM_SMP_TWD as well.

Cc Wim Van Sebroeck <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

Signed-off-by Wim Van Sebroeck <[email protected]>

I presume you Will keep this patch together with the other patches?

Patches against next-20110615 + my earlier GIC-PPI series. Tested on VExpress, PB11MP, Pandaboard, Tegra Harmony and Exynos4 SMDK-v310.

Thanks for an excellent patch set Marc, take that as an Acked-by from me FWIW.

Quoted 2 lines · Marc Zyngier

Patches against next-20110615 + my earlier GIC-PPI series. Tested on VExpress, PB11MP, Pandaboard, Tegra Harmony and Exynos4 SMDK-v310.

This is making it hopeless for me to test on a plain -rc3 kernel, do you have a public git branch I can just pull in to get all this?

I would like to test on U8500 and also rebase Colins TWD CPUfreq patch on top of this so it can go in on top of the consolidations.

[Me]

Quoted 3 lines

I would like to test on U8500 and also rebase Colins TWD CPUfreq patch on top of this so it can go in on top of the consolidations.

I've created a new repository on git.kernel.org:
git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
and rebased my stuff on top of -rc3 in the "local_timer_devices" branch.

Patches against next-20110615 + my earlier GIC-PPI series. Tested on VExpress, PB11MP, Pandaboard, Tegra Harmony and Exynos4 SMDK-v310.

Thomas and Arnd cc-ed on the whole series, as this could be consolidation material.

Quoted 3 lines · Marc Zyngier

Patches against next-20110615 + my earlier GIC-PPI series. Tested on VExpress, PB11MP, Pandaboard, Tegra Harmony and Exynos4 SMDK-v310.

Quoted 2 lines

Thomas and Arnd cc-ed on the whole series, as this could be consolidation material.

Very nice stuff. Should we take this all in one branch in the arm-soc tree, pending Acks from the subarch maintainers?

We could also push it through the individual subarch branches, but my feeling is that would be more work.

Quoted 3 lines · Marc Zyngier

Patches against next-20110615 + my earlier GIC-PPI series. Tested on VExpress, PB11MP, Pandaboard, Tegra Harmony and Exynos4 SMDK-v310.

Quoted 2 lines

Thomas and Arnd cc-ed on the whole series, as this could be consolidation material.

Very nice stuff. Should we take this all in one branch in the arm-soc tree, pending Acks from the subarch maintainers?

We could also push it through the individual subarch branches, but my feeling is that would be more work.

Arnd,

Quoted 4 lines · Arnd Bergmann

Quoted 3 lines · Marc Zyngier

Patches against next-20110615 + my earlier GIC-PPI series. Tested on VExpress, PB11MP, Pandaboard, Tegra Harmony and Exynos4 SMDK-v310.

Quoted 9 lines

Quoted 2 lines

Thomas and Arnd cc-ed on the whole series, as this could be consolidation material.

Very nice stuff. Should we take this all in one branch in the arm-soc tree, pending Acks from the subarch maintainers?

We could also push it through the individual subarch branches, but my feeling is that would be more work.

If this is the direction to go, then there are a lot of other timers that can be moved besides just local timers. But Russell didn't really like the shmobile patches for early platform devices which this relies on. So we should get agreement on the small bit of infrastructure needed first, then start moving individual timers.

Also, device tree support for early platform devices would be useful for this.

Quoted 5 lines · Rob Herring

If this is the direction to go, then there are a lot of other timers that can be moved besides just local timers. But Russell didn't really like the shmobile patches for early platform devices which this relies on. So we should get agreement on the small bit of infrastructure needed first, then start moving individual timers.

I'm open to suggestions. Should the early platform infrastructure be flagged as unsuitable, I'd be happy to switch to something else, as long as it is minimal, and does not create another ARM-specific API (been there, done that...).

Quoted 11 lines · Marc Zyngier

Quoted 5 lines · Rob Herring

If this is the direction to go, then there are a lot of other timers that can be moved besides just local timers. But Russell didn't really like the shmobile patches for early platform devices which this relies on. So we should get agreement on the small bit of infrastructure needed first, then start moving individual timers.

I'm open to suggestions. Should the early platform infrastructure be flagged as unsuitable, I'd be happy to switch to something else, as long as it is minimal, and does not create another ARM-specific API (been there, done that...).

I am most certainly not a fan of the early platform device stuff as I've said in the past.

It looks to me like:

1. it's overcomplex and fragile.
2. it abuses the early command line system.
3. it abuses platform devices etc.
4. it requires more initialization calls.

Lastly, and probably more importantly for the direction we're going, I can't see how it would hook into DT other than using the DT provided command line, which also appears to be an abuse there too.

Surely there's a better solution.

Quoted 11 lines · Marc Zyngier

Quoted 5 lines · Rob Herring

If this is the direction to go, then there are a lot of other timers that can be moved besides just local timers. But Russell didn't really like the shmobile patches for early platform devices which this relies on. So we should get agreement on the small bit of infrastructure needed first, then start moving individual timers.

I'm open to suggestions. Should the early platform infrastructure be flagged as unsuitable, I'd be happy to switch to something else, as long as it is minimal, and does not create another ARM-specific API (been there, done that...).

I am most certainly not a fan of the early platform device stuff as I've said in the past.

It looks to me like:

1. it's overcomplex and fragile.
2. it abuses the early command line system.
3. it abuses platform devices etc.
4. it requires more initialization calls.

Lastly, and probably more importantly for the direction we're going, I can't see how it would hook into DT other than using the DT provided command line, which also appears to be an abuse there too.

Quoted 18 lines · Russell King - ARM Linux

Quoted 11 lines · Marc Zyngier

Quoted 5 lines · Rob Herring

If this is the direction to go, then there are a lot of other timers that can be moved besides just local timers. But Russell didn't really like the shmobile patches for early platform devices which this relies on. So we should get agreement on the small bit of infrastructure needed first, then start moving individual timers.

I'm open to suggestions. Should the early platform infrastructure be flagged as unsuitable, I'd be happy to switch to something else, as long as it is minimal, and does not create another ARM-specific API (been there, done that...).

I am most certainly not a fan of the early platform device stuff as I've said in the past.

It looks to me like:

Quoted 8 lines

1. it's overcomplex and fragile.
2. it abuses the early command line system.
3. it abuses platform devices etc.
4. it requires more initialization calls.

Lastly, and probably more importantly for the direction we're going, I can't see how it would hook into DT other than using the DT provided command line, which also appears to be an abuse there too.

I'm going to second that opinion. ?I just looked through the early platform stuff, and I'm not thrilled with the model.

Setting up timers does not need to plug into the device model. ?Unless I'm completely reading the ARM code wrong, these timers are pretty fundamental to the operating system, and thus aren't really like other devices. ?I don't see any problem with individual timer init functions populated in machine_desc assuming that multiple boards with the same SoC will not require different timer initialization.

Also, device tree support for early platform devices would be useful for this.

Quoted 2 lines · Rob Herring

Also, device tree support for early platform devices would be useful for this.

What about the attached patch? I've used it to boot a VExpress board with the following DT snippet (and the corresponding support in the amp_smp_twd driver):

	localtimer at 2c000600 {
		device_type = "localtimer";

		compatible = "arm,smp-twd";
		reg = <0x2c000600 0x1000>;
		interrupts = <29>;
	};

Cheers,

	M.

Marc,

Quoted 8 lines · Marc Zyngier

Quoted 2 lines · Rob Herring

Also, device tree support for early platform devices would be useful for this.

What about the attached patch? I've used it to boot a VExpress board with the following DT snippet (and the corresponding support in the amp_smp_twd driver):

Quoted 2 lines

	localtimer at 2c000600 {
		device_type = "localtimer";

My understanding is device_type is only supposed to be used for devices with real OpenFirmware. You could just add localtimer or timer as a 2nd compatible string. I think localtimer is a bit of a Linux name, so just timer may be better.

Quoted 14 lines · Rob Herring

Marc,

Quoted 10 lines · Marc Zyngier

Quoted 2 lines · Rob Herring

Also, device tree support for early platform devices would be useful for this.

What about the attached patch? I've used it to boot a VExpress board with the following DT snippet (and the corresponding support in the amp_smp_twd driver):

	localtimer at 2c000600 {
		device_type = "localtimer";

Quoted 4 lines

My understanding is device_type is only supposed to be used for devices with real OpenFirmware. You could just add localtimer or timer as a 2nd compatible string. I think localtimer is a bit of a Linux name, so just timer may be better.

Using "compatible" would not work. You'd have to add the same compatible string to all drivers, and get drivers probing the wrong device. Any other property we could use instead?

As for the "localtimer" string, this is to differentiate the per-cpu timers versus the global timers, and they may need to be probed in a specific order (global first, and then local).

The reason for this is that TWD needs to be calibrated, hence the need for a global timer running. Colin's patch could help getting rid of this restriction though.

	M.

Quoted 15 lines · Marc Zyngier

Quoted 14 lines · Rob Herring

Marc,

Quoted 10 lines · Marc Zyngier

Quoted 2 lines · Rob Herring

Also, device tree support for early platform devices would be useful for this.

What about the attached patch? I've used it to boot a VExpress board with the following DT snippet (and the corresponding support in the amp_smp_twd driver):

	localtimer at 2c000600 {
		device_type = "localtimer";

Quoted 8 lines

Quoted 4 lines

My understanding is device_type is only supposed to be used for devices with real OpenFirmware. You could just add localtimer or timer as a 2nd compatible string. I think localtimer is a bit of a Linux name, so just timer may be better.

Using "compatible" would not work. You'd have to add the same compatible string to all drivers, and get drivers probing the wrong device. Any other property we could use instead?

No, I'm saying you have a compatible property like this:

compatible = "arm,smp-twd", "timer";

You create the early platform device(s) based on matches with "timer", but match the driver based on "arm,smp-twd".

Very nice stuff. Should we take this all in one branch in the arm-soc tree, pending Acks from the subarch maintainers?

We could also push it through the individual subarch branches, but my feeling is that would be more work.