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

Defined the first CLIC interrupts to be an alias of mie/mip #458

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

christian-herber-nxp
Copy link
Collaborator

No description provided.

@christian-herber-nxp christian-herber-nxp linked an issue Mar 5, 2025 that may be closed by this pull request
Copy link

@tovine tovine left a comment

Choose a reason for hiding this comment

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

I like these changes, they simplify the interaction with basic interrupts and removes a lot of the previous uncertainty and "platform defined behavior" regarding the lower interrupt indices.
But does this also mean that all external IRQs essentially have to start at CLIC index 1, or is there an offset between external IRQ number and xcause IRQ number?

Note that I've been out of the loop for a bit (busy with other tasks) so I might be missing something that's obvious in the spec, please forgive that ignorance.

@@ -788,20 +790,6 @@ If {mideleg} exists, the {mideleg} CSR ceases to have effect in CLIC mode. If {
CSR is still accessible and state bits retain their values when
switching between CLIC and CLINT interrupt modes.

==== Changes to {mie}/{mip} CSRs
Copy link

Choose a reason for hiding this comment

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

Does this removal mean that there are no longer any changes to the mie and mip CSRs (instead merging them into clicintip/ie[0-1]?

@@ -2898,128 +2884,6 @@ In all cases, conditionally swapping the stack to account for
potential privilege-mode changes adds two extra instructions to all
interrupt handlers.

[appendix]
Copy link

Choose a reason for hiding this comment

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

Nice simplification.
I guess this means that clicintip[0] and clicintie[0] will look like this then?
image

@@ -501,6 +497,11 @@ Each interrupt input has a dedicated interrupt pending bit
Software should assume `clicintip[__i__]=0` means no interrupt pending, and
`clicintip[__i__]=1` indicates an interrupt is pending.

The the lowest index `clicintip[__i__]` bits are an alias of the `mip` CSR.

Choose a reason for hiding this comment

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

The the -> The

NOTE: In contrast, since {status}.{ie} only takes effect in the current privilege
mode according to RISC-V convention, an interrupt `_i_` from a higher privilege mode
is enabled as long as `clicintie[__i__]` is set (regardless of the setting
of {status}.{ie} in the higher privilege modes).

The the lowest index `clicintie[__i__]` bits are an alias of the `mie` CSR.

Choose a reason for hiding this comment

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

The the -> The

@Silabs-ArjanB
Copy link

Hi @christian-herber-nxp

I can't see any reason why we would want to have CLIC aliases related to mie/mip (please let me know what benefit we might get from this), but I see many reasons why we do not want this aliasing:

  • The mip/mie indices have very specific (hard-wired, non-obvious) priorities and for CLIC interrupts priorities/levels are programmable. If you introduce the aliasing you only enforce a 'seemingly' consistent access mechanism for some IRQs, but the associated priorities/levels become unclear (e.g. does the CLINT spec apply, does the CLIC spec apply, does CLIC needs to be programmed to mimic CLINT, etc.?)
  • The aliasing only applies to some (and not all) IRQs, so we would have a non-uniform (and confusing) access paradigm.
  • The mip/mie bitfield allocation is not tightly packed. For hardware vectored interrupts this scheme will lead to a poor memory utilization of the IRQ vector table. (That is not an issue if you have say 1000 IRQs, but it is a problem if you for example have 10 IRQs).
  • There seems to be no benefit of the aliasing that contributes to the goals in the charter of the Fast Interrupt task group (noticeably 'faster IRQs')
  • Aliasing in general leads to confusion and lack of simplicity.
  • A similar proposal of sticking to the original CLINT IRQ ordering was discussed at length 5 years ago (see Proposal: reorder lower 16 bits #89 which was mainly discussed between @silabs-PaulZ, @kasanovic , @Kevin-Andes ) and in that ticket you see various reasons why we would not want to stick to the CLINT ordering.

CLIC should be a clean sheet design in my opinion not burdened by choices made for CLINT (or AIA).

On purpose #89 led to only recommendations on IRQ ordering. Please don't impose reserved indices if they lead to holes in the IRQ vector table. If you want to impose ordering at any cost, then I would propose that you make it part of a separate optional extension.

@christian-herber-nxp
Copy link
Collaborator Author

Hi @christian-herber-nxp

I can't see any reason why we would want to have CLIC aliases related to mie/mip (please let me know what benefit we might get from this), but I see many reasons why we do not want this aliasing:

  • The mip/mie indices have very specific (hard-wired, non-obvious) priorities and for CLIC interrupts priorities/levels are programmable. If you introduce the aliasing you only enforce a 'seemingly' consistent access mechanism for some IRQs, but the associated priorities/levels become unclear (e.g. does the CLINT spec apply, does the CLIC spec apply, does CLIC needs to be programmed to mimic CLINT, etc.?)

AIA defines this pretty clearly. If the prio registers are read only zero, the default prios apply. Else, it is the programmable priorities.

  • The aliasing only applies to some (and not all) IRQs, so we would have a non-uniform (and confusing) access paradigm.
  • The mip/mie bitfield allocation is not tightly packed. For hardware vectored interrupts this scheme will lead to a poor memory utilization of the IRQ vector table. (That is not an issue if you have say 1000 IRQs, but it is a problem if you for example have 10 IRQs).

Can you quantify the issue? You are wasting 4B per interrupt that is not implemented. Is this an issue? This issue does exist with CLINT also.

  • There seems to be no benefit of the aliasing that contributes to the goals in the charter of the Fast Interrupt task group (noticeably 'faster IRQs')
  • Aliasing in general leads to confusion and lack of simplicity.
  • A similar proposal of sticking to the original CLINT IRQ ordering was discussed at length 5 years ago (see Proposal: reorder lower 16 bits #89 which was mainly discussed between @silabs-PaulZ, @kasanovic , @Kevin-Andes ) and in that ticket you see various reasons why we would not want to stick to the CLINT ordering.

CLIC should be a clean sheet design in my opinion not burdened by choices made for CLINT (or AIA).

On purpose #89 led to only recommendations on IRQ ordering. Please don't impose reserved indices if they lead to holes in the IRQ vector table. If you want to impose ordering at any cost, then I would propose that you make it part of a separate optional extension.

I hear your concerns and at this point, we need to sort our options. We also want to make sure we are consistent with other interrupt models defined in RISC-V. Some of the choices that have been made under different circumstances (e.g. the registers being memory mapped, in which case the aliasing would have been almost impossible).

A clean sheet design is not a requirement in itself. We need fast interrupts, and need to achieve this in the most compatible way possible. Being fully clean sheet run into the risk of reinventing the wheel. This has happened in CLIC, mostly due to the long lifetime of the TG.

If we do the aliasing or not needs to be decided by the TG. In last weeks meeting, people were in favor of this. But this is really a rather minor issue in the big picture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undo the removal of xie / xip
3 participants