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

[nrf toup] getting boot reason on nRF54h20 #513

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[nrf toup] getting boot reason on nRF54h20 #513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/platform/SaveBootReasonDFUSuit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright (c) 2021 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
*/

#ifndef NRF_SAVE_BOOT_REASON_H__
#define NRF_SAVE_BOOT_REASON_H__

inline int SoftwareBootReasonSUIT __attribute__((section(".noinit")));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?


inline int getSoftwareRebootReasonSUIT()
{
return SoftwareBootReasonSUIT;
}

inline void setSoftwareRebootReasonSUIT(int reason)
{
SoftwareBootReasonSUIT = reason;
}

#endif
52 changes: 51 additions & 1 deletion src/platform/Zephyr/DiagnosticDataProviderImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,66 @@ const size_t kMaxHeapSize = CONFIG_SRAM_BASE_ADDRESS + KB(CONFIG_SRAM_SIZE) - PO

#endif

#ifdef CONFIG_SOC_SERIES_NRF54HX
#include <hal/nrf_resetinfo.h>
#include <platform/SaveBootReasonDFUSuit.h>
#endif

namespace chip {
namespace DeviceLayer {

namespace {

BootReasonType DetermineBootReason()
{
#ifdef CONFIG_HWINFO

#if defined(CONFIG_SOC_SERIES_NRF54HX) || defined(CONFIG_HWINFO)
uint32_t reason;
#endif

#ifdef CONFIG_SOC_SERIES_NRF54HX
Copy link
Contributor

Choose a reason for hiding this comment

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

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


bool isSoftwareBootReasonSUITInitialized = false;
if (!isSoftwareBootReasonSUITInitialized)
{
setSoftwareRebootReasonSUIT(0);
isSoftwareBootReasonSUITInitialized = true;
}

reason = nrf_resetinfo_resetreas_global_get(NRF_RESETINFO);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?


if (reason == RESETINFO_RESETREAS_GLOBAL_ResetValue)
{
return BootReasonType::kSoftwareReset;
}

if (reason & RESETINFO_RESETREAS_GLOBAL_RESETPORONLY_Msk)
{
return BootReasonType::kBrownOutReset;
}

if (reason & RESETINFO_RESETREAS_GLOBAL_DOG_Msk)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about changing ifs to else ifs?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{
return BootReasonType::kHardwareWatchdogReset;
}

if ((reason & (RESETINFO_RESETREAS_GLOBAL_RESETPIN_Msk | RESETINFO_RESETREAS_GLOBAL_RESETPOR_Msk)) ==
(RESETINFO_RESETREAS_GLOBAL_RESETPIN_Msk | RESETINFO_RESETREAS_GLOBAL_RESETPOR_Msk))
{
return BootReasonType::kPowerOnReboot;
}

if ((reason & (RESETINFO_RESETREAS_GLOBAL_RESETPOR_Msk | RESETINFO_RESETREAS_GLOBAL_SECSREQ_Msk)) ==
(RESETINFO_RESETREAS_GLOBAL_RESETPOR_Msk | RESETINFO_RESETREAS_GLOBAL_SECSREQ_Msk))
{
if (GetSoftwareRebootReason() == SoftwareRebootReason::kSoftwareUpdate)
{
return BootReasonType::kSoftwareUpdateCompleted;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

#endif

#ifdef CONFIG_HWINFO
if (hwinfo_get_reset_cause(&reason) != 0)
{
return BootReasonType::kUnspecified;
Expand Down
5 changes: 5 additions & 0 deletions src/platform/nrfconnect/OTAImageProcessorImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@
#include <zephyr/logging/log.h>
#include <zephyr/pm/device.h>

#ifdef CONFIG_SOC_SERIES_NRF54HX
#include <platform/SaveBootReasonDFUSuit.h>
#endif

namespace chip {
namespace {
#ifdef CONFIG_CHIP_CERTIFICATION_DECLARATION_STORAGE
Expand Down Expand Up @@ -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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

#else
Reboot(SoftwareRebootReason::kSoftwareUpdate);
Expand Down
35 changes: 33 additions & 2 deletions src/platform/nrfconnect/Reboot.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@
#include <hal/nrf_power.h>
#endif

#include <platform/SaveBootReasonDFUSuit.h>

namespace chip {
namespace DeviceLayer {

#if defined(CONFIG_ARCH_POSIX) || defined(CONFIG_SOC_SERIES_NRF54HX)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

#if defined(CONFIG_ARCH_POSIX)

void Reboot(SoftwareRebootReason reason)
{
Expand All @@ -40,7 +42,7 @@ SoftwareRebootReason GetSoftwareRebootReason()
return SoftwareRebootReason::kOther;
}

#else
#elif !(defined(CONFIG_SOC_SERIES_NRF54HX))

using RetainedReason = decltype(nrf_power_gpregret_get(NRF_POWER, 0));

Expand Down Expand Up @@ -73,6 +75,35 @@ SoftwareRebootReason GetSoftwareRebootReason()
}
}

#else

using RetainedReason = decltype(getSoftwareRebootReasonSUIT());

constexpr RetainedReason EncodeReason(SoftwareRebootReason reason)
{
// Set MSB to avoid collission with Zephyr's pre-defined reboot reasons.
constexpr RetainedReason kCustomReasonFlag = 0x80;

return static_cast<RetainedReason>(reason) | kCustomReasonFlag;
}

void SetSoftwareRebootReason(SoftwareRebootReason reason)
{
const RetainedReason retainedReason = EncodeReason(reason);
setSoftwareRebootReasonSUIT(retainedReason);
}

SoftwareRebootReason GetSoftwareRebootReason()
{
switch (getSoftwareRebootReasonSUIT())
{
case EncodeReason(SoftwareRebootReason::kSoftwareUpdate):
setSoftwareRebootReasonSUIT(0);
return SoftwareRebootReason::kSoftwareUpdate;
default:
return SoftwareRebootReason::kOther;
}
}
#endif

} // namespace DeviceLayer
Expand Down
1 change: 1 addition & 0 deletions src/platform/nrfconnect/Reboot.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ enum class SoftwareRebootReason : uint8_t

[[noreturn]] void Reboot(SoftwareRebootReason reason);
SoftwareRebootReason GetSoftwareRebootReason();
void SetSoftwareRebootReason(SoftwareRebootReason reason);

} // namespace DeviceLayer
} // namespace chip
Loading