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.

Sign up for GitHub

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

Jump to bottom

Add architectural support for shared interrupts (take 2) #61422

Merged
merged 7 commits into from Sep 14, 2023
Merged

Add architectural support for shared interrupts (take 2) #61422

merged 7 commits into from Sep 14, 2023

Conversation

LaurentiuM1234
Copy link

@LaurentiuM1234 LaurentiuM1234 commented Aug 11, 2023 •
edited

This is an enhancement/alternative to #61331 which is meant to allow making use of shared interrupts with IRQ_CONNECT() (statically) and irq_connect_dynamic (dynamically).

A few notes:

  • the shared interrupt support is transparent to the user and there's no API changes.
  • I've decided to keep shared_irq.c in arch/common/ for consistency reasons (it's somewhat the same thing as sw_isr_common.c which is placed there)
  • my knowledge of linker scripts is somewhat limited but the main reason for all the linker script changes is because _shared_irq_table should be placed in the same memory as _sw_isr_table (RAM or ROM).
  • this PR is inspired from Zephyr Shared ISRs (alternative) #57146

@LaurentiuM1234 LaurentiuM1234 force-pushed the shared_irq_my_alt branch 2 times, most recently from 01f40b9 to fedd16d Compare August 11, 2023 14:35
@LaurentiuM1234 LaurentiuM1234 mentioned this pull request Aug 11, 2023
Closed
@carlocaione carlocaione self-requested a review August 13, 2023 11:55
@LaurentiuM1234 LaurentiuM1234 force-pushed the shared_irq_my_alt branch 2 times, most recently from 315465e to 0561ac1 Compare August 14, 2023 12:11
@LaurentiuM1234 LaurentiuM1234 marked this pull request as ready for review August 14, 2023 12:57
nordicjm
nordicjm previously requested changes Aug 14, 2023
View reviewed changes
arch/Kconfig Show resolved Hide resolved
arch/common/shared_irq.c Outdated Show resolved Hide resolved
arch/common/shared_irq.c Outdated Show resolved Hide resolved
carlocaione
carlocaione requested changes Aug 15, 2023
View reviewed changes
arch/Kconfig Outdated Show resolved Hide resolved
arch/common/isr_tables.c Outdated Show resolved Hide resolved
include/zephyr/sw_isr_table.h Outdated Show resolved Hide resolved
arch/common/shared_irq.c Outdated Show resolved Hide resolved
arch/Kconfig Show resolved Hide resolved
tests/kernel/interrupt/src/shared_irq.c Outdated Show resolved Hide resolved
tests/kernel/interrupt/src/shared_irq.c Outdated Show resolved Hide resolved
include/zephyr/sw_isr_table.h Outdated Show resolved Hide resolved
tests/kernel/interrupt/testcase.yaml Outdated Show resolved Hide resolved
@lowlander lowlander mentioned this pull request Aug 25, 2023
Closed
ycsin added a commit to ycsin/zephyr that referenced this pull request Sep 13, 2023
@ycsin
arch: common: sw_isr_common: Move table index computing logic to func…
34d1c36 
…tion

Borrow from zephyrproject-rtos#61422.
Will rebased after it is merged.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
ycsin added a commit to ycsin/zephyr that referenced this pull request Sep 13, 2023
@ycsin
arch: common: sw_isr_common: Move table index computing logic to func…
5a87729 
…tion

Borrow from zephyrproject-rtos#61422.
Will rebased after it is merged.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
ycsin added a commit to ycsin/zephyr that referenced this pull request Sep 13, 2023
@ycsin
arch: common: sw_isr_common: Move table index computing logic to func…
d05b01b 
…tion

Borrow from zephyrproject-rtos#61422.
Will rebased after it is merged.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
carlocaione
carlocaione previously approved these changes Sep 13, 2023
View reviewed changes
Laurentiu Mihalcea added 7 commits September 13, 2023 12:44
arch: Add support for static shared interrupts
21b5a96 
This commit introduces all the necessary changes for
enabling the usage of shared interrupts.

This works by using a second interrupt table: _shared_sw_isr_table
which keeps track of all of the ISR/arg pairs sharing the same
interrupt line. Whenever a second ISR/arg pair is registered
on the same interrupt line using IRQ_CONNECT(), the entry in
_sw_isr_table will be overwriten by a
(shared_isr, _shared_sw_isr_table[irq]) pair. In turn, shared_isr()
will invoke all of the ISR/arg pairs registered on the same
interrupt line.

This feature only works statically, meaning you can only make use
of shared interrupts using IRQ_CONNECT(). Attempting to dynamically
register a ISR/arg pair will overwrite the hijacked _sw_isr_table
entry.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
arch: common: sw_isr_common: Move table index computing logic to func…
26a5a33 
…tion

Since the shared IRQ code will also use the same logic to compute
the _sw_isr_table index, move the computing logic to a separate
function: z_get_sw_isr_table_idx(), which can be used by other
modules.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
arch: Add support for dynamic shared interrupts
2bef522 
This works by overwriting z_isr_install()'s definition
(possible since the symbol is now weak) with our own definiton.

Whenever trying to register a new ISR/arg pair, z_isr_install()
will check to see if the interrupt line is already in use. If it's
not then it will not share the interrupt and will work exactly
as the old z_isr_install(), meaning it will just write the new
ISR/arg pair to _sw_isr_table.

If the interrupt line is already being used by an ISR/arg pair
then that line will become shared, meaning we'll overwrite
_sw_isr_table with our own (z_shared_isr, z_shared_sw_isr_table[irq])
pair.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
arch: Add support for dynamically disconnecting shared interrupts
85de999 
This commit provides the users a way to disconnect dynamic
interrupts. This is needed because we don't want to keep
piling up ISR/arg pairs until the number of registrable
clients is reached.

This feature is only relevant for shared and dynamic interrupts.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
tests: kernel: interrupt: Add testcase for shared interrupts
a6eaddd 
This commit introduces a new testcase for shared interrupts.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
doc: interrupts: Document support for shared interrupts
5eae6c4 
This commit adds the documentation for shared interrupts.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
doc: release: 3.5: Add note about shared interrupt support
931d81a 
Add a note that shared interrupts are now supported.

Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>
andyross
andyross approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Still good here

ycsin added a commit to ycsin/zephyr that referenced this pull request Sep 14, 2023
@ycsin
arch: common: sw_isr_common: Move table index computing logic to func…
1391f98 
…tion

Borrow from zephyrproject-rtos#61422.
Will rebased after it is merged.

Signed-off-by: Yong Cong Sin <ycsin@meta.com>
@carlescufi carlescufi merged commit ff1bb65 into zephyrproject-rtos:main Sep 14, 2023
28 checks passed
@dbaluta
Copy link
Collaborator

dbaluta commented Sep 14, 2023

Great work @LaurentiuM1234 !

@cfriedt
Copy link
Member

cfriedt commented Sep 19, 2023 •
edited

  • the static shared interrupt test is now skipped for all platforms except qemu_cortex_a53 and friends via #ifdef logic.

So does this mean shared interrupts are only compatible with Aarch64?

I didn't get a chance to review this change before it went in, but it looks like tests were tailored to arm64.

It's hard for me to tell from the tests that were added, but was any attention given to multi-level interrupts?

What are the implications in terms of size for multi-level interrupts? At a glance, it looks to be O(k^N) - so exponential.

@LaurentiuM1234
Copy link
Author

So does this mean shared interrupts are only compatible with Aarch64?

I didn't get a chance to review this change before it went in, but it looks like tests were tailored to arm64.

Shared interrupts work on any architecture. The restriction to qemu_cortex_a53 is only applicable to the static test because finding 2 interrupt lines for which _sw_isr_table entry is NULL/z_irq_spurious is very hard or impossible for all testing platforms. Also, removing the restriction would make the test more prone to errors.

It's hard for me to tell from the tests that were added, but was any attention given to multi-level interrupts?

The shared interrupt code is multi-level interrupt-aware. By this I mean it uses z_get_sw_isr_table_idx() to compute the table index as done by the default z_isr_install() (dynamic shared interrupts). For the static case, the shared interrupt table uses the same table_idx computed for _sw_isr_table.

What are the implications in terms of size for multi-level interrupts? At a glance, it looks to be O(k^N) - so exponential.

I'd say the formula is something like: O(N * (sizeof(size_t) + k * sizeof(struct z_shared_isr_client))), where N is the number of interrupts and k is the value of CONFIG_SHARED_IRQ_MAX_NUM_CLIENTS. Of course, to this you'd have to also factor in the size of the extra code. Since k is usually really small, I'd say it's linear. Worst case (k is comparable to N) the complexity is polynomial.

@ycsin ycsin mentioned this pull request Sep 29, 2023
Open
@davedesro davedesro mentioned this pull request Oct 19, 2023
Closed
@erwango erwango mentioned this pull request Nov 21, 2023
Merged
@ycsin ycsin mentioned this pull request Nov 22, 2023
Merged
@WardBenjamin WardBenjamin mentioned this pull request Jan 1, 2024
Closed
@henrikbrixandersen henrikbrixandersen mentioned this pull request Jan 8, 2024
Closed
@adri1mart1 adri1mart1 mentioned this pull request Jan 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcpleung dcpleung dcpleung left review comments

@dbaluta dbaluta dbaluta approved these changes

@andyross andyross andyross approved these changes

@fabiobaltieri fabiobaltieri fabiobaltieri approved these changes

@iuliana-prodan iuliana-prodan iuliana-prodan approved these changes

@carlocaione carlocaione carlocaione left review comments

@nashif nashif Awaiting requested review from nashif

@carlescufi carlescufi Awaiting requested review from carlescufi

@galak galak Awaiting requested review from galak

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@ioannisg ioannisg Awaiting requested review from ioannisg

@ceolin ceolin Awaiting requested review from ceolin

@cfriedt cfriedt Awaiting requested review from cfriedt

@jeremybettis jeremybettis Awaiting requested review from jeremybettis

@peter-mitsis peter-mitsis Awaiting requested review from peter-mitsis

@tejlmand tejlmand Awaiting requested review from tejlmand

@henrikbrixandersen henrikbrixandersen Awaiting requested review from henrikbrixandersen

@nordicjm nordicjm Awaiting requested review from nordicjm

Assignees

@andyross andyross

Labels
area: Architectures area: Base OS Base OS Library (lib/os) area: Build System area: Kernel Release Notes To be mentioned in the release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet