Update hci_core to execute vendor-specific commands to initialize BT Controller before hci_init calls common_init.

[npal-cy]

Is your enhancement proposal related to a problem? Please describe. For integration CYW43xxx Bluetooth IC into Zephyr i need to have possibility to execute vendor HCI commands before hci_init calls common_init. Main reason for that is: the CYW43xxx device requires downloading the controller firmware (via vendor HCI commands) after a power-on.

I am going to use the HCI H4 driver. HCI H4 driver provides _weak bt_hci_transport_setup to reset the Bluetooth IC. I will use it to power the CYW43xxx device. Unfortunately, I cannot implement FW downloading from bt_hci_transport_setup because the bt hci interface does not start on time when bt_hci_transport_setup is called.

Describe the solution you'd like I see the solution as update of the Zephyr common_init() function in hci_core.c, where the add hook to execute the vendor-specific init sequence is:

int __weak  bt_hci_vendor_init_sequence(void)
{
    return 0;
}

static int common_init(void)
{
    struct net_buf *rsp;
    int err;

    /* Execute vendor-specific commands to initialize the controller */
    err = bt_hci_vendor_init_sequence();
    if (err) {
        return err;
    }
...
}

My integration part will implement bt_hci_transport_setup and bt_hci_vendor_init_sequence functions.

Could you possibly share your considerations on the solution? There may exist another approach to the execution of vendor-specific commands for initializing the controller, before hci_core starts its own initialization

[cvinayak]

@jhedberg and @npal-cy Add earlier email conversations, here:

Adding such a feature to the Bluetooth stack sounds like a good idea, but I suspect we need to discuss a bit whether it should be a host extension using a __weak function, an extension to the HCI driver API or something else. Could you open a new enhancement request on github, providing initially the same information as in your email, and we’ll then continue the discussion there: https://github.com/zephyrproject-rtos/zephyr/issues/new/choose

Thanks!

Johan

Correct me, the requirement is

1. to be able to use Vendor HCI command/Response on bt_enable() after a HCI device is open but before the bt_init().
2. to be able to handle asynchronous Vendor HCI Events, if need be.
3. to be able to perform Device Firmware Update (DFU) or settings update, this would involve synchronization thereafter (similar to CONFIG_BT_WAIT_NOP).
4. to be able to use the feature conditionally, build-time and/or runtime.
5. to consider enhancements to Zephyr HCI Extensions

Agree that a discussion in an enhancement request will keep information collected. Do post the issue link and I can add my thoughts there.

[npal-cy]

Hi Vinayak,

1. to be able to use Vendor HCI command/Response on bt_enable() after a HCI device is open but before the bt_init().

correct. In CYW43xxx case, Host should download controller fw image into device (via vendor HCI commands) before start any HCI operation.

2. to be able to handle asynchronous Vendor HCI Events, if need be.

it does not requires for CYW43xxx case. In my porotype i am using bt_hci_cmd_send_sync to implement bt_hci_vendor_init_sequence.

3. to be able to perform Device Firmware Update (DFU) or settings update, this would involve synchronization thereafter (similar to CONFIG_BT_WAIT_NOP).

it does not requires for CYW43xxx case.

4. to be able to use the feature conditionally, build-time and/or runtime.

it is device specific feature, so looks like it should be build-time only.

5. to consider enhancements to Zephyr HCI Extensions

i do not have answer now.

[jhedberg]

@npal-cy in your original proposal with the __weak function - where would you place your implementation of it?

[npal-cy]

hi Johan,

I planed to locate it in the hal_infineon repo (cyw43xxx.c file):

hal_infineon
  |->drivers
          |->bluetooth
               |->firmware
                    |->COMPONENT_43012/...
                    |->COMPONENT_43438/...
                    |->COMPONENT_4343W/...
                    |->COMPONENT_4373/...
               |->cyw43xxx.c
               |->CMakeLists.txt                       

in zephyr\drivers\bluetooth\hci\Kconfig, i will add the options to select CYW44xxx with depends on H4 driver:

if BT_H4

config CYW43XXX_BT
    bool "CYW43XXX BT connectivity"
    select GPIO
    help
      Infineon’s AIROC™ Wi-Fi & combos portfolio integrates IEEE 802.11a/b/g/n/ac/ax Wi-Fi and Bluetooth® 5.2 
      in a single-chip solution to enable small-form-factor IoT designs. Combo, standalone Wi-Fi, and Wi-Fi SoCs
      with embedded MCU and on-chip networking capabilities are also offered in 1x1 SISO and 2x2 MIMO configurations.

choice
    prompt "Select CYW43XXX part"
    depends on CYW43XXX_BT

config CYW4343W
    bool "CYW4343W"
    help
      Enable Infineon CYW4343W BT connectivity,...

config CYW4373
    bool "CYW4373"
    help
      Enable Infineon CYW4373 BT connectivity,...

config CYW43012
    bool "CYW43012"
    help
      Enable Infineon CYW43012 BT connectivity,...

config CYW43438
    bool "CYW43438"
    help
      Enable Infineon CYW43438 BT connectivity,..
endchoice
endif # BT_H4

i attached initial version of cyw43xxx.c cyw43xxx.txt

Regards, Nazar

[jhedberg]

@npal-cy thanks for the overview. I think the general guidance is that if there's something purely Zephyr-specific in a HAL tree (as is the case with this HCI driver extension) it should be moved into the Zephyr main tree itself, i.e. it would then sit alongside of the other HCI drivers in Zephyr. Other parts might have to be elsewhere though. Having the CYW43XXX HCI driver extension in the main tree would also allow you to extend drivers/bluetooth/hci/Kconfig like you've now done, since AFAIK any Kconfig symbols intended for submodule usage should be placed under modules/* in the Zephyr main tree.

@carlescufi @alwa-nordic @MaureenHelm FYI

[jhedberg]

Since this vendor command sequence is implemented (in terms of Kconfig and c-file placement) as an extension of an HCI driver, and it does direct access to the struct device representing the UART, I'm not sure if it's right to call it directly from the host side. We might want to consider replicating the Linux solution for this, where HCI drivers provide a setup() callback, which would get called some time after the open() callback (basically in place of the new __weak function call that you've now added).

[jhedberg]

That said, I think this might also work as a h4.c driver-internal extension of h4_open(), before that function returns. Isn't everything set and ready for HCI command & event processing at that point? It'd then be done similar to the existing bt_hci_transport_setup() that's called in the beginning of h4_open(). @carlescufi would be good to hear your thoughts on this in particular, since it seems you were the one behind commit bca3deb1e73.

If we end up having multiple possible options, I'd still be slightly inclined to favor the Linux solution where hci_driver->open() and hci_driver->setup() are independent callbacks, if for no other reason than the fact that it has been time proven to be flexible enough to support all necessary drivers & controller configurations.

[cvinayak]

Since this vendor command sequence is implemented (in terms of Kconfig and c-file placement) as an extension of an HCI driver, and it does direct access to the struct device representing the UART, I'm not sure if it's right to call it directly from the host side. We might want to consider replicating the Linux solution for this, where HCI drivers provide a setup() callback, which would get called some time after the open() callback (basically in place of the new __weak function call that you've now added).

I second this, to have setup() callback that is implemented in a Kconfig selectable driver struct (field assignment). And the implementation resides in drivers rather than in host.

If we end up having multiple possible options, I'd still be slightly inclined to favor the Linux solution where hci_driver->open() and hci_driver->setup() are independent callbacks, if for no other reason than the fact that it has been time proven to be flexible enough to support all necessary drivers & controller configurations.

For the case of nRF51_pm that is referred in bca3deb1e7352f3d4813b0f0c2c9212d909709fb , the transport setup will be prior to setup(), i.e. the transport setup seems to be between UART open and UART thread being started inside the open()

[cvinayak]

@jhedberg something came up, that bt_addr_le_create_static needs to be executed before bt_enable and since bt_rand needs a seed to be generated using bt_hci_cmd_send_sync, the address generation stalls without the Tx and Rx thread running.

Wanted to highlight here as a consideration to allow HCI commands before hci_init/bt_enable

[carlescufi]

@npal-cy thanks for the overview. I think the general guidance is that if there's something purely Zephyr-specific in a HAL tree (as is the case with this HCI driver extension) it should be moved into the Zephyr main tree itself, i.e. it would then sit alongside of the other HCI drivers in Zephyr. Other parts might have to be elsewhere though. Having the CYW43XXX HCI driver extension in the main tree would also allow you to extend drivers/bluetooth/hci/Kconfig like you've now done, since AFAIK any Kconfig symbols intended for submodule usage should be placed under modules/* in the Zephyr main tree.

@carlescufi @alwa-nordic @MaureenHelm FYI

I completely agree with @jhedberg here. The proposed cyw43xxx.c, if we decide to follow this approach, belongs in the Zephyr main tree. In particular it belongs in drivers/bluetooth/hci. We can consider whether we want to create a folder in there to deal with the different vendor-specific routines that are required for controllers to boot up.

[carlescufi]

That said, I think this might also work as a h4.c driver-internal extension of h4_open(), before that function returns. Isn't everything set and ready for HCI command & event processing at that point? It'd then be done similar to the existing bt_hci_transport_setup() that's called in the beginning of h4_open(). @carlescufi would be good to hear your thoughts on this in particular, since it seems you were the one behind commit bca3deb.

If we end up having multiple possible options, I'd still be slightly inclined to favor the Linux solution where hci_driver->open() and hci_driver->setup() are independent callbacks, if for no other reason than the fact that it has been time proven to be flexible enough to support all necessary drivers & controller configurations.

I also agree with @jhedberg and @cvinayak that this does not belong in the Bluetooth Host, but rather in the Bluetooth drivers. So what I'd suggest is something similar to what @jhedberg outlines, which would be, from the Host's perspective:

hci_core.c:

static int hci_init(void)
{
    int err;
        if (bt_dev.drv->setup) {
                bt_dev.drv->setup();
        }

        err = common_init();
        ...

[carlescufi]

@jhedberg something came up, that bt_addr_le_create_static needs to be executed before bt_enable and since bt_rand needs a seed to be generated using bt_hci_cmd_send_sync, the address generation stalls without the Tx and Rx thread running.

Wanted to highlight here as a consideration to allow HCI commands before hci_init/bt_enable

Good point, but I think we can make it work with some flags and conditionals, let's first settle how we inject vendor-specific HCI commands prior to HCI Reset.