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

[Marc Zyngier]

Hi Hector,

On Fri, 02 Apr 2021 10:05:39 +0100,
Hector Martin <marcan at marcan.st> wrote:
> 
> This is the root interrupt controller used on Apple ARM SoCs such as the
> M1. This irqchip driver performs multiple functions:
> 
> * Handles both IRQs and FIQs
> 
> * Drives the AIC peripheral itself (which handles IRQs)
> 
> * Dispatches FIQs to downstream hard-wired clients (currently the ARM
>   timer).
> 
> * Implements a virtual IPI multiplexer to funnel multiple Linux IPIs
>   into a single hardware IPI
> 
> Signed-off-by: Hector Martin <marcan at marcan.st>
> ---
>  MAINTAINERS                     |   2 +
>  drivers/irqchip/Kconfig         |   8 +
>  drivers/irqchip/Makefile        |   1 +
>  drivers/irqchip/irq-apple-aic.c | 837 ++++++++++++++++++++++++++++++++
>  include/linux/cpuhotplug.h      |   1 +
>  5 files changed, 849 insertions(+)
>  create mode 100644 drivers/irqchip/irq-apple-aic.c

[...]

> +static int aic_irq_domain_translate(struct irq_domain *id,
> +				    struct irq_fwspec *fwspec,
> +				    unsigned long *hwirq,
> +				    unsigned int *type)
> +{
> +	struct aic_irq_chip *ic = id->host_data;
> +
> +	if (fwspec->param_count != 3 || !is_of_node(fwspec->fwnode))
> +		return -EINVAL;
> +
> +	switch (fwspec->param[0]) {
> +	case AIC_IRQ:
> +		if (fwspec->param[1] >= ic->nr_hw)
> +			return -EINVAL;
> +		*hwirq = fwspec->param[1];
> +		break;
> +	case AIC_FIQ:
> +		if (fwspec->param[1] >= AIC_NR_FIQ)
> +			return -EINVAL;
> +		*hwirq = ic->nr_hw + fwspec->param[1];
> +
> +		/*
> +		 * In EL1 the non-redirected registers are the guest's,
> +		 * not EL2's, so remap the hwirqs to match.
> +		 */
> +		if (!is_kernel_in_hyp_mode()) {
> +			switch (fwspec->param[1]) {
> +			case AIC_TMR_GUEST_PHYS:
> +				*hwirq = ic->nr_hw + AIC_TMR_HV_PHYS;
> +				break;
> +			case AIC_TMR_GUEST_VIRT:
> +				*hwirq = ic->nr_hw + AIC_TMR_HV_VIRT;
> +				break;
> +			case AIC_TMR_HV_PHYS:
> +			case AIC_TMR_HV_VIRT:
> +				return -ENOENT;
> +			default:
> +				break;
> +			}
> +		}

Urgh, this is nasty. You are internally remapping the hwirq from one
timer to another in order to avoid accessing the enable register
which happens to be an EL2 only register?

The way we normally deal with this kind of things is by providing a
different set of irq_chip callbacks. In this particular case, this
would leave mask/unmask as empty stubs. Or you could move the FIQ
handling to use handle_simple_irq(), because there isn't any callback
that is actually applicable.

It isn't a big deal for now, but that's something we should consider
addressing in the future. With that in mind:

Reviewed-by: Marc Zyngier <maz at kernel.org>

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.