-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
soc: nordic: Add LRCCONF management #79067
base: main
Are you sure you want to change the base?
soc: nordic: Add LRCCONF management #79067
Conversation
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
/** | ||
* @brief Request or release lrcconf main power domain | ||
*/ | ||
void clock_request_lrcconf_poweron_main(struct clock_lrcconf_sink *sink); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove also the struct clock_lrcconf_sink
definition. There is no need to keep it as it can be replaced with just sys_snode_t
.
@@ -8,7 +8,7 @@ | |||
#include "clock_control_nrf2_common.h" | |||
#include <zephyr/devicetree.h> | |||
#include <zephyr/drivers/clock_control/nrf_clock_control.h> | |||
#include <hal/nrf_lrcconf.h> | |||
#include <soc/nordic/common/soc_lrcconf.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include <soc/nordic/common/soc_lrcconf.h> | |
#include <soc_lrcconf.h> |
soc/nordic/common/soc_lrcconf.c
Outdated
sys_slist_t poweron_main_list; | ||
sys_slist_t poweron_active_list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be static
.
soc/nordic/nrf54h/CMakeLists.txt
Outdated
@@ -3,6 +3,7 @@ | |||
|
|||
if(CONFIG_ARM) | |||
zephyr_library_sources(soc.c) | |||
zephyr_library_sources(../common/soc_lrcconf.c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it added only for ARM cores?
I think that instead of the above entry, the following should be added in soc/nordic/common/CMakeLists.txt
(so that nRF92 could also use it):
zephyr_library_sources_ifdef(NRF_PLATFORM_HALTIUM soc_lrcconf.c)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was correct to only have it for arm cores, or rather for local domains, as global domains don't have an NRF_LRCCONF010 instance and it breaks their builds.
soc/nordic/nrf54h/power.c
Outdated
#include "pm_s2ram.h" | ||
|
||
#if !IS_ENABLED(CONFIG_SOC_NRF54H20_CPURAD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#if !IS_ENABLED(CONFIG_SOC_NRF54H20_CPURAD) | |
#if !defined(CONFIG_SOC_NRF54H20_CPURAD) |
IS_ENABLED
should only be used in C expressions. See:
zephyr/include/zephyr/sys/util_macro.h
Lines 117 to 118 in 7e892f8
* Note: Use of IS_ENABLED in a <tt>\#if</tt> statement is discouraged | |
* as it doesn't provide any benefit vs plain <tt>\#if defined()</tt> |
soc/nordic/nrf54h/soc.c
Outdated
sys_snode_t *node; | ||
#if IS_ENABLED(CONFIG_PM) || IS_ENABLED(CONFIG_POWEROFF) | ||
node = pm_sys_node_id_get(); | ||
#else | ||
node = &soc_node; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a leftover from some previous approach. node
is not used by the code below.
soc/nordic/nrf54h/power.h
Outdated
/** | ||
* @brief Perform a power off. | ||
* | ||
* This function performs a power off of the core. | ||
*/ | ||
void nrf_poweroff(void); | ||
|
||
/** | ||
* @brief Pepare to the idle state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "Pepare"
soc/nordic/nrf54h/power.h
Outdated
/** | ||
* @brief Pepare to the idle state. | ||
* | ||
* This function applies required PM configuration must be performed before the idle state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence seems to need rephrasing.
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
46ac139
to
f7ea669
Compare
soc/nordic/nrf54h/power.c
Outdated
do_suspend_to_ram(); | ||
#endif | ||
} | ||
|
||
void pm_state_exit_post_ops(enum pm_state state, uint8_t substate_id) | ||
{ | ||
#if !defined(CONFIG_SOC_NRF54H20_CPURAD) | ||
if (state == PM_STATE_SUSPEND_TO_IDLE) { | ||
soc_lrcconf_poweron_release(&pm_node, NRF_LRCCONF_POWER_DOMAIN_0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only releases the poweron request when PM_STATE_SUSPEND_TO_IDLE
was used to call k_cpu_idle()
via the pm subsystem, but if the pm subsystem decided against suspending the system it will call k_cpu_idle()
here instead and then not call the release.
This can happen if the is no suspend-to-idle
state defined in devicetree or it has a minimum residency time that isn't met, e.g. a k_msleep(10)
for with this pm configuration https://github.com/nrfconnect/sdk-nrf/blob/84492d6d9264745050a018d01f4f333b285864c1/tests/benchmarks/multicore/idle/boards/nrf54h20dk_nrf54h20_cpuapp_s2ram.overlay#L12C39-L12C39
This is still way better than losing power, but needs to be followed up to have all paths covered eventually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a mechanism to supply an exit hook on arm cores
Line 56 in abd9008
config ARM_ON_EXIT_CPU_IDLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is this mechanism but it is quite ugly and requires special header with a macro. It was added as a workaround for PAN where code need to be executed immediately after WFI (so function cannot be used). I have #71667 which adds function hook symmetric to on_enter hook but PR stuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe your PR would have a clean usecase now and it could be argued for again to avoid that assembly mess?
soc/nordic/nrf54h/power.h
Outdated
* | ||
* @return The pointer to the node assigned to PM module. | ||
*/ | ||
sys_snode_t *pm_sys_node_id_get(void); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function does not seem to be needed anymore. It looks a bit like a hack actually.
soc/nordic/nrf54h/Kconfig
Outdated
@@ -28,6 +28,7 @@ config SOC_NRF54H20_CPUAPP | |||
select NRFS_HAS_VBUS_DETECTOR_SERVICE | |||
select HAS_PM | |||
select HAS_POWEROFF | |||
select ARM_ON_ENTER_CPU_IDLE_HOOK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be selected only when actually needed, so if PM || POWEROFF
?
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
static sys_slist_t poweron_main_list; | ||
static sys_slist_t poweron_active_list; | ||
|
||
void soc_lrcconf_poweron_request(sys_snode_t *node, nrf_lrcconf_power_domain_mask_t domain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason why simple reference counter is not used here? I only see that the difference is that you keep list of clients (but its not used anywhere) and allow to have non-symmetric requests (second request from the same client is not counted) but it can potentially cause some issues. Reference counter would be simpler and use less resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least right now there is non-symmetric requests so it couldn't be dropped
soc/nordic/nrf54h/power.c
Outdated
do_suspend_to_ram(); | ||
#endif | ||
} | ||
|
||
void pm_state_exit_post_ops(enum pm_state state, uint8_t substate_id) | ||
{ | ||
#if !defined(CONFIG_SOC_NRF54H20_CPURAD) | ||
if (state == PM_STATE_SUSPEND_TO_IDLE) { | ||
soc_lrcconf_poweron_release(&pm_node, NRF_LRCCONF_POWER_DOMAIN_0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is this mechanism but it is quite ugly and requires special header with a macro. It was added as a workaround for PAN where code need to be executed immediately after WFI (so function cannot be used). I have #71667 which adds function hook symmetric to on_enter hook but PR stuck.
f7ea669
to
c13c926
Compare
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
c13c926
to
f046e9a
Compare
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
f046e9a
to
981bb2b
Compare
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
@@ -64,7 +64,7 @@ static const struct clock_options { | |||
struct fll16m_dev_data { | |||
STRUCT_CLOCK_CONFIG(fll16m, ARRAY_SIZE(clock_options)) clk_cfg; | |||
struct onoff_client hfxo_cli; | |||
struct clock_lrcconf_sink lrcconf_sink; | |||
sys_snode_t lrcconf_client_node; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: fll16m_node
instead of lrcconf_client_node
might be a bit less generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good - fixed for hfxo
module as well
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
981bb2b
to
dd7848f
Compare
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
dd7848f
to
aec753f
Compare
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no> (cherry picked from commit 2c5c052)
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no> (cherry picked from commit ed62533)
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no> (cherry picked from commit 2c5c052)
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no> (cherry picked from commit ed62533)
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no> (cherry picked from commit 2c5c052)
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no> (cherry picked from commit ed62533)
} | ||
|
||
void nrf_poweroff(void) | ||
{ | ||
nrf_resetinfo_resetreas_local_set(NRF_RESETINFO, 0); | ||
nrf_resetinfo_restore_valid_set(NRF_RESETINFO, false); | ||
|
||
#if !defined(CONFIG_SOC_NRF54H20_CPURAD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update these to:
#if !defined(CONFIG_SOC_NRF54H20_CPURAD) | |
#if !defined(CONFIG_SOC_NRF54H20_CPURAD) && !defined(CONFIG_SOC_NRF54H20_ENGB_CPURAD) |
or:
#if !defined(CONFIG_SOC_NRF54H20_CPURAD) | |
#if !defined(NRF_RADIOCORE) |
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no> (cherry picked from commit 2c5c052)
Add `idle` and `s2ram` power states for nRF54H20 cpuapp and cpurad. Also the substate `idle_cache_disable` added. Upstream PR: zephyrproject-rtos/zephyr#79067 Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no> (cherry picked from commit ed62533)
Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required.