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

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

[John Crispin]

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 <blogic@openwrt.org>
---
 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

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

[Florian Fainelli]

Hello John,

2013/7/29 John Crispin <blogic@openwrt.org>:
> 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.

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?

>
> Signed-off-by: John Crispin <blogic@openwrt.org>
> ---
>  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



-- 
Florian

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

[John Crispin]

Hi Florian,

> 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?
>

IRQF_SHARED | IRQF_TIMER is not allowed as a combination.


> 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?

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

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

[Florian Fainelli]

2013/7/29 John Crispin <john@phrozen.org>:
> Hi Florian,
>
>
>> 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?
>>
>
> 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?

>
>
>
>> 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?
>
>
> 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

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

[John Crispin]

>> 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.

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

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

[Florian Fainelli]

2013/7/29 John Crispin <john@phrozen.org>:
>
>>> 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.
>
>
> 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

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

[David Daney]

On 07/29/2013 04:14 AM, Florian Fainelli wrote:
> 2013/7/29 John Crispin <john@phrozen.org>:
[...]
>>
>>> 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?
>>
>>
>> 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.
>

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

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

[Florian Fainelli]

Le mercredi 31 juillet 2013 12:22:15 David Daney a écrit :
> On 07/29/2013 04:14 AM, Florian Fainelli wrote:
> > 2013/7/29 John Crispin <john@phrozen.org>:
> [...]
> 
> >>> 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?
> >> 
> >> 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.
> 
> 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?

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

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

[John Crispin]

On 31/07/13 21:26, Florian Fainelli wrote:
> Le mercredi 31 juillet 2013 12:22:15 David Daney a écrit :
>> On 07/29/2013 04:14 AM, Florian Fainelli wrote:
>>> 2013/7/29 John Crispin<john@phrozen.org>:
>> [...]
>>
>>>>> 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?
>>>>
>>>> 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.
>>
>> 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?
>
> Whatever works for you. I still would like to understand why plat_time_init()
> is not suitable for John's specific use case.

Hi Florian,

the reason is that fixing it in plat_time_init() works around the real 
problem. the double request of the irq is a symptom of the actual 
problem, which is, that the cevt-r4k sets up the timer during init and 
not during setup. additionally, plat_time_init is used to probe the cevt 
drivers from OF already. currently the mips code just assumes that on a 
r4k we always have and want to run the cevt-r4k. this assumption is 
wrong and can quickly be fixed by making the cevt-r4k use the correct api.

also fixing it this way allows the user to control the clocksource and 
change it at runtime via sysfs, a feature als not working currently on 
r4k as the cevt driver did not implement the set_mode() handler 
correctly. to be quite honest, i cannot think of a single way in which 
this can be fixed cleanly in the ralink plat_time_init() without using 
some weird heuristic. also if i fix this inside ralink plat_time_init() 
it is fixed only on ralink SoC and not on any other platform.

	John

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

[Ralf Baechle]

On Thu, Aug 01, 2013 at 08:15:50AM +0200, John Crispin wrote:

> >Whatever works for you. I still would like to understand why plat_time_init()
> >is not suitable for John's specific use case.
> 
> Hi Florian,
> 
> the reason is that fixing it in plat_time_init() works around the
> real problem. the double request of the irq is a symptom of the
> actual problem, which is, that the cevt-r4k sets up the timer during
> init and not during setup. additionally, plat_time_init is used to
> probe the cevt drivers from OF already. currently the mips code just
> assumes that on a r4k we always have and want to run the cevt-r4k.
> this assumption is wrong and can quickly be fixed by making the
> cevt-r4k use the correct api.
> 
> also fixing it this way allows the user to control the clocksource
> and change it at runtime via sysfs, a feature als not working
> currently on r4k as the cevt driver did not implement the set_mode()
> handler correctly. to be quite honest, i cannot think of a single
> way in which this can be fixed cleanly in the ralink
> plat_time_init() without using some weird heuristic. also if i fix
> this inside ralink plat_time_init() it is fixed only on ralink SoC
> and not on any other platform.

setup_irq() may fail but set_mode doesn't have a way to communicate an
error - other than leaving back a half-wrecked system so set_mode is not
a good place to do that kind of job.

How about using get_c0_compare_int() for a solution?  Currently
get_c0_compare_int() can not return an error.  If it could return a
negative value to indicate the unavailability of an interrupt for
cevt-r4k's use, that interrupt would be available for alternative use.

  Ralf

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

[John Crispin]

> setup_irq() may fail but set_mode doesn't have a way to communicate an
> error - other than leaving back a half-wrecked system so set_mode is not
> a good place to do that kind of job.

much like the current code which does not check set_mode either. also, a 
core that boots and cannot get its clock irq is not half wrecked, its 
fully wrecked and wont be able to boot anyway.

>
> How about using get_c0_compare_int() for a solution?  Currently
> get_c0_compare_int() can not return an error.  If it could return a
> negative value to indicate the unavailability of an interrupt for
> cevt-r4k's use, that interrupt would be available for alternative use.

the code could be changed but get_c0_compare_int() returns an unsigned 
so that would require changing the return value everywhere the function 
is defined.

i still think that my proposed patch is valid, we could add a panic call 
if set_mode fails. having your kernel tell you to start an instance of 
your main clock and not being able to request said irq, does look like a 
valid cause for a panic() to me

	John

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

[Maciej W. Rozycki]

On Mon, 29 Jul 2013, Florian Fainelli wrote:

> 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?

 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