Driver for the RISC-V Interrupt Controller

Driver for the RISC-V Interrupt Controller

[Palmer Dabbelt]

The RISC-V ISA mandantes the presence of a simple, per-hart (hardware
thread) interrupt controller availiable to supervisor mode.  This patch
set adds a driver for this interrupt controller.

The patch set itself has been around in various flavors for a while, but
as far as I remember it's never been properly cleaned up and submitted
before.  As it currently stands it's essentially three seperate patches,
but as they're all intertwined I'm keeping them together.  Sorry if
that's the wrong thing to do.

The patches are:

* A cleanup to arch/riscv to remove a bit of the old initialization
  mechanism that snuck in to our arch port.  This patch is trivial, so
  unless there's any feedback on it specificly I'll include it in my
  next pull request.
* The addition of device tree bindings to describe "riscv,cpu-intc".
* The actual irqchip driver.  If this gets merged before the arch/riscv
  patch then it'll cause our build to fail, but I'm assuming this will
  be targeted at the next merge window so we should be safe.

[PATCH 1/3] RISC-V: Don't include irq-riscv-intc.h

[Palmer Dabbelt]

This file has never existed in the upstream kernel, but it's guarded by
an #ifdef that's also never existed in the upstream kernel.  As a part
of our interrupt controller refactoring this header is no longer
necessary, but this reference managed to sneak in anyway.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 arch/riscv/kernel/irq.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
index b74cbfbce2d0..7bcdaed15703 100644
--- a/arch/riscv/kernel/irq.c
+++ b/arch/riscv/kernel/irq.c
@@ -16,10 +16,6 @@
 #include <linux/irqchip.h>
 #include <linux/irqdomain.h>
 
-#ifdef CONFIG_RISCV_INTC
-#include <linux/irqchip/irq-riscv-intc.h>
-#endif
-
 void __init init_IRQ(void)
 {
 	irqchip_init();
-- 
2.16.4

Re: [PATCH 1/3] RISC-V: Don't include irq-riscv-intc.h

[Christoph Hellwig]

On Fri, Jun 22, 2018 at 04:20:04PM -0700, Palmer Dabbelt wrote:
> This file has never existed in the upstream kernel, but it's guarded by
> an #ifdef that's also never existed in the upstream kernel.  As a part
> of our interrupt controller refactoring this header is no longer
> necessary, but this reference managed to sneak in anyway.
> 
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/3] dt-bindings: interrupt-controller: RISC-V local interrupt controller docs

[Palmer Dabbelt]

From: Palmer Dabbelt <palmer@dabbelt.com>

This patch adds documentation on the RISC-V local interrupt controller,
which is a per-hart interrupt controller that manages all interrupts
entering a RISC-V hart.  This interrupt controller is present on all
RISC-V systems.

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

diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
new file mode 100644
index 000000000000..61900e2e3868
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
@@ -0,0 +1,41 @@
+RISC-V Hart-Level Interrupt Controller (HLIC)
+---------------------------------------------
+
+RISC-V cores include Control Status Registers (CSRs) which are local to each
+hart and can be read or written by software. Some of these CSRs are used to
+control local interrupts connected to the core.  Every interrupt is ultimately
+routed through a hart's HLIC before it interrupts that hart.
+
+The RISC-V supervisor ISA manual specifies three interrupt sources that are
+attached to every HLIC: software interrupts, the timer interrupt, and external
+interrupts.  Software interrupts are used to send IPIs between cores.  The
+timer interrupt comes from an architecturally mandated real-time timer that is
+controller via SBI calls and CSR reads.  External interrupts connect all other
+device interrupts to the HLIC, which are routed via the platform-level
+interrupt controller (PLIC).
+
+All RISC-V systems that conform to the supervisor ISA specification are
+required to have a HLIC with these three interrupt sources present.  Since the
+interrupt map is defined by the ISA it's not listed in the HLIC's device tree
+entry, though external interrupt controllers (like the PLIC, for example) will
+need to define how their interrupts map to the relevant HLICs.
+
+Required properties:
+- compatible : "riscv,cpu-intc"
+- #interrupt-cells : should be <1>
+- interrupt-controller : Identifies the node as an interrupt controller
+
+Furthermore, this interrupt-controller MUST be embedded inside the cpu
+definition of the hart whose CSRs control these local interrupts.
+
+An example device tree entry for a HLIC is show below.
+
+	cpu1: cpu@1 {
+		compatible = "riscv";
+		...
+		cpu1-intc: interrupt-controller {
+			#interrupt-cells = <1>;
+			compatible = "riscv,cpu-intc";
+			interrupt-controller;
+		};
+	};
-- 
2.16.4

Re: [PATCH 2/3] dt-bindings: interrupt-controller: RISC-V local interrupt controller docs

[Christoph Böhmwalder]

On Fri, Jun 22, 2018 at 04:20:05PM -0700, Palmer Dabbelt wrote:
> From: Palmer Dabbelt <palmer@dabbelt.com>
> 
> This patch adds documentation on the RISC-V local interrupt controller,
> which is a per-hart interrupt controller that manages all interrupts
> entering a RISC-V hart.  This interrupt controller is present on all
> RISC-V systems.
> 
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> ---
>  .../interrupt-controller/riscv,cpu-intc.txt        | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
> new file mode 100644
> index 000000000000..61900e2e3868
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
> @@ -0,0 +1,41 @@
> +RISC-V Hart-Level Interrupt Controller (HLIC)
> +---------------------------------------------
> +
> +RISC-V cores include Control Status Registers (CSRs) which are local to each
> +hart and can be read or written by software. Some of these CSRs are used to
> +control local interrupts connected to the core.  Every interrupt is ultimately
> +routed through a hart's HLIC before it interrupts that hart.
> +
> +The RISC-V supervisor ISA manual specifies three interrupt sources that are
> +attached to every HLIC: software interrupts, the timer interrupt, and external
> +interrupts.  Software interrupts are used to send IPIs between cores.  The
> +timer interrupt comes from an architecturally mandated real-time timer that is
> +controller via SBI calls and CSR reads.  External interrupts connect all other
> +device interrupts to the HLIC, which are routed via the platform-level
> +interrupt controller (PLIC).
> +
> +All RISC-V systems that conform to the supervisor ISA specification are
> +required to have a HLIC with these three interrupt sources present.  Since the
> +interrupt map is defined by the ISA it's not listed in the HLIC's device tree
> +entry, though external interrupt controllers (like the PLIC, for example) will
> +need to define how their interrupts map to the relevant HLICs.
> +
> +Required properties:
> +- compatible : "riscv,cpu-intc"
> +- #interrupt-cells : should be <1>
> +- interrupt-controller : Identifies the node as an interrupt controller
> +
> +Furthermore, this interrupt-controller MUST be embedded inside the cpu
> +definition of the hart whose CSRs control these local interrupts.
> +
> +An example device tree entry for a HLIC is show below.

Spotted a typo here, "show" -> "shown".

> +
> +	cpu1: cpu@1 {
> +		compatible = "riscv";
> +		...
> +		cpu1-intc: interrupt-controller {
> +			#interrupt-cells = <1>;
> +			compatible = "riscv,cpu-intc";
> +			interrupt-controller;
> +		};
> +	};
> -- 
> 2.16.4

Also, I've noticed that double spaces after punctuation are used pretty
inconsistently throughout the document. Is that intended?

--
Regards,
Christoph

Re: [PATCH 2/3] dt-bindings: interrupt-controller: RISC-V local interrupt controller docs

[Palmer Dabbelt]

On Mon, 25 Jun 2018 13:04:48 PDT (-0700), christoph@boehmwalder.at wrote:
> On Fri, Jun 22, 2018 at 04:20:05PM -0700, Palmer Dabbelt wrote:
>> From: Palmer Dabbelt <palmer@dabbelt.com>
>>
>> This patch adds documentation on the RISC-V local interrupt controller,
>> which is a per-hart interrupt controller that manages all interrupts
>> entering a RISC-V hart.  This interrupt controller is present on all
>> RISC-V systems.
>>
>> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>> ---
>>  .../interrupt-controller/riscv,cpu-intc.txt        | 41 ++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
>> new file mode 100644
>> index 000000000000..61900e2e3868
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
>> @@ -0,0 +1,41 @@
>> +RISC-V Hart-Level Interrupt Controller (HLIC)
>> +---------------------------------------------
>> +
>> +RISC-V cores include Control Status Registers (CSRs) which are local to each
>> +hart and can be read or written by software. Some of these CSRs are used to
>> +control local interrupts connected to the core.  Every interrupt is ultimately
>> +routed through a hart's HLIC before it interrupts that hart.
>> +
>> +The RISC-V supervisor ISA manual specifies three interrupt sources that are
>> +attached to every HLIC: software interrupts, the timer interrupt, and external
>> +interrupts.  Software interrupts are used to send IPIs between cores.  The
>> +timer interrupt comes from an architecturally mandated real-time timer that is
>> +controller via SBI calls and CSR reads.  External interrupts connect all other
>> +device interrupts to the HLIC, which are routed via the platform-level
>> +interrupt controller (PLIC).
>> +
>> +All RISC-V systems that conform to the supervisor ISA specification are
>> +required to have a HLIC with these three interrupt sources present.  Since the
>> +interrupt map is defined by the ISA it's not listed in the HLIC's device tree
>> +entry, though external interrupt controllers (like the PLIC, for example) will
>> +need to define how their interrupts map to the relevant HLICs.
>> +
>> +Required properties:
>> +- compatible : "riscv,cpu-intc"
>> +- #interrupt-cells : should be <1>
>> +- interrupt-controller : Identifies the node as an interrupt controller
>> +
>> +Furthermore, this interrupt-controller MUST be embedded inside the cpu
>> +definition of the hart whose CSRs control these local interrupts.
>> +
>> +An example device tree entry for a HLIC is show below.
>
> Spotted a typo here, "show" -> "shown".

Thanks.  It looks like we're actually dropping this binding and integrating 
this first-level interrupt controller into the core RISC-V arch code as keeping 
it split out results in too many inefficiencies.

>> +
>> +	cpu1: cpu@1 {
>> +		compatible = "riscv";
>> +		...
>> +		cpu1-intc: interrupt-controller {
>> +			#interrupt-cells = <1>;
>> +			compatible = "riscv,cpu-intc";
>> +			interrupt-controller;
>> +		};
>> +	};
>> --
>> 2.16.4
>
> Also, I've noticed that double spaces after punctuation are used pretty
> inconsistently throughout the document. Is that intended?

No.  I noticed this in the PLIC document as well, I'll fix that one up.

Thanks!

Re: [PATCH 2/3] dt-bindings: interrupt-controller: RISC-V local interrupt controller docs

[Rob Herring]

On Fri, Jun 22, 2018 at 04:20:05PM -0700, Palmer Dabbelt wrote:
> From: Palmer Dabbelt <palmer@dabbelt.com>
> 
> This patch adds documentation on the RISC-V local interrupt controller,
> which is a per-hart interrupt controller that manages all interrupts
> entering a RISC-V hart.  This interrupt controller is present on all
> RISC-V systems.
> 
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> ---
>  .../interrupt-controller/riscv,cpu-intc.txt        | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
> new file mode 100644
> index 000000000000..61900e2e3868
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,cpu-intc.txt
> @@ -0,0 +1,41 @@
> +RISC-V Hart-Level Interrupt Controller (HLIC)
> +---------------------------------------------
> +
> +RISC-V cores include Control Status Registers (CSRs) which are local to each
> +hart and can be read or written by software. Some of these CSRs are used to
> +control local interrupts connected to the core.  Every interrupt is ultimately
> +routed through a hart's HLIC before it interrupts that hart.
> +
> +The RISC-V supervisor ISA manual specifies three interrupt sources that are
> +attached to every HLIC: software interrupts, the timer interrupt, and external
> +interrupts.  Software interrupts are used to send IPIs between cores.  The
> +timer interrupt comes from an architecturally mandated real-time timer that is
> +controller via SBI calls and CSR reads.  External interrupts connect all other
> +device interrupts to the HLIC, which are routed via the platform-level
> +interrupt controller (PLIC).
> +
> +All RISC-V systems that conform to the supervisor ISA specification are
> +required to have a HLIC with these three interrupt sources present.  Since the
> +interrupt map is defined by the ISA it's not listed in the HLIC's device tree
> +entry, though external interrupt controllers (like the PLIC, for example) will
> +need to define how their interrupts map to the relevant HLICs.

What are the PLIC to HLIC connections you need to support? 1-to-1 is 
easy. But I would imagine you'd want the PLIC irq to go to all the HLICs 
which can't really be described unless you list every CPU's HLIC in 
PLIC "interrupts" property.

We avoid this problem on ARM because a single DT interrupt controller 
node represents even per cpu interrupts and interrupt cells are used to 
indicate which CPU interrupts are routed too. That's not perfect either, 
but seems to be good enough (though maybe Marc Z or Mark R have more 
thoughts on that).

> +
> +Required properties:
> +- compatible : "riscv,cpu-intc"

Kind of vague. There's only one implementation and one set of bugs?

> +- #interrupt-cells : should be <1>
> +- interrupt-controller : Identifies the node as an interrupt controller
> +
> +Furthermore, this interrupt-controller MUST be embedded inside the cpu
> +definition of the hart whose CSRs control these local interrupts.
> +
> +An example device tree entry for a HLIC is show below.
> +
> +	cpu1: cpu@1 {
> +		compatible = "riscv";
> +		...
> +		cpu1-intc: interrupt-controller {
> +			#interrupt-cells = <1>;
> +			compatible = "riscv,cpu-intc";
> +			interrupt-controller;
> +		};
> +	};
> -- 
> 2.16.4
> 

[PATCH 3/3] irqchip: RISC-V Local Interrupt Controller Driver

[Palmer Dabbelt]

From: Palmer Dabbelt <palmer@dabbelt.com>

This patch adds a driver that manages the local interrupts on each
RISC-V hart, as specifiec by the RISC-V supervisor level ISA manual.
The local interrupt controller manages software interrupts, timer
interrupts, and hardware interrupts (which are routed via the
platform level interrupt controller).  Per-hart local interrupt
controllers are found on all RISC-V systems.

CC: Michael Clark <mjc@sifive.com>
Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 drivers/irqchip/Kconfig          |  13 +++
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-riscv-intc.c | 215 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 229 insertions(+)
 create mode 100644 drivers/irqchip/irq-riscv-intc.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index e9233db16e03..bf7fc86673b1 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_INTC
+	#bool "RISC-V Interrupt Controller"
+	depends on RISCV
+	default y
+	help
+	   This enables support for the local interrupt controller found in
+	   standard RISC-V systems.  The local interrupt controller handles
+	   timer interrupts, software interrupts, and hardware interrupts.
+	   Without a local interrupt controller the system will be unable to
+	   handle any interrupts, including those passed via 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..74e333cc274c 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_INTC)		+= irq-riscv-intc.o
diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
new file mode 100644
index 000000000000..7ca3060e76b4
--- /dev/null
+++ b/drivers/irqchip/irq-riscv-intc.c
@@ -0,0 +1,215 @@
+/*
+ * Copyright (C) 2012 Regents of the University of California
+ * Copyright (C) 2017-2018 SiFive
+ */
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/ftrace.h>
+#include <linux/of.h>
+#include <linux/seq_file.h>
+
+#include <asm/ptrace.h>
+#include <asm/sbi.h>
+#include <asm/smp.h>
+
+#define PTR_BITS (8 * sizeof(uintptr_t))
+
+struct riscv_irq_data {
+	struct irq_chip		chip;
+	struct irq_domain	*domain;
+	int			hart;
+	char			name[20];
+};
+DEFINE_PER_CPU(struct riscv_irq_data, riscv_irq_data);
+
+static void riscv_software_interrupt(void)
+{
+#ifdef CONFIG_SMP
+	irqreturn_t ret;
+
+	ret = handle_ipi();
+
+	WARN_ON(ret == IRQ_NONE);
+#else
+	/*
+	 * We currently only use software interrupts to pass inter-processor
+	 * interrupts, so if a non-SMP system gets a software interrupt then we
+	 * don't know what to do.
+	 */
+	pr_warning("Software Interrupt without CONFIG_SMP\n");
+#endif
+}
+
+static void riscv_intc_irq(struct pt_regs *regs)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+	struct irq_domain *domain;
+	unsigned long cause = csr_read(scause);
+
+	/*
+	 * 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 here.
+	 */
+	WARN_ON((cause & (1UL << (PTR_BITS - 1))) == 0);
+	cause = cause & ~(1UL << (PTR_BITS - 1));
+
+	irq_enter();
+
+	/*
+	 * There are three classes of interrupt: timer, software, and
+	 * external devices.  We dispatch between them here.  External
+	 * device interrupts use the generic IRQ mechanisms.
+	 */
+	switch (cause) {
+	case INTERRUPT_CAUSE_TIMER:
+		riscv_timer_interrupt();
+		break;
+	case INTERRUPT_CAUSE_SOFTWARE:
+		riscv_software_interrupt();
+		break;
+	default:
+		domain = per_cpu(riscv_irq_data, smp_processor_id()).domain;
+		generic_handle_irq(irq_find_mapping(domain, cause));
+		break;
+	}
+
+	irq_exit();
+	set_irq_regs(old_regs);
+}
+
+static int riscv_irqdomain_map(struct irq_domain *d, unsigned int irq,
+			       irq_hw_number_t hwirq)
+{
+	struct riscv_irq_data *data = d->host_data;
+
+	irq_set_chip_and_handler(irq, &data->chip, handle_simple_irq);
+	irq_set_chip_data(irq, data);
+	irq_set_noprobe(irq);
+	irq_set_affinity(irq, cpumask_of(data->hart));
+
+	return 0;
+}
+
+static const struct irq_domain_ops riscv_irqdomain_ops = {
+	.map	= riscv_irqdomain_map,
+	.xlate	= irq_domain_xlate_onecell,
+};
+
+/*
+ * On RISC-V systems local interrupts are masked or unmasked by writing the SIE
+ * (Supervisor Interrupt Enable) CSR.  As CSRs can only be written on the local
+ * hart, these functions can only be called on the hart that corresponds to the
+ * IRQ chip.  They are only called internally to this module, so they BUG_ON if
+ * this condition is violated rather than attempting to handle the error by
+ * forwarding to the target hart, as that's already expected to have been done.
+ */
+static void riscv_irq_mask(struct irq_data *d)
+{
+	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
+
+	BUG_ON(smp_processor_id() != data->hart);
+	csr_clear(sie, 1 << (long)d->hwirq);
+}
+
+static void riscv_irq_unmask(struct irq_data *d)
+{
+	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
+
+	BUG_ON(smp_processor_id() != data->hart);
+	csr_set(sie, 1 << (long)d->hwirq);
+}
+
+/* Callbacks for twiddling SIE on another hart. */
+static void riscv_irq_enable_helper(void *d)
+{
+	riscv_irq_unmask(d);
+}
+
+static void riscv_irq_disable_helper(void *d)
+{
+	riscv_irq_mask(d);
+}
+
+static void riscv_remote_ctrl(unsigned int cpu, void (*fn)(void *d),
+                              struct irq_data *data)
+{
+	smp_call_function_single(cpu, fn, data, true);
+}
+
+static void riscv_irq_enable(struct irq_data *d)
+{
+	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
+
+	/*
+	 * It's only possible to write SIE on the current hart.  This jumps
+	 * over to the target hart if it's not the current one.  It's invalid
+	 * to write SIE on a hart that's not currently running.
+	 */
+	if (data->hart == smp_processor_id())
+		riscv_irq_unmask(d);
+	else if (cpu_online(data->hart))
+		riscv_remote_ctrl(data->hart, riscv_irq_enable_helper, d);
+	else
+		WARN_ON_ONCE(1);
+}
+
+static void riscv_irq_disable(struct irq_data *d)
+{
+	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
+
+	/*
+	 * It's only possible to write SIE on the current hart.  This jumps
+	 * over to the target hart if it's not the current one.  It's invalid
+	 * to write SIE on a hart that's not currently running.
+	 */
+	if (data->hart == smp_processor_id())
+		riscv_irq_mask(d);
+	else if (cpu_online(data->hart))
+		riscv_remote_ctrl(data->hart, riscv_irq_disable_helper, d);
+	else
+		WARN_ON_ONCE(1);
+}
+
+static int __init riscv_intc_init(struct device_node *node, struct device_node *parent)
+{
+	int hart;
+	struct riscv_irq_data *data;
+
+	if (parent)
+		return 0;
+
+	hart = riscv_of_processor_hart(node->parent);
+	if (hart < 0)
+		return -EIO;
+
+	data = &per_cpu(riscv_irq_data, hart);
+	snprintf(data->name, sizeof(data->name), "riscv,cpu_intc,%d", hart);
+	data->hart = hart;
+	data->chip.name = data->name;
+	data->chip.irq_mask = riscv_irq_mask;
+	data->chip.irq_unmask = riscv_irq_unmask;
+	data->chip.irq_enable = riscv_irq_enable;
+	data->chip.irq_disable = riscv_irq_disable;
+	data->domain = irq_domain_add_linear(node, PTR_BITS,
+					     &riscv_irqdomain_ops, data);
+	if (!data->domain)
+		goto error_add_linear;
+
+	set_handle_irq(&riscv_intc_irq);
+
+	pr_info("%s: %lu local interrupts mapped\n", data->name, PTR_BITS);
+	return 0;
+
+error_add_linear:
+	pr_warning("%s: unable to add IRQ domain\n",
+		   data->name);
+	return -ENXIO;
+}
+
+IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
-- 
2.16.4

Re: [PATCH 3/3] irqchip: RISC-V Local Interrupt Controller Driver

[Randy Dunlap]

On 06/22/2018 04:20 PM, Palmer Dabbelt wrote:
> From: Palmer Dabbelt <palmer@dabbelt.com>
> 
> This patch adds a driver that manages the local interrupts on each
> RISC-V hart, as specifiec by the RISC-V supervisor level ISA manual.
> The local interrupt controller manages software interrupts, timer
> interrupts, and hardware interrupts (which are routed via the
> platform level interrupt controller).  Per-hart local interrupt
> controllers are found on all RISC-V systems.
> 
> CC: Michael Clark <mjc@sifive.com>
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> ---
>  drivers/irqchip/Kconfig          |  13 +++
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-riscv-intc.c | 215 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 229 insertions(+)
>  create mode 100644 drivers/irqchip/irq-riscv-intc.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index e9233db16e03..bf7fc86673b1 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_INTC
> +	#bool "RISC-V Interrupt Controller"

Hi,
What does the leading '#' do?


> +	depends on RISCV
> +	default y
> +	help
> +	   This enables support for the local interrupt controller found in
> +	   standard RISC-V systems.  The local interrupt controller handles
> +	   timer interrupts, software interrupts, and hardware interrupts.
> +	   Without a local interrupt controller the system will be unable to
> +	   handle any interrupts, including those passed via 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..74e333cc274c 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_INTC)		+= irq-riscv-intc.o


-- 
~Randy

Re: [PATCH 3/3] irqchip: RISC-V Local Interrupt Controller Driver

[Palmer Dabbelt]

On Fri, 22 Jun 2018 17:08:58 PDT (-0700), rdunlap@infradead.org wrote:
> On 06/22/2018 04:20 PM, Palmer Dabbelt wrote:
>> From: Palmer Dabbelt <palmer@dabbelt.com>
>>
>> This patch adds a driver that manages the local interrupts on each
>> RISC-V hart, as specifiec by the RISC-V supervisor level ISA manual.
>> The local interrupt controller manages software interrupts, timer
>> interrupts, and hardware interrupts (which are routed via the
>> platform level interrupt controller).  Per-hart local interrupt
>> controllers are found on all RISC-V systems.
>>
>> CC: Michael Clark <mjc@sifive.com>
>> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>> ---
>>  drivers/irqchip/Kconfig          |  13 +++
>>  drivers/irqchip/Makefile         |   1 +
>>  drivers/irqchip/irq-riscv-intc.c | 215 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 229 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-riscv-intc.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index e9233db16e03..bf7fc86673b1 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_INTC
>> +	#bool "RISC-V Interrupt Controller"
>
> Hi,
> What does the leading '#' do?

It's just a comment.  I'd seen this idiom used before to say "here's what this 
option would say if it was optional, which it may be at some point in the 
future, but for now it's just always enabled."

>> +	depends on RISCV
>> +	default y
>> +	help
>> +	   This enables support for the local interrupt controller found in
>> +	   standard RISC-V systems.  The local interrupt controller handles
>> +	   timer interrupts, software interrupts, and hardware interrupts.
>> +	   Without a local interrupt controller the system will be unable to
>> +	   handle any interrupts, including those passed via 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..74e333cc274c 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_INTC)		+= irq-riscv-intc.o

Re: [PATCH 3/3] irqchip: RISC-V Local Interrupt Controller Driver

[Christoph Hellwig]

> +config RISCV_INTC
> +	#bool "RISC-V Interrupt Controller"
> +	depends on RISCV
> +	default y
> +	help
> +	   This enables support for the local interrupt controller found in
> +	   standard RISC-V systems.  The local interrupt controller handles
> +	   timer interrupts, software interrupts, and hardware interrupts.
> +	   Without a local interrupt controller the system will be unable to
> +	   handle any interrupts, including those passed via the PLIC.

This can just be:

config RISCV_INTC
	def_bool y if RISCV
	depends on RISCV

as this isn't a user selectable option.

> index 000000000000..7ca3060e76b4
> --- /dev/null
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -0,0 +1,215 @@
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2017-2018 SiFive
> + */
> +/* SPDX-License-Identifier: GPL-2.0 */

SPDX tags need to be the first thing in the file, and use // -style
comments.

FYI, I've got a version with this and a few other cleanups here:

http://git.infradead.org/users/hch/riscv.git/commitdiff/d0789843f663ba8a6573ac5f73d8b6f999c8e6ed

Re: [PATCH 3/3] irqchip: RISC-V Local Interrupt Controller Driver

[Thomas Gleixner]

On Fri, 22 Jun 2018, Palmer Dabbelt wrote:
> +struct riscv_irq_data {
> +	struct irq_chip		chip;
> +	struct irq_domain	*domain;
> +	int			hart;
> +	char			name[20];
> +};
> +DEFINE_PER_CPU(struct riscv_irq_data, riscv_irq_data);

static ?

> +static void riscv_intc_irq(struct pt_regs *regs)
> +{
> +	struct pt_regs *old_regs = set_irq_regs(regs);
> +	struct irq_domain *domain;
> +	unsigned long cause = csr_read(scause);
> +
> +	/*
> +	 * 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 here.
> +	 */
> +	WARN_ON((cause & (1UL << (PTR_BITS - 1))) == 0);

So what's the point of continuing here? 


> +	cause = cause & ~(1UL << (PTR_BITS - 1));

Please define a proper CAUSE_MASK or such as this is really hard to read.

> +/*
> + * On RISC-V systems local interrupts are masked or unmasked by writing the SIE
> + * (Supervisor Interrupt Enable) CSR.  As CSRs can only be written on the local
> + * hart, these functions can only be called on the hart that corresponds to the
> + * IRQ chip.  They are only called internally to this module, so they BUG_ON if
> + * this condition is violated rather than attempting to handle the error by
> + * forwarding to the target hart, as that's already expected to have been done.
> + */
> +static void riscv_irq_mask(struct irq_data *d)
> +{
> +	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
> +
> +	BUG_ON(smp_processor_id() != data->hart);
> +	csr_clear(sie, 1 << (long)d->hwirq);

What's the point of this type cast? Whether you shift by unsigned long or by
long does not really matter, right? I'd rather expected to see 1U << d->hwirq

> +static int __init riscv_intc_init(struct device_node *node, struct device_node *parent)
> +{
> +	int hart;
> +	struct riscv_irq_data *data;

Nit. Reverse fir tree ordering please

	struct riscv_irq_data *data;
	int hart;

Simpler to parse.

> +
> +	if (parent)
> +		return 0;

Hmm, that wants a comment because it's not clear why this is done for the
casual reader.

> +
> +	hart = riscv_of_processor_hart(node->parent);
> +	if (hart < 0)
> +		return -EIO;
> +
> +	data = &per_cpu(riscv_irq_data, hart);
> +	snprintf(data->name, sizeof(data->name), "riscv,cpu_intc,%d", hart);
> +	data->hart = hart;
> +	data->chip.name = data->name;
> +	data->chip.irq_mask = riscv_irq_mask;
> +	data->chip.irq_unmask = riscv_irq_unmask;
> +	data->chip.irq_enable = riscv_irq_enable;
> +	data->chip.irq_disable = riscv_irq_disable;
> +	data->domain = irq_domain_add_linear(node, PTR_BITS,
> +					     &riscv_irqdomain_ops, data);
> +	if (!data->domain)
> +		goto error_add_linear;
> +
> +	set_handle_irq(&riscv_intc_irq);
> +
> +	pr_info("%s: %lu local interrupts mapped\n", data->name, PTR_BITS);
> +	return 0;
> +
> +error_add_linear:
> +	pr_warning("%s: unable to add IRQ domain\n",
> +		   data->name);

One line please.

> +	return -ENXIO;

Thanks,

	tglx