modules: hal_nordic: Add multi-instance DPPI and PPIB drivers #79857

e-rk · 2024-10-15T11:16:59Z

Added multi-instance DPPI driver and a new PPIB driver.

zephyrbot · 2024-10-15T11:17:55Z

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_nordic	zephyrproject-rtos/hal_nordic@`5c8d109`	zephyrproject-rtos/hal_nordic@`2dbb2ed` (`master`)	zephyrproject-rtos/[email protected]

✅ All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

nika-nordic · 2024-10-17T10:46:31Z

can you switch to zephyrproject-rtos/hal_nordic#232 ?

The new nrfx_ppib driver can now be enabled, when the corrensponding device tree node has the okay status. Upstream PR: zephyrproject-rtos/zephyr#79857 Signed-off-by: Rafał Kuźnia <[email protected]>

The resource reservation definitions were moved to a separate header file. The PPIB and DPPI channel and group resources can now be statically allocated for each individual instance. Upstream PR: zephyrproject-rtos/zephyr#79857 Signed-off-by: Rafał Kuźnia <[email protected]>

The nrfx_gppi module is an abstraction over nrfx_ppi and nrfx_dppi drivers. It now has a Kconfig option that is separate from nrfx_dppi and by default it enables all PPI/DPPI instances, if available. Upstream PR: zephyrproject-rtos/zephyr#79857 Signed-off-by: Rafał Kuźnia <[email protected]>

ankuns · 2024-11-18T07:01:06Z

modules/hal_nordic/nrfx/nrfx_config_reserved_resources.h

What about replacing LUMOS_XXAA with NRF54L_SERIES

@nika-nordic should LUMOX_XXAA or NRF54L_SERIES be preferred?

NRF54L_SERIES is definitely better.

I'll make a follow-up commit after this PR is merged.

ankuns · 2024-11-18T07:04:56Z

modules/hal_nordic/nrfx/nrfx_config_reserved_resources.h

Please have a look here
https://github.com/nrfconnect/sdk-nrfxlib/blob/main/softdevice_controller/include/sdc_soc.h

Shouldn't it be here

#include <sdc_soc.h>

?
Because of that shouldn't we use macros SDC_xxxx_CHANNELS_USED_MASK when defining
NRFX_xxxx_CHANNELS_USED_BY_xxxx below ?
https://github.com/nrfconnect/sdk-nrfxlib/blob/main/softdevice_controller/include/sdc_soc.h#L40-L43

This won't be added in upstream.

ankuns · 2024-11-18T07:13:50Z

modules/hal_nordic/nrfx/nrfx_config_reserved_resources.h

What about including
https://github.com/nrfconnect/sdk-nrfxlib/blob/main/mpsl/include/mpsl.h
and providing
NRFX_xxxxxx_CHANNELS_USED_BY_MPSL based on MPSL_xxxx_CHANNELS_USED_MASK
https://github.com/nrfconnect/sdk-nrfxlib/blob/main/mpsl/include/mpsl.h#L51-L54

This won't be added in upstream.

The hal_nordic revision was updated to bring in NRFX v3.8.0. Aligned the uses of single-instance API to use multi-instance instead. Upstream PR: zephyrproject-rtos/zephyr#79857 Signed-off-by: Rafał Kuźnia <[email protected]>

Added a binding description for the PPIB peripheral and added the device tree nodes of the PPIB instances to the nRF54L15 and nRF54L20. Upstream PR: zephyrproject-rtos/zephyr#79857 Signed-off-by: Rafał Kuźnia <[email protected]>

The new nrfx_ppib driver can now be enabled, when the corrensponding device tree node has the okay status. Upstream PR: zephyrproject-rtos/zephyr#79857 Signed-off-by: Rafał Kuźnia <[email protected]>

The resource reservation definitions were moved to a separate header file. The PPIB and DPPI channel and group resources can now be statically allocated for each individual instance. Upstream PR: zephyrproject-rtos/zephyr#79857 Signed-off-by: Rafał Kuźnia <[email protected]>

The nrfx_gppi module is an abstraction over nrfx_ppi and nrfx_dppi drivers. It now has a Kconfig option that is separate from nrfx_dppi and by default it enables all PPI/DPPI instances, if available. Upstream PR: zephyrproject-rtos/zephyr#79857 Signed-off-by: Rafał Kuźnia <[email protected]>

nika-nordic · 2024-11-18T10:54:56Z

@e-rk zephyrproject-rtos/hal_nordic#232 merged, please update SHA

The hal_nordic revision was updated to bring in NRFX v3.8.0. Aligned the uses of single-instance API to use multi-instance instead. Signed-off-by: Rafał Kuźnia <[email protected]>

Added a binding description for the PPIB peripheral and added the device tree nodes of the PPIB instances to the nRF54L15 and nRF54L20. Signed-off-by: Rafał Kuźnia <[email protected]>

The new nrfx_ppib driver can now be enabled, when the corrensponding device tree node has the okay status. Signed-off-by: Rafał Kuźnia <[email protected]>

The resource reservation definitions were moved to a separate header file. The PPIB and DPPI channel and group resources can now be statically allocated for each individual instance. Signed-off-by: Rafał Kuźnia <[email protected]>

The nrfx_gppi module is an abstraction over nrfx_ppi and nrfx_dppi drivers. It now has a Kconfig option that is separate from nrfx_dppi and by default it enables all PPI/DPPI instances, if available. Signed-off-by: Rafał Kuźnia <[email protected]>

The nrfx_gppi module is an abstraction over nrfx_ppi and nrfx_dppi drivers. It now has a Kconfig option that is separate from nrfx_dppi and by default it enables all PPI/DPPI instances, if available. Upstream PR: zephyrproject-rtos/zephyr#79857 Signed-off-by: Rafał Kuźnia <[email protected]>

anangl

LGTM, a few nitpicking comments only.

anangl · 2024-11-19T10:13:22Z

soc/nordic/nrf53/sync_rtc.c

 /* Free DPPI and RTC channels */
 static void free_resources(union rtc_sync_channels channels)
 {
+	nrfx_dppi_t dppi = NRFX_DPPI_INSTANCE(0);


Why not define this as static const at the top of the file?

This approach was suggested here: nrfconnect/sdk-nrf#18073 (comment)

I don't quite get how that justification given in the above comment applies to this case, but my point is that static const would cause that this structure ends up in flash, so this would give some savings on its initialization that now needs to be performed at runtime and twice in this file. But it's just a detail. I'm not going to insist on changing this.

anangl · 2024-11-19T10:16:35Z

drivers/counter/counter_nrfx_rtc.c

 	nrfy_rtc_event_enable(rtc, NRF_RTC_CHANNEL_INT_MASK(chan));
 #ifdef DPPI_PRESENT
-	result = nrfx_dppi_channel_alloc(&data->ppi_ch);
+	nrfx_dppi_t dppi = NRFX_DPPI_INSTANCE(0);


I think it would be nice to add a comment that instance 0 is used here because COUNTER_RTC_WITH_PPI_WRAP is currently only supported for nRF51/52/53/91 Series.

I'll try to add this in a follow up commit.

anangl · 2024-11-19T10:25:33Z

modules/hal_nordic/nrfx/Kconfig

+
+config NRFX_DPPI0
+	bool "DPPI0 driver instance"
+	depends on $(dt_nodelabel_has_compat,dppic,$(DT_COMPAT_NORDIC_NRF_DPPIC))


I think that for consistency it would be good to use ddpic0 here and add such node label to nodes labeled as dppic, like it is done for wdt0.

I'm reluctant to changing labels of nodes that existed for years, as it might break compatibility somewhere for no good reason really.

You can extend the existing nodelabel like this:

dppic0: dppic: dppic { ... };

in devicetree and update NRFX_DPPI0
to

depends on ($(dt_nodelabel_has_compat,dppic,$(DT_COMPAT_NORDIC_NRF_DPPIC)) || \ $(dt_nodelabel_has_compat,dppic0,$(DT_COMPAT_NORDIC_NRF_DPPIC)))

to keep backward compatibility

I haven't suggested changing a label but adding another one: dppic: dppic0: dppic@....

I'll address this in a follow up commit then. I want to avoid adding more changes here, as it will block further activities related to nrfx v3.9.

anangl · 2024-11-19T11:06:18Z

modules/hal_nordic/nrfx/CMakeLists.txt

 zephyr_library_sources(${HELPERS_DIR}/nrfx_flag32_allocator.c)
 zephyr_library_sources_ifdef(CONFIG_RETAINED_MEM_NRF_RAM_CTRL ${HELPERS_DIR}/nrfx_ram_ctrl.c)
-zephyr_library_sources_ifdef(CONFIG_NRFX_DPPI                 ${HELPERS_DIR}/nrfx_gppi_dppi.c)
+zephyr_library_sources_ifdef(CONFIG_NRFX_GPPI                 ${HELPERS_DIR}/nrfx_gppi_dppi.c)


This does not seem to be necessary/beneficial.

Can you elaborate? Do you mean that CONFIG_NRFX_GPPI kconfig is not beneficial or something else?

NRFX_GPPI enables NRFX_DPPI, so the change is not really needed. But it causes that nrfx_gppi_dppi.c will be compiled also for NRFX_PPI, unnecessarily.

It is possible to enable NRFX_DPPI without enabling any GPPI functionality now. Also NRFX_GPPI does more, as it is unified across the SoC families. Regardless of whether working with nRF52 or nRF54L or anything else, NRFX_GPPI will enable everything that's needed.

I just noticed that that NRFX_PPI in the next line should be replaced with NRFX_GPPI as well. I'll add this subsequent commit as well, if you don't mind.

anangl · 2024-11-19T11:08:22Z

modules/hal_nordic/nrfx/nrfx_config_reserved_resources.h

NRF54L_SERIES is definitely better.

e-rk requested a review from anangl as a code owner October 15, 2024 11:17

zephyrbot added area: Counter platform: nRF Nordic nRFx labels Oct 15, 2024

zephyrbot requested review from hubertmis, jaz1-nordic, kl-cruz, krish2718, magp-nordic, masz-nordic, nika-nordic, nordic-krch and sachinthegreen October 15, 2024 11:17

zephyrbot assigned anangl Oct 15, 2024

zephyrbot added manifest manifest-hal_nordic DNM This PR should not be merged (Do Not Merge) labels Oct 15, 2024

e-rk force-pushed the dppi branch from 49d84fb to 609866b Compare October 17, 2024 11:05

e-rk changed the title ~~modules: hal_nordic: test commit, not for review.~~ modules: hal_nordic: Add multi-instance DPPI and PPIB drivers Oct 17, 2024

e-rk force-pushed the dppi branch 10 times, most recently from 40c536f to d13657a Compare October 21, 2024 09:40

e-rk force-pushed the dppi branch from 3b243ad to 140be8d Compare November 15, 2024 09:08

ankuns reviewed Nov 18, 2024

View reviewed changes

e-rk added 5 commits November 18, 2024 11:56

manifest: update hal_nordic revision

7d0bb13

The hal_nordic revision was updated to bring in NRFX v3.8.0. Aligned the uses of single-instance API to use multi-instance instead. Signed-off-by: Rafał Kuźnia <[email protected]>

dts: nordic: 54l: Add PPIB device tree nodes and bindings

5493079

Added a binding description for the PPIB peripheral and added the device tree nodes of the PPIB instances to the nRF54L15 and nRF54L20. Signed-off-by: Rafał Kuźnia <[email protected]>

modules: hal_nordic: Enable nrfx_ppib drivers

9e1fe34

The new nrfx_ppib driver can now be enabled, when the corrensponding device tree node has the okay status. Signed-off-by: Rafał Kuźnia <[email protected]>

modules: hal_nordic: rework resource reservations

ab861fd

The resource reservation definitions were moved to a separate header file. The PPIB and DPPI channel and group resources can now be statically allocated for each individual instance. Signed-off-by: Rafał Kuźnia <[email protected]>

e-rk force-pushed the dppi branch from 140be8d to 895b8bc Compare November 18, 2024 10:56

zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Nov 18, 2024

nika-nordic approved these changes Nov 18, 2024

View reviewed changes

nika-nordic requested review from bjarki-andreasen, carlescufi and gmarull November 18, 2024 14:50

carlescufi approved these changes Nov 19, 2024

View reviewed changes

bjarki-andreasen approved these changes Nov 19, 2024

View reviewed changes

anangl approved these changes Nov 19, 2024

View reviewed changes

nika-nordic mentioned this pull request Nov 19, 2024

manifest: integrate nrfx 3.9.0 #81599

Closed

nashif merged commit e184109 into zephyrproject-rtos:main Nov 19, 2024
25 of 26 checks passed

nika-nordic mentioned this pull request Nov 19, 2024

nordic: nRF54L: Add the nRF54L05 and nRF54L10 ICs #80980

Merged

e-rk mentioned this pull request Nov 19, 2024

modules: hal_nordic: multi-instance DPPI minor fixes #81602

Merged

modules: hal_nordic: Add multi-instance DPPI and PPIB drivers #79857

modules: hal_nordic: Add multi-instance DPPI and PPIB drivers #79857

Uh oh!

Conversation

e-rk commented Oct 15, 2024

Uh oh!

zephyrbot commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nika-nordic commented Oct 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nika-nordic commented Nov 18, 2024

Uh oh!

anangl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

zephyrbot commented Oct 15, 2024 •

edited

Loading