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

drivers: bluetooth: hci: add support for bt_disable() function call #86645

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nirav-agrawal
Copy link

@nirav-agrawal nirav-agrawal commented Mar 5, 2025

  • Added flag in nxp setup driver to perform HCI controller firmware- load and configuration only once. This avoids loading/setting-up controller firmware on successive h4_open() after h4_close().
  • Added HCI command to send HCI_RESET in bt_disable() function and remove its call from hci-driver close() call.
  • Remove rsp buf pass to hci_reset_complete() function as status is already checked under 'bt_hci_cmd_send_sync()' func.

For HCI H4 driver, close() call is added in this PR: #86424.
one more PR to be merged before current PR, https://github.com/zephyrproject-rtos/zephyr/pull/86462/files

@zephyrbot zephyrbot added area: Bluetooth HCI Bluetooth HCI Driver area: Bluetooth platform: NXP Drivers NXP Semiconductors, drivers labels Mar 5, 2025
@nirav-agrawal nirav-agrawal force-pushed the add_hci_close_support_in_h4_driver branch 2 times, most recently from 1a3a575 to 3ac826f Compare March 5, 2025 08:26
@nirav-agrawal nirav-agrawal force-pushed the add_hci_close_support_in_h4_driver branch from 3ac826f to c221ccf Compare March 5, 2025 10:50
@zephyrbot zephyrbot added the area: Bluetooth Host Bluetooth Host (excluding BR/EDR) label Mar 5, 2025
@zephyrbot zephyrbot requested a review from rugeGerritsen March 5, 2025 10:50
Copy link
Contributor

@cvinayak cvinayak left a comment

Choose a reason for hiding this comment

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

Does this duplicate or supercede #86424 ?

Comment on lines 4388 to 4397
struct net_buf *rsp;
err = bt_hci_cmd_send_sync(BT_HCI_OP_RESET, NULL, &rsp);
if (err) {
LOG_ERR("Failed to reset BLE controller");
return err;
}
hci_reset_complete(rsp);
net_buf_unref(rsp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
struct net_buf *rsp;
err = bt_hci_cmd_send_sync(BT_HCI_OP_RESET, NULL, &rsp);
if (err) {
LOG_ERR("Failed to reset BLE controller");
return err;
}
hci_reset_complete(rsp);
net_buf_unref(rsp);
struct net_buf *rsp;
err = bt_hci_cmd_send_sync(BT_HCI_OP_RESET, NULL, &rsp);
if (err) {
LOG_ERR("Failed to reset BLE controller");
return err;
}
hci_reset_complete(rsp);
net_buf_unref(rsp);

Copy link
Author

@nirav-agrawal nirav-agrawal Mar 5, 2025

Choose a reason for hiding this comment

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

Hi @cvinayak, I did not notice that the PR is already raised for this requirement. I have checked the PR you mentioned. It has some partial change and might need some more work for other HCI driver to work with bt init & bt disable flow.

I have updated my current PR and removed changes made for H4 driver which is already done under #86424 PR. Rest all other changes are required to work bt_disable() correctly. Thank you.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @cvinayak , suggestion amended. thank you.

@nirav-agrawal nirav-agrawal force-pushed the add_hci_close_support_in_h4_driver branch from c221ccf to 4ff3eb4 Compare March 5, 2025 11:49
@nirav-agrawal nirav-agrawal force-pushed the add_hci_close_support_in_h4_driver branch from 4ff3eb4 to 7652d9c Compare March 5, 2025 13:46
@nirav-agrawal nirav-agrawal changed the title drivers: bluetooth: hci: added h4_close() support drivers: bluetooth: hci: add support for bt_disable() function call Mar 5, 2025
@nirav-agrawal nirav-agrawal requested a review from jhedberg March 5, 2025 14:01
@nirav-agrawal nirav-agrawal force-pushed the add_hci_close_support_in_h4_driver branch 3 times, most recently from 4feb752 to 028f284 Compare March 5, 2025 17:34

ret = PLATFORM_TerminateBle();
if (ret < 0) {
LOG_ERR("Failed to shutdown BLE controller");

This comment was marked as resolved.

Copy link
Author

@nirav-agrawal nirav-agrawal Mar 6, 2025

Choose a reason for hiding this comment

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

Hi @zhaynxp, the only change I did was to remove hci- reset calls. In this code, the above code was called as per return status of hci-reset command, so I updated code to remove that dependency.
I understand your point; I will wait for your PR to be merged and will rebase my PR to keep your change. It will be look like below code,

static int bt_nxp_close(const struct device *dev)
{
	struct bt_nxp_data *hci = dev->data;

	hci->recv = NULL;

	return 0;
}

please suggest. thank you.

regards,
Nirav

This comment was marked as resolved.

Copy link
Contributor

@axelnxp axelnxp left a comment

Choose a reason for hiding this comment

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

ok with the change, just remember this will conflict with #86462 and will need sequencing

- added HCI command to send hci-reset in bt_disable() func, and
  remove its call from the driver close() call.
- remove rsp buf pass to 'hci_reset_complete()' func as status is
  already checked under 'bt_hci_cmd_send_sync()'.

Signed-off-by: Nirav Agrawal <[email protected]>
@nirav-agrawal nirav-agrawal force-pushed the add_hci_close_support_in_h4_driver branch from 028f284 to 4952520 Compare March 10, 2025 07:49
@nirav-agrawal
Copy link
Author

HI @axelnxp, @jhedberg,
I rebased this PR with latest main where dependent PR is already merged. Pls approve this PR to get merged. Thank you.

Comment on lines 259 to 263
LOG_DBG("Calling bt_recv(%p)", buf);
h4->recv(dev, buf);
if (h4->recv != NULL) {
LOG_DBG("Calling bt_recv(%p)", buf);
h4->recv(dev, buf);
}
Copy link
Member

Choose a reason for hiding this comment

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

h4_close() calls k_thread_abort(cfg->rx_thread); before setting h4->recv = NULL;. I'm not sure how thread aborting works in practice, but I'd assume that the right thing to do here in case recv is NULL is to just return from the function, right?

Copy link
Member

Choose a reason for hiding this comment

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

Also, this change to h4.c seems independent of any NXP-related changes, so I'd keep it in its own commit (in fact, it seems like a bug fix to the h4_close() PR that was already merged).

Copy link
Author

Choose a reason for hiding this comment

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

Hi @jhedberg, Yes your point is valid. if we abort the thread then no need to put such condition. I will revert those change and update this PR. thank you

Copy link
Author

Choose a reason for hiding this comment

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

Hi @jhedberg, the changes in h4.c is reverted. please check. thank you.

- added flag in nxp setup driver to perform HCI controller firmware-
  load and configuration only once. This avoid loading/setting-up
  controller fw on successive bt_init() after bt_disable() call.

Signed-off-by: Nirav Agrawal <[email protected]>
@nirav-agrawal nirav-agrawal force-pushed the add_hci_close_support_in_h4_driver branch from 4952520 to 4608b11 Compare March 10, 2025 08:35
@nirav-agrawal nirav-agrawal requested a review from jhedberg March 10, 2025 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth HCI Bluetooth HCI Driver area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth platform: NXP Drivers NXP Semiconductors, drivers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants