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.

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

drivers: hwinfo: Support for reset reasons in nRF54H20 #81751

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

kl-cruz
Copy link
Collaborator

@kl-cruz kl-cruz commented Nov 22, 2024

Adding support for reset reasons in the nRF54H20 SoC.

@kl-cruz kl-cruz marked this pull request as ready for review November 22, 2024 12:27
@zephyrbot zephyrbot added platform: nRF Nordic nRFx area: HWINFO Hardware Information Driver labels Nov 22, 2024
Copy link
Member

@alexanderwachter alexanderwachter left a comment

Choose a reason for hiding this comment

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

LGTM from an API point of perspective.
Anyone from nordic who can verify?

@kl-cruz kl-cruz force-pushed the hwinfo_h20_support branch 4 times, most recently from 0d7e6d2 to ec1721d Compare December 6, 2024 07:43
@kl-cruz kl-cruz requested a review from anangl December 6, 2024 11:40
tejlmand
tejlmand previously approved these changes Dec 11, 2024
@alexanderwachter
Copy link
Member

alexanderwachter commented Dec 11, 2024

I would propose to keep the code path clean and define a mask with all the macro decisions on top

Top:
#ifdef ...
RESET_CAUSE_XXX_MASK NRF_RESET_XXX
#else

RESET_CAUSE_XXX_MASK NRF_RESET_YYY
#endif

Code:

if(reason & RESET_CAUSE_XXX_MASK) {
flags |= RESET_XXX;
}

@nordic-segl
Copy link
Contributor

Compliance check is failing - Please add missing commit body.

@nordic-piks
Copy link
Collaborator

@kl-cruz can you follow up with this change?

@@ -8,7 +8,7 @@
#include <zephyr/drivers/hwinfo.h>
#include <string.h>
#include <zephyr/sys/byteorder.h>
#if !defined(CONFIG_SOC_SERIES_NRF54HX) && !defined(CONFIG_BOARD_QEMU_CORTEX_M0)
#if !defined(CONFIG_BOARD_QEMU_CORTEX_M0)
#include <helpers/nrfx_reset_reason.h>
Copy link
Member

Choose a reason for hiding this comment

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

This file cannot be included for cpuppr and cpuflpr on nRF54H20 because this leads to #error "Unsupported device" (and excluding them just in tests/drivers/hwinfo/api is not sufficient).
How about handling this with something like:

#if defined(CONFIG_BOARD_QEMU_CORTEX_M0) || \
	(defined(CONFIG_NRF_PLATFORM_HALTIUM) && \
	 defined(CONFIG_RISCV_CORE_NORDIC_VPR))
#define RESET_CAUSE_AVAILABLE 0
#else
#include <helpers/nrfx_reset_reason.h>
#define RESET_CAUSE_AVAILABLE 1
#endif

and then use #if RESET_REASON_AVAILABLE before z_impl_hwinfo_get_reset_cause()?

@@ -1,5 +1,7 @@
tests:
drivers.hwinfo.api:
platform_exclude:
- nrf54h20dk/nrf54h20/cpuppr
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed?

anangl
anangl previously approved these changes Jan 29, 2025
nika-nordic
nika-nordic previously approved these changes Jan 29, 2025
@nordic-krch nordic-krch dismissed stale reviews from nika-nordic and anangl via 856cced January 29, 2025 14:54
Comment on lines 75 to 79
#define REASON_LOCKUP (NRFX_RESET_REASON_LOCKUP || NRFX_RESET_REASON_LOCAL_LOCKUP_MASK)
#define REASON_SOFTWARE (NRFX_RESET_REASON_SREQ || NRFX_RESET_REASON_LOCAL_SREQ_MASK)
#define REASON_WATCHDOG \
(NRFX_RESET_REASON_DOG_MASK || \
NRFX_RESET_REASON_LOCAL_DOG1_MASK || \
Copy link
Member

Choose a reason for hiding this comment

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

These are bitmasks, so you should use |, not ||.

anangl
anangl previously approved these changes Jan 31, 2025
#if NRF_POWER_HAS_RESETREAS
#define REASON_WATCHDOG NRFX_RESET_REASON_DOG_MASK
#else
#define REASON_WATCHDOG NRFX_RESET_REASON_DOG1_MASK
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be actually (NRFX_RESET_REASON_DOG0_MASK | NRFX_RESET_REASON_DOG1_MASK)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Added DOG0

@nordic-krch
Copy link
Contributor

Had to align to nrfx 3.10 where some enum names changed (e.g. NRFX_RESET_REASON_SREQ -> NRFX_RESET_REASON_SREQ_MASK)

#if NRF_POWER_HAS_RESETREAS
#define REASON_WATCHDOG NRFX_RESET_REASON_DOG_MASK
#else
#define REASON_WATCHDOG NRFX_RESET_REASON_DOG0_MASK | NRFX_RESET_REASON_DOG1_MASK
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#define REASON_WATCHDOG NRFX_RESET_REASON_DOG0_MASK | NRFX_RESET_REASON_DOG1_MASK
#define REASON_WATCHDOG (NRFX_RESET_REASON_DOG0_MASK | NRFX_RESET_REASON_DOG1_MASK)

Copy link
Contributor

Choose a reason for hiding this comment

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

done

Adding support for reset reasons in the nRF54H20 SoC.

Signed-off-by: Karol Lasończyk <karol.lasonczyk@nordicsemi.no>
@kartben kartben merged commit f551b2d into zephyrproject-rtos:main Feb 4, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: HWINFO Hardware Information Driver platform: nRF Nordic nRFx
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet