Messages in this thread | | | Date | Tue, 12 Jul 2022 20:52:08 +0200 | From | "Sven Peter" <> | Subject | Re: [PATCH 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs |
| |
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
| |