Re: [PATCH v7 04/15] irqchip: RISC-V Local Interrupt Controller Driver

Re: [PATCH v7 04/15] irqchip: RISC-V Local Interrupt Controller Driver

[Rob Herring]

On Mon, Jul 31, 2017 at 7:59 PM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> 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>
> ---


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

What happened with the DT bindings? Those need to come first.

Rob

Re: [PATCH v7 04/15] irqchip: RISC-V Local Interrupt Controller Driver

[Thomas Gleixner]

On Mon, 31 Jul 2017, Palmer Dabbelt wrote:
> +static void riscv_software_interrupt(void)
> +{
> +#ifdef CONFIG_SMP
> +	irqreturn_t ret;
> +
> +	ret = handle_ipi();
> +
> +	WARN_ON(ret == IRQ_NONE);

  	WARN_ON(handle_ipi() == IRQ_NONE);

perhaps?

> +#else
> +	/*
> +	 * We currently only use software interrupts to pass inter-processor
> +	 * interrupts, so if a non-SMP system gets a software interrupt then we
> +	 * don't know what to do.
> +	 */
> +	pr_warning("Software Interrupt without CONFIG_SMP\n");
> +#endif
> +}


> +static void riscv_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))
> +		smp_call_function_single(data->hart,
> +					 riscv_irq_enable_helper,
> +					 d,
> +					 true);
> +	else
> +		WARN_ON_ONCE(1);

If you write a small helper:

static void riscv_remote_ctrl(unsigned int cpu, void (*fn)(void *d),
       	    		      struct irq_data *data)
{
	smp_call_function_single(cpu, cb, data, true);
}

Then both riscv_irq_enable() and riscv_irq_disable() become readable
functions.

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

Hmm?

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

This is really horrible to read. What's wrong with using the full 80 chars?

	data->domain = irq_domain_add_linear(node, 8 * sizeof(uintptr_t),
					     &riscv_irqdomain_ops, data);

> +	if (!data->domain)
> +		goto error_add_linear;
> +	pr_info("%s: %d local interrupts mapped\n",
> +	        data->name, 8*(int)sizeof(uintptr_t));

Can we please make that '8 * sizeof()' a constant and use it in both
places? Which makes the pr_info also fit into a single line.

> +	return 0;
> +
> +error_add_linear:
> +	pr_warning("%s: unable to add IRQ domain\n",
> +		   data->name);

Single line please. Enough room.

> +	return -(ENXIO);

No braces.

Thanks,

	tglx

Re: [PATCH v7 04/15] irqchip: RISC-V Local Interrupt Controller Driver

[Randy Dunlap]

On 07/31/2017 05:59 PM, Palmer Dabbelt wrote:
> 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 | 213 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 228 insertions(+)
>  create mode 100644 drivers/irqchip/irq-riscv-intc.c

> diff --git a/drivers/irqchip/irq-riscv-intc.c b/drivers/irqchip/irq-riscv-intc.c
> new file mode 100644
> index 000000000000..96ae020cf1d5
> --- /dev/null
> +++ b/drivers/irqchip/irq-riscv-intc.c
> @@ -0,0 +1,213 @@
> +/*
[]

> +
> +error_add_linear:
> +	pr_warning("%s: unable to add IRQ domain\n",
> +		   data->name);
> +	return -(ENXIO);
> +

  Why the parentheses around ENXIO? Is it some macro calculation?
  Otherwise just use
	return -ENXIO;
  and drop the following blank line.

> +}
> +
> +IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
> 


-- 
~Randy

[PATCH v7 04/15] 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 | 213 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 228 insertions(+)
 create mode 100644 drivers/irqchip/irq-riscv-intc.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index f1fd5f44d1d4..7923d3fa8fae 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -306,3 +306,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 e88d856cc09c..b1aa9114afc4 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -78,3 +78,4 @@ obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.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..96ae020cf1d5
--- /dev/null
+++ b/drivers/irqchip/irq-riscv-intc.c
@@ -0,0 +1,213 @@
+/*
+ * 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);
+
+static void riscv_software_interrupt(void)
+{
+#ifdef CONFIG_SMP
+	irqreturn_t ret;
+
+	ret = handle_ipi();
+
+	WARN_ON(ret == IRQ_NONE);
+#else
+	/*
+	 * We currently only use software interrupts to pass inter-processor
+	 * interrupts, so if a non-SMP system gets a software interrupt then we
+	 * don't know what to do.
+	 */
+	pr_warning("Software Interrupt without CONFIG_SMP\n");
+#endif
+}
+
+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);
+	irq_set_affinity(irq, cpumask_of(data->hart));
+
+	return 0;
+}
+
+static const struct irq_domain_ops riscv_irqdomain_ops = {
+	.map	= riscv_irqdomain_map,
+	.xlate	= irq_domain_xlate_onecell,
+};
+
+/*
+ * On RISC-V systems local interrupts are masked or unmasked by writing the SIE
+ * (Supervisor Interrupt Enable) CSR.  As CSRs can only be written on the local
+ * hart, these functions can only be called on the hart that corresponds to the
+ * IRQ chip.  They are only called internally to this module, so they BUG_ON if
+ * this condition is violated rather than attempting to handle the error by
+ * forwarding to the target hart, as that's already expected to have been done.
+ */
+static void riscv_irq_mask(struct irq_data *d)
+{
+	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
+
+	BUG_ON(smp_processor_id() != data->hart);
+	csr_clear(sie, 1 << (long)d->hwirq);
+}
+
+static void riscv_irq_unmask(struct irq_data *d)
+{
+	struct riscv_irq_data *data = irq_data_get_irq_chip_data(d);
+
+	BUG_ON(smp_processor_id() != data->hart);
+	csr_set(sie, 1 << (long)d->hwirq);
+}
+
+/* Callbacks for twiddling SIE on another hart. */
+static void riscv_irq_enable_helper(void *d)
+{
+	riscv_irq_unmask(d);
+}
+
+static void riscv_irq_disable_helper(void *d)
+{
+	riscv_irq_mask(d);
+}
+
+static void riscv_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))
+		smp_call_function_single(data->hart,
+					 riscv_irq_enable_helper,
+					 d,
+					 true);
+	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))
+		smp_call_function_single(data->hart,
+					 riscv_irq_disable_helper,
+					 d,
+					 true);
+	else
+		WARN_ON_ONCE(1);
+}
+
+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)
+		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,
+		8*sizeof(uintptr_t),
+		&riscv_irqdomain_ops,
+		data);
+	if (!data->domain)
+		goto error_add_linear;
+	pr_info("%s: %d local interrupts mapped\n",
+	        data->name, 8*(int)sizeof(uintptr_t));
+	return 0;
+
+error_add_linear:
+	pr_warning("%s: unable to add IRQ domain\n",
+		   data->name);
+	return -(ENXIO);
+
+}
+
+IRQCHIP_DECLARE(riscv, "riscv,cpu-intc", riscv_intc_init);
-- 
2.13.0