[RFC PATCH 00/13] m68k: Drop arch_gettimeoffset and adopt clocksource API

[RFC PATCH 00/13] m68k: Drop arch_gettimeoffset and adopt clocksource API

[Finn Thain]

This series removes "select ARCH_USES_GETTIMEOFFSET" from arch/m68k
and converts users of arch_gettimeoffset to the clocksource API.
Various bugs are fixed along the way.

Those platforms which do not actually implement arch_gettimeoffset
(apollo, q40, sun3, sun3x) use the "jiffies" clocksource by default.

More testing and code review would be appreciated.


Finn Thain (13):
  arm: Fix mutual exclusion in arch_gettimeoffset
  m68k: Fix mutual exclusion in arch_gettimeoffset
  m68k: mac: Fix VIA timer counter accesses
  m68k: mac: Clean up unused timer definitions
  m68k: apollo, q40, sun3, sun3x: Remove arch_gettimeoffset
    implementations
  m68k: Drop ARCH_USES_GETTIMEOFFSET
  m68k: amiga: Convert to clocksource API
  m68k: atari: Convert to clocksource API
  m68k: bvme6000: Convert to clocksource API
  m68k: hp300: Convert to clocksource API
  m68k: mac: Convert to clocksource API
  m68k: mvme147: Convert to clocksource API
  m68k: mvme16x: Convert to clocksource API

 arch/arm/mach-ebsa110/core.c      |   5 ++
 arch/arm/mach-rpc/time.c          |   5 ++
 arch/m68k/Kconfig                 |   1 -
 arch/m68k/amiga/config.c          |  47 +++++++---
 arch/m68k/apollo/config.c         |   7 --
 arch/m68k/atari/config.c          |   2 -
 arch/m68k/atari/time.c            |  45 ++++++++--
 arch/m68k/bvme6000/config.c       |  66 +++++++++-----
 arch/m68k/hp300/config.c          |   1 -
 arch/m68k/hp300/time.c            |  41 +++++++--
 arch/m68k/hp300/time.h            |   1 -
 arch/m68k/include/asm/macints.h   |   3 -
 arch/m68k/include/asm/mvme147hw.h |   1 -
 arch/m68k/mac/config.c            |  10 +--
 arch/m68k/mac/via.c               | 145 ++++++++++++++++++++----------
 arch/m68k/mvme147/config.c        |  51 ++++++++---
 arch/m68k/mvme16x/config.c        |  48 ++++++----
 arch/m68k/q40/config.c            |   9 --
 arch/m68k/sun3/config.c           |   2 -
 arch/m68k/sun3/intersil.c         |   7 --
 arch/m68k/sun3x/config.c          |   1 -
 arch/m68k/sun3x/time.c            |   5 --
 arch/m68k/sun3x/time.h            |   1 -
 23 files changed, 326 insertions(+), 178 deletions(-)

-- 
2.18.1

[RFC PATCH 08/13] m68k: atari: Convert to clocksource API

[Finn Thain]

Add a platform clocksource by adapting the existing arch_gettimeoffset
implementation.

Cc: Michael Schmitz <schmitzmic@gmail.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/atari/time.c | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/arch/m68k/atari/time.c b/arch/m68k/atari/time.c
index 4765e9a58293..d066599fcc54 100644
--- a/arch/m68k/atari/time.c
+++ b/arch/m68k/atari/time.c
@@ -16,6 +16,7 @@
 #include <linux/init.h>
 #include <linux/rtc.h>
 #include <linux/bcd.h>
+#include <linux/clocksource.h>
 #include <linux/delay.h>
 #include <linux/export.h>
 
@@ -24,6 +25,30 @@
 DEFINE_SPINLOCK(rtc_lock);
 EXPORT_SYMBOL_GPL(rtc_lock);
 
+static u64 atari_read_clk(struct clocksource *cs);
+
+static struct clocksource atari_clk = {
+	.name   = "mfp",
+	.rating = 250,
+	.read   = atari_read_clk,
+	.mask   = CLOCKSOURCE_MASK(32),
+	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+static u32 clk_total;
+
+static irqreturn_t mfp_timer_handler(int irq, void *data)
+{
+	irq_handler_t func = data;
+	unsigned long flags;
+
+	local_irq_save(flags);
+	clk_total += INT_TICKS;
+	local_irq_restore(flags);
+
+	return func(irq, data);
+}
+
 void __init
 atari_sched_init(irq_handler_t timer_routine)
 {
@@ -32,15 +57,16 @@ atari_sched_init(irq_handler_t timer_routine)
     /* start timer C, div = 1:100 */
     st_mfp.tim_ct_cd = (st_mfp.tim_ct_cd & 15) | 0x60;
     /* install interrupt service routine for MFP Timer C */
-    if (request_irq(IRQ_MFP_TIMC, timer_routine, 0, "timer", timer_routine))
+    if (request_irq(IRQ_MFP_TIMC, mfp_timer_handler, IRQF_TIMER, "timer",
+                    timer_routine))
 	pr_err("Couldn't register timer interrupt\n");
+
+    clocksource_register_hz(&atari_clk, INT_CLK);
 }
 
 /* ++andreas: gettimeoffset fixed to check for pending interrupt */
 
-#define TICK_SIZE 10000
-
-u32 atari_gettimeoffset(void)
+static u64 atari_read_clk(struct clocksource *cs)
 {
   unsigned long flags;
   u32 ticks, offset = 0;
@@ -53,14 +79,13 @@ u32 atari_gettimeoffset(void)
   if (ticks > INT_TICKS - INT_TICKS / 50)
     /* Check for pending timer interrupt */
     if (st_mfp.int_pn_b & (1 << 5))
-      offset = TICK_SIZE;
+      offset = INT_TICKS;
 
   local_irq_restore(flags);
 
   ticks = INT_TICKS - ticks;
-  ticks = ticks * 10000L / INT_TICKS;
 
-  return (ticks + offset) * 1000;
+  return clk_total + ticks + offset;
 }
 
 
-- 
2.18.1

[RFC PATCH 04/13] m68k: mac: Clean up unused timer definitions

[Finn Thain]

Cc: Joshua Thompson <funaho@jurai.org>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/include/asm/macints.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/m68k/include/asm/macints.h b/arch/m68k/include/asm/macints.h
index cddb2d3ea49b..4da172bd048c 100644
--- a/arch/m68k/include/asm/macints.h
+++ b/arch/m68k/include/asm/macints.h
@@ -121,7 +121,4 @@
 #define SLOT2IRQ(x)	  (x + 47)
 #define IRQ2SLOT(x)	  (x - 47)
 
-#define INT_CLK   24576	    /* CLK while int_clk =2.456MHz and divide = 100 */
-#define INT_TICKS 246	    /* to make sched_time = 99.902... HZ */
-
 #endif /* asm/macints.h */
-- 
2.18.1

[RFC PATCH 10/13] m68k: hp300: Convert to clocksource API

[Finn Thain]

Add a platform clocksource by adapting the existing arch_gettimeoffset
implementation.

Cc: Philip Blundell <philb@gnu.org>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/hp300/time.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/arch/m68k/hp300/time.c b/arch/m68k/hp300/time.c
index 5cf711fd0858..ff2fc65bc5e7 100644
--- a/arch/m68k/hp300/time.c
+++ b/arch/m68k/hp300/time.c
@@ -8,6 +8,7 @@
  */
 
 #include <asm/ptrace.h>
+#include <linux/clocksource.h>
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/sched.h>
@@ -19,6 +20,18 @@
 #include <asm/traps.h>
 #include <asm/blinken.h>
 
+static u64 hp300_read_clk(struct clocksource *cs);
+
+static struct clocksource hp300_clk = {
+	.name   = "timer",
+	.rating = 250,
+	.read   = hp300_read_clk,
+	.mask   = CLOCKSOURCE_MASK(32),
+	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+static u32 clk_total;
+
 /* Clock hardware definitions */
 
 #define CLOCKBASE	0xf05f8000
@@ -32,22 +45,29 @@
 #define	CLKMSB3		0xD
 
 /* This is for machines which generate the exact clock. */
-#define USECS_PER_JIFFY (1000000/HZ)
 
-#define INTVAL ((10000 / 4) - 1)
+#define HP300_TIMER_CLOCK_FREQ 250000
+#define HP300_TIMER_CYCLES     (HP300_TIMER_CLOCK_FREQ / HZ)
+#define INTVAL                 (HP300_TIMER_CYCLES - 1)
 
 static irqreturn_t hp300_tick(int irq, void *dev_id)
 {
+	unsigned long flags;
 	unsigned long tmp;
 	irq_handler_t vector = dev_id;
+
+	local_irq_save(flags);
 	in_8(CLOCKBASE + CLKSR);
 	asm volatile ("movpw %1@(5),%0" : "=d" (tmp) : "a" (CLOCKBASE));
+	clk_total += INTVAL;
+	local_irq_restore(flags);
+
 	/* Turn off the network and SCSI leds */
 	blinken_leds(0, 0xe0);
 	return vector(irq, NULL);
 }
 
-u32 hp300_gettimeoffset(void)
+static u64 hp300_read_clk(struct clocksource *cs)
 {
   unsigned long flags;
   unsigned char lsb, msb1, msb2;
@@ -66,7 +86,8 @@ u32 hp300_gettimeoffset(void)
   local_irq_restore(flags);
 
   ticks = INTVAL - ((msb2 << 8) | lsb);
-  return ((USECS_PER_JIFFY * ticks) / INTVAL) * 1000;
+
+  return clk_total + ticks;
 }
 
 void __init hp300_sched_init(irq_handler_t vector)
@@ -76,9 +97,11 @@ void __init hp300_sched_init(irq_handler_t vector)
 
   asm volatile(" movpw %0,%1@(5)" : : "d" (INTVAL), "a" (CLOCKBASE));
 
-  if (request_irq(IRQ_AUTO_6, hp300_tick, 0, "timer tick", vector))
+  if (request_irq(IRQ_AUTO_6, hp300_tick, IRQF_TIMER, "timer tick", vector))
     pr_err("Couldn't register timer interrupt\n");
 
   out_8(CLOCKBASE + CLKCR2, 0x1);		/* select CR1 */
   out_8(CLOCKBASE + CLKCR1, 0x40);		/* enable irq */
+
+  clocksource_register_hz(&hp300_clk, HP300_TIMER_CLOCK_FREQ);
 }
-- 
2.18.1

[RFC PATCH 11/13] m68k: mac: Convert to clocksource API

[Finn Thain]

Add a platform clocksource by adapting the existing arch_gettimeoffset
implementation.

Cc: Joshua Thompson <funaho@jurai.org>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/mac/via.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
index 7130abec0b77..c4c304470fae 100644
--- a/arch/m68k/mac/via.c
+++ b/arch/m68k/mac/via.c
@@ -23,6 +23,7 @@
  *
  */
 
+#include <linux/clocksource.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
@@ -556,13 +557,23 @@ EXPORT_SYMBOL(via2_scsi_drq_pending);
 /* timer and clock source */
 
 #define VIA_CLOCK_FREQ     783360                /* VIA "phase 2" clock in Hz */
-#define VIA_TIMER_INTERVAL (1000000 / HZ)        /* microseconds per jiffy */
 #define VIA_TIMER_CYCLES   (VIA_CLOCK_FREQ / HZ) /* clock cycles per jiffy */
 
 #define VIA_TC             (VIA_TIMER_CYCLES - 2) /* including 0 and -1 */
 #define VIA_TC_LOW         (VIA_TC & 0xFF)
 #define VIA_TC_HIGH        (VIA_TC >> 8)
 
+static u64 mac_read_clk(struct clocksource *cs);
+
+static struct clocksource mac_clk = {
+	.name   = "via1",
+	.rating = 250,
+	.read   = mac_read_clk,
+	.mask   = CLOCKSOURCE_MASK(32),
+	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+static u32 clk_total;
 static bool lost_timer_intr;
 
 static irqreturn_t via_timer_handler(int irq, void *data)
@@ -573,16 +584,18 @@ static irqreturn_t via_timer_handler(int irq, void *data)
 	local_irq_save(flags);
 	if (lost_timer_intr) {
 		lost_timer_intr = false;
-		local_irq_restore(flags);
-		func(irq, data);
-	} else
-		local_irq_restore(flags);
+		clk_total += VIA_TIMER_CYCLES;
+	}
+	clk_total += VIA_TIMER_CYCLES;
+	local_irq_restore(flags);
+
 	return func(irq, data);
 }
 
 void __init via_init_clock(irq_handler_t func)
 {
-	if (request_irq(IRQ_MAC_TIMER_1, via_timer_handler, 0, "timer", func)) {
+	if (request_irq(IRQ_MAC_TIMER_1, via_timer_handler, IRQF_TIMER,
+			"timer", func)) {
 		pr_err("Couldn't register %s interrupt\n", "timer");
 		return;
 	}
@@ -592,11 +605,13 @@ void __init via_init_clock(irq_handler_t func)
 	via1[vT1CL] = VIA_TC_LOW;
 	via1[vT1CH] = VIA_TC_HIGH;
 	via1[vACR] |= 0x40;
+
+	clocksource_register_hz(&mac_clk, VIA_CLOCK_FREQ);
 }
 
 #define VIA_T1_INT_FLAG BIT(6)
 
-u32 mac_gettimeoffset(void)
+static u64 mac_read_clk(struct clocksource *cs)
 {
 	unsigned long flags;
 	u8 count_high, count_high_new, count_low;
@@ -631,5 +646,5 @@ u32 mac_gettimeoffset(void)
 	count = ((count_high << 8) | count_low) + 1;
 	count = VIA_TIMER_CYCLES - count + offset;
 
-	return ((count * VIA_TIMER_INTERVAL) / VIA_TIMER_CYCLES) * 1000;
+	return clk_total + count;
 }
-- 
2.18.1

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

Implementations of arch_gettimeoffset are generally not re-entrant
and assume that interrupts have been disabled. Unfortunately this
pre-condition got broken in v2.6.32.

Fixes: 5cfc8ee0bb51 ("ARM: convert arm to arch_gettimeoffset()")
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/arm/mach-ebsa110/core.c | 5 +++++
 arch/arm/mach-rpc/time.c     | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/arch/arm/mach-ebsa110/core.c b/arch/arm/mach-ebsa110/core.c
index 688e5fed49a7..479f89a1accf 100644
--- a/arch/arm/mach-ebsa110/core.c
+++ b/arch/arm/mach-ebsa110/core.c
@@ -160,12 +160,17 @@ static void __init ebsa110_init_early(void)
  */
 static u32 ebsa110_gettimeoffset(void)
 {
+	unsigned long flags;
 	unsigned long offset, count;
 
+	local_irq_save(flags);
+
 	__raw_writeb(0x40, PIT_CTRL);
 	count = __raw_readb(PIT_T1);
 	count |= __raw_readb(PIT_T1) << 8;
 
+	local_irq_restore(flags);
+
 	/*
 	 * If count > COUNT, make the number negative.
 	 */
diff --git a/arch/arm/mach-rpc/time.c b/arch/arm/mach-rpc/time.c
index 2689771c1d38..852bb3801638 100644
--- a/arch/arm/mach-rpc/time.c
+++ b/arch/arm/mach-rpc/time.c
@@ -29,9 +29,12 @@
 
 static u32 ioc_timer_gettimeoffset(void)
 {
+	unsigned long flags;
 	unsigned int count1, count2, status;
 	long offset;
 
+	local_irq_save(flags);
+
 	ioc_writeb (0, IOC_T0LATCH);
 	barrier ();
 	count1 = ioc_readb(IOC_T0CNTL) | (ioc_readb(IOC_T0CNTH) << 8);
@@ -42,6 +45,8 @@ static u32 ioc_timer_gettimeoffset(void)
 	barrier ();
 	count2 = ioc_readb(IOC_T0CNTL) | (ioc_readb(IOC_T0CNTH) << 8);
 
+	local_irq_restore(flags);
+
 	offset = count2;
 	if (count2 < count1) {
 		/*
-- 
2.18.1

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

Implementations of arch_gettimeoffset are generally not re-entrant
and assume that interrupts have been disabled. Unfortunately this
pre-condition got broken in v2.6.32.

Fixes: 5cfc8ee0bb51 ("ARM: convert arm to arch_gettimeoffset()")
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/arm/mach-ebsa110/core.c | 5 +++++
 arch/arm/mach-rpc/time.c     | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/arch/arm/mach-ebsa110/core.c b/arch/arm/mach-ebsa110/core.c
index 688e5fed49a7..479f89a1accf 100644
--- a/arch/arm/mach-ebsa110/core.c
+++ b/arch/arm/mach-ebsa110/core.c
@@ -160,12 +160,17 @@ static void __init ebsa110_init_early(void)
  */
 static u32 ebsa110_gettimeoffset(void)
 {
+	unsigned long flags;
 	unsigned long offset, count;
 
+	local_irq_save(flags);
+
 	__raw_writeb(0x40, PIT_CTRL);
 	count = __raw_readb(PIT_T1);
 	count |= __raw_readb(PIT_T1) << 8;
 
+	local_irq_restore(flags);
+
 	/*
 	 * If count > COUNT, make the number negative.
 	 */
diff --git a/arch/arm/mach-rpc/time.c b/arch/arm/mach-rpc/time.c
index 2689771c1d38..852bb3801638 100644
--- a/arch/arm/mach-rpc/time.c
+++ b/arch/arm/mach-rpc/time.c
@@ -29,9 +29,12 @@
 
 static u32 ioc_timer_gettimeoffset(void)
 {
+	unsigned long flags;
 	unsigned int count1, count2, status;
 	long offset;
 
+	local_irq_save(flags);
+
 	ioc_writeb (0, IOC_T0LATCH);
 	barrier ();
 	count1 = ioc_readb(IOC_T0CNTL) | (ioc_readb(IOC_T0CNTH) << 8);
@@ -42,6 +45,8 @@ static u32 ioc_timer_gettimeoffset(void)
 	barrier ();
 	count2 = ioc_readb(IOC_T0CNTL) | (ioc_readb(IOC_T0CNTH) << 8);
 
+	local_irq_restore(flags);
+
 	offset = count2;
 	if (count2 < count1) {
 		/*
-- 
2.18.1

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Christoph Hellwig]

On Mon, Nov 12, 2018 at 03:12:39PM +1100, Finn Thain wrote:
> Implementations of arch_gettimeoffset are generally not re-entrant
> and assume that interrupts have been disabled. Unfortunately this
> pre-condition got broken in v2.6.32.
> 
> Fixes: 5cfc8ee0bb51 ("ARM: convert arm to arch_gettimeoffset()")
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

This looks like a sensible fix for -stable backporting.  But with m68k
converted over it might be time for these two arm platforms to either
be converted to the clocksource API or to be dropped..

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Christoph Hellwig]

On Mon, Nov 12, 2018 at 03:12:39PM +1100, Finn Thain wrote:
> Implementations of arch_gettimeoffset are generally not re-entrant
> and assume that interrupts have been disabled. Unfortunately this
> pre-condition got broken in v2.6.32.
> 
> Fixes: 5cfc8ee0bb51 ("ARM: convert arm to arch_gettimeoffset()")
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

This looks like a sensible fix for -stable backporting.  But with m68k
converted over it might be time for these two arm platforms to either
be converted to the clocksource API or to be dropped..

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

On Mon, 12 Nov 2018, Christoph Hellwig wrote:

> On Mon, Nov 12, 2018 at 03:12:39PM +1100, Finn Thain wrote:
> > Implementations of arch_gettimeoffset are generally not re-entrant and 
> > assume that interrupts have been disabled. Unfortunately this 
> > pre-condition got broken in v2.6.32.
> > 
> > Fixes: 5cfc8ee0bb51 ("ARM: convert arm to arch_gettimeoffset()")
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> 
> This looks like a sensible fix for -stable backporting.  But with m68k
> converted over it might be time for these two arm platforms to either
> be converted to the clocksource API or to be dropped..
> 

You could remove the old arch_gettimeoffset API without dropping any 
platforms.

If no-one converts a given platform to the clocksource API it would mean 
that the default 'jiffies' clocksource will get used on that platform.

Clock resolution and timer precision would be degraded, but that might not 
matter.

Anyway, if someone who has this hardware is willing to test a clocksource 
API conversion, they can let me know and I'll attempt that patch.

-- 

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

On Mon, 12 Nov 2018, Christoph Hellwig wrote:

> On Mon, Nov 12, 2018 at 03:12:39PM +1100, Finn Thain wrote:
> > Implementations of arch_gettimeoffset are generally not re-entrant and 
> > assume that interrupts have been disabled. Unfortunately this 
> > pre-condition got broken in v2.6.32.
> > 
> > Fixes: 5cfc8ee0bb51 ("ARM: convert arm to arch_gettimeoffset()")
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> 
> This looks like a sensible fix for -stable backporting.  But with m68k
> converted over it might be time for these two arm platforms to either
> be converted to the clocksource API or to be dropped..
> 

You could remove the old arch_gettimeoffset API without dropping any 
platforms.

If no-one converts a given platform to the clocksource API it would mean 
that the default 'jiffies' clocksource will get used on that platform.

Clock resolution and timer precision would be degraded, but that might not 
matter.

Anyway, if someone who has this hardware is willing to test a clocksource 
API conversion, they can let me know and I'll attempt that patch.

-- 

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Tue, Nov 13, 2018 at 02:39:00PM +1100, Finn Thain wrote:
> On Mon, 12 Nov 2018, Christoph Hellwig wrote:
> 
> > On Mon, Nov 12, 2018 at 03:12:39PM +1100, Finn Thain wrote:
> > > Implementations of arch_gettimeoffset are generally not re-entrant and 
> > > assume that interrupts have been disabled. Unfortunately this 
> > > pre-condition got broken in v2.6.32.
> > > 
> > > Fixes: 5cfc8ee0bb51 ("ARM: convert arm to arch_gettimeoffset()")
> > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> > 
> > This looks like a sensible fix for -stable backporting.  But with m68k
> > converted over it might be time for these two arm platforms to either
> > be converted to the clocksource API or to be dropped..
> > 
> 
> You could remove the old arch_gettimeoffset API without dropping any 
> platforms.
> 
> If no-one converts a given platform to the clocksource API it would mean 
> that the default 'jiffies' clocksource will get used on that platform.
> 
> Clock resolution and timer precision would be degraded, but that might not 
> matter.
> 
> Anyway, if someone who has this hardware is willing to test a clocksource 
> API conversion, they can let me know and I'll attempt that patch.

There's reasons why that's not appropriate - such as not having two
separate timers in order to supply a clocksource and separate clock
event.

Not all hardware is suited to the clocksource + clockevent idea.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Tue, Nov 13, 2018 at 02:39:00PM +1100, Finn Thain wrote:
> On Mon, 12 Nov 2018, Christoph Hellwig wrote:
> 
> > On Mon, Nov 12, 2018 at 03:12:39PM +1100, Finn Thain wrote:
> > > Implementations of arch_gettimeoffset are generally not re-entrant and 
> > > assume that interrupts have been disabled. Unfortunately this 
> > > pre-condition got broken in v2.6.32.
> > > 
> > > Fixes: 5cfc8ee0bb51 ("ARM: convert arm to arch_gettimeoffset()")
> > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> > 
> > This looks like a sensible fix for -stable backporting.  But with m68k
> > converted over it might be time for these two arm platforms to either
> > be converted to the clocksource API or to be dropped..
> > 
> 
> You could remove the old arch_gettimeoffset API without dropping any 
> platforms.
> 
> If no-one converts a given platform to the clocksource API it would mean 
> that the default 'jiffies' clocksource will get used on that platform.
> 
> Clock resolution and timer precision would be degraded, but that might not 
> matter.
> 
> Anyway, if someone who has this hardware is willing to test a clocksource 
> API conversion, they can let me know and I'll attempt that patch.

There's reasons why that's not appropriate - such as not having two
separate timers in order to supply a clocksource and separate clock
event.

Not all hardware is suited to the clocksource + clockevent idea.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

On Tue, 13 Nov 2018, Russell King - ARM Linux wrote:

> On Tue, Nov 13, 2018 at 02:39:00PM +1100, Finn Thain wrote:
> > 
> > You could remove the old arch_gettimeoffset API without dropping any 
> > platforms.
> > 
> > If no-one converts a given platform to the clocksource API it would mean 
> > that the default 'jiffies' clocksource will get used on that platform.
> > 
> > Clock resolution and timer precision would be degraded, but that might not 
> > matter.
> > 
> > Anyway, if someone who has this hardware is willing to test a clocksource 
> > API conversion, they can let me know and I'll attempt that patch.
> 
> There's reasons why that's not appropriate - such as not having two
> separate timers in order to supply a clocksource and separate clock
> event.
> 
> Not all hardware is suited to the clocksource + clockevent idea.
> 

Sorry, I don't follow.

AFAIK, clocksources and clock event devices are orthogonal concepts. There 
are platforms with !ARCH_USES_GETTIMEOFFSET && !GENERIC_CLOCKEVENTS (and 
every other combination).

A clocksource read method just provides a cycle count, and in this sense 
arch_gettimeoffset() is equivalent to a clocksource.

If these two arm platforms have an existing clock event device which 
somehow precludes any new clocksources, why doesn't that also render 
arch_gettimeoffset() impossible?

-- 

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

On Tue, 13 Nov 2018, Russell King - ARM Linux wrote:

> On Tue, Nov 13, 2018 at 02:39:00PM +1100, Finn Thain wrote:
> > 
> > You could remove the old arch_gettimeoffset API without dropping any 
> > platforms.
> > 
> > If no-one converts a given platform to the clocksource API it would mean 
> > that the default 'jiffies' clocksource will get used on that platform.
> > 
> > Clock resolution and timer precision would be degraded, but that might not 
> > matter.
> > 
> > Anyway, if someone who has this hardware is willing to test a clocksource 
> > API conversion, they can let me know and I'll attempt that patch.
> 
> There's reasons why that's not appropriate - such as not having two
> separate timers in order to supply a clocksource and separate clock
> event.
> 
> Not all hardware is suited to the clocksource + clockevent idea.
> 

Sorry, I don't follow.

AFAIK, clocksources and clock event devices are orthogonal concepts. There 
are platforms with !ARCH_USES_GETTIMEOFFSET && !GENERIC_CLOCKEVENTS (and 
every other combination).

A clocksource read method just provides a cycle count, and in this sense 
arch_gettimeoffset() is equivalent to a clocksource.

If these two arm platforms have an existing clock event device which 
somehow precludes any new clocksources, why doesn't that also render 
arch_gettimeoffset() impossible?

-- 

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Wed, Nov 14, 2018 at 08:55:37AM +1100, Finn Thain wrote:
> On Tue, 13 Nov 2018, Russell King - ARM Linux wrote:
> 
> > On Tue, Nov 13, 2018 at 02:39:00PM +1100, Finn Thain wrote:
> > > 
> > > You could remove the old arch_gettimeoffset API without dropping any 
> > > platforms.
> > > 
> > > If no-one converts a given platform to the clocksource API it would mean 
> > > that the default 'jiffies' clocksource will get used on that platform.
> > > 
> > > Clock resolution and timer precision would be degraded, but that might not 
> > > matter.
> > > 
> > > Anyway, if someone who has this hardware is willing to test a clocksource 
> > > API conversion, they can let me know and I'll attempt that patch.
> > 
> > There's reasons why that's not appropriate - such as not having two
> > separate timers in order to supply a clocksource and separate clock
> > event.
> > 
> > Not all hardware is suited to the clocksource + clockevent idea.
> > 
> 
> Sorry, I don't follow.
> 
> AFAIK, clocksources and clock event devices are orthogonal concepts. There 
> are platforms with !ARCH_USES_GETTIMEOFFSET && !GENERIC_CLOCKEVENTS (and 
> every other combination).
> 
> A clocksource read method just provides a cycle count, and in this sense 
> arch_gettimeoffset() is equivalent to a clocksource.

A clocksource provides a cycle counter that monotonically changes and
does not wrap between clockevent events.

A clock event is responsible for providing events to the system when
some work is needing to be done, limited by the wrap interval of the
clocksource.

Each time the clock event triggers an interrupt, the clocksource is
read to determine how much time has passed, using:

	count = (new_value - old_value) & available_bits
	nanosecs = count * scale >> shift;

If you try to combine the clocksource and clockevent because you only
have a single counter, and the counter has the behaviour of:
- counting down towards zero
- when reaching zero, triggers an interrupt, and reloads with N

then this provides your clockevent, but you can't use this as a clock
source, because each time you receive an interrupt and try to read the
counter value, it will be approximately the same value.  This means
that the above calculation fails to register the correct number of
nanoseconds passing.  Hence, this does not work.

Also note where I said above that the clock event device must be able
to provide an interrupt _before_ the clocksource wraps - clearly with
such a timer, that is utterly impossible.

The simple solution is to not use such a counter as the clocksource,
which means you fall back to using the jiffies clocksource, and your
timekeeping has jiffy resolution - so 10ms, or possibly 1ms if you
want a 1kHz timer interval.  For most applications, that's simply way
to coarse, as was realised in the very early days of Linux.

If only there was a way to interpolate between timer interrupts...
which is exactly what arch_gettimeoffset() does, and is a historical
reminant of the pre-clocksource/clockevent days of the kernel - but
it is the only way to get better resolution from this sort of setup.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Wed, Nov 14, 2018 at 08:55:37AM +1100, Finn Thain wrote:
> On Tue, 13 Nov 2018, Russell King - ARM Linux wrote:
> 
> > On Tue, Nov 13, 2018 at 02:39:00PM +1100, Finn Thain wrote:
> > > 
> > > You could remove the old arch_gettimeoffset API without dropping any 
> > > platforms.
> > > 
> > > If no-one converts a given platform to the clocksource API it would mean 
> > > that the default 'jiffies' clocksource will get used on that platform.
> > > 
> > > Clock resolution and timer precision would be degraded, but that might not 
> > > matter.
> > > 
> > > Anyway, if someone who has this hardware is willing to test a clocksource 
> > > API conversion, they can let me know and I'll attempt that patch.
> > 
> > There's reasons why that's not appropriate - such as not having two
> > separate timers in order to supply a clocksource and separate clock
> > event.
> > 
> > Not all hardware is suited to the clocksource + clockevent idea.
> > 
> 
> Sorry, I don't follow.
> 
> AFAIK, clocksources and clock event devices are orthogonal concepts. There 
> are platforms with !ARCH_USES_GETTIMEOFFSET && !GENERIC_CLOCKEVENTS (and 
> every other combination).
> 
> A clocksource read method just provides a cycle count, and in this sense 
> arch_gettimeoffset() is equivalent to a clocksource.

A clocksource provides a cycle counter that monotonically changes and
does not wrap between clockevent events.

A clock event is responsible for providing events to the system when
some work is needing to be done, limited by the wrap interval of the
clocksource.

Each time the clock event triggers an interrupt, the clocksource is
read to determine how much time has passed, using:

	count = (new_value - old_value) & available_bits
	nanosecs = count * scale >> shift;

If you try to combine the clocksource and clockevent because you only
have a single counter, and the counter has the behaviour of:
- counting down towards zero
- when reaching zero, triggers an interrupt, and reloads with N

then this provides your clockevent, but you can't use this as a clock
source, because each time you receive an interrupt and try to read the
counter value, it will be approximately the same value.  This means
that the above calculation fails to register the correct number of
nanoseconds passing.  Hence, this does not work.

Also note where I said above that the clock event device must be able
to provide an interrupt _before_ the clocksource wraps - clearly with
such a timer, that is utterly impossible.

The simple solution is to not use such a counter as the clocksource,
which means you fall back to using the jiffies clocksource, and your
timekeeping has jiffy resolution - so 10ms, or possibly 1ms if you
want a 1kHz timer interval.  For most applications, that's simply way
to coarse, as was realised in the very early days of Linux.

If only there was a way to interpolate between timer interrupts...
which is exactly what arch_gettimeoffset() does, and is a historical
reminant of the pre-clocksource/clockevent days of the kernel - but
it is the only way to get better resolution from this sort of setup.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Michael Schmitz]

On 14/11/18 12:43 PM, Russell King - ARM Linux wrote:
> On Wed, Nov 14, 2018 at 08:55:37AM +1100, Finn Thain wrote:
>> On Tue, 13 Nov 2018, Russell King - ARM Linux wrote:
>>
>>> On Tue, Nov 13, 2018 at 02:39:00PM +1100, Finn Thain wrote:
>>>> You could remove the old arch_gettimeoffset API without dropping any
>>>> platforms.
>>>>
>>>> If no-one converts a given platform to the clocksource API it would mean
>>>> that the default 'jiffies' clocksource will get used on that platform.
>>>>
>>>> Clock resolution and timer precision would be degraded, but that might not
>>>> matter.
>>>>
>>>> Anyway, if someone who has this hardware is willing to test a clocksource
>>>> API conversion, they can let me know and I'll attempt that patch.
>>> There's reasons why that's not appropriate - such as not having two
>>> separate timers in order to supply a clocksource and separate clock
>>> event.
>>>
>>> Not all hardware is suited to the clocksource + clockevent idea.
>>>
>> Sorry, I don't follow.
>>
>> AFAIK, clocksources and clock event devices are orthogonal concepts. There
>> are platforms with !ARCH_USES_GETTIMEOFFSET && !GENERIC_CLOCKEVENTS (and
>> every other combination).
>>
>> A clocksource read method just provides a cycle count, and in this sense
>> arch_gettimeoffset() is equivalent to a clocksource.
> A clocksource provides a cycle counter that monotonically changes and
> does not wrap between clockevent events.
>
> A clock event is responsible for providing events to the system when
> some work is needing to be done, limited by the wrap interval of the
> clocksource.
>
> Each time the clock event triggers an interrupt, the clocksource is
> read to determine how much time has passed, using:
>
> 	count = (new_value - old_value) & available_bits
> 	nanosecs = count * scale >> shift;
>
> If you try to combine the clocksource and clockevent because you only
> have a single counter, and the counter has the behaviour of:
> - counting down towards zero
> - when reaching zero, triggers an interrupt, and reloads with N
>
> then this provides your clockevent, but you can't use this as a clock
> source, because each time you receive an interrupt and try to read the
> counter value, it will be approximately the same value.  This means
> that the above calculation fails to register the correct number of
> nanoseconds passing.  Hence, this does not work.

So we'd still have to use jiffies + interpolation from the current timer 
rundown counter as clocksource (since that will be monotonous and won't 
wrap)?

The way Finn did the clocksource for m68k, the clocksource counter does 
behave as it should (monotonous, doesn't wrap) at least so far as I've 
tested. Using the same timer for clocksource and clock events will 
degrade timekeeping based on timer events to jiffies resolution, as you 
point out. That would already have been the case before, so no gain in 
resolution. Other timekeeping would have worked at higher resolution 
before (interpolating through arch_gettimeoffset) just the same as now 
(interpolating through clocksource reads). Finn's clocksource read code 
essentially is arch_gettimeoffset under a new name.

Are you saying that's not possible on arm, because the current timer 
rundown counter can't be read while the timer is running?

If I were to run a second timer at higher rate for clocksource, but 
keeping the 10 ms timer as clock event (could easily do that by using 
timer D on Atari Falcon) - how would that improve my timekeeping? Clock 
events still only happen 10 ms apart ...

Cheers,

     Michael

>
> Also note where I said above that the clock event device must be able
> to provide an interrupt _before_ the clocksource wraps - clearly with
> such a timer, that is utterly impossible.
>
> The simple solution is to not use such a counter as the clocksource,
> which means you fall back to using the jiffies clocksource, and your
> timekeeping has jiffy resolution - so 10ms, or possibly 1ms if you
> want a 1kHz timer interval.  For most applications, that's simply way
> to coarse, as was realised in the very early days of Linux.
>
> If only there was a way to interpolate between timer interrupts...
> which is exactly what arch_gettimeoffset() does, and is a historical
> reminant of the pre-clocksource/clockevent days of the kernel - but
> it is the only way to get better resolution from this sort of setup.
>

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Michael Schmitz]

On 14/11/18 12:43 PM, Russell King - ARM Linux wrote:
> On Wed, Nov 14, 2018 at 08:55:37AM +1100, Finn Thain wrote:
>> On Tue, 13 Nov 2018, Russell King - ARM Linux wrote:
>>
>>> On Tue, Nov 13, 2018 at 02:39:00PM +1100, Finn Thain wrote:
>>>> You could remove the old arch_gettimeoffset API without dropping any
>>>> platforms.
>>>>
>>>> If no-one converts a given platform to the clocksource API it would mean
>>>> that the default 'jiffies' clocksource will get used on that platform.
>>>>
>>>> Clock resolution and timer precision would be degraded, but that might not
>>>> matter.
>>>>
>>>> Anyway, if someone who has this hardware is willing to test a clocksource
>>>> API conversion, they can let me know and I'll attempt that patch.
>>> There's reasons why that's not appropriate - such as not having two
>>> separate timers in order to supply a clocksource and separate clock
>>> event.
>>>
>>> Not all hardware is suited to the clocksource + clockevent idea.
>>>
>> Sorry, I don't follow.
>>
>> AFAIK, clocksources and clock event devices are orthogonal concepts. There
>> are platforms with !ARCH_USES_GETTIMEOFFSET && !GENERIC_CLOCKEVENTS (and
>> every other combination).
>>
>> A clocksource read method just provides a cycle count, and in this sense
>> arch_gettimeoffset() is equivalent to a clocksource.
> A clocksource provides a cycle counter that monotonically changes and
> does not wrap between clockevent events.
>
> A clock event is responsible for providing events to the system when
> some work is needing to be done, limited by the wrap interval of the
> clocksource.
>
> Each time the clock event triggers an interrupt, the clocksource is
> read to determine how much time has passed, using:
>
> 	count = (new_value - old_value) & available_bits
> 	nanosecs = count * scale >> shift;
>
> If you try to combine the clocksource and clockevent because you only
> have a single counter, and the counter has the behaviour of:
> - counting down towards zero
> - when reaching zero, triggers an interrupt, and reloads with N
>
> then this provides your clockevent, but you can't use this as a clock
> source, because each time you receive an interrupt and try to read the
> counter value, it will be approximately the same value.  This means
> that the above calculation fails to register the correct number of
> nanoseconds passing.  Hence, this does not work.

So we'd still have to use jiffies + interpolation from the current timer 
rundown counter as clocksource (since that will be monotonous and won't 
wrap)?

The way Finn did the clocksource for m68k, the clocksource counter does 
behave as it should (monotonous, doesn't wrap) at least so far as I've 
tested. Using the same timer for clocksource and clock events will 
degrade timekeeping based on timer events to jiffies resolution, as you 
point out. That would already have been the case before, so no gain in 
resolution. Other timekeeping would have worked at higher resolution 
before (interpolating through arch_gettimeoffset) just the same as now 
(interpolating through clocksource reads). Finn's clocksource read code 
essentially is arch_gettimeoffset under a new name.

Are you saying that's not possible on arm, because the current timer 
rundown counter can't be read while the timer is running?

If I were to run a second timer at higher rate for clocksource, but 
keeping the 10 ms timer as clock event (could easily do that by using 
timer D on Atari Falcon) - how would that improve my timekeeping? Clock 
events still only happen 10 ms apart ...

Cheers,

 ??? Michael

>
> Also note where I said above that the clock event device must be able
> to provide an interrupt _before_ the clocksource wraps - clearly with
> such a timer, that is utterly impossible.
>
> The simple solution is to not use such a counter as the clocksource,
> which means you fall back to using the jiffies clocksource, and your
> timekeeping has jiffy resolution - so 10ms, or possibly 1ms if you
> want a 1kHz timer interval.  For most applications, that's simply way
> to coarse, as was realised in the very early days of Linux.
>
> If only there was a way to interpolate between timer interrupts...
> which is exactly what arch_gettimeoffset() does, and is a historical
> reminant of the pre-clocksource/clockevent days of the kernel - but
> it is the only way to get better resolution from this sort of setup.
>

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Wed, Nov 14, 2018 at 02:35:29PM +1300, Michael Schmitz wrote:
> So we'd still have to use jiffies + interpolation from the current timer
> rundown counter as clocksource (since that will be monotonous and won't
> wrap)?
> 
> The way Finn did the clocksource for m68k, the clocksource counter does
> behave as it should (monotonous, doesn't wrap) at least so far as I've
> tested. Using the same timer for clocksource and clock events will degrade
> timekeeping based on timer events to jiffies resolution, as you point out.
> That would already have been the case before, so no gain in resolution.

... and that is where you are wrong.

RPC, for example, has gettimeofday() resolution of 500ns.  Removing
gettimeoffset() will result in a resolution of 1/HZ since there is
no longer any interpolation between interrupts.

> Other timekeeping would have worked at higher resolution before
> (interpolating through arch_gettimeoffset) just the same as now
> (interpolating through clocksource reads). Finn's clocksource read code
> essentially is arch_gettimeoffset under a new name.

Where is this code - all I've seen is code adding IRQ exclusion in
the gettimeoffset method.  If some other solution is being proposed,
then it's no good beating about the bush - show the damn code, and
then that can be considered.

However, what has been said in this thread is basically "remove the
gettimeoffset method", which is _not_ acceptable, it will cause
gettimeofday() on these platforms to lose resolution, which is a
user visible REGRESSION, plain and simple.

If what is actually meant is "we have a replacement for gettimeoffset"
then, and I'm sure we all know this, there needs to be a patch
proposed showing what is being proposed, rather than waffling around
the issue.

> Are you saying that's not possible on arm, because the current timer rundown
> counter can't be read while the timer is running?
> 
> If I were to run a second timer at higher rate for clocksource, but keeping
> the 10 ms timer as clock event (could easily do that by using timer D on
> Atari Falcon) - how would that improve my timekeeping? Clock events still
> only happen 10 ms apart ...

Ah, I think you're talking about something else.

You seem to be talking about what happens when time keeping interrupts
happen.

I'm talking about the resolution of gettimeofday() and the other
interfaces that are used (eg) for packet capture timestamping and
the like - the _user_ visible effects of the timekeeping system.

With the existing implementation, all these have better-than-jiffy
resolution - in the case of RPC, that's 500ns, rather than 10ms
which would be the case without gettimeoffset().  Removing
gettimeoffset() as Finn has proposed without preserving that
resolution is simply completely unacceptable.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Wed, Nov 14, 2018 at 02:35:29PM +1300, Michael Schmitz wrote:
> So we'd still have to use jiffies + interpolation from the current timer
> rundown counter as clocksource (since that will be monotonous and won't
> wrap)?
> 
> The way Finn did the clocksource for m68k, the clocksource counter does
> behave as it should (monotonous, doesn't wrap) at least so far as I've
> tested. Using the same timer for clocksource and clock events will degrade
> timekeeping based on timer events to jiffies resolution, as you point out.
> That would already have been the case before, so no gain in resolution.

... and that is where you are wrong.

RPC, for example, has gettimeofday() resolution of 500ns.  Removing
gettimeoffset() will result in a resolution of 1/HZ since there is
no longer any interpolation between interrupts.

> Other timekeeping would have worked at higher resolution before
> (interpolating through arch_gettimeoffset) just the same as now
> (interpolating through clocksource reads). Finn's clocksource read code
> essentially is arch_gettimeoffset under a new name.

Where is this code - all I've seen is code adding IRQ exclusion in
the gettimeoffset method.  If some other solution is being proposed,
then it's no good beating about the bush - show the damn code, and
then that can be considered.

However, what has been said in this thread is basically "remove the
gettimeoffset method", which is _not_ acceptable, it will cause
gettimeofday() on these platforms to lose resolution, which is a
user visible REGRESSION, plain and simple.

If what is actually meant is "we have a replacement for gettimeoffset"
then, and I'm sure we all know this, there needs to be a patch
proposed showing what is being proposed, rather than waffling around
the issue.

> Are you saying that's not possible on arm, because the current timer rundown
> counter can't be read while the timer is running?
> 
> If I were to run a second timer at higher rate for clocksource, but keeping
> the 10 ms timer as clock event (could easily do that by using timer D on
> Atari Falcon) - how would that improve my timekeeping? Clock events still
> only happen 10 ms apart ...

Ah, I think you're talking about something else.

You seem to be talking about what happens when time keeping interrupts
happen.

I'm talking about the resolution of gettimeofday() and the other
interfaces that are used (eg) for packet capture timestamping and
the like - the _user_ visible effects of the timekeeping system.

With the existing implementation, all these have better-than-jiffy
resolution - in the case of RPC, that's 500ns, rather than 10ms
which would be the case without gettimeoffset().  Removing
gettimeoffset() as Finn has proposed without preserving that
resolution is simply completely unacceptable.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Michael Schmitz]

On 14/11/18 8:58 PM, Russell King - ARM Linux wrote:
>
>> Are you saying that's not possible on arm, because the current timer rundown
>> counter can't be read while the timer is running?
>>
>> If I were to run a second timer at higher rate for clocksource, but keeping
>> the 10 ms timer as clock event (could easily do that by using timer D on
>> Atari Falcon) - how would that improve my timekeeping? Clock events still
>> only happen 10 ms apart ...
> Ah, I think you're talking about something else.
>
> You seem to be talking about what happens when time keeping interrupts
> happen.
That's what I understood your comment was about.
> I'm talking about the resolution of gettimeofday() and the other
> interfaces that are used (eg) for packet capture timestamping and
> the like - the _user_ visible effects of the timekeeping system.
>
> With the existing implementation, all these have better-than-jiffy
> resolution - in the case of RPC, that's 500ns, rather than 10ms
> which would be the case without gettimeoffset().  Removing
> gettimeoffset() as Finn has proposed without preserving that
> resolution is simply completely unacceptable.

I agree - but Finn had also offered to supply patches to arm that would 
have added a clocksource_read() function very much like for those m68k 
platforms that had used gettimeoffset(). I wondered what reason there 
was for these not to work on arm

I now realize you'd never seen that offer.

The proposal to drop architectures still relying on arch_gettimeoffset() 
had been raising enough of a response on linux-m68k to make me conclude 
that same proposal had been kicked on to other arch MLs affected as 
well. I'm a bit naive that way.

Now your criticism of arch_gettimeoffset() (missing timer rollover 
because the timer interrupt has been cleared by the interrupt handler) 
still stands. I just can't find the offset() functions shown in the 
5cfc8ee0bb51 patch. Any hints?

Cheers,

     Michael

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Michael Schmitz]

On 14/11/18 8:58 PM, Russell King - ARM Linux wrote:
>
>> Are you saying that's not possible on arm, because the current timer rundown
>> counter can't be read while the timer is running?
>>
>> If I were to run a second timer at higher rate for clocksource, but keeping
>> the 10 ms timer as clock event (could easily do that by using timer D on
>> Atari Falcon) - how would that improve my timekeeping? Clock events still
>> only happen 10 ms apart ...
> Ah, I think you're talking about something else.
>
> You seem to be talking about what happens when time keeping interrupts
> happen.
That's what I understood your comment was about.
> I'm talking about the resolution of gettimeofday() and the other
> interfaces that are used (eg) for packet capture timestamping and
> the like - the _user_ visible effects of the timekeeping system.
>
> With the existing implementation, all these have better-than-jiffy
> resolution - in the case of RPC, that's 500ns, rather than 10ms
> which would be the case without gettimeoffset().  Removing
> gettimeoffset() as Finn has proposed without preserving that
> resolution is simply completely unacceptable.

I agree - but Finn had also offered to supply patches to arm that would 
have added a clocksource_read() function very much like for those m68k 
platforms that had used gettimeoffset(). I wondered what reason there 
was for these not to work on arm

I now realize you'd never seen that offer.

The proposal to drop architectures still relying on arch_gettimeoffset() 
had been raising enough of a response on linux-m68k to make me conclude 
that same proposal had been kicked on to other arch MLs affected as 
well. I'm a bit naive that way.

Now your criticism of arch_gettimeoffset() (missing timer rollover 
because the timer interrupt has been cleared by the interrupt handler) 
still stands. I just can't find the offset() functions shown in the 
5cfc8ee0bb51 patch. Any hints?

Cheers,

 ??? Michael

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

On Tue, 13 Nov 2018, Russell King - ARM Linux wrote:

> 
> A clocksource provides a cycle counter that monotonically changes and 
> does not wrap between clockevent events.
> 
> A clock event is responsible for providing events to the system when 
> some work is needing to be done, limited by the wrap interval of the 
> clocksource.
> 
> Each time the clock event triggers an interrupt, the clocksource is
> read to determine how much time has passed, using:
> 
> 	count = (new_value - old_value) & available_bits
> 	nanosecs = count * scale >> shift;
> 
> If you try to combine the clocksource and clockevent because you only
> have a single counter, and the counter has the behaviour of:
> - counting down towards zero
> - when reaching zero, triggers an interrupt, and reloads with N
> 
> then this provides your clockevent, but you can't use this as a clock
> source, because each time you receive an interrupt and try to read the
> counter value, it will be approximately the same value.  This means
> that the above calculation fails to register the correct number of
> nanoseconds passing.  Hence, this does not work.
> 
> Also note where I said above that the clock event device must be able
> to provide an interrupt _before_ the clocksource wraps - clearly with
> such a timer, that is utterly impossible.
> 
> The simple solution is to not use such a counter as the clocksource,
> which means you fall back to using the jiffies clocksource, and your
> timekeeping has jiffy resolution - so 10ms, or possibly 1ms if you
> want a 1kHz timer interval.  For most applications, that's simply way
> to coarse, as was realised in the very early days of Linux.
> 
> If only there was a way to interpolate between timer interrupts...
> which is exactly what arch_gettimeoffset() does, and is a historical
> reminant of the pre-clocksource/clockevent days of the kernel - but
> it is the only way to get better resolution from this sort of setup.
> 

Both of the platforms in question (RPC and EBSA110) have not 
defined(CONFIG_GENERIC_CLOCKEVENTS) and have not defined any struct 
clock_event_device, AFAICT.

So, even assuming that you're right about the limitations of single-timer 
platforms in general, removal of arch_gettimeoffset wouldn't require the 
removal of any platforms, AFAICT.

-- 

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

On Tue, 13 Nov 2018, Russell King - ARM Linux wrote:

> 
> A clocksource provides a cycle counter that monotonically changes and 
> does not wrap between clockevent events.
> 
> A clock event is responsible for providing events to the system when 
> some work is needing to be done, limited by the wrap interval of the 
> clocksource.
> 
> Each time the clock event triggers an interrupt, the clocksource is
> read to determine how much time has passed, using:
> 
> 	count = (new_value - old_value) & available_bits
> 	nanosecs = count * scale >> shift;
> 
> If you try to combine the clocksource and clockevent because you only
> have a single counter, and the counter has the behaviour of:
> - counting down towards zero
> - when reaching zero, triggers an interrupt, and reloads with N
> 
> then this provides your clockevent, but you can't use this as a clock
> source, because each time you receive an interrupt and try to read the
> counter value, it will be approximately the same value.  This means
> that the above calculation fails to register the correct number of
> nanoseconds passing.  Hence, this does not work.
> 
> Also note where I said above that the clock event device must be able
> to provide an interrupt _before_ the clocksource wraps - clearly with
> such a timer, that is utterly impossible.
> 
> The simple solution is to not use such a counter as the clocksource,
> which means you fall back to using the jiffies clocksource, and your
> timekeeping has jiffy resolution - so 10ms, or possibly 1ms if you
> want a 1kHz timer interval.  For most applications, that's simply way
> to coarse, as was realised in the very early days of Linux.
> 
> If only there was a way to interpolate between timer interrupts...
> which is exactly what arch_gettimeoffset() does, and is a historical
> reminant of the pre-clocksource/clockevent days of the kernel - but
> it is the only way to get better resolution from this sort of setup.
> 

Both of the platforms in question (RPC and EBSA110) have not 
defined(CONFIG_GENERIC_CLOCKEVENTS) and have not defined any struct 
clock_event_device, AFAICT.

So, even assuming that you're right about the limitations of single-timer 
platforms in general, removal of arch_gettimeoffset wouldn't require the 
removal of any platforms, AFAICT.

-- 

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Wed, Nov 14, 2018 at 02:17:09PM +1100, Finn Thain wrote:
> On Tue, 13 Nov 2018, Russell King - ARM Linux wrote:
> 
> > 
> > A clocksource provides a cycle counter that monotonically changes and 
> > does not wrap between clockevent events.
> > 
> > A clock event is responsible for providing events to the system when 
> > some work is needing to be done, limited by the wrap interval of the 
> > clocksource.
> > 
> > Each time the clock event triggers an interrupt, the clocksource is
> > read to determine how much time has passed, using:
> > 
> > 	count = (new_value - old_value) & available_bits
> > 	nanosecs = count * scale >> shift;
> > 
> > If you try to combine the clocksource and clockevent because you only
> > have a single counter, and the counter has the behaviour of:
> > - counting down towards zero
> > - when reaching zero, triggers an interrupt, and reloads with N
> > 
> > then this provides your clockevent, but you can't use this as a clock
> > source, because each time you receive an interrupt and try to read the
> > counter value, it will be approximately the same value.  This means
> > that the above calculation fails to register the correct number of
> > nanoseconds passing.  Hence, this does not work.
> > 
> > Also note where I said above that the clock event device must be able
> > to provide an interrupt _before_ the clocksource wraps - clearly with
> > such a timer, that is utterly impossible.
> > 
> > The simple solution is to not use such a counter as the clocksource,
> > which means you fall back to using the jiffies clocksource, and your
> > timekeeping has jiffy resolution - so 10ms, or possibly 1ms if you
> > want a 1kHz timer interval.  For most applications, that's simply way
> > to coarse, as was realised in the very early days of Linux.
> > 
> > If only there was a way to interpolate between timer interrupts...
> > which is exactly what arch_gettimeoffset() does, and is a historical
> > reminant of the pre-clocksource/clockevent days of the kernel - but
> > it is the only way to get better resolution from this sort of setup.
> > 
> 
> Both of the platforms in question (RPC and EBSA110) have not 
> defined(CONFIG_GENERIC_CLOCKEVENTS) and have not defined any struct 
> clock_event_device, AFAICT.

Correct, because they don't use clockevents.  This is pre-clocksource,
pre-clockevent code, it uses the old timekeeping infrastructure,
which is xtime_update() and providing a gettimeoffset implementation.

> So, even assuming that you're right about the limitations of single-timer 
> platforms in general, removal of arch_gettimeoffset wouldn't require the 
> removal of any platforms, AFAICT.

I haven't proposed removing platforms.

I'm just objecting to the idea of removing arch_gettimeoffset(),
thereby causing a regression by changing the resolution of
gettimeofday() without any sign of equivalent functionality.

However, I now see (having searched mailing lists) what you are
trying to do - you have _not_ copied me or the mailing lists I'm
on with your cover message, so I'm *totally* lacking in the context
of your patch series, particularly where you are converting m68k
to use clocksources without needing the gettimeoffset() stuff.

You have failed to explain that in this thread - probably assuming
that I've read your cover message.  I haven't until now, because
you never sent it to me or the linux-arm-kernel mailing list.

I have found this thread _very_ frustrating, and frankly a waste of
my time discussing the finer points because of this lack of context.
Please ensure that if you're going to be sending a patch series,
that the cover message at least finds its way to the intended
audience of your patches, so that everyone has the context they
need when looking at (eg) the single patch they may receive.

Alternatively, if someone raises a problem with the patch, and you
_know_ you haven't done that, then please consider informing them
where they can get more context, eg, by providing a link to your
archived cover message.  It would help avoid misunderstandings.

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Wed, Nov 14, 2018 at 02:17:09PM +1100, Finn Thain wrote:
> On Tue, 13 Nov 2018, Russell King - ARM Linux wrote:
> 
> > 
> > A clocksource provides a cycle counter that monotonically changes and 
> > does not wrap between clockevent events.
> > 
> > A clock event is responsible for providing events to the system when 
> > some work is needing to be done, limited by the wrap interval of the 
> > clocksource.
> > 
> > Each time the clock event triggers an interrupt, the clocksource is
> > read to determine how much time has passed, using:
> > 
> > 	count = (new_value - old_value) & available_bits
> > 	nanosecs = count * scale >> shift;
> > 
> > If you try to combine the clocksource and clockevent because you only
> > have a single counter, and the counter has the behaviour of:
> > - counting down towards zero
> > - when reaching zero, triggers an interrupt, and reloads with N
> > 
> > then this provides your clockevent, but you can't use this as a clock
> > source, because each time you receive an interrupt and try to read the
> > counter value, it will be approximately the same value.  This means
> > that the above calculation fails to register the correct number of
> > nanoseconds passing.  Hence, this does not work.
> > 
> > Also note where I said above that the clock event device must be able
> > to provide an interrupt _before_ the clocksource wraps - clearly with
> > such a timer, that is utterly impossible.
> > 
> > The simple solution is to not use such a counter as the clocksource,
> > which means you fall back to using the jiffies clocksource, and your
> > timekeeping has jiffy resolution - so 10ms, or possibly 1ms if you
> > want a 1kHz timer interval.  For most applications, that's simply way
> > to coarse, as was realised in the very early days of Linux.
> > 
> > If only there was a way to interpolate between timer interrupts...
> > which is exactly what arch_gettimeoffset() does, and is a historical
> > reminant of the pre-clocksource/clockevent days of the kernel - but
> > it is the only way to get better resolution from this sort of setup.
> > 
> 
> Both of the platforms in question (RPC and EBSA110) have not 
> defined(CONFIG_GENERIC_CLOCKEVENTS) and have not defined any struct 
> clock_event_device, AFAICT.

Correct, because they don't use clockevents.  This is pre-clocksource,
pre-clockevent code, it uses the old timekeeping infrastructure,
which is xtime_update() and providing a gettimeoffset implementation.

> So, even assuming that you're right about the limitations of single-timer 
> platforms in general, removal of arch_gettimeoffset wouldn't require the 
> removal of any platforms, AFAICT.

I haven't proposed removing platforms.

I'm just objecting to the idea of removing arch_gettimeoffset(),
thereby causing a regression by changing the resolution of
gettimeofday() without any sign of equivalent functionality.

However, I now see (having searched mailing lists) what you are
trying to do - you have _not_ copied me or the mailing lists I'm
on with your cover message, so I'm *totally* lacking in the context
of your patch series, particularly where you are converting m68k
to use clocksources without needing the gettimeoffset() stuff.

You have failed to explain that in this thread - probably assuming
that I've read your cover message.  I haven't until now, because
you never sent it to me or the linux-arm-kernel mailing list.

I have found this thread _very_ frustrating, and frankly a waste of
my time discussing the finer points because of this lack of context.
Please ensure that if you're going to be sending a patch series,
that the cover message at least finds its way to the intended
audience of your patches, so that everyone has the context they
need when looking at (eg) the single patch they may receive.

Alternatively, if someone raises a problem with the patch, and you
_know_ you haven't done that, then please consider informing them
where they can get more context, eg, by providing a link to your
archived cover message.  It would help avoid misunderstandings.

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Geert Uytterhoeven]

Hi Russell,

On Wed, Nov 14, 2018 at 3:16 PM Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Nov 14, 2018 at 02:17:09PM +1100, Finn Thain wrote:
> > So, even assuming that you're right about the limitations of single-timer
> > platforms in general, removal of arch_gettimeoffset wouldn't require the
> > removal of any platforms, AFAICT.
>
> I haven't proposed removing platforms.
>
> I'm just objecting to the idea of removing arch_gettimeoffset(),
> thereby causing a regression by changing the resolution of
> gettimeofday() without any sign of equivalent functionality.
>
> However, I now see (having searched mailing lists) what you are
> trying to do - you have _not_ copied me or the mailing lists I'm
> on with your cover message, so I'm *totally* lacking in the context
> of your patch series, particularly where you are converting m68k
> to use clocksources without needing the gettimeoffset() stuff.
>
> You have failed to explain that in this thread - probably assuming
> that I've read your cover message.  I haven't until now, because
> you never sent it to me or the linux-arm-kernel mailing list.
>
> I have found this thread _very_ frustrating, and frankly a waste of
> my time discussing the finer points because of this lack of context.
> Please ensure that if you're going to be sending a patch series,
> that the cover message at least finds its way to the intended
> audience of your patches, so that everyone has the context they
> need when looking at (eg) the single patch they may receive.
>
> Alternatively, if someone raises a problem with the patch, and you
> _know_ you haven't done that, then please consider informing them
> where they can get more context, eg, by providing a link to your
> archived cover message.  It would help avoid misunderstandings.

Sorry for the lack of context.
The real trigger was also not explained in the cover message, and was a
the threat to remove platforms not using modern timekeeping APIs, cfr.
 "Removing support for old hardware from the kernel"
(https://lwn.net/Articles/769468/).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Geert Uytterhoeven]

Hi Russell,

On Wed, Nov 14, 2018 at 3:16 PM Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Nov 14, 2018 at 02:17:09PM +1100, Finn Thain wrote:
> > So, even assuming that you're right about the limitations of single-timer
> > platforms in general, removal of arch_gettimeoffset wouldn't require the
> > removal of any platforms, AFAICT.
>
> I haven't proposed removing platforms.
>
> I'm just objecting to the idea of removing arch_gettimeoffset(),
> thereby causing a regression by changing the resolution of
> gettimeofday() without any sign of equivalent functionality.
>
> However, I now see (having searched mailing lists) what you are
> trying to do - you have _not_ copied me or the mailing lists I'm
> on with your cover message, so I'm *totally* lacking in the context
> of your patch series, particularly where you are converting m68k
> to use clocksources without needing the gettimeoffset() stuff.
>
> You have failed to explain that in this thread - probably assuming
> that I've read your cover message.  I haven't until now, because
> you never sent it to me or the linux-arm-kernel mailing list.
>
> I have found this thread _very_ frustrating, and frankly a waste of
> my time discussing the finer points because of this lack of context.
> Please ensure that if you're going to be sending a patch series,
> that the cover message at least finds its way to the intended
> audience of your patches, so that everyone has the context they
> need when looking at (eg) the single patch they may receive.
>
> Alternatively, if someone raises a problem with the patch, and you
> _know_ you haven't done that, then please consider informing them
> where they can get more context, eg, by providing a link to your
> archived cover message.  It would help avoid misunderstandings.

Sorry for the lack of context.
The real trigger was also not explained in the cover message, and was a
the threat to remove platforms not using modern timekeeping APIs, cfr.
 "Removing support for old hardware from the kernel"
(https://lwn.net/Articles/769468/).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Wed, Nov 14, 2018 at 03:58:36PM +0100, Geert Uytterhoeven wrote:
> Hi Russell,
> 
> On Wed, Nov 14, 2018 at 3:16 PM Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Wed, Nov 14, 2018 at 02:17:09PM +1100, Finn Thain wrote:
> > > So, even assuming that you're right about the limitations of single-timer
> > > platforms in general, removal of arch_gettimeoffset wouldn't require the
> > > removal of any platforms, AFAICT.
> >
> > I haven't proposed removing platforms.
> >
> > I'm just objecting to the idea of removing arch_gettimeoffset(),
> > thereby causing a regression by changing the resolution of
> > gettimeofday() without any sign of equivalent functionality.
> >
> > However, I now see (having searched mailing lists) what you are
> > trying to do - you have _not_ copied me or the mailing lists I'm
> > on with your cover message, so I'm *totally* lacking in the context
> > of your patch series, particularly where you are converting m68k
> > to use clocksources without needing the gettimeoffset() stuff.
> >
> > You have failed to explain that in this thread - probably assuming
> > that I've read your cover message.  I haven't until now, because
> > you never sent it to me or the linux-arm-kernel mailing list.
> >
> > I have found this thread _very_ frustrating, and frankly a waste of
> > my time discussing the finer points because of this lack of context.
> > Please ensure that if you're going to be sending a patch series,
> > that the cover message at least finds its way to the intended
> > audience of your patches, so that everyone has the context they
> > need when looking at (eg) the single patch they may receive.
> >
> > Alternatively, if someone raises a problem with the patch, and you
> > _know_ you haven't done that, then please consider informing them
> > where they can get more context, eg, by providing a link to your
> > archived cover message.  It would help avoid misunderstandings.
> 
> Sorry for the lack of context.
> The real trigger was also not explained in the cover message, and was a
> the threat to remove platforms not using modern timekeeping APIs, cfr.
>  "Removing support for old hardware from the kernel"
> (https://lwn.net/Articles/769468/).

And naturally, because kernel developers are oh so great at
communication, that decision has been communicated to those
affected by it. *NOT*.

Clearly there is a need for much better communication.  We're
better at it when dealing with patches, but not when it comes to
physical meetings.

Maybe when some decision like "we're going to kill stuff still
using the old gettimeoffset API" then _someone_ needs to identify
which platforms are using that and make sure that those maintainers
know about that decision, much the same way that if a patch were
to touch the gettimeoffset API, we'd make damn sure that such a
patch went to those affected by the change.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Wed, Nov 14, 2018 at 03:58:36PM +0100, Geert Uytterhoeven wrote:
> Hi Russell,
> 
> On Wed, Nov 14, 2018 at 3:16 PM Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Wed, Nov 14, 2018 at 02:17:09PM +1100, Finn Thain wrote:
> > > So, even assuming that you're right about the limitations of single-timer
> > > platforms in general, removal of arch_gettimeoffset wouldn't require the
> > > removal of any platforms, AFAICT.
> >
> > I haven't proposed removing platforms.
> >
> > I'm just objecting to the idea of removing arch_gettimeoffset(),
> > thereby causing a regression by changing the resolution of
> > gettimeofday() without any sign of equivalent functionality.
> >
> > However, I now see (having searched mailing lists) what you are
> > trying to do - you have _not_ copied me or the mailing lists I'm
> > on with your cover message, so I'm *totally* lacking in the context
> > of your patch series, particularly where you are converting m68k
> > to use clocksources without needing the gettimeoffset() stuff.
> >
> > You have failed to explain that in this thread - probably assuming
> > that I've read your cover message.  I haven't until now, because
> > you never sent it to me or the linux-arm-kernel mailing list.
> >
> > I have found this thread _very_ frustrating, and frankly a waste of
> > my time discussing the finer points because of this lack of context.
> > Please ensure that if you're going to be sending a patch series,
> > that the cover message at least finds its way to the intended
> > audience of your patches, so that everyone has the context they
> > need when looking at (eg) the single patch they may receive.
> >
> > Alternatively, if someone raises a problem with the patch, and you
> > _know_ you haven't done that, then please consider informing them
> > where they can get more context, eg, by providing a link to your
> > archived cover message.  It would help avoid misunderstandings.
> 
> Sorry for the lack of context.
> The real trigger was also not explained in the cover message, and was a
> the threat to remove platforms not using modern timekeeping APIs, cfr.
>  "Removing support for old hardware from the kernel"
> (https://lwn.net/Articles/769468/).

And naturally, because kernel developers are oh so great at
communication, that decision has been communicated to those
affected by it. *NOT*.

Clearly there is a need for much better communication.  We're
better at it when dealing with patches, but not when it comes to
physical meetings.

Maybe when some decision like "we're going to kill stuff still
using the old gettimeoffset API" then _someone_ needs to identify
which platforms are using that and make sure that those maintainers
know about that decision, much the same way that if a patch were
to touch the gettimeoffset API, we'd make damn sure that such a
patch went to those affected by the change.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

On Wed, 14 Nov 2018, Russell King - ARM Linux wrote:

> However, I now see (having searched mailing lists) what you are trying 
> to do - you have _not_ copied me or the mailing lists I'm on with your 
> cover message, so I'm *totally* lacking in the context of your patch 
> series, particularly where you are converting m68k to use clocksources 
> without needing the gettimeoffset() stuff.
> 

True. I should have included all interested parties in the recipients of 
the cover letter. My bad.

> You have failed to explain that in this thread - probably assuming that 
> I've read your cover message.

I offered to write a patch to add a clocksource to replace the 
arch_gettimeoffset functionality for RPC and EBSA110.

You even replied to that offer.

I did not propose degrading functionality while there is someone able to 
test modernization patches (assuming there is...).

Would you consider merging untested modernization patches for the 
arch_gettimeoffset API?

> I haven't until now, because you never sent it to me or the 
> linux-arm-kernel mailing list.
> 
> I have found this thread _very_ frustrating, and frankly a waste of my 
> time discussing the finer points because of this lack of context.

Sorry for any frustration I've caused you.

The thread went way off-topic when Christoph took the opportunity to 
suggest the removal of RPC and EBSA110. That doesn't interest me.

My interest remains API modernization. The actual patches I've sent are 
intended to modernize the clock API *without* degrading any functionality.

> Please ensure that if you're going to be sending a patch series, that 
> the cover message at least finds its way to the intended audience of 
> your patches, so that everyone has the context they need when looking at 
> (eg) the single patch they may receive.
> 

OK. I'll have to improve my patch submission scripts.

-- 

> Alternatively, if someone raises a problem with the patch, and you 
> _know_ you haven't done that, then please consider informing them where 
> they can get more context, eg, by providing a link to your archived 
> cover message.  It would help avoid misunderstandings.
> 
> Thanks.
> 
> 

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

On Wed, 14 Nov 2018, Russell King - ARM Linux wrote:

> However, I now see (having searched mailing lists) what you are trying 
> to do - you have _not_ copied me or the mailing lists I'm on with your 
> cover message, so I'm *totally* lacking in the context of your patch 
> series, particularly where you are converting m68k to use clocksources 
> without needing the gettimeoffset() stuff.
> 

True. I should have included all interested parties in the recipients of 
the cover letter. My bad.

> You have failed to explain that in this thread - probably assuming that 
> I've read your cover message.

I offered to write a patch to add a clocksource to replace the 
arch_gettimeoffset functionality for RPC and EBSA110.

You even replied to that offer.

I did not propose degrading functionality while there is someone able to 
test modernization patches (assuming there is...).

Would you consider merging untested modernization patches for the 
arch_gettimeoffset API?

> I haven't until now, because you never sent it to me or the 
> linux-arm-kernel mailing list.
> 
> I have found this thread _very_ frustrating, and frankly a waste of my 
> time discussing the finer points because of this lack of context.

Sorry for any frustration I've caused you.

The thread went way off-topic when Christoph took the opportunity to 
suggest the removal of RPC and EBSA110. That doesn't interest me.

My interest remains API modernization. The actual patches I've sent are 
intended to modernize the clock API *without* degrading any functionality.

> Please ensure that if you're going to be sending a patch series, that 
> the cover message at least finds its way to the intended audience of 
> your patches, so that everyone has the context they need when looking at 
> (eg) the single patch they may receive.
> 

OK. I'll have to improve my patch submission scripts.

-- 

> Alternatively, if someone raises a problem with the patch, and you 
> _know_ you haven't done that, then please consider informing them where 
> they can get more context, eg, by providing a link to your archived 
> cover message.  It would help avoid misunderstandings.
> 
> Thanks.
> 
> 

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Thu, Nov 15, 2018 at 03:12:17PM +1100, Finn Thain wrote:
> The thread went way off-topic when Christoph took the opportunity to 
> suggest the removal of RPC and EBSA110. That doesn't interest me.

I suspect that is the right solution - I've been trying to get 4.19
to boot on my RPC and it's proving very difficult for several reasons,
not limited to the HDD seeming to be throwing the odd disk error, as
well as the kernel being rather large (a 4.19 kernel is 2.7M compressed
as opposed to 2.6.29 being 1.2M compressed for the equivalent
configuration.)

Given that memory on the RPC is not contiguous, that probably means
an uncompressed kernel overflows the size of the first memory bank,
so there's probably no hope for modern kernels to boot on the machine.

The EBSA110 is probably in a similar boat - I don't remember whether it
had 16MB or 32MB as the maximal amount of memory, but memory was getting
tight with some kernels even running a minimalist userspace.

So, it's probably time to say goodbyte to the kernel support for these
platforms.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Thu, Nov 15, 2018 at 03:12:17PM +1100, Finn Thain wrote:
> The thread went way off-topic when Christoph took the opportunity to 
> suggest the removal of RPC and EBSA110. That doesn't interest me.

I suspect that is the right solution - I've been trying to get 4.19
to boot on my RPC and it's proving very difficult for several reasons,
not limited to the HDD seeming to be throwing the odd disk error, as
well as the kernel being rather large (a 4.19 kernel is 2.7M compressed
as opposed to 2.6.29 being 1.2M compressed for the equivalent
configuration.)

Given that memory on the RPC is not contiguous, that probably means
an uncompressed kernel overflows the size of the first memory bank,
so there's probably no hope for modern kernels to boot on the machine.

The EBSA110 is probably in a similar boat - I don't remember whether it
had 16MB or 32MB as the maximal amount of memory, but memory was getting
tight with some kernels even running a minimalist userspace.

So, it's probably time to say goodbyte to the kernel support for these
platforms.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

On Fri, 16 Nov 2018, Russell King - ARM Linux wrote:

> 
> The EBSA110 is probably in a similar boat - I don't remember whether it 
> had 16MB or 32MB as the maximal amount of memory, but memory was getting 
> tight with some kernels even running a minimalist userspace.
> 
> So, it's probably time to say goodbyte to the kernel support for these 
> platforms.
> 

Your call.

Note that removing code from mainline won't help users obtain older, 
smaller, -stable kernel releases, free from the bug we were discussing. 
(The bug appeared in Linux v2.6.32.)

BTW, if you did want to boot Linux on a 16 MB system, you do have some 
options.

https://lwn.net/Articles/741494/
https://lwn.net/Articles/608945/
https://tiny.wiki.kernel.org/

Contributing to this kind of effort probably has value for IoT 
deployments. I suspect it also cuts a small amount of bloat from a large 
number of other Linux systems.

-- 

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

On Fri, 16 Nov 2018, Russell King - ARM Linux wrote:

> 
> The EBSA110 is probably in a similar boat - I don't remember whether it 
> had 16MB or 32MB as the maximal amount of memory, but memory was getting 
> tight with some kernels even running a minimalist userspace.
> 
> So, it's probably time to say goodbyte to the kernel support for these 
> platforms.
> 

Your call.

Note that removing code from mainline won't help users obtain older, 
smaller, -stable kernel releases, free from the bug we were discussing. 
(The bug appeared in Linux v2.6.32.)

BTW, if you did want to boot Linux on a 16 MB system, you do have some 
options.

https://lwn.net/Articles/741494/
https://lwn.net/Articles/608945/
https://tiny.wiki.kernel.org/

Contributing to this kind of effort probably has value for IoT 
deployments. I suspect it also cuts a small amount of bloat from a large 
number of other Linux systems.

-- 

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Michael Schmitz]

Hi Finn,

Am 17.11.2018 um 11:49 schrieb Finn Thain:
> On Fri, 16 Nov 2018, Russell King - ARM Linux wrote:
>
>>
>> The EBSA110 is probably in a similar boat - I don't remember whether it
>> had 16MB or 32MB as the maximal amount of memory, but memory was getting
>> tight with some kernels even running a minimalist userspace.
>>
>> So, it's probably time to say goodbyte to the kernel support for these
>> platforms.
>>
>
> Your call.
>
> Note that removing code from mainline won't help users obtain older,
> smaller, -stable kernel releases, free from the bug we were discussing.
> (The bug appeared in Linux v2.6.32.)
>
> BTW, if you did want to boot Linux on a 16 MB system, you do have some
> options.
>
> https://lwn.net/Articles/741494/
> https://lwn.net/Articles/608945/
> https://tiny.wiki.kernel.org/
>
> Contributing to this kind of effort probably has value for IoT
> deployments. I suspect it also cuts a small amount of bloat from a large
> number of other Linux systems.

I boot 4.19 on a system with 14 MB RAM - 10 MB remain for user space 
once the kernel loads. After remote login, 4 MB of that remain free for 
buffers or user code (1.5 MB swapped).
That's with sysvinit - Christian tried to boot a systemd userland on his 
Falcon once, and ran out of memory before swap could be activated.

I shouldn't say 16 or 32 MB are hopeless. And the 2.6 kernels were a lot 
more sluggish to my recollection.

Cheers,

	Michael

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Michael Schmitz]

Hi Finn,

Am 17.11.2018 um 11:49 schrieb Finn Thain:
> On Fri, 16 Nov 2018, Russell King - ARM Linux wrote:
>
>>
>> The EBSA110 is probably in a similar boat - I don't remember whether it
>> had 16MB or 32MB as the maximal amount of memory, but memory was getting
>> tight with some kernels even running a minimalist userspace.
>>
>> So, it's probably time to say goodbyte to the kernel support for these
>> platforms.
>>
>
> Your call.
>
> Note that removing code from mainline won't help users obtain older,
> smaller, -stable kernel releases, free from the bug we were discussing.
> (The bug appeared in Linux v2.6.32.)
>
> BTW, if you did want to boot Linux on a 16 MB system, you do have some
> options.
>
> https://lwn.net/Articles/741494/
> https://lwn.net/Articles/608945/
> https://tiny.wiki.kernel.org/
>
> Contributing to this kind of effort probably has value for IoT
> deployments. I suspect it also cuts a small amount of bloat from a large
> number of other Linux systems.

I boot 4.19 on a system with 14 MB RAM - 10 MB remain for user space 
once the kernel loads. After remote login, 4 MB of that remain free for 
buffers or user code (1.5 MB swapped).
That's with sysvinit - Christian tried to boot a systemd userland on his 
Falcon once, and ran out of memory before swap could be activated.

I shouldn't say 16 or 32 MB are hopeless. And the 2.6 kernels were a lot 
more sluggish to my recollection.

Cheers,

	Michael

Re: [RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Mon, Nov 12, 2018 at 03:12:39PM +1100, Finn Thain wrote:
> Implementations of arch_gettimeoffset are generally not re-entrant
> and assume that interrupts have been disabled. Unfortunately this
> pre-condition got broken in v2.6.32.

To me, it looks way worse than what you think.

The original code sequence before conversion to arch_gettimeoffset()
was:

1. lock (inc. disabling IRQs)
2. read gettimeoffset correction
3. add offset to current xtime
4. unlock

This means there is no chance for a timer interrupt to happen between
reading the current offset and adding it to the current kernel time.
If the timer does roll over, then the interrupt will remain pending,
so the whole "read offset and add to xtime" is one atomic block and
we can use the pending interrupt to indicate whether the timer has
rolled over and apply the appropriate offset correction.

With the arch_gettimeoffset() code, that changes:

1. read clocksource (which will be jiffies)
2. compute clocksource delta
3. increment nanoseconds
4. add gettimeoffset correction

If we receive a timer interrupt part-way through this, for example,
between 2 and 3, then jiffies will increment (via do_timer()) and the
interrupt will be cleared.  This means that the check in
ioc_timer_gettimeoffset() for a pending interrupt, for example, will
be false, and we will not know that an interrupt has happened.

So, unless I'm missing something, commit 5cfc8ee0bb51 well and truely
broke the accuracy of timekeeping on these platforms, and will result
in timekeeping spontaneously jumping backwards.  I had been wondering
why NTP had become less stable on EBSA110, but never investigated.

However, that still does not justify blowing away this functionality
without replacement, as Finn has been proposing.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[RFC PATCH 01/13] arm: Fix mutual exclusion in arch_gettimeoffset

[Russell King - ARM Linux]

On Mon, Nov 12, 2018 at 03:12:39PM +1100, Finn Thain wrote:
> Implementations of arch_gettimeoffset are generally not re-entrant
> and assume that interrupts have been disabled. Unfortunately this
> pre-condition got broken in v2.6.32.

To me, it looks way worse than what you think.

The original code sequence before conversion to arch_gettimeoffset()
was:

1. lock (inc. disabling IRQs)
2. read gettimeoffset correction
3. add offset to current xtime
4. unlock

This means there is no chance for a timer interrupt to happen between
reading the current offset and adding it to the current kernel time.
If the timer does roll over, then the interrupt will remain pending,
so the whole "read offset and add to xtime" is one atomic block and
we can use the pending interrupt to indicate whether the timer has
rolled over and apply the appropriate offset correction.

With the arch_gettimeoffset() code, that changes:

1. read clocksource (which will be jiffies)
2. compute clocksource delta
3. increment nanoseconds
4. add gettimeoffset correction

If we receive a timer interrupt part-way through this, for example,
between 2 and 3, then jiffies will increment (via do_timer()) and the
interrupt will be cleared.  This means that the check in
ioc_timer_gettimeoffset() for a pending interrupt, for example, will
be false, and we will not know that an interrupt has happened.

So, unless I'm missing something, commit 5cfc8ee0bb51 well and truely
broke the accuracy of timekeeping on these platforms, and will result
in timekeeping spontaneously jumping backwards.  I had been wondering
why NTP had become less stable on EBSA110, but never investigated.

However, that still does not justify blowing away this functionality
without replacement, as Finn has been proposing.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[RFC PATCH 02/13] m68k: Fix mutual exclusion in arch_gettimeoffset

[Finn Thain]

Implementations of arch_gettimeoffset are generally not re-entrant
and assume that interrupts have been disabled. Unfortunately this
pre-condition got broken in v2.6.32.

Cc: Philip Blundell <philb@gnu.org>
Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: Joshua Thompson <funaho@jurai.org>
Fixes: 4ad4c76b7afb ("m68k: convert to use arch_gettimeoffset()")
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/amiga/config.c    |  6 +++++-
 arch/m68k/atari/time.c      |  6 +++++-
 arch/m68k/bvme6000/config.c | 10 +++++++---
 arch/m68k/hp300/time.c      |  8 +++++++-
 arch/m68k/mac/via.c         |  5 +++++
 arch/m68k/mvme147/config.c  |  6 +++++-
 arch/m68k/mvme16x/config.c  |  1 -
 7 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/arch/m68k/amiga/config.c b/arch/m68k/amiga/config.c
index 65f63a457130..5ec3687984a9 100644
--- a/arch/m68k/amiga/config.c
+++ b/arch/m68k/amiga/config.c
@@ -492,12 +492,14 @@ static void __init amiga_sched_init(irq_handler_t timer_routine)
 
 #define TICK_SIZE 10000
 
-/* This is always executed with interrupts disabled.  */
 static u32 amiga_gettimeoffset(void)
 {
+	unsigned long flags;
 	unsigned short hi, lo, hi2;
 	u32 ticks, offset = 0;
 
+	local_irq_save(flags);
+
 	/* read CIA B timer A current value */
 	hi  = ciab.tahi;
 	lo  = ciab.talo;
@@ -515,6 +517,8 @@ static u32 amiga_gettimeoffset(void)
 		if (cia_set_irq(&ciab_base, 0) & CIA_ICR_TA)
 			offset = 10000;
 
+	local_irq_restore(flags);
+
 	ticks = jiffy_ticks - ticks;
 	ticks = (10000 * ticks) / jiffy_ticks;
 
diff --git a/arch/m68k/atari/time.c b/arch/m68k/atari/time.c
index 9cca64286464..4765e9a58293 100644
--- a/arch/m68k/atari/time.c
+++ b/arch/m68k/atari/time.c
@@ -40,11 +40,13 @@ atari_sched_init(irq_handler_t timer_routine)
 
 #define TICK_SIZE 10000
 
-/* This is always executed with interrupts disabled.  */
 u32 atari_gettimeoffset(void)
 {
+  unsigned long flags;
   u32 ticks, offset = 0;
 
+  local_irq_save(flags);
+
   /* read MFP timer C current value */
   ticks = st_mfp.tim_dt_c;
   /* The probability of underflow is less than 2% */
@@ -53,6 +55,8 @@ u32 atari_gettimeoffset(void)
     if (st_mfp.int_pn_b & (1 << 5))
       offset = TICK_SIZE;
 
+  local_irq_restore(flags);
+
   ticks = INT_TICKS - ticks;
   ticks = ticks * 10000L / INT_TICKS;
 
diff --git a/arch/m68k/bvme6000/config.c b/arch/m68k/bvme6000/config.c
index 143ee9fa3893..0afdef10a5a4 100644
--- a/arch/m68k/bvme6000/config.c
+++ b/arch/m68k/bvme6000/config.c
@@ -206,8 +206,6 @@ void bvme6000_sched_init (irq_handler_t timer_routine)
 }
 
 
-/* This is always executed with interrupts disabled.  */
-
 /*
  * NOTE:  Don't accept any readings within 5us of rollover, as
  * the T1INT bit may be a little slow getting set.  There is also
@@ -217,12 +215,16 @@ void bvme6000_sched_init (irq_handler_t timer_routine)
 
 u32 bvme6000_gettimeoffset(void)
 {
+    unsigned long flags;
     volatile RtcPtr_t rtc = (RtcPtr_t)BVME_RTC_BASE;
     volatile PitRegsPtr pit = (PitRegsPtr)BVME_PIT_BASE;
-    unsigned char msr = rtc->msr & 0xc0;
+    unsigned char msr;
     unsigned char t1int, t1op;
     u32 v = 800000, ov;
 
+    local_irq_save(flags);
+
+    msr = rtc->msr & 0xc0;
     rtc->msr = 0;	/* Ensure timer registers accessible */
 
     do {
@@ -245,6 +247,8 @@ u32 bvme6000_gettimeoffset(void)
 	v += 10000;			/* Int pending, + 10ms */
     rtc->msr = msr;
 
+    local_irq_restore(flags);
+
     return v * 1000;
 }
 
diff --git a/arch/m68k/hp300/time.c b/arch/m68k/hp300/time.c
index 289d928a46cb..5cf711fd0858 100644
--- a/arch/m68k/hp300/time.c
+++ b/arch/m68k/hp300/time.c
@@ -49,16 +49,22 @@ static irqreturn_t hp300_tick(int irq, void *dev_id)
 
 u32 hp300_gettimeoffset(void)
 {
-  /* Read current timer 1 value */
+  unsigned long flags;
   unsigned char lsb, msb1, msb2;
   unsigned short ticks;
 
+  local_irq_save(flags);
+
+  /* Read current timer 1 value */
   msb1 = in_8(CLOCKBASE + 5);
   lsb = in_8(CLOCKBASE + 7);
   msb2 = in_8(CLOCKBASE + 5);
   if (msb1 != msb2)
     /* A carry happened while we were reading.  Read it again */
     lsb = in_8(CLOCKBASE + 7);
+
+  local_irq_restore(flags);
+
   ticks = INTVAL - ((msb2 << 8) | lsb);
   return ((USECS_PER_JIFFY * ticks) / INTVAL) * 1000;
 }
diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
index e4facff0c1f3..e5dff74f59b3 100644
--- a/arch/m68k/mac/via.c
+++ b/arch/m68k/mac/via.c
@@ -318,8 +318,11 @@ void via_debug_dump(void)
 
 u32 mac_gettimeoffset(void)
 {
+	unsigned long flags;
 	unsigned long ticks, offset = 0;
 
+	local_irq_save(flags);
+
 	/* read VIA1 timer 2 current value */
 	ticks = via1[vT1CL] | (via1[vT1CH] << 8);
 	/* The probability of underflow is less than 2% */
@@ -327,6 +330,8 @@ u32 mac_gettimeoffset(void)
 		/* Check for pending timer interrupt in VIA1 IFR */
 		if (via1[vIFR] & 0x40) offset = TICK_SIZE;
 
+	local_irq_restore(flags);
+
 	ticks = MAC_CLOCK_TICK - ticks;
 	ticks = ticks * 10000L / MAC_CLOCK_TICK;
 
diff --git a/arch/m68k/mvme147/config.c b/arch/m68k/mvme147/config.c
index adea549d240e..8074940b0aa1 100644
--- a/arch/m68k/mvme147/config.c
+++ b/arch/m68k/mvme147/config.c
@@ -125,17 +125,21 @@ void mvme147_sched_init (irq_handler_t timer_routine)
 	m147_pcc->t1_int_cntrl = PCC_INT_ENAB|PCC_LEVEL_TIMER1;
 }
 
-/* This is always executed with interrupts disabled.  */
 /* XXX There are race hazards in this code XXX */
 u32 mvme147_gettimeoffset(void)
 {
+	unsigned long flags;
 	volatile unsigned short *cp = (volatile unsigned short *)0xfffe1012;
 	unsigned short n;
 
+	local_irq_save(flags);
+
 	n = *cp;
 	while (n != *cp)
 		n = *cp;
 
+	local_irq_restore(flags);
+
 	n -= PCC_TIMER_PRELOAD;
 	return ((unsigned long)n * 25 / 4) * 1000;
 }
diff --git a/arch/m68k/mvme16x/config.c b/arch/m68k/mvme16x/config.c
index 6ee36a5b528d..d4aec717e688 100644
--- a/arch/m68k/mvme16x/config.c
+++ b/arch/m68k/mvme16x/config.c
@@ -381,7 +381,6 @@ void mvme16x_sched_init (irq_handler_t timer_routine)
 }
 
 
-/* This is always executed with interrupts disabled.  */
 u32 mvme16x_gettimeoffset(void)
 {
     return (*(volatile u32 *)0xfff42008) * 1000;
-- 
2.18.1

[RFC PATCH 07/13] m68k: amiga: Convert to clocksource API

[Finn Thain]

Add a platform clocksource by adapting the existing arch_gettimeoffset
implementation.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/amiga/config.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/arch/m68k/amiga/config.c b/arch/m68k/amiga/config.c
index a575ad419b23..40ebd471cc3c 100644
--- a/arch/m68k/amiga/config.c
+++ b/arch/m68k/amiga/config.c
@@ -17,6 +17,7 @@
 #include <linux/mm.h>
 #include <linux/seq_file.h>
 #include <linux/tty.h>
+#include <linux/clocksource.h>
 #include <linux/console.h>
 #include <linux/rtc.h>
 #include <linux/init.h>
@@ -461,7 +462,30 @@ void __init config_amiga(void)
 		*(unsigned char *)ZTWO_VADDR(0xde0002) |= 0x80;
 }
 
+static u64 amiga_read_clk(struct clocksource *cs);
+
+static struct clocksource amiga_clk = {
+	.name   = "ciab",
+	.rating = 250,
+	.read   = amiga_read_clk,
+	.mask   = CLOCKSOURCE_MASK(32),
+	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
 static unsigned short jiffy_ticks;
+static u32 clk_total;
+
+static irqreturn_t ciab_timer_handler(int irq, void *data)
+{
+	irq_handler_t timer_routine = data;
+	unsigned long flags;
+
+	local_irq_save(flags);
+	clk_total += jiffy_ticks;
+	local_irq_restore(flags);
+
+	return timer_routine(irq, data);
+}
 
 static void __init amiga_sched_init(irq_handler_t timer_routine)
 {
@@ -481,15 +505,16 @@ static void __init amiga_sched_init(irq_handler_t timer_routine)
 	 * Please don't change this to use ciaa, as it interferes with the
 	 * SCSI code. We'll have to take a look at this later
 	 */
-	if (request_irq(IRQ_AMIGA_CIAB_TA, timer_routine, 0, "timer", NULL))
+	if (request_irq(IRQ_AMIGA_CIAB_TA, ciab_timer_handler, IRQF_TIMER,
+			"timer", timer_routine))
 		pr_err("Couldn't register timer interrupt\n");
 	/* start timer */
 	ciab.cra |= 0x11;
-}
 
-#define TICK_SIZE 10000
+	clocksource_register_hz(&amiga_clk, amiga_eclock);
+}
 
-static u32 amiga_gettimeoffset(void)
+static u64 amiga_read_clk(struct clocksource *cs)
 {
 	unsigned long flags;
 	unsigned short hi, lo, hi2;
@@ -512,14 +537,13 @@ static u32 amiga_gettimeoffset(void)
 	if (ticks > jiffy_ticks / 2)
 		/* check for pending interrupt */
 		if (cia_set_irq(&ciab_base, 0) & CIA_ICR_TA)
-			offset = 10000;
+			offset = jiffy_ticks;
 
 	local_irq_restore(flags);
 
 	ticks = jiffy_ticks - ticks;
-	ticks = (10000 * ticks) / jiffy_ticks;
 
-	return (ticks + offset) * 1000;
+	return clk_total + ticks + offset;
 }
 
 static void amiga_reset(void)  __noreturn;
-- 
2.18.1

Re: [RFC PATCH 07/13] m68k: amiga: Convert to clocksource API

[Geert Uytterhoeven]

Hi Finn,

On Mon, Nov 12, 2018 at 5:46 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> Add a platform clocksource by adapting the existing arch_gettimeoffset
> implementation.
>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Thanks for your patch!

> --- a/arch/m68k/amiga/config.c
> +++ b/arch/m68k/amiga/config.c
> @@ -17,6 +17,7 @@
>  #include <linux/mm.h>
>  #include <linux/seq_file.h>
>  #include <linux/tty.h>
> +#include <linux/clocksource.h>
>  #include <linux/console.h>
>  #include <linux/rtc.h>
>  #include <linux/init.h>
> @@ -461,7 +462,30 @@ void __init config_amiga(void)
>                 *(unsigned char *)ZTWO_VADDR(0xde0002) |= 0x80;
>  }
>
> +static u64 amiga_read_clk(struct clocksource *cs);
> +
> +static struct clocksource amiga_clk = {
> +       .name   = "ciab",
> +       .rating = 250,
> +       .read   = amiga_read_clk,
> +       .mask   = CLOCKSOURCE_MASK(32),
> +       .flags  = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
>  static unsigned short jiffy_ticks;
> +static u32 clk_total;

Shouldn't clk_total be u64?

> +
> +static irqreturn_t ciab_timer_handler(int irq, void *data)
> +{
> +       irq_handler_t timer_routine = data;
> +       unsigned long flags;
> +
> +       local_irq_save(flags);
> +       clk_total += jiffy_ticks;
> +       local_irq_restore(flags);
> +
> +       return timer_routine(irq, data);
> +}

[...]

> -static u32 amiga_gettimeoffset(void)
> +static u64 amiga_read_clk(struct clocksource *cs)
>  {
>         unsigned long flags;
>         unsigned short hi, lo, hi2;
> @@ -512,14 +537,13 @@ static u32 amiga_gettimeoffset(void)
>         if (ticks > jiffy_ticks / 2)
>                 /* check for pending interrupt */
>                 if (cia_set_irq(&ciab_base, 0) & CIA_ICR_TA)
> -                       offset = 10000;
> +                       offset = jiffy_ticks;
>
>         local_irq_restore(flags);
>
>         ticks = jiffy_ticks - ticks;
> -       ticks = (10000 * ticks) / jiffy_ticks;
>
> -       return (ticks + offset) * 1000;
> +       return clk_total + ticks + offset;

... to return a full 64-bit value here?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 07/13] m68k: amiga: Convert to clocksource API

[Finn Thain]

On Mon, 12 Nov 2018, Geert Uytterhoeven wrote:

> Hi Finn,
> 
> On Mon, Nov 12, 2018 at 5:46 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > Add a platform clocksource by adapting the existing arch_gettimeoffset
> > implementation.
> >
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> 
> Thanks for your patch!
> 

Thanks for your review.

> > --- a/arch/m68k/amiga/config.c
> > +++ b/arch/m68k/amiga/config.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/mm.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/tty.h>
> > +#include <linux/clocksource.h>
> >  #include <linux/console.h>
> >  #include <linux/rtc.h>
> >  #include <linux/init.h>
> > @@ -461,7 +462,30 @@ void __init config_amiga(void)
> >                 *(unsigned char *)ZTWO_VADDR(0xde0002) |= 0x80;
> >  }
> >
> > +static u64 amiga_read_clk(struct clocksource *cs);
> > +
> > +static struct clocksource amiga_clk = {
> > +       .name   = "ciab",
> > +       .rating = 250,
> > +       .read   = amiga_read_clk,
> > +       .mask   = CLOCKSOURCE_MASK(32),
> > +       .flags  = CLOCK_SOURCE_IS_CONTINUOUS,
> > +};
> > +
> >  static unsigned short jiffy_ticks;
> > +static u32 clk_total;
> 
> Shouldn't clk_total be u64?
> 

Well, it was intentionally u32, and amiga_clk.mask was intentionally 
CLOCKSOURCE_MASK(32)...

> > +
> > +static irqreturn_t ciab_timer_handler(int irq, void *data)
> > +{
> > +       irq_handler_t timer_routine = data;
> > +       unsigned long flags;
> > +
> > +       local_irq_save(flags);
> > +       clk_total += jiffy_ticks;
> > +       local_irq_restore(flags);
> > +
> > +       return timer_routine(irq, data);
> > +}
> 
> [...]
> 
> > -static u32 amiga_gettimeoffset(void)
> > +static u64 amiga_read_clk(struct clocksource *cs)
> >  {
> >         unsigned long flags;
> >         unsigned short hi, lo, hi2;
> > @@ -512,14 +537,13 @@ static u32 amiga_gettimeoffset(void)
> >         if (ticks > jiffy_ticks / 2)
> >                 /* check for pending interrupt */
> >                 if (cia_set_irq(&ciab_base, 0) & CIA_ICR_TA)
> > -                       offset = 10000;
> > +                       offset = jiffy_ticks;
> >
> >         local_irq_restore(flags);
> >
> >         ticks = jiffy_ticks - ticks;
> > -       ticks = (10000 * ticks) / jiffy_ticks;
> >
> > -       return (ticks + offset) * 1000;
> > +       return clk_total + ticks + offset;
> 
> ... to return a full 64-bit value here?
> 

It's certainly possible. Is it better? It sounds like a question about the 
cost of wrap-around in the clocksource core vs. the cost of 64-bit 
additions in the arch code.

Looking at __clocksource_update_freq_scale() there are some consequences 
to choosing a 32-bit mask --

                 * For clocksources which have a mask > 32-bit
                 * we need to limit the max sleep time to have a good
                 * conversion precision.

I don't know the answer, sorry.

-- 

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 

[RFC PATCH 09/13] m68k: bvme6000: Convert to clocksource API

[Finn Thain]

Add a platform clocksource by adapting the existing arch_gettimeoffset
implementation.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/bvme6000/config.c | 54 ++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/arch/m68k/bvme6000/config.c b/arch/m68k/bvme6000/config.c
index 3bd56bd04690..099cc6edebc8 100644
--- a/arch/m68k/bvme6000/config.c
+++ b/arch/m68k/bvme6000/config.c
@@ -18,6 +18,7 @@
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/tty.h>
+#include <linux/clocksource.h>
 #include <linux/console.h>
 #include <linux/linkage.h>
 #include <linux/init.h>
@@ -43,12 +44,6 @@ extern int bvme6000_hwclk (int, struct rtc_time *);
 extern void bvme6000_reset (void);
 void bvme6000_set_vectors (void);
 
-/* Save tick handler routine pointer, will point to xtime_update() in
- * kernel/timer/timekeeping.c, called via bvme6000_process_int() */
-
-static irq_handler_t tick_handler;
-
-
 int __init bvme6000_parse_bootinfo(const struct bi_record *bi)
 {
 	if (be16_to_cpu(bi->tag) == BI_VME_TYPE)
@@ -152,13 +147,34 @@ irqreturn_t bvme6000_abort_int (int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static u64 bvme6000_read_clk(struct clocksource *cs);
+
+static struct clocksource bvme6000_clk = {
+	.name   = "rtc",
+	.rating = 250,
+	.read   = bvme6000_read_clk,
+	.mask   = CLOCKSOURCE_MASK(32),
+	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+static u32 clk_total;
+
+#define RTC_TIMER_CLOCK_FREQ 8000000
+#define RTC_TIMER_CYCLES     (RTC_TIMER_CLOCK_FREQ / HZ)
+#define RTC_TIMER_COUNT      ((RTC_TIMER_CYCLES / 2) - 1)
 
 static irqreturn_t bvme6000_timer_int (int irq, void *dev_id)
 {
+    irq_handler_t tick_handler = dev_id;
     volatile RtcPtr_t rtc = (RtcPtr_t)BVME_RTC_BASE;
-    unsigned char msr = rtc->msr & 0xc0;
+    unsigned char msr;
+    unsigned long flags;
 
+    local_irq_save(flags);
+    msr = rtc->msr & 0xc0;
     rtc->msr = msr | 0x20;		/* Ack the interrupt */
+    clk_total += RTC_TIMER_CYCLES;
+    local_irq_restore(flags);
 
     return tick_handler(irq, dev_id);
 }
@@ -179,14 +195,13 @@ void bvme6000_sched_init (irq_handler_t timer_routine)
 
     rtc->msr = 0;	/* Ensure timer registers accessible */
 
-    tick_handler = timer_routine;
-    if (request_irq(BVME_IRQ_RTC, bvme6000_timer_int, 0,
-				"timer", bvme6000_timer_int))
+    if (request_irq(BVME_IRQ_RTC, bvme6000_timer_int, IRQF_TIMER,
+				"timer", timer_routine))
 	panic ("Couldn't register timer int");
 
     rtc->t1cr_omr = 0x04;	/* Mode 2, ext clk */
-    rtc->t1msb = 39999 >> 8;
-    rtc->t1lsb = 39999 & 0xff;
+    rtc->t1msb = RTC_TIMER_COUNT >> 8;
+    rtc->t1lsb = RTC_TIMER_COUNT & 0xff;
     rtc->irr_icr1 &= 0xef;	/* Route timer 1 to INTR pin */
     rtc->msr = 0x40;		/* Access int.cntrl, etc */
     rtc->pfr_icr0 = 0x80;	/* Just timer 1 ints enabled */
@@ -198,6 +213,8 @@ void bvme6000_sched_init (irq_handler_t timer_routine)
 
     rtc->msr = msr;
 
+    clocksource_register_hz(&bvme6000_clk, RTC_TIMER_CLOCK_FREQ);
+
     if (request_irq(BVME_IRQ_ABORT, bvme6000_abort_int, 0,
 				"abort", bvme6000_abort_int))
 	panic ("Couldn't register abort int");
@@ -211,7 +228,7 @@ void bvme6000_sched_init (irq_handler_t timer_routine)
  * results...
  */
 
-u32 bvme6000_gettimeoffset(void)
+static u64 bvme6000_read_clk(struct clocksource *cs)
 {
     unsigned long flags;
     volatile RtcPtr_t rtc = (RtcPtr_t)BVME_RTC_BASE;
@@ -235,19 +252,18 @@ u32 bvme6000_gettimeoffset(void)
     } while (t1int != (rtc->msr & 0x20) ||
 		t1op != (pit->pcdr & 0x04) ||
 			abs(ov-v) > 80 ||
-				v > 39960);
+				v > RTC_TIMER_COUNT - (RTC_TIMER_COUNT / 100));
 
-    v = 39999 - v;
+    v = RTC_TIMER_COUNT - v;
     if (!t1op)				/* If in second half cycle.. */
-	v += 40000;
-    v /= 8;				/* Convert ticks to microseconds */
+	v += RTC_TIMER_CYCLES / 2;
     if (t1int)
-	v += 10000;			/* Int pending, + 10ms */
+	v += RTC_TIMER_CYCLES;
     rtc->msr = msr;
 
     local_irq_restore(flags);
 
-    return v * 1000;
+    return clk_total + v;
 }
 
 /*
-- 
2.18.1

[RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Finn Thain]

The functions that implement arch_gettimeoffset are re-used by
new clocksource drivers in subsequent patches.

Cc: Philip Blundell <philb@gnu.org>
Cc: Michael Schmitz <schmitzmic@gmail.com>
Cc: Joshua Thompson <funaho@jurai.org>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/Kconfig           | 1 -
 arch/m68k/amiga/config.c    | 3 ---
 arch/m68k/atari/config.c    | 2 --
 arch/m68k/bvme6000/config.c | 2 --
 arch/m68k/hp300/config.c    | 1 -
 arch/m68k/hp300/time.h      | 1 -
 arch/m68k/mac/config.c      | 3 ---
 arch/m68k/mvme147/config.c  | 2 --
 arch/m68k/mvme16x/config.c  | 2 --
 9 files changed, 17 deletions(-)

diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
index 070553791e97..cc62660a5760 100644
--- a/arch/m68k/Kconfig
+++ b/arch/m68k/Kconfig
@@ -19,7 +19,6 @@ config M68K
 	select GENERIC_STRNCPY_FROM_USER if MMU
 	select GENERIC_STRNLEN_USER if MMU
 	select ARCH_WANT_IPC_PARSE_VERSION
-	select ARCH_USES_GETTIMEOFFSET if MMU && !COLDFIRE
 	select HAVE_FUTEX_CMPXCHG if MMU && FUTEX
 	select HAVE_MOD_ARCH_SPECIFIC
 	select MODULES_USE_ELF_REL
diff --git a/arch/m68k/amiga/config.c b/arch/m68k/amiga/config.c
index 5ec3687984a9..a575ad419b23 100644
--- a/arch/m68k/amiga/config.c
+++ b/arch/m68k/amiga/config.c
@@ -95,8 +95,6 @@ static char amiga_model_name[13] = "Amiga ";
 static void amiga_sched_init(irq_handler_t handler);
 static void amiga_get_model(char *model);
 static void amiga_get_hardware_list(struct seq_file *m);
-/* amiga specific timer functions */
-static u32 amiga_gettimeoffset(void);
 extern void amiga_mksound(unsigned int count, unsigned int ticks);
 static void amiga_reset(void);
 extern void amiga_init_sound(void);
@@ -386,7 +384,6 @@ void __init config_amiga(void)
 	mach_init_IRQ        = amiga_init_IRQ;
 	mach_get_model       = amiga_get_model;
 	mach_get_hardware_list = amiga_get_hardware_list;
-	arch_gettimeoffset   = amiga_gettimeoffset;
 
 	/*
 	 * default MAX_DMA=0xffffffff on all machines. If we don't do so, the SCSI
diff --git a/arch/m68k/atari/config.c b/arch/m68k/atari/config.c
index bd96702a1ad0..89e65be2655f 100644
--- a/arch/m68k/atari/config.c
+++ b/arch/m68k/atari/config.c
@@ -78,7 +78,6 @@ static void atari_heartbeat(int on);
 
 /* atari specific timer functions (in time.c) */
 extern void atari_sched_init(irq_handler_t);
-extern u32 atari_gettimeoffset(void);
 extern int atari_mste_hwclk (int, struct rtc_time *);
 extern int atari_tt_hwclk (int, struct rtc_time *);
 
@@ -205,7 +204,6 @@ void __init config_atari(void)
 	mach_init_IRQ        = atari_init_IRQ;
 	mach_get_model	 = atari_get_model;
 	mach_get_hardware_list = atari_get_hardware_list;
-	arch_gettimeoffset   = atari_gettimeoffset;
 	mach_reset           = atari_reset;
 	mach_max_dma_address = 0xffffff;
 #if IS_ENABLED(CONFIG_INPUT_M68K_BEEP)
diff --git a/arch/m68k/bvme6000/config.c b/arch/m68k/bvme6000/config.c
index 0afdef10a5a4..3bd56bd04690 100644
--- a/arch/m68k/bvme6000/config.c
+++ b/arch/m68k/bvme6000/config.c
@@ -39,7 +39,6 @@
 
 static void bvme6000_get_model(char *model);
 extern void bvme6000_sched_init(irq_handler_t handler);
-extern u32 bvme6000_gettimeoffset(void);
 extern int bvme6000_hwclk (int, struct rtc_time *);
 extern void bvme6000_reset (void);
 void bvme6000_set_vectors (void);
@@ -110,7 +109,6 @@ void __init config_bvme6000(void)
     mach_max_dma_address = 0xffffffff;
     mach_sched_init      = bvme6000_sched_init;
     mach_init_IRQ        = bvme6000_init_IRQ;
-    arch_gettimeoffset   = bvme6000_gettimeoffset;
     mach_hwclk           = bvme6000_hwclk;
     mach_reset		 = bvme6000_reset;
     mach_get_model       = bvme6000_get_model;
diff --git a/arch/m68k/hp300/config.c b/arch/m68k/hp300/config.c
index a19bcd23f80b..a161d44fd20b 100644
--- a/arch/m68k/hp300/config.c
+++ b/arch/m68k/hp300/config.c
@@ -254,7 +254,6 @@ void __init config_hp300(void)
 	mach_sched_init      = hp300_sched_init;
 	mach_init_IRQ        = hp300_init_IRQ;
 	mach_get_model       = hp300_get_model;
-	arch_gettimeoffset   = hp300_gettimeoffset;
 	mach_hwclk	     = hp300_hwclk;
 	mach_get_ss	     = hp300_get_ss;
 	mach_reset           = hp300_reset;
diff --git a/arch/m68k/hp300/time.h b/arch/m68k/hp300/time.h
index f5583ec4033d..1d77b55cc72a 100644
--- a/arch/m68k/hp300/time.h
+++ b/arch/m68k/hp300/time.h
@@ -1,2 +1 @@
 extern void hp300_sched_init(irq_handler_t vector);
-extern u32 hp300_gettimeoffset(void);
diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 2367c3792dc7..cecaaa100791 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -54,8 +54,6 @@ struct mac_booter_data mac_bi_data;
 /* The phys. video addr. - might be bogus on some machines */
 static unsigned long mac_orig_videoaddr;
 
-/* Mac specific timer functions */
-extern u32 mac_gettimeoffset(void);
 extern int mac_hwclk(int, struct rtc_time *);
 extern void iop_preinit(void);
 extern void iop_init(void);
@@ -150,7 +148,6 @@ void __init config_mac(void)
 	mach_sched_init = via_init_clock;
 	mach_init_IRQ = mac_init_IRQ;
 	mach_get_model = mac_get_model;
-	arch_gettimeoffset = mac_gettimeoffset;
 	mach_hwclk = mac_hwclk;
 	mach_reset = mac_reset;
 	mach_halt = mac_poweroff;
diff --git a/arch/m68k/mvme147/config.c b/arch/m68k/mvme147/config.c
index 8074940b0aa1..ff7141cf27d1 100644
--- a/arch/m68k/mvme147/config.c
+++ b/arch/m68k/mvme147/config.c
@@ -38,7 +38,6 @@
 
 static void mvme147_get_model(char *model);
 extern void mvme147_sched_init(irq_handler_t handler);
-extern u32 mvme147_gettimeoffset(void);
 extern int mvme147_hwclk (int, struct rtc_time *);
 extern void mvme147_reset (void);
 
@@ -89,7 +88,6 @@ void __init config_mvme147(void)
 	mach_max_dma_address	= 0x01000000;
 	mach_sched_init		= mvme147_sched_init;
 	mach_init_IRQ		= mvme147_init_IRQ;
-	arch_gettimeoffset	= mvme147_gettimeoffset;
 	mach_hwclk		= mvme147_hwclk;
 	mach_reset		= mvme147_reset;
 	mach_get_model		= mvme147_get_model;
diff --git a/arch/m68k/mvme16x/config.c b/arch/m68k/mvme16x/config.c
index d4aec717e688..f1ed52331df3 100644
--- a/arch/m68k/mvme16x/config.c
+++ b/arch/m68k/mvme16x/config.c
@@ -44,7 +44,6 @@ static MK48T08ptr_t volatile rtc = (MK48T08ptr_t)MVME_RTC_BASE;
 
 static void mvme16x_get_model(char *model);
 extern void mvme16x_sched_init(irq_handler_t handler);
-extern u32 mvme16x_gettimeoffset(void);
 extern int mvme16x_hwclk (int, struct rtc_time *);
 extern void mvme16x_reset (void);
 
@@ -277,7 +276,6 @@ void __init config_mvme16x(void)
     mach_max_dma_address = 0xffffffff;
     mach_sched_init      = mvme16x_sched_init;
     mach_init_IRQ        = mvme16x_init_IRQ;
-    arch_gettimeoffset   = mvme16x_gettimeoffset;
     mach_hwclk           = mvme16x_hwclk;
     mach_reset		 = mvme16x_reset;
     mach_get_model       = mvme16x_get_model;
-- 
2.18.1

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Geert Uytterhoeven]

Hi Finn,

Thanks for your patch!

On Mon, Nov 12, 2018 at 5:46 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> The functions that implement arch_gettimeoffset are re-used by
> new clocksource drivers in subsequent patches.

Disabling this first affects functionality during bisection, right?

> --- a/arch/m68k/amiga/config.c
> +++ b/arch/m68k/amiga/config.c
> @@ -95,8 +95,6 @@ static char amiga_model_name[13] = "Amiga ";
>  static void amiga_sched_init(irq_handler_t handler);
>  static void amiga_get_model(char *model);
>  static void amiga_get_hardware_list(struct seq_file *m);
> -/* amiga specific timer functions */
> -static u32 amiga_gettimeoffset(void);
>  extern void amiga_mksound(unsigned int count, unsigned int ticks);
>  static void amiga_reset(void);
>  extern void amiga_init_sound(void);
> @@ -386,7 +384,6 @@ void __init config_amiga(void)
>         mach_init_IRQ        = amiga_init_IRQ;
>         mach_get_model       = amiga_get_model;
>         mach_get_hardware_list = amiga_get_hardware_list;
> -       arch_gettimeoffset   = amiga_gettimeoffset;

In addition, won't this lead to "function defined statically but not called'
warnings?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Finn Thain]

On Mon, 12 Nov 2018, Geert Uytterhoeven wrote:

> Hi Finn,
> 
> Thanks for your patch!
> 
> On Mon, Nov 12, 2018 at 5:46 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > The functions that implement arch_gettimeoffset are re-used by
> > new clocksource drivers in subsequent patches.
> 
> Disabling this first affects functionality during bisection, right?
> 

It means that all platforms have to use the 'jiffies' clocksource.

At the end of the series, only apollo, q40, sun3 & sun3x continue to use 
that clocksource.

> > --- a/arch/m68k/amiga/config.c
> > +++ b/arch/m68k/amiga/config.c
> > @@ -95,8 +95,6 @@ static char amiga_model_name[13] = "Amiga ";
> >  static void amiga_sched_init(irq_handler_t handler);
> >  static void amiga_get_model(char *model);
> >  static void amiga_get_hardware_list(struct seq_file *m);
> > -/* amiga specific timer functions */
> > -static u32 amiga_gettimeoffset(void);
> >  extern void amiga_mksound(unsigned int count, unsigned int ticks);
> >  static void amiga_reset(void);
> >  extern void amiga_init_sound(void);
> > @@ -386,7 +384,6 @@ void __init config_amiga(void)
> >         mach_init_IRQ        = amiga_init_IRQ;
> >         mach_get_model       = amiga_get_model;
> >         mach_get_hardware_list = amiga_get_hardware_list;
> > -       arch_gettimeoffset   = amiga_gettimeoffset;
> 
> In addition, won't this lead to "function defined statically but not 
> called' warnings?
> 

I expect so. I haven't compile-tested each step in the series; but I'll do 
that before I send v2. Thanks for the reminder.

There are no new warnings at the end of the series.

-- 

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Michael Schmitz]

Hi Finn,

Am 12.11.2018 um 22:06 schrieb Finn Thain:
> On Mon, 12 Nov 2018, Geert Uytterhoeven wrote:
>
>> Hi Finn,
>>
>> Thanks for your patch!
>>
>> On Mon, Nov 12, 2018 at 5:46 AM Finn Thain <fthain@telegraphics.com.au> wrote:
>>> The functions that implement arch_gettimeoffset are re-used by
>>> new clocksource drivers in subsequent patches.
>>
>> Disabling this first affects functionality during bisection, right?
>>
>
> It means that all platforms have to use the 'jiffies' clocksource.

So all that happens is timer granularity drops to 10ms, then gets 
restored by the later patches?

I doubt that would be a large enough regression to matter for bisection, 
but the way you reuse the arch_gettimeoffset() code for the new 
read_clock() functions makes reordering of this patch to the end of the 
series impossible.

Best you can don, under the circumstances.

Cheers,

	Michael

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Finn Thain]

On Tue, 13 Nov 2018, Michael Schmitz wrote:

> Hi Finn,
> 
> Am 12.11.2018 um 22:06 schrieb Finn Thain:
> > On Mon, 12 Nov 2018, Geert Uytterhoeven wrote:
> > 
> > > Hi Finn,
> > > 
> > > Thanks for your patch!
> > > 
> > > On Mon, Nov 12, 2018 at 5:46 AM Finn Thain <fthain@telegraphics.com.au>
> > > wrote:
> > > > The functions that implement arch_gettimeoffset are re-used by
> > > > new clocksource drivers in subsequent patches.
> > > 
> > > Disabling this first affects functionality during bisection, right?
> > > 
> > 
> > It means that all platforms have to use the 'jiffies' clocksource.
> 
> So all that happens is timer granularity drops to 10ms, then gets restored by
> the later patches?
> 

Yes, that was the plan, but I can't confirm that it worked out as I don't 
have any physical 68k hardware in front of me right now. If you can 
confirm this on your Atari Falcon, that would be great.

(It appears that a QEMU-emulated Mac does not benefit from having a 
clocksource that's more accurate than the 'jiffies' clocksource, in spite 
of "clocksource: Switched to clocksource via1".)

The latest patches can be found at
https://github.com/fthain/linux/commits/mac68k-queue/

-- 

> I doubt that would be a large enough regression to matter for bisection, but
> the way you reuse the arch_gettimeoffset() code for the new read_clock()
> functions makes reordering of this patch to the end of the series impossible.
> 
> Best you can don, under the circumstances.
> 
> Cheers,
> 
> 	Michael
> 
> 

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Michael Schmitz]

Hi Finn,

Am 13.11.2018 um 16:14 schrieb Finn Thain:
> On Tue, 13 Nov 2018, Michael Schmitz wrote:
>
>> Hi Finn,
>>
>> Am 12.11.2018 um 22:06 schrieb Finn Thain:
>>> On Mon, 12 Nov 2018, Geert Uytterhoeven wrote:
>>>
>>>> Hi Finn,
>>>>
>>>> Thanks for your patch!
>>>>
>>>> On Mon, Nov 12, 2018 at 5:46 AM Finn Thain <fthain@telegraphics.com.au>
>>>> wrote:
>>>>> The functions that implement arch_gettimeoffset are re-used by
>>>>> new clocksource drivers in subsequent patches.
>>>>
>>>> Disabling this first affects functionality during bisection, right?
>>>>
>>>
>>> It means that all platforms have to use the 'jiffies' clocksource.
>>
>> So all that happens is timer granularity drops to 10ms, then gets restored by
>> the later patches?
>>
>
> Yes, that was the plan, but I can't confirm that it worked out as I don't
> have any physical 68k hardware in front of me right now. If you can
> confirm this on your Atari Falcon, that would be great.

Will do.

> (It appears that a QEMU-emulated Mac does not benefit from having a
> clocksource that's more accurate than the 'jiffies' clocksource, in spite
> of "clocksource: Switched to clocksource via1".)

With the current code, kernel log timestamps have 10 ms resolution on 
Atari. Time resolution of times reported by initcall_debug is roughly 40 
us. I'd expect that changes with falling back to jiffies only. Might be 
worth a try on QEMU Mac.

Cheers,

	Michael

> The latest patches can be found at
> https://github.com/fthain/linux/commits/mac68k-queue/
>

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Finn Thain]

On Tue, 13 Nov 2018, Michael Schmitz wrote:

> 
> > (It appears that a QEMU-emulated Mac does not benefit from having a 
> > clocksource that's more accurate than the 'jiffies' clocksource, in 
> > spite of "clocksource: Switched to clocksource via1".)
> 
> With the current code, kernel log timestamps have 10 ms resolution on 
> Atari. Time resolution of times reported by initcall_debug is roughly 40 
> us. I'd expect that changes with falling back to jiffies only. Might be 
> worth a try on QEMU Mac.

The initcall debug output shows the same precision as my earlier tests 
with userland. The VIA timer only gets updated when QEMU wants to update 
it, which seems to be about every 9765 us. Hence, using the 'jiffies' 
clocksource would be effectively no regression for a virtual Mac.

-- 

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Michael Schmitz]

Hi Finn,

Am 13.11.2018 um 19:15 schrieb Finn Thain:
> On Tue, 13 Nov 2018, Michael Schmitz wrote:
>
>>
>>> (It appears that a QEMU-emulated Mac does not benefit from having a
>>> clocksource that's more accurate than the 'jiffies' clocksource, in
>>> spite of "clocksource: Switched to clocksource via1".)
>>
>> With the current code, kernel log timestamps have 10 ms resolution on
>> Atari. Time resolution of times reported by initcall_debug is roughly 40
>> us. I'd expect that changes with falling back to jiffies only. Might be
>> worth a try on QEMU Mac.
>
> The initcall debug output shows the same precision as my earlier tests
> with userland. The VIA timer only gets updated when QEMU wants to update
> it, which seems to be about every 9765 us. Hence, using the 'jiffies'
> clocksource would be effectively no regression for a virtual Mac.

Running a recent kernel under ARAnyM shows 40 ns resolution so the Atari 
hardware emulation is a little more complete.

Using that for initial tests, I can confirm that timer resolution is 
reduced to 10ms after patch 6, and gets restored to 40ns after applying 
the full series (once clocksource_init runs, that is - the first part of 
the boot is at 10ms resolution only, a regression compared to with use 
of arch_gettimeoffset).

Unfortunately, I can't log in at the console with all patches applied. I 
immediately get the 'login timeout exceeded' message. Weird...

Cheers,

	Michael

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Finn Thain]

On Tue, 13 Nov 2018, Michael Schmitz wrote:

> 
> Running a recent kernel under ARAnyM shows 40 ns resolution so the Atari 
> hardware emulation is a little more complete.
> 

You mean, 40 us resolution, right?

> Using that for initial tests, I can confirm that timer resolution is 
> reduced to 10ms after patch 6, and gets restored to 40ns after applying 
> the full series

Thanks for testing!

> (once clocksource_init runs, that is - the first part of the boot is at 
> 10ms resolution only, a regression compared to with use of 
> arch_gettimeoffset).
> 

Sounds like a theoretical regression (?)

Is there any need for more precise timers (I mean, better than 40 us) 
before clocksource_init runs?

> Unfortunately, I can't log in at the console with all patches applied. I 
> immediately get the 'login timeout exceeded' message. Weird...
> 

I didn't see that in my tests... Was this aranym or real hardware or both?

Can you also test tree fbf8405cd982 please?

-- 

> Cheers,
> 
> 	Michael
> 
> 

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Michael Schmitz]

Hi Finn,

On 14/11/18 11:11 AM, Finn Thain wrote:
> On Tue, 13 Nov 2018, Michael Schmitz wrote:
>
>> Running a recent kernel under ARAnyM shows 40 ns resolution so the Atari
>> hardware emulation is a little more complete.
>>
> You mean, 40 us resolution, right?

Sorry, typo. Should have been us of course.

>
>> Using that for initial tests, I can confirm that timer resolution is
>> reduced to 10ms after patch 6, and gets restored to 40ns after applying
>> the full series
> Thanks for testing!
>
>> (once clocksource_init runs, that is - the first part of the boot is at
>> 10ms resolution only, a regression compared to with use of
>> arch_gettimeoffset).
>>
> Sounds like a theoretical regression (?)
>
> Is there any need for more precise timers (I mean, better than 40 us)
> before clocksource_init runs?

I don't think so, given that we can run just fine with 10 ms resolution.

>
>> Unfortunately, I can't log in at the console with all patches applied. I
>> immediately get the 'login timeout exceeded' message. Weird...
>>
> I didn't see that in my tests... Was this aranym or real hardware or both?

ARAnyM only so far.

>
> Can you also test tree fbf8405cd982 please?
>
My tests were on c606b5cf902 in case it wasn't clear. I've now seen 
fbf8405cd982, one moment please ...

That one does appear to work - different versions of ARAnyM, and 
different userland versions though. I'll test that again with the setup 
that saw c606b5cf902 fail.

Cheers,

     Michael

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Michael Schmitz]

Hi Finn,

Am 14.11.2018 um 14:08 schrieb Michael Schmitz:
>> Can you also test tree fbf8405cd982 please?
>>
> My tests were on c606b5cf902 in case it wasn't clear. I've now seen
> fbf8405cd982, one moment please ...
>
> That one does appear to work - different versions of ARAnyM, and
> different userland versions though. I'll test that again with the setup
> that saw c606b5cf902 fail.

Still fails on that emulator / userland.

Cheers,

	Michael


>
> Cheers,
>
>     Michael
>
>
>

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Michael Schmitz]

Hi Finn,

On 14/11/18 3:58 PM, Michael Schmitz wrote:
> Hi Finn,
>
> Am 14.11.2018 um 14:08 schrieb Michael Schmitz:
>>> Can you also test tree fbf8405cd982 please?
>>>
>> My tests were on c606b5cf902 in case it wasn't clear. I've now seen
>> fbf8405cd982, one moment please ...
>>
>> That one does appear to work - different versions of ARAnyM, and
>> different userland versions though. I'll test that again with the setup
>> that saw c606b5cf902 fail.
>
> Still fails on that emulator / userland.
>
Must be a quirk of ARAnyM 1.0.2 (or powerpc). With 0.9.15 on x86_64, 
it's fine.

I'm sufficiently convinced to try this on actual hardware now.

Cheers,

     Michael

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Finn Thain]

On Thu, 15 Nov 2018, Michael Schmitz wrote:

> Hi Finn,
> 
> On 14/11/18 3:58 PM, Michael Schmitz wrote:
> > Hi Finn,
> > 
> > Am 14.11.2018 um 14:08 schrieb Michael Schmitz:
> > > > Can you also test tree fbf8405cd982 please?
> > > > 
> > > My tests were on c606b5cf902 in case it wasn't clear. I've now seen
> > > fbf8405cd982, one moment please ...
> > > 
> > > That one does appear to work - different versions of ARAnyM, and
> > > different userland versions though. I'll test that again with the setup
> > > that saw c606b5cf902 fail.
> > 
> > Still fails on that emulator / userland.
> > 
> Must be a quirk of ARAnyM 1.0.2 (or powerpc). With 0.9.15 on x86_64, it's
> fine.
> 

Could be a regression in aranym?

Maybe it's worth trying 0.9.15 on the powerpc host?

> I'm sufficiently convinced to try this on actual hardware now.
> 

Thanks!

-- 

> Cheers,
> 
> ??? Michael
> 
> 
> 

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Michael Schmitz]

Hi Finn

Am 15.11.2018 um 12:54 schrieb Michael Schmitz:
>>> That one does appear to work - different versions of ARAnyM, and
>>> different userland versions though. I'll test that again with the setup
>>> that saw c606b5cf902 fail.
>>
>> Still fails on that emulator / userland.
>>
> Must be a quirk of ARAnyM 1.0.2 (or powerpc). With 0.9.15 on x86_64,
> it's fine.
>
> I'm sufficiently convinced to try this on actual hardware now.

Well, it sort of works - I've seen one login timeout on the console 
under load (less than 10 seconds after typing in the password), but most 
attempts went OK. Couldn't log in through SSH without increasing fatal: 
Timeout before authenticationthe LoginGraceTime parameter though.

I usually get reliable login using ssh key files with the default 
setting of 120 seconds (takes around 90 to 100 seconds to complete). 
With your patch, even increasing this to 4800 doesn't result in reliable 
login.

The error I see in the logs is 'fatal: Timeout before authentication'.

Cheers,

	Michael

Re: [RFC PATCH 06/13] m68k: Drop ARCH_USES_GETTIMEOFFSET

[Finn Thain]

On Thu, 15 Nov 2018, Michael Schmitz wrote:

> 
> Well, it sort of works - I've seen one login timeout on the console 
> under load (less than 10 seconds after typing in the password), but most 
> attempts went OK. Couldn't log in through SSH without increasing fatal: 
> Timeout before authenticationthe LoginGraceTime parameter though.
> 
> I usually get reliable login using ssh key files with the default 
> setting of 120 seconds (takes around 90 to 100 seconds to complete). 
> With your patch, even increasing this to 4800 doesn't result in reliable 
> login.
> 
> The error I see in the logs is 'fatal: Timeout before authentication'.
> 

Weird. Let's try 2942e9fd3e57. Please send me your .config so I can send 
you a kernel binary. (I don't trust gcc 4.6 on m68k...)

-- 

> Cheers,
> 
> 	Michael
> 
> 

[RFC PATCH 13/13] m68k: mvme16x: Convert to clocksource API

[Finn Thain]

Add a platform clocksource by adapting the existing arch_gettimeoffset
implementation.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/mvme16x/config.c | 45 +++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/arch/m68k/mvme16x/config.c b/arch/m68k/mvme16x/config.c
index f1ed52331df3..7bd3a25b2d75 100644
--- a/arch/m68k/mvme16x/config.c
+++ b/arch/m68k/mvme16x/config.c
@@ -19,6 +19,7 @@
 #include <linux/mm.h>
 #include <linux/seq_file.h>
 #include <linux/tty.h>
+#include <linux/clocksource.h>
 #include <linux/console.h>
 #include <linux/linkage.h>
 #include <linux/init.h>
@@ -49,12 +50,6 @@ extern void mvme16x_reset (void);
 
 int bcd2int (unsigned char b);
 
-/* Save tick handler routine pointer, will point to xtime_update() in
- * kernel/time/timekeeping.c, called via mvme16x_process_int() */
-
-static irq_handler_t tick_handler;
-
-
 unsigned short mvme16x_config;
 EXPORT_SYMBOL(mvme16x_config);
 
@@ -348,9 +343,31 @@ static irqreturn_t mvme16x_abort_int (int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static u64 mvme16x_read_clk(struct clocksource *cs);
+
+static struct clocksource mvme16x_clk = {
+	.name   = "pcc",
+	.rating = 250,
+	.read   = mvme16x_read_clk,
+	.mask   = CLOCKSOURCE_MASK(32),
+	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+static u32 clk_total;
+
+#define PCC_TIMER_CLOCK_FREQ 1000000
+#define PCC_TIMER_CYCLES     (PCC_TIMER_CLOCK_FREQ / HZ)
+
 static irqreturn_t mvme16x_timer_int (int irq, void *dev_id)
 {
+    irq_handler_t tick_handler = dev_id;
+    unsigned long flags;
+
+    local_irq_save(flags);
     *(volatile unsigned char *)0xfff4201b |= 8;
+    clk_total += PCC_TIMER_CYCLES;
+    local_irq_restore(flags);
+
     return tick_handler(irq, dev_id);
 }
 
@@ -359,16 +376,17 @@ void mvme16x_sched_init (irq_handler_t timer_routine)
     uint16_t brdno = be16_to_cpu(mvme_bdid.brdno);
     int irq;
 
-    tick_handler = timer_routine;
     /* Using PCCchip2 or MC2 chip tick timer 1 */
     *(volatile unsigned long *)0xfff42008 = 0;
-    *(volatile unsigned long *)0xfff42004 = 10000;	/* 10ms */
+    *(volatile unsigned long *)0xfff42004 = PCC_TIMER_CYCLES;
     *(volatile unsigned char *)0xfff42017 |= 3;
     *(volatile unsigned char *)0xfff4201b = 0x16;
-    if (request_irq(MVME16x_IRQ_TIMER, mvme16x_timer_int, 0,
-				"timer", mvme16x_timer_int))
+    if (request_irq(MVME16x_IRQ_TIMER, mvme16x_timer_int, IRQF_TIMER,
+				"timer", timer_routine))
 	panic ("Couldn't register timer int");
 
+    clocksource_register_hz(&mvme16x_clk, PCC_TIMER_CLOCK_FREQ);
+
     if (brdno == 0x0162 || brdno == 0x172)
 	irq = MVME162_IRQ_ABORT;
     else
@@ -378,10 +396,11 @@ void mvme16x_sched_init (irq_handler_t timer_routine)
 	panic ("Couldn't register abort int");
 }
 
-
-u32 mvme16x_gettimeoffset(void)
+static u64 mvme16x_read_clk(struct clocksource *cs)
 {
-    return (*(volatile u32 *)0xfff42008) * 1000;
+    u32 count = *(volatile u32 *)0xfff42008;
+
+    return clk_total + count;
 }
 
 int bcd2int (unsigned char b)
-- 
2.18.1

Re: [RFC PATCH 13/13] m68k: mvme16x: Convert to clocksource API

[Thomas Gleixner]

Finn,

On Mon, 12 Nov 2018, Finn Thain wrote:

First of all, thanks for tackling this!

> +static u32 clk_total;
> +
> +#define PCC_TIMER_CLOCK_FREQ 1000000
> +#define PCC_TIMER_CYCLES     (PCC_TIMER_CLOCK_FREQ / HZ)
> +
>  static irqreturn_t mvme16x_timer_int (int irq, void *dev_id)
>  {
> +    irq_handler_t tick_handler = dev_id;
> +    unsigned long flags;
> +
> +    local_irq_save(flags);

No need for local_irq_save() here. Interrupt handlers are guaranteed to be
called with interrupts disabled.

>      *(volatile unsigned char *)0xfff4201b |= 8;
> +    clk_total += PCC_TIMER_CYCLES;

Looking at the read function below, I assume that this magic 0xfff42008
register counts up to PCC_TIMER_CYCLES, which triggers the timer interrupt
and then starts from 0 again. And looking at the programming manual
actually confirms that assumption (COC is set in the control reg).

> -u32 mvme16x_gettimeoffset(void)
> +static u64 mvme16x_read_clk(struct clocksource *cs)
>  {
> -    return (*(volatile u32 *)0xfff42008) * 1000;
> +    u32 count = *(volatile u32 *)0xfff42008;
> +
> +    return clk_total + count;
>  }

There is a problem with that approach. Assume the following situation:

				Counter value (HZ = 100)
       local_irq_disable()	
       ...
       ktime_get()		9999
       ....			10000 -> 0 (interrupt is triggered)
       ktime_get()		1

IOW, time goes backwards.

There are two ways to solve that:

 1) Take the overflow bits in the timer control register into account,
    which makes time readout even slower because you need to do it like
    this:

    do {
       ovfl = read(TCR1);
       now = read(TCNT1);
    while (ovfl != read(TCR1));
    ....
  

 2) Use Timer2 in freerunning mode which means it will use the full 32bit
    and then wrap back to 0. That's fine and the clocksource core handles
    that.

    That removes the clk_total thing and just works.

Thanks,

	tglx

Re: [RFC PATCH 13/13] m68k: mvme16x: Convert to clocksource API

[Finn Thain]

On Mon, 12 Nov 2018, Thomas Gleixner wrote:

> Finn,
> 
> First of all, thanks for tackling this!
> 

Happy to help. Thanks for your review.

> > +static u32 clk_total;
> > +
> > +#define PCC_TIMER_CLOCK_FREQ 1000000
> > +#define PCC_TIMER_CYCLES     (PCC_TIMER_CLOCK_FREQ / HZ)
> > +
> >  static irqreturn_t mvme16x_timer_int (int irq, void *dev_id)
> >  {
> > +    irq_handler_t tick_handler = dev_id;
> > +    unsigned long flags;
> > +
> > +    local_irq_save(flags);
> 
> No need for local_irq_save() here. Interrupt handlers are guaranteed to be
> called with interrupts disabled.
> 

That's not the case on m68k, as I understand it. However, the CPU 
interrupt level does prevent interrupt handlers from nesting.

> >      *(volatile unsigned char *)0xfff4201b |= 8;
> > +    clk_total += PCC_TIMER_CYCLES;
> 
> Looking at the read function below, I assume that this magic 0xfff42008 
> register counts up to PCC_TIMER_CYCLES, which triggers the timer 
> interrupt and then starts from 0 again. And looking at the programming 
> manual actually confirms that assumption (COC is set in the control 
> reg).
> 
> > -u32 mvme16x_gettimeoffset(void)
> > +static u64 mvme16x_read_clk(struct clocksource *cs)
> >  {
> > -    return (*(volatile u32 *)0xfff42008) * 1000;
> > +    u32 count = *(volatile u32 *)0xfff42008;
> > +
> > +    return clk_total + count;
> >  }
> 
> There is a problem with that approach. Assume the following situation:
> 
> 				Counter value (HZ = 100)
>        local_irq_disable()	
>        ...
>        ktime_get()		9999
>        ....			10000 -> 0 (interrupt is triggered)
>        ktime_get()		1
> 
> IOW, time goes backwards.
> 

Yes. It's not a new problem. I think hp300 and mvme147 have similar 
issues.

If the clocksource core is badly affected by this problem then I'll have 
to address it.

> There are two ways to solve that:
> 
>  1) Take the overflow bits in the timer control register into account,
>     which makes time readout even slower because you need to do it like
>     this:
> 
>     do {
>        ovfl = read(TCR1);
>        now = read(TCNT1);
>     while (ovfl != read(TCR1));
>     ....
>   

How about,

	local_irq_save(flags);
	ovfl = read(TCR1);
	now = read(TCNT1);
	if (ovfl != read(TCR1))
		now = read(TCNT1);

	ticks = clk_total + now;
	offset = (ovfl >> 4) * PCC_TIMER_CYCLES;
	local_irq_restore(flags);

	return offset + ticks;

This has to be synchronized with the interrupt handler because the 
interrupt handler must clear the overflow counter in TCR1 when it does 
clk_total += PCC_TIMER_CYCLES;

BTW, there are some synchronization bugs in this patch series that will be 
addressed in the next submission. They have been fixed in my github repo.

> 
>  2) Use Timer2 in freerunning mode which means it will use the full 32bit
>     and then wrap back to 0. That's fine and the clocksource core handles
>     that.
> 
>     That removes the clk_total thing and just works.
> 

I really like this suggestion!

But I fear it is a change that cannot be checked by inspection. If I wrote 
it, I'd want to test it, or have someone else test it. (Stephen?)

I wonder if there exists an emulator for MVME 162/166/167/172/177 machines 
capable of booting Linux...

-- 

> Thanks,
> 
> 	tglx
> 

Re: [RFC PATCH 13/13] m68k: mvme16x: Convert to clocksource API

[Finn Thain]

On Tue, 13 Nov 2018, I wrote:

> On Mon, 12 Nov 2018, Thomas Gleixner wrote:
> 
> > > +static u32 clk_total;
> > > +
> > > +#define PCC_TIMER_CLOCK_FREQ 1000000
> > > +#define PCC_TIMER_CYCLES     (PCC_TIMER_CLOCK_FREQ / HZ)
> > > +
> > >  static irqreturn_t mvme16x_timer_int (int irq, void *dev_id)
> > >  {
> > > +    irq_handler_t tick_handler = dev_id;
> > > +    unsigned long flags;
> > > +
> > > +    local_irq_save(flags);
> > 
> > No need for local_irq_save() here. Interrupt handlers are guaranteed to be
> > called with interrupts disabled.
> > 
> 
> That's not the case on m68k, as I understand it. However, the CPU 
> interrupt level does prevent interrupt handlers from nesting.
> 

What I mean by that is, the interrupt level (IPL) prevents interrupt 
handlers from being re-entered. But a handler can still get interrupted by 
a higher priority interrupt request. In the past I've had to add defensive 
locking because of this.

In these patches I've assumed it was possible for some higher priority 
interrupt handler to perform a clocksource read after the timer handler 
started executing. Hence the use of local_irq_save/restore.

To be sure, I've just run a quick test and confirmed that the timer 
handler can indeed get interrupted by the ethernet interrupt handler.

-- 

Re: [RFC PATCH 13/13] m68k: mvme16x: Convert to clocksource APIy

[Thomas Gleixner]

On Wed, 14 Nov 2018, Finn Thain wrote:
> On Tue, 13 Nov 2018, I wrote:
> 
> > On Mon, 12 Nov 2018, Thomas Gleixner wrote:
> > 
> > > > +static u32 clk_total;
> > > > +
> > > > +#define PCC_TIMER_CLOCK_FREQ 1000000
> > > > +#define PCC_TIMER_CYCLES     (PCC_TIMER_CLOCK_FREQ / HZ)
> > > > +
> > > >  static irqreturn_t mvme16x_timer_int (int irq, void *dev_id)
> > > >  {
> > > > +    irq_handler_t tick_handler = dev_id;
> > > > +    unsigned long flags;
> > > > +
> > > > +    local_irq_save(flags);
> > > 
> > > No need for local_irq_save() here. Interrupt handlers are guaranteed to be
> > > called with interrupts disabled.
> > > 
> > 
> > That's not the case on m68k, as I understand it. However, the CPU 
> > interrupt level does prevent interrupt handlers from nesting.
> > 
> 
> What I mean by that is, the interrupt level (IPL) prevents interrupt 
> handlers from being re-entered. But a handler can still get interrupted by 
> a higher priority interrupt request. In the past I've had to add defensive 
> locking because of this.
> 
> In these patches I've assumed it was possible for some higher priority 
> interrupt handler to perform a clocksource read after the timer handler 
> started executing. Hence the use of local_irq_save/restore.
> 
> To be sure, I've just run a quick test and confirmed that the timer 
> handler can indeed get interrupted by the ethernet interrupt handler.

Urgh. Then you have more serious trouble. If the interrupting handler
calls any of the time accessor functions then you can actually live lock
when the interrupt happens in the middle of the write locked section of the
core timekeeping update. So you really want to disable interrupts across
the whole timer interrupt function or make sure that the timer interrupt is
the highest priority one on the system.

Thanks,

	tglx

Re: [RFC PATCH 13/13] m68k: mvme16x: Convert to clocksource APIy

[Finn Thain]

On Tue, 13 Nov 2018, Thomas Gleixner wrote:

> On Wed, 14 Nov 2018, Finn Thain wrote:
> > 
> > What I mean by that is, the interrupt level (IPL) prevents interrupt 
> > handlers from being re-entered. But a handler can still get 
> > interrupted by a higher priority interrupt request. In the past I've 
> > had to add defensive locking because of this.
> > 
> > In these patches I've assumed it was possible for some higher priority 
> > interrupt handler to perform a clocksource read after the timer 
> > handler started executing. Hence the use of local_irq_save/restore.
> > 
> > To be sure, I've just run a quick test and confirmed that the timer 
> > handler can indeed get interrupted by the ethernet interrupt handler.
> 
> Urgh. Then you have more serious trouble. If the interrupting handler 
> calls any of the time accessor functions then you can actually live lock 
> when the interrupt happens in the middle of the write locked section of 
> the core timekeeping update.

Does this apply to arch_gettimeoffset also? If so, that would mean another 
bug fix patch for -stable...

> So you really want to disable interrupts across the whole timer 
> interrupt function or make sure that the timer interrupt is the highest 
> priority one on the system.
> 

The timer interrupt priority isn't always what one would hope. 

But I can certainly disable interrupts for timer_interrupt() execution 
(that is, xtime_update() etc.)

-- 

> Thanks,
> 
> 	tglx
> 

Re: [RFC PATCH 13/13] m68k: mvme16x: Convert to clocksource APIy

[Thomas Gleixner]

Finn,

On Wed, 14 Nov 2018, Finn Thain wrote:
> On Tue, 13 Nov 2018, Thomas Gleixner wrote:
> > Urgh. Then you have more serious trouble. If the interrupting handler 
> > calls any of the time accessor functions then you can actually live lock 
> > when the interrupt happens in the middle of the write locked section of 
> > the core timekeeping update.
> 
> Does this apply to arch_gettimeoffset also? If so, that would mean another 
> bug fix patch for -stable...

arch_gettimeoffset() can be called from any context. The only interrupt
which can affect it is the timer interrupt. As m68k are all UP machines
this is not a problem because:

  ktime_get()
     do {
        seq = read_seqcount_begin(tk.seq);
	nsec = .... + arch_gettimeoffset();
     } while (seqcount_retry(tk.seq);

and the timer interrupt does:

  arch_timeoffset += value;
  ...
    xtime_update()
      ...
        write_seqcount_begin(tk.seq);
        ....
        write_seqcount_end(tk.seq);

So where ever the timer interrupt hits inside of ktime_get() or any other
accessor the seqcount will make it retry, so the result is always
consistent.

For SMP that would be a different story.

> > So you really want to disable interrupts across the whole timer 
> > interrupt function or make sure that the timer interrupt is the highest 
> > priority one on the system.
> > 
> 
> The timer interrupt priority isn't always what one would hope. 
> 
> But I can certainly disable interrupts for timer_interrupt() execution 
> (that is, xtime_update() etc.)

Oh yes, you should.

Thanks,

	tglx

[RFC PATCH 03/13] m68k: mac: Fix VIA timer counter accesses

[Finn Thain]

This resolves some bugs that affect VIA timer counter accesses.
Firstly, keep track of lost interrupts caused by reading the counter.
Secondly, check for inconsistent results when reading the counter.
Finally, make allowance for the fact that the counter will be
decremented to 0xFFFF before being reloaded.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Joshua Thompson <funaho@jurai.org>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/mac/config.c |   7 +--
 arch/m68k/mac/via.c    | 135 ++++++++++++++++++++++++-----------------
 2 files changed, 82 insertions(+), 60 deletions(-)

diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index cd9317d53276..2367c3792dc7 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -72,11 +72,6 @@ static void mac_get_model(char *str);
 static void mac_identify(void);
 static void mac_report_hardware(void);
 
-static void __init mac_sched_init(irq_handler_t vector)
-{
-	via_init_clock(vector);
-}
-
 /*
  * Parse a Macintosh-specific record in the bootinfo
  */
@@ -152,7 +147,7 @@ void __init config_mac(void)
 	if (!MACH_IS_MAC)
 		pr_err("ERROR: no Mac, but config_mac() called!!\n");
 
-	mach_sched_init = mac_sched_init;
+	mach_sched_init = via_init_clock;
 	mach_init_IRQ = mac_init_IRQ;
 	mach_get_model = mac_get_model;
 	arch_gettimeoffset = mac_gettimeoffset;
diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
index e5dff74f59b3..7130abec0b77 100644
--- a/arch/m68k/mac/via.c
+++ b/arch/m68k/mac/via.c
@@ -54,16 +54,6 @@ static __u8 rbv_clear;
 
 static int gIER,gIFR,gBufA,gBufB;
 
-/*
- * Timer defs.
- */
-
-#define TICK_SIZE		10000
-#define MAC_CLOCK_TICK		(783300/HZ)		/* ticks per HZ */
-#define MAC_CLOCK_LOW		(MAC_CLOCK_TICK&0xFF)
-#define MAC_CLOCK_HIGH		(MAC_CLOCK_TICK>>8)
-
-
 /*
  * On Macs with a genuine VIA chip there is no way to mask an individual slot
  * interrupt. This limitation also seems to apply to VIA clone logic cores in
@@ -267,22 +257,6 @@ void __init via_init(void)
 	}
 }
 
-/*
- * Start the 100 Hz clock
- */
-
-void __init via_init_clock(irq_handler_t func)
-{
-	via1[vACR] |= 0x40;
-	via1[vT1LL] = MAC_CLOCK_LOW;
-	via1[vT1LH] = MAC_CLOCK_HIGH;
-	via1[vT1CL] = MAC_CLOCK_LOW;
-	via1[vT1CH] = MAC_CLOCK_HIGH;
-
-	if (request_irq(IRQ_MAC_TIMER_1, func, 0, "timer", func))
-		pr_err("Couldn't register %s interrupt\n", "timer");
-}
-
 /*
  * Debugging dump, used in various places to see what's going on.
  */
@@ -310,34 +284,6 @@ void via_debug_dump(void)
 	}
 }
 
-/*
- * This is always executed with interrupts disabled.
- *
- * TBI: get time offset between scheduling timer ticks
- */
-
-u32 mac_gettimeoffset(void)
-{
-	unsigned long flags;
-	unsigned long ticks, offset = 0;
-
-	local_irq_save(flags);
-
-	/* read VIA1 timer 2 current value */
-	ticks = via1[vT1CL] | (via1[vT1CH] << 8);
-	/* The probability of underflow is less than 2% */
-	if (ticks > MAC_CLOCK_TICK - MAC_CLOCK_TICK / 50)
-		/* Check for pending timer interrupt in VIA1 IFR */
-		if (via1[vIFR] & 0x40) offset = TICK_SIZE;
-
-	local_irq_restore(flags);
-
-	ticks = MAC_CLOCK_TICK - ticks;
-	ticks = ticks * 10000L / MAC_CLOCK_TICK;
-
-	return (ticks + offset) * 1000;
-}
-
 /*
  * Flush the L2 cache on Macs that have it by flipping
  * the system into 24-bit mode for an instant.
@@ -606,3 +552,84 @@ int via2_scsi_drq_pending(void)
 	return via2[gIFR] & (1 << IRQ_IDX(IRQ_MAC_SCSIDRQ));
 }
 EXPORT_SYMBOL(via2_scsi_drq_pending);
+
+/* timer and clock source */
+
+#define VIA_CLOCK_FREQ     783360                /* VIA "phase 2" clock in Hz */
+#define VIA_TIMER_INTERVAL (1000000 / HZ)        /* microseconds per jiffy */
+#define VIA_TIMER_CYCLES   (VIA_CLOCK_FREQ / HZ) /* clock cycles per jiffy */
+
+#define VIA_TC             (VIA_TIMER_CYCLES - 2) /* including 0 and -1 */
+#define VIA_TC_LOW         (VIA_TC & 0xFF)
+#define VIA_TC_HIGH        (VIA_TC >> 8)
+
+static bool lost_timer_intr;
+
+static irqreturn_t via_timer_handler(int irq, void *data)
+{
+	irq_handler_t func = data;
+	unsigned long flags;
+
+	local_irq_save(flags);
+	if (lost_timer_intr) {
+		lost_timer_intr = false;
+		local_irq_restore(flags);
+		func(irq, data);
+	} else
+		local_irq_restore(flags);
+	return func(irq, data);
+}
+
+void __init via_init_clock(irq_handler_t func)
+{
+	if (request_irq(IRQ_MAC_TIMER_1, via_timer_handler, 0, "timer", func)) {
+		pr_err("Couldn't register %s interrupt\n", "timer");
+		return;
+	}
+
+	via1[vT1LL] = VIA_TC_LOW;
+	via1[vT1LH] = VIA_TC_HIGH;
+	via1[vT1CL] = VIA_TC_LOW;
+	via1[vT1CH] = VIA_TC_HIGH;
+	via1[vACR] |= 0x40;
+}
+
+#define VIA_T1_INT_FLAG BIT(6)
+
+u32 mac_gettimeoffset(void)
+{
+	unsigned long flags;
+	u8 count_high, count_high_new, count_low;
+	u16 count, offset = 0;
+
+	/*
+	 * Timer counter wrap-around is detected with the timer interrupt flag
+	 * but the flag gets reset when the counter low byte (vT1CL) is read.
+	 * Also, accessing both counter registers is an unavoidable data race.
+	 * To avoid these problems we could ignore the LSB. Clock accuracy
+	 * would get 256 times worse (error could reach 0.327 ms) but CPU
+	 * overhead would decrease (because VIA register accesses are so slow).
+	 */
+
+	local_irq_save(flags);
+
+	count_high = via1[vT1CH];
+again:
+	if (count_high == 0xFF && (via1[vIFR] & VIA_T1_INT_FLAG))
+		lost_timer_intr = true;
+	count_low = via1[vT1CL];
+	count_high_new = via1[vT1CH];
+	if (count_high_new != count_high) {
+		count_high = count_high_new;
+		goto again;
+	}
+	if (lost_timer_intr)
+		offset = VIA_TIMER_CYCLES;
+
+	local_irq_restore(flags);
+
+	count = ((count_high << 8) | count_low) + 1;
+	count = VIA_TIMER_CYCLES - count + offset;
+
+	return ((count * VIA_TIMER_INTERVAL) / VIA_TIMER_CYCLES) * 1000;
+}
-- 
2.18.1

[RFC PATCH 12/13] m68k: mvme147: Convert to clocksource API

[Finn Thain]

Add a platform clocksource by adapting the existing arch_gettimeoffset
implementation.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/include/asm/mvme147hw.h |  1 -
 arch/m68k/mvme147/config.c        | 43 +++++++++++++++++++++++--------
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/arch/m68k/include/asm/mvme147hw.h b/arch/m68k/include/asm/mvme147hw.h
index 9c7ff67c5ffd..7c3dd513128e 100644
--- a/arch/m68k/include/asm/mvme147hw.h
+++ b/arch/m68k/include/asm/mvme147hw.h
@@ -66,7 +66,6 @@ struct pcc_regs {
 #define PCC_INT_ENAB		0x08
 
 #define PCC_TIMER_INT_CLR	0x80
-#define PCC_TIMER_PRELOAD	63936l
 
 #define PCC_LEVEL_ABORT		0x07
 #define PCC_LEVEL_SERIAL	0x04
diff --git a/arch/m68k/mvme147/config.c b/arch/m68k/mvme147/config.c
index ff7141cf27d1..e7bd40e510d9 100644
--- a/arch/m68k/mvme147/config.c
+++ b/arch/m68k/mvme147/config.c
@@ -17,6 +17,7 @@
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/tty.h>
+#include <linux/clocksource.h>
 #include <linux/console.h>
 #include <linux/linkage.h>
 #include <linux/init.h>
@@ -44,12 +45,6 @@ extern void mvme147_reset (void);
 
 static int bcd2int (unsigned char b);
 
-/* Save tick handler routine pointer, will point to xtime_update() in
- * kernel/time/timekeeping.c, called via mvme147_process_int() */
-
-irq_handler_t tick_handler;
-
-
 int __init mvme147_parse_bootinfo(const struct bi_record *bi)
 {
 	uint16_t tag = be16_to_cpu(bi->tag);
@@ -97,34 +92,59 @@ void __init config_mvme147(void)
 		vme_brdtype = VME_TYPE_MVME147;
 }
 
+static u64 mvme147_read_clk(struct clocksource *cs);
+
+static struct clocksource mvme147_clk = {
+	.name   = "pcc",
+	.rating = 250,
+	.read   = mvme147_read_clk,
+	.mask   = CLOCKSOURCE_MASK(32),
+	.flags  = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+static u32 clk_total;
+
+#define PCC_TIMER_CLOCK_FREQ 160000
+#define PCC_TIMER_CYCLES     (PCC_TIMER_CLOCK_FREQ / HZ)
+#define PCC_TIMER_PRELOAD    (0x10000 - PCC_TIMER_CYCLES)
 
 /* Using pcc tick timer 1 */
 
 static irqreturn_t mvme147_timer_int (int irq, void *dev_id)
 {
+	irq_handler_t tick_handler = dev_id;
+	unsigned long flags;
+
 	m147_pcc->t1_int_cntrl = PCC_TIMER_INT_CLR;
 	m147_pcc->t1_int_cntrl = PCC_INT_ENAB|PCC_LEVEL_TIMER1;
+
+	local_irq_save(flags);
+	clk_total += PCC_TIMER_CYCLES;
+	local_irq_restore(flags);
+
 	return tick_handler(irq, dev_id);
 }
 
 
 void mvme147_sched_init (irq_handler_t timer_routine)
 {
-	tick_handler = timer_routine;
-	if (request_irq(PCC_IRQ_TIMER1, mvme147_timer_int, 0, "timer 1", NULL))
+	if (request_irq(PCC_IRQ_TIMER1, mvme147_timer_int, IRQF_TIMER,
+			"timer 1", timer_routine))
 		pr_err("Couldn't register timer interrupt\n");
 
 	/* Init the clock with a value */
-	/* our clock goes off every 6.25us */
+	/* The clock counter increments until 0xFFFF then reloads */
 	m147_pcc->t1_preload = PCC_TIMER_PRELOAD;
 	m147_pcc->t1_cntrl = 0x0;	/* clear timer */
 	m147_pcc->t1_cntrl = 0x3;	/* start timer */
 	m147_pcc->t1_int_cntrl = PCC_TIMER_INT_CLR;  /* clear pending ints */
 	m147_pcc->t1_int_cntrl = PCC_INT_ENAB|PCC_LEVEL_TIMER1;
+
+	clocksource_register_hz(&mvme147_clk, PCC_TIMER_CLOCK_FREQ);
 }
 
 /* XXX There are race hazards in this code XXX */
-u32 mvme147_gettimeoffset(void)
+static u64 mvme147_read_clk(struct clocksource *cs)
 {
 	unsigned long flags;
 	volatile unsigned short *cp = (volatile unsigned short *)0xfffe1012;
@@ -139,7 +159,8 @@ u32 mvme147_gettimeoffset(void)
 	local_irq_restore(flags);
 
 	n -= PCC_TIMER_PRELOAD;
-	return ((unsigned long)n * 25 / 4) * 1000;
+
+	return clk_total + n;
 }
 
 static int bcd2int (unsigned char b)
-- 
2.18.1

[RFC PATCH 05/13] m68k: apollo, q40, sun3, sun3x: Remove arch_gettimeoffset implementations

[Finn Thain]

These dummy implementations are no better than
default_arch_gettimeoffset() so remove them.

Cc: Sam Creasey <sammy@sammy.net>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/apollo/config.c | 7 -------
 arch/m68k/q40/config.c    | 9 ---------
 arch/m68k/sun3/config.c   | 2 --
 arch/m68k/sun3/intersil.c | 7 -------
 arch/m68k/sun3x/config.c  | 1 -
 arch/m68k/sun3x/time.c    | 5 -----
 arch/m68k/sun3x/time.h    | 1 -
 7 files changed, 32 deletions(-)

diff --git a/arch/m68k/apollo/config.c b/arch/m68k/apollo/config.c
index aef8d42e078d..7d168e6dfb01 100644
--- a/arch/m68k/apollo/config.c
+++ b/arch/m68k/apollo/config.c
@@ -29,7 +29,6 @@ u_long apollo_model;
 
 extern void dn_sched_init(irq_handler_t handler);
 extern void dn_init_IRQ(void);
-extern u32 dn_gettimeoffset(void);
 extern int dn_dummy_hwclk(int, struct rtc_time *);
 extern void dn_dummy_reset(void);
 #ifdef CONFIG_HEARTBEAT
@@ -152,7 +151,6 @@ void __init config_apollo(void)
 
 	mach_sched_init=dn_sched_init; /* */
 	mach_init_IRQ=dn_init_IRQ;
-	arch_gettimeoffset   = dn_gettimeoffset;
 	mach_max_dma_address = 0xffffffff;
 	mach_hwclk           = dn_dummy_hwclk; /* */
 	mach_reset	     = dn_dummy_reset;  /* */
@@ -205,11 +203,6 @@ void dn_sched_init(irq_handler_t timer_routine)
 		pr_err("Couldn't register timer interrupt\n");
 }
 
-u32 dn_gettimeoffset(void)
-{
-	return 0xdeadbeef;
-}
-
 int dn_dummy_hwclk(int op, struct rtc_time *t) {
 
 
diff --git a/arch/m68k/q40/config.c b/arch/m68k/q40/config.c
index 96810d91da2b..e63eb5f06999 100644
--- a/arch/m68k/q40/config.c
+++ b/arch/m68k/q40/config.c
@@ -40,7 +40,6 @@ extern void q40_init_IRQ(void);
 static void q40_get_model(char *model);
 extern void q40_sched_init(irq_handler_t handler);
 
-static u32 q40_gettimeoffset(void);
 static int q40_hwclk(int, struct rtc_time *);
 static unsigned int q40_get_ss(void);
 static int q40_get_rtc_pll(struct rtc_pll_info *pll);
@@ -169,7 +168,6 @@ void __init config_q40(void)
 	mach_sched_init = q40_sched_init;
 
 	mach_init_IRQ = q40_init_IRQ;
-	arch_gettimeoffset = q40_gettimeoffset;
 	mach_hwclk = q40_hwclk;
 	mach_get_ss = q40_get_ss;
 	mach_get_rtc_pll = q40_get_rtc_pll;
@@ -201,13 +199,6 @@ int __init q40_parse_bootinfo(const struct bi_record *rec)
 	return 1;
 }
 
-
-static u32 q40_gettimeoffset(void)
-{
-	return 5000 * (ql_ticks != 0) * 1000;
-}
-
-
 /*
  * Looks like op is non-zero for setting the clock, and zero for
  * reading the clock.
diff --git a/arch/m68k/sun3/config.c b/arch/m68k/sun3/config.c
index 79a2bb857906..867e68d92c71 100644
--- a/arch/m68k/sun3/config.c
+++ b/arch/m68k/sun3/config.c
@@ -37,7 +37,6 @@
 
 char sun3_reserved_pmeg[SUN3_PMEGS_NUM];
 
-extern u32 sun3_gettimeoffset(void);
 static void sun3_sched_init(irq_handler_t handler);
 extern void sun3_get_model (char* model);
 extern int sun3_hwclk(int set, struct rtc_time *t);
@@ -138,7 +137,6 @@ void __init config_sun3(void)
         mach_sched_init      =  sun3_sched_init;
         mach_init_IRQ        =  sun3_init_IRQ;
         mach_reset           =  sun3_reboot;
-	arch_gettimeoffset   =  sun3_gettimeoffset;
 	mach_get_model	     =  sun3_get_model;
 	mach_hwclk           =  sun3_hwclk;
 	mach_halt	     =  sun3_halt;
diff --git a/arch/m68k/sun3/intersil.c b/arch/m68k/sun3/intersil.c
index d911070af02a..8fc74864de81 100644
--- a/arch/m68k/sun3/intersil.c
+++ b/arch/m68k/sun3/intersil.c
@@ -22,13 +22,6 @@
 #define STOP_VAL (INTERSIL_STOP | INTERSIL_INT_ENABLE | INTERSIL_24H_MODE)
 #define START_VAL (INTERSIL_RUN | INTERSIL_INT_ENABLE | INTERSIL_24H_MODE)
 
-/* does this need to be implemented? */
-u32 sun3_gettimeoffset(void)
-{
-  return 1000;
-}
-
-
 /* get/set hwclock */
 
 int sun3_hwclk(int set, struct rtc_time *t)
diff --git a/arch/m68k/sun3x/config.c b/arch/m68k/sun3x/config.c
index 33d3a1c6fba0..03ce7f9facfe 100644
--- a/arch/m68k/sun3x/config.c
+++ b/arch/m68k/sun3x/config.c
@@ -49,7 +49,6 @@ void __init config_sun3x(void)
 	mach_sched_init      = sun3x_sched_init;
 	mach_init_IRQ        = sun3_init_IRQ;
 
-	arch_gettimeoffset   = sun3x_gettimeoffset;
 	mach_reset           = sun3x_reboot;
 
 	mach_hwclk           = sun3x_hwclk;
diff --git a/arch/m68k/sun3x/time.c b/arch/m68k/sun3x/time.c
index 047e2bcee3d7..5328220ed2d8 100644
--- a/arch/m68k/sun3x/time.c
+++ b/arch/m68k/sun3x/time.c
@@ -73,11 +73,6 @@ int sun3x_hwclk(int set, struct rtc_time *t)
 
 	return 0;
 }
-/* Not much we can do here */
-u32 sun3x_gettimeoffset(void)
-{
-    return 0L;
-}
 
 #if 0
 static void sun3x_timer_tick(int irq, void *dev_id, struct pt_regs *regs)
diff --git a/arch/m68k/sun3x/time.h b/arch/m68k/sun3x/time.h
index 496f406412ad..86ce78bb3c28 100644
--- a/arch/m68k/sun3x/time.h
+++ b/arch/m68k/sun3x/time.h
@@ -3,7 +3,6 @@
 #define SUN3X_TIME_H
 
 extern int sun3x_hwclk(int set, struct rtc_time *t);
-u32 sun3x_gettimeoffset(void);
 void sun3x_sched_init(irq_handler_t vector);
 
 struct mostek_dt {
-- 
2.18.1