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

[Rich Felker <dalias@libc.org>]

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