[PATCH 15/18] irqchip/apple-aic: Add support for the Apple Interrupt Controller

[Arnd Bergmann]

On Thu, Feb 4, 2021 at 9:39 PM Hector Martin <marcan at marcan.st> wrote:
> +/*
> + * AIC is a fairly simple interrupt controller with the following features:
> + *
> + * - 896 level-triggered hardware IRQs
> + *   - Single mask bit per IRQ
> + *   - Per-IRQ affinity setting
> + *   - Automatic masking on event delivery (auto-ack)
> + *   - Software triggering (ORed with hw line)
> + * - 2 per-CPU IPIs (meant as "self" and "other", but they are interchangeable if not symmetric)
> + * - Automatic prioritization (single event/ack register per CPU, lower IRQs = higher priority)
> + * - Automatic masking on ack
> + * - Default "this CPU" register view and explicit per-CPU views
> + *
> + * In addition, this driver also handles FIQs, as these are routed to the same IRQ vector. These
> + * are used for Fast IPIs (TODO) and the ARMv8 timer IRQs.
> + *
> + * Implementation notes:
> + *
> + * - This driver creates one IRQ domain for HW IRQs and the timer FIQs
> + * - FIQ hwirq numbers are assigned after true hwirqs, and are per-cpu
> + * - DT bindings use 3-cell form (like GIC):
> + *   - <0 nr flags> - hwirq #nr
> + *   - <1 nr flags> - FIQ #nr
> + *     - nr=0  physical timer
> + *     - nr=1  virtual timer
> + *   - <2 nr flags> - IPI #nr
> + *     - nr=0  other IPI
> + *     - nr=1  self IPI

I think we should discuss the binding a bit here. My initial thinking was that
it would be better to separate the AIC from the FIQ handling, as they don't
seem to have any relation in hardware, and representing them as two
separate nodes seems like a cleaner abstraction.

> +#define TIMER_FIRING(x)                                                        \
> +       (((x) & (ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_MASK |            \
> +                ARCH_TIMER_CTRL_IT_STAT)) ==                                  \
> +        (ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_STAT))
> +
> +static void aic_handle_fiq(struct pt_regs *regs)
> +{
> +       /*
> +        * It would be really nice to find a system register that lets us get the FIQ source
> +        * state without having to peek down into clients...
> +        */
> +       if (TIMER_FIRING(read_sysreg(cntp_ctl_el0))) {
> +               handle_domain_irq(aic_irqc->hw_domain,
> +                                 aic_irqc->nr_hw + AIC_TMR_PHYS, regs);
> +       }
> +
> +       if (TIMER_FIRING(read_sysreg(cntv_ctl_el0))) {
> +               handle_domain_irq(aic_irqc->hw_domain,
> +                                 aic_irqc->nr_hw + AIC_TMR_VIRT, regs);
> +       }
> +}

This seems to be a minor layering violation to me.

One idea I had was to just keep all the fiq handling in the timer driver
itself, jumping there directly from the top-level fiq entry whenever
we are on an Apple platform. At least as long as nothing else ever
uses fiq.

When we discussed the earlier submission for the aic, I understood
that FIQ is used for both timer and IPI, but the IPI actually has another
method based on normal AIC interrupts that can be used as an
alternative.

> +static void __exception_irq_entry aic_handle_irq_or_fiq(struct pt_regs *regs)
> +{
> +       u64 isr = read_sysreg(isr_el1);
> +
> +       if (isr & PSR_F_BIT)
> +               aic_handle_fiq(regs);
> +
> +       if (isr & PSR_I_BIT)
> +               aic_handle_irq(regs);
> +}

Having the shared entry point here looks reasonable to me though, it
does seem to make a few things easier.

I wonder if there is a possible race here: if we are ever in a situation
where one of the two -- fiq or irq -- is disabled while the other one
is enabled, we could get into a state where a handler is run while
it should be masked.

       Arnd