[PATCH v7 0/3] add support for Bluetooth on MT7622 SoC

[PATCH v7 0/3] add support for Bluetooth on MT7622 SoC

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

v7 and changes since v6:
 - drop already applied patches
 - move clk_* and pm_* operation to ->open and ->close 
 - label this driver as BT_MTKUART
 - make code style aligned with the other drivers and code better to read
   as these suggestions and fixes are mentioned in [1]

[1] http://lists.infradead.org/pipermail/linux-mediatek/2018-July/014458.html

v6 and changes since v5:
 - make btmtkuart become a separate driver.
 - drop already applied patches and the patch for btuart driver
 - refine comments in driver allowing people know that mtk extra header
   + length doesn't indicate a full H:4 frame, things can fragment.
 - enhance dt-binding document with removing mistaken added " and improve
   English sentence.
 - remove unnecessary '\n' with bt_dev_err.
 - refine code style.
 - set hdev->manufacturer as mtk id.

v5 and changes since v4:
 - add Reviewed-by Tag from Ulf Hansson for patch 2
 - remove default y in Kconfig for btmtkuart selection to avoid overkill for
   users which would like to have less an amount on stuff in kernel.
 - list header declarations in alphabetical order and add a proper blank line
   within.
 - remove unused macro.
 - use sizeof to calculate structure size instead of an aextra macro to hardcode.
 - use struct hci_dev * as input paraments for mtk_hci_wmt_sync and mtk_setup_fw
   for that can be reused in mtk bluetooth with other interfaces.
 - remove unused local variabled in mtk_btuart_recv.
 - remove superfluous :8 for dlen2 in struct mtk_stp_hdr definition.
 - give a reasonable naming for these labels and add a pm_runtime_put_noidle()
   in the path undoing failing pm_runtime_get_sync().
 - Turn __u8 into u8 in struct mtk_stp_hdr.

 Really thanks for these reviews by Johan Hovold and Andy Shevchenko

v4 and changes since v3:
 - refine patch 2 based on commit 919b7308fcc4 to allow that
   dev_pm_domain_attach() will return better error codes. 

v3 and changes since v2
* all changes happen on patch 6
 - fix up SPDX license style for btmtkuart.h.
 - change firmware download from in ACL data to in HCI commands
   and then remove unused mtk_acl_wmt_sync and related code.
 - add a workaround replacing bad vendor event id 0xe4 with 0xff every
   vendor should use.
 - add a sanity check for mtk_hci_wmt_sync to verifying if
   input parameters are valid.
 - add an atomic_inc(&bdev->hdev->cmd_cnt) for __hci_cmd_sync_ev.
 - be changed to use firmware with a header called mt7622pr2h.bin.

v2 and changes since v1
 - Dropped patches already being applied
 - Rewirte the whole driver using btuart [1], and add slight extension
   of btuart to fit into btmtkuart driver. Beware that [1] is also pulled
   into one part of the series for avoiding any breakage when the patchset
   is being compiled.

[1] btuart 
	https://www.spinics.net/lists/linux-bluetooth/msg74918.html

v1:

Hi,

This patchset introduces built-in Bluetooth support on MT7622 SoC.
And, it should be simple to make an extension to support other
MediaTek SoCs with adjusting a few of changes on the initialization
sequence of the device.

Before the main driver is being introduced, a few of things about
power-domain management should be re-worked for serdev core and MediaTek
SCPSYS to allow the Bluetooth to properly power up.

Patch 2: add a generic way attaching power domain to serdev
Patch 3 and 4: add cleanups with reuse APIs from Linux core
Patch 5: fix a limitation about power enablement Bluetooth depends on
Patch 1, 6 and 7: the major part of adding Bluetooth support to MT7622
	
	Sean

Sean Wang (3):
  dt-bindings: net: bluetooth: Add mediatek-bluetooth
  Bluetooth: mediatek: Add protocol support for MediaTek serial devices
  MAINTAINERS: add an entry for MediaTek Bluetooth driver

 .../devicetree/bindings/net/mediatek-bluetooth.txt |  35 ++
 MAINTAINERS                                        |   8 +
 drivers/bluetooth/Kconfig                          |  11 +
 drivers/bluetooth/Makefile                         |   2 +
 drivers/bluetooth/btmtkuart.c                      | 591 +++++++++++++++++++++
 5 files changed, 647 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mediatek-bluetooth.txt
 create mode 100644 drivers/bluetooth/btmtkuart.c

-- 
2.7.4

[PATCH v7 1/3] dt-bindings: net: bluetooth: Add mediatek-bluetooth

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Add binding document for a SoC built-in device using MediaTek protocol.
Which could be found on MT7622 SoC or other similar MediaTek SoCs.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/net/mediatek-bluetooth.txt | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/mediatek-bluetooth.txt

diff --git a/Documentation/devicetree/bindings/net/mediatek-bluetooth.txt b/Documentation/devicetree/bindings/net/mediatek-bluetooth.txt
new file mode 100644
index 0000000..14ceb2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mediatek-bluetooth.txt
@@ -0,0 +1,35 @@
+MediaTek SoC built-in Bluetooth Devices
+==================================
+
+This device is a serial attached device to BTIF device and thus it must be a
+child node of the serial node with BTIF. The dt-bindings details for BTIF
+device can be known via Documentation/devicetree/bindings/serial/8250.txt.
+
+Required properties:
+
+- compatible:	Must be
+		  "mediatek,mt7622-bluetooth": for MT7622 SoC
+- clocks:	Should be the clock specifiers corresponding to the entry in
+		clock-names property.
+- clock-names:	Should contain "ref" entries.
+- power-domains: Phandle to the power domain that the device is part of
+
+Example:
+
+	btif: serial@1100c000 {
+		compatible = "mediatek,mt7622-btif",
+			     "mediatek,mtk-btif";
+		reg = <0 0x1100c000 0 0x1000>;
+		interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_LOW>;
+		clocks = <&pericfg CLK_PERI_BTIF_PD>;
+		clock-names = "main";
+		reg-shift = <2>;
+		reg-io-width = <4>;
+
+		bluetooth {
+			compatible = "mediatek,mt7622-bluetooth";
+			power-domains = <&scpsys MT7622_POWER_DOMAIN_WB>;
+			clocks = <&clk25m>;
+			clock-names = "ref";
+		};
+	};
-- 
2.7.4

[PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

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 <sean.wang@mediatek.com>
---
 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

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index f3c643a..5ace676 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -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
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index ec16c55..12ad6e9 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -25,6 +25,8 @@ obj-$(CONFIG_BT_BCM)		+= btbcm.o
 obj-$(CONFIG_BT_RTL)		+= btrtl.o
 obj-$(CONFIG_BT_QCA)		+= btqca.o
 
+obj-$(CONFIG_BT_MTKUART)	+= btmtkuart.o
+
 obj-$(CONFIG_BT_HCIUART_NOKIA)	+= hci_nokia.o
 
 obj-$(CONFIG_BT_HCIRSI)		+= btrsi.o
diff --git a/drivers/bluetooth/btmtkuart.c b/drivers/bluetooth/btmtkuart.c
new file mode 100644
index 0000000..def3d4b
--- /dev/null
+++ b/drivers/bluetooth/btmtkuart.c
@@ -0,0 +1,591 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 MediaTek Inc.
+
+/*
+ * Bluetooth support for MediaTek serial devices
+ *
+ * Author: Sean Wang <sean.wang@mediatek.com>
+ *
+ */
+
+#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
+
+enum {
+	MTK_WMT_PATCH_DWNLD = 0x1,
+	MTK_WMT_FUNC_CTRL = 0x6,
+	MTK_WMT_RST = 0x7
+};
+
+struct mtk_stp_hdr {
+	u8 prefix;
+	u8 dlen1:4;
+	u8 type:4;
+	u8 dlen2;
+	u8 cs;
+} __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;
+	unsigned long tx_state;
+	struct sk_buff_head txq;
+
+	struct sk_buff *rx_skb;
+
+	struct mtk_stp_splitter *sp;
+	struct clk *clk;
+
+	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;
+	hdr->op = op;
+	hdr->dlen = cpu_to_le16(plen + 1);
+	hdr->flag = flag;
+	memcpy(wc.data, param, plen);
+
+	atomic_inc(&hdev->cmd_cnt);
+
+	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;
+	const u8 *fw_ptr;
+	size_t fw_size;
+	int err, dlen;
+	u8 flag;
+
+	fwname = FIRMWARE_MT7622;
+
+	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
+	 * 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. */
+	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)
+{
+	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);
+}
+
+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);
+		goto err_disable_rpm;
+	}
+
+	err = clk_prepare_enable(bdev->clk);
+	if (err < 0)
+		goto err_put_rpm;
+	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);
+	skb_queue_purge(&bdev->txq);
+
+	cancel_work_sync(&bdev->tx_work);
+
+	kfree_skb(bdev->rx_skb);
+	bdev->rx_skb = NULL;
+
+	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. */
+	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);
+
+	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;
+	}
+
+	/* 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];
+	skb_put_zero(skb, MTK_STP_TLR_SIZE);
+	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),
+	},
+};
+
+module_serdev_device_driver(btmtkuart_driver);
+
+MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
+MODULE_DESCRIPTION("MediaTek Bluetooth Serial driver" VERSION);
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

> 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 <sean.wang@mediatek.com>
> ---
> 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
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index f3c643a..5ace676 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -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
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index ec16c55..12ad6e9 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -25,6 +25,8 @@ obj-$(CONFIG_BT_BCM)		+= btbcm.o
> obj-$(CONFIG_BT_RTL)		+= btrtl.o
> obj-$(CONFIG_BT_QCA)		+= btqca.o
> 
> +obj-$(CONFIG_BT_MTKUART)	+= btmtkuart.o
> +
> obj-$(CONFIG_BT_HCIUART_NOKIA)	+= hci_nokia.o
> 
> obj-$(CONFIG_BT_HCIRSI)		+= btrsi.o
> diff --git a/drivers/bluetooth/btmtkuart.c b/drivers/bluetooth/btmtkuart.c
> new file mode 100644
> index 0000000..def3d4b
> --- /dev/null
> +++ b/drivers/bluetooth/btmtkuart.c
> @@ -0,0 +1,591 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018 MediaTek Inc.
> +
> +/*
> + * Bluetooth support for MediaTek serial devices
> + *
> + * Author: Sean Wang <sean.wang@mediatek.com>
> + *
> + */
> +
> +#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
> +
> +enum {
> +	MTK_WMT_PATCH_DWNLD = 0x1,
> +	MTK_WMT_FUNC_CTRL = 0x6,
> +	MTK_WMT_RST = 0x7
> +};
> +
> +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.

> +	u8 dlen2;
> +	u8 cs;

Are you checking the checksum on receive?

> +} __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;
> +	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.

> +	struct clk *clk;

Move the struct clk below struct serdev_device.

> +
> +	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;
> +	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.

> +
> +	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;
> +	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.

> +
> +	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
> +	 * 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.

> +	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)
> +{
> +	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.

> +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);
> +		goto err_disable_rpm;
> +	}
> +
> +	err = clk_prepare_enable(bdev->clk);
> +	if (err < 0)
> +		goto err_put_rpm;

Add an extra empty line here.

> +	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);
> +	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.

> +
> +	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. */
> +	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.

> +
> +	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;
> +	}
> +
> +	/* 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.

> +	skb_put_zero(skb, MTK_STP_TLR_SIZE);

Extra empty line here please.

> +	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),
> +	},
> +};
> +
> +module_serdev_device_driver(btmtkuart_driver);
> +
> +MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
> +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.

> +MODULE_VERSION(VERSION);
> +MODULE_LICENSE("GPL”);

You want to add a MODULE_FIRMWARE here as well.

Regards

Marcel

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Wed, 2018-08-01 at 09:53 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> > 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.
> > 

[ ... ]

> > +enum {
> > +	MTK_WMT_PATCH_DWNLD = 0x1,
> > +	MTK_WMT_FUNC_CTRL = 0x6,
> > +	MTK_WMT_RST = 0x7
> > +};
> > +
> > +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. 

> > +	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.

> > +} __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;
> > +	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

> > +	struct clk *clk;
> 
> Move the struct clk below struct serdev_device.
> 

okay, it is a nice arrangement

> > +
> > +	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;
> > +	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.

> > +
> > +	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;
> > +	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

> > +
> > +	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
> > +	 * 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

> > +	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)
> > +{
> > +	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

> > +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);
> > +		goto err_disable_rpm;
> > +	}
> > +
> > +	err = clk_prepare_enable(bdev->clk);
> > +	if (err < 0)
> > +		goto err_put_rpm;
> 
> Add an extra empty line here.
> 

okay

> > +	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);
> > +	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

> > +
> > +	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. */
> > +	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

> > +
> > +	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;
> > +	}
> > +
> > +	/* 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 

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

sure

> 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


> > +	skb_put_zero(skb, MTK_STP_TLR_SIZE);
> 
> Extra empty line here please.
> 

okay

> > +	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),
> > +	},
> > +};
> > +
> > +module_serdev_device_driver(btmtkuart_driver);
> > +
> > +MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
> > +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

> > +MODULE_VERSION(VERSION);
> > +MODULE_LICENSE("GPL”);
> 
> You want to add a MODULE_FIRMWARE here as well.
> 

okay

> Regards
> 
> Marcel
> 

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>> 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.
>>> 
> 
> [ ... ]
> 
>>> +enum {
>>> +	MTK_WMT_PATCH_DWNLD = 0x1,
>>> +	MTK_WMT_FUNC_CTRL = 0x6,
>>> +	MTK_WMT_RST = 0x7
>>> +};
>>> +
>>> +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. 
> 
>>> +	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.
> 
>>> +} __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;
>>> +	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
> 
>>> +	struct clk *clk;
>> 
>> Move the struct clk below struct serdev_device.
>> 
> 
> okay, it is a nice arrangement
> 
>>> +
>>> +	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;
>>> +	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.

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.

>>> +
>>> +	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;
>>> +	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
> 
>>> +
>>> +	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
>>> +	 * 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
> 
>>> +	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)
>>> +{
>>> +	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
> 
>>> +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);
>>> +		goto err_disable_rpm;
>>> +	}
>>> +
>>> +	err = clk_prepare_enable(bdev->clk);
>>> +	if (err < 0)
>>> +		goto err_put_rpm;
>> 
>> Add an extra empty line here.
>> 
> 
> okay
> 
>>> +	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);
>>> +	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
> 
>>> +
>>> +	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. */
>>> +	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
> 
>>> +
>>> +	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;
>>> +	}
>>> +
>>> +	/* 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 

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

> 
>> I would add another comment here that this added the STP trailer. And change the above to mention it adds the STP header.
>> 
> 
> sure
> 
>> 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
> 
> 
>>> +	skb_put_zero(skb, MTK_STP_TLR_SIZE);
>> 
>> Extra empty line here please.
>> 
> 
> okay
> 
>>> +	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),
>>> +	},
>>> +};
>>> +
>>> +module_serdev_device_driver(btmtkuart_driver);
>>> +
>>> +MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
>>> +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
> 
>>> +MODULE_VERSION(VERSION);
>>> +MODULE_LICENSE("GPL”);
>> 
>> You want to add a MODULE_FIRMWARE here as well.
>> 
> 
> okay

Regards

Marcel

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Thu, 2018-08-02 at 09:38 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 

[ ... ]

> >>> +
> >>> +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;
> >>> +	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.
> 
> 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);


> >>> +
> >>> +	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;
> >>> +}
> >>> +

[ ... ]

> >>> +	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.
> 

okay

> > 
> >> I would add another comment here that this added the STP trailer. And change the above to mention it adds the STP header.
> >> 
> > 
> > sure
> > 
> >> 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
> > 
> > 

[ ... ]

> >> You want to add a MODULE_FIRMWARE here as well.
> >> 
> > 
> > okay
> 
> Regards
> 
> Marcel
> 

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>>>> +
>>>>> +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;
>>>>> +	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.
>> 
>> 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);

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

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Thu, 2018-08-02 at 11:45 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>>>> +
> >>>>> +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;
> >>>>> +	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.
> >> 
> >> 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);
> 
> 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?

	Sean

> Regards
> 
> Marcel
> 

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>>>>>> +
>>>>>>> +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;
>>>>>>> +	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.
>>>> 
>>>> 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);
>> 
>> 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?

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

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Fri, 2018-08-03 at 14:51 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>>>>>> +
> >>>>>>> +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;
> >>>>>>> +	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.
> >>>> 
> >>>> 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);
> >> 
> >> 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?
> 
> 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);

> Regards
> 
> Marcel
> 

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>>>>>>>> +
>>>>>>>>> +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;
>>>>>>>>> +	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.
>>>>>> 
>>>>>> 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);
>>>> 
>>>> 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?
>> 
>> 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);

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

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Fri, 2018-08-03 at 19:19 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>>>>>>>> +
> >>>>>>>>> +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;
> >>>>>>>>> +	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.
> >>>>>> 
> >>>>>> 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);
> >>>> 
> >>>> 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?
> >> 
> >> 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);
> 
> 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.

> 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

> Regards
> 
> Marcel
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>>>>>>>>>> +
>>>>>>>>>>> +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;
>>>>>>>>>>> +	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.
>>>>>>>> 
>>>>>>>> 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);
>>>>>> 
>>>>>> 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?
>>>> 
>>>> 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);
>> 
>> 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.

>> 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

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Mon, 2018-08-06 at 17:39 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>>>>>>>>>> +

[ ... ]

> >> 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.

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.

> 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.

> >> 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 

> Regards
> 
> Marcel
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>>> 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.
> 
> 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.
> 
>> 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.

> 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.

>>>> 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 

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

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Tue, 2018-08-07 at 17:54 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>>> 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.
> > 
> > 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.
> > 
> >> 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.
> 
> > 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.
> 
> >>>> 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 
> 
> 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.

	Sean

> Regards
> 
> Marcel
> 

Re: [PATCH v7 2/3] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>>>>> 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.
>>> 
>>> 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.
>>> 
>>>> 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.
>> 
>>> 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.
>> 
>>>>>> 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 
>> 
>> 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 :)

Bluetooth USB transport was originally section H:2 in the specification (and UART was section H:4) that is where this naming comes wrong. The btusb.c driver is implementing H:2 transport.

How does /sys/kernel/debug/usb/devices look for these devices? And I don’t recall that there is a control in pipe.\x10 That concept doesn’t really exist in USB.

> 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.

Such a core API will not help you. The btusb.c driver has already bunch of examples where it has to fix up things. Qualcomm/Atheros have done there fun way of firmware download and Intel also has done fun stuff with event over bulk endpoints.

> 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.

If it is some NGFF mPCI card that I can plug into an adapter card and attach it to USB, or if you have some dev board, that would be interesting.

Regards

Marcel

[PATCH v7 3/3] MAINTAINERS: add an entry for MediaTek Bluetooth driver

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Add an entry for the MediaTek Bluetooth driver.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0f2cce4..e983d9e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9144,6 +9144,14 @@ F:	include/uapi/linux/meye.h
 F:	include/uapi/linux/ivtv*
 F:	include/uapi/linux/uvcvideo.h
 
+MEDIATEK BLUETOOTH DRIVER
+M:	Sean Wang <sean.wang@mediatek.com>
+L:	linux-bluetooth@vger.kernel.org
+L:	linux-mediatek@lists.infradead.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/mediatek-bluetooth.txt
+F:	drivers/bluetooth/btmtkuart.c
+
 MEDIATEK CIR DRIVER
 M:	Sean Wang <sean.wang@mediatek.com>
 S:	Maintained
-- 
2.7.4