Skip to content

isr_tables: Support hardware interrupt vector table-only configuration. #22742

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

Merged

Conversation

stephanosio
Copy link
Member

@stephanosio stephanosio commented Feb 12, 2020

The existing isr_tables implementation does not allow enabling only
hardware interrupt vector table without software isr table.

This commit ensures that CONFIG_GEN_IRQ_VECTOR_TABLE can be used
without setting CONFIG_GEN_SW_ISR_TABLE.

Signed-off-by: Stephanos Ioannidis [email protected]

NOTE: This is necessary to support the Cortex-R cores (Cortex-R4 and -R5) with the Vectored Interrupt Controller (VIC) port that allows direct interrupt vectoring.

@stephanosio stephanosio force-pushed the isr_tables_support_hwvec_only branch from 663dc23 to 9750566 Compare February 13, 2020 01:55
@stephanosio stephanosio added this to the v2.3.0 milestone Feb 13, 2020
@SebastianBoe
Copy link
Collaborator

I'm not familiar enough with the irq handling to review, removing myself as reviewer.

@SebastianBoe SebastianBoe removed their request for review February 13, 2020 10:38
@jhedberg
Copy link
Member

@stephanosio @andrewboie is this ready to go in, or you're waiting for more reviews? Btw, I just triggered a Shippable re-run since it was a month ago when it last happened.

@stephanosio
Copy link
Member Author

@stephanosio @andrewboie is this ready to go in, or you're waiting for more reviews? Btw, I just triggered a Shippable re-run since it was a month ago when it last happened.

Given this patch's potential to break a lot of things, I suppose more reviews can't hurt before merging.

This is not terribly urgent as it will take some time for me to upstream the TI Hercules SoC port and that will likely be the only user of it.

@andyross @ioannisg your insight on this would be greatly appreciated.

@ioannisg
Copy link
Member

@andyross @ioannisg your insight on this would be greatly appreciated.

I will take a look as soon as possible

*/
#ifdef CONFIG_GEN_SW_ISR_TABLE
#define IRQ_VECTOR_TABLE_DEFAULT_ISR _isr_wrapper
#else
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add a comment here?

Copy link
Member

Choose a reason for hiding this comment

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

how about an inline comment here at the #else? This seems to be the differentiation brought in this patch

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if I understand what you mean by "inline comment" here.

Copy link
Member

Choose a reason for hiding this comment

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

Well I'd like a comment as the one in lines 41-44, for the #else case, i.e. when only the vector tables are used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it was quite self-explanatory -- that unconnected IRQs should have z_irq_spurious, as you would in _sw_isr_table.

I will add a comment explaining this anyway, since this might not be as obvious to the person reading this for the first time.

Copy link
Member

Choose a reason for hiding this comment

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

yes, please

Copy link
Member Author

Choose a reason for hiding this comment

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

/* When both the IRQ vector table and the software ISR table are used, populate
 * the IRQ vector table with the common software ISR by default, such that all
 * indirect interrupt vectors are handled using the software ISR table;
 * otherwise, populate the IRQ vector table with z_irq_spurious so that all
 * un-connected IRQ vectors end up in the spurious IRQ handler.
 */

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.

Looks fine, some requests for comments' enhancements only.

@stephanosio stephanosio force-pushed the isr_tables_support_hwvec_only branch from 9750566 to 5197e1b Compare March 12, 2020 14:12
@stephanosio
Copy link
Member Author

Looks fine, some requests for comments' enhancements only.

@ioannisg Thanks for reviewing. I have addressed your comments and pushed a new commit.

@stephanosio stephanosio requested a review from ioannisg March 12, 2020 14:13
The existing isr_tables implementation does not allow enabling only
hardware interrupt vector table without software isr table.

This commit ensures that CONFIG_GEN_IRQ_VECTOR_TABLE can be used
without setting CONFIG_GEN_SW_ISR_TABLE.

Signed-off-by: Stephanos Ioannidis <[email protected]>
@stephanosio stephanosio force-pushed the isr_tables_support_hwvec_only branch from 5197e1b to 87d32cd Compare March 12, 2020 14:57
@ioannisg
Copy link
Member

Given this patch's potential to break a lot of things, I suppose more reviews can't hurt before merging.

Let's merge this one - we are early in the 2.3 release cycle, so hopefully, there is enough time to detect and fix any bad effects of this PR

@ioannisg ioannisg merged commit e816ac7 into zephyrproject-rtos:master Mar 13, 2020
@stephanosio stephanosio deleted the isr_tables_support_hwvec_only 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants