[PATCH v4 0/2] J-Core interrupt controller support

[PATCH v4 0/2] J-Core interrupt controller support

[Rich Felker]

This driver is used with the J2, an open-source VHDL reimplementation
of SH-2.

I've split this out from the main J-Core support patches going
upstream through my own linux-sh tree. Changes since the v3 stem out
of reworking the DT to use separate reg ranges per cpu rather than
cpu-offset. The intent is to avoid out-of-mapping arithmetic via
cpu-offset and to avoid artifically restricting how the hardware lays
out the mappings, but this also allows the cpu notifier logic which
was present before to be removed with simple iteration over the reg
ranges present in its place.

Rich


Rich Felker (2):
  of: add J-Core interrupt controller bindings
  irqchip: add J-Core AIC driver

 .../bindings/interrupt-controller/jcore,aic.txt    | 26 +++++++
 drivers/irqchip/Kconfig                            |  6 ++
 drivers/irqchip/Makefile                           |  1 +
 drivers/irqchip/irq-jcore-aic.c                    | 79 ++++++++++++++++++++++
 4 files changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
 create mode 100644 drivers/irqchip/irq-jcore-aic.c

-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/2] of: add J-Core interrupt controller bindings

[Rich Felker]

Signed-off-by: Rich Felker <dalias@libc.org>
---
 .../bindings/interrupt-controller/jcore,aic.txt    | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
new file mode 100644
index 0000000..b7a56ad
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
@@ -0,0 +1,26 @@
+J-Core Advanced Interrupt Controller
+
+Required properties:
+
+- compatible: Should be "jcore,aic1" for the (obsolete) first-generation aic
+  with 8 interrupt lines with programmable priorities, or "jcore,aic2" for
+  the "aic2" core with 64 interrupts.
+
+- reg: Memory region(s) for configuration. For SMP, there should be one
+  region per cpu, indexed by the sequential, zero-based hardware cpu
+  number (which is also the logical cpu number).
+
+- interrupt-controller: Identifies the node as an interrupt controller
+
+- #interrupt-cells: Specifies the number of cells needed to encode an
+  interrupt source. The value shall be 1.
+
+
+Example:
+
+aic: interrupt-controller@200 {
+	compatible = "jcore,aic2";
+	reg = < 0x200 0x30 0x500 0x30 >;
+	interrupt-controller;
+	#interrupt-cells = <1>;
+};
-- 
2.8.1

[PATCH v4 2/2] irqchip: add J-Core AIC driver

[Rich Felker]

There are two versions of the J-Core interrupt controller in use, aic1
which generates interrupts with programmable priorities, but only
supports 8 irq lines and maps them to cpu traps in the range 17 to 24,
and aic2 which uses traps in the range 64-127 and supports up to 128
irqs, with priorities dependent on the interrupt number. The Linux
driver does not make use of priorities anyway.

For simplicity, there is no aic1-specific logic in the driver beyond
setting the priority register, which is necessary for interrupts to
work at all. Eventually aic1 will likely be phased out, but it's
currently in use in deployments and all released bitstream binaries.

Signed-off-by: Rich Felker <dalias@libc.org>
---
 drivers/irqchip/Kconfig         |  6 ++++
 drivers/irqchip/Makefile        |  1 +
 drivers/irqchip/irq-jcore-aic.c | 79 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+)
 create mode 100644 drivers/irqchip/irq-jcore-aic.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index fa33c50..fe58177 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -150,6 +150,12 @@ config PIC32_EVIC
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
 
+config JCORE_AIC
+	bool "J-Core integrated AIC"
+	select IRQ_DOMAIN
+	help
+	  Support for the J-Core integrated AIC.
+
 config RENESAS_INTC_IRQPIN
 	bool
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 38853a1..5b1a2fa 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_I8259)			+= irq-i8259.o
 obj-$(CONFIG_IMGPDC_IRQ)		+= irq-imgpdc.o
 obj-$(CONFIG_IRQ_MIPS_CPU)		+= irq-mips-cpu.o
 obj-$(CONFIG_SIRF_IRQ)			+= irq-sirfsoc.o
+obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
 obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
 obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
 obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
diff --git a/drivers/irqchip/irq-jcore-aic.c b/drivers/irqchip/irq-jcore-aic.c
new file mode 100644
index 0000000..7b3b30e
--- /dev/null
+++ b/drivers/irqchip/irq-jcore-aic.c
@@ -0,0 +1,79 @@
+/*
+ * J-Core SoC AIC driver
+ *
+ * Copyright (C) 2015-2016 Smart Energy Instruments, Inc.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include <linux/irq.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/cpu.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#define AIC1_INTPRI 8
+
+static struct aic_data {
+	struct irq_chip chip;
+	struct irq_domain *domain;
+	struct notifier_block nb;
+} aic_data;
+
+static int aic_irqdomain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq)
+{
+	struct aic_data *aic = d->host_data;
+
+	irq_set_chip_data(irq, aic);
+	irq_set_chip_and_handler(irq, &aic->chip, handle_simple_irq);
+	irq_set_probe(irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops aic_irqdomain_ops = {
+	.map = aic_irqdomain_map,
+	.xlate = irq_domain_xlate_onecell,
+};
+
+static void noop(struct irq_data *data)
+{
+}
+
+int __init aic_irq_of_init(struct device_node *node, struct device_node *parent)
+{
+	struct aic_data *aic = &aic_data;
+	unsigned min_irq = 64;
+
+	pr_info("Initializing J-Core AIC\n");
+
+	if (!of_device_is_compatible(node, "jcore,aic2")) {
+		unsigned cpu;
+		for_each_possible_cpu(cpu) {
+			void __iomem *base = of_iomap(node, cpu);
+			if (!base)
+				continue;
+			pr_info("Local AIC1 enable for cpu %u at %p\n",
+				cpu, base + AIC1_INTPRI);
+			__raw_writel(0xffffffff, base + AIC1_INTPRI);
+		}
+		min_irq = 16;
+	}
+
+	aic->chip.name = node->name;
+	aic->chip.irq_mask = noop;
+	aic->chip.irq_unmask = noop;
+
+	aic->domain = irq_domain_add_linear(node, 128, &aic_irqdomain_ops, aic);
+	irq_create_strict_mappings(aic->domain, min_irq, min_irq, 128-min_irq);
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(jcore_aic2, "jcore,aic2", aic_irq_of_init);
+IRQCHIP_DECLARE(jcore_aic1, "jcore,aic1", aic_irq_of_init);
-- 
2.8.1

Re: [PATCH v4 1/2] of: add J-Core interrupt controller bindings

[Mark Rutland]

On Wed, Jul 27, 2016 at 05:35:09AM +0000, Rich Felker wrote:
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
>  .../bindings/interrupt-controller/jcore,aic.txt    | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> new file mode 100644
> index 0000000..b7a56ad
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> @@ -0,0 +1,26 @@
> +J-Core Advanced Interrupt Controller
> +
> +Required properties:
> +
> +- compatible: Should be "jcore,aic1" for the (obsolete) first-generation aic
> +  with 8 interrupt lines with programmable priorities, or "jcore,aic2" for
> +  the "aic2" core with 64 interrupts.
> +
> +- reg: Memory region(s) for configuration. For SMP, there should be one
> +  region per cpu, indexed by the sequential, zero-based hardware cpu
> +  number (which is also the logical cpu number).

Nit: remove the bit about the logical number. That's a linux detail that
doesn't belong in the binding.

> +- interrupt-controller: Identifies the node as an interrupt controller
> +
> +- #interrupt-cells: Specifies the number of cells needed to encode an
> +  interrupt source. The value shall be 1.

... where the value encoded in that cell is?

I guess it's the zero-based index of the interrupt?

No flags? Can the AIC only do one trigger type?

Thanks,
Mark.

Re: [PATCH v4 2/2] irqchip: add J-Core AIC driver

[Mark Rutland]

On Wed, Jul 27, 2016 at 05:35:09AM +0000, Rich Felker wrote:
> +int __init aic_irq_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	struct aic_data *aic = &aic_data;
> +	unsigned min_irq = 64;
> +
> +	pr_info("Initializing J-Core AIC\n");
> +
> +	if (!of_device_is_compatible(node, "jcore,aic2")) {
> +		unsigned cpu;
> +		for_each_possible_cpu(cpu) {
> +			void __iomem *base = of_iomap(node, cpu);
> +			if (!base)
> +				continue;

This sounds like it would be a critical error.

It would be best to at least pr_warn() if you can't map a CPU's AI
interface.

> +			pr_info("Local AIC1 enable for cpu %u at %p\n",
> +				cpu, base + AIC1_INTPRI);
> +			__raw_writel(0xffffffff, base + AIC1_INTPRI);
> +		}

Here base goes out of scope. If you don't need it, it would be best
practice to iounmap it (even if that happens to be a no-op on your
arch).

> +		min_irq = 16;
> +	}
> +
> +	aic->chip.name = node->name;

It's probably best to give the name explicitly in the driver (e.g.
"AIC"), rather than taknig whatever happens to be in the DT (which
should be 'interrupt-controller@<addr>'.

> +	aic->chip.irq_mask = noop;
> +	aic->chip.irq_unmask = noop;

If the core code wants to mask IRQs, how do you handle that? Can you
mask your CPU traps?

Thanks,
Mark.

Re: [PATCH v4 2/2] irqchip: add J-Core AIC driver

[Mark Rutland]

On Wed, Jul 27, 2016 at 05:35:09AM +0000, Rich Felker wrote:
> For simplicity, there is no aic1-specific logic in the driver beyond
> setting the priority register, which is necessary for interrupts to
> work at all. Eventually aic1 will likely be phased out, but it's
> currently in use in deployments and all released bitstream binaries.

[...]

> +	if (!of_device_is_compatible(node, "jcore,aic2")) {

If this is only meant to run for AIC1, it would be better to check for
the "jcore,aic1" compatible string explicitly.

While that shouldn't matter much currently, it better matches the intent
described in the commit message, and avoids surprises and/or churn in
future if you have AIC3+.

Mark.

> +IRQCHIP_DECLARE(jcore_aic2, "jcore,aic2", aic_irq_of_init);
> +IRQCHIP_DECLARE(jcore_aic1, "jcore,aic1", aic_irq_of_init);
> -- 
> 2.8.1
> 

Re: [PATCH v4 1/2] of: add J-Core interrupt controller bindings

[Rich Felker]

On Wed, Jul 27, 2016 at 11:05:05AM +0100, Mark Rutland wrote:
> On Wed, Jul 27, 2016 at 05:35:09AM +0000, Rich Felker wrote:
> > Signed-off-by: Rich Felker <dalias@libc.org>
> > ---
> >  .../bindings/interrupt-controller/jcore,aic.txt    | 26 ++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> > new file mode 100644
> > index 0000000..b7a56ad
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt
> > @@ -0,0 +1,26 @@
> > +J-Core Advanced Interrupt Controller
> > +
> > +Required properties:
> > +
> > +- compatible: Should be "jcore,aic1" for the (obsolete) first-generation aic
> > +  with 8 interrupt lines with programmable priorities, or "jcore,aic2" for
> > +  the "aic2" core with 64 interrupts.
> > +
> > +- reg: Memory region(s) for configuration. For SMP, there should be one
> > +  region per cpu, indexed by the sequential, zero-based hardware cpu
> > +  number (which is also the logical cpu number).
> 
> Nit: remove the bit about the logical number. That's a linux detail that
> doesn't belong in the binding.

OK.

> > +- interrupt-controller: Identifies the node as an interrupt controller
> > +
> > +- #interrupt-cells: Specifies the number of cells needed to encode an
> > +  interrupt source. The value shall be 1.
> 
> .... where the value encoded in that cell is?
> 
> I guess it's the zero-based index of the interrupt?
> 
> No flags? Can the AIC only do one trigger type?

This was asked before. The AIC does all the interrupt control (aside
from the cpu's main imask status) and does not expose trigger types or
require any kind of ack/eoi from the software. If you'd like to know
more about the motivations behind how it works I could ask on the
j-core list but for the purpose of the bindings and driver I'm just
documenting/supporting what the existing hw design does.

Rich

Re: [PATCH v4 2/2] irqchip: add J-Core AIC driver

[Rich Felker]

On Wed, Jul 27, 2016 at 11:12:36AM +0100, Mark Rutland wrote:
> On Wed, Jul 27, 2016 at 05:35:09AM +0000, Rich Felker wrote:
> > +int __init aic_irq_of_init(struct device_node *node, struct device_node *parent)
> > +{
> > +	struct aic_data *aic = &aic_data;
> > +	unsigned min_irq = 64;
> > +
> > +	pr_info("Initializing J-Core AIC\n");
> > +
> > +	if (!of_device_is_compatible(node, "jcore,aic2")) {
> > +		unsigned cpu;
> > +		for_each_possible_cpu(cpu) {
> > +			void __iomem *base = of_iomap(node, cpu);
> > +			if (!base)
> > +				continue;
> 
> This sounds like it would be a critical error.
> 
> It would be best to at least pr_warn() if you can't map a CPU's AI
> interface.

It's looping over possible cpus (per the kernel configuration for max
cpus) so it's expected that a system with fewer cpus will also have
fewer reg ranges for the aic. This is not an error. If you think
there's a different/better way I should write this code, I'm open to
suggestions.

> > +			pr_info("Local AIC1 enable for cpu %u at %p\n",
> > +				cpu, base + AIC1_INTPRI);
> > +			__raw_writel(0xffffffff, base + AIC1_INTPRI);
> > +		}
> 
> Here base goes out of scope. If you don't need it, it would be best
> practice to iounmap it (even if that happens to be a no-op on your
> arch).

OK. I can add that.

> > +		min_irq = 16;
> > +	}
> > +
> > +	aic->chip.name = node->name;
> 
> It's probably best to give the name explicitly in the driver (e.g.
> "AIC"), rather than taknig whatever happens to be in the DT (which
> should be 'interrupt-controller@<addr>'.

OK.

> > +	aic->chip.irq_mask = noop;
> > +	aic->chip.irq_unmask = noop;
> 
> If the core code wants to mask IRQs, how do you handle that? Can you
> mask your CPU traps?

There's a global imask in the cpu that masks all interrupts that's
used in the trap entry point, spinlocks, etc. already. This is a cpu
standard feature and not logically part of the AIC. My understanding
is that the kernel already keeps a logical mask of disabled irqs in
addition to mask/disable at the irqchip level so there's a fairly fast
path for ignoring/holding (potentially spurious) irqs while they're
supposed to be disabled and deferring them until they're enabled
again.

Rich
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] irqchip: add J-Core AIC driver

[Rich Felker]

On Wed, Jul 27, 2016 at 11:15:38AM +0100, Mark Rutland wrote:
> On Wed, Jul 27, 2016 at 05:35:09AM +0000, Rich Felker wrote:
> > For simplicity, there is no aic1-specific logic in the driver beyond
> > setting the priority register, which is necessary for interrupts to
> > work at all. Eventually aic1 will likely be phased out, but it's
> > currently in use in deployments and all released bitstream binaries.
> 
> [...]
> 
> > +	if (!of_device_is_compatible(node, "jcore,aic2")) {
> 
> If this is only meant to run for AIC1, it would be better to check for
> the "jcore,aic1" compatible string explicitly.
> 
> While that shouldn't matter much currently, it better matches the intent
> described in the commit message, and avoids surprises and/or churn in
> future if you have AIC3+.

My intent in doing this was to support a DT that might claim an aic2
is aic1-compatible as a fallback "compatible" property. The hardware
is designed such that this works (ignoring the spurious writes to
unused prio registers) as long as the DT still has the right irq
numbers for attached devices. I'm not sure we actually need to do that
for compatibility with any existing software but I thought it better
not to preclude it.

Rich
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] irqchip: add J-Core AIC driver

[Mark Rutland]

On Wed, Jul 27, 2016 at 09:06:06AM -0400, Rich Felker wrote:
> On Wed, Jul 27, 2016 at 11:12:36AM +0100, Mark Rutland wrote:
> > On Wed, Jul 27, 2016 at 05:35:09AM +0000, Rich Felker wrote:
> > > +int __init aic_irq_of_init(struct device_node *node, struct device_node *parent)
> > > +{
> > > +	struct aic_data *aic = &aic_data;
> > > +	unsigned min_irq = 64;
> > > +
> > > +	pr_info("Initializing J-Core AIC\n");
> > > +
> > > +	if (!of_device_is_compatible(node, "jcore,aic2")) {
> > > +		unsigned cpu;
> > > +		for_each_possible_cpu(cpu) {
> > > +			void __iomem *base = of_iomap(node, cpu);
> > > +			if (!base)
> > > +				continue;
> > 
> > This sounds like it would be a critical error.
> > 
> > It would be best to at least pr_warn() if you can't map a CPU's AI
> > interface.
> 
> It's looping over possible cpus (per the kernel configuration for max
> cpus) so it's expected that a system with fewer cpus will also have
> fewer reg ranges for the aic. This is not an error. If you think
> there's a different/better way I should write this code, I'm open to
> suggestions.

In your arch code, set possible cpus based on the DT, before
initialising irqchips. i.e. mark any CPUs not in the DT as not possible.

That will also net you savings in other areas (e.g. per-cpu maps not
having to be allocated for CPUs which don't exist).

Otherwise, you're missing real error cases, e.g. two CPUs with only one
AIC region.

> > > +	aic->chip.irq_mask = noop;
> > > +	aic->chip.irq_unmask = noop;
> > 
> > If the core code wants to mask IRQs, how do you handle that? Can you
> > mask your CPU traps?
> 
> There's a global imask in the cpu that masks all interrupts that's
> used in the trap entry point, spinlocks, etc. already. This is a cpu
> standard feature and not logically part of the AIC.

Just to check, is that a single bit that masks all IRQs, or is there a
mark per-IRQ?

> My understanding is that the kernel already keeps a logical mask of
> disabled irqs in addition to mask/disable at the irqchip level so
> there's a fairly fast path for ignoring/holding (potentially spurious)
> irqs while they're supposed to be disabled and deferring them until
> they're enabled again.

While we can ignore suprious IRQs, there are cases where that's
insufficient (e.g. screaming interrupts, suspend).

Can your CPU mask IRQs individually?

Thanks,
Mark.

Re: [PATCH v4 2/2] irqchip: add J-Core AIC driver

[Mark Rutland]

On Wed, Jul 27, 2016 at 09:08:21AM -0400, Rich Felker wrote:
> On Wed, Jul 27, 2016 at 11:15:38AM +0100, Mark Rutland wrote:
> > On Wed, Jul 27, 2016 at 05:35:09AM +0000, Rich Felker wrote:
> > > For simplicity, there is no aic1-specific logic in the driver beyond
> > > setting the priority register, which is necessary for interrupts to
> > > work at all. Eventually aic1 will likely be phased out, but it's
> > > currently in use in deployments and all released bitstream binaries.
> > 
> > [...]
> > 
> > > +	if (!of_device_is_compatible(node, "jcore,aic2")) {
> > 
> > If this is only meant to run for AIC1, it would be better to check for
> > the "jcore,aic1" compatible string explicitly.
> > 
> > While that shouldn't matter much currently, it better matches the intent
> > described in the commit message, and avoids surprises and/or churn in
> > future if you have AIC3+.
> 
> My intent in doing this was to support a DT that might claim an aic2
> is aic1-compatible as a fallback "compatible" property. The hardware
> is designed such that this works (ignoring the spurious writes to
> unused prio registers) as long as the DT still has the right irq
> numbers for attached devices.

Ok.

If the HW ignores it, what's the cost of those one-off spurious writes?
If it's not noticeable, you could allow the kernel to perform them
regardless.

Otherwise, please add a comment above the check, explaining why we do
the check this way around.

Thanks,
Mark.

Re: [PATCH v4 2/2] irqchip: add J-Core AIC driver

[Rich Felker]

On Wed, Jul 27, 2016 at 02:22:52PM +0100, Mark Rutland wrote:
> On Wed, Jul 27, 2016 at 09:06:06AM -0400, Rich Felker wrote:
> > On Wed, Jul 27, 2016 at 11:12:36AM +0100, Mark Rutland wrote:
> > > On Wed, Jul 27, 2016 at 05:35:09AM +0000, Rich Felker wrote:
> > > > +int __init aic_irq_of_init(struct device_node *node, struct device_node *parent)
> > > > +{
> > > > +	struct aic_data *aic = &aic_data;
> > > > +	unsigned min_irq = 64;
> > > > +
> > > > +	pr_info("Initializing J-Core AIC\n");
> > > > +
> > > > +	if (!of_device_is_compatible(node, "jcore,aic2")) {
> > > > +		unsigned cpu;
> > > > +		for_each_possible_cpu(cpu) {
> > > > +			void __iomem *base = of_iomap(node, cpu);
> > > > +			if (!base)
> > > > +				continue;
> > > 
> > > This sounds like it would be a critical error.
> > > 
> > > It would be best to at least pr_warn() if you can't map a CPU's AI
> > > interface.
> > 
> > It's looping over possible cpus (per the kernel configuration for max
> > cpus) so it's expected that a system with fewer cpus will also have
> > fewer reg ranges for the aic. This is not an error. If you think
> > there's a different/better way I should write this code, I'm open to
> > suggestions.
> 
> In your arch code, set possible cpus based on the DT, before
> initialising irqchips. i.e. mark any CPUs not in the DT as not possible.
> 
> That will also net you savings in other areas (e.g. per-cpu maps not
> having to be allocated for CPUs which don't exist).

Should it be done for possible or just present? I think the existing
code already sets both possible and present true if and only if they
have cpu nodes, so it should work as-is with either.

> Otherwise, you're missing real error cases, e.g. two CPUs with only one
> AIC region.

Sure, but do invalid DTBs need to be a diagnosable error? An invalid
DTB can clearly cause the system to blow up arbitrarily badly with
wrong memory regions, etc.

> > > > +	aic->chip.irq_mask = noop;
> > > > +	aic->chip.irq_unmask = noop;
> > > 
> > > If the core code wants to mask IRQs, how do you handle that? Can you
> > > mask your CPU traps?
> > 
> > There's a global imask in the cpu that masks all interrupts that's
> > used in the trap entry point, spinlocks, etc. already. This is a cpu
> > standard feature and not logically part of the AIC.
> 
> Just to check, is that a single bit that masks all IRQs, or is there a
> mark per-IRQ?

It's actually a 4-bit priority mask that masks all interrupts with
priority <= the configured value, but Linux has no use for interrupt
priorities and the kernel just sets the value to 0 or 15 for allowing
or blocking interrupts

> > My understanding is that the kernel already keeps a logical mask of
> > disabled irqs in addition to mask/disable at the irqchip level so
> > there's a fairly fast path for ignoring/holding (potentially spurious)
> > irqs while they're supposed to be disabled and deferring them until
> > they're enabled again.
> 
> While we can ignore suprious IRQs, there are cases where that's
> insufficient (e.g. screaming interrupts, suspend).
> 
> Can your CPU mask IRQs individually?

No. If there's SoC hardware needing that capability it would need to
be added, but I suspect off-SoC hardware generating interrupts might
want to use a secondary chained interrupt controller anyway, which
might have different functionality.

Rich

Re: [PATCH v4 2/2] irqchip: add J-Core AIC driver

[Rich Felker]

On Wed, Jul 27, 2016 at 02:27:54PM +0100, Mark Rutland wrote:
> On Wed, Jul 27, 2016 at 09:08:21AM -0400, Rich Felker wrote:
> > On Wed, Jul 27, 2016 at 11:15:38AM +0100, Mark Rutland wrote:
> > > On Wed, Jul 27, 2016 at 05:35:09AM +0000, Rich Felker wrote:
> > > > For simplicity, there is no aic1-specific logic in the driver beyond
> > > > setting the priority register, which is necessary for interrupts to
> > > > work at all. Eventually aic1 will likely be phased out, but it's
> > > > currently in use in deployments and all released bitstream binaries.
> > > 
> > > [...]
> > > 
> > > > +	if (!of_device_is_compatible(node, "jcore,aic2")) {
> > > 
> > > If this is only meant to run for AIC1, it would be better to check for
> > > the "jcore,aic1" compatible string explicitly.
> > > 
> > > While that shouldn't matter much currently, it better matches the intent
> > > described in the commit message, and avoids surprises and/or churn in
> > > future if you have AIC3+.
> > 
> > My intent in doing this was to support a DT that might claim an aic2
> > is aic1-compatible as a fallback "compatible" property. The hardware
> > is designed such that this works (ignoring the spurious writes to
> > unused prio registers) as long as the DT still has the right irq
> > numbers for attached devices.
> 
> Ok.
> 
> If the HW ignores it, what's the cost of those one-off spurious writes?
> If it's not noticeable, you could allow the kernel to perform them
> regardless.

Indeed, it essentially costs nothing. My motivation was more just to
document that it's not needed/used for aic2.

> Otherwise, please add a comment above the check, explaining why we do
> the check this way around.

Since the intent is documenting this might be the best approach.

Rich

Re: [PATCH v4 2/2] irqchip: add J-Core AIC driver

[Mark Rutland]

On Wed, Jul 27, 2016 at 01:07:09PM -0400, Rich Felker wrote:
> On Wed, Jul 27, 2016 at 02:22:52PM +0100, Mark Rutland wrote:
> > On Wed, Jul 27, 2016 at 09:06:06AM -0400, Rich Felker wrote:
> > > On Wed, Jul 27, 2016 at 11:12:36AM +0100, Mark Rutland wrote:
> > > > On Wed, Jul 27, 2016 at 05:35:09AM +0000, Rich Felker wrote:
> > > > > +int __init aic_irq_of_init(struct device_node *node, struct device_node *parent)
> > > > > +{
> > > > > +	struct aic_data *aic = &aic_data;
> > > > > +	unsigned min_irq = 64;
> > > > > +
> > > > > +	pr_info("Initializing J-Core AIC\n");
> > > > > +
> > > > > +	if (!of_device_is_compatible(node, "jcore,aic2")) {
> > > > > +		unsigned cpu;
> > > > > +		for_each_possible_cpu(cpu) {
> > > > > +			void __iomem *base = of_iomap(node, cpu);
> > > > > +			if (!base)
> > > > > +				continue;
> > > > 
> > > > This sounds like it would be a critical error.
> > > > 
> > > > It would be best to at least pr_warn() if you can't map a CPU's AI
> > > > interface.
> > > 
> > > It's looping over possible cpus (per the kernel configuration for max
> > > cpus) so it's expected that a system with fewer cpus will also have
> > > fewer reg ranges for the aic. This is not an error. If you think
> > > there's a different/better way I should write this code, I'm open to
> > > suggestions.
> > 
> > In your arch code, set possible cpus based on the DT, before
> > initialising irqchips. i.e. mark any CPUs not in the DT as not possible.
> > 
> > That will also net you savings in other areas (e.g. per-cpu maps not
> > having to be allocated for CPUs which don't exist).
> 
> Should it be done for possible or just present? I think the existing
> code already sets both possible and present true if and only if they
> have cpu nodes, so it should work as-is with either.

You'll want possible to be clear for the per-cpu maps case. 

I guess for the interrupt controller either is appropriate.

> > Otherwise, you're missing real error cases, e.g. two CPUs with only one
> > AIC region.
> 
> Sure, but do invalid DTBs need to be a diagnosable error?

To the extent that we can reasonably detect such issues, we should do
so. This case is trivial to detect, so I think we should do so.

This sort of logic helps to catch issues far earlier (e.g. when people
write their first DTs), and saves a fair amount of pain later down the
line.

> An invalid DTB can clearly cause the system to blow up arbitrarily
> badly with wrong memory regions, etc.

This is true, but in many cases such as with bad memory ranges we don't
have another source of truth to compare against, whereas we do for the
set of CPUs present/possible.

> > > > > +	aic->chip.irq_mask = noop;
> > > > > +	aic->chip.irq_unmask = noop;
> > > > 
> > > > If the core code wants to mask IRQs, how do you handle that? Can you
> > > > mask your CPU traps?
> > > 
> > > There's a global imask in the cpu that masks all interrupts that's
> > > used in the trap entry point, spinlocks, etc. already. This is a cpu
> > > standard feature and not logically part of the AIC.
> > 
> > Just to check, is that a single bit that masks all IRQs, or is there a
> > mark per-IRQ?
> 
> It's actually a 4-bit priority mask that masks all interrupts with
> priority <= the configured value, but Linux has no use for interrupt
> priorities and the kernel just sets the value to 0 or 15 for allowing
> or blocking interrupts

Ok.

IIUC, that means you *could* implement per-irq masking by having the
CPU's mask value set to 0, and flipping the priority of an IRQ between 0
and 1 to disable/enable.

Though from your prior comments it sounds like for AIC2 writes to the
MMIO priority registers are ignored, so that would not work for AIC2?

> > > My understanding is that the kernel already keeps a logical mask of
> > > disabled irqs in addition to mask/disable at the irqchip level so
> > > there's a fairly fast path for ignoring/holding (potentially spurious)
> > > irqs while they're supposed to be disabled and deferring them until
> > > they're enabled again.
> > 
> > While we can ignore suprious IRQs, there are cases where that's
> > insufficient (e.g. screaming interrupts, suspend).
> > 
> > Can your CPU mask IRQs individually?
> 
> No. If there's SoC hardware needing that capability it would need to
> be added, but I suspect off-SoC hardware generating interrupts might
> want to use a secondary chained interrupt controller anyway, which
> might have different functionality.

I was under the impression that the core IRQ code expected irq_mask and
irq_unmask to do what their names imply. I would be worried about what
might depend on that functionality.

I am far from an expert in that area, so I'll leave that to Marc,
Thomas, and Jason to comment on that.

Thanks,
Mark.

Re: [PATCH v4 2/2] irqchip: add J-Core AIC driver

[Rich Felker]

On Wed, Jul 27, 2016 at 06:31:52PM +0100, Mark Rutland wrote:
> > > > It's looping over possible cpus (per the kernel configuration for max
> > > > cpus) so it's expected that a system with fewer cpus will also have
> > > > fewer reg ranges for the aic. This is not an error. If you think
> > > > there's a different/better way I should write this code, I'm open to
> > > > suggestions.
> > > 
> > > In your arch code, set possible cpus based on the DT, before
> > > initialising irqchips. i.e. mark any CPUs not in the DT as not possible.
> > > 
> > > That will also net you savings in other areas (e.g. per-cpu maps not
> > > having to be allocated for CPUs which don't exist).
> > 
> > Should it be done for possible or just present? I think the existing
> > code already sets both possible and present true if and only if they
> > have cpu nodes, so it should work as-is with either.
> 
> You'll want possible to be clear for the per-cpu maps case. 
> 
> I guess for the interrupt controller either is appropriate.

OK. Semantically I should probably change it to present, but I agree
it doesn't matter much right now and it's a minor detail.

> > > Otherwise, you're missing real error cases, e.g. two CPUs with only one
> > > AIC region.
> > 
> > Sure, but do invalid DTBs need to be a diagnosable error?
> 
> To the extent that we can reasonably detect such issues, we should do
> so. This case is trivial to detect, so I think we should do so.
> 
> This sort of logic helps to catch issues far earlier (e.g. when people
> write their first DTs), and saves a fair amount of pain later down the
> line.

OK.

> > > > > > +	aic->chip.irq_mask = noop;
> > > > > > +	aic->chip.irq_unmask = noop;
> > > > > 
> > > > > If the core code wants to mask IRQs, how do you handle that? Can you
> > > > > mask your CPU traps?
> > > > 
> > > > There's a global imask in the cpu that masks all interrupts that's
> > > > used in the trap entry point, spinlocks, etc. already. This is a cpu
> > > > standard feature and not logically part of the AIC.
> > > 
> > > Just to check, is that a single bit that masks all IRQs, or is there a
> > > mark per-IRQ?
> > 
> > It's actually a 4-bit priority mask that masks all interrupts with
> > priority <= the configured value, but Linux has no use for interrupt
> > priorities and the kernel just sets the value to 0 or 15 for allowing
> > or blocking interrupts
> 
> Ok.
> 
> IIUC, that means you *could* implement per-irq masking by having the
> CPU's mask value set to 0, and flipping the priority of an IRQ between 0
> and 1 to disable/enable.
> 
> Though from your prior comments it sounds like for AIC2 writes to the
> MMIO priority registers are ignored, so that would not work for AIC2?

Right. The register with 8 4-bit fields only made sense for the setup
with 8 irq lines with variable priority; the aic2 has 64 lines with
static priorities.

> > > > My understanding is that the kernel already keeps a logical mask of
> > > > disabled irqs in addition to mask/disable at the irqchip level so
> > > > there's a fairly fast path for ignoring/holding (potentially spurious)
> > > > irqs while they're supposed to be disabled and deferring them until
> > > > they're enabled again.
> > > 
> > > While we can ignore suprious IRQs, there are cases where that's
> > > insufficient (e.g. screaming interrupts, suspend).
> > > 
> > > Can your CPU mask IRQs individually?
> > 
> > No. If there's SoC hardware needing that capability it would need to
> > be added, but I suspect off-SoC hardware generating interrupts might
> > want to use a secondary chained interrupt controller anyway, which
> > might have different functionality.
> 
> I was under the impression that the core IRQ code expected irq_mask and
> irq_unmask to do what their names imply. I would be worried about what
> might depend on that functionality.
> 
> I am far from an expert in that area, so I'll leave that to Marc,
> Thomas, and Jason to comment on that.

I'm a little bit confused about the intended requirements too.
kernel/irq/chip.c contains both code that tests if the chip->irq_mask
pointer is non-null (in mask_irq) and code that assumes chip->irq_mask
is non-null if chip->irq_disable is null (e.g. in irq_percpu_disable).
This suggests to me that irq_mask can be null/unsupported and that
irq_disable can be, but I'm not sure which should be defined when
there's no such operation for either, and I don't see anywhere it's
documented what's required. The comment for irq_disable even has a
typo and repeats the "If the chip does not implement the irq_disable
callback" condition for both cases whereas presumably it's only
intended to apply to the second.

Anyway the current code seems to work fine but it would be nice to
know what it's "supposed" to do.

Rich

Re: [PATCH v4 2/2] irqchip: add J-Core AIC driver

[Mark Rutland]

On Wed, Jul 27, 2016 at 07:01:24PM -0400, Rich Felker wrote:
> On Wed, Jul 27, 2016 at 06:31:52PM +0100, Mark Rutland wrote:

> > IIUC, that means you *could* implement per-irq masking by having the
> > CPU's mask value set to 0, and flipping the priority of an IRQ between 0
> > and 1 to disable/enable.
> > 
> > Though from your prior comments it sounds like for AIC2 writes to the
> > MMIO priority registers are ignored, so that would not work for AIC2?
> 
> Right. The register with 8 4-bit fields only made sense for the setup
> with 8 irq lines with variable priority; the aic2 has 64 lines with
> static priorities.

Thinking about this a little further, this is a good argument for the
"jcore,aic1" *not* being a valid fallback entry in an AIC2 compatible
list. Anything wanting to rely on this behaviour of AIC1 would be broken
on AIC2.

That being the case, no DT should have "jcore,aic1" for an AIC2 node,
and we can explciitly check for the AIC1 string for AIC1-specific
initialisation code (though regardless, it's worht a comment).

A further curiosity: what static priority values for AIC2 apply to
interrupts? Does it apply 0xf (or some over value) uniformly, or can
AIC2 interrupts potentially have varied priorities?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html