Skip to content

API for request MCUboot mode by the application #44512

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

Closed
wants to merge 3 commits into from
Closed

API for request MCUboot mode by the application #44512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a59acb
dfu/Kconfig: clean up MCUBOOT_IMG_MANAGER dependence
nvlsianpu Apr 4, 2022
8738722
dfu: API for request MCUboot mode
nvlsianpu Apr 4, 2022
8fb78c4
soc/arm/nordic_nrf: implementation of request and fetch the MCUboot mode
nvlsianpu Apr 4, 2022
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
35 changes: 35 additions & 0 deletions include/dfu/mcuboot_boot_mode.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright (c) 2022 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/

#ifndef ZEPHYR_INCLUDE_MCUBOOT_BOOT_MODE_H_

#define BOOT_MODE_REQ_NORMAL 0x00
#define BOOT_MODE_REQ_RECOVERY_DFU 0x01
Comment on lines +7 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

It's been a while since I've looked at zephyr's MCUboot related APIs, but I'm confused about this mix of "MCUBOOT" and "BOOT" in the namespace. Is this meant to be MCUboot-specific, or generic across all bootloaders?

If it is meant to be mcuboot-specific, is there any harm in using MCUBOOT consistently? (I think this is the real question I am trying to ask.)

If it is meant to be generic, why is MCUBOOT appearing sometimes here?


/**
* Set bootloader mode
*
* Using this the application can request specific bootloader mode.
* MCUboot might enter this mode once system reboot.
*
* @param mode requested mode
*
* @retval 0 on success, negative errno code on fail.
*/
int boot_mode_set(uint8_t mode);

/**
* Get bootloader mode
*
* Using this the bootloader might obtain the mode requested by the application.
* Only first call returns valid value as the implementation should cleanup
* hardware resource used for transferring the mode from the application.
*
* @retval bootloader mode
*/
uint8_t boot_mode_get(void);

#endif /* ZEPHYR_INCLUDE_MCUBOOT_BOOT_MODE_H_ */
16 changes: 16 additions & 0 deletions soc/arm/nordic_nrf/nrf52/soc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,22 @@ extern void z_arm_nmi_init(void);
#define LOG_LEVEL CONFIG_SOC_LOG_LEVEL
LOG_MODULE_REGISTER(soc);

int soc_mcuboot_mode_set(uint8_t mode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing wrong with this line of code, but the Signed-off-by: line in the commit message has the wrong format.

{
nrf_power_gpregret2_set(NRF_POWER, mode);
Copy link
Contributor

Choose a reason for hiding this comment

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

From spot-checking a product specification, the GPREGRET and GPREGRET2 registers seem to be available for application developer use too. Is there any risk in using them for this purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GPREGRET is used by NCS internally for instance, it is also place where nRF SoC stores reboot-type in zephyr.

Copy link
Collaborator Author

@nvlsianpu nvlsianpu May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I can use GPREGRET for transfer both the reboot-type and the recovery-dfu request. This implies that I need to introduce an API for retrieve reboot-type as well.
What your opinion @mbolivar

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't understand your last comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you see usage of GPREGRET2 as the waste of resources I can make a better solution:

The idea would be to start using GPREGRET for request of recovery-dfu as well (along with reboot-type to keep of which it is used currently).
As intermediate steep I would implement API for retrieve reboot-type as there is sys_arch_reboot(int type) call but there is no a counterpart to it which would allows to check what a reboot-type was used.
Of course this mean that zephyr common API for get reboot type will be needed:
there is void sys_reboot(int type) but zephyr doesn't have API for retrieve the type

@mbolivar-nordic What is your opinion on that ^^ idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, OK, I think I understand now. No, if we're already using GPREGRET, from my side I see no harm in using GPREGRET2 as well. You're saying you're not aware of any customers already using this that will be negatively impacted, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

alternative: #46084


return 0;
}

uint8_t soc_mcuboot_mode_get(void)
{
uint8_t mode = nrf_power_gpregret2_get(NRF_POWER);

nrf_power_gpregret2_set(NRF_POWER, 0);

return mode;
}

/* Overrides the weak ARM implementation:
Set general purpose retention register and reboot */
void sys_arch_reboot(int type)
Expand Down
20 changes: 20 additions & 0 deletions soc/arm/nordic_nrf/nrf53/soc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,13 @@
#include <hal/nrf_gpio.h>
#include <hal/nrf_oscillators.h>
#include <hal/nrf_regulators.h>
#include <hal/nrf_power.h>
#elif defined(CONFIG_SOC_NRF5340_CPUNET)
#include <hal/nrf_nvmc.h>
#endif
#include <soc_secure.h>


#define PIN_XL1 0
#define PIN_XL2 1

Expand Down Expand Up @@ -66,6 +68,24 @@ extern void z_arm_nmi_init(void);
#define LOG_LEVEL CONFIG_SOC_LOG_LEVEL
LOG_MODULE_REGISTER(soc);

#if defined(CONFIG_SOC_NRF5340_CPUAPP)
int soc_mcuboot_mode_set(uint8_t mode)
{
nrf_power_gpregret2_set(NRF_POWER, mode);

return 0;
}

uint8_t soc_mcuboot_mode_get(void)
{
uint8_t mode = nrf_power_gpregret2_get(NRF_POWER);

nrf_power_gpregret2_set(NRF_POWER, 0);

return mode;
}
#endif

static int nordicsemi_nrf53_init(const struct device *arg)
{
uint32_t key;
Expand Down
17 changes: 17 additions & 0 deletions soc/arm/nordic_nrf/nrf91/soc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <arch/arm/aarch32/cortex_m/cmsis.h>
#include <soc/nrfx_coredep.h>
#include <logging/log.h>
#include <hal/nrf_power.h>

#ifdef CONFIG_RUNTIME_NMI
extern void z_arm_nmi_init(void);
Expand All @@ -34,6 +35,22 @@ extern void z_arm_nmi_init(void);
#define LOG_LEVEL CONFIG_SOC_LOG_LEVEL
LOG_MODULE_REGISTER(soc);

int soc_mcuboot_mode_set(uint8_t mode)
{
nrf_power_gpregret_ext_set(NRF_POWER, 1, mode);

return 0;
}

uint8_t soc_mcuboot_mode_set(void)
{
uint8_t mode = nrf_power_gpregret_ext_get(NRF_POWER, 1);

nrf_power_gpregret_ext_set(NRF_POWER, 1, 0);

return mode;
}

static int nordicsemi_nrf91_init(const struct device *arg)
{
uint32_t key;
Expand Down
2 changes: 1 addition & 1 deletion subsys/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ add_subdirectory_ifdef(CONFIG_EMUL emul)
add_subdirectory(fs)
add_subdirectory(ipc)
add_subdirectory(mgmt)
add_subdirectory_ifdef(CONFIG_MCUBOOT_IMG_MANAGER dfu)
add_subdirectory(dfu)
add_subdirectory_ifdef(CONFIG_NET_BUF net)
add_subdirectory_ifdef(CONFIG_USB_DEVICE_STACK usb)
add_subdirectory(random)
Expand Down
5 changes: 3 additions & 2 deletions subsys/dfu/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# SPDX-License-Identifier: Apache-2.0

add_subdirectory(boot)
add_subdirectory(img_util)
add_subdirectory_ifdef(CONFIG_MCUBOOT_IMG_MANAGER boot)
add_subdirectory_ifdef(CONFIG_MCUBOOT_IMG_MANAGER img_util)
add_subdirectory(boot_mode)
18 changes: 13 additions & 5 deletions subsys/dfu/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
# DFU
#

menu "DFU"
menuconfig IMG_MANAGER
bool "DFU image manager"
select STREAM_FLASH
Expand All @@ -31,10 +32,10 @@ config MCUBOOT_IMG_MANAGER

endchoice

if MCUBOOT_IMG_MANAGER
config MCUBOOT_SHELL
bool "MCUboot shell"
default y
depends on MCUBOOT_IMG_MANAGER
depends on SHELL
help
Enable shell module, which provides information about image slots and
Expand All @@ -44,7 +45,6 @@ config MCUBOOT_SHELL
config MCUBOOT_TRAILER_SWAP_TYPE
bool "use trailer's swap_type field"
default y
depends on MCUBOOT_IMG_MANAGER
help
Enables usage swap type field which is required after
"Fix double swap on interrupted revert" mcuboot patch
Expand All @@ -54,15 +54,13 @@ config MCUBOOT_TRAILER_SWAP_TYPE

config IMG_BLOCK_BUF_SIZE
int "Image writer buffer size"
depends on MCUBOOT_IMG_MANAGER
default 512
help
Size (in Bytes) of buffer for image writer. Must be a multiple of
the access alignment required by used flash driver.

config IMG_ERASE_PROGRESSIVELY
bool "Erase flash progressively when receiving new firmware"
depends on MCUBOOT_IMG_MANAGER
select STREAM_FLASH_ERASE
help
If enabled, flash is erased as necessary when receiving new firmware,
Expand All @@ -72,7 +70,6 @@ config IMG_ERASE_PROGRESSIVELY

config IMG_ENABLE_IMAGE_CHECK
bool "Image check functions"
depends on MCUBOOT_IMG_MANAGER
select FLASH_AREA_CHECK_INTEGRITY
help
If enabled, there will be available the function to check flash
Expand All @@ -81,6 +78,8 @@ config IMG_ENABLE_IMAGE_CHECK
Another use is to ensure that firmware upgrade routines from internet
server to flash slot are performing properly.

endif # MCUBOOT_IMG_MANAGER

module = IMG_MANAGER
module-str = image manager
source "subsys/logging/Kconfig.template.log_config"
Expand All @@ -96,3 +95,12 @@ config UPDATEABLE_IMAGE_NUMBER
endif

endif # IMG_MANAGER

config MCUBOOT_BOOT_MODE_API
bool "API for request bootloader mode"
help
Application can request specific bootloader mode. MCUboot might enter
this mode once system reboot. Implementations of this use SoC specific
resources, like a reset retention register.

endmenu # DFU
3 changes: 3 additions & 0 deletions subsys/dfu/boot_mode/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# SPDX-License-Identifier: Apache-2.0

zephyr_sources_ifdef(CONFIG_MCUBOOT_BOOT_MODE_API mcuboot_boot_mode.c)
19 changes: 19 additions & 0 deletions subsys/dfu/boot_mode/mcuboot_boot_mode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* Copyright (c) 2022 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/
#include <zephyr.h>

extern int soc_mcuboot_mode_set(uint8_t mode);
extern uint8_t soc_mcuboot_mode_get(void);

int boot_mode_set(uint8_t mode)
{
return soc_mcuboot_mode_set(mode);
}

uint8_t boot_mode_get(void)
{
return soc_mcuboot_mode_get();
}