From: Stan Skowronek <[email protected]>
This includes IPI support and a workaround for non-working WFI on
Apple processors.
Signed-off-by: Stan Skowronek <[email protected]>
Signed-off-by: Mohamed Mediouni <[email protected]>
---
drivers/irqchip/irq-apple-aic.c | 177 +++++++++++++++++++++++++++++---
1 file changed, 165 insertions(+), 12 deletions(-)
diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
index c601bc4b501a..ce4e39d56fcf 100644
--- a/drivers/irqchip/irq-apple-aic.c
+++ b/drivers/irqchip/irq-apple-aic.c
@@ -17,6 +17,7 @@
#include <asm/exception.h>
#include <asm/irq.h>
+#include <asm/smp.h>
#define REG_ID_REVISION 0x0000
#define REG_ID_CONFIG 0x0004
@@ -53,12 +54,17 @@
#define REG_PERCPU(r, c) \
((r) + REG_CPU_REGION - REG_CPU_LOCAL + ((c) << REG_CPU_SHIFT))
+#define NUM_IPI 8
+
static struct aic_chip_data {
void __iomem *base;
struct irq_domain *domain;
unsigned int num_irqs;
+ bool fast_ipi;
} aic;
+static DEFINE_PER_CPU(atomic_t, aic_ipi_mask);
+
static void apple_aic_irq_mask(struct irq_data *d)
{
writel(REG_IRQ_xABLE_MASK(d->hwirq),
@@ -78,18 +84,71 @@ static struct irq_chip apple_aic_irq_chip = {
.irq_unmask = apple_aic_irq_unmask,
};
-static void apple_aic_fiq_mask(struct irq_data *d)
+static void apple_aic_fiq_ipi_mask(struct irq_data *d)
{
}
-static void apple_aic_fiq_unmask(struct irq_data *d)
+static void apple_aic_fiq_ipi_unmask(struct irq_data *d)
{
}
static struct irq_chip apple_aic_irq_chip_fiq = {
.name = "apple_aic_fiq",
- .irq_mask = apple_aic_fiq_mask,
- .irq_unmask = apple_aic_fiq_unmask,
+ .irq_mask = apple_aic_fiq_ipi_mask,
+ .irq_unmask = apple_aic_fiq_ipi_unmask,
+};
+
+#define SR_APPLE_IPI_LOCAL s3_5_c15_c0_0
+#define SR_APPLE_IPI_REMOTE s3_5_c15_c0_1
+#define SR_APPLE_IPI_STAT s3_5_c15_c1_1
+
+#ifdef CONFIG_SMP
+static void apple_aic_ipi_send_mask(struct irq_data *d,
+ const struct cpumask *mask)
+{
+ int cpu, lcpu;
+ int irqnr = d->hwirq - (aic.num_irqs + 2);
+
+ if (WARN_ON(irqnr < 0 || irqnr >= NUM_IPI))
+ return;
+
+ /*
+ * Ensure that stores to Normal memory are visible to the
+ * other CPUs before issuing the IPI.
+ */
+ wmb();
+
+ for_each_cpu (cpu, mask) {
+ smp_mb__before_atomic();
+ atomic_or(1u << irqnr, per_cpu_ptr(&aic_ipi_mask, cpu));
+ smp_mb__after_atomic();
+ lcpu = get_cpu();
+ if (aic.fast_ipi) {
+ if ((lcpu >> 2) == (cpu >> 2))
+ write_sysreg(cpu & 3, SR_APPLE_IPI_LOCAL);
+ else
+ write_sysreg((cpu & 3) | ((cpu >> 2) << 16),
+ SR_APPLE_IPI_REMOTE);
+ } else
+ writel(lcpu == cpu ? REG_IPI_FLAG_SELF :
+ (REG_IPI_FLAG_OTHER << cpu),
+ aic.base + REG_IPI_SET);
+ put_cpu();
+ }
+
+ /* Force the above writes to be executed */
+ if (aic.fast_ipi)
+ isb();
+}
+#else
+#define apple_aic_ipi_send_mask NULL
+#endif
+
+static struct irq_chip apple_aic_irq_chip_ipi = {
+ .name = "apple_aic_ipi",
+ .irq_mask = apple_aic_fiq_ipi_mask,
+ .irq_unmask = apple_aic_fiq_ipi_unmask,
+ .ipi_send_mask = apple_aic_ipi_send_mask,
};
static int apple_aic_irq_domain_xlate(struct irq_domain *d,
@@ -98,16 +157,27 @@ static int apple_aic_irq_domain_xlate(struct irq_domain *d,
unsigned long *out_hwirq,
unsigned int *out_type)
{
- if (intspec[0]) { /* FIQ */
+ switch (intspec[0]) {
+ case 0: /* IRQ */
+ if (intspec[1] >= aic.num_irqs)
+ return -EINVAL;
+ if (out_hwirq)
+ *out_hwirq = intspec[1];
+ break;
+ case 1: /* FIQ */
if (intspec[1] >= 2)
return -EINVAL;
if (out_hwirq)
*out_hwirq = aic.num_irqs + intspec[1];
- } else {
- if (intspec[1] >= aic.num_irqs)
+ break;
+ case 2: /* IPI */
+ if (intspec[1] >= NUM_IPI)
return -EINVAL;
if (out_hwirq)
- *out_hwirq = intspec[1];
+ *out_hwirq = aic.num_irqs + 2 + intspec[1];
+ break;
+ default:
+ return -EINVAL;
}
if (out_type)
@@ -118,7 +188,13 @@ static int apple_aic_irq_domain_xlate(struct irq_domain *d,
static int apple_aic_irq_domain_map(struct irq_domain *d, unsigned int virq,
irq_hw_number_t hw)
{
- if (hw >= aic.num_irqs) {
+ if (hw >= aic.num_irqs + 2) {
+ irq_set_percpu_devid(virq);
+ irq_domain_set_info(d, virq, hw, &apple_aic_irq_chip_ipi,
+ d->host_data, handle_percpu_devid_irq, NULL,
+ NULL);
+ irq_set_status_flags(virq, IRQ_NOAUTOEN);
+ } else if (hw >= aic.num_irqs) {
irq_set_percpu_devid(virq);
irq_domain_set_info(d, virq, hw, &apple_aic_irq_chip_fiq,
d->host_data, handle_percpu_devid_irq, NULL,
@@ -141,8 +217,10 @@ static const struct irq_domain_ops apple_aic_irq_domain_ops = {
static void __exception_irq_entry apple_aic_handle_irq(struct pt_regs *regs)
{
+ atomic_t *maskptr;
uint32_t ack;
- unsigned done = 0;
+ unsigned done = 0, irqnr;
+ unsigned long mask;
while (1) {
ack = readl(aic.base + REG_IRQ_ACK);
@@ -154,6 +232,36 @@ static void __exception_irq_entry apple_aic_handle_irq(struct pt_regs *regs)
handle_domain_irq(aic.domain,
ack & REG_IRQ_ACK_NUM_MASK, regs);
break;
+ case REG_IRQ_ACK_TYPE_IPI:
+#ifdef CONFIG_SMP
+ if (ack == REG_IRQ_ACK_IPI_SELF)
+ writel(REG_IPI_FLAG_SELF,
+ aic.base + REG_IPI_CLEAR);
+ else
+ writel(REG_IPI_FLAG_OTHER,
+ aic.base + REG_IPI_CLEAR);
+ maskptr = get_cpu_ptr(&aic_ipi_mask);
+ smp_mb__before_atomic();
+ mask = atomic_xchg(maskptr, 0);
+ smp_mb__after_atomic();
+ put_cpu_ptr(&aic_ipi_mask);
+ for_each_set_bit (irqnr, &mask, NUM_IPI) {
+ handle_domain_irq(aic.domain,
+ aic.num_irqs + 2 + irqnr,
+ regs);
+ }
+ if (ack == REG_IRQ_ACK_IPI_SELF)
+ writel(REG_IPI_FLAG_SELF,
+ aic.base +
+ REG_PERCPU(REG_IPI_ENABLE,
+ __smp_processor_id()));
+ else
+ writel(REG_IPI_FLAG_OTHER,
+ aic.base +
+ REG_PERCPU(REG_IPI_ENABLE,
+ __smp_processor_id()));
+#endif
+ break;
}
if (done)
break;
@@ -162,6 +270,27 @@ static void __exception_irq_entry apple_aic_handle_irq(struct pt_regs *regs)
static void __exception_irq_entry apple_aic_handle_fiq(struct pt_regs *regs)
{
+#ifdef CONFIG_SMP
+ atomic_t *maskptr;
+ unsigned long mask;
+ unsigned irqnr;
+
+ if (aic.fast_ipi) {
+ if (read_sysreg(SR_APPLE_IPI_STAT)) {
+ write_sysreg(1, SR_APPLE_IPI_STAT);
+
+ maskptr = get_cpu_ptr(&aic_ipi_mask);
+ smp_mb__before_atomic();
+ mask = atomic_xchg(maskptr, 0);
+ smp_mb__after_atomic();
+ put_cpu_ptr(&aic_ipi_mask);
+ for_each_set_bit (irqnr, &mask, NUM_IPI)
+ handle_domain_irq(aic.domain,
+ aic.num_irqs + 2 + irqnr,
+ regs);
+ }
+ }
+#endif
handle_domain_irq(aic.domain, aic.num_irqs, regs);
}
@@ -169,6 +298,13 @@ void apple_aic_cpu_prepare(unsigned int cpu)
{
unsigned i;
+ if (aic.fast_ipi)
+ writel(REG_IPI_FLAG_SELF | REG_IPI_FLAG_OTHER,
+ aic.base + REG_PERCPU(REG_IPI_DISABLE, cpu));
+ else
+ writel(REG_IPI_FLAG_SELF | REG_IPI_FLAG_OTHER,
+ aic.base + REG_PERCPU(REG_IPI_ENABLE, cpu));
+
for (i = 0; i < aic.num_irqs; i++)
writel(readl(aic.base + REG_IRQ_AFFINITY(i)) | (1u << cpu),
aic.base + REG_IRQ_AFFINITY(i));
@@ -178,6 +314,9 @@ static int __init apple_aic_init(struct device_node *node,
struct device_node *interrupt_parent)
{
unsigned i;
+#ifdef CONFIG_SMP
+ int base_ipi, ret;
+#endif
if (!node)
return -ENODEV;
@@ -186,8 +325,11 @@ static int __init apple_aic_init(struct device_node *node,
if (WARN(!aic.base, "unable to map aic registers\n"))
return -EINVAL;
+ aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
+
aic.num_irqs = readl(aic.base + REG_ID_CONFIG) & 0xFFFF;
- pr_info("Apple AIC: %d IRQs + 1 FIQ + 1 dummy\n", aic.num_irqs);
+ pr_info("Apple AIC: %d IRQs + 1 FIQ + 1 dummy + %d IPIs%s\n",
+ aic.num_irqs, NUM_IPI, aic.fast_ipi ? " (fast)" : "");
for (i = 0; i < aic.num_irqs; i++)
writel(1, aic.base + REG_IRQ_AFFINITY(i));
@@ -201,10 +343,21 @@ static int __init apple_aic_init(struct device_node *node,
apple_aic_cpu_prepare(0);
- aic.domain = irq_domain_add_linear(node, aic.num_irqs + 2,
+ aic.domain = irq_domain_add_linear(node, aic.num_irqs + 2 + NUM_IPI,
&apple_aic_irq_domain_ops,
&apple_aic_irq_chip);
irq_set_default_host(aic.domain);
+
+#ifdef CONFIG_SMP
+ base_ipi = aic.num_irqs + 2;
+ ret = irq_create_strict_mappings(aic.domain, base_ipi, aic.num_irqs + 2,
+ NUM_IPI);
+ if (ret < 0)
+ pr_err("%s: irq_create_strict_mappings failed with %d\n",
+ __func__, ret);
+ set_smp_ipi_range(base_ipi, NUM_IPI);
+#endif
+
return 0;
}
--
2.29.2
On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
<[email protected]> wrote:
> +#ifdef CONFIG_SMP
> +static void apple_aic_ipi_send_mask(struct irq_data *d,
> + const struct cpumask *mask)
Not sure we care about the #ifdef here, given that arch/arm64 does not
allow building a kernel without CONFIG_SMP.
> + /*
> + * Ensure that stores to Normal memory are visible to the
> + * other CPUs before issuing the IPI.
> + */
> + wmb();
> +
> + for_each_cpu (cpu, mask) {
> + smp_mb__before_atomic();
> + atomic_or(1u << irqnr, per_cpu_ptr(&aic_ipi_mask, cpu));
> + smp_mb__after_atomic();
> + lcpu = get_cpu();
> + if (aic.fast_ipi) {
> + if ((lcpu >> 2) == (cpu >> 2))
> + write_sysreg(cpu & 3, SR_APPLE_IPI_LOCAL);
> + else
> + write_sysreg((cpu & 3) | ((cpu >> 2) << 16),
> + SR_APPLE_IPI_REMOTE);
> + } else
> + writel(lcpu == cpu ? REG_IPI_FLAG_SELF :
> + (REG_IPI_FLAG_OTHER << cpu),
> + aic.base + REG_IPI_SET);
> + put_cpu();
> + }
> +
> + /* Force the above writes to be executed */
> + if (aic.fast_ipi)
> + isb();
> +}
Since this just loops over all CPUs, I'd probably just turn it into
an ipi_send_single() callback and have the caller do the
loop for simplicity.
I also have the feeling that splitting one hardware IPI into multiple
logical interrupts, which are then all registered by the same irq
handler adds a little more complexity than necessary.
Changing this would of course require modifications to
arch/arm64/kernel/smp.c, which is hardwired to use
CONFIG_GENERIC_IRQ_IPI in smp_cross_call(), and allowing
a different code path there may be worse than emulating an
irqchip.
> @@ -186,8 +325,11 @@ static int __init apple_aic_init(struct device_node *node,
> if (WARN(!aic.base, "unable to map aic registers\n"))
> return -EINVAL;
>
> + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
Where is this property documented, and what decides which one to use?
Arnd
> On 21 Jan 2021, at 13:44, Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
> <[email protected]> wrote:
>
>> +#ifdef CONFIG_SMP
>> +static void apple_aic_ipi_send_mask(struct irq_data *d,
>> + const struct cpumask *mask)
>
> Not sure we care about the #ifdef here, given that arch/arm64 does not
> allow building a kernel without CONFIG_SMP.
>
>> + /*
>> + * Ensure that stores to Normal memory are visible to the
>> + * other CPUs before issuing the IPI.
>> + */
>> + wmb();
>> +
>> + for_each_cpu (cpu, mask) {
>> + smp_mb__before_atomic();
>> + atomic_or(1u << irqnr, per_cpu_ptr(&aic_ipi_mask, cpu));
>> + smp_mb__after_atomic();
>> + lcpu = get_cpu();
>> + if (aic.fast_ipi) {
>> + if ((lcpu >> 2) == (cpu >> 2))
>> + write_sysreg(cpu & 3, SR_APPLE_IPI_LOCAL);
>> + else
>> + write_sysreg((cpu & 3) | ((cpu >> 2) << 16),
>> + SR_APPLE_IPI_REMOTE);
>> + } else
>> + writel(lcpu == cpu ? REG_IPI_FLAG_SELF :
>> + (REG_IPI_FLAG_OTHER << cpu),
>> + aic.base + REG_IPI_SET);
>> + put_cpu();
>> + }
>> +
>> + /* Force the above writes to be executed */
>> + if (aic.fast_ipi)
>> + isb();
>> +}
>
> Since this just loops over all CPUs, I'd probably just turn it into
> an ipi_send_single() callback and have the caller do the
> loop for simplicity.
>
> I also have the feeling that splitting one hardware IPI into multiple
> logical interrupts, which are then all registered by the same irq
> handler adds a little more complexity than necessary.
>
> Changing this would of course require modifications to
> arch/arm64/kernel/smp.c, which is hardwired to use
> CONFIG_GENERIC_IRQ_IPI in smp_cross_call(), and allowing
> a different code path there may be worse than emulating an
> irqchip.
>
>> @@ -186,8 +325,11 @@ static int __init apple_aic_init(struct device_node *node,
>> if (WARN(!aic.base, "unable to map aic registers\n"))
>> return -EINVAL;
>>
>> + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
>
> Where is this property documented, and what decides which one to use?
It’s getting documented in the next patch set.
This property is there to enable support for older iPhone processors
later on, some of which do not have fast IPI support.
On Apple M1, fast-ipi is always on.
Thank you,
> Arnd
On Thu, Jan 21, 2021 at 1:50 PM Mohamed Mediouni
<[email protected]> wrote:
> > On 21 Jan 2021, at 13:44, Arnd Bergmann <[email protected]> wrote:
> >> @@ -186,8 +325,11 @@ static int __init apple_aic_init(struct device_node *node,
> >> if (WARN(!aic.base, "unable to map aic registers\n"))
> >> return -EINVAL;
> >>
> >> + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
> >
> > Where is this property documented, and what decides which one to use?
> It’s getting documented in the next patch set.
>
> This property is there to enable support for older iPhone processors
> later on, some of which do not have fast IPI support.
>
> On Apple M1, fast-ipi is always on.
Ok, makes sense. Does fast-ipi mean you cannot use the other mode at
all, or is it just faster as implied by the name? If so, how much faster?
Arnd
On 21/01/2021 22.00, Arnd Bergmann wrote:
> Ok, makes sense. Does fast-ipi mean you cannot use the other mode at
> all, or is it just faster as implied by the name? If so, how much faster?
The other mode still works, as the AIC IPI registers are still there on
M1 and work properly. I don't have performance numbers though.
--
Hector Martin "marcan" ([email protected])
Public Key: https://mrcn.st/pub
On Thu, Jan 21, 2021 at 01:22:37PM +0000, Marc Zyngier wrote:
> On 2021-01-21 12:50, Mohamed Mediouni wrote:
> > > On 21 Jan 2021, at 13:44, Arnd Bergmann <[email protected]> wrote:
> > >
> > > On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
>
> [...]
>
> > > > + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
> > >
> > > Where is this property documented, and what decides which one to use?
> > It’s getting documented in the next patch set.
> >
> > This property is there to enable support for older iPhone processors
> > later on, some of which do not have fast IPI support.
> >
> > On Apple M1, fast-ipi is always on.
>
> Then please focus on a single implementation. Additional features can
> always be merged later once something is up and running.
>
> Also, there sysregs can be detected by matching the MIDR, so I don't
> think we need a DT property for that.
Generally we do not detect IMP-DEF sysregs based on MIDR because they
won't necessarily be exposed to a VM, so I suspect that we do need DT
properties to describe that IMP-DEF sysregs are accessible, and should
not rely on the MIDR alone. Maybe that's implicit in another property,
but worth bearing in mind.
Thanks,
Mark.
> On 21 Jan 2021, at 14:22, Marc Zyngier <[email protected]> wrote:
>
> On 2021-01-21 12:50, Mohamed Mediouni wrote:
>>> On 21 Jan 2021, at 13:44, Arnd Bergmann <[email protected]> wrote:
>>> On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
>
> [...]
>
>>>> + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
>>> Where is this property documented, and what decides which one to use?
>> It’s getting documented in the next patch set.
>> This property is there to enable support for older iPhone processors
>> later on, some of which do not have fast IPI support.
>> On Apple M1, fast-ipi is always on.
>
> Then please focus on a single implementation. Additional features can
> always be merged later once something is up and running.
>
> Also, there sysregs can be detected by matching the MIDR, so I don't
> think we need a DT property for that.
>
> Thanks,
>
Because UART access adapters for the new M1 Macs aren’t plentiful
at all, I actually use this for development, with iPhones which have
much more easy to buy Lightning-to-UART adapters.
(That’s why the old implementation is there too)
Might be worth splitting the new one to a new commit though...
Thank you,
> M.
> --
> Jazz is not dead. It just smells funny...
On 2021-01-21 13:32, Mark Rutland wrote:
> On Thu, Jan 21, 2021 at 01:22:37PM +0000, Marc Zyngier wrote:
>> On 2021-01-21 12:50, Mohamed Mediouni wrote:
>> > > On 21 Jan 2021, at 13:44, Arnd Bergmann <[email protected]> wrote:
>> > >
>> > > On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
>>
>> [...]
>>
>> > > > + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
>> > >
>> > > Where is this property documented, and what decides which one to use?
>> > It’s getting documented in the next patch set.
>> >
>> > This property is there to enable support for older iPhone processors
>> > later on, some of which do not have fast IPI support.
>> >
>> > On Apple M1, fast-ipi is always on.
>>
>> Then please focus on a single implementation. Additional features can
>> always be merged later once something is up and running.
>>
>> Also, there sysregs can be detected by matching the MIDR, so I don't
>> think we need a DT property for that.
>
> Generally we do not detect IMP-DEF sysregs based on MIDR because they
> won't necessarily be exposed to a VM, so I suspect that we do need DT
> properties to describe that IMP-DEF sysregs are accessible, and should
> not rely on the MIDR alone. Maybe that's implicit in another property,
> but worth bearing in mind.
Hmm. That's a good point. I think this could be keyed off
the compatible property, which should accurately reflect
the version of the interrupt controller.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2021-01-21 13:34, Mohamed Mediouni wrote:
>> On 21 Jan 2021, at 14:22, Marc Zyngier <[email protected]> wrote:
>>
>> On 2021-01-21 12:50, Mohamed Mediouni wrote:
>>>> On 21 Jan 2021, at 13:44, Arnd Bergmann <[email protected]> wrote:
>>>> On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
>>
>> [...]
>>
>>>>> + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
>>>> Where is this property documented, and what decides which one to
>>>> use?
>>> It’s getting documented in the next patch set.
>>> This property is there to enable support for older iPhone processors
>>> later on, some of which do not have fast IPI support.
>>> On Apple M1, fast-ipi is always on.
>>
>> Then please focus on a single implementation. Additional features can
>> always be merged later once something is up and running.
>>
>> Also, there sysregs can be detected by matching the MIDR, so I don't
>> think we need a DT property for that.
>>
>> Thanks,
>>
> Because UART access adapters for the new M1 Macs aren’t plentiful
> at all, I actually use this for development, with iPhones which have
> much more easy to buy Lightning-to-UART adapters.
>
> (That’s why the old implementation is there too)
>
> Might be worth splitting the new one to a new commit though...
This series is supposed to cover M1 only, and adding extra support
as part of it is only likely to make the code harder to review.
I'd rather you focus on a single IPI interface (fast or slow,
I don't really care). Extra features can come in later.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On Thu, Jan 21, 2021 at 3:10 PM Marc Zyngier <[email protected]> wrote:
> On 2021-01-21 13:34, Mohamed Mediouni wrote:
> >> On 21 Jan 2021, at 14:22, Marc Zyngier <[email protected]> wrote:
> > Because UART access adapters for the new M1 Macs aren’t plentiful
> > at all, I actually use this for development, with iPhones which have
> > much more easy to buy Lightning-to-UART adapters.
> >
> > (That’s why the old implementation is there too)
> >
> > Might be worth splitting the new one to a new commit though...
>
> This series is supposed to cover M1 only, and adding extra support
> as part of it is only likely to make the code harder to review.
>
> I'd rather you focus on a single IPI interface (fast or slow,
> I don't really care). Extra features can come in later.
Agreed. The slow interface is probably easier to start with,
because it avoids hooking into the FIQ, so the FIQ can be
completely decoupled from AIC and just used for the timer.
Maybe there is even a way to use more than one hardware IPI in
the AIC?
Arnd
> On 21 Jan 2021, at 16:09, Arnd Bergmann <[email protected]> wrote:
>
> On Thu, Jan 21, 2021 at 3:10 PM Marc Zyngier <[email protected]> wrote:
>> On 2021-01-21 13:34, Mohamed Mediouni wrote:
>>>> On 21 Jan 2021, at 14:22, Marc Zyngier <[email protected]> wrote:
>>> Because UART access adapters for the new M1 Macs aren’t plentiful
>>> at all, I actually use this for development, with iPhones which have
>>> much more easy to buy Lightning-to-UART adapters.
>>>
>>> (That’s why the old implementation is there too)
>>>
>>> Might be worth splitting the new one to a new commit though...
>>
>> This series is supposed to cover M1 only, and adding extra support
>> as part of it is only likely to make the code harder to review.
>>
>> I'd rather you focus on a single IPI interface (fast or slow,
>> I don't really care). Extra features can come in later.
>
> Agreed. The slow interface is probably easier to start with,
> because it avoids hooking into the FIQ, so the FIQ can be
> completely decoupled from AIC and just used for the timer.
>
> Maybe there is even a way to use more than one hardware IPI in
> the AIC?
>
> Arnd
Hello,
Decided to only have only the slow interface in the second patch series.
Thank you,
On 2021-01-21 12:50, Mohamed Mediouni wrote:
>> On 21 Jan 2021, at 13:44, Arnd Bergmann <[email protected]> wrote:
>>
>> On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
[...]
>>> + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
>>
>> Where is this property documented, and what decides which one to use?
> It’s getting documented in the next patch set.
>
> This property is there to enable support for older iPhone processors
> later on, some of which do not have fast IPI support.
>
> On Apple M1, fast-ipi is always on.
Then please focus on a single implementation. Additional features can
always be merged later once something is up and running.
Also, there sysregs can be detected by matching the MIDR, so I don't
think we need a DT property for that.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On Thu, Jan 21, 2021 at 6:52 AM Mohamed Mediouni
<[email protected]> wrote:
>
>
>
> > On 21 Jan 2021, at 13:44, Arnd Bergmann <[email protected]> wrote:
> >
> > On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
> > <[email protected]> wrote:
> >
> >> +#ifdef CONFIG_SMP
> >> +static void apple_aic_ipi_send_mask(struct irq_data *d,
> >> + const struct cpumask *mask)
> >
> > Not sure we care about the #ifdef here, given that arch/arm64 does not
> > allow building a kernel without CONFIG_SMP.
> >
> >> + /*
> >> + * Ensure that stores to Normal memory are visible to the
> >> + * other CPUs before issuing the IPI.
> >> + */
> >> + wmb();
> >> +
> >> + for_each_cpu (cpu, mask) {
> >> + smp_mb__before_atomic();
> >> + atomic_or(1u << irqnr, per_cpu_ptr(&aic_ipi_mask, cpu));
> >> + smp_mb__after_atomic();
> >> + lcpu = get_cpu();
> >> + if (aic.fast_ipi) {
> >> + if ((lcpu >> 2) == (cpu >> 2))
> >> + write_sysreg(cpu & 3, SR_APPLE_IPI_LOCAL);
> >> + else
> >> + write_sysreg((cpu & 3) | ((cpu >> 2) << 16),
> >> + SR_APPLE_IPI_REMOTE);
> >> + } else
> >> + writel(lcpu == cpu ? REG_IPI_FLAG_SELF :
> >> + (REG_IPI_FLAG_OTHER << cpu),
> >> + aic.base + REG_IPI_SET);
> >> + put_cpu();
> >> + }
> >> +
> >> + /* Force the above writes to be executed */
> >> + if (aic.fast_ipi)
> >> + isb();
> >> +}
> >
> > Since this just loops over all CPUs, I'd probably just turn it into
> > an ipi_send_single() callback and have the caller do the
> > loop for simplicity.
> >
> > I also have the feeling that splitting one hardware IPI into multiple
> > logical interrupts, which are then all registered by the same irq
> > handler adds a little more complexity than necessary.
> >
> > Changing this would of course require modifications to
> > arch/arm64/kernel/smp.c, which is hardwired to use
> > CONFIG_GENERIC_IRQ_IPI in smp_cross_call(), and allowing
> > a different code path there may be worse than emulating an
> > irqchip.
> >
> >> @@ -186,8 +325,11 @@ static int __init apple_aic_init(struct device_node *node,
> >> if (WARN(!aic.base, "unable to map aic registers\n"))
> >> return -EINVAL;
> >>
> >> + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
> >
> > Where is this property documented, and what decides which one to use?
> It’s getting documented in the next patch set.
>
> This property is there to enable support for older iPhone processors
> later on, some of which do not have fast IPI support.
>
> On Apple M1, fast-ipi is always on.
This should be implied by the compatible string which needs to be more
specific and include the SoC name.
Rob
> On 21 Jan 2021, at 17:40, Rob Herring <[email protected]> wrote:
>
> On Thu, Jan 21, 2021 at 6:52 AM Mohamed Mediouni
> <[email protected]> wrote:
>>
>>
>>
>>> On 21 Jan 2021, at 13:44, Arnd Bergmann <[email protected]> wrote:
>>>
>>> On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
>>> <[email protected]> wrote:
>>>
>>>> +#ifdef CONFIG_SMP
>>>> +static void apple_aic_ipi_send_mask(struct irq_data *d,
>>>> + const struct cpumask *mask)
>>>
>>> Not sure we care about the #ifdef here, given that arch/arm64 does not
>>> allow building a kernel without CONFIG_SMP.
>>>
>>>> + /*
>>>> + * Ensure that stores to Normal memory are visible to the
>>>> + * other CPUs before issuing the IPI.
>>>> + */
>>>> + wmb();
>>>> +
>>>> + for_each_cpu (cpu, mask) {
>>>> + smp_mb__before_atomic();
>>>> + atomic_or(1u << irqnr, per_cpu_ptr(&aic_ipi_mask, cpu));
>>>> + smp_mb__after_atomic();
>>>> + lcpu = get_cpu();
>>>> + if (aic.fast_ipi) {
>>>> + if ((lcpu >> 2) == (cpu >> 2))
>>>> + write_sysreg(cpu & 3, SR_APPLE_IPI_LOCAL);
>>>> + else
>>>> + write_sysreg((cpu & 3) | ((cpu >> 2) << 16),
>>>> + SR_APPLE_IPI_REMOTE);
>>>> + } else
>>>> + writel(lcpu == cpu ? REG_IPI_FLAG_SELF :
>>>> + (REG_IPI_FLAG_OTHER << cpu),
>>>> + aic.base + REG_IPI_SET);
>>>> + put_cpu();
>>>> + }
>>>> +
>>>> + /* Force the above writes to be executed */
>>>> + if (aic.fast_ipi)
>>>> + isb();
>>>> +}
>>>
>>> Since this just loops over all CPUs, I'd probably just turn it into
>>> an ipi_send_single() callback and have the caller do the
>>> loop for simplicity.
>>>
>>> I also have the feeling that splitting one hardware IPI into multiple
>>> logical interrupts, which are then all registered by the same irq
>>> handler adds a little more complexity than necessary.
>>>
>>> Changing this would of course require modifications to
>>> arch/arm64/kernel/smp.c, which is hardwired to use
>>> CONFIG_GENERIC_IRQ_IPI in smp_cross_call(), and allowing
>>> a different code path there may be worse than emulating an
>>> irqchip.
>>>
>>>> @@ -186,8 +325,11 @@ static int __init apple_aic_init(struct device_node *node,
>>>> if (WARN(!aic.base, "unable to map aic registers\n"))
>>>> return -EINVAL;
>>>>
>>>> + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
>>>
>>> Where is this property documented, and what decides which one to use?
>> It’s getting documented in the next patch set.
>>
>> This property is there to enable support for older iPhone processors
>> later on, some of which do not have fast IPI support.
>>
>> On Apple M1, fast-ipi is always on.
>
> This should be implied by the compatible string which needs to be more
> specific and include the SoC name.
>
> Rob
Then we’ll eventually have two aic compatible strings, aic which is compatible
with Apple A7 onwards and aicv2 which is a superset with fast IPI (introduced
on the Apple A11, 3 years ago, with no further programmer-visible changes since
then).
Does that look right?
Thank you,
On Thu, Jan 21, 2021 at 10:43 AM Mohamed Mediouni
<[email protected]> wrote:
> > On 21 Jan 2021, at 17:40, Rob Herring <[email protected]> wrote:
> > On Thu, Jan 21, 2021 at 6:52 AM Mohamed Mediouni
> > <[email protected]> wrote:
> >>> On 21 Jan 2021, at 13:44, Arnd Bergmann <[email protected]> wrote:
> >>> On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
> >>> <[email protected]> wrote:
[...]
> >>>> @@ -186,8 +325,11 @@ static int __init apple_aic_init(struct device_node *node,
> >>>> if (WARN(!aic.base, "unable to map aic registers\n"))
> >>>> return -EINVAL;
> >>>>
> >>>> + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
> >>>
> >>> Where is this property documented, and what decides which one to use?
> >> It’s getting documented in the next patch set.
> >>
> >> This property is there to enable support for older iPhone processors
> >> later on, some of which do not have fast IPI support.
> >>
> >> On Apple M1, fast-ipi is always on.
> >
> > This should be implied by the compatible string which needs to be more
> > specific and include the SoC name.
> >
> > Rob
>
> Then we’ll eventually have two aic compatible strings, aic which is compatible
> with Apple A7 onwards and aicv2 which is a superset with fast IPI (introduced
> on the Apple A11, 3 years ago, with no further programmer-visible changes since
> then).
>
> Does that look right?
If we did this from the start, it would evolve like this:
A7: "AAPL,a7-aic"
A8: "AAPL,a8-aic", "AAPL,a7-aic" # Read this as A8 AIC is backwards
compatible with A7 AIC
A9: "AAPL,a9-aic", "AAPL,a7-aic"
A11: "AAPL,a11-aic", "AAPL,a7-aic"
If the A11 version could work on an OS that only supported the
original model (sounds like this is the case) Or if it's not backwards
compatible:
A11: "AAPL,a11-aic"
If the A11 is different and not backwards compatible.
Then M1 could be:
M1: "AAPL,m1-aic", "AAPL,a11-aic"
Or to even support an OS with only v1 support:
M1: "AAPL,m1-aic", "AAPL,a11-aic", "AAPL,a7-aic"
You don't really need the fallback here because there isn't any
existing OS support and the baseline is the M1.
If you want to have generic fallback compatible strings with versions,
that's fine too. I'm not really a fan of version numbers that are just
made up by the binding author though. Most SoC vendors don't have
rigorous versioning of their IP and those that do seem to have a new
version on every SoC.
The important part is *always* having an SoC specific compatible so
you can deal with any quirk or feature without having to change the
DTB. Everyone says blocks are 'the same' until they aren't.
Rob
> On 21 Jan 2021, at 18:37, Rob Herring <[email protected]> wrote:
>
> On Thu, Jan 21, 2021 at 10:43 AM Mohamed Mediouni
> <[email protected]> wrote:
>>> On 21 Jan 2021, at 17:40, Rob Herring <[email protected]> wrote:
>>> On Thu, Jan 21, 2021 at 6:52 AM Mohamed Mediouni
>>> <[email protected]> wrote:
>>>>> On 21 Jan 2021, at 13:44, Arnd Bergmann <[email protected]> wrote:
>>>>> On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
>>>>> <[email protected]> wrote:
>
> [...]
>
>>>>>> @@ -186,8 +325,11 @@ static int __init apple_aic_init(struct device_node *node,
>>>>>> if (WARN(!aic.base, "unable to map aic registers\n"))
>>>>>> return -EINVAL;
>>>>>>
>>>>>> + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
>>>>>
>>>>> Where is this property documented, and what decides which one to use?
>>>> It’s getting documented in the next patch set.
>>>>
>>>> This property is there to enable support for older iPhone processors
>>>> later on, some of which do not have fast IPI support.
>>>>
>>>> On Apple M1, fast-ipi is always on.
>>>
>>> This should be implied by the compatible string which needs to be more
>>> specific and include the SoC name.
>>>
>>> Rob
>>
>> Then we’ll eventually have two aic compatible strings, aic which is compatible
>> with Apple A7 onwards and aicv2 which is a superset with fast IPI (introduced
>> on the Apple A11, 3 years ago, with no further programmer-visible changes since
>> then).
>>
>> Does that look right?
>
> If we did this from the start, it would evolve like this:
>
> A7: "AAPL,a7-aic"
> A8: "AAPL,a8-aic", "AAPL,a7-aic" # Read this as A8 AIC is backwards
> compatible with A7 AIC
> A9: "AAPL,a9-aic", "AAPL,a7-aic"
>
> A11: "AAPL,a11-aic", "AAPL,a7-aic"
>
> If the A11 version could work on an OS that only supported the
> original model (sounds like this is the case) Or if it's not backwards
> compatible:
>
The A11 AIC indeed can be used by older drivers that aren’t aware
of the fast IPI path introduced on A11 just fine.
> A11: "AAPL,a11-aic"
>
> If the A11 is different and not backwards compatible.
>
> Then M1 could be:
>
> M1: "AAPL,m1-aic", "AAPL,a11-aic"
>
> Or to even support an OS with only v1 support:
>
> M1: "AAPL,m1-aic", "AAPL,a11-aic", "AAPL,a7-aic"
>
> You don't really need the fallback here because there isn't any
> existing OS support and the baseline is the M1.
>
> If you want to have generic fallback compatible strings with versions,
> that's fine too. I'm not really a fan of version numbers that are just
> made up by the binding author though. Most SoC vendors don't have
> rigorous versioning of their IP and those that do seem to have a new
> version on every SoC.
>
> The important part is *always* having an SoC specific compatible so
> you can deal with any quirk or feature without having to change the
> DTB. Everyone says blocks are 'the same' until they aren’t.
>
Is it fine if such a SoC-specific compatible is present but with having
the driver only know about AAPL,a11-aic for example?
(To just have it when it’d be needed if ever in the future, but not uselessly
add entries to the driver that will not be currently used)
On a tangent:
The internal naming scheme used by Apple is off-by-one:
Apple A14 for example is Apple H13P (H-series 13th gen processor, Phone)
Apple M1 is Apple H13G (H-series 13th gen, G series)
(And Apple A12X is Apple H11G for example, with A12 being H11P)
Should we bother with those or use the marketing names? Especially because
the beefier SoCs might not be of the H series anyway… as the internal scheme
reveals that M1 could as well have been an A14X.
And there’s also the other internal naming scheme:
Apple A12 being t8020, Apple A12X being t8027
Apple A14 being t8101
Apple M1 being t8103
T there means the foundry at which the chip was manufactured, in the cases above TSMC.
Of course Apple itself uses both… with the marketing name being nowhere in their device
trees.
Thank you,
> Rob
On Thu, Jan 21, 2021 at 12:09 PM Mohamed Mediouni
<[email protected]> wrote:
>
>
>
> > On 21 Jan 2021, at 18:37, Rob Herring <[email protected]> wrote:
> >
> > On Thu, Jan 21, 2021 at 10:43 AM Mohamed Mediouni
> > <[email protected]> wrote:
> >>> On 21 Jan 2021, at 17:40, Rob Herring <[email protected]> wrote:
> >>> On Thu, Jan 21, 2021 at 6:52 AM Mohamed Mediouni
> >>> <[email protected]> wrote:
> >>>>> On 21 Jan 2021, at 13:44, Arnd Bergmann <[email protected]> wrote:
> >>>>> On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
> >>>>> <[email protected]> wrote:
> >
> > [...]
> >
> >>>>>> @@ -186,8 +325,11 @@ static int __init apple_aic_init(struct device_node *node,
> >>>>>> if (WARN(!aic.base, "unable to map aic registers\n"))
> >>>>>> return -EINVAL;
> >>>>>>
> >>>>>> + aic.fast_ipi = of_property_read_bool(node, "fast-ipi");
> >>>>>
> >>>>> Where is this property documented, and what decides which one to use?
> >>>> It’s getting documented in the next patch set.
> >>>>
> >>>> This property is there to enable support for older iPhone processors
> >>>> later on, some of which do not have fast IPI support.
> >>>>
> >>>> On Apple M1, fast-ipi is always on.
> >>>
> >>> This should be implied by the compatible string which needs to be more
> >>> specific and include the SoC name.
> >>>
> >>> Rob
> >>
> >> Then we’ll eventually have two aic compatible strings, aic which is compatible
> >> with Apple A7 onwards and aicv2 which is a superset with fast IPI (introduced
> >> on the Apple A11, 3 years ago, with no further programmer-visible changes since
> >> then).
> >>
> >> Does that look right?
> >
> > If we did this from the start, it would evolve like this:
> >
> > A7: "AAPL,a7-aic"
> > A8: "AAPL,a8-aic", "AAPL,a7-aic" # Read this as A8 AIC is backwards
> > compatible with A7 AIC
> > A9: "AAPL,a9-aic", "AAPL,a7-aic"
> >
> > A11: "AAPL,a11-aic", "AAPL,a7-aic"
> >
> > If the A11 version could work on an OS that only supported the
> > original model (sounds like this is the case) Or if it's not backwards
> > compatible:
> >
>
> The A11 AIC indeed can be used by older drivers that aren’t aware
> of the fast IPI path introduced on A11 just fine.
>
> > A11: "AAPL,a11-aic"
> >
> > If the A11 is different and not backwards compatible.
> >
> > Then M1 could be:
> >
> > M1: "AAPL,m1-aic", "AAPL,a11-aic"
> >
> > Or to even support an OS with only v1 support:
> >
> > M1: "AAPL,m1-aic", "AAPL,a11-aic", "AAPL,a7-aic"
> >
> > You don't really need the fallback here because there isn't any
> > existing OS support and the baseline is the M1.
> >
> > If you want to have generic fallback compatible strings with versions,
> > that's fine too. I'm not really a fan of version numbers that are just
> > made up by the binding author though. Most SoC vendors don't have
> > rigorous versioning of their IP and those that do seem to have a new
> > version on every SoC.
> >
> > The important part is *always* having an SoC specific compatible so
> > you can deal with any quirk or feature without having to change the
> > DTB. Everyone says blocks are 'the same' until they aren’t.
> >
> Is it fine if such a SoC-specific compatible is present but with having
> the driver only know about AAPL,a11-aic for example?
> (To just have it when it’d be needed if ever in the future, but not uselessly
> add entries to the driver that will not be currently used)
Yes, that's expected. You add the more specific compatible when you
add the feature or quirk work-around.
>
> On a tangent:
>
> The internal naming scheme used by Apple is off-by-one:
>
> Apple A14 for example is Apple H13P (H-series 13th gen processor, Phone)
> Apple M1 is Apple H13G (H-series 13th gen, G series)
> (And Apple A12X is Apple H11G for example, with A12 being H11P)
>
> Should we bother with those or use the marketing names? Especially because
> the beefier SoCs might not be of the H series anyway… as the internal scheme
> reveals that M1 could as well have been an A14X.
>
> And there’s also the other internal naming scheme:
> Apple A12 being t8020, Apple A12X being t8027
> Apple A14 being t8101
> Apple M1 being t8103
>
> T there means the foundry at which the chip was manufactured, in the cases above TSMC.
>
> Of course Apple itself uses both… with the marketing name being nowhere in their device
> trees.
I'd probably lean toward the marketing names, but don't really care as
long as you're consistent both for a given SoC and across generations.
Rob
Hi Mohamed,
just a small drive-by comment:
On Wed, Jan 20, 2021 at 2:32 PM Mohamed Mediouni
<[email protected]> wrote:
> + for_each_cpu (cpu, mask) {
> + smp_mb__before_atomic();
> + atomic_or(1u << irqnr, per_cpu_ptr(&aic_ipi_mask, cpu));
Use:
#include <linux/bits.h>
atomic_or(BIT(irqnr), per_cpu_ptr(&aic_ipi_mask, cpu));
Yours,
Linus Walleij