Re: [RFC Implement clockevents/clocksource for R4000-style cp0 timer [take #3]

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Hello.

Franck Bui-Huu wrote:

> This patch is an attempt to add clocksource/clockevent support for
> platforms that use R4000-style cp0 timer as the tick source of the
> system.

[...]

> It implements clockevent/clocksource support for cp0 counter in a new
> file "hpt-cp0.c" but let the platform code to initialize it if it
> chooses to use it. Therefore there's no more generic code which tries
> to guess what the platform wants to use as tick source because in
> practice only the platform code can drive the initialization of all
> timer devices and interrupts properly.

> Take #3 includes minor changes (Thanks to Sergei Shtylyov's feedbacks)


> Changes since take #2:
> ---------------------

>    - clocksource rating does not depend on cp0-count frequency anymore
>      since a clocksource should be selected for its stability first,
>      not its precision.

>    - Rename CP0_HPT_TIMER into CP0_CLOCKS but I'm still not sure. If

    Why not just CP0_TIMER? Hm, CP0_HPT_TIMER was even redundant... :-)

>      someone can come with a better idea that would be nice. BTW
>      hpt-cp0.c file should be renamed into clock-cp0.c too.

    I suggest timer-cp0.c ot cp0-timer.c

> TODO:
> -----

>    - There are still few places to fix that uses 'mips_hpt_frequency'
>      global.

>    - If this patch is accepted, fix all platforms that use cp0
>      counter.

> Please have a look,

> 		Franck

> -- 8< --

> Subject: [PATCH] Implement clockevents/clocksource for R4000-style cp0 timer

> Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
> ---
>  arch/mips/Kconfig          |    9 +
>  arch/mips/kernel/Makefile  |    2 +
>  arch/mips/kernel/hpt-cp0.c |  259 +++++++++++++++++++++++++++
>  arch/mips/kernel/process.c |    3 +
>  arch/mips/kernel/smp.c     |    2 +
>  arch/mips/kernel/time.c    |  416 ++++----------------------------------------
>  include/asm-mips/hpt.h     |   36 ++++

    That HPT just doesn't want to die. :-)

[...]
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 7bcf38d..a994af1 100644
[...]
> @@ -1741,6 +1749,7 @@ config HZ
>  	default 1000 if HZ_1000
>  	default 1024 if HZ_1024
>  
> +source "kernel/time/Kconfig"
>  source "kernel/Kconfig.preempt"

    I would have put this part into a separate patch, along with tickless mode
hooks in arch/mips/kernel/process.c...

> diff --git a/arch/mips/kernel/hpt-cp0.c b/arch/mips/kernel/hpt-cp0.c
> new file mode 100644
> index 0000000..e2defcd
> --- /dev/null
> +++ b/arch/mips/kernel/hpt-cp0.c
> @@ -0,0 +1,259 @@
> +/*
> + * hpt-cp0.c - CP0 clock driver
> + *
> + * Copyright (C) 2007,  Franck Bui-Huu <fbuihuu@gmail.com>
> + *
> + * This code is released under the GNU General Public License,
> + * Version 2 (GPL v2).
> + */
> +#include <linux/kernel_stat.h>
> +#include <linux/spinlock.h>
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +
> +#include <asm/time.h>
> +#include <asm/hpt.h>
> +
> +
> +#define CP0_CLOCK_NAME	"cp0-count"
> +
> +static unsigned (*get_freq)(int cpu) __initdata;
> +static irqreturn_t (*perf_handler)(int irq, void *dev_id) __read_mostly;
> +
> +/*
> + * cp0 clocks can be disabled by boot command line
> + */
> +static int disable_clockevent __initdata;
> +static int disable_clocksource __initdata;
> +
> +static int __init cp0_clock_setup(char *arg)
> +{
> +	if (arg == NULL)
> +		return -EINVAL;
> +
> +	if (!strcmp(arg, "disable_event"))
> +		disable_clockevent = 1;
> +	else if (!strcmp(arg, "disable_source"))
> +		disable_clocksource = 1;
> +	else if (!strcmp(arg, "disable_both")) {
> +		disable_clocksource = 1;
> +		disable_clockevent = 1;

    How about "noevent", "nosource", and "none"?

> +	}
> +	return 0;
> +}
> +early_param("cp0_clock", cp0_clock_setup);
> +
> +/*
> + * cp0 count/compare operations.
> + */
> +static void cp0_count_ack(void)
> +{
> +	write_c0_compare(read_c0_compare());
> +}
> +
> +static cycle_t cp0_count_read(void)
> +{
> +        return read_c0_count();

    "Entab" spaces here please.

> +}
> +
> +/*
> + * Clocksource device. Its rating should not depend on its frequency:
> + * stability is a feature more valuable for a clock source.
> + */
> +struct clocksource cp0_clocksource = {
> +	.name		= CP0_CLOCK_NAME,
> +	.rating		= 200,

    Perhaps we even need to rise the rating to 300 or 400 -- according to what
<linux/clocksource.h> says...

> +	.mask		= CLOCKSOURCE_MASK(32),
> +	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
> +	.read		= cp0_count_read,
> +};
> +
> +static void __init setup_cp0_clocksource(void)
> +{
> +	int cpu = smp_processor_id();
> +	unsigned freq = get_freq(cpu);

    Might fold these 2 lines into:

unsigned freq = get_freq(smp_processor_id());

> +	unsigned shift = 0;

    Unneeded initializer.

> +	u64 mult;
> +
> +	for (shift = 32; shift > 0; shift--) {
> +		mult = (u64)NSEC_PER_SEC << shift;
> +		do_div(mult, freq);
> +		if ((mult >> 32) == 0)
> +			break;
> +	}
> +
> +	cp0_clocksource.shift = shift;
> +	cp0_clocksource.mult = mult;
> +
> +	clocksource_register(&cp0_clocksource);
> +}
> +
> +/*
> + * High precision timer functions
> + */
> +static int cp0_set_next_event(unsigned long delta,
> +				   struct clock_event_device *evt)
> +{
> +	unsigned int cnt;
> +
> +	BUG_ON(evt->mode != CLOCK_EVT_MODE_ONESHOT);
> +
> +	/* interrupt ack is done by setting up the next event */

    We acknowledge interrupt independently from this code anyway, so the 
comment seems misplaced.

> +	cnt = read_c0_count();
> +	cnt += delta;

   Could be merge into one statement...

> +	write_c0_compare(cnt);
> +
> +	return ((long)(read_c0_count() - cnt) > 0L) ? -ETIME : 0;
> +}
> +
> +static void cp0_set_mode(enum clock_event_mode mode,
> +			     struct clock_event_device *evt)
> +{
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_UNUSED:
> +	case CLOCK_EVT_MODE_SHUTDOWN:
> +		/*
> +		 * For now, we don't disable cp0 hpt interrupts. So we

    A reference to hpt needs to be killed.

> +		 * leave them enabled, and ignore them in this mode.
> +		 * Therefore we will get one useless but also harmless
> +		 * interrupt every 2^32 cycles...
> +		 */
> +		cp0_count_ack();
> +		break;
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		/* nothing to do */
> +		break;
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		BUG();
> +	};
> +}
> +
> +static struct clock_event_device cp0_clockevent __initdata = {
> +	.name		= CP0_CLOCK_NAME,
> +	.mode		= CLOCK_EVT_MODE_UNUSED,

    Needless intialization, that constant is 0 anyway, so this field will just 
get zeroed implicitly.

> +	.features	= CLOCK_EVT_FEAT_ONESHOT,
> +	.shift		= 32,
> +	.set_mode	= cp0_set_mode,
> +	.set_next_event	= cp0_set_next_event,
> +	.irq		= -1,
> +};
> +
> +static DEFINE_PER_CPU(struct clock_event_device, cp0_clock_events);
> +
> +void __init setup_cp0_clockevent(void)
> +{
> +	struct clock_event_device *evt;
> +	int cpu = smp_processor_id();
> +	unsigned freq;
> +
> +	if (disable_clockevent)
> +		return;
> +
> +	evt = &__get_cpu_var(cp0_clock_events);

    Could use per_cpu() here, as we already have "sampled" smp_processor_id().

> +
> +	memcpy(evt, &cp0_clockevent, sizeof(*evt));
> +
> +	freq = get_freq(cpu);
> +
> +	evt->rating = 200 + freq/10000000;
> +	evt->mult = div_sc(freq, NSEC_PER_SEC, evt->shift);
> +	evt->cpumask = cpumask_of_cpu(cpu);
> +
> +	evt->max_delta_ns = clockevent_delta2ns(0x7fffffff, evt);
> +	evt->min_delta_ns = clockevent_delta2ns(0x10, evt);
> +
> +	clockevents_register_device(evt);
> +
> +	printk("Using %u.%03u MHz CP0 high precision timer on CPU #%d.\n",

    The "high precision " part should disapper.

> +	       ((freq + 500) / 1000) / 1000,
> +	       ((freq + 500) / 1000) % 1000,
> +		cpu);
> +}
> +
> +static irqreturn_t cp0_clockevent_interrupt(int irq, void *dev_id)
> +{
> +	const int r2 = cpu_has_mips_r2;
> +	struct clock_event_device *evt;
> +
[...]
> +	/*
> +	 * The same applies to performance counter interrupts.  But with the
> +	 * above we now know that the reason we got here must be a timer
> +	 * interrupt.  Being the paranoiacs we are we check anyway.
> +	 */
> +	if (!r2 || (read_c0_cause() & (1 << 30))) {
> +		evt = &__get_cpu_var(cp0_clock_events);

    I'd think 'evt' should be declared in this block too, not at the function 
level.

> +
> +		/*
> +		 * We can get interrupts whereas the hpt clock event

    A reference to hpt needs to be killed there too...

> +		 * device has been disabled since we can't shut it
> +		 * down. So always ack the timer.
> +		 */
> +		cp0_count_ack();
> +
> +		switch (evt->mode) {
> +		case CLOCK_EVT_MODE_ONESHOT:
> +			evt->event_handler(evt);
> +			break;
> +		case CLOCK_EVT_MODE_UNUSED:
> +		case CLOCK_EVT_MODE_SHUTDOWN:
> +		case CLOCK_EVT_MODE_PERIODIC:
> +			/* nothing */;
> +		}

    Isn't this over-engineered? Why use *switch*' where the simple *if* would 
suffice?

> +	}
> +out:
> +	return IRQ_HANDLED;
> +}
> +
> +static struct irqaction cp0_clockevent_irqaction = {
> +	.handler	= cp0_clockevent_interrupt,
> +	.flags		= IRQF_DISABLED | IRQF_PERCPU,
> +	.name		= CP0_CLOCK_NAME,
> +};
> +
> +
> +/*
> + * This function is used by platforms which use the hpt as clock
> + * source and timer.
> + */
> +int __init setup_cp0_clocks(struct cp0_clock_info *info)
> +{

    Erm, do we need this function at all after we have separate setups for 
clock source/event?  Just move all the override checks there.

> +	if (!cpu_has_counter)
> +		goto disable_all;
> +	if (info->get_freq == NULL)
> +		goto disable_all;
> +
> +	get_freq = info->get_freq;
> +	perf_handler = info->perf_handler;

    I seriously don't understand why are you expecting this to be passed by 
the platform code... :-/  Currently this is not so -- since it's the Oprofile 
code that handles the performance interrupt.

> +
> +	if (info->irq == 0)
> +		disable_clockevent = 1;
> +
> +	if (!disable_clocksource)
> +		setup_cp0_clocksource();
> +	if (!disable_clockevent) {
> +		setup_cp0_clockevent();
> +		setup_irq(info->irq, &cp0_clockevent_irqaction);

    Erm... why not include setup_irq() into setup_cp0_clockevent()? This way, 
it'll be autonomous and callable by the platfrom code on its own... but there 
would remain perf_irq and get_freq problems. Wouldn't it be better to declare 
get_cp0_timer_freq as a pointer ot function and let the platform just fill it 
before calling setup?

> +	}
> +	return 0;
> +
> +disable_all:
> +	disable_clocksource = disable_clockevent = 1;
> +	return -EINVAL;
> +}
> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
> index 6bdfb5a..23b8858 100644
> --- a/arch/mips/kernel/process.c
> +++ b/arch/mips/kernel/process.c
> @@ -25,6 +25,7 @@
>  #include <linux/init.h>
>  #include <linux/completion.h>
>  #include <linux/kallsyms.h>
> +#include <linux/tick.h>
>  
>  #include <asm/bootinfo.h>
>  #include <asm/cpu.h>
> @@ -50,6 +51,7 @@ ATTRIB_NORET void cpu_idle(void)
>  {
>  	/* endless idle loop with no priority at all */
>  	while (1) {
> +		tick_nohz_stop_sched_tick();
>  		while (!need_resched()) {
>  #ifdef CONFIG_SMTC_IDLE_HOOK_DEBUG
>  			extern void smtc_idle_loop_hook(void);
> @@ -59,6 +61,7 @@ ATTRIB_NORET void cpu_idle(void)
>  			if (cpu_wait)
>  				(*cpu_wait)();
>  		}
> +		tick_nohz_restart_sched_tick();
>  		preempt_enable_no_resched();
>  		schedule();
>  		preempt_disable();
> diff --git a/arch/mips/kernel/time.c b/arch/mips/kernel/time.c
> index 72df0bf..0cc8363 100644
> --- a/arch/mips/kernel/time.c
> +++ b/arch/mips/kernel/time.c
[...]
> + * If you don't know timer 'X' frequency and have another timer 'Y'
> + * that flips at HZ rate, you can use this helper to determinate the
> + * timer 'X' freq.
>   */
> -
> -unsigned int mips_hpt_frequency;
> -
> -static struct irqaction timer_irqaction = {
> -	.handler = timer_interrupt,
> -	.flags = IRQF_DISABLED | IRQF_PERCPU,
> -	.name = "timer",
> -};
> -
> -static unsigned int __init calibrate_hpt(void)
> +unsigned __init calibrate_timer(cycle_t (*x_read)(void),
> +				int (*y_state)(void))

    Hm, with those x/y it looks a bit like joystick code. :-)

> diff --git a/include/asm-mips/hpt.h b/include/asm-mips/hpt.h
> new file mode 100644
> index 0000000..f0acab3
> --- /dev/null
> +++ b/include/asm-mips/hpt.h
> @@ -0,0 +1,36 @@
> +#ifndef _ASM_HPT_H
> +#define _ASM_HPT_H
> +
> +#ifdef CONFIG_CP0_CLOCKS
> +
> +struct cp0_clock_info {
> +	/*
> +	 * This is the irq num of the cp0 count/compare timer.
> +	 */
> +	int irq;
> +
> +	/*
> +	 * This mandartory hook is called to get the frequency of
> +	 * the running processor.
> +	 */
> +	unsigned (*get_freq)(int cpu);

    Creating a global variable looks like a better idea...

> +
> +	/*
> +	 * The performance counter overflow irq may be shared with the
> +	 * hpt interrupt. In that case this handler will be called
> +	 * during a hpt interrupt.
> +	 */
> +	irqreturn_t (*perf_handler)(int irq, void *dev_id);

    The only issue is that the platform code may have no idea what it should 
be...  I think we need to stick to the old approach here.

> +};

    ... this would leave us with only IRQ field and eliminate the need for 
this structure.

> +
> +extern int setup_cp0_clocks(struct cp0_clock_info *info);
> +extern void setup_cp0_clockevent(void);
> +
> +#else
> +
> +static inline void setup_cp0_clockevent(void) {}
> +
> +#endif	/* CONFIG_CP0_CLOCKS */
> +
> +#endif	/* _ASM_HPT_H */

    The remaining declaration might be joined to time.h, thus making this 
whole header unneeded... well, that's up to you.

WBR, Sergei