-
Notifications
You must be signed in to change notification settings - Fork 8.4k
drivers: riscv: Add AIA (Advanced Interrupt Architecture) support #99767
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
base: main
Are you sure you want to change the base?
drivers: riscv: Add AIA (Advanced Interrupt Architecture) support #99767
Conversation
01c0855 to
b2411b6
Compare
|
Twister failures are not directly tied to my PR or involve Zephyr Rust Lang. I can fix them once the PR is approved, but I think more important things will appear in reviews before I get to that. |
|
| { | ||
| uint32_t topei; | ||
|
|
||
| __asm__ volatile("csrrw %0, " STRINGIFY(CSR_MTOPEI) ", x0" : "=r"(topei)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add read/write function to https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/arch/riscv/csr.h and use this instead.
| { | ||
| uint32_t value; | ||
|
|
||
| __asm__ volatile("csrw 0x350, %0" : : "r"(icsr_addr)); /* miselect */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/arch/riscv/csr.h already defines icsr read/write function. For sure, those should be used. I do not believe an additional imsic csr access function adds readability over that.
|
I am unsure if the current architecture in terms of how the functions are divided among files will stand the test of time. I think a single intc_riscv_aplic.c is fine. |
fkokosinski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some problems w/r to the commit history still unfortunately persist.
Could we split this PR into two: one which adds the base AIA support and one of the irq controller drivers, and then a subsequent one adding the other controller?
Judging by Kconfig, these two are fairly independent of each other. I reckon that would reduce the review time quite significantly.
| zephyr_library_sources_ifdef(CONFIG_RENESAS_RZ_EXT_IRQ intc_renesas_rz_ext_irq.c) | ||
| zephyr_library_sources_ifdef(CONFIG_RISCV_AIA intc_riscv_aia.c) | ||
| zephyr_library_sources_ifdef(CONFIG_RISCV_APLIC_MSI intc_riscv_aplic_msi.c) | ||
| zephyr_library_sources_ifdef(CONFIG_RISCV_IMSIC intc_riscv_imsic.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-IMSIC lines are included here
| @@ -0,0 +1,247 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No KConfig for this driver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to put them both (APLIC and IMSIC) inside the AIA Kconfig as AIA to keep them unified, i.e. I don't expect anyone to use both drivers separately.
| # Copyright (c) 2025 Synopsys, Inc. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| config RISCV_IMSIC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMSIC has been added in the previous commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the continuation of the error.
| @@ -0,0 +1,170 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing Kconfig symbol for this driver in this commit (has been added in the previous commit)
| config RISCV_AIA | ||
| bool "RISC-V AIA unified coordinator" | ||
| default y | ||
| depends on RISCV_IMSIC || RISCV_APLIC_MSI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should't this depend on RISCV_HAS_AIA as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PLIC also doesn't depend on any RISCV_HAS_PLIC - it just depends on DT_HAS_SIFIVE_PLIC_1_0_0_ENABLED, I just tried to follow the same approach. but with
- RISCV_IMSIC: default y if DT_HAS_RISCV_IMSIC_ENABLED
- RISCV_APLIC_MSI: default y if DT_HAS_RISCV_APLIC_MSI_ENABLED
I think this makes sense as AIA is kind of the abstraction of both, but I may add that dependency as well!
Well, the problem is that if we just do that I don't have real proof that they both work, as they work together, maybe doing some test with a direct icsr wrtie to IMSIC could work. Although they are independent I was trying to put them under AIA umbrella "unified" and planning that users would only use them through AIA APIs. However, I may introduce them separately and later unify them in a PR structure that would go IMSIC - APLIC - AIA - Qemu w/ APLIC. Do you think this would ease your life reviewing it? |
The Linux Kernel divides them and I took inspiration from there, indeed the two behaviors are very distinct and interrupt delivery has nothing to do, so keeping it this way would be easier IMO. What do you think should be split and what not split in your suggestion? |
Essentially anything that does not fall under the AIA section 4.9 "interrupt forwarding by MSIs" or is directly related to it should not be in a file that has msi in the filename. E.g. riscv_aplic_msi_global_enable would work just as well for direct delivery, as it even leaves the DM bit untouched. Interestingly, aplic_init() is rather MSI specific and does not carry the msi infix. |
| * Do not modify DM bit - respect hardware/props configuration. | ||
| */ | ||
| v |= APLIC_DOMAINCFG_IE; | ||
| v &= ~APLIC_DOMAINCFG_BE; /* LE */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems strange that clearing of BE would be hardcoded here.
99.9% of SoCs will be LE (with a 0.1% uncertainty).
As this is a read-modify-write sequence, and I would expect this LE systems to be configured for LE out of reset, this line seems unnecessary.
If you truly want to set it, you should rather make the setting based on BYTE_ORDER macro.
Yes, that would be much appreciated. Especially when it comes to reviewing and keeping the commit history clean. |



This PR implements support for RISC-V Advanced Interrupt Architecture (AIA) as proposed in RFC #82487, with some changes from the RFC scope.
Overview
Implements the APLIC (Advanced Platform-Level Interrupt Controller) and IMSIC (Incoming Message-Signaled Interrupt Controller) drivers, enabling AIA support for RISC-V SoCs. The implementation provides a PLIC-compatible interface while supporting modern MSI-based interrupt delivery. Also enables QEMU support to ensure correctness.
Implementation
Following the RFC guidance, this implementation:
irq_enable()/irq_disable()APIsriscv_aia.hAPI abstracts APLIC+IMSIC hierarchy and MSI configurationsoc_common_irq.cBeyond RFC Scope
The RFC proposed a single driver that "could be subdivided internally" and would later be "reworked into two separate drivers." This implementation goes directly to that split architecture:
intc_riscv_imsic.c,intc_riscv_aplic_msi.c, andintc_riscv_aia.ccoordinatorTesting
uart_echo_aia(single-hart) anduart_echo_aia_smp(multi-hart)I am keeping this as draft for now as I still need to catch the infinite loose ends that continuously appear. Reviews are very welcome.