modules: hal_nordic: nrfx: update API version to 3.2.0 #64476

nika-nordic · 2023-10-27T07:32:47Z

Updated API version enables multi-instance GPIOTE driver.

Additionally obsolete symbol that was used to specify API version in the past was removed.

nika-nordic · 2023-11-08T09:09:04Z

@anangl CI seems to be green, can we proceed with review and merging?

nika-nordic · 2023-12-01T09:44:03Z

Rebased the PR. Hopefully now the unrelated:

FAILED: zephyr/subsys/net/CMakeFiles/subsys__net.dir/lib/tls_credentials/tls_credentials_trusted.c.obj
ccache /opt/toolchains/zephyr-sdk-0.16.3/arm-zephyr-eabi/bin/arm-zephyr-eabi-gcc -DKERNEL -DNRF5340_XXAA_APPLICATION -DNRF_SKIP_FICR_NS_COPY_TO_RAM -DNRF_TRUSTZONE_NONSECURE -DPICOLIBC_LONG_LONG_PRINTF_SCANF -DTC_RUNID=9473c3a39eb23922342372ed882ae5f4 -D_FORTIFY_SOURCE=1 -D_POSIX_C_SOURCE=200809 -D__LINUX_ERRNO_EXTENSIONS__ -D__PROGRAM_START -D__ZEPHYR__=1 -I/__w/zephyr/zephyr/subsys/net/lib/tls_credentials/. -I/__w/zephyr/zephyr/include -I/__w/zephyr/zephyr/twister-out/thingy53_nrf5340_cpuapp_ns/tests/net/lib/tls_credentials/net.tls_credentials.trusted_tfm/zephyr/include/generated -I/__w/zephyr/zephyr/soc/arm/nordic_nrf/nrf53 -I/__w/zephyr/zephyr/soc/arm/nordic_nrf/common/. -I/__w/zephyr/zephyr/subsys/testsuite/include -I/__w/zephyr/zephyr/subsys/testsuite/ztest/include -I/__w/zephyr/zephyr/subsys/usb/device -I/__w/zephyr/zephyr/subsys/net/lib/dns/. -I/__w/zephyr/zephyr/drivers/usb/common/nrf_usbd_common/. -I/__w/zephyr/modules/hal/cmsis/CMSIS/Core/Include -I/__w/zephyr/zephyr/modules/cmsis/. -I/__w/zephyr/modules/hal/nordic/nrfx -I/__w/zephyr/modules/hal/nordic/nrfx/drivers/include -I/__w/zephyr/modules/hal/nordic/nrfx/mdk -I/__w/zephyr/zephyr/modules/hal_nordic/nrfx/. -I/__w/zephyr/zephyr/twister-out/thingy53_nrf5340_cpuapp_ns/tests/net/lib/tls_credentials/net.tls_credentials.trusted_tfm/tfm/generated/interface/include -isystem /__w/zephyr/zephyr/lib/libc/common/include -Wshadow -fno-strict-aliasing -Werror -Os -imacros /__w/zephyr/zephyr/twister-out/thingy53_nrf5340_cpuapp_ns/tests/net/lib/tls_credentials/net.tls_credentials.trusted_tfm/zephyr/include/generated/autoconf.h -fno-printf-return-value -fno-common -g -gdwarf-4 -fdiagnostics-color=always -mcpu=cortex-m33 -mthumb -mabi=aapcs -mfp16-format=ieee -mtp=soft --sysroot=/opt/toolchains/zephyr-sdk-0.16.3/arm-zephyr-eabi/arm-zephyr-eabi -imacros /__w/zephyr/zephyr/include/zephyr/toolchain/zephyr_stdint.h -Wall -Wformat -Wformat-security -Wno-format-zero-length -Wno-pointer-sign -Wpointer-arith -Wexpansion-to-defined -Wno-unused-but-set-variable -Werror=implicit-int -fno-pic -fno-pie -fno-asynchronous-unwind-tables -ftls-model=local-exec -fno-reorder-functions --param=min-pagesize=0 -fno-defer-pop -fmacro-prefix-map=/__w/zephyr/zephyr/tests/net/lib/tls_credentials=CMAKE_SOURCE_DIR -fmacro-prefix-map=/__w/zephyr/zephyr=ZEPHYR_BASE -fmacro-prefix-map=/__w/zephyr=WEST_TOPDIR -ffunction-sections -fdata-sections --specs=picolibc.specs -std=c99 -MD -MT zephyr/subsys/net/CMakeFiles/subsys__net.dir/lib/tls_credentials/tls_credentials_trusted.c.obj -MF zephyr/subsys/net/CMakeFiles/subsys__net.dir/lib/tls_credentials/tls_credentials_trusted.c.obj.d -o zephyr/subsys/net/CMakeFiles/subsys__net.dir/lib/tls_credentials/tls_credentials_trusted.c.obj -c /__w/zephyr/zephyr/subsys/net/lib/tls_credentials/tls_credentials_trusted.c
/__w/zephyr/zephyr/subsys/net/lib/tls_credentials/tls_credentials_trusted.c:12:10: fatal error: psa/protected_storage.h: No such file or directory
12 | #include <psa/protected_storage.h>

will be fixed

anangl · 2023-12-01T09:48:25Z

modules/hal_nordic/nrfx/nrfx_config_common.h

@@ -18,7 +18,7 @@

 /** @brief Symbol specifying minor version of the nrfx API to be used. */
 #ifndef NRFX_CONFIG_API_VER_MINOR
-#define NRFX_CONFIG_API_VER_MINOR 0
+#define NRFX_CONFIG_API_VER_MINOR 2


To preserve bisectability, it would be good to make all related changes in drivers in the same commit.

That would make that one commit huge, combining multiple areas - is it really better?

Yes, because otherwise the code does not build. Any commit that does not build is significantly slowing down bisect because you have to skip one, or more, commits which significantly increases the required number of checks. The required number of checks, when all commits in history do build correctly, is log2(N) where N is the number of commits in between good and bad revisions.

The commits should really be:

Update nrfx API version with all drivers adjusted for new API. No actual functional changes (if possible).

Any subsequent commits are the functional changes related to things that were not possible with earlier nrfx version.

anangl · 2023-12-01T09:50:34Z

drivers/adc/adc_nrfx_saadc.c

@@ -7,6 +7,7 @@
 #define ADC_CONTEXT_USES_KERNEL_TIMER
 #include "adc_context.h"
 #include <hal/nrf_saadc.h>
+#include <nrfx_saadc.h>


The nrfx_saadc driver is not used here, so it's a bit weird and misleading to include this header just to use one macro that is defined there. Also the commit message is misleading.

NRFX_SAADC_SAMPLES_TO_BYTES is used.

Yes, and in my opinion this is wrong.

Dropped whole SAADC

anangl · 2023-12-01T09:59:40Z

drivers/adc/adc_nrfx_saadc.c

@@ -17,6 +17,7 @@ LOG_MODULE_REGISTER(adc_nrfx_saadc);

 #define DT_DRV_COMPAT nordic_nrf_saadc

+#if !(NRF_SAADC_HAS_AIN_AS_PIN)


The changes in adc_nrfx_saadc.c are not related to the API update to 3.2.0. I think it would be better to process them in a separate PR.

Dropped whole SAADC

anangl · 2023-12-01T10:05:06Z

modules/hal_nordic/nrfx/nrfx_config_nrf5340_application.h

-#if defined(NRF_TRUSTZONE_NONSECURE)
-#define NRF_GPIOTE1      NRF_GPIOTE1_NS
-#else
+#if !defined(NRF_TRUSTZONE_NONSECURE)
 #define NRF_CACHE        NRF_CACHE_S
 #define NRF_CACHEINFO    NRF_CACHEINFO_S
 #define NRF_CACHEDATA    NRF_CACHEDATA_S
 #define NRF_CRYPTOCELL   NRF_CRYPTOCELL_S
 #define NRF_CTI          NRF_CTI_S
 #define NRF_FICR         NRF_FICR_S
-#define NRF_GPIOTE0      NRF_GPIOTE0_S


These changes should be dropped, for nRF91 as well.

anangl · 2023-12-01T10:16:42Z

boards/arm/nrf5340_audio_dk_nrf5340/nrf5340_audio_dk_cpunet_reset.c

+#define BUTTON_PORT    DT_GPIO_CTLR(DT_ALIAS(sw0), gpios)
+#define GPIOTE_PHANDLE DT_PHANDLE(BUTTON_PORT, gpiote_instance)
+#define GPIOTE_IDX     DT_PROP(GPIOTE_PHANDLE, instance)


The code below is not related to the sw0 button so these changes in the current form are misleading. I'd propose to use instead:

Suggested change

#define BUTTON_PORT DT_GPIO_CTLR(DT_ALIAS(sw0), gpios)

#define GPIOTE_PHANDLE DT_PHANDLE(BUTTON_PORT, gpiote_instance)

#define GPIOTE_IDX DT_PROP(GPIOTE_PHANDLE, instance)

#define GPIOTE_IDX DT_PROP(DT_NODELABEL(gpiote), instance)

anangl · 2023-12-01T10:30:36Z

soc/arm/nordic_nrf/validate_base_addresses.c

-CHECK_DT_REG(gpiote, NRF_GPIOTE);
+#if defined(NRF_GPIOTE)
+CHECK_DT_REG(gpiote0, NRF_GPIOTE);
+#else
+CHECK_DT_REG(gpiote0, NRF_GPIOTE0);
+#endif


Why?
IMO it should be:

Suggested change

CHECK_DT_REG(gpiote, NRF_GPIOTE);

#if defined(NRF_GPIOTE)

CHECK_DT_REG(gpiote0, NRF_GPIOTE);

#else

CHECK_DT_REG(gpiote0, NRF_GPIOTE0);

#endif

CHECK_DT_REG(gpiote, NRF_GPIOTE);

CHECK_DT_REG(gpiote0, NRF_GPIOTE0);

with

#if !defined(NRF_GPIOTE0) && defined(NRF_GPIOTE) #define NRF_GPIOTE0 NRF_GPIOTE #endif

added at the top of the file.

This still causes compilation error on nrf5340_cpuapp as it has both gpiote and gpiote0 nodes defined, but only NRF_GPIOTE0. Adding #define NRF_GPIOTE NRF_GPIOTE0 would fix it but I'm not sure if this is a better solution than what we have now.

anangl · 2023-12-01T10:45:03Z

samples/boards/nrf/nrfx/src/main.c

+#define INPUT_PIN      DT_GPIO_PIN(DT_ALIAS(sw0), gpios)
+#define INPUT_PORT     DT_GPIO_CTLR(DT_ALIAS(sw0), gpios)
+#define GPIOTE_PHANDLE DT_PHANDLE(INPUT_PORT, gpiote_instance)
+#define GPIOTE_IDX     DT_PROP(GPIOTE_PHANDLE, instance)


This does not make much sense as the sample will anyway work only with GPIOTE0 (only this one is enabled in Kconfig). Since only nrf52840dk_nrf52840 and nrf9160dk_nrf9160 are allowed platforms here, I'd rather go with hardcoded instance 0 in the code.

Reworked Kconfig to take into account any GPIOTE instance.

anangl · 2023-12-01T10:54:25Z

drivers/pwm/pwm_nrf_sw.c

+#define GPIO_NODE(pin)		    DT_GPIO_CTLR_BY_IDX(DT_DRV_INST(0), channel_gpios, pin)
+#define GPIOTE_PHANDLE(pin)	    DT_PHANDLE(GPIO_NODE(pin), gpiote_instance)
+#define GPIOTE_IDX(pin)		    DT_PROP(GPIOTE_PHANDLE(pin), instance)


Suggested change

#define GPIO_NODE(pin) DT_GPIO_CTLR_BY_IDX(DT_DRV_INST(0), channel_gpios, pin)

#define GPIOTE_PHANDLE(pin) DT_PHANDLE(GPIO_NODE(pin), gpiote_instance)

#define GPIOTE_IDX(pin) DT_PROP(GPIOTE_PHANDLE(pin), instance)

#define GPIO_NODE(channel) DT_GPIO_CTLR_BY_IDX(DT_DRV_INST(0), \

channel_gpios, channel)

#define GPIOTE_PHANDLE(channel) DT_PHANDLE(GPIO_NODE(channel), gpiote_instance)

#define GPIOTE_IDX(channel) DT_PROP(GPIOTE_PHANDLE(channel), instance)

Removed unnecessary whitespaces, but breaking line 60 is not needed.

anangl · 2023-12-01T11:10:18Z

drivers/display/display_nrf_led_matrix.c

+#define GPIOTE_PHANDLE(pin) DT_PHANDLE(GPIO_NODE(pin), gpiote_instance)
+#define GPIOTE_IDX(pin)     DT_PROP(GPIOTE_PHANDLE(pin), instance)
+
+static nrfx_gpiote_t gpiote = NRFX_GPIOTE_INSTANCE(GPIOTE_IDX(col_gpios));


This should not assume a single GPIOTE instance. I think that a similar scheme as in pwm_nrf_sw.c should be used instead.

anangl · 2023-12-01T11:21:04Z

drivers/spi/spi_nrfx_spim.c

 };

+#ifdef CONFIG_SOC_NRF52832_ALLOW_SPIM_DESPITE_PAN_58
+static const nrfx_gpiote_t gpiote = NRFX_GPIOTE_INSTANCE(0);


It would be better to move this to anomaly_58_workaround_init() as it is needed only there. In other places NRF_GPIOTE can be used as it is now.

modules/hal_nordic/nrfx/nrfx_config_nrf5340_application.h

boards/arm/nrf5340_audio_dk_nrf5340/nrf5340_audio_dk_cpunet_reset.c

dts/arm/nordic/nrf5340_cpuapp.dtsi

drivers/spi/spi_nrfx_common.h

samples/boards/nrf/nrfx/Kconfig

drivers/display/display_nrf_led_matrix.c

samples/boards/nrf/nrfx/Kconfig

scripts/kconfig/kconfigfunctions.py

soc/arm/nordic_nrf/common/soc_nrf_common.h

subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.c

subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_nrf5_dppi.h

samples/boards/nrf/nrfx/src/main.c

Change the GPIOTE driver to HAL to prevent instantiation issues with a multi-instance GPIOTE driver. Signed-off-by: Jakub Zymelka <[email protected]>

drivers/display/display_nrf_led_matrix.c

drivers/gpio/gpio_nrfx.c

soc/arm/nordic_nrf/common/soc_nrf_common.h

drivers/spi/spi_nrfx_spi.c

drivers/display/display_nrf_led_matrix.c

dts/bindings/gpio/nordic,nrf-gpio.yaml

dts/bindings/gpio/nordic,nrf-gpiote.yaml

soc/arm/nordic_nrf/common/soc_nrf_common.h

Added GPIOTE0, GPIOTE1 instances for legacy devices, GPIOTE20, GPIOTE30 for Moonlight and GPIOTE130, GPIOTE131 instances for Haltium. Signed-off-by: Jakub Zymelka <[email protected]>

After adding new GPIOTE instances, there is a need to enable the instance for individual boards. Signed-off-by: Jakub Zymelka <[email protected]>

Updated API version enables multi-instance GPIOTE driver. Additionally obsolete symbol that was used to specify API version in the past was removed. Affected drivers have been adjusted and appropriate changes in affected files have been made. Signed-off-by: Jakub Zymelka <[email protected]>

nika-nordic requested a review from nashif as a code owner October 27, 2023 07:32

nika-nordic requested review from carlescufi and anangl October 27, 2023 07:32

zephyrbot added the platform: nRF Nordic nRFx label Oct 27, 2023

zephyrbot requested review from hubertmis and nordic-krch October 27, 2023 07:33

zephyrbot assigned anangl Oct 27, 2023

nika-nordic force-pushed the update_nrfx_api_to_3_2_0 branch from c949017 to 2a755b3 Compare October 27, 2023 14:05

zephyrbot added the area: ADC Analog-to-Digital Converter (ADC) label Oct 27, 2023

jaz1-nordic requested review from cvinayak, thoh-ot, kruithofa, tbursztyka, mnkp, MaureenHelm and galak as code owners November 6, 2023 08:45

zephyrbot requested review from greg-leach, mtpr-ot and rerickson1 November 6, 2023 08:46

jaz1-nordic force-pushed the update_nrfx_api_to_3_2_0 branch 2 times, most recently from 6f91599 to 8b47ff6 Compare November 7, 2023 12:43

masz-nordic force-pushed the update_nrfx_api_to_3_2_0 branch 4 times, most recently from ebe5cce to 0dfdbd2 Compare November 27, 2023 15:05

nika-nordic force-pushed the update_nrfx_api_to_3_2_0 branch from 0dfdbd2 to 7eca915 Compare December 1, 2023 09:43

anangl requested changes Dec 1, 2023

View reviewed changes

masz-nordic force-pushed the update_nrfx_api_to_3_2_0 branch from 7eca915 to e93e113 Compare December 7, 2023 12:45

masz-nordic force-pushed the update_nrfx_api_to_3_2_0 branch 3 times, most recently from 2d151ad to f7d640d Compare December 11, 2023 16:11

masz-nordic requested a review from anangl December 11, 2023 16:11

masz-nordic force-pushed the update_nrfx_api_to_3_2_0 branch from f7d640d to 5a2b291 Compare December 13, 2023 14:17

anangl requested changes Jan 3, 2024

View reviewed changes

jaz1-nordic force-pushed the update_nrfx_api_to_3_2_0 branch 2 times, most recently from e6385cd to 8f2a74c Compare January 5, 2024 09:58

anangl reviewed Jan 5, 2024

View reviewed changes

samples/boards/nrf/nrfx/src/main.c Outdated Show resolved Hide resolved

samples/boards/nrf/nrfx/src/main.c Outdated Show resolved Hide resolved

boards: arm: nrf5340_audio_dk_cpunet_reset: switch to HAL

1fab997

Change the GPIOTE driver to HAL to prevent instantiation issues with a multi-instance GPIOTE driver. Signed-off-by: Jakub Zymelka <[email protected]>

jaz1-nordic force-pushed the update_nrfx_api_to_3_2_0 branch from 8f2a74c to 93d3fa6 Compare January 5, 2024 16:56

anangl reviewed Jan 5, 2024

View reviewed changes

drivers/display/display_nrf_led_matrix.c Outdated Show resolved Hide resolved

drivers/gpio/gpio_nrfx.c Outdated Show resolved Hide resolved

soc/arm/nordic_nrf/common/soc_nrf_common.h Outdated Show resolved Hide resolved

anangl reviewed Jan 5, 2024

View reviewed changes

drivers/spi/spi_nrfx_spi.c Outdated Show resolved Hide resolved

jaz1-nordic force-pushed the update_nrfx_api_to_3_2_0 branch 2 times, most recently from 7a980f9 to d655294 Compare January 5, 2024 17:56

anangl reviewed Jan 5, 2024

View reviewed changes

drivers/display/display_nrf_led_matrix.c Outdated Show resolved Hide resolved

anangl reviewed Jan 8, 2024

View reviewed changes

dts/bindings/gpio/nordic,nrf-gpio.yaml Outdated Show resolved Hide resolved

dts/bindings/gpio/nordic,nrf-gpiote.yaml Outdated Show resolved Hide resolved

soc/arm/nordic_nrf/common/soc_nrf_common.h Show resolved Hide resolved

jaz1-nordic added 3 commits January 8, 2024 09:49

dts: arm: add new gpiote instances definition

be8513c

Added GPIOTE0, GPIOTE1 instances for legacy devices, GPIOTE20, GPIOTE30 for Moonlight and GPIOTE130, GPIOTE131 instances for Haltium. Signed-off-by: Jakub Zymelka <[email protected]>

boards: arm: set gpiote status to okay as default

96f4746

After adding new GPIOTE instances, there is a need to enable the instance for individual boards. Signed-off-by: Jakub Zymelka <[email protected]>

nika-nordic force-pushed the update_nrfx_api_to_3_2_0 branch from d655294 to aecd53a Compare January 8, 2024 08:51

anangl approved these changes Jan 8, 2024

View reviewed changes

carlescufi requested review from masz-nordic and tmon-nordic January 8, 2024 09:57

masz-nordic approved these changes Jan 8, 2024

View reviewed changes

carlescufi approved these changes Jan 8, 2024

View reviewed changes

carlescufi merged commit ade49f0 into zephyrproject-rtos:main Jan 8, 2024
24 checks passed

jaz1-nordic deleted the update_nrfx_api_to_3_2_0 branch February 8, 2024 11:54

anangl mentioned this pull request Feb 12, 2024

Several nrfx samples fail to build #68851

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modules: hal_nordic: nrfx: update API version to 3.2.0 #64476

modules: hal_nordic: nrfx: update API version to 3.2.0 #64476

nika-nordic commented Oct 27, 2023

nika-nordic commented Nov 8, 2023

nika-nordic commented Dec 1, 2023

anangl Dec 1, 2023

masz-nordic Dec 7, 2023

tmon-nordic Dec 7, 2023

tmon-nordic Dec 7, 2023

anangl Dec 1, 2023

masz-nordic Dec 1, 2023

anangl Dec 1, 2023

masz-nordic Dec 7, 2023

anangl Dec 1, 2023

masz-nordic Dec 7, 2023

anangl Dec 1, 2023

anangl Dec 1, 2023

masz-nordic Dec 7, 2023

anangl Dec 1, 2023

masz-nordic Dec 7, 2023

anangl Dec 1, 2023

masz-nordic Dec 7, 2023

anangl Dec 1, 2023

masz-nordic Dec 7, 2023

anangl Dec 1, 2023

masz-nordic Dec 11, 2023

anangl Dec 1, 2023

masz-nordic Dec 8, 2023

		@@ -17,6 +17,7 @@ LOG_MODULE_REGISTER(adc_nrfx_saadc);

		#define DT_DRV_COMPAT nordic_nrf_saadc

		#if !(NRF_SAADC_HAS_AIN_AS_PIN)

-#define GPIO_NODE(pin)		    DT_GPIO_CTLR_BY_IDX(DT_DRV_INST(0), channel_gpios, pin)
-#define GPIOTE_PHANDLE(pin)	    DT_PHANDLE(GPIO_NODE(pin), gpiote_instance)
-#define GPIOTE_IDX(pin)		    DT_PROP(GPIOTE_PHANDLE(pin), instance)
+#define GPIO_NODE(channel)	DT_GPIO_CTLR_BY_IDX(DT_DRV_INST(0), \
+						    channel_gpios, channel)
+#define GPIOTE_PHANDLE(channel)	DT_PHANDLE(GPIO_NODE(channel), gpiote_instance)
+#define GPIOTE_IDX(channel)	DT_PROP(GPIOTE_PHANDLE(channel), instance)

modules: hal_nordic: nrfx: update API version to 3.2.0 #64476

modules: hal_nordic: nrfx: update API version to 3.2.0 #64476

Conversation

nika-nordic commented Oct 27, 2023

nika-nordic commented Nov 8, 2023

nika-nordic commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment