[PATCH][MIPS] add GT641xx timer0 clockevent

[PATCH][MIPS] add GT641xx timer0 clockevent

[Yoichi Yuasa]

Add GT641xx timer0 clockevent.

Signed-off-by: Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp>

diff -pruN -X mips/Documentation/dontdiff mips-orig/arch/mips/Kconfig mips/arch/mips/Kconfig
--- mips-orig/arch/mips/Kconfig	2007-10-22 09:19:37.959724750 +0900
+++ mips/arch/mips/Kconfig	2007-10-22 09:19:23.342811250 +0900
@@ -66,6 +66,7 @@ config BCM47XX
 config MIPS_COBALT
 	bool "Cobalt Server"
 	select CEVT_R4K
+	select CEVT_GT641XX
 	select DMA_NONCOHERENT
 	select HW_HAS_PCI
 	select I8253
@@ -729,6 +730,9 @@ config ARCH_MAY_HAVE_PC_FDC
 config BOOT_RAW
 	bool
 
+config CEVT_GT641XX
+	bool
+
 config CEVT_R4K
 	bool
 
diff -pruN -X mips/Documentation/dontdiff mips-orig/arch/mips/cobalt/Makefile mips/arch/mips/cobalt/Makefile
--- mips-orig/arch/mips/cobalt/Makefile	2007-10-22 09:19:37.971725500 +0900
+++ mips/arch/mips/cobalt/Makefile	2007-10-22 09:19:23.354812000 +0900
@@ -2,7 +2,7 @@
 # Makefile for the Cobalt micro systems family specific parts of the kernel
 #
 
-obj-y := buttons.o irq.o led.o reset.o rtc.o serial.o setup.o
+obj-y := buttons.o irq.o led.o reset.o rtc.o serial.o setup.o time.o
 
 obj-$(CONFIG_PCI)		+= pci.o
 obj-$(CONFIG_EARLY_PRINTK)	+= console.o
diff -pruN -X mips/Documentation/dontdiff mips-orig/arch/mips/cobalt/setup.c mips/arch/mips/cobalt/setup.c
--- mips-orig/arch/mips/cobalt/setup.c	2007-10-22 09:19:37.987726500 +0900
+++ mips/arch/mips/cobalt/setup.c	2007-10-22 09:19:23.366812750 +0900
@@ -9,19 +9,17 @@
  * Copyright (C) 2001, 2002, 2003 by Liam Davies (ldavies@agile.tv)
  *
  */
-#include <linux/interrupt.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
 #include <linux/pm.h>
 
 #include <asm/bootinfo.h>
-#include <asm/time.h>
-#include <asm/i8253.h>
-#include <asm/io.h>
 #include <asm/reboot.h>
 #include <asm/gt64120.h>
 
 #include <cobalt.h>
-#include <irq.h>
 
 extern void cobalt_machine_restart(char *command);
 extern void cobalt_machine_halt(void);
@@ -41,17 +39,6 @@ const char *get_system_type(void)
 	return "MIPS Cobalt";
 }
 
-void __init plat_timer_setup(struct irqaction *irq)
-{
-	/* Load timer value for HZ (TCLK is 50MHz) */
-	GT_WRITE(GT_TC0_OFS, 50*1000*1000 / HZ);
-
-	/* Enable timer0 */
-	GT_WRITE(GT_TC_CONTROL_OFS, GT_TC_CONTROL_ENTC0_MSK | GT_TC_CONTROL_SELTC0_MSK);
-
-	setup_irq(GT641XX_TIMER0_IRQ, irq);
-}
-
 /*
  * Cobalt doesn't have PS/2 keyboard/mouse interfaces,
  * keyboard conntroller is never used.
@@ -84,11 +71,6 @@ static struct resource cobalt_reserved_r
 	},
 };
 
-void __init plat_time_init(void)
-{
-	setup_pit_timer();
-}
-
 void __init plat_mem_setup(void)
 {
 	int i;
diff -pruN -X mips/Documentation/dontdiff mips-orig/arch/mips/cobalt/time.c mips/arch/mips/cobalt/time.c
--- mips-orig/arch/mips/cobalt/time.c	1970-01-01 09:00:00.000000000 +0900
+++ mips/arch/mips/cobalt/time.c	2007-10-22 09:24:18.157236000 +0900
@@ -0,0 +1,35 @@
+/*
+ *  Cobalt time initialization.
+ *
+ *  Copyright (C) 2007  Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+#include <linux/init.h>
+
+#include <asm/gt64120.h>
+#include <asm/i8253.h>
+#include <asm/time.h>
+
+#define GT641XX_BASE_CLOCK	50000000	/* 50MHz */
+
+void __init plat_time_init(void)
+{
+	setup_pit_timer();
+
+	gt641xx_set_base_clock(GT641XX_BASE_CLOCK);
+
+	mips_timer_state = gt641xx_timer0_state;
+}
diff -pruN -X mips/Documentation/dontdiff mips-orig/arch/mips/kernel/Makefile mips/arch/mips/kernel/Makefile
--- mips-orig/arch/mips/kernel/Makefile	2007-10-22 09:19:38.011728000 +0900
+++ mips/arch/mips/kernel/Makefile	2007-10-22 09:19:23.366812750 +0900
@@ -9,6 +9,7 @@ obj-y		+= cpu-probe.o branch.o entry.o g
 		   time.o topology.o traps.o unaligned.o
 
 obj-$(CONFIG_CEVT_R4K)		+= cevt-r4k.o
+obj-$(CONFIG_CEVT_GT641XX)	+= cevt-gt641xx.o
 
 binfmt_irix-objs	:= irixelf.o irixinv.o irixioctl.o irixsig.o	\
 			   irix5sys.o sysirix.o
diff -pruN -X mips/Documentation/dontdiff mips-orig/arch/mips/kernel/cevt-gt641xx.c mips/arch/mips/kernel/cevt-gt641xx.c
--- mips-orig/arch/mips/kernel/cevt-gt641xx.c	1970-01-01 09:00:00.000000000 +0900
+++ mips/arch/mips/kernel/cevt-gt641xx.c	2007-10-22 09:24:33.146172750 +0900
@@ -0,0 +1,144 @@
+/*
+ *  GT641xx clockevent routines.
+ *
+ *  Copyright (C) 2007  Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+#include <linux/clockchips.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/spinlock.h>
+
+#include <asm/gt64120.h>
+#include <asm/time.h>
+
+#include <irq.h>
+
+static DEFINE_SPINLOCK(gt641xx_timer_lock);
+static unsigned int gt641xx_base_clock;
+
+void gt641xx_set_base_clock(unsigned int clock)
+{
+	gt641xx_base_clock = clock;
+}
+
+int gt641xx_timer0_state(void)
+{
+	if (GT_READ(GT_TC0_OFS))
+		return 0;
+
+	GT_WRITE(GT_TC0_OFS, gt641xx_base_clock / HZ);
+	GT_WRITE(GT_TC_CONTROL_OFS, GT_TC_CONTROL_ENTC0_MSK);
+
+	return 1;
+}
+
+static int gt641xx_timer0_set_next_event(unsigned long delta,
+					 struct clock_event_device *evt)
+{
+	unsigned long flags;
+	u32 ctrl;
+
+	spin_lock_irqsave(&gt641xx_timer_lock, flags);
+
+	ctrl = GT_READ(GT_TC_CONTROL_OFS);
+	ctrl &= ~(GT_TC_CONTROL_ENTC0_MSK | GT_TC_CONTROL_SELTC0_MSK);
+	ctrl |= GT_TC_CONTROL_ENTC0_MSK;
+
+	GT_WRITE(GT_TC0_OFS, delta);
+	GT_WRITE(GT_TC_CONTROL_OFS, ctrl);
+
+	spin_unlock_irqrestore(&gt641xx_timer_lock, flags);
+
+	return 0;
+}
+
+static void gt641xx_timer0_set_mode(enum clock_event_mode mode,
+				    struct clock_event_device *evt)
+{
+	unsigned long flags;
+	u32 ctrl;
+
+	spin_lock_irqsave(&gt641xx_timer_lock, flags);
+
+	ctrl = GT_READ(GT_TC_CONTROL_OFS);
+	ctrl &= ~(GT_TC_CONTROL_ENTC0_MSK | GT_TC_CONTROL_SELTC0_MSK);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		ctrl |= GT_TC_CONTROL_ENTC0_MSK | GT_TC_CONTROL_SELTC0_MSK;
+		break;
+	case CLOCK_EVT_MODE_ONESHOT:
+		ctrl |= GT_TC_CONTROL_ENTC0_MSK;
+		break;
+	default:
+		break;
+	}
+
+	GT_WRITE(GT_TC_CONTROL_OFS, ctrl);
+
+	spin_unlock_irqrestore(&gt641xx_timer_lock, flags);
+}
+
+static void gt641xx_timer0_event_handler(struct clock_event_device *dev)
+{
+}
+
+static struct clock_event_device gt641xx_timer0_clockevent = {
+	.name		= "gt641xx-timer0",
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.cpumask	= CPU_MASK_CPU0,
+	.irq		= GT641XX_TIMER0_IRQ,
+	.set_next_event	= gt641xx_timer0_set_next_event,
+	.set_mode	= gt641xx_timer0_set_mode,
+	.event_handler	= gt641xx_timer0_event_handler,
+};
+
+static irqreturn_t gt641xx_timer0_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *cd = &gt641xx_timer0_clockevent;
+
+	cd->event_handler(cd);
+
+	return IRQ_HANDLED;
+}
+
+static struct irqaction gt641xx_timer0_irqaction = {
+	.handler	= gt641xx_timer0_interrupt,
+	.flags		= IRQF_DISABLED | IRQF_PERCPU,
+	.name		= "gt641xx_timer0",
+};
+
+static int __init gt641xx_timer0_clockevent_init(void)
+{
+	struct clock_event_device *cd;
+
+	if (!gt641xx_base_clock)
+		return 0;
+
+	GT_WRITE(GT_TC0_OFS, gt641xx_base_clock / HZ);
+
+	cd = &gt641xx_timer0_clockevent;
+	cd->rating = 200 + gt641xx_base_clock / 10000000;
+	cd->max_delta_ns = clockevent_delta2ns(0x7fffffff, cd);
+	cd->min_delta_ns = clockevent_delta2ns(0x300, cd);
+	clockevent_set_clock(cd, gt641xx_base_clock);
+
+	clockevents_register_device(&gt641xx_timer0_clockevent);
+
+	return setup_irq(GT641XX_TIMER0_IRQ, &gt641xx_timer0_irqaction);
+}
+arch_initcall(gt641xx_timer0_clockevent_init);
diff -pruN -X mips/Documentation/dontdiff mips-orig/include/asm-mips/gt64120.h mips/include/asm-mips/gt64120.h
--- mips-orig/include/asm-mips/gt64120.h	2007-10-22 09:19:38.055730750 +0900
+++ mips/include/asm-mips/gt64120.h	2007-10-22 09:19:23.370813000 +0900
@@ -21,6 +21,8 @@
 #ifndef _ASM_GT64120_H
 #define _ASM_GT64120_H
 
+#include <linux/clocksource.h>
+
 #include <asm/addrspace.h>
 #include <asm/byteorder.h>
 
@@ -572,4 +574,7 @@
 #define GT_READ(ofs)		le32_to_cpu(__GT_READ(ofs))
 #define GT_WRITE(ofs, data)	__GT_WRITE(ofs, cpu_to_le32(data))
 
+extern void gt641xx_set_base_clock(unsigned int clock);
+extern int gt641xx_timer0_state(void);
+
 #endif /* _ASM_GT64120_H */

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Ralf Baechle]

On Mon, Oct 22, 2007 at 07:43:15PM +0900, Yoichi Yuasa wrote:

> Add GT641xx timer0 clockevent.

Thanks, applied.

Btw, I like that you put cevt-gt641xx.c into arch/mips/kernel.  Unlike the
per-vendor subdirectories under arch/mips which have a tradition of
promoting messy code and endorsing code duplication that rather is the
way to go.  Similar for clocksources.

  Ralf

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Atsushi Nemoto]

On Mon, 22 Oct 2007 19:43:15 +0900, Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp> wrote:
> +static int gt641xx_timer0_set_next_event(unsigned long delta,
> +					 struct clock_event_device *evt)
> +{
> +	unsigned long flags;
> +	u32 ctrl;
> +
> +	spin_lock_irqsave(&gt641xx_timer_lock, flags);
> +
> +	ctrl = GT_READ(GT_TC_CONTROL_OFS);
> +	ctrl &= ~(GT_TC_CONTROL_ENTC0_MSK | GT_TC_CONTROL_SELTC0_MSK);
> +	ctrl |= GT_TC_CONTROL_ENTC0_MSK;
> +
> +	GT_WRITE(GT_TC0_OFS, delta);
> +	GT_WRITE(GT_TC_CONTROL_OFS, ctrl);
> +
> +	spin_unlock_irqrestore(&gt641xx_timer_lock, flags);
> +
> +	return 0;
> +}
> +
> +static void gt641xx_timer0_set_mode(enum clock_event_mode mode,
> +				    struct clock_event_device *evt)
> +{
> +	unsigned long flags;
> +	u32 ctrl;
> +
> +	spin_lock_irqsave(&gt641xx_timer_lock, flags);
> +
> +	ctrl = GT_READ(GT_TC_CONTROL_OFS);
> +	ctrl &= ~(GT_TC_CONTROL_ENTC0_MSK | GT_TC_CONTROL_SELTC0_MSK);
> +
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		ctrl |= GT_TC_CONTROL_ENTC0_MSK | GT_TC_CONTROL_SELTC0_MSK;
> +		break;
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		ctrl |= GT_TC_CONTROL_ENTC0_MSK;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	GT_WRITE(GT_TC_CONTROL_OFS, ctrl);
> +
> +	spin_unlock_irqrestore(&gt641xx_timer_lock, flags);
> +}

These clockevent routines are always called with interrupt disabled,
so I suppose those spin_lock_irqsave/spin_unlock_irqrestore pairs can
be omitted. (or this timer can be used on SMP?)

> +	cd = &gt641xx_timer0_clockevent;
> +	cd->rating = 200 + gt641xx_base_clock / 10000000;
> +	cd->max_delta_ns = clockevent_delta2ns(0x7fffffff, cd);
> +	cd->min_delta_ns = clockevent_delta2ns(0x300, cd);
> +	clockevent_set_clock(cd, gt641xx_base_clock);

clockevent_delta2ns() use shift and mult value.  So you should call
clockevent_set_clock() first.

---
Atsushi Nemoto

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Maciej W. Rozycki]

On Mon, 22 Oct 2007, Ralf Baechle wrote:

> > Add GT641xx timer0 clockevent.
> 
> Thanks, applied.

 Ah, we could use this one with the Malta and some CoreLV cards too. ;-)

  Maciej

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Ralf Baechle]

On Mon, Oct 22, 2007 at 04:06:55PM +0100, Maciej W. Rozycki wrote:

> > > Add GT641xx timer0 clockevent.
> > 
> > Thanks, applied.
> 
>  Ah, we could use this one with the Malta and some CoreLV cards too. ;-)

We could.  The other question is of course if it is a good idea.  It isn't
always, there might be a better timer available.

Honestly, no idea about the Cobalt.  Afair the compare interrupt was usable
there so cevt-gt641xx.c isn't necessarily needed or a good idea there.
Too long :-)

  Ralf

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Maciej W. Rozycki]

On Mon, 22 Oct 2007, Ralf Baechle wrote:

> We could.  The other question is of course if it is a good idea.  It isn't
> always, there might be a better timer available.

 The interrupt is shared with other sources internally in the system 
controller -- that may complicate handling and affect accuracy.  That does 
not mean making a note of it and possibly investigating at leisure might 
not be a reasonable idea.

  Maciej

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Yoichi Yuasa]

On Mon, 22 Oct 2007 23:47:55 +0900 (JST)
Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:

> On Mon, 22 Oct 2007 19:43:15 +0900, Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp> wrote:
> > +static int gt641xx_timer0_set_next_event(unsigned long delta,
> > +					 struct clock_event_device *evt)
> > +{
> > +	unsigned long flags;
> > +	u32 ctrl;
> > +
> > +	spin_lock_irqsave(&gt641xx_timer_lock, flags);
> > +
> > +	ctrl = GT_READ(GT_TC_CONTROL_OFS);
> > +	ctrl &= ~(GT_TC_CONTROL_ENTC0_MSK | GT_TC_CONTROL_SELTC0_MSK);
> > +	ctrl |= GT_TC_CONTROL_ENTC0_MSK;
> > +
> > +	GT_WRITE(GT_TC0_OFS, delta);
> > +	GT_WRITE(GT_TC_CONTROL_OFS, ctrl);
> > +
> > +	spin_unlock_irqrestore(&gt641xx_timer_lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static void gt641xx_timer0_set_mode(enum clock_event_mode mode,
> > +				    struct clock_event_device *evt)
> > +{
> > +	unsigned long flags;
> > +	u32 ctrl;
> > +
> > +	spin_lock_irqsave(&gt641xx_timer_lock, flags);
> > +
> > +	ctrl = GT_READ(GT_TC_CONTROL_OFS);
> > +	ctrl &= ~(GT_TC_CONTROL_ENTC0_MSK | GT_TC_CONTROL_SELTC0_MSK);
> > +
> > +	switch (mode) {
> > +	case CLOCK_EVT_MODE_PERIODIC:
> > +		ctrl |= GT_TC_CONTROL_ENTC0_MSK | GT_TC_CONTROL_SELTC0_MSK;
> > +		break;
> > +	case CLOCK_EVT_MODE_ONESHOT:
> > +		ctrl |= GT_TC_CONTROL_ENTC0_MSK;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	GT_WRITE(GT_TC_CONTROL_OFS, ctrl);
> > +
> > +	spin_unlock_irqrestore(&gt641xx_timer_lock, flags);
> > +}
> 
> These clockevent routines are always called with interrupt disabled,
> so I suppose those spin_lock_irqsave/spin_unlock_irqrestore pairs can
> be omitted. (or this timer can be used on SMP?)

Yes, it can be used on Malta(SMP).

> > +	cd = &gt641xx_timer0_clockevent;
> > +	cd->rating = 200 + gt641xx_base_clock / 10000000;
> > +	cd->max_delta_ns = clockevent_delta2ns(0x7fffffff, cd);
> > +	cd->min_delta_ns = clockevent_delta2ns(0x300, cd);
> > +	clockevent_set_clock(cd, gt641xx_base_clock);
> 
> clockevent_delta2ns() use shift and mult value.  So you should call
> clockevent_set_clock() first.

Thank you for your comment.
I'll fix it up.

Yoichi

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Atsushi Nemoto]

On Tue, 23 Oct 2007 09:55:55 +0900, Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp> wrote:
> > These clockevent routines are always called with interrupt disabled,
> > so I suppose those spin_lock_irqsave/spin_unlock_irqrestore pairs can
> > be omitted. (or this timer can be used on SMP?)
> 
> Yes, it can be used on Malta(SMP).

Then spin_lock()/spin_unlock() is enough, isn't it?

---
Atsushi Nemoto

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Yoichi Yuasa]

On Tue, 23 Oct 2007 10:06:45 +0900 (JST)
Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:

> On Tue, 23 Oct 2007 09:55:55 +0900, Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp> wrote:
> > > These clockevent routines are always called with interrupt disabled,
> > > so I suppose those spin_lock_irqsave/spin_unlock_irqrestore pairs can
> > > be omitted. (or this timer can be used on SMP?)
> > 
> > Yes, it can be used on Malta(SMP).
> 
> Then spin_lock()/spin_unlock() is enough, isn't it?

The timer control register(GT_TC_CONTROL_OFS) is shared with 4 timers.
The 4 timers are connected with separate IRQ. 

clockevents_program_event() and clockevents_set_mode() can be called from
anywhere(in the kernel).

I think that it's necessary for it.

Yoichi

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Atsushi Nemoto]

Added Thomas Gleixner for CC list.

On Tue, 23 Oct 2007 11:15:58 +0900, Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp> wrote:
> > > > These clockevent routines are always called with interrupt disabled,
> > > > so I suppose those spin_lock_irqsave/spin_unlock_irqrestore pairs can
> > > > be omitted. (or this timer can be used on SMP?)
> > > 
> > > Yes, it can be used on Malta(SMP).
> > 
> > Then spin_lock()/spin_unlock() is enough, isn't it?
> 
> The timer control register(GT_TC_CONTROL_OFS) is shared with 4 timers.
> The 4 timers are connected with separate IRQ. 
> 
> clockevents_program_event() and clockevents_set_mode() can be called from
> anywhere(in the kernel).
> 
> I think that it's necessary for it.

Hmm... clockevents_set_mode() must be called with interrupt disabled
as stated in its comment.  There are no such a comment for
clockevents_program_event(), but it is always called with interrupt
disabled if the interrupt for the event was registered with
IRQF_DISABLED flag.

I agree that saving/restoring irq_flag would be safer, but I think it
can be omitted at least for now.

If clockevents_program_event() could be called with interrupt enabled,
mips_next_event() in cevt-r4k.c should be fixed.


Thomas, clockevents_program_event() (or ->set_next_event() method for
clock_event_device) is supposed to be called with interrupt enabled?

---
Atsushi Nemoto

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Thomas Gleixner]

On Tue, 23 Oct 2007, Atsushi Nemoto wrote:
> Added Thomas Gleixner for CC list.
> 
> On Tue, 23 Oct 2007 11:15:58 +0900, Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp> wrote:
> > > > > These clockevent routines are always called with interrupt disabled,
> > > > > so I suppose those spin_lock_irqsave/spin_unlock_irqrestore pairs can
> > > > > be omitted. (or this timer can be used on SMP?)
> > > > 
> > > > Yes, it can be used on Malta(SMP).
> > > 
> > > Then spin_lock()/spin_unlock() is enough, isn't it?
> > 
> > The timer control register(GT_TC_CONTROL_OFS) is shared with 4 timers.
> > The 4 timers are connected with separate IRQ. 
> > 
> > clockevents_program_event() and clockevents_set_mode() can be called from
> > anywhere(in the kernel).
> > 
> > I think that it's necessary for it.
> 
> Hmm... clockevents_set_mode() must be called with interrupt disabled
> as stated in its comment.  There are no such a comment for
> clockevents_program_event(), but it is always called with interrupt
> disabled if the interrupt for the event was registered with
> IRQF_DISABLED flag.
> 
> I agree that saving/restoring irq_flag would be safer, but I think it
> can be omitted at least for now.
> 
> If clockevents_program_event() could be called with interrupt enabled,
> mips_next_event() in cevt-r4k.c should be fixed.
> 
> 
> Thomas, clockevents_program_event() (or ->set_next_event() method for
> clock_event_device) is supposed to be called with interrupt enabled?

Actually all call sites have interrupts disabled right now and I can
not think of a reason why we would ever call with interrupts
enabled. Time to add some comment.

	 tglx

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Yoichi Yuasa]

On Tue, 23 Oct 2007 07:21:28 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 23 Oct 2007, Atsushi Nemoto wrote:
> > Added Thomas Gleixner for CC list.
> > 
> > On Tue, 23 Oct 2007 11:15:58 +0900, Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp> wrote:
> > > > > > These clockevent routines are always called with interrupt disabled,
> > > > > > so I suppose those spin_lock_irqsave/spin_unlock_irqrestore pairs can
> > > > > > be omitted. (or this timer can be used on SMP?)
> > > > > 
> > > > > Yes, it can be used on Malta(SMP).
> > > > 
> > > > Then spin_lock()/spin_unlock() is enough, isn't it?
> > > 
> > > The timer control register(GT_TC_CONTROL_OFS) is shared with 4 timers.
> > > The 4 timers are connected with separate IRQ. 
> > > 
> > > clockevents_program_event() and clockevents_set_mode() can be called from
> > > anywhere(in the kernel).
> > > 
> > > I think that it's necessary for it.
> > 
> > Hmm... clockevents_set_mode() must be called with interrupt disabled
> > as stated in its comment.  There are no such a comment for
> > clockevents_program_event(), but it is always called with interrupt
> > disabled if the interrupt for the event was registered with
> > IRQF_DISABLED flag.
> > 
> > I agree that saving/restoring irq_flag would be safer, but I think it
> > can be omitted at least for now.
> > 
> > If clockevents_program_event() could be called with interrupt enabled,
> > mips_next_event() in cevt-r4k.c should be fixed.
> > 
> > 
> > Thomas, clockevents_program_event() (or ->set_next_event() method for
> > clock_event_device) is supposed to be called with interrupt enabled?
> 
> Actually all call sites have interrupts disabled right now and I can
> not think of a reason why we would ever call with interrupts
> enabled. Time to add some comment.

OK, I'll update GT641xx clockevent.

Thanks Nemoto-san and Thomas,

Yoichi

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Ralf Baechle]

On Tue, Oct 23, 2007 at 07:21:28AM +0200, Thomas Gleixner wrote:

> > Thomas, clockevents_program_event() (or ->set_next_event() method for
> > clock_event_device) is supposed to be called with interrupt enabled?
> 
> Actually all call sites have interrupts disabled right now and I can
> not think of a reason why we would ever call with interrupts
> enabled. Time to add some comment.

In which case arch/{mips,x86]/kernel/i8253.c can both be improved too.

  Ralf

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

 arch/mips/kernel/i8253.c |   12 ++++--------
 arch/x86/kernel/i8253.c  |   12 ++++--------
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/arch/mips/kernel/i8253.c b/arch/mips/kernel/i8253.c
index 5d9830d..1b54674 100644
--- a/arch/mips/kernel/i8253.c
+++ b/arch/mips/kernel/i8253.c
@@ -23,9 +23,7 @@ static DEFINE_SPINLOCK(i8253_lock);
 static void init_pit_timer(enum clock_event_mode mode,
 			   struct clock_event_device *evt)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&i8253_lock, flags);
+	spin_lock(&i8253_lock);
 
 	switch(mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
@@ -54,7 +52,7 @@ static void init_pit_timer(enum clock_event_mode mode,
 		/* Nothing to do here */
 		break;
 	}
-	spin_unlock_irqrestore(&i8253_lock, flags);
+	spin_unlock(&i8253_lock);
 }
 
 /*
@@ -64,12 +62,10 @@ static void init_pit_timer(enum clock_event_mode mode,
  */
 static int pit_next_event(unsigned long delta, struct clock_event_device *evt)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&i8253_lock, flags);
+	spin_lock(&i8253_lock);
 	outb_p(delta & 0xff , PIT_CH0);	/* LSB */
 	outb(delta >> 8 , PIT_CH0);	/* MSB */
-	spin_unlock_irqrestore(&i8253_lock, flags);
+	spin_unlock(&i8253_lock);
 
 	return 0;
 }
diff --git a/arch/x86/kernel/i8253.c b/arch/x86/kernel/i8253.c
index a42c807..5246515 100644
--- a/arch/x86/kernel/i8253.c
+++ b/arch/x86/kernel/i8253.c
@@ -31,9 +31,7 @@ struct clock_event_device *global_clock_event;
 static void init_pit_timer(enum clock_event_mode mode,
 			   struct clock_event_device *evt)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&i8253_lock, flags);
+	spin_lock(&i8253_lock);
 
 	switch(mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
@@ -62,7 +60,7 @@ static void init_pit_timer(enum clock_event_mode mode,
 		/* Nothing to do here */
 		break;
 	}
-	spin_unlock_irqrestore(&i8253_lock, flags);
+	spin_unlock(&i8253_lock);
 }
 
 /*
@@ -72,12 +70,10 @@ static void init_pit_timer(enum clock_event_mode mode,
  */
 static int pit_next_event(unsigned long delta, struct clock_event_device *evt)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&i8253_lock, flags);
+	spin_lock(&i8253_lock);
 	outb_p(delta & 0xff , PIT_CH0);	/* LSB */
 	outb(delta >> 8 , PIT_CH0);	/* MSB */
-	spin_unlock_irqrestore(&i8253_lock, flags);
+	spin_unlock(&i8253_lock);
 
 	return 0;
 }

Re: [PATCH][MIPS] add GT641xx timer0 clockevent

[Atsushi Nemoto]

On Tue, 23 Oct 2007 07:21:28 +0200 (CEST), Thomas Gleixner <tglx@linutronix.de> wrote:
> > Thomas, clockevents_program_event() (or ->set_next_event() method for
> > clock_event_device) is supposed to be called with interrupt enabled?
> 
> Actually all call sites have interrupts disabled right now and I can
> not think of a reason why we would ever call with interrupts
> enabled. Time to add some comment.

Thanks.  Then I thought i8253 can be optimized little bit, and Ralf
has aleady sent the patch :)

---
Atsushi Nemoto