Introduce pm config #1666

adamkondraciuk · 2024-04-24T16:53:10Z

No description provided.

hubertmis · 2024-04-25T08:19:25Z

include/zephyr/arch/common/pm_s2ram.h

+ * @retval true if marking is found which indicates resuming after suspend-to-RAM.
+ * @retval false if marking is not found which indicates standard boot.
+ */
+bool pm_s2ram_mark_check_and_clear(void);


The function documentation should describe if this function is called before or after RAMs are initialized. I would indicate if it is safe to use local variables inside.

hubertmis · 2024-04-25T08:20:31Z

subsys/pm/Kconfig

+	depends on PM_S2RAM
+	help
+	  By default a magic word in RAM is used to mark entering suspend-to-RAM. Enabling
+	  this option allows custom implementation of functions which handles the marking.


of functions which handle the marking.

hubertmis · 2024-04-25T08:25:01Z

dts/bindings/mmu_mpu/arm,armv6m-mpu.yaml

@@ -10,3 +10,8 @@ include: base.yaml
 properties:
  reg:
    required: true
+
+  arm,num-mpu-regions:


It seems we should revert zephyrproject-rtos/zephyr#60780 upstream instead of creating a noup commit. @gmarull, please have a look

what we should avoid is to disalign with upstream, all this code should be developed upstream-first.

hubertmis · 2024-04-25T08:36:07Z

soc/arm/nordic_nrf/common/poweroff.c

@@ -6,6 +6,7 @@
 #include <zephyr/sys/poweroff.h>
 #include <zephyr/toolchain.h>

+#if !defined(CONFIG_SOC_SERIES_NRF54HX)
 #if defined(CONFIG_SOC_SERIES_NRF51X) || defined(CONFIG_SOC_SERIES_NRF52X)
 #include <hal/nrf_power.h>
 #else


I guess this should be #elif defined (CONFIG_SOC_SERIES_NRF53...) instead of #if !defined(NRF54H) above.

hubertmis · 2024-04-25T08:37:06Z

soc/arm/nordic_nrf/nrf54h/poweroff.c

I don't like the path, because this will apply to other SoCs from the family. Not only nRF54H

hubertmis · 2024-04-25T08:47:57Z

soc/arm/nordic_nrf/nrf54h/poweroff.c

+
+void pm_s2ram_mark_set(void)
+{
+	/* empty */


I would add a comment that this mark is set by nrf_resetinfo_restore_valid_set(NRF_RESETINFO, true); in z_pm_sys_suspend()

hubertmis · 2024-04-25T08:51:10Z

soc/arm/nordic_nrf/nrf54h/poweroff.c

+	bool unretained_wake;
+	bool restore_valid;
+
+	unretained_wake = nrf_resetinfo_resetreas_local_get(NRF_RESETINFO) &


You should check other reset reasons. If other reset reasons than unretained are set, this function must return false. (until suspend-to-ram ready fo systemoff is implemented - wake up from this sleep state will require more complex condition).

hubertmis · 2024-04-25T08:59:05Z

soc/arm/nordic_nrf/nrf54h/poweroff.c

+	 */
+
+	if (ret == 0) {
+		z_pm_sys_resume();


z_pm_sys_resume must be executed even if pm_s2ram_suspend() returns an error. E.g. if there was a pending IRQ and WFI was NOP-ed and pm_s2ram_suspend() returns -EBUSY, the caches must be reinitialized afterwards with z_pm_sys_resume();

hubertmis · 2024-04-25T09:01:22Z

soc/arm/nordic_nrf/nrf54h/poweroff.c

+}
+
+/* Function called during local domain suspend to RAM. */
+int z_pm_sys_suspend(void)


I would rename this and similar functions to clearly indicate they are about suspend_to_ram not about suspend to idle or suspend to disk.

hubertmis · 2024-04-25T09:04:05Z

arch/arm/core/cortex_m/pm_s2ram.c

This looks like a generic Zepyr code. I would consider upstreaming. @gmarull , FYI

jonathannilsen · 2024-04-29T11:30:17Z

soc/arm/nordic_nrf/nrf54h/poweroff.c

+#if defined(NRF_APPLICATION)
+#define RAMBLOCK_CONTROL_BIT_ICACHE 1
+#define RAMBLOCK_CONTROL_BIT_DCACHE 2
+#define RAMBLOCK_POWER_ID	    0
+#define RAMBLOCK_CONTROL_OFF	    0
+#elif defined(NRF_RADIOCORE)
+#define RAMBLOCK_CONTROL_BIT_ICACHE 2
+#define RAMBLOCK_CONTROL_BIT_DCACHE 3
+#define RAMBLOCK_POWER_ID	    0
+#define RAMBLOCK_CONTROL_OFF	    0
+#else
+#error "Unsupported domain."
+#endif


Is there any way we could make it possible to support other cores than app or radio instead of triggering a compile time error? I.e. making these values configurable outside this C file.

One idea: how about wrapping this with #ifndef RAMBLOCK_CONTROL_BIT_ICACHE and pass the values through a build system

hubertmis · 2024-04-29T15:29:46Z

soc/arm/nordic_nrf/nrf54h/soc.c

 	nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_MAIN, true);
+#if defined(CONFIG_HW_REVISION_SOC1)
 	nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_DOMAIN_0, true);
+#endif

 	nrf_lrcconf_retain_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_MAIN, true);
 	nrf_lrcconf_retain_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_DOMAIN_0, true);


This code should be moved to z_arm_on_enter_cpu_idle_prepare() and accompanied by DCACHE handling (for now write-back of caches should be enough).
Also, a z_arm_on_exit_cpu_idle() function should be added that would be responsible for restoring LRCCONF configuration to the reset values.

For caches write-back in z_arm_on_enter_cpu_idle_prepare() to be bullet-proof, the function must be called after

sdk-zephyr/arch/arm/core/cortex_m/cpu_idle.S

Line 113 in 99187c8

cpsid i

Remove CONFIG_NRF_ENABLE_ICACHE as it is not needed. There is CONFIG_ICACHE which is by default enabled for nrf54h. Signed-off-by: Krzysztof Chruściński <krzysztof.chruscinski@nordicsemi.no> (cherry picked from commit cfa6e25)

Add initialization of the data cache. Signed-off-by: Krzysztof Chruściński <krzysztof.chruscinski@nordicsemi.no> (cherry picked from commit d82b27b)

Add nrf_cache_lineaddr_get to nordic HAL. Signed-off-by: Krzysztof Chruściński <krzysztof.chruscinski@nordicsemi.no> (cherry picked from commit 29a4bb6)

nrf54h20 has a bug that requires to manually set 28th bit in the line address. 28th bit indicates secure memory space. Add handling to the cache driver. Signed-off-by: Krzysztof Chruściński <krzysztof.chruscinski@nordicsemi.no> (cherry picked from commit 73d4f58)

Driver was disabling cache before full range operations and that was causing hanging on polling for cache busy status since state was never changing (because cache was disabled). Additionally, added flushing to data cache disabling. If flushing is not performed then execution fails since data in cache is lost. Signed-off-by: Krzysztof Chruściński <krzysztof.chruscinski@nordicsemi.no> (cherry picked from commit b83a112)

Apply changes which significantly speed up cache operations. Removed k_busy_wait from function which polls for cache busyness. Removed spinlock from cache operation. Lock taking and releasing takes time and it is faster to check if LINEADDR changed after performing the operation. If LINEADDR changed then it indicates that current context was preempted by another cache operation. If such state is detected current operation is repeated. Signed-off-by: Krzysztof Chruściński <krzysztof.chruscinski@nordicsemi.no> (cherry picked from commit 9f6567b)

Use Kconfig instead of fixed value in the driver code. Signed-off-by: Krzysztof Chruściński <krzysztof.chruscinski@nordicsemi.no> (cherry picked from commit 4e880e6)

s2ram procedure used RAM magic word for marking suspend-to-RAM. This method may not work in some cases, e.g. when global reset does not reset RAM content. In that case resuming from s2ram is detected when global reset occurred. RAM magic word method is the default but with CONFIG_PM_S2RAM_CUSTOM_MARKING a custom implementation can be provided. Upstream PR: zephyrproject-rtos/zephyr#68860 Signed-off-by: Krzysztof Chruściński <krzysztof.chruscinski@nordicsemi.no> Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

Add mpu node to cpu in haltium ARM cores. Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

Add functions for local domain suspend to RAM. Add matching resume procedure. Add pm_s2ram function for determining source of reset. Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

Add preserving NVIC and MPU state in retained RAM when CPU is powered off during S2RAM procedure. Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

Currently function `z_nrf_grtc_wakeup_prepare()` should be available only for the entity which is starting the peripheral. Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

hubertmis

I marked multiple remarks that require attention. However, they can be fixed in the future PRs. For now, this PR should allow moving forward in the task force.

hubertmis · 2024-05-09T14:27:20Z

soc/nordic/common/poweroff.c

@@ -6,6 +6,7 @@
 #include <zephyr/sys/poweroff.h>
 #include <zephyr/toolchain.h>

+#if !defined(CONFIG_SOC_SERIES_NRF54HX)


This #if shows that this file is not "common" for Nordic SoCs, so it should be moved to another directory than soc/nordic/common.

hubertmis · 2024-05-09T14:29:05Z

soc/nordic/nrf54h/poweroff.c

+		/* Flush, disable and power down DCACHE */
+		sys_cache_data_flush_all();
+		sys_cache_data_disable();
+		nrf_memconf_ramblock_control_enable_set(NRF_MEMCONF, RAMBLOCK_POWER_ID,


A little updated procedure is described in this PR: https://github.com/nrfconnect/doc-internal/pull/93

hubertmis · 2024-05-09T14:31:46Z

soc/nordic/nrf54h/poweroff.c

+	nrf_lrcconf_retain_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_DOMAIN_0, false);
+	nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_DOMAIN_0, false);
+
+	k_cpu_idle();


k_cpu_idle() should not be called from s2ram functions. k_cpu_idle() should contain some boilerplate around caches to safely suspend the CPU.

hubertmis · 2024-05-09T14:33:25Z

soc/nordic/nrf54h/poweroff.c

+	nrf_lrcconf_retain_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_MAIN, false);
+	nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_MAIN, false);
+
+	nrf_lrcconf_task_trigger(NRF_LRCCONF010, NRF_LRCCONF_TASK_SYSTEMOFFREADY);


This task must be triggered after suspend_common() because of the timing (there is a timeout in System Controller).
After suspend_common() this function must call WFI. The code after WFI is reachable in some scenarios, so CODE_UNREACHABLE; below is invalid.

hubertmis · 2024-05-09T14:34:41Z

soc/nordic/nrf54h/poweroff.c

+	nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_MAIN, true);
+	nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_DOMAIN_0, true);


The domains shall not be forced on here. I guess this is a workaround for the incomplete k_cpu_idle() implementation for nRF54H. I would at least add a TODO comment here, because when k_cpu_idle() will be implemented, we'll encounter surprises like unexpected forcing power domains ON.

hubertmis · 2024-05-09T14:38:15Z

soc/nordic/nrf54h/poweroff.c

+	bool unretained_wake;
+	bool restore_valid;
+
+	unretained_wake = nrf_resetinfo_resetreas_local_get(NRF_RESETINFO) &


This code shall check if any other reset reasons are set in the RESETINFO. If they are, the function shall return false.

hubertmis · 2024-05-09T14:40:39Z

soc/nordic/nrf54h/soc.c

+	nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_MAIN,
+			!IS_ENABLED(CONFIG_HALTIUM_CPURAD));
+	nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_DOMAIN_0, false);


This code shall be moved around k_cpu_idle() implementation.

hubertmis · 2024-05-17T14:31:48Z

soc/nordic/nrf54h/poweroff.c

+	 * Save the CPU context (including the return address),set the SRAM
+	 * marker and power off the system.
+	 */
+	(void)pm_s2ram_suspend(z_pm_sys_suspend);


It seems this function should be called with IRQs disabled (wrapped with __disable_irq() - __enable_irq()).

If IRQs are enabled, we risk a race condition: after the NVIC or MPU state is saved in RAM, ZLI is triggered, resulting in a change in NVIC or MPU. When ZLI ends, this function resumes execution storing CPU registers, configuring LRCCONF, etc. When the CPU wakes up and restores NVIC and MPU, it restores incorrect values, saved before ZLI was triggered.

If IRQs are disabled, ZLI wouldn't be handled until we exit this function, WFI would result in NOP, so the system state would be coherent.

Or maybe disabling and enabling IRQs should be inside the implementation of this function? Anyway, it should be clearly documented in API docs.

adamkondraciuk requested review from kl-cruz, hubertmis, jonathannilsen, ppryga-nordic, meijemac and nordic-piks April 24, 2024 16:53

NordicBuilder added platform: nRF area: Power Management area: Devicetree area: Devicetree Binding area: Timer area: ARM area: Architectures labels Apr 24, 2024

NordicBuilder requested review from anangl and stephanosio April 24, 2024 16:54

adamkondraciuk force-pushed the introduce_pm_config branch 2 times, most recently from dc6053e to 9c947e2 Compare April 25, 2024 08:51

hubertmis requested changes Apr 25, 2024

View reviewed changes

NordicBuilder mentioned this pull request Apr 26, 2024

samples: multicore: hello_world: Align sample configuration nrfconnect/sdk-nrf#15066

Closed

jonathannilsen reviewed Apr 29, 2024

View reviewed changes

hubertmis reviewed Apr 29, 2024

View reviewed changes

adamkondraciuk force-pushed the introduce_pm_config branch from 9c947e2 to 933192d Compare May 9, 2024 11:18

adamkondraciuk requested review from koffes, alexsven, erikrobstad, rick1082, gWacey, ioannisg, carlescufi and lemrey as code owners May 9, 2024 11:18

NordicBuilder added manifest-nrf_hw_models manifest-babblesim_ext_2G4_libPhyComv1 labels May 9, 2024

nordic-krch and others added 12 commits May 9, 2024 15:29

[nrf fromtree] soc: nordic: nrf54h: Add DCACHE initialization

c815261

Add initialization of the data cache. Signed-off-by: Krzysztof Chruściński <krzysztof.chruscinski@nordicsemi.no> (cherry picked from commit d82b27b)

[nrf fromtree] manifest: Update hal_nordic with nrf_cache change

3114e4d

Add nrf_cache_lineaddr_get to nordic HAL. Signed-off-by: Krzysztof Chruściński <krzysztof.chruscinski@nordicsemi.no> (cherry picked from commit 29a4bb6)

[nrf fromtree] drivers: cache: nrf: Use CONFIG_DCACHE_LINE_SIZE

cbcc780

Use Kconfig instead of fixed value in the driver code. Signed-off-by: Krzysztof Chruściński <krzysztof.chruscinski@nordicsemi.no> (cherry picked from commit 4e880e6)

[nrf noup] dts: arm: nrf54h20: Add mpu node to cpuapp and cpurad

2d40098

Add mpu node to cpu in haltium ARM cores. Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

[nrf noup] soc: arm: nrf54h: poweroff: Add support for s2ram

aeff82d

Add functions for local domain suspend to RAM. Add matching resume procedure. Add pm_s2ram function for determining source of reset. Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

[nrf noup] arch: arm: cortex_m: Extend S2RAM support

fc514cd

Add preserving NVIC and MPU state in retained RAM when CPU is powered off during S2RAM procedure. Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

[nrf noup] drivers: timer: nrf_grtc: Add GRTC fix for app and rad

af63f91

Currently function `z_nrf_grtc_wakeup_prepare()` should be available only for the entity which is starting the peripheral. Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

adamkondraciuk force-pushed the introduce_pm_config branch from 933192d to af63f91 Compare May 9, 2024 13:42

NordicBuilder added the area: Cache label May 9, 2024

adamkondraciuk removed request for a team, koffes, lemrey, ioannisg, carlescufi, anangl, rick1082, alexsven, erikrobstad, gWacey and stephanosio May 9, 2024 13:46

hubertmis approved these changes May 9, 2024

View reviewed changes

hubertmis reviewed May 17, 2024

View reviewed changes

adamkondraciuk closed this Jul 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce pm config #1666

Introduce pm config #1666

adamkondraciuk commented Apr 24, 2024

hubertmis Apr 25, 2024

hubertmis Apr 25, 2024

hubertmis Apr 25, 2024

gmarull Apr 26, 2024

hubertmis Apr 25, 2024

hubertmis Apr 25, 2024

hubertmis Apr 25, 2024

hubertmis Apr 25, 2024

hubertmis Apr 25, 2024

hubertmis Apr 25, 2024

hubertmis Apr 25, 2024

jonathannilsen Apr 29, 2024

hubertmis Apr 29, 2024

hubertmis Apr 29, 2024

hubertmis Apr 29, 2024

hubertmis left a comment

hubertmis May 9, 2024

hubertmis May 9, 2024

hubertmis May 9, 2024

hubertmis May 9, 2024

hubertmis May 9, 2024

hubertmis May 9, 2024

hubertmis May 9, 2024

hubertmis May 17, 2024 •

edited

Loading

		nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_MAIN, true);
		nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_DOMAIN_0, true);

Introduce pm config #1666

Introduce pm config #1666

Conversation

adamkondraciuk commented Apr 24, 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

hubertmis left a 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

hubertmis May 17, 2024 • edited Loading

Choose a reason for hiding this comment

hubertmis May 17, 2024 •

edited

Loading