[RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt

[marc.zyngier@arm.com (Marc Zyngier)]

On 03/07/18 19:28, Atish Patra wrote:
> On 7/2/18 1:32 AM, Marc Zyngier wrote:
>> On 29/06/18 19:01, Atish Patra wrote:
>>> Currently, ther timer interrupt handleri(riscv_timer_interrupt())
>>> is invoked directly from the arch handler. This
>>> approach creates interwinding of driver and architecture
>>> code which is not ideal.
>>>
>>> Register timer interrupt as a per cpu based irq behind
>>> generic Linux IRQ infrastructure. As the local timer node
>>> is directly connected to individual harts, HLIC acts as
>>> the parent irq domain.
>>>
>>> This patch requires necessary bbl changes that adds timer
>>> node in device tree which can be found here.
>>>
>>> https://github.com/riscv/riscv-pk/pull/112
>>>
>>> Before the patch
>>> ---------------------------------------------------
>>>       CPU1 CPU2 CPU3 CPU4
>>>   21: 12    8    6    5  riscv,plic0,c000000  53  eth0
>>>   37: 0     0    0    0  riscv,plic0,c000000  32  xilinx-pcie
>>>   43: 0     0    0    0  riscv,plic0,c000000   4  10010000.serial
>>>   44: 0     0    0    0  riscv,plic0,c000000   5  10011000.serial
>>>   45: 0     0    0    0  riscv,plic0,c000000  51  10040000.spi
>>>   46: 0     0    0    0  riscv,plic0,c000000  52  10041000.spi
>>>   47: 25    1    26   50  riscv,plic0,c000000   6  10050000.spi
>>>
>>> After the patch
>>> ---------------------------------------------------
>>>        CPU1 CPU2 CPU3 CPU4
>>>    5:  271   0    0    0    riscv,cpu_intc,1   5  local_timer
>>>    6:  0     307  0    0    riscv,cpu_intc,2   5  local_timer
>>>    7:  0     0    303  0    riscv,cpu_intc,3   5  local_timer
>>>    8:  0     0    0    223  riscv,cpu_intc,4   5  local_timer
>>>   47:  337   489  389  35   riscv,plic0,c000000   4  10010000.serial
>>>   48:  0     0    0    0    riscv,plic0,c000000   5  10011000.serial
>>>   49:  0     0    0    0    riscv,plic0,c000000  51  10040000.spi
>>>   50:  0     0    0    0    riscv,plic0,c000000  52  10041000.spi
>>>   51:  2     14   47   39   riscv,plic0,c000000   6  10050000.spi
>>>
>>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>>> ---
>>>   arch/riscv/include/asm/irq.h      |  2 --
>>>   arch/riscv/kernel/time.c          | 21 --------------
>>>   drivers/clocksource/riscv_timer.c | 61 ++++++++++++++++++++++++++++++++++++---
>>>   drivers/irqchip/irq-riscv-intc.c  | 11 ++++---
>>>   4 files changed, 64 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
>>> index 4dee9d4..2d692d0 100644
>>> --- a/arch/riscv/include/asm/irq.h
>>> +++ b/arch/riscv/include/asm/irq.h
>>> @@ -21,8 +21,6 @@
>>>   #define INTERRUPT_CAUSE_TIMER       5
>>>   #define INTERRUPT_CAUSE_EXTERNAL    9
>>>   
>>> -void riscv_timer_interrupt(void);
>>> -
>>>   #include <asm-generic/irq.h>
>>>   
>>>   #endif /* _ASM_RISCV_IRQ_H */
>>> diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
>>> index 94e220e..fbdfec5 100644
>>> --- a/arch/riscv/kernel/time.c
>>> +++ b/arch/riscv/kernel/time.c
>>> @@ -24,27 +24,6 @@
>>>   
>>>   unsigned long riscv_timebase;
>>>   
>>> -DECLARE_PER_CPU(struct clock_event_device, riscv_clock_event);
>>> -
>>> -void riscv_timer_interrupt(void)
>>> -{
>>> -#ifdef CONFIG_RISCV_TIMER
>>> -	/*
>>> -	 * FIXME: This needs to be cleaned up along with the rest of the IRQ
>>> -	 * handling cleanup.  See irq.c for more details.
>>> -	 */
>>> -	struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event);
>>> -
>>> -	/*
>>> -	 * There are no direct SBI calls to clear pending timer interrupt bit.
>>> -	 * Disable timer interrupt to ignore pending interrupt until next
>>> -	 * interrupt.
>>> -	 */
>>> -	csr_clear(sie, SIE_STIE);
>>> -	evdev->event_handler(evdev);
>>> -#endif
>>> -}
>>> -
>>>   static long __init timebase_frequency(void)
>>>   {
>>>   	struct device_node *cpu;
>>> diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
>>> index f4147db..a968a69 100644
>>> --- a/drivers/clocksource/riscv_timer.c
>>> +++ b/drivers/clocksource/riscv_timer.c
>>> @@ -15,9 +15,12 @@
>>>   #include <linux/clocksource.h>
>>>   #include <linux/clockchips.h>
>>>   #include <linux/delay.h>
>>> +#include <linux/interrupt.h>
>>>   #include <linux/timer_riscv.h>
>>>   #include <linux/sched_clock.h>
>>>   #include <linux/cpu.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_irq.h>
>>>   #include <asm/sbi.h>
>>>   
>>>   #define MINDELTA 100
>>> @@ -73,6 +76,23 @@ DEFINE_PER_CPU(struct clocksource, riscv_clocksource) = {
>>>   	.read = rdtime,
>>>   };
>>>   
>>> +static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id)
>>> +{
>>> +	struct clock_event_device *evdev = dev_id;
>>> +#ifdef CONFIG_RISCV_TIMER
>>
>> Why the #ifdef? It is always selected at the architecture level.
>>
> 
> Correct. It was present in the existing code.
> I will remove it.
> 
> 
>>> +
>>> +	/*
>>> +	 * There are no direct SBI calls to clear pending timer interrupt bit.
>>> +	 * Disable timer interrupt to ignore pending interrupt until next
>>> +	 * interrupt.
>>> +	 */
>>> +	csr_clear(sie, SIE_STIE);
>>> +	evdev->event_handler(evdev);
>>> +#endif
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +
>>>   static int hart_of_timer(struct device_node *dev)
>>>   {
>>>   	u32 hart;
>>> @@ -100,14 +120,17 @@ static int timer_riscv_starting_cpu(unsigned int cpu)
>>>   	clockevents_config_and_register(ce, riscv_timebase, MINDELTA, MAXDELTA);
>>>   	/* Enable timer interrupt for this cpu */
>>>   	csr_set(sie, SIE_STIE);
>>> +	enable_percpu_irq(ce->irq, IRQ_TYPE_NONE);
>>>   
>>>   	return 0;
>>>   }
>>>   
>>>   static int timer_riscv_dying_cpu(unsigned int cpu)
>>>   {
>>> +	struct clock_event_device *ce = per_cpu_ptr(&riscv_clock_event, cpu);
>>>   	/* Disable timer interrupt for this cpu */
>>>   	csr_clear(sie, SIE_STIE);
>>> +	disable_percpu_irq(ce->irq);
>>>   
>>>   	return 0;
>>>   }
>>> @@ -115,9 +138,36 @@ static int timer_riscv_dying_cpu(unsigned int cpu)
>>>   static int __init timer_riscv_init_dt(struct device_node *n)
>>>   {
>>>   	int err = 0;
>>> -	int cpu_id = hart_of_timer(n);
>>> -	struct clocksource *cs = per_cpu_ptr(&riscv_clocksource, cpu_id);
>>> +	int cpu_id, timer_int;
>>> +	struct device_node *parent;
>>> +	struct clocksource *cs;
>>> +	struct clock_event_device *ce;
>>> +
>>> +	timer_int = irq_of_parse_and_map(n, 0);
>>> +	if (!timer_int) {
>>> +		pr_err("Unable to find local timer irq\n");
>>> +		return -EINVAL;
>>> +	}
>>>   
>>> +	parent = of_get_parent(n);
>>> +	if (!parent) {
>>> +		pr_err("Parent of timer node doesn't exist\n");
>>> +		return -EINVAL;
>>> +	}
>>> +	cpu_id = hart_of_timer(parent);
>>> +
>>> +	cs = per_cpu_ptr(&riscv_clocksource, cpu_id);
>>> +	ce = per_cpu_ptr(&riscv_clock_event, cpu_id);
>>> +	ce->irq = timer_int;
>>> +
>>> +	err = request_percpu_irq(ce->irq, riscv_timer_interrupt,
>>> +				 "local_timer", &riscv_clock_event);
>>> +	if (err) {
>>> +		pr_err("local timer can't register for interrupt [%d] [%d]\n",
>>> +			timer_int, err);
>>> +		free_percpu_irq(ce->irq, ce);
>>> +		return err;
>>> +	}
>>>   	if (cpu_id == smp_processor_id()) {
>>>   		clocksource_register_hz(cs, riscv_timebase);
>>>   		sched_clock_register(timer_riscv_sched_read, 64, riscv_timebase);
>>> @@ -125,11 +175,14 @@ static int __init timer_riscv_init_dt(struct device_node *n)
>>>   		err = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
>>>   			 "clockevents/riscv/timer:starting",
>>>   			 timer_riscv_starting_cpu, timer_riscv_dying_cpu);
>>> -		if (err)
>>> +		if (err) {
>>>   			pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
>>>   			       err, cpu_id);
>>> +			free_percpu_irq(ce->irq, ce);
>>> +			return err;
>>> +		}
>>>   	}
>>>   	return err;
>>>   }
>>>   
>>> -TIMER_OF_DECLARE(riscv_timer, "riscv", timer_riscv_init_dt);
>>> +TIMER_OF_DECLARE(riscv_timer, "riscv,local-timer", timer_riscv_init_dt);
>>> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
>>> index fec897d..fda174f 100644
>>> --- a/drivers/irqchip/irq-riscv-intc.c
>>> +++ b/drivers/irqchip/irq-riscv-intc.c
>>> @@ -75,9 +75,6 @@ void riscv_intc_irq(struct pt_regs *regs)
>>>   	 * device interrupts use the generic IRQ mechanisms.
>>>   	 */
>>>   	switch (cause) {
>>> -	case INTERRUPT_CAUSE_TIMER:
>>> -		riscv_timer_interrupt();
>>> -		break;
>>>   	case INTERRUPT_CAUSE_SOFTWARE:
>>>   		riscv_software_interrupt();
>>>   		break;
>>> @@ -96,7 +93,13 @@ static int riscv_irqdomain_map(struct irq_domain *d, unsigned int irq,
>>>   {
>>>   	struct riscv_irq_data *data = d->host_data;
>>>   
>>> -	irq_set_chip_and_handler(irq, &data->chip, handle_simple_irq);
>>> +	if (hwirq == INTERRUPT_CAUSE_TIMER) {
>>> +		irq_set_percpu_devid(irq);
>>
>> If you're making the interrupt a percpu_devid, why are you using it as a
>> per-cpu interrupt, and not a percpu_devid? That's just wrong.
>>
> 
> I am not quite sure if I understand your point clearly. I tried to 
> follow PPI interrupt code in ARM.
> 
>> Making it a percpu_devid looks like the right thing to do (all the timer
>> interrupts share the same hwirq). Please fix the timer code accordingly.
>>
> 
> I looked at the timer code in arm. Both global & arch timer uses per cpu 
> interrupts by parsing DT using irq_of_parse_and_map().
> 
> I might have missed something here. Can you please explain a bit more or 
> point to me some existing code?

There is clearly something wrong with the way interrupts are numbered:

>>>        CPU1 CPU2 CPU3 CPU4
>>>    5:  271   0    0    0    riscv,cpu_intc,1   5  local_timer
>>>    6:  0     307  0    0    riscv,cpu_intc,2   5  local_timer
>>>    7:  0     0    303  0    riscv,cpu_intc,3   5  local_timer
>>>    8:  0     0    0    223  riscv,cpu_intc,4   5  local_timer

On arm/arm64, we end-up with:

 19:       4151       3859     GICv2  30 Level     arch_timer

--> A single interrupt number common across all CPUs.

It looks like the issue might well be with the way the interrupt
controller handles those (I suspect it uses a domain per CPU for the
local interrupts, so the whole concept breaks down badly).

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