Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

driver: bluetooth: hci: Add NXP BT module support on RT1170 EVKB #70815

Merged

Conversation

lylezhu2012
Copy link
Contributor

Implement UART firmware download driver for NXP BT module.

Only support Murata 2EL M.2 Module on RT1170 EVKB.

@pdgendt
Copy link
Collaborator

pdgendt commented Jun 4, 2024

@pdgendt can you confirm @lylezhu2012 response to your feedback

My comments have been addressed, I lack the experience to ACK the functionality though. Probably someone with Bluetooth driver knowledge should take a look.

dleach02
dleach02 previously approved these changes Jun 4, 2024
Comment on lines +242 to +253
config BT_H4_NXP_CTLR
bool "NXP Bluetooth Controller"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense to add the following here:

default y
depends on DT_HAS_NXP_BT_HCI_UART_ENABLED

Then you can remove any explicit enablement in the app Kconfig overlays.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and then it'll also be easier for me to do the conversion as part of #72323

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense to add the following here:

default y
depends on DT_HAS_NXP_BT_HCI_UART_ENABLED

Then you can remove any explicit enablement in the app Kconfig overlays.

Sorry, the dependency cannot be added.

The Bluetooth controller is module, it can work with any MCU with UART peripheral.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lylezhu2012 I'm not sure I follow. Even in the case that you describe, those MCUs that have this peripheral connected will still need to have the appropriate devicetree node with compatible = "nxp,bt-hci-uart";, won't they? If not, what is the purpose of the devicetree node?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compatible = "nxp,bt-hci-uart"; is the devicetree node required by NXP Bluetooth Controller. If you want to use NXP Bluetooth Controller, you need to add the devicetree node with compatible = "nxp,bt-hci-uart";.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lylezhu2012 the compatible strings are used to associate devicetree nodes with drivers. Which driver implements this compatible? You're adding the hci_nxp_setup.c extension driver in this same PR where you add the devicetree binding and compatible, so it seems natural to assume that these go together, even though the c-file currently doesn't #define DT_DRV_COMPAT nxp_bt_hci_uart (which I assumed was primarily because it only supports a single instance at the moment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, nxp,bt-hci-uart is used for driver hci_nxp_setup.c.

@lylezhu2012 the compatible strings are used to associate devicetree nodes with drivers. Which driver implements this compatible? You're adding the hci_nxp_setup.c extension driver in this same PR where you add the devicetree binding and compatible, so it seems natural to assume that these go together, even though the c-file currently doesn't #define DT_DRV_COMPAT nxp_bt_hci_uart (which I assumed was primarily because it only supports a single instance at the moment).

Yes. Currently, only one instance can be supported.

And I have a question, if does Bluetooth Host support multiple instance? I mean can we call bt_enable multiple times to enable multiple Bluetooth Host?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, nxp,bt-hci-uart is used for driver hci_nxp_setup.c.

So in that case you should be able to apply my proposal wrt default and depends on. If there isn't a node with that compatible string it doesn't make sense to enable the driver.

And I have a question, if does Bluetooth Host support multiple instance? I mean can we call bt_enable multiple times to enable multiple Bluetooth Host?

Not right now. This may become possible at some point in the future, especially thanks to #72323. With that PR you could quite easily construct a host that supports multiple controllers, or an app which exposes multiple controllers over different HCI transports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in that case you should be able to apply my proposal wrt default and depends on. If there isn't a node with that compatible string it doesn't make sense to enable the driver.

Updated.

@lylezhu2012 lylezhu2012 force-pushed the enable_rt1170evkb_2el_bluetooth branch from 0587066 to 8d48cb5 Compare June 11, 2024 10:29
@lylezhu2012 lylezhu2012 force-pushed the enable_rt1170evkb_2el_bluetooth branch from 8d48cb5 to bf10a1d Compare June 11, 2024 10:35
@lylezhu2012
Copy link
Contributor Author

Fix conflict issue.

@jori-nordic
Copy link
Contributor

@lylezhu2012 could you make it also work on MIMXRT1040-EVK, as that's the one you suggested (in #67145) and we bought.

@lylezhu2012
Copy link
Contributor Author

lylezhu2012 commented Jun 12, 2024

@lylezhu2012 could you make it also work on MIMXRT1040-EVK, as that's the one you suggested (in #67145) and we bought.

There is a pin conflict on MIMXRT1040-EVK. If PCM pins for handsfree (or AG) are configured by soldering 0 ohm resistors, the board cannot be flashed. So for BR cases, I do not recommend using MIMXRT1040-EVK. MIMXRT1170-EVKB does not have this limitation. That's why I made this PR to support MIMXRT1170-EVKB.

@jori-nordic
Copy link
Contributor

If PCM pins for handsfree (or AG) are configured by soldering 0 ohm resistors

We don't really care about the PCM data, it's only for testing that the ACL links and L2CAP channel still work. We could still test, e.g. RFCOMM or A2DP with all-zeroes PCM data.

That board can also be useful to test LE.

the board cannot be flashed

Can you share where in the official documentation I can find this information?

@lylezhu2012
Copy link
Contributor Author

We don't really care about the PCM data, it's only for testing that the ACL links and L2CAP channel still work. We could still test, e.g. RFCOMM or A2DP with all-zeroes PCM data.
That board can also be useful to test LE.

Got it. I can enable it after the PR merged.

Can you share where in the official documentation I can find this information?

No. We flashed it internally by adding an extra circuit. Or, remove the module manually.

@lylezhu2012
Copy link
Contributor Author

We don't really care about the PCM data, it's only for testing that the ACL links and L2CAP channel still work. We could still test, e.g. RFCOMM or A2DP with all-zeroes PCM data.
That board can also be useful to test LE.

Got it. I can enable it after the PR merged.

Can you share where in the official documentation I can find this information?

No. We flashed it internally by adding an extra circuit. Or, remove the module or a jumper manually.

Implement UART firmware download driver for NXP
BT module.

Only support Murata 2EL M.2 module on RT1170EVKB.

And only one instance can be supported now.

Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>
Add Kconfig.nxp to support NXP Bluetooth Chipset.
Current only NXP IW612 Chipset (BT_NXP_NW612) has
been supported.

Add modules/hal_nxp/bt_controller/CMakeLists.txt to
determine whether any firmware is selected, and
check whether the firmware exists.

If the firmware exists, copy the firmware to the
temporary folder ${ZEPHYR_BINARY_DIR}/include/
generated/bt_nxp_ctlr_fw.h. OR, raise a fatal error.

In file hci_nxp_setup.c, includes the temporary file
bt_nxp_ctlr_fw.h.

Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>
Overlay sram when BT is enabled on board MIMXRT1170 EVKB.

Select NXP NW612 chipset.

Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>
Add `fetch Binary Blobs` section to guide how to
fetch binary blobs.

Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>
Use zephyr_blobs_verify to check the blob file is valid
or not.

The function zephyr_blobs_verify will check if the file
exists. And it checks if the file is valid.

Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>
Overlay sram when BT is enabled on board MIMXRT1170 EVKB.

Select NXP NW612 chipset for Handsfree_ag example

Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>
@lylezhu2012 lylezhu2012 force-pushed the enable_rt1170evkb_2el_bluetooth branch from bf10a1d to e0695f2 Compare June 12, 2024 06:36
@lylezhu2012
Copy link
Contributor Author

@jori-nordic , I saw that 72323 has been merged. And I updated the PR to support it.

Please help review it.

@jori-nordic
Copy link
Contributor

Will do. I don't have the HW though so I can only try building 👍

@lylezhu2012
Copy link
Contributor Author

Will do. I don't have the HW though so I can only try building 👍

Sure. Please. I have tested it locally.

@nashif nashif merged commit c4df21e into zephyrproject-rtos:main Jun 13, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants