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

irq: multilevel: compile 3rd level IRQ APIs only when enabled #81630

Merged

Conversation

ycsin
Copy link
Member

@ycsin ycsin commented Nov 20, 2024

This revert the idea of 3fa7d78 from #78845.

The 3rd level IRQ APIs won't compile when CONFIG_3RD_LEVEL_INTERRUPT_BITS=0.

Updated testcases accordingly.

Note

This PR is the second attempt of #81318, which broke main branch CI and was reverted by #81628

@@ -33,18 +37,26 @@ static void test_multi_level_bit_masks_fn(uint32_t irq1, uint32_t irq2, uint32_t
zassert_equal(hwirq1, irq_parent_level(irqn, 2));
}

if (has_l3) {
#ifdef CONFIG_3RD_LEVEL_INTERRUPTS
Copy link
Member Author

Choose a reason for hiding this comment

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

The level 3 APIs weren't guarded in #81318 which caused the compilation issue

Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jan 20, 2025
@cfriedt
Copy link
Member

cfriedt commented Jan 29, 2025

@ycsin - does this still need to be resolved?

@ycsin ycsin removed the Stale label Jan 29, 2025
@ycsin
Copy link
Member Author

ycsin commented Jan 29, 2025

@ycsin - does this still need to be resolved?

yeah, might need a rebase since it's been a while. Ping @andyross

@ycsin ycsin force-pushed the pr/kconfig-multilevel-alt branch from 48a1ae9 to 1cbe244 Compare January 29, 2025 17:26
cfriedt
cfriedt previously approved these changes Jan 30, 2025
@ycsin
Copy link
Member Author

ycsin commented Feb 6, 2025

ping @andyross @peter-mitsis

Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

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

A couple of suggestions for doxygen.

Otherwise, just a question - do we have build asserts to verify the value of each CONFIG_*_BITS value is within reason?

E.g. 0 < L1 < 31, 0 < L2 < 31, 0 < L3 < 30, 2 <= L2 + L2 <= 32, 3 <= L1 + L2 + L3 <= 32

Some of those checks might need to be made behind an ifdef.

Also, because it wasn't immediately obvious to me, it might be worth it to comment near the L2 values in the struct that there is no need to check if L2 is enabled if multi-level is enabled

This revert the idea of 3fa7d78 from zephyrproject-rtos#78845.

The 3rd level IRQ APIs won't compile when
CONFIG_3RD_LEVEL_INTERRUPT_BITS=0.

Updated testcases accordingly.

Signed-off-by: Yong Cong Sin <[email protected]>
Signed-off-by: Yong Cong Sin <[email protected]>
@ycsin
Copy link
Member Author

ycsin commented Feb 11, 2025

Otherwise, just a question - do we have build asserts to verify the value of each CONFIG_*_BITS value is within reason?

E.g. 0 < L1 < 31, 0 < L2 < 31, 0 < L3 < 30, 2 <= L2 + L2 <= 32, 3 <= L1 + L2 + L3 <= 32

Some of those checks might need to be made behind an ifdef.

The Kconfig define a range for the number of bits on each level, if their sum happen to be more than 32 bits, compilation will fail as it wont fit into uint32_t.

Also, because it wasn't immediately obvious to me, it might be worth it to comment near the L2 values in the struct that there is no need to check if L2 is enabled if multi-level is enabled

The current Kconfig allows 2ND level interrupt to be disabled when CONFIG_MULTI_LEVEL_INTERRUPTS=y, even though it doesn't make sense. I think it makes sense to select 2ND_LEVEL_INTERRUPTS when MULTI_LEVEL_INTERRUPTS=y

@ycsin ycsin requested a review from cfriedt February 11, 2025 15:36
@fabiobaltieri fabiobaltieri merged commit b2b2963 into zephyrproject-rtos:main Feb 19, 2025
22 checks passed
@ycsin ycsin deleted the pr/kconfig-multilevel-alt branch February 21, 2025 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants