drivers: hci: da1469x: Add driver for CMAC core on DA1469x #57951

andrzej-kaczmarek · 2023-05-16T13:47:04Z

This adds HCI driver which enables communication with CMAC core on
Renesas SmartBond(tm) DA1469x series. The CMAC firmware is provided as
a library via hal_renesas along with APIs to communicate via shared
memory. The protocol over shared memory mailboxes is H4 so the code is
based on existing hci_h4 driver.

zephyrbot · 2023-05-16T13:48:08Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_renesas	zephyrproject-rtos/hal_renesas@`e3560c7`	zephyrproject-rtos/hal_renesas@`8390c9d` (main)	zephyrproject-rtos/[email protected]

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

nordicjm · 2023-05-16T13:49:37Z

The controller image is provided in hal_renesas repository

So that's a blob? This needs to follow https://docs.zephyrproject.org/latest/contribute/bin_blobs.html and have TSC approval

andrzej-kaczmarek · 2023-05-17T08:20:22Z

So that's a blob? This needs to follow docs.zephyrproject.org/latest/contribute/bin_blobs.html and have TSC approval

Binary blobs (or blobs for short) are files containing proprietary machine code or data in a binary format, e.g. without corresponding source code released under an OSI approved license.

So if I understand the above correctly it does not apply here since included image is a prebuilt Apache NimBLE controller. The source code is publicly available and licensed under Apache 2.0 license. It's exactly the same image that newt tool builds and links when building Apache Mynewt for DA1469x.

carlescufi · 2023-05-17T09:40:53Z

So that's a blob? This needs to follow docs.zephyrproject.org/latest/contribute/bin_blobs.html and have TSC approval

Binary blobs (or blobs for short) are files containing proprietary machine code or data in a binary format, e.g. without corresponding source code released under an OSI approved license.

So if I understand the above correctly it does not apply here since included image is a prebuilt Apache NimBLE controller. The source code is publicly available and licensed under Apache 2.0 license. It's exactly the same image that newt tool builds and links when building Apache Mynewt for DA1469x.

This is an edge case then. You are still providing a precompiled library in binary form, so we need to at least discuss it in the TSC.

nordicjm · 2023-05-17T09:43:53Z

Note that another vendor has done similar previously and it was submitted as a blob: #54534

andrzej-kaczmarek · 2023-05-17T10:00:18Z

Binary blobs (or blobs for short) are files containing proprietary machine code or data in a binary format, e.g. without corresponding source code released under an OSI approved license.
So if I understand the above correctly it does not apply here since included image is a prebuilt Apache NimBLE controller. The source code is publicly available and licensed under Apache 2.0 license. It's exactly the same image that newt tool builds and links when building Apache Mynewt for DA1469x.

This is an edge case then. You are still providing a precompiled library in binary form, so we need to at least discuss it in the TSC.

Understood. Do you want me to create an issue for binary blob anyway or you will discuss it first as it is now? I can also add some kind of readme to PR in hal_renesas with steps to rebuild that binary.

The reason why we want to provide that prebuilt binary is to make BLE work on DA1469x ootb, otherwise people will need to get complete Mynewt env just to create binary.

carlescufi · 2023-05-17T10:12:22Z

Note that another vendor has done similar previously and it was submitted as a blob: #54534

But I am not sure that the source code for those Infineon libraries was available under an open source license actually, was it? @ifyall @npal-cy

carlescufi · 2023-05-17T10:13:25Z

Understood. Do you want me to create an issue for binary blob anyway or you will discuss it first as it is now? I can also add some kind of readme to PR in hal_renesas with steps to rebuild that binary.

Let's create the issue for a binary blob @andrzej-kaczmarek, because precompiling an open source project is new. If you do this today we can include it in today's TSC.

The reason why we want to provide that prebuilt binary is to make BLE work on DA1469x ootb, otherwise people will need to get complete Mynewt env just to create binary.

I know this is not what you propose, but what about using the Zephyr LL instead?

andrzej-kaczmarek · 2023-05-17T10:42:24Z

Understood. Do you want me to create an issue for binary blob anyway or you will discuss it first as it is now? I can also add some kind of readme to PR in hal_renesas with steps to rebuild that binary.

Let's create the issue for a binary blob @andrzej-kaczmarek, because precompiling an open source project is new. If you do this today we can include it in today's TSC.

Done: #57987

The reason why we want to provide that prebuilt binary is to make BLE work on DA1469x ootb, otherwise people will need to get complete Mynewt env just to create binary.

I know this is not what you propose, but what about using the Zephyr LL instead?

Porting Zephyr LL to CMAC architecture would require lots of work so for now I'm afraid that's not an option.

github-actions · 2023-11-19T00:30:47Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

jhedberg · 2024-04-23T15:20:18Z

@andrzej-kaczmarek please fix the compliance check failures

jhedberg · 2024-04-24T06:21:04Z

drivers/bluetooth/hci/hci_da1469x.c

+	.fifo = Z_FIFO_INITIALIZER(rx.fifo),
+};
+
+static inline void h4_get_type(void)


Unless you've profiled the code and concluded that the compiler doesn't do an optimal job, please don't do these explicit inline declarations. Same goes for the other functions here (I won't comment on them separately).

drivers/bluetooth/hci/hci_da1469x.c

jhedberg · 2024-04-24T06:35:59Z

drivers/bluetooth/hci/hci_da1469x.c

+void cmac_read_req(void)
+{
+	while (cmac_mbox_has_data()) {
+		process_rx();
+	}
+}
+
+void cmac_rng_req(void)
+{
+	k_sem_give(&rng_sem);
+}


Maybe add code comments explaining where the calls to these functions are coming from, since they're not in the main Zephyr tree.

jhedberg · 2024-04-24T06:38:15Z

drivers/bluetooth/hci/hci_da1469x.c

+#ifdef CONFIG_BT_HCI_HOST
+	.close          = bt_da1469x_close,
+#endif


Not directly related to this PR, but it seems a bit arbitrary that close() would depend on the Zephyr host stack. The only other user of the HCI driver API right now. is HCI_RAW, which indeed doesn't use close(), but it seems wrong to me that the HCI driver API makes such assumptions about the higher level implementation that uses the API. I guess we can fix this once we move to a native Zephyr driver API.

@andrzej-kaczmarek could I get some comment here? Drivers shouldn't generally know or care who their user is (the normal bluetooth host stack or HCI_RAW). So I think it might be cleaner to just remove the #ifdefs from close().

tbh I added #ifdefs based on other driver (iirc ipm_stm32wb), so didn't really know if they were required. I'll just remove them.

jhedberg

A few more minor comments. Also, please pay attention to the work done in #72323. This driver will also need to be converted as part of that other PR (assuming this PR gets merged first).

jhedberg · 2024-05-13T10:35:48Z

drivers/bluetooth/hci/hci_da1469x.c

+#define H4_NONE 0x00
+#define H4_CMD  0x01
+#define H4_ACL  0x02
+#define H4_EVT  0x04
+#define H4_ISO  0x05


These are all available in include/zephyr/bluetooth/hci_types.h please use those defines instead.

jhedberg · 2024-05-13T10:36:33Z

drivers/bluetooth/hci/hci_da1469x.c

+
+	ret = cmac_mbox_read(rx.hdr + bytes_read, rx.remaining);
+	if (unlikely(ret < 0)) {
+		LOG_ERR("Unable to read from UART (ret %d)", ret);


"UART" in the log message seems wrong, since you're not calling a UART API

jhedberg · 2024-05-13T10:38:06Z

drivers/bluetooth/hci/hci_da1469x.c

+
+	err = cmac_mbox_read(buf, MIN(len, sizeof(buf)));
+	if (unlikely(err < 0)) {
+		LOG_ERR("Unable to read from UART (err %d)", err);


jhedberg · 2024-05-13T10:38:17Z

drivers/bluetooth/hci/hci_da1469x.c

+
+	read = cmac_mbox_read(net_buf_tail(rx.buf), rx.remaining);
+	if (unlikely(read < 0)) {
+		LOG_ERR("Failed to read UART (err %d)", read);


Pull changes required for CMAC driver. Signed-off-by: Andrzej Kaczmarek <[email protected]>

andrzej-kaczmarek · 2024-05-13T11:48:46Z

west.yml is now updated with proper hash for HAL

jhedberg · 2024-05-15T13:06:44Z

@andrzej-kaczmarek the PR looks generally fine to me. I'd be willing to approve if it wasn't for the open question regarding the #ifdefs around close().

This adds HCI driver which enables communication with CMAC core on Renesas SmartBond DA1469x series. The CMAC core is running an Apache NimBLE controller binary and uses shared memory for communcation via mailboxes. Signed-off-by: Andrzej Kaczmarek <[email protected]>

Adds required defconfigs for BT on DA1469x DK. Signed-off-by: Andrzej Kaczmarek <[email protected]>

blauret · 2024-05-16T13:52:28Z

@jhedberg, I think your last comment was addressed.

andrzej-kaczmarek requested review from alwa-nordic, jhedberg, Vudentz, MaureenHelm, galak and ioannisg as code owners May 16, 2023 13:47

zephyrbot added area: Bluetooth platform: Renesas SmartBond Renesas Electronics Corporation, SmartBond labels May 16, 2023

zephyrbot requested review from asbjornsabo, hermabe, sjanc and Thalley May 16, 2023 13:47

zephyrbot assigned jhedberg May 16, 2023

zephyrbot added manifest manifest-hal_renesas DNM This PR should not be merged (Do Not Merge) labels May 16, 2023

nordicjm requested a review from carlescufi May 16, 2023 13:49

andrzej-kaczmarek force-pushed the da1469x-cmac branch from dd002b4 to edd337f Compare May 16, 2023 13:54

carlescufi added the TSC Topics that need TSC discussion label May 17, 2023

andrzej-kaczmarek force-pushed the da1469x-cmac branch 2 times, most recently from be81dd7 to 1b0c0eb Compare May 23, 2023 20:36

github-actions bot added the Stale label Nov 19, 2023

github-actions bot closed this Dec 4, 2023

andrzej-kaczmarek reopened this Apr 23, 2024

andrzej-kaczmarek force-pushed the da1469x-cmac branch from 1b0c0eb to 6558e88 Compare April 23, 2024 12:46

zephyrbot added the area: Bluetooth Host label Apr 23, 2024

zephyrbot requested review from HoZHel, jori-nordic and theob-pro April 23, 2024 12:47

andrzej-kaczmarek force-pushed the da1469x-cmac branch from 6558e88 to 12e6aa2 Compare April 23, 2024 16:01

github-actions bot removed the Stale label Apr 24, 2024

jhedberg requested changes Apr 24, 2024

View reviewed changes

andrzej-kaczmarek force-pushed the da1469x-cmac branch from 12e6aa2 to 0118035 Compare May 13, 2024 09:29

jhedberg requested changes May 13, 2024

View reviewed changes

west.yml: Update revision for hal_renesas

41c75d3

Pull changes required for CMAC driver. Signed-off-by: Andrzej Kaczmarek <[email protected]>

andrzej-kaczmarek force-pushed the da1469x-cmac branch from 0118035 to 6aa099d Compare May 13, 2024 11:46

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

andrzej-kaczmarek force-pushed the da1469x-cmac branch from 6aa099d to bf24716 Compare May 13, 2024 11:49

andrzej-kaczmarek added 2 commits May 15, 2024 23:23

boards: renesas: Add configuration for BT

272e30a

Adds required defconfigs for BT on DA1469x DK. Signed-off-by: Andrzej Kaczmarek <[email protected]>

andrzej-kaczmarek force-pushed the da1469x-cmac branch from bf24716 to 272e30a Compare May 15, 2024 21:24

zephyrbot requested a review from ioannis-karachalios May 15, 2024 21:24

blauret self-requested a review May 16, 2024 07:05

blauret approved these changes May 16, 2024

View reviewed changes

jhedberg approved these changes May 16, 2024

View reviewed changes

nashif merged commit dd395e7 into zephyrproject-rtos:main May 16, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: hci: da1469x: Add driver for CMAC core on DA1469x #57951

drivers: hci: da1469x: Add driver for CMAC core on DA1469x #57951

andrzej-kaczmarek commented May 16, 2023 •

edited

Loading

zephyrbot commented May 16, 2023 •

edited

Loading

nordicjm commented May 16, 2023

andrzej-kaczmarek commented May 17, 2023

carlescufi commented May 17, 2023

nordicjm commented May 17, 2023

andrzej-kaczmarek commented May 17, 2023 •

edited

Loading

carlescufi commented May 17, 2023

carlescufi commented May 17, 2023

andrzej-kaczmarek commented May 17, 2023

github-actions bot commented Nov 19, 2023

jhedberg commented Apr 23, 2024

jhedberg Apr 24, 2024

jhedberg Apr 24, 2024

jhedberg Apr 24, 2024

jhedberg May 15, 2024

andrzej-kaczmarek May 15, 2024

jhedberg left a comment

jhedberg May 13, 2024

jhedberg May 13, 2024

jhedberg May 13, 2024

jhedberg May 13, 2024

andrzej-kaczmarek commented May 13, 2024

jhedberg commented May 15, 2024

blauret commented May 16, 2024

drivers: hci: da1469x: Add driver for CMAC core on DA1469x #57951

drivers: hci: da1469x: Add driver for CMAC core on DA1469x #57951

Conversation

andrzej-kaczmarek commented May 16, 2023 • edited Loading

zephyrbot commented May 16, 2023 • edited Loading

nordicjm commented May 16, 2023

andrzej-kaczmarek commented May 17, 2023

carlescufi commented May 17, 2023

nordicjm commented May 17, 2023

andrzej-kaczmarek commented May 17, 2023 • edited Loading

carlescufi commented May 17, 2023

carlescufi commented May 17, 2023

andrzej-kaczmarek commented May 17, 2023

github-actions bot commented Nov 19, 2023

jhedberg commented Apr 23, 2024

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

jhedberg left a 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

andrzej-kaczmarek commented May 13, 2024

jhedberg commented May 15, 2024

blauret commented May 16, 2024

andrzej-kaczmarek commented May 16, 2023 •

edited

Loading

zephyrbot commented May 16, 2023 •

edited

Loading

andrzej-kaczmarek commented May 17, 2023 •

edited

Loading