* [PATCH v2 1/2] dt-bindings: apple,aic: Document A7-A11 compatibles @ 2022-09-29 14:40 Konrad Dybcio 2022-09-29 14:40 ` [PATCH v2 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs Konrad Dybcio 0 siblings, 1 reply; 6+ messages in thread From: Konrad Dybcio @ 2022-09-29 14:40 UTC (permalink / raw) To: asahi, linux-arm-kernel Cc: towinchenmi, Konrad Dybcio, Hector Martin, Sven Peter, Alyssa Rosenzweig, Thomas Gleixner, Marc Zyngier, Rob Herring, Krzysztof Kozlowski, linux-kernel, devicetree Document the compatibles for Apple A7-A11 SoCs. Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> --- Changes since v1: - separate t8015 out, it does not fall back to s5l8960x .../bindings/interrupt-controller/apple,aic.yaml | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml b/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml index e18107eafe7c..0619dd73a7b0 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml +++ b/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml @@ -36,9 +36,19 @@ allOf: properties: compatible: - items: - - const: apple,t8103-aic - - const: apple,aic + oneOf: + - const: apple,s5l8960x-aic + - items: + - enum: + - apple,s8000-aic + - apple,t7000-aic + - apple,t8010-aic + - const: apple,s5l8960x-aic + - items: + - enum: + - apple,t8015-aic + - apple,t8103-aic + - const: apple,aic interrupt-controller: true -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs 2022-09-29 14:40 [PATCH v2 1/2] dt-bindings: apple,aic: Document A7-A11 compatibles Konrad Dybcio @ 2022-09-29 14:40 ` Konrad Dybcio 2022-09-29 14:50 ` Sven Peter 0 siblings, 1 reply; 6+ messages in thread From: Konrad Dybcio @ 2022-09-29 14:40 UTC (permalink / raw) To: asahi, linux-arm-kernel Cc: towinchenmi, Konrad Dybcio, Hector Martin, Sven Peter, Alyssa Rosenzweig, Thomas Gleixner, Marc Zyngier, Rob Herring, Krzysztof Kozlowski, linux-kernel, devicetree Add support for A7-A11 SoCs by if-ing out some features only present on: * A11 & newer (implementation-defined IPI & UNCORE registers) * A11[1] & newer (fast IPI support). 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. 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. [1] A11 is supposed to use this feature, but it currently doesn't work for reasons unknown and hence remains disabled. It can easily be enabled on A11 only, as there is a SoC-specific compatible in the DT with a fallback to apple,aic, so that the interrupt controller gets to probe regardless of whether IPI Sn_... registers are used or not. That said, it is not yet necessary, especially with only one core up, and it has worked a-ok so far. Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> --- Changes since v1: - remove EL2 register check (dts change covered this) - use static_branch instead of ifs - rename "uncore2 registers" to "uncore registers" in added code and update the commit message accordingly - create a "legacy" config struct for pre-A11 targets - rewrite the commit message a bit to match actual status drivers/irqchip/irq-apple-aic.c | 56 ++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c index 1c2813ad8bbe..cdef99bfcfb3 100644 --- a/drivers/irqchip/irq-apple-aic.c +++ b/drivers/irqchip/irq-apple-aic.c @@ -229,6 +229,7 @@ #define AIC_TMR_EL02_VIRT AIC_TMR_GUEST_VIRT static DEFINE_STATIC_KEY_TRUE(use_fast_ipi); +static DEFINE_STATIC_KEY_TRUE(has_uncore_regs); struct aic_info { int version; @@ -246,6 +247,7 @@ struct aic_info { /* Features */ bool fast_ipi; + bool uncore_regs; }; static const struct aic_info aic1_info = { @@ -253,6 +255,8 @@ static const struct aic_info aic1_info = { .event = AIC_EVENT, .target_cpu = AIC_TARGET_CPU, + + .uncore_regs = true, }; static const struct aic_info aic1_fipi_info = { @@ -264,6 +268,13 @@ static const struct aic_info aic1_fipi_info = { .fast_ipi = true, }; +static const struct aic_info aic1_legacy_info = { + .version = 1, + + .event = AIC_EVENT, + .target_cpu = AIC_TARGET_CPU, +}; + static const struct aic_info aic2_info = { .version = 2, @@ -273,6 +284,10 @@ static const struct aic_info aic2_info = { }; static const struct of_device_id aic_info_match[] = { + { + .compatible = "apple,s5l8960x-aic", + .data = &aic1_legacy_info, + }, { .compatible = "apple,t8103-aic", .data = &aic1_fipi_info, @@ -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 (static_branch_likely(&use_fast_ipi)) { + 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 (static_branch_likely(&has_uncore_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 +963,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(&use_fast_ipi)) + 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 +985,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_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 +1146,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_regs) + static_branch_enable(&has_uncore_regs); + else + static_branch_disable(&has_uncore_regs); + irqc->info.die_stride = off - start_off; irqc->hw_domain = irq_domain_create_tree(of_node_to_fwnode(node), -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs 2022-09-29 14:40 ` [PATCH v2 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs Konrad Dybcio @ 2022-09-29 14:50 ` Sven Peter 2022-09-29 17:10 ` Konrad Dybcio 2022-09-29 19:02 ` Mark Kettenis 0 siblings, 2 replies; 6+ messages in thread From: Sven Peter @ 2022-09-29 14:50 UTC (permalink / raw) To: Konrad Dybcio, asahi, linux-arm-kernel Cc: towinchenmi, Hector Martin, Alyssa Rosenzweig, Thomas Gleixner, Marc Zyngier, Rob Herring, Krzysztof Kozlowski, linux-kernel, devicetree On Thu, Sep 29, 2022, at 16:40, Konrad Dybcio wrote: > Add support for A7-A11 SoCs by if-ing out some features only present > on: > > * A11 & newer (implementation-defined IPI & UNCORE registers) > * A11[1] & newer (fast IPI support). > > 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. 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. > > [1] A11 is supposed to use this feature, but it currently doesn't work > for reasons unknown and hence remains disabled. It can easily be enabled > on A11 only, as there is a SoC-specific compatible in the DT with a > fallback to apple,aic, so that the interrupt controller gets to probe > regardless of whether IPI Sn_... registers are used or not. > That said, it is not yet necessary, especially with only one core up, > and it has worked a-ok so far. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > --- > Changes since v1: > - remove EL2 register check (dts change covered this) > - use static_branch instead of ifs > - rename "uncore2 registers" to "uncore registers" in added code and > update the commit message accordingly > - create a "legacy" config struct for pre-A11 targets > - rewrite the commit message a bit to match actual status > > drivers/irqchip/irq-apple-aic.c | 56 ++++++++++++++++++++++++--------- > 1 file changed, 41 insertions(+), 15 deletions(-) > > diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c > index 1c2813ad8bbe..cdef99bfcfb3 100644 > --- a/drivers/irqchip/irq-apple-aic.c > +++ b/drivers/irqchip/irq-apple-aic.c > @@ -229,6 +229,7 @@ > #define AIC_TMR_EL02_VIRT AIC_TMR_GUEST_VIRT > > static DEFINE_STATIC_KEY_TRUE(use_fast_ipi); > +static DEFINE_STATIC_KEY_TRUE(has_uncore_regs); > > struct aic_info { > int version; > @@ -246,6 +247,7 @@ struct aic_info { > > /* Features */ > bool fast_ipi; > + bool uncore_regs; > }; > > static const struct aic_info aic1_info = { > @@ -253,6 +255,8 @@ static const struct aic_info aic1_info = { > > .event = AIC_EVENT, > .target_cpu = AIC_TARGET_CPU, > + > + .uncore_regs = true, > }; > > static const struct aic_info aic1_fipi_info = { > @@ -264,6 +268,13 @@ static const struct aic_info aic1_fipi_info = { > .fast_ipi = true, > }; > > +static const struct aic_info aic1_legacy_info = { > + .version = 1, > + > + .event = AIC_EVENT, > + .target_cpu = AIC_TARGET_CPU, > +}; > + > static const struct aic_info aic2_info = { > .version = 2, > > @@ -273,6 +284,10 @@ static const struct aic_info aic2_info = { > }; > > static const struct of_device_id aic_info_match[] = { > + { > + .compatible = "apple,s5l8960x-aic", > + .data = &aic1_legacy_info, > + }, Maybe I'm confused but shouldn't this be the apple,aic fallback and uncore_regs should be enabled for e.g. t8103-aic then? > { > .compatible = "apple,t8103-aic", > .data = &aic1_fipi_info, > @@ -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 (static_branch_likely(&use_fast_ipi)) { > + 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 doesn't make much sense: if (A) { if (B) { if (A) { // A is already guaranteed to be true here, why check it again? // ... } else { // how can this ever be reached then? } } } > } > } > > @@ -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 (static_branch_likely(&has_uncore_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 +963,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(&use_fast_ipi)) > + 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 +985,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_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 +1146,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_regs) > + static_branch_enable(&has_uncore_regs); > + else > + static_branch_disable(&has_uncore_regs); > + > irqc->info.die_stride = off - start_off; > > irqc->hw_domain = irq_domain_create_tree(of_node_to_fwnode(node), > -- > 2.30.2 Sven ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs 2022-09-29 14:50 ` Sven Peter @ 2022-09-29 17:10 ` Konrad Dybcio 2022-09-29 19:02 ` Mark Kettenis 1 sibling, 0 replies; 6+ messages in thread From: Konrad Dybcio @ 2022-09-29 17:10 UTC (permalink / raw) To: Sven Peter, asahi, linux-arm-kernel Cc: towinchenmi, Hector Martin, Alyssa Rosenzweig, Thomas Gleixner, Marc Zyngier, Rob Herring, Krzysztof Kozlowski, linux-kernel, devicetree On 29/09/2022 16:50, Sven Peter wrote: > On Thu, Sep 29, 2022, at 16:40, Konrad Dybcio wrote: >> Add support for A7-A11 SoCs by if-ing out some features only present >> on: >> >> * A11 & newer (implementation-defined IPI & UNCORE registers) >> * A11[1] & newer (fast IPI support). >> >> 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. 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. >> >> [1] A11 is supposed to use this feature, but it currently doesn't work >> for reasons unknown and hence remains disabled. It can easily be enabled >> on A11 only, as there is a SoC-specific compatible in the DT with a >> fallback to apple,aic, so that the interrupt controller gets to probe >> regardless of whether IPI Sn_... registers are used or not. >> That said, it is not yet necessary, especially with only one core up, >> and it has worked a-ok so far. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> >> --- >> Changes since v1: >> - remove EL2 register check (dts change covered this) >> - use static_branch instead of ifs >> - rename "uncore2 registers" to "uncore registers" in added code and >> update the commit message accordingly >> - create a "legacy" config struct for pre-A11 targets >> - rewrite the commit message a bit to match actual status >> >> drivers/irqchip/irq-apple-aic.c | 56 ++++++++++++++++++++++++--------- >> 1 file changed, 41 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c >> index 1c2813ad8bbe..cdef99bfcfb3 100644 >> --- a/drivers/irqchip/irq-apple-aic.c >> +++ b/drivers/irqchip/irq-apple-aic.c >> @@ -229,6 +229,7 @@ >> #define AIC_TMR_EL02_VIRT AIC_TMR_GUEST_VIRT >> >> static DEFINE_STATIC_KEY_TRUE(use_fast_ipi); >> +static DEFINE_STATIC_KEY_TRUE(has_uncore_regs); >> >> struct aic_info { >> int version; >> @@ -246,6 +247,7 @@ struct aic_info { >> >> /* Features */ >> bool fast_ipi; >> + bool uncore_regs; >> }; >> >> static const struct aic_info aic1_info = { >> @@ -253,6 +255,8 @@ static const struct aic_info aic1_info = { >> >> .event = AIC_EVENT, >> .target_cpu = AIC_TARGET_CPU, >> + >> + .uncore_regs = true, >> }; >> >> static const struct aic_info aic1_fipi_info = { >> @@ -264,6 +268,13 @@ static const struct aic_info aic1_fipi_info = { >> .fast_ipi = true, >> }; >> >> +static const struct aic_info aic1_legacy_info = { >> + .version = 1, >> + >> + .event = AIC_EVENT, >> + .target_cpu = AIC_TARGET_CPU, >> +}; >> + >> static const struct aic_info aic2_info = { >> .version = 2, >> >> @@ -273,6 +284,10 @@ static const struct aic_info aic2_info = { >> }; >> >> static const struct of_device_id aic_info_match[] = { >> + { >> + .compatible = "apple,s5l8960x-aic", >> + .data = &aic1_legacy_info, >> + }, > Maybe I'm confused but shouldn't this be the apple,aic fallback and uncore_regs > should be enabled for e.g. t8103-aic then? Yes, looks like.. > >> { >> .compatible = "apple,t8103-aic", >> .data = &aic1_fipi_info, >> @@ -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 (static_branch_likely(&use_fast_ipi)) { >> + 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 doesn't make much sense: > > if (A) { > if (B) { > if (A) { // A is already guaranteed to be true here, why check it again? > // ... > } else { > // how can this ever be reached then? > } > } > } Good point, I went too far with squashing "has IPI regs" and "use fast IPI" together and didn't notice it's now unreachable.. Konrad > >> } >> } >> >> @@ -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 (static_branch_likely(&has_uncore_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 +963,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(&use_fast_ipi)) >> + 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 +985,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_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 +1146,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_regs) >> + static_branch_enable(&has_uncore_regs); >> + else >> + static_branch_disable(&has_uncore_regs); >> + >> irqc->info.die_stride = off - start_off; >> >> irqc->hw_domain = irq_domain_create_tree(of_node_to_fwnode(node), >> -- >> 2.30.2 > > Sven ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs 2022-09-29 14:50 ` Sven Peter 2022-09-29 17:10 ` Konrad Dybcio @ 2022-09-29 19:02 ` Mark Kettenis 1 sibling, 0 replies; 6+ messages in thread From: Mark Kettenis @ 2022-09-29 19:02 UTC (permalink / raw) To: Sven Peter Cc: konrad.dybcio, asahi, linux-arm-kernel, towinchenmi, marcan, alyssa, tglx, maz, robh+dt, krzysztof.kozlowski+dt, linux-kernel, devicetree > Date: Thu, 29 Sep 2022 16:50:24 +0200 > From: "Sven Peter" <sven@svenpeter.dev> > > On Thu, Sep 29, 2022, at 16:40, Konrad Dybcio wrote: > > Add support for A7-A11 SoCs by if-ing out some features only present > > on: > > > > * A11 & newer (implementation-defined IPI & UNCORE registers) > > * A11[1] & newer (fast IPI support). > > > > 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. 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. > > > > [1] A11 is supposed to use this feature, but it currently doesn't work > > for reasons unknown and hence remains disabled. It can easily be enabled > > on A11 only, as there is a SoC-specific compatible in the DT with a > > fallback to apple,aic, so that the interrupt controller gets to probe > > regardless of whether IPI Sn_... registers are used or not. > > That said, it is not yet necessary, especially with only one core up, > > and it has worked a-ok so far. > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > > --- > > Changes since v1: > > - remove EL2 register check (dts change covered this) > > - use static_branch instead of ifs > > - rename "uncore2 registers" to "uncore registers" in added code and > > update the commit message accordingly > > - create a "legacy" config struct for pre-A11 targets > > - rewrite the commit message a bit to match actual status > > > > drivers/irqchip/irq-apple-aic.c | 56 ++++++++++++++++++++++++--------- > > 1 file changed, 41 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c > > index 1c2813ad8bbe..cdef99bfcfb3 100644 > > --- a/drivers/irqchip/irq-apple-aic.c > > +++ b/drivers/irqchip/irq-apple-aic.c > > @@ -229,6 +229,7 @@ > > #define AIC_TMR_EL02_VIRT AIC_TMR_GUEST_VIRT > > > > static DEFINE_STATIC_KEY_TRUE(use_fast_ipi); > > +static DEFINE_STATIC_KEY_TRUE(has_uncore_regs); > > > > struct aic_info { > > int version; > > @@ -246,6 +247,7 @@ struct aic_info { > > > > /* Features */ > > bool fast_ipi; > > + bool uncore_regs; > > }; > > > > static const struct aic_info aic1_info = { > > @@ -253,6 +255,8 @@ static const struct aic_info aic1_info = { > > > > .event = AIC_EVENT, > > .target_cpu = AIC_TARGET_CPU, > > + > > + .uncore_regs = true, > > }; > > > > static const struct aic_info aic1_fipi_info = { > > @@ -264,6 +268,13 @@ static const struct aic_info aic1_fipi_info = { > > .fast_ipi = true, > > }; > > > > +static const struct aic_info aic1_legacy_info = { > > + .version = 1, > > + > > + .event = AIC_EVENT, > > + .target_cpu = AIC_TARGET_CPU, > > +}; > > + > > static const struct aic_info aic2_info = { > > .version = 2, > > > > @@ -273,6 +284,10 @@ static const struct aic_info aic2_info = { > > }; > > > > static const struct of_device_id aic_info_match[] = { > > + { > > + .compatible = "apple,s5l8960x-aic", > > + .data = &aic1_legacy_info, > > + }, > > Maybe I'm confused but shouldn't this be the apple,aic fallback and > uncore_regs should be enabled for e.g. t8103-aic then? In principle, no. Several Linux kernels shipped with a apple_aic irqchip driver that assumes the presence of the "uncore" registers for "apple,aic". And that means the "apple,aic" shouldn't be used to represent the interrupt controllers on SoCs with older cores that don't have those uncore registers. That said, given that there was no support for these older SoCs in those Linux kernels anyway, one could argue that the existing "apple,aic" support is unused and that we can redefine it to mean that the uncore registers are there. I agree that would be a bit more logical. > > { > > .compatible = "apple,t8103-aic", > > .data = &aic1_fipi_info, > > @@ -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 (static_branch_likely(&use_fast_ipi)) { > > + 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 doesn't make much sense: > > if (A) { > if (B) { > if (A) { // A is already guaranteed to be true here, why check it again? > // ... > } else { > // how can this ever be reached then? > } > } > } > > > } > > } > > > > @@ -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 (static_branch_likely(&has_uncore_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 +963,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(&use_fast_ipi)) > > + 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 +985,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_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 +1146,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_regs) > > + static_branch_enable(&has_uncore_regs); > > + else > > + static_branch_disable(&has_uncore_regs); > > + > > irqc->info.die_stride = off - start_off; > > > > irqc->hw_domain = irq_domain_create_tree(of_node_to_fwnode(node), > > -- > > 2.30.2 > > > Sven > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs @ 2022-09-29 14:47 Nick Chan 0 siblings, 0 replies; 6+ messages in thread From: Nick Chan @ 2022-09-29 14:47 UTC (permalink / raw) To: Konrad Dybcio Cc: Alyssa Rosenzweig, asahi, devicetree, krzysztof.kozlowski+dt, linux-arm-kernel, linux-kernel, Hector Martin, Marc Zyngier, robh+dt, Sven Peter, Thomas Gleixner, towinchenmi Tested-by: Nick Chan <towinchenmi@gmail.com> # iPad Pro 9.7-inch (Wi-Fi) ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-29 19:09 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-29 14:40 [PATCH v2 1/2] dt-bindings: apple,aic: Document A7-A11 compatibles Konrad Dybcio 2022-09-29 14:40 ` [PATCH v2 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs Konrad Dybcio 2022-09-29 14:50 ` Sven Peter 2022-09-29 17:10 ` Konrad Dybcio 2022-09-29 19:02 ` Mark Kettenis -- strict thread matches above, loose matches on Subject: below -- 2022-09-29 14:47 Nick Chan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).