RISC-V Interrupt Controllers

RISC-V Interrupt Controllers

[Palmer Dabbelt]

This patch set contains two interrupt controller drivers for RISC-V systems:
the local interrupt controller is a per-hart controller, and the PLIC is a
per-chip controller.

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

[Palmer Dabbelt]

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.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 drivers/irqchip/Kconfig          |  14 +++
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-riscv-intc.c | 239 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 254 insertions(+)
 create mode 100644 drivers/irqchip/irq-riscv-intc.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 478f8ace2664..c25a75325acb 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -301,3 +301,17 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config RISCV_INTC
+	def_bool y if RISCV
+	#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 b64c59b838a0..150aacad5e25 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.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..8150a035aada
--- /dev/null
+++ b/drivers/irqchip/irq-riscv-intc.c
@@ -0,0 +1,239 @@
+/*
+ * 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.
+ */
+
+#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>
+
+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);
+DEFINE_PER_CPU(atomic_long_t, riscv_early_sie);
+
+static void riscv_software_interrupt(void)
+{
+#ifdef CONFIG_SMP
+	irqreturn_t ret;
+
+	ret = handle_ipi();
+	if (ret != IRQ_NONE)
+		return;
+#endif
+
+	BUG();
+}
+
+asmlinkage void __irq_entry do_IRQ(unsigned int cause, struct pt_regs *regs)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+	struct irq_domain *domain;
+
+	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);
+
+	return 0;
+}
+
+static const struct irq_domain_ops riscv_irqdomain_ops = {
+	.map	= riscv_irqdomain_map,
+	.xlate	= irq_domain_xlate_onecell,
+};
+
+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);
+}
+
+static void riscv_irq_enable_helper(void *d)
+{
+	riscv_irq_unmask(d);
+}
+
+static void riscv_irq_enable(struct irq_data *d)
+{
+	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
+
+	/* There are two phases to setting up an interrupt: first we set a bit
+	 * in this bookkeeping structure, which is used by trap_init to
+	 * initialize SIE for each hart as it comes up.
+	 */
+	atomic_long_or((1 << (long)d->hwirq),
+		       &per_cpu(riscv_early_sie, data->hart));
+
+	/* The CPU is usually online, so here we just attempt to enable the
+	 * interrupt by writing SIE directly.  We need to write SIE on the
+	 * correct hart, which might be another hart.
+	 */
+	if (data->hart == smp_processor_id())
+		riscv_irq_unmask(d);
+	else if (cpu_online(data->hart))
+		smp_call_function_single(data->hart,
+					 riscv_irq_enable_helper,
+					 d,
+					 true);
+}
+
+static void riscv_irq_disable_helper(void *d)
+{
+	riscv_irq_mask(d);
+}
+
+static void riscv_irq_disable(struct irq_data *d)
+{
+	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
+
+	/* This is the mirror of riscv_irq_enable. */
+	atomic_long_and(~(1 << (long)d->hwirq),
+			&per_cpu(riscv_early_sie, data->hart));
+	if (data->hart == smp_processor_id())
+		riscv_irq_mask(d);
+	else if (cpu_online(data->hart))
+		smp_call_function_single(data->hart,
+					 riscv_irq_disable_helper,
+					 d,
+					 true);
+}
+
+static void riscv_irq_mask_noop(struct irq_data *d) { }
+
+static void riscv_irq_unmask_noop(struct irq_data *d) { }
+
+static void riscv_irq_enable_noop(struct irq_data *d)
+{
+	struct device_node *data = irq_data_get_irq_chip_data(d);
+	u32 hart;
+
+	if (!of_property_read_u32(data, "reg", &hart))
+		printk(
+		  KERN_WARNING "enabled interrupt %d for missing hart %d (this interrupt has no handler)\n",
+		  (int)d->hwirq, hart);
+}
+
+static struct irq_chip riscv_noop_chip = {
+	.name = "riscv,cpu-intc,noop",
+	.irq_mask = riscv_irq_mask_noop,
+	.irq_unmask = riscv_irq_unmask_noop,
+	.irq_enable = riscv_irq_enable_noop,
+};
+
+static int riscv_irqdomain_map_noop(struct irq_domain *d, unsigned int irq,
+				    irq_hw_number_t hwirq)
+{
+	struct device_node *data = d->host_data;
+
+	irq_set_chip_and_handler(irq, &riscv_noop_chip, handle_simple_irq);
+	irq_set_chip_data(irq, data);
+	return 0;
+}
+
+static const struct irq_domain_ops riscv_irqdomain_ops_noop = {
+	.map    = riscv_irqdomain_map_noop,
+	.xlate  = irq_domain_xlate_onecell,
+};
+
+static int 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) {
+		/* If a hart is disabled, create a no-op irq domain.  Devices
+		 * may still have interrupts connected to those harts.  This is
+		 * not wrong... unless they actually load a driver that needs
+		 * it!
+		 */
+		irq_domain_add_linear(
+			node,
+			8*sizeof(uintptr_t),
+			&riscv_irqdomain_ops_noop,
+			node->parent);
+		return 0;
+	}
+
+	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,
+		8*sizeof(uintptr_t),
+		&riscv_irqdomain_ops,
+		data);
+	WARN_ON(!data->domain);
+	printk(KERN_INFO "%s: %d local interrupts mapped\n",
+	       data->name, 8*(int)sizeof(uintptr_t));
+	return 0;
+}
+
+IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
-- 
2.13.0

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

[Thomas Gleixner]

On Mon, 26 Jun 2017, Palmer Dabbelt wrote:
> +DEFINE_PER_CPU(struct riscv_irq_data, riscv_irq_data);
> +DEFINE_PER_CPU(atomic_long_t, riscv_early_sie);
> +
> +static void riscv_software_interrupt(void)
> +{
> +#ifdef CONFIG_SMP
> +	irqreturn_t ret;
> +
> +	ret = handle_ipi();
> +	if (ret != IRQ_NONE)
> +		return;
> +#endif
> +
> +	BUG();

Are you sure you want to crash the system just because a spurious interrupt
happened?

> +}
> +
> +asmlinkage void __irq_entry do_IRQ(unsigned int cause, struct pt_regs *regs)
> +{
> +	struct pt_regs *old_regs = set_irq_regs(regs);
> +	struct irq_domain *domain;
> +
> +	irq_enter();
> +
> +	/* There are three classes of interrupt: timer, software, and

Please use proper multiline comment formatting:

        /*
	 * bla.....
	 * foo.....
	 */

> +	 * 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);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops riscv_irqdomain_ops = {
> +	.map	= riscv_irqdomain_map,
> +	.xlate	= irq_domain_xlate_onecell,
> +};
> +
> +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);

Crashing the machine is the last resort if there is no chance to handle a
situation gracefully. Something like

	if (WARN_ON_ONCE(smp_processor_id() != data->hart))
		do_something_sensible();
	else
		.....

might at least keep the machine halfways functional for debugging.

> +	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);
> +}
> +
> +static void riscv_irq_enable_helper(void *d)
> +{
> +	riscv_irq_unmask(d);
> +}
> +
> +static void riscv_irq_enable(struct irq_data *d)
> +{
> +	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
> +
> +	/* There are two phases to setting up an interrupt: first we set a bit
> +	 * in this bookkeeping structure, which is used by trap_init to
> +	 * initialize SIE for each hart as it comes up.

And what exactly has this to do with irq_enable()? Why would you call that
for an interrupt which solely goes to a offline cpu?

> +static void riscv_irq_disable(struct irq_data *d)
> +{
> +	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
> +
> +	/* This is the mirror of riscv_irq_enable. */
> +	atomic_long_and(~(1 << (long)d->hwirq),
> +			&per_cpu(riscv_early_sie, data->hart));
> +	if (data->hart == smp_processor_id())
> +		riscv_irq_mask(d);
> +	else if (cpu_online(data->hart))
> +		smp_call_function_single(data->hart,
> +					 riscv_irq_disable_helper,
> +					 d,
> +					 true);

Same question as above.

> +}
> +
> +static void riscv_irq_mask_noop(struct irq_data *d) { }
> +
> +static void riscv_irq_unmask_noop(struct irq_data *d) { }
> 
> +static void riscv_irq_enable_noop(struct irq_data *d)
> +{
> +	struct device_node *data = irq_data_get_irq_chip_data(d);
> +	u32 hart;
> +
> +	if (!of_property_read_u32(data, "reg", &hart))
> +		printk(
> +		  KERN_WARNING "enabled interrupt %d for missing hart %d (this interrupt has no handler)\n",

Has no handler? I really have a hard time to understand the logic here.

> +		  (int)d->hwirq, hart);
> +}
> +
> +static struct irq_chip riscv_noop_chip = {
> +	.name = "riscv,cpu-intc,noop",
> +	.irq_mask = riscv_irq_mask_noop,
> +	.irq_unmask = riscv_irq_unmask_noop,
> +	.irq_enable = riscv_irq_enable_noop,

Please write that in tabular fashion:

	.name		= "riscv,cpu-intc,noop",
	.irq_mask	= riscv_irq_mask_noop,

> +};
> +
> +static int riscv_irqdomain_map_noop(struct irq_domain *d, unsigned int irq,
> +				    irq_hw_number_t hwirq)
> +{
> +	struct device_node *data = d->host_data;
> +
> +	irq_set_chip_and_handler(irq, &riscv_noop_chip, handle_simple_irq);
> +	irq_set_chip_data(irq, data);
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops riscv_irqdomain_ops_noop = {
> +	.map    = riscv_irqdomain_map_noop,
> +	.xlate  = irq_domain_xlate_onecell,
> +};
> +
> +static int 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) {
> +		/* If a hart is disabled, create a no-op irq domain.  Devices
> +		 * may still have interrupts connected to those harts.  This is
> +		 * not wrong... unless they actually load a driver that needs
> +		 * it!
> +		 */
> +		irq_domain_add_linear(
> +			node,
> +			8*sizeof(uintptr_t),
> +			&riscv_irqdomain_ops_noop,
> +			node->parent);
> +		return 0;

I have a hard time to understand the logic here. Why do you need an
interrupt domain for something which does not exist? That does not make any
sense.

> +	}
> +
> +	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,
> +		8*sizeof(uintptr_t),
> +		&riscv_irqdomain_ops,
> +		data);
> +	WARN_ON(!data->domain);

That warn_on is great. You warn and then continue as if nothing
happened. Machine will crash later dereferencing a NULL pointer.

Some more epxlanations about the inner workings of all of this would be
appreciated.

Thanks,

	tglx

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

[Palmer Dabbelt]

On Wed, 28 Jun 2017 13:47:40 PDT (-0700), tglx@linutronix.de wrote:
> On Mon, 26 Jun 2017, Palmer Dabbelt wrote:
>> +DEFINE_PER_CPU(struct riscv_irq_data, riscv_irq_data);
>> +DEFINE_PER_CPU(atomic_long_t, riscv_early_sie);
>> +
>> +static void riscv_software_interrupt(void)
>> +{
>> +#ifdef CONFIG_SMP
>> +	irqreturn_t ret;
>> +
>> +	ret = handle_ipi();
>> +	if (ret != IRQ_NONE)
>> +		return;
>> +#endif
>> +
>> +	BUG();
>
> Are you sure you want to crash the system just because a spurious interrupt
> happened?

In this case the software interrupt is to handle IPIs, so it doesn't really
make any sense to handle one without SMP.  I'm OK with just warning, though, as
the IPIs are just for TLB shootdowns so skipping one on a non-SMP system seems
safe.

How does this look?

>> +}
>> +
>> +asmlinkage void __irq_entry do_IRQ(unsigned int cause, struct pt_regs *regs)
>> +{
>> +	struct pt_regs *old_regs = set_irq_regs(regs);
>> +	struct irq_domain *domain;
>> +
>> +	irq_enter();
>> +
>> +	/* There are three classes of interrupt: timer, software, and
>
> Please use proper multiline comment formatting:

Sorry about that.  I went and fixed all the comments in here, I'll do it in the
PLIC driver (patch 2) as well.

>         /*
> 	 * bla.....
> 	 * foo.....
> 	 */
>
>> +	 * 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);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct irq_domain_ops riscv_irqdomain_ops = {
>> +	.map	= riscv_irqdomain_map,
>> +	.xlate	= irq_domain_xlate_onecell,
>> +};
>> +
>> +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);
>
> Crashing the machine is the last resort if there is no chance to handle a
> situation gracefully. Something like
>
> 	if (WARN_ON_ONCE(smp_processor_id() != data->hart))
> 		do_something_sensible();
> 	else
> 		.....
>
> might at least keep the machine halfways functional for debugging.

In this case I think BUG_ON is the only sane thing to do.  I've gone and added
a comment that explains what's going on here

  diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
  index 7e55fe57e95f..3dd421ade128 100644
  --- a/drivers/irqchip/irq-riscv-intc.c
  +++ b/drivers/irqchip/irq-riscv-intc.c
  @@ -97,6 +97,14 @@ static const struct irq_domain_ops riscv_irqdomain_ops = {
          .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 cooresponds 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);

I think there's three options here:

 * BUG_ON like we do.
 * Attempt to jump to the correct hart to set the CSR, but since we just did
   that I don't really see why it would work the second time.
 * Provide a warning and then ignore {un,}masking the IRQ, but that seems
   dangerous.

I can change it to a warning if you think it's better.

>> +	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);
>> +}
>> +
>> +static void riscv_irq_enable_helper(void *d)
>> +{
>> +	riscv_irq_unmask(d);
>> +}
>> +
>> +static void riscv_irq_enable(struct irq_data *d)
>> +{
>> +	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
>> +
>> +	/* There are two phases to setting up an interrupt: first we set a bit
>> +	 * in this bookkeeping structure, which is used by trap_init to
>> +	 * initialize SIE for each hart as it comes up.
>
> And what exactly has this to do with irq_enable()? Why would you call that
> for an interrupt which solely goes to a offline cpu?
>
>> +static void riscv_irq_disable(struct irq_data *d)
>> +{
>> +	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
>> +
>> +	/* This is the mirror of riscv_irq_enable. */
>> +	atomic_long_and(~(1 << (long)d->hwirq),
>> +			&per_cpu(riscv_early_sie, data->hart));
>> +	if (data->hart == smp_processor_id())
>> +		riscv_irq_mask(d);
>> +	else if (cpu_online(data->hart))
>> +		smp_call_function_single(data->hart,
>> +					 riscv_irq_disable_helper,
>> +					 d,
>> +					 true);
>
> Same question as above.

I've attempted to improve the comment that describes why this is necessary.

  diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
  index 3dd421ade128..b2643f7131ff 100644
  --- a/drivers/irqchip/irq-riscv-intc.c
  +++ b/drivers/irqchip/irq-riscv-intc.c
  @@ -137,9 +137,13 @@ static void riscv_irq_enable(struct irq_data *d)
          struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);

          /*
  -        * There are two phases to setting up an interrupt: first we set a bit
  -        * in this bookkeeping structure, which is used by trap_init to
  -        * initialize SIE for each hart as it comes up.
  +        * When booting a RISC-V system, procesors can come online at any time.
  +        * Interrupts can only be enabled or disabled by writing a CSR on the
  +        * hart that cooresponds to that interrupt controller, but CSRs can
  +        * only be written locally.  In order to avoid waiting a long time for
  +        * a hart to boot, we instead collect the interrupts to be enabled upon
  +        * booting a hart in this bookkeeping structure, which is used by
  +        * trap_init to initialize SIE for each hart as it comes up.
           */
          atomic_long_or((1 << (long)d->hwirq),
                         &per_cpu(riscv_early_sie, data->hart));

I'm not sure how to get around this.

>> +}
>> +
>> +static void riscv_irq_mask_noop(struct irq_data *d) { }
>> +
>> +static void riscv_irq_unmask_noop(struct irq_data *d) { }
>>
>> +static void riscv_irq_enable_noop(struct irq_data *d)
>> +{
>> +	struct device_node *data = irq_data_get_irq_chip_data(d);
>> +	u32 hart;
>> +
>> +	if (!of_property_read_u32(data, "reg", &hart))
>> +		printk(
>> +		  KERN_WARNING "enabled interrupt %d for missing hart %d (this interrupt has no handler)\n",
>
> Has no handler? I really have a hard time to understand the logic here.
>
>> +		  (int)d->hwirq, hart);
>> +}
>> +
>> +static struct irq_chip riscv_noop_chip = {
>> +	.name = "riscv,cpu-intc,noop",
>> +	.irq_mask = riscv_irq_mask_noop,
>> +	.irq_unmask = riscv_irq_unmask_noop,
>> +	.irq_enable = riscv_irq_enable_noop,
>
> Please write that in tabular fashion:
>
> 	.name		= "riscv,cpu-intc,noop",
> 	.irq_mask	= riscv_irq_mask_noop,
>
>> +};
>> +
>> +static int riscv_irqdomain_map_noop(struct irq_domain *d, unsigned int irq,
>> +				    irq_hw_number_t hwirq)
>> +{
>> +	struct device_node *data = d->host_data;
>> +
>> +	irq_set_chip_and_handler(irq, &riscv_noop_chip, handle_simple_irq);
>> +	irq_set_chip_data(irq, data);
>> +	return 0;
>> +}
>> +
>> +static const struct irq_domain_ops riscv_irqdomain_ops_noop = {
>> +	.map    = riscv_irqdomain_map_noop,
>> +	.xlate  = irq_domain_xlate_onecell,
>> +};
>> +
>> +static int 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) {
>> +		/* If a hart is disabled, create a no-op irq domain.  Devices
>> +		 * may still have interrupts connected to those harts.  This is
>> +		 * not wrong... unless they actually load a driver that needs
>> +		 * it!
>> +		 */
>> +		irq_domain_add_linear(
>> +			node,
>> +			8*sizeof(uintptr_t),
>> +			&riscv_irqdomain_ops_noop,
>> +			node->parent);
>> +		return 0;
>
> I have a hard time to understand the logic here. Why do you need an
> interrupt domain for something which does not exist? That does not make any
> sense.

I think this is best explained as a comment, which I've added

  diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
  index a62a1f04198e..52db58a94bdc 100644
  --- a/drivers/irqchip/irq-riscv-intc.c
  +++ b/drivers/irqchip/irq-riscv-intc.c
  @@ -178,6 +178,32 @@ static void riscv_irq_disable(struct irq_data *d)
                                           true);
   }

  +/*
  + * This "no op" interrupt handler deals with harts that for some reason exist
  + * but can't actually run Linux.  Examples of these sorts of harts include:
  + * - Harts that don't report an "okay" status, presumably because of a hardware
  + *   fault.
  + * - Harts with an ID larger than NR_CPUS.
  + * - Harts with an ISA we don't understand.
  + *
  + * Interrupts targeted to these harts won't even actually be seen by Linux, as
  + * there's no code running to ever receive the interrupt.  If this is an
  + * invalid system configuration then there's nothing we can really do about it,
  + * but we expect these configurations to usually be valid.
  + *
  + * For example, we generally allow the PLIC to route interrupts to every hart
  + * in the system via the local interrupt controller on every hart.  When Linux
  + * is built without CONFIG_SMP this still allows for a working system, as the
  + * single enabled hart can handle every device interrupt in the system.  It is
  + * possible to build a PLIC that doesn't allow interrupts to be routed to the
  + * first hart to boot.  If for some reason the PLIC was instantiated in this
  + * manner then whatever devices could not be directed to the first hart to boot
  + * would be unusable on non-CONFIG_SMP kernels, but as we can't change the PLIC
  + * hardware there's nothing we can do about that.
  + *
  + * In order to avoid the complexity of having unmapped IRQ domains, we instead
  + * just install the noop IRQ handler in domains we can't handle.
  + */
   static void riscv_irq_mask_noop(struct irq_data *d) { }

   static void riscv_irq_unmask_noop(struct irq_data *d) { }
  @@ -189,7 +215,7 @@ static void riscv_irq_enable_noop(struct irq_data *d)

          if (!of_property_read_u32(data, "reg", &hart))
                  printk(
  -                 KERN_WARNING "enabled interrupt %d for missing hart %d (this interrupt has no handler)\n",
  +                 KERN_WARNING "enabled no-op handler for interrupt %d on missing hart %d\n",
                    (int)d->hwirq, hart);
   }

  @@ -229,7 +255,7 @@ static int riscv_intc_init(struct device_node *node, struct device_node *parent)
                   * If a hart is disabled, create a no-op irq domain.  Devices
                   * may still have interrupts connected to those harts.  This is
                   * not wrong... unless they actually load a driver that needs
  -                * it!
  +                * it!  See the comment above for more details.
                   */
                  irq_domain_add_linear(
                          node,

Sorry if that's an odd way to do things -- if there's a better way to do this
then I'm all ears!

>> +	}
>> +
>> +	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,
>> +		8*sizeof(uintptr_t),
>> +		&riscv_irqdomain_ops,
>> +		data);
>> +	WARN_ON(!data->domain);
>
> That warn_on is great. You warn and then continue as if nothing
> happened. Machine will crash later dereferencing a NULL pointer.

OK, makes sense.  I checked another driver and they're returning ENXIO in this
case, so I did that.

  diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
  index 52db58a94bdc..11d8084a0ab1 100644
  --- a/drivers/irqchip/irq-riscv-intc.c
  +++ b/drivers/irqchip/irq-riscv-intc.c
  @@ -245,6 +245,7 @@ static int riscv_intc_init(struct device_node *node, struct device_node *parent)
   {
          int hart;
          struct riscv_irq_data *data;
  +       struct irq_domain *domain;

          if (parent)
                  return 0;
  @@ -257,11 +258,13 @@ static int riscv_intc_init(struct device_node *node, struct device_node *parent)
                   * not wrong... unless they actually load a driver that needs
                   * it!  See the comment above for more details.
                   */
  -               irq_domain_add_linear(
  +               domain = irq_domain_add_linear(
                          node,
                          8*sizeof(uintptr_t),
                          &riscv_irqdomain_ops_noop,
                          node->parent);
  +               if (!domain)
  +                       goto error_add_linear;
                  return 0;
          }

  @@ -278,10 +281,17 @@ static int riscv_intc_init(struct device_node *node, struct device_node *parent)
                  8*sizeof(uintptr_t),
                  &riscv_irqdomain_ops,
                  data);
  -       WARN_ON(!data->domain);
  +       if (!data->domain)
  +               goto error_add_linear;
          printk(KERN_INFO "%s: %d local interrupts mapped\n",
                 data->name, 8*(int)sizeof(uintptr_t));
          return 0;
  +
  +error_add_linear:
  +       printk(KERN_WARNING "%s: unable to add IRQ domain\n",
  +              data->name);
  +       return -(ENXIO);
  +
   }

   IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);

> Some more epxlanations about the inner workings of all of this would be
> appreciated.

Hopefully the comments I added help here.

Thanks!

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

[Thomas Gleixner]

On Thu, 29 Jun 2017, Palmer Dabbelt wrote:
> On Wed, 28 Jun 2017 13:47:40 PDT (-0700), tglx@linutronix.de wrote:
> In this case the software interrupt is to handle IPIs, so it doesn't really
> make any sense to handle one without SMP.  I'm OK with just warning, though, as
> the IPIs are just for TLB shootdowns so skipping one on a non-SMP system seems
> safe.

Indeed.

> >> +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);
> >
> > Crashing the machine is the last resort if there is no chance to handle a
> > situation gracefully. Something like
> >
> > 	if (WARN_ON_ONCE(smp_processor_id() != data->hart))
> > 		do_something_sensible();
> > 	else
> > 		.....
> >
> > might at least keep the machine halfways functional for debugging.
> 
> In this case I think BUG_ON is the only sane thing to do.  I've gone and added
> a comment that explains what's going on here
> 
>   diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
>   index 7e55fe57e95f..3dd421ade128 100644
>   --- a/drivers/irqchip/irq-riscv-intc.c
>   +++ b/drivers/irqchip/irq-riscv-intc.c
>   @@ -97,6 +97,14 @@ static const struct irq_domain_ops riscv_irqdomain_ops = {
>           .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 cooresponds 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);
> 
> I think there's three options here:
> 
>  * BUG_ON like we do.
>  * Attempt to jump to the correct hart to set the CSR, but since we just did
>    that I don't really see why it would work the second time.
>  * Provide a warning and then ignore {un,}masking the IRQ, but that seems
>    dangerous.
> 
> I can change it to a warning if you think it's better.

With a coherent explanation why a BUG_ON() is the right thing to do, the
BUG_ON can stay.

>   diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
>   index 3dd421ade128..b2643f7131ff 100644
>   --- a/drivers/irqchip/irq-riscv-intc.c
>   +++ b/drivers/irqchip/irq-riscv-intc.c
>   @@ -137,9 +137,13 @@ static void riscv_irq_enable(struct irq_data *d)
>           struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
> 
>           /*
>   -        * There are two phases to setting up an interrupt: first we set a bit
>   -        * in this bookkeeping structure, which is used by trap_init to
>   -        * initialize SIE for each hart as it comes up.
>   +        * When booting a RISC-V system, procesors can come online at any time.

Not so much at any time. The kernel orchestrates that.

>   +        * Interrupts can only be enabled or disabled by writing a CSR on the
>   +        * hart that cooresponds to that interrupt controller, but CSRs can
>   +        * only be written locally.  In order to avoid waiting a long time for
>   +        * a hart to boot, we instead collect the interrupts to be enabled upon
>   +        * booting a hart in this bookkeeping structure, which is used by
>   +        * trap_init to initialize SIE for each hart as it comes up.
>            */
>           atomic_long_or((1 << (long)d->hwirq),
>                          &per_cpu(riscv_early_sie, data->hart));

That still does not answer the question WHY an interrupt for a not
available CPU would be enabled in the first place.
 
> >> +static int 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) {
> >> +		/* If a hart is disabled, create a no-op irq domain.  Devices
> >> +		 * may still have interrupts connected to those harts.  This is
> >> +		 * not wrong... unless they actually load a driver that needs
> >> +		 * it!
> >> +		 */
> >> +		irq_domain_add_linear(
> >> +			node,
> >> +			8*sizeof(uintptr_t),
> >> +			&riscv_irqdomain_ops_noop,
> >> +			node->parent);
> >> +		return 0;
> >
> > I have a hard time to understand the logic here. Why do you need an
> > interrupt domain for something which does not exist? That does not make any
> > sense.
> 
> I think this is best explained as a comment, which I've added
> 
>   diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
>   index a62a1f04198e..52db58a94bdc 100644
>   --- a/drivers/irqchip/irq-riscv-intc.c
>   +++ b/drivers/irqchip/irq-riscv-intc.c
>   @@ -178,6 +178,32 @@ static void riscv_irq_disable(struct irq_data *d)
>                                            true);
>    }
> 
>   +/*
>   + * This "no op" interrupt handler deals with harts that for some reason exist
>   + * but can't actually run Linux.  Examples of these sorts of harts include:
>   + * - Harts that don't report an "okay" status, presumably because of a hardware
>   + *   fault.
>   + * - Harts with an ID larger than NR_CPUS.
>   + * - Harts with an ISA we don't understand.
>   + *
>   + * Interrupts targeted to these harts won't even actually be seen by Linux, as
>   + * there's no code running to ever receive the interrupt.  If this is an
>   + * invalid system configuration then there's nothing we can really do about it,
>   + * but we expect these configurations to usually be valid.

But what would enable such interrupts in the first place? The device whose
interrupt is routed into nirwana is going to be non-functional. See below.

>   + * For example, we generally allow the PLIC to route interrupts to every hart
>   + * in the system via the local interrupt controller on every hart.

That's fine, but you really want to control that via the interrupt affinity
mechanism and not by blindly routing every interrupt to every possible CPU
in the system. That mechanism is there for a reason.

>   + * When Linux
>   + * is built without CONFIG_SMP this still allows for a working system, as the
>   + * single enabled hart can handle every device interrupt in the system.

Well, that's how it should be. But that still does not require to handle
anything which is not usable.

>   + * It is
>   + * possible to build a PLIC that doesn't allow interrupts to be routed to the
>   + * first hart to boot.  If for some reason the PLIC was instantiated in this
>   + * manner then whatever devices could not be directed to the first hart to boot
>   + * would be unusable on non-CONFIG_SMP kernels, but as we can't change the PLIC
>   + * hardware there's nothing we can do about that.
>   + *
>   + * In order to avoid the complexity of having unmapped IRQ domains, we instead
>   + * just install the noop IRQ handler in domains we can't handle.

What's complex about that? If a device driver tries to request an unmapped
irq then the request will fail and the driver can act accordingly.

Silently pretending success and then letting the user bang his head against
the wall is definitely the worst thing you can do.

Thanks,

	tglx

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

[Palmer Dabbelt]

On Mon, 03 Jul 2017 04:13:34 PDT (-0700), tglx@linutronix.de wrote:
> On Thu, 29 Jun 2017, Palmer Dabbelt wrote:
>> On Wed, 28 Jun 2017 13:47:40 PDT (-0700), tglx@linutronix.de wrote:
>> In this case the software interrupt is to handle IPIs, so it doesn't really
>> make any sense to handle one without SMP.  I'm OK with just warning, though, as
>> the IPIs are just for TLB shootdowns so skipping one on a non-SMP system seems
>> safe.
>
> Indeed.
>
>> >> +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);
>> >
>> > Crashing the machine is the last resort if there is no chance to handle a
>> > situation gracefully. Something like
>> >
>> > 	if (WARN_ON_ONCE(smp_processor_id() != data->hart))
>> > 		do_something_sensible();
>> > 	else
>> > 		.....
>> >
>> > might at least keep the machine halfways functional for debugging.
>>
>> In this case I think BUG_ON is the only sane thing to do.  I've gone and added
>> a comment that explains what's going on here
>>
>>   diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
>>   index 7e55fe57e95f..3dd421ade128 100644
>>   --- a/drivers/irqchip/irq-riscv-intc.c
>>   +++ b/drivers/irqchip/irq-riscv-intc.c
>>   @@ -97,6 +97,14 @@ static const struct irq_domain_ops riscv_irqdomain_ops = {
>>           .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 cooresponds 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);
>>
>> I think there's three options here:
>>
>>  * BUG_ON like we do.
>>  * Attempt to jump to the correct hart to set the CSR, but since we just did
>>    that I don't really see why it would work the second time.
>>  * Provide a warning and then ignore {un,}masking the IRQ, but that seems
>>    dangerous.
>>
>> I can change it to a warning if you think it's better.
>
> With a coherent explanation why a BUG_ON() is the right thing to do, the
> BUG_ON can stay.
>
>>   diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
>>   index 3dd421ade128..b2643f7131ff 100644
>>   --- a/drivers/irqchip/irq-riscv-intc.c
>>   +++ b/drivers/irqchip/irq-riscv-intc.c
>>   @@ -137,9 +137,13 @@ static void riscv_irq_enable(struct irq_data *d)
>>           struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
>>
>>           /*
>>   -        * There are two phases to setting up an interrupt: first we set a bit
>>   -        * in this bookkeeping structure, which is used by trap_init to
>>   -        * initialize SIE for each hart as it comes up.
>>   +        * When booting a RISC-V system, procesors can come online at any time.
>
> Not so much at any time. The kernel orchestrates that.
>
>>   +        * Interrupts can only be enabled or disabled by writing a CSR on the
>>   +        * hart that cooresponds to that interrupt controller, but CSRs can
>>   +        * only be written locally.  In order to avoid waiting a long time for
>>   +        * a hart to boot, we instead collect the interrupts to be enabled upon
>>   +        * booting a hart in this bookkeeping structure, which is used by
>>   +        * trap_init to initialize SIE for each hart as it comes up.
>>            */
>>           atomic_long_or((1 << (long)d->hwirq),
>>                          &per_cpu(riscv_early_sie, data->hart));
>
> That still does not answer the question WHY an interrupt for a not
> available CPU would be enabled in the first place.
>
>> >> +static int 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) {
>> >> +		/* If a hart is disabled, create a no-op irq domain.  Devices
>> >> +		 * may still have interrupts connected to those harts.  This is
>> >> +		 * not wrong... unless they actually load a driver that needs
>> >> +		 * it!
>> >> +		 */
>> >> +		irq_domain_add_linear(
>> >> +			node,
>> >> +			8*sizeof(uintptr_t),
>> >> +			&riscv_irqdomain_ops_noop,
>> >> +			node->parent);
>> >> +		return 0;
>> >
>> > I have a hard time to understand the logic here. Why do you need an
>> > interrupt domain for something which does not exist? That does not make any
>> > sense.
>>
>> I think this is best explained as a comment, which I've added
>>
>>   diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
>>   index a62a1f04198e..52db58a94bdc 100644
>>   --- a/drivers/irqchip/irq-riscv-intc.c
>>   +++ b/drivers/irqchip/irq-riscv-intc.c
>>   @@ -178,6 +178,32 @@ static void riscv_irq_disable(struct irq_data *d)
>>                                            true);
>>    }
>>
>>   +/*
>>   + * This "no op" interrupt handler deals with harts that for some reason exist
>>   + * but can't actually run Linux.  Examples of these sorts of harts include:
>>   + * - Harts that don't report an "okay" status, presumably because of a hardware
>>   + *   fault.
>>   + * - Harts with an ID larger than NR_CPUS.
>>   + * - Harts with an ISA we don't understand.
>>   + *
>>   + * Interrupts targeted to these harts won't even actually be seen by Linux, as
>>   + * there's no code running to ever receive the interrupt.  If this is an
>>   + * invalid system configuration then there's nothing we can really do about it,
>>   + * but we expect these configurations to usually be valid.
>
> But what would enable such interrupts in the first place? The device whose
> interrupt is routed into nirwana is going to be non-functional. See below.
>
>>   + * For example, we generally allow the PLIC to route interrupts to every hart
>>   + * in the system via the local interrupt controller on every hart.
>
> That's fine, but you really want to control that via the interrupt affinity
> mechanism and not by blindly routing every interrupt to every possible CPU
> in the system. That mechanism is there for a reason.
>
>>   + * When Linux
>>   + * is built without CONFIG_SMP this still allows for a working system, as the
>>   + * single enabled hart can handle every device interrupt in the system.
>
> Well, that's how it should be. But that still does not require to handle
> anything which is not usable.
>
>>   + * It is
>>   + * possible to build a PLIC that doesn't allow interrupts to be routed to the
>>   + * first hart to boot.  If for some reason the PLIC was instantiated in this
>>   + * manner then whatever devices could not be directed to the first hart to boot
>>   + * would be unusable on non-CONFIG_SMP kernels, but as we can't change the PLIC
>>   + * hardware there's nothing we can do about that.
>>   + *
>>   + * In order to avoid the complexity of having unmapped IRQ domains, we instead
>>   + * just install the noop IRQ handler in domains we can't handle.
>
> What's complex about that? If a device driver tries to request an unmapped
> irq then the request will fail and the driver can act accordingly.
>
> Silently pretending success and then letting the user bang his head against
> the wall is definitely the worst thing you can do.

OK, we'll do it that way.  I'll submit another patch set soon.

Thanks!

[PATCH 2/2] irqchip: New RISC-V PLIC Driver

[Palmer Dabbelt]

This patch adds a driver for the Platform Level Interrupt Controller
(PLIC) specified as part of the RISC-V supervisor level ISA manual.
The PLIC connocts global interrupt sources to the local interrupt
controller on each hart.  A PLIC is present on all RISC-V systems.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
---
 drivers/irqchip/Kconfig          |  13 ++
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-riscv-plic.c | 363 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 377 insertions(+)
 create mode 100644 drivers/irqchip/irq-riscv-plic.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index c25a75325acb..8feb0e4856fb 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -302,6 +302,19 @@ config QCOM_IRQ_COMBINER
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
 
+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.
+
 config RISCV_INTC
 	def_bool y if RISCV
 	#bool "RISC-V Interrupt Controller"
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 150aacad5e25..bc9a6b45903b 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -76,4 +76,5 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_RISCV_PLIC)		+= irq-riscv-plic.o
 obj-$(CONFIG_RISCV_INTC)		+= irq-riscv-intc.o
diff --git a/drivers/irqchip/irq-riscv-plic.c b/drivers/irqchip/irq-riscv-plic.c
new file mode 100644
index 000000000000..9b1939f24556
--- /dev/null
+++ b/drivers/irqchip/irq-riscv-plic.c
@@ -0,0 +1,363 @@
+/*
+ * 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.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+/* From the RISC-V Privlidged 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-existant so this device really only supports 1023
+ * devices.
+ */
+#define MAX_DEVICES	1024
+#define MAX_CONTEXTS	15872
+
+/* The PLIC consists of memory-mapped control registers, with a memory map as
+ * follows:
+ *
+ * base + 0x000000: Reserved (interrupt source 0 does not exist)
+ * base + 0x000004: Interrupt source 1 priority
+ * base + 0x000008: Interrupt source 2 priority
+ * ...
+ * base + 0x000FFC: Interrupt source 1023 priority
+ * base + 0x001000: Pending 0
+ * base + 0x001FFF: Pending
+ * base + 0x002000: Enable bits for sources 0-31 on context 0
+ * base + 0x002004: Enable bits for sources 32-63 on context 0
+ * ...
+ * base + 0x0020FC: Enable bits for sources 992-1023 on context 0
+ * base + 0x002080: Enable bits for sources 0-31 on context 1
+ * ...
+ * base + 0x002100: Enable bits for sources 0-31 on context 2
+ * ...
+ * base + 0x1F1F80: Enable bits for sources 992-1023 on context 15871
+ * base + 0x1F1F84: Reserved
+ * ...              (higher context IDs would fit here, but wouldn't fit
+ *                   inside the per-context priority vector)
+ * base + 0x1FFFFC: Reserved
+ * base + 0x200000: Priority threshold for context 0
+ * base + 0x200004: Claim/complete for context 0
+ * base + 0x200008: Reserved
+ * ...
+ * base + 0x200FFC: Reserved
+ * base + 0x201000: Priority threshold for context 1
+ * base + 0x201004: Claim/complete for context 1
+ * ...
+ * base + 0xFFE000: Priority threshold for context 15871
+ * base + 0xFFE004: Claim/complete for context 15871
+ * base + 0xFFF008: Reserved
+ * ...
+ * base + 0xFFFFFC: Reserved
+ */
+
+/* Each interrupt source has a priority register associated with it. */
+#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	0
+#define CONTEXT_CLAIM		4
+
+/* PLIC devices are named like 'riscv,plic0,%llx', this is enough space to
+ * store that name.
+ */
+#define PLIC_DATA_NAME_SIZE 30
+
+struct plic_handler {
+	bool			present;
+	int			contextid;
+	struct plic_data	*data;
+};
+
+struct plic_data {
+	struct irq_chip		chip;
+	struct irq_domain	*domain;
+	u32			ndev;
+	void __iomem		*reg;
+	int			handlers;
+	struct plic_handler	*handler;
+	char			name[PLIC_DATA_NAME_SIZE];
+	spinlock_t		lock;
+};
+
+/* Addressing helper functions. */
+static inline
+void __iomem *plic_enable_vector(struct plic_data *data, int contextid)
+{
+	return data->reg + ENABLE_BASE + contextid * ENABLE_PER_HART;
+}
+
+static inline
+void __iomem *plic_priority(struct plic_data *data, int hwirq)
+{
+	return data->reg + PRIORITY_BASE + hwirq * PRIORITY_PER_ID;
+}
+
+static inline
+void __iomem *plic_hart_threshold(struct plic_data *data, int contextid)
+{
+	return data->reg + CONTEXT_BASE + CONTEXT_PER_HART * contextid + CONTEXT_THRESHOLD;
+}
+
+static inline
+void __iomem *plic_hart_claim(struct plic_data *data, int contextid)
+{
+	return data->reg + CONTEXT_BASE + CONTEXT_PER_HART * contextid + CONTEXT_CLAIM;
+}
+
+/* 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 inline
+u32 plic_claim(struct plic_data *data, int contextid)
+{
+	return readl(plic_hart_claim(data, contextid));
+}
+
+static inline
+void plic_complete(struct plic_data *data, int contextid, u32 claim)
+{
+	writel(claim, plic_hart_claim(data, contextid));
+}
+
+/* Explicit interrupt masking. */
+static void plic_disable(struct plic_data *data, int contextid, int hwirq)
+{
+	void __iomem *reg = plic_enable_vector(data, contextid) + (hwirq / 32);
+	u32 mask = ~(1 << (hwirq % 32));
+
+	spin_lock(&data->lock);
+	writel(readl(reg) & mask, reg);
+	spin_unlock(&data->lock);
+}
+
+static void plic_enable(struct plic_data *data, int contextid, int hwirq)
+{
+	void __iomem *reg = plic_enable_vector(data, contextid) + (hwirq / 32);
+	u32 bit = 1 << (hwirq % 32);
+
+	spin_lock(&data->lock);
+	writel(readl(reg) | bit, reg);
+	spin_unlock(&data->lock);
+}
+
+/* There is no need to mask/unmask PLIC interrupts
+ * They are "masked" by reading claim and "unmasked" when writing it back.
+ */
+static void plic_irq_mask(struct irq_data *d) { }
+static void plic_irq_unmask(struct irq_data *d) { }
+
+static void plic_irq_enable(struct irq_data *d)
+{
+	struct plic_data *data = irq_data_get_irq_chip_data(d);
+	void __iomem *priority = plic_priority(data, d->hwirq);
+	int i;
+
+	writel(1, priority);
+	for (i = 0; i < data->handlers; ++i)
+		if (data->handler[i].present)
+			plic_enable(data, i, d->hwirq);
+}
+
+static void plic_irq_disable(struct irq_data *d)
+{
+	struct plic_data *data = irq_data_get_irq_chip_data(d);
+	void __iomem *priority = plic_priority(data, d->hwirq);
+	int i;
+
+	writel(0, priority);
+	for (i = 0; i < data->handlers; ++i)
+		if (data->handler[i].present)
+			plic_disable(data, i, d->hwirq);
+}
+
+static void plic_irq_eoi(struct irq_data *d)
+{
+	struct plic_handler *handler = d->common->handler_data;
+	plic_complete(handler->data, handler->contextid, d->hwirq);
+}
+
+static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
+			      irq_hw_number_t hwirq)
+{
+	struct plic_data *data = d->host_data;
+
+	irq_set_chip_and_handler(irq, &data->chip, handle_fasteoi_irq);
+	irq_set_chip_data(irq, data);
+	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 void plic_chained_handle_irq(struct irq_desc *desc)
+{
+	struct plic_handler *handler = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct irq_domain *domain = handler->data->domain;
+	u32 what;
+
+	chained_irq_enter(chip, desc);
+
+	while ((what = plic_claim(handler->data, handler->contextid))) {
+		int irq = irq_find_mapping(domain, what);
+
+		if (irq > 0)
+			generic_handle_irq(irq);
+		else
+			handle_bad_irq(desc);
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int plic_init(struct device_node *node, struct device_node *parent)
+{
+	struct plic_data *data;
+	struct resource resource;
+	int i, ok = 0;
+	int out = -1;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (WARN_ON(!data))
+		return -ENOMEM;
+
+	spin_lock_init(&data->lock);
+
+	data->reg = of_iomap(node, 0);
+	if (WARN_ON(!data->reg)) {
+		out = -EIO;
+		goto free_data;
+	}
+
+	of_property_read_u32(node, "riscv,ndev", &data->ndev);
+	if (WARN_ON(!data->ndev)) {
+		out = -EINVAL;
+		goto free_reg;
+	}
+
+	data->handlers = of_irq_count(node);
+	if (WARN_ON(!data->handlers)) {
+		out = -EINVAL;
+		goto free_reg;
+	}
+
+	data->handler =
+		kcalloc(data->handlers, sizeof(*data->handler), GFP_KERNEL);
+	if (WARN_ON(!data->handler)) {
+		out = -ENOMEM;
+		goto free_reg;
+	}
+
+	data->domain = irq_domain_add_linear(node, data->ndev+1, &plic_irqdomain_ops, data);
+	if (WARN_ON(!data->domain)) {
+		out = -ENOMEM;
+		goto free_handler;
+	}
+
+	of_address_to_resource(node, 0, &resource);
+	snprintf(data->name, sizeof(data->name),
+		 "riscv,plic0,%llx", resource.start);
+	data->chip.name = data->name;
+	data->chip.irq_mask = plic_irq_mask;
+	data->chip.irq_unmask = plic_irq_unmask;
+	data->chip.irq_enable = plic_irq_enable;
+	data->chip.irq_disable = plic_irq_disable;
+	data->chip.irq_eoi = plic_irq_eoi;
+
+	for (i = 0; i < data->handlers; ++i) {
+		struct plic_handler *handler = &data->handler[i];
+		struct of_phandle_args parent;
+		int parent_irq, hwirq;
+
+		handler->present = false;
+
+		if (of_irq_parse_one(node, i, &parent))
+			continue;
+		/* skip context holes */
+		if (parent.args[0] == -1)
+			continue;
+
+		/* skip any contexts that lead to inactive harts */
+		if (of_device_is_compatible(parent.np, "riscv,cpu-intc") &&
+		    parent.np->parent &&
+		    riscv_of_processor_hart(parent.np->parent) < 0)
+			continue;
+
+		parent_irq = irq_create_of_mapping(&parent);
+		if (!parent_irq)
+			continue;
+
+		handler->present = true;
+		handler->contextid = i;
+		handler->data = data;
+		/* hwirq prio must be > this to trigger an interrupt */
+		writel(0, plic_hart_threshold(data, i));
+
+		for (hwirq = 1; hwirq <= data->ndev; ++hwirq)
+			plic_disable(data, i, hwirq);
+		irq_set_chained_handler_and_data(parent_irq, plic_chained_handle_irq, handler);
+		++ok;
+	}
+
+	printk(KERN_INFO "%s: mapped %d interrupts to %d/%d handlers\n",
+	       data->name, data->ndev, ok, data->handlers);
+	WARN_ON(!ok);
+	return 0;
+
+free_handler:
+	kfree(data->handler);
+free_reg:
+	iounmap(data->reg);
+free_data:
+	kfree(data);
+	return out;
+}
+
+IRQCHIP_DECLARE(plic0, "riscv,plic0", plic_init);
-- 
2.13.0