soc: nordic: common: CONFIG_SOC_NRF_FORCE_CONSTLAT
#96948
Conversation
zephyrbot
added
platform: nRF
zephyrbot
requested review from
anangl,
bjarki-andreasen,
JarmouniA,
jaz1-nordic,
kartben,
kl-cruz,
lstnl,
magp-nordic,
masz-nordic,
nashif and
nika-nordic
| } | ||
|
|
||
| /* Immediately after the SoC init functions */ | ||
| SYS_INIT(nrf_const_lat, PRE_KERNEL_1, 1); |
There was a problem hiding this comment.
Couldn't this code actually be in one of the hooks, to avoid yet another SYS_INIT ? Or perhaps in soc.c?
There was a problem hiding this comment.
I had a look around for an appropriate place and couldn't find one. For soc.c it would need to duplicated for each Nordic SoC. If that is preferred I can do that.
There was a problem hiding this comment.
We need to use on-off service and port Bluetooth Controllers to use them. Otherwise, low power mode would get set after radio events.
JordanYates
dismissed stale reviews from carlescufi, nordicjm, and bjarki-andreasen
via
1d29606
|
Added Bluetooth controller change to address @cvinayak comment. |
| #if defined(CONFIG_NRF_SYS_EVENT) | ||
| (void)nrf_sys_event_request_global_constlat(); | ||
| #else /* !CONFIG_NRF_SYS_EVENT */ |
There was a problem hiding this comment.
I have no expertise here, but the changes look reasonable.
soc/nordic/common/Kconfig
Outdated
| bool "nRF system event support" | ||
| select NRFX_POWER if !NRF_PLATFORM_HALTIUM | ||
|
|
||
| config NRF_FORCE_CONSTANT_LATENCY |
There was a problem hiding this comment.
There is already a feature like that in nRF54L familiy.
zephyr/soc/nordic/nrf54l/Kconfig
Line 79 in 9ae10ea
Can you rename it to SOC_NRF_FORCE_CONSTLAT here and remove the Kconfig from nrf54l/Kconfig.
There was a problem hiding this comment.
I did not know that existed, and based on comments from @cvinayak the option is probably broken when used with a Bluetooth controller
There was a problem hiding this comment.
Dupilicate code from nrf54l/soc.c would need to be removed:
zephyr/soc/nordic/nrf54l/soc.c
Line 142 in 9ae10ea
soc/nordic/common/Kconfig
Outdated
|
|
||
| config NRF_FORCE_CONSTANT_LATENCY | ||
| bool "Force constant latency mode in system ON" | ||
| depends on NRF_SYS_EVENT |
There was a problem hiding this comment.
Imo it should be a select.
There should be depends on !SOC_NRF54H20 && !RISCV
There was a problem hiding this comment.
NRF_SYS_EVENT is available to the NRF54H20 app and radio cores, so only !RISCV would be helpful.
There was a problem hiding this comment.
!RISCV ? I have this being changed in the register directly from the flpr core and it doesn't seem to cause problems so that condition seems wrong:
/* Switch to constant latency */
//NRF_POWER_NS_BASE
NRF_POWER->TASKS_LOWPWR = 0;
NRF_POWER->TASKS_CONSTLAT = 1;
k_sleep(K_SECONDS(2));
Ah becuase of sys event
There was a problem hiding this comment.
If CONSTLAT is supported on RISCV cores, we should probably extend SYS_EVENT?
Move the option to force constant latency mode outside of nRF54l, since it is an option applicable to most Nordic SoCs. Signed-off-by: Jordan Yates <[email protected]>
Validate that `CONFIG_SOC_NRF_FORCE_CONSTLAT=y` compiles. Signed-off-by: Jordan Yates <[email protected]>
When `CONFIG_NRF_SYS_EVENT` is enabled, route constant latency requests through the reference counted API. Signed-off-by: Jordan Yates <[email protected]>
JordanYates
dismissed stale reviews from cvinayak and bjarki-andreasen
via
88a5a53
JordanYates
force-pushed
the
251003_nrf_constlat
branch
from
1d29606 to
88a5a53
Compare
JordanYates
changed the title
CONSTLAT kconfigCONFIG_SOC_NRF_FORCE_CONSTLAT
|
Updated PR to pull the already existing option out to the common level, so current users on nRF54L won't see any changes. |
|
Move the option to force constant latency mode outside of nRF54l, since it is an option applicable to most Nordic SoCs.
This is a quality-of-life option to prevent boards/applications from needing to implement the calls themselves.