Re: [RFC PATCH 7/7] irqchip/apple-aic: add SMP support to the Apple AIC driver.

[Mohamed Mediouni]

> On 21 Jan 2021, at 13:44, Arnd Bergmann <arnd@xxxxxxxxxx> wrote:
> 
> On Wed, Jan 20, 2021 at 2:27 PM Mohamed Mediouni
> <mohamed.mediouni@xxxxxxxxxxxx> 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