drivers: bluetooth: Add Ambiq HCI driver for Apollo4 Blue Plus #66227

aaronyegx · 2023-12-06T12:46:05Z

This PR implements the SPI based HCI driver for Ambiq Apollo4 Blue Plus. We will use HCI-SPI to communicate between the Zephyr Bluetooth host stack and Ambiq Apollo4 Blue Plus internal Bluetooth controller.
The Bluetooth low energy controller inside of Apollo4 Blue Plus is another die running Cortex-M0 core. It works as SPI slave, while Apollo4 Blue Plus host (Cortex-M4) works as SPI master. There are only SPI, interrupt GPIO, clock request GPIO, reset GPIO, XO32MHz and XO32kHz connecting internally between controller and host. The host provides the clock source to controller. No pin of controller die is exposed on the Apollo4 Blue Plus soc.

This PR should be merged after the hal_ambiq PR zephyrproject-rtos/hal_ambiq#9 merged.

Test passed on the following samples:
-samples/bluetooth/peripheral
-samples/bluetooth/peripheral_hr
-samples/bluetooth/peripheral_ht
-samples/bluetooth/peripheral_esp
-samples/bluetooth/peripheral_dis
-samples/bluetooth/peripheral_gatt_write
-samples/bluetooth/peripheral_accept_list
-samples/bluetooth/peripheral_sc_only
-samples/bluetooth/central
-samples/bluetooth/central_hr
-samples/bluetooth/central_ht
-samples/bluetooth/central_gatt_write
-samples/bluetooth/central_past
-samples/bluetooth/broadcaster
-samples/bluetooth/broadcaster_multiple
-samples/bluetooth/beacon
-samples/bluetooth/observer
-samples/bluetooth/direct_adv
-samples/bluetooth/scan_adv
-samples/bluetooth/mtu_update
-samples/bluetooth/direction_finding_central
-samples/bluetooth/direction_finding_peripheral
-samples/bluetooth/periodic_adv
-samples/bluetooth/periodic_sync
-samples/bluetooth/eddystone
-samples/bluetooth/ibeacon
-samples/bluetooth/hci_uart
-tests/bluetooth/init

zephyrbot · 2023-12-06T12:47:07Z

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

Name	Old Revision	New Revision	Diff
hal_ambiq	zephyrproject-rtos/hal_ambiq@`0c5ea57`	zephyrproject-rtos/hal_ambiq@`323048f` (main)	zephyrproject-rtos/hal_ambiq@0c5ea574..323048f9

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

jhedberg

Thanks for the submission! A few initial comments inline.

jhedberg · 2023-12-07T09:07:02Z

drivers/bluetooth/hci/hci_ambiq.c

+static K_SEM_DEFINE(sem_irq, 0, 1);
+static K_SEM_DEFINE(sem_spi_available, 1, 1);
+
+#if defined(CONFIG_SOC_APOLLO4P_BLUE)


It doesn't look like this driver does anything useful for any other SoC than SOC_APOLLO4P_BLUE so these conditions seem rather useless. I'd suggest to just remove them. If at any point you need to support multiple platforms or SoCs you can then start introducing feature-based conditions, but right now it seems premature.

Hi @jhedberg , thanks for your comments. Yes, we are integrating Apollo3 Blue Plus which uses SPI for the HCI as well, so I just try to reserve the condition then I can add Apollo3 Blue Plus support soon. I'm happy to remove these soc conditions at this moment.

Hi @jhedberg , thanks for your comments. Yes, we are integrating Apollo3 Blue Plus which uses SPI for the HCI as well, so I just try to reserve the condition then I can add Apollo3 Blue Plus support soon. I'm happy to remove these soc conditions at this moment.

The code that remains if APOLLO4P is not set doesn't seem to result any kind of functional driver. The sections behind APOLLO4P are so large, that if you will have equally large sections for Apollo3 then it sounds like you might want to make these two separate drivers (potentially refactoring shared parts into a common c-file).

Hi @jhedberg , I removed the #if defined(CONFIG_SOC_APOLLO4P_BLUE) to avoid confusing at this moment. And will add the adaptable condition when I add Apollo3 Blue support further.
I refactored the HCI driver to two files: hci_ambiq.c to include the shared parts (spi, bt related codes) and apollox_blue.c to include the Ambiq Apollox specific codes. When it's time to add Apollo3 support, it is most likely that I only need to add something in apollox_blue.c file. Could you help check if it is acceptable?

drivers/bluetooth/hci/hci_ambiq.c

jhedberg · 2023-12-07T10:32:08Z

Btw, now that we're starting to have multiple SPI HCI drivers, it makes the existence of drivers/bluetooth/hci/spi.c potentially confusing. As there is no standard SPI HCI transport specification (i.e. it's not part of the Bluetooth Core Specification) we should probably not have a driver whose name implies that it's "the standard". Instead, the names of the SPI HCI drivers should likely reflect the specific controller implementation or platform that they support. spi.c could perhaps contain the shared parts that all SPI-based HCI drivers need (assuming there's any substantial amount of code that can be shared between them).

@HoZHel @msmttchr @arbrauns I'd appreciate your thoughts on this.

msmttchr · 2023-12-07T14:34:01Z

Btw, now that we're starting to have multiple SPI HCI drivers, it makes the existence of drivers/bluetooth/hci/spi.c potentially confusing. As there is no standard SPI HCI transport specification (i.e. it's not part of the Bluetooth Core Specification) we should probably not have a driver whose name implies that it's "the standard". Instead, the names of the SPI HCI drivers should likely reflect the specific controller implementation or platform that they support. spi.c could perhaps contain the shared parts that all SPI-based HCI drivers need (assuming there's any substantial amount of code that can be shared between them).

@HoZHel @msmttchr @arbrauns I'd appreciate your thoughts on this.

@jhedberg, I fully agree with you:

HCI SPI protocol is not standard
SPI BlueNRG protocol v1 and v2 is specific for ST, even though v2 is implemented in software. This protocol is, in my opinion, more suitable for low power management and performances.
SPI protocol provided by hci_spi sample is a "clone" of ST with some differences. This is suitable to be implemented by all the controller implementation in Zephyr.
SPI Ambiq is a new proprietary protocol.

I am more and more convinced that splitting the files is eventually the best approach.
I would go for something like:

spi_zephyr.c (BTLE controller SPI interface as provided by hci_spi)
spi_st.c (BlueNRG SPI protocol)
spi_ambiq.c (Ambiq SPI protocol)

Finally, whether there is a common part, it is a bit hard to evaluate (at least for me).
It is also possible that in the future v2 and hci_spi could converge, but it is very hard for me to predict.

jhedberg · 2023-12-11T11:33:12Z

drivers/bluetooth/hci/hci_ambiq.c

+extern int bt_apollo_dev_init(void);
+extern int bt_apollo_spi_send(uint8_t *data, uint16_t len, bt_spi_transceive_fun transceive);
+extern int bt_apollo_spi_rcv(uint8_t *data, uint16_t *len, bt_spi_transceive_fun transceive);
+extern int bt_apollo_controller_init(spi_transmit_fun transmit);
+extern int bt_apollo_vnd_setup(void);
+extern bool bt_apollo_vnd_rcv_ongoing(uint8_t *data, uint16_t len);


Wouldn't it be cleaner to have a proper header file for these?

I added a header file in hal_ambiq to declare these functions to make the HCI driver cleaner.

I added a header file in hal_ambiq to declare these functions to make the HCI driver cleaner.

Aren't all of these APIs implemented by apollox_blue.c? The right place for the header file would therefore be here in the same directory, not in the HAL tree.

Aren't all of these APIs implemented by apollox_blue.c? The right place for the header file would therefore be here in the same directory, not in the HAL tree.

I put the header file apollox_blue.h in the same directory with apollox_blue.c, thank you.

This commits create the dts binding for Ambiq BT HCI instance. And create the SPI based common HCI driver for Ambiq Apollox Blue SoC and the extended soc driver for HCI. Signed-off-by: Aaron Ye <aye@ambiq.com>

The BT HCI uses internal IOM4 (SPI4) for communication bus. Set the default configuration for BT based on the controller supported capability and HCI driver dependency. Signed-off-by: Aaron Ye <aye@ambiq.com>

Imported the original Cooper (Apollo4x BLE controller) device files from AmbiqSuite SDK 4.4.0 and adapts them to support the new implemented HCI driver for Apollo4 Blue Plus. Signed-off-by: Aaron Ye <aye@ambiq.com>

aaronyegx · 2023-12-13T12:48:32Z

Hi @jhedberg , we merged the hal_ambiq PR and updated the revision in west.yml accordingly. Could you reapprove this? Thank you.

aaronyegx · 2023-12-13T13:08:27Z

Hi @fkokosinski @tgorochowik , this PR enables the Bluetooth/HCI feature for Ambiq Apollo4 Blue Plus and incoming other Bluetooth series soc of Ambiq platform. If possible, could you also take a look at this? Thanks very much.

msmttchr

LGTM. I think now also hci_uart is working

aaronyegx · 2023-12-14T10:24:09Z

Hi @fkokosinski @tgorochowik , we passed most of the adaptable Zephyr Bluetooth samples (refer to the test list in the PR description) with the implemented Ambiq HCI driver of this PR on apollo4p_blue_kxr_evb board. We are trying to release this important feature required by the waiting wide customers for evaluation as soon as possible and getting feedback from them for code quality improvement or bug fix.
If there is no big concern, could you help move forward this, please? Thanks very much for your understanding and support.

fkokosinski

Board and dts bindings changes LGTM. :)

carlescufi

Before I merge this, may I ask the following 2 questions:

~~Why wasn't the existing hci_spi.c reused by extending it?~~ I realize now by reading the comments.
Why the need to the apollox_blue.* files if we have hci_ambiq.c? Couldn't we just have a single file with all the code?

aaronyegx · 2023-12-14T13:36:38Z

Before I merge this, may I ask the following 2 questions:

~~Why wasn't the existing hci_spi.c reused by extending it?~~ I realize now by reading the comments.

Why the need to the apollox_blue.* files if we have hci_ambiq.c? Couldn't we just have a single file with all the code?

Hi @carlescufi , based on our previous discussion in this PR (see #66227 (comment)), we will integrate Apollo3 Blue Plus which also uses SPI for HCI but there are many differences between Apollo4 and Apollo3 in SPI protocol. Using a single file was the initial submission in this PR, but we thought using a single file to include all the contents would make the file too large and less readable/maintainable. So, we decide to create apollox_blue.c for specific SPI protocol for the soc, while hci_ambiq.c will include the common parts. It is most likely we only need to modify apollox_blue.c to enable support for Apollo3 Blue Plus in incoming implementation. Thank you.

aaronyegx · 2023-12-15T14:44:17Z

Before I merge this, may I ask the following 2 questions:

~~Why wasn't the existing hci_spi.c reused by extending it?~~ I realize now by reading the comments.

Why the need to the apollox_blue.* files if we have hci_ambiq.c? Couldn't we just have a single file with all the code?

Hi @carlescufi , based on our previous discussion in this PR (see #66227 (comment)), we will integrate Apollo3 Blue Plus which also uses SPI for HCI but there are many differences between Apollo4 and Apollo3 in SPI protocol. Using a single file was the initial submission in this PR, but we thought using a single file to include all the contents would make the file too large and less readable/maintainable. So, we decide to create apollox_blue.c for specific SPI protocol for the soc, while hci_ambiq.c will include the common parts. It is most likely we only need to modify apollox_blue.c to enable support for Apollo3 Blue Plus in incoming implementation. Thank you.

Hi @carlescufi , could you help revisit this, please? Thank you.

aaronyegx · 2023-12-18T08:56:15Z

Before I merge this, may I ask the following 2 questions:

~~Why wasn't the existing hci_spi.c reused by extending it?~~ I realize now by reading the comments.

Why the need to the apollox_blue.* files if we have hci_ambiq.c? Couldn't we just have a single file with all the code?

Hi @carlescufi , based on our previous discussion in this PR (see #66227 (comment)), we will integrate Apollo3 Blue Plus which also uses SPI for HCI but there are many differences between Apollo4 and Apollo3 in SPI protocol. Using a single file was the initial submission in this PR, but we thought using a single file to include all the contents would make the file too large and less readable/maintainable. So, we decide to create apollox_blue.c for specific SPI protocol for the soc, while hci_ambiq.c will include the common parts. It is most likely we only need to modify apollox_blue.c to enable support for Apollo3 Blue Plus in incoming implementation. Thank you.

Hi @carlescufi , could you help revisit this, please? Thank you.

Hi @carlescufi , could you look at the answer for your merging question? Sorry I marked some comments resolved to make you miss some information in your first review. I think the comments #66227 (comment), #66227 (comment), #66227 (comment) should clear your confusion. We started to announce Zephyr supports Apollo family SoCs, the Bluetooth is the basic supported feature, which is waiting for merging here. Thank you so much.

carlescufi · 2023-12-18T13:54:22Z

Before I merge this, may I ask the following 2 questions:

~~Why wasn't the existing hci_spi.c reused by extending it?~~ I realize now by reading the comments.

Why the need to the apollox_blue.* files if we have hci_ambiq.c? Couldn't we just have a single file with all the code?

Hi @carlescufi , based on our previous discussion in this PR (see #66227 (comment)), we will integrate Apollo3 Blue Plus which also uses SPI for HCI but there are many differences between Apollo4 and Apollo3 in SPI protocol. Using a single file was the initial submission in this PR, but we thought using a single file to include all the contents would make the file too large and less readable/maintainable. So, we decide to create apollox_blue.c for specific SPI protocol for the soc, while hci_ambiq.c will include the common parts. It is most likely we only need to modify apollox_blue.c to enable support for Apollo3 Blue Plus in incoming implementation. Thank you.

Thanks for your explanation. Approving now.

fabiobaltieri · 2024-01-08T21:24:03Z

boards/arm/apollo4p_blue_kxr_evb/apollo4p_blue_kxr_evb.dts

+&iom4 {
+	compatible = "ambiq,spi";
+	pinctrl-0 = <&spi4_default>;
+	pinctrl-names = "default";
+	cs-gpios = <&gpio32_63 22 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
+	clock-frequency = <DT_FREQ_M(24)>;
+	status = "okay";
+
+	bt-hci@0 {
+		compatible = "ambiq,bt-hci-spi";
+		reg = <0>;
+		irq-gpios = <&gpio32_63 21 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio32_63 23 GPIO_ACTIVE_LOW>;
+		clkreq-gpios = <&gpio32_63 20 GPIO_ACTIVE_HIGH>;
+	};
+};


Are these wired internally in the chip? If that's the case I think it would make sense to have the nodes declared in ambiq_apollo4p_blue.dtsi so that you don't have to redefine the same things on every board using the chip, it's not like you can change it anyway. See stm32wl.dtsi with the stm32wl-subghz-radio node in the internal spi for example.

@fabiobaltieri thanks for your following. Will have an independent PR to optimize this.

zephyrbot added manifest manifest-hal_ambiq DNM This PR should not be merged (Do Not Merge) labels Dec 6, 2023

aaronyegx force-pushed the add_ambiq_hci_driver branch 2 times, most recently from 8872afa to f83c9a8 Compare December 6, 2023 15:11

aaronyegx marked this pull request as ready for review December 6, 2023 15:18

aaronyegx requested review from alwa-nordic, jhedberg and Vudentz as code owners December 6, 2023 15:18

zephyrbot added area: Bluetooth Host platform: Ambiq Ambiq area: Bluetooth area: Devicetree Binding PR modifies or adds a Device Tree binding labels Dec 6, 2023

zephyrbot requested review from asbjornsabo, fkokosinski, galak, hermabe, jori-nordic, kruithofa, sjanc, tgorochowik, Thalley and theob-pro December 6, 2023 15:18

zephyrbot assigned jhedberg Dec 6, 2023

jhedberg requested changes Dec 7, 2023

View reviewed changes

aaronyegx force-pushed the add_ambiq_hci_driver branch 2 times, most recently from a9a4fe1 to 607157a Compare December 8, 2023 06:36

jhedberg reviewed Dec 11, 2023

View reviewed changes

aaronyegx force-pushed the add_ambiq_hci_driver branch 2 times, most recently from 90212d9 to 11a8770 Compare December 12, 2023 03:48

aaronyegx added 2 commits December 13, 2023 12:18

drivers: bluetooth: Add Ambiq HCI driver for Apollo4 Blue Plus.

5519c5b

This commits create the dts binding for Ambiq BT HCI instance. And create the SPI based common HCI driver for Ambiq Apollox Blue SoC and the extended soc driver for HCI. Signed-off-by: Aaron Ye <aye@ambiq.com>

boards: arm: apollo4p_blue_kxr_evb: Enable BT HCI.

b7e988b

The BT HCI uses internal IOM4 (SPI4) for communication bus. Set the default configuration for BT based on the controller supported capability and HCI driver dependency. Signed-off-by: Aaron Ye <aye@ambiq.com>

aaronyegx force-pushed the add_ambiq_hci_driver branch from 11a8770 to 9875be9 Compare December 13, 2023 04:21

jhedberg previously approved these changes Dec 13, 2023

View reviewed changes

aaronyegx mentioned this pull request Dec 13, 2023

Add Cooper (Apollo4x BLE controller) driver to support Bluetooth HCI zephyrproject-rtos/hal_ambiq#9

Merged

west.yml: hal_ambiq: Add Bluetooth support.

b9ffe32

Imported the original Cooper (Apollo4x BLE controller) device files from AmbiqSuite SDK 4.4.0 and adapts them to support the new implemented HCI driver for Apollo4 Blue Plus. Signed-off-by: Aaron Ye <aye@ambiq.com>

aaronyegx dismissed jhedberg’s stale review via b9ffe32 December 13, 2023 12:42

aaronyegx force-pushed the add_ambiq_hci_driver branch from 9875be9 to b9ffe32 Compare December 13, 2023 12:42

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

jhedberg approved these changes Dec 13, 2023

View reviewed changes

aaronyegx requested a review from msmttchr December 13, 2023 13:05

msmttchr approved these changes Dec 13, 2023

View reviewed changes

fkokosinski approved these changes Dec 14, 2023

View reviewed changes

carlescufi requested changes Dec 14, 2023

View reviewed changes

aaronyegx requested a review from carlescufi December 15, 2023 09:30

carlescufi approved these changes Dec 18, 2023

View reviewed changes

carlescufi merged commit 9a642ca into zephyrproject-rtos:main Dec 18, 2023
22 of 23 checks passed

fabiobaltieri reviewed Jan 8, 2024

View reviewed changes

aaronyegx mentioned this pull request Jan 11, 2024

boards: arm: apollo4p_blue_kxr_evb: Move the bt-spi instance to soc dts #67466

Merged

aaronyegx deleted the add_ambiq_hci_driver branch January 15, 2024 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: bluetooth: Add Ambiq HCI driver for Apollo4 Blue Plus #66227

drivers: bluetooth: Add Ambiq HCI driver for Apollo4 Blue Plus #66227

aaronyegx commented Dec 6, 2023 •

edited

zephyrbot commented Dec 6, 2023 •

edited

jhedberg left a comment

jhedberg Dec 7, 2023

aaronyegx Dec 7, 2023 •

edited

jhedberg Dec 7, 2023

aaronyegx Dec 8, 2023

jhedberg commented Dec 7, 2023

msmttchr commented Dec 7, 2023

jhedberg Dec 11, 2023

aaronyegx Dec 11, 2023

jhedberg Dec 11, 2023 •

edited

aaronyegx Dec 12, 2023

aaronyegx commented Dec 13, 2023

aaronyegx commented Dec 13, 2023

msmttchr left a comment

aaronyegx commented Dec 14, 2023

fkokosinski left a comment

carlescufi left a comment •

edited

aaronyegx commented Dec 14, 2023 •

edited

aaronyegx commented Dec 15, 2023

aaronyegx commented Dec 18, 2023

carlescufi commented Dec 18, 2023

fabiobaltieri Jan 8, 2024

aaronyegx Jan 9, 2024

drivers: bluetooth: Add Ambiq HCI driver for Apollo4 Blue Plus #66227

drivers: bluetooth: Add Ambiq HCI driver for Apollo4 Blue Plus #66227

Conversation

aaronyegx commented Dec 6, 2023 • edited

zephyrbot commented Dec 6, 2023 • edited

jhedberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronyegx Dec 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhedberg commented Dec 7, 2023

msmttchr commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhedberg Dec 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronyegx commented Dec 13, 2023

aaronyegx commented Dec 13, 2023

msmttchr left a comment

Choose a reason for hiding this comment

aaronyegx commented Dec 14, 2023

fkokosinski left a comment

Choose a reason for hiding this comment

carlescufi left a comment • edited

Choose a reason for hiding this comment

aaronyegx commented Dec 14, 2023 • edited

aaronyegx commented Dec 15, 2023

aaronyegx commented Dec 18, 2023

carlescufi commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronyegx commented Dec 6, 2023 •

edited

zephyrbot commented Dec 6, 2023 •

edited

aaronyegx Dec 7, 2023 •

edited

jhedberg Dec 11, 2023 •

edited

carlescufi left a comment •

edited

aaronyegx commented Dec 14, 2023 •

edited