core: interrupt: factorization & renaming by etienne-lms · Pull Request #6030 · OP-TEE/optee_os

etienne-lms · 2023-05-17T07:05:01Z

This change prepares changes to move to the interrupt management API function proposed in OP-TEE/optee_docs#192 and to the generic interrupt chip framework proposed by #5954.

Commit "core: define main interrupt controller data from its driver" factorizes CPU ~~core~~ main interrupt controller initialization and ~~root~~ main native interrupt handling.

Commit "drivers: gic: factorize call to gic_init() or gic_init_base_addr()" makes gic_init() a unique entry point to initialize GIC driver.

Commit "core: interrupt: rename itr_core_handler()" renames itr_core_handler() to ~~interrupt_core_handler()~~ interrupt_main_handler().

Commit "core: interrupt: rename itr_init()" renames itr_init() to ~~interrupt_core_init()~~ interrupt_main_init().

(edited) updated from review comments

jenswikl

Comment for "core: define main interrupt controller data from its driver"

jenswikl

Comment for "core: define main interrupt controller data from its driver"

jenswikl

Comment for "core: define main interrupt controller data from its driver"

etienne-lms

@jforissier, do these changes make sense to you?
I'll wait for some status in this P-R before rebasing #5954.

jforissier · 2023-05-23T06:26:54Z

@jforissier, do these changes make sense to you? I'll wait for some status in this P-R before rebasing #5954.

They do seem to simplify things quite a bit and the diffstat is nice (+193 -339), so I'd say: yes, I'm fine with that.

jenswikl · 2023-05-23T12:51:32Z

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

All but one platforms define CPU core interrupt controller from their platform main.c source file next to its main interrupt handler. This change factorize these implementation by moving the definition of the controller data instance straight in the controller driver source file. This change makes each controller driver to implement straight itr_core_handler() function, preventing a extra branch on interrupt execution. Interrupt controller driver initialization function now straight calls itr_core_init(). This changes treats case when CFG_CORE_WORKAROUND_ARM_NMFI is enable to not conflict with core/arch/arm/kernel/thread.c that already overrides itr_core_handler() weak implementation. With this change, the main controller initialization function (gic_init(), gic_init_base_addr(), gic_cpu_init() and hfic_init()) no more gets the controller data as input argument. As a consequence, definition of struct hfic_data and struct gic_data moves from their respective driver header file to the respective driver source file. As a consequence, gic_dump() no more requires an argument. Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>

Platforms call either gic_init() or gic_init_base_addr() depending on whether CFG_WITH_ARM_TRUSTED_FW is defined or not. This change factorize this logic from gic_init() implementation and makes gic_init_base_addr() local to gic.c. For that purpose functions gic_init_base_address() and gic_dt_get_irq() are moved inside gic.c source file. source file. Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>

Renames itr_core_handler() to interrupt_main_handler() as a later change will modify interrupt chip API functions using interrupt_ as prefix. Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>

Renames itr_init() to interrupt_main_init() as a later change will modify interrupt chip API functions using interrupt_ as prefix. Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>

etienne-lms · 2023-05-23T15:33:27Z

fixup commit squashed and review tag applied.

jforissier · 2025-06-30T10:21:32Z

It looks like RockPi4 was broken by this PR. When testing 4.7.0-rc1, I get a panic "Secure interrupt handler not defined". Anyone has an idea what's missing? Thanks.

etienne-lms · 2025-06-30T12:11:47Z

You get this panic trace message when no interrupt_main_handler() defines the CPU main interrupt controller.
IIUC, RockPi4 is PLATFORM=rockchip-rk3399 and embeds GIC driver core/drivers/gic.c without enabling CFG_CORE_WORKAROUND_ARM_NMFI=y hence interrupt_main_handler() should come from gic.c driver.

jforissier · 2025-06-30T13:14:22Z

You get this panic trace message when no interrupt_main_handler() defines the CPU main interrupt controller. IIUC, RockPi4 is PLATFORM=rockchip-rk3399 and embeds GIC driver core/drivers/gic.c without enabling CFG_CORE_WORKAROUND_ARM_NMFI=y hence interrupt_main_handler() should come from gic.c driver.

gic.c is not compiled because CFG_GIC is not enabled due to https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-rockchip/conf.mk#L4 which was introduced by commit 447c5f6. If I add this file to the build anyways, xtest hangs in regression_1003 Core internal read/write mutex.

etienne-lms · 2025-06-30T19:42:58Z

Right! indeed. I was fooled by https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-rockchip/conf.mk#L33 enforcing CFG_ARM_GICV3=y.
interrupt_main_handler() is called when the platform receives a secure (native) interrupt. What does the platform expect to do if there is no interrupt driver?

jenswikl reviewed May 17, 2023

View reviewed changes

Comment thread core/drivers/gic.c

jenswikl reviewed May 17, 2023

View reviewed changes

Comment thread core/include/kernel/interrupt.h

etienne-lms commented May 17, 2023

View reviewed changes

Comment thread core/drivers/gic.c

Comment thread core/drivers/gic.c

Comment thread core/include/kernel/interrupt.h

etienne-lms commented May 22, 2023

View reviewed changes

etienne-lms added 4 commits May 23, 2023 17:30

core: interrupt: rename itr_init()

913ad05

Renames itr_init() to interrupt_main_init() as a later change will modify interrupt chip API functions using interrupt_ as prefix. Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>

etienne-lms force-pushed the itr-1 branch from 5005a72 to 913ad05 Compare May 23, 2023 15:33

jforissier merged commit 01980f3 into OP-TEE:master May 23, 2023

etienne-lms mentioned this pull request May 24, 2023

core: riscv: fix interrupt_main_handler() reference #6044

Merged

etienne-lms deleted the itr-1 branch May 24, 2023 14:04

etienne-lms mentioned this pull request May 30, 2023

drivers: implement SEC driver #5894

Closed

Uh oh!

Conversation

etienne-lms commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jenswikl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jenswikl left a comment

Choose a reason for hiding this comment

Uh oh!

jenswikl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

jforissier commented May 23, 2023

Uh oh!

jenswikl commented May 23, 2023

Uh oh!

etienne-lms commented May 23, 2023

Uh oh!

jforissier commented Jun 30, 2025

Uh oh!

etienne-lms commented Jun 30, 2025

Uh oh!

jforissier commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etienne-lms commented Jun 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

etienne-lms commented May 17, 2023 •

edited

Loading

jforissier commented Jun 30, 2025 •

edited

Loading