arm:omap1/2/3/4: 32k sync timer cleanup

Convert counter_32k clocksource driver to standard platform_driver and move it drivers/clocksource/ directory.

Also, rename it to more generic name "omap-32k.c".

Quoted 4 lines · Vaibhav Hiremath

Convert counter_32k clocksource driver to standard platform_driver and move it drivers/clocksource/ directory.

Also, rename it to more generic name "omap-32k.c".

NAK. sched_clock is supposed to be available early. Platform device driver initialization is FAR too late.

Note that some of the code you're moving has changed in current kernels.

Quoted 9 lines · Russell King - ARM Linux

Quoted 4 lines · Vaibhav Hiremath

Convert counter_32k clocksource driver to standard platform_driver and move it drivers/clocksource/ directory.

Also, rename it to more generic name "omap-32k.c".

NAK. sched_clock is supposed to be available early. Platform device driver initialization is FAR too late.

Russell,

Sched_clock is available very early during boot sequence. Initially gp-timer (dmtimer) will get registered as a clocksource. Please refer to the file mach-omap2/timer.c

32k_sync timer (omap-32k.c) will come get registered during arch_init.

Just FYI, the way I tested it is, I used kernel parameter "clocksourse=counter-32k", the switch from gp-timer to 32k timer will happen once it gets registered.

Quoted 18 lines · Vaibhav

Quoted 9 lines · Russell King - ARM Linux

Quoted 4 lines · Vaibhav Hiremath

Convert counter_32k clocksource driver to standard platform_driver and move it drivers/clocksource/ directory.

Also, rename it to more generic name "omap-32k.c".

NAK. sched_clock is supposed to be available early. Platform device driver initialization is FAR too late.

Russell,

Sched_clock is available very early during boot sequence. Initially gp-timer (dmtimer) will get registered as a clocksource. Please refer to the file mach-omap2/timer.c

32k_sync timer (omap-32k.c) will come get registered during arch_init.

Quoted 3 lines

Just FYI, the way I tested it is, I used kernel parameter "clocksourse=counter-32k", the switch from gp-timer to 32k timer will happen once it gets registered.

So please delete the sched_clock code from the 32k timer stuff you've moved to a platform driver. It will cause sched_clock to reset to zero, and that's bad news.

Only one sched_clock() should ever be registered, and that should only be registered very early at boot time.

Quoted 16 lines · Vaibhav

Quoted 9 lines · Russell King - ARM Linux

Quoted 4 lines · Vaibhav Hiremath

Convert counter_32k clocksource driver to standard platform_driver and move it drivers/clocksource/ directory.

Also, rename it to more generic name "omap-32k.c".

NAK. sched_clock is supposed to be available early. Platform device driver initialization is FAR too late.

Russell,

Sched_clock is available very early during boot sequence. Initially gp-timer (dmtimer) will get registered as a clocksource. Please refer to the file mach-omap2/timer.c

Quoted 5 lines

32k_sync timer (omap-32k.c) will come get registered during arch_init.

Just FYI, the way I tested it is, I used kernel parameter "clocksourse=counter-32k", the switch from gp-timer to 32k timer will happen once it gets registered.

So please delete the sched_clock code from the 32k timer stuff you've moved to a platform driver. It will cause sched_clock to reset to zero, and that's bad news.

Only one sched_clock() should ever be registered, and that should only be registered very early at boot time.

Quoted 27 lines · Russell King - ARM Linux

Quoted 16 lines · Vaibhav

Quoted 9 lines · Russell King - ARM Linux

Quoted 4 lines · Vaibhav Hiremath

Convert counter_32k clocksource driver to standard platform_driver and move it drivers/clocksource/ directory.

Also, rename it to more generic name "omap-32k.c".

NAK. sched_clock is supposed to be available early. Platform device driver initialization is FAR too late.

Russell,

Sched_clock is available very early during boot sequence. Initially gp-timer (dmtimer) will get registered as a clocksource. Please refer to the file mach-omap2/timer.c

Quoted 5 lines

32k_sync timer (omap-32k.c) will come get registered during arch_init.

Just FYI, the way I tested it is, I used kernel parameter "clocksourse=counter-32k", the switch from gp-timer to 32k timer will happen once it gets registered.

So please delete the sched_clock code from the 32k timer stuff you've moved to a platform driver. It will cause sched_clock to reset to zero, and that's bad news.

Quoted 2 lines

Only one sched_clock() should ever be registered, and that should only be registered very early at boot time.

The kernel will WARN if two sched_clock() are registered. I hope this will be enough for people not to persist with such a thing...

From sched_clock perspective, we have only one function exported in

	M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Quoted 28 lines · Marc Zyngier

Quoted 27 lines · Russell King - ARM Linux

Quoted 16 lines · Vaibhav

Quoted 9 lines · Russell King - ARM Linux

Quoted 4 lines · Vaibhav Hiremath

Convert counter_32k clocksource driver to standard platform_driver and move it drivers/clocksource/ directory.

Also, rename it to more generic name "omap-32k.c".

NAK. sched_clock is supposed to be available early. Platform device driver initialization is FAR too late.

Russell,

Sched_clock is available very early during boot sequence. Initially gp-timer (dmtimer) will get registered as a clocksource. Please refer to the file mach-omap2/timer.c

Quoted 5 lines

32k_sync timer (omap-32k.c) will come get registered during arch_init.

Just FYI, the way I tested it is, I used kernel parameter "clocksourse=counter-32k", the switch from gp-timer to 32k timer will happen once it gets registered.

So please delete the sched_clock code from the 32k timer stuff you've moved to a platform driver. It will cause sched_clock to reset to zero, and that's bad news.

Quoted 6 lines

Quoted 2 lines

Only one sched_clock() should ever be registered, and that should only be registered very early at boot time.

The kernel will WARN if two sched_clock() are registered. I hope this will be enough for people not to persist with such a thing...

Marc,

This code-snippet is not registering multiple sched_clock() functions, Here we have 2 timers
- gp-timer (available on all devices)
- 32k-sync timer (may not available on all devices, like AM33xx)

So we are registering both the timers as a clocksource and kernel Chooses one based on kernel-parameter or rating.
From sched_clock perspective, we have only one function exported in mach-omap2/timer.c, which in-turn calls omap-32k.c timer api.

Something like this,

unsigned long long notrace sched_clock(void)

{
	U32 cyc;

	/* First call 32k-sync timer sched_clock() */
	/* return 0 means, the device doesn't have 32k-sync timer available */
	cyc = sched_clock_32k();
	if (cyc)
		return cyc;

	/* Fall back to gp-timer approach */
}

So there is and will not be any warning from kernel here.

Can't you instead use the ARM sched_clock framework?

Quoted 1 line · Marc Zyngier

Can't you instead use the ARM sched_clock framework?

OMAP does, and has done for quite some time. The problem is these patches are against an old kernel.

Quoted 5 lines · Russell King - ARM Linux

Quoted 1 line · Marc Zyngier

Can't you instead use the ARM sched_clock framework?

OMAP does, and has done for quite some time. The problem is these patches are against an old kernel.

Additionally we're already handling the 32k timer or not case already on omap1 as it does not exist on 1510, so there should only minimal patching needed AFAIK.

Tony

drivers/clocksource/omap-32k.c new file +21 -0

+/**
+ * read_persistent_clock -  Return time from a persistent clock.
+ *
+ * Reads the time from a source which isn't disabled during PM, the
+ * 32k sync timer.  Convert the cycles elapsed since last read into
+ * nsecs and adds to a monotonically increasing timespec.
+ */
+void read_persistent_clock(struct timespec *ts)
+{
+	cycles_t delta;
+	struct timespec *tsp;
+	unsigned long long nsecs;
+	struct omap_counter_32k_device  *omap = cs;
+
+	if (!omap) {
+		ts->tv_sec = 0;
+		ts->tv_nsec = 0;
+		return;
+	}
+	tsp = &omap->persistent_ts;
+

Quoted 21 lines · Vaibhav Hiremath

drivers/clocksource/omap-32k.c new file +21 -0

+/**
+ * read_persistent_clock -  Return time from a persistent clock.
+ *
+ * Reads the time from a source which isn't disabled during PM, the
+ * 32k sync timer.  Convert the cycles elapsed since last read into
+ * nsecs and adds to a monotonically increasing timespec.
+ */
+void read_persistent_clock(struct timespec *ts)
+{
+	cycles_t delta;
+	struct timespec *tsp;
+	unsigned long long nsecs;
+	struct omap_counter_32k_device  *omap = cs;
+
+	if (!omap) {
+		ts->tv_sec = 0;
+		ts->tv_nsec = 0;
+		return;
+	}
+	tsp = &omap->persistent_ts;
+

drivers/clocksource/omap-32k.c line 111

Quoted 10 lines

drivers/clocksource/omap-32k.c new file +10 -0

+	omap->last_cycles = omap->cycles;
+	omap->cycles = omap->cs.read(&omap->cs);
+	delta = omap->cycles - omap->last_cycles;
+
+	nsecs = clocksource_cyc2ns(delta,
+			omap->cs.mult, omap->cs.shift);
+
+	timespec_add_ns(tsp, nsecs);
+	*ts = *tsp;
+}

Hrm. So read_persistent_clock should probably be defined once per arch. So I'm not sure if it makes sense to include this implementation into the generic drivers/clocksource directory, as if some other arch tried to include this clocksource (say if they had the same hardware) they might have collisions w/ their read_persistent_clock implementation.

I'm all for being able to re-use clocksource drivers. But this is the sort of thing that makes me worry we're maybe being too aggressive in pushing clocksources that really are fairly arch/platform specific into drivers/clocksource/

thanks
-john