RE: [PATCH v3 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets

From: Sherry Sun
Date: Thu Feb 16 2023 - 05:52:57 EST

Next message: Baolin Wang: "Re: [PATCH v2 1/2] mm/memory_hotplug: return nothing from do_migrate_range()"
Previous message: Chen-Yu Tsai: "Re: [PATCH v2 23/47] clk: mediatek: mt7622: Move apmixedsys clock driver to its own file"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Neeraj sanjay kale <neeraj.sanjaykale@xxxxxxx>
> Sent: 2023年2月13日 22:55
> To: davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; marcel@xxxxxxxxxxxx;
> johan.hedberg@xxxxxxxxx; luiz.dentz@xxxxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx; jirislaby@xxxxxxxxxx; alok.a.tiwari@xxxxxxxxxx;
> hdanton@xxxxxxxx; ilpo.jarvinen@xxxxxxxxxxxxxxx; leon@xxxxxxxxxx
> Cc: netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-bluetooth@xxxxxxxxxxxxxxx; linux-
> serial@xxxxxxxxxxxxxxx; Amitkumar Karwar <amitkumar.karwar@xxxxxxx>;
> Rohit Fule <rohit.fule@xxxxxxx>; Sherry Sun <sherry.sun@xxxxxxx>; Neeraj
> sanjay kale <neeraj.sanjaykale@xxxxxxx>
> Subject: [PATCH v3 3/3] Bluetooth: NXP: Add protocol support for NXP
> Bluetooth chipsets
>
> This adds a driver based on serdev driver for the NXP BT serial protocol
> based on running H:4, which can enable the built-in Bluetooth device inside a
> generic NXP BT chip.
>
> This driver has Power Save feature that will put the chip into sleep state
> whenever there is no activity for 2000ms, and will be woken up when any
> activity is to be initiated over UART.
>
> This driver enables the power save feature by default by sending the vendor
> specific commands to the chip during setup.
>
> During setup, the driver checks if a FW is already running on the chip based
> on the CTS line, and downloads device specific FW file into the chip over
> UART.
>
> The driver contains certain device specific default parameters related to FW
> filename, baudrate and timeouts which can be overwritten by an optional
> user space config file. These parameters may vary from one module vendor
> to another.
>
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx>
> ---
> v2: Removed conf file support and added static data for each chip based on
> compatibility devices mentioned in DT bindings. Handled potential memory
> leaks and null pointer dereference issues, simplified FW download feature,
> handled byte-order and few cosmetic changes. (Ilpo Järvinen, Alok Tiwari,
> Hillf Danton)
> v3: Added conf file support necessary to support different vendor modules,
> moved .h file contents to .c, cosmetic changes. (Luiz Augusto von Dentz, Rob
> Herring, Leon Romanovsky)
> ---
> drivers/bluetooth/Kconfig | 11 +
> drivers/bluetooth/Makefile | 1 +
> drivers/bluetooth/btnxpuart.c | 1370
> +++++++++++++++++++++++++++++++++
> 3 files changed, 1382 insertions(+)
> create mode 100644 drivers/bluetooth/btnxpuart.c
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig index
> 5a1a7bec3c42..773b40d34b7b 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -465,4 +465,15 @@ config BT_VIRTIO
> Say Y here to compile support for HCI over Virtio into the
> kernel or say M to compile as a module.
>
> +config BT_NXPUART
> + tristate "NXP protocol support"
> + depends on SERIAL_DEV_BUS
> + help
> + NXP is serial driver required for NXP Bluetooth
> + devices with UART interface.
> +
> + Say Y here to compile support for NXP Bluetooth UART device into
> + the kernel, or say M here to compile as a module.
> +
> +
> endmenu
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile index
> e0b261f24fc9..7a5967e9ac48 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_BT_QCA) += btqca.o
> obj-$(CONFIG_BT_MTK) += btmtk.o
>
> obj-$(CONFIG_BT_VIRTIO) += virtio_bt.o
> +obj-$(CONFIG_BT_NXPUART) += btnxpuart.o
>
> obj-$(CONFIG_BT_HCIUART_NOKIA) += hci_nokia.o
>
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> new file mode 100644 index 000000000000..d774564d47ce
> --- /dev/null
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -0,0 +1,1370 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * NXP Bluetooth driver
> + * Copyright 2018-2023 NXP
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +
> +#include <linux/serdev.h>
> +#include <linux/of.h>
> +#include <linux/skbuff.h>
> +#include <asm/unaligned.h>
> +#include <linux/firmware.h>
> +#include <linux/string.h>
> +#include <linux/crc8.h>
> +
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include "h4_recv.h"
> +
> +#define BTNXPUART_TX_STATE_ACTIVE 1
> +#define BTNXPUART_FW_DOWNLOADING 2
> +
> +#define MAX_TAG_STR_LEN 20
> +#define BT_FW_CONF_FILE
> "nxp/bt_mod_para.conf"
> +#define USER_CONFIG_TAG "user_config"
> +#define FW_NAME_TAG "fw_name"
> +#define OPER_SPEED_TAG "oper_speed"
> +#define FW_DL_PRI_BAUDRATE_TAG "fw_dl_pri_speed"
> +#define FW_DL_SEC_BAUDRATE_TAG "fw_dl_sec_speed"
> +#define FW_INIT_BAUDRATE "fw_init_speed"
> +#define PS_INTERVAL_MS "ps_interval_ms"
> +
> +#define FIRMWARE_W8987 "nxp/sdiouart8987_combo_v0.bin"

Why here use combo firmware?

> +#define FIRMWARE_W8997 "nxp/uartuart8997_bt_v4.bin"
> +#define FIRMWARE_W9098 "nxp/uartuart9098_bt_v1.bin"
> +#define FIRMWARE_IW416 "nxp/sdiouartiw416_combo_v0.bin"
> +#define FIRMWARE_IW612 "nxp/sduart_nw61x_v1.bin.se"

Same for IW416 and IW612, please use BT-only firmware.

> +
> +#define MAX_FW_FILE_NAME_LEN 50
> +
> +/* Default ps timeout period in milli-second */
> +#define PS_DEFAULT_TIMEOUT_PERIOD 2000
> +

......

> +static void ps_init_timer(struct hci_dev *hdev) {
> + struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> + struct ps_data *psdata = nxpdev->psdata;
> +
> + psdata->timer_on = 0;
> + timer_setup(&psdata->ps_timer, ps_timeout_func, 0); }
> +
> +static int ps_wakeup(struct btnxpuart_dev *nxpdev) {

The return value of ps_wakeup() does not seem to be used, why not set its return value to void?

> + struct ps_data *psdata = nxpdev->psdata;
> +
> + if (psdata->ps_state == PS_STATE_AWAKE)
> + return 0;
> + psdata->ps_cmd = PS_CMD_EXIT_PS;
> + schedule_work(&psdata->work);
> +
> + return 1;
> +}
> +

......

> +
> +/* Following default values are as per Murata M.2 modules
> + * For modules from different vendor, if any of the device
> + * parameters are different, they can be over-written by
> + * config file /lib/firmware/nxp/bt_mod_para.conf
> + */
> +static struct btnxpuart_data w8987_data = {
> + .fw_dnld_pri_baudrate = 115200,
> + .fw_dnld_sec_baudrate = 3000000,
> + .fw_init_baudrate = 115200,
> + .oper_speed = 3000000,

Do we really need so much different speed settings here?
Such as for fw_dnld_pri_baudrate, if all chips default speed is 115200, how about set it directly in the code? Same for oper_speed. These two speed should be same for all nxp BT chips I think, right?

> + .ps_interval_ms = PS_DEFAULT_TIMEOUT_PERIOD,

This value is also same for all chips, so you don't need the struct variable here. Just use the value in the code directly.

> + .fw_name = FIRMWARE_W8987,
> +};
> +
> +static struct btnxpuart_data w8997_data = {
> + .fw_dnld_pri_baudrate = 115200,
> + .fw_dnld_sec_baudrate = 115200,
> + .fw_init_baudrate = 115200,
> + .oper_speed = 3000000,
> + .ps_interval_ms = PS_DEFAULT_TIMEOUT_PERIOD,
> + .fw_name = FIRMWARE_W8997,
> +};
> +
> +static struct btnxpuart_data w9098_data = {
> + .fw_dnld_pri_baudrate = 115200,
> + .fw_dnld_sec_baudrate = 3000000,
> + .fw_init_baudrate = 115200,
> + .oper_speed = 3000000,
> + .ps_interval_ms = PS_DEFAULT_TIMEOUT_PERIOD,
> + .fw_name = FIRMWARE_W9098,
> +};
> +
> +static struct btnxpuart_data iw416_data = {
> + .fw_dnld_pri_baudrate = 115200,
> + .fw_dnld_sec_baudrate = 3000000,
> + .fw_init_baudrate = 115200,
> + .oper_speed = 3000000,
> + .ps_interval_ms = PS_DEFAULT_TIMEOUT_PERIOD,
> + .fw_name = FIRMWARE_IW416,
> +};
> +
> +static struct btnxpuart_data iw612_data = {
> + .fw_dnld_pri_baudrate = 115200,
> + .fw_dnld_sec_baudrate = 3000000,
> + .fw_init_baudrate = 115200,
> + .oper_speed = 3000000,
> + .ps_interval_ms = PS_DEFAULT_TIMEOUT_PERIOD,
> + .fw_name = FIRMWARE_IW612,
> +};
> +
> +static const struct of_device_id nxpuart_of_match_table[] = {
> + { .compatible = "nxp,88w8987-bt", .data = &w8987_data },
> + { .compatible = "nxp,88w8997-bt", .data = &w8997_data },
> + { .compatible = "nxp,88w9098-bt", .data = &w9098_data },
> + { .compatible = "nxp,iw416-bt", .data = &iw416_data },
> + { .compatible = "nxp,iw612-bt", .data = &iw612_data },
> + { }

As I commented in the patch 2, it will be better to use a common compatible for all the chips that support V3 bootloader, which can set the different parameters according to the different Chip ID from bootloader. Otherwise the table will get longer and longer when new BT chips added later.
For the legacy chips, if the bootloader don't support chip ID, maybe you can continue to use the different dts compatible to distinguish them.

Best Regards
Sherry

> +};
> +MODULE_DEVICE_TABLE(of, nxpuart_of_match_table);
> +
> +static struct serdev_device_driver nxp_serdev_driver = {
> + .probe = nxp_serdev_probe,
> + .remove = nxp_serdev_remove,
> + .driver = {
> + .name = "btnxpuart",
> + .of_match_table = of_match_ptr(nxpuart_of_match_table),
> + },
> +};
> +
> +module_serdev_device_driver(nxp_serdev_driver);
> +
> +MODULE_AUTHOR("Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx>");
> +MODULE_DESCRIPTION("NXP Bluetooth Serial driver v1.0 ");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1

Next message: Baolin Wang: "Re: [PATCH v2 1/2] mm/memory_hotplug: return nothing from do_migrate_range()"
Previous message: Chen-Yu Tsai: "Re: [PATCH v2 23/47] clk: mediatek: mt7622: Move apmixedsys clock driver to its own file"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]