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

arch: call arch_smp_init() directly, do not use SYS_INIT #73908

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

nashif
Copy link
Member

@nashif nashif commented Jun 7, 2024

Move this to a call in the init process. arch_* calls are no services
and should be called consistently during initialization.

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

@zephyrbot zephyrbot added area: Kernel area: ARM64 ARM (64-bit) Architecture area: ARC ARC Architecture area: Xtensa Xtensa Architecture area: X86 x86 Architecture (32-bit) area: ARM ARM (32-bit) Architecture area: RISCV RISCV Architecture (32-bit & 64-bit) labels Jun 7, 2024
ithinuel
ithinuel previously approved these changes Jun 7, 2024
@@ -661,6 +661,9 @@ FUNC_NORETURN void z_cstart(void)
/* perform basic hardware initialization */
z_sys_init_run_level(INIT_LEVEL_PRE_KERNEL_1);
z_sys_init_run_level(INIT_LEVEL_PRE_KERNEL_2);
#if defined(CONFIG_SMP)
arch_smp_init();
Copy link
Collaborator

@evgeniy-paltsev evgeniy-paltsev Jun 7, 2024

Choose a reason for hiding this comment

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

Is there a reason for calling it in other place (earlier)? We call INIT_LEVEL_SMP here:

z_sys_init_run_level(INIT_LEVEL_SMP);

This breaks SMP on ARC as we need to call ARC arch_smp_init after we initialize secondary CPU cores (after z_smp_init is called)

Copy link
Contributor

Choose a reason for hiding this comment

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

Then maybe we want to move this into soc/arch layers and out of arch_interface.h entirely? Really there's zero kernel involvement to the function, it's purely there so the platform layers can do their setup, but when/how isn't really something the OS can (or wants to) decide on.

Copy link
Member Author

Choose a reason for hiding this comment

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

now on arc, arch_smp_init is called in PRE_KERNEL_1., so I am not following.

INIT_LEVEL_SMP has been something we wanted to get rid of, if I am not mistaken, we just have 1-2 users of that level.

Copy link
Collaborator

Choose a reason for hiding this comment

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

SMP level and actually initializing smp is indeed completely different.

andyross
andyross previously approved these changes Jun 7, 2024
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Agreed, this is sort of a cargo-cult thing; not sure where the pattern came from originally.

tbursztyka
tbursztyka previously approved these changes Jun 7, 2024
edersondisouza
edersondisouza previously approved these changes Jun 7, 2024
ithinuel
ithinuel previously approved these changes Jun 9, 2024
@nashif
Copy link
Member Author

nashif commented Jun 11, 2024

@evgeniy-paltsev can you please take another look?

@nashif nashif requested a review from evgeniy-paltsev June 11, 2024 12:05
@evgeniy-paltsev
Copy link
Collaborator

Ok, initially I was thinking about

SYS_INIT(arc_shared_intc_update_post_smp, SMP, 0);

as it's only SYS_INIT with SMP level for ARC, but we indeed changing the different part, so my initial comment is not relevant.

However, I've tested this on ARC SMP platforms - and I've got lot's of test failures (~120 failed test per platform). I need to investigate this.

@evgeniy-paltsev evgeniy-paltsev added the DNM This PR should not be merged (Do Not Merge) label Jun 11, 2024
@nashif
Copy link
Member Author

nashif commented Jun 11, 2024

However, I've tested this on ARC SMP platforms - and I've got lot's of test failures (~120 failed test per platform). I need to investigate this.

can you try moving:

#if defined(CONFIG_SMP)
        arch_smp_init();
#endif

to be between PRE_KERNEL_1 and PRE_KERNEL_2? I think it is related to the timer initialization.

@nashif nashif removed the DNM This PR should not be merged (Do Not Merge) label Jun 11, 2024
@nashif
Copy link
Member Author

nashif commented Jun 11, 2024

No need for a DNM, it is already blocked by your review.

Move this to a call in the init process. arch_* calls are no services
and should be called consistently during initialization.

Place it between PRE_KERNEL_1 and PRE_KERNEL_2 as some drivers
initialized in PRE_KERNEL_2 might depend on SMP being setup.

Signed-off-by: Anas Nashif <anas.nashif@intel.com>
@nashif nashif force-pushed the topic/arch/smp_init branch from 06d2e79 to fa25184 Compare June 12, 2024 13:48
Copy link
Collaborator

@evgeniy-paltsev evgeniy-paltsev left a comment

Choose a reason for hiding this comment

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

Now it works.

In the previous implementation there was a conflict of initialization between arch_smp_init and a timer driver for ARC.

@nashif nashif added this to the v3.7.0 milestone Jun 12, 2024
@nashif nashif merged commit c20e798 into zephyrproject-rtos:main Jun 12, 2024
30 checks passed
@nashif nashif deleted the topic/arch/smp_init branch June 12, 2024 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARC ARC Architecture area: ARM ARM (32-bit) Architecture area: ARM64 ARM (64-bit) Architecture area: Kernel area: RISCV RISCV Architecture (32-bit & 64-bit) area: X86 x86 Architecture (32-bit) area: Xtensa Xtensa Architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants