linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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

* [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

* [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

* 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 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 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

* 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

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).