* [PATCH v6 0/6] New RISC-V Local Interrupt Controller Driver
@ 2020-05-30 10:07 Anup Patel
2020-05-30 10:07 ` [PATCH v6 1/6] RISC-V: self-contained IPI handling routine Anup Patel
` (5 more replies)
0 siblings, 6 replies; 18+ messages in thread
From: Anup Patel @ 2020-05-30 10:07 UTC (permalink / raw)
To: Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano,
Thomas Gleixner, Jason Cooper, Marc Zyngier
Cc: Atish Patra, Alistair Francis, Anup Patel, linux-riscv,
linux-kernel, Anup Patel
This patchset provides a new RISC-V Local Interrupt Controller Driver
for managing per-CPU local interrupts. The overall approach is inspired
from the way per-CPU local interrupts are handled by Linux ARM64 and
ARM GICv3 driver.
Few advantages of this new driver over previous one are:
1. All local interrupts are registered as per-CPU interrupts
2. The RISC-V timer driver can register timer interrupt handler
using kernel irq subsystem without relying on arch/riscv to
explicitly call it's interrupt handler
3. The KVM RISC-V can use this driver to implement interrupt
handler for per-HART guest external interrupt defined by
the RISC-V H-Extension
4. In future, we can develop drivers for devices with per-HART
interrupts without changing arch code or this driver (example,
CLINT timer driver for RISC-V M-mode kernel)
With this patchset, output of "cat /proc/interrupts" looks as follows:
CPU0 CPU1 CPU2 CPU3
2: 379 0 0 0 SiFive PLIC 10 ttyS0
3: 591 0 0 0 SiFive PLIC 8 virtio0
5: 5079 10821 8435 12984 RISC-V INTC 5 riscv-timer
IPI0: 2045 2537 891 870 Rescheduling interrupts
IPI1: 9 269 91 168 Function call interrupts
IPI2: 0 0 0 0 CPU stop interrupts
The patchset is based up Linux-5.7-rc7 and can be found at riscv_intc_v6
branch of: https://github.com/avpatel/linux.git
This series is tested on:
1. QEMU RV64 virt machine using Linux RISC-V S-mode
2. QEMU RV32 virt machine using Linux RISC-V S-mode
3. QEMU RV64 virt machine using Linux RISC-V M-mode (i.e. NoMMU)
Changes since v5:
- Rebased to Linux-5.7-rc7 with PLIC improvement patches
- Removed riscv_of_parent_hartid() from PATCH3
- Addressed other minor comments from Palmer and Marc Z
Changes since v4:
- Rebased to Linux-5.7-rc6 and multi-PLIC improvement patches
- Added separate patch to force select RISCV_INTC for CONFIG_RISCV
- Fixed the driver for Linux RISC-V NoMMU
Changes since v3:
- Rebased to Linux-5.6-rc5 and Atish's PLIC patches
- Added separate patch to rename and move plic_find_hart_id()
to arch directory
- Use riscv_of_parent_hartid() in riscv_intc_init() instead of
atomic counter
Changes since v2:
- Dropped PATCH2 since it was merged long-time back
- Rebased series from Linux-4.19-rc2 to Linux-5.6-rc2
Changes since v1:
- Removed changes related to puggable IPI triggering
- Separate patch for self-contained IPI handling routine
- Removed patch for GENERIC_IRQ kconfig options
- Added patch to remove do_IRQ() function
- Rebased upon Atish's SMP patches
Anup Patel (6):
RISC-V: self-contained IPI handling routine
RISC-V: Rename and move plic_find_hart_id() to arch directory
irqchip: RISC-V per-HART local interrupt controller driver
clocksource/drivers/timer-riscv: Use per-CPU timer interrupt
RISC-V: Remove do_IRQ() function
RISC-V: Force select RISCV_INTC for CONFIG_RISCV
arch/riscv/Kconfig | 2 +
arch/riscv/include/asm/irq.h | 5 --
arch/riscv/include/asm/processor.h | 1 +
arch/riscv/include/asm/smp.h | 3 +
arch/riscv/kernel/cpu.c | 16 ++++
arch/riscv/kernel/entry.S | 4 +-
arch/riscv/kernel/irq.c | 33 +------
arch/riscv/kernel/smp.c | 11 ++-
arch/riscv/kernel/traps.c | 2 -
drivers/clocksource/timer-riscv.c | 41 ++++++++-
drivers/irqchip/Kconfig | 13 +++
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-riscv-intc.c | 140 +++++++++++++++++++++++++++++
drivers/irqchip/irq-sifive-plic.c | 44 ++++-----
include/linux/cpuhotplug.h | 1 +
15 files changed, 253 insertions(+), 64 deletions(-)
create mode 100644 drivers/irqchip/irq-riscv-intc.c
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v6 1/6] RISC-V: self-contained IPI handling routine 2020-05-30 10:07 [PATCH v6 0/6] New RISC-V Local Interrupt Controller Driver Anup Patel @ 2020-05-30 10:07 ` Anup Patel 2020-05-30 10:07 ` [PATCH v6 2/6] RISC-V: Rename and move plic_find_hart_id() to arch directory Anup Patel ` (4 subsequent siblings) 5 siblings, 0 replies; 18+ messages in thread From: Anup Patel @ 2020-05-30 10:07 UTC (permalink / raw) To: Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Marc Zyngier Cc: Atish Patra, Alistair Francis, Anup Patel, linux-riscv, linux-kernel, Anup Patel, Palmer Dabbelt Currently, the IPI handling routine riscv_software_interrupt() does not take any argument and also does not perform irq_enter()/irq_exit(). This patch makes IPI handling routine more self-contained by: 1. Passing "pt_regs *" argument 2. Explicitly doing irq_enter()/irq_exit() 3. Explicitly save/restore "pt_regs *" using set_irq_regs() With above changes, IPI handling routine does not depend on caller function to perform irq_enter()/irq_exit() and save/restore of "pt_regs *" hence its more self-contained. This also enables us to call IPI handling routine from IRQCHIP drivers. Signed-off-by: Anup Patel <anup.patel@wdc.com> Reviewed-by: Atish Patra <atish.patra@wdc.com> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com> Acked-by: Palmer Dabbelt <palmerdabbelt@google.com> --- arch/riscv/include/asm/irq.h | 1 - arch/riscv/include/asm/smp.h | 3 +++ arch/riscv/kernel/irq.c | 16 ++++++++++------ arch/riscv/kernel/smp.c | 11 +++++++++-- 4 files changed, 22 insertions(+), 9 deletions(-) diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h index 6e1b0e0325eb..0183e15ace66 100644 --- a/arch/riscv/include/asm/irq.h +++ b/arch/riscv/include/asm/irq.h @@ -13,7 +13,6 @@ #define NR_IRQS 0 void riscv_timer_interrupt(void); -void riscv_software_interrupt(void); #include <asm-generic/irq.h> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h index f4c7cfda6b7f..40bb1c15a731 100644 --- a/arch/riscv/include/asm/smp.h +++ b/arch/riscv/include/asm/smp.h @@ -28,6 +28,9 @@ void show_ipi_stats(struct seq_file *p, int prec); /* SMP initialization hook for setup_arch */ void __init setup_smp(void); +/* Called from C code, this handles an IPI. */ +void handle_IPI(struct pt_regs *regs); + /* Hook for the generic smp_call_function_many() routine. */ void arch_send_call_function_ipi_mask(struct cpumask *mask); diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c index 345c4f2eba13..bb0bfcd537e7 100644 --- a/arch/riscv/kernel/irq.c +++ b/arch/riscv/kernel/irq.c @@ -19,12 +19,15 @@ int arch_show_interrupts(struct seq_file *p, int prec) asmlinkage __visible void __irq_entry do_IRQ(struct pt_regs *regs) { - struct pt_regs *old_regs = set_irq_regs(regs); + struct pt_regs *old_regs; - irq_enter(); switch (regs->cause & ~CAUSE_IRQ_FLAG) { case RV_IRQ_TIMER: + old_regs = set_irq_regs(regs); + irq_enter(); riscv_timer_interrupt(); + irq_exit(); + set_irq_regs(old_regs); break; #ifdef CONFIG_SMP case RV_IRQ_SOFT: @@ -32,19 +35,20 @@ asmlinkage __visible void __irq_entry do_IRQ(struct pt_regs *regs) * We only use software interrupts to pass IPIs, so if a non-SMP * system gets one, then we don't know what to do. */ - riscv_software_interrupt(); + handle_IPI(regs); break; #endif case RV_IRQ_EXT: + old_regs = set_irq_regs(regs); + irq_enter(); handle_arch_irq(regs); + irq_exit(); + set_irq_regs(old_regs); break; default: pr_alert("unexpected interrupt cause 0x%lx", regs->cause); BUG(); } - irq_exit(); - - set_irq_regs(old_regs); } void __init init_IRQ(void) diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c index a65a8fa0c22d..b1d4f452f843 100644 --- a/arch/riscv/kernel/smp.c +++ b/arch/riscv/kernel/smp.c @@ -123,11 +123,14 @@ static inline void clear_ipi(void) clint_clear_ipi(cpuid_to_hartid_map(smp_processor_id())); } -void riscv_software_interrupt(void) +void handle_IPI(struct pt_regs *regs) { + struct pt_regs *old_regs = set_irq_regs(regs); unsigned long *pending_ipis = &ipi_data[smp_processor_id()].bits; unsigned long *stats = ipi_data[smp_processor_id()].stats; + irq_enter(); + clear_ipi(); while (true) { @@ -138,7 +141,7 @@ void riscv_software_interrupt(void) ops = xchg(pending_ipis, 0); if (ops == 0) - return; + goto done; if (ops & (1 << IPI_RESCHEDULE)) { stats[IPI_RESCHEDULE]++; @@ -160,6 +163,10 @@ void riscv_software_interrupt(void) /* Order data access and bit testing. */ mb(); } + +done: + irq_exit(); + set_irq_regs(old_regs); } static const char * const ipi_names[] = { -- 2.25.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 2/6] RISC-V: Rename and move plic_find_hart_id() to arch directory 2020-05-30 10:07 [PATCH v6 0/6] New RISC-V Local Interrupt Controller Driver Anup Patel 2020-05-30 10:07 ` [PATCH v6 1/6] RISC-V: self-contained IPI handling routine Anup Patel @ 2020-05-30 10:07 ` Anup Patel 2020-05-30 10:07 ` [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver Anup Patel ` (3 subsequent siblings) 5 siblings, 0 replies; 18+ messages in thread From: Anup Patel @ 2020-05-30 10:07 UTC (permalink / raw) To: Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Marc Zyngier Cc: Atish Patra, Alistair Francis, Anup Patel, linux-riscv, linux-kernel, Anup Patel, Palmer Dabbelt The plic_find_hart_id() can be useful to other interrupt controller drivers (such as RISC-V local interrupt driver) so we rename this function to riscv_of_parent_hartid() and place it in arch directory along with riscv_of_processor_hartid(). Signed-off-by: Anup Patel <anup.patel@wdc.com> Reviewed-by: Atish Patra <atish.patra@wdc.com> Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com> Acked-by: Palmer Dabbelt <palmerdabbelt@google.com> --- arch/riscv/include/asm/processor.h | 1 + arch/riscv/kernel/cpu.c | 16 ++++++++++++++++ drivers/irqchip/irq-sifive-plic.c | 16 +--------------- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h index 3ddb798264f1..b1efd840003c 100644 --- a/arch/riscv/include/asm/processor.h +++ b/arch/riscv/include/asm/processor.h @@ -75,6 +75,7 @@ static inline void wait_for_interrupt(void) struct device_node; int riscv_of_processor_hartid(struct device_node *node); +int riscv_of_parent_hartid(struct device_node *node); extern void riscv_fill_hwcap(void); diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c index 40a3c442ac5f..6d59e6906fdd 100644 --- a/arch/riscv/kernel/cpu.c +++ b/arch/riscv/kernel/cpu.c @@ -44,6 +44,22 @@ int riscv_of_processor_hartid(struct device_node *node) return hart; } +/* + * Find hart ID of the CPU DT node under which given DT node falls. + * + * To achieve this, we walk up the DT tree until we find an active + * RISC-V core (HART) node and extract the cpuid from it. + */ +int riscv_of_parent_hartid(struct device_node *node) +{ + for (; node; node = node->parent) { + if (of_device_is_compatible(node, "riscv")) + return riscv_of_processor_hartid(node); + } + + return -1; +} + #ifdef CONFIG_PROC_FS static void print_isa(struct seq_file *f, const char *isa) diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index d9c53f85a68e..16d31d114c30 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -240,20 +240,6 @@ static void plic_handle_irq(struct pt_regs *regs) csr_set(CSR_IE, IE_EIE); } -/* - * Walk up the DT tree until we find an active RISC-V core (HART) node and - * extract the cpuid from it. - */ -static int plic_find_hart_id(struct device_node *node) -{ - for (; node; node = node->parent) { - if (of_device_is_compatible(node, "riscv")) - return riscv_of_processor_hartid(node); - } - - return -1; -} - static void plic_set_threshold(struct plic_handler *handler, u32 threshold) { /* priority must be > threshold to trigger an interrupt */ @@ -330,7 +316,7 @@ static int __init plic_init(struct device_node *node, if (parent.args[0] != RV_IRQ_EXT) continue; - hartid = plic_find_hart_id(parent.np); + hartid = riscv_of_parent_hartid(parent.np); if (hartid < 0) { pr_warn("failed to parse hart ID for context %d.\n", i); continue; -- 2.25.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver 2020-05-30 10:07 [PATCH v6 0/6] New RISC-V Local Interrupt Controller Driver Anup Patel 2020-05-30 10:07 ` [PATCH v6 1/6] RISC-V: self-contained IPI handling routine Anup Patel 2020-05-30 10:07 ` [PATCH v6 2/6] RISC-V: Rename and move plic_find_hart_id() to arch directory Anup Patel @ 2020-05-30 10:07 ` Anup Patel 2020-05-30 12:01 ` Marc Zyngier 2020-05-30 10:07 ` [PATCH v6 4/6] clocksource/drivers/timer-riscv: Use per-CPU timer interrupt Anup Patel ` (2 subsequent siblings) 5 siblings, 1 reply; 18+ messages in thread From: Anup Patel @ 2020-05-30 10:07 UTC (permalink / raw) To: Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Marc Zyngier Cc: Atish Patra, Alistair Francis, Anup Patel, linux-riscv, linux-kernel, Anup Patel, Palmer Dabbelt The RISC-V per-HART local interrupt controller manages software interrupts, timer interrupts, external interrupts (which are routed via the platform level interrupt controller) and other per-HART local interrupts. This patch adds a driver for the RISC-V local interrupt controller. It is a major re-write over perviously submitted version. (Refer, https://www.spinics.net/lists/devicetree/msg241230.html) The driver is compliant with RISC-V Hart-Level Interrupt Controller DT bindings located at: Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt Co-developed-by: Palmer Dabbelt <palmer@dabbelt.com> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com> Signed-off-by: Anup Patel <anup.patel@wdc.com> Acked-by: Palmer Dabbelt <palmerdabbelt@google.com> --- arch/riscv/Kconfig | 1 + arch/riscv/include/asm/irq.h | 2 - arch/riscv/kernel/irq.c | 33 +------ arch/riscv/kernel/traps.c | 2 - drivers/irqchip/Kconfig | 13 +++ drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-riscv-intc.c | 148 ++++++++++++++++++++++++++++++ drivers/irqchip/irq-sifive-plic.c | 30 ++++-- include/linux/cpuhotplug.h | 1 + 9 files changed, 191 insertions(+), 40 deletions(-) create mode 100644 drivers/irqchip/irq-riscv-intc.c diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 90a008e28f7e..822cb0e1a380 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -40,6 +40,7 @@ config RISCV select HAVE_PERF_REGS select HAVE_PERF_USER_STACK_DUMP select HAVE_SYSCALL_TRACEPOINTS + select HANDLE_DOMAIN_IRQ select IRQ_DOMAIN select SPARSE_IRQ select SYSCTL_EXCEPTION_TRACE diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h index 0183e15ace66..a9e5f07a7e9c 100644 --- a/arch/riscv/include/asm/irq.h +++ b/arch/riscv/include/asm/irq.h @@ -10,8 +10,6 @@ #include <linux/interrupt.h> #include <linux/linkage.h> -#define NR_IRQS 0 - void riscv_timer_interrupt(void); #include <asm-generic/irq.h> diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c index bb0bfcd537e7..eb8777642ce6 100644 --- a/arch/riscv/kernel/irq.c +++ b/arch/riscv/kernel/irq.c @@ -7,7 +7,6 @@ #include <linux/interrupt.h> #include <linux/irqchip.h> -#include <linux/irqdomain.h> #include <linux/seq_file.h> #include <asm/smp.h> @@ -19,39 +18,13 @@ int arch_show_interrupts(struct seq_file *p, int prec) asmlinkage __visible void __irq_entry do_IRQ(struct pt_regs *regs) { - struct pt_regs *old_regs; - - switch (regs->cause & ~CAUSE_IRQ_FLAG) { - case RV_IRQ_TIMER: - old_regs = set_irq_regs(regs); - irq_enter(); - riscv_timer_interrupt(); - irq_exit(); - set_irq_regs(old_regs); - break; -#ifdef CONFIG_SMP - case RV_IRQ_SOFT: - /* - * We only use software interrupts to pass IPIs, so if a non-SMP - * system gets one, then we don't know what to do. - */ - handle_IPI(regs); - break; -#endif - case RV_IRQ_EXT: - old_regs = set_irq_regs(regs); - irq_enter(); + if (handle_arch_irq) handle_arch_irq(regs); - irq_exit(); - set_irq_regs(old_regs); - break; - default: - pr_alert("unexpected interrupt cause 0x%lx", regs->cause); - BUG(); - } } void __init init_IRQ(void) { irqchip_init(); + if (!handle_arch_irq) + panic("No interrupt controller found."); } diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c index 7f58fa53033f..f48c76aadbf3 100644 --- a/arch/riscv/kernel/traps.c +++ b/arch/riscv/kernel/traps.c @@ -178,6 +178,4 @@ void trap_init(void) csr_write(CSR_SCRATCH, 0); /* Set the exception vector address */ csr_write(CSR_TVEC, &handle_exception); - /* Enable interrupts */ - csr_write(CSR_IE, IE_SIE); } diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index a85aada04a64..95d6137a8117 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -493,6 +493,19 @@ config TI_SCI_INTA_IRQCHIP If you wish to use interrupt aggregator irq resources managed by the TI System Controller, say Y here. Otherwise, say N. +config RISCV_INTC + bool "RISC-V Local Interrupt Controller" + depends on RISCV + default y + help + This enables support for the per-HART local interrupt controller + found in standard RISC-V systems. The per-HART local interrupt + controller handles timer interrupts, software interrupts, and + hardware interrupts. Without a per-HART local interrupt controller, + a RISC-V system will be unable to handle any interrupts. + + If you don't know what to do here, say Y. + config SIFIVE_PLIC bool "SiFive Platform-Level Interrupt Controller" depends on RISCV diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 37bbe39bf909..b8319f045472 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -98,6 +98,7 @@ obj-$(CONFIG_NDS32) += irq-ativic32.o obj-$(CONFIG_QCOM_PDC) += qcom-pdc.o obj-$(CONFIG_CSKY_MPINTC) += irq-csky-mpintc.o obj-$(CONFIG_CSKY_APB_INTC) += irq-csky-apb-intc.o +obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c new file mode 100644 index 000000000000..84e7bda3a090 --- /dev/null +++ b/drivers/irqchip/irq-riscv-intc.c @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2012 Regents of the University of California + * Copyright (C) 2017-2018 SiFive + * Copyright (C) 2020 Western Digital Corporation or its affiliates. + */ + +#define pr_fmt(fmt) "riscv-intc: " fmt +#include <linux/atomic.h> +#include <linux/bits.h> +#include <linux/cpu.h> +#include <linux/irq.h> +#include <linux/irqchip.h> +#include <linux/irqdomain.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/smp.h> + +static struct irq_domain *intc_domain; + +static asmlinkage void riscv_intc_irq(struct pt_regs *regs) +{ + struct pt_regs *old_regs; + unsigned long cause = regs->cause & ~CAUSE_IRQ_FLAG; + + if (unlikely(cause >= BITS_PER_LONG)) + panic("unexpected interrupt cause"); + + switch (cause) { + case RV_IRQ_TIMER: + old_regs = set_irq_regs(regs); + irq_enter(); + riscv_timer_interrupt(); + irq_exit(); + set_irq_regs(old_regs); + break; +#ifdef CONFIG_SMP + case RV_IRQ_SOFT: + /* + * We only use software interrupts to pass IPIs, so if a + * non-SMP system gets one, then we don't know what to do. + */ + handle_IPI(regs); + break; +#endif + default: + handle_domain_irq(intc_domain, cause, regs); + break; + } +} + +/* + * On RISC-V systems local interrupts are masked or unmasked by writing + * the SIE (Supervisor Interrupt Enable) CSR. As CSRs can only be written + * on the local hart, these functions can only be called on the hart that + * corresponds to the IRQ chip. + */ + +static void riscv_intc_irq_mask(struct irq_data *d) +{ + csr_clear(CSR_IE, 1UL << d->hwirq); +} + +static void riscv_intc_irq_unmask(struct irq_data *d) +{ + csr_set(CSR_IE, 1UL << d->hwirq); +} + +static int riscv_intc_cpu_starting(unsigned int cpu) +{ + csr_write(CSR_IE, 1UL << RV_IRQ_SOFT); + csr_write(CSR_IP, 0); + return 0; +} + +static int riscv_intc_cpu_dying(unsigned int cpu) +{ + csr_clear(CSR_IE, 1UL << RV_IRQ_SOFT); + return 0; +} + +static struct irq_chip riscv_intc_chip = { + .name = "RISC-V INTC", + .irq_mask = riscv_intc_irq_mask, + .irq_unmask = riscv_intc_irq_unmask, +}; + +static int riscv_intc_domain_map(struct irq_domain *d, unsigned int irq, + irq_hw_number_t hwirq) +{ + irq_set_percpu_devid(irq); + irq_domain_set_info(d, irq, hwirq, &riscv_intc_chip, d->host_data, + handle_percpu_devid_irq, NULL, NULL); + + return 0; +} + +static const struct irq_domain_ops riscv_intc_domain_ops = { + .map = riscv_intc_domain_map, + .xlate = irq_domain_xlate_onecell, +}; + +static int __init riscv_intc_init(struct device_node *node, + struct device_node *parent) +{ + int rc, hartid; + + hartid = riscv_of_parent_hartid(node); + if (hartid < 0) { + pr_warn("unable to fine hart id for %pOF\n", node); + return 0; + } + + /* + * The DT will have one INTC DT node under each CPU (or HART) + * DT node so riscv_intc_init() function will be called once + * for each INTC DT node. We only need to do INTC initialization + * for the INTC DT node belonging to boot CPU (or boot HART). + */ + if (riscv_hartid_to_cpuid(hartid) != smp_processor_id()) + return 0; + + intc_domain = irq_domain_add_linear(node, BITS_PER_LONG, + &riscv_intc_domain_ops, NULL); + if (!intc_domain) { + rc = -ENXIO; + pr_err("unable to add IRQ domain\n"); + return rc; + } + + rc = set_handle_irq(&riscv_intc_irq); + if (rc) { + pr_err("failed to set irq handler\n"); + return rc; + } + + cpuhp_setup_state(CPUHP_AP_IRQ_RISCV_STARTING, + "irqchip/riscv/intc:starting", + riscv_intc_cpu_starting, + riscv_intc_cpu_dying); + + pr_info("%d local interrupts mapped\n", BITS_PER_LONG); + + return 0; +} + +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init); diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index 16d31d114c30..cda2e246aee3 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -9,6 +9,7 @@ #include <linux/io.h> #include <linux/irq.h> #include <linux/irqchip.h> +#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h> #include <linux/module.h> #include <linux/of.h> @@ -76,6 +77,7 @@ struct plic_handler { void __iomem *enable_base; struct plic_priv *priv; }; +static int plic_parent_irq; static bool plic_cpuhp_setup_done; static DEFINE_PER_CPU(struct plic_handler, plic_handlers); @@ -219,15 +221,17 @@ static const struct irq_domain_ops plic_irqdomain_ops = { * that source ID back to the same claim register. This automatically enables * and disables the interrupt, so there's nothing else to do. */ -static void plic_handle_irq(struct pt_regs *regs) +static void plic_handle_irq(struct irq_desc *desc) { struct plic_handler *handler = this_cpu_ptr(&plic_handlers); + struct irq_chip *chip = irq_desc_get_chip(desc); void __iomem *claim = handler->hart_base + CONTEXT_CLAIM; irq_hw_number_t hwirq; WARN_ON_ONCE(!handler->present); - csr_clear(CSR_IE, IE_EIE); + chained_irq_enter(chip, desc); + while ((hwirq = readl(claim))) { int irq = irq_find_mapping(handler->priv->irqdomain, hwirq); @@ -237,7 +241,8 @@ static void plic_handle_irq(struct pt_regs *regs) else generic_handle_irq(irq); } - csr_set(CSR_IE, IE_EIE); + + chained_irq_exit(chip, desc); } static void plic_set_threshold(struct plic_handler *handler, u32 threshold) @@ -250,7 +255,8 @@ static int plic_dying_cpu(unsigned int cpu) { struct plic_handler *handler = this_cpu_ptr(&plic_handlers); - csr_clear(CSR_IE, IE_EIE); + if (plic_parent_irq) + disable_percpu_irq(plic_parent_irq); plic_set_threshold(handler, PLIC_DISABLE_THRESHOLD); return 0; @@ -260,7 +266,11 @@ static int plic_starting_cpu(unsigned int cpu) { struct plic_handler *handler = this_cpu_ptr(&plic_handlers); - csr_set(CSR_IE, IE_EIE); + if (plic_parent_irq) + enable_percpu_irq(plic_parent_irq, + irq_get_trigger_type(plic_parent_irq)); + else + pr_warn("cpu%d: parent irq not available\n"); plic_set_threshold(handler, PLIC_ENABLE_THRESHOLD); return 0; @@ -273,6 +283,7 @@ static int __init plic_init(struct device_node *node, u32 nr_irqs; struct plic_priv *priv; struct plic_handler *handler; + struct of_phandle_args intc_args; priv = kzalloc(sizeof(*priv), GFP_KERNEL); if (!priv) @@ -328,6 +339,14 @@ static int __init plic_init(struct device_node *node, continue; } + /* Find parent domain and register chained handler */ + if (!plic_parent_irq && irq_find_host(parent.np)) { + plic_parent_irq = irq_of_parse_and_map(node, i); + if (plic_parent_irq) + irq_set_chained_handler(plic_parent_irq, + plic_handle_irq); + } + /* * When running in M-mode we need to ignore the S-mode handler. * Here we assume it always comes later, but that might be a @@ -368,7 +387,6 @@ static int __init plic_init(struct device_node *node, pr_info("%pOFP: mapped %d interrupts with %d handlers for" " %d contexts.\n", node, nr_irqs, nr_handlers, nr_contexts); - set_handle_irq(plic_handle_irq); return 0; out_iounmap: diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h index 77d70b633531..57b1f8f777d9 100644 --- a/include/linux/cpuhotplug.h +++ b/include/linux/cpuhotplug.h @@ -102,6 +102,7 @@ enum cpuhp_state { CPUHP_AP_IRQ_ARMADA_XP_STARTING, CPUHP_AP_IRQ_BCM2836_STARTING, CPUHP_AP_IRQ_MIPS_GIC_STARTING, + CPUHP_AP_IRQ_RISCV_STARTING, CPUHP_AP_IRQ_SIFIVE_PLIC_STARTING, CPUHP_AP_ARM_MVEBU_COHERENCY, CPUHP_AP_MICROCODE_LOADER, -- 2.25.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver 2020-05-30 10:07 ` [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver Anup Patel @ 2020-05-30 12:01 ` Marc Zyngier 2020-05-31 5:36 ` Anup Patel 0 siblings, 1 reply; 18+ messages in thread From: Marc Zyngier @ 2020-05-30 12:01 UTC (permalink / raw) To: Anup Patel Cc: Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Atish Patra, Alistair Francis, Anup Patel, linux-riscv, linux-kernel, Palmer Dabbelt On 2020-05-30 11:07, Anup Patel wrote: > The RISC-V per-HART local interrupt controller manages software > interrupts, timer interrupts, external interrupts (which are routed > via the platform level interrupt controller) and other per-HART > local interrupts. > > This patch adds a driver for the RISC-V local interrupt controller. Please don't use "This patch...". Say something like: "Add a driver driver for this component, which eventually replaces the RISC-V architecture code, allowing for a better split between arch code and drivers." > It is a major re-write over perviously submitted version. > (Refer, https://www.spinics.net/lists/devicetree/msg241230.html) I'm not sure that's very useful. Again, this is better placed in the cover letter, and can be retrived via the Link: tags. > The driver is compliant with RISC-V Hart-Level Interrupt Controller > DT bindings located at: > Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt > > Co-developed-by: Palmer Dabbelt <palmer@dabbelt.com> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com> > Signed-off-by: Anup Patel <anup.patel@wdc.com> > Acked-by: Palmer Dabbelt <palmerdabbelt@google.com> > --- > arch/riscv/Kconfig | 1 + > arch/riscv/include/asm/irq.h | 2 - > arch/riscv/kernel/irq.c | 33 +------ > arch/riscv/kernel/traps.c | 2 - > drivers/irqchip/Kconfig | 13 +++ > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-riscv-intc.c | 148 ++++++++++++++++++++++++++++++ > drivers/irqchip/irq-sifive-plic.c | 30 ++++-- > include/linux/cpuhotplug.h | 1 + > 9 files changed, 191 insertions(+), 40 deletions(-) > create mode 100644 drivers/irqchip/irq-riscv-intc.c > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index 90a008e28f7e..822cb0e1a380 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -40,6 +40,7 @@ config RISCV > select HAVE_PERF_REGS > select HAVE_PERF_USER_STACK_DUMP > select HAVE_SYSCALL_TRACEPOINTS > + select HANDLE_DOMAIN_IRQ > select IRQ_DOMAIN > select SPARSE_IRQ > select SYSCTL_EXCEPTION_TRACE > diff --git a/arch/riscv/include/asm/irq.h > b/arch/riscv/include/asm/irq.h > index 0183e15ace66..a9e5f07a7e9c 100644 > --- a/arch/riscv/include/asm/irq.h > +++ b/arch/riscv/include/asm/irq.h > @@ -10,8 +10,6 @@ > #include <linux/interrupt.h> > #include <linux/linkage.h> > > -#define NR_IRQS 0 > - > void riscv_timer_interrupt(void); > > #include <asm-generic/irq.h> > diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c > index bb0bfcd537e7..eb8777642ce6 100644 > --- a/arch/riscv/kernel/irq.c > +++ b/arch/riscv/kernel/irq.c > @@ -7,7 +7,6 @@ > > #include <linux/interrupt.h> > #include <linux/irqchip.h> > -#include <linux/irqdomain.h> > #include <linux/seq_file.h> > #include <asm/smp.h> > > @@ -19,39 +18,13 @@ int arch_show_interrupts(struct seq_file *p, int > prec) > > asmlinkage __visible void __irq_entry do_IRQ(struct pt_regs *regs) > { > - struct pt_regs *old_regs; > - > - switch (regs->cause & ~CAUSE_IRQ_FLAG) { > - case RV_IRQ_TIMER: > - old_regs = set_irq_regs(regs); > - irq_enter(); > - riscv_timer_interrupt(); > - irq_exit(); > - set_irq_regs(old_regs); > - break; > -#ifdef CONFIG_SMP > - case RV_IRQ_SOFT: > - /* > - * We only use software interrupts to pass IPIs, so if a non-SMP > - * system gets one, then we don't know what to do. > - */ > - handle_IPI(regs); > - break; > -#endif > - case RV_IRQ_EXT: > - old_regs = set_irq_regs(regs); > - irq_enter(); > + if (handle_arch_irq) > handle_arch_irq(regs); > - irq_exit(); > - set_irq_regs(old_regs); > - break; > - default: > - pr_alert("unexpected interrupt cause 0x%lx", regs->cause); > - BUG(); > - } > } > > void __init init_IRQ(void) > { > irqchip_init(); > + if (!handle_arch_irq) > + panic("No interrupt controller found."); > } > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c > index 7f58fa53033f..f48c76aadbf3 100644 > --- a/arch/riscv/kernel/traps.c > +++ b/arch/riscv/kernel/traps.c > @@ -178,6 +178,4 @@ void trap_init(void) > csr_write(CSR_SCRATCH, 0); > /* Set the exception vector address */ > csr_write(CSR_TVEC, &handle_exception); > - /* Enable interrupts */ > - csr_write(CSR_IE, IE_SIE); > } > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index a85aada04a64..95d6137a8117 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -493,6 +493,19 @@ config TI_SCI_INTA_IRQCHIP > If you wish to use interrupt aggregator irq resources managed by > the > TI System Controller, say Y here. Otherwise, say N. > > +config RISCV_INTC > + bool "RISC-V Local Interrupt Controller" > + depends on RISCV > + default y > + help > + This enables support for the per-HART local interrupt controller > + found in standard RISC-V systems. The per-HART local interrupt > + controller handles timer interrupts, software interrupts, and > + hardware interrupts. Without a per-HART local interrupt > controller, > + a RISC-V system will be unable to handle any interrupts. > + > + If you don't know what to do here, say Y. > + > config SIFIVE_PLIC > bool "SiFive Platform-Level Interrupt Controller" > depends on RISCV > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 37bbe39bf909..b8319f045472 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -98,6 +98,7 @@ obj-$(CONFIG_NDS32) += irq-ativic32.o > obj-$(CONFIG_QCOM_PDC) += qcom-pdc.o > obj-$(CONFIG_CSKY_MPINTC) += irq-csky-mpintc.o > obj-$(CONFIG_CSKY_APB_INTC) += irq-csky-apb-intc.o > +obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o > obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o > obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o > obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o > diff --git a/drivers/irqchip/irq-riscv-intc.c > b/drivers/irqchip/irq-riscv-intc.c > new file mode 100644 > index 000000000000..84e7bda3a090 > --- /dev/null > +++ b/drivers/irqchip/irq-riscv-intc.c > @@ -0,0 +1,148 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2012 Regents of the University of California > + * Copyright (C) 2017-2018 SiFive > + * Copyright (C) 2020 Western Digital Corporation or its affiliates. > + */ > + > +#define pr_fmt(fmt) "riscv-intc: " fmt > +#include <linux/atomic.h> > +#include <linux/bits.h> > +#include <linux/cpu.h> > +#include <linux/irq.h> > +#include <linux/irqchip.h> > +#include <linux/irqdomain.h> > +#include <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/smp.h> > + > +static struct irq_domain *intc_domain; > + > +static asmlinkage void riscv_intc_irq(struct pt_regs *regs) > +{ > + struct pt_regs *old_regs; > + unsigned long cause = regs->cause & ~CAUSE_IRQ_FLAG; > + > + if (unlikely(cause >= BITS_PER_LONG)) > + panic("unexpected interrupt cause"); > + > + switch (cause) { > + case RV_IRQ_TIMER: > + old_regs = set_irq_regs(regs); > + irq_enter(); > + riscv_timer_interrupt(); > + irq_exit(); > + set_irq_regs(old_regs); > + break; > +#ifdef CONFIG_SMP > + case RV_IRQ_SOFT: > + /* > + * We only use software interrupts to pass IPIs, so if a > + * non-SMP system gets one, then we don't know what to do. > + */ > + handle_IPI(regs); > + break; > +#endif > + default: > + handle_domain_irq(intc_domain, cause, regs); > + break; > + } > +} > + > +/* > + * On RISC-V systems local interrupts are masked or unmasked by > writing > + * the SIE (Supervisor Interrupt Enable) CSR. As CSRs can only be > written > + * on the local hart, these functions can only be called on the hart > that > + * corresponds to the IRQ chip. > + */ > + > +static void riscv_intc_irq_mask(struct irq_data *d) > +{ > + csr_clear(CSR_IE, 1UL << d->hwirq); As I mentionned earlier, BIT(d->hwirq) is the way to write this. > +} > + > +static void riscv_intc_irq_unmask(struct irq_data *d) > +{ > + csr_set(CSR_IE, 1UL << d->hwirq); > +} > + > +static int riscv_intc_cpu_starting(unsigned int cpu) > +{ > + csr_write(CSR_IE, 1UL << RV_IRQ_SOFT); > + csr_write(CSR_IP, 0); > + return 0; > +} > + > +static int riscv_intc_cpu_dying(unsigned int cpu) > +{ > + csr_clear(CSR_IE, 1UL << RV_IRQ_SOFT); > + return 0; > +} > + > +static struct irq_chip riscv_intc_chip = { > + .name = "RISC-V INTC", > + .irq_mask = riscv_intc_irq_mask, > + .irq_unmask = riscv_intc_irq_unmask, > +}; > + > +static int riscv_intc_domain_map(struct irq_domain *d, unsigned int > irq, > + irq_hw_number_t hwirq) > +{ > + irq_set_percpu_devid(irq); > + irq_domain_set_info(d, irq, hwirq, &riscv_intc_chip, d->host_data, > + handle_percpu_devid_irq, NULL, NULL); > + > + return 0; > +} > + > +static const struct irq_domain_ops riscv_intc_domain_ops = { > + .map = riscv_intc_domain_map, > + .xlate = irq_domain_xlate_onecell, > +}; > + > +static int __init riscv_intc_init(struct device_node *node, > + struct device_node *parent) > +{ > + int rc, hartid; > + > + hartid = riscv_of_parent_hartid(node); > + if (hartid < 0) { > + pr_warn("unable to fine hart id for %pOF\n", node); > + return 0; > + } > + > + /* > + * The DT will have one INTC DT node under each CPU (or HART) > + * DT node so riscv_intc_init() function will be called once > + * for each INTC DT node. We only need to do INTC initialization > + * for the INTC DT node belonging to boot CPU (or boot HART). > + */ > + if (riscv_hartid_to_cpuid(hartid) != smp_processor_id()) > + return 0; > + > + intc_domain = irq_domain_add_linear(node, BITS_PER_LONG, > + &riscv_intc_domain_ops, NULL); > + if (!intc_domain) { > + rc = -ENXIO; > + pr_err("unable to add IRQ domain\n"); > + return rc; Also written as "return -ENXIO". > + } > + > + rc = set_handle_irq(&riscv_intc_irq); > + if (rc) { > + pr_err("failed to set irq handler\n"); > + return rc; > + } > + > + cpuhp_setup_state(CPUHP_AP_IRQ_RISCV_STARTING, > + "irqchip/riscv/intc:starting", > + riscv_intc_cpu_starting, > + riscv_intc_cpu_dying); > + > + pr_info("%d local interrupts mapped\n", BITS_PER_LONG); > + > + return 0; > +} > + > +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init); > diff --git a/drivers/irqchip/irq-sifive-plic.c > b/drivers/irqchip/irq-sifive-plic.c > index 16d31d114c30..cda2e246aee3 100644 > --- a/drivers/irqchip/irq-sifive-plic.c > +++ b/drivers/irqchip/irq-sifive-plic.c > @@ -9,6 +9,7 @@ > #include <linux/io.h> > #include <linux/irq.h> > #include <linux/irqchip.h> > +#include <linux/irqchip/chained_irq.h> > #include <linux/irqdomain.h> > #include <linux/module.h> > #include <linux/of.h> > @@ -76,6 +77,7 @@ struct plic_handler { > void __iomem *enable_base; > struct plic_priv *priv; > }; > +static int plic_parent_irq; > static bool plic_cpuhp_setup_done; > static DEFINE_PER_CPU(struct plic_handler, plic_handlers); > > @@ -219,15 +221,17 @@ static const struct irq_domain_ops > plic_irqdomain_ops = { > * that source ID back to the same claim register. This automatically > enables > * and disables the interrupt, so there's nothing else to do. > */ > -static void plic_handle_irq(struct pt_regs *regs) > +static void plic_handle_irq(struct irq_desc *desc) > { > struct plic_handler *handler = this_cpu_ptr(&plic_handlers); > + struct irq_chip *chip = irq_desc_get_chip(desc); > void __iomem *claim = handler->hart_base + CONTEXT_CLAIM; > irq_hw_number_t hwirq; > > WARN_ON_ONCE(!handler->present); > > - csr_clear(CSR_IE, IE_EIE); > + chained_irq_enter(chip, desc); > + > while ((hwirq = readl(claim))) { > int irq = irq_find_mapping(handler->priv->irqdomain, hwirq); > > @@ -237,7 +241,8 @@ static void plic_handle_irq(struct pt_regs *regs) > else > generic_handle_irq(irq); > } > - csr_set(CSR_IE, IE_EIE); > + > + chained_irq_exit(chip, desc); > } > > static void plic_set_threshold(struct plic_handler *handler, u32 > threshold) > @@ -250,7 +255,8 @@ static int plic_dying_cpu(unsigned int cpu) > { > struct plic_handler *handler = this_cpu_ptr(&plic_handlers); > > - csr_clear(CSR_IE, IE_EIE); > + if (plic_parent_irq) > + disable_percpu_irq(plic_parent_irq); Given what is below, is that something that can happen if plic_parent_irq is 0? > plic_set_threshold(handler, PLIC_DISABLE_THRESHOLD); Why do you need to both disable the interrupt *and* change the priority threshold? It seems to be that one of them should be enough, but my kno9wledge of the PLIC is limited. In any case, this would deserve a comment. > > return 0; > @@ -260,7 +266,11 @@ static int plic_starting_cpu(unsigned int cpu) > { > struct plic_handler *handler = this_cpu_ptr(&plic_handlers); > > - csr_set(CSR_IE, IE_EIE); > + if (plic_parent_irq) > + enable_percpu_irq(plic_parent_irq, > + irq_get_trigger_type(plic_parent_irq)); > + else > + pr_warn("cpu%d: parent irq not available\n"); What does it mean to carry on if the interrupt cannot be signaled? Shouldn't you error out instead, and leave the CPU dead? > plic_set_threshold(handler, PLIC_ENABLE_THRESHOLD); > > return 0; > @@ -273,6 +283,7 @@ static int __init plic_init(struct device_node > *node, > u32 nr_irqs; > struct plic_priv *priv; > struct plic_handler *handler; > + struct of_phandle_args intc_args; Unused variable? > > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > if (!priv) > @@ -328,6 +339,14 @@ static int __init plic_init(struct device_node > *node, > continue; > } > > + /* Find parent domain and register chained handler */ > + if (!plic_parent_irq && irq_find_host(parent.np)) { > + plic_parent_irq = irq_of_parse_and_map(node, i); > + if (plic_parent_irq) > + irq_set_chained_handler(plic_parent_irq, > + plic_handle_irq); > + } > + > /* > * When running in M-mode we need to ignore the S-mode handler. > * Here we assume it always comes later, but that might be a > @@ -368,7 +387,6 @@ static int __init plic_init(struct device_node > *node, > > pr_info("%pOFP: mapped %d interrupts with %d handlers for" > " %d contexts.\n", node, nr_irqs, nr_handlers, nr_contexts); > - set_handle_irq(plic_handle_irq); > return 0; > > out_iounmap: > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h > index 77d70b633531..57b1f8f777d9 100644 > --- a/include/linux/cpuhotplug.h > +++ b/include/linux/cpuhotplug.h > @@ -102,6 +102,7 @@ enum cpuhp_state { > CPUHP_AP_IRQ_ARMADA_XP_STARTING, > CPUHP_AP_IRQ_BCM2836_STARTING, > CPUHP_AP_IRQ_MIPS_GIC_STARTING, > + CPUHP_AP_IRQ_RISCV_STARTING, > CPUHP_AP_IRQ_SIFIVE_PLIC_STARTING, > CPUHP_AP_ARM_MVEBU_COHERENCY, > CPUHP_AP_MICROCODE_LOADER, Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver 2020-05-30 12:01 ` Marc Zyngier @ 2020-05-31 5:36 ` Anup Patel 2020-05-31 9:33 ` Marc Zyngier 0 siblings, 1 reply; 18+ messages in thread From: Anup Patel @ 2020-05-31 5:36 UTC (permalink / raw) To: Marc Zyngier Cc: Anup Patel, Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Atish Patra, Alistair Francis, linux-riscv, linux-kernel@vger.kernel.org List, Palmer Dabbelt On Sat, May 30, 2020 at 5:31 PM Marc Zyngier <maz@kernel.org> wrote: > > On 2020-05-30 11:07, Anup Patel wrote: > > The RISC-V per-HART local interrupt controller manages software > > interrupts, timer interrupts, external interrupts (which are routed > > via the platform level interrupt controller) and other per-HART > > local interrupts. > > > > This patch adds a driver for the RISC-V local interrupt controller. > > Please don't use "This patch...". Say something like: > > "Add a driver driver for this component, which eventually replaces > the RISC-V architecture code, allowing for a better split between > arch code and drivers." Okay, will update. > > > It is a major re-write over perviously submitted version. > > (Refer, https://www.spinics.net/lists/devicetree/msg241230.html) > > I'm not sure that's very useful. Again, this is better placed in the > cover letter, and can be retrived via the Link: tags. Okay, will update. > > > The driver is compliant with RISC-V Hart-Level Interrupt Controller > > DT bindings located at: > > Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt > > > > Co-developed-by: Palmer Dabbelt <palmer@dabbelt.com> > > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com> > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > Acked-by: Palmer Dabbelt <palmerdabbelt@google.com> > > --- > > arch/riscv/Kconfig | 1 + > > arch/riscv/include/asm/irq.h | 2 - > > arch/riscv/kernel/irq.c | 33 +------ > > arch/riscv/kernel/traps.c | 2 - > > drivers/irqchip/Kconfig | 13 +++ > > drivers/irqchip/Makefile | 1 + > > drivers/irqchip/irq-riscv-intc.c | 148 ++++++++++++++++++++++++++++++ > > drivers/irqchip/irq-sifive-plic.c | 30 ++++-- > > include/linux/cpuhotplug.h | 1 + > > 9 files changed, 191 insertions(+), 40 deletions(-) > > create mode 100644 drivers/irqchip/irq-riscv-intc.c > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > index 90a008e28f7e..822cb0e1a380 100644 > > --- a/arch/riscv/Kconfig > > +++ b/arch/riscv/Kconfig > > @@ -40,6 +40,7 @@ config RISCV > > select HAVE_PERF_REGS > > select HAVE_PERF_USER_STACK_DUMP > > select HAVE_SYSCALL_TRACEPOINTS > > + select HANDLE_DOMAIN_IRQ > > select IRQ_DOMAIN > > select SPARSE_IRQ > > select SYSCTL_EXCEPTION_TRACE > > diff --git a/arch/riscv/include/asm/irq.h > > b/arch/riscv/include/asm/irq.h > > index 0183e15ace66..a9e5f07a7e9c 100644 > > --- a/arch/riscv/include/asm/irq.h > > +++ b/arch/riscv/include/asm/irq.h > > @@ -10,8 +10,6 @@ > > #include <linux/interrupt.h> > > #include <linux/linkage.h> > > > > -#define NR_IRQS 0 > > - > > void riscv_timer_interrupt(void); > > > > #include <asm-generic/irq.h> > > diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c > > index bb0bfcd537e7..eb8777642ce6 100644 > > --- a/arch/riscv/kernel/irq.c > > +++ b/arch/riscv/kernel/irq.c > > @@ -7,7 +7,6 @@ > > > > #include <linux/interrupt.h> > > #include <linux/irqchip.h> > > -#include <linux/irqdomain.h> > > #include <linux/seq_file.h> > > #include <asm/smp.h> > > > > @@ -19,39 +18,13 @@ int arch_show_interrupts(struct seq_file *p, int > > prec) > > > > asmlinkage __visible void __irq_entry do_IRQ(struct pt_regs *regs) > > { > > - struct pt_regs *old_regs; > > - > > - switch (regs->cause & ~CAUSE_IRQ_FLAG) { > > - case RV_IRQ_TIMER: > > - old_regs = set_irq_regs(regs); > > - irq_enter(); > > - riscv_timer_interrupt(); > > - irq_exit(); > > - set_irq_regs(old_regs); > > - break; > > -#ifdef CONFIG_SMP > > - case RV_IRQ_SOFT: > > - /* > > - * We only use software interrupts to pass IPIs, so if a non-SMP > > - * system gets one, then we don't know what to do. > > - */ > > - handle_IPI(regs); > > - break; > > -#endif > > - case RV_IRQ_EXT: > > - old_regs = set_irq_regs(regs); > > - irq_enter(); > > + if (handle_arch_irq) > > handle_arch_irq(regs); > > - irq_exit(); > > - set_irq_regs(old_regs); > > - break; > > - default: > > - pr_alert("unexpected interrupt cause 0x%lx", regs->cause); > > - BUG(); > > - } > > } > > > > void __init init_IRQ(void) > > { > > irqchip_init(); > > + if (!handle_arch_irq) > > + panic("No interrupt controller found."); > > } > > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c > > index 7f58fa53033f..f48c76aadbf3 100644 > > --- a/arch/riscv/kernel/traps.c > > +++ b/arch/riscv/kernel/traps.c > > @@ -178,6 +178,4 @@ void trap_init(void) > > csr_write(CSR_SCRATCH, 0); > > /* Set the exception vector address */ > > csr_write(CSR_TVEC, &handle_exception); > > - /* Enable interrupts */ > > - csr_write(CSR_IE, IE_SIE); > > } > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > > index a85aada04a64..95d6137a8117 100644 > > --- a/drivers/irqchip/Kconfig > > +++ b/drivers/irqchip/Kconfig > > @@ -493,6 +493,19 @@ config TI_SCI_INTA_IRQCHIP > > If you wish to use interrupt aggregator irq resources managed by > > the > > TI System Controller, say Y here. Otherwise, say N. > > > > +config RISCV_INTC > > + bool "RISC-V Local Interrupt Controller" > > + depends on RISCV > > + default y > > + help > > + This enables support for the per-HART local interrupt controller > > + found in standard RISC-V systems. The per-HART local interrupt > > + controller handles timer interrupts, software interrupts, and > > + hardware interrupts. Without a per-HART local interrupt > > controller, > > + a RISC-V system will be unable to handle any interrupts. > > + > > + If you don't know what to do here, say Y. > > + > > config SIFIVE_PLIC > > bool "SiFive Platform-Level Interrupt Controller" > > depends on RISCV > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > > index 37bbe39bf909..b8319f045472 100644 > > --- a/drivers/irqchip/Makefile > > +++ b/drivers/irqchip/Makefile > > @@ -98,6 +98,7 @@ obj-$(CONFIG_NDS32) += irq-ativic32.o > > obj-$(CONFIG_QCOM_PDC) += qcom-pdc.o > > obj-$(CONFIG_CSKY_MPINTC) += irq-csky-mpintc.o > > obj-$(CONFIG_CSKY_APB_INTC) += irq-csky-apb-intc.o > > +obj-$(CONFIG_RISCV_INTC) += irq-riscv-intc.o > > obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o > > obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o > > obj-$(CONFIG_IMX_INTMUX) += irq-imx-intmux.o > > diff --git a/drivers/irqchip/irq-riscv-intc.c > > b/drivers/irqchip/irq-riscv-intc.c > > new file mode 100644 > > index 000000000000..84e7bda3a090 > > --- /dev/null > > +++ b/drivers/irqchip/irq-riscv-intc.c > > @@ -0,0 +1,148 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2012 Regents of the University of California > > + * Copyright (C) 2017-2018 SiFive > > + * Copyright (C) 2020 Western Digital Corporation or its affiliates. > > + */ > > + > > +#define pr_fmt(fmt) "riscv-intc: " fmt > > +#include <linux/atomic.h> > > +#include <linux/bits.h> > > +#include <linux/cpu.h> > > +#include <linux/irq.h> > > +#include <linux/irqchip.h> > > +#include <linux/irqdomain.h> > > +#include <linux/interrupt.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/smp.h> > > + > > +static struct irq_domain *intc_domain; > > + > > +static asmlinkage void riscv_intc_irq(struct pt_regs *regs) > > +{ > > + struct pt_regs *old_regs; > > + unsigned long cause = regs->cause & ~CAUSE_IRQ_FLAG; > > + > > + if (unlikely(cause >= BITS_PER_LONG)) > > + panic("unexpected interrupt cause"); > > + > > + switch (cause) { > > + case RV_IRQ_TIMER: > > + old_regs = set_irq_regs(regs); > > + irq_enter(); > > + riscv_timer_interrupt(); > > + irq_exit(); > > + set_irq_regs(old_regs); > > + break; > > +#ifdef CONFIG_SMP > > + case RV_IRQ_SOFT: > > + /* > > + * We only use software interrupts to pass IPIs, so if a > > + * non-SMP system gets one, then we don't know what to do. > > + */ > > + handle_IPI(regs); > > + break; > > +#endif > > + default: > > + handle_domain_irq(intc_domain, cause, regs); > > + break; > > + } > > +} > > + > > +/* > > + * On RISC-V systems local interrupts are masked or unmasked by > > writing > > + * the SIE (Supervisor Interrupt Enable) CSR. As CSRs can only be > > written > > + * on the local hart, these functions can only be called on the hart > > that > > + * corresponds to the IRQ chip. > > + */ > > + > > +static void riscv_intc_irq_mask(struct irq_data *d) > > +{ > > + csr_clear(CSR_IE, 1UL << d->hwirq); > > As I mentionned earlier, BIT(d->hwirq) is the way to write this. Okay, will update here and below as well. > > > +} > > + > > +static void riscv_intc_irq_unmask(struct irq_data *d) > > +{ > > + csr_set(CSR_IE, 1UL << d->hwirq); > > +} > > + > > +static int riscv_intc_cpu_starting(unsigned int cpu) > > +{ > > + csr_write(CSR_IE, 1UL << RV_IRQ_SOFT); > > + csr_write(CSR_IP, 0); > > + return 0; > > +} > > + > > +static int riscv_intc_cpu_dying(unsigned int cpu) > > +{ > > + csr_clear(CSR_IE, 1UL << RV_IRQ_SOFT); > > + return 0; > > +} > > + > > +static struct irq_chip riscv_intc_chip = { > > + .name = "RISC-V INTC", > > + .irq_mask = riscv_intc_irq_mask, > > + .irq_unmask = riscv_intc_irq_unmask, > > +}; > > + > > +static int riscv_intc_domain_map(struct irq_domain *d, unsigned int > > irq, > > + irq_hw_number_t hwirq) > > +{ > > + irq_set_percpu_devid(irq); > > + irq_domain_set_info(d, irq, hwirq, &riscv_intc_chip, d->host_data, > > + handle_percpu_devid_irq, NULL, NULL); > > + > > + return 0; > > +} > > + > > +static const struct irq_domain_ops riscv_intc_domain_ops = { > > + .map = riscv_intc_domain_map, > > + .xlate = irq_domain_xlate_onecell, > > +}; > > + > > +static int __init riscv_intc_init(struct device_node *node, > > + struct device_node *parent) > > +{ > > + int rc, hartid; > > + > > + hartid = riscv_of_parent_hartid(node); > > + if (hartid < 0) { > > + pr_warn("unable to fine hart id for %pOF\n", node); > > + return 0; > > + } > > + > > + /* > > + * The DT will have one INTC DT node under each CPU (or HART) > > + * DT node so riscv_intc_init() function will be called once > > + * for each INTC DT node. We only need to do INTC initialization > > + * for the INTC DT node belonging to boot CPU (or boot HART). > > + */ > > + if (riscv_hartid_to_cpuid(hartid) != smp_processor_id()) > > + return 0; > > + > > + intc_domain = irq_domain_add_linear(node, BITS_PER_LONG, > > + &riscv_intc_domain_ops, NULL); > > + if (!intc_domain) { > > + rc = -ENXIO; > > + pr_err("unable to add IRQ domain\n"); > > + return rc; > > Also written as "return -ENXIO". Okay, will update. > > > + } > > + > > + rc = set_handle_irq(&riscv_intc_irq); > > + if (rc) { > > + pr_err("failed to set irq handler\n"); > > + return rc; > > + } > > + > > + cpuhp_setup_state(CPUHP_AP_IRQ_RISCV_STARTING, > > + "irqchip/riscv/intc:starting", > > + riscv_intc_cpu_starting, > > + riscv_intc_cpu_dying); > > + > > + pr_info("%d local interrupts mapped\n", BITS_PER_LONG); > > + > > + return 0; > > +} > > + > > +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init); > > diff --git a/drivers/irqchip/irq-sifive-plic.c > > b/drivers/irqchip/irq-sifive-plic.c > > index 16d31d114c30..cda2e246aee3 100644 > > --- a/drivers/irqchip/irq-sifive-plic.c > > +++ b/drivers/irqchip/irq-sifive-plic.c > > @@ -9,6 +9,7 @@ > > #include <linux/io.h> > > #include <linux/irq.h> > > #include <linux/irqchip.h> > > +#include <linux/irqchip/chained_irq.h> > > #include <linux/irqdomain.h> > > #include <linux/module.h> > > #include <linux/of.h> > > @@ -76,6 +77,7 @@ struct plic_handler { > > void __iomem *enable_base; > > struct plic_priv *priv; > > }; > > +static int plic_parent_irq; > > static bool plic_cpuhp_setup_done; > > static DEFINE_PER_CPU(struct plic_handler, plic_handlers); > > > > @@ -219,15 +221,17 @@ static const struct irq_domain_ops > > plic_irqdomain_ops = { > > * that source ID back to the same claim register. This automatically > > enables > > * and disables the interrupt, so there's nothing else to do. > > */ > > -static void plic_handle_irq(struct pt_regs *regs) > > +static void plic_handle_irq(struct irq_desc *desc) > > { > > struct plic_handler *handler = this_cpu_ptr(&plic_handlers); > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > void __iomem *claim = handler->hart_base + CONTEXT_CLAIM; > > irq_hw_number_t hwirq; > > > > WARN_ON_ONCE(!handler->present); > > > > - csr_clear(CSR_IE, IE_EIE); > > + chained_irq_enter(chip, desc); > > + > > while ((hwirq = readl(claim))) { > > int irq = irq_find_mapping(handler->priv->irqdomain, hwirq); > > > > @@ -237,7 +241,8 @@ static void plic_handle_irq(struct pt_regs *regs) > > else > > generic_handle_irq(irq); > > } > > - csr_set(CSR_IE, IE_EIE); > > + > > + chained_irq_exit(chip, desc); > > } > > > > static void plic_set_threshold(struct plic_handler *handler, u32 > > threshold) > > @@ -250,7 +255,8 @@ static int plic_dying_cpu(unsigned int cpu) > > { > > struct plic_handler *handler = this_cpu_ptr(&plic_handlers); > > > > - csr_clear(CSR_IE, IE_EIE); > > + if (plic_parent_irq) > > + disable_percpu_irq(plic_parent_irq); > > Given what is below, is that something that can happen if > plic_parent_irq is 0? For multiple PLIC instances, the plic_init() will be called once for each PLIC instance. The "interrupts-extended" property of one of the PLIC DT node must point to boot CPU INTC DT node for plic_parent_irq to be found. The plic_parent_irq can be 0 only if none of PLIC DT nodes point to boot CPU INTC DT node. > > > plic_set_threshold(handler, PLIC_DISABLE_THRESHOLD); > > Why do you need to both disable the interrupt *and* change the priority > threshold? It seems to be that one of them should be enough, but my > kno9wledge of the PLIC is limited. In any case, this would deserve a > comment. Okay, I will test and remove "disable the interrupt" part from plic_dying_cpu(). > > > > > return 0; > > @@ -260,7 +266,11 @@ static int plic_starting_cpu(unsigned int cpu) > > { > > struct plic_handler *handler = this_cpu_ptr(&plic_handlers); > > > > - csr_set(CSR_IE, IE_EIE); > > + if (plic_parent_irq) > > + enable_percpu_irq(plic_parent_irq, > > + irq_get_trigger_type(plic_parent_irq)); > > + else > > + pr_warn("cpu%d: parent irq not available\n"); > > What does it mean to carry on if the interrupt cannot be signaled? > Shouldn't you error out instead, and leave the CPU dead? The CPU is not dead if we cannot enable RISC-V INTC external interrupt because the Timer and IPIs interrupts are always through RISC-V INTC. The PLIC external interrupt not present for a CPU only means that that CPU cannot receive peripherial interrupts. On a sane RISC-V system, if PLIC is present then all CPUs should be able to get RISC-V INTC external interrupt. Base on this rationale, I have put a warning for plic_parent_irq == 0. > > > plic_set_threshold(handler, PLIC_ENABLE_THRESHOLD); > > > > return 0; > > @@ -273,6 +283,7 @@ static int __init plic_init(struct device_node > > *node, > > u32 nr_irqs; > > struct plic_priv *priv; > > struct plic_handler *handler; > > + struct of_phandle_args intc_args; > > Unused variable? Okay, will update. > > > > > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > if (!priv) > > @@ -328,6 +339,14 @@ static int __init plic_init(struct device_node > > *node, > > continue; > > } > > > > + /* Find parent domain and register chained handler */ > > + if (!plic_parent_irq && irq_find_host(parent.np)) { > > + plic_parent_irq = irq_of_parse_and_map(node, i); > > + if (plic_parent_irq) > > + irq_set_chained_handler(plic_parent_irq, > > + plic_handle_irq); > > + } > > + > > /* > > * When running in M-mode we need to ignore the S-mode handler. > > * Here we assume it always comes later, but that might be a > > @@ -368,7 +387,6 @@ static int __init plic_init(struct device_node > > *node, > > > > pr_info("%pOFP: mapped %d interrupts with %d handlers for" > > " %d contexts.\n", node, nr_irqs, nr_handlers, nr_contexts); > > - set_handle_irq(plic_handle_irq); > > return 0; > > > > out_iounmap: > > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h > > index 77d70b633531..57b1f8f777d9 100644 > > --- a/include/linux/cpuhotplug.h > > +++ b/include/linux/cpuhotplug.h > > @@ -102,6 +102,7 @@ enum cpuhp_state { > > CPUHP_AP_IRQ_ARMADA_XP_STARTING, > > CPUHP_AP_IRQ_BCM2836_STARTING, > > CPUHP_AP_IRQ_MIPS_GIC_STARTING, > > + CPUHP_AP_IRQ_RISCV_STARTING, > > CPUHP_AP_IRQ_SIFIVE_PLIC_STARTING, > > CPUHP_AP_ARM_MVEBU_COHERENCY, > > CPUHP_AP_MICROCODE_LOADER, > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... Regards, Anup ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver 2020-05-31 5:36 ` Anup Patel @ 2020-05-31 9:33 ` Marc Zyngier 2020-05-31 10:06 ` Anup Patel 0 siblings, 1 reply; 18+ messages in thread From: Marc Zyngier @ 2020-05-31 9:33 UTC (permalink / raw) To: Anup Patel Cc: Anup Patel, Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Atish Patra, Alistair Francis, linux-riscv, linux-kernel@vger.kernel.org List, Palmer Dabbelt On 2020-05-31 06:36, Anup Patel wrote: > On Sat, May 30, 2020 at 5:31 PM Marc Zyngier <maz@kernel.org> wrote: [...] >> > plic_set_threshold(handler, PLIC_DISABLE_THRESHOLD); >> >> Why do you need to both disable the interrupt *and* change the >> priority >> threshold? It seems to be that one of them should be enough, but my >> kno9wledge of the PLIC is limited. In any case, this would deserve a >> comment. > > Okay, I will test and remove "disable the interrupt" part from > plic_dying_cpu(). Be careful, as interrupt enabling/disabling is refcounted in order to allow nesting. If you only enable on CPU_ON and not disable on CPU_OFF, you will end-up with a depth that only increases, up to the point where you hit the roof (it will take a while though). I would keep the enable/disable as is, and drop the priority setting from the CPU_OFF path. >> > return 0; >> > @@ -260,7 +266,11 @@ static int plic_starting_cpu(unsigned int cpu) >> > { >> > struct plic_handler *handler = this_cpu_ptr(&plic_handlers); >> > >> > - csr_set(CSR_IE, IE_EIE); >> > + if (plic_parent_irq) >> > + enable_percpu_irq(plic_parent_irq, >> > + irq_get_trigger_type(plic_parent_irq)); >> > + else >> > + pr_warn("cpu%d: parent irq not available\n"); >> >> What does it mean to carry on if the interrupt cannot be signaled? >> Shouldn't you error out instead, and leave the CPU dead? > > The CPU is not dead if we cannot enable RISC-V INTC external > interrupt because the Timer and IPIs interrupts are always through > RISC-V INTC. The PLIC external interrupt not present for a CPU > only means that that CPU cannot receive peripherial interrupts. > > On a sane RISC-V system, if PLIC is present then all CPUs should > be able to get RISC-V INTC external interrupt. Base on this rationale, > I have put a warning for plic_parent_irq == 0. Fair enough. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver 2020-05-31 9:33 ` Marc Zyngier @ 2020-05-31 10:06 ` Anup Patel 2020-05-31 10:53 ` Marc Zyngier 0 siblings, 1 reply; 18+ messages in thread From: Anup Patel @ 2020-05-31 10:06 UTC (permalink / raw) To: Marc Zyngier Cc: Anup Patel, Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Atish Patra, Alistair Francis, linux-riscv, linux-kernel@vger.kernel.org List, Palmer Dabbelt On Sun, May 31, 2020 at 3:03 PM Marc Zyngier <maz@kernel.org> wrote: > > On 2020-05-31 06:36, Anup Patel wrote: > > On Sat, May 30, 2020 at 5:31 PM Marc Zyngier <maz@kernel.org> wrote: > > [...] > > >> > plic_set_threshold(handler, PLIC_DISABLE_THRESHOLD); > >> > >> Why do you need to both disable the interrupt *and* change the > >> priority > >> threshold? It seems to be that one of them should be enough, but my > >> kno9wledge of the PLIC is limited. In any case, this would deserve a > >> comment. > > > > Okay, I will test and remove "disable the interrupt" part from > > plic_dying_cpu(). > > Be careful, as interrupt enabling/disabling is refcounted in order > to allow nesting. If you only enable on CPU_ON and not disable > on CPU_OFF, you will end-up with a depth that only increases, > up to the point where you hit the roof (it will take a while though). > > I would keep the enable/disable as is, and drop the priority > setting from the CPU_OFF path. The PLIC threshold is like GICv2 CPU interface enable/disable. Based on your comment, we should only program the PLIC threshold in CPU_ON and don't touch the PLIC threshold in CPU_OFF. Right?? > > >> > return 0; > >> > @@ -260,7 +266,11 @@ static int plic_starting_cpu(unsigned int cpu) > >> > { > >> > struct plic_handler *handler = this_cpu_ptr(&plic_handlers); > >> > > >> > - csr_set(CSR_IE, IE_EIE); > >> > + if (plic_parent_irq) > >> > + enable_percpu_irq(plic_parent_irq, > >> > + irq_get_trigger_type(plic_parent_irq)); > >> > + else > >> > + pr_warn("cpu%d: parent irq not available\n"); > >> > >> What does it mean to carry on if the interrupt cannot be signaled? > >> Shouldn't you error out instead, and leave the CPU dead? > > > > The CPU is not dead if we cannot enable RISC-V INTC external > > interrupt because the Timer and IPIs interrupts are always through > > RISC-V INTC. The PLIC external interrupt not present for a CPU > > only means that that CPU cannot receive peripherial interrupts. > > > > On a sane RISC-V system, if PLIC is present then all CPUs should > > be able to get RISC-V INTC external interrupt. Base on this rationale, > > I have put a warning for plic_parent_irq == 0. > > Fair enough. > > M. > -- > Jazz is not dead. It just smells funny... Regards, Anup ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver 2020-05-31 10:06 ` Anup Patel @ 2020-05-31 10:53 ` Marc Zyngier 2020-06-01 4:09 ` Anup Patel 0 siblings, 1 reply; 18+ messages in thread From: Marc Zyngier @ 2020-05-31 10:53 UTC (permalink / raw) To: Anup Patel Cc: Anup Patel, Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Atish Patra, Alistair Francis, linux-riscv, linux-kernel@vger.kernel.org List, Palmer Dabbelt On 2020-05-31 11:06, Anup Patel wrote: > On Sun, May 31, 2020 at 3:03 PM Marc Zyngier <maz@kernel.org> wrote: >> >> On 2020-05-31 06:36, Anup Patel wrote: >> > On Sat, May 30, 2020 at 5:31 PM Marc Zyngier <maz@kernel.org> wrote: >> >> [...] >> >> >> > plic_set_threshold(handler, PLIC_DISABLE_THRESHOLD); >> >> >> >> Why do you need to both disable the interrupt *and* change the >> >> priority >> >> threshold? It seems to be that one of them should be enough, but my >> >> kno9wledge of the PLIC is limited. In any case, this would deserve a >> >> comment. >> > >> > Okay, I will test and remove "disable the interrupt" part from >> > plic_dying_cpu(). >> >> Be careful, as interrupt enabling/disabling is refcounted in order >> to allow nesting. If you only enable on CPU_ON and not disable >> on CPU_OFF, you will end-up with a depth that only increases, >> up to the point where you hit the roof (it will take a while though). >> >> I would keep the enable/disable as is, and drop the priority >> setting from the CPU_OFF path. > > The PLIC threshold is like GICv2 CPU interface enable/disable. Looking at the documentation[1], that's not really a correct analogy: - The PLIC is far removed from the CPU, and is more akin a GICv3 distributor. The INTC itself is more like a GICv3 redistributor, as it deals with local interrupts only. I don't see anything in the RISC-V architecture that actually behaves like a GIC CPU interface (not necessarily a bad thing...). - The threshold register is not an ON/OFF, but a priority mask, similar to the GIC PMR (except that the PMR lives in the CPU interface and affects all interrupts targetting this CPU while this only seem to affect PLIC interrupts and not the INTC interrupts). You may be using it as an ON/OFF for now as you don't support multiple priorities yet, but that's just a SW thing. > Based on your comment, we should only program the PLIC threshold > in CPU_ON and don't touch the PLIC threshold in CPU_OFF. Right?? This seems like the correct thing to do. M. [1] https://sifive.cdn.prismic.io/sifive%2Fdc4980ff-17db-448b-b521-4c7ab26b7488_sifive+u54-mc+manual+v19.08.pdf -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver 2020-05-31 10:53 ` Marc Zyngier @ 2020-06-01 4:09 ` Anup Patel 2020-06-01 7:41 ` Marc Zyngier 2020-06-01 9:33 ` Guo Ren 0 siblings, 2 replies; 18+ messages in thread From: Anup Patel @ 2020-06-01 4:09 UTC (permalink / raw) To: Marc Zyngier Cc: Anup Patel, Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Atish Patra, Alistair Francis, linux-riscv, linux-kernel@vger.kernel.org List, Palmer Dabbelt On Sun, May 31, 2020 at 4:23 PM Marc Zyngier <maz@kernel.org> wrote: > > On 2020-05-31 11:06, Anup Patel wrote: > > On Sun, May 31, 2020 at 3:03 PM Marc Zyngier <maz@kernel.org> wrote: > >> > >> On 2020-05-31 06:36, Anup Patel wrote: > >> > On Sat, May 30, 2020 at 5:31 PM Marc Zyngier <maz@kernel.org> wrote: > >> > >> [...] > >> > >> >> > plic_set_threshold(handler, PLIC_DISABLE_THRESHOLD); > >> >> > >> >> Why do you need to both disable the interrupt *and* change the > >> >> priority > >> >> threshold? It seems to be that one of them should be enough, but my > >> >> kno9wledge of the PLIC is limited. In any case, this would deserve a > >> >> comment. > >> > > >> > Okay, I will test and remove "disable the interrupt" part from > >> > plic_dying_cpu(). > >> > >> Be careful, as interrupt enabling/disabling is refcounted in order > >> to allow nesting. If you only enable on CPU_ON and not disable > >> on CPU_OFF, you will end-up with a depth that only increases, > >> up to the point where you hit the roof (it will take a while though). > >> > >> I would keep the enable/disable as is, and drop the priority > >> setting from the CPU_OFF path. > > > > The PLIC threshold is like GICv2 CPU interface enable/disable. > > Looking at the documentation[1], that's not really a correct analogy: > > - The PLIC is far removed from the CPU, and is more akin a GICv3 > distributor. The INTC itself is more like a GICv3 redistributor, > as it deals with local interrupts only. I don't see anything > in the RISC-V architecture that actually behaves like a GIC > CPU interface (not necessarily a bad thing...). > > - The threshold register is not an ON/OFF, but a priority mask, > similar to the GIC PMR (except that the PMR lives in the CPU > interface and affects all interrupts targetting this CPU while > this only seem to affect PLIC interrupts and not the INTC interrupts). > You may be using it as an ON/OFF for now as you don't support > multiple priorities yet, but that's just a SW thing. Yes, your analogy is correct. BTW, PLIC does not handle MSI and does not have virtualization support pass-through interrupts. We will most likely see a new RISC-V interrupt controller spec for these capabilities. Also, the PLIC spec is now owned by RISC-V foundation (not SiFive) so we will have to rename the driver to "irq-riscv-plic" and will have a new generic compatible string "riscv,plic-1.0.0". One of us (me or Palmer) will send separate patches for this renaming. I hope you will be fine with this?? (Refer, https://github.com/riscv/riscv-plic-spec) > > > Based on your comment, we should only program the PLIC threshold > > in CPU_ON and don't touch the PLIC threshold in CPU_OFF. Right?? > > This seems like the correct thing to do. Sure, I will update. > > M. > > [1] > https://sifive.cdn.prismic.io/sifive%2Fdc4980ff-17db-448b-b521-4c7ab26b7488_sifive+u54-mc+manual+v19.08.pdf > -- > Jazz is not dead. It just smells funny... Regards, Anup ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver 2020-06-01 4:09 ` Anup Patel @ 2020-06-01 7:41 ` Marc Zyngier 2020-06-01 9:13 ` Anup Patel 2020-06-01 9:33 ` Guo Ren 1 sibling, 1 reply; 18+ messages in thread From: Marc Zyngier @ 2020-06-01 7:41 UTC (permalink / raw) To: Anup Patel Cc: Anup Patel, Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Atish Patra, Alistair Francis, linux-riscv, linux-kernel@vger.kernel.org List, Palmer Dabbelt On 2020-06-01 05:09, Anup Patel wrote: > On Sun, May 31, 2020 at 4:23 PM Marc Zyngier <maz@kernel.org> wrote: >> >> On 2020-05-31 11:06, Anup Patel wrote: [...] > Also, the PLIC spec is now owned by RISC-V foundation (not SiFive) so > we will have to rename the driver to "irq-riscv-plic" and will have a > new > generic compatible string "riscv,plic-1.0.0". One of us (me or Palmer) > will > send separate patches for this renaming. I hope you will be fine with > this?? > (Refer, https://github.com/riscv/riscv-plic-spec) Do we really need the churn of a renaming? A new compatible, and maybe a new config option should be enough, no? What does the renaming give us? Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver 2020-06-01 7:41 ` Marc Zyngier @ 2020-06-01 9:13 ` Anup Patel 0 siblings, 0 replies; 18+ messages in thread From: Anup Patel @ 2020-06-01 9:13 UTC (permalink / raw) To: Marc Zyngier Cc: Anup Patel, Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Atish Patra, Alistair Francis, linux-riscv, linux-kernel@vger.kernel.org List, Palmer Dabbelt On Mon, Jun 1, 2020 at 1:11 PM Marc Zyngier <maz@kernel.org> wrote: > > On 2020-06-01 05:09, Anup Patel wrote: > > On Sun, May 31, 2020 at 4:23 PM Marc Zyngier <maz@kernel.org> wrote: > >> > >> On 2020-05-31 11:06, Anup Patel wrote: > > [...] > > > Also, the PLIC spec is now owned by RISC-V foundation (not SiFive) so > > we will have to rename the driver to "irq-riscv-plic" and will have a > > new > > generic compatible string "riscv,plic-1.0.0". One of us (me or Palmer) > > will > > send separate patches for this renaming. I hope you will be fine with > > this?? > > (Refer, https://github.com/riscv/riscv-plic-spec) > > Do we really need the churn of a renaming? A new compatible, and maybe > a new config option should be enough, no? What does the renaming give > us? I thought renaming the file would be good to reflect ownership of RISC-V PLIC spec but I guess renaming just Kconfig option and new compatible string is fine as well. Regards, Anup ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver 2020-06-01 4:09 ` Anup Patel 2020-06-01 7:41 ` Marc Zyngier @ 2020-06-01 9:33 ` Guo Ren 1 sibling, 0 replies; 18+ messages in thread From: Guo Ren @ 2020-06-01 9:33 UTC (permalink / raw) To: Anup Patel Cc: Marc Zyngier, Daniel Lezcano, Jason Cooper, Anup Patel, linux-kernel@vger.kernel.org List, Atish Patra, Albert Ou, Palmer Dabbelt, Paul Walmsley, Palmer Dabbelt, Alistair Francis, Thomas Gleixner, linux-riscv Hi Anup, On Mon, Jun 1, 2020 at 12:09 PM Anup Patel <anup@brainfault.org> wrote: > > On Sun, May 31, 2020 at 4:23 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On 2020-05-31 11:06, Anup Patel wrote: > > > On Sun, May 31, 2020 at 3:03 PM Marc Zyngier <maz@kernel.org> wrote: > > >> > > >> On 2020-05-31 06:36, Anup Patel wrote: > > >> > On Sat, May 30, 2020 at 5:31 PM Marc Zyngier <maz@kernel.org> wrote: > > >> > > >> [...] > > >> > > >> >> > plic_set_threshold(handler, PLIC_DISABLE_THRESHOLD); > > >> >> > > >> >> Why do you need to both disable the interrupt *and* change the > > >> >> priority > > >> >> threshold? It seems to be that one of them should be enough, but my > > >> >> kno9wledge of the PLIC is limited. In any case, this would deserve a > > >> >> comment. > > >> > > > >> > Okay, I will test and remove "disable the interrupt" part from > > >> > plic_dying_cpu(). > > >> > > >> Be careful, as interrupt enabling/disabling is refcounted in order > > >> to allow nesting. If you only enable on CPU_ON and not disable > > >> on CPU_OFF, you will end-up with a depth that only increases, > > >> up to the point where you hit the roof (it will take a while though). > > >> > > >> I would keep the enable/disable as is, and drop the priority > > >> setting from the CPU_OFF path. > > > > > > The PLIC threshold is like GICv2 CPU interface enable/disable. > > > > Looking at the documentation[1], that's not really a correct analogy: > > > > - The PLIC is far removed from the CPU, and is more akin a GICv3 > > distributor. The INTC itself is more like a GICv3 redistributor, > > as it deals with local interrupts only. I don't see anything > > in the RISC-V architecture that actually behaves like a GIC > > CPU interface (not necessarily a bad thing...). > > > > - The threshold register is not an ON/OFF, but a priority mask, > > similar to the GIC PMR (except that the PMR lives in the CPU > > interface and affects all interrupts targetting this CPU while > > this only seem to affect PLIC interrupts and not the INTC interrupts). > > You may be using it as an ON/OFF for now as you don't support > > multiple priorities yet, but that's just a SW thing. > > Yes, your analogy is correct. > > BTW, PLIC does not handle MSI and does not have virtualization support > pass-through interrupts. We will most likely see a new RISC-V interrupt > controller spec for these capabilities. > > Also, the PLIC spec is now owned by RISC-V foundation (not SiFive) so > we will have to rename the driver to "irq-riscv-plic" and will have a new > generic compatible string "riscv,plic-1.0.0". One of us (me or Palmer) will > send separate patches for this renaming. I hope you will be fine with this?? > (Refer, https://github.com/riscv/riscv-plic-spec) That's great, we follow riscv-plic in hardware, but don't want to use sifive string in dts. Acked & Thx for the job. -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 4/6] clocksource/drivers/timer-riscv: Use per-CPU timer interrupt 2020-05-30 10:07 [PATCH v6 0/6] New RISC-V Local Interrupt Controller Driver Anup Patel ` (2 preceding siblings ...) 2020-05-30 10:07 ` [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver Anup Patel @ 2020-05-30 10:07 ` Anup Patel 2020-05-30 11:41 ` Marc Zyngier 2020-05-30 10:07 ` [PATCH v6 5/6] RISC-V: Remove do_IRQ() function Anup Patel 2020-05-30 10:07 ` [PATCH v6 6/6] RISC-V: Force select RISCV_INTC for CONFIG_RISCV Anup Patel 5 siblings, 1 reply; 18+ messages in thread From: Anup Patel @ 2020-05-30 10:07 UTC (permalink / raw) To: Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Marc Zyngier Cc: Atish Patra, Alistair Francis, Anup Patel, linux-riscv, linux-kernel, Anup Patel Instead of directly calling RISC-V timer interrupt handler from RISC-V local interrupt conntroller driver, this patch implements RISC-V timer interrupt as a per-CPU interrupt using per-CPU APIs of Linux IRQ subsystem. Signed-off-by: Anup Patel <anup.patel@wdc.com> Reviewed-by: Atish Patra <atish.patra@wdc.com> --- arch/riscv/include/asm/irq.h | 2 -- drivers/clocksource/timer-riscv.c | 41 ++++++++++++++++++++++++++++--- drivers/irqchip/irq-riscv-intc.c | 8 ------ 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h index a9e5f07a7e9c..9807ad164015 100644 --- a/arch/riscv/include/asm/irq.h +++ b/arch/riscv/include/asm/irq.h @@ -10,8 +10,6 @@ #include <linux/interrupt.h> #include <linux/linkage.h> -void riscv_timer_interrupt(void); - #include <asm-generic/irq.h> #endif /* _ASM_RISCV_IRQ_H */ diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c index c4f15c4068c0..1fe847983f50 100644 --- a/drivers/clocksource/timer-riscv.c +++ b/drivers/clocksource/timer-riscv.c @@ -12,8 +12,11 @@ #include <linux/cpu.h> #include <linux/delay.h> #include <linux/irq.h> +#include <linux/irqdomain.h> #include <linux/sched_clock.h> #include <linux/io-64-nonatomic-lo-hi.h> +#include <linux/interrupt.h> +#include <linux/of_irq.h> #include <asm/smp.h> #include <asm/sbi.h> @@ -39,6 +42,7 @@ static int riscv_clock_next_event(unsigned long delta, return 0; } +static unsigned int riscv_clock_event_irq; static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = { .name = "riscv_timer_clockevent", .features = CLOCK_EVT_FEAT_ONESHOT, @@ -74,30 +78,36 @@ static int riscv_timer_starting_cpu(unsigned int cpu) struct clock_event_device *ce = per_cpu_ptr(&riscv_clock_event, cpu); ce->cpumask = cpumask_of(cpu); + ce->irq = riscv_clock_event_irq; clockevents_config_and_register(ce, riscv_timebase, 100, 0x7fffffff); - csr_set(CSR_IE, IE_TIE); + enable_percpu_irq(riscv_clock_event_irq, + irq_get_trigger_type(riscv_clock_event_irq)); return 0; } static int riscv_timer_dying_cpu(unsigned int cpu) { - csr_clear(CSR_IE, IE_TIE); + disable_percpu_irq(riscv_clock_event_irq); return 0; } /* called directly from the low-level interrupt handler */ -void riscv_timer_interrupt(void) +static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id) { struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event); csr_clear(CSR_IE, IE_TIE); evdev->event_handler(evdev); + + return IRQ_HANDLED; } static int __init riscv_timer_init_dt(struct device_node *n) { int cpuid, hartid, error; + struct device_node *child; + struct irq_domain *domain; hartid = riscv_of_processor_hartid(n); if (hartid < 0) { @@ -115,6 +125,23 @@ static int __init riscv_timer_init_dt(struct device_node *n) if (cpuid != smp_processor_id()) return 0; + domain = NULL; + for_each_child_of_node(n, child) { + domain = irq_find_host(child); + if (domain) + break; + } + if (!domain) { + pr_err("Failed to find interrupt domain for node [%pOF]\n", n); + return -ENODEV; + } + + riscv_clock_event_irq = irq_create_mapping(domain, RV_IRQ_TIMER); + if (!riscv_clock_event_irq) { + pr_err("Failed to map timer interrupt for node [%pOF]\n", n); + return -ENODEV; + } + pr_info("%s: Registering clocksource cpuid [%d] hartid [%d]\n", __func__, cpuid, hartid); error = clocksource_register_hz(&riscv_clocksource, riscv_timebase); @@ -126,6 +153,14 @@ static int __init riscv_timer_init_dt(struct device_node *n) sched_clock_register(riscv_sched_clock, 64, riscv_timebase); + error = request_percpu_irq(riscv_clock_event_irq, + riscv_timer_interrupt, + "riscv-timer", &riscv_clock_event); + if (error) { + pr_err("registering percpu irq failed [%d]\n", error); + return error; + } + error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING, "clockevents/riscv/timer:starting", riscv_timer_starting_cpu, riscv_timer_dying_cpu); diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c index 84e7bda3a090..e6c07d8f3893 100644 --- a/drivers/irqchip/irq-riscv-intc.c +++ b/drivers/irqchip/irq-riscv-intc.c @@ -21,20 +21,12 @@ static struct irq_domain *intc_domain; static asmlinkage void riscv_intc_irq(struct pt_regs *regs) { - struct pt_regs *old_regs; unsigned long cause = regs->cause & ~CAUSE_IRQ_FLAG; if (unlikely(cause >= BITS_PER_LONG)) panic("unexpected interrupt cause"); switch (cause) { - case RV_IRQ_TIMER: - old_regs = set_irq_regs(regs); - irq_enter(); - riscv_timer_interrupt(); - irq_exit(); - set_irq_regs(old_regs); - break; #ifdef CONFIG_SMP case RV_IRQ_SOFT: /* -- 2.25.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v6 4/6] clocksource/drivers/timer-riscv: Use per-CPU timer interrupt 2020-05-30 10:07 ` [PATCH v6 4/6] clocksource/drivers/timer-riscv: Use per-CPU timer interrupt Anup Patel @ 2020-05-30 11:41 ` Marc Zyngier 2020-05-31 5:52 ` Anup Patel 0 siblings, 1 reply; 18+ messages in thread From: Marc Zyngier @ 2020-05-30 11:41 UTC (permalink / raw) To: Anup Patel Cc: Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Atish Patra, Alistair Francis, Anup Patel, linux-riscv, linux-kernel On 2020-05-30 11:07, Anup Patel wrote: > Instead of directly calling RISC-V timer interrupt handler from > RISC-V local interrupt conntroller driver, this patch implements > RISC-V timer interrupt as a per-CPU interrupt using per-CPU APIs > of Linux IRQ subsystem. > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > Reviewed-by: Atish Patra <atish.patra@wdc.com> > --- > arch/riscv/include/asm/irq.h | 2 -- > drivers/clocksource/timer-riscv.c | 41 ++++++++++++++++++++++++++++--- > drivers/irqchip/irq-riscv-intc.c | 8 ------ > 3 files changed, 38 insertions(+), 13 deletions(-) > > diff --git a/arch/riscv/include/asm/irq.h > b/arch/riscv/include/asm/irq.h > index a9e5f07a7e9c..9807ad164015 100644 > --- a/arch/riscv/include/asm/irq.h > +++ b/arch/riscv/include/asm/irq.h > @@ -10,8 +10,6 @@ > #include <linux/interrupt.h> > #include <linux/linkage.h> > > -void riscv_timer_interrupt(void); > - > #include <asm-generic/irq.h> > > #endif /* _ASM_RISCV_IRQ_H */ > diff --git a/drivers/clocksource/timer-riscv.c > b/drivers/clocksource/timer-riscv.c > index c4f15c4068c0..1fe847983f50 100644 > --- a/drivers/clocksource/timer-riscv.c > +++ b/drivers/clocksource/timer-riscv.c > @@ -12,8 +12,11 @@ > #include <linux/cpu.h> > #include <linux/delay.h> > #include <linux/irq.h> > +#include <linux/irqdomain.h> > #include <linux/sched_clock.h> > #include <linux/io-64-nonatomic-lo-hi.h> > +#include <linux/interrupt.h> > +#include <linux/of_irq.h> > #include <asm/smp.h> > #include <asm/sbi.h> > > @@ -39,6 +42,7 @@ static int riscv_clock_next_event(unsigned long > delta, > return 0; > } > > +static unsigned int riscv_clock_event_irq; > static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = > { > .name = "riscv_timer_clockevent", > .features = CLOCK_EVT_FEAT_ONESHOT, > @@ -74,30 +78,36 @@ static int riscv_timer_starting_cpu(unsigned int > cpu) > struct clock_event_device *ce = per_cpu_ptr(&riscv_clock_event, cpu); > > ce->cpumask = cpumask_of(cpu); > + ce->irq = riscv_clock_event_irq; > clockevents_config_and_register(ce, riscv_timebase, 100, 0x7fffffff); > > - csr_set(CSR_IE, IE_TIE); > + enable_percpu_irq(riscv_clock_event_irq, > + irq_get_trigger_type(riscv_clock_event_irq)); > return 0; > } > > static int riscv_timer_dying_cpu(unsigned int cpu) > { > - csr_clear(CSR_IE, IE_TIE); > + disable_percpu_irq(riscv_clock_event_irq); > return 0; > } > > /* called directly from the low-level interrupt handler */ > -void riscv_timer_interrupt(void) > +static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id) > { > struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event); > > csr_clear(CSR_IE, IE_TIE); > evdev->event_handler(evdev); > + > + return IRQ_HANDLED; > } > > static int __init riscv_timer_init_dt(struct device_node *n) > { > int cpuid, hartid, error; > + struct device_node *child; > + struct irq_domain *domain; > > hartid = riscv_of_processor_hartid(n); > if (hartid < 0) { > @@ -115,6 +125,23 @@ static int __init riscv_timer_init_dt(struct > device_node *n) > if (cpuid != smp_processor_id()) > return 0; > > + domain = NULL; > + for_each_child_of_node(n, child) { > + domain = irq_find_host(child); > + if (domain) > + break; > + } This is a bit clumsy, and probably better written as: child = of_get_compatible(n, "riscv,cpu-intc"); if (!child) { error out } domain = irq_find_host(child); It would be even better if each CPU would have its per-CPU interrupt controller designated as such (with an interrupt-parent property), meaning that you wouldn't have to do anything at all. Too late for that anyway. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6 4/6] clocksource/drivers/timer-riscv: Use per-CPU timer interrupt 2020-05-30 11:41 ` Marc Zyngier @ 2020-05-31 5:52 ` Anup Patel 0 siblings, 0 replies; 18+ messages in thread From: Anup Patel @ 2020-05-31 5:52 UTC (permalink / raw) To: Marc Zyngier Cc: Anup Patel, Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Atish Patra, Alistair Francis, linux-riscv, linux-kernel@vger.kernel.org List On Sat, May 30, 2020 at 5:11 PM Marc Zyngier <maz@kernel.org> wrote: > > On 2020-05-30 11:07, Anup Patel wrote: > > Instead of directly calling RISC-V timer interrupt handler from > > RISC-V local interrupt conntroller driver, this patch implements > > RISC-V timer interrupt as a per-CPU interrupt using per-CPU APIs > > of Linux IRQ subsystem. > > > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > Reviewed-by: Atish Patra <atish.patra@wdc.com> > > --- > > arch/riscv/include/asm/irq.h | 2 -- > > drivers/clocksource/timer-riscv.c | 41 ++++++++++++++++++++++++++++--- > > drivers/irqchip/irq-riscv-intc.c | 8 ------ > > 3 files changed, 38 insertions(+), 13 deletions(-) > > > > diff --git a/arch/riscv/include/asm/irq.h > > b/arch/riscv/include/asm/irq.h > > index a9e5f07a7e9c..9807ad164015 100644 > > --- a/arch/riscv/include/asm/irq.h > > +++ b/arch/riscv/include/asm/irq.h > > @@ -10,8 +10,6 @@ > > #include <linux/interrupt.h> > > #include <linux/linkage.h> > > > > -void riscv_timer_interrupt(void); > > - > > #include <asm-generic/irq.h> > > > > #endif /* _ASM_RISCV_IRQ_H */ > > diff --git a/drivers/clocksource/timer-riscv.c > > b/drivers/clocksource/timer-riscv.c > > index c4f15c4068c0..1fe847983f50 100644 > > --- a/drivers/clocksource/timer-riscv.c > > +++ b/drivers/clocksource/timer-riscv.c > > @@ -12,8 +12,11 @@ > > #include <linux/cpu.h> > > #include <linux/delay.h> > > #include <linux/irq.h> > > +#include <linux/irqdomain.h> > > #include <linux/sched_clock.h> > > #include <linux/io-64-nonatomic-lo-hi.h> > > +#include <linux/interrupt.h> > > +#include <linux/of_irq.h> > > #include <asm/smp.h> > > #include <asm/sbi.h> > > > > @@ -39,6 +42,7 @@ static int riscv_clock_next_event(unsigned long > > delta, > > return 0; > > } > > > > +static unsigned int riscv_clock_event_irq; > > static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = > > { > > .name = "riscv_timer_clockevent", > > .features = CLOCK_EVT_FEAT_ONESHOT, > > @@ -74,30 +78,36 @@ static int riscv_timer_starting_cpu(unsigned int > > cpu) > > struct clock_event_device *ce = per_cpu_ptr(&riscv_clock_event, cpu); > > > > ce->cpumask = cpumask_of(cpu); > > + ce->irq = riscv_clock_event_irq; > > clockevents_config_and_register(ce, riscv_timebase, 100, 0x7fffffff); > > > > - csr_set(CSR_IE, IE_TIE); > > + enable_percpu_irq(riscv_clock_event_irq, > > + irq_get_trigger_type(riscv_clock_event_irq)); > > return 0; > > } > > > > static int riscv_timer_dying_cpu(unsigned int cpu) > > { > > - csr_clear(CSR_IE, IE_TIE); > > + disable_percpu_irq(riscv_clock_event_irq); > > return 0; > > } > > > > /* called directly from the low-level interrupt handler */ > > -void riscv_timer_interrupt(void) > > +static irqreturn_t riscv_timer_interrupt(int irq, void *dev_id) > > { > > struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event); > > > > csr_clear(CSR_IE, IE_TIE); > > evdev->event_handler(evdev); > > + > > + return IRQ_HANDLED; > > } > > > > static int __init riscv_timer_init_dt(struct device_node *n) > > { > > int cpuid, hartid, error; > > + struct device_node *child; > > + struct irq_domain *domain; > > > > hartid = riscv_of_processor_hartid(n); > > if (hartid < 0) { > > @@ -115,6 +125,23 @@ static int __init riscv_timer_init_dt(struct > > device_node *n) > > if (cpuid != smp_processor_id()) > > return 0; > > > > + domain = NULL; > > + for_each_child_of_node(n, child) { > > + domain = irq_find_host(child); > > + if (domain) > > + break; > > + } > > This is a bit clumsy, and probably better written as: > > child = of_get_compatible(n, "riscv,cpu-intc"); > if (!child) { error out } > domain = irq_find_host(child); I thought of not hard-coding INTC compatible string here but both RISC-V INTC and RISC-V Timer are RISC-V specific so I guess it's simpler to use of_get_compatible() directly. I will update. > > It would be even better if each CPU would have its per-CPU interrupt > controller designated as such (with an interrupt-parent property), > meaning that you wouldn't have to do anything at all. > > Too late for that anyway. Yes, too late now. Regards, Anup ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v6 5/6] RISC-V: Remove do_IRQ() function 2020-05-30 10:07 [PATCH v6 0/6] New RISC-V Local Interrupt Controller Driver Anup Patel ` (3 preceding siblings ...) 2020-05-30 10:07 ` [PATCH v6 4/6] clocksource/drivers/timer-riscv: Use per-CPU timer interrupt Anup Patel @ 2020-05-30 10:07 ` Anup Patel 2020-05-30 10:07 ` [PATCH v6 6/6] RISC-V: Force select RISCV_INTC for CONFIG_RISCV Anup Patel 5 siblings, 0 replies; 18+ messages in thread From: Anup Patel @ 2020-05-30 10:07 UTC (permalink / raw) To: Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Marc Zyngier Cc: Atish Patra, Alistair Francis, Anup Patel, linux-riscv, linux-kernel, Anup Patel The only thing do_IRQ() does is call handle_arch_irq function pointer. We can very well call handle_arch_irq function pointer directly from assembly and remove do_IRQ() function hence this patch. Signed-off-by: Anup Patel <anup.patel@wdc.com> Reviewed-by: Atish Patra <atish.patra@wdc.com> --- arch/riscv/kernel/entry.S | 4 +++- arch/riscv/kernel/irq.c | 6 ------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S index 56d071b2c0a1..cae7e6d4c7ef 100644 --- a/arch/riscv/kernel/entry.S +++ b/arch/riscv/kernel/entry.S @@ -106,7 +106,9 @@ _save_context: /* Handle interrupts */ move a0, sp /* pt_regs */ - tail do_IRQ + la a1, handle_arch_irq + REG_L a1, (a1) + jr a1 1: /* * Exceptions run with interrupts enabled or disabled depending on the diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c index eb8777642ce6..7207fa08d78f 100644 --- a/arch/riscv/kernel/irq.c +++ b/arch/riscv/kernel/irq.c @@ -16,12 +16,6 @@ int arch_show_interrupts(struct seq_file *p, int prec) return 0; } -asmlinkage __visible void __irq_entry do_IRQ(struct pt_regs *regs) -{ - if (handle_arch_irq) - handle_arch_irq(regs); -} - void __init init_IRQ(void) { irqchip_init(); -- 2.25.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v6 6/6] RISC-V: Force select RISCV_INTC for CONFIG_RISCV 2020-05-30 10:07 [PATCH v6 0/6] New RISC-V Local Interrupt Controller Driver Anup Patel ` (4 preceding siblings ...) 2020-05-30 10:07 ` [PATCH v6 5/6] RISC-V: Remove do_IRQ() function Anup Patel @ 2020-05-30 10:07 ` Anup Patel 5 siblings, 0 replies; 18+ messages in thread From: Anup Patel @ 2020-05-30 10:07 UTC (permalink / raw) To: Palmer Dabbelt, Paul Walmsley, Albert Ou, Daniel Lezcano, Thomas Gleixner, Jason Cooper, Marc Zyngier Cc: Atish Patra, Alistair Francis, Anup Patel, linux-riscv, linux-kernel, Anup Patel The RISC-V per-HART local interrupt controller driver is mandatory for all RISC-V system (with/without MMU) hence we force select it for CONFIG_RISCV (just like RISCV_TIMER). Signed-off-by: Anup Patel <anup.patel@wdc.com> Reviewed-by: Atish Patra <atish.patra@wdc.com> --- arch/riscv/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 822cb0e1a380..2cf0c83c1a47 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -51,6 +51,7 @@ config RISCV select THREAD_INFO_IN_TASK select PCI_DOMAINS_GENERIC if PCI select PCI_MSI if PCI + select RISCV_INTC select RISCV_TIMER select GENERIC_IRQ_MULTI_HANDLER select GENERIC_ARCH_TOPOLOGY if SMP -- 2.25.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-06-01 9:34 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-30 10:07 [PATCH v6 0/6] New RISC-V Local Interrupt Controller Driver Anup Patel 2020-05-30 10:07 ` [PATCH v6 1/6] RISC-V: self-contained IPI handling routine Anup Patel 2020-05-30 10:07 ` [PATCH v6 2/6] RISC-V: Rename and move plic_find_hart_id() to arch directory Anup Patel 2020-05-30 10:07 ` [PATCH v6 3/6] irqchip: RISC-V per-HART local interrupt controller driver Anup Patel 2020-05-30 12:01 ` Marc Zyngier 2020-05-31 5:36 ` Anup Patel 2020-05-31 9:33 ` Marc Zyngier 2020-05-31 10:06 ` Anup Patel 2020-05-31 10:53 ` Marc Zyngier 2020-06-01 4:09 ` Anup Patel 2020-06-01 7:41 ` Marc Zyngier 2020-06-01 9:13 ` Anup Patel 2020-06-01 9:33 ` Guo Ren 2020-05-30 10:07 ` [PATCH v6 4/6] clocksource/drivers/timer-riscv: Use per-CPU timer interrupt Anup Patel 2020-05-30 11:41 ` Marc Zyngier 2020-05-31 5:52 ` Anup Patel 2020-05-30 10:07 ` [PATCH v6 5/6] RISC-V: Remove do_IRQ() function Anup Patel 2020-05-30 10:07 ` [PATCH v6 6/6] RISC-V: Force select RISCV_INTC for CONFIG_RISCV Anup Patel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).