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

soc: miv: polarfire: Increase NUM_IRQS to cover 1st and 2nd level irqs #86220

Merged
merged 1 commit into from
Mar 19, 2025

Conversation

mars-low
Copy link
Contributor

@mars-low mars-low commented Feb 24, 2025

It was detected by test_sw_isr_irq_parent_table_idx in arch.interrupt:

ASSERTION FAIL [table_idx < (186 - 0)] @ WEST_TOPDIR/zephyr/arch/common/multilevel_irq.c:91 table_idx(186) < IRQ_TABLE_SIZE(186)

NUM_IRQS was previously set to the same value as MAX_IRQ_PER_AGGREGATOR and it didn't take into account the number of 1st level interrupts specified by 2ND_LVL_ISR_TBL_OFFSET. In the generated __sw_isr_table, Level 2 interrupts start at the offset specified by 2ND_LVL_ISR_TBL_OFFSET.

For PolarFire SoC, upper interrupt sources for PLIC correspond to Bus Error Unit and Fabric Interface. They are currently not used by platforms in Zephyr, so the previous value of NUM_IRQS hasn't caused issues for regular applications.

As it doesn't look like an explicit memory optimization, increase NUM_IRQS to allow kernel interrupt tests to pass.

Note:

2ND_LVL_ISR_TBL_OFFSET=13 and NUM_IRQS=199 don't include additional 48 Local Interrupts supported by cores. In total, there are 64 bits in Machine Interrupt Pending Register (mip) which can be used to configure 1st level interrupts.

As a further extension to the platform, values could be extended to 2ND_LVL_ISR_TBL_OFFSET=64 and NUM_IRQS=250. This commit increases NUM_IRQS by a minimal value required to pass kernel interrupt tests.

Link: https://ww1.microchip.com/downloads/aemDocuments/documents/FPGA/ProductDocuments/ReferenceManuals/PolarFire_SoC_FPGA_MSS_Technical_Reference_Manual_VC.pdf

Fixes #86219

Copy link

Hello @mars-low, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

It was detected by test_sw_isr_irq_parent_table_idx in arch.interrupt:

ASSERTION FAIL [table_idx < (186 - 0)]
@ WEST_TOPDIR/zephyr/arch/common/multilevel_irq.c:91
table_idx(186) < IRQ_TABLE_SIZE(186)

NUM_IRQS was previously set to the same value as MAX_IRQ_PER_AGGREGATOR
and it didn't take into account the number of 1st level interrupts
specified by 2ND_LVL_ISR_TBL_OFFSET. In the generated __sw_isr_table,
Level 2 interrupts start at the offset specified by 2ND_LVL_ISR_TBL_OFFSET.

For PolarFire SoC, upper interrupt sources for PLIC correspond to
Bus Error Unit and Fabric Interface. They are currently not used by
platforms in Zephyr, so the previous value of NUM_IRQS hasn't caused
issues for regular applications.

As it doesn't look like an explicit memory optimization, increase NUM_IRQS
to allow kernel interrupt tests to pass.

Note:

2ND_LVL_ISR_TBL_OFFSET=13 and NUM_IRQS=199 don't include additional
48 Local Interrupts supported by cores. In total, there are
64 bits in Machine Interrupt Pending Register (mip)
which can be used to configure 1st level interrupts.

As a further extension to the platform, values could be extended to
2ND_LVL_ISR_TBL_OFFSET=64 and NUM_IRQS=250. This commit increases
NUM_IRQS by a minimal value required to pass kernel interrupt tests.

Link: https://ww1.microchip.com/downloads/aemDocuments/documents/FPGA/ProductDocuments/ReferenceManuals/PolarFire_SoC_FPGA_MSS_Technical_Reference_Manual_VC.pdf

Signed-off-by: Marek Slowinski <mslowinski@antmicro.com>
@mars-low mars-low force-pushed the mpfs-increase_num_irqs branch from e6915c5 to fef1fa1 Compare February 25, 2025 08:29
@fkokosinski
Copy link
Member

Hey @con-pax - could you take a look at this PR?

@con-pax
Copy link
Contributor

con-pax commented Mar 3, 2025

Hi @mars-low,
Thanks for the PR. To be honest, this is something that I have struggled with in Zephyr, i.e, the definition of 2nd level interrupt offset. the 13 represents the delta between the MSS interrupt line and the MSS->fabric Interrupt Controller Vector (M2F-Vector).
However, the PLIC only has 186 lines, so NUM_IRQ's being greater than that feels a little weird.

In testing, I almost always set 2ND_LVL_ISR_TBL_OFFSET to zero, aligning with Linux

@mars-low
Copy link
Contributor Author

mars-low commented Mar 5, 2025

Hello @con-pax,
I see the possible source of confusion when working with these offsets in Zephyr.
In Zephyr nomenclature 2ND_LVL_ISR_TBL_OFFSET refers to Software Interrupts as opposed to Vectored Interrupts:

config 2ND_LVL_ISR_TBL_OFFSET
int "Offset in _sw_isr_table for level 2 interrupts"
default 0
depends on 2ND_LEVEL_INTERRUPTS
help
This is the offset in _sw_isr_table, the generated ISR handler table,
where storage for 2nd level interrupt ISRs begins. This is
typically allocated after ISRs for level 1 interrupts.

The generated file has the following shape:

isr_tables.c
/* AUTO-GENERATED by gen_isr_tables.py, do not edit! */

#include <zephyr/toolchain.h>
#include <zephyr/linker/sections.h>
#include <zephyr/sw_isr_table.h>
#include <zephyr/arch/cpu.h>

typedef void (* ISR)(const void *);
struct _isr_table_entry __sw_isr_table _sw_isr_table[199] = {
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 0 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 1 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 2 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 3 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 4 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 5 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 6 */
	{(const void *)0x0, (ISR)0x800030b0}, /* 7 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 8 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 9 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 10 */
	{(const void *)0x80007058, (ISR)0x800021b8}, /* 11 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 12 */
	/* Level 2 interrupts start here (offset: 13) */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 13 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 14 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 15 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 16 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 17 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 18 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 19 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 20 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 21 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 22 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 23 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 24 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 25 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 26 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 27 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 28 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 29 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 30 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 31 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 32 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 33 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 34 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 35 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 36 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 37 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 38 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 39 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 40 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 41 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 42 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 43 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 44 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 45 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 46 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 47 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 48 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 49 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 50 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 51 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 52 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 53 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 54 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 55 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 56 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 57 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 58 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 59 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 60 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 61 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 62 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 63 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 64 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 65 */
	{(const void *)0x80007080, (ISR)0x8000279e}, /* 66 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 67 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 68 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 69 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 70 */
	{(const void *)0x800070d0, (ISR)0x800028da}, /* 71 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 72 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 73 */
	{(const void *)0x800070a8, (ISR)0x800028da}, /* 74 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 75 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 76 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 77 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 78 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 79 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 80 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 81 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 82 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 83 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 84 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 85 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 86 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 87 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 88 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 89 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 90 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 91 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 92 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 93 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 94 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 95 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 96 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 97 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 98 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 99 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 100 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 101 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 102 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 103 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 104 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 105 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 106 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 107 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 108 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 109 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 110 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 111 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 112 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 113 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 114 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 115 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 116 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 117 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 118 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 119 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 120 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 121 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 122 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 123 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 124 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 125 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 126 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 127 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 128 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 129 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 130 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 131 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 132 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 133 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 134 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 135 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 136 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 137 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 138 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 139 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 140 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 141 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 142 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 143 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 144 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 145 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 146 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 147 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 148 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 149 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 150 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 151 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 152 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 153 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 154 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 155 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 156 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 157 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 158 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 159 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 160 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 161 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 162 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 163 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 164 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 165 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 166 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 167 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 168 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 169 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 170 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 171 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 172 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 173 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 174 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 175 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 176 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 177 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 178 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 179 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 180 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 181 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 182 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 183 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 184 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 185 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 186 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 187 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 188 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 189 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 190 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 191 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 192 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 193 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 194 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 195 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 196 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 197 */
	{(const void *)0x0, (ISR)z_irq_spurious}, /* 198 */
};

There are handlers allocated for Machine Timer Interrupt (7) and for Machine Global Interrupt (11) - these are 1st level interrupts.
All ISRs for PLIC (2nd level interrupts) have offset of 2ND_LVL_ISR_TBL_OFFSET and they follow 1st level interrupts.

If the original intent for 2ND_LVL_ISR_TBL_OFFSET=13 was to represent the delta between the MSS interrupt line and the MSS->fabric Interrupt Controller Vector (M2F-Vector), we can actually set 2ND_LVL_ISR_TBL_OFFSET=12 per Zephyr nomenclature (as no 1st level interrupts above index 11 are used)) to indicate that it has a distinct meaning in this context. Or maybe even better we set 2ND_LVL_ISR_TBL_OFFSET=64 so that all 1st level interrupts are covered.

@con-pax
Copy link
Contributor

con-pax commented Mar 10, 2025

Hey @mars-low,
Apologies for the delay, last week got away from me.
I am happy enough with this current PR, with the view to extend NUM_IRQ's in the future if required. I want to test this PR out today prior to approving. Cheers!

@fkokosinski
Copy link
Member

Hey @mars-low, Apologies for the delay, last week got away from me. I am happy enough with this current PR, with the view to extend NUM_IRQ's in the future if required. I want to test this PR out today prior to approving. Cheers!

Hey @con-pax - did you perhaps had the chance to test this PR? Thanks!

@con-pax
Copy link
Contributor

con-pax commented Mar 19, 2025

Hey @fkokosinski apologies for the delay, I was holding out to test with an AMP example that registers interrupts in the higher numbers. I didnt get my hands on it, but I'm confident there are no breaking changes here. I am happy enough with this so I will approve.

Copy link
Contributor

@con-pax con-pax left a comment

Choose a reason for hiding this comment

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

LGTM

@nashif nashif merged commit 9946c35 into zephyrproject-rtos:main Mar 19, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: Microchip RISC-V size: XS A PR changing only a single line of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

soc: miv: polarfire: test_sw_isr_irq_parent_table_idx assertion failure
7 participants