Skip to content

core: using native interrupt handlers - #192

Closed
etienne-lms wants to merge 4 commits into
OP-TEE:masterfrom
etienne-lms:itr
Closed

core: using native interrupt handlers#192
etienne-lms wants to merge 4 commits into
OP-TEE:masterfrom
etienne-lms:itr

Conversation

@etienne-lms

Copy link
Copy Markdown
Contributor

The P-R is related to OP-TEE/optee_os#5954.

It adds a section in core architecture documentation on how to implement and register interrupt controllers and handler functions.

@jforissier jforissier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comments below, then:

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Comment thread architecture/core.rst
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated

@etienne-lms etienne-lms left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @jforissier. I won't apply your review tag until OP-TEE/optee_os#5954 (or equivament) is merged, as this P-R adds a description for something that is not yet supported in OP-TEE core.

Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
platform. When used, the platform must provide a secure devicetree blob
(DTB) which platform and device drivers can use to getreferences to
interrupt controllers and relative interrupt numbers theyconsume.
Main API functions are:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The main

Ok, thanks.

(some definite articles are missing in a few other places I believe but my English is not good enough to decide it omitting them is OK or not).

mine neither ;)

Comment thread architecture/core.rst
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
@jforissier

Copy link
Copy Markdown
Contributor

LGTM you may squash the commits if you want, I will hold off merging this until there is code to actually implement it.

Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
Comment thread architecture/core.rst Outdated
@etienne-lms

Copy link
Copy Markdown
Contributor Author

I'll remove commit "faq: Fix typo" that is completely unrelated and already addressed by #190. Sorry for the noise.

@etienne-lms

Copy link
Copy Markdown
Contributor Author

review comments addressed

@jockebech

Copy link
Copy Markdown
Contributor

Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

@jforissier

Copy link
Copy Markdown
Contributor

My ack still holds.

Adds a section in core architecture documentation on how to
implement and register interrupt controllers and handler functions.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
@etienne-lms

etienne-lms commented May 10, 2023

Copy link
Copy Markdown
Contributor Author

squashed and applied review tags.
thanks for the reviews and feedback.

Please remember to not merge this change until OP-TEE/optee_os#5954 (or equiv) is merged.

Update description to P-R #5954 latest state:
- update function labels
- mask/unmask operation handlers are required and enable/disable
  are optional.
- mention itr_chip_init() and interrupt_main_init().

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
@etienne-lms

Copy link
Copy Markdown
Contributor Author

Updated according to OP-TEE/optee_os#5954 latest state.

Fixes a example code snippet.

Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
Explains how the using an async notifier and an interrupt handler
to relax CPU in a thread task.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
@etienne-lms

etienne-lms commented Jun 13, 2023

Copy link
Copy Markdown
Contributor Author

Added a fixup commit for a typo in the provided example.

Added a section exampling how to use aysnc notif + an interrupt to relax CPU in a thread execution context.

Added a section on a interrupt framework evolution to allows a interrupt consumer to register a bottom half handler function to be called in OP-TEE common bottom half notification thread. There is no P-R created for the changes required in interrupt.c/.h, i'll create a P-R later, if the API presented here looks OK.
(edited) Remove, you'll see bottom half interrupt handling later.

@github-actions

Copy link
Copy Markdown

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants