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.

Sign up for GitHub

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

Jump to bottom

init: per-core init hook #78496

Merged
merged 4 commits into from
Nov 16, 2024
Merged

init: per-core init hook #78496

merged 4 commits into from
Nov 16, 2024

Conversation

ycsin
Copy link
Member

@ycsin ycsin commented Sep 16, 2024 •
edited
Loading

Spinned-off from #65824, with an in-tree example of how it can be used.

npitre
npitre reviewed Sep 17, 2024
View reviewed changes
Copy link
Collaborator

@npitre npitre left a comment

Choose a reason for hiding this comment

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

I think calling z_riscv_pmp_init()) deep from SOC code is odd and wrong.
That ought to remain out of the SOC code's concern.
Similarly for IRQs.

@ycsin ycsin force-pushed the pr/soc-per-core-init branch 3 times, most recently from 7e95a68 to d38858b Compare September 17, 2024 06:46
@ycsin
Copy link
Member Author

ycsin commented Sep 17, 2024

I think calling z_riscv_pmp_init()) deep from SOC code is odd and wrong. That ought to remain out of the SOC code's concern. Similarly for IRQs.

Reverted the implementation to align with #65824, is it better now?

nashif
nashif reviewed Sep 18, 2024
View reviewed changes
arch/riscv/Kconfig Outdated
@@ -112,6 +112,12 @@ config RISCV_SOC_HAS_CUSTOM_SYS_IO
the RISC-V SoC needs to do something different and more than reading and
writing the registers.

config RISCV_SOC_HAS_CUSTOM_PER_CORE_INIT
Copy link
Member

Choose a reason for hiding this comment

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

this should not be riscv specific, the same pattern is use elsewhere, maybe such customization is used here first, but IMO this should be generalized to accomodate future usage by other arches, so I suggest doing this the same way we did it in include/zephyr/platform/hooks.h and use a similar pattern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nashif - addressed this in #65824 , on which this PR is based. Please review there, then this PR will pick that up

nashif
nashif reviewed Sep 18, 2024
View reviewed changes
arch/riscv/core/smp.c Outdated
@@ -28,6 +28,10 @@ extern void __start(void);
void soc_interrupt_init(void);
#endif

#ifdef CONFIG_RISCV_SOC_HAS_CUSTOM_PER_CORE_INIT
void z_soc_per_core_init(void);
Copy link
Member

Choose a reason for hiding this comment

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

lets not add more z_soc... code into the tree, just drop the z_ from the name, this can be confusing as it describes something internal to zephyr that should not be used outside of zephyr, what you are doing here is defining a new "interface", so it is an API somehow.

@ycsin ycsin force-pushed the pr/soc-per-core-init branch from d38858b to 7f3da46 Compare October 30, 2024 14:43
@ycsin ycsin changed the title arch: riscv: per-core init function init: per-core init hook Oct 30, 2024
@ycsin ycsin requested a review from jimmyzhe October 30, 2024 14:46
@ycsin ycsin marked this pull request as ready for review October 30, 2024 14:47
@ycsin ycsin requested review from npitre and nashif October 30, 2024 14:48
evgeniy-paltsev
evgeniy-paltsev previously approved these changes Oct 30, 2024
View reviewed changes
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.

Looks reasonable. ACK for ARC part.

ycsin and others added 2 commits November 1, 2024 14:13
@ycsin
soc: andestech: run pma_init_per_core() with soc_per_core_init_hook()
b0e9955 
The function `pma_init_per_core()`, as its name suggest, should be
run from every core, so call it from `soc_per_core_init_hook()`

Signed-off-by: Yong Cong Sin <[email protected]>
Signed-off-by: Yong Cong Sin <[email protected]>
@ycsin
soc: andestech: refactor out soc_early_init_hook() from pma.c
b819176 
Refactor out the `soc_early_init_hook()` function from `pma.c` to
`soc.c` which is always compiled so that it can be extended to run
other init functions easily in the future. Then, restore the function
in `pma.c` to `pma_init()`.

Signed-off-by: Yong Cong Sin <[email protected]>
@ycsin ycsin force-pushed the pr/soc-per-core-init branch from 7f3da46 to b819176 Compare November 1, 2024 06:14
@ycsin
Copy link
Member Author

ycsin commented Nov 1, 2024 •
edited
Loading

Thank you for applying per core init hook on ae350 PMA feature , but I got error when I tested it on adp_xc7k/ae350 with CONFIG_SOC_ANDES_V5_PMA=y.

/home/jimmyzhe/zephyrproject-upstream/zephyr/arch/riscv/include/kernel_arch_func.h: In function 'arch_kernel_init':
/home/jimmyzhe/zephyrproject-upstream/zephyr/arch/riscv/include/kernel_arch_func.h:57:9: error: implicit declaration of function 'soc_per_core_init_hook' [-Werror=implicit-function-declaration]
   57 |         soc_per_core_init_hook();
      |         ^~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

I think all file in 053112f need to include zephyr/platform/hooks.h.

Oops, good catch. I also refactored the soc_*_init_hook() functions from pma.c to soc.c

@ycsin ycsin mentioned this pull request Nov 11, 2024
Draft
@nashif nashif merged commit ad7f3a9 into zephyrproject-rtos:main Nov 16, 2024
35 checks passed
@ycsin ycsin deleted the pr/soc-per-core-init branch November 17, 2024 16:41
@ycsin ycsin mentioned this pull request Nov 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmyzhe jimmyzhe jimmyzhe approved these changes

@dcpleung dcpleung dcpleung approved these changes

@stephanosio stephanosio stephanosio approved these changes

@evgeniy-paltsev evgeniy-paltsev evgeniy-paltsev approved these changes

@ithinuel ithinuel ithinuel approved these changes

@luchnikov luchnikov luchnikov approved these changes

@cfriedt cfriedt cfriedt approved these changes

@npitre npitre npitre approved these changes

@carlocaione carlocaione Awaiting requested review from carlocaione

@fkokosinski fkokosinski Awaiting requested review from fkokosinski

@bbolen bbolen Awaiting requested review from bbolen

@galak galak Awaiting requested review from galak

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@microbuilder microbuilder Awaiting requested review from microbuilder

@nashif nashif Awaiting requested review from nashif

@ruuddw ruuddw Awaiting requested review from ruuddw

Assignees

@nashif nashif

@ithinuel ithinuel

@dcpleung dcpleung

Labels
area: Architectures
Projects
None yet
Milestone
v4.1.0
Development

Successfully merging this pull request may close these issues.