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

Add Interrupt and timer support #2

Closed
wants to merge 10 commits into from

Conversation

WiseCrohn
Copy link
Collaborator

@WiseCrohn WiseCrohn commented Feb 7, 2023

Key aims in the design of this API:

  • Support systems that are M-mode only, and those which use M/U privilege modes
  • Leave all hardware-specific implementation to the BSP, so the API fully supports portable code (so details of routing interrupts, configuring parameters like edge / level triggered etc form part of the BSP implementation and are not on the API)
  • Functions should be reentrant / thread-safe: so one instance per hart, and separate context structures (memory spaces) for each instance, passed in at initialization time.
  • Also support the latest version of the RISC-V privileged spec (1.12 currently in review) where hardware support for handling traps in U-mode is removed. Traps must be handled in M-mode and passed to U-mode using MRET where applicable. This approach is backwards-compatible with older versions of the spec.

Wider assumptions about the "embedded" systems we support:

  • No virtualisation
  • No virtual memory / address translation / MMU
  • S/VS/HS privilege modes not applicable

To see the output specification in PDF form, get the latest from github actions here: https://github.com/riscv-non-isa/riscv-rvm-csi/actions/workflows/build-pdf.yml

- this also required some enhancements to schema and parser, to support
function type declarations and to ensure correct content in docs and headers.
Add u-mode functions for raising signals and setting timeouts
Split timer config into 2 functions
Note on thread-safety
Get parser unit test working again
@fanghuaqi
Copy link

Hi @WiseCrohn, could you put more details in the PR description about the interrupt API design assumption, and what should be put in CSI core part, what should be put in BSP part from your design.

@WiseCrohn
Copy link
Collaborator Author

Hi @fanghuaqi I added something, is this what you need?

@fanghuaqi
Copy link

Hi @WiseCrohn, could you put more details in the PR description about the interrupt API design assumption, and what should be put in CSI core part, what should be put in BSP part from your design.

Yes, that is what I want to know, thanks.

CSI_ENUM_LOAD_PAGE_FAULT,
CSI_ENUM_STORE_PAGE_FAULT,
CSI_NUM_STANDARD_TRAP_SOURCES, /* Must come last in this list */
} csi_trap_source_t;

Choose a reason for hiding this comment

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

Should trap source classified to interrupt and exception to match mcause.

image

This is how we define in NMSIS Core https://github.com/Nuclei-Software/NMSIS/blob/master/Device/_Template_Vendor/Vendor/Device/Include/Device.h#L64-L114

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

csi_trap_source_t is different from mcause, because it is much more detailed. For example, mcause only has a single entry for "machine external interrrupt", whereas csi_trap_source_t has to enumerate all the external interrupts in the system (the BSP extends it for this purpose). Also, it is useful to have all values of csi_trap_source_t contiguous (without gaps), so that the BSP can maintain a table which is indexed by this enumeration (without wasting memory).

I think there are no efficiency problems here because a decode from mcause (+ PLIC/AIA configuration) to csi_trap_source_t does not need to happen at the time an interrupt is taken (it can happen at the time a handler is registered). csi_trap_source_t just gives a convenient way of indexing sources for the purpose of the HAL API.

Choose a reason for hiding this comment

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

Also, it is useful to have all values of csi_trap_source_t contiguous (without gaps), so that the BSP can maintain a table which is indexed by this enumeration (without wasting memory)

Regarding the contiguous values, I think exception and interrupt should be enumerated in two enum, since exception id and interrupt id could be the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I think we have a misunderstanding. The way I conceived it, the actual enumerated values are not important, they don't relate to anything in the hardware. We have a single function (csi_register_m_isr) to register a (machine-mode) user handler for either interrupts or exceptions, so need a single enum.

Choose a reason for hiding this comment

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

OK, so all the handing of the csi_trap_source_t will leave to the implementation of csi_register_m_isr to process this enum.

Copy link

@fanghuaqi fanghuaqi Feb 16, 2023

Choose a reason for hiding this comment

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

And did you take a look at the riscv fast interrupt for mcu, it support vector interrupt and non-vector interrupt, it might be more necessary than plic/aia.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I would suggest a couple of enhancements arising from this. Firstly, the CLIC introduces interrupt levels as well as priorities (levels determine what interrupts can preempt others at the same privilege level, whereas priorities determine the order in which simultaneous interrupts are taken). So I propose to add APIs for setting interrupt levels and level threshold.
Also, I propose to say that the BSP should use vectored interrupts where possible, and we can introduce an additional API to allow users to register a "raw" handler that will be plugged directly into the vector table for a particular mcause (bypassing the base trap handler supplied by the BSP). This allows users to create their own fast, customized handler for a particular mcause (but the disadvantage is that the user's handler in that case would need to do all context save-and restore for itself - whereas a handler registered with csi_register_m_isr or csi_register_u_isr doesn't need to deal with that because the base handler does it).

@WiseCrohn
Copy link
Collaborator Author

I also propose a further enhancement which is to add arguments to the initialization function allowing to specify stack space for interrupt handling (separate stacks for M-mode and U-mode). The use of a separate stack for interrupts may be preferred instead of using the stack of the interrupted thread (easier system design and may save memory)

…vel,

csi_set_interrupt_level_thresh, csi_get_interrupt_level_thresh,
csi_register_raw_interrupt_handler, csi_register_raw_exception_handler.
- Add parameters to init function for U-mode and M-mode stack spaces.
- Further explanatory text and corrections.
(no change of functionality in this commit)
- Add priority argument to csi_set_m_timeout and csi_set_u_timeout
- Improve explanatory text
Add U-mode version of raw interrupt handling
@fanghuaqi
Copy link

Hi @WiseCrohn , I opened a issue in riscv/riscv-profiles#98 questioning about mcu profile, so we can set a more suitable interrupt api for specified mcu profile, which might be good to make the design for interrupt api narrowed for specified case.

Thanks
Huaqi

Also revise some of the introductory text.
No change to functionality.
@WiseCrohn WiseCrohn closed this Jun 9, 2023
@WiseCrohn
Copy link
Collaborator Author

Closing this PR and raising another as there has been a re-think to header file structure etc. following gap analysis.

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.

2 participants