Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor ARM interrupt system (Cortex-A & Cortex-R) #22718

Conversation

stephanosio
Copy link
Member

interrupt_controller: gic: Refactor GIC driver interface 

The current Generic Interrupt Controller (GIC) driver makes use of the
multi-level interrupt mechanism and `irq_nextlevel` public interface.

This is a less-than-ideal implementation for the following reasons:

1. The GIC is often used as the main interrupt controller for the
  Cortex-A and Cortex-R family SoCs and, in this case, it is not a 2nd
  level interrupt controller; in fact, it is the root interrupt
  controller and therefore should be treated as such.

2. The only reason for using `irq_nextlevel` here is to interface the
  architecture implementation to the interrupt controller functions.
  Since there is no nesting or multiple instances of an interrupt
  controller involved, there is really no point in adding such an
  abstraction.

3. 2nd level topology adds many unnecessary abstractions and results
  in strange coding artefacts as well as performance penalty due to
  additional branching.

This commit refactors the GIC driver interface as follows:

1. Remove the current GIC driver interface based on the multi-level
  interrupt mechanism and the `irq_nextlevel` public interface.

2. Define the GIC driver interface in
  `include/drivers/interrupt_controller/gic.h` and allow the arch
  implementation to directly invoke this interface.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
arch: arm: aarch32: Refactor interrupt interface 

The current AArch32 (Cortex-R and to-be-added Cortex-A) interrupt
system relies on the multi-level interrupt mechanism and the
`irq_nextlevel` public interface to invoke the Generic Interrupt
Controller (GIC) driver functions.

Since the GIC driver has been refactored to provide a direct interface,
in order to resolve various implementation issues described in the GIC
driver refactoring commit, the architecture interrupt control functions
are updated to directly invoke the GIC driver functions.

This commit also adds support for the Cortex-A and Cortex-R cores
(Cortex-A5, Cortex-R4/5, ...) that allow interfacing to a custom
external interrupt controller (i.e. non-GIC) by introducing the
`ARM_CUSTOM_INTERRUPT_CONTROLLER` configuration that maps the
architecture interrupt control functions to the SoC layer interrupt
control functions.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
arch: arm: aarch64: Refactor interrupt interface

The current AArch64 interrupt system relies on the multi-level
interrupt mechanism and the `irq_nextlevel` public interface to invoke
the Generic Interrupt Controller (GIC) driver functions.

Since the GIC driver has been refactored to provide a direct interface,
in order to resolve various implementation issues described in the GIC
driver refactoring commit, the architecture interrupt control functions
are updated to directly invoke the GIC driver functions.

This commit also adds support for the ARMv8 cores (e.g. Cortex-A53)
that allow interfacing to a custom external interrupt controller
(i.e. non-GIC) by mapping the architecture interrupt control functions
to the SoC layer interrupt control functions when
`ARM_CUSTOM_INTERRUPT_CONTROLLER` configuration is enabled.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>

@stephanosio stephanosio added the area: ARM ARM (32-bit) Architecture label Feb 11, 2020
@zephyrbot zephyrbot added the area: API Changes to public APIs label Feb 11, 2020
@carlescufi carlescufi added this to the v2.3.0 milestone Feb 11, 2020
Copy link
Contributor

@andrewboie andrewboie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just a minor nit

sys_write32((1 << int_off), (GICD_ICENABLERn + int_grp * 4));
}

int arm_gic_irq_is_enabled(unsigned int irq)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add here something like:

+       /* SGIs are never available */
+       if (irq < 16)
+               return 0;
+

The problem comes from the tests/kernel/interrupt/arch.interrupt test that is failing the assert at https://github.com/zephyrproject-rtos/zephyr/blob/master/arch/common/sw_isr_common.c#L19 when irq == 0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least, for PL-390 and GIC-400, the SGIs (0-15) are always enabled and this is an expected behaviour (arm_gic_irq_is_enabled should report the actual enable status of the IRQ, which is true).

The real problem is that the dynamic interrupt test scans the SW ISR table for the first spurious ISR entry and this happens to be the IRQ 0, which is an SGI that cannot be disabled on the GIC-400.

IMO, this problem should be addressed separately in the issue #22670. After all, without proper GIC awareness in the interrupt test, removing the interrupt test ignore tag would only provide a false sense of security.

@carlocaione
Copy link
Collaborator

With this in place we can:

--- a/boards/arm/qemu_cortex_a53/qemu_cortex_a53.yaml
+++ b/boards/arm/qemu_cortex_a53/qemu_cortex_a53.yaml
@@ -9,4 +9,4 @@ toolchain:
 ram: 128
 testing:
   ignore_tags:
-    - interrupt
+    - nmi

@zephyrbot
Copy link
Collaborator

zephyrbot commented Feb 12, 2020

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@stephanosio stephanosio force-pushed the no_more_multilevel_interrupt_insanity branch from 731a695 to bf8170a Compare February 12, 2020 02:25
@stephanosio stephanosio mentioned this pull request Feb 12, 2020
25 tasks
@wentongwu wentongwu self-requested a review March 4, 2020 02:34
@stephanosio stephanosio force-pushed the no_more_multilevel_interrupt_insanity branch from bf8170a to b61f9b5 Compare March 10, 2020 13:39
@stephanosio
Copy link
Member Author

Rebased

Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with the refactoring, with the exception of mixing Kconfig-based #ifdef's

  • CONFIG_ARMV...
  • CONFIG_CPU_CORTEX_...

I know it is not perfect in the code, but we should rather fix the problems that continue mixing the define usage, IMHO

include/arch/arm/aarch32/irq.h Outdated Show resolved Hide resolved
include/arch/arm/aarch32/error.h Outdated Show resolved Hide resolved
include/arch/arm/aarch32/asm_inline_gcc.h Outdated Show resolved Hide resolved
arch/arm/core/aarch32/isr_wrapper.S Outdated Show resolved Hide resolved
arch/arm/core/aarch32/isr_wrapper.S Outdated Show resolved Hide resolved
Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephanosio I did a pass on this PR - please take a look

@stephanosio stephanosio force-pushed the no_more_multilevel_interrupt_insanity branch from 0dc1171 to 2eab0bd Compare March 12, 2020 15:06
@ioannisg
Copy link
Member

Needs rebase now @stephanosio :)

The current Generic Interrupt Controller (GIC) driver makes use of the
multi-level interrupt mechanism and `irq_nextlevel` public interface.

This is a less-than-ideal implementation for the following reasons:

1. The GIC is often used as the main interrupt controller for the
  Cortex-A and Cortex-R family SoCs and, in this case, it is not a 2nd
  level interrupt controller; in fact, it is the root interrupt
  controller and therefore should be treated as such.

2. The only reason for using `irq_nextlevel` here is to interface the
  architecture implementation to the interrupt controller functions.
  Since there is no nesting or multiple instances of an interrupt
  controller involved, there is really no point in adding such an
  abstraction.

3. 2nd level topology adds many unnecessary abstractions and results
  in strange coding artefacts as well as performance penalty due to
  additional branching.

This commit refactors the GIC driver interface as follows:

1. Remove the current GIC driver interface based on the multi-level
  interrupt mechanism and the `irq_nextlevel` public interface.

2. Define the GIC driver interface in
  `include/drivers/interrupt_controller/gic.h` and allow the arch
  implementation to directly invoke this interface.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
This commit fixes incorrect header descriptions for the ARM AArch32
public headers.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
The current AArch32 (Cortex-R and to-be-added Cortex-A) interrupt
system relies on the multi-level interrupt mechanism and the
`irq_nextlevel` public interface to invoke the Generic Interrupt
Controller (GIC) driver functions.

Since the GIC driver has been refactored to provide a direct interface,
in order to resolve various implementation issues described in the GIC
driver refactoring commit, the architecture interrupt control functions
are updated to directly invoke the GIC driver functions.

This commit also adds support for the Cortex-R cores (Cortex-R4 and R5)
that allow interfacing to a custom external interrupt controller
(i.e. non-GIC) by introducing the `ARM_CUSTOM_INTERRUPT_CONTROLLER`
configuration that maps the architecture interrupt control functions to
the SoC layer interrupt control functions.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
This commit updates the `xilinx_zynqmp` platform to use the refactored
AArch32 interrupt system.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
The current AArch64 interrupt system relies on the multi-level
interrupt mechanism and the `irq_nextlevel` public interface to invoke
the Generic Interrupt Controller (GIC) driver functions.

Since the GIC driver has been refactored to provide a direct interface,
in order to resolve various implementation issues described in the GIC
driver refactoring commit, the architecture interrupt control functions
are updated to directly invoke the GIC driver functions.

This commit also adds support for the ARMv8 cores (e.g. Cortex-A53)
that allow interfacing to a custom external interrupt controller
(i.e. non-GIC) by mapping the architecture interrupt control functions
to the SoC layer interrupt control functions when
`ARM_CUSTOM_INTERRUPT_CONTROLLER` configuration is enabled.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
This commit updates the `qemu_cortex_a53` platform to use the
refactored AArch64 interrupt system.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
@stephanosio stephanosio force-pushed the no_more_multilevel_interrupt_insanity branch from 2eab0bd to 4d1f4d1 Compare March 12, 2020 23:48
@stephanosio
Copy link
Member Author

Rebased

@ioannisg ioannisg added the Enhancement Changes/Updates/Additions to existing features label Mar 13, 2020
@ioannisg ioannisg merged commit 685bf54 into zephyrproject-rtos:master Mar 13, 2020
@stephanosio stephanosio deleted the no_more_multilevel_interrupt_insanity branch April 23, 2020 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: ARM ARM (32-bit) Architecture area: Drivers Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants