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

core interrupt: use device tree data #6361

Merged
merged 6 commits into from
Nov 22, 2023

Conversation

etienne-lms
Copy link
Contributor

Following changes done recently in the interrupt framework, this P-R is based on #6097. It provides means to use DT bindings to describe interrupts and their controllers.

2nd commit adds an API function (dt_register_interrupt_provider())for interrupt controller drivers to register as itr_chip providers into the DT_DRIVER framework and few API functions for consumer driver to use DT info to find their interrupt (interrupt_dt_get(), interrupt_dt_get_by_index(), interrupt_dt_get_by_name()).

3rd commit Adds a API function to register a callback handler for an interrupt.
interrupt_create_handler() differs from existing interrupt_add_handler() in that the former does not reconfigure the interrupt since already done by interrupt_dt_get() or friend. Maybe interrupt_add_handler2() would be a better label.

Last commit registers GIC driver as a DT_DRIVER_INTERRUPT driver so that interrupt consumer drivers can use interrupt_dt_get_by_index() and friends.

core/drivers/gic.c Outdated Show resolved Hide resolved
@jenswi-linaro
Copy link
Contributor

Once you're done with the initial preparations of the PR, please squash in the fixups.

@etienne-lms
Copy link
Contributor Author

I've squashed the fixup commits.

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

One small comment for "core: interrupt: registering interrupt providers", with or without it:

Acked-by: Jerome Forissier <[email protected]>

core/include/kernel/interrupt.h Outdated Show resolved Hide resolved
Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

"core: dt_driver: don't enforce phandle 1st arg is a phandle":

Acked-by: Jerome Forissier <[email protected]>

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

"core: interrupt: add interrupt_create_handler()":

Acked-by: Jerome Forissier <[email protected]>

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

"drivers: gic: register to dt_driver":

Acked-by: Jerome Forissier <[email protected]>

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

"drivers: gic: use DT bindings":

Reviewed-by: Jerome Forissier <[email protected]>

@etienne-lms
Copy link
Contributor Author

#6361 (comment) addressed.
Tags applied.
Thanks for the review.

I've created a P-R in optee_docs for these new API functions: OP-TEE/optee_docs#218.

@jforissier
Copy link
Contributor

Please rebase onto master to fix the IBART error, thanks!

@etienne-lms
Copy link
Contributor Author

right, thanks.

@etienne-lms
Copy link
Contributor Author

rebased

@etienne-lms
Copy link
Contributor Author

Arrg! IBART still fails.

@jforissier
Copy link
Contributor

Arrg! IBART still fails.

Memory allocation error. Maybe a larger CFG_CORE_HEAP_SIZE is needed. Unfortunately not easy to change since the IBART config is not in this project.

@etienne-lms
Copy link
Contributor Author

I did not expect this change would make such platforms to consume more heap. IBART does not use DT for OP-TEE drivers inits.

@etienne-lms
Copy link
Contributor Author

Comment addressed.

core/drivers/gic.c Outdated Show resolved Hide resolved
@etienne-lms
Copy link
Contributor Author

oups... needs some rebasing.

@etienne-lms
Copy link
Contributor Author

rebased.

@jenswi-linaro
Copy link
Contributor

If "core: kernel: dt_driver: reference output device reference as void *" was possible before, why did we ever have pointer-pointers as arguments to those functions?

@etienne-lms
Copy link
Contributor Author

My bad. The several driver frameworks use struct clk **, struct rstctrl **, struct regulator **, etc... hence I chose void ** generic type in core/kernel/dt_driver.c to represent that (see commit b357d34) in the generic functions the several device frameworks rely on. Maybe a bad idea, I could have used void * at the time.

Note I will place commit "core: kernel: dt_driver: reference output device reference as void *" at the beginning of the series when I will squash the fixup commits.

@jenswi-linaro
Copy link
Contributor

Feel free to squash the fixup commits now. I'll take another look once it's done.

@etienne-lms
Copy link
Contributor Author

I've squashed the fixup commits and reordered the series.

@jforissier, I removed your A-b tag from commits "core: interrupt: registering interrupt providers"
and "drivers: gic: register to dt_driver" as content sligthly changed since you reviewed them.

@jenswi-linaro
Copy link
Contributor

Acked-by: Jens Wiklander <[email protected]>

@jforissier
Copy link
Contributor

I've squashed the fixup commits and reordered the series.

@jforissier, I removed your A-b tag from commits "core: interrupt: registering interrupt providers" and "drivers: gic: register to dt_driver" as content sligthly changed since you reviewed them.

Acked-by: Jerome Forissier <[email protected]> for both, thanks.

@etienne-lms etienne-lms force-pushed the itr-chip-2-dt branch 2 times, most recently from 257be21 to bb1d48a Compare November 8, 2023 09:33
@etienne-lms
Copy link
Contributor Author

etienne-lms commented Nov 8, 2023

@jbech-linaro, could you help me understand why IBART fails with this series? #6361 (comment) mentions a core heap exhaustion but since the platform used by IBART does not embed any secure DTB, I don't understand why these changes make the platform to consume more core heap.

(edited)

@etienne-lms
Copy link
Contributor Author

I forgot to mention that i've applied the review tags.

@jbech-linaro
Copy link
Contributor

As discussed in email. I've been able to reproduce the error on two difference machines. So, although I don't now that causes it, it at least seems reproducible.

@jbech-linaro jbech-linaro mentioned this pull request Nov 20, 2023
Changes local function device_from_provider_prop() to assume its
argument @prop points to the first argument to pass with phandle.

This change allows a later change to support other DT bindings
("interrupts" property) where 1st cell of the property is not
a phandle but the 1st phandle argument to be passed.

Acked-by: Jerome Forissier <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Changes dt_driver API function to reference device reference as
void * instead of void ** which could be confusing as the reference
can be a pointer to a device pointer (e.g. in clk_dt.c) or a pointer
to a structure (e.g. interrupt.c).

Acked-by: Jens Wiklander <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Adds interrupt chip framework API functions for an interrupt controller
to register as an interrupt provider in the driver probing sequence
based on device tree. This allows interrupt consumer to be deferred
when a dependent interrupt controller is not yet initialized.

Interrupt controllers register a driver in DT_DRIVER providers list
with: interrupt_register_provider().

Interrupt consumer can get their interrupt through DT data with
interrupt_dt_get(), interrupt_dt_get_by_index() or
interrupt_dt_get_by_name().

This change removes inclusion of interrupt.h from kernel/dt.h as it is
not needed and conflicts with inclusion of kernel/dt.h from
kernel/interrupt.h.

Acked-by: Jens Wiklander <[email protected]>
Acked-by: Jerome Forissier <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Adds interrupt_create_handler() API function in interrupt framework.
The function is to be used with interrupt controls obtained
from the DT with  interrupt_dt_get() interrupt_dt_get_by_index() or
interrupt_dt_get_by_name().

The function differs from legacy interrupt_add_handler() in that
this latter always reconfigure the interrupt while new
interrupt_create_handler() function assumes the interrupt was configured
from interrupt_dt_get() or friends.

Acked-by: Jerome Forissier <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Registers GIC driver as an interrupt controller in DT_DRIVER
providers when DT is supported. This change allows interrupt
consumer nodes to leverage interrupts and interrupts-extended
properties DT bindings for their device drivers to retrieve
their interrupts.

Acked-by: Jens Wiklander <[email protected]>
Acked-by: Jerome Forissier <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
Use DT bindings GIC_PPI and GIC_SIP instead of 1 and 0 raw values.

Acked-by: Jerome Forissier <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Signed-off-by: Etienne Carriere <[email protected]>
@etienne-lms
Copy link
Contributor Author

IBART CI test failure should be fixed by OP-TEE/optee_test#711.

@etienne-lms
Copy link
Contributor Author

IBART's happy!

@jforissier jforissier merged commit 8c7282b into OP-TEE:master Nov 22, 2023
8 checks passed
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