J-Core interrupt controller support

Signed-off-by Rich Felker <[email protected]>

---
 .../bindings/interrupt-controller/jcore,aic.txt    | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +1 -0

new file mode 100644
index 0000000..b7a56ad
@@ -0,0 +1,26 @@
+J-Core Advanced Interrupt Controller

Quoted 18 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 .../bindings/interrupt-controller/jcore,aic.txt    | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +6 -0

new file mode 100644
index 0000000..b7a56ad
@@ -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

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt line 11

Quoted 5 lines

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +5 -0

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

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt line 16

Quoted 4 lines

Documentation/devicetree/bindings/interrupt-controller/jcore,aic.txt new file +4 -0

+- 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?

drivers/irqchip/irq-jcore-aic.c line 60

Quoted 13 lines · Rich Felker

drivers/irqchip/irq-jcore-aic.c new file +13 -0

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

drivers/irqchip/irq-jcore-aic.c line 64

Quoted 4 lines

drivers/irqchip/irq-jcore-aic.c new file +4 -0

+			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).

drivers/irqchip/irq-jcore-aic.c line 68

Quoted 4 lines

drivers/irqchip/irq-jcore-aic.c new file +4 -0

+		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>'.

drivers/irqchip/irq-jcore-aic.c line 70

Quoted 2 lines

drivers/irqchip/irq-jcore-aic.c new file +2 -0

+	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?

Quoted 17 lines · Mark Rutland

drivers/irqchip/irq-jcore-aic.c line 60

Quoted 13 lines · Rich Felker

drivers/irqchip/irq-jcore-aic.c new file +13 -0

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

Quoted 2 lines

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.

Quoted 5 lines

drivers/irqchip/irq-jcore-aic.c line 70

Quoted 2 lines

drivers/irqchip/irq-jcore-aic.c new file +2 -0

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

Quoted 18 lines · Rich Felker

Quoted 17 lines · Mark Rutland

drivers/irqchip/irq-jcore-aic.c line 60

Quoted 13 lines · Rich Felker

drivers/irqchip/irq-jcore-aic.c new file +13 -0

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 5 lines

drivers/irqchip/irq-jcore-aic.c line 70

Quoted 2 lines

drivers/irqchip/irq-jcore-aic.c new file +2 -0

+	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?

Quoted 3 lines

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?

Quoted 5 lines

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?

Quoted 31 lines · Mark Rutland

Quoted 18 lines · Rich Felker

Quoted 17 lines · Mark Rutland

drivers/irqchip/irq-jcore-aic.c line 60

Quoted 13 lines · Rich Felker

drivers/irqchip/irq-jcore-aic.c new file +13 -0

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

drivers/irqchip/irq-jcore-aic.c line 70

Quoted 2 lines

drivers/irqchip/irq-jcore-aic.c new file +2 -0

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

Quoted 2 lines

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

Quoted 9 lines

Quoted 5 lines

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

Quoted 1 line

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.

Quoted 12 lines

Quoted 11 lines

Quoted 5 lines

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

Quoted 3 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

drivers/irqchip/irq-jcore-aic.c line 70

Quoted 2 lines

drivers/irqchip/irq-jcore-aic.c new file +2 -0

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

Quoted 7 lines

Quoted 2 lines

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?

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 4 lines · Mark Rutland

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.

Quoted 2 lines

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.

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.

drivers/irqchip/irq-jcore-aic.c new file +2 -0

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

Signature · 1 line

2.8.1

Quoted 4 lines · Rich Felker

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.

[...]

drivers/irqchip/irq-jcore-aic.c line 55

Quoted 1 line

drivers/irqchip/irq-jcore-aic.c new file +1 -0

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

Quoted 8 lines · Mark Rutland

Quoted 4 lines · Rich Felker

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.

[...]

Quoted 8 lines

drivers/irqchip/irq-jcore-aic.c line 55

Quoted 1 line

drivers/irqchip/irq-jcore-aic.c new file +1 -0

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

Quoted 18 lines · Rich Felker

Quoted 8 lines · Mark Rutland

Quoted 4 lines · Rich Felker

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.

[...]

Quoted 8 lines

drivers/irqchip/irq-jcore-aic.c line 55

Quoted 1 line

drivers/irqchip/irq-jcore-aic.c new file +1 -0

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

Quoted 5 lines

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.