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:

quoted

On Wed, Jul 27, 2016 at 11:12:36AM +0100, Mark Rutland wrote:

quoted

On Wed, Jul 27, 2016 at 05:35:09AM +0000, Rich Felker wrote:

quoted

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

quoted

quoted

quoted

+	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

quoted

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