[nrf toup] getting boot reason on nRF54h20 #513

kg-nordicsemi · 2024-11-25T14:50:31Z

Use NRF_RESETINFO register to get boot reason

ArekBalysNordic · 2024-11-26T12:35:20Z

src/platform/nrfconnect/Reboot.cpp

@@ -28,19 +28,7 @@
 namespace chip {
 namespace DeviceLayer {

-#if defined(CONFIG_ARCH_POSIX) || defined(CONFIG_SOC_SERIES_NRF54HX)


Why is this change needed?

Because in nrf54h20 case we don't use Reboot function right? we use dfu_target_suit_reboot. Reset reason is saved in RESETINFO register, so we also don't need to use GetSoftwareRebootReason(), so I thought it is unnecessary to define these functions in case of using nrf54h20.

In case of nRF54H20 that's true, but what about POSIX? Will this change work in Matter upstream repo? You've assigned it as [nrf toup].

ArekBalysNordic · 2024-11-26T12:39:30Z

src/platform/Zephyr/DiagnosticDataProviderImpl.cpp

+        return BootReasonType::kBrownOutReset;
+    }
+
+    if (reason & RESETINFO_RESETREAS_GLOBAL_DOG_Msk)


What about changing ifs to else ifs?

I know this is subjective, but this is actually opposite to what clang-tidy considers readability improvement: https://clang.llvm.org/extra/clang-tidy/checks/readability/else-after-return.html :)

LuDuda · 2024-11-26T13:09:44Z

src/platform/Zephyr/DiagnosticDataProviderImpl.cpp

@@ -60,6 +64,32 @@ namespace {
 BootReasonType DetermineBootReason()
 {
 #ifdef CONFIG_HWINFO
+
+#ifdef CONFIG_SOC_SERIES_NRF54HX
+    uint32_t reason = nrf_resetinfo_resetreas_global_get(NRF_RESETINFO);


Any chance to stop using nrf_ specific API, and instead use HW_INFO module or similar? Probably new nrfx which will be part of NCS 2.9.0 introduces support for nRF54H20 and reset reason.

I tried using hwinfo_get_reset_cause, but it doesn't work with nrf54h20 for now

Do we know when or how it is planned to be supported?

Damian-Nordic · 2024-11-27T08:26:41Z

src/platform/Zephyr/DiagnosticDataProviderImpl.cpp

@@ -60,6 +64,32 @@ namespace {
 BootReasonType DetermineBootReason()
 {
 #ifdef CONFIG_HWINFO
+
+#ifdef CONFIG_SOC_SERIES_NRF54HX


Looks like this code doesn't depend on HWINFO, so why is it inside #ifdef CONFIG_HWINFO?

Damian-Nordic · 2024-11-27T08:28:09Z

src/platform/Zephyr/DiagnosticDataProviderImpl.cpp

+        (RESETINFO_RESETREAS_GLOBAL_RESETPOR_Msk | RESETINFO_RESETREAS_GLOBAL_SECSREQ_Msk))
+    {
+        return BootReasonType::kSoftwareUpdateCompleted;
+    }


Don't you need to clear the reset cause somewhere so that it is not accumulated in subsequent reboots?

I didn't see any api to do this, but I tested manually several reboot reasons, and it seems that everything works as expected

LuDuda · 2024-11-28T13:43:39Z

src/platform/Zephyr/DiagnosticDataProviderImpl.cpp

+    if ((reason & (RESETINFO_RESETREAS_GLOBAL_RESETPOR_Msk | RESETINFO_RESETREAS_GLOBAL_SECSREQ_Msk)) ==
+        (RESETINFO_RESETREAS_GLOBAL_RESETPOR_Msk | RESETINFO_RESETREAS_GLOBAL_SECSREQ_Msk))
+    {
+        return BootReasonType::kSoftwareUpdateCompleted;


Hmm, how do we know it was caused by the SoftwareUpdate, and not for example crash, or reset from the application layer?

I performed several manual tests, and checked what are the values returned from RESETINFO in different cases, for example:

reset after DFU over Matter -> returned value: 528

reset using button on board -> returned value:514

reset from the application -> returned value: 0

reset from disabling power supply -> returned value: 1

Events has unique values, and thanks to it, it can be parsed to actual boot reason
But, I don't know what is returning when board is crashing

Right, but I RESETINFO_RESETREAS_GLOBAL_RESETPOR_Msk | RESETINFO_RESETREAS_GLOBAL_SECSREQ_Msk does not seem to mean that Firmware Upgrade has been done - and the event to Matter core might be generate incorrectly.

We need to either find out if suit module might indicate it, or find some register/retention RAM to store information about reset being scheduled due to image update, not for other reason.

@SylwesterKonczyk @tomchy maybe might help if we have anything like this in SUIT, else we need to look at different way.

LuDuda · 2024-12-03T10:08:58Z

src/platform/Zephyr/DiagnosticDataProviderImpl.cpp

+#endif
+
+#ifdef CONFIG_SOC_SERIES_NRF54HX
+    reason = nrf_resetinfo_resetreas_global_get(NRF_RESETINFO);


I would like to not use this API as it is HW specific.

What about to wait for this PR: nrfconnect/sdk-zephyr#2293 as dependency. And use the API of HW_INFO instead?

ArekBalysNordic · 2024-12-10T09:32:45Z

src/platform/SaveBootReasonDFUSuit.h

+#ifndef NRF_SAVE_BOOT_REASON_H__
+#define NRF_SAVE_BOOT_REASON_H__
+
+extern int BootReason;


Why did you do it in this way?

ArekBalysNordic · 2024-12-10T09:32:58Z

src/platform/nrfconnect/OTAImageProcessorImpl.cpp

@@ -182,6 +185,8 @@ CHIP_ERROR OTAImageProcessorImpl::Apply()
                PlatformMgr().HandleServerShuttingDown();
                k_msleep(CHIP_DEVICE_CONFIG_SERVER_SHUTDOWN_ACTIONS_SLEEP_MS);
 #ifdef CONFIG_DFU_TARGET_SUIT
+                BootReason = 5;
+                ChipLogDetail(DeviceLayer, "Software Boot reason %d", BootReason);


ArekBalysNordic · 2024-12-10T09:35:26Z

src/platform/nrfconnect/OTAImageProcessorImpl.cpp

@@ -182,6 +185,8 @@ CHIP_ERROR OTAImageProcessorImpl::Apply()
                PlatformMgr().HandleServerShuttingDown();
                k_msleep(CHIP_DEVICE_CONFIG_SERVER_SHUTDOWN_ACTIONS_SLEEP_MS);
 #ifdef CONFIG_DFU_TARGET_SUIT
+                BootReason = 5;


Can we use a pre-defined value for this?

ArekBalysNordic · 2024-12-10T09:36:01Z

src/platform/Zephyr/DiagnosticDataProviderImpl.cpp

+    if ((reason & (RESETINFO_RESETREAS_GLOBAL_RESETPOR_Msk | RESETINFO_RESETREAS_GLOBAL_SECSREQ_Msk)) ==
+        (RESETINFO_RESETREAS_GLOBAL_RESETPOR_Msk | RESETINFO_RESETREAS_GLOBAL_SECSREQ_Msk))
+    {
+        if (BootReason == 5)


Please use a pre-defined value here.

ArekBalysNordic · 2024-12-10T09:37:39Z

src/platform/Zephyr/DiagnosticDataProviderImpl.cpp

    uint32_t reason;
+#endif
+    ChipLogDetail(DeviceLayer, "Software Boot reason %d", BootReason);


sorry I forgot to remove Debug logs

-Use NRF_RESETINFO register to get boot reason Signed-off-by: Konrad Grucel <[email protected]>

LuDuda · 2024-12-15T20:32:08Z

src/platform/SaveBootReasonDFUSuit.h

+#ifndef NRF_SAVE_BOOT_REASON_H__
+#define NRF_SAVE_BOOT_REASON_H__
+
+inline int SoftwareBootReasonSUIT __attribute__((section(".noinit")));


Please move it inside the Reboot.cpp file.

Btw. We can't have a Nordic sepecific file in the src/platform file anyway.

Btw2. Do we need to add a SUIT in the name?

LuDuda · 2024-12-15T20:34:09Z

src/platform/nrfconnect/OTAImageProcessorImpl.cpp

@@ -182,6 +186,7 @@ CHIP_ERROR OTAImageProcessorImpl::Apply()
                PlatformMgr().HandleServerShuttingDown();
                k_msleep(CHIP_DEVICE_CONFIG_SERVER_SHUTDOWN_ACTIONS_SLEEP_MS);
 #ifdef CONFIG_DFU_TARGET_SUIT
+                SetSoftwareRebootReason(SoftwareRebootReason::kSoftwareUpdate);
                dfu_target_suit_reboot();


Just wonder.. Maybe we can call Reboot(SoftwareRebootReason::kSoftwareUpdate); and inside Reboot.cpp call dfu_target_suit_reboot(); for kSoftwareUpdate?

@ArekBalysNordic what do you think?

github-actions bot added platform nrf connect zephyr labels Nov 25, 2024

NordicBuilder mentioned this pull request Nov 25, 2024

manifest: sdk-connectedhomeip: Update revision nrfconnect/sdk-nrf#19059

Open

ArekBalysNordic requested a review from a team November 26, 2024 12:29

ArekBalysNordic requested changes Nov 26, 2024

View reviewed changes

LuDuda requested changes Nov 26, 2024

View reviewed changes

Damian-Nordic reviewed Nov 27, 2024

View reviewed changes

kg-nordicsemi force-pushed the save-reboot-reason-nrf54h20 branch from 69d980e to 5ac2b95 Compare November 27, 2024 14:05

LuDuda reviewed Nov 28, 2024

View reviewed changes

kg-nordicsemi force-pushed the save-reboot-reason-nrf54h20 branch 2 times, most recently from e5809a0 to 286f3d3 Compare November 28, 2024 14:48

LuDuda reviewed Dec 3, 2024

View reviewed changes

kg-nordicsemi force-pushed the save-reboot-reason-nrf54h20 branch from 286f3d3 to e39f5ca Compare December 10, 2024 08:31

ArekBalysNordic requested changes Dec 10, 2024

View reviewed changes

[nrf toup] Saving boot reason on nRF54h20

cae80de

-Use NRF_RESETINFO register to get boot reason Signed-off-by: Konrad Grucel <[email protected]>

kg-nordicsemi force-pushed the save-reboot-reason-nrf54h20 branch from e39f5ca to cae80de Compare December 10, 2024 16:00

LuDuda requested changes Dec 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nrf toup] getting boot reason on nRF54h20 #513

[nrf toup] getting boot reason on nRF54h20 #513

kg-nordicsemi commented Nov 25, 2024

ArekBalysNordic Nov 26, 2024

kg-nordicsemi Nov 27, 2024

ArekBalysNordic Nov 27, 2024

ArekBalysNordic Nov 26, 2024

Damian-Nordic Nov 27, 2024

LuDuda Nov 26, 2024 •

edited

Loading

kg-nordicsemi Nov 27, 2024

ArekBalysNordic Nov 27, 2024

Damian-Nordic Nov 27, 2024

Damian-Nordic Nov 27, 2024

kg-nordicsemi Nov 27, 2024

LuDuda Nov 28, 2024

kg-nordicsemi Nov 28, 2024

LuDuda Dec 3, 2024

LuDuda Dec 3, 2024

ArekBalysNordic Dec 10, 2024

ArekBalysNordic Dec 10, 2024

ArekBalysNordic Dec 10, 2024

ArekBalysNordic Dec 10, 2024

ArekBalysNordic Dec 10, 2024

kg-nordicsemi Dec 10, 2024

LuDuda Dec 15, 2024

LuDuda Dec 15, 2024

[nrf toup] getting boot reason on nRF54h20 #513

Are you sure you want to change the base?

[nrf toup] getting boot reason on nRF54h20 #513

Conversation

kg-nordicsemi commented Nov 25, 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

LuDuda Nov 26, 2024 • edited Loading

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

Choose a reason for hiding this comment

LuDuda Nov 26, 2024 •

edited

Loading