Re: [PATCH v2 7/7] spmi: apple: interrupt controller functionality

[Janne Grunau <j@jannau.net>]

On Tue, Jul 28, 2026 at 11:28:13AM +0200, Sasha Finkelstein wrote:
> From: Alba Mendez <me@alba.sh>
> 
> Add support for interrupts sent by slave devices.
> 
> Signed-off-by: Alba Mendez <me@alba.sh>
> Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
> ---
>  drivers/spmi/Kconfig                 |   3 ++-
>  drivers/spmi/spmi-apple-controller.c | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 150 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spmi/Kconfig b/drivers/spmi/Kconfig
> index a80cf4047b86..7243863a09b4 100644
> --- a/drivers/spmi/Kconfig
> +++ b/drivers/spmi/Kconfig
> @@ -13,7 +13,8 @@ if SPMI
>  
>  config SPMI_APPLE
>  	tristate "Apple SoC SPMI Controller platform driver"
> -	depends on ARCH_APPLE || COMPILE_TEST
> +	select IRQ_DOMAIN_HIERARCHY
> +	depends on ARCH_APPLE || (COMPILE_TEST && 64BIT)
>  	help
>  	  If you say yes to this option, support will be included for the
>  	  SPMI controller present on many Apple SoCs, including the
> diff --git a/drivers/spmi/spmi-apple-controller.c b/drivers/spmi/spmi-apple-controller.c
> index 109c5d2c735d..8f6a0d560da8 100644
> --- a/drivers/spmi/spmi-apple-controller.c
> +++ b/drivers/spmi/spmi-apple-controller.c
> @@ -15,9 +15,12 @@
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
> +#include <linux/spinlock.h>
>  #include <linux/spmi.h>
>  
>  /* SPMI Controller Registers */
> @@ -47,6 +50,9 @@ struct apple_spmi {
>  	struct mutex fifo_lock;
>  	bool fifo_rx_irq;
>  	struct completion fifo_rx;
> +	struct irq_domain *irqd;
> +	raw_spinlock_t irq_mask_lock;
> +	u64 irq_mask_cache[SPMI_IRQ_USER_SIZE / sizeof(u64)];

this could be a bitmap to avoid the manual bit manipulations

>  };
>  
>  #define poll_reg(spmi, reg, val, cond) \
> @@ -226,11 +232,123 @@ static void apple_spmi_irq_unmask_raw(struct apple_spmi *spmi, u32 irq)
>  	writel(readl(reg) | BIT(irq % 32), reg);
>  }
>  
> +static void apple_spmi_irq_ack(struct irq_data *d)
> +{
> +	struct apple_spmi *spmi = irq_data_get_irq_chip_data(d);
> +
> +	apple_spmi_irq_ack_raw(spmi, d->hwirq);
> +}
> +
> +static void apple_spmi_irq_mask(struct irq_data *d)
> +{
> +	struct apple_spmi *spmi = irq_data_get_irq_chip_data(d);
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&spmi->irq_mask_lock, flags);
> +	apple_spmi_irq_mask_raw(spmi, d->hwirq);
> +	spmi->irq_mask_cache[d->hwirq / 64] &= ~BIT_ULL(d->hwirq % 64);
> +	raw_spin_unlock_irqrestore(&spmi->irq_mask_lock, flags);
> +}
> +
> +static void apple_spmi_irq_unmask(struct irq_data *d)
> +{
> +	struct apple_spmi *spmi = irq_data_get_irq_chip_data(d);
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&spmi->irq_mask_lock, flags);
> +	apple_spmi_irq_unmask_raw(spmi, d->hwirq);
> +	spmi->irq_mask_cache[d->hwirq / 64] |= BIT_ULL(d->hwirq % 64);
> +	raw_spin_unlock_irqrestore(&spmi->irq_mask_lock, flags);
> +}
> +
> +static int apple_spmi_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	/* all interrupts have MSI semantics */
> +	return type == IRQ_TYPE_EDGE_RISING ? 0 : -EINVAL;
> +}
> +
> +static struct irq_chip apple_spmi_irq_chip = {
> +	.name = "apple_spmi",
> +	.irq_mask = apple_spmi_irq_mask,
> +	.irq_unmask = apple_spmi_irq_unmask,
> +	.irq_ack = apple_spmi_irq_ack,
> +	.irq_set_type = apple_spmi_irq_set_type,
> +	.flags = IRQCHIP_ONESHOT_SAFE,
> +};
> +
> +static int apple_spmi_irq_domain_map(struct irq_domain *irqd,
> +					unsigned int irq, irq_hw_number_t hw)
> +{
> +	irq_domain_set_info(irqd, irq, hw, &apple_spmi_irq_chip, irqd->host_data,
> +				handle_edge_irq, NULL, NULL);
> +	return 0;
> +}
> +
> +static int apple_spmi_irq_domain_translate(struct irq_domain *irqd,
> +					struct irq_fwspec *fwspec,
> +					unsigned long *hwirq,
> +					unsigned int *type)
> +{
> +	u32 *args = fwspec->param;
> +
> +	if (fwspec->param_count != 2)
> +		return -EINVAL;
> +
> +	if (args[0] >= SPMI_IRQ_USER_SIZE * 8)
> +		return -EINVAL;
> +	*hwirq = args[0];
> +	*type = args[1] & IRQ_TYPE_SENSE_MASK;
> +	return 0;
> +}
> +
> +static int apple_spmi_irq_domain_alloc(struct irq_domain *irqd, unsigned int virq,
> +				unsigned int nr_irqs, void *arg)
> +{
> +	unsigned int type = IRQ_TYPE_NONE;
> +	struct irq_fwspec *fwspec = arg;
> +	irq_hw_number_t hwirq;
> +	int i, ret;
> +
> +	ret = apple_spmi_irq_domain_translate(irqd, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	if (hwirq + nr_irqs > SPMI_IRQ_USER_SIZE * 8)
> +		return -EINVAL;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		ret = apple_spmi_irq_domain_map(irqd, virq + i, hwirq + i);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void apple_spmi_irq_domain_free(struct irq_domain *irqd, unsigned int virq,
> +				unsigned int nr_irqs)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		struct irq_data *d = irq_domain_get_irq_data(irqd, virq + i);
> +
> +		irq_set_handler(virq + i, NULL);
> +		irq_domain_reset_irq_data(d);
> +	}
> +}
> +
> +static const struct irq_domain_ops apple_spmi_irq_domain_ops = {
> +	.translate	= apple_spmi_irq_domain_translate,
> +	.alloc		= apple_spmi_irq_domain_alloc,
> +	.free		= apple_spmi_irq_domain_free,
> +};
> +
>  static irqreturn_t apple_spmi_irq_handler(int irq, void *dev_id)
>  {
>  	struct apple_spmi *spmi = dev_id;
>  	bool handled = false;
> -	u32 val;
> +	u64 val, offset, bit;

use unsigned long for bitmap compat, rest may remain as is since the
driver now depends on 64BIT
>  
>  	val = readl(spmi->regs + SPMI_IRQ_ACK_BASE + SPMI_IRQ_USER_SIZE);
>  	if (val & BIT(SPMI_IRQ_FIFO_RX)) {
> @@ -239,6 +357,23 @@ static irqreturn_t apple_spmi_irq_handler(int irq, void *dev_id)
>  		handled = true;
>  	}
>  
> +	for (offset = 0; offset < SPMI_IRQ_USER_SIZE; offset += sizeof(val)) {
> +		val = readq(spmi->regs + SPMI_IRQ_ACK_BASE + offset);
> +		/**
> +		 * because of other masters in the bus, we're going to get a multitude of
> +		 * interrupts we're not interested in. irq_resolve_mapping isn't very
> +		 * optimized for the nonexistent path, so instead we mask with (a locally
> +		 * cached version of) the IRQ mask
> +		 */

I don't understand the comment. Interrupt bits in SPMI_IRQ_ACK_BASE..
are set although the SPMI controller has masked them?

> +		val &= spmi->irq_mask_cache[offset / sizeof(val)];
> +		while (val) {
> +			bit = __builtin_ctzll(val);

this can be
		for_each_set_bit(bit, val, 64) {

> +			generic_handle_domain_irq(spmi->irqd, offset * 8 + bit);
> +			handled = true;
> +			val &= ~BIT(bit);
> +		}
> +	}

I think this loops needs to be wrapped in chained_irq_enter() and chained_irq_exit()

Janne