Add support for NRF53 and DPPI #18

auroraslb · 2020-12-15T12:24:20Z

The goal of providing HW models for the nrf53 network core is to be able run tests in a simulator with the nrf53 HAL layer.
This will make it easier to find bugs and regressions in the hal layer of a Bluetooth Controller.

The models share the same source code as the models for the nrf52 series, as most HW peripherals are identical.
The same approach can also be used to create models for nrf51 series and similar.

@aescolar, I would like to get some input on how to adapt the makefiles to be able to build a 53-series model in a standalone build environment.
To build for the nrf53-series in zephyr is done here: zephyrproject-rtos/zephyr#30734

carlescufi · 2020-12-15T12:32:22Z

@cvinayak FYI

rugeGerritsen

The DPPI model can be simplified slightly. Added some other comments as well

rugeGerritsen · 2020-12-15T13:09:05Z

CMakeLists.txt

 # CMake file to compile this BabbleSim component as a west module in Zephyr

-if(CONFIG_BOARD_NRF52_BSIM)
+if(CONFIG_BOARD_NRF52_BSIM OR CONFIG_BOARD_NRF53_BSIM)


Maybe we should add a configuration CONFIG_BOARD_BSIM_COMPATIBLE in upstream zephyr so that it becomes easier to support other boards in the future?

rugeGerritsen · 2020-12-15T13:18:31Z

CMakeLists.txt

  zephyr_library_compile_definitions(NO_POSIX_CHEATS)

  file(GLOB_RECURSE HW_MODEL_SRCS . src/*.c)
+  if (CONFIG_BOARD_NRF52_BSIM)


I believe we can replace CONFIG_BOARD_NRF52_BSIM with CONFIG_HAS_HW_NRF_DPPIC

rugeGerritsen · 2020-12-15T13:21:34Z

src/HW_models/NRF_DPPI.h

+#endif
+
+extern NRF_DPPIC_Type NRF_DPPI_regs;
+#define NUMBER_DPPI_CHANNELS 32


You may use DPPI_CH_NUM instead. It is defined in nrf5340_network_peripherals.h

rugeGerritsen · 2020-12-15T13:24:18Z

src/HW_models/core_cm33.h

@@ -0,0 +1,49 @@
+/*


Maybe we should extract out a common file? We can make core_cmxx.h and let core_cm33.h and core_cm4.h include this file?

rugeGerritsen · 2020-12-15T13:26:56Z

src/nrfx/hal/nrf_ccm.c

+    case NRF_CCM_TASK_STOP:
+      nrf_dppi_subscriber_add(channel, nrf_ccm_TASK_STOP);
+      break;
+#if defined(CCM_RATEOVERRIDE_RATEOVERRIDE_Pos) || defined(__NRFX_DOXYGEN__)


No need to add || defined(__NRFX_DOXYGEN__) here. Similar in other places

rugeGerritsen · 2020-12-15T13:33:40Z

src/nrfx/mdk_replacements/nrf_bsim_redef.h

 /*
 * Redefine the base addresses.
 */
+


I would consider splitting this file. @aescolar , what do you think?

rugeGerritsen · 2020-12-15T13:35:46Z

src/HW_models/NRF_DPPI.c

+static subscr_list_elem_t* dppi_subscriber_list[NUMBER_DPPI_CHANNELS];
+
+//Function for checking if an element is in the subscriber linked list
+static bool subscriber_list_check(dest_f_t task_function, subscr_list_elem_t ** pp_head)


No need to use pointer to pointer in this function. You may use
subscriber_in_list(dest_f_t task_function, subscr_list_elem_t * p_head)?

rugeGerritsen · 2020-12-15T13:38:47Z

src/HW_models/NRF_DPPI.c

+{
+  if (!subscriber_list_check(task_function, pp_head))
+  {
+    struct subscr_list_elem* p_elem = (struct subscr_list_elem*) malloc(sizeof(struct subscr_list_elem));


Strictly speaking, you should check the return code of the malloc

rugeGerritsen · 2020-12-15T13:42:45Z

src/HW_models/NRF_DPPI.c

+{
+  if(subscriber_list_check(task_function, pp_head))
+  {
+    struct subscr_list_elem* temp = (struct subscr_list_elem*) malloc(sizeof(struct subscr_list_elem));


You don't need this malloc and the free's below. The temp variable is assigned in the line below, so the new memory is never used. There are some similar cases in functions below.

rugeGerritsen · 2020-12-15T13:45:25Z

src/HW_models/NRF_DPPI.c

+/**
+ * Initialize the DPPI model
+ */
+void nrf_dppi_init()


The function signature should be explicit to indicate that the function takes no parameters

Suggested change

void nrf_dppi_init()

void nrf_dppi_init(void)

aescolar · 2020-12-15T16:38:20Z

@wopu-ot FYI

aescolar

A couple of general comments before anything else:

It is a risky choice to throw yourselves into a mayor change, without agreeing the direction first. (Meaning you risk spending a lot of time writing code which will be scrapped).
For mayor PRs, or mayor rework, it would be very nice if you gave a heads up, so we knew what is coming.

Before dwelling into details:

It would be good to talk and understand what you want to do overall, and agree the general direction.
That code does not account compiling without Zephyr (it does not compile without it)

(I just browsed very fast thru the code. The 4 things I pointed are in the only function I read carefully)

aescolar · 2020-12-15T16:49:42Z

src/HW_models/NRF_DPPI.c

+ */
+void nrf_dppi_publish(uint8_t channel)
+{
+  struct subscr_list_elem* temp = (struct subscr_list_elem*) malloc(sizeof(struct subscr_list_elem));


that's a memory leak :) (that malloc is not needed)

aescolar · 2020-12-15T16:50:04Z

src/HW_models/NRF_DPPI.c

+    temp = temp->p_next;
+  }
+
+  free(temp);


this free is not needed (nor safe)

aescolar · 2020-12-15T16:50:09Z

src/HW_models/NRF_DPPI.c

+  while (temp != NULL)
+  {
+    temp->task_function();
+    prev = temp;


aescolar · 2020-12-15T16:50:15Z

src/HW_models/NRF_DPPI.c

+{
+  struct subscr_list_elem* temp = (struct subscr_list_elem*) malloc(sizeof(struct subscr_list_elem));
+  temp = dppi_subscriber_list[channel];
+  subscr_list_elem_t *prev;


auroraslb · 2020-12-16T07:31:18Z

The goal of providing nrf53_bsim is to be able run tests in a simulator with the nrf53 HAL layer. This is currently not possible. We have had some bugs that would like have been uncovered if we had used nrf53_bsim

aescolar · 2020-12-16T07:48:52Z

The goal of providing nrf53_bsim is to be able ..

Thanks :)

Add DPPI in order to be able to model the NRF53 HW. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

Changes in CMakeLists, NRF_HW_model_top.c, nrfx_common.c and nrf_bsim_redef.h in order for the model to support both the NRF52 and the NRF53 series. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

NRF53 runs on cm33. Add this core in order for the model to support NRF53. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

Add irq sources for NRF53. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

Add use of DPPI to AAR if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

Add use of DPPI to CCM if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

Add use of DPPI to ECB if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

Add use of DPPI to CLOCK if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

Add use of DPPI to RADIO if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

Add use of DPPI to RNG if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

Add use of DPPI to TIMER if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

Add use of DPPI to RTC if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

Add a common core-file for easier maintenance. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

wopu-ot · 2021-05-04T11:47:37Z

@auroraslb Do you still want to proceed with this PR or should I close it?

rugeGerritsen · 2021-05-05T06:24:56Z

@auroraslb Do you still want to proceed with this PR or should I close it?

@wopu-ot , we can close this PR for now and reopen later if needed.

carlescufi requested review from aescolar, anangl and rugeGerritsen December 15, 2020 12:32

rugeGerritsen suggested changes Dec 15, 2020

View reviewed changes

aescolar suggested changes Dec 15, 2020

View reviewed changes

auroraslb added 13 commits February 10, 2021 16:06

dppi: Add NRF_DPPI.h, NRF_DPPI.c and nrf_dppi.c files

adb60c0

Add DPPI in order to be able to model the NRF53 HW. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

nrf5x: Add support for both NRF52 and NRF53

d791e73

Changes in CMakeLists, NRF_HW_model_top.c, nrfx_common.c and nrf_bsim_redef.h in order for the model to support both the NRF52 and the NRF53 series. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

HW models: Add core_cm33.h

94988e5

NRF53 runs on cm33. Add this core in order for the model to support NRF53. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

HW models: irq: Add irq sources for nrf53

5cc4de2

Add irq sources for NRF53. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

aar: Add support for DPPI to AAR

77138fd

Add use of DPPI to AAR if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

ccm: Add support for DPPI to CCM

f8ec2ba

Add use of DPPI to CCM if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

ecb: Add support for DPPI to ECB

0411656

Add use of DPPI to ECB if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

clock: Add support for DPPI to CLOCK

96cbfc4

Add use of DPPI to CLOCK if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

radio: Add support for DPPI to RADIO

f5dc3a8

Add use of DPPI to RADIO if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

rng: Add support for DPPI to RNG

d30478b

Add use of DPPI to RNG if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

timer: Add support for DPPI to TIMER

abb53a0

Add use of DPPI to TIMER if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

rtc: Add support for DPPI to RTC

43afd6c

Add use of DPPI to RTC if DPPI is defined. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

HW models: Add common core-file

2ed9e22

Add a common core-file for easier maintenance. Signed-off-by: Sletnes Bjørlo, Aurora <[email protected]>

auroraslb force-pushed the add_dppi branch from 829ad62 to 2ed9e22 Compare February 24, 2021 08:44

jalgroy mentioned this pull request Feb 24, 2021

Implement native posix or qemu support Simula-UiB/mesh-node#10

Open

aescolar closed this May 5, 2021

Add support for NRF53 and DPPI #18

Add support for NRF53 and DPPI #18

Uh oh!

Conversation

auroraslb commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlescufi commented Dec 15, 2020

Uh oh!

rugeGerritsen 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!

aescolar commented Dec 15, 2020

Uh oh!

aescolar 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!

auroraslb commented Dec 16, 2020

Uh oh!

aescolar commented Dec 16, 2020

Uh oh!

wopu-ot commented May 4, 2021

Uh oh!

rugeGerritsen commented May 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

auroraslb commented Dec 15, 2020 •

edited

Loading