On Mon, 15 Feb 2021 12:17:03 +0000,
Hector Martin <marcan@xxxxxxxxx> wrote:
This patch introduces basic UP irqchip support, without SMP/IPI support.
This last comment seems outdated now.
+config APPLE_AIC
+ bool "Apple Interrupt Controller (AIC)"
+ depends on ARM64
+ default ARCH_APPLE
+ select IRQ_DOMAIN
+ select IRQ_DOMAIN_HIERARCHY
arm64 selects GENERIC_IRQ_IPI, which selects IRQ_DOMAIN_HIERARCHY,
which selects IRQ_DOMAIN. So these two lines are superfluous.
+ * In addition, this driver also handles FIQs, as these are routed to the same IRQ vector. These
+ * are used for Fast IPIs (TODO), the ARMv8 timer IRQs, and performance counters (TODO).
+ *
nit: A bit of comment formatting could be helpful.
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/cpuhotplug.h>
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/arm-gic-v3.h>
I'd rather you move the ICH_HCR_* definitions to sysreg.h rather than
including the GICv3 stuff. They are only there for historical reasons
(such as supporting KVM on 32bit systems), none of which apply anymore.
+ aic_ic_write(ic, AIC_TARGET_CPU + hwirq * 4, BIT(cpu));
+ irq_data_update_effective_affinity(d, cpumask_of(cpu));
It is fine to pick a single CPU out of the whole affinity set, but you
should tell the kernel that this is the case (irqd_set_single_target()).
+
+ irq_set_status_flags(irq, IRQ_LEVEL);
I'm definitely not keen on this override, and the trigger information
should be the one coming from the DT, which is already set for you.
It'd probably be useful to provide an irq_set_type() callback that
returns an error when fed an unsupported trigger.
+ irq_set_noprobe(irq);
This seems to be cargo-culted, and I don't believe this is necessary.
+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,
+};
You are mixing two APIs: the older OF-specific one, and the newer one
that uses fwnode_handle for hierarchical support. That's OK for older
drivers that were forcefully converted to using generic IPIs, but as
this is a brand new driver, I'd rather it consistently used the new
API. See a proposed rework at [1] (compile tested only).
+ atomic_and(~irq_bit, &aic_vipi_mask[this_cpu]);
atomic_andnot()?
+
+ if (!atomic_read(&aic_vipi_mask[this_cpu]))
+ aic_ic_write(ic, AIC_IPI_MASK_SET, AIC_IPI_OTHER);
This is odd. It means that you still perform the MMIO write if the bit
was already clear. I think this could be written as:
u32 val;
val = atomic_fetch_andnot(irq_bit, &aic_vipi_mask[this_cpu]);
if (val && !(val & ~irq_bit))
aic_ic_write();
val = atomic_fetch_or(irq_bit, &aic_vipi_mask[this_cpu]);
if (!val)
aic_ic_write();
+ for_each_cpu(cpu, mask) {
+ if (atomic_read(&aic_vipi_mask[cpu]) & irq_bit) {
+ atomic_or(irq_bit, &aic_vipi_flag[cpu]);
+ send |= AIC_IPI_SEND_CPU(cpu);
That's really odd. A masked IPI should be made pending, and delivered
on unmask. I think this all works because we never mask individual
IPIs, as this would otherwise drop interrupts on the floor.
+static void aic_handle_ipi(struct pt_regs *regs)
+{
+ int this_cpu = smp_processor_id();
+ int i;
+ unsigned long firing;
+
+ aic_ic_write(aic_irqc, AIC_IPI_ACK, AIC_IPI_OTHER);
+
+ /*
+ * Ensure that we've received and acked the IPI before we load the vIPI
+ * flags. This pairs with the second wmb() above.
+ */
+ mb();
I don't get your ordering here.
If you are trying to order against something that has happened on
another CPU (which is pretty likely in the case of an IPI), why isn't
this a smp_mb_before_atomic() (and conversely smp_mb_after_atomic() in
aic_ipi_send_mask())?
Although this looks to me like a good case for _acquire/_release
semantics.
+ /*
+ * Make sure the kernel's idea of logical CPU order is the same as AIC's
+ * If we ever end up with a mismatch here, we will have to introduce
+ * a mapping table similar to what other irqchip drivers do.
+ */
+ WARN_ON(aic_ic_read(aic_irqc, AIC_WHOAMI) != smp_processor_id());
This is unlikely to work as soon as you get kexec up and running. You
may not have to worry about this for some time...