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

Jump to bottom

soc: nordic: Add LRCCONF management #79067

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+162 −73
Open

soc: nordic: Add LRCCONF management #79067

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
35 changes: 0 additions & 35 deletions drivers/clock_control/clock_control_nrf2_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
*/

#include "clock_control_nrf2_common.h"
#include <hal/nrf_lrcconf.h>

#include <zephyr/logging/log.h>
LOG_MODULE_REGISTER(clock_control_nrf2, CONFIG_CLOCK_CONTROL_LOG_LEVEL);
Expand All @@ -25,9 +24,6 @@ LOG_MODULE_REGISTER(clock_control_nrf2, CONFIG_CLOCK_CONTROL_LOG_LEVEL);
*/
STRUCT_CLOCK_CONFIG(generic, ONOFF_CNT_MAX);

static sys_slist_t poweron_main_list;
static struct k_spinlock poweron_main_lock;

static void update_config(struct clock_config_generic *cfg)
{
atomic_val_t prev_flags = atomic_or(&cfg->flags, FLAG_UPDATE_NEEDED);
Expand Down Expand Up @@ -163,34 +159,3 @@ int api_nosys_on_off(const struct device *dev, clock_control_subsys_t sys)

return -ENOSYS;
}

void clock_request_lrcconf_poweron_main(struct clock_lrcconf_sink *sink)
{
K_SPINLOCK(&poweron_main_lock) {
if (sys_slist_len(&poweron_main_list) == 0) {
LOG_DBG("%s forced on", "main domain");
NRF_LRCCONF010->POWERON &= ~LRCCONF_POWERON_MAIN_Msk;
NRF_LRCCONF010->POWERON |= LRCCONF_POWERON_MAIN_AlwaysOn;
}

sys_slist_find_and_remove(&poweron_main_list, &sink->node);
sys_slist_append(&poweron_main_list, &sink->node);
}
}

void clock_release_lrcconf_poweron_main(struct clock_lrcconf_sink *sink)
{
K_SPINLOCK(&poweron_main_lock) {
if (!sys_slist_find_and_remove(&poweron_main_list, &sink->node)) {
K_SPINLOCK_BREAK;
}

if (sys_slist_len(&poweron_main_list) > 0) {
K_SPINLOCK_BREAK;
}

LOG_DBG("%s automatic", "main domain");
NRF_LRCCONF010->POWERON &= ~LRCCONF_POWERON_MAIN_Msk;
NRF_LRCCONF010->POWERON |= LRCCONF_POWERON_MAIN_Automatic;
}
}
10 changes: 0 additions & 10 deletions drivers/clock_control/clock_control_nrf2_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,4 @@ void clock_config_update_end(void *clk_cfg, int status);

int api_nosys_on_off(const struct device *dev, clock_control_subsys_t sys);

struct clock_lrcconf_sink {
sys_snode_t node;
};

/**
* @brief Request or release lrcconf main power domain
*/
void clock_request_lrcconf_poweron_main(struct clock_lrcconf_sink *sink);
Copy link
Member

Choose a reason for hiding this comment

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

Please remove also the struct clock_lrcconf_sink definition. There is no need to keep it as it can be replaced with just sys_snode_t.

void clock_release_lrcconf_poweron_main(struct clock_lrcconf_sink *sink);

#endif /* ZEPHYR_DRIVERS_CLOCK_CONTROL_NRF2_COMMON_H_ */
8 changes: 4 additions & 4 deletions drivers/clock_control/clock_control_nrf2_fll16m.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#include "clock_control_nrf2_common.h"
#include <zephyr/devicetree.h>
#include <zephyr/drivers/clock_control/nrf_clock_control.h>
#include <hal/nrf_lrcconf.h>
#include <soc_lrcconf.h>

#include <zephyr/logging/log.h>
LOG_MODULE_DECLARE(clock_control_nrf2, CONFIG_CLOCK_CONTROL_LOG_LEVEL);
Expand Down Expand Up @@ -64,7 +64,7 @@ static const struct clock_options {
struct fll16m_dev_data {
STRUCT_CLOCK_CONFIG(fll16m, ARRAY_SIZE(clock_options)) clk_cfg;
struct onoff_client hfxo_cli;
struct clock_lrcconf_sink lrcconf_sink;
sys_snode_t lrcconf_client_node;
};

struct fll16m_dev_config {
Expand All @@ -76,13 +76,13 @@ static void activate_fll16m_mode(struct fll16m_dev_data *dev_data, uint8_t mode)
/* TODO: change to nrf_lrcconf_* function when such is available. */

if (mode != FLL16M_MODE_DEFAULT) {
clock_request_lrcconf_poweron_main(&dev_data->lrcconf_sink);
soc_lrcconf_poweron_request(&dev_data->lrcconf_client_node, NRF_LRCCONF_POWER_MAIN);
}

NRF_LRCCONF010->CLKCTRL[0].SRC = mode;

if (mode == FLL16M_MODE_DEFAULT) {
clock_release_lrcconf_poweron_main(&dev_data->lrcconf_sink);
soc_lrcconf_poweron_release(&dev_data->lrcconf_client_node, NRF_LRCCONF_POWER_MAIN);
}

nrf_lrcconf_task_trigger(NRF_LRCCONF010, NRF_LRCCONF_TASK_CLKSTART_0);
Expand Down
8 changes: 4 additions & 4 deletions drivers/clock_control/clock_control_nrf2_hfxo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#include <zephyr/logging/log.h>
LOG_MODULE_DECLARE(clock_control_nrf2, CONFIG_CLOCK_CONTROL_LOG_LEVEL);

#include <hal/nrf_lrcconf.h>
#include <soc_lrcconf.h>

BUILD_ASSERT(DT_NUM_INST_STATUS_OKAY(DT_DRV_COMPAT) == 1,
"multiple instances not supported");
Expand All @@ -20,7 +20,7 @@ struct dev_data_hfxo {
struct onoff_manager mgr;
onoff_notify_fn notify;
struct k_timer timer;
struct clock_lrcconf_sink lrcconf_sink;
sys_snode_t lrcconf_client_node;
};

struct dev_config_hfxo {
Expand Down Expand Up @@ -58,7 +58,7 @@ static void onoff_start_hfxo(struct onoff_manager *mgr, onoff_notify_fn notify)
dev_data->notify = notify;

nrf_lrcconf_event_clear(NRF_LRCCONF010, NRF_LRCCONF_EVENT_HFXOSTARTED);
clock_request_lrcconf_poweron_main(&dev_data->lrcconf_sink);
soc_lrcconf_poweron_request(&dev_data->lrcconf_client_node, NRF_LRCCONF_POWER_MAIN);
nrf_lrcconf_task_trigger(NRF_LRCCONF010, NRF_LRCCONF_TASK_REQHFXO);

/* Due to a hardware issue, the HFXOSTARTED event is currently
Expand All @@ -74,7 +74,7 @@ static void onoff_stop_hfxo(struct onoff_manager *mgr, onoff_notify_fn notify)
CONTAINER_OF(mgr, struct dev_data_hfxo, mgr);

nrf_lrcconf_task_trigger(NRF_LRCCONF010, NRF_LRCCONF_TASK_STOPREQHFXO);
clock_release_lrcconf_poweron_main(&dev_data->lrcconf_sink);
soc_lrcconf_poweron_release(&dev_data->lrcconf_client_node, NRF_LRCCONF_POWER_MAIN);
notify(mgr, 0);
}

Expand Down
1 change: 1 addition & 0 deletions soc/nordic/common/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ if(CONFIG_ARM)
endif()

zephyr_library_sources_ifdef(CONFIG_POWEROFF poweroff.c)
zephyr_library_sources_ifdef(CONFIG_NRF_PLATFORM_HALTIUM soc_lrcconf.c)

if((CONFIG_SOC_SERIES_NRF54HX OR CONFIG_SOC_SERIES_NRF92X) AND CONFIG_CPU_HAS_CUSTOM_FIXED_SOC_MPU_REGIONS)
zephyr_library_sources(nrf54hx_nrf92x_mpu_regions.c)
Expand Down
63 changes: 63 additions & 0 deletions soc/nordic/common/soc_lrcconf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/*
* Copyright (c) 2024 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <soc_lrcconf.h>
#include <zephyr/kernel.h>

static struct k_spinlock lock;
static sys_slist_t poweron_main_list;
static sys_slist_t poweron_active_list;

void soc_lrcconf_poweron_request(sys_snode_t *node, nrf_lrcconf_power_domain_mask_t domain)
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason why simple reference counter is not used here? I only see that the difference is that you keep list of clients (but its not used anywhere) and allow to have non-symmetric requests (second request from the same client is not counted) but it can potentially cause some issues. Reference counter would be simpler and use less resources.

Copy link
Collaborator

Choose a reason for hiding this comment

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

At least right now there is non-symmetric requests so it couldn't be dropped

{
__ASSERT(is_power_of_two(domain),
"Only one bit can be set for the domain parameter");

Check notice on line 18 in soc/nordic/common/soc_lrcconf.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

soc/nordic/common/soc_lrcconf.c:18 
-	__ASSERT(is_power_of_two(domain),
-		 "Only one bit can be set for the domain parameter");
+	__ASSERT(is_power_of_two(domain), "Only one bit can be set for the domain parameter");
sys_slist_t *poweron_list;

if (domain == NRF_LRCCONF_POWER_MAIN) {
poweron_list = &poweron_main_list;
} else if (domain == NRF_LRCCONF_POWER_DOMAIN_0) {
poweron_list = &poweron_active_list;
} else {
return;
}
K_SPINLOCK(&lock) {
if (sys_slist_len(poweron_list) == 0) {
nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, domain, true);
}

sys_slist_find_and_remove(poweron_list, node);
sys_slist_append(poweron_list, node);
}
}

void soc_lrcconf_poweron_release(sys_snode_t *node, nrf_lrcconf_power_domain_mask_t domain)
{
__ASSERT(is_power_of_two(domain),
"Only one bit can be set for the domain parameter");

Check notice on line 42 in soc/nordic/common/soc_lrcconf.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

soc/nordic/common/soc_lrcconf.c:42 
-	__ASSERT(is_power_of_two(domain),
-		 "Only one bit can be set for the domain parameter");
+	__ASSERT(is_power_of_two(domain), "Only one bit can be set for the domain parameter");
sys_slist_t *poweron_list;

if (domain == NRF_LRCCONF_POWER_MAIN) {
poweron_list = &poweron_main_list;
} else if (domain == NRF_LRCCONF_POWER_DOMAIN_0) {
poweron_list = &poweron_active_list;
} else {
return;
}

K_SPINLOCK(&lock) {
if (!sys_slist_find_and_remove(poweron_list, node)) {
K_SPINLOCK_BREAK;
}

if (sys_slist_len(poweron_list) > 0) {
K_SPINLOCK_BREAK;
}
nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, domain, false);
}
}
34 changes: 34 additions & 0 deletions soc/nordic/common/soc_lrcconf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright (c) 2024 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/

/**
* @file nRF SoC specific helpers for lrcconf management
*/

#ifndef ZEPHYR_SOC_NORDIC_COMMON_LRCCONF_H_
#define ZEPHYR_SOC_NORDIC_COMMON_LRCCONF_H_

#include <hal/nrf_lrcconf.h>

/**
* @brief Request lrcconf power domain
*
* @param node Pointer to the @ref sys_snode_t structure which is the ID of the
* requesting module.
* @param domain The mask that represents the power domain ID.
*/
void soc_lrcconf_poweron_request(sys_snode_t *node, nrf_lrcconf_power_domain_mask_t domain);

/**
* @brief Release lrcconf power domain
*
* @param node Pointer to the @ref sys_snode_t structure which is the ID of the
* requesting module.
* @param domain The mask that represents the power domain ID.
*/
void soc_lrcconf_poweron_release(sys_snode_t *node, nrf_lrcconf_power_domain_mask_t domain);

#endif /* ZEPHYR_SOC_NORDIC_COMMON_LRCCONF_H_ */
1 change: 1 addition & 0 deletions soc/nordic/nrf54h/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ config SOC_NRF54H20_CPUAPP
select NRFS_HAS_VBUS_DETECTOR_SERVICE
select HAS_PM
select HAS_POWEROFF
select ARM_ON_ENTER_CPU_IDLE_HOOK if PM || POWEROFF

config SOC_NRF54H20_CPURAD
select ARM
Expand Down
41 changes: 26 additions & 15 deletions soc/nordic/nrf54h/power.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,16 @@
#include <zephyr/pm/policy.h>
#include <zephyr/arch/common/pm_s2ram.h>
#include <hal/nrf_resetinfo.h>
#include <hal/nrf_lrcconf.h>
#include <hal/nrf_memconf.h>
#include <zephyr/cache.h>
#include <power.h>
#include <soc_lrcconf.h>
#include "pm_s2ram.h"

#if !defined(CONFIG_SOC_NRF54H20_CPURAD)
static sys_snode_t pm_node;
#endif

static void suspend_common(void)
{

Expand All @@ -31,21 +35,20 @@ static void suspend_common(void)
RAMBLOCK_CONTROL_BIT_ICACHE, false);
}

#if !defined(CONFIG_SOC_NRF54H20_CPURAD)
/* Disable retention */
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);
#endif
}

void nrf_poweroff(void)
{
nrf_resetinfo_resetreas_local_set(NRF_RESETINFO, 0);
nrf_resetinfo_restore_valid_set(NRF_RESETINFO, false);

#if !defined(CONFIG_SOC_NRF54H20_CPURAD)
nrf_lrcconf_retain_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_MAIN, false);

/* TODO: Move it around k_cpu_idle() implementation. */
nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_MAIN, false);
nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_DOMAIN_0, false);
#endif

suspend_common();

Expand All @@ -59,7 +62,7 @@ void nrf_poweroff(void)
CODE_UNREACHABLE;
}

#if IS_ENABLED(CONFIG_PM_S2RAM)
#if defined(CONFIG_PM_S2RAM)
/* Resume domain after local suspend to RAM. */
static void sys_resume(void)
{
Expand All @@ -77,12 +80,10 @@ static void sys_resume(void)
sys_cache_data_enable();
}

#if !defined(CONFIG_SOC_NRF54H20_CPURAD)
/* Re-enable domain retention. */
nrf_lrcconf_retain_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_DOMAIN_0, true);

/* TODO: Move it around k_cpu_idle() implementation. */
nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_MAIN,
!IS_ENABLED(CONFIG_SOC_NRF54H20_CPURAD));
#endif
}

/* Function called during local domain suspend to RAM. */
Expand All @@ -94,8 +95,6 @@ static int sys_suspend_to_ram(void)
nrf_resetinfo_resetreas_local_set(NRF_RESETINFO,
NRF_RESETINFO_RESETREAS_LOCAL_UNRETAINED_MASK);
nrf_resetinfo_restore_valid_set(NRF_RESETINFO, true);
nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_DOMAIN_0, false);
nrf_lrcconf_poweron_force_set(NRF_LRCCONF010, NRF_LRCCONF_POWER_MAIN, false);

suspend_common();

Expand Down Expand Up @@ -126,20 +125,32 @@ static void do_suspend_to_ram(void)

sys_resume();
}
#endif /* IS_ENABLED(CONFIG_PM_S2RAM) */
#endif /* defined(CONFIG_PM_S2RAM) */

void pm_state_set(enum pm_state state, uint8_t substate_id)
{
if (state != PM_STATE_SUSPEND_TO_RAM) {
k_cpu_idle();
return;
}
#if IS_ENABLED(CONFIG_PM_S2RAM)
#if defined(CONFIG_PM_S2RAM)
do_suspend_to_ram();
#endif
}

void pm_state_exit_post_ops(enum pm_state state, uint8_t substate_id)
{
#if !defined(CONFIG_SOC_NRF54H20_CPURAD)
if (state == PM_STATE_SUSPEND_TO_IDLE) {
soc_lrcconf_poweron_release(&pm_node, NRF_LRCCONF_POWER_DOMAIN_0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This only releases the poweron request when PM_STATE_SUSPEND_TO_IDLE was used to call k_cpu_idle() via the pm subsystem, but if the pm subsystem decided against suspending the system it will call k_cpu_idle() here instead and then not call the release.
This can happen if the is no suspend-to-idle state defined in devicetree or it has a minimum residency time that isn't met, e.g. a k_msleep(10) for with this pm configuration https://github.com/nrfconnect/sdk-nrf/blob/84492d6d9264745050a018d01f4f333b285864c1/tests/benchmarks/multicore/idle/boards/nrf54h20dk_nrf54h20_cpuapp_s2ram.overlay#L12C39-L12C39

This is still way better than losing power, but needs to be followed up to have all paths covered eventually

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a mechanism to supply an exit hook on arm cores

zephyr/arch/arm/Kconfig

Line 56 in abd9008

config ARM_ON_EXIT_CPU_IDLE

Copy link
Contributor

Choose a reason for hiding this comment

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

there is this mechanism but it is quite ugly and requires special header with a macro. It was added as a workaround for PAN where code need to be executed immediately after WFI (so function cannot be used). I have #71667 which adds function hook symmetric to on_enter hook but PR stuck.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe your PR would have a clean usecase now and it could be argued for again to avoid that assembly mess?

}
#endif
irq_unlock(0);
}

#if !defined(CONFIG_SOC_NRF54H20_CPURAD)
void pm_state_enter_idle_prepare(void)
{
soc_lrcconf_poweron_request(&pm_node, NRF_LRCCONF_POWER_DOMAIN_0);
}
#endif
9 changes: 9 additions & 0 deletions soc/nordic/nrf54h/power.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,20 @@
#ifndef _ZEPHYR_SOC_ARM_NORDIC_NRF_POWER_H_
#define _ZEPHYR_SOC_ARM_NORDIC_NRF_POWER_H_

#include <zephyr/sys/slist.h>

/**
* @brief Perform a power off.
*
* This function performs a power off of the core.
*/
void nrf_poweroff(void);

/**
* @brief Prepare to the idle state.
*
* This function applies the required PM configuration before entering the idle state.
*/
void pm_state_enter_idle_prepare(void);

#endif /* _ZEPHYR_SOC_ARM_NORDIC_NRF_POWER_H_ */
Loading
Loading