Hi,
Thanks for considering.
Sergei Shtylyov wrote:
@@ -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...
TIMER is confusing with timers implemented in kernel/timer.c
Maybe ?
CP0_HRT,
CP0_CLOCKS,
+obj-$(CONFIG_CP0_HPT_TIMER) += hpt-cp0.o
cp0-timer.o here too.
ditto.
+
+#define MIPS_HPT_NAME "cp0-hpt"
I'd named it "cp0-timer" or something.
ditto
+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...
What about this other alternative ?
static void cp0_hpt_set_mode(enum clock_event_mode mode,
struct clock_event_device *evt)
{
switch (mode) {
case CLOCK_EVT_MODE_UNUSED:
free_irq(hpt_irq, NULL);
break;
case CLOCK_EVT_MODE_SHUTDOWN:
cp0_hpt_ack();
break;
case CLOCK_EVT_MODE_ONESHOT:
setup_irq(hpt_irq, &hpt_irqaction);
break;
case CLOCK_EVT_MODE_PERIODIC:
BUG();
};
}
+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...
If you don't mind I prefer let it as is. I think it's more readable and
the second comment right before the "if (!r2)" condition can be well
placed.
+
+ /*
+ * 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. :-)
Sorry I don't see what you mean. It's an interrupt handler, what
do you mean by "upper level code" ?
But maybe CLOCK_EVT_MODE_UNUSED should also be checked?
yes. Actually this test should be:
if (likely(cd->mode == CLOCK_EVT_MODE_ONESHOT))
...
+ 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.
agreed.
+ 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...
OK.
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.
Alright.
+ /* FIXME: should we add a disable_irq method ? */
Couldn't it be handled in somegeneric way?
Not really, or at least there're some configs where you can't. See
this thread from:
http://marc.info/?l=linux-mips&m=118121616820659&w=2
That said maybe we can provide a default disable_irq() that would
disable CP0 hpt irq from CP0. That would be the 'generic' way. And
we still give the possibility to override it through the cp0_hpt_info
structure. Of course the same thing would be needed to enable CP0
hpt interrupts.
What do you think ?
+ 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?
As the comment above tries to explain, the cp0 hpt interrupts can
be shared with the perf interrupts on some systems.
Should I rephrase the comment ?
+};
+
+
+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.
Just a matter of coding style. The same happens with "unsigned" and
"unsigned int"...
Thanks again,
--
Franck
