Skip to content

core: interrupt: new interrupt API functions - #6096

Merged
jforissier merged 5 commits into
OP-TEE:masterfrom
etienne-lms:itr-chip-1
Jun 14, 2023
Merged

core: interrupt: new interrupt API functions#6096
jforissier merged 5 commits into
OP-TEE:masterfrom
etienne-lms:itr-chip-1

Conversation

@etienne-lms

@etienne-lms etienne-lms commented Jun 7, 2023

Copy link
Copy Markdown
Contributor

5 patches etracted from #5954 + a fixup patch (for an added change not proposed in that reference P-R: sanity test on pageable attribute of interrupt handling resources) to propose new interrupt management API functions that can be used on CPU main interrupt controller as well as on secondary interrupt controllers.

@etienne-lms

Copy link
Copy Markdown
Contributor Author

I forgot to mention that commit "core: interrupt: core controller uses irq_chip list head" was reviewed through #5954 so I preserved @jenswi-linaro applied review tag.

@etienne-lms

Copy link
Copy Markdown
Contributor Author

Oups, I missed to add the gic/hfic/saic updata patches to provide mask/unmask handlers, hence why CI failed when running Qemu. I'll force push the series.

@etienne-lms

etienne-lms commented Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

I updated my branch but it seems something went wrong with github: the P-R was not updated.
(edited: i see the same issue with #6082)
re-edited: discard, everything went back to normal.

@borneoa borneoa left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Few typos and some suggest for consistency

Comment thread core/include/kernel/interrupt.h Outdated
* interrupt_configure() - Configure an interrupt in an interrupt controller
* @chip Interrupt controller
* @itr_num Interrupt number
* @type Trigger type ITR_TYPE_* of ITR_TYPE_NONE

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

s/of/or/ ?
Probably copy-paste the description from next API interrupt_configure(); it's more clear and keeps consistency. Also copy-paste for next argument "prio"

Comment thread core/include/kernel/interrupt.h Outdated
* @type Trigger type ITR_TYPE_* of ITR_TYPE_NONE
* @prio Interrupt priority
*
* Interrupt consumer that get their interrupt from the DT do not need to

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

s/consumer/consumers/

Comment thread core/include/kernel/interrupt.h Outdated
* interrupt_add_handler() - Register an interrupt handler
* @hdl Interrupt handler to register
*
* This helper function assumes interrypt type is set to IRQ_TYPE_NONE

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

s/interrypt/interrupt/

Comment thread core/include/kernel/interrupt.h Outdated
* interrupt_alloc_add_handler() - Allocate and register an interrupt handler
* @chip Interrupt controller
* @itr_num Interrupt number
* @handler Handler function

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For consistency, "Interrupt handler to register" ?

@etienne-lms

Copy link
Copy Markdown
Contributor Author

Comments addressed. Thanks @borneoa for the review.

Comment thread core/include/kernel/interrupt.h Outdated
*/
static inline void interrupt_enable(struct itr_chip *chip, size_t itr_num)
{
if (chip->ops->enable)

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.

Didn't we agree on making these mandatory?

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.

#5954 (comment):

Why not make this function pointer mandatory? Same for the other ops below.

Right, I missed the last sentence, sorry. Ok to make also .enable/.disable mandatory.

@etienne-lms

Copy link
Copy Markdown
Contributor Author

comment addressed

@jenswikl

jenswikl commented Jun 9, 2023

Copy link
Copy Markdown
Contributor

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Extends itr_chip framework to allow interrupt controllers to register
as interrupt chip and other interrupt management methods it their
owns interrupt consumer through the interrupt_xxx() API function.

This change does not modify the existing interrupt API function that
allow a driver to get an interrupt from the CPU main interrupt controller.
A later change will remove these old API functions.

This changes adds fields in existing structures defined in interrupt.h:
- itr_handler::chip back references the interrupt controller
- itr_chip::handlers is a list head for controller registered handlers
- itr_chip::name for debug trace purpose
- itr_ops::mask and itr_ops::unmask to mask/unmask an interrupt

The new API functions exposed to interrupt consumers are:
- interrupt_add_configure_handler(), interrupt_remove_handler() and
  helper functions interrupt_add_handler() and
  interrupt_add_handler_with_chip();
- interrupt_alloc_add_handler() and interrupt_remove_free_handler();
- interrupt_configure(), interrupt_enable(), interrupt_disable(),
  interrupt_mask() and interrupt_unmask();

Interrupt controllers shall call generic API function
interrupt_call_handlers() to have their registered consumer handlers
called upon their related interrupt occurrences.

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Implements GIC interrupts mask/unmask operation handlers using
interrupt disable/enable operation handlers.

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Implements Atmel SAIC interrupts mask/unmask operation handlers using
interrupt disable/enable operation handlers. This change is needed as
mask/unmask operation handlers are required by the new native interrupt
framework.

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Implements Hafnium interrupts mask/unmask operation handlers using
interrupt disable/enable operation handlers. This change is needed as
mask/unmask operation handlers are required by the new native interrupt
framework.

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Changes core interrupt controller API function (from interrupt.c) to
use the handlers list head added in struct itr_handler instead of
local list head. With this change, main itr_chip is managed as a
standard itr_chip and its interrupts can be fetched from the irq_chip
handler functions.

CPU primary interrupt handler itr_handle() function now calls
generic interrupt controller interrupt_call_handlers().

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
@etienne-lms

Copy link
Copy Markdown
Contributor Author

Thanks for the review and help.
Review tag applied.

@jforissier
jforissier merged commit 5f21fda into OP-TEE:master Jun 14, 2023
@etienne-lms
etienne-lms deleted the itr-chip-1 branch June 14, 2023 09:39
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.

4 participants