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