linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/3] Add support for NXP bluetooth chipsets
@ 2023-02-21 16:25 Neeraj Sanjay Kale
  2023-02-21 16:25 ` [PATCH v4 1/3] serdev: Add method to assert break signal over tty UART port Neeraj Sanjay Kale
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Neeraj Sanjay Kale @ 2023-02-21 16:25 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
	marcel, johan.hedberg, luiz.dentz, gregkh, jirislaby,
	alok.a.tiwari, hdanton, ilpo.jarvinen, leon
  Cc: netdev, devicetree, linux-kernel, linux-bluetooth, linux-serial,
	amitkumar.karwar, rohit.fule, sherry.sun, neeraj.sanjaykale

This patch adds a driver for NXP bluetooth chipsets.

The driver is based on H4 protocol, and uses serdev APIs. It supports host
to chip power save feature, which is signalled by the host by asserting
break over UART TX lines, to put the chip into sleep state.

To support this feature, break_ctl has also been added to serdev-tty along
with a new serdev API serdev_device_break_ctl().

This driver is capable of downloading firmware into the chip over UART.

The document specifying device tree bindings for this driver is also
included in this patch series.

Neeraj Sanjay Kale (3):
  serdev: Add method to assert break signal over tty UART port
  dt-bindings: net: bluetooth: Add NXP bluetooth support
  Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets

 .../bindings/net/bluetooth/nxp,w8987-bt.yaml  |   38 +
 MAINTAINERS                                   |    7 +
 drivers/bluetooth/Kconfig                     |   11 +
 drivers/bluetooth/Makefile                    |    1 +
 drivers/bluetooth/btnxpuart.c                 | 1292 +++++++++++++++++
 drivers/tty/serdev/core.c                     |   11 +
 drivers/tty/serdev/serdev-ttyport.c           |   17 +
 include/linux/serdev.h                        |    6 +
 8 files changed, 1383 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/bluetooth/nxp,w8987-bt.yaml
 create mode 100644 drivers/bluetooth/btnxpuart.c

-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v4 1/3] serdev: Add method to assert break signal over tty UART port
  2023-02-21 16:25 [PATCH v4 0/3] Add support for NXP bluetooth chipsets Neeraj Sanjay Kale
@ 2023-02-21 16:25 ` Neeraj Sanjay Kale
  2023-02-21 16:25 ` [PATCH v4 2/3] dt-bindings: net: bluetooth: Add NXP bluetooth support Neeraj Sanjay Kale
  2023-02-21 16:25 ` [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets Neeraj Sanjay Kale
  2 siblings, 0 replies; 10+ messages in thread
From: Neeraj Sanjay Kale @ 2023-02-21 16:25 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
	marcel, johan.hedberg, luiz.dentz, gregkh, jirislaby,
	alok.a.tiwari, hdanton, ilpo.jarvinen, leon
  Cc: netdev, devicetree, linux-kernel, linux-bluetooth, linux-serial,
	amitkumar.karwar, rohit.fule, sherry.sun, neeraj.sanjaykale

Adds serdev_device_break_ctl() and an implementation for ttyport.
This function simply calls the break_ctl in tty layer, which can
assert a break signal over UART-TX line, if the tty and the
underlying platform and UART peripheral supports this operation.

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
v3: Add details to the commit message. (Greg KH)
v4: Add a check for SERPORT_ACTIVE flag before asserting break over
UART-TX.
---
 drivers/tty/serdev/core.c           | 11 +++++++++++
 drivers/tty/serdev/serdev-ttyport.c | 17 +++++++++++++++++
 include/linux/serdev.h              |  6 ++++++
 3 files changed, 34 insertions(+)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 0180e1e4e75d..f2fdd6264e5d 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -405,6 +405,17 @@ int serdev_device_set_tiocm(struct serdev_device *serdev, int set, int clear)
 }
 EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
 
+int serdev_device_break_ctl(struct serdev_device *serdev, int break_state)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (!ctrl || !ctrl->ops->break_ctl)
+		return -EOPNOTSUPP;
+
+	return ctrl->ops->break_ctl(ctrl, break_state);
+}
+EXPORT_SYMBOL_GPL(serdev_device_break_ctl);
+
 static int serdev_drv_probe(struct device *dev)
 {
 	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index d367803e2044..be6044fc0e6d 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -247,6 +247,22 @@ static int ttyport_set_tiocm(struct serdev_controller *ctrl, unsigned int set, u
 	return tty->ops->tiocmset(tty, set, clear);
 }
 
+static int ttyport_break_ctl(struct serdev_controller *ctrl, unsigned int break_state)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+
+	if (!test_bit(SERPORT_ACTIVE, &serport->flags))
+		return -EOPNOTSUPP;
+
+	tty = serport->tty;
+
+	if (!tty->ops->break_ctl)
+		return -EOPNOTSUPP;
+
+	return tty->ops->break_ctl(tty, break_state);
+}
+
 static const struct serdev_controller_ops ctrl_ops = {
 	.write_buf = ttyport_write_buf,
 	.write_flush = ttyport_write_flush,
@@ -259,6 +275,7 @@ static const struct serdev_controller_ops ctrl_ops = {
 	.wait_until_sent = ttyport_wait_until_sent,
 	.get_tiocm = ttyport_get_tiocm,
 	.set_tiocm = ttyport_set_tiocm,
+	.break_ctl = ttyport_break_ctl,
 };
 
 struct device *serdev_tty_port_register(struct tty_port *port,
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index 66f624fc618c..c065ef1c82f1 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -92,6 +92,7 @@ struct serdev_controller_ops {
 	void (*wait_until_sent)(struct serdev_controller *, long);
 	int (*get_tiocm)(struct serdev_controller *);
 	int (*set_tiocm)(struct serdev_controller *, unsigned int, unsigned int);
+	int (*break_ctl)(struct serdev_controller *ctrl, unsigned int break_state);
 };
 
 /**
@@ -202,6 +203,7 @@ int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_
 void serdev_device_wait_until_sent(struct serdev_device *, long);
 int serdev_device_get_tiocm(struct serdev_device *);
 int serdev_device_set_tiocm(struct serdev_device *, int, int);
+int serdev_device_break_ctl(struct serdev_device *serdev, int break_state);
 void serdev_device_write_wakeup(struct serdev_device *);
 int serdev_device_write(struct serdev_device *, const unsigned char *, size_t, long);
 void serdev_device_write_flush(struct serdev_device *);
@@ -255,6 +257,10 @@ static inline int serdev_device_set_tiocm(struct serdev_device *serdev, int set,
 {
 	return -ENOTSUPP;
 }
+static inline int serdev_device_break_ctl(struct serdev_device *serdev, int break_state)
+{
+	return -EOPNOTSUPP;
+}
 static inline int serdev_device_write(struct serdev_device *sdev, const unsigned char *buf,
 				      size_t count, unsigned long timeout)
 {
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v4 2/3] dt-bindings: net: bluetooth: Add NXP bluetooth support
  2023-02-21 16:25 [PATCH v4 0/3] Add support for NXP bluetooth chipsets Neeraj Sanjay Kale
  2023-02-21 16:25 ` [PATCH v4 1/3] serdev: Add method to assert break signal over tty UART port Neeraj Sanjay Kale
@ 2023-02-21 16:25 ` Neeraj Sanjay Kale
  2023-02-22  8:44   ` Krzysztof Kozlowski
  2023-02-21 16:25 ` [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets Neeraj Sanjay Kale
  2 siblings, 1 reply; 10+ messages in thread
From: Neeraj Sanjay Kale @ 2023-02-21 16:25 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
	marcel, johan.hedberg, luiz.dentz, gregkh, jirislaby,
	alok.a.tiwari, hdanton, ilpo.jarvinen, leon
  Cc: netdev, devicetree, linux-kernel, linux-bluetooth, linux-serial,
	amitkumar.karwar, rohit.fule, sherry.sun, neeraj.sanjaykale

Add binding document for NXP bluetooth chipsets attached over UART.

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
v2: Resolved dt_binding_check errors. (Rob Herring)
v2: Modified description, added specific compatibility devices, corrected indentations. (Krzysztof Kozlowski)
v3: Modified description, renamed file (Krzysztof Kozlowski)
v4: Resolved dt_binding_check errors, corrected indentation. (Rob
Herring, Krzysztof Kozlowski)
---
 .../bindings/net/bluetooth/nxp,w8987-bt.yaml  | 38 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/bluetooth/nxp,w8987-bt.yaml

diff --git a/Documentation/devicetree/bindings/net/bluetooth/nxp,w8987-bt.yaml b/Documentation/devicetree/bindings/net/bluetooth/nxp,w8987-bt.yaml
new file mode 100644
index 000000000000..de361ce4ab73
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/bluetooth/nxp,w8987-bt.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/bluetooth/nxp,w8987-bt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP Bluetooth chips
+
+description:
+  This binding describes UART-attached NXP bluetooth chips.
+  These chips are dual-radio chips supporting WiFi and Bluetooth.
+  The bluetooth works on standard H4 protocol over 4-wire UART.
+  The RTS and CTS lines are used during FW download.
+  To enable power save mode, the host asserts break signal
+  over UART-TX line to put the chip into power save state.
+  De-asserting break wakes-up the BT chip.
+
+maintainers:
+  - Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
+
+properties:
+  compatible:
+    enum:
+      - nxp,88w8987-bt
+      - nxp,88w8997-bt
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    uart2 {
+        bluetooth {
+            compatible = "nxp,88w8987-bt";
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 32dd41574930..6d36f52dc124 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22835,6 +22835,12 @@ L:	linux-mm@kvack.org
 S:	Maintained
 F:	mm/zswap.c
 
+NXP BLUETOOTH WIRELESS DRIVERS
+M:	Amitkumar Karwar <amitkumar.karwar@nxp.com>
+M:	Neeraj Kale <neeraj.sanjaykale@nxp.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/bluetooth/nxp,w8987-bt.yaml
+
 THE REST
 M:	Linus Torvalds <torvalds@linux-foundation.org>
 L:	linux-kernel@vger.kernel.org
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets
  2023-02-21 16:25 [PATCH v4 0/3] Add support for NXP bluetooth chipsets Neeraj Sanjay Kale
  2023-02-21 16:25 ` [PATCH v4 1/3] serdev: Add method to assert break signal over tty UART port Neeraj Sanjay Kale
  2023-02-21 16:25 ` [PATCH v4 2/3] dt-bindings: net: bluetooth: Add NXP bluetooth support Neeraj Sanjay Kale
@ 2023-02-21 16:25 ` Neeraj Sanjay Kale
  2023-02-21 16:47   ` Greg KH
  2023-02-21 21:37   ` Luiz Augusto von Dentz
  2 siblings, 2 replies; 10+ messages in thread
From: Neeraj Sanjay Kale @ 2023-02-21 16:25 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
	marcel, johan.hedberg, luiz.dentz, gregkh, jirislaby,
	alok.a.tiwari, hdanton, ilpo.jarvinen, leon
  Cc: netdev, devicetree, linux-kernel, linux-bluetooth, linux-serial,
	amitkumar.karwar, rohit.fule, sherry.sun, neeraj.sanjaykale

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@nxp.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)
---
 MAINTAINERS                   |    1 +
 drivers/bluetooth/Kconfig     |   11 +
 drivers/bluetooth/Makefile    |    1 +
 drivers/bluetooth/btnxpuart.c | 1292 +++++++++++++++++++++++++++++++++
 4 files changed, 1305 insertions(+)
 create mode 100644 drivers/bluetooth/btnxpuart.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 6d36f52dc124..7343f4943458 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22840,6 +22840,7 @@ M:	Amitkumar Karwar <amitkumar.karwar@nxp.com>
 M:	Neeraj Kale <neeraj.sanjaykale@nxp.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/net/bluetooth/nxp,w8987-bt.yaml
+F:	drivers/bluetooth/btnxpuart.c
 
 THE REST
 M:	Linus Torvalds <torvalds@linux-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..608185be2b30
--- /dev/null
+++ b/drivers/bluetooth/btnxpuart.c
@@ -0,0 +1,1292 @@
+// 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
+
+/* Bluetooth Host Wakeup Methods */
+#define BT_HOST_WAKEUP_METHOD_NONE      0x00
+#define BT_HOST_WAKEUP_METHOD_DTR       0x01
+#define BT_HOST_WAKEUP_METHOD_BREAK     0x02
+#define BT_HOST_WAKEUP_METHOD_GPIO      0x03
+
+/* Bluetooth Chip Wakeup Methods */
+#define BT_CTRL_WAKEUP_METHOD_DSR       0x00
+#define BT_CTRL_WAKEUP_METHOD_BREAK     0x01
+#define BT_CTRL_WAKEUP_METHOD_GPIO      0x02
+#define BT_CTRL_WAKEUP_METHOD_EXT_BREAK 0x04
+#define BT_CTRL_WAKEUP_METHOD_RTS       0x05
+
+struct ps_data {
+	u8    ps_mode;
+	u8    cur_psmode;
+	u8    ps_state;
+	u8    ps_cmd;
+	u8    wakeupmode;
+	u8    cur_wakeupmode;
+	bool  driver_sent_cmd;
+	bool  timer_on;
+	u32   interval;
+	struct hci_dev *hdev;
+	struct work_struct work;
+	struct timer_list ps_timer;
+};
+
+struct btnxpuart_data {
+	bool fw_dnld_use_high_baudrate;
+	const u8 *fw_name;
+};
+
+struct btnxpuart_dev {
+	struct hci_dev *hdev;
+	struct serdev_device *serdev;
+
+	struct work_struct tx_work;
+	unsigned long tx_state;
+	struct sk_buff_head txq;
+	struct sk_buff *rx_skb;
+
+	const struct firmware *fw;
+	u8 fw_name[MAX_FW_FILE_NAME_LEN];
+	u32 fw_dnld_v1_offset;
+	u32 fw_v1_sent_bytes;
+	u32 fw_v3_offset_correction;
+	u32 fw_v1_expected_len;
+	wait_queue_head_t suspend_wait_q;
+
+	u32 new_baudrate;
+	u32 current_baudrate;
+	bool timeout_changed;
+	bool baudrate_changed;
+
+	struct ps_data *psdata;
+	struct btnxpuart_data *nxp_data;
+};
+
+#define NXP_V1_FW_REQ_PKT	0xa5
+#define NXP_V1_CHIP_VER_PKT	0xaa
+#define NXP_V3_FW_REQ_PKT	0xa7
+#define NXP_V3_CHIP_VER_PKT	0xab
+
+#define NXP_ACK_V1		0x5a
+#define NXP_NAK_V1		0xbf
+#define NXP_ACK_V3		0x7a
+#define NXP_NAK_V3		0x7b
+#define NXP_CRC_ERROR_V3	0x7c
+
+#define HDR_LEN			16
+
+#define NXP_RECV_FW_REQ_V1 \
+	.type = NXP_V1_FW_REQ_PKT, \
+	.hlen = 4, \
+	.loff = 0, \
+	.lsize = 0, \
+	.maxlen = 4
+
+#define NXP_RECV_CHIP_VER_V3 \
+	.type = NXP_V3_CHIP_VER_PKT, \
+	.hlen = 4, \
+	.loff = 0, \
+	.lsize = 0, \
+	.maxlen = 4
+
+#define NXP_RECV_FW_REQ_V3 \
+	.type = NXP_V3_FW_REQ_PKT, \
+	.hlen = 9, \
+	.loff = 0, \
+	.lsize = 0, \
+	.maxlen = 9
+
+struct v1_data_req {
+	__le16 len;
+	__le16 len_comp;
+} __packed;
+
+struct v3_data_req {
+	__le16 len;
+	__le32 offset;
+	__le16 error;
+	u8 crc;
+} __packed;
+
+struct v3_start_ind {
+	__le16 chip_id;
+	u8 loader_ver;
+	u8 crc;
+} __packed;
+
+/* UART register addresses of BT chip */
+#define CLKDIVADDR	0x7f00008f
+#define UARTDIVADDR	0x7f000090
+#define UARTMCRADDR	0x7f000091
+#define UARTREINITADDR	0x7f000092
+#define UARTICRADDR	0x7f000093
+#define UARTFCRADDR	0x7f000094
+
+#define MCR		0x00000022
+#define INIT		0x00000001
+#define ICR		0x000000c7
+#define FCR		0x000000c7
+
+#define POLYNOMIAL8	0x07
+#define POLYNOMIAL32	0x04c11db7L
+
+struct uart_reg {
+	__le32 address;
+	__le32 value;
+} __packed;
+
+struct uart_config {
+	struct uart_reg clkdiv;
+	struct uart_reg uartdiv;
+	struct uart_reg mcr;
+	struct uart_reg re_init;
+	struct uart_reg icr;
+	struct uart_reg fcr;
+	__le32 crc;
+} __packed;
+
+struct nxp_bootloader_cmd {
+	__le32 header;
+	__le32 arg;
+	__le32 payload_len;
+	__le32 crc;
+} __packed;
+
+static u8 crc8_table[CRC8_TABLE_SIZE];
+static unsigned long crc32_table[256];
+
+/* Default Power Save configuration */
+static int wakeupmode = WAKEUP_METHOD_BREAK;
+static int ps_mode = PS_MODE_ENABLE;
+
+static int init_baudrate = 115200;
+
+static struct sk_buff *nxp_drv_send_cmd(struct hci_dev *hdev, u16 opcode,
+					u32 plen,
+					void *param)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct ps_data *psdata = nxpdev->psdata;
+	struct sk_buff *skb;
+
+	psdata->driver_sent_cmd = true;	/* set flag to prevent re-sending command in nxp_enqueue */
+	skb = __hci_cmd_sync(hdev, opcode, plen, param, HCI_CMD_TIMEOUT);
+	psdata->driver_sent_cmd = false;
+
+	return skb;
+}
+
+static void btnxpuart_tx_wakeup(struct btnxpuart_dev *nxpdev)
+{
+	if (schedule_work(&nxpdev->tx_work))
+		set_bit(BTNXPUART_TX_STATE_ACTIVE, &nxpdev->tx_state);
+}
+
+/* NXP Power Save Feature */
+static void ps_start_timer(struct btnxpuart_dev *nxpdev)
+{
+	struct ps_data *psdata = nxpdev->psdata;
+
+	if (!psdata)
+		return;
+
+	if (psdata->cur_psmode == PS_MODE_ENABLE) {
+		psdata->timer_on = true;
+		mod_timer(&psdata->ps_timer, jiffies + msecs_to_jiffies(psdata->interval));
+	}
+}
+
+static void ps_cancel_timer(struct btnxpuart_dev *nxpdev)
+{
+	struct ps_data *psdata = nxpdev->psdata;
+
+	if (!psdata)
+		return;
+
+	flush_work(&psdata->work);
+	if (psdata->timer_on)
+		del_timer_sync(&psdata->ps_timer);
+	kfree(psdata);
+}
+
+static void ps_control(struct hci_dev *hdev, u8 ps_state)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct ps_data *psdata = nxpdev->psdata;
+	int status;
+
+	if (psdata->ps_state == ps_state)
+		return;
+
+	switch (psdata->cur_wakeupmode) {
+	case WAKEUP_METHOD_DTR:
+		if (ps_state == PS_STATE_AWAKE)
+			serdev_device_set_tiocm(nxpdev->serdev, TIOCM_DTR, 0);
+		else
+			serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_DTR);
+		break;
+	case WAKEUP_METHOD_BREAK:
+	default:
+		if (ps_state == PS_STATE_AWAKE)
+			status = serdev_device_break_ctl(nxpdev->serdev, 0);
+		else
+			status = serdev_device_break_ctl(nxpdev->serdev, -1);
+		bt_dev_info(hdev, "Set UART break: %s, status=%d",
+			    ps_state == PS_STATE_AWAKE ? "off" : "on", status);
+		break;
+	}
+	psdata->ps_state = ps_state;
+	if (ps_state == PS_STATE_AWAKE)
+		btnxpuart_tx_wakeup(nxpdev);
+}
+
+static void ps_work_func(struct work_struct *work)
+{
+	struct ps_data *data = container_of(work, struct ps_data, work);
+
+	if (!data)
+		return;
+
+	if (data->ps_cmd == PS_CMD_ENTER_PS && data->cur_psmode == PS_MODE_ENABLE)
+		ps_control(data->hdev, PS_STATE_SLEEP);
+	else if (data->ps_cmd == PS_CMD_EXIT_PS)
+		ps_control(data->hdev, PS_STATE_AWAKE);
+}
+
+static void ps_timeout_func(struct timer_list *t)
+{
+	struct ps_data *data = from_timer(data, t, ps_timer);
+	struct hci_dev *hdev = data->hdev;
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+
+	data->timer_on = false;
+	if (test_bit(BTNXPUART_TX_STATE_ACTIVE, &nxpdev->tx_state)) {
+		ps_start_timer(nxpdev);
+	} else {
+		data->ps_cmd = PS_CMD_ENTER_PS;
+		schedule_work(&data->work);
+	}
+}
+
+static int ps_init_work(struct hci_dev *hdev)
+{
+	struct ps_data *psdata = kzalloc(sizeof(*psdata), GFP_KERNEL);
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+
+	if (!psdata) {
+		bt_dev_err(hdev, "Can't allocate control structure for Power Save feature");
+		return -ENOMEM;
+	}
+	nxpdev->psdata = psdata;
+
+	psdata->interval = PS_DEFAULT_TIMEOUT_PERIOD;
+	psdata->ps_state = PS_STATE_AWAKE;
+	psdata->ps_mode = ps_mode;
+	psdata->hdev = hdev;
+
+	switch (wakeupmode) {
+	case WAKEUP_METHOD_DTR:
+		psdata->wakeupmode = WAKEUP_METHOD_DTR;
+		break;
+	case WAKEUP_METHOD_BREAK:
+	default:
+		psdata->wakeupmode = WAKEUP_METHOD_BREAK;
+		break;
+	}
+	psdata->cur_psmode = PS_MODE_DISABLE;
+	psdata->cur_wakeupmode = WAKEUP_METHOD_INVALID;
+	INIT_WORK(&psdata->work, ps_work_func);
+
+	return 0;
+}
+
+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 = false;
+	timer_setup(&psdata->ps_timer, ps_timeout_func, 0);
+}
+
+static int ps_wakeup(struct btnxpuart_dev *nxpdev)
+{
+	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;
+}
+
+static int send_ps_cmd(struct hci_dev *hdev, void *data)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct ps_data *psdata = nxpdev->psdata;
+	u8 pcmd;
+	struct sk_buff *skb;
+	u8 *status;
+
+	if (psdata->ps_mode == PS_MODE_ENABLE)
+		pcmd = BT_PS_ENABLE;
+	else
+		pcmd = BT_PS_DISABLE;
+
+	skb = nxp_drv_send_cmd(hdev, HCI_NXP_AUTO_SLEEP_MODE, 1, &pcmd);
+	if (IS_ERR(skb)) {
+		bt_dev_err(hdev, "Setting Power Save mode failed (%ld)", PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+
+	status = skb_pull_data(skb, 1);
+	if (status) {
+		if (!*status)
+			psdata->cur_psmode = psdata->ps_mode;
+		else
+			psdata->ps_mode = psdata->cur_psmode;
+		if (psdata->cur_psmode == PS_MODE_ENABLE)
+			ps_start_timer(nxpdev);
+		else
+			ps_wakeup(nxpdev);
+		bt_dev_info(hdev, "Power Save mode response: status=%d, ps_mode=%d",
+			    *status, psdata->cur_psmode);
+	}
+	kfree_skb(skb);
+
+	return 0;
+}
+
+static int send_wakeup_method_cmd(struct hci_dev *hdev, void *data)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct ps_data *psdata = nxpdev->psdata;
+	u8 pcmd[4];
+	struct sk_buff *skb;
+	u8 *status;
+
+	pcmd[0] = BT_HOST_WAKEUP_METHOD_NONE;
+	pcmd[1] = 0xff;
+	switch (psdata->wakeupmode) {
+	case WAKEUP_METHOD_DTR:
+		pcmd[2] = BT_CTRL_WAKEUP_METHOD_DSR;
+		break;
+	case WAKEUP_METHOD_BREAK:
+	default:
+		pcmd[2] = BT_CTRL_WAKEUP_METHOD_BREAK;
+		break;
+	}
+	pcmd[3] = 0xff;
+
+	skb = nxp_drv_send_cmd(hdev, HCI_NXP_WAKEUP_METHOD, 4, pcmd);
+	if (IS_ERR(skb)) {
+		bt_dev_err(hdev, "Setting wake-up method failed (%ld)", PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+
+	status = skb_pull_data(skb, 1);
+	if (status) {
+		if (*status == 0)
+			psdata->cur_wakeupmode = psdata->wakeupmode;
+		else
+			psdata->wakeupmode = psdata->cur_wakeupmode;
+		bt_dev_info(hdev, "Set Wakeup Method response: status=%d, wakeupmode=%d",
+			    *status, psdata->cur_wakeupmode);
+	}
+	kfree_skb(skb);
+
+	return 0;
+}
+
+static int ps_init(struct hci_dev *hdev)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct ps_data *psdata = nxpdev->psdata;
+
+	serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_RTS);
+	usleep_range(5000, 10000);
+	serdev_device_set_tiocm(nxpdev->serdev, TIOCM_RTS, 0);
+	usleep_range(5000, 10000);
+
+	switch (psdata->wakeupmode) {
+	case WAKEUP_METHOD_DTR:
+		serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_DTR);
+		serdev_device_set_tiocm(nxpdev->serdev, TIOCM_DTR, 0);
+		break;
+	case WAKEUP_METHOD_BREAK:
+	default:
+		serdev_device_break_ctl(nxpdev->serdev, -1);
+		usleep_range(5000, 10000);
+		serdev_device_break_ctl(nxpdev->serdev, 0);
+		usleep_range(5000, 10000);
+		break;
+	}
+	if (!test_bit(HCI_RUNNING, &hdev->flags)) {
+		bt_dev_err(hdev, "HCI_RUNNING is not set");
+		return -EBUSY;
+	}
+	if (psdata->cur_wakeupmode != psdata->wakeupmode)
+		hci_cmd_sync_queue(hdev, send_wakeup_method_cmd, NULL, NULL);
+	if (psdata->cur_psmode != psdata->ps_mode)
+		hci_cmd_sync_queue(hdev, send_ps_cmd, NULL, NULL);
+
+	return 0;
+}
+
+/* NXP Firmware Download Feature */
+static void nxp_fw_dnld_gen_crc32_table(void)
+{
+	int i, j;
+	unsigned long crc_accum;
+
+	for (i = 0; i < 256; i++) {
+		crc_accum = ((unsigned long)i << 24);
+		for (j = 0; j < 8; j++) {
+			if (crc_accum & 0x80000000L)
+				crc_accum = (crc_accum << 1) ^ POLYNOMIAL32;
+			else
+				crc_accum = (crc_accum << 1);
+		}
+		crc32_table[i] = crc_accum;
+	}
+}
+
+static unsigned long nxp_fw_dnld_update_crc(unsigned long crc_accum,
+					    char *data_blk_ptr,
+					    int data_blk_size)
+{
+	unsigned long i, j;
+
+	for (j = 0; j < data_blk_size; j++) {
+		i = ((unsigned long)(crc_accum >> 24) ^ *data_blk_ptr++) & 0xff;
+		crc_accum = (crc_accum << 8) ^ crc32_table[i];
+	}
+	return crc_accum;
+}
+
+static int nxp_download_firmware(struct hci_dev *hdev)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	int err = 0;
+
+	nxpdev->fw_dnld_v1_offset = 0;
+	nxpdev->fw_v1_sent_bytes = 0;
+	nxpdev->fw_v1_expected_len = HDR_LEN;
+	nxpdev->fw_v3_offset_correction = 0;
+	nxpdev->baudrate_changed = false;
+	nxpdev->timeout_changed = false;
+
+	crc8_populate_msb(crc8_table, POLYNOMIAL8);
+	nxp_fw_dnld_gen_crc32_table();
+
+	serdev_device_set_baudrate(nxpdev->serdev, HCI_NXP_PRI_BAUDRATE);
+	serdev_device_set_flow_control(nxpdev->serdev, 0);
+	nxpdev->current_baudrate = HCI_NXP_PRI_BAUDRATE;
+
+	/* Wait till FW is downloaded and CTS becomes low */
+	err = wait_event_interruptible_timeout(nxpdev->suspend_wait_q,
+					       !test_bit(BTNXPUART_FW_DOWNLOADING,
+							 &nxpdev->tx_state),
+					       msecs_to_jiffies(60000));
+	if (err == 0) {
+		bt_dev_err(hdev, "FW Download Timeout.");
+		return -ETIMEDOUT;
+	}
+
+	serdev_device_set_flow_control(nxpdev->serdev, 1);
+	err = serdev_device_wait_for_cts(nxpdev->serdev, 1, 60000);
+	if (err < 0) {
+		bt_dev_err(hdev, "CTS is still high. FW Download failed.");
+		return err;
+	}
+	bt_dev_info(hdev, "CTS is low");
+	release_firmware(nxpdev->fw);
+	memset(nxpdev->fw_name, 0, MAX_FW_FILE_NAME_LEN);
+
+	/* Allow the downloaded FW to initialize */
+	usleep_range(800000, 1000000);
+
+	return 0;
+}
+
+static int nxp_send_ack(u8 ack, struct hci_dev *hdev)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	u8 ack_nak[2];
+
+	if (ack == NXP_ACK_V1 || ack == NXP_NAK_V1) {
+		ack_nak[0] = ack;
+		serdev_device_write_buf(nxpdev->serdev, ack_nak, 1);
+	} else if (ack == NXP_ACK_V3) {
+		ack_nak[0] = ack;
+		ack_nak[1] = crc8(crc8_table, ack_nak, 1, 0xff);
+		serdev_device_write_buf(nxpdev->serdev, ack_nak, 2);
+	}
+	return 0;
+}
+
+static bool nxp_fw_change_baudrate(struct hci_dev *hdev, u16 req_len)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct nxp_bootloader_cmd nxp_cmd5;
+	struct uart_config uart_config;
+
+	if (req_len == sizeof(nxp_cmd5)) {
+		nxp_cmd5.header = __cpu_to_le32(5);
+		nxp_cmd5.arg = 0;
+		nxp_cmd5.payload_len = __cpu_to_le32(sizeof(uart_config));
+		nxp_cmd5.crc = swab32(nxp_fw_dnld_update_crc(0UL,
+							     (char *)&nxp_cmd5,
+							     sizeof(nxp_cmd5) - 4));
+
+		serdev_device_write_buf(nxpdev->serdev, (u8 *)&nxp_cmd5, req_len);
+		nxpdev->fw_v3_offset_correction += req_len;
+	} else if (req_len == sizeof(uart_config)) {
+		uart_config.clkdiv.address = __cpu_to_le32(CLKDIVADDR);
+		uart_config.clkdiv.value = __cpu_to_le32(0x00c00000);
+		uart_config.uartdiv.address = __cpu_to_le32(UARTDIVADDR);
+		uart_config.uartdiv.value = __cpu_to_le32(1);
+		uart_config.mcr.address = __cpu_to_le32(UARTMCRADDR);
+		uart_config.mcr.value = __cpu_to_le32(MCR);
+		uart_config.re_init.address = __cpu_to_le32(UARTREINITADDR);
+		uart_config.re_init.value = __cpu_to_le32(INIT);
+		uart_config.icr.address = __cpu_to_le32(UARTICRADDR);
+		uart_config.icr.value = __cpu_to_le32(ICR);
+		uart_config.fcr.address = __cpu_to_le32(UARTFCRADDR);
+		uart_config.fcr.value = __cpu_to_le32(FCR);
+		uart_config.crc = swab32(nxp_fw_dnld_update_crc(0UL,
+								(char *)&uart_config,
+								sizeof(uart_config) - 4));
+		serdev_device_write_buf(nxpdev->serdev, (u8 *)&uart_config, req_len);
+		serdev_device_wait_until_sent(nxpdev->serdev, 0);
+		nxpdev->fw_v3_offset_correction += req_len;
+		return true;
+	}
+	return false;
+}
+
+static bool nxp_fw_change_timeout(struct hci_dev *hdev, u16 req_len)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct nxp_bootloader_cmd nxp_cmd7;
+
+	if (req_len != sizeof(nxp_cmd7))
+		return false;
+
+	nxp_cmd7.header = __cpu_to_le32(7);
+	nxp_cmd7.arg = __cpu_to_le32(0x70);
+	nxp_cmd7.payload_len = 0;
+	nxp_cmd7.crc = swab32(nxp_fw_dnld_update_crc(0UL,
+						     (char *)&nxp_cmd7,
+						     sizeof(nxp_cmd7) - 4));
+
+	serdev_device_write_buf(nxpdev->serdev, (u8 *)&nxp_cmd7, req_len);
+	serdev_device_wait_until_sent(nxpdev->serdev, 0);
+	nxpdev->fw_v3_offset_correction += req_len;
+	return true;
+}
+
+static u32 nxp_get_data_len(const u8 *buf)
+{
+	struct nxp_bootloader_cmd *hdr = (struct nxp_bootloader_cmd *)buf;
+
+	return __le32_to_cpu(hdr->payload_len);
+}
+
+/* for legacy chipsets with V1 bootloader */
+static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct v1_data_req *req = skb_pull_data(skb, sizeof(struct v1_data_req));
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct btnxpuart_data *nxp_data = nxpdev->nxp_data;
+	u32 requested_len;
+	int err;
+
+	if (!test_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state))
+		goto ret;
+
+	if (req && (req->len ^ req->len_comp) != 0xffff) {
+		bt_dev_info(hdev, "ERR: Send NAK");
+		nxp_send_ack(NXP_NAK_V1, hdev);
+		goto ret;
+	}
+	nxp_send_ack(NXP_ACK_V1, hdev);
+
+	if (nxp_data->fw_dnld_use_high_baudrate) {
+		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 (!strlen(nxpdev->fw_name)) {
+		snprintf(nxpdev->fw_name, MAX_FW_FILE_NAME_LEN, "%s",
+			 nxp_data->fw_name);
+		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);
+			return err;
+		}
+	}
+
+	requested_len = req->len;
+	if (requested_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 (requested_len & 0x01) {
+		/* The CRC did not match at the other end.
+		 * Simply send the same bytes again.
+		 */
+		requested_len = nxpdev->fw_v1_sent_bytes;
+		bt_dev_err(hdev, "CRC error. Resend %d bytes of FW.", requested_len);
+	} else {
+		nxpdev->fw_dnld_v1_offset += nxpdev->fw_v1_sent_bytes;
+
+		/* The FW bin file is made up of many blocks of
+		 * 16 byte header and payload data chunks. If the
+		 * FW has requested a header, read the payload length
+		 * info from the header, before sending the header.
+		 * In the next iteration, the FW should request the
+		 * payload data chunk, which should be equal to the
+		 * payload length read from header. If there is a
+		 * mismatch, clearly the driver and FW are out of sync,
+		 * and we need to re-send the previous header again.
+		 */
+		if (requested_len == nxpdev->fw_v1_expected_len) {
+			if (requested_len == HDR_LEN)
+				nxpdev->fw_v1_expected_len = nxp_get_data_len(nxpdev->fw->data +
+									nxpdev->fw_dnld_v1_offset);
+			else
+				nxpdev->fw_v1_expected_len = HDR_LEN;
+		} else {
+			if (requested_len == HDR_LEN) {
+				/* FW download out of sync. Send previous chunk again */
+				nxpdev->fw_dnld_v1_offset -= nxpdev->fw_v1_sent_bytes;
+				nxpdev->fw_v1_expected_len = HDR_LEN;
+			}
+		}
+	}
+
+	if (nxpdev->fw_dnld_v1_offset + requested_len <= nxpdev->fw->size)
+		serdev_device_write_buf(nxpdev->serdev,
+					nxpdev->fw->data + nxpdev->fw_dnld_v1_offset,
+					requested_len);
+	nxpdev->fw_v1_sent_bytes = requested_len;
+
+ret:
+	kfree_skb(skb);
+	return 0;
+}
+
+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;
+		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_err(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_info(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_info(hdev, "CTS high. Need FW Download");
+		err = nxp_download_firmware(hdev);
+		if (err < 0)
+			return err;
+	} else {
+		bt_dev_info(hdev, "CTS low. FW already running.");
+		clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
+	}
+
+	serdev_device_set_flow_control(nxpdev->serdev, 1);
+	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;
+		hci_cmd_sync_queue(hdev, nxp_set_baudrate_cmd, NULL, NULL);
+	}
+
+	ps_init(hdev);
+
+	return 0;
+}
+
+static int nxp_enqueue(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct ps_data *psdata = nxpdev->psdata;
+	struct hci_command_hdr *hdr;
+	u8 *param;
+
+	if (!nxpdev || !psdata)
+		goto free_skb;
+
+	/* if vendor commands are received from user space (e.g. hcitool), update
+	 * driver flags accordingly and ask driver to re-send the command to FW.
+	 */
+	if (bt_cb(skb)->pkt_type == HCI_COMMAND_PKT && !psdata->driver_sent_cmd) {
+		hdr = (struct hci_command_hdr *)skb->data;
+		param = skb->data + HCI_COMMAND_HDR_SIZE;
+		switch (__le16_to_cpu(hdr->opcode)) {
+		case HCI_NXP_AUTO_SLEEP_MODE:
+			if (hdr->plen >= 1) {
+				if (param[0] == BT_PS_ENABLE)
+					psdata->ps_mode = PS_MODE_ENABLE;
+				else if (param[0] == BT_PS_DISABLE)
+					psdata->ps_mode = PS_MODE_DISABLE;
+				hci_cmd_sync_queue(hdev, send_ps_cmd, NULL, NULL);
+				goto free_skb;
+			}
+			break;
+		case HCI_NXP_WAKEUP_METHOD:
+			if (hdr->plen >= 4) {
+				switch (param[2]) {
+				case BT_CTRL_WAKEUP_METHOD_DSR:
+					psdata->wakeupmode = WAKEUP_METHOD_DTR;
+					break;
+				case BT_CTRL_WAKEUP_METHOD_BREAK:
+				default:
+					psdata->wakeupmode = WAKEUP_METHOD_BREAK;
+					break;
+				}
+				hci_cmd_sync_queue(hdev, send_wakeup_method_cmd, NULL, NULL);
+				goto free_skb;
+			}
+			break;
+		case HCI_NXP_SET_OPER_SPEED:
+			if (hdr->plen == 4) {
+				nxpdev->new_baudrate = *((u32 *)param);
+				hci_cmd_sync_queue(hdev, nxp_set_baudrate_cmd, NULL, NULL);
+				goto free_skb;
+			}
+			break;
+		case HCI_NXP_IND_RESET:
+			if (hdr->plen == 1) {
+				hci_cmd_sync_queue(hdev, nxp_set_ind_reset, NULL, NULL);
+				goto free_skb;
+			}
+			break;
+		default:
+			break;
+		}
+	}
+
+	/* Prepend skb with frame type */
+	memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
+	skb_queue_tail(&nxpdev->txq, skb);
+
+	btnxpuart_tx_wakeup(nxpdev);
+ret:
+	return 0;
+
+free_skb:
+	kfree_skb(skb);
+	goto ret;
+}
+
+static struct sk_buff *nxp_dequeue(void *data)
+{
+	struct btnxpuart_dev *nxpdev = (struct btnxpuart_dev *)data;
+
+	ps_wakeup(nxpdev);
+	ps_start_timer(nxpdev);
+	return skb_dequeue(&nxpdev->txq);
+}
+
+/* btnxpuart based on serdev */
+static void btnxpuart_tx_work(struct work_struct *work)
+{
+	struct btnxpuart_dev *nxpdev = container_of(work, struct btnxpuart_dev,
+						   tx_work);
+	struct serdev_device *serdev = nxpdev->serdev;
+	struct hci_dev *hdev = nxpdev->hdev;
+	struct sk_buff *skb;
+	int len;
+
+	while ((skb = nxp_dequeue(nxpdev))) {
+		len = serdev_device_write_buf(serdev, skb->data, skb->len);
+		hdev->stat.byte_tx += len;
+
+		skb_pull(skb, len);
+		if (skb->len > 0) {
+			skb_queue_head(&nxpdev->txq, skb);
+			break;
+		}
+
+		switch (hci_skb_pkt_type(skb)) {
+		case HCI_COMMAND_PKT:
+			hdev->stat.cmd_tx++;
+			break;
+		case HCI_ACLDATA_PKT:
+			hdev->stat.acl_tx++;
+			break;
+		case HCI_SCODATA_PKT:
+			hdev->stat.sco_tx++;
+			break;
+		}
+
+		kfree_skb(skb);
+	}
+	clear_bit(BTNXPUART_TX_STATE_ACTIVE, &nxpdev->tx_state);
+}
+
+static int btnxpuart_open(struct hci_dev *hdev)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	int err = 0;
+
+	err = serdev_device_open(nxpdev->serdev);
+	if (err) {
+		bt_dev_err(hdev, "Unable to open UART device %s",
+			   dev_name(&nxpdev->serdev->dev));
+	}
+
+	return err;
+}
+
+static int btnxpuart_close(struct hci_dev *hdev)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+
+	if (!nxpdev)
+		return 0;
+
+	serdev_device_close(nxpdev->serdev);
+
+	return 0;
+}
+
+static int btnxpuart_flush(struct hci_dev *hdev)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+
+	if (!nxpdev)
+		return 0;
+
+	/* Flush any pending characters */
+	serdev_device_write_flush(nxpdev->serdev);
+	skb_queue_purge(&nxpdev->txq);
+
+	cancel_work_sync(&nxpdev->tx_work);
+
+	kfree_skb(nxpdev->rx_skb);
+	nxpdev->rx_skb = NULL;
+
+	return 0;
+}
+
+static const struct h4_recv_pkt nxp_recv_pkts[] = {
+	{ H4_RECV_ACL,          .recv = hci_recv_frame },
+	{ H4_RECV_SCO,          .recv = hci_recv_frame },
+	{ H4_RECV_EVENT,        .recv = hci_recv_frame },
+	{ NXP_RECV_FW_REQ_V1,   .recv = nxp_recv_fw_req_v1 },
+	{ NXP_RECV_CHIP_VER_V3, .recv = nxp_recv_chip_ver_v3 },
+	{ NXP_RECV_FW_REQ_V3,   .recv = nxp_recv_fw_req_v3 },
+};
+
+static int btnxpuart_receive_buf(struct serdev_device *serdev, const u8 *data,
+				 size_t count)
+{
+	struct btnxpuart_dev *nxpdev = serdev_device_get_drvdata(serdev);
+
+	if (test_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state)) {
+		if (*data != NXP_V1_FW_REQ_PKT && *data != NXP_V1_CHIP_VER_PKT &&
+		    *data != NXP_V3_FW_REQ_PKT && *data != NXP_V3_CHIP_VER_PKT) {
+			/* Unknown bootloader signature, skip without returning error */
+			return count;
+		}
+	}
+
+	ps_start_timer(nxpdev);
+
+	nxpdev->rx_skb = h4_recv_buf(nxpdev->hdev, nxpdev->rx_skb, data, count,
+				     nxp_recv_pkts, ARRAY_SIZE(nxp_recv_pkts));
+	if (IS_ERR(nxpdev->rx_skb)) {
+		int err = PTR_ERR(nxpdev->rx_skb);
+
+		bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
+		nxpdev->rx_skb = NULL;
+		return err;
+	}
+	nxpdev->hdev->stat.byte_rx += count;
+	return count;
+}
+
+static void btnxpuart_write_wakeup(struct serdev_device *serdev)
+{
+	serdev_device_write_wakeup(serdev);
+}
+
+static const struct serdev_device_ops btnxpuart_client_ops = {
+	.receive_buf = btnxpuart_receive_buf,
+	.write_wakeup = btnxpuart_write_wakeup,
+};
+
+static int nxp_serdev_probe(struct serdev_device *serdev)
+{
+	struct hci_dev *hdev;
+	struct btnxpuart_dev *nxpdev;
+
+	nxpdev = devm_kzalloc(&serdev->dev, sizeof(*nxpdev), GFP_KERNEL);
+	if (!nxpdev)
+		return -ENOMEM;
+
+	nxpdev->nxp_data = (struct btnxpuart_data *)device_get_match_data(&serdev->dev);
+
+	nxpdev->serdev = serdev;
+	serdev_device_set_drvdata(serdev, nxpdev);
+
+	serdev_device_set_client_ops(serdev, &btnxpuart_client_ops);
+
+	INIT_WORK(&nxpdev->tx_work, btnxpuart_tx_work);
+	skb_queue_head_init(&nxpdev->txq);
+
+	/* Initialize and register HCI device */
+	hdev = hci_alloc_dev();
+	if (!hdev) {
+		dev_err(&serdev->dev, "Can't allocate HCI device\n");
+		return -ENOMEM;
+	}
+
+	nxpdev->hdev = hdev;
+
+	hdev->bus = HCI_UART;
+	hci_set_drvdata(hdev, nxpdev);
+
+	hdev->manufacturer = MANUFACTURER_NXP;
+	hdev->open  = btnxpuart_open;
+	hdev->close = btnxpuart_close;
+	hdev->flush = btnxpuart_flush;
+	hdev->setup = nxp_setup;
+	hdev->send  = nxp_enqueue;
+	SET_HCIDEV_DEV(hdev, &serdev->dev);
+
+	if (hci_register_dev(hdev) < 0) {
+		dev_err(&serdev->dev, "Can't register HCI device\n");
+		hci_free_dev(hdev);
+		return -ENODEV;
+	}
+
+	if (!ps_init_work(hdev))
+		ps_init_timer(hdev);
+
+	return 0;
+}
+
+static void nxp_serdev_remove(struct serdev_device *serdev)
+{
+	struct btnxpuart_dev *nxpdev = serdev_device_get_drvdata(serdev);
+	struct hci_dev *hdev = nxpdev->hdev;
+
+	/* Restore FW baudrate to init_baudrate if changed.
+	 * This will ensure FW baudrate is in sync with
+	 * driver baudrate in case this driver is re-inserted.
+	 */
+	if (init_baudrate != nxpdev->current_baudrate) {
+		nxpdev->new_baudrate = init_baudrate;
+		nxp_set_baudrate_cmd(hdev, NULL);
+	}
+
+	ps_cancel_timer(nxpdev);
+	hci_unregister_dev(hdev);
+	hci_free_dev(hdev);
+}
+
+static struct btnxpuart_data w8987_data = {
+	.fw_dnld_use_high_baudrate = true,
+	.fw_name = FIRMWARE_W8987,
+};
+
+static struct btnxpuart_data w8997_data = {
+	.fw_dnld_use_high_baudrate = false,
+	.fw_name = FIRMWARE_W8997,
+};
+
+static const struct of_device_id nxpuart_of_match_table[] = {
+	{ .compatible = "nxp,88w8987-bt", .data = &w8987_data },
+	{ .compatible = "nxp,88w8997-bt", .data = &w8997_data },
+	{ }
+};
+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);
+
+/* This module parameter is "chip-module vendor" dependent.
+ * Same chip can have different FW init speed depending
+ * on caliberation done by different module vendors.
+ */
+module_param(init_baudrate, int, 0444);
+MODULE_PARM_DESC(init_baudrate, "host baudrate after FW download: default=115200");
+
+MODULE_AUTHOR("Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>");
+MODULE_DESCRIPTION("NXP Bluetooth Serial driver v1.0 ");
+MODULE_LICENSE("GPL");
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets
  2023-02-21 16:25 ` [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets Neeraj Sanjay Kale
@ 2023-02-21 16:47   ` Greg KH
  2023-02-23 10:40     ` Neeraj sanjay kale
  2023-02-21 21:37   ` Luiz Augusto von Dentz
  1 sibling, 1 reply; 10+ messages in thread
From: Greg KH @ 2023-02-21 16:47 UTC (permalink / raw)
  To: Neeraj Sanjay Kale
  Cc: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
	marcel, johan.hedberg, luiz.dentz, jirislaby, alok.a.tiwari,
	hdanton, ilpo.jarvinen, leon, netdev, devicetree, linux-kernel,
	linux-bluetooth, linux-serial, amitkumar.karwar, rohit.fule,
	sherry.sun

On Tue, Feb 21, 2023 at 09:55:41PM +0530, Neeraj Sanjay Kale wrote:
> +		bt_dev_info(hdev, "Set UART break: %s, status=%d",
> +			    ps_state == PS_STATE_AWAKE ? "off" : "on", status);

You have a lot of "noise" in this driver, remove all "info" messages, as
if a driver is working properly, it is quiet.

> +		break;
> +	}
> +	psdata->ps_state = ps_state;
> +	if (ps_state == PS_STATE_AWAKE)
> +		btnxpuart_tx_wakeup(nxpdev);
> +}
> +
> +static void ps_work_func(struct work_struct *work)
> +{
> +	struct ps_data *data = container_of(work, struct ps_data, work);
> +
> +	if (!data)
> +		return;

You did not test this, that check can never happen, please do not do
pointless checks.



> +
> +	if (data->ps_cmd == PS_CMD_ENTER_PS && data->cur_psmode == PS_MODE_ENABLE)
> +		ps_control(data->hdev, PS_STATE_SLEEP);
> +	else if (data->ps_cmd == PS_CMD_EXIT_PS)
> +		ps_control(data->hdev, PS_STATE_AWAKE);
> +}
> +
> +static void ps_timeout_func(struct timer_list *t)
> +{
> +	struct ps_data *data = from_timer(data, t, ps_timer);
> +	struct hci_dev *hdev = data->hdev;
> +	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +
> +	data->timer_on = false;
> +	if (test_bit(BTNXPUART_TX_STATE_ACTIVE, &nxpdev->tx_state)) {
> +		ps_start_timer(nxpdev);
> +	} else {
> +		data->ps_cmd = PS_CMD_ENTER_PS;
> +		schedule_work(&data->work);
> +	}
> +}
> +
> +static int ps_init_work(struct hci_dev *hdev)
> +{
> +	struct ps_data *psdata = kzalloc(sizeof(*psdata), GFP_KERNEL);

Don't do allocations in variable declarations :(

> +	} else if (req_len == sizeof(uart_config)) {
> +		uart_config.clkdiv.address = __cpu_to_le32(CLKDIVADDR);
> +		uart_config.clkdiv.value = __cpu_to_le32(0x00c00000);
> +		uart_config.uartdiv.address = __cpu_to_le32(UARTDIVADDR);
> +		uart_config.uartdiv.value = __cpu_to_le32(1);
> +		uart_config.mcr.address = __cpu_to_le32(UARTMCRADDR);
> +		uart_config.mcr.value = __cpu_to_le32(MCR);
> +		uart_config.re_init.address = __cpu_to_le32(UARTREINITADDR);
> +		uart_config.re_init.value = __cpu_to_le32(INIT);
> +		uart_config.icr.address = __cpu_to_le32(UARTICRADDR);
> +		uart_config.icr.value = __cpu_to_le32(ICR);
> +		uart_config.fcr.address = __cpu_to_le32(UARTFCRADDR);
> +		uart_config.fcr.value = __cpu_to_le32(FCR);
> +		uart_config.crc = swab32(nxp_fw_dnld_update_crc(0UL,
> +								(char *)&uart_config,
> +								sizeof(uart_config) - 4));
> +		serdev_device_write_buf(nxpdev->serdev, (u8 *)&uart_config, req_len);
> +		serdev_device_wait_until_sent(nxpdev->serdev, 0);

You are sending magic commands over the serial connection, are you sure
that is ok?

> +	if (requested_len & 0x01) {
> +		/* The CRC did not match at the other end.
> +		 * Simply send the same bytes again.
> +		 */
> +		requested_len = nxpdev->fw_v1_sent_bytes;
> +		bt_dev_err(hdev, "CRC error. Resend %d bytes of FW.", requested_len);

Why is this an error sent to the kernel log?

Again, be quiet if there is nothing that a user can do.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets
  2023-02-21 16:25 ` [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets Neeraj Sanjay Kale
  2023-02-21 16:47   ` Greg KH
@ 2023-02-21 21:37   ` Luiz Augusto von Dentz
  2023-02-23 10:57     ` Neeraj sanjay kale
  1 sibling, 1 reply; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2023-02-21 21:37 UTC (permalink / raw)
  To: Neeraj Sanjay Kale
  Cc: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
	marcel, johan.hedberg, gregkh, jirislaby, alok.a.tiwari, hdanton,
	ilpo.jarvinen, leon, netdev, devicetree, linux-kernel,
	linux-bluetooth, linux-serial, amitkumar.karwar, rohit.fule,
	sherry.sun

Hi Neeraj,

On Tue, Feb 21, 2023 at 8:26 AM Neeraj Sanjay Kale
<neeraj.sanjaykale@nxp.com> wrote:
>
> 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@nxp.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)
> ---
>  MAINTAINERS                   |    1 +
>  drivers/bluetooth/Kconfig     |   11 +
>  drivers/bluetooth/Makefile    |    1 +
>  drivers/bluetooth/btnxpuart.c | 1292 +++++++++++++++++++++++++++++++++
>  4 files changed, 1305 insertions(+)
>  create mode 100644 drivers/bluetooth/btnxpuart.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6d36f52dc124..7343f4943458 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22840,6 +22840,7 @@ M:      Amitkumar Karwar <amitkumar.karwar@nxp.com>
>  M:     Neeraj Kale <neeraj.sanjaykale@nxp.com>
>  S:     Maintained
>  F:     Documentation/devicetree/bindings/net/bluetooth/nxp,w8987-bt.yaml
> +F:     drivers/bluetooth/btnxpuart.c
>
>  THE REST
>  M:     Linus Torvalds <torvalds@linux-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..608185be2b30
> --- /dev/null
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -0,0 +1,1292 @@
> +// 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
> +
> +/* Bluetooth Host Wakeup Methods */
> +#define BT_HOST_WAKEUP_METHOD_NONE      0x00
> +#define BT_HOST_WAKEUP_METHOD_DTR       0x01
> +#define BT_HOST_WAKEUP_METHOD_BREAK     0x02
> +#define BT_HOST_WAKEUP_METHOD_GPIO      0x03
> +
> +/* Bluetooth Chip Wakeup Methods */
> +#define BT_CTRL_WAKEUP_METHOD_DSR       0x00
> +#define BT_CTRL_WAKEUP_METHOD_BREAK     0x01
> +#define BT_CTRL_WAKEUP_METHOD_GPIO      0x02
> +#define BT_CTRL_WAKEUP_METHOD_EXT_BREAK 0x04
> +#define BT_CTRL_WAKEUP_METHOD_RTS       0x05
> +
> +struct ps_data {
> +       u8    ps_mode;
> +       u8    cur_psmode;
> +       u8    ps_state;
> +       u8    ps_cmd;
> +       u8    wakeupmode;
> +       u8    cur_wakeupmode;
> +       bool  driver_sent_cmd;
> +       bool  timer_on;
> +       u32   interval;
> +       struct hci_dev *hdev;
> +       struct work_struct work;
> +       struct timer_list ps_timer;
> +};
> +
> +struct btnxpuart_data {
> +       bool fw_dnld_use_high_baudrate;
> +       const u8 *fw_name;
> +};
> +
> +struct btnxpuart_dev {
> +       struct hci_dev *hdev;
> +       struct serdev_device *serdev;
> +
> +       struct work_struct tx_work;
> +       unsigned long tx_state;
> +       struct sk_buff_head txq;
> +       struct sk_buff *rx_skb;
> +
> +       const struct firmware *fw;
> +       u8 fw_name[MAX_FW_FILE_NAME_LEN];
> +       u32 fw_dnld_v1_offset;
> +       u32 fw_v1_sent_bytes;
> +       u32 fw_v3_offset_correction;
> +       u32 fw_v1_expected_len;
> +       wait_queue_head_t suspend_wait_q;
> +
> +       u32 new_baudrate;
> +       u32 current_baudrate;
> +       bool timeout_changed;
> +       bool baudrate_changed;
> +
> +       struct ps_data *psdata;
> +       struct btnxpuart_data *nxp_data;
> +};
> +
> +#define NXP_V1_FW_REQ_PKT      0xa5
> +#define NXP_V1_CHIP_VER_PKT    0xaa
> +#define NXP_V3_FW_REQ_PKT      0xa7
> +#define NXP_V3_CHIP_VER_PKT    0xab
> +
> +#define NXP_ACK_V1             0x5a
> +#define NXP_NAK_V1             0xbf
> +#define NXP_ACK_V3             0x7a
> +#define NXP_NAK_V3             0x7b
> +#define NXP_CRC_ERROR_V3       0x7c
> +
> +#define HDR_LEN                        16
> +
> +#define NXP_RECV_FW_REQ_V1 \
> +       .type = NXP_V1_FW_REQ_PKT, \
> +       .hlen = 4, \
> +       .loff = 0, \
> +       .lsize = 0, \
> +       .maxlen = 4
> +
> +#define NXP_RECV_CHIP_VER_V3 \
> +       .type = NXP_V3_CHIP_VER_PKT, \
> +       .hlen = 4, \
> +       .loff = 0, \
> +       .lsize = 0, \
> +       .maxlen = 4
> +
> +#define NXP_RECV_FW_REQ_V3 \
> +       .type = NXP_V3_FW_REQ_PKT, \
> +       .hlen = 9, \
> +       .loff = 0, \
> +       .lsize = 0, \
> +       .maxlen = 9
> +
> +struct v1_data_req {
> +       __le16 len;
> +       __le16 len_comp;
> +} __packed;
> +
> +struct v3_data_req {
> +       __le16 len;
> +       __le32 offset;
> +       __le16 error;
> +       u8 crc;
> +} __packed;
> +
> +struct v3_start_ind {
> +       __le16 chip_id;
> +       u8 loader_ver;
> +       u8 crc;
> +} __packed;
> +
> +/* UART register addresses of BT chip */
> +#define CLKDIVADDR     0x7f00008f
> +#define UARTDIVADDR    0x7f000090
> +#define UARTMCRADDR    0x7f000091
> +#define UARTREINITADDR 0x7f000092
> +#define UARTICRADDR    0x7f000093
> +#define UARTFCRADDR    0x7f000094
> +
> +#define MCR            0x00000022
> +#define INIT           0x00000001
> +#define ICR            0x000000c7
> +#define FCR            0x000000c7
> +
> +#define POLYNOMIAL8    0x07
> +#define POLYNOMIAL32   0x04c11db7L
> +
> +struct uart_reg {
> +       __le32 address;
> +       __le32 value;
> +} __packed;
> +
> +struct uart_config {
> +       struct uart_reg clkdiv;
> +       struct uart_reg uartdiv;
> +       struct uart_reg mcr;
> +       struct uart_reg re_init;
> +       struct uart_reg icr;
> +       struct uart_reg fcr;
> +       __le32 crc;
> +} __packed;
> +
> +struct nxp_bootloader_cmd {
> +       __le32 header;
> +       __le32 arg;
> +       __le32 payload_len;
> +       __le32 crc;
> +} __packed;
> +
> +static u8 crc8_table[CRC8_TABLE_SIZE];
> +static unsigned long crc32_table[256];
> +
> +/* Default Power Save configuration */
> +static int wakeupmode = WAKEUP_METHOD_BREAK;
> +static int ps_mode = PS_MODE_ENABLE;
> +
> +static int init_baudrate = 115200;
> +
> +static struct sk_buff *nxp_drv_send_cmd(struct hci_dev *hdev, u16 opcode,
> +                                       u32 plen,
> +                                       void *param)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +       struct ps_data *psdata = nxpdev->psdata;
> +       struct sk_buff *skb;
> +
> +       psdata->driver_sent_cmd = true; /* set flag to prevent re-sending command in nxp_enqueue */
> +       skb = __hci_cmd_sync(hdev, opcode, plen, param, HCI_CMD_TIMEOUT);
> +       psdata->driver_sent_cmd = false;
> +
> +       return skb;
> +}
> +
> +static void btnxpuart_tx_wakeup(struct btnxpuart_dev *nxpdev)
> +{
> +       if (schedule_work(&nxpdev->tx_work))
> +               set_bit(BTNXPUART_TX_STATE_ACTIVE, &nxpdev->tx_state);
> +}
> +
> +/* NXP Power Save Feature */
> +static void ps_start_timer(struct btnxpuart_dev *nxpdev)
> +{
> +       struct ps_data *psdata = nxpdev->psdata;
> +
> +       if (!psdata)
> +               return;
> +
> +       if (psdata->cur_psmode == PS_MODE_ENABLE) {
> +               psdata->timer_on = true;
> +               mod_timer(&psdata->ps_timer, jiffies + msecs_to_jiffies(psdata->interval));
> +       }
> +}
> +
> +static void ps_cancel_timer(struct btnxpuart_dev *nxpdev)
> +{
> +       struct ps_data *psdata = nxpdev->psdata;
> +
> +       if (!psdata)
> +               return;
> +
> +       flush_work(&psdata->work);
> +       if (psdata->timer_on)
> +               del_timer_sync(&psdata->ps_timer);
> +       kfree(psdata);

It seems that ps_cancel_timer is only called when unregister so
psdata, wouldn't be used anymore, but in case ps_cancel_timer would be
reused in the future this would become a problem so I recommend moving
it out if it.

> +}
> +
> +static void ps_control(struct hci_dev *hdev, u8 ps_state)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +       struct ps_data *psdata = nxpdev->psdata;
> +       int status;
> +
> +       if (psdata->ps_state == ps_state)
> +               return;
> +
> +       switch (psdata->cur_wakeupmode) {
> +       case WAKEUP_METHOD_DTR:
> +               if (ps_state == PS_STATE_AWAKE)
> +                       serdev_device_set_tiocm(nxpdev->serdev, TIOCM_DTR, 0);
> +               else
> +                       serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_DTR);
> +               break;
> +       case WAKEUP_METHOD_BREAK:
> +       default:
> +               if (ps_state == PS_STATE_AWAKE)
> +                       status = serdev_device_break_ctl(nxpdev->serdev, 0);
> +               else
> +                       status = serdev_device_break_ctl(nxpdev->serdev, -1);
> +               bt_dev_info(hdev, "Set UART break: %s, status=%d",
> +                           ps_state == PS_STATE_AWAKE ? "off" : "on", status);
> +               break;
> +       }
> +       psdata->ps_state = ps_state;
> +       if (ps_state == PS_STATE_AWAKE)
> +               btnxpuart_tx_wakeup(nxpdev);
> +}
> +
> +static void ps_work_func(struct work_struct *work)
> +{
> +       struct ps_data *data = container_of(work, struct ps_data, work);
> +
> +       if (!data)
> +               return;
> +
> +       if (data->ps_cmd == PS_CMD_ENTER_PS && data->cur_psmode == PS_MODE_ENABLE)
> +               ps_control(data->hdev, PS_STATE_SLEEP);
> +       else if (data->ps_cmd == PS_CMD_EXIT_PS)
> +               ps_control(data->hdev, PS_STATE_AWAKE);
> +}
> +
> +static void ps_timeout_func(struct timer_list *t)
> +{
> +       struct ps_data *data = from_timer(data, t, ps_timer);
> +       struct hci_dev *hdev = data->hdev;
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +
> +       data->timer_on = false;
> +       if (test_bit(BTNXPUART_TX_STATE_ACTIVE, &nxpdev->tx_state)) {
> +               ps_start_timer(nxpdev);
> +       } else {
> +               data->ps_cmd = PS_CMD_ENTER_PS;
> +               schedule_work(&data->work);
> +       }
> +}
> +
> +static int ps_init_work(struct hci_dev *hdev)
> +{
> +       struct ps_data *psdata = kzalloc(sizeof(*psdata), GFP_KERNEL);
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +
> +       if (!psdata) {
> +               bt_dev_err(hdev, "Can't allocate control structure for Power Save feature");
> +               return -ENOMEM;
> +       }
> +       nxpdev->psdata = psdata;
> +
> +       psdata->interval = PS_DEFAULT_TIMEOUT_PERIOD;
> +       psdata->ps_state = PS_STATE_AWAKE;
> +       psdata->ps_mode = ps_mode;
> +       psdata->hdev = hdev;
> +
> +       switch (wakeupmode) {
> +       case WAKEUP_METHOD_DTR:
> +               psdata->wakeupmode = WAKEUP_METHOD_DTR;
> +               break;
> +       case WAKEUP_METHOD_BREAK:
> +       default:
> +               psdata->wakeupmode = WAKEUP_METHOD_BREAK;
> +               break;
> +       }
> +       psdata->cur_psmode = PS_MODE_DISABLE;
> +       psdata->cur_wakeupmode = WAKEUP_METHOD_INVALID;
> +       INIT_WORK(&psdata->work, ps_work_func);
> +
> +       return 0;
> +}
> +
> +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 = false;
> +       timer_setup(&psdata->ps_timer, ps_timeout_func, 0);
> +}
> +
> +static int ps_wakeup(struct btnxpuart_dev *nxpdev)
> +{
> +       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;
> +}
> +
> +static int send_ps_cmd(struct hci_dev *hdev, void *data)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +       struct ps_data *psdata = nxpdev->psdata;
> +       u8 pcmd;
> +       struct sk_buff *skb;
> +       u8 *status;
> +
> +       if (psdata->ps_mode == PS_MODE_ENABLE)
> +               pcmd = BT_PS_ENABLE;
> +       else
> +               pcmd = BT_PS_DISABLE;
> +
> +       skb = nxp_drv_send_cmd(hdev, HCI_NXP_AUTO_SLEEP_MODE, 1, &pcmd);
> +       if (IS_ERR(skb)) {
> +               bt_dev_err(hdev, "Setting Power Save mode failed (%ld)", PTR_ERR(skb));
> +               return PTR_ERR(skb);
> +       }
> +
> +       status = skb_pull_data(skb, 1);
> +       if (status) {
> +               if (!*status)
> +                       psdata->cur_psmode = psdata->ps_mode;
> +               else
> +                       psdata->ps_mode = psdata->cur_psmode;
> +               if (psdata->cur_psmode == PS_MODE_ENABLE)
> +                       ps_start_timer(nxpdev);
> +               else
> +                       ps_wakeup(nxpdev);
> +               bt_dev_info(hdev, "Power Save mode response: status=%d, ps_mode=%d",
> +                           *status, psdata->cur_psmode);

Like Greg already mentioned the above should probably be converted to
bt_dev_dbg otherwise we just flood the logs with message that are not
really useful if you are not debugging this driver.

> +       }
> +       kfree_skb(skb);
> +
> +       return 0;
> +}
> +
> +static int send_wakeup_method_cmd(struct hci_dev *hdev, void *data)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +       struct ps_data *psdata = nxpdev->psdata;
> +       u8 pcmd[4];
> +       struct sk_buff *skb;
> +       u8 *status;
> +
> +       pcmd[0] = BT_HOST_WAKEUP_METHOD_NONE;
> +       pcmd[1] = 0xff;
> +       switch (psdata->wakeupmode) {
> +       case WAKEUP_METHOD_DTR:
> +               pcmd[2] = BT_CTRL_WAKEUP_METHOD_DSR;
> +               break;
> +       case WAKEUP_METHOD_BREAK:
> +       default:
> +               pcmd[2] = BT_CTRL_WAKEUP_METHOD_BREAK;
> +               break;
> +       }
> +       pcmd[3] = 0xff;
> +
> +       skb = nxp_drv_send_cmd(hdev, HCI_NXP_WAKEUP_METHOD, 4, pcmd);
> +       if (IS_ERR(skb)) {
> +               bt_dev_err(hdev, "Setting wake-up method failed (%ld)", PTR_ERR(skb));
> +               return PTR_ERR(skb);
> +       }
> +
> +       status = skb_pull_data(skb, 1);
> +       if (status) {
> +               if (*status == 0)
> +                       psdata->cur_wakeupmode = psdata->wakeupmode;
> +               else
> +                       psdata->wakeupmode = psdata->cur_wakeupmode;
> +               bt_dev_info(hdev, "Set Wakeup Method response: status=%d, wakeupmode=%d",
> +                           *status, psdata->cur_wakeupmode);

Ditto.

> +       }
> +       kfree_skb(skb);
> +
> +       return 0;
> +}
> +
> +static int ps_init(struct hci_dev *hdev)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +       struct ps_data *psdata = nxpdev->psdata;
> +
> +       serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_RTS);
> +       usleep_range(5000, 10000);
> +       serdev_device_set_tiocm(nxpdev->serdev, TIOCM_RTS, 0);
> +       usleep_range(5000, 10000);
> +
> +       switch (psdata->wakeupmode) {
> +       case WAKEUP_METHOD_DTR:
> +               serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_DTR);
> +               serdev_device_set_tiocm(nxpdev->serdev, TIOCM_DTR, 0);
> +               break;
> +       case WAKEUP_METHOD_BREAK:
> +       default:
> +               serdev_device_break_ctl(nxpdev->serdev, -1);
> +               usleep_range(5000, 10000);
> +               serdev_device_break_ctl(nxpdev->serdev, 0);
> +               usleep_range(5000, 10000);
> +               break;
> +       }
> +       if (!test_bit(HCI_RUNNING, &hdev->flags)) {
> +               bt_dev_err(hdev, "HCI_RUNNING is not set");
> +               return -EBUSY;
> +       }
> +       if (psdata->cur_wakeupmode != psdata->wakeupmode)
> +               hci_cmd_sync_queue(hdev, send_wakeup_method_cmd, NULL, NULL);
> +       if (psdata->cur_psmode != psdata->ps_mode)
> +               hci_cmd_sync_queue(hdev, send_ps_cmd, NULL, NULL);
> +
> +       return 0;
> +}
> +
> +/* NXP Firmware Download Feature */
> +static void nxp_fw_dnld_gen_crc32_table(void)
> +{
> +       int i, j;
> +       unsigned long crc_accum;
> +
> +       for (i = 0; i < 256; i++) {
> +               crc_accum = ((unsigned long)i << 24);
> +               for (j = 0; j < 8; j++) {
> +                       if (crc_accum & 0x80000000L)
> +                               crc_accum = (crc_accum << 1) ^ POLYNOMIAL32;
> +                       else
> +                               crc_accum = (crc_accum << 1);
> +               }
> +               crc32_table[i] = crc_accum;
> +       }
> +}
> +
> +static unsigned long nxp_fw_dnld_update_crc(unsigned long crc_accum,
> +                                           char *data_blk_ptr,
> +                                           int data_blk_size)
> +{
> +       unsigned long i, j;
> +
> +       for (j = 0; j < data_blk_size; j++) {
> +               i = ((unsigned long)(crc_accum >> 24) ^ *data_blk_ptr++) & 0xff;
> +               crc_accum = (crc_accum << 8) ^ crc32_table[i];
> +       }
> +       return crc_accum;
> +}
> +
> +static int nxp_download_firmware(struct hci_dev *hdev)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +       int err = 0;
> +
> +       nxpdev->fw_dnld_v1_offset = 0;
> +       nxpdev->fw_v1_sent_bytes = 0;
> +       nxpdev->fw_v1_expected_len = HDR_LEN;
> +       nxpdev->fw_v3_offset_correction = 0;
> +       nxpdev->baudrate_changed = false;
> +       nxpdev->timeout_changed = false;
> +
> +       crc8_populate_msb(crc8_table, POLYNOMIAL8);
> +       nxp_fw_dnld_gen_crc32_table();
> +
> +       serdev_device_set_baudrate(nxpdev->serdev, HCI_NXP_PRI_BAUDRATE);
> +       serdev_device_set_flow_control(nxpdev->serdev, 0);
> +       nxpdev->current_baudrate = HCI_NXP_PRI_BAUDRATE;
> +
> +       /* Wait till FW is downloaded and CTS becomes low */
> +       err = wait_event_interruptible_timeout(nxpdev->suspend_wait_q,
> +                                              !test_bit(BTNXPUART_FW_DOWNLOADING,
> +                                                        &nxpdev->tx_state),
> +                                              msecs_to_jiffies(60000));
> +       if (err == 0) {
> +               bt_dev_err(hdev, "FW Download Timeout.");
> +               return -ETIMEDOUT;
> +       }
> +
> +       serdev_device_set_flow_control(nxpdev->serdev, 1);
> +       err = serdev_device_wait_for_cts(nxpdev->serdev, 1, 60000);
> +       if (err < 0) {
> +               bt_dev_err(hdev, "CTS is still high. FW Download failed.");
> +               return err;
> +       }
> +       bt_dev_info(hdev, "CTS is low");

Ditto, I actually would just get rid of this one since you can infer
it when the firmware loading succeeded.

> +       release_firmware(nxpdev->fw);
> +       memset(nxpdev->fw_name, 0, MAX_FW_FILE_NAME_LEN);
> +
> +       /* Allow the downloaded FW to initialize */
> +       usleep_range(800000, 1000000);
> +
> +       return 0;
> +}
> +
> +static int nxp_send_ack(u8 ack, struct hci_dev *hdev)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +       u8 ack_nak[2];
> +
> +       if (ack == NXP_ACK_V1 || ack == NXP_NAK_V1) {
> +               ack_nak[0] = ack;
> +               serdev_device_write_buf(nxpdev->serdev, ack_nak, 1);
> +       } else if (ack == NXP_ACK_V3) {
> +               ack_nak[0] = ack;
> +               ack_nak[1] = crc8(crc8_table, ack_nak, 1, 0xff);
> +               serdev_device_write_buf(nxpdev->serdev, ack_nak, 2);
> +       }
> +       return 0;
> +}
> +
> +static bool nxp_fw_change_baudrate(struct hci_dev *hdev, u16 req_len)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +       struct nxp_bootloader_cmd nxp_cmd5;
> +       struct uart_config uart_config;
> +
> +       if (req_len == sizeof(nxp_cmd5)) {
> +               nxp_cmd5.header = __cpu_to_le32(5);
> +               nxp_cmd5.arg = 0;
> +               nxp_cmd5.payload_len = __cpu_to_le32(sizeof(uart_config));
> +               nxp_cmd5.crc = swab32(nxp_fw_dnld_update_crc(0UL,
> +                                                            (char *)&nxp_cmd5,
> +                                                            sizeof(nxp_cmd5) - 4));
> +
> +               serdev_device_write_buf(nxpdev->serdev, (u8 *)&nxp_cmd5, req_len);
> +               nxpdev->fw_v3_offset_correction += req_len;
> +       } else if (req_len == sizeof(uart_config)) {
> +               uart_config.clkdiv.address = __cpu_to_le32(CLKDIVADDR);
> +               uart_config.clkdiv.value = __cpu_to_le32(0x00c00000);
> +               uart_config.uartdiv.address = __cpu_to_le32(UARTDIVADDR);
> +               uart_config.uartdiv.value = __cpu_to_le32(1);
> +               uart_config.mcr.address = __cpu_to_le32(UARTMCRADDR);
> +               uart_config.mcr.value = __cpu_to_le32(MCR);
> +               uart_config.re_init.address = __cpu_to_le32(UARTREINITADDR);
> +               uart_config.re_init.value = __cpu_to_le32(INIT);
> +               uart_config.icr.address = __cpu_to_le32(UARTICRADDR);
> +               uart_config.icr.value = __cpu_to_le32(ICR);
> +               uart_config.fcr.address = __cpu_to_le32(UARTFCRADDR);
> +               uart_config.fcr.value = __cpu_to_le32(FCR);
> +               uart_config.crc = swab32(nxp_fw_dnld_update_crc(0UL,
> +                                                               (char *)&uart_config,
> +                                                               sizeof(uart_config) - 4));
> +               serdev_device_write_buf(nxpdev->serdev, (u8 *)&uart_config, req_len);
> +               serdev_device_wait_until_sent(nxpdev->serdev, 0);
> +               nxpdev->fw_v3_offset_correction += req_len;
> +               return true;
> +       }
> +       return false;
> +}
> +
> +static bool nxp_fw_change_timeout(struct hci_dev *hdev, u16 req_len)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +       struct nxp_bootloader_cmd nxp_cmd7;
> +
> +       if (req_len != sizeof(nxp_cmd7))
> +               return false;
> +
> +       nxp_cmd7.header = __cpu_to_le32(7);
> +       nxp_cmd7.arg = __cpu_to_le32(0x70);
> +       nxp_cmd7.payload_len = 0;
> +       nxp_cmd7.crc = swab32(nxp_fw_dnld_update_crc(0UL,
> +                                                    (char *)&nxp_cmd7,
> +                                                    sizeof(nxp_cmd7) - 4));
> +
> +       serdev_device_write_buf(nxpdev->serdev, (u8 *)&nxp_cmd7, req_len);
> +       serdev_device_wait_until_sent(nxpdev->serdev, 0);
> +       nxpdev->fw_v3_offset_correction += req_len;
> +       return true;
> +}
> +
> +static u32 nxp_get_data_len(const u8 *buf)
> +{
> +       struct nxp_bootloader_cmd *hdr = (struct nxp_bootloader_cmd *)buf;
> +
> +       return __le32_to_cpu(hdr->payload_len);
> +}
> +
> +/* for legacy chipsets with V1 bootloader */
> +static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +       struct v1_data_req *req = skb_pull_data(skb, sizeof(struct v1_data_req));
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +       struct btnxpuart_data *nxp_data = nxpdev->nxp_data;
> +       u32 requested_len;
> +       int err;
> +
> +       if (!test_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state))
> +               goto ret;
> +
> +       if (req && (req->len ^ req->len_comp) != 0xffff) {
> +               bt_dev_info(hdev, "ERR: Send NAK");

Ditto, please use bt_dev_dbg, btw you wouldn't have to do this sort
for logging if there you add decoding support for these message to the
likes of btmon.

> +               nxp_send_ack(NXP_NAK_V1, hdev);
> +               goto ret;
> +       }
> +       nxp_send_ack(NXP_ACK_V1, hdev);
> +
> +       if (nxp_data->fw_dnld_use_high_baudrate) {
> +               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 (!strlen(nxpdev->fw_name)) {
> +               snprintf(nxpdev->fw_name, MAX_FW_FILE_NAME_LEN, "%s",
> +                        nxp_data->fw_name);
> +               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);
> +                       return err;
> +               }
> +       }
> +
> +       requested_len = req->len;
> +       if (requested_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 (requested_len & 0x01) {
> +               /* The CRC did not match at the other end.
> +                * Simply send the same bytes again.
> +                */
> +               requested_len = nxpdev->fw_v1_sent_bytes;
> +               bt_dev_err(hdev, "CRC error. Resend %d bytes of FW.", requested_len);
> +       } else {
> +               nxpdev->fw_dnld_v1_offset += nxpdev->fw_v1_sent_bytes;
> +
> +               /* The FW bin file is made up of many blocks of
> +                * 16 byte header and payload data chunks. If the
> +                * FW has requested a header, read the payload length
> +                * info from the header, before sending the header.
> +                * In the next iteration, the FW should request the
> +                * payload data chunk, which should be equal to the
> +                * payload length read from header. If there is a
> +                * mismatch, clearly the driver and FW are out of sync,
> +                * and we need to re-send the previous header again.
> +                */
> +               if (requested_len == nxpdev->fw_v1_expected_len) {
> +                       if (requested_len == HDR_LEN)
> +                               nxpdev->fw_v1_expected_len = nxp_get_data_len(nxpdev->fw->data +
> +                                                                       nxpdev->fw_dnld_v1_offset);
> +                       else
> +                               nxpdev->fw_v1_expected_len = HDR_LEN;
> +               } else {
> +                       if (requested_len == HDR_LEN) {
> +                               /* FW download out of sync. Send previous chunk again */
> +                               nxpdev->fw_dnld_v1_offset -= nxpdev->fw_v1_sent_bytes;
> +                               nxpdev->fw_v1_expected_len = HDR_LEN;
> +                       }
> +               }
> +       }
> +
> +       if (nxpdev->fw_dnld_v1_offset + requested_len <= nxpdev->fw->size)
> +               serdev_device_write_buf(nxpdev->serdev,
> +                                       nxpdev->fw->data + nxpdev->fw_dnld_v1_offset,
> +                                       requested_len);
> +       nxpdev->fw_v1_sent_bytes = requested_len;
> +
> +ret:
> +       kfree_skb(skb);
> +       return 0;
> +}
> +
> +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;
> +               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_err(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");

Can't we actually power cycle instead of printing an error?

> +               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_info(hdev, "Set baudrate response: status=%d, baudrate=%d",
> +                           *status, nxpdev->new_baudrate);

Ditto, use bt_dev_dbg above.

> +       }
> +       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_info(hdev, "CTS high. Need FW Download");

Ditto.

> +               err = nxp_download_firmware(hdev);
> +               if (err < 0)
> +                       return err;
> +       } else {
> +               bt_dev_info(hdev, "CTS low. FW already running.");

Ditto.

> +               clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
> +       }
> +
> +       serdev_device_set_flow_control(nxpdev->serdev, 1);
> +       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;
> +               hci_cmd_sync_queue(hdev, nxp_set_baudrate_cmd, NULL, NULL);
> +       }
> +
> +       ps_init(hdev);
> +
> +       return 0;
> +}
> +
> +static int nxp_enqueue(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +       struct ps_data *psdata = nxpdev->psdata;
> +       struct hci_command_hdr *hdr;
> +       u8 *param;
> +
> +       if (!nxpdev || !psdata)
> +               goto free_skb;
> +
> +       /* if vendor commands are received from user space (e.g. hcitool), update
> +        * driver flags accordingly and ask driver to re-send the command to FW.
> +        */
> +       if (bt_cb(skb)->pkt_type == HCI_COMMAND_PKT && !psdata->driver_sent_cmd) {
> +               hdr = (struct hci_command_hdr *)skb->data;

It is not safe to access the contents of skb->data without first
checking skb->len, I understand you can't use skb_pull_data since that
changes the packet but Im not so happy with this code either way since
you appear to be doing this only to support userspace initiating these
commands but is that really expected or you are just doing this for
testing purpose? Also why not doing this handling on the command
complete/command status event as that would be common to both driver
or userspace initiated?


> +               param = skb->data + HCI_COMMAND_HDR_SIZE;
> +               switch (__le16_to_cpu(hdr->opcode)) {
> +               case HCI_NXP_AUTO_SLEEP_MODE:
> +                       if (hdr->plen >= 1) {
> +                               if (param[0] == BT_PS_ENABLE)
> +                                       psdata->ps_mode = PS_MODE_ENABLE;
> +                               else if (param[0] == BT_PS_DISABLE)
> +                                       psdata->ps_mode = PS_MODE_DISABLE;
> +                               hci_cmd_sync_queue(hdev, send_ps_cmd, NULL, NULL);
> +                               goto free_skb;
> +                       }
> +                       break;
> +               case HCI_NXP_WAKEUP_METHOD:
> +                       if (hdr->plen >= 4) {
> +                               switch (param[2]) {
> +                               case BT_CTRL_WAKEUP_METHOD_DSR:
> +                                       psdata->wakeupmode = WAKEUP_METHOD_DTR;
> +                                       break;
> +                               case BT_CTRL_WAKEUP_METHOD_BREAK:
> +                               default:
> +                                       psdata->wakeupmode = WAKEUP_METHOD_BREAK;
> +                                       break;
> +                               }
> +                               hci_cmd_sync_queue(hdev, send_wakeup_method_cmd, NULL, NULL);
> +                               goto free_skb;
> +                       }
> +                       break;
> +               case HCI_NXP_SET_OPER_SPEED:
> +                       if (hdr->plen == 4) {
> +                               nxpdev->new_baudrate = *((u32 *)param);
> +                               hci_cmd_sync_queue(hdev, nxp_set_baudrate_cmd, NULL, NULL);
> +                               goto free_skb;
> +                       }
> +                       break;
> +               case HCI_NXP_IND_RESET:
> +                       if (hdr->plen == 1) {
> +                               hci_cmd_sync_queue(hdev, nxp_set_ind_reset, NULL, NULL);
> +                               goto free_skb;
> +                       }
> +                       break;
> +               default:
> +                       break;
> +               }
> +       }
> +
> +       /* Prepend skb with frame type */
> +       memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
> +       skb_queue_tail(&nxpdev->txq, skb);
> +
> +       btnxpuart_tx_wakeup(nxpdev);
> +ret:
> +       return 0;
> +
> +free_skb:
> +       kfree_skb(skb);
> +       goto ret;
> +}
> +
> +static struct sk_buff *nxp_dequeue(void *data)
> +{
> +       struct btnxpuart_dev *nxpdev = (struct btnxpuart_dev *)data;
> +
> +       ps_wakeup(nxpdev);
> +       ps_start_timer(nxpdev);
> +       return skb_dequeue(&nxpdev->txq);
> +}
> +
> +/* btnxpuart based on serdev */
> +static void btnxpuart_tx_work(struct work_struct *work)
> +{
> +       struct btnxpuart_dev *nxpdev = container_of(work, struct btnxpuart_dev,
> +                                                  tx_work);
> +       struct serdev_device *serdev = nxpdev->serdev;
> +       struct hci_dev *hdev = nxpdev->hdev;
> +       struct sk_buff *skb;
> +       int len;
> +
> +       while ((skb = nxp_dequeue(nxpdev))) {
> +               len = serdev_device_write_buf(serdev, skb->data, skb->len);
> +               hdev->stat.byte_tx += len;
> +
> +               skb_pull(skb, len);
> +               if (skb->len > 0) {
> +                       skb_queue_head(&nxpdev->txq, skb);
> +                       break;
> +               }
> +
> +               switch (hci_skb_pkt_type(skb)) {
> +               case HCI_COMMAND_PKT:
> +                       hdev->stat.cmd_tx++;
> +                       break;
> +               case HCI_ACLDATA_PKT:
> +                       hdev->stat.acl_tx++;
> +                       break;
> +               case HCI_SCODATA_PKT:
> +                       hdev->stat.sco_tx++;
> +                       break;
> +               }
> +
> +               kfree_skb(skb);
> +       }
> +       clear_bit(BTNXPUART_TX_STATE_ACTIVE, &nxpdev->tx_state);
> +}
> +
> +static int btnxpuart_open(struct hci_dev *hdev)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +       int err = 0;
> +
> +       err = serdev_device_open(nxpdev->serdev);
> +       if (err) {
> +               bt_dev_err(hdev, "Unable to open UART device %s",
> +                          dev_name(&nxpdev->serdev->dev));
> +       }
> +
> +       return err;
> +}
> +
> +static int btnxpuart_close(struct hci_dev *hdev)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +
> +       if (!nxpdev)
> +               return 0;
> +
> +       serdev_device_close(nxpdev->serdev);
> +
> +       return 0;
> +}
> +
> +static int btnxpuart_flush(struct hci_dev *hdev)
> +{
> +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +
> +       if (!nxpdev)
> +               return 0;
> +
> +       /* Flush any pending characters */
> +       serdev_device_write_flush(nxpdev->serdev);
> +       skb_queue_purge(&nxpdev->txq);
> +
> +       cancel_work_sync(&nxpdev->tx_work);
> +
> +       kfree_skb(nxpdev->rx_skb);
> +       nxpdev->rx_skb = NULL;
> +
> +       return 0;
> +}
> +
> +static const struct h4_recv_pkt nxp_recv_pkts[] = {
> +       { H4_RECV_ACL,          .recv = hci_recv_frame },
> +       { H4_RECV_SCO,          .recv = hci_recv_frame },
> +       { H4_RECV_EVENT,        .recv = hci_recv_frame },
> +       { NXP_RECV_FW_REQ_V1,   .recv = nxp_recv_fw_req_v1 },
> +       { NXP_RECV_CHIP_VER_V3, .recv = nxp_recv_chip_ver_v3 },
> +       { NXP_RECV_FW_REQ_V3,   .recv = nxp_recv_fw_req_v3 },
> +};
> +
> +static int btnxpuart_receive_buf(struct serdev_device *serdev, const u8 *data,
> +                                size_t count)
> +{
> +       struct btnxpuart_dev *nxpdev = serdev_device_get_drvdata(serdev);
> +
> +       if (test_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state)) {
> +               if (*data != NXP_V1_FW_REQ_PKT && *data != NXP_V1_CHIP_VER_PKT &&
> +                   *data != NXP_V3_FW_REQ_PKT && *data != NXP_V3_CHIP_VER_PKT) {
> +                       /* Unknown bootloader signature, skip without returning error */
> +                       return count;
> +               }
> +       }
> +
> +       ps_start_timer(nxpdev);
> +
> +       nxpdev->rx_skb = h4_recv_buf(nxpdev->hdev, nxpdev->rx_skb, data, count,
> +                                    nxp_recv_pkts, ARRAY_SIZE(nxp_recv_pkts));
> +       if (IS_ERR(nxpdev->rx_skb)) {
> +               int err = PTR_ERR(nxpdev->rx_skb);
> +
> +               bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
> +               nxpdev->rx_skb = NULL;
> +               return err;
> +       }
> +       nxpdev->hdev->stat.byte_rx += count;
> +       return count;
> +}
> +
> +static void btnxpuart_write_wakeup(struct serdev_device *serdev)
> +{
> +       serdev_device_write_wakeup(serdev);
> +}
> +
> +static const struct serdev_device_ops btnxpuart_client_ops = {
> +       .receive_buf = btnxpuart_receive_buf,
> +       .write_wakeup = btnxpuart_write_wakeup,
> +};
> +
> +static int nxp_serdev_probe(struct serdev_device *serdev)
> +{
> +       struct hci_dev *hdev;
> +       struct btnxpuart_dev *nxpdev;
> +
> +       nxpdev = devm_kzalloc(&serdev->dev, sizeof(*nxpdev), GFP_KERNEL);
> +       if (!nxpdev)
> +               return -ENOMEM;
> +
> +       nxpdev->nxp_data = (struct btnxpuart_data *)device_get_match_data(&serdev->dev);
> +
> +       nxpdev->serdev = serdev;
> +       serdev_device_set_drvdata(serdev, nxpdev);
> +
> +       serdev_device_set_client_ops(serdev, &btnxpuart_client_ops);
> +
> +       INIT_WORK(&nxpdev->tx_work, btnxpuart_tx_work);
> +       skb_queue_head_init(&nxpdev->txq);
> +
> +       /* Initialize and register HCI device */
> +       hdev = hci_alloc_dev();
> +       if (!hdev) {
> +               dev_err(&serdev->dev, "Can't allocate HCI device\n");
> +               return -ENOMEM;
> +       }
> +
> +       nxpdev->hdev = hdev;
> +
> +       hdev->bus = HCI_UART;
> +       hci_set_drvdata(hdev, nxpdev);
> +
> +       hdev->manufacturer = MANUFACTURER_NXP;
> +       hdev->open  = btnxpuart_open;
> +       hdev->close = btnxpuart_close;
> +       hdev->flush = btnxpuart_flush;
> +       hdev->setup = nxp_setup;
> +       hdev->send  = nxp_enqueue;
> +       SET_HCIDEV_DEV(hdev, &serdev->dev);
> +
> +       if (hci_register_dev(hdev) < 0) {
> +               dev_err(&serdev->dev, "Can't register HCI device\n");
> +               hci_free_dev(hdev);
> +               return -ENODEV;
> +       }
> +
> +       if (!ps_init_work(hdev))
> +               ps_init_timer(hdev);
> +
> +       return 0;
> +}
> +
> +static void nxp_serdev_remove(struct serdev_device *serdev)
> +{
> +       struct btnxpuart_dev *nxpdev = serdev_device_get_drvdata(serdev);
> +       struct hci_dev *hdev = nxpdev->hdev;
> +
> +       /* Restore FW baudrate to init_baudrate if changed.
> +        * This will ensure FW baudrate is in sync with
> +        * driver baudrate in case this driver is re-inserted.
> +        */
> +       if (init_baudrate != nxpdev->current_baudrate) {
> +               nxpdev->new_baudrate = init_baudrate;
> +               nxp_set_baudrate_cmd(hdev, NULL);
> +       }
> +
> +       ps_cancel_timer(nxpdev);
> +       hci_unregister_dev(hdev);
> +       hci_free_dev(hdev);
> +}
> +
> +static struct btnxpuart_data w8987_data = {
> +       .fw_dnld_use_high_baudrate = true,
> +       .fw_name = FIRMWARE_W8987,
> +};
> +
> +static struct btnxpuart_data w8997_data = {
> +       .fw_dnld_use_high_baudrate = false,
> +       .fw_name = FIRMWARE_W8997,
> +};
> +
> +static const struct of_device_id nxpuart_of_match_table[] = {
> +       { .compatible = "nxp,88w8987-bt", .data = &w8987_data },
> +       { .compatible = "nxp,88w8997-bt", .data = &w8997_data },
> +       { }
> +};
> +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);
> +
> +/* This module parameter is "chip-module vendor" dependent.
> + * Same chip can have different FW init speed depending
> + * on caliberation done by different module vendors.
> + */
> +module_param(init_baudrate, int, 0444);
> +MODULE_PARM_DESC(init_baudrate, "host baudrate after FW download: default=115200");
> +
> +MODULE_AUTHOR("Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>");
> +MODULE_DESCRIPTION("NXP Bluetooth Serial driver v1.0 ");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add NXP bluetooth support
  2023-02-21 16:25 ` [PATCH v4 2/3] dt-bindings: net: bluetooth: Add NXP bluetooth support Neeraj Sanjay Kale
@ 2023-02-22  8:44   ` Krzysztof Kozlowski
  2023-02-23 11:00     ` Neeraj sanjay kale
  0 siblings, 1 reply; 10+ messages in thread
From: Krzysztof Kozlowski @ 2023-02-22  8:44 UTC (permalink / raw)
  To: Neeraj Sanjay Kale, davem, edumazet, kuba, pabeni, robh+dt,
	krzysztof.kozlowski+dt, marcel, johan.hedberg, luiz.dentz,
	gregkh, jirislaby, alok.a.tiwari, hdanton, ilpo.jarvinen, leon
  Cc: netdev, devicetree, linux-kernel, linux-bluetooth, linux-serial,
	amitkumar.karwar, rohit.fule, sherry.sun

On 21/02/2023 17:25, Neeraj Sanjay Kale wrote:
> Add binding document for NXP bluetooth chipsets attached over UART.
> 
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> ---
> v2: Resolved dt_binding_check errors. (Rob Herring)
> v2: Modified description, added specific compatibility devices, corrected indentations. (Krzysztof Kozlowski)
> v3: Modified description, renamed file (Krzysztof Kozlowski)
> v4: Resolved dt_binding_check errors, corrected indentation. (Rob
> Herring, Krzysztof Kozlowski)
> ---
>  .../bindings/net/bluetooth/nxp,w8987-bt.yaml  | 38 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++
>  2 files changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/bluetooth/nxp,w8987-bt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/nxp,w8987-bt.yaml b/Documentation/devicetree/bindings/net/bluetooth/nxp,w8987-bt.yaml
> new file mode 100644
> index 000000000000..de361ce4ab73
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/bluetooth/nxp,w8987-bt.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/bluetooth/nxp,w8987-bt.yaml#

I think list of compatibles changed... now they are nxp,88w8987-bt, so
shouldn't the filename be "nxp,88w8987-bt.yaml"?

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP Bluetooth chips
> +
> +description:
> +  This binding describes UART-attached NXP bluetooth chips.
> +  These chips are dual-radio chips supporting WiFi and Bluetooth.
> +  The bluetooth works on standard H4 protocol over 4-wire UART.
> +  The RTS and CTS lines are used during FW download.
> +  To enable power save mode, the host asserts break signal
> +  over UART-TX line to put the chip into power save state.
> +  De-asserting break wakes-up the BT chip.
> +
> +maintainers:
> +  - Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,88w8987-bt
> +      - nxp,88w8997-bt
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    uart2 {

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets
  2023-02-21 16:47   ` Greg KH
@ 2023-02-23 10:40     ` Neeraj sanjay kale
  0 siblings, 0 replies; 10+ messages in thread
From: Neeraj sanjay kale @ 2023-02-23 10:40 UTC (permalink / raw)
  To: Greg KH
  Cc: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
	marcel, johan.hedberg, luiz.dentz, jirislaby, alok.a.tiwari,
	hdanton, ilpo.jarvinen, leon, netdev, devicetree, linux-kernel,
	linux-bluetooth, linux-serial, Amitkumar Karwar, Rohit Fule,
	Sherry Sun

Hi Greg,

Thank you for reviewing this patch.

> > +             bt_dev_info(hdev, "Set UART break: %s, status=%d",
> > +                         ps_state == PS_STATE_AWAKE ? "off" : "on",
> > + status);
> 
> You have a lot of "noise" in this driver, remove all "info" messages, as if a
> driver is working properly, it is quiet.
> 
Replaced all bt_dev_info() and bt_dev_err() with bt_dev_dbg() for all instances where user action is not possible.

> 
> > +     } else if (req_len == sizeof(uart_config)) {
> > +             uart_config.clkdiv.address = __cpu_to_le32(CLKDIVADDR);
> > +             uart_config.clkdiv.value = __cpu_to_le32(0x00c00000);
> > +             uart_config.uartdiv.address = __cpu_to_le32(UARTDIVADDR);
> > +             uart_config.uartdiv.value = __cpu_to_le32(1);
> > +             uart_config.mcr.address = __cpu_to_le32(UARTMCRADDR);
> > +             uart_config.mcr.value = __cpu_to_le32(MCR);
> > +             uart_config.re_init.address = __cpu_to_le32(UARTREINITADDR);
> > +             uart_config.re_init.value = __cpu_to_le32(INIT);
> > +             uart_config.icr.address = __cpu_to_le32(UARTICRADDR);
> > +             uart_config.icr.value = __cpu_to_le32(ICR);
> > +             uart_config.fcr.address = __cpu_to_le32(UARTFCRADDR);
> > +             uart_config.fcr.value = __cpu_to_le32(FCR);
> > +             uart_config.crc = swab32(nxp_fw_dnld_update_crc(0UL,
> > +                                                             (char *)&uart_config,
> > +                                                             sizeof(uart_config) - 4));
> > +             serdev_device_write_buf(nxpdev->serdev, (u8 *)&uart_config,
> req_len);
> > +             serdev_device_wait_until_sent(nxpdev->serdev, 0);
> 
> You are sending magic commands over the serial connection, are you sure
> that is ok?
Yes, we are sending this only when the BT chip's bootloader is requesting for payload for the CMD5 sent earlier during FW download.

Thanks,
Neeraj

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets
  2023-02-21 21:37   ` Luiz Augusto von Dentz
@ 2023-02-23 10:57     ` Neeraj sanjay kale
  0 siblings, 0 replies; 10+ messages in thread
From: Neeraj sanjay kale @ 2023-02-23 10:57 UTC (permalink / raw)
  To: Luiz Augusto von Dentz
  Cc: davem, edumazet, kuba, pabeni, robh+dt, krzysztof.kozlowski+dt,
	marcel, johan.hedberg, gregkh, jirislaby, alok.a.tiwari, hdanton,
	ilpo.jarvinen, leon, netdev, devicetree, linux-kernel,
	linux-bluetooth, linux-serial, Amitkumar Karwar, Rohit Fule,
	Sherry Sun

Hi Luiz,

Thank you for reviewing this patch. I have resolved all the comments in V5 patch.

> > +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_err(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");
> 
> Can't we actually power cycle instead of printing an error?
The NXP chips draw power from the platform's 5V power supply, which is used by WLAN as well as BT sub-system inside the chip. These chips have no mechanism to reset or power-cycle BT only sub-system independently.
> 
> > +               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_enqueue(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +       struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> > +       struct ps_data *psdata = nxpdev->psdata;
> > +       struct hci_command_hdr *hdr;
> > +       u8 *param;
> > +
> > +       if (!nxpdev || !psdata)
> > +               goto free_skb;
> > +
> > +       /* if vendor commands are received from user space (e.g. hcitool),
> update
> > +        * driver flags accordingly and ask driver to re-send the command to
> FW.
> > +        */
> > +       if (bt_cb(skb)->pkt_type == HCI_COMMAND_PKT && !psdata-
> >driver_sent_cmd) {
> > +               hdr = (struct hci_command_hdr *)skb->data;
> 
> It is not safe to access the contents of skb->data without first
> checking skb->len, I understand you can't use skb_pull_data since that
> changes the packet but Im not so happy with this code either way since
> you appear to be doing this only to support userspace initiating these
> commands but is that really expected or you are just doing this for
> testing purpose? Also why not doing this handling on the command
> complete/command status event as that would be common to both driver
> or userspace initiated?
> 
I have made few changes to handle this issue in a safe way by checking
skb->len and hdr->plen before using the parameters.
We do need to parse a couple of user space vendor commands before forwarding
them to the FW, since the driver needs to update its internal flags and mechanism
accordingly. We do not usually get the parameters while handling command complete
or command status events.
In one of the previous patches I was parsing parameters in nxp_enqueue, and updating
driver flags in ps_check_event_packet() on status success.
https://patchwork.kernel.org/project/bluetooth/patch/1669207413-9637-1-git-send-email-neeraj.sanjaykale@nxp.com/

> 
> > +               param = skb->data + HCI_COMMAND_HDR_SIZE;
> > +               switch (__le16_to_cpu(hdr->opcode)) {
> > +               case HCI_NXP_AUTO_SLEEP_MODE:
> > +                       if (hdr->plen >= 1) {
> > +                               if (param[0] == BT_PS_ENABLE)
> > +                                       psdata->ps_mode = PS_MODE_ENABLE;
> > +                               else if (param[0] == BT_PS_DISABLE)
> > +                                       psdata->ps_mode = PS_MODE_DISABLE;
> > +                               hci_cmd_sync_queue(hdev, send_ps_cmd, NULL, NULL);
> > +                               goto free_skb;
> > +                       }
> > +                       break;
> > +               case HCI_NXP_WAKEUP_METHOD:
> > +                       if (hdr->plen >= 4) {
> > +                               switch (param[2]) {
> > +                               case BT_CTRL_WAKEUP_METHOD_DSR:
> > +                                       psdata->wakeupmode = WAKEUP_METHOD_DTR;
> > +                                       break;
> > +                               case BT_CTRL_WAKEUP_METHOD_BREAK:
> > +                               default:
> > +                                       psdata->wakeupmode = WAKEUP_METHOD_BREAK;
> > +                                       break;
> > +                               }
> > +                               hci_cmd_sync_queue(hdev, send_wakeup_method_cmd,
> NULL, NULL);
> > +                               goto free_skb;
> > +                       }
> > +                       break;
> > +               case HCI_NXP_SET_OPER_SPEED:
> > +                       if (hdr->plen == 4) {
> > +                               nxpdev->new_baudrate = *((u32 *)param);
> > +                               hci_cmd_sync_queue(hdev, nxp_set_baudrate_cmd,
> NULL, NULL);
> > +                               goto free_skb;
> > +                       }
> > +                       break;
> > +               case HCI_NXP_IND_RESET:
> > +                       if (hdr->plen == 1) {
> > +                               hci_cmd_sync_queue(hdev, nxp_set_ind_reset, NULL,
> NULL);
> > +                               goto free_skb;
> > +                       }
> > +                       break;
> > +               default:
> > +                       break;
> > +               }
> > +       }
> > +
> > +       /* Prepend skb with frame type */
> > +       memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
> > +       skb_queue_tail(&nxpdev->txq, skb);
> > +
> > +       btnxpuart_tx_wakeup(nxpdev);
> > +ret:
> > +       return 0;
> > +
> > +free_skb:
> > +       kfree_skb(skb);
> > +       goto ret;
> > +}
> > +

Thanks,
Neeraj

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add NXP bluetooth support
  2023-02-22  8:44   ` Krzysztof Kozlowski
@ 2023-02-23 11:00     ` Neeraj sanjay kale
  0 siblings, 0 replies; 10+ messages in thread
From: Neeraj sanjay kale @ 2023-02-23 11:00 UTC (permalink / raw)
  To: Krzysztof Kozlowski, davem, edumazet, kuba, pabeni, robh+dt,
	krzysztof.kozlowski+dt, marcel, johan.hedberg, luiz.dentz,
	gregkh, jirislaby, alok.a.tiwari, hdanton, ilpo.jarvinen, leon
  Cc: netdev, devicetree, linux-kernel, linux-bluetooth, linux-serial,
	Amitkumar Karwar, Rohit Fule, Sherry Sun

Hi Krzysztof,

Thank you for reviewing this patch.

> 
> I think list of compatibles changed... now they are nxp,88w8987-bt, so
> shouldn't the filename be "nxp,88w8987-bt.yaml"?
Updated file name.

> > +examples:
> > +  - |
> > +    uart2 {
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all requested
> changes or keep discussing them.
> 
Changed "uart2" to "serial" in v5 patch.

Thanks,
Neeraj

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-02-23 11:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-21 16:25 [PATCH v4 0/3] Add support for NXP bluetooth chipsets Neeraj Sanjay Kale
2023-02-21 16:25 ` [PATCH v4 1/3] serdev: Add method to assert break signal over tty UART port Neeraj Sanjay Kale
2023-02-21 16:25 ` [PATCH v4 2/3] dt-bindings: net: bluetooth: Add NXP bluetooth support Neeraj Sanjay Kale
2023-02-22  8:44   ` Krzysztof Kozlowski
2023-02-23 11:00     ` Neeraj sanjay kale
2023-02-21 16:25 ` [PATCH v4 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets Neeraj Sanjay Kale
2023-02-21 16:47   ` Greg KH
2023-02-23 10:40     ` Neeraj sanjay kale
2023-02-21 21:37   ` Luiz Augusto von Dentz
2023-02-23 10:57     ` Neeraj sanjay kale

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).