* [RFC PATCH 0/2] Local timer interrupt fix @ 2018-06-29 18:01 Atish Patra 2018-06-29 18:01 ` [RFC PATCH 1/2] dt-binding: RISC-V local timer docs Atish Patra 2018-06-29 18:01 ` [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt Atish Patra 0 siblings, 2 replies; 20+ messages in thread From: Atish Patra @ 2018-06-29 18:01 UTC (permalink / raw) To: linux-riscv This patchset introduces per cpu based local timer interrupt. 1st patch is device tree documentation while the 2nd patch contains the actual interrupt registration and handling. It requires device tree modifications as well which is available at https://github.com/riscv/riscv-pk/pull/112 FYI: These patches alone without bbl changes will fail to boot. Atish Patra (2): dt-binding: RISC-V local timer docs. riscv: Introduce per cpu based local timer interrupt Documentation/devicetree/bindings/riscv/timer.txt | 35 +++++++++++++ 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 ++-- 5 files changed, 99 insertions(+), 31 deletions(-) create mode 100644 Documentation/devicetree/bindings/riscv/timer.txt -- 2.7.4 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 1/2] dt-binding: RISC-V local timer docs. 2018-06-29 18:01 [RFC PATCH 0/2] Local timer interrupt fix Atish Patra @ 2018-06-29 18:01 ` Atish Patra 2018-07-02 8:22 ` Marc Zyngier 2018-06-29 18:01 ` [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt Atish Patra 1 sibling, 1 reply; 20+ messages in thread From: Atish Patra @ 2018-06-29 18:01 UTC (permalink / raw) To: linux-riscv This patch adds documentation for the RISC-V local timer node which defines per-hart based timer interrupts. This is specified by RISC-V supervisor manual. Signed-off-by: Atish Patra <atish.patra@wdc.com> --- Documentation/devicetree/bindings/riscv/timer.txt | 35 +++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 Documentation/devicetree/bindings/riscv/timer.txt diff --git a/Documentation/devicetree/bindings/riscv/timer.txt b/Documentation/devicetree/bindings/riscv/timer.txt new file mode 100644 index 0000000..8dcc930 --- /dev/null +++ b/Documentation/devicetree/bindings/riscv/timer.txt @@ -0,0 +1,35 @@ +* RISC-V local timer + +RISC-V supervisor manual specifies that timer interrupt is connected to +individual harts directly to minimize the interrupt latency. These per +hart timer is connected via Hart Level Interrupt Controller (HLIC). The +HLIC will also act as the interrupt parent for timer interrupt. + +Required properties: + +- compatible : "riscv,local-timer" +- interrupts : Should be Timer interrupt number. Current supervisor manual + defines it be 5. +- interrupt-parent : Should point to local interrupt-controller phandle. + +Example: + cpus { + #address-cells = <0x00000001>; + .. + .. + cpu at 0 { + .. + .. + timer { + interrupts = <0x00000005>; + interrupt-parent = <0x00000004>; + compatible = "riscv,local-timer"; + } + interrupt-controller { + .. + .. + compatible = "riscv,cpu-intc"; + linux,phandle = <0x00000004>; + phandle = <0x00000004>; + } + } -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [RFC PATCH 1/2] dt-binding: RISC-V local timer docs. 2018-06-29 18:01 ` [RFC PATCH 1/2] dt-binding: RISC-V local timer docs Atish Patra @ 2018-07-02 8:22 ` Marc Zyngier 2018-07-03 18:18 ` Atish Patra 0 siblings, 1 reply; 20+ messages in thread From: Marc Zyngier @ 2018-07-02 8:22 UTC (permalink / raw) To: linux-riscv On 29/06/18 19:01, Atish Patra wrote: > This patch adds documentation for the RISC-V local timer node which > defines per-hart based timer interrupts. This is specified by RISC-V > supervisor manual. > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > --- > Documentation/devicetree/bindings/riscv/timer.txt | 35 +++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > create mode 100644 Documentation/devicetree/bindings/riscv/timer.txt > > diff --git a/Documentation/devicetree/bindings/riscv/timer.txt b/Documentation/devicetree/bindings/riscv/timer.txt > new file mode 100644 > index 0000000..8dcc930 > --- /dev/null > +++ b/Documentation/devicetree/bindings/riscv/timer.txt > @@ -0,0 +1,35 @@ > +* RISC-V local timer > + > +RISC-V supervisor manual specifies that timer interrupt is connected to > +individual harts directly to minimize the interrupt latency. These per > +hart timer is connected via Hart Level Interrupt Controller (HLIC). The > +HLIC will also act as the interrupt parent for timer interrupt. > + > +Required properties: > + > +- compatible : "riscv,local-timer" > +- interrupts : Should be Timer interrupt number. Current supervisor manual > + defines it be 5. > +- interrupt-parent : Should point to local interrupt-controller phandle. > + > +Example: > + cpus { > + #address-cells = <0x00000001>; > + .. > + .. > + cpu at 0 { > + .. > + .. > + timer { > + interrupts = <0x00000005>; > + interrupt-parent = <0x00000004>; > + compatible = "riscv,local-timer"; > + } > + interrupt-controller { > + .. > + .. > + compatible = "riscv,cpu-intc"; > + linux,phandle = <0x00000004>; > + phandle = <0x00000004>; This looks like the output of a decompiling of a dtb. Please document the binding in the way a human being would write it, not how it is compiled by dtc. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 1/2] dt-binding: RISC-V local timer docs. 2018-07-02 8:22 ` Marc Zyngier @ 2018-07-03 18:18 ` Atish Patra 0 siblings, 0 replies; 20+ messages in thread From: Atish Patra @ 2018-07-03 18:18 UTC (permalink / raw) To: linux-riscv On 7/2/18 1:22 AM, Marc Zyngier wrote: > On 29/06/18 19:01, Atish Patra wrote: >> This patch adds documentation for the RISC-V local timer node which >> defines per-hart based timer interrupts. This is specified by RISC-V >> supervisor manual. >> >> Signed-off-by: Atish Patra <atish.patra@wdc.com> >> --- >> Documentation/devicetree/bindings/riscv/timer.txt | 35 +++++++++++++++++++++++ >> 1 file changed, 35 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/riscv/timer.txt >> >> diff --git a/Documentation/devicetree/bindings/riscv/timer.txt b/Documentation/devicetree/bindings/riscv/timer.txt >> new file mode 100644 >> index 0000000..8dcc930 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/riscv/timer.txt >> @@ -0,0 +1,35 @@ >> +* RISC-V local timer >> + >> +RISC-V supervisor manual specifies that timer interrupt is connected to >> +individual harts directly to minimize the interrupt latency. These per >> +hart timer is connected via Hart Level Interrupt Controller (HLIC). The >> +HLIC will also act as the interrupt parent for timer interrupt. >> + >> +Required properties: >> + >> +- compatible : "riscv,local-timer" >> +- interrupts : Should be Timer interrupt number. Current supervisor manual >> + defines it be 5. >> +- interrupt-parent : Should point to local interrupt-controller phandle. >> + >> +Example: >> + cpus { >> + #address-cells = <0x00000001>; >> + .. >> + .. >> + cpu at 0 { >> + .. >> + .. >> + timer { >> + interrupts = <0x00000005>; >> + interrupt-parent = <0x00000004>; >> + compatible = "riscv,local-timer"; >> + } >> + interrupt-controller { >> + .. >> + .. >> + compatible = "riscv,cpu-intc"; >> + linux,phandle = <0x00000004>; >> + phandle = <0x00000004>; > > This looks like the output of a decompiling of a dtb. Please document > the binding in the way a human being would write it, not how it is > compiled by dtc. > I will fix it in v2. Thanks for the review. Regards, Atish > Thanks, > > M. > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-06-29 18:01 [RFC PATCH 0/2] Local timer interrupt fix Atish Patra 2018-06-29 18:01 ` [RFC PATCH 1/2] dt-binding: RISC-V local timer docs Atish Patra @ 2018-06-29 18:01 ` Atish Patra 2018-07-02 8:31 ` Marc Zyngier 1 sibling, 1 reply; 20+ messages in thread From: Atish Patra @ 2018-06-29 18:01 UTC (permalink / raw) To: linux-riscv 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 + + /* + * 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); + irq_set_chip_and_handler(irq, &data->chip, + handle_percpu_devid_irq); + irq_set_status_flags(irq, IRQ_NOAUTOEN); + } else + irq_set_chip_and_handler(irq, &data->chip, handle_simple_irq); irq_set_chip_data(irq, data); irq_set_noprobe(irq); irq_set_affinity(irq, cpumask_of(data->hart)); -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-06-29 18:01 ` [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt Atish Patra @ 2018-07-02 8:31 ` Marc Zyngier 2018-07-03 18:28 ` Atish Patra 2018-08-02 21:53 ` Palmer Dabbelt 0 siblings, 2 replies; 20+ messages in thread From: Marc Zyngier @ 2018-07-02 8:31 UTC (permalink / raw) To: linux-riscv 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. > + > + /* > + * 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. 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. > + irq_set_chip_and_handler(irq, &data->chip, > + handle_percpu_devid_irq); > + irq_set_status_flags(irq, IRQ_NOAUTOEN); > + } else > + irq_set_chip_and_handler(irq, &data->chip, handle_simple_irq); > irq_set_chip_data(irq, data); > irq_set_noprobe(irq); > irq_set_affinity(irq, cpumask_of(data->hart)); > Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-02 8:31 ` Marc Zyngier @ 2018-07-03 18:28 ` Atish Patra 2018-07-04 8:24 ` Marc Zyngier 2018-08-02 21:53 ` Palmer Dabbelt 1 sibling, 1 reply; 20+ messages in thread From: Atish Patra @ 2018-07-03 18:28 UTC (permalink / raw) To: linux-riscv 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? Regards, Atish >> + irq_set_chip_and_handler(irq, &data->chip, >> + handle_percpu_devid_irq); >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >> + } else >> + irq_set_chip_and_handler(irq, &data->chip, handle_simple_irq); >> irq_set_chip_data(irq, data); >> irq_set_noprobe(irq); >> irq_set_affinity(irq, cpumask_of(data->hart)); >> > > Thanks, > > M. > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-03 18:28 ` Atish Patra @ 2018-07-04 8:24 ` Marc Zyngier 2018-07-05 1:55 ` Atish Patra 0 siblings, 1 reply; 20+ messages in thread From: Marc Zyngier @ 2018-07-04 8:24 UTC (permalink / raw) To: linux-riscv 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... ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-04 8:24 ` Marc Zyngier @ 2018-07-05 1:55 ` Atish Patra 2018-07-05 2:17 ` Anup Patel 2018-07-05 7:48 ` Marc Zyngier 0 siblings, 2 replies; 20+ messages in thread From: Atish Patra @ 2018-07-05 1:55 UTC (permalink / raw) To: linux-riscv On 7/4/18 1:25 AM, Marc Zyngier wrote: > 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). > We have different interrupt number(Linux IRQ number) because of per cpu based interrupt controller called as Hart Level Interrupt Controller (HLIC). Some more details are on this patch. https://lkml.org/lkml/2018/6/22/914 Thus, each HLIC registers it's own irq domain. Each timer interrupt in DT defines that CPU's HLIC as it's interrupt parent. Here is an example of DT. + L3: cpus { + #address-cells = <1>; + .. + .. + L9: cpu at 0 { + .. + .. + L10: interrupt-controller { + .. + .. + compatible = "riscv,cpu-intc"; + interrupt-controller; + } + timer { + interrupts = <5>; + interrupt-parent = <&L10>; + compatible = "riscv,local-timer"; + } + } Is this design not acceptable or breaks something? Regards, Atish > M. > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-05 1:55 ` Atish Patra @ 2018-07-05 2:17 ` Anup Patel 2018-07-05 7:48 ` Marc Zyngier 1 sibling, 0 replies; 20+ messages in thread From: Anup Patel @ 2018-07-05 2:17 UTC (permalink / raw) To: linux-riscv Hi Atish, > > We have different interrupt number(Linux IRQ number) because of per cpu > based interrupt controller called as Hart Level Interrupt Controller (HLIC). > Some more details are on this patch. > > https://lkml.org/lkml/2018/6/22/914 > > Thus, each HLIC registers it's own irq domain. Each timer interrupt in DT > defines that CPU's HLIC as it's interrupt parent. Here is an example of DT. > > > + L3: cpus { > + #address-cells = <1>; > + .. > + .. > + L9: cpu at 0 { > + .. > + .. > + L10: interrupt-controller { > + .. > + .. > + compatible = "riscv,cpu-intc"; > + interrupt-controller; > + } > + timer { > + interrupts = <5>; > + interrupt-parent = <&L10>; > + compatible = "riscv,local-timer"; > + } > + } > > Is this design not acceptable or breaks something? I think its more about fully utilising the capabilities of Linux IRQ subsystem. The Linux IRQ subsystem has capability of handling per-CPU IRQs. The best matching case in ARM world (with your use-case) is the RPi3 board where we have local IRQCHIP for each CPU as-well-as timer in each CPU. (Very similar to RISC-V systems). Refer, arch/arm/boot/dts/bcm2837.dtsi I think it would be best to have only one DT node for per-CPU local INTC and per-CPU timer. Regards, Anup ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-05 1:55 ` Atish Patra 2018-07-05 2:17 ` Anup Patel @ 2018-07-05 7:48 ` Marc Zyngier 2018-07-05 15:42 ` hch at infradead.org 1 sibling, 1 reply; 20+ messages in thread From: Marc Zyngier @ 2018-07-05 7:48 UTC (permalink / raw) To: linux-riscv On 05/07/18 02:55, Atish Patra wrote: > On 7/4/18 1:25 AM, Marc Zyngier wrote: >> 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). >> > > We have different interrupt number(Linux IRQ number) because of per cpu > based interrupt controller called as Hart Level Interrupt Controller > (HLIC). Some more details are on this patch. > > https://lkml.org/lkml/2018/6/22/914 > > Thus, each HLIC registers it's own irq domain. Each timer interrupt in > DT defines that CPU's HLIC as it's interrupt parent. Here is an example > of DT. > > > + L3: cpus { > + #address-cells = <1>; > + .. > + .. > + L9: cpu at 0 { > + .. > + .. > + L10: interrupt-controller { > + .. > + .. > + compatible = "riscv,cpu-intc"; > + interrupt-controller; > + } > + timer { > + interrupts = <5>; > + interrupt-parent = <&L10>; > + compatible = "riscv,local-timer"; > + } > + } > > Is this design not acceptable or breaks something? This is not how percpu_devid interrupts are supposed to be used. The expected use case is that you have a single domain, and that the same Linux irq spans all the CPUs. On ARM, we declare a *single* timer that is connected to a *single* interrupt controller, and the distribution is completely implicit. That's what percpu_devid interrupts are for. Here, you have discrete timers, discrete irqchip, and thus individual domains. You might as well keep everything as non-percpu interrupts with a fixed affinity, assuming each HLIC is reachable from any CPU (which cannot universally work on ARM). Or you change the way you describe HLICs and timers, but that's a much more ambitious endeavour, and the gain isn't obvious. Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-05 7:48 ` Marc Zyngier @ 2018-07-05 15:42 ` hch at infradead.org 2018-07-05 15:58 ` Marc Zyngier 2018-07-05 17:15 ` Atish Patra 0 siblings, 2 replies; 20+ messages in thread From: hch at infradead.org @ 2018-07-05 15:42 UTC (permalink / raw) To: linux-riscv On Thu, Jul 05, 2018 at 08:48:23AM +0100, Marc Zyngier wrote: > >> 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). > >> > > > > We have different interrupt number(Linux IRQ number) because of per cpu > > based interrupt controller called as Hart Level Interrupt Controller > > (HLIC). Some more details are on this patch. > > > > https://lkml.org/lkml/2018/6/22/914 > > > > Thus, each HLIC registers it's own irq domain. Each timer interrupt in > > DT defines that CPU's HLIC as it's interrupt parent. Here is an example > > of DT. Btw, for the next iteration of the serie I really think it needs to be against mainline and include the heary level irqchip driver. Basing against an unknown unknown in some riscv tree makes reviews confusing, and we also really need to get riscv irqs and timers upstream ASAP, so we need to drive these together. > This is not how percpu_devid interrupts are supposed to be used. The > expected use case is that you have a single domain, and that the same > Linux irq spans all the CPUs. On ARM, we declare a *single* timer that > is connected to a *single* interrupt controller, and the distribution is > completely implicit. That's what percpu_devid interrupts are for. > > Here, you have discrete timers, discrete irqchip, and thus individual > domains. You might as well keep everything as non-percpu interrupts with > a fixed affinity, assuming each HLIC is reachable from any CPU (which > cannot universally work on ARM). The hart level interrupt controller is based around RISC-V control registers, which per defintion are CPU local. So you can't touch them at all from other cores and any access of the remote 'irqchip' needs and IPI. I'm not an irqchip expert, but sometimes I really wonder if the irqchip framework really is the right abstraction for this. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-05 15:42 ` hch at infradead.org @ 2018-07-05 15:58 ` Marc Zyngier 2018-07-05 22:22 ` hch at infradead.org 2018-08-03 0:14 ` Palmer Dabbelt 2018-07-05 17:15 ` Atish Patra 1 sibling, 2 replies; 20+ messages in thread From: Marc Zyngier @ 2018-07-05 15:58 UTC (permalink / raw) To: linux-riscv On 05/07/18 16:42, hch at infradead.org wrote: > On Thu, Jul 05, 2018 at 08:48:23AM +0100, Marc Zyngier wrote: >> Here, you have discrete timers, discrete irqchip, and thus individual >> domains. You might as well keep everything as non-percpu interrupts with >> a fixed affinity, assuming each HLIC is reachable from any CPU (which >> cannot universally work on ARM). > > The hart level interrupt controller is based around RISC-V control > registers, which per defintion are CPU local. So you can't touch them > at all from other cores and any access of the remote 'irqchip' needs > and IPI. Then this is no different from what we have on ARM with GICv1/v2, where private interrupts are strictly private, and cannot be accessed from another CPU. > I'm not an irqchip expert, but sometimes I really wonder if the irqchip > framework really is the right abstraction for this. Well, it served us pretty well so far. Seems like RISC-V has decided to describe the HW in a subtly (and maybe pointlessly?) different way, rather than reusing the existing abstraction. It is not necessarily bad, but it just makes things more difficult. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-05 15:58 ` Marc Zyngier @ 2018-07-05 22:22 ` hch at infradead.org 2018-07-06 8:25 ` Marc Zyngier 2018-07-06 9:41 ` Thomas Gleixner 2018-08-03 0:14 ` Palmer Dabbelt 1 sibling, 2 replies; 20+ messages in thread From: hch at infradead.org @ 2018-07-05 22:22 UTC (permalink / raw) To: linux-riscv On Thu, Jul 05, 2018 at 04:58:59PM +0100, Marc Zyngier wrote: > Then this is no different from what we have on ARM with GICv1/v2, where > private interrupts are strictly private, and cannot be accessed from > another CPU. Ok. > > I'm not an irqchip expert, but sometimes I really wonder if the irqchip > > framework really is the right abstraction for this. > Well, it served us pretty well so far. Seems like RISC-V has decided to > describe the HW in a subtly (and maybe pointlessly?) different way, > rather than reusing the existing abstraction. It is not necessarily bad, > but it just makes things more difficult. As said I'm no expert by any means. The use for the plic controller looks fine to me, but writing a driver around a few instructions to manage a tiny amount of per-core interrupt state just looks like overkill to me. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-05 22:22 ` hch at infradead.org @ 2018-07-06 8:25 ` Marc Zyngier 2018-07-06 9:41 ` Thomas Gleixner 1 sibling, 0 replies; 20+ messages in thread From: Marc Zyngier @ 2018-07-06 8:25 UTC (permalink / raw) To: linux-riscv On 05/07/18 23:22, hch at infradead.org wrote: > On Thu, Jul 05, 2018 at 04:58:59PM +0100, Marc Zyngier wrote: >> Then this is no different from what we have on ARM with GICv1/v2, where >> private interrupts are strictly private, and cannot be accessed from >> another CPU. > > Ok. > >>> I'm not an irqchip expert, but sometimes I really wonder if the irqchip >>> framework really is the right abstraction for this. >> Well, it served us pretty well so far. Seems like RISC-V has decided to >> describe the HW in a subtly (and maybe pointlessly?) different way, >> rather than reusing the existing abstraction. It is not necessarily bad, >> but it just makes things more difficult. > > As said I'm no expert by any means. The use for the plic controller > looks fine to me, but writing a driver around a few instructions to > manage a tiny amount of per-core interrupt state just looks like > overkill to me. I'm not advocating one way or another. If people are comfortable with signalling timer events in a different way than using the irq subsystem, that's absolutely fine by me. But if they are using existing abstractions, they should use them correctly. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-05 22:22 ` hch at infradead.org 2018-07-06 8:25 ` Marc Zyngier @ 2018-07-06 9:41 ` Thomas Gleixner 2018-07-06 18:00 ` Atish Patra 1 sibling, 1 reply; 20+ messages in thread From: Thomas Gleixner @ 2018-07-06 9:41 UTC (permalink / raw) To: linux-riscv On Thu, 5 Jul 2018, hch at infradead.org wrote: > On Thu, Jul 05, 2018 at 04:58:59PM +0100, Marc Zyngier wrote: > > Then this is no different from what we have on ARM with GICv1/v2, where > > private interrupts are strictly private, and cannot be accessed from > > another CPU. > > Ok. > > > > I'm not an irqchip expert, but sometimes I really wonder if the irqchip > > > framework really is the right abstraction for this. > > Well, it served us pretty well so far. Seems like RISC-V has decided to > > describe the HW in a subtly (and maybe pointlessly?) different way, > > rather than reusing the existing abstraction. It is not necessarily bad, > > but it just makes things more difficult. > > As said I'm no expert by any means. The use for the plic controller > looks fine to me, but writing a driver around a few instructions to > manage a tiny amount of per-core interrupt state just looks like > overkill to me. I don't think it's overkill. If you use the percpu interrupt infrastructure correctly then you have advantages: - The same hardware and Linux irq number is used on all CPUs, which makes a lot of sense as the interrupts are strictly CPU local. That avoids having nr_percpu_irqs * nr_cpus interrupt numbers for those local ones. You just have nr_percpu_irqs and be done with it. Just look at /proc/interrupts: CPU0 CPU1 CPU2 CPU3 1: 20 20 20 20 timer instead of: CPU0 CPU1 CPU2 CPU3 1: 20 0 0 0 timer 2: 0 21 0 0 timer 3: 0 0 22 0 timer 4: 0 0 0 23 timer While that's purely cosmetic, it also avoids to keep track on which Linux irq number is on which CPU internally. - That also avoids having nr_cpus * nr_percpu_irqs device tree entries which all describe exactly the same register set.... Vs. the current patch: Either it uses request_irq_percpu() and the underlying infrastructure correctly or not at all. If riscv people want to have the waste in both irq space and DT, it's their problem, but then they shall stick to the regular interrupt mechanisms and not abuse the percpu stuff. Thanks, tglx ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-06 9:41 ` Thomas Gleixner @ 2018-07-06 18:00 ` Atish Patra 0 siblings, 0 replies; 20+ messages in thread From: Atish Patra @ 2018-07-06 18:00 UTC (permalink / raw) To: linux-riscv On 7/6/18 2:42 AM, Thomas Gleixner wrote: > On Thu, 5 Jul 2018, hch at infradead.org wrote: >> On Thu, Jul 05, 2018 at 04:58:59PM +0100, Marc Zyngier wrote: >>> Then this is no different from what we have on ARM with GICv1/v2, where >>> private interrupts are strictly private, and cannot be accessed from >>> another CPU. >> >> Ok. >> >>>> I'm not an irqchip expert, but sometimes I really wonder if the irqchip >>>> framework really is the right abstraction for this. >>> Well, it served us pretty well so far. Seems like RISC-V has decided to >>> describe the HW in a subtly (and maybe pointlessly?) different way, >>> rather than reusing the existing abstraction. It is not necessarily bad, >>> but it just makes things more difficult. >> >> As said I'm no expert by any means. The use for the plic controller >> looks fine to me, but writing a driver around a few instructions to >> manage a tiny amount of per-core interrupt state just looks like >> overkill to me. > > I don't think it's overkill. If you use the percpu interrupt infrastructure > correctly then you have advantages: > > - The same hardware and Linux irq number is used on all CPUs, which makes a > lot of sense as the interrupts are strictly CPU local. > > That avoids having nr_percpu_irqs * nr_cpus interrupt numbers for those > local ones. You just have nr_percpu_irqs and be done with it. > > Just look at /proc/interrupts: > > CPU0 CPU1 CPU2 CPU3 > 1: 20 20 20 20 timer > > instead of: > > CPU0 CPU1 CPU2 CPU3 > 1: 20 0 0 0 timer > 2: 0 21 0 0 timer > 3: 0 0 22 0 timer > 4: 0 0 0 23 timer > > While that's purely cosmetic, it also avoids to keep track on which > Linux irq number is on which CPU internally. > > - That also avoids having nr_cpus * nr_percpu_irqs device tree entries which > all describe exactly the same register set.... > > Vs. the current patch: > I also think using percpu interrupts is definitely beneficial in the long run. Especially if we have local interrupts are connected apart from software and timer in future. It will avoid interrupt stats bloating. Thanks all for reviewing the patch and the explanation. Regards, Atish > Either it uses request_irq_percpu() and the underlying infrastructure > correctly or not at all. If riscv people want to have the waste in both irq > space and DT, it's their problem, but then they shall stick to the regular > interrupt mechanisms and not abuse the percpu stuff. > > Thanks, > > tglx > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-05 15:58 ` Marc Zyngier 2018-07-05 22:22 ` hch at infradead.org @ 2018-08-03 0:14 ` Palmer Dabbelt 1 sibling, 0 replies; 20+ messages in thread From: Palmer Dabbelt @ 2018-08-03 0:14 UTC (permalink / raw) To: linux-riscv On Thu, 05 Jul 2018 08:58:59 PDT (-0700), marc.zyngier at arm.com wrote: > On 05/07/18 16:42, hch at infradead.org wrote: >> On Thu, Jul 05, 2018 at 08:48:23AM +0100, Marc Zyngier wrote: >>> Here, you have discrete timers, discrete irqchip, and thus individual >>> domains. You might as well keep everything as non-percpu interrupts with >>> a fixed affinity, assuming each HLIC is reachable from any CPU (which >>> cannot universally work on ARM). >> >> The hart level interrupt controller is based around RISC-V control >> registers, which per defintion are CPU local. So you can't touch them >> at all from other cores and any access of the remote 'irqchip' needs >> and IPI. > > Then this is no different from what we have on ARM with GICv1/v2, where > private interrupts are strictly private, and cannot be accessed from > another CPU. > >> I'm not an irqchip expert, but sometimes I really wonder if the irqchip >> framework really is the right abstraction for this. > Well, it served us pretty well so far. Seems like RISC-V has decided to > describe the HW in a subtly (and maybe pointlessly?) different way, > rather than reusing the existing abstraction. It is not necessarily bad, > but it just makes things more difficult. I think a lot of this boils down to me not really knowing what I'm doing :). The device tree descriptions we provide are generated along with the hardware, so they tend to match the structure of the hardware. In this case there's an interrupt widget inside each core, and the generation of that RTL triggers the generation of this device tree. There's then a platform-level interrupt controller (which looks a lot more like the GIC) that triggers the generation of a global interrupt controller device tree node. We do end up with a bunch of messiness this way, so I'm fine changing it -- we're essentially defining the RISC-V device tree standard here, and I consider that all up for change until we start writing down a platform specification to formalize it. While I still haven't given it a proper look, I believe Christoph has a solution that meshes a bit better with how these things are generally done that was just sent out for review. Thanks for the feedback, and sorry I'm so slow! ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-05 15:42 ` hch at infradead.org 2018-07-05 15:58 ` Marc Zyngier @ 2018-07-05 17:15 ` Atish Patra 1 sibling, 0 replies; 20+ messages in thread From: Atish Patra @ 2018-07-05 17:15 UTC (permalink / raw) To: linux-riscv On 7/5/18 8:42 AM, hch at infradead.org wrote: > On Thu, Jul 05, 2018 at 08:48:23AM +0100, Marc Zyngier wrote: >>>> 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). >>>> >>> >>> We have different interrupt number(Linux IRQ number) because of per cpu >>> based interrupt controller called as Hart Level Interrupt Controller >>> (HLIC). Some more details are on this patch. >>> >>> https://lkml.org/lkml/2018/6/22/914 >>> >>> Thus, each HLIC registers it's own irq domain. Each timer interrupt in >>> DT defines that CPU's HLIC as it's interrupt parent. Here is an example >>> of DT. > > Btw, for the next iteration of the serie I really think it needs to be > against mainline and include the heary level irqchip driver. Basing > against an unknown unknown in some riscv tree makes reviews confusing, > and we also really need to get riscv irqs and timers upstream ASAP, so > we need to drive these together. > Agreed. I will resubmit the v2 either with palmer's base patch set or after the base IRQ patches are accepted mainline (whatever makes things move faster). >> This is not how percpu_devid interrupts are supposed to be used. The >> expected use case is that you have a single domain, and that the same >> Linux irq spans all the CPUs. On ARM, we declare a *single* timer that >> is connected to a *single* interrupt controller, and the distribution is >> completely implicit. That's what percpu_devid interrupts are for. >> >> Here, you have discrete timers, discrete irqchip, and thus individual >> domains. You might as well keep everything as non-percpu interrupts with >> a fixed affinity, assuming each HLIC is reachable from any CPU (which >> cannot universally work on ARM). > > The hart level interrupt controller is based around RISC-V control > registers, which per defintion are CPU local. So you can't touch them > at all from other cores and any access of the remote 'irqchip' needs > and IPI. > > I'm not an irqchip expert, but sometimes I really wonder if the irqchip > framework really is the right abstraction for this. > I will let palmer to comment on the rationale behind abstracting control registers as an irqchip i.e. HLIC. Regards, Atish ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt 2018-07-02 8:31 ` Marc Zyngier 2018-07-03 18:28 ` Atish Patra @ 2018-08-02 21:53 ` Palmer Dabbelt 1 sibling, 0 replies; 20+ messages in thread From: Palmer Dabbelt @ 2018-08-02 21:53 UTC (permalink / raw) To: linux-riscv On Mon, 02 Jul 2018 01:31:44 PDT (-0700), marc.zyngier at arm.com 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. > >> + >> + /* >> + * 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. > > 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. Christoph has a cleaned up version of this patch set that simplifies a lot of these issues. Thanks for the feedback! > >> + irq_set_chip_and_handler(irq, &data->chip, >> + handle_percpu_devid_irq); >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >> + } else >> + irq_set_chip_and_handler(irq, &data->chip, handle_simple_irq); >> irq_set_chip_data(irq, data); >> irq_set_noprobe(irq); >> irq_set_affinity(irq, cpumask_of(data->hart)); >> > > Thanks, > > M. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-08-03 0:14 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-29 18:01 [RFC PATCH 0/2] Local timer interrupt fix Atish Patra 2018-06-29 18:01 ` [RFC PATCH 1/2] dt-binding: RISC-V local timer docs Atish Patra 2018-07-02 8:22 ` Marc Zyngier 2018-07-03 18:18 ` Atish Patra 2018-06-29 18:01 ` [RFC PATCH 2/2] riscv: Introduce per cpu based local timer interrupt Atish Patra 2018-07-02 8:31 ` Marc Zyngier 2018-07-03 18:28 ` Atish Patra 2018-07-04 8:24 ` Marc Zyngier 2018-07-05 1:55 ` Atish Patra 2018-07-05 2:17 ` Anup Patel 2018-07-05 7:48 ` Marc Zyngier 2018-07-05 15:42 ` hch at infradead.org 2018-07-05 15:58 ` Marc Zyngier 2018-07-05 22:22 ` hch at infradead.org 2018-07-06 8:25 ` Marc Zyngier 2018-07-06 9:41 ` Thomas Gleixner 2018-07-06 18:00 ` Atish Patra 2018-08-03 0:14 ` Palmer Dabbelt 2018-07-05 17:15 ` Atish Patra 2018-08-02 21:53 ` Palmer Dabbelt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.