Re: [PATCH 3/5] Deforest the function pointer jungle in the time code.

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

Hello.

Franck Bui-Huu wrote:

> Subject: [PATCH 6/6] Implement clockevents for R4000-style cp0 timer
[...]
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 7bcf38d..d852cb0 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -723,6 +723,14 @@ config GENERIC_TIME
>  	bool
>  	default y
>  
> +config GENERIC_CLOCKEVENTS
> +	bool
> +	default y
> +
> +config CP0_HPT_TIMER

    I'd suggest just CP0_TIMER...

> +	bool
> +	default y
> +
>  config GENERIC_CMOS_UPDATE
>  	bool
>  	default y
[...]
> diff --git a/arch/mips/kernel/Makefile b/arch/mips/kernel/Makefile
> index 4924626..ffd4352 100644
> --- a/arch/mips/kernel/Makefile
> +++ b/arch/mips/kernel/Makefile
> @@ -11,6 +11,8 @@ obj-y		+= cpu-probe.o branch.o entry.o genex.o irq.o process.o \
>  binfmt_irix-objs	:= irixelf.o irixinv.o irixioctl.o irixsig.o	\
>  			   irix5sys.o sysirix.o
>  
> +obj-$(CONFIG_CP0_HPT_TIMER)	+= hpt-cp0.o

    cp0-timer.o here too.

> +
>  obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
>  obj-$(CONFIG_MODULES)		+= mips_ksyms.o module.o
>  
> diff --git a/arch/mips/kernel/hpt-cp0.c b/arch/mips/kernel/hpt-cp0.c
> new file mode 100644
> index 0000000..8581a20
> --- /dev/null
> +++ b/arch/mips/kernel/hpt-cp0.c
> @@ -0,0 +1,248 @@
> +/*
> + * This is a driver for CP0 hpt.
> + */
> +#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 MIPS_HPT_NAME	"cp0-hpt"

    I'd named it "cp0-timer" or something.

> +/*
> + * High precision timer functions
> + */
> +
> +static int cp0_hpt_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 */
> +	cnt = read_c0_count();
> +	cnt += delta;
> +	write_c0_compare(cnt);
> +
> +	return ((long)(read_c0_count() - cnt) > 0L) ? -ETIME : 0;
> +}
> +
> +static void cp0_hpt_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 can't disable cp0 hpt interrupts. So we
> +		 * leave them enabled, and ignore them in this mode.
> +		 * Therefore we will get one useless but also harmless
> +		 * interrupt every 2^32 cycles...
> +		 */
> +		cp0_hpt_ack();

    Good idea...

> +		break;
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		/* nothing to do */
> +		break;
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		BUG();
> +	};
> +}
> +
> +static struct clock_event_device hpt_clockevent __initdata = {
> +	.name		= MIPS_HPT_NAME,
> +	.mode		= CLOCK_EVT_MODE_UNUSED,
> +	.features	= CLOCK_EVT_FEAT_ONESHOT,
> +	.shift		= 32,
> +	.set_mode	= cp0_hpt_set_mode,
> +	.set_next_event	= cp0_hpt_set_next_event,
> +	.irq		= -1,
> +};
> +
> +static DEFINE_PER_CPU(struct clock_event_device, cp0_hpt_clock_events);

   Oh, these are declared per-CPU at last...

> +static irqreturn_t cp0_hpt_interrupt(int irq, void *dev_id)
> +{
> +	const int r2 = cpu_has_mips_r2;
> +	struct clock_event_device *cd;
> +
> +	/*
> +	 * Suckage alert:
> +	 * Before R2 of the architecture there was no way to see if a
> +	 * performance counter interrupt was pending, so we have to run
> +	 * the performance counter interrupt handler anyway.
> +	 */
> +	if (perf_handler && perf_handler(irq, dev_id) == IRQ_HANDLED)
> +		/*
> +		 * The performance counter overflow interrupt may be
> +		 * shared with the timer interrupt. If it is (!r2)
> +		 * then we can't reliably determine if a counter
> +		 * interrupt has also happened. So don't check for a
> +		 * timer interrupt in this case.
> +		 */
> +		if (!r2)
> +			goto out;

    Might be folded into one if stmt...

> +
> +	/*
> +	 * 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))) {
> +		/*
> +		 * We can get interrupts whereas the hpt clock event
> +		 * device has been disabled since we can't shut it
> +		 * down. So always ack the timer.
> +		 */
> +		cp0_hpt_ack();
> +
> +		cd = &__get_cpu_var(cp0_hpt_clock_events);
> +		if (likely(cd->mode != CLOCK_EVT_MODE_SHUTDOWN))

    Hm, I thought the upper level code takes care of this case... well, it
might have in 2.6.18 time. :-)
    But maybe CLOCK_EVT_MODE_UNUSED should also be checked?

> +			cd->event_handler(cd);
> +	}
> +out:
> +	return IRQ_HANDLED;
> +}
> +
> +struct irqaction hpt_irqaction = {
> +	.handler	= cp0_hpt_interrupt,
> +	.flags		= IRQF_DISABLED | IRQF_PERCPU,
> +	.name		= MIPS_HPT_NAME,
> +};

> +/*
> + * This function is used by platforms which use the hpt as clock
> + * source and timer.
> + */
> +int __init setup_cp0_hpt(struct cp0_hpt_info *info)
> +{
> +	if (cp0_hpt_disabled)
> +		goto out;
> +	if (!cpu_has_counter)
> +		goto disable;
> +
> +	if (info->irq == 0)
> +		goto disable;

    Shouldn't harm clocksource, in theory.

> +	if (info->get_freq == NULL)
> +		goto disable;
> +
> +	cp0_hpt_get_freq = info->get_freq;
> +	perf_handler = info->perf_handler;
> +
> +	setup_cp0_hpt_clocksource();
> +	setup_cp0_hpt_clockevent();

    Probably not both. It would have been the best thing to have the separate
init. functions...

> diff --git a/include/asm-mips/hpt.h b/include/asm-mips/hpt.h
> new file mode 100644
> index 0000000..2b62827
> --- /dev/null
> +++ b/include/asm-mips/hpt.h
> @@ -0,0 +1,30 @@
> +#ifndef _ASM_HPT_H
> +#define _ASM_HPT_H
> +
> +#ifdef CONFIG_CP0_HPT_TIMER
> +
> +struct cp0_hpt_info {

    Not sure if we need the structure at this point at all...

> +	/* FIXME: could we let the user override hpt ops ? */

    No.

> +	/* FIXME: should we add a disable_irq method ? */

    Couldn't it be handled in somegeneric way?

> +	int		irq;
> +	unsigned	(*get_freq)(int cpu);
> +
> +	/*
> +	 * 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);

    Hm... what it's doing here, in this structure?

> +};
> +
> +
> +extern int setup_cp0_hpt(struct cp0_hpt_info *info);
> +extern void setup_cp0_hpt_clockevent(void);

    No explicit 'extern' needed for functions -- they all have that memory 
class by deafult.

WBR, Sergei