Re: [PATCH v6 4/6] clocksource/drivers/timer-riscv: Use per-CPU timer interrupt

[Marc Zyngier]

On 2020-05-30 11:07, Anup Patel wrote:

Instead of directly calling RISC-V timer interrupt handler from
RISC-V local interrupt conntroller driver, this patch implements
RISC-V timer interrupt as a per-CPU interrupt using per-CPU APIs
of Linux IRQ subsystem.

Signed-off-by: Anup Patel <anup.pa...@wdc.com>
Reviewed-by: Atish Patra <atish.pa...@wdc.com>
---
 arch/riscv/include/asm/irq.h      |  2 --
 drivers/clocksource/timer-riscv.c | 41 ++++++++++++++++++++++++++++---
 drivers/irqchip/irq-riscv-intc.c  |  8 ------
 3 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h

index a9e5f07a7e9c..9807ad164015 100644
--- a/arch/riscv/include/asm/irq.h
+++ b/arch/riscv/include/asm/irq.h
@@ -10,8 +10,6 @@
 #include <linux/interrupt.h>
 #include <linux/linkage.h>

-void riscv_timer_interrupt(void);
-
 #include <asm-generic/irq.h>

 #endif /* _ASM_RISCV_IRQ_H */
diff --git a/drivers/clocksource/timer-riscv.c
b/drivers/clocksource/timer-riscv.c
index c4f15c4068c0..1fe847983f50 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -12,8 +12,11 @@
 #include <linux/cpu.h>
 #include <linux/delay.h>
 #include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/sched_clock.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/interrupt.h>
+#include <linux/of_irq.h>
 #include <asm/smp.h>
 #include <asm/sbi.h>

@@ -39,6 +42,7 @@ static int riscv_clock_next_event(unsigned long delta,

        return 0;
 }

+static unsigned int riscv_clock_event_irq;

static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {

        .name                   = "riscv_timer_clockevent",
        .features               = CLOCK_EVT_FEAT_ONESHOT,

@@ -74,30 +78,36 @@ static int riscv_timer_starting_cpu(unsigned int cpu)

        struct clock_event_device *ce = per_cpu_ptr(&riscv_clock_event, cpu);

        ce->cpumask = cpumask_of(cpu);
+       ce->irq = riscv_clock_event_irq;
        clockevents_config_and_register(ce, riscv_timebase, 100, 0x7fffffff);

-       csr_set(CSR_IE, IE_TIE);
+       enable_percpu_irq(riscv_clock_event_irq,
+                         irq_get_trigger_type(riscv_clock_event_irq));
        return 0;
 }

 static int riscv_timer_dying_cpu(unsigned int cpu)
 {
-       csr_clear(CSR_IE, IE_TIE);
+       disable_percpu_irq(riscv_clock_event_irq);
        return 0;
 }

 /* called directly from the low-level interrupt handler */
-void riscv_timer_interrupt(void)
+static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id)
 {
        struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event);

        csr_clear(CSR_IE, IE_TIE);
        evdev->event_handler(evdev);
+
+       return IRQ_HANDLED;
 }

 static int __init riscv_timer_init_dt(struct device_node *n)
 {
        int cpuid, hartid, error;
+       struct device_node *child;
+       struct irq_domain *domain;

        hartid = riscv_of_processor_hartid(n);
        if (hartid < 0) {
@@ -115,6 +125,23 @@ static int __init riscv_timer_init_dt(struct
device_node *n)
        if (cpuid != smp_processor_id())
                return 0;

+       domain = NULL;
+       for_each_child_of_node(n, child) {
+               domain = irq_find_host(child);
+               if (domain)
+                       break;
+       }

This is a bit clumsy, and probably better written as:

        child = of_get_compatible(n, "riscv,cpu-intc");
        if (!child) { error out }
        domain = irq_find_host(child);

It would be even better if each CPU would have its per-CPU interrupt
controller designated as such (with an interrupt-parent property),
meaning that you wouldn't have to do anything at all.

Too late for that anyway.

        M.
--
Jazz is not dead. It just smells funny...