On Thu, 04 Feb 2021 20:39:48 +0000,
Hector Martin [off-list ref] wrote:

I really do not want to expose IPIs in the DT. The OS defines what
IPIs are used for, not the firmware/HW. No other platform requires
this either, so is there any reason to do so?

+ *
+ */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>

There isn't any chained interrupt controller here, AFAICT.

+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include <asm/exception.h>
+
+#include <dt-bindings/interrupt-controller/apple-aic.h>
+
+#define AIC_INFO		0x0004
+#define AIC_INFO_NR_HW(i)	((i) & 0x0000ffff)
+
+#define AIC_CONFIG		0x0010
+
+#define AIC_WHOAMI		0x2000
+#define AIC_EVENT		0x2004
+
+#define AIC_EVENT_TYPE_HW	1
+#define AIC_EVENT_TYPE_IPI	4
+#define AIC_EVENT_IPI_OTHER	1
+#define AIC_EVENT_IPI_SELF	2
+
+#define AIC_IPI_SEND		0x2008
+#define AIC_IPI_ACK		0x200c
+#define AIC_IPI_MASK_SET	0x2024
+#define AIC_IPI_MASK_CLR	0x2028
+
+#define AIC_IPI_SEND_CPU(cpu)	BIT(cpu)
+
+#define AIC_IPI_OTHER		BIT(0)
+#define AIC_IPI_SELF		BIT(31)
+
+#define AIC_TARGET_CPU		0x3000
+#define AIC_SW_SET		0x4000
+#define AIC_SW_CLR		0x4080
+#define AIC_MASK_SET		0x4100
+#define AIC_MASK_CLR		0x4180
+
+#define AIC_CPU_IPI_SET(cpu)	(0x5008 + (cpu << 7))
+#define AIC_CPU_IPI_CLR(cpu)	(0x500c + (cpu << 7))
+#define AIC_CPU_IPI_MASK_SET(cpu) (0x5024 + (cpu << 7))
+#define AIC_CPU_IPI_MASK_CLR(cpu) (0x5028 + (cpu << 7))
+
+#define MASK_REG(x)		(4 * ((x) >> 5))
+#define MASK_BIT(x)		BIT((x) & 0x1f)
+
+#define AIC_NR_FIQ		2
+#define AIC_NR_IPI		2
+
+/*
+ * Max 31 bits in IPI SEND register (top bit is self).
+ * >=32-core chips will need code changes anyway.
+ */
+#define AIC_MAX_CPUS 31
+
+struct aic_irq_chip {
+	void __iomem *base;
+	struct irq_domain *hw_domain;
+	int nr_hw;
+};
+
+static struct aic_irq_chip *aic_irqc;
+
+static inline u32 aic_ic_read(struct aic_irq_chip *ic, u32 reg)

Please drop the inline, the compiler should manage this on its own
(and in most case will totally ignore that keyword one way or
another).

+{
+	return readl(ic->base + reg);

Please consider using the _relaxed accessors, as I don't think any of
these interacts with memory (apart from IPIs, of course).

+}
+
+static inline void aic_ic_write(struct aic_irq_chip *ic, u32 reg, u32 val)
+{
+	writel(val, ic->base + reg);
+}
+
+/* These functions do nothing for FIQs, because they have no masks */
+static void aic_irq_mask(struct irq_data *d)
+{
+	struct aic_irq_chip *ic = irq_data_get_irq_chip_data(d);
+
+	if (d->hwirq < ic->nr_hw)
+		aic_ic_write(ic, AIC_MASK_SET + MASK_REG(d->hwirq),
+			     MASK_BIT(d->hwirq));
+}

If these functions have no impact on the per-CPU interrupts, then
maybe these interrupts should be given a different irqchip.

+
+static void aic_irq_unmask(struct irq_data *d)
+{
+	struct aic_irq_chip *ic = irq_data_get_irq_chip_data(d);
+
+	if (d->hwirq < ic->nr_hw)
+		aic_ic_write(ic, AIC_MASK_CLR + MASK_REG(d->hwirq),
+			     MASK_BIT(d->hwirq));
+}
+
+static void aic_irq_eoi(struct irq_data *d)
+{
+	/*
+	 * Reading the interrupt reason automatically acknowledges and masks
+	 * the IRQ, so we just unmask it here if needed.
+	 */
+	if (!irqd_irq_disabled(d) && !irqd_irq_masked(d))
+		aic_irq_unmask(d);

This doesn't apply to per-CPU interrupts, right? Or does it?

+}
+
+static void aic_handle_irq(struct pt_regs *regs)
+{
+	struct aic_irq_chip *ic = aic_irqc;
+	u32 event = aic_ic_read(ic, AIC_EVENT);
+
+	while (event) {
+		u32 type = event >> 16, irq = event & 0xffff;

Nit: please consider introducing masks and using the bitfield macros
to extract the various fields.

+
+		/* AIC_EVENT is read-sensitive, ensure it happens before we proceed */
+		isb();

You seem to have a data dependency after this, so I can't see how the
ISB influences the read from AIC_EVENT. However you need to order it
with the read from the timer registers, and I believe it'd be better
to move the barrier there.

+
+		if (type == AIC_EVENT_TYPE_HW) {
+			handle_domain_irq(aic_irqc->hw_domain, irq, regs);
+		} else if (type == AIC_EVENT_TYPE_IPI) {
+			handle_domain_irq(aic_irqc->hw_domain,
+					  ic->nr_hw + AIC_NR_FIQ + irq - 1, regs);

nit: it would be slightly less cumbersome to compute the hwirq in a
switch, and have a single call to handle_domain_irq().

I also wonder whether using two top-level domains would be better. Not
a big deal though.

+		} else {
+			pr_err("spurious IRQ event %d, %d\n", type, irq);

Spurious interrupts aren't an error, in general. If you really want to
keep this, at the very least make it rate-limited.

+		}
+
+		event = aic_ic_read(ic, AIC_EVENT);

Consider turning the whole thing into a do{}while() so that there is
only a single read of AIC_EVENT in the function.

+	}
+}
+
+#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...
+	 */

nit: please try to keep comments within the 80 cols limit. I don't
mind code being wider, but comments benefit from being more rigorously
structured.

And yes, having to poll each end-point IP is really a drag. How does
the PMU work on this system? Is there any other per-CPU source?

+	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 system runs VHE, so there is also CNT{P,V}_CTL_EL02 to consider.
But I really wonder how the whole thing works once these two timers
are assigned to a guest. Somehow, something must control the masking,
otherwise you wouldn't be able to enter a guest with a timer firing.

It also means that there is no way to have threaded per-CPU
interrupts, which means no Preempt-RT. You could wire the mask/unmask
callbacks to mess with the IMASK bit in individual timers, but that
doesn't solve the problem for guests.

+}
+
+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);
+}
+
+static struct irq_chip aic_chip = {
+	.name = "AIC",
+	.irq_mask = aic_irq_mask,
+	.irq_unmask = aic_irq_unmask,
+	.irq_eoi = aic_irq_eoi,
+};
+
+static int aic_irq_domain_map(struct irq_domain *id, unsigned int irq,
+			      irq_hw_number_t hw)
+{
+	struct aic_irq_chip *ic = id->host_data;
+
+	irq_set_chip_data(irq, ic);
+	if (hw < ic->nr_hw) {
+		irq_set_chip_and_handler(irq, &aic_chip, handle_fasteoi_irq);
+	} else {
+		irq_set_percpu_devid(irq);
+		irq_set_chip_and_handler(irq, &aic_chip,
+					 handle_percpu_devid_irq);
+	}
+	irq_set_status_flags(irq, IRQ_LEVEL);

Are all interrupts level? How are MSIs implemented?

+	irq_set_noprobe(irq);
+
+	return 0;
+}
+
+static void aic_irq_domain_unmap(struct irq_domain *id, unsigned int irq)
+{
+	irq_set_chip_and_handler(irq, NULL, NULL);
+}
+
+static int aic_irq_domain_xlate(struct irq_domain *id,
+				struct device_node *ctrlr, const u32 *intspec,
+				unsigned int intsize,
+				irq_hw_number_t *out_hwirq,
+				unsigned int *out_type)
+{
+	struct aic_irq_chip *ic = id->host_data;
+
+	if (intsize != 3)
+		return -EINVAL;
+
+	if (intspec[0] == AIC_IRQ && intspec[1] < ic->nr_hw)
+		*out_hwirq = intspec[1];
+	else if (intspec[0] == AIC_FIQ && intspec[1] < AIC_NR_FIQ)
+		*out_hwirq = ic->nr_hw + intspec[1];
+	else if (intspec[0] == AIC_IPI && intspec[1] < AIC_NR_IPI)
+		*out_hwirq = ic->nr_hw + AIC_NR_FIQ + intspec[1];
+	else
+		return -EINVAL;
+
+	*out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
+
+	return 0;
+}
+
+static const struct irq_domain_ops aic_irq_domain_ops = {
+	.map = aic_irq_domain_map,
+	.unmap = aic_irq_domain_unmap,
+	.xlate = aic_irq_domain_xlate,
+};
+
+static int __init aic_of_ic_init(struct device_node *node,
+				 struct device_node *parent)
+{
+	int i;
+	void __iomem *regs;
+	u32 info;
+	struct aic_irq_chip *irqc;
+
+	regs = of_iomap(node, 0);
+	if (WARN_ON(!regs))
+		return -EIO;
+
+	irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
+	if (!irqc)
+		return -ENOMEM;
+
+	aic_irqc = irqc;
+	irqc->base = regs;
+
+	info = aic_ic_read(irqc, AIC_INFO);
+	irqc->nr_hw = AIC_INFO_NR_HW(info);
+
+	irqc->hw_domain =
+		irq_domain_add_linear(node,
+				      irqc->nr_hw + AIC_NR_FIQ + AIC_NR_IPI,
+				      &aic_irq_domain_ops, irqc);

Please keep assignments on a single line.

+	if (WARN_ON(!irqc->hw_domain)) {
+		iounmap(irqc->base);
+		kfree(irqc);
+		return -ENODEV;
+	}
+
+	irq_domain_update_bus_token(irqc->hw_domain, DOMAIN_BUS_WIRED);
+
+	set_handle_irq(aic_handle_irq_or_fiq);
+
+	for (i = 0; i < BITS_TO_LONGS(irqc->nr_hw); i++)

long is 64bit on arm64, so this loop is unlikely to do what you
want. Consider using BITS_TO_U32.

+		aic_ic_write(irqc, AIC_MASK_SET + i * 4, ~0);
+	for (i = 0; i < BITS_TO_LONGS(irqc->nr_hw); i++)
+		aic_ic_write(irqc, AIC_SW_CLR + i * 4, ~0);
+	for (i = 0; i < irqc->nr_hw; i++)
+		aic_ic_write(irqc, AIC_TARGET_CPU + i * 4, 1);
+
+	pr_info("AIC: initialized with %d IRQs, %d FIQs, %d IPIs\n",
+		irqc->nr_hw, AIC_NR_FIQ, AIC_NR_IPI);
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(apple_m1_aic, "AAPL,aic", aic_of_ic_init);

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel