Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs

[Konrad Dybcio]

On 8.10.2022 11:33, Sven Peter wrote:
> 
> Hi,
> 
>> On 7. Oct 2022, at 22:00, Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxx> wrote:
>>
>> Add support for A7-A11 SoCs by if-ing out some features only present
>> on A11 & newer (implementation-defined IPI & UNCORE registers).
>>
>> Also, annotate IPI regs support in the aic struct so that the driver
>> can tell whether the SoC supports these, as they are written to,
>> even if fast IPI is disabled.
> 
> No.
> 
>> This in turn causes a crash on older
>> platforms, as the implemention-defined registers either do
>> something else or are not supposed to be touched - definitely not a
>> NOP though.
> 
> This entire description needs to be rewritten. All you want to do is guard both fastipi and uncore reg access on pre-A11.
> 
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxx>
>> ---
>> Changes since v3:
>> - Replace use_fast_ipi with has_uncore_ipi_regs in aic_init_cpu
>> (logic error, this was written to regardless of FIPI usage before,
>> but touching Sn_... regs on SoCs that don't explicitly use them for
>> IPIs makes them sepuku..)
>> - Drop A11 compatible
>>
>> drivers/irqchip/irq-apple-aic.c | 47 ++++++++++++++++++++++-----------
>> 1 file changed, 32 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
>> index 1c2813ad8bbe..2609d6b60487 100644
>> --- a/drivers/irqchip/irq-apple-aic.c
>> +++ b/drivers/irqchip/irq-apple-aic.c
>> @@ -230,6 +230,9 @@
>>
>> static DEFINE_STATIC_KEY_TRUE(use_fast_ipi);
>>
>> +/* True if UNCORE/UNCORE2 and Sn_... IPI registers are present (A11+) */
>> +static DEFINE_STATIC_KEY_TRUE(has_uncore_ipi_regs);
>> +
>> struct aic_info {
>>    int version;
>>
>> @@ -246,6 +249,7 @@ struct aic_info {
>>
>>    /* Features */
>>    bool fast_ipi;
>> +    bool uncore_ipi_regs;
> 
> Why two flags? Didn’t we come to the conclusion last time that fastipi and uncore were introduced at the same time? Below you also either have both true or both false so there’s really no need to track both of them.
> 
> 
>> };
>>
>> static const struct aic_info aic1_info = {
>> @@ -261,6 +265,7 @@ static const struct aic_info aic1_fipi_info = {
>>    .event        = AIC_EVENT,
>>    .target_cpu    = AIC_TARGET_CPU,
>>
>> +    .uncore_ipi_regs    = true,
>>    .fast_ipi    = true,
>> };
>>
>> @@ -269,6 +274,7 @@ static const struct aic_info aic2_info = {
>>
>>    .irq_cfg    = AIC2_IRQ_CFG,
>>
>> +    .uncore_ipi_regs    = true,
>>    .fast_ipi    = true,
>> };
>>
>> @@ -524,12 +530,14 @@ static void __exception_irq_entry aic_handle_fiq(struct pt_regs *regs)
>>     * we check for everything here, even things we don't support yet.
>>     */
>>
>> -    if (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING) {
>> -        if (static_branch_likely(&use_fast_ipi)) {
>> -            aic_handle_ipi(regs);
>> -        } else {
>> -            pr_err_ratelimited("Fast IPI fired. Acking.\n");
>> -            write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
>> +    if (static_branch_likely(&has_uncore_ipi_regs)) {
>> +        if (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING) {
>> +            if (static_branch_likely(&use_fast_ipi)) {
>> +                aic_handle_ipi(regs);
>> +            } else {
>> +                pr_err_ratelimited("Fast IPI fired. Acking.\n");
>> +                write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
> 
> This still can’t be reached because both static branches will always have the same value. Didn’t we also realize a version or two ago that this can just be dropped?
> 
Ok, so I didn't realize you wanted this to become a single variable - I thought it would have
been useful to keep them separate, as A7-A10 *should* use fast IPIs as far as I'm aware, but
they don't use the impl-defined registers for that (or at least not the same ones).

For the sake of this patch, I can squash it into one as all known users to date set both in the
current form. Also, before this patch, "apple,aic" used to essentially be has_uncore_ipi_regs=true,
use_fast_ipi=false, but since there are no users, I assume that combination is not useful to keep
around?

Konrad

>> +            }
>>        }
>>    }
>>
>> @@ -566,12 +574,14 @@ static void __exception_irq_entry aic_handle_fiq(struct pt_regs *regs)
>>                      AIC_FIQ_HWIRQ(irq));
>>    }
>>
>> -    if (FIELD_GET(UPMCR0_IMODE, read_sysreg_s(SYS_IMP_APL_UPMCR0_EL1)) == UPMCR0_IMODE_FIQ &&
>> -            (read_sysreg_s(SYS_IMP_APL_UPMSR_EL1) & UPMSR_IACT)) {
>> -        /* Same story with uncore PMCs */
>> -        pr_err_ratelimited("Uncore PMC FIQ fired. Masking.\n");
>> -        sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
>> -                   FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
>> +    if (static_branch_likely(&has_uncore_ipi_regs)) {
>> +        if (FIELD_GET(UPMCR0_IMODE, read_sysreg_s(SYS_IMP_APL_UPMCR0_EL1)) ==
>> +            UPMCR0_IMODE_FIQ && (read_sysreg_s(SYS_IMP_APL_UPMSR_EL1) & UPMSR_IACT)) {
>> +            /* Same story with uncore PMCs */
>> +            pr_err_ratelimited("Uncore PMC FIQ fired. Masking.\n");
>> +            sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
>> +                    FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
>> +        }
>>    }
>> }
>>
>> @@ -944,7 +954,8 @@ static int aic_init_cpu(unsigned int cpu)
>>    /* Mask all hard-wired per-CPU IRQ/FIQ sources */
>>
>>    /* Pending Fast IPI FIQs */
>> -    write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
>> +    if (static_branch_likely(&has_uncore_ipi_regs))
>> +        write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
>>
>>    /* Timer FIQs */
>>    sysreg_clear_set(cntp_ctl_el0, 0, ARCH_TIMER_CTRL_IT_MASK);
>> @@ -965,8 +976,9 @@ static int aic_init_cpu(unsigned int cpu)
>>               FIELD_PREP(PMCR0_IMODE, PMCR0_IMODE_OFF));
>>
>>    /* Uncore PMC FIQ */
>> -    sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
>> -               FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
>> +    if (static_branch_likely(&has_uncore_ipi_regs))
>> +        sysreg_clear_set_s(SYS_IMP_APL_UPMCR0_EL1, UPMCR0_IMODE,
>> +                   FIELD_PREP(UPMCR0_IMODE, UPMCR0_IMODE_OFF));
>>
>>    /* Commit all of the above */
>>    isb();
>> @@ -1125,6 +1137,11 @@ static int __init aic_of_ic_init(struct device_node *node, struct device_node *p
>>    else
>>        static_branch_disable(&use_fast_ipi);
>>
>> +    if (irqc->info.uncore_ipi_regs)
>> +        static_branch_enable(&has_uncore_ipi_regs);
>> +    else
>> +        static_branch_disable(&has_uncore_ipi_regs);
>> +
>>    irqc->info.die_stride = off - start_off;
>>
>>    irqc->hw_domain = irq_domain_create_tree(of_node_to_fwnode(node),
>> -- 
>> 2.37.3
> 
> 
> Sven
>