* [PATCH v4 0/2] A7-A11 AIC support @ 2022-10-07 20:00 Konrad Dybcio 2022-10-07 20:00 ` Konrad Dybcio 2022-10-07 20:00 ` Konrad Dybcio 0 siblings, 2 replies; 21+ messages in thread From: Konrad Dybcio @ 2022-10-07 20:00 UTC (permalink / raw) To: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig Cc: martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Konrad Dybcio Limit some later-introduced features in the AIC driver to make it work for older Apple Silicon. Changes since v3: - Add cover letter :) - Bindings: A7-A10+M1 only get SoC compat and generic fallback, A11 gets M1 fallback and generic fallback (removed A11 compatible) - Drop Rob's A-b as this is essentially yet another rewrite.. - 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 Konrad Dybcio (2): dt-bindings: apple,aic: Document A7-A11 compatibles irqchip/apple-aic: Add support for A7-A11 SoCs .../interrupt-controller/apple,aic.yaml | 16 +++++-- drivers/irqchip/irq-apple-aic.c | 47 +++++++++++++------ 2 files changed, 45 insertions(+), 18 deletions(-) -- 2.37.3 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 1/2] dt-bindings: apple,aic: Document A7-A11 compatibles 2022-10-07 20:00 [PATCH v4 0/2] A7-A11 AIC support Konrad Dybcio @ 2022-10-07 20:00 ` Konrad Dybcio 2022-10-07 20:00 ` Konrad Dybcio 1 sibling, 0 replies; 21+ messages in thread From: Konrad Dybcio @ 2022-10-07 20:00 UTC (permalink / raw) To: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig Cc: martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Konrad Dybcio, Thomas Gleixner, Marc Zyngier, Rob Herring, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, devicetree Document the compatibles for Apple A7-A11 SoCs. Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> --- Changes since v3: - Bindings: A7-A10+M1 only get SoC compat and generic fallback, A11 gets M1 fallback and generic fallback (removed A11 compatible) - Drop Rob's A-b as this is essentially yet another rewrite.. .../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..1427ea9b04a2 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: + - items: + - enum: + - apple,s5l8960x-aic + - apple,s8000-aic + - apple,t7000-aic + - apple,t8010-aic + - apple,t8103-aic + - const: apple,aic + - items: + - const: apple,t8015-aic + - const: apple,t8103-aic + - const: apple,aic interrupt-controller: true -- 2.37.3 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 1/2] dt-bindings: apple,aic: Document A7-A11 compatibles @ 2022-10-07 20:00 ` Konrad Dybcio 0 siblings, 0 replies; 21+ messages in thread From: Konrad Dybcio @ 2022-10-07 20:00 UTC (permalink / raw) To: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig Cc: martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Konrad Dybcio, Thomas Gleixner, Marc Zyngier, Rob Herring, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, devicetree Document the compatibles for Apple A7-A11 SoCs. Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> --- Changes since v3: - Bindings: A7-A10+M1 only get SoC compat and generic fallback, A11 gets M1 fallback and generic fallback (removed A11 compatible) - Drop Rob's A-b as this is essentially yet another rewrite.. .../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..1427ea9b04a2 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: + - items: + - enum: + - apple,s5l8960x-aic + - apple,s8000-aic + - apple,t7000-aic + - apple,t8010-aic + - apple,t8103-aic + - const: apple,aic + - items: + - const: apple,t8015-aic + - const: apple,t8103-aic + - const: apple,aic interrupt-controller: true -- 2.37.3 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: apple,aic: Document A7-A11 compatibles 2022-10-07 20:00 ` Konrad Dybcio @ 2022-10-10 12:27 ` Rob Herring -1 siblings, 0 replies; 21+ messages in thread From: Rob Herring @ 2022-10-10 12:27 UTC (permalink / raw) To: Konrad Dybcio Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, Marc Zyngier, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, devicetree On Fri, Oct 07, 2022 at 10:00:21PM +0200, Konrad Dybcio wrote: > Document the compatibles for Apple A7-A11 SoCs. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > --- > Changes since v3: > - Bindings: A7-A10+M1 only get SoC compat and generic fallback, > A11 gets M1 fallback and generic fallback (removed A11 compatible) I have no idea if that makes sense or not. Do I have to go read the rationale in the prior version? The question to ask on fallbacks, is would s/w that only understands the fallback compatible work as-is? > - Drop Rob's A-b as this is essentially yet another rewrite.. > > .../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..1427ea9b04a2 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: > + - items: > + - enum: > + - apple,s5l8960x-aic > + - apple,s8000-aic > + - apple,t7000-aic > + - apple,t8010-aic > + - apple,t8103-aic > + - const: apple,aic > + - items: > + - const: apple,t8015-aic > + - const: apple,t8103-aic > + - const: apple,aic > > interrupt-controller: true > > -- > 2.37.3 > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: apple,aic: Document A7-A11 compatibles @ 2022-10-10 12:27 ` Rob Herring 0 siblings, 0 replies; 21+ messages in thread From: Rob Herring @ 2022-10-10 12:27 UTC (permalink / raw) To: Konrad Dybcio Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, Marc Zyngier, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, devicetree On Fri, Oct 07, 2022 at 10:00:21PM +0200, Konrad Dybcio wrote: > Document the compatibles for Apple A7-A11 SoCs. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > --- > Changes since v3: > - Bindings: A7-A10+M1 only get SoC compat and generic fallback, > A11 gets M1 fallback and generic fallback (removed A11 compatible) I have no idea if that makes sense or not. Do I have to go read the rationale in the prior version? The question to ask on fallbacks, is would s/w that only understands the fallback compatible work as-is? > - Drop Rob's A-b as this is essentially yet another rewrite.. > > .../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..1427ea9b04a2 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: > + - items: > + - enum: > + - apple,s5l8960x-aic > + - apple,s8000-aic > + - apple,t7000-aic > + - apple,t8010-aic > + - apple,t8103-aic > + - const: apple,aic > + - items: > + - const: apple,t8015-aic > + - const: apple,t8103-aic > + - const: apple,aic > > interrupt-controller: true > > -- > 2.37.3 > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: apple,aic: Document A7-A11 compatibles 2022-10-10 12:27 ` Rob Herring @ 2022-10-10 14:10 ` konrad.dybcio -1 siblings, 0 replies; 21+ messages in thread From: konrad.dybcio @ 2022-10-10 14:10 UTC (permalink / raw) To: Rob Herring Cc: Konrad Dybcio, ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, Marc Zyngier, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, devicetree On 2022-10-10 14:27, Rob Herring wrote: > On Fri, Oct 07, 2022 at 10:00:21PM +0200, Konrad Dybcio wrote: >> Document the compatibles for Apple A7-A11 SoCs. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> >> --- >> Changes since v3: >> - Bindings: A7-A10+M1 only get SoC compat and generic fallback, >> A11 gets M1 fallback and generic fallback (removed A11 compatible) > > I have no idea if that makes sense or not. Do I have to go read the > rationale in the prior version? > > The question to ask on fallbacks, is would s/w that only understands > the > fallback compatible work as-is? Yep, A7-A10 share the same feature level ("apple,aic"), but I think having a separate, per-SoC compatible in the DT from the get-go makes a lot of sense, since we're not sure what's gonna happen when we, for example, add SMP support for these older chipsets, maybe they'll need some special handling. Similar story with A11 and M1. They can both work with "apple,aic", but they also support things that are enabled with the "apple,t8103-aic" compat string. I do not plan on adding specific compatibles for A7-A11 SoCs in the driver, unless we uncover some bugs / deviations. Konrad > >> - Drop Rob's A-b as this is essentially yet another rewrite.. >> >> .../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..1427ea9b04a2 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: >> + - items: >> + - enum: >> + - apple,s5l8960x-aic >> + - apple,s8000-aic >> + - apple,t7000-aic >> + - apple,t8010-aic >> + - apple,t8103-aic >> + - const: apple,aic >> + - items: >> + - const: apple,t8015-aic >> + - const: apple,t8103-aic >> + - const: apple,aic >> >> interrupt-controller: true >> >> -- >> 2.37.3 >> >> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: apple,aic: Document A7-A11 compatibles @ 2022-10-10 14:10 ` konrad.dybcio 0 siblings, 0 replies; 21+ messages in thread From: konrad.dybcio @ 2022-10-10 14:10 UTC (permalink / raw) To: Rob Herring Cc: Konrad Dybcio, ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, Marc Zyngier, Krzysztof Kozlowski, linux-arm-kernel, linux-kernel, devicetree On 2022-10-10 14:27, Rob Herring wrote: > On Fri, Oct 07, 2022 at 10:00:21PM +0200, Konrad Dybcio wrote: >> Document the compatibles for Apple A7-A11 SoCs. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> >> --- >> Changes since v3: >> - Bindings: A7-A10+M1 only get SoC compat and generic fallback, >> A11 gets M1 fallback and generic fallback (removed A11 compatible) > > I have no idea if that makes sense or not. Do I have to go read the > rationale in the prior version? > > The question to ask on fallbacks, is would s/w that only understands > the > fallback compatible work as-is? Yep, A7-A10 share the same feature level ("apple,aic"), but I think having a separate, per-SoC compatible in the DT from the get-go makes a lot of sense, since we're not sure what's gonna happen when we, for example, add SMP support for these older chipsets, maybe they'll need some special handling. Similar story with A11 and M1. They can both work with "apple,aic", but they also support things that are enabled with the "apple,t8103-aic" compat string. I do not plan on adding specific compatibles for A7-A11 SoCs in the driver, unless we uncover some bugs / deviations. Konrad > >> - Drop Rob's A-b as this is essentially yet another rewrite.. >> >> .../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..1427ea9b04a2 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: >> + - items: >> + - enum: >> + - apple,s5l8960x-aic >> + - apple,s8000-aic >> + - apple,t7000-aic >> + - apple,t8010-aic >> + - apple,t8103-aic >> + - const: apple,aic >> + - items: >> + - const: apple,t8015-aic >> + - const: apple,t8103-aic >> + - const: apple,aic >> >> interrupt-controller: true >> >> -- >> 2.37.3 >> >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs 2022-10-07 20:00 [PATCH v4 0/2] A7-A11 AIC support Konrad Dybcio @ 2022-10-07 20:00 ` Konrad Dybcio 2022-10-07 20:00 ` Konrad Dybcio 1 sibling, 0 replies; 21+ messages in thread From: Konrad Dybcio @ 2022-10-07 20:00 UTC (permalink / raw) To: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig Cc: martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Konrad Dybcio, Thomas Gleixner, Marc Zyngier, linux-arm-kernel, linux-kernel 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. 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. Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> --- 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; }; 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); + } } } @@ -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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs @ 2022-10-07 20:00 ` Konrad Dybcio 0 siblings, 0 replies; 21+ messages in thread From: Konrad Dybcio @ 2022-10-07 20:00 UTC (permalink / raw) To: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig Cc: martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Konrad Dybcio, Thomas Gleixner, Marc Zyngier, linux-arm-kernel, linux-kernel 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. 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. Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> --- 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; }; 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); + } } } @@ -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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs 2022-10-07 20:00 ` Konrad Dybcio @ 2022-10-07 23:17 ` Marc Zyngier -1 siblings, 0 replies; 21+ messages in thread From: Marc Zyngier @ 2022-10-07 23:17 UTC (permalink / raw) To: Konrad Dybcio Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, linux-arm-kernel, linux-kernel On 2022-10-07 21:00, 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). > > 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. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > --- > 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(-) Since you cannot be bothered to read the review comments on the previous versions of this series, I'll do the same with these patches. Feel free to stop Cc-ing me. M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs @ 2022-10-07 23:17 ` Marc Zyngier 0 siblings, 0 replies; 21+ messages in thread From: Marc Zyngier @ 2022-10-07 23:17 UTC (permalink / raw) To: Konrad Dybcio Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, linux-arm-kernel, linux-kernel On 2022-10-07 21:00, 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). > > 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. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > --- > 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(-) Since you cannot be bothered to read the review comments on the previous versions of this series, I'll do the same with these patches. Feel free to stop Cc-ing me. M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs 2022-10-07 23:17 ` Marc Zyngier @ 2022-10-08 7:03 ` Konrad Dybcio -1 siblings, 0 replies; 21+ messages in thread From: Konrad Dybcio @ 2022-10-08 7:03 UTC (permalink / raw) To: Marc Zyngier Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, linux-arm-kernel, linux-kernel On 8.10.2022 01:17, Marc Zyngier wrote: > On 2022-10-07 21:00, 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). >> >> 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. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> >> --- >> 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(-) > > Since you cannot be bothered to read the review comments on > the previous versions of this series, I'll do the same with > these patches. Feel free to stop Cc-ing me. > > M. I'm sorry, I'm working on a lot of stuff right now and I was almost sure your email only concerned adding a cover letter and affirming the discussion on the fallback compatible, so I did not go through it again - but that was obviously incorrect. If you don't mind, I can respin another version of this patchset, addressing your comments. Konrad ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs @ 2022-10-08 7:03 ` Konrad Dybcio 0 siblings, 0 replies; 21+ messages in thread From: Konrad Dybcio @ 2022-10-08 7:03 UTC (permalink / raw) To: Marc Zyngier Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, linux-arm-kernel, linux-kernel On 8.10.2022 01:17, Marc Zyngier wrote: > On 2022-10-07 21:00, 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). >> >> 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. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> >> --- >> 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(-) > > Since you cannot be bothered to read the review comments on > the previous versions of this series, I'll do the same with > these patches. Feel free to stop Cc-ing me. > > M. I'm sorry, I'm working on a lot of stuff right now and I was almost sure your email only concerned adding a cover letter and affirming the discussion on the fallback compatible, so I did not go through it again - but that was obviously incorrect. If you don't mind, I can respin another version of this patchset, addressing your comments. Konrad _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs 2022-10-08 7:03 ` Konrad Dybcio @ 2022-10-09 0:56 ` Marc Zyngier -1 siblings, 0 replies; 21+ messages in thread From: Marc Zyngier @ 2022-10-09 0:56 UTC (permalink / raw) To: Konrad Dybcio Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, linux-arm-kernel, linux-kernel On Sat, 08 Oct 2022 08:03:23 +0100, Konrad Dybcio <konrad.dybcio@somainline.org> wrote: > > > > On 8.10.2022 01:17, Marc Zyngier wrote: > > On 2022-10-07 21:00, 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). > >> > >> 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. > >> > >> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > >> --- > >> 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(-) > > > > Since you cannot be bothered to read the review comments on > > the previous versions of this series, I'll do the same with > > these patches. Feel free to stop Cc-ing me. > > > > M. > I'm sorry, I'm working on a lot of stuff right now and I was almost sure > your email only concerned adding a cover letter And yet you ignored it again. > and affirming the discussion on the fallback compatible, I couldn't care less about that, but I admire your ability to guess what an email may or may not contain! > so I did not go through it again - but that was > obviously incorrect. If you don't mind, I can respin another version of this > patchset, addressing your comments. That's pretty pointless at the moment, as I'm not queuing anything other than fixes (and one week between versions is a sensible pace). Come back after -rc1 with a cover letter and the various comments addressed. M. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs @ 2022-10-09 0:56 ` Marc Zyngier 0 siblings, 0 replies; 21+ messages in thread From: Marc Zyngier @ 2022-10-09 0:56 UTC (permalink / raw) To: Konrad Dybcio Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Sven Peter, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, linux-arm-kernel, linux-kernel On Sat, 08 Oct 2022 08:03:23 +0100, Konrad Dybcio <konrad.dybcio@somainline.org> wrote: > > > > On 8.10.2022 01:17, Marc Zyngier wrote: > > On 2022-10-07 21:00, 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). > >> > >> 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. > >> > >> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org> > >> --- > >> 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(-) > > > > Since you cannot be bothered to read the review comments on > > the previous versions of this series, I'll do the same with > > these patches. Feel free to stop Cc-ing me. > > > > M. > I'm sorry, I'm working on a lot of stuff right now and I was almost sure > your email only concerned adding a cover letter And yet you ignored it again. > and affirming the discussion on the fallback compatible, I couldn't care less about that, but I admire your ability to guess what an email may or may not contain! > so I did not go through it again - but that was > obviously incorrect. If you don't mind, I can respin another version of this > patchset, addressing your comments. That's pretty pointless at the moment, as I'm not queuing anything other than fixes (and one week between versions is a sensible pace). Come back after -rc1 with a cover letter and the various comments addressed. M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs 2022-10-07 20:00 ` Konrad Dybcio @ 2022-10-08 9:33 ` Sven Peter -1 siblings, 0 replies; 21+ messages in thread From: Sven Peter @ 2022-10-08 9:33 UTC (permalink / raw) To: Konrad Dybcio Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, Marc Zyngier, linux-arm-kernel, linux-kernel Hi, > On 7. Oct 2022, at 22:00, Konrad Dybcio <konrad.dybcio@somainline.org> 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@somainline.org> > --- > 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? > + } > } > } > > @@ -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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs @ 2022-10-08 9:33 ` Sven Peter 0 siblings, 0 replies; 21+ messages in thread From: Sven Peter @ 2022-10-08 9:33 UTC (permalink / raw) To: Konrad Dybcio Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, Marc Zyngier, linux-arm-kernel, linux-kernel Hi, > On 7. Oct 2022, at 22:00, Konrad Dybcio <konrad.dybcio@somainline.org> 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@somainline.org> > --- > 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? > + } > } > } > > @@ -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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs 2022-10-08 9:33 ` Sven Peter @ 2022-10-08 14:06 ` Konrad Dybcio -1 siblings, 0 replies; 21+ messages in thread From: Konrad Dybcio @ 2022-10-08 14:06 UTC (permalink / raw) To: Sven Peter Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, Marc Zyngier, linux-arm-kernel, linux-kernel On 8.10.2022 11:33, Sven Peter wrote: > > Hi, > >> On 7. Oct 2022, at 22:00, Konrad Dybcio <konrad.dybcio@somainline.org> 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@somainline.org> >> --- >> 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 > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs @ 2022-10-08 14:06 ` Konrad Dybcio 0 siblings, 0 replies; 21+ messages in thread From: Konrad Dybcio @ 2022-10-08 14:06 UTC (permalink / raw) To: Sven Peter Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, Marc Zyngier, linux-arm-kernel, linux-kernel On 8.10.2022 11:33, Sven Peter wrote: > > Hi, > >> On 7. Oct 2022, at 22:00, Konrad Dybcio <konrad.dybcio@somainline.org> 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@somainline.org> >> --- >> 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs 2022-10-08 14:06 ` Konrad Dybcio @ 2022-10-09 9:57 ` Sven Peter -1 siblings, 0 replies; 21+ messages in thread From: Sven Peter @ 2022-10-09 9:57 UTC (permalink / raw) To: Konrad Dybcio Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, Marc Zyngier, linux-arm-kernel, linux-kernel Hi, > On 8. Oct 2022, at 16:06, Konrad Dybcio <konrad.dybcio@somainline.org> wrote: > > > >> On 8.10.2022 11:33, Sven Peter wrote: >> >> Hi, >> >>>> On 7. Oct 2022, at 22:00, Konrad Dybcio <konrad.dybcio@somainline.org> 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@somainline.org> >>> --- >>> 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). What makes you think that? > > 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? > That combination just doesn’t make any sense. It translates to “we know the chip supports fastipi but we’re not gonna use it because we want everything to be slower!”. As I said before, I’m pretty sure that else block was just a leftover from when this driver didn’t support fastipi at all but we already knew that M1 supported it. Sven ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs @ 2022-10-09 9:57 ` Sven Peter 0 siblings, 0 replies; 21+ messages in thread From: Sven Peter @ 2022-10-09 9:57 UTC (permalink / raw) To: Konrad Dybcio Cc: ~postmarketos/upstreaming, asahi, Hector Martin, Alyssa Rosenzweig, martin.botka, angelogioacchino.delregno, marijn.suijten, jamipkettunen, Thomas Gleixner, Marc Zyngier, linux-arm-kernel, linux-kernel Hi, > On 8. Oct 2022, at 16:06, Konrad Dybcio <konrad.dybcio@somainline.org> wrote: > > > >> On 8.10.2022 11:33, Sven Peter wrote: >> >> Hi, >> >>>> On 7. Oct 2022, at 22:00, Konrad Dybcio <konrad.dybcio@somainline.org> 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@somainline.org> >>> --- >>> 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). What makes you think that? > > 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? > That combination just doesn’t make any sense. It translates to “we know the chip supports fastipi but we’re not gonna use it because we want everything to be slower!”. As I said before, I’m pretty sure that else block was just a leftover from when this driver didn’t support fastipi at all but we already knew that M1 supported it. Sven _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2022-10-10 14:34 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-10-07 20:00 [PATCH v4 0/2] A7-A11 AIC support Konrad Dybcio 2022-10-07 20:00 ` [PATCH v4 1/2] dt-bindings: apple,aic: Document A7-A11 compatibles Konrad Dybcio 2022-10-07 20:00 ` Konrad Dybcio 2022-10-10 12:27 ` Rob Herring 2022-10-10 12:27 ` Rob Herring 2022-10-10 14:10 ` konrad.dybcio 2022-10-10 14:10 ` konrad.dybcio 2022-10-07 20:00 ` [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs Konrad Dybcio 2022-10-07 20:00 ` Konrad Dybcio 2022-10-07 23:17 ` Marc Zyngier 2022-10-07 23:17 ` Marc Zyngier 2022-10-08 7:03 ` Konrad Dybcio 2022-10-08 7:03 ` Konrad Dybcio 2022-10-09 0:56 ` Marc Zyngier 2022-10-09 0:56 ` Marc Zyngier 2022-10-08 9:33 ` Sven Peter 2022-10-08 9:33 ` Sven Peter 2022-10-08 14:06 ` Konrad Dybcio 2022-10-08 14:06 ` Konrad Dybcio 2022-10-09 9:57 ` Sven Peter 2022-10-09 9:57 ` Sven Peter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.