| Message ID | 20220712160919.740878-2-konrad.dybcio@somainline.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dt-bindings: apple,aic: Document A7-A11 compatibles | expand |
Tested-by: Nick Chan <email@example.org> # iPad Pro 9.7 Inch (Wi-Fi), iPhone X (Global)
I don't see where you actually set the new features to false, or indeed check the compatible at all? Am I missing a patch? > Add support for A7-A11 SoCs by if-ing out some features only present on > A12 & newer (UNCORE2 registers) or M1 & newer (EL2 registers - the > older SoCs don't implement EL2). > > Also, annotate IPI regs support (A11 and newer*) so that the driver can > tell whether the SoC supports these (they are written to even if fast > IPI is disabled, when the registers are there of course). > > *A11 is supposed to use this feature, but it is currently not working. > That said, it is not yet necessary, especially with only one core up, > and it works a-ok using the same featureset as earlier SoCs. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > --- > drivers/irqchip/irq-apple-aic.c | 54 +++++++++++++++++++++++---------- > 1 file changed, 38 insertions(+), 16 deletions(-) > > diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c > index 12dd48727a15..36f4b52addc2 100644 > --- a/drivers/irqchip/irq-apple-aic.c > +++ b/drivers/irqchip/irq-apple-aic.c > @@ -245,7 +245,10 @@ struct aic_info { > u32 die_stride; > > /* Features */ > + bool el2_regs; > bool fast_ipi; > + bool ipi_regs; > + bool uncore2_regs; > }; > > static const struct aic_info aic1_info = { > @@ -261,7 +264,10 @@ static const struct aic_info aic1_fipi_info = { > .event = AIC_EVENT, > .target_cpu = AIC_TARGET_CPU, > > + .el2_regs = true, > .fast_ipi = true, > + .ipi_regs = true, > + .uncore2_regs = true, > }; > > static const struct aic_info aic2_info = { > @@ -269,7 +275,10 @@ static const struct aic_info aic2_info = { > > .irq_cfg = AIC2_IRQ_CFG, > > + .el2_regs = true, > .fast_ipi = true, > + .ipi_regs = true, > + .uncore2_regs = true, > }; > > static const struct of_device_id aic_info_match[] = { > @@ -452,6 +461,9 @@ static unsigned long aic_fiq_get_idx(struct irq_data *d) > > static void aic_fiq_set_mask(struct irq_data *d) > { > + if (!aic_irqc->info.el2_regs) > + return; > + > /* Only the guest timers have real mask bits, unfortunately. */ > switch (aic_fiq_get_idx(d)) { > case AIC_TMR_EL02_PHYS: > @@ -469,6 +481,9 @@ static void aic_fiq_set_mask(struct irq_data *d) > > static void aic_fiq_clear_mask(struct irq_data *d) > { > + if (!aic_irqc->info.el2_regs) > + return; > + > switch (aic_fiq_get_idx(d)) { > case AIC_TMR_EL02_PHYS: > sysreg_clear_set_s(SYS_IMP_APL_VM_TMR_FIQ_ENA_EL2, 0, VM_TMR_FIQ_ENABLE_P); > @@ -524,12 +539,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 (aic_irqc->info.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); > + } > } > } > > @@ -566,12 +583,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 (aic_irqc->info.uncore2_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)); > + } > } > } > > @@ -676,7 +695,8 @@ static int aic_irq_domain_translate(struct irq_domain *id, > break; > case AIC_TMR_HV_PHYS: > case AIC_TMR_HV_VIRT: > - return -ENOENT; > + if (aic_irqc->info.el2_regs) > + return -ENOENT; > default: > break; > } > @@ -944,7 +964,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 (aic_irqc->info.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 +986,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 (aic_irqc->info.uncore2_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(); > -- > 2.37.0 >
On 12.07.2022 20:23, Alyssa Rosenzweig wrote: > I don't see where you actually set the new features to false, or indeed > check the compatible at all? Am I missing a patch? No, it's okay. The 'features' are enabled for aic1_fipi (M1) and aic2 (newer) and not enabled (as in the struct is partially initialized so they are implicitly 0-initialized) on aic1, so that 'the most basic' compatible works for all chipsets. Konrad > >> Add support for A7-A11 SoCs by if-ing out some features only present on >> A12 & newer (UNCORE2 registers) or M1 & newer (EL2 registers - the >> older SoCs don't implement EL2). >> >> Also, annotate IPI regs support (A11 and newer*) so that the driver can >> tell whether the SoC supports these (they are written to even if fast >> IPI is disabled, when the registers are there of course). >> >> *A11 is supposed to use this feature, but it is currently not working. >> That said, it is not yet necessary, especially with only one core up, >> and it works a-ok using the same featureset as earlier SoCs. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> >> --- >> drivers/irqchip/irq-apple-aic.c | 54 +++++++++++++++++++++++---------- >> 1 file changed, 38 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c >> index 12dd48727a15..36f4b52addc2 100644 >> --- a/drivers/irqchip/irq-apple-aic.c >> +++ b/drivers/irqchip/irq-apple-aic.c >> @@ -245,7 +245,10 @@ struct aic_info { >> u32 die_stride; >> >> /* Features */ >> + bool el2_regs; >> bool fast_ipi; >> + bool ipi_regs; >> + bool uncore2_regs; >> }; >> >> static const struct aic_info aic1_info = { >> @@ -261,7 +264,10 @@ static const struct aic_info aic1_fipi_info = { >> .event = AIC_EVENT, >> .target_cpu = AIC_TARGET_CPU, >> >> + .el2_regs = true, >> .fast_ipi = true, >> + .ipi_regs = true, >> + .uncore2_regs = true, >> }; >> >> static const struct aic_info aic2_info = { >> @@ -269,7 +275,10 @@ static const struct aic_info aic2_info = { >> >> .irq_cfg = AIC2_IRQ_CFG, >> >> + .el2_regs = true, >> .fast_ipi = true, >> + .ipi_regs = true, >> + .uncore2_regs = true, >> }; >> >> static const struct of_device_id aic_info_match[] = { >> @@ -452,6 +461,9 @@ static unsigned long aic_fiq_get_idx(struct irq_data *d) >> >> static void aic_fiq_set_mask(struct irq_data *d) >> { >> + if (!aic_irqc->info.el2_regs) >> + return; >> + >> /* Only the guest timers have real mask bits, unfortunately. */ >> switch (aic_fiq_get_idx(d)) { >> case AIC_TMR_EL02_PHYS: >> @@ -469,6 +481,9 @@ static void aic_fiq_set_mask(struct irq_data *d) >> >> static void aic_fiq_clear_mask(struct irq_data *d) >> { >> + if (!aic_irqc->info.el2_regs) >> + return; >> + >> switch (aic_fiq_get_idx(d)) { >> case AIC_TMR_EL02_PHYS: >> sysreg_clear_set_s(SYS_IMP_APL_VM_TMR_FIQ_ENA_EL2, 0, VM_TMR_FIQ_ENABLE_P); >> @@ -524,12 +539,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 (aic_irqc->info.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); >> + } >> } >> } >> >> @@ -566,12 +583,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 (aic_irqc->info.uncore2_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)); >> + } >> } >> } >> >> @@ -676,7 +695,8 @@ static int aic_irq_domain_translate(struct irq_domain *id, >> break; >> case AIC_TMR_HV_PHYS: >> case AIC_TMR_HV_VIRT: >> - return -ENOENT; >> + if (aic_irqc->info.el2_regs) >> + return -ENOENT; >> default: >> break; >> } >> @@ -944,7 +964,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 (aic_irqc->info.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 +986,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 (aic_irqc->info.uncore2_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(); >> -- >> 2.37.0 >>
marcan probably has to review this in detail but two comments from me: On Tue, Jul 12, 2022, at 18:09, Konrad Dybcio wrote: > Add support for A7-A11 SoCs by if-ing out some features only present on > A12 & newer (UNCORE2 registers) or M1 & newer (EL2 registers - the > older SoCs don't implement EL2). > > Also, annotate IPI regs support (A11 and newer*) so that the driver can > tell whether the SoC supports these (they are written to even if fast > IPI is disabled, when the registers are there of course). > > *A11 is supposed to use this feature, but it is currently not working. > That said, it is not yet necessary, especially with only one core up, > and it works a-ok using the same featureset as earlier SoCs. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > --- > drivers/irqchip/irq-apple-aic.c | 54 +++++++++++++++++++++++---------- > 1 file changed, 38 insertions(+), 16 deletions(-) > > diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c > index 12dd48727a15..36f4b52addc2 100644 > --- a/drivers/irqchip/irq-apple-aic.c > +++ b/drivers/irqchip/irq-apple-aic.c > @@ -245,7 +245,10 @@ struct aic_info { > u32 die_stride; > > /* Features */ > + bool el2_regs; > bool fast_ipi; > + bool ipi_regs; > + bool uncore2_regs; I don't quite understand the difference between fast_ipi and ipi_regs. Don't we always have fast_ipi suppport when those regs are available? > }; > > static const struct aic_info aic1_info = { > @@ -261,7 +264,10 @@ static const struct aic_info aic1_fipi_info = { > .event = AIC_EVENT, > .target_cpu = AIC_TARGET_CPU, > > + .el2_regs = true, > .fast_ipi = true, > + .ipi_regs = true, > + .uncore2_regs = true, > }; > > static const struct aic_info aic2_info = { > @@ -269,7 +275,10 @@ static const struct aic_info aic2_info = { > > .irq_cfg = AIC2_IRQ_CFG, > > + .el2_regs = true, > .fast_ipi = true, > + .ipi_regs = true, > + .uncore2_regs = true, > }; > > static const struct of_device_id aic_info_match[] = { > @@ -452,6 +461,9 @@ static unsigned long aic_fiq_get_idx(struct irq_data *d) > > static void aic_fiq_set_mask(struct irq_data *d) > { > + if (!aic_irqc->info.el2_regs) > + return; > + > /* Only the guest timers have real mask bits, unfortunately. */ > switch (aic_fiq_get_idx(d)) { > case AIC_TMR_EL02_PHYS: > @@ -469,6 +481,9 @@ static void aic_fiq_set_mask(struct irq_data *d) > > static void aic_fiq_clear_mask(struct irq_data *d) > { > + if (!aic_irqc->info.el2_regs) > + return; > + > switch (aic_fiq_get_idx(d)) { > case AIC_TMR_EL02_PHYS: > sysreg_clear_set_s(SYS_IMP_APL_VM_TMR_FIQ_ENA_EL2, 0, > VM_TMR_FIQ_ENABLE_P); > @@ -524,12 +539,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 (aic_irqc->info.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 is a pretty hot path and the use_fast_ipi check uses the jump label support (static_branch_likely, static_branch_enable) to avoid dereferencing memory here. We'll probably want the same for the other features. For this branch here the else can probably just be removed: I think that's a leftover from when this driver just didn't support fastipi at all even when the registers were available. Sven
On 12.07.2022 20:52, Sven Peter wrote: > marcan probably has to review this in detail but two comments from me: > > On Tue, Jul 12, 2022, at 18:09, Konrad Dybcio wrote: >> Add support for A7-A11 SoCs by if-ing out some features only present on >> A12 & newer (UNCORE2 registers) or M1 & newer (EL2 registers - the >> older SoCs don't implement EL2). >> >> Also, annotate IPI regs support (A11 and newer*) so that the driver can >> tell whether the SoC supports these (they are written to even if fast >> IPI is disabled, when the registers are there of course). >> >> *A11 is supposed to use this feature, but it is currently not working. >> That said, it is not yet necessary, especially with only one core up, >> and it works a-ok using the same featureset as earlier SoCs. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> >> --- >> drivers/irqchip/irq-apple-aic.c | 54 +++++++++++++++++++++++---------- >> 1 file changed, 38 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c >> index 12dd48727a15..36f4b52addc2 100644 >> --- a/drivers/irqchip/irq-apple-aic.c >> +++ b/drivers/irqchip/irq-apple-aic.c >> @@ -245,7 +245,10 @@ struct aic_info { >> u32 die_stride; >> >> /* Features */ >> + bool el2_regs; >> bool fast_ipi; >> + bool ipi_regs; >> + bool uncore2_regs; > > I don't quite understand the difference between fast_ipi and ipi_regs. > Don't we always have fast_ipi suppport when those regs are available? https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/irqchip/irq-apple-aic.c?h=next-20220712#n532 Both cases invoke accessing IPI regs, and there was a ipi/no-ipi variant before, so I didn't want to mess with that. > >> }; >> >> static const struct aic_info aic1_info = { >> @@ -261,7 +264,10 @@ static const struct aic_info aic1_fipi_info = { >> .event = AIC_EVENT, >> .target_cpu = AIC_TARGET_CPU, >> >> + .el2_regs = true, >> .fast_ipi = true, >> + .ipi_regs = true, >> + .uncore2_regs = true, >> }; >> >> static const struct aic_info aic2_info = { >> @@ -269,7 +275,10 @@ static const struct aic_info aic2_info = { >> >> .irq_cfg = AIC2_IRQ_CFG, >> >> + .el2_regs = true, >> .fast_ipi = true, >> + .ipi_regs = true, >> + .uncore2_regs = true, >> }; >> >> static const struct of_device_id aic_info_match[] = { >> @@ -452,6 +461,9 @@ static unsigned long aic_fiq_get_idx(struct irq_data *d) >> >> static void aic_fiq_set_mask(struct irq_data *d) >> { >> + if (!aic_irqc->info.el2_regs) >> + return; >> + >> /* Only the guest timers have real mask bits, unfortunately. */ >> switch (aic_fiq_get_idx(d)) { >> case AIC_TMR_EL02_PHYS: >> @@ -469,6 +481,9 @@ static void aic_fiq_set_mask(struct irq_data *d) >> >> static void aic_fiq_clear_mask(struct irq_data *d) >> { >> + if (!aic_irqc->info.el2_regs) >> + return; >> + >> switch (aic_fiq_get_idx(d)) { >> case AIC_TMR_EL02_PHYS: >> sysreg_clear_set_s(SYS_IMP_APL_VM_TMR_FIQ_ENA_EL2, 0, >> VM_TMR_FIQ_ENABLE_P); >> @@ -524,12 +539,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 (aic_irqc->info.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 is a pretty hot path and the use_fast_ipi check uses the jump label support > (static_branch_likely, static_branch_enable) to avoid dereferencing memory here. > We'll probably want the same for the other features. > > For this branch here the else can probably just be removed: I think that's > a leftover from when this driver just didn't support fastipi at all even > when the registers were available. If there's no use for non-fast-ipi paths, perhaps they can just be removed? That could simplify the fast_ipi/ipi_regs situation. Konrad > > > > Sven >
Hi Konrad, Please add a cover letter when sending more than a single patch. On Tue, 12 Jul 2022 17:09:19 +0100, Konrad Dybcio <konrad.dybcio@somainline.org> wrote: > > Add support for A7-A11 SoCs by if-ing out some features only present on > A12 & newer (UNCORE2 registers) or M1 & newer (EL2 registers - the > older SoCs don't implement EL2). > > Also, annotate IPI regs support (A11 and newer*) so that the driver can > tell whether the SoC supports these (they are written to even if fast > IPI is disabled, when the registers are there of course). > > *A11 is supposed to use this feature, but it is currently not working. > That said, it is not yet necessary, especially with only one core up, > and it works a-ok using the same featureset as earlier SoCs. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > --- > drivers/irqchip/irq-apple-aic.c | 54 +++++++++++++++++++++++---------- > 1 file changed, 38 insertions(+), 16 deletions(-) > > diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c > index 12dd48727a15..36f4b52addc2 100644 > --- a/drivers/irqchip/irq-apple-aic.c > +++ b/drivers/irqchip/irq-apple-aic.c > @@ -245,7 +245,10 @@ struct aic_info { > u32 die_stride; > > /* Features */ > + bool el2_regs; > bool fast_ipi; > + bool ipi_regs; > + bool uncore2_regs; > }; > > static const struct aic_info aic1_info = { > @@ -261,7 +264,10 @@ static const struct aic_info aic1_fipi_info = { > .event = AIC_EVENT, > .target_cpu = AIC_TARGET_CPU, > > + .el2_regs = true, > .fast_ipi = true, > + .ipi_regs = true, > + .uncore2_regs = true, > }; > > static const struct aic_info aic2_info = { > @@ -269,7 +275,10 @@ static const struct aic_info aic2_info = { > > .irq_cfg = AIC2_IRQ_CFG, > > + .el2_regs = true, > .fast_ipi = true, > + .ipi_regs = true, > + .uncore2_regs = true, So to sum it up, all recent cores have all the cool features, and the older ones have none of them. Surely we can do better than adding 3 fields that have the same value. Turn 'fast_ipi' into something that means 'full_fat', and key everything on that. And if this is meant to evolve into a more differentiated set of features, the usual idiom is to have a set of flags as part of an unsigned long instead of a set of booleans. > }; > > static const struct of_device_id aic_info_match[] = { > @@ -452,6 +461,9 @@ static unsigned long aic_fiq_get_idx(struct irq_data *d) > > static void aic_fiq_set_mask(struct irq_data *d) > { > + if (!aic_irqc->info.el2_regs) > + return; Why? AIC_TMR_EL02_PHYS is defined as the interrupt that fires in the context of a guest. There is no guest here (no EL2 either), so what you should have as interrupt number is AIC_TMR_EL0_{PHYS,VIRT}, and this change becomes irrelevant (nothing to mask). Which is also what happens when running an M1 under the m1n1 hypervisor. > + > /* Only the guest timers have real mask bits, unfortunately. */ > switch (aic_fiq_get_idx(d)) { > case AIC_TMR_EL02_PHYS: > @@ -469,6 +481,9 @@ static void aic_fiq_set_mask(struct irq_data *d) > > static void aic_fiq_clear_mask(struct irq_data *d) > { > + if (!aic_irqc->info.el2_regs) > + return; > + > switch (aic_fiq_get_idx(d)) { > case AIC_TMR_EL02_PHYS: > sysreg_clear_set_s(SYS_IMP_APL_VM_TMR_FIQ_ENA_EL2, 0, VM_TMR_FIQ_ENABLE_P); > @@ -524,12 +539,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 (aic_irqc->info.ipi_regs) { This is probably the hottest path in the whole kernel. Do we want an extra read here? Absolutely not. At the very least, this should be a static key. > + 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); > + } > } > } > > @@ -566,12 +583,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 (aic_irqc->info.uncore2_regs) { Same thing. > + 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)); > + } > } > } > > @@ -676,7 +695,8 @@ static int aic_irq_domain_translate(struct irq_domain *id, > break; > case AIC_TMR_HV_PHYS: > case AIC_TMR_HV_VIRT: > - return -ENOENT; > + if (aic_irqc->info.el2_regs) > + return -ENOENT; See my comment above about the use of these interrupt numbers. > default: > break; > } > @@ -944,7 +964,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 (aic_irqc->info.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 +986,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 (aic_irqc->info.uncore2_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(); I must be missing something though. Where is the code that actually enables support for the SoCs mentioned in $SUBJECT? Thanks, M.
On 12.07.2022 21:12, Marc Zyngier wrote: > Hi Konrad, > > Please add a cover letter when sending more than a single patch. > > On Tue, 12 Jul 2022 17:09:19 +0100, > Konrad Dybcio <konrad.dybcio@somainline.org> wrote: >> >> Add support for A7-A11 SoCs by if-ing out some features only present on >> A12 & newer (UNCORE2 registers) or M1 & newer (EL2 registers - the >> older SoCs don't implement EL2). >> >> Also, annotate IPI regs support (A11 and newer*) so that the driver can >> tell whether the SoC supports these (they are written to even if fast >> IPI is disabled, when the registers are there of course). >> >> *A11 is supposed to use this feature, but it is currently not working. >> That said, it is not yet necessary, especially with only one core up, >> and it works a-ok using the same featureset as earlier SoCs. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> >> --- >> drivers/irqchip/irq-apple-aic.c | 54 +++++++++++++++++++++++---------- >> 1 file changed, 38 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c >> index 12dd48727a15..36f4b52addc2 100644 >> --- a/drivers/irqchip/irq-apple-aic.c >> +++ b/drivers/irqchip/irq-apple-aic.c >> @@ -245,7 +245,10 @@ struct aic_info { >> u32 die_stride; >> >> /* Features */ >> + bool el2_regs; >> bool fast_ipi; >> + bool ipi_regs; >> + bool uncore2_regs; >> }; >> >> static const struct aic_info aic1_info = { >> @@ -261,7 +264,10 @@ static const struct aic_info aic1_fipi_info = { >> .event = AIC_EVENT, >> .target_cpu = AIC_TARGET_CPU, >> >> + .el2_regs = true, >> .fast_ipi = true, >> + .ipi_regs = true, >> + .uncore2_regs = true, >> }; >> >> static const struct aic_info aic2_info = { >> @@ -269,7 +275,10 @@ static const struct aic_info aic2_info = { >> >> .irq_cfg = AIC2_IRQ_CFG, >> >> + .el2_regs = true, >> .fast_ipi = true, >> + .ipi_regs = true, >> + .uncore2_regs = true, > > So to sum it up, all recent cores have all the cool features, and the > older ones have none of them. Surely we can do better than adding 3 > fields that have the same value. Turn 'fast_ipi' into something that > means 'full_fat', and key everything on that. > > And if this is meant to evolve into a more differentiated set of > features, the usual idiom is to have a set of flags as part of an > unsigned long instead of a set of booleans. The latter would be true if a bootrom exploit or any equivalent means of booting Linux would be found for A12 (M1 is family with A14 for context). We can think of 4 feature levels, I think: A7-A10: 'nothing fancy' A11: fast_ipi (broken currently, need to investigate) A12: A11 + UNCORE2 regs M1+: A12 + EL2 We *could* squash the A12-A14 case into M1, but then if a means of booting Linux appears, this would have to be untangled again.. > >> }; >> >> static const struct of_device_id aic_info_match[] = { >> @@ -452,6 +461,9 @@ static unsigned long aic_fiq_get_idx(struct irq_data *d) >> >> static void aic_fiq_set_mask(struct irq_data *d) >> { >> + if (!aic_irqc->info.el2_regs) >> + return; > > Why? AIC_TMR_EL02_PHYS is defined as the interrupt that fires in the > context of a guest. There is no guest here (no EL2 either), so what > you should have as interrupt number is AIC_TMR_EL0_{PHYS,VIRT}, and > this change becomes irrelevant (nothing to mask). Which is also what > happens when running an M1 under the m1n1 hypervisor. This func accesses impl-defined regs that are not present on earlier SoCs. > >> + >> /* Only the guest timers have real mask bits, unfortunately. */ >> switch (aic_fiq_get_idx(d)) { >> case AIC_TMR_EL02_PHYS: >> @@ -469,6 +481,9 @@ static void aic_fiq_set_mask(struct irq_data *d) >> >> static void aic_fiq_clear_mask(struct irq_data *d) >> { >> + if (!aic_irqc->info.el2_regs) >> + return; >> + >> switch (aic_fiq_get_idx(d)) { >> case AIC_TMR_EL02_PHYS: >> sysreg_clear_set_s(SYS_IMP_APL_VM_TMR_FIQ_ENA_EL2, 0, VM_TMR_FIQ_ENABLE_P); >> @@ -524,12 +539,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 (aic_irqc->info.ipi_regs) { > > This is probably the hottest path in the whole kernel. Do we want an > extra read here? Absolutely not. At the very least, this should be a > static key. Yeah, makes sense.. > >> + 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); >> + } >> } >> } >> >> @@ -566,12 +583,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 (aic_irqc->info.uncore2_regs) { > > Same thing. > >> + 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)); >> + } >> } >> } >> >> @@ -676,7 +695,8 @@ static int aic_irq_domain_translate(struct irq_domain *id, >> break; >> case AIC_TMR_HV_PHYS: >> case AIC_TMR_HV_VIRT: >> - return -ENOENT; >> + if (aic_irqc->info.el2_regs) >> + return -ENOENT; > > See my comment above about the use of these interrupt numbers. `if (!is_kernel_in_hyp_mode()) {` always evaluates to true, since there's no EL2. Hence, accessing AIC_TMR_HV_{VIRT,PHYS} makes this return ENOENT, which means timer can't probe and that's no bueno. > >> default: >> break; >> } >> @@ -944,7 +964,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 (aic_irqc->info.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 +986,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 (aic_irqc->info.uncore2_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(); > > I must be missing something though. Where is the code that actually > enables support for the SoCs mentioned in $SUBJECT? In this peculiar case, enabling support means stripping away the so-called 'features', otherwise the interrupt controller won't budge. Konrad > > Thanks, > > M. >
On Tue, Jul 12, 2022, at 21:23, Konrad Dybcio wrote: > On 12.07.2022 21:12, Marc Zyngier wrote: >> Hi Konrad, >> >> Please add a cover letter when sending more than a single patch. >> >> On Tue, 12 Jul 2022 17:09:19 +0100, >> Konrad Dybcio <konrad.dybcio@somainline.org> wrote: >>> >>> Add support for A7-A11 SoCs by if-ing out some features only present on >>> A12 & newer (UNCORE2 registers) or M1 & newer (EL2 registers - the >>> older SoCs don't implement EL2). >>> >>> Also, annotate IPI regs support (A11 and newer*) so that the driver can >>> tell whether the SoC supports these (they are written to even if fast >>> IPI is disabled, when the registers are there of course). >>> >>> *A11 is supposed to use this feature, but it is currently not working. >>> That said, it is not yet necessary, especially with only one core up, >>> and it works a-ok using the same featureset as earlier SoCs. >>> >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> >>> --- >>> drivers/irqchip/irq-apple-aic.c | 54 +++++++++++++++++++++++---------- >>> 1 file changed, 38 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c >>> index 12dd48727a15..36f4b52addc2 100644 >>> --- a/drivers/irqchip/irq-apple-aic.c >>> +++ b/drivers/irqchip/irq-apple-aic.c >>> @@ -245,7 +245,10 @@ struct aic_info { >>> u32 die_stride; >>> >>> /* Features */ >>> + bool el2_regs; >>> bool fast_ipi; >>> + bool ipi_regs; >>> + bool uncore2_regs; >>> }; >>> >>> static const struct aic_info aic1_info = { >>> @@ -261,7 +264,10 @@ static const struct aic_info aic1_fipi_info = { >>> .event = AIC_EVENT, >>> .target_cpu = AIC_TARGET_CPU, >>> >>> + .el2_regs = true, >>> .fast_ipi = true, >>> + .ipi_regs = true, >>> + .uncore2_regs = true, >>> }; >>> >>> static const struct aic_info aic2_info = { >>> @@ -269,7 +275,10 @@ static const struct aic_info aic2_info = { >>> >>> .irq_cfg = AIC2_IRQ_CFG, >>> >>> + .el2_regs = true, >>> .fast_ipi = true, >>> + .ipi_regs = true, >>> + .uncore2_regs = true, >> >> So to sum it up, all recent cores have all the cool features, and the >> older ones have none of them. Surely we can do better than adding 3 >> fields that have the same value. Turn 'fast_ipi' into something that >> means 'full_fat', and key everything on that. >> >> And if this is meant to evolve into a more differentiated set of >> features, the usual idiom is to have a set of flags as part of an >> unsigned long instead of a set of booleans. > The latter would be true if a bootrom exploit or any equivalent means > of booting Linux would be found for A12 (M1 is family with A14 for context). > > We can think of 4 feature levels, I think: > > A7-A10: 'nothing fancy' > A11: fast_ipi (broken currently, need to investigate) > A12: A11 + UNCORE2 regs > M1+: A12 + EL2 > > We *could* squash the A12-A14 case into M1, but then if a means of booting > Linux appears, this would have to be untangled again.. > >> >>> }; >>> >>> static const struct of_device_id aic_info_match[] = { >>> @@ -452,6 +461,9 @@ static unsigned long aic_fiq_get_idx(struct irq_data *d) >>> >>> static void aic_fiq_set_mask(struct irq_data *d) >>> { >>> + if (!aic_irqc->info.el2_regs) >>> + return; >> >> Why? AIC_TMR_EL02_PHYS is defined as the interrupt that fires in the >> context of a guest. There is no guest here (no EL2 either), so what >> you should have as interrupt number is AIC_TMR_EL0_{PHYS,VIRT}, and >> this change becomes irrelevant (nothing to mask). Which is also what >> happens when running an M1 under the m1n1 hypervisor. > This func accesses impl-defined regs that are not present on earlier SoCs. > >> >>> + >>> /* Only the guest timers have real mask bits, unfortunately. */ >>> switch (aic_fiq_get_idx(d)) { >>> case AIC_TMR_EL02_PHYS: >>> @@ -469,6 +481,9 @@ static void aic_fiq_set_mask(struct irq_data *d) >>> >>> static void aic_fiq_clear_mask(struct irq_data *d) >>> { >>> + if (!aic_irqc->info.el2_regs) >>> + return; >>> + >>> switch (aic_fiq_get_idx(d)) { >>> case AIC_TMR_EL02_PHYS: >>> sysreg_clear_set_s(SYS_IMP_APL_VM_TMR_FIQ_ENA_EL2, 0, VM_TMR_FIQ_ENABLE_P); >>> @@ -524,12 +539,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 (aic_irqc->info.ipi_regs) { >> >> This is probably the hottest path in the whole kernel. Do we want an >> extra read here? Absolutely not. At the very least, this should be a >> static key. > Yeah, makes sense.. > > >> >>> + 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); >>> + } >>> } >>> } >>> >>> @@ -566,12 +583,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 (aic_irqc->info.uncore2_regs) { >> >> Same thing. >> >>> + 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)); >>> + } >>> } >>> } >>> >>> @@ -676,7 +695,8 @@ static int aic_irq_domain_translate(struct irq_domain *id, >>> break; >>> case AIC_TMR_HV_PHYS: >>> case AIC_TMR_HV_VIRT: >>> - return -ENOENT; >>> + if (aic_irqc->info.el2_regs) >>> + return -ENOENT; >> >> See my comment above about the use of these interrupt numbers. > `if (!is_kernel_in_hyp_mode()) {` always evaluates to true, since there's > no EL2. Hence, accessing AIC_TMR_HV_{VIRT,PHYS} makes this return ENOENT, > which means timer can't probe and that's no bueno. Sounds like an issue with your device tree. There should be no reference to AIC_TMR_HV_{VIRT,PHYS} in there. Sven
On Tue, 12 Jul 2022 20:23:31 +0100, Konrad Dybcio <konrad.dybcio@somainline.org> wrote: > > > > On 12.07.2022 21:12, Marc Zyngier wrote: > > Hi Konrad, > > > > Please add a cover letter when sending more than a single patch. > > > > On Tue, 12 Jul 2022 17:09:19 +0100, > > Konrad Dybcio <konrad.dybcio@somainline.org> wrote: > >> > >> Add support for A7-A11 SoCs by if-ing out some features only present on > >> A12 & newer (UNCORE2 registers) or M1 & newer (EL2 registers - the > >> older SoCs don't implement EL2). > >> > >> Also, annotate IPI regs support (A11 and newer*) so that the driver can > >> tell whether the SoC supports these (they are written to even if fast > >> IPI is disabled, when the registers are there of course). > >> > >> *A11 is supposed to use this feature, but it is currently not working. > >> That said, it is not yet necessary, especially with only one core up, > >> and it works a-ok using the same featureset as earlier SoCs. > >> > >> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > >> --- > >> drivers/irqchip/irq-apple-aic.c | 54 +++++++++++++++++++++++---------- > >> 1 file changed, 38 insertions(+), 16 deletions(-) > >> > >> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c > >> index 12dd48727a15..36f4b52addc2 100644 > >> --- a/drivers/irqchip/irq-apple-aic.c > >> +++ b/drivers/irqchip/irq-apple-aic.c > >> @@ -245,7 +245,10 @@ struct aic_info { > >> u32 die_stride; > >> > >> /* Features */ > >> + bool el2_regs; > >> bool fast_ipi; > >> + bool ipi_regs; > >> + bool uncore2_regs; > >> }; > >> > >> static const struct aic_info aic1_info = { > >> @@ -261,7 +264,10 @@ static const struct aic_info aic1_fipi_info = { > >> .event = AIC_EVENT, > >> .target_cpu = AIC_TARGET_CPU, > >> > >> + .el2_regs = true, > >> .fast_ipi = true, > >> + .ipi_regs = true, > >> + .uncore2_regs = true, > >> }; > >> > >> static const struct aic_info aic2_info = { > >> @@ -269,7 +275,10 @@ static const struct aic_info aic2_info = { > >> > >> .irq_cfg = AIC2_IRQ_CFG, > >> > >> + .el2_regs = true, > >> .fast_ipi = true, > >> + .ipi_regs = true, > >> + .uncore2_regs = true, > > > > So to sum it up, all recent cores have all the cool features, and the > > older ones have none of them. Surely we can do better than adding 3 > > fields that have the same value. Turn 'fast_ipi' into something that > > means 'full_fat', and key everything on that. > > > > And if this is meant to evolve into a more differentiated set of > > features, the usual idiom is to have a set of flags as part of an > > unsigned long instead of a set of booleans. > The latter would be true if a bootrom exploit or any equivalent means > of booting Linux would be found for A12 (M1 is family with A14 for context). > > We can think of 4 feature levels, I think: > > A7-A10: 'nothing fancy' > A11: fast_ipi (broken currently, need to investigate) > A12: A11 + UNCORE2 regs > M1+: A12 + EL2 > > We *could* squash the A12-A14 case into M1, but then if a means of booting > Linux appears, this would have to be untangled again.. We don't add code for systems that could only hypothetically run Linux. If and when this becomes possible, we'll add support for them. In the meantime, I suggest you focus on supporting what actually works. > > > > >> }; > >> > >> static const struct of_device_id aic_info_match[] = { > >> @@ -452,6 +461,9 @@ static unsigned long aic_fiq_get_idx(struct irq_data *d) > >> > >> static void aic_fiq_set_mask(struct irq_data *d) > >> { > >> + if (!aic_irqc->info.el2_regs) > >> + return; > > > > Why? AIC_TMR_EL02_PHYS is defined as the interrupt that fires in the > > context of a guest. There is no guest here (no EL2 either), so what > > you should have as interrupt number is AIC_TMR_EL0_{PHYS,VIRT}, and > > this change becomes irrelevant (nothing to mask). Which is also what > > happens when running an M1 under the m1n1 hypervisor. > This func accesses impl-defined regs that are not present on earlier SoCs. You're missing my point. Why are you encoding your timer interrupts with a hwirq that requires you to skip existing code? If you used the interrupt number that represent a bare-metal interrupt, you'd be just fine. Specially considering that the interrupt numbers in the DT are nothing but made-up numbers, as there is no interrupt controller to speak of. > >> @@ -676,7 +695,8 @@ static int aic_irq_domain_translate(struct irq_domain *id, > >> break; > >> case AIC_TMR_HV_PHYS: > >> case AIC_TMR_HV_VIRT: > >> - return -ENOENT; > >> + if (aic_irqc->info.el2_regs) > >> + return -ENOENT; > > > > See my comment above about the use of these interrupt numbers. > `if (!is_kernel_in_hyp_mode()) {` always evaluates to true, since there's > no EL2. Hence, accessing AIC_TMR_HV_{VIRT,PHYS} makes this return ENOENT, > which means timer can't probe and that's no bueno. Again, you have the wrong end of the stick, and this is about changing your DT rather than the driver. > > > > > >> default: > >> break; > >> } > >> @@ -944,7 +964,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 (aic_irqc->info.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 +986,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 (aic_irqc->info.uncore2_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(); > > > > I must be missing something though. Where is the code that actually > > enables support for the SoCs mentioned in $SUBJECT? > In this peculiar case, enabling support means stripping away the so-called > 'features', otherwise the interrupt controller won't budge. What I would like to see is a different compatibility string that makes the support for a given IP explicit instead of making everything implicit. M.
diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c index 12dd48727a15..36f4b52addc2 100644 --- a/drivers/irqchip/irq-apple-aic.c +++ b/drivers/irqchip/irq-apple-aic.c @@ -245,7 +245,10 @@ struct aic_info { u32 die_stride; /* Features */ + bool el2_regs; bool fast_ipi; + bool ipi_regs; + bool uncore2_regs; }; static const struct aic_info aic1_info = { @@ -261,7 +264,10 @@ static const struct aic_info aic1_fipi_info = { .event = AIC_EVENT, .target_cpu = AIC_TARGET_CPU, + .el2_regs = true, .fast_ipi = true, + .ipi_regs = true, + .uncore2_regs = true, }; static const struct aic_info aic2_info = { @@ -269,7 +275,10 @@ static const struct aic_info aic2_info = { .irq_cfg = AIC2_IRQ_CFG, + .el2_regs = true, .fast_ipi = true, + .ipi_regs = true, + .uncore2_regs = true, }; static const struct of_device_id aic_info_match[] = { @@ -452,6 +461,9 @@ static unsigned long aic_fiq_get_idx(struct irq_data *d) static void aic_fiq_set_mask(struct irq_data *d) { + if (!aic_irqc->info.el2_regs) + return; + /* Only the guest timers have real mask bits, unfortunately. */ switch (aic_fiq_get_idx(d)) { case AIC_TMR_EL02_PHYS: @@ -469,6 +481,9 @@ static void aic_fiq_set_mask(struct irq_data *d) static void aic_fiq_clear_mask(struct irq_data *d) { + if (!aic_irqc->info.el2_regs) + return; + switch (aic_fiq_get_idx(d)) { case AIC_TMR_EL02_PHYS: sysreg_clear_set_s(SYS_IMP_APL_VM_TMR_FIQ_ENA_EL2, 0, VM_TMR_FIQ_ENABLE_P); @@ -524,12 +539,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 (aic_irqc->info.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); + } } } @@ -566,12 +583,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 (aic_irqc->info.uncore2_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)); + } } } @@ -676,7 +695,8 @@ static int aic_irq_domain_translate(struct irq_domain *id, break; case AIC_TMR_HV_PHYS: case AIC_TMR_HV_VIRT: - return -ENOENT; + if (aic_irqc->info.el2_regs) + return -ENOENT; default: break; } @@ -944,7 +964,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 (aic_irqc->info.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 +986,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 (aic_irqc->info.uncore2_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();
Add support for A7-A11 SoCs by if-ing out some features only present on A12 & newer (UNCORE2 registers) or M1 & newer (EL2 registers - the older SoCs don't implement EL2). Also, annotate IPI regs support (A11 and newer*) so that the driver can tell whether the SoC supports these (they are written to even if fast IPI is disabled, when the registers are there of course). *A11 is supposed to use this feature, but it is currently not working. That said, it is not yet necessary, especially with only one core up, and it works a-ok using the same featureset as earlier SoCs. Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> --- drivers/irqchip/irq-apple-aic.c | 54 +++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 16 deletions(-)