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

[andrzej-kaczmarek]

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]

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]

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]

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]

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]

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

[andrzej-kaczmarek]

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]

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]

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]

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]

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]

@andrzej-kaczmarek please fix the compliance check failures

[jhedberg]

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).

[jhedberg]

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

[jhedberg]

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.

[jhedberg]

@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().

[andrzej-kaczmarek]

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]

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

[jhedberg]

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

[jhedberg]

same here

[jhedberg]

and here

[andrzej-kaczmarek]

west.yml is now updated with proper hash for HAL

[jhedberg]

@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().

[blauret]

@jhedberg, I think your last comment was addressed.