Subject: [PATCH] MIPS: add proper set_mode() to cevt-r4k

From: John Crispin <[email-masked]>
Date: Mon, 29 Jul 2013 11:55:43 +0200

On ralink SoC a secondary cevt exists, that shares irq 7 with the r4k timer.
For this to work, we first need to teach cevt-r4k to not hog the irq.

Signed-off-by: John Crispin <[email-masked]>
---
 arch/mips/kernel/cevt-r4k.c |   39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c
index 50d3f5a..b726422 100644
--- a/arch/mips/kernel/cevt-r4k.c
+++ b/arch/mips/kernel/cevt-r4k.c
@@ -38,12 +38,6 @@ static int mips_next_event(unsigned long delta,
 
 #endif /* CONFIG_MIPS_MT_SMTC */
 
-void mips_set_clock_mode(enum clock_event_mode mode,
-				struct clock_event_device *evt)
-{
-	/* Nothing to do ...  */
-}
-
 DEFINE_PER_CPU(struct clock_event_device, mips_clockevent_device);
 int cp0_timer_irq_installed;
 
@@ -90,6 +84,32 @@ struct irqaction c0_compare_irqaction = {
 	.name = "timer",
 };
 
+void mips_set_clock_mode(enum clock_event_mode mode,
+				struct clock_event_device *evt)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_ONESHOT:
+		if (cp0_timer_irq_installed)
+			break;
+
+		cp0_timer_irq_installed = 1;
+
+		setup_irq(evt->irq, &c0_compare_irqaction);
+		break;
+
+	case CLOCK_EVT_MODE_SHUTDOWN:
+		if (!cp0_timer_irq_installed)
+			break;
+
+		cp0_timer_irq_installed = 0;
+		free_irq(evt->irq, &c0_compare_irqaction);
+		break;
+
+	default:
+		pr_err("Unhandeled mips clock_mode\n");
+		break;
+	}
+}
 
 void mips_event_handler(struct clock_event_device *dev)
 {
@@ -215,13 +235,6 @@ int r4k_clockevent_init(void)
 #endif
 	clockevents_register_device(cd);
 
-	if (cp0_timer_irq_installed)
-		return 0;
-
-	cp0_timer_irq_installed = 1;
-
-	setup_irq(irq, &c0_compare_irqaction);
-
 	return 0;
 }
 
-- 
1.7.10.4

---

From: Florian Fainelli <[email-masked]>
Date: Mon, 29 Jul 2013 11:40:10 +0100

Hello John,

> For this to work, we first need to teach cevt-r4k to not hog the irq.

It is not clear to me whether this secondary cevt is also a r4k-cevt
device, or if it is something else? If the IRQ is shared, is there any
way to differentiate the ralink cevt from the r4k cevt, such that both
could request the same irq with the IRQF_SHARED flag?

It looks to me like you are moving the irq setup later just to ensure
that your ralink clockevent device has been registered before and has
set cp0_timer_irq_installed when the set_mode() r4k clockevent device
runs, such that it won't register the same IRQ that your platforms
uses. If that it the case, cannot you just ensure that you run your
cevt device registration before mips_clockevent_init() is called?

-- 
Florian

---

From: John Crispin <[email-masked]>
Date: Mon, 29 Jul 2013 12:52:13 +0200

Hi Florian,

> could request the same irq with the IRQF_SHARED flag?

IRQF_SHARED | IRQF_TIMER is not allowed as a combination.

> cevt device registration before mips_clockevent_init() is called?

i dont like relying on the order in which the modules get loaded.

the actual problem is not the irq sharing but that the cevt-r4k 
registers the irq when the cevt is registered and not when it is 
activated. i believe that the patch fixes this problem

	John

---

From: Florian Fainelli <[email-masked]>
Date: Mon, 29 Jul 2013 12:14:36 +0100

> IRQF_SHARED | IRQF_TIMER is not allowed as a combination.

Good point, forgot about that. Then how about a way to let a platform
specify its own callback? Pretty much like what is done with the
handle_perf(r2) case?

> i dont like relying on the order in which the modules get loaded.

plat_time_init() runs before mips_clockevent_init() and the ordering
is explicit, would not that work for what you are trying to do?

>
> the actual problem is not the irq sharing but that the cevt-r4k registers
> the irq when the cevt is registered and not when it is activated. i believe
> that the patch fixes this problem

Your patch certainly does what you say it does, but that is kind of an
abuse of the set_mode() callback.
-- 
Florian

---

From: John Crispin <[email-masked]>
Date: Mon, 29 Jul 2013 13:14:35 +0200

> abuse of the set_mode() callback.

well there are 2 modes "run as oneshot timer and dont run. i dont see 
how this is an abuse?

---

From: "Maciej W. Rozycki" <[email-masked]>
Date: Mon, 29 Jul 2013 15:53:13 +0100 (BST)

> could request the same irq with the IRQF_SHARED flag?

 As from rev. 2 of the MIPS architecture processors are required to 
implement a CP0.Cause.TI bit to indicate a CP0.Count/CP0.Compare timer 
interrupt pending -- so it may all bail down to figuring out what MIPS
architecture level this SoC implements.  FWIW.  HTH.

  Maciej

---

From: Florian Fainelli <[email-masked]>
Date: Tue, 30 Jul 2013 11:01:53 +0100

2013/7/29 John Crispin <[email-masked]>:
> well there are 2 modes "run as oneshot timer and dont run. i dont see how
> this is an abuse?

What you are doing here should be done in some sort of open() call to
the device, not in some sort of ioctl() like interface in my opinion,
I agree that this is your only choice here though.

How about my former proposal to hook your specific use case in
plat_time_init(), does not that work for you?
-- 
Florian

---

From: David Daney <[email-masked]>
Date: Wed, 31 Jul 2013 12:22:15 -0700

> 2013/7/29 John Crispin <[email-masked]>:
[...]
> abuse of the set_mode() callback.

I might as add my $0.02...

There are many other clockevent drivers that do this type of thing 
aren't there?  The clockevent framework uses this to 
install/remove/switch drivers, so why should cevt-r4k not be made to 
work like this?

David Daney

---

From: Florian Fainelli <[email-masked]>
Date: Wed, 31 Jul 2013 20:26:02 +0100

> install/remove/switch drivers, so why should cevt-r4k not be made to

Whatever works for you. I still would like to understand why plat_time_init() 
is not suitable for John's specific use case.
-- 
Florian