init: add sub-priority to internal ordering #60410

JordanYates · 2023-07-16T01:06:33Z

Add a sub-priority field to Z_INIT_ENTRY_SECTION, which is used to
ensure that multiple drivers running at the same init priority still
run in an order that respects their devicetree dependencies.

This is primarily useful when multiple instances of the same device are
instantiated that can depend on each other, for example power domains
and i2c muxes.

After this PR, an init level now runs all SYS_INIT/DEVICE_DEFINE macros first,
then DEVICE_DT_DEFINE macros in order of devicetree ordinals.

Linker section names now look something like this:

 *(SORT_BY_NAME(SORT_BY_ALIGNMENT(.z_init_POST_KERNEL?_*)))
 *(SORT_BY_NAME(SORT_BY_ALIGNMENT(.z_init_POST_KERNEL??_*)))
 .z_init_POST_KERNEL40_0006_
                0x00000000000042b8        0x8 zephyr/drivers/gpio/libdrivers__gpio.a(gpio_stellaris.c.obj)
 .z_init_POST_KERNEL40_0017_
                0x00000000000042c0        0x8 zephyr/drivers/gpio/libdrivers__gpio.a(gpio_stellaris.c.obj)
 .z_init_POST_KERNEL40_0018_
                0x00000000000042c8        0x8 zephyr/drivers/gpio/libdrivers__gpio.a(gpio_stellaris.c.obj)
 .z_init_POST_KERNEL40_0019_
                0x00000000000042d0        0x8 zephyr/drivers/gpio/libdrivers__gpio.a(gpio_stellaris.c.obj)
 .z_init_POST_KERNEL40_0020_
                0x00000000000042d8        0x8 zephyr/drivers/gpio/libdrivers__gpio.a(gpio_stellaris.c.obj)

Resolves #22545 for multiple instances of the same device

Generate a zero padded variant of `_ORD` that is suitable for use in linker scripts with the `SORT` property, so that `6` is correctly placed before `24`, and so on. Signed-off-by: Jordan Yates <[email protected]>

Add a sub-priority field to `Z_INIT_ENTRY_SECTION`, which is used to ensure that multiple drivers running at the same init priority still run in an order that respects their devicetree dependencies. This is primarily useful when multiple instances of the same device are instantiated that can depend on each other, for example power domains and i2c muxes. Signed-off-by: Jordan Yates <[email protected]>

Add tests for sorting devices at the same init priority based on their devicetree ordinals. Signed-off-by: Jordan Yates <[email protected]>

include/zephyr/devicetree/ordinals.h

Add basic testing of the new `DT_DEP_ORD_STR_SORTABLE` macro. Signed-off-by: Jordan Yates <[email protected]>

fabiobaltieri

@JordanYates could you update https://github.com/zephyrproject-rtos/zephyr/blob/main/scripts/build/check_init_priorities.py#L68 to parse the new format and compare/print it properly? (there's also some unit tests https://github.com/zephyrproject-rtos/zephyr/blob/main/scripts/build/check_init_priorities_test.py and an integration test that somehow is still working https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/misc/check_init_priorities)

Update the script to parse the new section naming. The ordering type is converted from an integer to a tuple, which still compares correctly due to the elementwise behaviour of tuple comparison. Signed-off-by: Jordan Yates <[email protected]>

Fix the test script for the changes made to section naming. Signed-off-by: Jordan Yates <[email protected]>

fabiobaltieri

Thanks for the extra fixes!

tests/misc/check_init_priorities/validate_check_init_priorities_output.py

andyross

Seems reasonable, but also seems worth pointing out that this is yet more code we're adding to make up for our lack of proper dependency tracking on static devices...

JordanYates · 2023-07-19T23:35:16Z

Seems reasonable, but also seems worth pointing out that this is yet more code we're adding to make up for our lack of proper dependency tracking on static devices...

Totally agree, but as a minimal internal change with no effect on the API that solves a real problem I think it is worth it

gmarull · 2023-07-20T07:52:27Z

Seems reasonable, but also seems worth pointing out that this is yet more code we're adding to make up for our lack of proper dependency tracking on static devices...

Totally agree, but as a minimal internal change with no effect on the API that solves a real problem I think it is worth it

+1, the init problem® won't be easy to solve. For example, all the flexibility we've given with SYS_INIT is now hitting us hard.

fabiobaltieri · 2023-07-20T09:54:57Z

Seems reasonable, but also seems worth pointing out that this is yet more code we're adding to make up for our lack of proper dependency tracking on static devices...

Totally agree, but as a minimal internal change with no effect on the API that solves a real problem I think it is worth it

+1, the init problem® won't be easy to solve. For example, all the flexibility we've given with SYS_INIT is now hitting us hard.

I'd be surprised at this point if someone comes up with a obviously perfect solution, but for now I think these progressive changes are moving in the right direction. I'm thinking, this takes care of same-level, the build time checking does multiple-level, @bjarki-trackunit were you looking into printing the init call sequence in #55654 (comment)? I'm thinking that having an init sequence printer could be used by subsystem tests and downstream application to compare the final init sequence with a golden sample so that any change gets caught and manually reviewed. That'd be a pretty solid system IMO.

fabiobaltieri · 2023-07-20T11:15:39Z

@JordanYates I linked this up in #release on Discord and it's been pointed out that since it's been a hot topic recently, it may be worth running it through the Architecture WG before hitting the merge button (@carlescufi). Any chance you could talk about it? Happy to help out if the time zone is a problem btw.

Tagging as DNM for now.

JordanYates · 2023-07-20T12:58:43Z

I don't think the change is controversial enough to require me to personally argue its merits 🙂 1am is a high bar. If you are happy to introduce the topic that's good enough for me.

fabiobaltieri · 2023-07-20T14:33:02Z

I don't think the change is controversial enough to require me to personally argue its merits slightly_smiling_face 1am is a high bar. If you are happy to introduce the topic that's good enough for me.

Yeah well, you never know, but I think the same. Happy to present on your behalf at the next WG then, @gmarull will you be there as well?

gmarull · 2023-07-20T14:34:22Z

I don't think the change is controversial enough to require me to personally argue its merits slightly_smiling_face 1am is a high bar. If you are happy to introduce the topic that's good enough for me.

Yeah well, you never know, but I think the same. Happy to present on your behalf at the next WG then, @gmarull will you be there as well?

I'll be there, but TBH, I'd just go ahead and merge. This is not really a substantial change, just resolves some issues present in the current implementation.

dkalowsk

Looks pretty clean!

cfriedt · 2023-07-22T10:18:36Z

Seems reasonable, but also seems worth pointing out that this is yet more code we're adding to make up for our lack of proper dependency tracking on static devices...

Totally agree, but as a minimal internal change with no effect on the API that solves a real problem I think it is worth it

Honestly, an LTO plugin seems increasingly more appealing these days ..

carlescufi · 2023-07-25T15:56:29Z

Architecture WG:

Incremental improvement using current infrastructure. Removes dependency on linking order for devices that have the same priority. This instead appends the Devicetree ordinal to remove any dependency from linking order.
Anyone relying on linking order shouldn't have been in the first place.

Following #60410, the NXP S32 external interrupt controller device initializes after the core interrupt controller. Bump the NXP S32 intc init level to initialize after the core intc and before the GPIO device driver. Fixes #61218 Signed-off-by: Manuel Argüelles <[email protected]>

Jordan Yates added 2 commits July 16, 2023 10:47

dts: gen_defines: generate _ORD_STR_SORTABLE

d1f525f

Generate a zero padded variant of `_ORD` that is suitable for use in linker scripts with the `SORT` property, so that `6` is correctly placed before `24`, and so on. Signed-off-by: Jordan Yates <[email protected]>

JordanYates requested review from MaureenHelm, andyross, carlescufi, dcpleung, galak, mbolivar-nordic, nashif and tbursztyka as code owners July 16, 2023 01:06

zephyrbot added area: Devicetree area: Device Model area: Kernel labels Jul 16, 2023

zephyrbot requested review from ceolin, gmarull and peter-mitsis July 16, 2023 01:07

zephyrbot assigned gmarull Jul 16, 2023

tests: kernel: device: test sub-priority

5403660

Add tests for sorting devices at the same init priority based on their devicetree ordinals. Signed-off-by: Jordan Yates <[email protected]>

This was referenced Jul 16, 2023

pm: device: add driver init helper #60411

Merged

drivers: power_domain: gpio: Allow enabled at boot #59772

Closed

gmarull reviewed Jul 17, 2023

View reviewed changes

include/zephyr/devicetree/ordinals.h Show resolved Hide resolved

tests: lib: devicetree: api: test DT_DEP_ORD_STR_SORTABLE

cc71d4e

Add basic testing of the new `DT_DEP_ORD_STR_SORTABLE` macro. Signed-off-by: Jordan Yates <[email protected]>

gmarull previously approved these changes Jul 18, 2023

View reviewed changes

gmarull requested review from carlocaione and fabiobaltieri July 19, 2023 08:10

fabiobaltieri requested changes Jul 19, 2023

View reviewed changes

Jordan Yates added 2 commits July 19, 2023 21:20

scripts: build: check_init_priorities_test: fix

a0bbdc1

Fix the test script for the changes made to section naming. Signed-off-by: Jordan Yates <[email protected]>

JordanYates dismissed gmarull’s stale review via a0bbdc1 July 19, 2023 11:22

zephyrbot added the area: Build System label Jul 19, 2023

fabiobaltieri approved these changes Jul 19, 2023

View reviewed changes

tests/misc/check_init_priorities/validate_check_init_priorities_output.py Show resolved Hide resolved

andyross approved these changes Jul 19, 2023

View reviewed changes

fabiobaltieri requested a review from a team July 20, 2023 11:13

fabiobaltieri added the DNM This PR should not be merged (Do Not Merge) label Jul 20, 2023

dkalowsk approved these changes Jul 20, 2023

View reviewed changes

cfriedt approved these changes Jul 22, 2023

View reviewed changes

JordanYates mentioned this pull request Jul 25, 2023

Revert "pm: device: add driver init helper" #60761

Merged

bjarki-andreasen approved these changes Jul 25, 2023

View reviewed changes

fabiobaltieri removed the DNM This PR should not be merged (Do Not Merge) label Jul 25, 2023

fabiobaltieri merged commit f38e6aa into zephyrproject-rtos:main Jul 25, 2023

JordanYates deleted the 230716_device_sub_prio branch July 25, 2023 23:03

JordanYates mentioned this pull request Jul 25, 2023

pm: device: add driver init helper #60802

Merged

gmarull mentioned this pull request Jul 26, 2023

Device initialization order that respects devicetree dependencies #22545

Open

nashif added the Architecture Review Discussion in the Architecture WG required label Aug 5, 2023

This was referenced Aug 7, 2023

intc_eirq_nxp_s32 initialization order broken #61218

Closed

init: fix sub-priority for named init functions #61219

Closed

intc: nxp_s32: initialize after core intc #61244

Merged

maass-hamburg mentioned this pull request Nov 11, 2025

regulator: add ti tps55287 #98662

Open

init: add sub-priority to internal ordering #60410

init: add sub-priority to internal ordering #60410

Uh oh!

Conversation

JordanYates commented Jul 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fabiobaltieri left a comment

Choose a reason for hiding this comment

Uh oh!

fabiobaltieri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andyross left a comment

Choose a reason for hiding this comment

Uh oh!

JordanYates commented Jul 19, 2023

Uh oh!

gmarull commented Jul 20, 2023

Uh oh!

fabiobaltieri commented Jul 20, 2023

Uh oh!

fabiobaltieri commented Jul 20, 2023

Uh oh!

JordanYates commented Jul 20, 2023

Uh oh!

fabiobaltieri commented Jul 20, 2023

Uh oh!

gmarull commented Jul 20, 2023

Uh oh!

dkalowsk left a comment

Choose a reason for hiding this comment

Uh oh!

cfriedt commented Jul 22, 2023

Uh oh!

carlescufi commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

JordanYates commented Jul 16, 2023 •

edited

Loading

carlescufi commented Jul 25, 2023 •

edited

Loading