Message-ID: <AS8PR04MB8404D3C45734179D80BC242092AB9@AS8PR04MB8404.eurprd04.prod.outlook.com>
Date:   Thu, 23 Feb 2023 11:32:42 +0000
From:   Sherry Sun <sherry.sun@....com>
To:     Neeraj sanjay kale <neeraj.sanjaykale@....com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "marcel@...tmann.org" <marcel@...tmann.org>,
        "johan.hedberg@...il.com" <johan.hedberg@...il.com>,
        "luiz.dentz@...il.com" <luiz.dentz@...il.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "jirislaby@...nel.org" <jirislaby@...nel.org>,
        "alok.a.tiwari@...cle.com" <alok.a.tiwari@...cle.com>,
        "hdanton@...a.com" <hdanton@...a.com>,
        "ilpo.jarvinen@...ux.intel.com" <ilpo.jarvinen@...ux.intel.com>,
        "leon@...nel.org" <leon@...nel.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        Amitkumar Karwar <amitkumar.karwar@....com>,
        Rohit Fule <rohit.fule@....com>
Subject: RE: [PATCH v5] Bluetooth: NXP: Add protocol support for NXP Bluetooth
 chipsets

> -----Original Message-----
> From: Neeraj sanjay kale <neeraj.sanjaykale@....com>
> Sent: 2023年2月23日 18:36
> To: davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> pabeni@...hat.com; robh+dt@...nel.org;
> krzysztof.kozlowski+dt@...aro.org; marcel@...tmann.org;
> johan.hedberg@...il.com; luiz.dentz@...il.com;
> gregkh@...uxfoundation.org; jirislaby@...nel.org; alok.a.tiwari@...cle.com;
> hdanton@...a.com; ilpo.jarvinen@...ux.intel.com; leon@...nel.org
> Cc: netdev@...r.kernel.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; linux-bluetooth@...r.kernel.org; linux-
> serial@...r.kernel.org; Amitkumar Karwar <amitkumar.karwar@....com>;
> Rohit Fule <rohit.fule@....com>; Sherry Sun <sherry.sun@....com>; Neeraj
> sanjay kale <neeraj.sanjaykale@....com>
> Subject: [PATCH v5] 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
> an 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.
> 
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@....com>
> ---
> 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)
> v4: Removed conf file support, optimized driver data, add logic to select FW
> name based on chip signature (Greg KH, Ilpo Jarvinen, Sherry Sun)
> v5: Replaced bt_dev_info() with bt_dev_dbg(), handled user-space cmd
> parsing in nxp_enqueue() in a better way. (Greg KH, Luiz Augusto von Dentz)
> ---
>  MAINTAINERS                   |    1 +
>  drivers/bluetooth/Kconfig     |   11 +
>  drivers/bluetooth/Makefile    |    1 +
>  drivers/bluetooth/btnxpuart.c | 1312
> +++++++++++++++++++++++++++++++++
>  4 files changed, 1325 insertions(+)
>  create mode 100644 drivers/bluetooth/btnxpuart.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 030ec6fe89df..fdb9b0788c89 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22840,6 +22840,7 @@ M:	Amitkumar Karwar
> <amitkumar.karwar@....com>
>  M:	Neeraj Kale <neeraj.sanjaykale@....com>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-
> bt.yaml
> +F:	drivers/bluetooth/btnxpuart.c
> 
>  THE REST
>  M:	Linus Torvalds <torvalds@...ux-foundation.org>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig index
> 5a1a7bec3c42..359a4833e31f 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 (btnxpuart).
> +
> +
>  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..55f6bf7c5d87
> --- /dev/null
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -0,0 +1,1312 @@
> +// 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 MANUFACTURER_NXP		37
> +
> +#define BTNXPUART_TX_STATE_ACTIVE	1
> +#define BTNXPUART_FW_DOWNLOADING	2
> +
> +#define FIRMWARE_W8987	"nxp/uartuart8987_bt.bin"
> +#define FIRMWARE_W8997	"nxp/uartuart8997_bt_v4.bin"
> +#define FIRMWARE_W9098	"nxp/uartuart9098_bt_v1.bin"
> +#define FIRMWARE_IW416	"nxp/uartiw416_bt_v0.bin"
> +#define FIRMWARE_IW612	"nxp/uartspi_n61x_v1.bin.se"
> +
> +#define CHIP_ID_W9098		0x5c03
> +#define CHIP_ID_IW416		0x7201
> +#define CHIP_ID_IW612		0x7601
> +
> +#define HCI_NXP_PRI_BAUDRATE	115200
> +#define HCI_NXP_SEC_BAUDRATE	3000000
> +
> +#define MAX_FW_FILE_NAME_LEN    50
> +
> +/* Default ps timeout period in milli-second */
> +#define PS_DEFAULT_TIMEOUT_PERIOD     2000
> +
> +/* wakeup methods */
> +#define WAKEUP_METHOD_DTR       0
> +#define WAKEUP_METHOD_BREAK     1
> +#define WAKEUP_METHOD_EXT_BREAK 2
> +#define WAKEUP_METHOD_RTS       3
> +#define WAKEUP_METHOD_INVALID   0xff
> +
> +/* power save mode status */
> +#define PS_MODE_DISABLE         0
> +#define PS_MODE_ENABLE          1
> +
> +/* Power Save Commands to ps_work_func  */
> +#define PS_CMD_EXIT_PS          1
> +#define PS_CMD_ENTER_PS         2
> +
> +/* power save state */
> +#define PS_STATE_AWAKE          0
> +#define PS_STATE_SLEEP          1
> +
> +/* Bluetooth vendor command : Sleep mode */
> +#define HCI_NXP_AUTO_SLEEP_MODE	0xfc23
> +/* Bluetooth vendor command : Wakeup method */
> +#define HCI_NXP_WAKEUP_METHOD	0xfc53
> +/* Bluetooth vendor command : Set operational baudrate */
> +#define HCI_NXP_SET_OPER_SPEED	0xfc09
> +/* Bluetooth vendor command: Independent Reset */
> +#define HCI_NXP_IND_RESET	0xfcfc
> +
> +/* Bluetooth Power State : Vendor cmd params */
> +#define BT_PS_ENABLE			0x02
> +#define BT_PS_DISABLE			0x03
> +
> +

......

> +static u8 *nxp_get_fw_name_from_chipid(struct hci_dev *hdev, u16
> +chipid) {
> +	u8 *fw_name = NULL;
> +
> +	switch (chipid) {
> +	case CHIP_ID_W9098:
> +		fw_name = FIRMWARE_W9098;
> +		break;
> +	case CHIP_ID_IW416:
> +		fw_name = FIRMWARE_IW416;
> +		break;
> +	case CHIP_ID_IW612:
> +		fw_name = FIRMWARE_IW612;

Suppose for each V3 BT chips, you not only need to update the fw_name, but also fw_dnld_use_high_baudrate. Don't use the default value in drv_data for all chips here.

> +		break;
> +	default:
> +		bt_dev_err(hdev, "Unknown chip signature %04X", chipid);
> +		break;
> +	}
> +	return fw_name;
> +}
> +
> +static int nxp_recv_chip_ver_v3(struct hci_dev *hdev, struct sk_buff
> +*skb) {
> +	struct v3_start_ind *req = skb_pull_data(skb, sizeof(struct
> v3_start_ind));
> +	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +	int err;
> +
> +	if (!test_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state))
> +		goto ret;
> +
> +	if (!strlen(nxpdev->fw_name)) {
> +		snprintf(nxpdev->fw_name, MAX_FW_FILE_NAME_LEN, "%s",
> +			 nxp_get_fw_name_from_chipid(hdev, req->chip_id));
> +
> +		bt_dev_info(hdev, "Request Firmware: %s", nxpdev-
> >fw_name);
> +		err = request_firmware(&nxpdev->fw, nxpdev->fw_name,
> &hdev->dev);
> +		if (err < 0) {
> +			bt_dev_err(hdev, "Firmware file %s not found",
> nxpdev->fw_name);
> +			clear_bit(BTNXPUART_FW_DOWNLOADING,
> &nxpdev->tx_state);
> +			goto ret;
> +		}
> +	}
> +	nxp_send_ack(NXP_ACK_V3, hdev);
> +ret:
> +	kfree_skb(skb);
> +	return 0;
> +}
> +
> +static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff
> +*skb) {
> +	struct v3_data_req *req = skb_pull_data(skb, sizeof(struct
> v3_data_req));
> +	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +
> +	if (!req || !nxpdev || !nxpdev->fw)
> +		goto ret;
> +
> +	if (!test_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state))
> +		goto ret;
> +
> +	nxp_send_ack(NXP_ACK_V3, hdev);
> +
> +	if (!nxpdev->timeout_changed) {
> +		nxpdev->timeout_changed = nxp_fw_change_timeout(hdev,
> req->len);
> +		goto ret;
> +	}
> +
> +	if (!nxpdev->baudrate_changed) {
> +		nxpdev->baudrate_changed =
> nxp_fw_change_baudrate(hdev, req->len);
> +		if (nxpdev->baudrate_changed) {
> +			serdev_device_set_baudrate(nxpdev->serdev,
> +						   HCI_NXP_SEC_BAUDRATE);
> +			serdev_device_set_flow_control(nxpdev->serdev, 1);
> +			nxpdev->current_baudrate =
> HCI_NXP_SEC_BAUDRATE;
> +		}
> +		goto ret;
> +	}
> +
> +	if (req->len == 0) {
> +		bt_dev_info(hdev, "FW Downloaded Successfully: %zu bytes",
> nxpdev->fw->size);
> +		clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev-
> >tx_state);
> +		wake_up_interruptible(&nxpdev->suspend_wait_q);
> +		goto ret;
> +	}
> +	if (req->error)
> +		bt_dev_dbg(hdev, "FW Download received err 0x%02x from
> chip. Resending FW chunk.",
> +			   req->error);
> +
> +	if (req->offset < nxpdev->fw_v3_offset_correction) {
> +		/* This scenario should ideally never occur.
> +		 * But if it ever does, FW is out of sync and
> +		 * needs a power cycle.
> +		 */
> +		bt_dev_err(hdev, "Something went wrong during FW
> download. Please power cycle and try again");
> +		goto ret;
> +	}
> +
> +	serdev_device_write_buf(nxpdev->serdev,
> +				nxpdev->fw->data + req->offset - nxpdev-
> >fw_v3_offset_correction,
> +				req->len);
> +
> +ret:
> +	kfree_skb(skb);
> +	return 0;
> +}
> +
> +static int nxp_set_baudrate_cmd(struct hci_dev *hdev, void *data) {
> +	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +	u32 new_baudrate = __cpu_to_le32(nxpdev->new_baudrate);
> +	struct ps_data *psdata = nxpdev->psdata;
> +	u8 *pcmd = (u8 *)&new_baudrate;
> +	struct sk_buff *skb;
> +	u8 *status;
> +
> +	if (!psdata)
> +		return 0;
> +
> +	skb = nxp_drv_send_cmd(hdev, HCI_NXP_SET_OPER_SPEED, 4,
> pcmd);
> +	if (IS_ERR(skb)) {
> +		bt_dev_err(hdev, "Setting baudrate failed (%ld)",
> PTR_ERR(skb));
> +		return PTR_ERR(skb);
> +	}
> +
> +	status = skb_pull_data(skb, 1);
> +	if (status) {
> +		if (*status == 0) {
> +			serdev_device_set_baudrate(nxpdev->serdev,
> nxpdev->new_baudrate);
> +			nxpdev->current_baudrate = nxpdev->new_baudrate;
> +		}
> +		bt_dev_dbg(hdev, "Set baudrate response: status=%d,
> baudrate=%d",
> +			   *status, nxpdev->new_baudrate);
> +	}
> +	kfree_skb(skb);
> +
> +	return 0;
> +}
> +
> +static int nxp_set_ind_reset(struct hci_dev *hdev, void *data) {
> +	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +	struct sk_buff *skb;
> +	u8 *status;
> +	u8 pcmd = 0;
> +	int err;
> +
> +	skb = nxp_drv_send_cmd(hdev, HCI_NXP_IND_RESET, 1, &pcmd);
> +	if (IS_ERR(skb))
> +		return PTR_ERR(skb);
> +
> +	status = skb_pull_data(skb, 1);
> +	if (status) {
> +		if (*status == 0) {
> +			set_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev-
> >tx_state);
> +			err = nxp_download_firmware(hdev);
> +			if (err < 0)
> +				return err;
> +			serdev_device_set_baudrate(nxpdev->serdev,
> init_baudrate);
> +			nxpdev->current_baudrate = init_baudrate;
> +			if (nxpdev->current_baudrate !=
> HCI_NXP_SEC_BAUDRATE) {
> +				nxpdev->new_baudrate =
> HCI_NXP_SEC_BAUDRATE;
> +				nxp_set_baudrate_cmd(hdev, NULL);
> +			}
> +		}
> +	}
> +	kfree_skb(skb);
> +
> +	return 0;
> +}
> +
> +/* NXP protocol */
> +static int nxp_setup(struct hci_dev *hdev) {
> +	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +	int err = 0;
> +
> +	if (!nxpdev)
> +		return 0;
> +
> +	set_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
> +	init_waitqueue_head(&nxpdev->suspend_wait_q);
> +
> +	if (!serdev_device_get_cts(nxpdev->serdev)) {
> +		bt_dev_dbg(hdev, "CTS high. Need FW Download");

I don't think it's a good idea to use CTS status to determine the BT FW status, because many uart drivers only support auto hardware flow control, and cannot return the CTS line state.
Such as for LPUART, serdev_device_get_cts() will always return TIOCM_CTS, so here FW may never be downloaded.

Best Regards
Sherry

Powered by blists - more mailing lists