RFC: simplified RISC-V interrupt and clocksource handling

RFC: simplified RISC-V interrupt and clocksource handling

[Christoph Hellwig]

This series tries adds support for interrupt handling and timers
for the RISC-V architecture.

The basic per-hart interrupt handling implemented by the scause
and sie CSRs is extremely simple and implemented directly in
arch/riscv/kernel/irq.c.  In addition there is a irqchip driver
for the PLIC external interrupt controller, which is called through
the set_handle_irq API, and a clocksource driver that gets its
timer interrupt directly from the low-level interrupt handling.

Compared to previous iterations this version does not try to use an
irqchip driver for the low-level interrupt handling.  This saves
a couple indirect calls and an additional read of the scause CSR
in the hot path, makes the code much simpler and last but not least
avoid the dependency on a device tree for a mandatory architectural
feature.

A git tree is available here (contains a few more patches before
the ones in this series)

    git://git.infradead.org/users/hch/riscv.git riscv-irq-simple

Gitweb:

    http://git.infradead.org/users/hch/riscv.git/shortlog/refs/heads/riscv-irq-simple

[PATCH 1/9] RISC-V: remove timer leftovers

[Christoph Hellwig]

This code is currently unused and will be added back later in a different
place with the real interrupt and clocksource support.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/kernel/time.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
index 2463fcca719e..0df9b2cbd645 100644
--- a/arch/riscv/kernel/time.c
+++ b/arch/riscv/kernel/time.c
@@ -13,32 +13,11 @@
  */
 
 #include <linux/clocksource.h>
-#include <linux/clockchips.h>
 #include <linux/delay.h>
-
-#ifdef CONFIG_RISCV_TIMER
-#include <linux/timer_riscv.h>
-#endif
-
 #include <asm/sbi.h>
 
 unsigned long riscv_timebase;
 
-DECLARE_PER_CPU(struct clock_event_device, riscv_clock_event);
-
-void riscv_timer_interrupt(void)
-{
-#ifdef CONFIG_RISCV_TIMER
-	/*
-	 * FIXME: This needs to be cleaned up along with the rest of the IRQ
-	 * handling cleanup.  See irq.c for more details.
-	 */
-	struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event);
-
-	evdev->event_handler(evdev);
-#endif
-}
-
 void __init init_clockevent(void)
 {
 	timer_probe();
-- 
2.18.0

[PATCH 2/9] RISC-V: simplify software interrupt / IPI code

[Christoph Hellwig]

Rename handle_ipi to riscv_software_interrupt, drop the unused return
value and move the prototype to irq.h together with riscv_timer_interupt.
This allows simplifying the upcoming interrupt handling support.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/irq.h | 1 +
 arch/riscv/include/asm/smp.h | 3 ---
 arch/riscv/kernel/smp.c      | 6 ++----
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
index 4dee9d4c13c0..c871661c9df4 100644
--- a/arch/riscv/include/asm/irq.h
+++ b/arch/riscv/include/asm/irq.h
@@ -22,6 +22,7 @@
 #define INTERRUPT_CAUSE_EXTERNAL    9
 
 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 85e4220839b0..c9395fff246f 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -44,9 +44,6 @@ void arch_send_call_function_single_ipi(int cpu);
  */
 #define raw_smp_processor_id() (*((int*)((char*)get_current() + TASK_TI_CPU)))
 
-/* Interprocessor interrupt handler */
-irqreturn_t handle_ipi(void);
-
 #endif /* CONFIG_SMP */
 
 #endif /* _ASM_RISCV_SMP_H */
diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
index 6d3962435720..906fe21ea21b 100644
--- a/arch/riscv/kernel/smp.c
+++ b/arch/riscv/kernel/smp.c
@@ -45,7 +45,7 @@ int setup_profiling_timer(unsigned int multiplier)
 	return -EINVAL;
 }
 
-irqreturn_t handle_ipi(void)
+void riscv_software_interrupt(void)
 {
 	unsigned long *pending_ipis = &ipi_data[smp_processor_id()].bits;
 
@@ -60,7 +60,7 @@ irqreturn_t handle_ipi(void)
 
 		ops = xchg(pending_ipis, 0);
 		if (ops == 0)
-			return IRQ_HANDLED;
+			return;
 
 		if (ops & (1 << IPI_RESCHEDULE))
 			scheduler_ipi();
@@ -73,8 +73,6 @@ irqreturn_t handle_ipi(void)
 		/* Order data access and bit testing. */
 		mb();
 	}
-
-	return IRQ_HANDLED;
 }
 
 static void
-- 
2.18.0

[PATCH 3/9] RISC-V: remove INTERRUPT_CAUSE_* defines from asm/irq.h

[Christoph Hellwig]

These are only of use to the local irq controller driver, so add them in
that driver implementation instead, which will be submitted soon.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/irq.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/riscv/include/asm/irq.h b/arch/riscv/include/asm/irq.h
index c871661c9df4..996b6fbe17a6 100644
--- a/arch/riscv/include/asm/irq.h
+++ b/arch/riscv/include/asm/irq.h
@@ -17,10 +17,6 @@
 
 #define NR_IRQS         0
 
-#define INTERRUPT_CAUSE_SOFTWARE    1
-#define INTERRUPT_CAUSE_TIMER       5
-#define INTERRUPT_CAUSE_EXTERNAL    9
-
 void riscv_timer_interrupt(void);
 void riscv_software_interrupt(void);
 
-- 
2.18.0

[PATCH 4/9] RISC-V: add a definition for the SIE SEIE bit

[Christoph Hellwig]

This mirrors the SIE_SSIE and SETE bits that are used in a similar
fashion.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/csr.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index 421fa3585798..28a0d1cb374c 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -54,6 +54,7 @@
 /* Interrupt Enable and Interrupt Pending flags */
 #define SIE_SSIE _AC(0x00000002, UL) /* Software Interrupt Enable */
 #define SIE_STIE _AC(0x00000020, UL) /* Timer Interrupt Enable */
+#define SIE_SEIE _AC(0x00000200, UL) /* External Interrupt Enable */
 
 #define EXC_INST_MISALIGNED     0
 #define EXC_INST_ACCESS         1
-- 
2.18.0

[PATCH 5/9] RISC-V: implement low-level interrupt handling

[Christoph Hellwig]

Add support for a routine that dispatches exceptions with the interrupt
flags set to either the IPI or irqdomain code (and the clock source in the
future).

Loosely based on the irq-riscv-int.c irqchip driver from the RISC-V tree.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/kernel/entry.S |  4 +--
 arch/riscv/kernel/irq.c   | 52 ++++++++++++++++++++++++++++++++-------
 2 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 9aaf6c986771..fa2c08e3c05e 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -168,8 +168,8 @@ ENTRY(handle_exception)
 
 	/* Handle interrupts */
 	move a0, sp /* pt_regs */
-	REG_L a1, handle_arch_irq
-	jr a1
+	move a1, s4 /* scause */
+	tail do_IRQ
 1:
 	/* Exceptions run with interrupts enabled */
 	csrs sstatus, SR_SIE
diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
index 7bcdaed15703..ab5f3e22c7cc 100644
--- a/arch/riscv/kernel/irq.c
+++ b/arch/riscv/kernel/irq.c
@@ -1,21 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (C) 2012 Regents of the University of California
  * Copyright (C) 2017 SiFive
- *
- *   This program is free software; you can redistribute it and/or
- *   modify it under the terms of the GNU General Public License
- *   as published by the Free Software Foundation, version 2.
- *
- *   This program is distributed in the hope that it will be useful,
- *   but WITHOUT ANY WARRANTY; without even the implied warranty of
- *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *   GNU General Public License for more details.
+ * Copyright (C) 2018 Christoph Hellwig
  */
 
 #include <linux/interrupt.h>
 #include <linux/irqchip.h>
 #include <linux/irqdomain.h>
 
+/*
+ * Possible interrupt causes:
+ */
+#define INTERRUPT_CAUSE_SOFTWARE    1
+#define INTERRUPT_CAUSE_TIMER       5
+#define INTERRUPT_CAUSE_EXTERNAL    9
+
+/*
+ * The high order bit of the trap cause register is always set for
+ * interrupts, which allows us to differentiate them from exceptions
+ * quickly.  The INTERRUPT_CAUSE_* macros don't contain that bit, so we
+ * need to mask it off.
+ */
+#define INTERRUPT_CAUSE_FLAG	(1UL << (__riscv_xlen - 1))
+
+asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs, unsigned long cause)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+
+	irq_enter();
+	switch (cause & ~INTERRUPT_CAUSE_FLAG) {
+#ifdef CONFIG_SMP
+	case INTERRUPT_CAUSE_SOFTWARE:
+		/*
+		 * 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();
+		break;
+#endif
+	case INTERRUPT_CAUSE_EXTERNAL:
+		handle_arch_irq(regs);
+		break;
+	default:
+		panic("unexpected interrupt cause");
+	}
+	irq_exit();
+
+	set_irq_regs(old_regs);
+}
+
 void __init init_IRQ(void)
 {
 	irqchip_init();
-- 
2.18.0

[PATCH 6/9] RISC-V: Support per-hart timebase-frequency

[Christoph Hellwig]

From: Palmer Dabbelt <palmer@sifive.com>

This isn't actually how I want to do it, I just needed something right
now.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 arch/riscv/kernel/time.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
index 0df9b2cbd645..1bb01dc2d0f1 100644
--- a/arch/riscv/kernel/time.c
+++ b/arch/riscv/kernel/time.c
@@ -24,17 +24,24 @@ void __init init_clockevent(void)
 	csr_set(sie, SIE_STIE);
 }
 
-void __init time_init(void)
+static long __init timebase_frequency(void)
 {
 	struct device_node *cpu;
 	u32 prop;
 
 	cpu = of_find_node_by_path("/cpus");
-	if (!cpu || of_property_read_u32(cpu, "timebase-frequency", &prop))
-		panic(KERN_WARNING "RISC-V system with no 'timebase-frequency' in DTS\n");
-	riscv_timebase = prop;
+	if (cpu && !of_property_read_u32(cpu, "timebase-frequency", &prop))
+		return prop;
+	cpu = of_find_node_by_path("/cpus/cpu@0");
+	if (cpu && !of_property_read_u32(cpu, "timebase-frequency", &prop))
+		return prop;
 
-	lpj_fine = riscv_timebase / HZ;
+	panic(KERN_WARNING "RISC-V system with no 'timebase-frequency' in DTS\n");
+}
 
+void __init time_init(void)
+{
+	riscv_timebase = timebase_frequency();
+	lpj_fine = riscv_timebase / HZ;
 	init_clockevent();
 }
-- 
2.18.0

[PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Christoph Hellwig]

This patch adds a driver for the Platform Level Interrupt Controller (PLIC)
specified as part of the RISC-V supervisor level ISA manual, in the memory
layout implemented by SiFive and qemu.

The PLIC connects global interrupt sources to the local interrupt controller
on each hart.

This driver is based on the driver in the RISC-V tree from Palmer Dabbelt,
but has been almost entirely rewritten since.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/irqchip/Kconfig          |  13 ++
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-riscv-plic.c | 219 +++++++++++++++++++++++++++++++
 3 files changed, 233 insertions(+)
 create mode 100644 drivers/irqchip/irq-riscv-plic.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index e9233db16e03..5ac6dd922a0d 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -372,3 +372,16 @@ config QCOM_PDC
 	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
 
 endmenu
+
+config RISCV_PLIC
+	bool "Platform-Level Interrupt Controller"
+	depends on RISCV
+	default y
+	help
+	   This enables support for the PLIC chip found in standard RISC-V
+	   systems.  The PLIC controls devices interrupts and connects them to
+	   each core's local interrupt controller.  Aside from timer and
+	   software interrupts, all other interrupt sources (MSI, GPIO, etc)
+	   are subordinate to the PLIC.
+
+	   If you don't know what to do here, say Y.
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 15f268f646bf..bf9238da8a31 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -87,3 +87,4 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
 obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
 obj-$(CONFIG_NDS32)			+= irq-ativic32.o
 obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
+obj-$(CONFIG_RISCV_PLIC)		+= irq-riscv-plic.o
diff --git a/drivers/irqchip/irq-riscv-plic.c b/drivers/irqchip/irq-riscv-plic.c
new file mode 100644
index 000000000000..274fe2cba544
--- /dev/null
+++ b/drivers/irqchip/irq-riscv-plic.c
@@ -0,0 +1,219 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 SiFive
+ * Copyright (C) 2018 Christoph Hellwig
+ */
+#define pr_fmt(fmt) "plic: " fmt
+#include <linux/cpumask.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+/*
+ * From the RISC-V Priviledged Spec v1.10:
+ *
+ * Global interrupt sources are assigned small unsigned integer identifiers,
+ * beginning at the value 1.  An interrupt ID of 0 is reserved to mean "no
+ * interrupt".  Interrupt identifiers are also used to break ties when two or
+ * more interrupt sources have the same assigned priority. Smaller values of
+ * interrupt ID take precedence over larger values of interrupt ID.
+ *
+ * While the RISC-V supervisor spec doesn't define the maximum number of
+ * devices supported by the PLIC, the largest number supported by devices
+ * marked as 'riscv,plic0' (which is the only device type this driver supports,
+ * and is the only extant PLIC as of now) is 1024.  As mentioned above, device
+ * 0 is defined to be non-existent so this device really only supports 1023
+ * devices.
+ */
+#define MAX_DEVICES			1024
+#define MAX_CONTEXTS			15872
+
+/*
+ * Each interrupt source has a priority register associated with it.
+ * We always hardwire it to one in Linux.
+ */
+#define PRIORITY_BASE			0
+#define	    PRIORITY_PER_ID		4
+
+/*
+ * Each hart context has a vector of interupt enable bits associated with it.
+ * There's one bit for each interrupt source.
+ */
+#define ENABLE_BASE			0x2000
+#define     ENABLE_PER_HART		0x80
+
+/*
+ * Each hart context has a set of control registers associated with it.  Right
+ * now there's only two: a source priority threshold over which the hart will
+ * take an interrupt, and a register to claim interrupts.
+ */
+#define CONTEXT_BASE			0x200000
+#define     CONTEXT_PER_HART		0x1000
+#define     CONTEXT_THRESHOLD		0x00
+#define     CONTEXT_CLAIM		0x04
+
+static void __iomem *plic_regs;
+
+static inline void __iomem *plic_hart_offset(int ctxid)
+{
+	return plic_regs + CONTEXT_BASE + ctxid * CONTEXT_PER_HART;
+}
+
+/*
+ * Protect mask operations on the registers given that we can't assume that
+ * atomic memory operations work on them.
+ */
+static DEFINE_SPINLOCK(plic_toggle_lock);
+
+static inline void plic_toggle(int ctxid, int hwirq, int enable)
+{
+	u32 __iomem *reg = plic_regs + ENABLE_BASE + ctxid * ENABLE_PER_HART;
+	u32 hwirq_mask = 1 << (hwirq % 32);
+
+	spin_lock(&plic_toggle_lock);
+	if (enable)
+		writel(readl(reg) | hwirq_mask, reg);
+	else
+		writel(readl(reg) & ~hwirq_mask, reg);
+	spin_unlock(&plic_toggle_lock);
+}
+
+static inline void plic_irq_toggle(struct irq_data *d, int enable)
+{
+	int cpu;
+
+	writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
+	for_each_present_cpu(cpu)
+		plic_toggle(cpu, d->hwirq, enable);
+}
+
+static void plic_irq_enable(struct irq_data *d)
+{
+	plic_irq_toggle(d, 1);
+}
+
+static void plic_irq_disable(struct irq_data *d)
+{
+	plic_irq_toggle(d, 0);
+}
+
+static struct irq_chip plic_chip = {
+	.name		= "riscv,plic0",
+	/*
+	 * There is no need to mask/unmask PLIC interrupts.  They are "masked"
+	 * by reading claim and "unmasked" when writing it back.
+	 */
+	.irq_enable	= plic_irq_enable,
+	.irq_disable	= plic_irq_disable,
+};
+
+static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
+			      irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq);
+	irq_set_chip_data(irq, NULL);
+	irq_set_noprobe(irq);
+	return 0;
+}
+
+static const struct irq_domain_ops plic_irqdomain_ops = {
+	.map		= plic_irqdomain_map,
+	.xlate		= irq_domain_xlate_onecell,
+};
+
+static struct irq_domain *plic_irqdomain;
+
+/*
+ * Handling an interrupt is a two-step process: first you claim the interrupt
+ * by reading the claim register, then you complete the interrupt by writing
+ * 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)
+{
+	void __iomem *claim =
+		plic_hart_offset(smp_processor_id()) + CONTEXT_CLAIM;
+	irq_hw_number_t hwirq;
+
+	csr_clear(sie, SIE_STIE);
+	while ((hwirq = readl(claim))) {
+		int irq = irq_find_mapping(plic_irqdomain, hwirq);
+
+		if (unlikely(irq <= 0)) {
+			pr_warn_ratelimited("can't find mapping for hwirq %lu\n",
+					hwirq);
+			ack_bad_irq(irq);
+		} else {
+			generic_handle_irq(irq);
+		}
+		writel(hwirq, claim);
+	}
+	csr_set(sie, SIE_STIE);
+}
+
+static int __init plic_init(struct device_node *node,
+		struct device_node *parent)
+{
+	int error = 0, nr_mapped = 0, nr_handlers, cpu;
+	u32 nr_irqs;
+
+	if (plic_regs) {
+		pr_warning("PLIC already present.\n");
+		return -ENXIO;
+	}
+
+	plic_regs = of_iomap(node, 0);
+	if (WARN_ON(!plic_regs))
+		return -EIO;
+
+	error = -EINVAL;
+	of_property_read_u32(node, "riscv,ndev", &nr_irqs);
+	if (WARN_ON(!nr_irqs))
+		goto out_iounmap;
+
+	nr_handlers = of_irq_count(node);
+	if (WARN_ON(!nr_handlers))
+		goto out_iounmap;
+	if (WARN_ON(nr_handlers < num_possible_cpus()))
+		goto out_iounmap;
+
+	error = -ENOMEM;
+	plic_irqdomain = irq_domain_add_linear(node, nr_irqs + 1,
+			&plic_irqdomain_ops, NULL);
+	if (WARN_ON(!plic_irqdomain))
+		goto out_iounmap;
+
+	/*
+	 * We assume that each present hart is wire up to the PLIC.
+	 * If that isn't the case in the future this code will need to be
+	 * modified.
+	 */
+	for_each_present_cpu(cpu) {
+		irq_hw_number_t hwirq;
+
+		/* priority must be > threshold to trigger an interrupt */
+		writel(0, plic_hart_offset(cpu) + CONTEXT_THRESHOLD);
+		for (hwirq = 1; hwirq <= nr_irqs; ++hwirq)
+			plic_toggle(cpu, hwirq, 0);
+		nr_mapped++;
+	}
+
+	pr_info("mapped %d interrupts to %d (out of %d) handlers.\n",
+		nr_irqs, nr_mapped, nr_handlers);
+	set_handle_irq(plic_handle_irq);
+	return 0;
+
+out_iounmap:
+	iounmap(plic_regs);
+	return error;
+}
+
+IRQCHIP_DECLARE(plic0, "riscv,plic0", plic_init);
-- 
2.18.0

[PATCH 8/9] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Christoph Hellwig]

From: Palmer Dabbelt <palmer@dabbelt.com>

This patch adds documentation for the platform-level interrupt
controller (PLIC) found in all RISC-V systems.  This interrupt
controller routes interrupts from all the devices in the system to each
hart-local interrupt controller.

Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
want to change how we're specifying holes in the hart list.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 .../interrupt-controller/riscv,plic0.txt      | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt
new file mode 100644
index 000000000000..99cd359dbd43
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt
@@ -0,0 +1,55 @@
+RISC-V Platform-Level Interrupt Controller (PLIC)
+-------------------------------------------------
+
+The RISC-V supervisor ISA specification allows for the presence of a
+platform-level interrupt controller (PLIC).   The PLIC connects all external
+interrupts in the system to all hart contexts in the system, via the external
+interrupt source in each hart's hart-local interrupt controller (HLIC).  A hart
+context is a privilege mode in a hardware execution thread.  For example, in
+an 4 core system with 2-way SMT, you have 8 harts and probably at least two
+privilege modes per hart; machine mode and supervisor mode.
+
+Each interrupt can be enabled on per-context basis. Any context can claim
+a pending enabled interrupt and then release it once it has been handled.
+
+Each interrupt has a configurable priority. Higher priority interrupts are
+serviced firs. Each context can specify a priority threshold. Interrupts
+with priority below this threshold will not cause the PLIC to raise its
+interrupt line leading to the context.
+
+While the PLIC supports both edge-triggered and level-triggered interrupts,
+interrupt handlers are oblivious to this distinction and therefor it is not
+specific in the PLIC device-tree binding.
+
+While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
+"riscv,plic0" device is a concrete implementation of the PLIC that contains a
+specific memory layout.  More details about the memory layout of the
+"riscv,plic0" device can be found as a comment in the device driver, or as part
+of the SiFive U5 Coreplex Series Manual (page 22 of the PDF of version 1.0)
+<https://www.sifive.com/documentation/coreplex/u5-coreplex-series-manual/>
+
+Required properties:
+- compatible : "riscv,plic0"
+- #address-cells : should be <0>
+- #interrupt-cells : should be <1>
+- interrupt-controller : Identifies the node as an interrupt controller
+- reg : Should contain 1 register range (address and length)
+- interrupts-extended : Specifies which contexts are connected to the PLIC,
+  with "-1" specifying that a context is not present.
+
+Example:
+
+	plic: interrupt-controller@c000000 {
+		#address-cells = <0>;
+		#interrupt-cells = <1>;
+		compatible = "riscv,plic0";
+		interrupt-controller;
+		interrupts-extended = <
+			&cpu0-intc 11
+			&cpu1-intc 11 &cpu1-intc 9
+			&cpu2-intc 11 &cpu2-intc 9
+			&cpu3-intc 11 &cpu3-intc 9
+			&cpu4-intc 11 &cpu4-intc 9>;
+		reg = <0xc000000 0x4000000>;
+		riscv,ndev = <10>;
+	};
-- 
2.18.0

[PATCH 9/9] clocksource: new RISC-V SBI timer driver

[Christoph Hellwig]

From: Palmer Dabbelt <palmer@dabbelt.com>

The RISC-V ISA defines a per-hart real-time clock and timer, which is
present on all systems.  The clock is accessed via the 'rdtime'
pseudo-instruction (which reads a CSR), and the timer is set via an SBI
call.

Contains various improvements from Atish Patra <atish.patra@wdc.com>.

Signed-off-by: Dmitriy Cherkasov <dmitriy@oss-tech.org>
Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
[hch: remove dead code, add SPDX tags, minor cleanups, merged
 hotplug cpu and other improvements from Atish]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/smp.h      |   3 -
 arch/riscv/kernel/irq.c           |   3 +
 arch/riscv/kernel/smpboot.c       |   1 -
 arch/riscv/kernel/time.c          |   8 +-
 drivers/clocksource/Kconfig       |  10 +++
 drivers/clocksource/Makefile      |   1 +
 drivers/clocksource/riscv_timer.c | 121 ++++++++++++++++++++++++++++++
 include/linux/cpuhotplug.h        |   1 +
 8 files changed, 137 insertions(+), 11 deletions(-)
 create mode 100644 drivers/clocksource/riscv_timer.c

diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
index c9395fff246f..36016845461d 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -24,9 +24,6 @@
 
 #ifdef CONFIG_SMP
 
-/* SMP initialization hook for setup_arch */
-void __init init_clockevent(void);
-
 /* SMP initialization hook for setup_arch */
 void __init setup_smp(void);
 
diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
index ab5f3e22c7cc..0cfac48a1272 100644
--- a/arch/riscv/kernel/irq.c
+++ b/arch/riscv/kernel/irq.c
@@ -30,6 +30,9 @@ asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs, unsigned long cause)
 
 	irq_enter();
 	switch (cause & ~INTERRUPT_CAUSE_FLAG) {
+	case INTERRUPT_CAUSE_TIMER:
+		riscv_timer_interrupt();
+		break;
 #ifdef CONFIG_SMP
 	case INTERRUPT_CAUSE_SOFTWARE:
 		/*
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index f741458c5a3f..56abab6a9812 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -104,7 +104,6 @@ asmlinkage void __init smp_callin(void)
 	current->active_mm = mm;
 
 	trap_init();
-	init_clockevent();
 	notify_cpu_starting(smp_processor_id());
 	set_cpu_online(smp_processor_id(), 1);
 	local_flush_tlb_all();
diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
index 1bb01dc2d0f1..94e9ca18f5fa 100644
--- a/arch/riscv/kernel/time.c
+++ b/arch/riscv/kernel/time.c
@@ -18,12 +18,6 @@
 
 unsigned long riscv_timebase;
 
-void __init init_clockevent(void)
-{
-	timer_probe();
-	csr_set(sie, SIE_STIE);
-}
-
 static long __init timebase_frequency(void)
 {
 	struct device_node *cpu;
@@ -43,5 +37,5 @@ void __init time_init(void)
 {
 	riscv_timebase = timebase_frequency();
 	lpj_fine = riscv_timebase / HZ;
-	init_clockevent();
+	timer_probe();
 }
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index dec0dd88ec15..a57083efaae8 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -609,4 +609,14 @@ config ATCPIT100_TIMER
 	help
 	  This option enables support for the Andestech ATCPIT100 timers.
 
+config RISCV_TIMER
+	bool "Timer for the RISC-V platform"
+	depends on RISCV || COMPILE_TEST
+	select TIMER_PROBE
+	select TIMER_OF
+	help
+	  This enables the per-hart timer built into all RISC-V systems, which
+	  is accessed via both the SBI and the rdcycle instruction.  This is
+	  required for all RISC-V systems.
+
 endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 00caf37e52f9..ded31f720bd9 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -78,3 +78,4 @@ obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
 obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
 obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
 obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
+obj-$(CONFIG_RISCV_TIMER)		+= riscv_timer.o
diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
new file mode 100644
index 000000000000..146156448bdd
--- /dev/null
+++ b/drivers/clocksource/riscv_timer.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2012 Regents of the University of California
+ * Copyright (C) 2017 SiFive
+ */
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/cpu.h>
+#include <linux/delay.h>
+#include <linux/irq.h>
+#include <asm/sbi.h>
+
+/*
+ * All RISC-V systems have a timer attached to every hart.  These timers can be
+ * read by the 'rdcycle' pseudo instruction, and can use the SBI to setup
+ * events.  In order to abstract the architecture-specific timer reading and
+ * setting functions away from the clock event insertion code, we provide
+ * function pointers to the clockevent subsystem that perform two basic
+ * operations: rdtime() reads the timer on the current CPU, and
+ * next_event(delta) sets the next timer event to 'delta' cycles in the future.
+ * As the timers are inherently a per-cpu resource, these callbacks perform
+ * operations on the current hart.  There is guaranteed to be exactly one timer
+ * per hart on all RISC-V systems.
+ */
+
+#define MINDELTA 100
+#define MAXDELTA 0x7fffffff
+
+static int riscv_clock_next_event(unsigned long delta,
+		struct clock_event_device *ce)
+{
+	csr_set(sie, SIE_STIE);
+	sbi_set_timer(get_cycles64() + delta);
+	return 0;
+}
+
+static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {
+	.name			= "riscv_timer_clockevent",
+	.features		= CLOCK_EVT_FEAT_ONESHOT,
+	.rating			= 100,
+	.set_next_event		= riscv_clock_next_event,
+};
+
+/*
+ * It is guarnteed that all the timers across all the harts are synchronized
+ * within one tick of each other, so while this could technically go
+ * backwards when hopping between CPUs, practically it won't happen.
+ */
+static unsigned long long riscv_clocksource_rdtime(struct clocksource *cs)
+{
+	return get_cycles64();
+}
+
+static DEFINE_PER_CPU(struct clocksource, riscv_clocksource) = {
+	.name		= "riscv_clocksource",
+	.rating		= 300,
+	.mask		= CLOCKSOURCE_MASK(BITS_PER_LONG),
+	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
+	.read		= riscv_clocksource_rdtime,
+};
+
+static int timer_riscv_starting_cpu(unsigned int cpu)
+{
+	struct clock_event_device *ce = per_cpu_ptr(&riscv_clock_event, cpu);
+
+	ce->cpumask = cpumask_of(cpu);
+	clockevents_config_and_register(ce, riscv_timebase, MINDELTA, MAXDELTA);
+	csr_set(sie, SIE_STIE);
+	return 0;
+}
+
+static int timer_riscv_dying_cpu(unsigned int cpu)
+{
+	csr_clear(sie, SIE_STIE);
+	return 0;
+}
+
+/* called directly from the low-level interrupt handler */
+void riscv_timer_interrupt(void)
+{
+	struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event);
+
+	csr_clear(sie, SIE_STIE);
+	evdev->event_handler(evdev);
+}
+
+static int hart_of_timer(struct device_node *dev)
+{
+	u32 hart;
+
+	if (!dev)
+		return -1;
+	if (!of_device_is_compatible(dev, "riscv"))
+		return -1;
+	if (of_property_read_u32(dev, "reg", &hart))
+		return -1;
+
+	return hart;
+}
+
+static int __init timer_riscv_init_dt(struct device_node *n)
+{
+	int cpu_id = hart_of_timer(n), error;
+	struct clocksource *cs;
+
+	if (cpu_id != smp_processor_id())
+		return 0;
+
+	cs = per_cpu_ptr(&riscv_clocksource, cpu_id);
+	clocksource_register_hz(cs, riscv_timebase);
+
+	error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
+			 "clockevents/riscv/timer:starting",
+			 timer_riscv_starting_cpu, timer_riscv_dying_cpu);
+	if (error)
+		pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
+		       error, cpu_id);
+	return error;
+}
+
+TIMER_OF_DECLARE(riscv_timer, "riscv", timer_riscv_init_dt);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 8796ba387152..554c27f6cfbd 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -125,6 +125,7 @@ enum cpuhp_state {
 	CPUHP_AP_MARCO_TIMER_STARTING,
 	CPUHP_AP_MIPS_GIC_TIMER_STARTING,
 	CPUHP_AP_ARC_TIMER_STARTING,
+	CPUHP_AP_RISCV_TIMER_STARTING,
 	CPUHP_AP_KVM_STARTING,
 	CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
 	CPUHP_AP_KVM_ARM_VGIC_STARTING,
-- 
2.18.0

Re: [PATCH 9/9] clocksource: new RISC-V SBI timer driver

[Atish Patra]

minor nits.

On 7/26/18 7:38 AM, Christoph Hellwig wrote:
> From: Palmer Dabbelt <palmer@dabbelt.com>
> 
> The RISC-V ISA defines a per-hart real-time clock and timer, which is
> present on all systems.  The clock is accessed via the 'rdtime'
> pseudo-instruction (which reads a CSR), and the timer is set via an SBI
> call.
> 
> Contains various improvements from Atish Patra <atish.patra@wdc.com>.
> 
> Signed-off-by: Dmitriy Cherkasov <dmitriy@oss-tech.org>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> [hch: remove dead code, add SPDX tags, minor cleanups, merged
>   hotplug cpu and other improvements from Atish]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   arch/riscv/include/asm/smp.h      |   3 -
>   arch/riscv/kernel/irq.c           |   3 +
>   arch/riscv/kernel/smpboot.c       |   1 -
>   arch/riscv/kernel/time.c          |   8 +-
>   drivers/clocksource/Kconfig       |  10 +++
>   drivers/clocksource/Makefile      |   1 +
>   drivers/clocksource/riscv_timer.c | 121 ++++++++++++++++++++++++++++++
>   include/linux/cpuhotplug.h        |   1 +
>   8 files changed, 137 insertions(+), 11 deletions(-)
>   create mode 100644 drivers/clocksource/riscv_timer.c
> 
> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
> index c9395fff246f..36016845461d 100644
> --- a/arch/riscv/include/asm/smp.h
> +++ b/arch/riscv/include/asm/smp.h
> @@ -24,9 +24,6 @@
>   
>   #ifdef CONFIG_SMP
>   
> -/* SMP initialization hook for setup_arch */
> -void __init init_clockevent(void);
> -
>   /* SMP initialization hook for setup_arch */
>   void __init setup_smp(void);
>   
> diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
> index ab5f3e22c7cc..0cfac48a1272 100644
> --- a/arch/riscv/kernel/irq.c
> +++ b/arch/riscv/kernel/irq.c
> @@ -30,6 +30,9 @@ asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs, unsigned long cause)
>   
>   	irq_enter();
>   	switch (cause & ~INTERRUPT_CAUSE_FLAG) {
> +	case INTERRUPT_CAUSE_TIMER:
> +		riscv_timer_interrupt();
> +		break;
>   #ifdef CONFIG_SMP
>   	case INTERRUPT_CAUSE_SOFTWARE:
>   		/*
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index f741458c5a3f..56abab6a9812 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -104,7 +104,6 @@ asmlinkage void __init smp_callin(void)
>   	current->active_mm = mm;
>   
>   	trap_init();
> -	init_clockevent();
>   	notify_cpu_starting(smp_processor_id());
>   	set_cpu_online(smp_processor_id(), 1);
>   	local_flush_tlb_all();
> diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
> index 1bb01dc2d0f1..94e9ca18f5fa 100644
> --- a/arch/riscv/kernel/time.c
> +++ b/arch/riscv/kernel/time.c
> @@ -18,12 +18,6 @@
>   
>   unsigned long riscv_timebase;
>   
> -void __init init_clockevent(void)
> -{
> -	timer_probe();
> -	csr_set(sie, SIE_STIE);
> -}
> -
>   static long __init timebase_frequency(void)
>   {
>   	struct device_node *cpu;
> @@ -43,5 +37,5 @@ void __init time_init(void)
>   {
>   	riscv_timebase = timebase_frequency();
>   	lpj_fine = riscv_timebase / HZ;
> -	init_clockevent();
> +	timer_probe();
>   }
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index dec0dd88ec15..a57083efaae8 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -609,4 +609,14 @@ config ATCPIT100_TIMER
>   	help
>   	  This option enables support for the Andestech ATCPIT100 timers.
>   
> +config RISCV_TIMER
> +	bool "Timer for the RISC-V platform"
> +	depends on RISCV || COMPILE_TEST
> +	select TIMER_PROBE
> +	select TIMER_OF
> +	help
> +	  This enables the per-hart timer built into all RISC-V systems, which
> +	  is accessed via both the SBI and the rdcycle instruction.  This is
> +	  required for all RISC-V systems.
> +
>   endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 00caf37e52f9..ded31f720bd9 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
>   obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
>   obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
>   obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
> +obj-$(CONFIG_RISCV_TIMER)		+= riscv_timer.o
> diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
> new file mode 100644
> index 000000000000..146156448bdd
> --- /dev/null
> +++ b/drivers/clocksource/riscv_timer.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2017 SiFive
> + */
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +#include <linux/cpu.h>
> +#include <linux/delay.h>
> +#include <linux/irq.h>
> +#include <asm/sbi.h>
> +
> +/*
> + * All RISC-V systems have a timer attached to every hart.  These timers can be
> + * read by the 'rdcycle' pseudo instruction, and can use the SBI to setup
> + * events.  In order to abstract the architecture-specific timer reading and
> + * setting functions away from the clock event insertion code, we provide
> + * function pointers to the clockevent subsystem that perform two basic
> + * operations: rdtime() reads the timer on the current CPU, and
> + * next_event(delta) sets the next timer event to 'delta' cycles in the future.
> + * As the timers are inherently a per-cpu resource, these callbacks perform
> + * operations on the current hart.  There is guaranteed to be exactly one timer
> + * per hart on all RISC-V systems.
> + */
> +
> +#define MINDELTA 100
> +#define MAXDELTA 0x7fffffff
> +
> +static int riscv_clock_next_event(unsigned long delta,
> +		struct clock_event_device *ce)
> +{
> +	csr_set(sie, SIE_STIE);
> +	sbi_set_timer(get_cycles64() + delta);
> +	return 0;
> +}
> +
> +static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {
> +	.name			= "riscv_timer_clockevent",
> +	.features		= CLOCK_EVT_FEAT_ONESHOT,
> +	.rating			= 100,
> +	.set_next_event		= riscv_clock_next_event,
> +};
> +
> +/*
> + * It is guarnteed that all the timers across all the harts are synchronized

/s/guarnteed/guaranteed

> + * within one tick of each other, so while this could technically go
> + * backwards when hopping between CPUs, practically it won't happen.
> + */
> +static unsigned long long riscv_clocksource_rdtime(struct clocksource *cs)
> +{
> +	return get_cycles64();
> +}
> +
> +static DEFINE_PER_CPU(struct clocksource, riscv_clocksource) = {
> +	.name		= "riscv_clocksource",
> +	.rating		= 300,
> +	.mask		= CLOCKSOURCE_MASK(BITS_PER_LONG),
> +	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
> +	.read		= riscv_clocksource_rdtime,
> +};
> +
> +static int timer_riscv_starting_cpu(unsigned int cpu)
> +{
> +	struct clock_event_device *ce = per_cpu_ptr(&riscv_clock_event, cpu);
> +
> +	ce->cpumask = cpumask_of(cpu);
> +	clockevents_config_and_register(ce, riscv_timebase, MINDELTA, MAXDELTA);
> +	csr_set(sie, SIE_STIE);
> +	return 0;
> +}
> +
> +static int timer_riscv_dying_cpu(unsigned int cpu)
> +{
> +	csr_clear(sie, SIE_STIE);
> +	return 0;
> +}
> +
> +/* called directly from the low-level interrupt handler */
> +void riscv_timer_interrupt(void)
> +{

Should we follow the same prefix for these functions?
either timer_riscv* or riscv_timer* ?

Apologies for overlooking this in my timer patch as well.

> +	struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event);
> +

The comment about the purpose of clearing the interrupt in the original 
patch is removed here. If that's intentional, it's fine.

I thought having that comment helps understanding the distinction 
between clearing the timer interrupt in SBI call & here.
> +	csr_clear(sie, SIE_STIE);
> +	evdev->event_handler(evdev);
> +}
> +
> +static int hart_of_timer(struct device_node *dev)
> +{
> +	u32 hart;
> +
> +	if (!dev)
> +		return -1;
> +	if (!of_device_is_compatible(dev, "riscv"))
> +		return -1;
> +	if (of_property_read_u32(dev, "reg", &hart))
> +		return -1;
> +
> +	return hart;
> +}
> +
> +static int __init timer_riscv_init_dt(struct device_node *n)
> +{
> +	int cpu_id = hart_of_timer(n), error;
> +	struct clocksource *cs;
> +
> +	if (cpu_id != smp_processor_id())
> +		return 0;
> +
> +	cs = per_cpu_ptr(&riscv_clocksource, cpu_id);
> +	clocksource_register_hz(cs, riscv_timebase);
> +
> +	error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
> +			 "clockevents/riscv/timer:starting",
> +			 timer_riscv_starting_cpu, timer_riscv_dying_cpu);
> +	if (error)
> +		pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
> +		       error, cpu_id);
> +	return error;
> +}
> +
> +TIMER_OF_DECLARE(riscv_timer, "riscv", timer_riscv_init_dt);
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 8796ba387152..554c27f6cfbd 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -125,6 +125,7 @@ enum cpuhp_state {
>   	CPUHP_AP_MARCO_TIMER_STARTING,
>   	CPUHP_AP_MIPS_GIC_TIMER_STARTING,
>   	CPUHP_AP_ARC_TIMER_STARTING,
> +	CPUHP_AP_RISCV_TIMER_STARTING,
>   	CPUHP_AP_KVM_STARTING,
>   	CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
>   	CPUHP_AP_KVM_ARM_VGIC_STARTING,
> 

Re: RFC: simplified RISC-V interrupt and clocksource handling

[Atish Patra]

On 7/26/18 7:37 AM, Christoph Hellwig wrote:
> This series tries adds support for interrupt handling and timers
> for the RISC-V architecture.
> 
> The basic per-hart interrupt handling implemented by the scause
> and sie CSRs is extremely simple and implemented directly in
> arch/riscv/kernel/irq.c.  In addition there is a irqchip driver
> for the PLIC external interrupt controller, which is called through
> the set_handle_irq API, and a clocksource driver that gets its
> timer interrupt directly from the low-level interrupt handling.
> 
> Compared to previous iterations this version does not try to use an
> irqchip driver for the low-level interrupt handling.  This saves
> a couple indirect calls and an additional read of the scause CSR
> in the hot path, makes the code much simpler and last but not least
> avoid the dependency on a device tree for a mandatory architectural
> feature.
> 
I agree that this code is much simpler than HLIC code.
Few doubts though

1. As per my understanding, timer interrupt now can't be registered as a 
Linux IRQ now. Thus, /proc/interrupts will not be automatically 
populated for timer interrupt stats. Am I wrong in my assumption?

2. The future version of local interrupt controller known as Core Level 
Interrupt Controller aka CLIC. Do we have to change the current design
again for CLIC in future?

Here are the docs:
https://github.com/sifive/clic-spec/blob/master/clic.adoc

Regards,
Atish
> A git tree is available here (contains a few more patches before
> the ones in this series)
> 
>      git://git.infradead.org/users/hch/riscv.git riscv-irq-simple
> 
> Gitweb:
> 
>      http://git.infradead.org/users/hch/riscv.git/shortlog/refs/heads/riscv-irq-simple
> 

Re: [PATCH 9/9] clocksource: new RISC-V SBI timer driver

[Christoph Hellwig]

On Thu, Jul 26, 2018 at 11:51:56AM -0700, Atish Patra wrote:
> Should we follow the same prefix for these functions?
> either timer_riscv* or riscv_timer* ?
>
> Apologies for overlooking this in my timer patch as well.

riscv_timer_* sounds saner to me, I can update that.

>> +	struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event);
>> +
>
> The comment about the purpose of clearing the interrupt in the original 
> patch is removed here. If that's intentional, it's fine.
>
> I thought having that comment helps understanding the distinction between 
> clearing the timer interrupt in SBI call & here.

Yes, that was intentional.  But given that I don't even understand why
not using an ABI for architectural interrupt source enable/disable maybe
I'm confused and should revisit that decision.

Re: RFC: simplified RISC-V interrupt and clocksource handling

[Christoph Hellwig]

On Thu, Jul 26, 2018 at 04:38:43PM -0700, Atish Patra wrote:
> 1. As per my understanding, timer interrupt now can't be registered as a 
> Linux IRQ now. Thus, /proc/interrupts will not be automatically populated 
> for timer interrupt stats. Am I wrong in my assumption?

Yes, with this code the timer interrupt does not show up in
/proc/interrupts.  I wonder if that is an issue and if there is any
precedence for it?

> 2. The future version of local interrupt controller known as Core Level 
> Interrupt Controller aka CLIC. Do we have to change the current design
> again for CLIC in future?
>
> Here are the docs:
> https://github.com/sifive/clic-spec/blob/master/clic.adoc

This doesn't really look like 'the future' version but a proposal for
something more like low end realtime microcontrollers ala ARM Cortex M*.
At least the priorities don't really make much sense for a general
purpose SOC.

Either way the existing architectural scause/sie interrupt handling will
remain but can be opted out, but if we really want to support the CLIC
it would have to grow a new irqchip driver, and the PLIC driver would
require a few dozend new lines of glue code to chain underneath it.

Re: [PATCH 9/9] clocksource: new RISC-V SBI timer driver

[Atish Patra]

On 7/27/18 7:38 AM, Christoph Hellwig wrote:
> On Thu, Jul 26, 2018 at 11:51:56AM -0700, Atish Patra wrote:
>> Should we follow the same prefix for these functions?
>> either timer_riscv* or riscv_timer* ?
>>
>> Apologies for overlooking this in my timer patch as wel
> 
> riscv_timer_* sounds saner to me, I can update that.
> 

Thanks.
>>> +	struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event);
>>> +
>>
>> The comment about the purpose of clearing the interrupt in the original
>> patch is removed here. If that's intentional, it's fine.
>>
>> I thought having that comment helps understanding the distinction between
>> clearing the timer interrupt in SBI call & here.
> 
> Yes, that was intentional.  But given that I don't even understand why
> not using an ABI for architectural interrupt source enable/disable maybe
> I'm confused and should revisit that decision.
> 

I tried adding a new SBI call to disable the interrupts. However, I 
realized that it is not recommended to change the SBI unless absolutely 
required.

Here is the PR & following discussion.
https://github.com/riscv/riscv-pk/pull/108

Regards,
Atish

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Atish Patra]

On 7/26/18 7:38 AM, Christoph Hellwig wrote:
> This patch adds a driver for the Platform Level Interrupt Controller (PLIC)
> specified as part of the RISC-V supervisor level ISA manual, in the memory
> layout implemented by SiFive and qemu.
> 
> The PLIC connects global interrupt sources to the local interrupt controller
> on each hart.
> 
> This driver is based on the driver in the RISC-V tree from Palmer Dabbelt,
> but has been almost entirely rewritten since.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I tried to boot HighFive Unleashed with the patch series after applying 
all the patches from riscv-all branch except timer & irq patches. It 
gets stuck pretty early.

Here is my github repo with all the changes:
https://github.com/atishp04/riscv-linux/commits/master_chris_cleanup_hifive

I am still looking into it.
Palmer: Did I miss something?

FWIW, here is the boot log.
--------- Boot log -------------------------------------------
[    0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=5
[    0.000000] NR_IRQS: 0, nr_irqs: 0, preallocated irqs: 0
[    0.000000] plic: mapped 53 interrupts to 4 (out of 9) handlers.
[    0.000000] clocksource: riscv_clocksource: mask: 0xffffffffffffffff 
max_cycles: 0x1d854df40, max_idle_ns: 3526361616960 ns
[    0.000000] Calibrating delay loop (skipped), value calculated using 
timer frequency.. 2.00 BogoMIPS (lpj=10000)
[    0.010000] pid_max: default: 32768 minimum: 301
[    0.010000] Mount-cache hash table entries: 16384 (order: 5, 131072 
bytes)
[    0.020000] Mountpoint-cache hash table entries: 16384 (order: 5, 
131072 bytes)
[    0.020000] Hierarchical SRCU implementation.
[    0.030000] smp: Bringing up secondary CPUs ...
> ---
>   drivers/irqchip/Kconfig          |  13 ++
>   drivers/irqchip/Makefile         |   1 +
>   drivers/irqchip/irq-riscv-plic.c | 219 +++++++++++++++++++++++++++++++
>   3 files changed, 233 insertions(+)
>   create mode 100644 drivers/irqchip/irq-riscv-plic.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index e9233db16e03..5ac6dd922a0d 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -372,3 +372,16 @@ config QCOM_PDC
>   	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>   
>   endmenu
> +
> +config RISCV_PLIC
> +	bool "Platform-Level Interrupt Controller"
> +	depends on RISCV
> +	default y
> +	help
> +	   This enables support for the PLIC chip found in standard RISC-V
> +	   systems.  The PLIC controls devices interrupts and connects them to
> +	   each core's local interrupt controller.  Aside from timer and
> +	   software interrupts, all other interrupt sources (MSI, GPIO, etc)
> +	   are subordinate to the PLIC.
> +
> +	   If you don't know what to do here, say Y.
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 15f268f646bf..bf9238da8a31 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -87,3 +87,4 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
>   obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>   obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>   obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
> +obj-$(CONFIG_RISCV_PLIC)		+= irq-riscv-plic.o
> diff --git a/drivers/irqchip/irq-riscv-plic.c b/drivers/irqchip/irq-riscv-plic.c
> new file mode 100644
> index 000000000000..274fe2cba544
> --- /dev/null
> +++ b/drivers/irqchip/irq-riscv-plic.c
> @@ -0,0 +1,219 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 SiFive
> + * Copyright (C) 2018 Christoph Hellwig
> + */
> +#define pr_fmt(fmt) "plic: " fmt
> +#include <linux/cpumask.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +/*
> + * From the RISC-V Priviledged Spec v1.10:
> + *
> + * Global interrupt sources are assigned small unsigned integer identifiers,
> + * beginning at the value 1.  An interrupt ID of 0 is reserved to mean "no
> + * interrupt".  Interrupt identifiers are also used to break ties when two or
> + * more interrupt sources have the same assigned priority. Smaller values of
> + * interrupt ID take precedence over larger values of interrupt ID.
> + *
> + * While the RISC-V supervisor spec doesn't define the maximum number of
> + * devices supported by the PLIC, the largest number supported by devices
> + * marked as 'riscv,plic0' (which is the only device type this driver supports,
> + * and is the only extant PLIC as of now) is 1024.  As mentioned above, device
> + * 0 is defined to be non-existent so this device really only supports 1023
> + * devices.
> + */
> +#define MAX_DEVICES			1024
> +#define MAX_CONTEXTS			15872
> +

Is there any way we can preserve some of the comments in the original 
patch about memory-mapped control registers or at least a reference 
where to find the register offset calculations?

IMHO, it is helpful for anybody who is not familiar with the details.
> +/*
> + * Each interrupt source has a priority register associated with it.
> + * We always hardwire it to one in Linux.
> + */
> +#define PRIORITY_BASE			0
> +#define	    PRIORITY_PER_ID		4
> +
> +/*
> + * Each hart context has a vector of interupt enable bits associated with it.
> + * There's one bit for each interrupt source.
> + */
> +#define ENABLE_BASE			0x2000
> +#define     ENABLE_PER_HART		0x80
> +
> +/*
> + * Each hart context has a set of control registers associated with it.  Right
> + * now there's only two: a source priority threshold over which the hart will
> + * take an interrupt, and a register to claim interrupts.
> + */
> +#define CONTEXT_BASE			0x200000
> +#define     CONTEXT_PER_HART		0x1000
> +#define     CONTEXT_THRESHOLD		0x00
> +#define     CONTEXT_CLAIM		0x04
> +
> +static void __iomem *plic_regs;
> +
> +static inline void __iomem *plic_hart_offset(int ctxid)
> +{
> +	return plic_regs + CONTEXT_BASE + ctxid * CONTEXT_PER_HART;
> +}
> +
> +/*
> + * Protect mask operations on the registers given that we can't assume that
> + * atomic memory operations work on them.
> + */
> +static DEFINE_SPINLOCK(plic_toggle_lock);
> +
> +static inline void plic_toggle(int ctxid, int hwirq, int enable)
> +{
> +	u32 __iomem *reg = plic_regs + ENABLE_BASE + ctxid * ENABLE_PER_HART;

shouldn't it be
u32 __iomem *reg = plic_regs + ENABLE_BASE + ctxid * ENABLE_PER_HART + 
(hwirq / 32) * 4;

Without that change, plic_handle_irq() gets called before irq mapping as 
plic was not disabled for that irq.

As per comment in the original patch,
--------------------------------------------------------------
* base + 0x002000: Enable bits for sources 0-31 on context 0
* base + 0x002004: Enable bits for sources 32-63 on context 0
--------------------------------------------------------------

> +	u32 hwirq_mask = 1 << (hwirq % 32);
> +
> +	spin_lock(&plic_toggle_lock);
> +	if (enable)
> +		writel(readl(reg) | hwirq_mask, reg);
> +	else
> +		writel(readl(reg) & ~hwirq_mask, reg);
> +	spin_unlock(&plic_toggle_lock);
> +}
> +
> +static inline void plic_irq_toggle(struct irq_data *d, int enable)
> +{
> +	int cpu;
> +
> +	writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
> +	for_each_present_cpu(cpu)
> +		plic_toggle(cpu, d->hwirq, enable);
> +}
> +
> +static void plic_irq_enable(struct irq_data *d)
> +{
> +	plic_irq_toggle(d, 1);
> +}
> +
> +static void plic_irq_disable(struct irq_data *d)
> +{
> +	plic_irq_toggle(d, 0);
> +}
> +
> +static struct irq_chip plic_chip = {
> +	.name		= "riscv,plic0",
> +	/*
> +	 * There is no need to mask/unmask PLIC interrupts.  They are "masked"
> +	 * by reading claim and "unmasked" when writing it back.
> +	 */
> +	.irq_enable	= plic_irq_enable,
> +	.irq_disable	= plic_irq_disable,
> +};
> +
> +static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> +			      irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq);
> +	irq_set_chip_data(irq, NULL);
> +	irq_set_noprobe(irq);
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops plic_irqdomain_ops = {
> +	.map		= plic_irqdomain_map,
> +	.xlate		= irq_domain_xlate_onecell,
> +};
> +
> +static struct irq_domain *plic_irqdomain;
> +
> +/*
> + * Handling an interrupt is a two-step process: first you claim the interrupt
> + * by reading the claim register, then you complete the interrupt by writing
> + * 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)
> +{
> +	void __iomem *claim =
> +		plic_hart_offset(smp_processor_id()) + CONTEXT_CLAIM;
> +	irq_hw_number_t hwirq;
> +
> +	csr_clear(sie, SIE_STIE);
> +	while ((hwirq = readl(claim))) {
> +		int irq = irq_find_mapping(plic_irqdomain, hwirq);
> +
> +		if (unlikely(irq <= 0)) {
> +			pr_warn_ratelimited("can't find mapping for hwirq %lu\n",
> +					hwirq);

Ratlimiting the warning message here didn't help as ack_bad_irq() still 
print message still flooded the console without any useful info.

Regards,
Atish
> +			ack_bad_irq(irq);
> +		} else {
> +			generic_handle_irq(irq);
> +		}
> +		writel(hwirq, claim);
> +	}
> +	csr_set(sie, SIE_STIE);
> +}
> +
> +static int __init plic_init(struct device_node *node,
> +		struct device_node *parent)
> +{
> +	int error = 0, nr_mapped = 0, nr_handlers, cpu;
> +	u32 nr_irqs;
> +
> +	if (plic_regs) {
> +		pr_warning("PLIC already present.\n");
> +		return -ENXIO;
> +	}
> +
> +	plic_regs = of_iomap(node, 0);
> +	if (WARN_ON(!plic_regs))
> +		return -EIO;
> +
> +	error = -EINVAL;
> +	of_property_read_u32(node, "riscv,ndev", &nr_irqs);
> +	if (WARN_ON(!nr_irqs))
> +		goto out_iounmap;
> +
> +	nr_handlers = of_irq_count(node);
> +	if (WARN_ON(!nr_handlers))
> +		goto out_iounmap;
> +	if (WARN_ON(nr_handlers < num_possible_cpus()))
> +		goto out_iounmap;
> +
> +	error = -ENOMEM;
> +	plic_irqdomain = irq_domain_add_linear(node, nr_irqs + 1,
> +			&plic_irqdomain_ops, NULL);
> +	if (WARN_ON(!plic_irqdomain))
> +		goto out_iounmap;
> +
> +	/*
> +	 * We assume that each present hart is wire up to the PLIC.
> +	 * If that isn't the case in the future this code will need to be
> +	 * modified.
> +	 */
> +	for_each_present_cpu(cpu) {
> +		irq_hw_number_t hwirq;
> +
> +		/* priority must be > threshold to trigger an interrupt */
> +		writel(0, plic_hart_offset(cpu) + CONTEXT_THRESHOLD);
> +		for (hwirq = 1; hwirq <= nr_irqs; ++hwirq)
> +			plic_toggle(cpu, hwirq, 0);
> +		nr_mapped++;
> +	}
> +
> +	pr_info("mapped %d interrupts to %d (out of %d) handlers.\n",
> +		nr_irqs, nr_mapped, nr_handlers);
> +	set_handle_irq(plic_handle_irq);
> +	return 0;
> +
> +out_iounmap:
> +	iounmap(plic_regs);
> +	return error;
> +}
> +
> +IRQCHIP_DECLARE(plic0, "riscv,plic0", plic_init);
> 

Re: [PATCH 9/9] clocksource: new RISC-V SBI timer driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6235 bytes --]

Hi Palmer,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc6 next-20180727]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Christoph-Hellwig/RISC-V-remove-timer-leftovers/20180729-021511
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=sparc 

All errors (new ones prefixed by >>):

   drivers/clocksource/riscv_timer.c: In function 'riscv_clock_next_event':
>> drivers/clocksource/riscv_timer.c:32:2: error: implicit declaration of function 'csr_set' [-Werror=implicit-function-declaration]
     csr_set(sie, SIE_STIE);
     ^~~~~~~
>> drivers/clocksource/riscv_timer.c:32:10: error: 'sie' undeclared (first use in this function)
     csr_set(sie, SIE_STIE);
             ^~~
   drivers/clocksource/riscv_timer.c:32:10: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/clocksource/riscv_timer.c:32:15: error: 'SIE_STIE' undeclared (first use in this function); did you mean 'S_CTIME'?
     csr_set(sie, SIE_STIE);
                  ^~~~~~~~
                  S_CTIME
>> drivers/clocksource/riscv_timer.c:33:2: error: implicit declaration of function 'sbi_set_timer'; did you mean 'do_setitimer'? [-Werror=implicit-function-declaration]
     sbi_set_timer(get_cycles64() + delta);
     ^~~~~~~~~~~~~
     do_setitimer
>> drivers/clocksource/riscv_timer.c:33:16: error: implicit declaration of function 'get_cycles64'; did you mean 'get_cycles'? [-Werror=implicit-function-declaration]
     sbi_set_timer(get_cycles64() + delta);
                   ^~~~~~~~~~~~
                   get_cycles
   drivers/clocksource/riscv_timer.c: In function 'timer_riscv_starting_cpu':
>> drivers/clocksource/riscv_timer.c:67:38: error: 'riscv_timebase' undeclared (first use in this function); did you mean 'init_timers'?
     clockevents_config_and_register(ce, riscv_timebase, MINDELTA, MAXDELTA);
                                         ^~~~~~~~~~~~~~
                                         init_timers
   drivers/clocksource/riscv_timer.c:68:10: error: 'sie' undeclared (first use in this function)
     csr_set(sie, SIE_STIE);
             ^~~
   drivers/clocksource/riscv_timer.c:68:15: error: 'SIE_STIE' undeclared (first use in this function); did you mean 'S_CTIME'?
     csr_set(sie, SIE_STIE);
                  ^~~~~~~~
                  S_CTIME
   drivers/clocksource/riscv_timer.c: In function 'timer_riscv_dying_cpu':
>> drivers/clocksource/riscv_timer.c:74:2: error: implicit declaration of function 'csr_clear'; did you mean 'cap_clear'? [-Werror=implicit-function-declaration]
     csr_clear(sie, SIE_STIE);
     ^~~~~~~~~
     cap_clear
>> drivers/clocksource/riscv_timer.c:74:12: error: 'sie' undeclared (first use in this function); did you mean 'ksize'?
     csr_clear(sie, SIE_STIE);
               ^~~
               ksize
   drivers/clocksource/riscv_timer.c:74:17: error: 'SIE_STIE' undeclared (first use in this function); did you mean 'S_CTIME'?
     csr_clear(sie, SIE_STIE);
                    ^~~~~~~~
                    S_CTIME
   drivers/clocksource/riscv_timer.c: In function 'riscv_timer_interrupt':
   drivers/clocksource/riscv_timer.c:83:12: error: 'sie' undeclared (first use in this function); did you mean 'ksize'?
     csr_clear(sie, SIE_STIE);
               ^~~
               ksize
   drivers/clocksource/riscv_timer.c:83:17: error: 'SIE_STIE' undeclared (first use in this function); did you mean 'S_CTIME'?
     csr_clear(sie, SIE_STIE);
                    ^~~~~~~~
                    S_CTIME
   drivers/clocksource/riscv_timer.c: In function 'timer_riscv_init_dt':
   drivers/clocksource/riscv_timer.c:110:30: error: 'riscv_timebase' undeclared (first use in this function); did you mean 'init_timers'?
     clocksource_register_hz(cs, riscv_timebase);
                                 ^~~~~~~~~~~~~~
                                 init_timers
   cc1: some warnings being treated as errors

vim +/csr_set +32 drivers/clocksource/riscv_timer.c

    28	
    29	static int riscv_clock_next_event(unsigned long delta,
    30			struct clock_event_device *ce)
    31	{
  > 32		csr_set(sie, SIE_STIE);
  > 33		sbi_set_timer(get_cycles64() + delta);
    34		return 0;
    35	}
    36	
    37	static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {
    38		.name			= "riscv_timer_clockevent",
    39		.features		= CLOCK_EVT_FEAT_ONESHOT,
    40		.rating			= 100,
    41		.set_next_event		= riscv_clock_next_event,
    42	};
    43	
    44	/*
    45	 * It is guarnteed that all the timers across all the harts are synchronized
    46	 * within one tick of each other, so while this could technically go
    47	 * backwards when hopping between CPUs, practically it won't happen.
    48	 */
    49	static unsigned long long riscv_clocksource_rdtime(struct clocksource *cs)
    50	{
    51		return get_cycles64();
    52	}
    53	
    54	static DEFINE_PER_CPU(struct clocksource, riscv_clocksource) = {
    55		.name		= "riscv_clocksource",
    56		.rating		= 300,
    57		.mask		= CLOCKSOURCE_MASK(BITS_PER_LONG),
    58		.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
    59		.read		= riscv_clocksource_rdtime,
    60	};
    61	
    62	static int timer_riscv_starting_cpu(unsigned int cpu)
    63	{
    64		struct clock_event_device *ce = per_cpu_ptr(&riscv_clock_event, cpu);
    65	
    66		ce->cpumask = cpumask_of(cpu);
  > 67		clockevents_config_and_register(ce, riscv_timebase, MINDELTA, MAXDELTA);
  > 68		csr_set(sie, SIE_STIE);
    69		return 0;
    70	}
    71	
    72	static int timer_riscv_dying_cpu(unsigned int cpu)
    73	{
  > 74		csr_clear(sie, SIE_STIE);
    75		return 0;
    76	}
    77	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54283 bytes --]

Re: [PATCH 9/9] clocksource: new RISC-V SBI timer driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

Hi Palmer,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc6 next-20180727]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Christoph-Hellwig/RISC-V-remove-timer-leftovers/20180729-021511
config: powerpc64-allmodconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=powerpc64 

All errors (new ones prefixed by >>):

>> drivers/clocksource/riscv_timer.c:11:10: fatal error: asm/sbi.h: No such file or directory
    #include <asm/sbi.h>
             ^~~~~~~~~~~
   compilation terminated.

vim +11 drivers/clocksource/riscv_timer.c

  > 11	#include <asm/sbi.h>
    12	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 57495 bytes --]

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Anup Patel]

On Sat 28 Jul, 2018, 5:34 AM Atish Patra, <atish.patra@wdc.com> wrote:
>
> On 7/26/18 7:38 AM, Christoph Hellwig wrote:
> > This patch adds a driver for the Platform Level Interrupt Controller (PLIC)
> > specified as part of the RISC-V supervisor level ISA manual, in the memory
> > layout implemented by SiFive and qemu.
> >
> > The PLIC connects global interrupt sources to the local interrupt controller
> > on each hart.
> >
> > This driver is based on the driver in the RISC-V tree from Palmer Dabbelt,
> > but has been almost entirely rewritten since.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> I tried to boot HighFive Unleashed with the patch series after applying
> all the patches from riscv-all branch except timer & irq patches. It
> gets stuck pretty early.
>
> Here is my github repo with all the changes:
> https://github.com/atishp04/riscv-linux/commits/master_chris_cleanup_hifive
>
> I am still looking into it.
> Palmer: Did I miss something?
>
> FWIW, here is the boot log.
> --------- Boot log -------------------------------------------
> [    0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=5
> [    0.000000] NR_IRQS: 0, nr_irqs: 0, preallocated irqs: 0
> [    0.000000] plic: mapped 53 interrupts to 4 (out of 9) handlers.
> [    0.000000] clocksource: riscv_clocksource: mask: 0xffffffffffffffff
> max_cycles: 0x1d854df40, max_idle_ns: 3526361616960 ns
> [    0.000000] Calibrating delay loop (skipped), value calculated using
> timer frequency.. 2.00 BogoMIPS (lpj=10000)
> [    0.010000] pid_max: default: 32768 minimum: 301
> [    0.010000] Mount-cache hash table entries: 16384 (order: 5, 131072
> bytes)
> [    0.020000] Mountpoint-cache hash table entries: 16384 (order: 5,
> 131072 bytes)
> [    0.020000] Hierarchical SRCU implementation.
> [    0.030000] smp: Bringing up secondary CPUs ...

I have noticed following:

1. plic_irq_toggle() works on all present CPUs which means an
IRQ will be enabled/disabled for all present CPUs. This further
imply that whenever an IRQ is triggered, all online CPUs will take
the interrupt but only one CPU will be successful in claiming the
IRQ and other CPUs will check for IRQ in vain.

2. irq_set_affinity() is not available which means IRQ balancing
will not work.

3. A PLIC context is for a particular HART+MODE. A HW designer
can choose to connect PLIC context only for particular MODE of
HART/CPU whereas this driver assumes that we have context
available for both M-mode and S-mode of all HARTs/CPUs.

Regards,
Anup

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Atish Patra]

On 7/27/18 5:04 PM, Atish Patra wrote:
> On 7/26/18 7:38 AM, Christoph Hellwig wrote:
>> This patch adds a driver for the Platform Level Interrupt Controller (PLIC)
>> specified as part of the RISC-V supervisor level ISA manual, in the memory
>> layout implemented by SiFive and qemu.
>>
>> The PLIC connects global interrupt sources to the local interrupt controller
>> on each hart.
>>
>> This driver is based on the driver in the RISC-V tree from Palmer Dabbelt,
>> but has been almost entirely rewritten since.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> I tried to boot HighFive Unleashed with the patch series after applying
> all the patches from riscv-all branch except timer & irq patches. It
> gets stuck pretty early.
> 
> Here is my github repo with all the changes:
> https://github.com/atishp04/riscv-linux/commits/master_chris_cleanup_hifive
> 
> I am still looking into it.
> 

I found the issue. As per PLIC documentation, a hart context is a given 
privilege mode on a given hart. Thus, cpu context ID & cpu numbers are 
not same. Here is the PLIC register Maps in U54 core:

Ref: https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf

Memory address for Interrupt enable
Address
0x0C00-2080   Hart 1 M-mode enables
0x0C00 2094   End of Hart 1 M-mode enables

0x0C00-2100   Hart 1 S-mode enables
0x0C00-2114   End of Hart 1 S-mode enables

Memory map Claim/Threshold
Address
0x0C20-1000 4B 	 M-mode priority threshold
0x0C20-1004 4B   M-mode claim/complete
0x0C20-2000 4B   S-mode priority threshold
0x0C20-2004 4B   S-mode claim/complete

The original PLIC patch was calculating based on handle->contextid which 
will assume numbers on a HighFive Unleashed board as 2 4 6 8.

In this patch, context id is assigned as cpu numbers which will be 1 2 3 
4. Thus it will lead to incorrect plic address access as shown below.

CPU1 enable register:
plic: plic_toggle: In for hwirq = [1] ctxid [1] reg = [0x2080]
plic: plic_toggle: In for hwirq = [32] ctxid [1] reg = [0x2084]


Palmer: Did I miss something?
> 
> FWIW, here is the boot log.
> --------- Boot log -------------------------------------------
> [    0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=5
> [    0.000000] NR_IRQS: 0, nr_irqs: 0, preallocated irqs: 0
> [    0.000000] plic: mapped 53 interrupts to 4 (out of 9) handlers.
> [    0.000000] clocksource: riscv_clocksource: mask: 0xffffffffffffffff
> max_cycles: 0x1d854df40, max_idle_ns: 3526361616960 ns
> [    0.000000] Calibrating delay loop (skipped), value calculated using
> timer frequency.. 2.00 BogoMIPS (lpj=10000)
> [    0.010000] pid_max: default: 32768 minimum: 301
> [    0.010000] Mount-cache hash table entries: 16384 (order: 5, 131072
> bytes)
> [    0.020000] Mountpoint-cache hash table entries: 16384 (order: 5,
> 131072 bytes)
> [    0.020000] Hierarchical SRCU implementation.
> [    0.030000] smp: Bringing up secondary CPUs ...
>> ---
>>    drivers/irqchip/Kconfig          |  13 ++
>>    drivers/irqchip/Makefile         |   1 +
>>    drivers/irqchip/irq-riscv-plic.c | 219 +++++++++++++++++++++++++++++++
>>    3 files changed, 233 insertions(+)
>>    create mode 100644 drivers/irqchip/irq-riscv-plic.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index e9233db16e03..5ac6dd922a0d 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -372,3 +372,16 @@ config QCOM_PDC
>>    	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>>    
>>    endmenu
>> +
>> +config RISCV_PLIC
>> +	bool "Platform-Level Interrupt Controller"
>> +	depends on RISCV
>> +	default y
>> +	help
>> +	   This enables support for the PLIC chip found in standard RISC-V
>> +	   systems.  The PLIC controls devices interrupts and connects them to
>> +	   each core's local interrupt controller.  Aside from timer and
>> +	   software interrupts, all other interrupt sources (MSI, GPIO, etc)
>> +	   are subordinate to the PLIC.
>> +
>> +	   If you don't know what to do here, say Y.
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 15f268f646bf..bf9238da8a31 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -87,3 +87,4 @@ obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
>>    obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
>>    obj-$(CONFIG_NDS32)			+= irq-ativic32.o
>>    obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
>> +obj-$(CONFIG_RISCV_PLIC)		+= irq-riscv-plic.o
>> diff --git a/drivers/irqchip/irq-riscv-plic.c b/drivers/irqchip/irq-riscv-plic.c
>> new file mode 100644
>> index 000000000000..274fe2cba544
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-riscv-plic.c
>> @@ -0,0 +1,219 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2017 SiFive
>> + * Copyright (C) 2018 Christoph Hellwig
>> + */
>> +#define pr_fmt(fmt) "plic: " fmt
>> +#include <linux/cpumask.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/spinlock.h>
>> +
>> +/*
>> + * From the RISC-V Priviledged Spec v1.10:
>> + *
>> + * Global interrupt sources are assigned small unsigned integer identifiers,
>> + * beginning at the value 1.  An interrupt ID of 0 is reserved to mean "no
>> + * interrupt".  Interrupt identifiers are also used to break ties when two or
>> + * more interrupt sources have the same assigned priority. Smaller values of
>> + * interrupt ID take precedence over larger values of interrupt ID.
>> + *
>> + * While the RISC-V supervisor spec doesn't define the maximum number of
>> + * devices supported by the PLIC, the largest number supported by devices
>> + * marked as 'riscv,plic0' (which is the only device type this driver supports,
>> + * and is the only extant PLIC as of now) is 1024.  As mentioned above, device
>> + * 0 is defined to be non-existent so this device really only supports 1023
>> + * devices.
>> + */
>> +#define MAX_DEVICES			1024
>> +#define MAX_CONTEXTS			15872
>> +
> 
> Is there any way we can preserve some of the comments in the original
> patch about memory-mapped control registers or at least a reference
> where to find the register offset calculations?
> 
> IMHO, it is helpful for anybody who is not familiar with the details.
>> +/*
>> + * Each interrupt source has a priority register associated with it.
>> + * We always hardwire it to one in Linux.
>> + */
>> +#define PRIORITY_BASE			0
>> +#define	    PRIORITY_PER_ID		4
>> +
>> +/*
>> + * Each hart context has a vector of interupt enable bits associated with it.
>> + * There's one bit for each interrupt source.
>> + */
>> +#define ENABLE_BASE			0x2000
>> +#define     ENABLE_PER_HART		0x80
>> +
>> +/*
>> + * Each hart context has a set of control registers associated with it.  Right
>> + * now there's only two: a source priority threshold over which the hart will
>> + * take an interrupt, and a register to claim interrupts.
>> + */
>> +#define CONTEXT_BASE			0x200000
>> +#define     CONTEXT_PER_HART		0x1000
>> +#define     CONTEXT_THRESHOLD		0x00
>> +#define     CONTEXT_CLAIM		0x04
>> +
>> +static void __iomem *plic_regs;
>> +
>> +static inline void __iomem *plic_hart_offset(int ctxid)
>> +{
>> +	return plic_regs + CONTEXT_BASE + ctxid * CONTEXT_PER_HART;
>> +}
>> +
>> +/*
>> + * Protect mask operations on the registers given that we can't assume that
>> + * atomic memory operations work on them.
>> + */
>> +static DEFINE_SPINLOCK(plic_toggle_lock);
>> +
>> +static inline void plic_toggle(int ctxid, int hwirq, int enable)
>> +{
>> +	u32 __iomem *reg = plic_regs + ENABLE_BASE + ctxid * ENABLE_PER_HART;
> 
> shouldn't it be
> u32 __iomem *reg = plic_regs + ENABLE_BASE + ctxid * ENABLE_PER_HART +
> (hwirq / 32) * 4;
> 
> Without that change, plic_handle_irq() gets called before irq mapping as
> plic was not disabled for that irq.
> 
> As per comment in the original patch,
> --------------------------------------------------------------
> * base + 0x002000: Enable bits for sources 0-31 on context 0
> * base + 0x002004: Enable bits for sources 32-63 on context 0
> --------------------------------------------------------------
> 
>> +	u32 hwirq_mask = 1 << (hwirq % 32);
>> +
>> +	spin_lock(&plic_toggle_lock);
>> +	if (enable)
>> +		writel(readl(reg) | hwirq_mask, reg);
>> +	else
>> +		writel(readl(reg) & ~hwirq_mask, reg);
>> +	spin_unlock(&plic_toggle_lock);
>> +}
>> +
>> +static inline void plic_irq_toggle(struct irq_data *d, int enable)
>> +{
>> +	int cpu;
>> +
>> +	writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
>> +	for_each_present_cpu(cpu)
>> +		plic_toggle(cpu, d->hwirq, enable);
>> +}
>> +
>> +static void plic_irq_enable(struct irq_data *d)
>> +{
>> +	plic_irq_toggle(d, 1);
>> +}
>> +
>> +static void plic_irq_disable(struct irq_data *d)
>> +{
>> +	plic_irq_toggle(d, 0);
>> +}
>> +
>> +static struct irq_chip plic_chip = {
>> +	.name		= "riscv,plic0",
>> +	/*
>> +	 * There is no need to mask/unmask PLIC interrupts.  They are "masked"
>> +	 * by reading claim and "unmasked" when writing it back.
>> +	 */
>> +	.irq_enable	= plic_irq_enable,
>> +	.irq_disable	= plic_irq_disable,
>> +};
>> +
>> +static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
>> +			      irq_hw_number_t hwirq)
>> +{
>> +	irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq);
>> +	irq_set_chip_data(irq, NULL);
>> +	irq_set_noprobe(irq);
>> +	return 0;
>> +}
>> +
>> +static const struct irq_domain_ops plic_irqdomain_ops = {
>> +	.map		= plic_irqdomain_map,
>> +	.xlate		= irq_domain_xlate_onecell,
>> +};
>> +
>> +static struct irq_domain *plic_irqdomain;
>> +
>> +/*
>> + * Handling an interrupt is a two-step process: first you claim the interrupt
>> + * by reading the claim register, then you complete the interrupt by writing
>> + * 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)
>> +{
>> +	void __iomem *claim =
>> +		plic_hart_offset(smp_processor_id()) + CONTEXT_CLAIM;
>> +	irq_hw_number_t hwirq;
>> +
>> +	csr_clear(sie, SIE_STIE);
>> +	while ((hwirq = readl(claim))) {
>> +		int irq = irq_find_mapping(plic_irqdomain, hwirq);
>> +
>> +		if (unlikely(irq <= 0)) {
>> +			pr_warn_ratelimited("can't find mapping for hwirq %lu\n",
>> +					hwirq);
> 
> Ratlimiting the warning message here didn't help as ack_bad_irq() still
> print message still flooded the console without any useful info.
> 
> Regards,
> Atish
>> +			ack_bad_irq(irq);
>> +		} else {
>> +			generic_handle_irq(irq);
>> +		}
>> +		writel(hwirq, claim);
>> +	}
>> +	csr_set(sie, SIE_STIE);
>> +}
>> +
>> +static int __init plic_init(struct device_node *node,
>> +		struct device_node *parent)
>> +{
>> +	int error = 0, nr_mapped = 0, nr_handlers, cpu;
>> +	u32 nr_irqs;
>> +
>> +	if (plic_regs) {
>> +		pr_warning("PLIC already present.\n");
>> +		return -ENXIO;
>> +	}
>> +
>> +	plic_regs = of_iomap(node, 0);
>> +	if (WARN_ON(!plic_regs))
>> +		return -EIO;
>> +
>> +	error = -EINVAL;
>> +	of_property_read_u32(node, "riscv,ndev", &nr_irqs);
>> +	if (WARN_ON(!nr_irqs))
>> +		goto out_iounmap;
>> +
>> +	nr_handlers = of_irq_count(node);
>> +	if (WARN_ON(!nr_handlers))
>> +		goto out_iounmap;
>> +	if (WARN_ON(nr_handlers < num_possible_cpus()))
>> +		goto out_iounmap;
>> +
>> +	error = -ENOMEM;
>> +	plic_irqdomain = irq_domain_add_linear(node, nr_irqs + 1,
>> +			&plic_irqdomain_ops, NULL);
>> +	if (WARN_ON(!plic_irqdomain))
>> +		goto out_iounmap;
>> +
>> +	/*
>> +	 * We assume that each present hart is wire up to the PLIC.
>> +	 * If that isn't the case in the future this code will need to be
>> +	 * modified.
>> +	 */
>> +	for_each_present_cpu(cpu) {
>> +		irq_hw_number_t hwirq;
>> +
>> +		/* priority must be > threshold to trigger an interrupt */
>> +		writel(0, plic_hart_offset(cpu) + CONTEXT_THRESHOLD);
In correct context id as explained above.

>> +		for (hwirq = 1; hwirq <= nr_irqs; ++hwirq)
>> +			plic_toggle(cpu, hwirq, 0);

In correct context id as explained above.


Regards,
Atish
>> +		nr_mapped++;
>> +	}
>> +
>> +	pr_info("mapped %d interrupts to %d (out of %d) handlers.\n",
>> +		nr_irqs, nr_mapped, nr_handlers);
>> +	set_handle_irq(plic_handle_irq);
>> +	return 0;
>> +
>> +out_iounmap:
>> +	iounmap(plic_regs);
>> +	return error;
>> +}
>> +
>> +IRQCHIP_DECLARE(plic0, "riscv,plic0", plic_init);
>>
> 
> 

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Christoph Hellwig]

On Fri, Jul 27, 2018 at 05:04:52PM -0700, Atish Patra wrote:
>> +#define MAX_DEVICES			1024
>> +#define MAX_CONTEXTS			15872
>> +
>
> Is there any way we can preserve some of the comments in the original patch 
> about memory-mapped control registers or at least a reference where to find 
> the register offset calculations?

The comments really do not help to describe a why or how.  I'd love to
add a reference to a spec, but I could not find anything that looks
like an authoritative spec for the SiFive PLIC layout.

>> +	u32 __iomem *reg = plic_regs + ENABLE_BASE + ctxid * ENABLE_PER_HART;
>
> shouldn't it be
> u32 __iomem *reg = plic_regs + ENABLE_BASE + ctxid * ENABLE_PER_HART + 
> (hwirq / 32) * 4;

Yes, it should.  Fixed.

>> +		if (unlikely(irq <= 0)) {
>> +			pr_warn_ratelimited("can't find mapping for hwirq %lu\n",
>> +					hwirq);
>
> Ratlimiting the warning message here didn't help as ack_bad_irq() still 
> print message still flooded the console without any useful info.

I've dropped the somewhat pointless ack_bad_irq call, thanks.

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Christoph Hellwig]

On Mon, Jul 30, 2018 at 08:21:33PM -0700, Atish Patra wrote:
> I found the issue. As per PLIC documentation, a hart context is a given 
> privilege mode on a given hart. Thus, cpu context ID & cpu numbers are not 
> same. Here is the PLIC register Maps in U54 core:
>
> Ref: https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf
>
> Memory address for Interrupt enable
> Address
> 0x0C00-2080   Hart 1 M-mode enables
> 0x0C00 2094   End of Hart 1 M-mode enables
>
> 0x0C00-2100   Hart 1 S-mode enables
> 0x0C00-2114   End of Hart 1 S-mode enables
>
> Memory map Claim/Threshold
> Address
> 0x0C20-1000 4B 	 M-mode priority threshold
> 0x0C20-1004 4B   M-mode claim/complete
> 0x0C20-2000 4B   S-mode priority threshold
> 0x0C20-2004 4B   S-mode claim/complete
>
> The original PLIC patch was calculating based on handle->contextid which 
> will assume numbers on a HighFive Unleashed board as 2 4 6 8.
>
> In this patch, context id is assigned as cpu numbers which will be 1 2 3 4. 
> Thus it will lead to incorrect plic address access as shown below.

Indeed.  Can you try this branch, which puts back the OF contextid
parsing from the original code:

   git://git.infradead.org/users/hch/riscv.git riscv-irq-simple.2

Gitweb:

   http://git.infradead.org/users/hch/riscv.git/shortlog/refs/heads/riscv-irq-simple.2

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Atish Patra]

On 7/31/18 9:52 AM, Christoph Hellwig wrote:
> On Mon, Jul 30, 2018 at 08:21:33PM -0700, Atish Patra wrote:
>> I found the issue. As per PLIC documentation, a hart context is a given
>> privilege mode on a given hart. Thus, cpu context ID & cpu numbers are not
>> same. Here is the PLIC register Maps in U54 core:
>>
>> Ref: https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf
>>
>> Memory address for Interrupt enable
>> Address
>> 0x0C00-2080   Hart 1 M-mode enables
>> 0x0C00 2094   End of Hart 1 M-mode enables
>>
>> 0x0C00-2100   Hart 1 S-mode enables
>> 0x0C00-2114   End of Hart 1 S-mode enables
>>
>> Memory map Claim/Threshold
>> Address
>> 0x0C20-1000 4B 	 M-mode priority threshold
>> 0x0C20-1004 4B   M-mode claim/complete
>> 0x0C20-2000 4B   S-mode priority threshold
>> 0x0C20-2004 4B   S-mode claim/complete
>>
>> The original PLIC patch was calculating based on handle->contextid which
>> will assume numbers on a HighFive Unleashed board as 2 4 6 8.
>>
>> In this patch, context id is assigned as cpu numbers which will be 1 2 3 4.
>> Thus it will lead to incorrect plic address access as shown below.
> 
> Indeed.  Can you try this branch, which puts back the OF contextid
> parsing from the original code:
> 
>     git://git.infradead.org/users/hch/riscv.git riscv-irq-simple.2
> 
> Gitweb:
> 
>     http://git.infradead.org/users/hch/riscv.git/shortlog/refs/heads/riscv-irq-simple.2
> 
> 

Some typos in the above repo in the PLIC driver patch. The following 
changes are required. Inline patch below

diff --git a/drivers/irqchip/irq-riscv-plic.c 
b/drivers/irqchip/irq-riscv-plic.c
index 0e524e3e..9dbaca47 100644
--- a/drivers/irqchip/irq-riscv-plic.c
+++ b/drivers/irqchip/irq-riscv-plic.c
@@ -79,7 +79,7 @@ static DEFINE_SPINLOCK(plic_toggle_lock);
  static inline void plic_toggle(int ctxid, int hwirq, int enable)
  {
         u32 __iomem *reg = plic_regs + ENABLE_BASE +
-               ctxid * ENABLE_PER_HART + (hwirq / 32);
+               ctxid * ENABLE_PER_HART + (hwirq / 32) * 4;
         u32 hwirq_mask = 1 << (hwirq % 32);

         spin_lock(&plic_toggle_lock);
@@ -166,7 +166,7 @@ static void plic_handle_irq(struct pt_regs *regs)
  static int __init plic_init(struct device_node *node,
                 struct device_node *parent)
  {
-       int error = 0, nr_mapped = 0, cpu, i;
+       int error = 0, nr_mapped = 0, i;
         u32 nr_irqs;

         if (plic_regs) {
@@ -211,8 +211,7 @@ static int __init plic_init(struct device_node *node,
                         pr_err("invalid OF parent, skipping context 
%d.\n", i);
                         continue;
                 }
-
-               if (riscv_of_processor_hart(parent.np->parent < 0))
+               if (riscv_of_processor_hart(parent.np->parent) < 0)
                         continue;

                 plic_handler_present[i] = true;


With the above changes, I am able to boot quite far. But it still 
crashes which may be a driver issue. I might have missed something while 
merging all the out-of-tree drivers from riscv-all branch.

Here is my git repo.
https://github.com/atishp04/riscv-linux/tree/master_chris_cleanup_v4

crash details are at
https://paste.debian.net/1036078/

Regards,
Atish

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Christoph Hellwig]

On Tue, Jul 31, 2018 at 05:38:01PM -0700, Atish Patra wrote:
> Some typos in the above repo in the PLIC driver patch. The following 
> changes are required. Inline patch below

Thanks, I'll fold this in.

> With the above changes, I am able to boot quite far. But it still crashes 
> which may be a driver issue. I might have missed something while merging 

It probably is.  For now I want the qemu virt machine to boot the kernel
at least as that unblocks a lot of things like enabling CI bots and
people being able to actually hack and test their contributions upstream.

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Christoph Hellwig]

On Tue, Jul 31, 2018 at 05:38:01PM -0700, Atish Patra wrote:
> crash details are at
> https://paste.debian.net/1036078/

Is this running without kallsyms?  It seems to lack useful symbols
and a backtrace unfortunately.

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Christoph Hellwig]

I've pushed out an update to the riscv-irq-simple.2 branch to better
handle with sparse contexid maps, please retry with that.

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Atish Patra]

On 8/1/18 7:14 AM, Christoph Hellwig wrote:
> I've pushed out an update to the riscv-irq-simple.2 branch to better
> handle with sparse contexid maps, please retry with that.
> 

I see you have changed the driver file name from irq-riscv-plic to 
irq-riscv-sifive along with default Y for SIFIVE_PLIC. I guess it was 
done because PLIC register spec is SIFIVE specific rather than RISC-V.

But can we keep the new kconfig option "SIFIVE_PLIC" enabled in 
driver/irqchip/Kconfig or arch/riscv/Kconfig for now to avoid breakage 
without linux_defconfig update.

With the config enabled, Qemu virt machine booting has no issues with 
riscv-irq-simple.2 branch. I am still looking at the crash from the 
hardware.

Regards,
Atish

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Atish Patra]

On 8/1/18 5:12 AM, Christoph Hellwig wrote:
> On Tue, Jul 31, 2018 at 05:38:01PM -0700, Atish Patra wrote:
>> crash details are at
>> https://paste.debian.net/1036078/
> 
> Is this running without kallsyms?  It seems to lack useful symbols
> and a backtrace unfortunately.
> 
Yes. I checked the config. All KALLSYMS options and STACKTRACE are enabled

CONFIG_KALLSYMS=y
CONFIG_KALLSYMS_ALL=y
CONFIG_KALLSYMS_BASE_RELATIVE=y

CONFIG_STACKTRACE_SUPPORT=y
CONFIG_STACKTRACE=y

Not sure if I missed something.

Regards,
Atish

Re: [PATCH 8/9] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Nikolay Borisov]

On 26.07.2018 17:37, Christoph Hellwig wrote:
> From: Palmer Dabbelt <palmer@dabbelt.com>
> 
> This patch adds documentation for the platform-level interrupt
> controller (PLIC) found in all RISC-V systems.  This interrupt
> controller routes interrupts from all the devices in the system to each
> hart-local interrupt controller.
> 
> Note: the DTS bindings for the PLIC aren't set in stone yet, as we might
> want to change how we're specifying holes in the hart list.
> 
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> ---
>  .../interrupt-controller/riscv,plic0.txt      | 55 +++++++++++++++++++
>  1 file changed, 55 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt
> new file mode 100644
> index 000000000000..99cd359dbd43
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,plic0.txt
> @@ -0,0 +1,55 @@
> +RISC-V Platform-Level Interrupt Controller (PLIC)
> +-------------------------------------------------
> +
> +The RISC-V supervisor ISA specification allows for the presence of a
> +platform-level interrupt controller (PLIC).   The PLIC connects all external
> +interrupts in the system to all hart contexts in the system, via the external
> +interrupt source in each hart's hart-local interrupt controller (HLIC).  A hart
> +context is a privilege mode in a hardware execution thread.  For example, in
> +an 4 core system with 2-way SMT, you have 8 harts and probably at least two
> +privilege modes per hart; machine mode and supervisor mode.
> +
> +Each interrupt can be enabled on per-context basis. Any context can claim
> +a pending enabled interrupt and then release it once it has been handled.
> +
> +Each interrupt has a configurable priority. Higher priority interrupts are
> +serviced firs. Each context can specify a priority threshold. Interrupts
                ^^ missing 't'
> +with priority below this threshold will not cause the PLIC to raise its
> +interrupt line leading to the context.
> +
> +While the PLIC supports both edge-triggered and level-triggered interrupts,
> +interrupt handlers are oblivious to this distinction and therefor it is not
                                                                    ^^
missing 'e'
> +specific in the PLIC device-tree binding.
> +
> +While the RISC-V ISA doesn't specify a memory layout for the PLIC, the
> +"riscv,plic0" device is a concrete implementation of the PLIC that contains a
> +specific memory layout.  More details about the memory layout of the
> +"riscv,plic0" device can be found as a comment in the device driver, or as part
> +of the SiFive U5 Coreplex Series Manual (page 22 of the PDF of version 1.0)
> +<https://www.sifive.com/documentation/coreplex/u5-coreplex-series-manual/>
> +
> +Required properties:
> +- compatible : "riscv,plic0"
> +- #address-cells : should be <0>
> +- #interrupt-cells : should be <1>
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- reg : Should contain 1 register range (address and length)
> +- interrupts-extended : Specifies which contexts are connected to the PLIC,
> +  with "-1" specifying that a context is not present.
> +
> +Example:
> +
> +	plic: interrupt-controller@c000000 {
> +		#address-cells = <0>;
> +		#interrupt-cells = <1>;
> +		compatible = "riscv,plic0";
> +		interrupt-controller;
> +		interrupts-extended = <
> +			&cpu0-intc 11
> +			&cpu1-intc 11 &cpu1-intc 9
> +			&cpu2-intc 11 &cpu2-intc 9
> +			&cpu3-intc 11 &cpu3-intc 9
> +			&cpu4-intc 11 &cpu4-intc 9>;
> +		reg = <0xc000000 0x4000000>;
> +		riscv,ndev = <10>;
> +	};
> 

Re: [PATCH 5/9] RISC-V: implement low-level interrupt handling

[Thomas Gleixner]

On Thu, 26 Jul 2018, Christoph Hellwig wrote:
> Add support for a routine that dispatches exceptions with the interrupt
> flags set to either the IPI or irqdomain code (and the clock source in the
> future).
> 
> Loosely based on the irq-riscv-int.c irqchip driver from the RISC-V tree.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/riscv/kernel/entry.S |  4 +--
>  arch/riscv/kernel/irq.c   | 52 ++++++++++++++++++++++++++++++++-------
>  2 files changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index 9aaf6c986771..fa2c08e3c05e 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -168,8 +168,8 @@ ENTRY(handle_exception)
>  
>  	/* Handle interrupts */
>  	move a0, sp /* pt_regs */
> -	REG_L a1, handle_arch_irq
> -	jr a1
> +	move a1, s4 /* scause */
> +	tail do_IRQ

What's the reason for doing the whole exception dance in ASM ?

>  1:
>  	/* Exceptions run with interrupts enabled */
>  	csrs sstatus, SR_SIE

> +asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs, unsigned long cause)
> +{
> +	struct pt_regs *old_regs = set_irq_regs(regs);
> +
> +	irq_enter();
> +	switch (cause & ~INTERRUPT_CAUSE_FLAG) {
> +#ifdef CONFIG_SMP
> +	case INTERRUPT_CAUSE_SOFTWARE:
> +		/*
> +		 * 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();
> +		break;
> +#endif
> +	case INTERRUPT_CAUSE_EXTERNAL:
> +		handle_arch_irq(regs);
> +		break;
> +	default:
> +		panic("unexpected interrupt cause");
> +	}
> +	irq_exit();

Looks about right.

Thanks,

	tglx

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Christoph Hellwig]

On Wed, Aug 01, 2018 at 06:02:52PM -0700, Atish Patra wrote:
> But can we keep the new kconfig option "SIFIVE_PLIC" enabled in 
> driver/irqchip/Kconfig or arch/riscv/Kconfig for now to avoid breakage 
> without linux_defconfig update.

I'll update arch/riscv/configs/defconfig for the next repost.

Re: [PATCH 8/9] dt-bindings: interrupt-controller: RISC-V PLIC documentation

[Christoph Hellwig]

Thanks Nikolay,

I've added all the spelling fixes.

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Christoph Hellwig]

On Wed, Aug 01, 2018 at 06:09:42PM -0700, Atish Patra wrote:
> On 8/1/18 5:12 AM, Christoph Hellwig wrote:
>> On Tue, Jul 31, 2018 at 05:38:01PM -0700, Atish Patra wrote:
>>> crash details are at
>>> https://paste.debian.net/1036078/
>>
>> Is this running without kallsyms?  It seems to lack useful symbols
>> and a backtrace unfortunately.
>>
> Yes. I checked the config. All KALLSYMS options and STACKTRACE are enabled
>
> CONFIG_KALLSYMS=y
> CONFIG_KALLSYMS_ALL=y
> CONFIG_KALLSYMS_BASE_RELATIVE=y
>
> CONFIG_STACKTRACE_SUPPORT=y
> CONFIG_STACKTRACE=y
>
> Not sure if I missed something.

That should be all that is needed normally.

Re: [PATCH 5/9] RISC-V: implement low-level interrupt handling

[Christoph Hellwig]

On Thu, Aug 02, 2018 at 11:48:55AM +0200, Thomas Gleixner wrote:
> > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> > index 9aaf6c986771..fa2c08e3c05e 100644
> > --- a/arch/riscv/kernel/entry.S
> > +++ b/arch/riscv/kernel/entry.S
> > @@ -168,8 +168,8 @@ ENTRY(handle_exception)
> >  
> >  	/* Handle interrupts */
> >  	move a0, sp /* pt_regs */
> > -	REG_L a1, handle_arch_irq
> > -	jr a1
> > +	move a1, s4 /* scause */
> > +	tail do_IRQ
> 
> What's the reason for doing the whole exception dance in ASM ?

I'll let Palmer defend it.  But for now I just want minimal changes
to actually get a booting system..

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Thomas Gleixner]

On Thu, 26 Jul 2018, Christoph Hellwig wrote:

> This patch adds a driver for the Platform Level Interrupt Controller (PLIC)

See Documentation/process/submitting-patches.rst and search for 'This patch'

> +static inline void __iomem *plic_hart_offset(int ctxid)
> +{
> +	return plic_regs + CONTEXT_BASE + ctxid * CONTEXT_PER_HART;
> +}
> +
> +/*
> + * Protect mask operations on the registers given that we can't assume that
> + * atomic memory operations work on them.
> + */
> +static DEFINE_SPINLOCK(plic_toggle_lock);

RAW_SPINLOCK please.

> +
> +static inline void plic_toggle(int ctxid, int hwirq, int enable)
> +{
> +	u32 __iomem *reg = plic_regs + ENABLE_BASE + ctxid * ENABLE_PER_HART;
> +	u32 hwirq_mask = 1 << (hwirq % 32);
> +
> +	spin_lock(&plic_toggle_lock);
> +	if (enable)
> +		writel(readl(reg) | hwirq_mask, reg);
> +	else
> +		writel(readl(reg) & ~hwirq_mask, reg);
> +	spin_unlock(&plic_toggle_lock);
> +}
> +
> +static inline void plic_irq_toggle(struct irq_data *d, int enable)
> +{
> +	int cpu;
> +
> +	writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
> +	for_each_present_cpu(cpu)
> +		plic_toggle(cpu, d->hwirq, enable);

I suggest to make that:

	for_each_cpu(cpu, irq_data_get_affinity_mask(d))
		plic_toggle(cpu, d->hwirq, enable);
       
That gives you immediately support for interrupt affinity. And then it's
trivial to do the actual irq_chip::irq_set_affinity() magic as well.

> +/*
> + * Handling an interrupt is a two-step process: first you claim the interrupt
> + * by reading the claim register, then you complete the interrupt by writing
> + * 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)
> +{
> +	void __iomem *claim =
> +		plic_hart_offset(smp_processor_id()) + CONTEXT_CLAIM;

Either ignore the 80 char thing or just move the assignment into the code
section please. That line break is horrible to read.

> +	irq_hw_number_t hwirq;
> +

Other than that this looks halfways sane.

Thanks,

	tglx

Re: [PATCH 7/9] irqchip: add a RISC-V PLIC driver

[Christoph Hellwig]

On Thu, Aug 02, 2018 at 12:04:04PM +0200, Thomas Gleixner wrote:
> 
> On Thu, 26 Jul 2018, Christoph Hellwig wrote:
> 
> > This patch adds a driver for the Platform Level Interrupt Controller (PLIC)
> 
> See Documentation/process/submitting-patches.rst and search for 'This patch'

Fixed.

> > +static DEFINE_SPINLOCK(plic_toggle_lock);
> 
> RAW_SPINLOCK please.

Done.

> > +static inline void plic_irq_toggle(struct irq_data *d, int enable)
> > +{
> > +	int cpu;
> > +
> > +	writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
> > +	for_each_present_cpu(cpu)
> > +		plic_toggle(cpu, d->hwirq, enable);
> 
> I suggest to make that:
> 
> 	for_each_cpu(cpu, irq_data_get_affinity_mask(d))
> 		plic_toggle(cpu, d->hwirq, enable);

Done.

> That gives you immediately support for interrupt affinity. And then it's
> trivial to do the actual irq_chip::irq_set_affinity() magic as well.

I'll defer that to an incremental patch (added to my todo list).

> > +static void plic_handle_irq(struct pt_regs *regs)
> > +{
> > +	void __iomem *claim =
> > +		plic_hart_offset(smp_processor_id()) + CONTEXT_CLAIM;
> 
> Either ignore the 80 char thing or just move the assignment into the code
> section please. That line break is horrible to read.

That area has been rewritten anyway as we need a cpuid to context
lookup to cover real SOCs vs just qemu.