RISC-V irqchip drivers

RISC-V irqchip drivers

[Christoph Hellwig]

The RISC-V ISA mandantes the presence of a simple, per-hart (hardware
thread) interrupt controller availiable to supervisor mode.  In addition
the RISC-V specification contains the definition of a programmable
interrupt controller that is present on all current RISC-V cores (at
least as far as a I know).

This series adds both of them.  For the per-hart controller this series
tries to address all comments vs the last posting from Palmr in June,
and for the PLIC it has a lot of cleanups which I think should address
all outstanding comments, but it has been a while since it was last
posted.

Without these irqchip drivers the RISC-V port in mainline is rather
useless as it can't boot on any SOC or emulator.  With it it still is
almost as useless as a clocksource driver is still missing, but at least
we're only a patch or two away from a booting system, and the
clocksource  driver will need the per-hart interrupt driver to work as
well.

Palmer: I assume you are ok with me pushing this forward.  If not I'll
happily drop this series.

A git tree with the patches in this series, the missing clocksource
driver a few pending patches to allow booting a RISC-V kernel in qemu
is available here:

    git://git.infradead.org/users/hch/riscv.git riscv-linux-4.18

Gitweb:

    http://git.infradead.org/users/hch/riscv.git/shortlog/refs/heads/riscv-linux-4.18

[PATCH 1/6] RISC-V: simplify software interrupt / IPI code

[Christoph Hellwig]

Rename handle_ipi to riscv_software_interrupt, drop the unused return
value and provide a stub for the !SMP build.  This allows simplifying
the upcoming interrupt controller driver by not providing a wrapper
for it.

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

diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
index 85e4220839b0..80ecb957fe9f 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -44,8 +44,17 @@ 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);
+/* Software interrupt handler */
+void riscv_software_interrupt(void);
+
+#else /* CONFIG_SMP */
+
+/*
+ * 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.
+ */
+#define riscv_software_interrupt()	WARN_ON()
 
 #endif /* CONFIG_SMP */
 
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

Re: [PATCH 1/6] RISC-V: simplify software interrupt / IPI code

[Palmer Dabbelt]

On Wed, 25 Jul 2018 02:36:44 PDT (-0700), Christoph Hellwig wrote:
> Rename handle_ipi to riscv_software_interrupt, drop the unused return
> value and provide a stub for the !SMP build.  This allows simplifying
> the upcoming interrupt controller driver by not providing a wrapper
> for it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/riscv/include/asm/smp.h | 13 +++++++++++--
>  arch/riscv/kernel/smp.c      |  6 ++----
>  2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
> index 85e4220839b0..80ecb957fe9f 100644
> --- a/arch/riscv/include/asm/smp.h
> +++ b/arch/riscv/include/asm/smp.h
> @@ -44,8 +44,17 @@ 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);
> +/* Software interrupt handler */
> +void riscv_software_interrupt(void);
> +
> +#else /* CONFIG_SMP */
> +
> +/*
> + * 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.
> + */
> +#define riscv_software_interrupt()	WARN_ON()
>
>  #endif /* CONFIG_SMP */
>
> 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

Acked-by: Palmer Dabbelt <palmer@sifive.com>

I think it's probably easier to have the whole patch set go through the IRQ
tree, so I won't put these in the RISC-V tree unless someone says something.

Re: [PATCH 1/6] RISC-V: simplify software interrupt / IPI code

[Christoph Hellwig]

On Wed, Jul 25, 2018 at 02:44:32PM -0700, Palmer Dabbelt wrote:
> I think it's probably easier to have the whole patch set go through the IRQ
> tree, so I won't put these in the RISC-V tree unless someone says something.

No matter where it goes it probably makes sense to keep the whole
series together..

[PATCH 2/6] 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 4dee9d4c13c0..93eb75eac4ff 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);
 
 #include <asm-generic/irq.h>
-- 
2.18.0

Re: [PATCH 2/6] RISC-V: remove INTERRUPT_CAUSE_* defines from asm/irq.h

[Palmer Dabbelt]

On Wed, 25 Jul 2018 02:36:45 PDT (-0700), Christoph Hellwig wrote:
> 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 4dee9d4c13c0..93eb75eac4ff 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);
>
>  #include <asm-generic/irq.h>

Acked-by: Palmer Dabbelt <palmer@sifive.com>

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

[Christoph Hellwig]

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.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
[hch: Kconfig simplifications, various cleanups]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/irqchip/Kconfig          |   4 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-riscv-intc.c | 197 +++++++++++++++++++++++++++++++
 3 files changed, 202 insertions(+)
 create mode 100644 drivers/irqchip/irq-riscv-intc.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index e9233db16e03..8460fdcecc2c 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -372,3 +372,7 @@ config QCOM_PDC
 	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
 
 endmenu
+
+config RISCV_INTC
+	def_bool y
+	depends on RISCV
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..883efaa154b8
--- /dev/null
+++ b/drivers/irqchip/irq-riscv-intc.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2012 Regents of the University of California
+ * Copyright (C) 2017 SiFive
+ */
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/smp.h>
+#include <asm/sbi.h>
+
+#define NR_RISCV_IRQS (8 * sizeof(uintptr_t))
+
+/*
+ * 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_MASK	(1UL << (NR_RISCV_IRQS - 1))
+
+struct riscv_irq_data {
+	struct irq_chip		chip;
+	struct irq_domain	*domain;
+	int			hart;
+	char			name[20];
+};
+
+static DEFINE_PER_CPU(struct riscv_irq_data, riscv_irq_data);
+
+static void riscv_intc_irq(struct pt_regs *regs)
+{
+	struct pt_regs *old_regs = set_irq_regs(regs);
+	unsigned long cause = csr_read(scause);
+	struct irq_domain *domain;
+
+	WARN_ON((cause & INTERRUPT_CAUSE_MASK) == 0);
+	cause &= ~INTERRUPT_CAUSE_MASK;
+
+	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 << 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 << 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)
+{
+	struct riscv_irq_data *data;
+	int hart;
+
+	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, NR_RISCV_IRQS,
+			&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, NR_RISCV_IRQS);
+	return 0;
+
+error_add_linear:
+	pr_warn("%s: unable to add IRQ domain\n", data->name);
+	return -ENXIO;
+}
+
+IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
-- 
2.18.0

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

[Marc Zyngier]

On 25/07/18 10:36, Christoph Hellwig 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.
> 
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> [hch: Kconfig simplifications, various cleanups]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/irqchip/Kconfig          |   4 +
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-riscv-intc.c | 197 +++++++++++++++++++++++++++++++
>  3 files changed, 202 insertions(+)
>  create mode 100644 drivers/irqchip/irq-riscv-intc.c
> 

[...]

> +/*
> + * 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 << 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 << 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);

This feels odd. It means that you cannot have the following sequence:

	local_irq_disable();
	enable_irq(x); // where x is owned by a remote hart

as smp_call_function_single() requires interrupts to be enabled.

More fundamentally, why are you trying to make these interrupts look
global while they aren't? arm/arm64 have similar restrictions with GICv2
and earlier, and treats these interrupts as per-cpu.

Given that the drivers that deal with drivers connected to the per-hart
irqchip are themselves likely to be aware of the per-cpu aspect, it
would make sense to align things (we've been through that same
discussion about the clocksource driver a few weeks back).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Christoph Hellwig]

On Wed, Jul 25, 2018 at 12:18:39PM +0100, Marc Zyngier wrote:
> This feels odd. It means that you cannot have the following sequence:
> 
> 	local_irq_disable();
> 	enable_irq(x); // where x is owned by a remote hart
> 
> as smp_call_function_single() requires interrupts to be enabled.
> 
> More fundamentally, why are you trying to make these interrupts look
> global while they aren't? arm/arm64 have similar restrictions with GICv2
> and earlier, and treats these interrupts as per-cpu.
> 
> Given that the drivers that deal with drivers connected to the per-hart
> irqchip are themselves likely to be aware of the per-cpu aspect, it
> would make sense to align things (we've been through that same
> discussion about the clocksource driver a few weeks back).

Right now the only direct consumers are said clocksource, the PLIC
driver later in this series and the RISC-V arch IPI code.  None of them
is going to do a manual enable_irq, so I guess the remote case of the
code is simply dead code.  I'll take a look at converting them to
per-cpu.  I guess the GICv2 driver is the best template?

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

[Marc Zyngier]

On 25/07/18 12:24, Christoph Hellwig wrote:
> On Wed, Jul 25, 2018 at 12:18:39PM +0100, Marc Zyngier wrote:
>> This feels odd. It means that you cannot have the following sequence:
>>
>> 	local_irq_disable();
>> 	enable_irq(x); // where x is owned by a remote hart
>>
>> as smp_call_function_single() requires interrupts to be enabled.
>>
>> More fundamentally, why are you trying to make these interrupts look
>> global while they aren't? arm/arm64 have similar restrictions with GICv2
>> and earlier, and treats these interrupts as per-cpu.
>>
>> Given that the drivers that deal with drivers connected to the per-hart
>> irqchip are themselves likely to be aware of the per-cpu aspect, it
>> would make sense to align things (we've been through that same
>> discussion about the clocksource driver a few weeks back).
> 
> Right now the only direct consumers are said clocksource, the PLIC
> driver later in this series and the RISC-V arch IPI code.  None of them
> is going to do a manual enable_irq, so I guess the remote case of the
> code is simply dead code.  I'll take a look at converting them to
> per-cpu.  I guess the GICv2 driver is the best template?

I think you can do a much better job than the GICv2 driver ;-). You have
the chance of a clean slate, and no legacy (or ACPI) junk to deal with!

I think this is just a matter of moving the HLIC declaration in DT to be
outside of the cpu nodes (you just have a single HLIC node that is valid
for all the CPUs in the system), and making the interrupts percpu_devid
in your mapping function (see gic_irq_domain_map for reference).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Atish Patra]

On 7/25/18 4:37 AM, Marc Zyngier wrote:
> On 25/07/18 12:24, Christoph Hellwig wrote:
>> On Wed, Jul 25, 2018 at 12:18:39PM +0100, Marc Zyngier wrote:
>>> This feels odd. It means that you cannot have the following sequence:
>>>
>>> 	local_irq_disable();
>>> 	enable_irq(x); // where x is owned by a remote hart
>>>
>>> as smp_call_function_single() requires interrupts to be enabled.
>>>
>>> More fundamentally, why are you trying to make these interrupts look
>>> global while they aren't? arm/arm64 have similar restrictions with GICv2
>>> and earlier, and treats these interrupts as per-cpu.
>>>
>>> Given that the drivers that deal with drivers connected to the per-hart
>>> irqchip are themselves likely to be aware of the per-cpu aspect, it
>>> would make sense to align things (we've been through that same
>>> discussion about the clocksource driver a few weeks back).
>>
>> Right now the only direct consumers are said clocksource, the PLIC
>> driver later in this series and the RISC-V arch IPI code.  None of them
>> is going to do a manual enable_irq, so I guess the remote case of the
>> code is simply dead code.  I'll take a look at converting them to
>> per-cpu.  I guess the GICv2 driver is the best template?
> 
> I think you can do a much better job than the GICv2 driver ;-). You have
> the chance of a clean slate, and no legacy (or ACPI) junk to deal with!
> 
> I think this is just a matter of moving the HLIC declaration in DT to be
> outside of the cpu nodes (you just have a single HLIC node that is valid
> for all the CPUs in the system), and making the interrupts percpu_devid
> in your mapping function (see gic_irq_domain_map for reference).
> 

If I am not wrong, we need to change the interrupt-extended property in 
PLIC DT entry as well. Currently, there are 5 entries corresponding to 
individual HLIC. I was also planning to start working on converting HLIC 
to per-cpu but it got delayed because of travel.

Christoph: I am not sure if you have access to HighFive Unleashed board. 
If not I would be happy to test it on the board whenever your patches 
are ready.

Regards,
Atish
> Thanks,
> 
> 	M.
> 

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

[Anup Patel]

On Wed, Jul 25, 2018 at 4:54 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Wed, Jul 25, 2018 at 12:18:39PM +0100, Marc Zyngier wrote:
>> This feels odd. It means that you cannot have the following sequence:
>>
>>       local_irq_disable();
>>       enable_irq(x); // where x is owned by a remote hart
>>
>> as smp_call_function_single() requires interrupts to be enabled.
>>
>> More fundamentally, why are you trying to make these interrupts look
>> global while they aren't? arm/arm64 have similar restrictions with GICv2
>> and earlier, and treats these interrupts as per-cpu.
>>
>> Given that the drivers that deal with drivers connected to the per-hart
>> irqchip are themselves likely to be aware of the per-cpu aspect, it
>> would make sense to align things (we've been through that same
>> discussion about the clocksource driver a few weeks back).
>
> Right now the only direct consumers are said clocksource, the PLIC
> driver later in this series and the RISC-V arch IPI code.  None of them
> is going to do a manual enable_irq, so I guess the remote case of the
> code is simply dead code.  I'll take a look at converting them to
> per-cpu.  I guess the GICv2 driver is the best template?

Actually, RISCV HLIC and PLIC are very similar to RPi2 and RPi3 SOCs.

On RPi2 and RPi3, we have per-CPU BCM2836 local intc and the global
interrupts are managed using BCM2835 intc. You should certainly have
a look a this drivers because these very simple compared to GICv2 and
GICv3 drivers.

Regards,
Anup

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

[Christoph Hellwig]

On Thu, Jul 26, 2018 at 09:08:00AM +0530, Anup Patel wrote:
> Actually, RISCV HLIC and PLIC are very similar to RPi2 and RPi3 SOCs.
> 
> On RPi2 and RPi3, we have per-CPU BCM2836 local intc and the global
> interrupts are managed using BCM2835 intc. You should certainly have
> a look a this drivers because these very simple compared to GICv2 and
> GICv3 drivers.

Yes, using that model makes writing the per-cpu irq controller driver
trivial.  But retrofitting it into the device tree, where the existing
bootloader (bbl) assumes the old DT layout is a giant pain in the neck.

At the same time I'm still not conveninced RISC-V really needs a full
irqchip driver for the per-cpu interrupt 'controller' really is nothing
but 1 and a half architectural control registers:

  - the scause register that contains the reason for an exception
    (any exception including syscalls and page faults) for the entry
    into supervisor mode.  This includes a bit to indicate interrupts,
    and then logical interrupt reason, out of which only three are
    interesting for supervisor mode (timer, software, external)
  - the sie register that allows to to enable/disable each of the above
    causes individually

So after burning out on DT hacking (never mind retrofitting that into
actual shipping SOCs vs just qemu) I'm going to try a version that
doesn't add an irqchip for this but just handles it hardcoded in
RISC-V do_IRQ.  I'll still keep the irqchip for the PLIC, which while
specificed in the RISC-V spec isn't architectural but an actual
periphal.

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

[Anup Patel]

On Thu, Jul 26, 2018 at 1:57 PM, Christoph Hellwig <hch@lst.de> wrote:
> On Thu, Jul 26, 2018 at 09:08:00AM +0530, Anup Patel wrote:
>> Actually, RISCV HLIC and PLIC are very similar to RPi2 and RPi3 SOCs.
>>
>> On RPi2 and RPi3, we have per-CPU BCM2836 local intc and the global
>> interrupts are managed using BCM2835 intc. You should certainly have
>> a look a this drivers because these very simple compared to GICv2 and
>> GICv3 drivers.
>
> Yes, using that model makes writing the per-cpu irq controller driver
> trivial.  But retrofitting it into the device tree, where the existing
> bootloader (bbl) assumes the old DT layout is a giant pain in the neck.

This can also be taken care in HLIC driver probe function with something
like below:

if (parent)
return 0;

if (of_is_compatible(parent, "riscv,cpu")) {
/*
* Legacy DT binding so we have HLIC DT node
* under each CPU DT node. To provide backwared
* compatiblity we go forward for only one HLIC
* DT node
*/
if (atomic_inc_return(&hlic_lottery) > 1)
return 0;
}

In PLIC driver probe, register nested IRQ handler for only first two
entries of interrupts-extended because it is HLIC IRQs are per-CPU. We
can happily ignore other entries in interrupts-extended of Legacy DTS.

>
> At the same time I'm still not conveninced RISC-V really needs a full
> irqchip driver for the per-cpu interrupt 'controller' really is nothing
> but 1 and a half architectural control registers:
>
>   - the scause register that contains the reason for an exception
>     (any exception including syscalls and page faults) for the entry
>     into supervisor mode.  This includes a bit to indicate interrupts,
>     and then logical interrupt reason, out of which only three are
>     interesting for supervisor mode (timer, software, external)
>   - the sie register that allows to to enable/disable each of the above
>     causes individually

Biggest plus is the ability show stats for per-CPU interrupts via
"cat /proc/interrupts" (just like other architectures).

Currently, we have only three per-CPU interrupts (timer, software,
and external) but in-future people might come-up with interesting
devices which might have per-CPU interrupts.

>
> So after burning out on DT hacking (never mind retrofitting that into
> actual shipping SOCs vs just qemu) I'm going to try a version that
> doesn't add an irqchip for this but just handles it hardcoded in
> RISC-V do_IRQ.  I'll still keep the irqchip for the PLIC, which while
> specificed in the RISC-V spec isn't architectural but an actual
> periphal.

I believe it possible to have RICV HLIC driver which maintains
backward compatibility with legacy DTS. I haven't tried above
approach myself on QEMU so I will let you decide.

Regards,
Anup

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

[Thomas Gleixner]

On Wed, 25 Jul 2018, Christoph Hellwig wrote:

> On Wed, Jul 25, 2018 at 12:18:39PM +0100, Marc Zyngier wrote:
> > This feels odd. It means that you cannot have the following sequence:
> > 
> > 	local_irq_disable();
> > 	enable_irq(x); // where x is owned by a remote hart
> > 
> > as smp_call_function_single() requires interrupts to be enabled.
> > 
> > More fundamentally, why are you trying to make these interrupts look
> > global while they aren't? arm/arm64 have similar restrictions with GICv2
> > and earlier, and treats these interrupts as per-cpu.
> > 
> > Given that the drivers that deal with drivers connected to the per-hart
> > irqchip are themselves likely to be aware of the per-cpu aspect, it
> > would make sense to align things (we've been through that same
> > discussion about the clocksource driver a few weeks back).
> 
> Right now the only direct consumers are said clocksource, the PLIC
> driver later in this series and the RISC-V arch IPI code.  None of them
> is going to do a manual enable_irq, so I guess the remote case of the
> code is simply dead code.  I'll take a look at converting them to
> per-cpu.  I guess the GICv2 driver is the best template?

Confused. The timer and the IPI are separate causes and have nothing to do
with the per cpu irq domain. That's what the low level interrupt handling
code tells me.

If I understand correctly then the per cpu irq domain is for device
interrupts, right? If so, then this simply cannot work and there is no way
to make it work with per cpu interrupts either.

Is there some high level documentation about the design (or the lack of) or
can someone give a concise explanation how this stuff is supposed to work?

Thanks,

	tglx

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

[Christoph Hellwig]

On Wed, Aug 01, 2018 at 08:55:06PM +0200, Thomas Gleixner wrote:
> Confused. The timer and the IPI are separate causes and have nothing to do
> with the per cpu irq domain. That's what the low level interrupt handling
> code tells me.

Yes.

> If I understand correctly then the per cpu irq domain is for device
> interrupts, right? If so, then this simply cannot work and there is no way
> to make it work with per cpu interrupts either.

Actually I don't think we even need the per cpu irq domain, see my
latest version of these patches here (which I'll send out again after
fixing up the remaining device tree binding review comments):

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

> Is there some high level documentation about the design (or the lack of) or
> can someone give a concise explanation how this stuff is supposed to work?

All of the current RISC-V irq handling concepts are document in the RISC-V
privile spec:

https://content.riscv.org/wp-content/uploads/2017/05/riscv-privileged-v1.10.pdf

The cpu local interrupt handling, which was irq-riscv-intc.c in this
series and has been moved to arch/riscv/kernel/irq.c in my new series
is split over a few control registers (CSRs in RISC-V speak):

The exception handler, which includes the delivery of interrupts to
the CPU is set up using the stvec CSR (Section 4.1.4).  The vector mode
mentioned there is not supported by Linux (and not by any hardware known
to me), so ignore it.

Once an exception has been triggered Linux reads the scause CSR
(Section 4.1.10) to check the exception cause.  If the interrupt
bit is set we have three possible exception causes that matter for
the kernel: Supervisor software interrupt, Supervisor timer interrupt,
Supervisor external interrupt (ignore the user versions, I'm not even
sure they are implementable, and they certainly are not at the moment).

To enable / disable any of these logical interrupt sources the sie
CSR (Section 4.1.5) has a bit for each kind thast can be set/cleared.

Also there is the sip CSR (also section 4.1.5) which tells if any of those
is pending at the moment.

The PLIC itself is only described logically in the RISC-V privileged
spec (Section 7), the actual register layout is left to implementations.
The one implemented here is documented in Chaper 5 of this document:

https://static.dev.sifive.com/SiFive-E3-Coreplex-v1.2.pdf

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

[Thomas Gleixner]

On Thu, 2 Aug 2018, Christoph Hellwig wrote:
> The cpu local interrupt handling, which was irq-riscv-intc.c in this
> series and has been moved to arch/riscv/kernel/irq.c in my new series
> is split over a few control registers (CSRs in RISC-V speak):
> 
> The exception handler, which includes the delivery of interrupts to
> the CPU is set up using the stvec CSR (Section 4.1.4).  The vector mode
> mentioned there is not supported by Linux (and not by any hardware known
> to me), so ignore it.

And even if it would be available then it would just avoid the software
decoding of the cause register. So no fundamental difference.

> Once an exception has been triggered Linux reads the scause CSR
> (Section 4.1.10) to check the exception cause.  If the interrupt
> bit is set we have three possible exception causes that matter for
> the kernel: Supervisor software interrupt, Supervisor timer interrupt,
> Supervisor external interrupt (ignore the user versions, I'm not even
> sure they are implementable, and they certainly are not at the moment).

Yeah. I would upfront declare the user stuff broken and not supported.

> To enable / disable any of these logical interrupt sources the sie
> CSR (Section 4.1.5) has a bit for each kind thast can be set/cleared.
> 
> Also there is the sip CSR (also section 4.1.5) which tells if any of those
> is pending at the moment.

So that's the low level per cpu interrupt/exception distribution mechanism,
i.e. a distinct per cpu 'vector' space with fixed functionality. It does
not make sense to actually handle that as an irq chip. It has absolutely no
relevance. The software interrupts are enabled when the CPU is started and
the external ones as well as they are gated by the PLIC.

The only thing which might need to access the enable register is the local
timer interrupt. That really does not require an extra irq chip as the
enable/disable is really just at cpu up/down time and the magic happens on
the local CPU so no smp functional call hackery is required.

The PLIC is the beast which wants a proper irqdomain/irqchip
implementation.

Thanks,

	tglx

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

[Christoph Hellwig]

On Thu, Aug 02, 2018 at 11:35:43AM +0200, Thomas Gleixner wrote:
> On Thu, 2 Aug 2018, Christoph Hellwig wrote:
> > The cpu local interrupt handling, which was irq-riscv-intc.c in this
> > series and has been moved to arch/riscv/kernel/irq.c in my new series
> > is split over a few control registers (CSRs in RISC-V speak):
> > 
> > The exception handler, which includes the delivery of interrupts to
> > the CPU is set up using the stvec CSR (Section 4.1.4).  The vector mode
> > mentioned there is not supported by Linux (and not by any hardware known
> > to me), so ignore it.
> 
> And even if it would be available then it would just avoid the software
> decoding of the cause register. So no fundamental difference.
> 
> > Once an exception has been triggered Linux reads the scause CSR
> > (Section 4.1.10) to check the exception cause.  If the interrupt
> > bit is set we have three possible exception causes that matter for
> > the kernel: Supervisor software interrupt, Supervisor timer interrupt,
> > Supervisor external interrupt (ignore the user versions, I'm not even
> > sure they are implementable, and they certainly are not at the moment).
> 
> Yeah. I would upfront declare the user stuff broken and not supported.
> 
> > To enable / disable any of these logical interrupt sources the sie
> > CSR (Section 4.1.5) has a bit for each kind thast can be set/cleared.
> > 
> > Also there is the sip CSR (also section 4.1.5) which tells if any of those
> > is pending at the moment.
> 
> So that's the low level per cpu interrupt/exception distribution mechanism,
> i.e. a distinct per cpu 'vector' space with fixed functionality. It does
> not make sense to actually handle that as an irq chip. It has absolutely no
> relevance. The software interrupts are enabled when the CPU is started and
> the external ones as well as they are gated by the PLIC.
> 
> The only thing which might need to access the enable register is the local
> timer interrupt. That really does not require an extra irq chip as the
> enable/disable is really just at cpu up/down time and the magic happens on
> the local CPU so no smp functional call hackery is required.
> 
> The PLIC is the beast which wants a proper irqdomain/irqchip
> implementation.

And that is exactly what I've done in the repost.

Local interrupt handling:

http://git.infradead.org/users/hch/riscv.git/commitdiff/149ae0008effe80e1fb79444eb6a03c45bed29ba

PLIC driver:

http://git.infradead.org/users/hch/riscv.git/commitdiff/00f68341fa6dcda9b7c6c4da7eeb52d7ef933368

Clocksource:

http://git.infradead.org/users/hch/riscv.git/commitdiff/51981cbba2fe2a118bb84b4c4aabb0f0d988ed2a

I need to polish the DT binding a little more and will repost later today.

> 
> Thanks,
> 
> 	tglx
---end quoted text---

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

[Thomas Gleixner]

On Thu, 2 Aug 2018, Christoph Hellwig wrote:
> On Thu, Aug 02, 2018 at 11:35:43AM +0200, Thomas Gleixner wrote:
> > So that's the low level per cpu interrupt/exception distribution mechanism,
> > i.e. a distinct per cpu 'vector' space with fixed functionality. It does
> > not make sense to actually handle that as an irq chip. It has absolutely no
> > relevance. The software interrupts are enabled when the CPU is started and
> > the external ones as well as they are gated by the PLIC.
> > 
> > The only thing which might need to access the enable register is the local
> > timer interrupt. That really does not require an extra irq chip as the
> > enable/disable is really just at cpu up/down time and the magic happens on
> > the local CPU so no smp functional call hackery is required.
> > 
> > The PLIC is the beast which wants a proper irqdomain/irqchip
> > implementation.
> 
> And that is exactly what I've done in the repost.

Ok.

> I need to polish the DT binding a little more and will repost later today.

Lemme go through that reposted series quickly.

Thanks,

	tglx

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

[Palmer Dabbelt]

On Wed, 01 Aug 2018 11:55:06 PDT (-0700), tglx@linutronix.de wrote:
>
> On Wed, 25 Jul 2018, Christoph Hellwig wrote:
>
>> On Wed, Jul 25, 2018 at 12:18:39PM +0100, Marc Zyngier wrote:
>> > This feels odd. It means that you cannot have the following sequence:
>> >
>> > 	local_irq_disable();
>> > 	enable_irq(x); // where x is owned by a remote hart
>> >
>> > as smp_call_function_single() requires interrupts to be enabled.
>> >
>> > More fundamentally, why are you trying to make these interrupts look
>> > global while they aren't? arm/arm64 have similar restrictions with GICv2
>> > and earlier, and treats these interrupts as per-cpu.
>> >
>> > Given that the drivers that deal with drivers connected to the per-hart
>> > irqchip are themselves likely to be aware of the per-cpu aspect, it
>> > would make sense to align things (we've been through that same
>> > discussion about the clocksource driver a few weeks back).
>>
>> Right now the only direct consumers are said clocksource, the PLIC
>> driver later in this series and the RISC-V arch IPI code.  None of them
>> is going to do a manual enable_irq, so I guess the remote case of the
>> code is simply dead code.  I'll take a look at converting them to
>> per-cpu.  I guess the GICv2 driver is the best template?
>
> Confused. The timer and the IPI are separate causes and have nothing to do
> with the per cpu irq domain. That's what the low level interrupt handling
> code tells me.
>
> If I understand correctly then the per cpu irq domain is for device
> interrupts, right? If so, then this simply cannot work and there is no way
> to make it work with per cpu interrupts either.
>
> Is there some high level documentation about the design (or the lack of) or
> can someone give a concise explanation how this stuff is supposed to work?

The ISA spec defines this, and I'm not sure I can be a whole lot more concise 
than what's there but I'll try:

This driver is for the ISA-mandated interrupts present on all RISC-V systems.  
These interrupts are tracked on a per-hart (hart is a RISC-V term that means 
hardware thread, it a hyperthread in Intel land) basis.

On RISC-V systems there is a set of CSRs (control and status registers) local 
to each hart.  These registers can be accessed by special instructions, and are 
used to perform hart-local actions that are of a medium speed: things like 
accessing the floating point exception state or changing the page table base 
pointer.  When a hart is in supervisor mode, it has access to a handful of CSRs 
that are related to interrupts:

* stvec: The trap vector, which is the entry point for both interrupts and 
  exceptions.
* sie: Supervisor Interrupts Enabled.  This is a bit mask of enabled 
  interrupts.
* sip: Supervisor Interrupts Pending.  This is a bit mask of pending 
  interrupts, which can be polled when interrupts are disabled (or I guess when 
  they're enabled too, but that's not particularly useful).
* scause: Supervisor Trap Cause.  This allows code to determine what sort of 
  trap was taken.
* sepc: Supervisor Exception PC (actually the PC for all traps).  This allows 
  code to determine how to get back to where it needs to continue after 
  handling a trap.
* stval: Supervisor Trap VALue.  This provides additional trap-specific 
  information: for example, if a trap says "you attempted to write to an 
  address that isn't mapped or is read only" then stval contains the address 
  that the write was directed at.

As such, the ISA itself doesn't really differentiate between exceptions and 
interrupts: they both cause a privilege transition to happen, redirect 
execution to stvec, and write a handful of CSRs as side effects.  It's up to 
software to determine the difference.

In order to make it fast for software to determine if a scause value is an 
exception or an interrupt that is encoded in the high bit.  There are 11 
defined exceptions, which include system calls, breakpoints, page faults, and 
access faults -- that's all handled in core RISC-V arch code.  Additionally, 
there are 6 defined interrupt types:

* User software interrupt
* User timer interrupt
* User external interrupt
* Supervisor software interrupt
* Supervisor timer interrupt
* Supervisor external interrupt

The user interrupts are unsupported by Linux and thus never happen -- they're 
really designed for real-time embedded code, in POSIX land we use the standard 
signal delivery mechanisms as it'd be insane to put that in an ISA.  Thus, 
functionally there are three interrupt types:

* Supervisor software interrupt: These are never triggered by a hardware 
  device, and are therefor left for software use.  On Linux we use these to 
  indicate that a message may have arrived in the per-hart message queue, which 
  is used to implement IPIs.
* Supervisor timer interrupt: When the RISC-V ISA was originally defined we 
  thought that it was important to mandate that accurate real-time facilities 
  exist, so there's a handful of time-related bits encoded into the ISA.  In 
  retrospect it seems like this didn't end up being so clean as it's becoming a 
  burden when doing things like formally specifying the ISA, but we have 
  workarounds for these issues so we don't deem it worth re-spinning the ISA.  
  On Linux this is attached to a clocksource driver, with the only extant 
  example being SiFive's clock driver.
* Supervisor external interrupt: Essentially all the interrupts, aside from the 
  timer being a special case.  This is what's attached to the proper interrupt 
  controller, with the only extant example being SiFive's PLIC (which was 
  mistakenly called a RISC-V plic in the device tree, but there's another 
  thread going about fixing that).

Originally this hart-local interrupt code was included in the RISC-V arch port, 
right along our exception handling code.  This matches up well because RISC-V 
systems don't really differentiate between interrupts and exceptions, so it 
fits in well.  The trouble is that it smells like an interrupt controller and 
doesn't follow the standard mechanisms in Linux.

As part of our original push to upstream the arch code it was suggested that we 
move this out to an irqchip driver, but after actually attempting to do that it 
appears that the mechanics of doing so have overshadowed the complexity of the 
actual interrupt handling code, which is only a dozen or so instructions.  In 
retrospect this is just another instance of me not knowing what I'm doing, 
sorry!

I like Christoph's approach of merging the ISA-mandated interrupt handling code 
back into arch/riscv, as it's much saner that way.  The one big headache is 
that because we special-cased timer interrupts in the ISA they now disappear 
from the standard Linux mechanisms for handling these.

That said, I'd rather have this warts and all then wait around for something 
perfect, as maintaining a dozen or so out of tree drivers that are tightly 
coupled to the core arch code has proven to be a mess.  If we can get the code 
upstream then everyone will be on the same page so we can work on actually 
improving this, as opposed to just spinning our wheels keeping this big mess 
alive.

Hopefully that makes some amount of sense...

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

[Thomas Gleixner]

On Fri, 3 Aug 2018, Palmer Dabbelt wrote:
> On Wed, 01 Aug 2018 11:55:06 PDT (-0700), tglx@linutronix.de wrote:
> > Is there some high level documentation about the design (or the lack of) or
> > can someone give a concise explanation how this stuff is supposed to work?
> As part of our original push to upstream the arch code it was suggested that
> we move this out to an irqchip driver, but after actually attempting to do
> that it appears that the mechanics of doing so have overshadowed the
> complexity of the actual interrupt handling code, which is only a dozen or so
> instructions.  In retrospect this is just another instance of me not knowing
> what I'm doing, sorry!
> 
> I like Christoph's approach of merging the ISA-mandated interrupt handling
> code back into arch/riscv, as it's much saner that way.  The one big headache
> is that because we special-cased timer interrupts in the ISA they now
> disappear from the standard Linux mechanisms for handling these.
> 
> That said, I'd rather have this warts and all then wait around for something
> perfect, as maintaining a dozen or so out of tree drivers that are tightly
> coupled to the core arch code has proven to be a mess.  If we can get the code
> upstream then everyone will be on the same page so we can work on actually
> improving this, as opposed to just spinning our wheels keeping this big mess
> alive.
> 
> Hopefully that makes some amount of sense...

Thanks for the detailed explanation. It's what I extracted from the
documents to which Christoph pointed me. And as I said in the other thread
it does not realiy make sense to force that low level mechanism into an irq
chip. That would be overkill for no real value.

Thanks,

	tglx

[PATCH 4/6] dt-bindings: interrupt-controller: RISC-V local interrupt controller docs

[Christoph Hellwig]

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

Re: [PATCH 4/6] dt-bindings: interrupt-controller: RISC-V local interrupt controller docs

[Rob Herring]

On Wed, Jul 25, 2018 at 11:36:47AM +0200, Christoph Hellwig 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

My questions and comments on the prior version from Palmer remain.

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

Re: [PATCH 4/6] dt-bindings: interrupt-controller: RISC-V local interrupt controller docs

[Christoph Hellwig]

On Tue, Jul 31, 2018 at 04:37:14PM -0600, Rob Herring wrote:
> On Wed, Jul 25, 2018 at 11:36:47AM +0200, Christoph Hellwig 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
> 
> My questions and comments on the prior version from Palmer remain.

Can you point to these questions please?  I don't even rember when this
was last posted as it must have been a long time ago.

Re: [PATCH 4/6] dt-bindings: interrupt-controller: RISC-V local interrupt controller docs

[Rob Herring]

On Wed, Aug 1, 2018 at 1:08 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Jul 31, 2018 at 04:37:14PM -0600, Rob Herring wrote:
> > On Wed, Jul 25, 2018 at 11:36:47AM +0200, Christoph Hellwig 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
> >
> > My questions and comments on the prior version from Palmer remain.
>
> Can you point to these questions please?  I don't even rember when this
> was last posted as it must have been a long time ago.

It was just a month ago[1], but in googling for the links some
comments from the 1st posting a year ago[2] aren't addressed either.
That applies to the PLIC too.

Rob

[1] https://lkml.org/lkml/2018/7/3/1001
[2] https://lkml.org/lkml/2017/6/27/24

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

[Christoph Hellwig]

From: Palmer Dabbelt <palmer@dabbelt.com>

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 connects global interrupt sources to the local interrupt
controller on each hart.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
[hch: various cleanups, fixed typos, added SPDX tag]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/irqchip/Kconfig          |  13 ++
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-riscv-plic.c | 295 +++++++++++++++++++++++++++++++
 3 files changed, 309 insertions(+)
 create mode 100644 drivers/irqchip/irq-riscv-plic.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 8460fdcecc2c..d1afac8829ae 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -376,3 +376,16 @@ endmenu
 config RISCV_INTC
 	def_bool y
 	depends on RISCV
+
+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 74e333cc274c..7954f4c4a629 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -88,3 +88,4 @@ 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
+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..7b80b73c28a0
--- /dev/null
+++ b/drivers/irqchip/irq-riscv-plic.c
@@ -0,0 +1,295 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017 SiFive
+ */
+#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 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
+
+struct plic_handler {
+	bool			present;
+	int			contextid;
+	struct plic_data	*data;
+};
+
+/*
+ * PLIC devices are named like 'riscv,plic0,%llx', this is enough space to
+ * store that name.
+ */
+#define PLIC_DATA_NAME_SIZE 30
+
+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;
+};
+
+/*
+ * Each interrupt source has a priority register associated with it.
+ * We always hardwire it to one in Linux.
+ */
+static inline u32 __iomem *plic_priority(struct plic_data *data, int hwirq)
+{
+	return data->reg + hwirq * 0x04;
+}
+
+/*
+ * Each hart context has a vector of interrupt enable bits associated with it.
+ * There is one bit for each interrupt source.
+ */
+static inline u32 __iomem *plic_enable_vector(struct plic_data *data,
+		int contextid)
+{
+	return data->reg + (1 << 13) + contextid * 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_THRESHOLD	0
+#define CONTEXT_CLAIM		4
+
+static inline u32 __iomem *plic_hart_data(struct plic_data *data,
+		int contextid)
+{
+	return data->reg + (1 << 21) + contextid * 0x1000;
+}
+
+/* Explicit interrupt masking. */
+static void plic_disable(struct plic_data *data, int contextid, int hwirq)
+{
+	u32 __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)
+{
+	u32 __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);
+	int i;
+
+	writel(1, plic_priority(data, d->hwirq));
+	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);
+	int i;
+
+	writel(0, plic_priority(data, d->hwirq));
+	for (i = 0; i < data->handlers; ++i)
+		if (data->handler[i].present)
+			plic_disable(data, i, 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_simple_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,
+};
+
+/*
+ * 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_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;
+	void __iomem *ph = plic_hart_data(handler->data, handler->contextid);
+	u32 what;
+
+	chained_irq_enter(chip, desc);
+	while ((what = readl(ph + CONTEXT_CLAIM))) {
+		int irq = irq_find_mapping(domain, what);
+
+		if (irq > 0)
+			generic_handle_irq(irq);
+		else
+			handle_bad_irq(desc);
+		writel(what, ph + CONTEXT_CLAIM);
+	}
+	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;
+
+	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_data(data, i) + CONTEXT_THRESHOLD);
+
+		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;
+	}
+
+	pr_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.18.0

[PATCH 6/6] 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

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

[Rob Herring]

On Wed, Jul 25, 2018 at 11:36:49AM +0200, 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
> +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"

Perhaps this should be 'sifive,plic0'

Normally this would have an SoC specific compatible too. Sometimes we 
can get away without, but it doesn't seem like the PLIC is very tightly 
specified nor has common implementations.

> +- #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>;

Not documented.

> +	};
> -- 
> 2.18.0
> 

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

[Christoph Hellwig]

On Tue, Jul 31, 2018 at 04:46:30PM -0600, Rob Herring wrote:
> Perhaps this should be 'sifive,plic0'

Excepet for the fact this the old name has already been in shipping
hardware and release of qemu and other emulators it should.

> Normally this would have an SoC specific compatible too. Sometimes we 
> can get away without, but it doesn't seem like the PLIC is very tightly 
> specified nor has common implementations.

It is a giant f***cking mess to be honest.  Adding a highlevel spec
to the ISA but not a register layout is completely idotic, but if you
look at the current riscv-sw list this decision is still defended by
SiFive / the RISC-V foundation.  The whole stale of the RISC-V platform
Ecosystem is rather pathetic unfortunately, and people don't seem to
be willing to learn from past good practice nor mistakes in ARM land.

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

[Rob Herring]

On Wed, Aug 1, 2018 at 1:12 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Jul 31, 2018 at 04:46:30PM -0600, Rob Herring wrote:
> > Perhaps this should be 'sifive,plic0'
>
> Excepet for the fact this the old name has already been in shipping
> hardware and release of qemu and other emulators it should.

Not really my problem that they didn't follow the process and upstream
their binding first. But this alone is just a string identifier, so I
don't really care that much. If things are really a mess, then the
next implementations will have to have better compatible strings. More
likely, I'll just see folks trying to add various properties to deal
with all the differences.

You could always define a better compatible and leave 'riscv,plic0' as
a fallback to avoid breaking things.

> > Normally this would have an SoC specific compatible too. Sometimes we
> > can get away without, but it doesn't seem like the PLIC is very tightly
> > specified nor has common implementations.
>
> It is a giant f***cking mess to be honest.  Adding a highlevel spec
> to the ISA but not a register layout is completely idotic, but if you
> look at the current riscv-sw list this decision is still defended by
> SiFive / the RISC-V foundation.  The whole stale of the RISC-V platform
> Ecosystem is rather pathetic unfortunately, and people don't seem to
> be willing to learn from past good practice nor mistakes in ARM land.

Interrupt controllers are where the differentiation is. ;)

Rob

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

[Christoph Hellwig]

On Wed, Aug 01, 2018 at 12:26:31PM -0600, Rob Herring wrote:
> Not really my problem that they didn't follow the process and upstream
> their binding first. But this alone is just a string identifier, so I
> don't really care that much. If things are really a mess, then the
> next implementations will have to have better compatible strings. More
> likely, I'll just see folks trying to add various properties to deal
> with all the differences.
> 
> You could always define a better compatible and leave 'riscv,plic0' as
> a fallback to avoid breaking things.

Is there any better way to define a compatible other than having
duplicate IRQCHIP_DECLARE() statements?

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

[Rob Herring]

On Thu, Aug 2, 2018 at 3:50 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Wed, Aug 01, 2018 at 12:26:31PM -0600, Rob Herring wrote:
> > Not really my problem that they didn't follow the process and upstream
> > their binding first. But this alone is just a string identifier, so I
> > don't really care that much. If things are really a mess, then the
> > next implementations will have to have better compatible strings. More
> > likely, I'll just see folks trying to add various properties to deal
> > with all the differences.
> >
> > You could always define a better compatible and leave 'riscv,plic0' as
> > a fallback to avoid breaking things.
>
> Is there any better way to define a compatible other than having
> duplicate IRQCHIP_DECLARE() statements?

No, but you only need the fallback if the compatible is
'"sifive,plic0", "riscv,plic0";'.

Rob

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

[Palmer Dabbelt]

On Wed, 01 Aug 2018 11:26:31 PDT (-0700), robh@kernel.org wrote:
> On Wed, Aug 1, 2018 at 1:12 AM Christoph Hellwig <hch@lst.de> wrote:
>>
>> On Tue, Jul 31, 2018 at 04:46:30PM -0600, Rob Herring wrote:
>> > Perhaps this should be 'sifive,plic0'
>>
>> Excepet for the fact this the old name has already been in shipping
>> hardware and release of qemu and other emulators it should.
>
> Not really my problem that they didn't follow the process and upstream
> their binding first. But this alone is just a string identifier, so I
> don't really care that much. If things are really a mess, then the
> next implementations will have to have better compatible strings. More
> likely, I'll just see folks trying to add various properties to deal
> with all the differences.
>
> You could always define a better compatible and leave 'riscv,plic0' as
> a fallback to avoid breaking things.

Ya, sorry about that.  FWIW, we don't consider any of the bindings stable until 
they're actually accepted upstream, so it's on us to fix our bootloaders to 
match what actually lands upstream.  Luckily there's not that much hardware out 
there and none of it is in production, so I'm OK forcing people to upgrade 
bootloaders to make this all work.

I think it's probably best to leave the extra compat string out of the kernel 
proper, as then it'll never be enshrined as a RISC-V standard.

>> > Normally this would have an SoC specific compatible too. Sometimes we
>> > can get away without, but it doesn't seem like the PLIC is very tightly
>> > specified nor has common implementations.
>>
>> It is a giant f***cking mess to be honest.  Adding a highlevel spec
>> to the ISA but not a register layout is completely idotic, but if you
>> look at the current riscv-sw list this decision is still defended by
>> SiFive / the RISC-V foundation.  The whole stale of the RISC-V platform
>> Ecosystem is rather pathetic unfortunately, and people don't seem to
>> be willing to learn from past good practice nor mistakes in ARM land.
>
> Interrupt controllers are where the differentiation is. ;)

Again, sorry about that :).  The RISC-V platform specification really should 
have started a year ago, but I'm afraid there's just a bit too much going on on 
my end.

If it helps any, we just submitted a plumbers dev room with one topic being the 
RISC-V platform spec, so I guess I'm in official trouble now it there isn't at 
least something to talk about by November...

Re: RISC-V irqchip drivers

[Palmer Dabbelt]

On Wed, 25 Jul 2018 02:36:43 PDT (-0700), Christoph Hellwig wrote:
> The RISC-V ISA mandantes the presence of a simple, per-hart (hardware
> thread) interrupt controller availiable to supervisor mode.  In addition
> the RISC-V specification contains the definition of a programmable
> interrupt controller that is present on all current RISC-V cores (at
> least as far as a I know).
>
> This series adds both of them.  For the per-hart controller this series
> tries to address all comments vs the last posting from Palmr in June,
> and for the PLIC it has a lot of cleanups which I think should address
> all outstanding comments, but it has been a while since it was last
> posted.
>
> Without these irqchip drivers the RISC-V port in mainline is rather
> useless as it can't boot on any SOC or emulator.  With it it still is
> almost as useless as a clocksource driver is still missing, but at least
> we're only a patch or two away from a booting system, and the
> clocksource  driver will need the per-hart interrupt driver to work as
> well.
>
> Palmer: I assume you are ok with me pushing this forward.  If not I'll
> happily drop this series.

Thanks for taking this over, and sorry I've dropped the ball a bit here -- 
there's just a bit too much to do!

> A git tree with the patches in this series, the missing clocksource
> driver a few pending patches to allow booting a RISC-V kernel in qemu
> is available here:
>
>     git://git.infradead.org/users/hch/riscv.git riscv-linux-4.18
>
> Gitweb:
>
>     http://git.infradead.org/users/hch/riscv.git/shortlog/refs/heads/riscv-linux-4.18