Message-ID: <CABBYNZJ5XDasAfVxcFU+K=ru2PpZJVXkRuf_kokv1z66KF=Xaw@mail.gmail.com>
Date: Wed, 12 Feb 2025 11:14:33 -0500
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Alexis Lothoré <alexis.lothore@...tlin.com>
Cc: Marcel Holtmann <marcel@...tmann.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Ajay Singh <ajay.kathat@...rochip.com>, Claudiu Beznea <claudiu.beznea@...on.dev>, 
	Kalle Valo <kvalo@...nel.org>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, Nicolas Ferre <nicolas.ferre@...rochip.com>, 
	Alexandre Belloni <alexandre.belloni@...tlin.com>, Marek Vasut <marex@...x.de>, 
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>, linux-bluetooth@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-wireless@...r.kernel.org, netdev@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 10/12] bluetooth: hci_wilc: add wilc hci driver

Hi Alexis,

On Wed, Feb 12, 2025 at 10:47 AM Alexis Lothoré
<alexis.lothore@...tlin.com> wrote:
>
> WILC3000 is a combo chip providing 802.11b/g/n and Bluetooth 5.0
> capabilities. The wifi side is used either over SDIO or SPI, and the
> bluetooth side is used over uart, with standard hci. The wifi side is
> already supported by Linux.
>
> Add a dedicated bluetooth driver to support the bluetooth feature from
> wilc3000 chip. Similarly to other supported bluetooth chips, wilc
> devices need a firmware to be uploaded before being able to use
> bluetooth. However, the major difference is that the firmware needs to
> be uploaded through the wifi bus (SDIO or SPI). This constraint makes
> this new driver depends on the corresponding wilc wlan driver for the
> bluetooth setup. Once the basic BT initialization has been performed,
> both side can be used independently, and in parallel.
>
> Since this creates a dependency to some wlan driver, create a dedicated
> module (hci_wilc) rather than integrating wilc bluetooth support in
> hci_uart, to avoid propagating this dependency.
>
> Signed-off-by: Alexis Lothoré <alexis.lothore@...tlin.com>
> ---
>  drivers/bluetooth/Kconfig    |  13 ++
>  drivers/bluetooth/Makefile   |   3 +-
>  drivers/bluetooth/hci_uart.h |   1 +
>  drivers/bluetooth/hci_wilc.c | 333 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 349 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 4ab32abf0f48644715d4c27ec0d2fdaccef62b76..a96607fb0cc8fed2ccb1811a4b1b4fe586396b07 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -147,6 +147,19 @@ config BT_HCIUART_NOKIA
>
>           Say Y here to compile support for Nokia's H4+ protocol.
>
> +config BT_HCIUART_WILC
> +       tristate "WILC protocol support"
> +       depends on (WILC1000_SDIO || WILC1000_SPI)
> +       select WILC_BT
> +       depends on BT_HCIUART
> +       depends on BT_HCIUART_SERDEV
> +       select BT_HCIUART_H4
> +       help
> +         The WILC uart protocol allows interacting with wilc3000 chips
> +         with HCI over UART. The bluetooth firmware needs to be uploaded
> +         to the chip through the main bus, so this driver needs the
> +         corresponding wlan driver.
> +
>  config BT_HCIUART_BCSP
>         bool "BCSP protocol support"
>         depends on BT_HCIUART
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index 81856512ddd030ba8172ff106b80b4b951188cbd..a1e69884acf4ce91f02ff5592541616048b07e31 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -35,10 +35,11 @@ obj-$(CONFIG_BT_NXPUART)    += btnxpuart.o
>  obj-$(CONFIG_BT_HCIUART_NOKIA) += hci_nokia.o
>
>  obj-$(CONFIG_BT_HCIRSI)                += btrsi.o
> -
>  btmrvl-y                       := btmrvl_main.o
>  btmrvl-$(CONFIG_DEBUG_FS)      += btmrvl_debugfs.o
>
> +obj-$(CONFIG_BT_HCIUART_WILC)  += hci_wilc.o
> +
>  hci_uart-y                             := hci_ldisc.o
>  hci_uart-$(CONFIG_BT_HCIUART_SERDEV)   += hci_serdev.o
>  hci_uart-$(CONFIG_BT_HCIUART_H4)       += hci_h4.o
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index fbf3079b92a5339154f8847ff36b3c08ef49e2bb..83fc48be4335e0946853fdec32c38bf2fb195009 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -35,6 +35,7 @@
>  #define HCI_UART_NOKIA 10
>  #define HCI_UART_MRVL  11
>  #define HCI_UART_AML   12
> +#define HCI_UART_WILC  13
>
>  #define HCI_UART_RAW_DEVICE    0
>  #define HCI_UART_RESET_ON_INIT 1
> diff --git a/drivers/bluetooth/hci_wilc.c b/drivers/bluetooth/hci_wilc.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..97dc4620c74ef0733469839adda7890bda90406e
> --- /dev/null
> +++ b/drivers/bluetooth/hci_wilc.c
> @@ -0,0 +1,333 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  Bluetooth HCI UART driver for WILC devices
> + *
> + */
> +#include "linux/bitops.h"
> +#include "linux/byteorder/generic.h"
> +#include "linux/err.h"
> +#include "linux/gfp_types.h"
> +#include "net/bluetooth/bluetooth.h"
> +#include "net/bluetooth/hci.h"
> +#include <linux/module.h>
> +#include <linux/firmware.h>
> +#include <linux/of.h>
> +#include <linux/serdev.h>
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +#include <net/wilc.h>
> +
> +#include "hci_uart.h"
> +
> +#define WILC_BT_UART_MANUFACTURER      205
> +#define WILC_UART_DEFAULT_BAUDRATE     115200
> +#define WILC_UART_BAUDRATE             460800
> +
> +#define HCI_VERSION_BOOTROM    0xFF
> +#define HCI_VERSION_FIRMWARE   0x06
> +
> +#define HCI_VENDOR_CMD_WRITE_MEM       0xFC52
> +#define HCI_VENDOR_CMD_UPDATE_UART     0xFC53
> +#define HCI_VENDOR_CMD_UPDATE_ADDR     0xFC54
> +#define HCI_VENDOR_CMD_RESET           0xFC55
> +#define HCI_VENDOR_CMD_READ_REG                0xFC01
> +
> +struct wilc_adapter {
> +       struct hci_uart hu;
> +       struct device *dev;
> +       void *wlan_priv;
> +       bool flow_control;
> +};
> +
> +struct wilc_data {
> +       struct sk_buff *rx_skb;
> +       struct sk_buff_head txq;
> +};
> +
> +struct hci_update_uart_param {
> +       __le32 baudrate;
> +       __u8 flow_control;
> +} __packed;
> +
> +static int wilc_open(struct hci_uart *hu)
> +{
> +       struct wilc_data *wdata;
> +
> +       BT_DBG("hci_wilc: open");

Afaik you don't need to include the function name with the likes of
pr_debug/BT_DBG, that said you should really be using bt_dev_dbg if
you have hu->hdev set at this point, and this is valid for all other
places where BT_DBG could be replaced with bt_dev_dbg.

> +       wdata = kzalloc(sizeof(*wdata), GFP_KERNEL);
> +       if (!wdata)
> +               return -ENOMEM;

Add an empty after something like an if statement to make it clearer
that it is not under the same scope.

> +       skb_queue_head_init(&wdata->txq);
> +       hu->priv = wdata;
> +
> +       return 0;
> +}
> +
> +static int wilc_close(struct hci_uart *hu)
> +{
> +       struct wilc_data *wdata = hu->priv;
> +
> +       BT_DBG("hci_wilc: close");
> +       skb_queue_purge(&wdata->txq);
> +       kfree_skb(wdata->rx_skb);
> +       kfree(wdata);
> +       hu->priv = NULL;
> +       return 0;
> +}
> +
> +static int wilc_flush(struct hci_uart *hu)
> +{
> +       struct wilc_data *wdata = hu->priv;
> +
> +       BT_DBG("hci_wilc: flush");
> +       skb_queue_purge(&wdata->txq);
> +       return 0;
> +}
> +
> +static const struct h4_recv_pkt wilc_bt_recv_pkts[] = {
> +       { H4_RECV_ACL, .recv = hci_recv_frame },
> +       { H4_RECV_SCO, .recv = hci_recv_frame },
> +       { H4_RECV_EVENT, .recv = hci_recv_frame },
> +};
> +
> +static int wilc_recv(struct hci_uart *hu, const void *data, int len)
> +{
> +       struct wilc_data *wdata = hu->priv;
> +       int err;
> +
> +       if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
> +               return -EUNATCH;

Ditto, add empty line

> +       wdata->rx_skb = h4_recv_buf(hu->hdev, wdata->rx_skb, data, len,
> +                                   wilc_bt_recv_pkts,
> +                                   ARRAY_SIZE(wilc_bt_recv_pkts));
> +       if (IS_ERR(wdata->rx_skb)) {
> +               err = PTR_ERR(wdata->rx_skb);
> +               bt_dev_err(hu->hdev, "Frame reassembly failed (%d)", err);
> +               wdata->rx_skb = NULL;
> +       }
> +
> +       return len;
> +}
> +
> +static int wilc_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> +{
> +       struct wilc_data *wdata = hu->priv;
> +
> +       BT_DBG("hci_wilc: enqueue skb %pK", skb);
> +       memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
> +       skb_queue_tail(&wdata->txq, skb);
> +       return 0;
> +}
> +
> +static struct sk_buff *wilc_dequeue(struct hci_uart *hu)
> +{
> +       struct wilc_data *wdata = hu->priv;
> +
> +       BT_DBG("hci_wilc: dequeue skb");
> +       return skb_dequeue(&wdata->txq);
> +}
> +
> +static int _set_uart_settings(struct hci_uart *hu, unsigned int speed,
> +                             bool flow_control)
> +{
> +       struct hci_update_uart_param param;
> +       int ret;
> +
> +       param.baudrate = cpu_to_le32(speed);
> +       param.flow_control = flow_control ? 1 : 0;
> +       ret = __hci_cmd_sync_status(hu->hdev, HCI_VENDOR_CMD_UPDATE_UART,
> +                                   sizeof(param), &param, HCI_CMD_TIMEOUT);
> +       if (ret) {
> +               BT_ERR("Failed to update UART settings");
> +               return ret;
> +       }
> +
> +       serdev_device_set_baudrate(hu->serdev, speed);
> +       serdev_device_set_flow_control(hu->serdev, flow_control);
> +
> +       return 0;
> +}
> +
> +static int wilc_set_baudrate(struct hci_uart *hu, unsigned int speed)
> +{
> +       struct wilc_adapter *wilc_adapter;
> +
> +       BT_INFO("WILC uart settings update request: speed=%d", speed);
> +       wilc_adapter = serdev_device_get_drvdata(hu->serdev);
> +
> +       return _set_uart_settings(hu, speed, wilc_adapter->flow_control);
> +}
> +
> +static int check_firmware_running(struct hci_uart *hu)
> +{
> +       struct hci_rp_read_local_version *version;
> +       struct sk_buff *skb;
> +       int ret = 0;
> +
> +       BT_DBG("Resetting bluetooth chip");
> +       ret = __hci_cmd_sync_status(hu->hdev, HCI_OP_RESET, 0, NULL,
> +                                   HCI_CMD_TIMEOUT);
> +       if (ret) {
> +               BT_ERR("Can not reset wilc");
> +               return ret;
> +       }
> +
> +       BT_DBG("Checking chip state");
> +       skb = __hci_cmd_sync(hu->hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
> +                            HCI_CMD_TIMEOUT);
> +       if (IS_ERR(skb)) {
> +               BT_ERR("Error while checking bootrom");
> +               return PTR_ERR(skb);
> +       }
> +
> +       if (skb->len != sizeof(struct hci_rp_read_local_version)) {
> +               BT_ERR("Can not read local version");
> +               return -1;
> +       }

Ditto, add an empty line.

> +       version = (struct hci_rp_read_local_version *)skb->data;
> +       BT_DBG("Status: 0x%1X, HCI version: 0x%1X", version->status,
> +              version->hci_ver);
> +       kfree_skb(skb);
> +       if (version->hci_ver != HCI_VERSION_FIRMWARE) {
> +               BT_ERR("Bluetooth firmware is not running !");
> +               if (version->hci_ver == HCI_VERSION_BOOTROM)
> +                       BT_WARN("Bootrom is running");
> +               return 1;
> +       }

Ditto

> +       BT_DBG("Firmware is running");
> +       return 0;
> +}
> +
> +static int wilc_setup(struct hci_uart *hu)
> +{
> +       struct wilc_adapter *wilc_adapter;
> +       int ret;
> +
> +       BT_DBG("hci_wilc: setup");
> +       serdev_device_set_baudrate(hu->serdev, WILC_UART_DEFAULT_BAUDRATE);
> +       serdev_device_set_flow_control(hu->serdev, false);
> +       ret = check_firmware_running(hu);
> +       if (ret)
> +               return ret;
> +
> +       BT_DBG("Updating firmware uart settings");
> +
> +       wilc_adapter = serdev_device_get_drvdata(hu->serdev);
> +       ret = _set_uart_settings(&wilc_adapter->hu, WILC_UART_BAUDRATE, true);
> +       if (ret) {
> +               BT_ERR("Failed to reconfigure firmware uart settings");
> +               return ret;
> +       }

Ditto

> +       wilc_adapter->flow_control = true;
> +
> +       BT_INFO("Wilc successfully initialized");
> +       return ret;
> +}
> +
> +static const struct hci_uart_proto wilc_bt_proto = {
> +       .id = HCI_UART_WILC,
> +       .name = "Microchip",
> +       .manufacturer = WILC_BT_UART_MANUFACTURER,
> +       .init_speed = WILC_UART_DEFAULT_BAUDRATE,
> +       .open = wilc_open,
> +       .close = wilc_close,
> +       .flush = wilc_flush,
> +       .recv = wilc_recv,
> +       .enqueue = wilc_enqueue,
> +       .dequeue = wilc_dequeue,
> +       .setup = wilc_setup,
> +       .set_baudrate = wilc_set_baudrate,
> +};
> +
> +static int wilc_bt_serdev_probe(struct serdev_device *serdev)
> +{
> +       struct wilc_adapter *wilc_adapter;
> +       struct device_node *wlan_node;
> +       void *wlan = NULL;
> +       int ret;
> +
> +       wilc_adapter = kzalloc(sizeof(*wilc_adapter), GFP_KERNEL);
> +       if (!wilc_adapter)
> +               return -ENOMEM;
> +
> +       wlan_node = of_parse_phandle(serdev->dev.of_node, "wlan", 0);
> +       if (!wlan_node) {
> +               BT_ERR("Can not run wilc bluetooth without wlan node");
> +               ret = -EINVAL;
> +               goto exit_free_adapter;
> +       }
> +
> +#if IS_ENABLED(CONFIG_WILC1000_SDIO)
> +       wlan = wilc_sdio_get_byphandle(wlan_node);
> +#endif
> +#if IS_ENABLED(CONFIG_WILC1000_SPI)
> +       if (!wlan || wlan == ERR_PTR(-EPROBE_DEFER))
> +               wlan = wilc_spi_get_byphandle(wlan_node);
> +#endif
> +       if (IS_ERR(wlan)) {
> +               pr_warn("Can not initialize bluetooth: %pe\n", wlan);
> +               ret = PTR_ERR(wlan);
> +               goto exit_put_wlan_node;
> +       }
> +
> +       of_node_put(wlan_node);
> +       wilc_adapter->wlan_priv = wlan;
> +       ret = wilc_bt_init(wlan);
> +       if (ret) {
> +               pr_err("Failed to initialize bluetooth firmware (%d)\n", ret);
> +               goto exit_put_wlan;
> +       }
> +
> +       wilc_adapter->dev = &serdev->dev;
> +       wilc_adapter->hu.serdev = serdev;
> +       wilc_adapter->flow_control = false;
> +       serdev_device_set_drvdata(serdev, wilc_adapter);
> +       ret = hci_uart_register_device(&wilc_adapter->hu, &wilc_bt_proto);
> +       if (ret) {
> +               dev_err(&serdev->dev, "Failed to register hci device");
> +               goto exit_deinit_bt;
> +       }
> +
> +       dev_info(&serdev->dev, "WILC hci interface registered");
> +       return 0;
> +
> +exit_deinit_bt:
> +       wilc_bt_shutdown(wlan);
> +exit_put_wlan:
> +       wilc_put(wlan);
> +exit_put_wlan_node:
> +       of_node_put(wlan_node);
> +exit_free_adapter:
> +       kfree(wilc_adapter);
> +       return ret;
> +}
> +
> +static void wilc_bt_serdev_remove(struct serdev_device *serdev)
> +{
> +       struct wilc_adapter *wilc_adapter = serdev_device_get_drvdata(serdev);
> +
> +       hci_uart_unregister_device(&wilc_adapter->hu);
> +       wilc_bt_shutdown(wilc_adapter->wlan_priv);
> +       wilc_put(wilc_adapter->wlan_priv);
> +       kfree(wilc_adapter);
> +}
> +
> +static const struct of_device_id wilc_bt_of_match[] = {
> +       { .compatible = "microchip,wilc3000-bt" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, wilc_bt_of_match);
> +
> +static struct serdev_device_driver wilc_bt_serdev_driver = {
> +       .probe = wilc_bt_serdev_probe,
> +       .remove = wilc_bt_serdev_remove,
> +       .driver = {
> +               .name = "hci_uart_wilc",
> +               .of_match_table = of_match_ptr(wilc_bt_of_match),
> +       },
> +};
> +
> +module_serdev_device_driver(wilc_bt_serdev_driver)
> +MODULE_AUTHOR("Alexis Lothoré <alexis.lothore@...tlin.com>");
> +MODULE_DESCRIPTION("Bluetooth HCI Uart for WILC devices");
> +MODULE_LICENSE("GPL");
>
> --
> 2.48.0

Once you address these comments please fill free to add:

Reviewed-by: Luiz Augusto von Dentz <luiz.von.dentz@...el.com>

-- 
Luiz Augusto von Dentz

Powered by blists - more mailing lists