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

timer: start directly in init.c #74228

Closed

Conversation

nashif
Copy link
Member

@nashif nashif commented Jun 13, 2024

Do not use SYS_INIT, directly call init_sys_clock_driver() during the
init process.

Signed-off-by: Anas Nashif anas.nashif@intel.com

Comment on lines 163 to 171

/**
* @brief Initialize system clock driver
* @note This function is called by the kernel during system initialization.
* It should not be called by application code.
* @return 0 on success, negative errno code on fail
*/
int init_sys_clock_driver(void);
Copy link
Member

Choose a reason for hiding this comment

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

this prototype should likely not be exposed in the docs, otherwise it may seem like a public API (even if stated it's not like that).

Ideally we should have an internal level of headers, where APIs to be consumed by Zephyr could be exposed to the Kernel/arch/soc code, but not to the application.

Copy link
Member Author

Choose a reason for hiding this comment

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

fully agree.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, should we prefix these with z_? To avoid any init_ name collisions and to make it clearer that these aren't supposed to be called by the application.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I am working my way through all of this and already thinking about a unified prefix for all those initialization calls. we now have a mix of z_ , init_, ..._init() etc.
This PR is just trying to show the potential of how we could fix mis-use of SYS_INIT and reduce the boot dependency problem to something manageable.

@jukkar
Copy link
Member

jukkar commented Jun 13, 2024

The commit message fails to say why this needs to be changed, it would be nice to mention that.

@nashif nashif force-pushed the topic/init/system_timer branch from 6e7503b to a2b9988 Compare June 13, 2024 11:21
@nashif
Copy link
Member Author

nashif commented Jun 13, 2024

The commit message fails to say why this needs to be changed, it would be nice to mention that.

done :)

Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

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

CONFIG_SYSTEM_CLOCK_INIT_PRIORITY Can be removed as well from drivers/timers/Kconfig

@@ -666,6 +667,7 @@ FUNC_NORETURN void z_cstart(void)
arch_smp_init();
#endif
z_sys_init_run_level(INIT_LEVEL_PRE_KERNEL_2);
init_sys_clock_driver();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps invert these 2 lines?
clock priority is 0, so started as one of the very first of PRE_KERNEL_2

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, we should not be far away from getting rid of PRE_KERNEL_2 anyways :)

Do not use SYS_INIT, directly call init_sys_clock_driver() during the
init process.

The system timer driver is not a traditional driver and its dependencies
are fixed. It is needed at a fixes location during the boot process and
is not something you want to change or tamper with. Any change to where
the timer is started might have negative consquences and non-functioning
system. Thus, call this "driver" and initialize it at a fixed location
and avoid the SYS_INIT use.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif nashif force-pushed the topic/init/system_timer branch from a2b9988 to 981f6c7 Compare July 15, 2024 19:51
Copy link

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

@github-actions github-actions bot added the Stale label Sep 14, 2024
@nashif nashif removed the Stale label Sep 14, 2024
Copy link

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

@github-actions github-actions bot added the Stale label Nov 14, 2024
@github-actions github-actions bot closed this Nov 28, 2024
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.

5 participants