add support for Bluetooth on MT7622 SoC

This adds a driver based on serdev driver for the MediaTek serial protocol based on running H:4, which can enable the built-in Bluetooth device inside MT7622 SoC.

Signed-off-by Sean Wang <[email protected]>

---

drivers/bluetooth/Kconfig     |  11 +
drivers/bluetooth/Makefile    |   2 +
drivers/bluetooth/btmtkuart.c | 591 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 604 insertions(+)

create mode 100644 drivers/bluetooth/btmtkuart.c

drivers/bluetooth/Kconfig +11 -0

index f3c643a..5ace676 100644
@@ -74,6 +74,17 @@ config BT_HCIBTSDIO
 	  Say Y here to compile support for Bluetooth SDIO devices into the
 	  kernel or say M to compile it as module (btsdio).
 
+config BT_MTKUART
+	tristate "MediaTek HCI UART driver"
+	depends on SERIAL_DEV_BUS
+	help
+	  MediaTek Bluetooth HCI UART driver.
+	  This driver is required if you want to use MediaTek Bluetooth
+	  with serial interface.
+
+	  Say Y here to compile support for MediaTek Bluetooth UART devices
+	  into the kernel or say M to compile it as module (btmtkuart).
+

config BT_HCIUART

	tristate "HCI UART driver"
	depends on SERIAL_DEV_BUS || !SERIAL_DEV_BUS

drivers/bluetooth/Makefile +0 -0

index ec16c55..12ad6e9 100644
@@ -25,6 +25,8 @@ obj-$(CONFIG_BT_BCM)		+= btbcm.o

obj-$(CONFIG_BT_RTL) += btrtl.o
obj-$(CONFIG_BT_QCA) += btqca.o

drivers/bluetooth/Makefile +2 -0

+obj-$(CONFIG_BT_MTKUART)	+= btmtkuart.o
+

obj-$(CONFIG_BT_HCIUART_NOKIA) += hci_nokia.o

obj-$(CONFIG_BT_HCIRSI) += btrsi.o

drivers/bluetooth/btmtkuart.c new file +35 -0

new file mode 100644
index 0000000..def3d4b
@@ -0,0 +1,591 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 MediaTek Inc.
+
+/*
+ * Bluetooth support for MediaTek serial devices
+ *
+ * Author: Sean Wang <[email protected]>
+ *
+ */
+
+#include <asm/unaligned.h>
+#include <linux/atomic.h>
+#include <linux/clk.h>
+#include <linux/firmware.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm_runtime.h>
+#include <linux/serdev.h>
+#include <linux/skbuff.h>
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include "h4_recv.h"
+
+#define VERSION "0.1"
+
+#define FIRMWARE_MT7622		"mediatek/mt7622pr2h.bin"
+
+#define MTK_STP_TLR_SIZE	2
+
+#define BTMTKUART_TX_STATE_ACTIVE	1
+#define BTMTKUART_TX_STATE_WAKEUP	2
+

drivers/bluetooth/btmtkuart.c new file +15 -0

+} __packed;
+
+struct mtk_wmt_hdr {
+	u8	dir;
+	u8	op;
+	__le16	dlen;
+	u8	flag;
+} __packed;
+
+struct mtk_hci_wmt_cmd {
+	struct mtk_wmt_hdr hdr;
+	u8 data[256];
+} __packed;
+
+struct btmtkuart_dev {

drivers/bluetooth/btmtkuart.c new file +16 -0

+
+	u8	stp_pad[6];
+	u8	stp_cursor;
+	u16	stp_dlen;
+};
+
+static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
+			    const void *param)
+{
+	struct mtk_hci_wmt_cmd wc;
+	struct mtk_wmt_hdr *hdr;
+	struct sk_buff *skb;
+	u32 hlen;
+
+	hlen = sizeof(*hdr) + plen;
+	if (hlen > 255)

drivers/bluetooth/btmtkuart.c new file +16 -0

+
+	skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
+				HCI_INIT_TIMEOUT);
+
+	if (IS_ERR(skb)) {
+		int err = PTR_ERR(skb);
+
+		bt_dev_err(hdev, "Failed to send wmt cmd (%d)", err);
+		return err;
+	}
+
+	kfree_skb(skb);
+
+	return 0;
+}
+

drivers/bluetooth/btmtkuart.c new file +42 -0

+
+	err = request_firmware(&fw, fwname, &hdev->dev);
+	if (err < 0) {
+		bt_dev_err(hdev, "Failed to load firmware file (%d)", err);
+		return err;
+	}
+
+	fw_ptr = fw->data;
+	fw_size = fw->size;
+
+	/* The size of patch header is 30 bytes, should be skip. */
+	if (fw_size < 30)
+		return -EINVAL;
+
+	fw_size -= 30;
+	fw_ptr += 30;
+	flag = 1;
+
+	while (fw_size > 0) {
+		dlen = min_t(int, 250, fw_size);
+
+		/* Tell deivice the position in sequence. */
+		if (fw_size - dlen <= 0)
+			flag = 3;
+		else if (fw_size < fw->size - 30)
+			flag = 2;
+
+		err = mtk_hci_wmt_sync(hdev, MTK_WMT_PATCH_DWNLD, flag, dlen,
+				       fw_ptr);
+		if (err < 0)
+			break;
+
+		fw_size -= dlen;
+		fw_ptr += dlen;
+	}
+
+	release_firmware(fw);
+
+	return err;
+}
+
+static int btmtkuart_recv_event(struct hci_dev *hdev, struct sk_buff *skb)

drivers/bluetooth/btmtkuart.c new file +151 -0

+	return hci_recv_frame(hdev, skb);
+}
+
+static const struct h4_recv_pkt mtk_recv_pkts[] = {
+	{ H4_RECV_ACL,      .recv = hci_recv_frame },
+	{ H4_RECV_SCO,      .recv = hci_recv_frame },
+	{ H4_RECV_EVENT,    .recv = btmtkuart_recv_event },
+};
+
+static const unsigned char *
+mtk_stp_split(struct btmtkuart_dev *bdev, const unsigned char *data, int count,
+	      int *sz_h4)
+{
+	struct mtk_stp_hdr *shdr;
+
+	/* The cursor is reset when all the data of STP is consumed out. */
+	if (!bdev->stp_dlen && bdev->stp_cursor >= 6)
+		bdev->stp_cursor = 0;
+
+	/* Filling pad until all STP info is obtained. */
+	while (bdev->stp_cursor < 6 && count > 0) {
+		bdev->stp_pad[bdev->stp_cursor] = *data;
+		bdev->stp_cursor++;
+		data++;
+		count--;
+	}
+
+	/* Retrieve STP info and have a sanity check. */
+	if (!bdev->stp_dlen && bdev->stp_cursor >= 6) {
+		shdr = (struct mtk_stp_hdr *)&bdev->stp_pad[2];
+		bdev->stp_dlen = shdr->dlen1 << 8 | shdr->dlen2;
+
+		/* Resync STP when unexpected data is being read. */
+		if (shdr->prefix != 0x80 || bdev->stp_dlen > 2048) {
+			bt_dev_err(bdev->hdev, "stp format unexpect (%d, %d)",
+				   shdr->prefix, bdev->stp_dlen);
+			bdev->stp_cursor = 2;
+			bdev->stp_dlen = 0;
+		}
+	}
+
+	/* Directly quit when there's no data found for H4 can process. */
+	if (count <= 0)
+		return NULL;
+
+	/* Tranlate to how much the size of data H4 can handle so far. */
+	*sz_h4 = min_t(int, count, bdev->stp_dlen);
+
+	/* Update the remaining size of STP packet. */
+	bdev->stp_dlen -= *sz_h4;
+
+	/* Data points to STP payload which can be handled by H4. */
+	return data;
+}
+
+static int btmtkuart_recv(struct hci_dev *hdev, const u8 *data, size_t count)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+	const unsigned char *p_left = data, *p_h4;
+	int sz_left = count, sz_h4, adv;
+	int err;
+
+	while (sz_left > 0) {
+		/*  The serial data received from MT7622 BT controller is
+		 *  at all time padded around with the STP header and tailer.
+		 *
+		 *  A full STP packet is looking like
+		 *   -----------------------------------
+		 *  | STP header  |  H:4   | STP tailer |
+		 *   -----------------------------------
+		 *  but it doesn't guarantee to contain a full H:4 packet which
+		 *  means that it's possible for multiple STP packets forms a
+		 *  full H:4 packet that means extra STP header + length doesn't
+		 *  indicate a full H:4 frame, things can fragment. Whose length
+		 *  recorded in STP header just shows up the most length the
+		 *  H:4 engine can handle currently.
+		 */
+
+		p_h4 = mtk_stp_split(bdev, p_left, sz_left, &sz_h4);
+		if (!p_h4)
+			break;
+
+		adv = p_h4 - p_left;
+		sz_left -= adv;
+		p_left += adv;
+
+		bdev->rx_skb = h4_recv_buf(bdev->hdev, bdev->rx_skb, p_h4,
+					   sz_h4, mtk_recv_pkts,
+					   sizeof(mtk_recv_pkts));
+		if (IS_ERR(bdev->rx_skb)) {
+			err = PTR_ERR(bdev->rx_skb);
+			bt_dev_err(bdev->hdev,
+				   "Frame reassembly failed (%d)", err);
+			bdev->rx_skb = NULL;
+			return err;
+		}
+
+		sz_left -= sz_h4;
+		p_left += sz_h4;
+	}
+
+	return 0;
+}
+
+static void btmtkuart_tx_work(struct work_struct *work)
+{
+	struct btmtkuart_dev *bdev = container_of(work, struct btmtkuart_dev,
+						   tx_work);
+	struct serdev_device *serdev = bdev->serdev;
+	struct hci_dev *hdev = bdev->hdev;
+
+	while (1) {
+		clear_bit(BTMTKUART_TX_STATE_WAKEUP, &bdev->tx_state);
+
+		while (1) {
+			struct sk_buff *skb = skb_dequeue(&bdev->txq);
+			int len;
+
+			if (!skb)
+				break;
+
+			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(&bdev->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);
+		}
+
+		if (!test_bit(BTMTKUART_TX_STATE_WAKEUP, &bdev->tx_state))
+			break;
+	}
+
+	clear_bit(BTMTKUART_TX_STATE_ACTIVE, &bdev->tx_state);

drivers/bluetooth/btmtkuart.c new file +46 -0

+static int btmtkuart_receive_buf(struct serdev_device *serdev, const u8 *data,
+				 size_t count)
+{
+	struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
+	int err;
+
+	err = btmtkuart_recv(bdev->hdev, data, count);
+	if (err < 0)
+		return err;
+
+	bdev->hdev->stat.byte_rx += count;
+
+	return count;
+}
+
+static void btmtkuart_write_wakeup(struct serdev_device *serdev)
+{
+	struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
+
+	btmtkuart_tx_wakeup(bdev);
+}
+
+static const struct serdev_device_ops btmtkuart_client_ops = {
+	.receive_buf = btmtkuart_receive_buf,
+	.write_wakeup = btmtkuart_write_wakeup,
+};
+
+static int btmtkuart_open(struct hci_dev *hdev)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+	struct device *dev;
+	int err;
+
+	err = serdev_device_open(bdev->serdev);
+	if (err) {
+		bt_dev_err(hdev, "Unable to open UART device %s",
+			   dev_name(&bdev->serdev->dev));
+		goto err_open;
+	}
+
+	dev = &bdev->serdev->dev;
+
+	bdev->stp_cursor = 2;
+	bdev->stp_dlen = 0;
+
+	/* Enable the power domain and clock the device requires. */

drivers/bluetooth/btmtkuart.c new file +27 -0

+	return 0;
+
+err_put_rpm:
+	pm_runtime_put_sync(dev);
+err_disable_rpm:
+	pm_runtime_disable(dev);
+err_open:
+	return err;
+}
+
+static int btmtkuart_close(struct hci_dev *hdev)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+	struct device *dev = &bdev->serdev->dev;
+
+	/* Shutdown the clock and power domain the device requires. */
+	clk_disable_unprepare(bdev->clk);
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+
+	serdev_device_close(bdev->serdev);
+
+	return 0;
+}
+
+static int btmtkuart_flush(struct hci_dev *hdev)
+{

drivers/bluetooth/btmtkuart.c new file +16 -0

+
+	return 0;
+}
+
+static int btmtkuart_setup(struct hci_dev *hdev)
+{
+	u8 param = 0x1;
+	int err = 0;
+
+	/* Setup a firmware which the device definitely requires. */
+	err = mtk_setup_fw(hdev);
+	if (err < 0)
+		return err;
+
+	/* Activate funciton the firmware providing to. */
+	err = mtk_hci_wmt_sync(hdev, MTK_WMT_RST, 0x4, 0, 0);

drivers/bluetooth/btmtkuart.c new file +33 -0

+
+	return 0;
+}
+
+static int btmtkuart_shutdown(struct hci_dev *hdev)
+{
+	u8 param = 0x0;
+	int err;
+
+	/* Disable the device. */
+	err = mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param),
+			       &param);
+
+	return err;
+}
+
+static int btmtkuart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+	struct mtk_stp_hdr *shdr;
+	struct sk_buff *new_skb;
+	int dlen;
+	u8 *p;
+
+	/* Prepend skb with frame type */
+	memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
+	dlen = skb->len;
+
+	/* Make sure of STP header at least has 4-bytes free space to fill. */
+	if (unlikely(skb_headroom(skb) < sizeof(*shdr))) {
+		new_skb = skb_realloc_headroom(skb, sizeof(*shdr));
+		kfree_skb(skb);
+		skb = new_skb;

drivers/bluetooth/btmtkuart.c new file +78 -0

+	skb_queue_tail(&bdev->txq, skb);
+
+	btmtkuart_tx_wakeup(bdev);
+	return 0;
+}
+
+static int btmtkuart_probe(struct serdev_device *serdev)
+{
+	struct btmtkuart_dev *bdev;
+	struct hci_dev *hdev;
+
+	bdev = devm_kzalloc(&serdev->dev, sizeof(*bdev), GFP_KERNEL);
+	if (!bdev)
+		return -ENOMEM;
+
+	bdev->clk = devm_clk_get(&serdev->dev, "ref");
+	if (IS_ERR(bdev->clk))
+		return PTR_ERR(bdev->clk);
+
+	bdev->serdev = serdev;
+	serdev_device_set_drvdata(serdev, bdev);
+
+	serdev_device_set_client_ops(serdev, &btmtkuart_client_ops);
+
+	INIT_WORK(&bdev->tx_work, btmtkuart_tx_work);
+	skb_queue_head_init(&bdev->txq);
+
+	/* Initialize and register HCI device */
+	hdev = hci_alloc_dev();
+	if (!hdev) {
+		dev_err(&serdev->dev, "Can't allocate HCI device\n");
+		return -ENOMEM;
+	}
+
+	bdev->hdev = hdev;
+
+	hdev->bus = HCI_UART;
+	hci_set_drvdata(hdev, bdev);
+
+	hdev->open  = btmtkuart_open;
+	hdev->close = btmtkuart_close;
+	hdev->flush = btmtkuart_flush;
+	hdev->setup = btmtkuart_setup;
+	hdev->shutdown = btmtkuart_shutdown;
+	hdev->send  = btmtkuart_send_frame;
+	SET_HCIDEV_DEV(hdev, &serdev->dev);
+
+	hdev->manufacturer = 70;
+
+	if (hci_register_dev(hdev) < 0) {
+		dev_err(&serdev->dev, "Can't register HCI device\n");
+		hci_free_dev(hdev);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void btmtkuart_remove(struct serdev_device *serdev)
+{
+	struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
+	struct hci_dev *hdev = bdev->hdev;
+
+	hci_unregister_dev(hdev);
+	hci_free_dev(hdev);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id mtk_of_match_table[] = {
+	{ .compatible = "mediatek,mt7622-bluetooth"},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mtk_of_match_table);
+#endif
+
+static struct serdev_device_driver btmtkuart_driver = {
+	.probe = btmtkuart_probe,
+	.remove = btmtkuart_remove,

Hi Sean,

Quoted 4 lines

This adds a driver based on serdev driver for the MediaTek serial protocol based on running H:4, which can enable the built-in Bluetooth device inside MT7622 SoC.

Quoted 4 lines

drivers/bluetooth/btmtkuart.c new file +4 -0

+enum {
+	MTK_WMT_PATCH_DWNLD = 0x1,
+	MTK_WMT_FUNC_CTRL = 0x6,
+	MTK_WMT_RST = 0x7

drivers/bluetooth/btmtkuart.c line 45

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+};
+
+struct mtk_stp_hdr {
+	u8 prefix;
+	u8 dlen1:4;
+	u8 type:4;

So this is the hard one. I doubt that this is endian safe. It is also some fun way of packing it. Can you find a better variable name and just pack it into an u16 in the function. And then also label this __le16 or __be16 accordingly.

drivers/bluetooth/btmtkuart.c line 47

Quoted 2 lines

drivers/bluetooth/btmtkuart.c new file +2 -0

+	u8 dlen2;
+	u8 cs;

Are you checking the checksum on receive?

Quoted 19 lines

drivers/bluetooth/btmtkuart.c new file +19 -0

+} __packed;
+
+struct mtk_wmt_hdr {
+	u8	dir;
+	u8	op;
+	__le16	dlen;
+	u8	flag;
+} __packed;
+
+struct mtk_hci_wmt_cmd {
+	struct mtk_wmt_hdr hdr;
+	u8 data[256];
+} __packed;
+
+struct btmtkuart_dev {
+	struct hci_dev *hdev;
+	struct serdev_device *serdev;
+
+	struct work_struct tx_work;

drivers/bluetooth/btmtkuart.c line 72

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	unsigned long tx_state;
+	struct sk_buff_head txq;
+
+	struct sk_buff *rx_skb;
+
+	struct mtk_stp_splitter *sp;

This should be a leftover and no longer be needed.

drivers/bluetooth/btmtkuart.c line 73

Quoted 1 line

drivers/bluetooth/btmtkuart.c new file +1 -0

+	struct clk *clk;

Move the struct clk below struct serdev_device.

Quoted 20 lines

drivers/bluetooth/btmtkuart.c new file +20 -0

+
+	u8	stp_pad[6];
+	u8	stp_cursor;
+	u16	stp_dlen;
+};
+
+static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
+			    const void *param)
+{
+	struct mtk_hci_wmt_cmd wc;
+	struct mtk_wmt_hdr *hdr;
+	struct sk_buff *skb;
+	u32 hlen;
+
+	hlen = sizeof(*hdr) + plen;
+	if (hlen > 255)
+		return -EINVAL;
+
+	hdr = (struct mtk_wmt_hdr *)&wc;
+	hdr->dir = 1;

drivers/bluetooth/btmtkuart.c line 99

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	hdr->op = op;
+	hdr->dlen = cpu_to_le16(plen + 1);
+	hdr->flag = flag;
+	memcpy(wc.data, param, plen);
+
+	atomic_inc(&hdev->cmd_cnt);

Why are you doing this one. It will need a comment here if really needed. However I doubt that this is needed. You are only using it from hdev->setup and hdev->shutdown callbacks.

Quoted 20 lines

drivers/bluetooth/btmtkuart.c new file +20 -0

+
+	skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
+				HCI_INIT_TIMEOUT);
+
+	if (IS_ERR(skb)) {
+		int err = PTR_ERR(skb);
+
+		bt_dev_err(hdev, "Failed to send wmt cmd (%d)", err);
+		return err;
+	}
+
+	kfree_skb(skb);
+
+	return 0;
+}
+
+static int mtk_setup_fw(struct hci_dev *hdev)
+{
+	const struct firmware *fw;
+	const char *fwname;

drivers/bluetooth/btmtkuart.c line 125

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	const u8 *fw_ptr;
+	size_t fw_size;
+	int err, dlen;
+	u8 flag;
+
+	fwname = FIRMWARE_MT7622;

Scrap the fwname variable and use it directly. If you later want to support newer/older hardware with other firmware names, we deal with it then.

Quoted 46 lines

drivers/bluetooth/btmtkuart.c new file +46 -0

+
+	err = request_firmware(&fw, fwname, &hdev->dev);
+	if (err < 0) {
+		bt_dev_err(hdev, "Failed to load firmware file (%d)", err);
+		return err;
+	}
+
+	fw_ptr = fw->data;
+	fw_size = fw->size;
+
+	/* The size of patch header is 30 bytes, should be skip. */
+	if (fw_size < 30)
+		return -EINVAL;
+
+	fw_size -= 30;
+	fw_ptr += 30;
+	flag = 1;
+
+	while (fw_size > 0) {
+		dlen = min_t(int, 250, fw_size);
+
+		/* Tell deivice the position in sequence. */
+		if (fw_size - dlen <= 0)
+			flag = 3;
+		else if (fw_size < fw->size - 30)
+			flag = 2;
+
+		err = mtk_hci_wmt_sync(hdev, MTK_WMT_PATCH_DWNLD, flag, dlen,
+				       fw_ptr);
+		if (err < 0)
+			break;
+
+		fw_size -= dlen;
+		fw_ptr += dlen;
+	}
+
+	release_firmware(fw);
+
+	return err;
+}
+
+static int btmtkuart_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct hci_event_hdr *hdr = (void *)skb->data;
+
+	/* Fix up the vendor event id with HCI_VENDOR_PKT instead of

drivers/bluetooth/btmtkuart.c line 177

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	 * 0xe4 so that btmon can parse the kind of vendor event properly.
+	 */
+	if (hdr->evt == 0xe4)
+		hdr->evt = HCI_VENDOR_PKT;
+
+	/* Each HCI event would go through the core. */

This comment adds really no value here. Just remove it.

Quoted 155 lines

drivers/bluetooth/btmtkuart.c new file +155 -0

+	return hci_recv_frame(hdev, skb);
+}
+
+static const struct h4_recv_pkt mtk_recv_pkts[] = {
+	{ H4_RECV_ACL,      .recv = hci_recv_frame },
+	{ H4_RECV_SCO,      .recv = hci_recv_frame },
+	{ H4_RECV_EVENT,    .recv = btmtkuart_recv_event },
+};
+
+static const unsigned char *
+mtk_stp_split(struct btmtkuart_dev *bdev, const unsigned char *data, int count,
+	      int *sz_h4)
+{
+	struct mtk_stp_hdr *shdr;
+
+	/* The cursor is reset when all the data of STP is consumed out. */
+	if (!bdev->stp_dlen && bdev->stp_cursor >= 6)
+		bdev->stp_cursor = 0;
+
+	/* Filling pad until all STP info is obtained. */
+	while (bdev->stp_cursor < 6 && count > 0) {
+		bdev->stp_pad[bdev->stp_cursor] = *data;
+		bdev->stp_cursor++;
+		data++;
+		count--;
+	}
+
+	/* Retrieve STP info and have a sanity check. */
+	if (!bdev->stp_dlen && bdev->stp_cursor >= 6) {
+		shdr = (struct mtk_stp_hdr *)&bdev->stp_pad[2];
+		bdev->stp_dlen = shdr->dlen1 << 8 | shdr->dlen2;
+
+		/* Resync STP when unexpected data is being read. */
+		if (shdr->prefix != 0x80 || bdev->stp_dlen > 2048) {
+			bt_dev_err(bdev->hdev, "stp format unexpect (%d, %d)",
+				   shdr->prefix, bdev->stp_dlen);
+			bdev->stp_cursor = 2;
+			bdev->stp_dlen = 0;
+		}
+	}
+
+	/* Directly quit when there's no data found for H4 can process. */
+	if (count <= 0)
+		return NULL;
+
+	/* Tranlate to how much the size of data H4 can handle so far. */
+	*sz_h4 = min_t(int, count, bdev->stp_dlen);
+
+	/* Update the remaining size of STP packet. */
+	bdev->stp_dlen -= *sz_h4;
+
+	/* Data points to STP payload which can be handled by H4. */
+	return data;
+}
+
+static int btmtkuart_recv(struct hci_dev *hdev, const u8 *data, size_t count)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+	const unsigned char *p_left = data, *p_h4;
+	int sz_left = count, sz_h4, adv;
+	int err;
+
+	while (sz_left > 0) {
+		/*  The serial data received from MT7622 BT controller is
+		 *  at all time padded around with the STP header and tailer.
+		 *
+		 *  A full STP packet is looking like
+		 *   -----------------------------------
+		 *  | STP header  |  H:4   | STP tailer |
+		 *   -----------------------------------
+		 *  but it doesn't guarantee to contain a full H:4 packet which
+		 *  means that it's possible for multiple STP packets forms a
+		 *  full H:4 packet that means extra STP header + length doesn't
+		 *  indicate a full H:4 frame, things can fragment. Whose length
+		 *  recorded in STP header just shows up the most length the
+		 *  H:4 engine can handle currently.
+		 */
+
+		p_h4 = mtk_stp_split(bdev, p_left, sz_left, &sz_h4);
+		if (!p_h4)
+			break;
+
+		adv = p_h4 - p_left;
+		sz_left -= adv;
+		p_left += adv;
+
+		bdev->rx_skb = h4_recv_buf(bdev->hdev, bdev->rx_skb, p_h4,
+					   sz_h4, mtk_recv_pkts,
+					   sizeof(mtk_recv_pkts));
+		if (IS_ERR(bdev->rx_skb)) {
+			err = PTR_ERR(bdev->rx_skb);
+			bt_dev_err(bdev->hdev,
+				   "Frame reassembly failed (%d)", err);
+			bdev->rx_skb = NULL;
+			return err;
+		}
+
+		sz_left -= sz_h4;
+		p_left += sz_h4;
+	}
+
+	return 0;
+}
+
+static void btmtkuart_tx_work(struct work_struct *work)
+{
+	struct btmtkuart_dev *bdev = container_of(work, struct btmtkuart_dev,
+						   tx_work);
+	struct serdev_device *serdev = bdev->serdev;
+	struct hci_dev *hdev = bdev->hdev;
+
+	while (1) {
+		clear_bit(BTMTKUART_TX_STATE_WAKEUP, &bdev->tx_state);
+
+		while (1) {
+			struct sk_buff *skb = skb_dequeue(&bdev->txq);
+			int len;
+
+			if (!skb)
+				break;
+
+			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(&bdev->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);
+		}
+
+		if (!test_bit(BTMTKUART_TX_STATE_WAKEUP, &bdev->tx_state))
+			break;
+	}
+
+	clear_bit(BTMTKUART_TX_STATE_ACTIVE, &bdev->tx_state);
+}
+
+static void btmtkuart_tx_wakeup(struct btmtkuart_dev *bdev)
+{

drivers/bluetooth/btmtkuart.c line 338

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	if (test_and_set_bit(BTMTKUART_TX_STATE_ACTIVE, &bdev->tx_state))
+		set_bit(BTMTKUART_TX_STATE_WAKEUP, &bdev->tx_state);
+
+	schedule_work(&bdev->tx_work);
+}
+

Move btmtkuart_recv and mtk_stp_split above this function to keep them close where they are used.

Quoted 50 lines

drivers/bluetooth/btmtkuart.c new file +50 -0

+static int btmtkuart_receive_buf(struct serdev_device *serdev, const u8 *data,
+				 size_t count)
+{
+	struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
+	int err;
+
+	err = btmtkuart_recv(bdev->hdev, data, count);
+	if (err < 0)
+		return err;
+
+	bdev->hdev->stat.byte_rx += count;
+
+	return count;
+}
+
+static void btmtkuart_write_wakeup(struct serdev_device *serdev)
+{
+	struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
+
+	btmtkuart_tx_wakeup(bdev);
+}
+
+static const struct serdev_device_ops btmtkuart_client_ops = {
+	.receive_buf = btmtkuart_receive_buf,
+	.write_wakeup = btmtkuart_write_wakeup,
+};
+
+static int btmtkuart_open(struct hci_dev *hdev)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+	struct device *dev;
+	int err;
+
+	err = serdev_device_open(bdev->serdev);
+	if (err) {
+		bt_dev_err(hdev, "Unable to open UART device %s",
+			   dev_name(&bdev->serdev->dev));
+		goto err_open;
+	}
+
+	dev = &bdev->serdev->dev;
+
+	bdev->stp_cursor = 2;
+	bdev->stp_dlen = 0;
+
+	/* Enable the power domain and clock the device requires. */
+	pm_runtime_enable(dev);
+	err = pm_runtime_get_sync(dev);
+	if (err < 0) {
+		pm_runtime_put_noidle(dev);

drivers/bluetooth/btmtkuart.c line 394

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+		goto err_disable_rpm;
+	}
+
+	err = clk_prepare_enable(bdev->clk);
+	if (err < 0)
+		goto err_put_rpm;

Add an extra empty line here.

Quoted 31 lines

drivers/bluetooth/btmtkuart.c new file +31 -0

+	return 0;
+
+err_put_rpm:
+	pm_runtime_put_sync(dev);
+err_disable_rpm:
+	pm_runtime_disable(dev);
+err_open:
+	return err;
+}
+
+static int btmtkuart_close(struct hci_dev *hdev)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+	struct device *dev = &bdev->serdev->dev;
+
+	/* Shutdown the clock and power domain the device requires. */
+	clk_disable_unprepare(bdev->clk);
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+
+	serdev_device_close(bdev->serdev);
+
+	return 0;
+}
+
+static int btmtkuart_flush(struct hci_dev *hdev)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+
+	/* Flush any pending characters */
+	serdev_device_write_flush(bdev->serdev);

drivers/bluetooth/btmtkuart.c line 431

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	skb_queue_purge(&bdev->txq);
+
+	cancel_work_sync(&bdev->tx_work);
+
+	kfree_skb(bdev->rx_skb);
+	bdev->rx_skb = NULL;

I would assume you want to reset the stp_cursor here as well.

Quoted 20 lines

drivers/bluetooth/btmtkuart.c new file +20 -0

+
+	return 0;
+}
+
+static int btmtkuart_setup(struct hci_dev *hdev)
+{
+	u8 param = 0x1;
+	int err = 0;
+
+	/* Setup a firmware which the device definitely requires. */
+	err = mtk_setup_fw(hdev);
+	if (err < 0)
+		return err;
+
+	/* Activate funciton the firmware providing to. */
+	err = mtk_hci_wmt_sync(hdev, MTK_WMT_RST, 0x4, 0, 0);
+	if (err < 0)
+		return err;
+
+	/* Enable Bluetooth protocol. */

drivers/bluetooth/btmtkuart.c line 457

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	err = mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param),
+			       &param);
+	if (err < 0)
+		return err;
+
+	set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);

Since you have your own driver. Just move this after the hdev->manufacturer setting in probe(). There is no need to keep setting this over and over again.

Quoted 35 lines

drivers/bluetooth/btmtkuart.c new file +35 -0

+
+	return 0;
+}
+
+static int btmtkuart_shutdown(struct hci_dev *hdev)
+{
+	u8 param = 0x0;
+	int err;
+
+	/* Disable the device. */
+	err = mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param),
+			       &param);
+
+	return err;
+}
+
+static int btmtkuart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+	struct mtk_stp_hdr *shdr;
+	struct sk_buff *new_skb;
+	int dlen;
+	u8 *p;
+
+	/* Prepend skb with frame type */
+	memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
+	dlen = skb->len;
+
+	/* Make sure of STP header at least has 4-bytes free space to fill. */
+	if (unlikely(skb_headroom(skb) < sizeof(*shdr))) {
+		new_skb = skb_realloc_headroom(skb, sizeof(*shdr));
+		kfree_skb(skb);
+		skb = new_skb;
+	}
+

drivers/bluetooth/btmtkuart.c line 500

Quoted 8 lines

drivers/bluetooth/btmtkuart.c new file +8 -0

+	/* Build for STP packet format. */
+	shdr = skb_push(skb, sizeof(*shdr));
+	p = (u8 *)shdr;
+	shdr->prefix = 0x80;
+	shdr->dlen1 = (dlen & 0xf00) >> 8;
+	shdr->type = 0;
+	shdr->dlen2 = dlen & 0xff;
+	shdr->cs = p[0] + p[1] + p[2];

I would add another comment here that this added the STP trailer. And change the above to mention it adds the STP header.

And you might want to check if there is space for the trailer as well. Otherwise skb_put tends to call BUG() if I remember correctly. I know this is super unlikely since our bt_skb_alloc is pretty large.

drivers/bluetooth/btmtkuart.c line 501

Quoted 1 line

drivers/bluetooth/btmtkuart.c new file +1 -0

+	skb_put_zero(skb, MTK_STP_TLR_SIZE);

Extra empty line here please.

Quoted 82 lines

drivers/bluetooth/btmtkuart.c new file +82 -0

+	skb_queue_tail(&bdev->txq, skb);
+
+	btmtkuart_tx_wakeup(bdev);
+	return 0;
+}
+
+static int btmtkuart_probe(struct serdev_device *serdev)
+{
+	struct btmtkuart_dev *bdev;
+	struct hci_dev *hdev;
+
+	bdev = devm_kzalloc(&serdev->dev, sizeof(*bdev), GFP_KERNEL);
+	if (!bdev)
+		return -ENOMEM;
+
+	bdev->clk = devm_clk_get(&serdev->dev, "ref");
+	if (IS_ERR(bdev->clk))
+		return PTR_ERR(bdev->clk);
+
+	bdev->serdev = serdev;
+	serdev_device_set_drvdata(serdev, bdev);
+
+	serdev_device_set_client_ops(serdev, &btmtkuart_client_ops);
+
+	INIT_WORK(&bdev->tx_work, btmtkuart_tx_work);
+	skb_queue_head_init(&bdev->txq);
+
+	/* Initialize and register HCI device */
+	hdev = hci_alloc_dev();
+	if (!hdev) {
+		dev_err(&serdev->dev, "Can't allocate HCI device\n");
+		return -ENOMEM;
+	}
+
+	bdev->hdev = hdev;
+
+	hdev->bus = HCI_UART;
+	hci_set_drvdata(hdev, bdev);
+
+	hdev->open  = btmtkuart_open;
+	hdev->close = btmtkuart_close;
+	hdev->flush = btmtkuart_flush;
+	hdev->setup = btmtkuart_setup;
+	hdev->shutdown = btmtkuart_shutdown;
+	hdev->send  = btmtkuart_send_frame;
+	SET_HCIDEV_DEV(hdev, &serdev->dev);
+
+	hdev->manufacturer = 70;
+
+	if (hci_register_dev(hdev) < 0) {
+		dev_err(&serdev->dev, "Can't register HCI device\n");
+		hci_free_dev(hdev);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void btmtkuart_remove(struct serdev_device *serdev)
+{
+	struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
+	struct hci_dev *hdev = bdev->hdev;
+
+	hci_unregister_dev(hdev);
+	hci_free_dev(hdev);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id mtk_of_match_table[] = {
+	{ .compatible = "mediatek,mt7622-bluetooth"},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mtk_of_match_table);
+#endif
+
+static struct serdev_device_driver btmtkuart_driver = {
+	.probe = btmtkuart_probe,
+	.remove = btmtkuart_remove,
+	.driver = {
+		.name = "btmtkuart",
+		.of_match_table = of_match_ptr(mtk_of_match_table),
+	},

drivers/bluetooth/btmtkuart.c line 589

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+};
+
+module_serdev_device_driver(btmtkuart_driver);
+
+MODULE_AUTHOR("Sean Wang <[email protected]>");
+MODULE_DESCRIPTION("MediaTek Bluetooth Serial driver" VERSION);

You are missing a ? ver ? at the end of your string here. Check with modinfo that it looks correct.

Review

Quoted 2 lines

(quoted diff) +2 -0

+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL?);

You want to add a MODULE_FIRMWARE here as well.

Regards

Marcel

Quoted 4 lines

Quoted 4 lines

This adds a driver based on serdev driver for the MediaTek serial protocol based on running H:4, which can enable the built-in Bluetooth device inside MT7622 SoC.

[ ... ]

Quoted 4 lines

Quoted 4 lines

drivers/bluetooth/btmtkuart.c new file +4 -0

+enum {
+	MTK_WMT_PATCH_DWNLD = 0x1,
+	MTK_WMT_FUNC_CTRL = 0x6,
+	MTK_WMT_RST = 0x7

Quoted 8 lines

drivers/bluetooth/btmtkuart.c line 45

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+};
+
+struct mtk_stp_hdr {
+	u8 prefix;
+	u8 dlen1:4;
+	u8 type:4;

So this is the hard one. I doubt that this is endian safe. It is also some fun way of packing it. Can you find a better variable name and just pack it into an u16 in the function. And then also label this __le16 or __be16 accordingly.

okay, I will do it. here I suppose 'u8 dlen1:4 and u8 type:4' only take up one byte.

Quoted 5 lines

drivers/bluetooth/btmtkuart.c line 47

Quoted 2 lines

drivers/bluetooth/btmtkuart.c new file +2 -0

+	u8 dlen2;
+	u8 cs;

Are you checking the checksum on receive?

it is no needs. cs always shows zeros when I dump these received packets.

Quoted 19 lines

Quoted 19 lines

drivers/bluetooth/btmtkuart.c new file +19 -0

+} __packed;
+
+struct mtk_wmt_hdr {
+	u8	dir;
+	u8	op;
+	__le16	dlen;
+	u8	flag;
+} __packed;
+
+struct mtk_hci_wmt_cmd {
+	struct mtk_wmt_hdr hdr;
+	u8 data[256];
+} __packed;
+
+struct btmtkuart_dev {
+	struct hci_dev *hdev;
+	struct serdev_device *serdev;
+
+	struct work_struct tx_work;

Quoted 9 lines

drivers/bluetooth/btmtkuart.c line 72

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	unsigned long tx_state;
+	struct sk_buff_head txq;
+
+	struct sk_buff *rx_skb;
+
+	struct mtk_stp_splitter *sp;

This should be a leftover and no longer be needed.

okay. it's my fault and I should have a removal in the version

Quoted 4 lines

drivers/bluetooth/btmtkuart.c line 73

Quoted 1 line

drivers/bluetooth/btmtkuart.c new file +1 -0

+	struct clk *clk;

Move the struct clk below struct serdev_device.

okay, it is a nice arrangement

Quoted 20 lines

Quoted 20 lines

drivers/bluetooth/btmtkuart.c new file +20 -0

+
+	u8	stp_pad[6];
+	u8	stp_cursor;
+	u16	stp_dlen;
+};
+
+static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
+			    const void *param)
+{
+	struct mtk_hci_wmt_cmd wc;
+	struct mtk_wmt_hdr *hdr;
+	struct sk_buff *skb;
+	u32 hlen;
+
+	hlen = sizeof(*hdr) + plen;
+	if (hlen > 255)
+		return -EINVAL;
+
+	hdr = (struct mtk_wmt_hdr *)&wc;
+	hdr->dir = 1;

Quoted 9 lines

drivers/bluetooth/btmtkuart.c line 99

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	hdr->op = op;
+	hdr->dlen = cpu_to_le16(plen + 1);
+	hdr->flag = flag;
+	memcpy(wc.data, param, plen);
+
+	atomic_inc(&hdev->cmd_cnt);

Why are you doing this one. It will need a comment here if really needed. However I doubt that this is needed. You are only using it from hdev->setup and hdev->shutdown callbacks.

An increment on cmd_cnt is really needed because hci_cmd_work would check whether cmd_cnt is positive and then has a decrement on cmd_cnt before a packet is being sent out.

okay will add a comment.

Quoted 20 lines

Quoted 20 lines

drivers/bluetooth/btmtkuart.c new file +20 -0

+
+	skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
+				HCI_INIT_TIMEOUT);
+
+	if (IS_ERR(skb)) {
+		int err = PTR_ERR(skb);
+
+		bt_dev_err(hdev, "Failed to send wmt cmd (%d)", err);
+		return err;
+	}
+
+	kfree_skb(skb);
+
+	return 0;
+}
+
+static int mtk_setup_fw(struct hci_dev *hdev)
+{
+	const struct firmware *fw;
+	const char *fwname;

Quoted 9 lines

drivers/bluetooth/btmtkuart.c line 125

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	const u8 *fw_ptr;
+	size_t fw_size;
+	int err, dlen;
+	u8 flag;
+
+	fwname = FIRMWARE_MT7622;

Scrap the fwname variable and use it directly. If you later want to support newer/older hardware with other firmware names, we deal with it then.

okay

Quoted 46 lines

Quoted 46 lines

drivers/bluetooth/btmtkuart.c new file +46 -0

+
+	err = request_firmware(&fw, fwname, &hdev->dev);
+	if (err < 0) {
+		bt_dev_err(hdev, "Failed to load firmware file (%d)", err);
+		return err;
+	}
+
+	fw_ptr = fw->data;
+	fw_size = fw->size;
+
+	/* The size of patch header is 30 bytes, should be skip. */
+	if (fw_size < 30)
+		return -EINVAL;
+
+	fw_size -= 30;
+	fw_ptr += 30;
+	flag = 1;
+
+	while (fw_size > 0) {
+		dlen = min_t(int, 250, fw_size);
+
+		/* Tell deivice the position in sequence. */
+		if (fw_size - dlen <= 0)
+			flag = 3;
+		else if (fw_size < fw->size - 30)
+			flag = 2;
+
+		err = mtk_hci_wmt_sync(hdev, MTK_WMT_PATCH_DWNLD, flag, dlen,
+				       fw_ptr);
+		if (err < 0)
+			break;
+
+		fw_size -= dlen;
+		fw_ptr += dlen;
+	}
+
+	release_firmware(fw);
+
+	return err;
+}
+
+static int btmtkuart_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct hci_event_hdr *hdr = (void *)skb->data;
+
+	/* Fix up the vendor event id with HCI_VENDOR_PKT instead of

Quoted 9 lines

drivers/bluetooth/btmtkuart.c line 177

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	 * 0xe4 so that btmon can parse the kind of vendor event properly.
+	 */
+	if (hdr->evt == 0xe4)
+		hdr->evt = HCI_VENDOR_PKT;
+
+	/* Each HCI event would go through the core. */

This comment adds really no value here. Just remove it.

okay

Quoted 155 lines

Quoted 155 lines

drivers/bluetooth/btmtkuart.c new file +155 -0

+	return hci_recv_frame(hdev, skb);
+}
+
+static const struct h4_recv_pkt mtk_recv_pkts[] = {
+	{ H4_RECV_ACL,      .recv = hci_recv_frame },
+	{ H4_RECV_SCO,      .recv = hci_recv_frame },
+	{ H4_RECV_EVENT,    .recv = btmtkuart_recv_event },
+};
+
+static const unsigned char *
+mtk_stp_split(struct btmtkuart_dev *bdev, const unsigned char *data, int count,
+	      int *sz_h4)
+{
+	struct mtk_stp_hdr *shdr;
+
+	/* The cursor is reset when all the data of STP is consumed out. */
+	if (!bdev->stp_dlen && bdev->stp_cursor >= 6)
+		bdev->stp_cursor = 0;
+
+	/* Filling pad until all STP info is obtained. */
+	while (bdev->stp_cursor < 6 && count > 0) {
+		bdev->stp_pad[bdev->stp_cursor] = *data;
+		bdev->stp_cursor++;
+		data++;
+		count--;
+	}
+
+	/* Retrieve STP info and have a sanity check. */
+	if (!bdev->stp_dlen && bdev->stp_cursor >= 6) {
+		shdr = (struct mtk_stp_hdr *)&bdev->stp_pad[2];
+		bdev->stp_dlen = shdr->dlen1 << 8 | shdr->dlen2;
+
+		/* Resync STP when unexpected data is being read. */
+		if (shdr->prefix != 0x80 || bdev->stp_dlen > 2048) {
+			bt_dev_err(bdev->hdev, "stp format unexpect (%d, %d)",
+				   shdr->prefix, bdev->stp_dlen);
+			bdev->stp_cursor = 2;
+			bdev->stp_dlen = 0;
+		}
+	}
+
+	/* Directly quit when there's no data found for H4 can process. */
+	if (count <= 0)
+		return NULL;
+
+	/* Tranlate to how much the size of data H4 can handle so far. */
+	*sz_h4 = min_t(int, count, bdev->stp_dlen);
+
+	/* Update the remaining size of STP packet. */
+	bdev->stp_dlen -= *sz_h4;
+
+	/* Data points to STP payload which can be handled by H4. */
+	return data;
+}
+
+static int btmtkuart_recv(struct hci_dev *hdev, const u8 *data, size_t count)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+	const unsigned char *p_left = data, *p_h4;
+	int sz_left = count, sz_h4, adv;
+	int err;
+
+	while (sz_left > 0) {
+		/*  The serial data received from MT7622 BT controller is
+		 *  at all time padded around with the STP header and tailer.
+		 *
+		 *  A full STP packet is looking like
+		 *   -----------------------------------
+		 *  | STP header  |  H:4   | STP tailer |
+		 *   -----------------------------------
+		 *  but it doesn't guarantee to contain a full H:4 packet which
+		 *  means that it's possible for multiple STP packets forms a
+		 *  full H:4 packet that means extra STP header + length doesn't
+		 *  indicate a full H:4 frame, things can fragment. Whose length
+		 *  recorded in STP header just shows up the most length the
+		 *  H:4 engine can handle currently.
+		 */
+
+		p_h4 = mtk_stp_split(bdev, p_left, sz_left, &sz_h4);
+		if (!p_h4)
+			break;
+
+		adv = p_h4 - p_left;
+		sz_left -= adv;
+		p_left += adv;
+
+		bdev->rx_skb = h4_recv_buf(bdev->hdev, bdev->rx_skb, p_h4,
+					   sz_h4, mtk_recv_pkts,
+					   sizeof(mtk_recv_pkts));
+		if (IS_ERR(bdev->rx_skb)) {
+			err = PTR_ERR(bdev->rx_skb);
+			bt_dev_err(bdev->hdev,
+				   "Frame reassembly failed (%d)", err);
+			bdev->rx_skb = NULL;
+			return err;
+		}
+
+		sz_left -= sz_h4;
+		p_left += sz_h4;
+	}
+
+	return 0;
+}
+
+static void btmtkuart_tx_work(struct work_struct *work)
+{
+	struct btmtkuart_dev *bdev = container_of(work, struct btmtkuart_dev,
+						   tx_work);
+	struct serdev_device *serdev = bdev->serdev;
+	struct hci_dev *hdev = bdev->hdev;
+
+	while (1) {
+		clear_bit(BTMTKUART_TX_STATE_WAKEUP, &bdev->tx_state);
+
+		while (1) {
+			struct sk_buff *skb = skb_dequeue(&bdev->txq);
+			int len;
+
+			if (!skb)
+				break;
+
+			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(&bdev->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);
+		}
+
+		if (!test_bit(BTMTKUART_TX_STATE_WAKEUP, &bdev->tx_state))
+			break;
+	}
+
+	clear_bit(BTMTKUART_TX_STATE_ACTIVE, &bdev->tx_state);
+}
+
+static void btmtkuart_tx_wakeup(struct btmtkuart_dev *bdev)
+{

Quoted 9 lines

drivers/bluetooth/btmtkuart.c line 338

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	if (test_and_set_bit(BTMTKUART_TX_STATE_ACTIVE, &bdev->tx_state))
+		set_bit(BTMTKUART_TX_STATE_WAKEUP, &bdev->tx_state);
+
+	schedule_work(&bdev->tx_work);
+}
+

Move btmtkuart_recv and mtk_stp_split above this function to keep them close where they are used.

okay

Quoted 50 lines

Quoted 50 lines

drivers/bluetooth/btmtkuart.c new file +50 -0

+static int btmtkuart_receive_buf(struct serdev_device *serdev, const u8 *data,
+				 size_t count)
+{
+	struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
+	int err;
+
+	err = btmtkuart_recv(bdev->hdev, data, count);
+	if (err < 0)
+		return err;
+
+	bdev->hdev->stat.byte_rx += count;
+
+	return count;
+}
+
+static void btmtkuart_write_wakeup(struct serdev_device *serdev)
+{
+	struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
+
+	btmtkuart_tx_wakeup(bdev);
+}
+
+static const struct serdev_device_ops btmtkuart_client_ops = {
+	.receive_buf = btmtkuart_receive_buf,
+	.write_wakeup = btmtkuart_write_wakeup,
+};
+
+static int btmtkuart_open(struct hci_dev *hdev)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+	struct device *dev;
+	int err;
+
+	err = serdev_device_open(bdev->serdev);
+	if (err) {
+		bt_dev_err(hdev, "Unable to open UART device %s",
+			   dev_name(&bdev->serdev->dev));
+		goto err_open;
+	}
+
+	dev = &bdev->serdev->dev;
+
+	bdev->stp_cursor = 2;
+	bdev->stp_dlen = 0;
+
+	/* Enable the power domain and clock the device requires. */
+	pm_runtime_enable(dev);
+	err = pm_runtime_get_sync(dev);
+	if (err < 0) {
+		pm_runtime_put_noidle(dev);

Quoted 9 lines

drivers/bluetooth/btmtkuart.c line 394

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+		goto err_disable_rpm;
+	}
+
+	err = clk_prepare_enable(bdev->clk);
+	if (err < 0)
+		goto err_put_rpm;

Add an extra empty line here.

okay

Quoted 31 lines

Quoted 31 lines

drivers/bluetooth/btmtkuart.c new file +31 -0

+	return 0;
+
+err_put_rpm:
+	pm_runtime_put_sync(dev);
+err_disable_rpm:
+	pm_runtime_disable(dev);
+err_open:
+	return err;
+}
+
+static int btmtkuart_close(struct hci_dev *hdev)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+	struct device *dev = &bdev->serdev->dev;
+
+	/* Shutdown the clock and power domain the device requires. */
+	clk_disable_unprepare(bdev->clk);
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+
+	serdev_device_close(bdev->serdev);
+
+	return 0;
+}
+
+static int btmtkuart_flush(struct hci_dev *hdev)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+
+	/* Flush any pending characters */
+	serdev_device_write_flush(bdev->serdev);

Quoted 9 lines

drivers/bluetooth/btmtkuart.c line 431

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	skb_queue_purge(&bdev->txq);
+
+	cancel_work_sync(&bdev->tx_work);
+
+	kfree_skb(bdev->rx_skb);
+	bdev->rx_skb = NULL;

I would assume you want to reset the stp_cursor here as well.

yes, it can be and is better

Quoted 20 lines

Quoted 20 lines

drivers/bluetooth/btmtkuart.c new file +20 -0

+
+	return 0;
+}
+
+static int btmtkuart_setup(struct hci_dev *hdev)
+{
+	u8 param = 0x1;
+	int err = 0;
+
+	/* Setup a firmware which the device definitely requires. */
+	err = mtk_setup_fw(hdev);
+	if (err < 0)
+		return err;
+
+	/* Activate funciton the firmware providing to. */
+	err = mtk_hci_wmt_sync(hdev, MTK_WMT_RST, 0x4, 0, 0);
+	if (err < 0)
+		return err;
+
+	/* Enable Bluetooth protocol. */

Quoted 9 lines

drivers/bluetooth/btmtkuart.c line 457

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+	err = mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param),
+			       &param);
+	if (err < 0)
+		return err;
+
+	set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);

Since you have your own driver. Just move this after the hdev->manufacturer setting in probe(). There is no need to keep setting this over and over again.

okay

Quoted 35 lines

Quoted 35 lines

drivers/bluetooth/btmtkuart.c new file +35 -0

+
+	return 0;
+}
+
+static int btmtkuart_shutdown(struct hci_dev *hdev)
+{
+	u8 param = 0x0;
+	int err;
+
+	/* Disable the device. */
+	err = mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param),
+			       &param);
+
+	return err;
+}
+
+static int btmtkuart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
+	struct mtk_stp_hdr *shdr;
+	struct sk_buff *new_skb;
+	int dlen;
+	u8 *p;
+
+	/* Prepend skb with frame type */
+	memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
+	dlen = skb->len;
+
+	/* Make sure of STP header at least has 4-bytes free space to fill. */
+	if (unlikely(skb_headroom(skb) < sizeof(*shdr))) {
+		new_skb = skb_realloc_headroom(skb, sizeof(*shdr));
+		kfree_skb(skb);
+		skb = new_skb;
+	}
+

Quoted 9 lines

drivers/bluetooth/btmtkuart.c line 500

Quoted 8 lines

drivers/bluetooth/btmtkuart.c new file +8 -0

+	/* Build for STP packet format. */
+	shdr = skb_push(skb, sizeof(*shdr));
+	p = (u8 *)shdr;
+	shdr->prefix = 0x80;
+	shdr->dlen1 = (dlen & 0xf00) >> 8;
+	shdr->type = 0;
+	shdr->dlen2 = dlen & 0xff;
+	shdr->cs = p[0] + p[1] + p[2];

as above discussion about shr->cs , it can be filled with zero to have less computing

Quoted 3 lines

I would add another comment here that this added the STP trailer. And change the above to mention it adds the STP header.

sure

Quoted 2 lines

And you might want to check if there is space for the trailer as well. Otherwise skb_put tends to call BUG() if I remember correctly. I know this is super unlikely since our bt_skb_alloc is pretty large.

sure, I will add the handling for that. it should be better to make sure all rooms are enough for header and trailer before adding content to them

Quoted 4 lines

drivers/bluetooth/btmtkuart.c line 501

Quoted 1 line

drivers/bluetooth/btmtkuart.c new file +1 -0

+	skb_put_zero(skb, MTK_STP_TLR_SIZE);

Extra empty line here please.

okay

Quoted 82 lines

Quoted 82 lines

drivers/bluetooth/btmtkuart.c new file +82 -0

+	skb_queue_tail(&bdev->txq, skb);
+
+	btmtkuart_tx_wakeup(bdev);
+	return 0;
+}
+
+static int btmtkuart_probe(struct serdev_device *serdev)
+{
+	struct btmtkuart_dev *bdev;
+	struct hci_dev *hdev;
+
+	bdev = devm_kzalloc(&serdev->dev, sizeof(*bdev), GFP_KERNEL);
+	if (!bdev)
+		return -ENOMEM;
+
+	bdev->clk = devm_clk_get(&serdev->dev, "ref");
+	if (IS_ERR(bdev->clk))
+		return PTR_ERR(bdev->clk);
+
+	bdev->serdev = serdev;
+	serdev_device_set_drvdata(serdev, bdev);
+
+	serdev_device_set_client_ops(serdev, &btmtkuart_client_ops);
+
+	INIT_WORK(&bdev->tx_work, btmtkuart_tx_work);
+	skb_queue_head_init(&bdev->txq);
+
+	/* Initialize and register HCI device */
+	hdev = hci_alloc_dev();
+	if (!hdev) {
+		dev_err(&serdev->dev, "Can't allocate HCI device\n");
+		return -ENOMEM;
+	}
+
+	bdev->hdev = hdev;
+
+	hdev->bus = HCI_UART;
+	hci_set_drvdata(hdev, bdev);
+
+	hdev->open  = btmtkuart_open;
+	hdev->close = btmtkuart_close;
+	hdev->flush = btmtkuart_flush;
+	hdev->setup = btmtkuart_setup;
+	hdev->shutdown = btmtkuart_shutdown;
+	hdev->send  = btmtkuart_send_frame;
+	SET_HCIDEV_DEV(hdev, &serdev->dev);
+
+	hdev->manufacturer = 70;
+
+	if (hci_register_dev(hdev) < 0) {
+		dev_err(&serdev->dev, "Can't register HCI device\n");
+		hci_free_dev(hdev);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void btmtkuart_remove(struct serdev_device *serdev)
+{
+	struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
+	struct hci_dev *hdev = bdev->hdev;
+
+	hci_unregister_dev(hdev);
+	hci_free_dev(hdev);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id mtk_of_match_table[] = {
+	{ .compatible = "mediatek,mt7622-bluetooth"},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mtk_of_match_table);
+#endif
+
+static struct serdev_device_driver btmtkuart_driver = {
+	.probe = btmtkuart_probe,
+	.remove = btmtkuart_remove,
+	.driver = {
+		.name = "btmtkuart",
+		.of_match_table = of_match_ptr(mtk_of_match_table),
+	},

Quoted 9 lines

drivers/bluetooth/btmtkuart.c line 589

Quoted 6 lines

drivers/bluetooth/btmtkuart.c new file +6 -0

+};
+
+module_serdev_device_driver(btmtkuart_driver);
+
+MODULE_AUTHOR("Sean Wang <[email protected]>");
+MODULE_DESCRIPTION("MediaTek Bluetooth Serial driver" VERSION);

You are missing a ? ver ? at the end of your string here. Check with modinfo that it looks correct.

okay

Quoted 5 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL?);

You want to add a MODULE_FIRMWARE here as well.

okay

Hi Sean,

Quoted 25 lines

Quoted 24 lines

Quoted 11 lines

drivers/bluetooth/btmtkuart.c new file +11 -0

+
+static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
+			    const void *param)
+{
+	struct mtk_hci_wmt_cmd wc;
+	struct mtk_wmt_hdr *hdr;
+	struct sk_buff *skb;
+	u32 hlen;
+
+	hlen = sizeof(*hdr) + plen;
+	if (hlen > 255)

drivers/bluetooth/btmtkuart.c line 99

Quoted 10 lines

drivers/bluetooth/btmtkuart.c new file +10 -0

+		return -EINVAL;
+
+	hdr = (struct mtk_wmt_hdr *)&wc;
+	hdr->dir = 1;
+	hdr->op = op;
+	hdr->dlen = cpu_to_le16(plen + 1);
+	hdr->flag = flag;
+	memcpy(wc.data, param, plen);
+
+	atomic_inc(&hdev->cmd_cnt);

Why are you doing this one. It will need a comment here if really needed. However I doubt that this is needed. You are only using it from hdev->setup and hdev->shutdown callbacks.

Quoted 3 lines

An increment on cmd_cnt is really needed because hci_cmd_work would check whether cmd_cnt is positive and then has a decrement on cmd_cnt before a packet is being sent out.

okay will add a comment.

but you are in ->setup callback this time. So if you need this, then all the other ->setup routines would actually fail as well. Either this is leftover from when you did things in ->probe or ->open or this is some thing we might better fix properly in the core instead of papering over it. Can you recheck if this is really needed.

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

drivers/bluetooth/btmtkuart.c new file +16 -0

+
+	skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
+				HCI_INIT_TIMEOUT);
+
+	if (IS_ERR(skb)) {
+		int err = PTR_ERR(skb);
+
+		bt_dev_err(hdev, "Failed to send wmt cmd (%d)", err);
+		return err;
+	}
+
+	kfree_skb(skb);
+
+	return 0;
+}
+

Quoted 5 lines

Quoted 3 lines

drivers/bluetooth/btmtkuart.c line 500

Quoted 2 lines

drivers/bluetooth/btmtkuart.c new file +2 -0

+	shdr->dlen2 = dlen & 0xff;
+	shdr->cs = p[0] + p[1] + p[2];

as above discussion about shr->cs , it can be filled with zero to have less computing

If it has no value, then zero it out and add a comment for it.

Quoted 12 lines

Quoted 3 lines

I would add another comment here that this added the STP trailer. And change the above to mention it adds the STP header.

sure

Quoted 2 lines

And you might want to check if there is space for the trailer as well. Otherwise skb_put tends to call BUG() if I remember correctly. I know this is super unlikely since our bt_skb_alloc is pretty large.

sure, I will add the handling for that. it should be better to make sure all rooms are enough for header and trailer before adding content to them

Quoted 4 lines

Quoted 2 lines

You want to add a MODULE_FIRMWARE here as well.

okay

Regards

Marcel

Quoted 25 lines

Quoted 25 lines

Quoted 24 lines

Quoted 11 lines

drivers/bluetooth/btmtkuart.c new file +11 -0

+
+static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
+			    const void *param)
+{
+	struct mtk_hci_wmt_cmd wc;
+	struct mtk_wmt_hdr *hdr;
+	struct sk_buff *skb;
+	u32 hlen;
+
+	hlen = sizeof(*hdr) + plen;
+	if (hlen > 255)

drivers/bluetooth/btmtkuart.c line 99

Quoted 10 lines

drivers/bluetooth/btmtkuart.c new file +10 -0

+		return -EINVAL;
+
+	hdr = (struct mtk_wmt_hdr *)&wc;
+	hdr->dir = 1;
+	hdr->op = op;
+	hdr->dlen = cpu_to_le16(plen + 1);
+	hdr->flag = flag;
+	memcpy(wc.data, param, plen);
+
+	atomic_inc(&hdev->cmd_cnt);

Why are you doing this one. It will need a comment here if really needed. However I doubt that this is needed. You are only using it from hdev->setup and hdev->shutdown callbacks.

Quoted 6 lines

Quoted 3 lines

An increment on cmd_cnt is really needed because hci_cmd_work would check whether cmd_cnt is positive and then has a decrement on cmd_cnt before a packet is being sent out.

okay will add a comment.

but you are in ->setup callback this time. So if you need this, then all the other ->setup routines would actually fail as well. Either this is leftover from when you did things in ->probe or ->open or this is some thing we might better fix properly in the core instead of papering over it. Can you recheck if this is really needed.

I added a counter print and the counter increments as below

	/* atomic_inc(&hdev->cmd_cnt); */
       pr_info("cmd_cnt = %d\n" , atomic_read(&hdev->cmd_cnt));

       skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
                               HCI_INIT_TIMEOUT);

and the log show up that

[  334.049156] Bluetooth: hci0: command 0xfc6f tx timeout
[  334.054840] cmd_cnt = 0
[  336.065076] Bluetooth: hci0: command 0xfc6f tx timeout
[  336.070795] cmd_cnt = 0
[  338.080997] Bluetooth: hci0: command 0xfc6f tx timeout
[  338.086683] cmd_cnt = 0
[  340.096907] Bluetooth: hci0: command 0xfc6f tx timeout
[  340.102609] cmd_cnt = 0
[  342.112824] Bluetooth: hci0: command 0xfc6f tx timeout
[  342.118520] cmd_cnt = 0
[  344.128747] Bluetooth: hci0: command 0xfc6f tx timeout
[  344.134454] cmd_cnt = 0
[  346.144667] Bluetooth: hci0: command 0xfc6f tx timeout
[  346.150372] cmd_cnt = 0

The packet is dropped by hci_cmd_work at [1], so I also wondered why the other vendor driver works, it seems the counter needs to be incremented before every skb is being queued to cmd_q.

4257 static void hci_cmd_work(struct work_struct *work) 4258 {
4259 struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_work);
4260 struct sk_buff *skb;
4261
4262 BT_DBG("%s cmd_cnt %d cmd queued %d", hdev->name,
4263 atomic_read(&hdev->cmd_cnt), skb_queue_len(&hdev->cmd_q));
4264
4265 /* Send queued commands */

[1]
4266 if (atomic_read(&hdev->cmd_cnt)) { /* dropped when cmd_cnt is zero */
4267 skb = skb_dequeue(&hdev->cmd_q);
4268 if (!skb)
4269 return;
4270
4271 kfree_skb(hdev->sent_cmd);
4272
4273 hdev->sent_cmd = skb_clone(skb, GFP_KERNEL);
4274 if (hdev->sent_cmd) {
4275 atomic_dec(&hdev->cmd_cnt); /* cmd_cnt-- */
4276 hci_send_frame(hdev, skb);

Hi Sean,

Quoted 73 lines

Quoted 25 lines

Quoted 25 lines

Quoted 24 lines

Quoted 11 lines

drivers/bluetooth/btmtkuart.c new file +11 -0

+
+static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
+			    const void *param)
+{
+	struct mtk_hci_wmt_cmd wc;
+	struct mtk_wmt_hdr *hdr;
+	struct sk_buff *skb;
+	u32 hlen;
+
+	hlen = sizeof(*hdr) + plen;
+	if (hlen > 255)

drivers/bluetooth/btmtkuart.c line 99

Quoted 10 lines

drivers/bluetooth/btmtkuart.c new file +10 -0

+		return -EINVAL;
+
+	hdr = (struct mtk_wmt_hdr *)&wc;
+	hdr->dir = 1;
+	hdr->op = op;
+	hdr->dlen = cpu_to_le16(plen + 1);
+	hdr->flag = flag;
+	memcpy(wc.data, param, plen);
+
+	atomic_inc(&hdev->cmd_cnt);

Why are you doing this one. It will need a comment here if really needed. However I doubt that this is needed. You are only using it from hdev->setup and hdev->shutdown callbacks.

Quoted 6 lines

Quoted 3 lines

An increment on cmd_cnt is really needed because hci_cmd_work would check whether cmd_cnt is positive and then has a decrement on cmd_cnt before a packet is being sent out.

okay will add a comment.

but you are in ->setup callback this time. So if you need this, then all the other ->setup routines would actually fail as well. Either this is leftover from when you did things in ->probe or ->open or this is some thing we might better fix properly in the core instead of papering over it. Can you recheck if this is really needed.

I added a counter print and the counter increments as below

	/* atomic_inc(&hdev->cmd_cnt); */
       pr_info("cmd_cnt = %d\n" , atomic_read(&hdev->cmd_cnt));

       skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
                               HCI_INIT_TIMEOUT);

and the log show up that

[  334.049156] Bluetooth: hci0: command 0xfc6f tx timeout
[  334.054840] cmd_cnt = 0
[  336.065076] Bluetooth: hci0: command 0xfc6f tx timeout
[  336.070795] cmd_cnt = 0
[  338.080997] Bluetooth: hci0: command 0xfc6f tx timeout
[  338.086683] cmd_cnt = 0
[  340.096907] Bluetooth: hci0: command 0xfc6f tx timeout
[  340.102609] cmd_cnt = 0
[  342.112824] Bluetooth: hci0: command 0xfc6f tx timeout
[  342.118520] cmd_cnt = 0
[  344.128747] Bluetooth: hci0: command 0xfc6f tx timeout
[  344.134454] cmd_cnt = 0
[  346.144667] Bluetooth: hci0: command 0xfc6f tx timeout
[  346.150372] cmd_cnt = 0

The packet is dropped by hci_cmd_work at [1], so I also wondered why the other vendor driver works, it seems the counter needs to be incremented before every skb is being queued to cmd_q.

4257 static void hci_cmd_work(struct work_struct *work) 4258 {
4259 struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_work);
4260 struct sk_buff *skb;
4261
4262 BT_DBG("%s cmd_cnt %d cmd queued %d", hdev->name,
4263 atomic_read(&hdev->cmd_cnt), skb_queue_len(&hdev->cmd_q));
4264
4265 /* Send queued commands */

Quoted 12 lines

[1]
4266 if (atomic_read(&hdev->cmd_cnt)) { /* dropped when cmd_cnt is zero */
4267 skb = skb_dequeue(&hdev->cmd_q);
4268 if (!skb)
4269 return;
4270
4271 kfree_skb(hdev->sent_cmd);
4272
4273 hdev->sent_cmd = skb_clone(skb, GFP_KERNEL);
4274 if (hdev->sent_cmd) {
4275 atomic_dec(&hdev->cmd_cnt); /* cmd_cnt-- */
4276 hci_send_frame(hdev, skb);

actually the command also needs to better go via the raw_q anyway since it doesn?t come back with the cmd status or cmd complete. You have it waiting for a vendor event. Maybe with is something we need to consider with __hci_cmd_sync_ev anyway.

Johan would know best since he wrote that code. Anyway, we should fix that in the core and not have you hack around it.

Regards

Marcel

Quoted 86 lines

Quoted 85 lines

Quoted 25 lines

Quoted 25 lines

Quoted 24 lines

Quoted 11 lines

(quoted diff) +11 -0

+
+static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
+			    const void *param)
+{
+	struct mtk_hci_wmt_cmd wc;
+	struct mtk_wmt_hdr *hdr;
+	struct sk_buff *skb;
+	u32 hlen;
+
+	hlen = sizeof(*hdr) + plen;
+	if (hlen > 255)

Review

Quoted 10 lines

(quoted diff) +10 -0

+		return -EINVAL;
+
+	hdr = (struct mtk_wmt_hdr *)&wc;
+	hdr->dir = 1;
+	hdr->op = op;
+	hdr->dlen = cpu_to_le16(plen + 1);
+	hdr->flag = flag;
+	memcpy(wc.data, param, plen);
+
+	atomic_inc(&hdev->cmd_cnt);

Why are you doing this one. It will need a comment here if really needed. However I doubt that this is needed. You are only using it from hdev->setup and hdev->shutdown callbacks.

Quoted 6 lines

Quoted 3 lines

An increment on cmd_cnt is really needed because hci_cmd_work would check whether cmd_cnt is positive and then has a decrement on cmd_cnt before a packet is being sent out.

okay will add a comment.

but you are in ->setup callback this time. So if you need this, then all the other ->setup routines would actually fail as well. Either this is leftover from when you did things in ->probe or ->open or this is some thing we might better fix properly in the core instead of papering over it. Can you recheck if this is really needed.

I added a counter print and the counter increments as below

	/* atomic_inc(&hdev->cmd_cnt); */
      pr_info("cmd_cnt = %d\n" , atomic_read(&hdev->cmd_cnt));

      skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
                              HCI_INIT_TIMEOUT);

and the log show up that

[  334.049156] Bluetooth: hci0: command 0xfc6f tx timeout
[  334.054840] cmd_cnt = 0
[  336.065076] Bluetooth: hci0: command 0xfc6f tx timeout
[  336.070795] cmd_cnt = 0
[  338.080997] Bluetooth: hci0: command 0xfc6f tx timeout
[  338.086683] cmd_cnt = 0
[  340.096907] Bluetooth: hci0: command 0xfc6f tx timeout
[  340.102609] cmd_cnt = 0
[  342.112824] Bluetooth: hci0: command 0xfc6f tx timeout
[  342.118520] cmd_cnt = 0
[  344.128747] Bluetooth: hci0: command 0xfc6f tx timeout
[  344.134454] cmd_cnt = 0
[  346.144667] Bluetooth: hci0: command 0xfc6f tx timeout
[  346.150372] cmd_cnt = 0

The packet is dropped by hci_cmd_work at [1], so I also wondered why the other vendor driver works, it seems the counter needs to be incremented before every skb is being queued to cmd_q.

4257 static void hci_cmd_work(struct work_struct *work) 4258 {
4259 struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_work);
4260 struct sk_buff *skb;
4261
4262 BT_DBG("%s cmd_cnt %d cmd queued %d", hdev->name,
4263 atomic_read(&hdev->cmd_cnt), skb_queue_len(&hdev->cmd_q));
4264
4265 /* Send queued commands */

[1]
4266 if (atomic_read(&hdev->cmd_cnt)) { /* dropped when cmd_cnt is zero */
4267 skb = skb_dequeue(&hdev->cmd_q);
4268 if (!skb)
4269 return;
4270
4271 kfree_skb(hdev->sent_cmd);
4272
4273 hdev->sent_cmd = skb_clone(skb, GFP_KERNEL);
4274 if (hdev->sent_cmd) {
4275 atomic_dec(&hdev->cmd_cnt); /* cmd_cnt-- */
4276 hci_send_frame(hdev, skb);

Quoted 4 lines

actually the command also needs to better go via the raw_q anyway since it doesn?t come back with the cmd status or cmd complete. You have it waiting for a vendor event. Maybe with is something we need to consider with __hci_cmd_sync_ev anyway.

Johan would know best since he wrote that code. Anyway, we should fix that in the core and not have you hack around it.

yes, my case is that received event is neither cmd status nor cmd complete. It is completely a vendor event.

if it wants to be solved by the core layer, do you permit that I remove the hack and then send it in the next version?

Hi Sean,

Quoted 91 lines

Quoted 90 lines

Quoted 73 lines

Quoted 25 lines

Quoted 25 lines

Quoted 24 lines

Quoted 11 lines

(quoted diff) +11 -0

+
+static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
+			    const void *param)
+{
+	struct mtk_hci_wmt_cmd wc;
+	struct mtk_wmt_hdr *hdr;
+	struct sk_buff *skb;
+	u32 hlen;
+
+	hlen = sizeof(*hdr) + plen;
+	if (hlen > 255)

Review

Quoted 10 lines

(quoted diff) +10 -0

+		return -EINVAL;
+
+	hdr = (struct mtk_wmt_hdr *)&wc;
+	hdr->dir = 1;
+	hdr->op = op;
+	hdr->dlen = cpu_to_le16(plen + 1);
+	hdr->flag = flag;
+	memcpy(wc.data, param, plen);
+
+	atomic_inc(&hdev->cmd_cnt);

Why are you doing this one. It will need a comment here if really needed. However I doubt that this is needed. You are only using it from hdev->setup and hdev->shutdown callbacks.

Quoted 6 lines

Quoted 3 lines

An increment on cmd_cnt is really needed because hci_cmd_work would check whether cmd_cnt is positive and then has a decrement on cmd_cnt before a packet is being sent out.

okay will add a comment.

but you are in ->setup callback this time. So if you need this, then all the other ->setup routines would actually fail as well. Either this is leftover from when you did things in ->probe or ->open or this is some thing we might better fix properly in the core instead of papering over it. Can you recheck if this is really needed.

I added a counter print and the counter increments as below

	/* atomic_inc(&hdev->cmd_cnt); */
      pr_info("cmd_cnt = %d\n" , atomic_read(&hdev->cmd_cnt));

      skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
                              HCI_INIT_TIMEOUT);

and the log show up that

[  334.049156] Bluetooth: hci0: command 0xfc6f tx timeout
[  334.054840] cmd_cnt = 0
[  336.065076] Bluetooth: hci0: command 0xfc6f tx timeout
[  336.070795] cmd_cnt = 0
[  338.080997] Bluetooth: hci0: command 0xfc6f tx timeout
[  338.086683] cmd_cnt = 0
[  340.096907] Bluetooth: hci0: command 0xfc6f tx timeout
[  340.102609] cmd_cnt = 0
[  342.112824] Bluetooth: hci0: command 0xfc6f tx timeout
[  342.118520] cmd_cnt = 0
[  344.128747] Bluetooth: hci0: command 0xfc6f tx timeout
[  344.134454] cmd_cnt = 0
[  346.144667] Bluetooth: hci0: command 0xfc6f tx timeout
[  346.150372] cmd_cnt = 0

The packet is dropped by hci_cmd_work at [1], so I also wondered why the other vendor driver works, it seems the counter needs to be incremented before every skb is being queued to cmd_q.

4257 static void hci_cmd_work(struct work_struct *work) 4258 {
4259 struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_work);
4260 struct sk_buff *skb;
4261
4262 BT_DBG("%s cmd_cnt %d cmd queued %d", hdev->name,
4263 atomic_read(&hdev->cmd_cnt), skb_queue_len(&hdev->cmd_q));
4264
4265 /* Send queued commands */

Quoted 12 lines

[1]
4266 if (atomic_read(&hdev->cmd_cnt)) { /* dropped when cmd_cnt is zero */
4267 skb = skb_dequeue(&hdev->cmd_q);
4268 if (!skb)
4269 return;
4270
4271 kfree_skb(hdev->sent_cmd);
4272
4273 hdev->sent_cmd = skb_clone(skb, GFP_KERNEL);
4274 if (hdev->sent_cmd) {
4275 atomic_dec(&hdev->cmd_cnt); /* cmd_cnt-- */
4276 hci_send_frame(hdev, skb);

actually the command also needs to better go via the raw_q anyway since it doesn?t come back with the cmd status or cmd complete. You have it waiting for a vendor event. Maybe with is something we need to consider with __hci_cmd_sync_ev anyway.

Johan would know best since he wrote that code. Anyway, we should fix that in the core and not have you hack around it.

Quoted 3 lines

yes, my case is that received event is neither cmd status nor cmd complete. It is completely a vendor event.

if it wants to be solved by the core layer, do you permit that I remove the hack and then send it in the next version?

we need to have a __hci_raw_sync_ev that uses the hdev->raw_q and waits for the specified event to come back. I never realized that you are missing the cmd status or cmd complete. So this is similar to the original CSR vendor commands which had the same behavior.

I have the feeling that you hdev->cmd_cnt increment is just hiding the problem here. If you really think that it is not chains any side effects we can merge the driver with a big warning and fix this up. However the clean way would be for you to create a patch that introduces __hci_raw_sync_ev as describe above.

Regards

Marcel

Quoted 91 lines

Quoted 91 lines

Quoted 90 lines

Quoted 73 lines

Quoted 25 lines

Quoted 25 lines

Quoted 24 lines

Quoted 11 lines

(quoted diff) +11 -0

+
+static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
+			    const void *param)
+{
+	struct mtk_hci_wmt_cmd wc;
+	struct mtk_wmt_hdr *hdr;
+	struct sk_buff *skb;
+	u32 hlen;
+
+	hlen = sizeof(*hdr) + plen;
+	if (hlen > 255)

Review

Quoted 10 lines

(quoted diff) +10 -0

+		return -EINVAL;
+
+	hdr = (struct mtk_wmt_hdr *)&wc;
+	hdr->dir = 1;
+	hdr->op = op;
+	hdr->dlen = cpu_to_le16(plen + 1);
+	hdr->flag = flag;
+	memcpy(wc.data, param, plen);
+
+	atomic_inc(&hdev->cmd_cnt);

Why are you doing this one. It will need a comment here if really needed. However I doubt that this is needed. You are only using it from hdev->setup and hdev->shutdown callbacks.

Quoted 6 lines

Quoted 3 lines

An increment on cmd_cnt is really needed because hci_cmd_work would check whether cmd_cnt is positive and then has a decrement on cmd_cnt before a packet is being sent out.

okay will add a comment.

but you are in ->setup callback this time. So if you need this, then all the other ->setup routines would actually fail as well. Either this is leftover from when you did things in ->probe or ->open or this is some thing we might better fix properly in the core instead of papering over it. Can you recheck if this is really needed.

I added a counter print and the counter increments as below

	/* atomic_inc(&hdev->cmd_cnt); */
     pr_info("cmd_cnt = %d\n" , atomic_read(&hdev->cmd_cnt));

     skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
                             HCI_INIT_TIMEOUT);

and the log show up that

[  334.049156] Bluetooth: hci0: command 0xfc6f tx timeout
[  334.054840] cmd_cnt = 0
[  336.065076] Bluetooth: hci0: command 0xfc6f tx timeout
[  336.070795] cmd_cnt = 0
[  338.080997] Bluetooth: hci0: command 0xfc6f tx timeout
[  338.086683] cmd_cnt = 0
[  340.096907] Bluetooth: hci0: command 0xfc6f tx timeout
[  340.102609] cmd_cnt = 0
[  342.112824] Bluetooth: hci0: command 0xfc6f tx timeout
[  342.118520] cmd_cnt = 0
[  344.128747] Bluetooth: hci0: command 0xfc6f tx timeout
[  344.134454] cmd_cnt = 0
[  346.144667] Bluetooth: hci0: command 0xfc6f tx timeout
[  346.150372] cmd_cnt = 0

The packet is dropped by hci_cmd_work at [1], so I also wondered why the other vendor driver works, it seems the counter needs to be incremented before every skb is being queued to cmd_q.

4257 static void hci_cmd_work(struct work_struct *work) 4258 {
4259 struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_work);
4260 struct sk_buff *skb;
4261
4262 BT_DBG("%s cmd_cnt %d cmd queued %d", hdev->name,
4263 atomic_read(&hdev->cmd_cnt), skb_queue_len(&hdev->cmd_q));
4264
4265 /* Send queued commands */

Quoted 12 lines

[1]
4266 if (atomic_read(&hdev->cmd_cnt)) { /* dropped when cmd_cnt is zero */
4267 skb = skb_dequeue(&hdev->cmd_q);
4268 if (!skb)
4269 return;
4270
4271 kfree_skb(hdev->sent_cmd);
4272
4273 hdev->sent_cmd = skb_clone(skb, GFP_KERNEL);
4274 if (hdev->sent_cmd) {
4275 atomic_dec(&hdev->cmd_cnt); /* cmd_cnt-- */
4276 hci_send_frame(hdev, skb);

actually the command also needs to better go via the raw_q anyway since it doesn?t come back with the cmd status or cmd complete. You have it waiting for a vendor event. Maybe with is something we need to consider with __hci_cmd_sync_ev anyway.

Johan would know best since he wrote that code. Anyway, we should fix that in the core and not have you hack around it.

Quoted 7 lines

Quoted 3 lines

yes, my case is that received event is neither cmd status nor cmd complete. It is completely a vendor event.

if it wants to be solved by the core layer, do you permit that I remove the hack and then send it in the next version?

we need to have a __hci_raw_sync_ev that uses the hdev->raw_q and waits for the specified event to come back. I never realized that you are missing the cmd status or cmd complete. So this is similar to the original CSR vendor commands which had the same behavior.

I have the feeling that you hdev->cmd_cnt increment is just hiding the problem here. If you really think that it is not chains any side effects we can merge the driver with a big warning and fix this up. However the clean way would be for you to create a patch that introduces __hci_raw_sync_ev as describe above.

What do you think of this? If I add extra atomic_set 1 on cmd_cnt after driver really got a vendor event back instead of blinding to increment for every packet sent.

the behavior is the same to receive a cmd status or complete. it should not have side effects.

96 skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
97 HCI_INIT_TIMEOUT);
98
99 if (IS_ERR(skb)) {
100 int err = PTR_ERR(skb);
101
102 bt_dev_err(hdev, "Failed to send wmt cmd (%d)", err);
103 return err;
104 }
105
106 if (!test_bit(HCI_RESET, &hdev->flags)) <<<<<<
107 atomic_set(&hdev->cmd_cnt, 1); <<<<<<
108
109 kfree_skb(skb);

Hi Sean,

Quoted 103 lines

Quoted 91 lines

Quoted 91 lines

Quoted 90 lines

Quoted 73 lines

Quoted 25 lines

Quoted 25 lines

Quoted 24 lines

Quoted 11 lines

(quoted diff) +11 -0

+
+static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
+			    const void *param)
+{
+	struct mtk_hci_wmt_cmd wc;
+	struct mtk_wmt_hdr *hdr;
+	struct sk_buff *skb;
+	u32 hlen;
+
+	hlen = sizeof(*hdr) + plen;
+	if (hlen > 255)

Review

Quoted 10 lines

(quoted diff) +10 -0

+		return -EINVAL;
+
+	hdr = (struct mtk_wmt_hdr *)&wc;
+	hdr->dir = 1;
+	hdr->op = op;
+	hdr->dlen = cpu_to_le16(plen + 1);
+	hdr->flag = flag;
+	memcpy(wc.data, param, plen);
+
+	atomic_inc(&hdev->cmd_cnt);

Why are you doing this one. It will need a comment here if really needed. However I doubt that this is needed. You are only using it from hdev->setup and hdev->shutdown callbacks.

Quoted 6 lines

Quoted 3 lines

An increment on cmd_cnt is really needed because hci_cmd_work would check whether cmd_cnt is positive and then has a decrement on cmd_cnt before a packet is being sent out.

okay will add a comment.

but you are in ->setup callback this time. So if you need this, then all the other ->setup routines would actually fail as well. Either this is leftover from when you did things in ->probe or ->open or this is some thing we might better fix properly in the core instead of papering over it. Can you recheck if this is really needed.

I added a counter print and the counter increments as below

	/* atomic_inc(&hdev->cmd_cnt); */
     pr_info("cmd_cnt = %d\n" , atomic_read(&hdev->cmd_cnt));

     skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
                             HCI_INIT_TIMEOUT);

and the log show up that

[  334.049156] Bluetooth: hci0: command 0xfc6f tx timeout
[  334.054840] cmd_cnt = 0
[  336.065076] Bluetooth: hci0: command 0xfc6f tx timeout
[  336.070795] cmd_cnt = 0
[  338.080997] Bluetooth: hci0: command 0xfc6f tx timeout
[  338.086683] cmd_cnt = 0
[  340.096907] Bluetooth: hci0: command 0xfc6f tx timeout
[  340.102609] cmd_cnt = 0
[  342.112824] Bluetooth: hci0: command 0xfc6f tx timeout
[  342.118520] cmd_cnt = 0
[  344.128747] Bluetooth: hci0: command 0xfc6f tx timeout
[  344.134454] cmd_cnt = 0
[  346.144667] Bluetooth: hci0: command 0xfc6f tx timeout
[  346.150372] cmd_cnt = 0

The packet is dropped by hci_cmd_work at [1], so I also wondered why the other vendor driver works, it seems the counter needs to be incremented before every skb is being queued to cmd_q.

4257 static void hci_cmd_work(struct work_struct *work) 4258 {
4259 struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_work);
4260 struct sk_buff *skb;
4261
4262 BT_DBG("%s cmd_cnt %d cmd queued %d", hdev->name,
4263 atomic_read(&hdev->cmd_cnt), skb_queue_len(&hdev->cmd_q));
4264
4265 /* Send queued commands */

Quoted 12 lines

[1]
4266 if (atomic_read(&hdev->cmd_cnt)) { /* dropped when cmd_cnt is zero */
4267 skb = skb_dequeue(&hdev->cmd_q);
4268 if (!skb)
4269 return;
4270
4271 kfree_skb(hdev->sent_cmd);
4272
4273 hdev->sent_cmd = skb_clone(skb, GFP_KERNEL);
4274 if (hdev->sent_cmd) {
4275 atomic_dec(&hdev->cmd_cnt); /* cmd_cnt-- */
4276 hci_send_frame(hdev, skb);

actually the command also needs to better go via the raw_q anyway since it doesn?t come back with the cmd status or cmd complete. You have it waiting for a vendor event. Maybe with is something we need to consider with __hci_cmd_sync_ev anyway.

Johan would know best since he wrote that code. Anyway, we should fix that in the core and not have you hack around it.

Quoted 7 lines

Quoted 3 lines

yes, my case is that received event is neither cmd status nor cmd complete. It is completely a vendor event.

if it wants to be solved by the core layer, do you permit that I remove the hack and then send it in the next version?

we need to have a __hci_raw_sync_ev that uses the hdev->raw_q and waits for the specified event to come back. I never realized that you are missing the cmd status or cmd complete. So this is similar to the original CSR vendor commands which had the same behavior.

I have the feeling that you hdev->cmd_cnt increment is just hiding the problem here. If you really think that it is not chains any side effects we can merge the driver with a big warning and fix this up. However the clean way would be for you to create a patch that introduces __hci_raw_sync_ev as describe above.

What do you think of this? If I add extra atomic_set 1 on cmd_cnt after driver really got a vendor event back instead of blinding to increment for every packet sent.

the behavior is the same to receive a cmd status or complete. it should not have side effects.

Quoted 14 lines

96 skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
97 HCI_INIT_TIMEOUT);
98
99 if (IS_ERR(skb)) {
100 int err = PTR_ERR(skb);
101
102 bt_dev_err(hdev, "Failed to send wmt cmd (%d)", err);
103 return err;
104 }
105
106 if (!test_bit(HCI_RESET, &hdev->flags)) <<<<<<
107 atomic_set(&hdev->cmd_cnt, 1); <<<<<<
108
109 kfree_skb(skb);

this is even more hackish since the __hci_cmd_sync_ev command is really meant to get a cmd status first before waiting for that event.

Are all Mediatek vendor commands this way? Or just the ones for loading the firmware? So only the WMT ones?

Regards

Marcel

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Quoted 118 lines

Quoted 117 lines

Quoted 91 lines

Quoted 91 lines

Quoted 90 lines

Quoted 73 lines

Quoted 25 lines

Quoted 25 lines

Quoted 24 lines

Quoted 11 lines

(quoted diff) +11 -0

+
+static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
+			    const void *param)
+{
+	struct mtk_hci_wmt_cmd wc;
+	struct mtk_wmt_hdr *hdr;
+	struct sk_buff *skb;
+	u32 hlen;
+
+	hlen = sizeof(*hdr) + plen;
+	if (hlen > 255)

Review

Quoted 10 lines

(quoted diff) +10 -0

+		return -EINVAL;
+
+	hdr = (struct mtk_wmt_hdr *)&wc;
+	hdr->dir = 1;
+	hdr->op = op;
+	hdr->dlen = cpu_to_le16(plen + 1);
+	hdr->flag = flag;
+	memcpy(wc.data, param, plen);
+
+	atomic_inc(&hdev->cmd_cnt);

Why are you doing this one. It will need a comment here if really needed. However I doubt that this is needed. You are only using it from hdev->setup and hdev->shutdown callbacks.

Quoted 6 lines

Quoted 3 lines

An increment on cmd_cnt is really needed because hci_cmd_work would check whether cmd_cnt is positive and then has a decrement on cmd_cnt before a packet is being sent out.

okay will add a comment.

but you are in ->setup callback this time. So if you need this, then all the other ->setup routines would actually fail as well. Either this is leftover from when you did things in ->probe or ->open or this is some thing we might better fix properly in the core instead of papering over it. Can you recheck if this is really needed.

I added a counter print and the counter increments as below

	/* atomic_inc(&hdev->cmd_cnt); */
    pr_info("cmd_cnt = %d\n" , atomic_read(&hdev->cmd_cnt));

    skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
                            HCI_INIT_TIMEOUT);

and the log show up that

[  334.049156] Bluetooth: hci0: command 0xfc6f tx timeout
[  334.054840] cmd_cnt = 0
[  336.065076] Bluetooth: hci0: command 0xfc6f tx timeout
[  336.070795] cmd_cnt = 0
[  338.080997] Bluetooth: hci0: command 0xfc6f tx timeout
[  338.086683] cmd_cnt = 0
[  340.096907] Bluetooth: hci0: command 0xfc6f tx timeout
[  340.102609] cmd_cnt = 0
[  342.112824] Bluetooth: hci0: command 0xfc6f tx timeout
[  342.118520] cmd_cnt = 0
[  344.128747] Bluetooth: hci0: command 0xfc6f tx timeout
[  344.134454] cmd_cnt = 0
[  346.144667] Bluetooth: hci0: command 0xfc6f tx timeout
[  346.150372] cmd_cnt = 0

The packet is dropped by hci_cmd_work at [1], so I also wondered why the other vendor driver works, it seems the counter needs to be incremented before every skb is being queued to cmd_q.

4257 static void hci_cmd_work(struct work_struct *work) 4258 {
4259 struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_work);
4260 struct sk_buff *skb;
4261
4262 BT_DBG("%s cmd_cnt %d cmd queued %d", hdev->name,
4263 atomic_read(&hdev->cmd_cnt), skb_queue_len(&hdev->cmd_q));
4264
4265 /* Send queued commands */

Quoted 12 lines

[1]
4266 if (atomic_read(&hdev->cmd_cnt)) { /* dropped when cmd_cnt is zero */
4267 skb = skb_dequeue(&hdev->cmd_q);
4268 if (!skb)
4269 return;
4270
4271 kfree_skb(hdev->sent_cmd);
4272
4273 hdev->sent_cmd = skb_clone(skb, GFP_KERNEL);
4274 if (hdev->sent_cmd) {
4275 atomic_dec(&hdev->cmd_cnt); /* cmd_cnt-- */
4276 hci_send_frame(hdev, skb);

actually the command also needs to better go via the raw_q anyway since it doesn?t come back with the cmd status or cmd complete. You have it waiting for a vendor event. Maybe with is something we need to consider with __hci_cmd_sync_ev anyway.

Johan would know best since he wrote that code. Anyway, we should fix that in the core and not have you hack around it.

Quoted 7 lines

Quoted 3 lines

yes, my case is that received event is neither cmd status nor cmd complete. It is completely a vendor event.

if it wants to be solved by the core layer, do you permit that I remove the hack and then send it in the next version?

we need to have a __hci_raw_sync_ev that uses the hdev->raw_q and waits for the specified event to come back. I never realized that you are missing the cmd status or cmd complete. So this is similar to the original CSR vendor commands which had the same behavior.

I have the feeling that you hdev->cmd_cnt increment is just hiding the problem here. If you really think that it is not chains any side effects we can merge the driver with a big warning and fix this up. However the clean way would be for you to create a patch that introduces __hci_raw_sync_ev as describe above.

What do you think of this? If I add extra atomic_set 1 on cmd_cnt after driver really got a vendor event back instead of blinding to increment for every packet sent.

the behavior is the same to receive a cmd status or complete. it should not have side effects.

96 skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
97 HCI_INIT_TIMEOUT);
98
99 if (IS_ERR(skb)) {
100 int err = PTR_ERR(skb);
101
102 bt_dev_err(hdev, "Failed to send wmt cmd (%d)", err);
103 return err;
104 }
105
106 if (!test_bit(HCI_RESET, &hdev->flags)) <<<<<<
107 atomic_set(&hdev->cmd_cnt, 1); <<<<<<
108
109 kfree_skb(skb);

Quoted 2 lines

this is even more hackish since the __hci_cmd_sync_ev command is really meant to get a cmd status first before waiting for that event.

Understood.

I've stopped the hack in v8. could we merge v8 first ? and then I will a fix up with __hci_raw_sync_ev that uses the hdev->raw_q instead of __hci_cmd_sync_ev in TODO.

Quoted 2 lines

Are all Mediatek vendor commands this way? Or just the ones for loading the firmware? So only the WMT ones?

Only the WMT ones, WMT commands/events are usually used in system controlling, for example, global function on/off, firmware download, reset and so on. most only appear on device initialization

Hi Sean,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+

Quoted 6 lines

Quoted 2 lines

this is even more hackish since the __hci_cmd_sync_ev command is really meant to get a cmd status first before waiting for that event.

Understood.

I've stopped the hack in v8. could we merge v8 first ? and then I will a fix up with __hci_raw_sync_ev that uses the hdev->raw_q instead of __hci_cmd_sync_ev in TODO.

so I looked into this a bit more. We actually added __hci_cmd_send for a Qualcomm firmware loader that was doing something similar. So instead of trying to add a yet another command to the core, I actually used that and implemented the wait for vendor event in the driver.

You will see my v9 on the mailing list. I also did a bunch of cosmetic minor cleanup and spelling correction. Please test this version. I also make __le16 dlen instead of dlen1 + dlen2 since I think that is what your hardware does.

If this version of the driver works for you then I am happy to merge it. You can then add support for hdev->set_bdaddr and hdev->set_diag in later patches. I also like to clean up the STP receive handler since it can be done a lot simpler and smaller, but that has to wait.

Quoted 4 lines

Quoted 2 lines

Are all Mediatek vendor commands this way? Or just the ones for loading the firmware? So only the WMT ones?

Only the WMT ones, WMT commands/events are usually used in system controlling, for example, global function on/off, firmware download, reset and so on. most only appear on device initialization

Since you never checked the result of the vendor event, I opted for just signaling that it arrived. If they can report success or failure, we need to add some extra code for that.

Regards

Marcel

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

this is even more hackish since the __hci_cmd_sync_ev command is really meant to get a cmd status first before waiting for that event.

Understood.

I've stopped the hack in v8. could we merge v8 first ? and then I will a fix up with __hci_raw_sync_ev that uses the hdev->raw_q instead of __hci_cmd_sync_ev in TODO.

so I looked into this a bit more. We actually added __hci_cmd_send for a Qualcomm firmware loader that was doing something similar. So instead of trying to add a yet another command to the core, I actually used that and implemented the wait for vendor event in the driver.

Quoted 1 line

You will see my v9 on the mailing list. I also did a bunch of cosmetic minor cleanup and spelling correction. Please test this version. I also make __le16 dlen instead of dlen1 + dlen2 since I think that is what your hardware does.

Only one thing needs to be corrected in v9. that is __be16 is required instead of dlen1 + dlen2. I will fix it up in v10 and the other changes all look good to me.

Quoted 2 lines

If this version of the driver works for you then I am happy to merge it. You can then add support for hdev->set_bdaddr and hdev->set_diag in later patches. I also like to clean up the STP receive handler since it can be done a lot simpler and smaller, but that has to wait.

hopefully v10 also can be merged :)

I will investigate more about how to add ->set_bdaddr, ->set_diag and STP receive enhancement in later patches.

but so far I have not much idea about how to make STP multiplexer be a independent driver.

my thought is that it would be really better and cleaner a chain of serdev is be used as the base of mtkbtuart. something like

8250 serial bus <----> STP multiplexer serdev <----> mtkbtuart serdev

however, STP multiplexer serdev is not a real device, that doesn't no request any resource. I think it should not be allowed to be added in a device tree and even in dt-binding document.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

Are all Mediatek vendor commands this way? Or just the ones for loading the firmware? So only the WMT ones?

Only the WMT ones, WMT commands/events are usually used in system controlling, for example, global function on/off, firmware download, reset and so on. most only appear on device initialization

Since you never checked the result of the vendor event, I opted for just signaling that it arrived. If they can report success or failure, we need to add some extra code for that.

I will consider more WMT event status when I add more Bluetooth devices such as MT7668U usb based Bluetooth which I plan to add the support in later patches in the next weeks

Hi Sean,

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

this is even more hackish since the __hci_cmd_sync_ev command is really meant to get a cmd status first before waiting for that event.

Understood.

I've stopped the hack in v8. could we merge v8 first ? and then I will a fix up with __hci_raw_sync_ev that uses the hdev->raw_q instead of __hci_cmd_sync_ev in TODO.

so I looked into this a bit more. We actually added __hci_cmd_send for a Qualcomm firmware loader that was doing something similar. So instead of trying to add a yet another command to the core, I actually used that and implemented the wait for vendor event in the driver.

Quoted 8 lines

Quoted 1 line

You will see my v9 on the mailing list. I also did a bunch of cosmetic minor cleanup and spelling correction. Please test this version. I also make __le16 dlen instead of dlen1 + dlen2 since I think that is what your hardware does.

Only one thing needs to be corrected in v9. that is __be16 is required instead of dlen1 + dlen2. I will fix it up in v10 and the other changes all look good to me.

Quoted 2 lines

If this version of the driver works for you then I am happy to merge it. You can then add support for hdev->set_bdaddr and hdev->set_diag in later patches. I also like to clean up the STP receive handler since it can be done a lot simpler and smaller, but that has to wait.

hopefully v10 also can be merged :)

send me a v10 and I can merge it.

Quoted 9 lines

I will investigate more about how to add ->set_bdaddr, ->set_diag and STP receive enhancement in later patches.

but so far I have not much idea about how to make STP multiplexer be a independent driver.

my thought is that it would be really better and cleaner a chain of serdev is be used as the base of mtkbtuart. something like

8250 serial bus <----> STP multiplexer serdev <----> mtkbtuart serdev

however, STP multiplexer serdev is not a real device, that doesn't no request any resource. I think it should not be allowed to be added in a device tree and even in dt-binding document.

Before we do that, lets get a cleaner parser for it. I just don?t have enough time to wrap my head around this one yet.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

Are all Mediatek vendor commands this way? Or just the ones for loading the firmware? So only the WMT ones?

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Only the WMT ones, WMT commands/events are usually used in system controlling, for example, global function on/off, firmware download, reset and so on. most only appear on device initialization

Since you never checked the result of the vendor event, I opted for just signaling that it arrived. If they can report success or failure, we need to add some extra code for that.

I will consider more WMT event status when I add more Bluetooth devices such as MT7668U usb based Bluetooth which I plan to add the support in later patches in the next weeks

Are the USB ones also using STP or are they H:2 based like all the others. What are prominent MT7668U based ones that I could buy?

Regards

Marcel

Quoted 35 lines

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

Quoted 2 lines

this is even more hackish since the __hci_cmd_sync_ev command is really meant to get a cmd status first before waiting for that event.

Understood.

I've stopped the hack in v8. could we merge v8 first ? and then I will a fix up with __hci_raw_sync_ev that uses the hdev->raw_q instead of __hci_cmd_sync_ev in TODO.

so I looked into this a bit more. We actually added __hci_cmd_send for a Qualcomm firmware loader that was doing something similar. So instead of trying to add a yet another command to the core, I actually used that and implemented the wait for vendor event in the driver.

Quoted 8 lines

Quoted 1 line

You will see my v9 on the mailing list. I also did a bunch of cosmetic minor cleanup and spelling correction. Please test this version. I also make __le16 dlen instead of dlen1 + dlen2 since I think that is what your hardware does.

Only one thing needs to be corrected in v9. that is __be16 is required instead of dlen1 + dlen2. I will fix it up in v10 and the other changes all look good to me.

Quoted 2 lines

If this version of the driver works for you then I am happy to merge it. You can then add support for hdev->set_bdaddr and hdev->set_diag in later patches. I also like to clean up the STP receive handler since it can be done a lot simpler and smaller, but that has to wait.

hopefully v10 also can be merged :)

send me a v10 and I can merge it.

Quoted 9 lines

I will investigate more about how to add ->set_bdaddr, ->set_diag and STP receive enhancement in later patches.

but so far I have not much idea about how to make STP multiplexer be a independent driver.

my thought is that it would be really better and cleaner a chain of serdev is be used as the base of mtkbtuart. something like

8250 serial bus <----> STP multiplexer serdev <----> mtkbtuart serdev

however, STP multiplexer serdev is not a real device, that doesn't no request any resource. I think it should not be allowed to be added in a device tree and even in dt-binding document.

Before we do that, lets get a cleaner parser for it. I just don?t have enough time to wrap my head around this one yet.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

Are all Mediatek vendor commands this way? Or just the ones for loading the firmware? So only the WMT ones?

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Only the WMT ones, WMT commands/events are usually used in system controlling, for example, global function on/off, firmware download, reset and so on. most only appear on device initialization

Since you never checked the result of the vendor event, I opted for just signaling that it arrived. If they can report success or failure, we need to add some extra code for that.

I will consider more WMT event status when I add more Bluetooth devices such as MT7668U usb based Bluetooth which I plan to add the support in later patches in the next weeks

Are the USB ones also using STP or are they H:2 based like all the others. What are prominent MT7668U based ones that I could buy?

1.
USB ones don't use any STP framing, which is totally dedicated to the serial based device.

I don't exactly know what the term H:2 means you mentioned here. I only know the btusb driver can be reused for M7668U and just only one weird thing to solve in btusb driver. That is HCI WMT event coming through control in pipe, not through interrupt pipe :(

And as for the others generic hci/acl/sco data, they all work well as btusb usually work. I will show you the code to let you exactly know what I'm meaning instead of just talking :)

2.
Another thing is I think it's better if the core layer can support __hci_raw_sync_ev-like APIs to allow each transport driver not to care the details about cmd/event synchronization. If it can be done in this way, that helps to help WMT cmd/event handling can be put into a commonplace to allow btmtkuart and btusb for mtk port to have the same codeshare.

3.
MT7668U should always be bundled with CE product, I am not really sure whether it is easy to get from the retailer. Or you really like to want a sample, maybe I can try to contact with internal people to make it happen.