soc: nordic: Add LRCCONF management #79067

adamkondraciuk · 2024-09-26T15:28:17Z

Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required.

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>

anangl · 2024-09-27T07:08:14Z

drivers/clock_control/clock_control_nrf2_common.h

-/**
- * @brief Request or release lrcconf main power domain
- */
-void clock_request_lrcconf_poweron_main(struct clock_lrcconf_sink *sink);


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.

anangl · 2024-09-27T07:08:30Z

drivers/clock_control/clock_control_nrf2_fll16m.c

@@ -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>


Suggested change

#include <soc/nordic/common/soc_lrcconf.h>

#include <soc_lrcconf.h>

anangl · 2024-09-27T07:09:10Z

soc/nordic/common/soc_lrcconf.c

+sys_slist_t poweron_main_list;
+sys_slist_t poweron_active_list;


These should be static.

anangl · 2024-09-27T07:20:05Z

soc/nordic/nrf54h/CMakeLists.txt

@@ -3,6 +3,7 @@

 if(CONFIG_ARM)
 zephyr_library_sources(soc.c)
+ zephyr_library_sources(../common/soc_lrcconf.c)


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)

anangl · 2024-09-27T07:30:35Z

soc/nordic/nrf54h/power.c

 #include "pm_s2ram.h"

+#if !IS_ENABLED(CONFIG_SOC_NRF54H20_CPURAD)


Suggested change

#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>

anangl · 2024-09-27T07:37:03Z

soc/nordic/nrf54h/soc.c

+ sys_snode_t *node;
+#if IS_ENABLED(CONFIG_PM) || IS_ENABLED(CONFIG_POWEROFF)
+ node = pm_sys_node_id_get();
+#else
+ node = &soc_node;
+#endif


This looks like a leftover from some previous approach. node is not used by the code below.

anangl · 2024-09-27T07:38:15Z

soc/nordic/nrf54h/power.h

 /**
 * @brief Perform a power off.
 *
 * This function performs a power off of the core.
 */
 void nrf_poweroff(void);

+/**
+ * @brief Pepare to the idle state.


typo: "Pepare"

anangl · 2024-09-27T07:39:09Z

soc/nordic/nrf54h/power.h

+/**
+ * @brief Pepare to the idle state.
+ *
+ * This function applies required PM configuration must be performed before the idle state.


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>

karstenkoenig · 2024-09-27T10:31:28Z

soc/nordic/nrf54h/power.c

 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);


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 is a mechanism to supply an exit hook on arm cores

zephyr/arch/arm/Kconfig

Line 56 in abd9008

config ARM_ON_EXIT_CPU_IDLE

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.

Maybe your PR would have a clean usecase now and it could be argued for again to avoid that assembly mess?

anangl · 2024-09-27T10:20:31Z

soc/nordic/nrf54h/power.h

+ *
+ * @return The pointer to the node assigned to PM module.
+ */
+sys_snode_t *pm_sys_node_id_get(void);


This function does not seem to be needed anymore. It looks a bit like a hack actually.

anangl · 2024-09-27T10:30:02Z

soc/nordic/nrf54h/Kconfig

@@ -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


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>

nordic-krch · 2024-09-27T11:30:19Z

soc/nordic/common/soc_lrcconf.c

+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)


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.

At least right now there is non-symmetric requests so it couldn't be dropped

nordic-krch · 2024-09-27T11:37:57Z

soc/nordic/nrf54h/power.c

 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 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.

Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. 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>

adamkondraciuk requested a review from nordic-krch as a code owner September 26, 2024 15:28

zephyrbot added platform: nRF Nordic nRFx area: Clock Control labels Sep 26, 2024

zephyrbot requested review from anangl, jaz1-nordic, kl-cruz, magp-nordic, masz-nordic and nika-nordic September 26, 2024 15:29

zephyrbot assigned nordic-krch Sep 26, 2024

anangl reviewed Sep 27, 2024

View reviewed changes

adamkondraciuk force-pushed the idle_lrcconf_config branch from 46ac139 to f7ea669 Compare September 27, 2024 10:17

karstenkoenig reviewed Sep 27, 2024

View reviewed changes

anangl reviewed Sep 27, 2024

View reviewed changes

nordic-krch reviewed Sep 27, 2024

View reviewed changes

soc: nordic: Add LRCCONF management

c13c926

Due to the possibility of simultaneous accesess to LRCCONF registers, additional management is required. Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

adamkondraciuk force-pushed the idle_lrcconf_config branch from f7ea669 to c13c926 Compare September 27, 2024 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

soc: nordic: Add LRCCONF management #79067

soc: nordic: Add LRCCONF management #79067

adamkondraciuk commented Sep 26, 2024

anangl Sep 27, 2024

anangl Sep 27, 2024

anangl Sep 27, 2024

anangl Sep 27, 2024

anangl Sep 27, 2024

anangl Sep 27, 2024

anangl Sep 27, 2024

anangl Sep 27, 2024

karstenkoenig Sep 27, 2024

karstenkoenig Sep 27, 2024

nordic-krch Sep 27, 2024

karstenkoenig Sep 27, 2024

anangl Sep 27, 2024

anangl Sep 27, 2024

nordic-krch Sep 27, 2024

karstenkoenig Sep 27, 2024

nordic-krch Sep 27, 2024

	#include <soc/nordic/common/soc_lrcconf.h>
	#include <soc_lrcconf.h>

		sys_slist_t poweron_main_list;
		sys_slist_t poweron_active_list;

		#include "pm_s2ram.h"

		#if !IS_ENABLED(CONFIG_SOC_NRF54H20_CPURAD)

	#if !IS_ENABLED(CONFIG_SOC_NRF54H20_CPURAD)
	#if !defined(CONFIG_SOC_NRF54H20_CPURAD)

	* 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: Add LRCCONF management #79067

Are you sure you want to change the base?

soc: nordic: Add LRCCONF management #79067

Conversation

adamkondraciuk commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment