[PATCH v5 0/7] add support for Bluetooth on MT7622 SoC

[PATCH v5 0/7] add support for Bluetooth on MT7622 SoC

[sean.wang at mediatek.com]

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

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.
 - Change coding style for align 80-chars wrap

 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

Marcel Holtmann (1):
  Bluetooth: Add new serdev based driver for UART attached controllers

Sean Wang (6):
  dt-bindings: net: bluetooth: Add mediatek-bluetooth
  serdev: add dev_pm_domain_attach|detach()
  Bluetooth: Add new quirk for non-persistent setup settings
  Bluetooth: Extend btuart driver for join more vendor devices
  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                          |  22 +
 drivers/bluetooth/Makefile                         |   3 +
 drivers/bluetooth/btmtkuart.c                      | 352 ++++++++++++++
 drivers/bluetooth/btmtkuart.h                      | 116 +++++
 drivers/bluetooth/btuart.c                         | 527 +++++++++++++++++++++
 drivers/bluetooth/btuart.h                         |  30 ++
 drivers/tty/serdev/core.c                          |  15 +-
 include/net/bluetooth/hci.h                        |   9 +
 net/bluetooth/hci_core.c                           |   3 +-
 11 files changed, 1118 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/mediatek-bluetooth.txt
 create mode 100644 drivers/bluetooth/btmtkuart.c
 create mode 100644 drivers/bluetooth/btmtkuart.h
 create mode 100644 drivers/bluetooth/btuart.c
 create mode 100644 drivers/bluetooth/btuart.h

-- 
2.7.4

[PATCH v5 1/7] dt-bindings: net: bluetooth: Add mediatek-bluetooth

[sean.wang at mediatek.com]

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..1335429
--- /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 one of
+		  "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 at 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 v5 2/7] serdev: add dev_pm_domain_attach|detach()

[sean.wang at mediatek.com]

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

In order to open up the required power gate before any operation can be
effectively performed over the serial bus between CPU and serdev, it's
clearly essential to add common attach functions for PM domains to serdev
at the probe phase.

Similarly, the relevant dettach function for the PM domains should be
properly and reversely added at the remove phase.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-serial at vger.kernel.org
---
 drivers/tty/serdev/core.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index bd47c46..9db93f5 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/serdev.h>
 #include <linux/slab.h>
@@ -350,8 +351,17 @@ EXPORT_SYMBOL_GPL(serdev_device_set_tiocm);
 static int serdev_drv_probe(struct device *dev)
 {
 	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
+	int ret;
 
-	return sdrv->probe(to_serdev_device(dev));
+	ret = dev_pm_domain_attach(dev, true);
+	if (ret)
+		return ret;
+
+	ret = sdrv->probe(to_serdev_device(dev));
+	if (ret)
+		dev_pm_domain_detach(dev, true);
+
+	return ret;
 }
 
 static int serdev_drv_remove(struct device *dev)
@@ -359,6 +369,9 @@ static int serdev_drv_remove(struct device *dev)
 	const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
 	if (sdrv->remove)
 		sdrv->remove(to_serdev_device(dev));
+
+	dev_pm_domain_detach(dev, true);
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH v5 3/7] Bluetooth: Add new serdev based driver for UART attached controllers

[sean.wang at mediatek.com]

From: Marcel Holtmann <marcel@holtmann.org>

This is a from scratch written driver to run H:4 on serdev based system
with a Bluetooth controller attached via an UART. It is currently tested
on RPi3 and it has Broadcom integration. It is DT only and is missing
GPIO and runtime power management integration. Also Apple or ACPI
support is currently not added.

To integrate with controllers from Intel and Qualcomm, similar handling
like with btusb.c has to be done. A simple abstraction for that has been
provided to make it similar to hci_uart.

The goal is to run individual drivers on serdev capable systems so that
we can retire hci_uart on these system and continue with a lot simpler
and easier to maintain driver. It seems that hci_uart has too many race
conditions due to handling TTY and line disciplines. And fixes for that
are not really related to serdev based drivers. In a serdev only world
it makes sense to remove any of the complex code.

Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 drivers/bluetooth/Kconfig  |  11 +
 drivers/bluetooth/Makefile |   1 +
 drivers/bluetooth/btuart.c | 506 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 518 insertions(+)
 create mode 100644 drivers/bluetooth/btuart.c

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index f3c643a..00fdf5f 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_HCIBTUART
+	tristate "HCI UART driver"
+	depends on SERIAL_DEV_BUS
+	help
+	  Bluetooth HCI UART driver.
+	  This driver is required if you want to use Bluetooth device with
+	  UART interface.
+
+	  Say Y here to compile support for Bluetooth UART devices into the
+	  kernel or say M to compile it as module (btuart).
+
 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..60a19cb 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_BT_HCIBLUECARD)	+= bluecard_cs.o
 
 obj-$(CONFIG_BT_HCIBTUSB)	+= btusb.o
 obj-$(CONFIG_BT_HCIBTSDIO)	+= btsdio.o
+obj-$(CONFIG_BT_HCIBTUART)	+= btuart.o
 
 obj-$(CONFIG_BT_INTEL)		+= btintel.o
 obj-$(CONFIG_BT_ATH3K)		+= ath3k.o
diff --git a/drivers/bluetooth/btuart.c b/drivers/bluetooth/btuart.c
new file mode 100644
index 0000000..a900aac
--- /dev/null
+++ b/drivers/bluetooth/btuart.c
@@ -0,0 +1,506 @@
+/*
+ *
+ *  Generic Bluetooth HCI UART driver
+ *
+ *  Copyright (C) 2015-2018  Intel Corporation
+ *
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/skbuff.h>
+#include <linux/serdev.h>
+#include <linux/of.h>
+#include <linux/firmware.h>
+#include <asm/unaligned.h>
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include "h4_recv.h"
+#include "btbcm.h"
+
+#define VERSION "1.0"
+
+struct btuart_vnd {
+	const struct h4_recv_pkt *recv_pkts;
+	int recv_pkts_cnt;
+	unsigned int manufacturer;
+	int (*open)(struct hci_dev *hdev);
+	int (*close)(struct hci_dev *hdev);
+	int (*setup)(struct hci_dev *hdev);
+};
+
+struct btuart_dev {
+	struct hci_dev *hdev;
+	struct serdev_device *serdev;
+
+	struct work_struct tx_work;
+	unsigned long tx_state;
+	struct sk_buff_head txq;
+
+	struct sk_buff *rx_skb;
+
+	const struct btuart_vnd *vnd;
+};
+
+#define BTUART_TX_STATE_ACTIVE	1
+#define BTUART_TX_STATE_WAKEUP	2
+
+static void btuart_tx_work(struct work_struct *work)
+{
+	struct btuart_dev *bdev = container_of(work, struct btuart_dev,
+					       tx_work);
+	struct serdev_device *serdev = bdev->serdev;
+	struct hci_dev *hdev = bdev->hdev;
+
+	while (1) {
+		clear_bit(BTUART_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(BTUART_TX_STATE_WAKEUP, &bdev->tx_state))
+			break;
+	}
+
+	clear_bit(BTUART_TX_STATE_ACTIVE, &bdev->tx_state);
+}
+
+static int btuart_tx_wakeup(struct btuart_dev *bdev)
+{
+	if (test_and_set_bit(BTUART_TX_STATE_ACTIVE, &bdev->tx_state)) {
+		set_bit(BTUART_TX_STATE_WAKEUP, &bdev->tx_state);
+		return 0;
+	}
+
+	schedule_work(&bdev->tx_work);
+	return 0;
+}
+
+static int btuart_open(struct hci_dev *hdev)
+{
+	struct btuart_dev *bdev = hci_get_drvdata(hdev);
+	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));
+		return err;
+	}
+
+	if (bdev->vnd->open) {
+		err = bdev->vnd->open(hdev);
+		if (err) {
+			serdev_device_close(bdev->serdev);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int btuart_close(struct hci_dev *hdev)
+{
+	struct btuart_dev *bdev = hci_get_drvdata(hdev);
+	int err;
+
+	if (bdev->vnd->close) {
+		err = bdev->vnd->close(hdev);
+		if (err)
+			return err;
+	}
+
+	serdev_device_close(bdev->serdev);
+
+	return 0;
+}
+
+static int btuart_flush(struct hci_dev *hdev)
+{
+	struct btuart_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 btuart_setup(struct hci_dev *hdev)
+{
+	struct btuart_dev *bdev = hci_get_drvdata(hdev);
+
+	if (bdev->vnd->setup)
+		return bdev->vnd->setup(hdev);
+
+	return 0;
+}
+
+static int btuart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct btuart_dev *bdev = hci_get_drvdata(hdev);
+
+	/* Prepend skb with frame type */
+	memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
+	skb_queue_tail(&bdev->txq, skb);
+
+	btuart_tx_wakeup(bdev);
+	return 0;
+}
+
+static int btuart_receive_buf(struct serdev_device *serdev, const u8 *data,
+			      size_t count)
+{
+	struct btuart_dev *bdev = serdev_device_get_drvdata(serdev);
+	const struct btuart_vnd *vnd = bdev->vnd;
+
+	bdev->rx_skb = h4_recv_buf(bdev->hdev, bdev->rx_skb, data, count,
+				   vnd->recv_pkts, vnd->recv_pkts_cnt);
+	if (IS_ERR(bdev->rx_skb)) {
+		int err = PTR_ERR(bdev->rx_skb);
+		bt_dev_err(bdev->hdev, "Frame reassembly failed (%d)", err);
+		bdev->rx_skb = NULL;
+		return err;
+	}
+
+	bdev->hdev->stat.byte_rx += count;
+
+	return count;
+}
+
+static void btuart_write_wakeup(struct serdev_device *serdev)
+{
+	struct btuart_dev *bdev = serdev_device_get_drvdata(serdev);
+
+	btuart_tx_wakeup(bdev);
+}
+
+static const struct serdev_device_ops btuart_client_ops = {
+	.receive_buf = btuart_receive_buf,
+	.write_wakeup = btuart_write_wakeup,
+};
+
+#define BCM_NULL_PKT 0x00
+#define BCM_NULL_SIZE 0
+
+#define BCM_LM_DIAG_PKT 0x07
+#define BCM_LM_DIAG_SIZE 63
+
+#define BCM_RECV_LM_DIAG \
+	.type = BCM_LM_DIAG_PKT, \
+	.hlen = BCM_LM_DIAG_SIZE, \
+	.loff = 0, \
+	.lsize = 0, \
+	.maxlen = BCM_LM_DIAG_SIZE
+
+#define BCM_RECV_NULL \
+	.type = BCM_NULL_PKT, \
+	.hlen = BCM_NULL_SIZE, \
+	.loff = 0, \
+	.lsize = 0, \
+	.maxlen = BCM_NULL_SIZE
+
+static int bcm_set_diag(struct hci_dev *hdev, bool enable)
+{
+	struct btuart_dev *bdev = hci_get_drvdata(hdev);
+	struct sk_buff *skb;
+
+	if (!test_bit(HCI_RUNNING, &hdev->flags))
+		return -ENETDOWN;
+
+	skb = bt_skb_alloc(3, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	skb_put_u8(skb, BCM_LM_DIAG_PKT);
+	skb_put_u8(skb, 0xf0);
+	skb_put_u8(skb, enable);
+
+	skb_queue_tail(&bdev->txq, skb);
+	btuart_tx_wakeup(bdev);
+
+	return 0;
+}
+
+static int bcm_set_baudrate(struct btuart_dev *bdev, unsigned int speed)
+{
+	struct hci_dev *hdev = bdev->hdev;
+	struct sk_buff *skb;
+	struct bcm_update_uart_baud_rate param;
+
+	if (speed > 3000000) {
+		struct bcm_write_uart_clock_setting clock;
+
+		clock.type = BCM_UART_CLOCK_48MHZ;
+
+		bt_dev_dbg(hdev, "Set Controller clock (%d)", clock.type);
+
+		/* This Broadcom specific command changes the UART's controller
+		 * clock for baud rate > 3000000.
+		 */
+		skb = __hci_cmd_sync(hdev, 0xfc45, 1, &clock, HCI_INIT_TIMEOUT);
+		if (IS_ERR(skb)) {
+			int err = PTR_ERR(skb);
+			bt_dev_err(hdev, "Failed to write clock (%d)", err);
+			return err;
+		}
+
+		kfree_skb(skb);
+	}
+
+	bt_dev_dbg(hdev, "Set Controller UART speed to %d bit/s", speed);
+
+	param.zero = cpu_to_le16(0);
+	param.baud_rate = cpu_to_le32(speed);
+
+	/* This Broadcom specific command changes the UART's controller baud
+	 * rate.
+	 */
+	skb = __hci_cmd_sync(hdev, 0xfc18, sizeof(param), &param,
+			     HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		int err = PTR_ERR(skb);
+		bt_dev_err(hdev, "Failed to write update baudrate (%d)", err);
+		return err;
+	}
+
+	kfree_skb(skb);
+
+	return 0;
+}
+
+static int bcm_setup(struct hci_dev *hdev)
+{
+	struct btuart_dev *bdev = hci_get_drvdata(hdev);
+	char fw_name[64];
+	const struct firmware *fw;
+	unsigned int speed;
+	int err;
+
+	hdev->set_diag = bcm_set_diag;
+	hdev->set_bdaddr = btbcm_set_bdaddr;
+
+	/* Init speed if any */
+	speed = 115200;
+
+	if (speed)
+		serdev_device_set_baudrate(bdev->serdev, speed);
+
+	/* Operational speed if any */
+	speed = 115200;
+
+	if (speed) {
+		err = bcm_set_baudrate(bdev, speed);
+		if (err)
+			bt_dev_err(hdev, "Failed to set baudrate");
+		else
+			serdev_device_set_baudrate(bdev->serdev, speed);
+	}
+
+	err = btbcm_initialize(hdev, fw_name, sizeof(fw_name), false);
+	if (err)
+		return err;
+
+	err = request_firmware(&fw, fw_name, &hdev->dev);
+	if (err < 0) {
+		bt_dev_warn(hdev, "Patch %s not found", fw_name);
+		return 0;
+	}
+
+	err = btbcm_patchram(bdev->hdev, fw);
+	if (err) {
+		bt_dev_err(hdev, "Patching failed (%d)", err);
+		goto finalize;
+	}
+
+	/* Init speed if any */
+	speed = 115200;
+
+	if (speed)
+		serdev_device_set_baudrate(bdev->serdev, speed);
+
+	/* Operational speed if any */
+	speed = 115200;
+
+	if (speed) {
+		err = bcm_set_baudrate(bdev, speed);
+		if (!err)
+			serdev_device_set_baudrate(bdev->serdev, speed);
+	}
+
+finalize:
+	release_firmware(fw);
+
+	err = btbcm_finalize(hdev);
+	if (err)
+		return err;
+
+	return err;
+}
+
+static const struct h4_recv_pkt bcm_recv_pkts[] = {
+	{ H4_RECV_ACL,      .recv = hci_recv_frame },
+	{ H4_RECV_SCO,      .recv = hci_recv_frame },
+	{ H4_RECV_EVENT,    .recv = hci_recv_frame },
+	{ BCM_RECV_LM_DIAG, .recv = hci_recv_diag  },
+	{ BCM_RECV_NULL,    .recv = hci_recv_diag  },
+};
+
+static const struct btuart_vnd bcm_vnd = {
+	.recv_pkts	= bcm_recv_pkts,
+	.recv_pkts_cnt	= ARRAY_SIZE(bcm_recv_pkts),
+	.manufacturer	= 15,
+	.setup		= bcm_setup,
+};
+
+static const struct h4_recv_pkt default_recv_pkts[] = {
+	{ H4_RECV_ACL,      .recv = hci_recv_frame },
+	{ H4_RECV_SCO,      .recv = hci_recv_frame },
+	{ H4_RECV_EVENT,    .recv = hci_recv_frame },
+};
+
+static const struct btuart_vnd default_vnd = {
+	.recv_pkts	= default_recv_pkts,
+	.recv_pkts_cnt	= ARRAY_SIZE(default_recv_pkts),
+};
+
+static int btuart_probe(struct serdev_device *serdev)
+{
+	struct btuart_dev *bdev;
+	struct hci_dev *hdev;
+
+	bdev = devm_kzalloc(&serdev->dev, sizeof(*bdev), GFP_KERNEL);
+	if (!bdev)
+		return -ENOMEM;
+
+	/* Request the vendor specific data and callbacks */
+	bdev->vnd = device_get_match_data(&serdev->dev);
+	if (!bdev->vnd)
+		bdev->vnd = &default_vnd;
+
+	bdev->serdev = serdev;
+	serdev_device_set_drvdata(serdev, bdev);
+
+	serdev_device_set_client_ops(serdev, &btuart_client_ops);
+
+	INIT_WORK(&bdev->tx_work, btuart_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);
+
+	/* Only when vendor specific setup callback is provided, consider
+	 * the manufacturer information valid. This avoids filling in the
+	 * value for Ericsson when nothing is specified.
+	 */
+	if (bdev->vnd->setup)
+		hdev->manufacturer = bdev->vnd->manufacturer;
+
+	hdev->open  = btuart_open;
+	hdev->close = btuart_close;
+	hdev->flush = btuart_flush;
+	hdev->setup = btuart_setup;
+	hdev->send  = btuart_send_frame;
+	SET_HCIDEV_DEV(hdev, &serdev->dev);
+
+	if (hci_register_dev(hdev) < 0) {
+		dev_err(&serdev->dev, "Can't register HCI device\n");
+		hci_free_dev(hdev);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void btuart_remove(struct serdev_device *serdev)
+{
+	struct btuart_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 btuart_of_match_table[] = {
+	{ .compatible = "brcm,bcm43438-bt", .data = &bcm_vnd },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, btuart_of_match_table);
+#endif
+
+static struct serdev_device_driver btuart_driver = {
+	.probe = btuart_probe,
+	.remove = btuart_remove,
+	.driver = {
+		.name = "btuart",
+		.of_match_table = of_match_ptr(btuart_of_match_table),
+	},
+};
+
+module_serdev_device_driver(btuart_driver);
+
+MODULE_AUTHOR("Marcel Holtmann <marcel@holtmann.org>");
+MODULE_DESCRIPTION("Generic Bluetooth UART driver ver " VERSION);
+MODULE_VERSION(VERSION);
+MODULE_LICENSE("GPL");
-- 
2.7.4

[PATCH v5 4/7] Bluetooth: Add new quirk for non-persistent setup settings

[sean.wang at mediatek.com]

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

Add a new quirk HCI_QUIRK_NON_PERSISTENT_SETUP allowing that a quirk that
runs setup() after every open() and not just after the first open().

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 include/net/bluetooth/hci.h | 9 +++++++++
 net/bluetooth/hci_core.c    | 3 ++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 73e48be..d3ec5b2a8 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -183,6 +183,15 @@ enum {
 	 * during the hdev->setup vendor callback.
 	 */
 	HCI_QUIRK_NON_PERSISTENT_DIAG,
+
+	/* When this quirk is set, setup() would be run after every
+	 * open() and not just after the first open().
+	 *
+	 * This quirk can be set before hci_register_dev is called or
+	 * during the hdev->setup vendor callback.
+	 *
+	 */
+	HCI_QUIRK_NON_PERSISTENT_SETUP,
 };
 
 /* HCI device flags */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index f5c21004..0111280 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1396,7 +1396,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
 	atomic_set(&hdev->cmd_cnt, 1);
 	set_bit(HCI_INIT, &hdev->flags);
 
-	if (hci_dev_test_flag(hdev, HCI_SETUP)) {
+	if (hci_dev_test_flag(hdev, HCI_SETUP) ||
+	    test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
 		hci_sock_dev_event(hdev, HCI_DEV_SETUP);
 
 		if (hdev->setup)
-- 
2.7.4

[PATCH v5 5/7] Bluetooth: Extend btuart driver for join more vendor devices

[sean.wang at mediatek.com]

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

Adding an independent btuart.h header allows these essential definitions
can be reused in vendor driver. Also, struct btuart_vnd is extended with
additional callbacks such as .init initializing vendor data, .shtudown,
.recv and .send supporting SoC specific framing for that btuart can
simply adapt to various Bluetooth uart-based devices.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/bluetooth/btuart.c | 73 ++++++++++++++++++++++++----------------------
 drivers/bluetooth/btuart.h | 30 +++++++++++++++++++
 2 files changed, 68 insertions(+), 35 deletions(-)
 create mode 100644 drivers/bluetooth/btuart.h

diff --git a/drivers/bluetooth/btuart.c b/drivers/bluetooth/btuart.c
index a900aac..65d0086 100644
--- a/drivers/bluetooth/btuart.c
+++ b/drivers/bluetooth/btuart.c
@@ -33,35 +33,11 @@
 #include <net/bluetooth/hci_core.h>
 
 #include "h4_recv.h"
+#include "btuart.h"
 #include "btbcm.h"
 
 #define VERSION "1.0"
 
-struct btuart_vnd {
-	const struct h4_recv_pkt *recv_pkts;
-	int recv_pkts_cnt;
-	unsigned int manufacturer;
-	int (*open)(struct hci_dev *hdev);
-	int (*close)(struct hci_dev *hdev);
-	int (*setup)(struct hci_dev *hdev);
-};
-
-struct btuart_dev {
-	struct hci_dev *hdev;
-	struct serdev_device *serdev;
-
-	struct work_struct tx_work;
-	unsigned long tx_state;
-	struct sk_buff_head txq;
-
-	struct sk_buff *rx_skb;
-
-	const struct btuart_vnd *vnd;
-};
-
-#define BTUART_TX_STATE_ACTIVE	1
-#define BTUART_TX_STATE_WAKEUP	2
-
 static void btuart_tx_work(struct work_struct *work)
 {
 	struct btuart_dev *bdev = container_of(work, struct btuart_dev,
@@ -187,13 +163,27 @@ static int btuart_setup(struct hci_dev *hdev)
 	return 0;
 }
 
+static int btuart_shutdown(struct hci_dev *hdev)
+{
+	struct btuart_dev *bdev = hci_get_drvdata(hdev);
+
+	if (bdev->vnd->shutdown)
+		return bdev->vnd->shutdown(hdev);
+
+	return 0;
+}
+
 static int btuart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct btuart_dev *bdev = hci_get_drvdata(hdev);
 
-	/* Prepend skb with frame type */
-	memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
-	skb_queue_tail(&bdev->txq, skb);
+	if (bdev->vnd->send) {
+		bdev->vnd->send(hdev, skb);
+	} else {
+		/* Prepend skb with frame type */
+		memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
+		skb_queue_tail(&bdev->txq, skb);
+	}
 
 	btuart_tx_wakeup(bdev);
 	return 0;
@@ -204,14 +194,23 @@ static int btuart_receive_buf(struct serdev_device *serdev, const u8 *data,
 {
 	struct btuart_dev *bdev = serdev_device_get_drvdata(serdev);
 	const struct btuart_vnd *vnd = bdev->vnd;
+	int err;
 
-	bdev->rx_skb = h4_recv_buf(bdev->hdev, bdev->rx_skb, data, count,
-				   vnd->recv_pkts, vnd->recv_pkts_cnt);
-	if (IS_ERR(bdev->rx_skb)) {
-		int err = PTR_ERR(bdev->rx_skb);
-		bt_dev_err(bdev->hdev, "Frame reassembly failed (%d)", err);
-		bdev->rx_skb = NULL;
-		return err;
+	if (bdev->vnd->recv) {
+		err = bdev->vnd->recv(bdev->hdev, data, count);
+		if (err < 0)
+			return err;
+	} else {
+		bdev->rx_skb = h4_recv_buf(bdev->hdev, bdev->rx_skb,
+					   data, count, vnd->recv_pkts,
+					   vnd->recv_pkts_cnt);
+		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;
+		}
 	}
 
 	bdev->hdev->stat.byte_rx += count;
@@ -429,6 +428,9 @@ static int btuart_probe(struct serdev_device *serdev)
 	if (!bdev->vnd)
 		bdev->vnd = &default_vnd;
 
+	if (bdev->vnd->init)
+		bdev->data = bdev->vnd->init(&serdev->dev);
+
 	bdev->serdev = serdev;
 	serdev_device_set_drvdata(serdev, bdev);
 
@@ -460,6 +462,7 @@ static int btuart_probe(struct serdev_device *serdev)
 	hdev->close = btuart_close;
 	hdev->flush = btuart_flush;
 	hdev->setup = btuart_setup;
+	hdev->shutdown = btuart_shutdown;
 	hdev->send  = btuart_send_frame;
 	SET_HCIDEV_DEV(hdev, &serdev->dev);
 
diff --git a/drivers/bluetooth/btuart.h b/drivers/bluetooth/btuart.h
new file mode 100644
index 0000000..6c1fe31
--- /dev/null
+++ b/drivers/bluetooth/btuart.h
@@ -0,0 +1,30 @@
+struct btuart_vnd {
+	const struct h4_recv_pkt *recv_pkts;
+	int recv_pkts_cnt;
+	unsigned int manufacturer;
+	void *(*init)(struct device *dev);
+
+	int (*open)(struct hci_dev *hdev);
+	int (*close)(struct hci_dev *hdev);
+	int (*setup)(struct hci_dev *hdev);
+	int (*shutdown)(struct hci_dev *hdev);
+	int (*send)(struct hci_dev *hdev, struct sk_buff *skb);
+	int (*recv)(struct hci_dev *hdev, const u8 *data, size_t count);
+};
+
+struct btuart_dev {
+	struct hci_dev *hdev;
+	struct serdev_device *serdev;
+
+	struct work_struct tx_work;
+	unsigned long tx_state;
+	struct sk_buff_head txq;
+
+	struct sk_buff *rx_skb;
+
+	const struct btuart_vnd *vnd;
+	void *data;
+};
+
+#define BTUART_TX_STATE_ACTIVE	1
+#define BTUART_TX_STATE_WAKEUP	2
-- 
2.7.4

[PATCH v5 6/7] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[sean.wang at mediatek.com]

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

This adds a driver to run on the top of btuart 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 | 352 ++++++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btmtkuart.h | 116 ++++++++++++++
 drivers/bluetooth/btuart.c    |  18 +++
 5 files changed, 499 insertions(+)
 create mode 100644 drivers/bluetooth/btmtkuart.c
 create mode 100644 drivers/bluetooth/btmtkuart.h

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 00fdf5f..4d7d640 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -85,6 +85,17 @@ config BT_HCIBTUART
 	  Say Y here to compile support for Bluetooth UART devices into the
 	  kernel or say M to compile it as module (btuart).
 
+config BT_HCIBTUART_MTK
+	tristate "MediaTek HCI UART driver"
+	depends on BT_HCIBTUART
+	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 60a19cb..c9a8926 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -26,6 +26,8 @@ obj-$(CONFIG_BT_BCM)		+= btbcm.o
 obj-$(CONFIG_BT_RTL)		+= btrtl.o
 obj-$(CONFIG_BT_QCA)		+= btqca.o
 
+obj-$(CONFIG_BT_HCIBTUART_MTK)	+= 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..9eed21c
--- /dev/null
+++ b/drivers/bluetooth/btmtkuart.c
@@ -0,0 +1,352 @@
+// 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/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/serdev.h>
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include "btmtkuart.h"
+#include "btuart.h"
+#include "h4_recv.h"
+
+static void mtk_stp_reset(struct mtk_stp_splitter *sp)
+{
+	sp->cursor = 2;
+	sp->dlen = 0;
+}
+
+static const unsigned char *
+mtk_stp_split(struct btuart_dev *bdev, struct mtk_stp_splitter *sp,
+	      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 (!sp->dlen && sp->cursor >= 6)
+		sp->cursor = 0;
+
+	/* Filling pad until all STP info is obtained. */
+	while (sp->cursor < 6 && count > 0) {
+		sp->pad[sp->cursor] = *data;
+		sp->cursor++;
+		data++;
+		count--;
+	}
+
+	/* Retrieve STP info and have a sanity check. */
+	if (!sp->dlen && sp->cursor >= 6) {
+		shdr = (struct mtk_stp_hdr *)&sp->pad[2];
+		sp->dlen = shdr->dlen1 << 8 | shdr->dlen2;
+
+		/* Resync STP when unexpected data is being read. */
+		if (shdr->prefix != 0x80 || sp->dlen > 2048) {
+			bt_dev_err(bdev->hdev, "stp format unexpect (%d, %d)",
+				   shdr->prefix, sp->dlen);
+			mtk_stp_reset(sp);
+		}
+	}
+
+	/* 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, sp->dlen);
+	/* Update the remaining size of STP packet. */
+	sp->dlen -= *sz_h4;
+
+	/* Data points to STP payload which can be handled by H4. */
+	return data;
+}
+
+static int mtk_stp_send(struct btuart_dev *bdev, struct sk_buff *skb)
+{
+	struct mtk_stp_hdr *shdr;
+	struct sk_buff *new_skb;
+	int dlen;
+
+	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));
+	mtk_make_stp_hdr(shdr, 0, dlen);
+	skb_put_zero(skb, MTK_STP_TLR_SIZE);
+
+	skb_queue_tail(&bdev->txq, skb);
+
+	return 0;
+}
+
+static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 opcode, 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;
+	mtk_make_wmt_hdr(hdr, opcode, plen, 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)\n", 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;
+
+	while (fw_size > 0) {
+		dlen = min_t(int, 250, fw_size);
+
+		/* Tell deivice the position in sequence. */
+		flag = (fw_size - dlen <= 0) ? 3 :
+		       (fw_size < fw->size - 30) ? 2 : 1;
+
+		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;
+}
+
+void *mtk_btuart_init(struct device *dev)
+{
+	struct mtk_bt_dev *soc;
+
+	soc = devm_kzalloc(dev, sizeof(*soc), GFP_KERNEL);
+	if (!soc)
+		return ERR_PTR(-ENOMEM);
+
+	soc->sp = devm_kzalloc(dev, sizeof(*soc->sp), GFP_KERNEL);
+	if (!soc->sp)
+		return ERR_PTR(-ENOMEM);
+
+	soc->clk = devm_clk_get(dev, "ref");
+	if (IS_ERR(soc->clk))
+		return ERR_CAST(soc->clk);
+
+	return soc;
+}
+EXPORT_SYMBOL_GPL(mtk_btuart_init);
+
+int mtk_btuart_send(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct btuart_dev *bdev = hci_get_drvdata(hdev);
+
+	return mtk_stp_send(bdev, skb);
+}
+EXPORT_SYMBOL_GPL(mtk_btuart_send);
+
+int mtk_btuart_hci_frame(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);
+}
+EXPORT_SYMBOL_GPL(mtk_btuart_hci_frame);
+
+int mtk_btuart_recv(struct hci_dev *hdev, const u8 *data, size_t count)
+{
+	struct btuart_dev *bdev = hci_get_drvdata(hdev);
+	const unsigned char *p_left = data, *p_h4;
+	const struct btuart_vnd *vnd = bdev->vnd;
+	struct mtk_bt_dev *soc = bdev->data;
+	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 don'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 and whose length recorded in STP header can
+		 *  shows up the most length the H:4 engine can handle in one
+		 *  time.
+		 */
+
+		p_h4 = mtk_stp_split(bdev, soc->sp, 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, vnd->recv_pkts,
+					   vnd->recv_pkts_cnt);
+		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;
+}
+EXPORT_SYMBOL_GPL(mtk_btuart_recv);
+
+int mtk_btuart_setup(struct hci_dev *hdev)
+{
+	struct btuart_dev *bdev = hci_get_drvdata(hdev);
+	struct mtk_bt_dev *soc = bdev->data;
+	struct device *dev;
+	u8 param = 0x1;
+	int err = 0;
+
+	dev = &bdev->serdev->dev;
+
+	mtk_stp_reset(soc->sp);
+
+	/* 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(soc->clk);
+	if (err < 0)
+		goto err_put_rpm;
+
+	/* Setup a firmware which the device definitely requires. */
+	err = mtk_setup_fw(hdev);
+	if (err < 0)
+		goto err_clk;
+
+	/* Activate funciton the firmware providing to. */
+	err = mtk_hci_wmt_sync(hdev, MTK_WMT_RST, 0x4, 0, 0);
+	if (err < 0)
+		goto err_clk;
+
+	/* Enable Bluetooth protocol. */
+	err = mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param),
+			       &param);
+	if (err < 0)
+		goto err_clk;
+
+	set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+
+	return 0;
+err_clk:
+	clk_disable_unprepare(soc->clk);
+err_put_rpm:
+	pm_runtime_put_sync(dev);
+err_disable_rpm:
+	pm_runtime_disable(dev);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(mtk_btuart_setup);
+
+int mtk_btuart_shutdown(struct hci_dev *hdev)
+{
+	struct btuart_dev *bdev = hci_get_drvdata(hdev);
+	struct device *dev = &bdev->serdev->dev;
+	struct mtk_bt_dev *soc = bdev->data;
+	u8 param = 0x0;
+
+	/* Disable the device. */
+	mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param), &param);
+
+	/* Shutdown the clock and power domain the device requires. */
+	clk_disable_unprepare(soc->clk);
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mtk_btuart_shutdown);
+
+MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
+MODULE_DESCRIPTION("Bluetooth Support for MediaTek Serial Devices");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/bluetooth/btmtkuart.h b/drivers/bluetooth/btmtkuart.h
new file mode 100644
index 0000000..4c2c24e
--- /dev/null
+++ b/drivers/bluetooth/btmtkuart.h
@@ -0,0 +1,116 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ *
+ * Bluetooth support for MediaTek serial devices
+ *
+ * Author: Sean Wang <sean.wang@mediatek.com>
+ *
+ */
+
+#define FIRMWARE_MT7622		"mediatek/mt7622pr2h.bin"
+
+#define MTK_STP_TLR_SIZE	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 mtk_stp_splitter {
+	u8	pad[6];
+	u8	cursor;
+	u16	dlen;
+};
+
+struct mtk_bt_dev {
+	struct clk *clk;
+	struct completion wmt_cmd;
+	struct mtk_stp_splitter *sp;
+};
+
+static inline void
+mtk_make_stp_hdr(struct mtk_stp_hdr *hdr, u8 type, u32 dlen)
+{
+	u8 *p = (u8 *)hdr;
+
+	hdr->prefix = 0x80;
+	hdr->dlen1 = (dlen & 0xf00) >> 8;
+	hdr->type = type;
+	hdr->dlen2 = dlen & 0xff;
+	hdr->cs = p[0] + p[1] + p[2];
+}
+
+static inline void
+mtk_make_wmt_hdr(struct mtk_wmt_hdr *hdr, u8 op, u16 plen, u8 flag)
+{
+	hdr->dir = 1;
+	hdr->op = op;
+	hdr->dlen = cpu_to_le16(plen + 1);
+	hdr->flag = flag;
+}
+
+#if IS_ENABLED(CONFIG_BT_HCIBTUART_MTK)
+
+void *mtk_btuart_init(struct device *dev);
+int mtk_btuart_setup(struct hci_dev *hdev);
+int mtk_btuart_shutdown(struct hci_dev *hdev);
+int mtk_btuart_send(struct hci_dev *hdev, struct sk_buff *skb);
+int mtk_btuart_hci_frame(struct hci_dev *hdev, struct sk_buff *skb);
+int mtk_btuart_recv(struct hci_dev *hdev, const u8 *data, size_t count);
+
+#else
+
+static void *mtk_btuart_init(struct device *dev)
+{
+	return 0;
+}
+
+static inline int mtk_btuart_setup(struct hci_dev *hdev)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int mtk_btuart_shutdown(struct hci_dev *hdev)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int mtk_btuart_send(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	return -EOPNOTSUPP;
+}
+
+static int mtk_btuart_hci_frame(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int mtk_btuart_recv(struct hci_dev *hdev, const u8 *data,
+				  size_t count)
+{
+	return -EOPNOTSUPP;
+}
+
+#endif
diff --git a/drivers/bluetooth/btuart.c b/drivers/bluetooth/btuart.c
index 65d0086..2e715a5 100644
--- a/drivers/bluetooth/btuart.c
+++ b/drivers/bluetooth/btuart.c
@@ -35,6 +35,7 @@
 #include "h4_recv.h"
 #include "btuart.h"
 #include "btbcm.h"
+#include "btmtkuart.h"
 
 #define VERSION "1.0"
 
@@ -396,6 +397,12 @@ static const struct h4_recv_pkt bcm_recv_pkts[] = {
 	{ BCM_RECV_NULL,    .recv = hci_recv_diag  },
 };
 
+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 = mtk_btuart_hci_frame },
+};
+
 static const struct btuart_vnd bcm_vnd = {
 	.recv_pkts	= bcm_recv_pkts,
 	.recv_pkts_cnt	= ARRAY_SIZE(bcm_recv_pkts),
@@ -403,6 +410,16 @@ static const struct btuart_vnd bcm_vnd = {
 	.setup		= bcm_setup,
 };
 
+static const struct btuart_vnd mtk_vnd = {
+	.recv_pkts	= mtk_recv_pkts,
+	.recv_pkts_cnt	= ARRAY_SIZE(mtk_recv_pkts),
+	.init		= mtk_btuart_init,
+	.setup		= mtk_btuart_setup,
+	.shutdown	= mtk_btuart_shutdown,
+	.send		= mtk_btuart_send,
+	.recv		= mtk_btuart_recv,
+};
+
 static const struct h4_recv_pkt default_recv_pkts[] = {
 	{ H4_RECV_ACL,      .recv = hci_recv_frame },
 	{ H4_RECV_SCO,      .recv = hci_recv_frame },
@@ -487,6 +504,7 @@ static void btuart_remove(struct serdev_device *serdev)
 #ifdef CONFIG_OF
 static const struct of_device_id btuart_of_match_table[] = {
 	{ .compatible = "brcm,bcm43438-bt", .data = &bcm_vnd },
+	{ .compatible = "mediatek,mt7622-bluetooth", .data = &mtk_vnd },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, btuart_of_match_table);
-- 
2.7.4

[PATCH v5 7/7] MAINTAINERS: add an entry for MediaTek Bluetooth driver

[sean.wang at mediatek.com]

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 2ad887b..c3d5c2e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9044,6 +9044,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 at vger.kernel.org
+L:	linux-mediatek at 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

[PATCH v5 1/7] dt-bindings: net: bluetooth: Add mediatek-bluetooth

[Marcel Holtmann]

Hi Sean,

> 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..1335429
> --- /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 one of
> +		  "mediatek,mt7622-bluetooth"": for MT7622 SoC

this does not match with the example below. And one of, should be normally be a list.

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

Regards

Marcel

[PATCH v5 2/7] serdev: add dev_pm_domain_attach|detach()

[Marcel Holtmann]

Hi Sean,

> In order to open up the required power gate before any operation can be
> effectively performed over the serial bus between CPU and serdev, it's
> clearly essential to add common attach functions for PM domains to serdev
> at the probe phase.
> 
> Similarly, the relevant dettach function for the PM domains should be
> properly and reversely added at the remove phase.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: linux-serial at vger.kernel.org
> ---
> drivers/tty/serdev/core.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)

can we take this through the serial subsystem? Or should I just take it when the driver is ready to be included?

Regards

Marcel

[PATCH v5 6/7] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

> This adds a driver to run on the top of btuart 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 | 352 ++++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btmtkuart.h | 116 ++++++++++++++
> drivers/bluetooth/btuart.c    |  18 +++
> 5 files changed, 499 insertions(+)
> create mode 100644 drivers/bluetooth/btmtkuart.c
> create mode 100644 drivers/bluetooth/btmtkuart.h
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 00fdf5f..4d7d640 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -85,6 +85,17 @@ config BT_HCIBTUART
> 	  Say Y here to compile support for Bluetooth UART devices into the
> 	  kernel or say M to compile it as module (btuart).
> 
> +config BT_HCIBTUART_MTK
> +	tristate "MediaTek HCI UART driver"
> +	depends on BT_HCIBTUART
> +	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 60a19cb..c9a8926 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -26,6 +26,8 @@ obj-$(CONFIG_BT_BCM)		+= btbcm.o
> obj-$(CONFIG_BT_RTL)		+= btrtl.o
> obj-$(CONFIG_BT_QCA)		+= btqca.o
> 
> +obj-$(CONFIG_BT_HCIBTUART_MTK)	+= 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..9eed21c
> --- /dev/null
> +++ b/drivers/bluetooth/btmtkuart.c
> @@ -0,0 +1,352 @@
> +// 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/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/serdev.h>
> +
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include "btmtkuart.h"
> +#include "btuart.h"
> +#include "h4_recv.h"
> +
> +static void mtk_stp_reset(struct mtk_stp_splitter *sp)
> +{
> +	sp->cursor = 2;
> +	sp->dlen = 0;
> +}
> +
> +static const unsigned char *
> +mtk_stp_split(struct btuart_dev *bdev, struct mtk_stp_splitter *sp,
> +	      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 (!sp->dlen && sp->cursor >= 6)
> +		sp->cursor = 0;
> +
> +	/* Filling pad until all STP info is obtained. */
> +	while (sp->cursor < 6 && count > 0) {
> +		sp->pad[sp->cursor] = *data;
> +		sp->cursor++;
> +		data++;
> +		count--;
> +	}
> +
> +	/* Retrieve STP info and have a sanity check. */
> +	if (!sp->dlen && sp->cursor >= 6) {
> +		shdr = (struct mtk_stp_hdr *)&sp->pad[2];
> +		sp->dlen = shdr->dlen1 << 8 | shdr->dlen2;
> +
> +		/* Resync STP when unexpected data is being read. */
> +		if (shdr->prefix != 0x80 || sp->dlen > 2048) {
> +			bt_dev_err(bdev->hdev, "stp format unexpect (%d, %d)",
> +				   shdr->prefix, sp->dlen);
> +			mtk_stp_reset(sp);
> +		}
> +	}
> +
> +	/* 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, sp->dlen);
> +	/* Update the remaining size of STP packet. */
> +	sp->dlen -= *sz_h4;
> +
> +	/* Data points to STP payload which can be handled by H4. */
> +	return data;
> +}
> +
> +static int mtk_stp_send(struct btuart_dev *bdev, struct sk_buff *skb)
> +{
> +	struct mtk_stp_hdr *shdr;
> +	struct sk_buff *new_skb;
> +	int dlen;
> +
> +	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));
> +	mtk_make_stp_hdr(shdr, 0, dlen);
> +	skb_put_zero(skb, MTK_STP_TLR_SIZE);
> +
> +	skb_queue_tail(&bdev->txq, skb);
> +
> +	return 0;
> +}
> +
> +static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 opcode, 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;
> +	mtk_make_wmt_hdr(hdr, opcode, plen, 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);

you have two spaces between = and __hci..
> +
> +	if (IS_ERR(skb)) {
> +		int err = PTR_ERR(skb);
> +
> +		bt_dev_err(hdev, "Failed to send wmt cmd (%d)\n", err);

No \n here since bt_dev_err already adds it.

> +		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;
> +
> +	while (fw_size > 0) {
> +		dlen = min_t(int, 250, fw_size);
> +
> +		/* Tell deivice the position in sequence. */
> +		flag = (fw_size - dlen <= 0) ? 3 :
> +		       (fw_size < fw->size - 30) ? 2 : 1;

Use an if statement here. It is easier to read.

> +
> +		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;
> +}
> +
> +void *mtk_btuart_init(struct device *dev)
> +{
> +	struct mtk_bt_dev *soc;
> +
> +	soc = devm_kzalloc(dev, sizeof(*soc), GFP_KERNEL);
> +	if (!soc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	soc->sp = devm_kzalloc(dev, sizeof(*soc->sp), GFP_KERNEL);
> +	if (!soc->sp)
> +		return ERR_PTR(-ENOMEM);
> +
> +	soc->clk = devm_clk_get(dev, "ref");
> +	if (IS_ERR(soc->clk))
> +		return ERR_CAST(soc->clk);
> +
> +	return soc;
> +}
> +EXPORT_SYMBOL_GPL(mtk_btuart_init);
> +
> +int mtk_btuart_send(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	struct btuart_dev *bdev = hci_get_drvdata(hdev);
> +
> +	return mtk_stp_send(bdev, skb);
> +}
> +EXPORT_SYMBOL_GPL(mtk_btuart_send);
> +
> +int mtk_btuart_hci_frame(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);
> +}
> +EXPORT_SYMBOL_GPL(mtk_btuart_hci_frame);
> +
> +int mtk_btuart_recv(struct hci_dev *hdev, const u8 *data, size_t count)
> +{
> +	struct btuart_dev *bdev = hci_get_drvdata(hdev);
> +	const unsigned char *p_left = data, *p_h4;
> +	const struct btuart_vnd *vnd = bdev->vnd;
> +	struct mtk_bt_dev *soc = bdev->data;
> +	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 don'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 and whose length recorded in STP header can
> +		 *  shows up the most length the H:4 engine can handle in one
> +		 *  time.
> +		 */
> +
> +		p_h4 = mtk_stp_split(bdev, soc->sp, 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, vnd->recv_pkts,
> +					   vnd->recv_pkts_cnt);
> +		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;
> +}
> +EXPORT_SYMBOL_GPL(mtk_btuart_recv);
> +
> +int mtk_btuart_setup(struct hci_dev *hdev)
> +{
> +	struct btuart_dev *bdev = hci_get_drvdata(hdev);
> +	struct mtk_bt_dev *soc = bdev->data;
> +	struct device *dev;
> +	u8 param = 0x1;
> +	int err = 0;
> +
> +	dev = &bdev->serdev->dev;
> +
> +	mtk_stp_reset(soc->sp);
> +
> +	/* 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(soc->clk);
> +	if (err < 0)
> +		goto err_put_rpm;
> +
> +	/* Setup a firmware which the device definitely requires. */
> +	err = mtk_setup_fw(hdev);
> +	if (err < 0)
> +		goto err_clk;
> +
> +	/* Activate funciton the firmware providing to. */
> +	err = mtk_hci_wmt_sync(hdev, MTK_WMT_RST, 0x4, 0, 0);
> +	if (err < 0)
> +		goto err_clk;
> +
> +	/* Enable Bluetooth protocol. */
> +	err = mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param),
> +			       &param);
> +	if (err < 0)
> +		goto err_clk;
> +
> +	set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
> +
> +	return 0;
> +err_clk:
> +	clk_disable_unprepare(soc->clk);
> +err_put_rpm:
> +	pm_runtime_put_sync(dev);
> +err_disable_rpm:
> +	pm_runtime_disable(dev);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(mtk_btuart_setup);
> +
> +int mtk_btuart_shutdown(struct hci_dev *hdev)
> +{
> +	struct btuart_dev *bdev = hci_get_drvdata(hdev);
> +	struct device *dev = &bdev->serdev->dev;
> +	struct mtk_bt_dev *soc = bdev->data;
> +	u8 param = 0x0;
> +
> +	/* Disable the device. */
> +	mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param), &param);
> +
> +	/* Shutdown the clock and power domain the device requires. */
> +	clk_disable_unprepare(soc->clk);
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mtk_btuart_shutdown);
> +
> +MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
> +MODULE_DESCRIPTION("Bluetooth Support for MediaTek Serial Devices");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/bluetooth/btmtkuart.h b/drivers/bluetooth/btmtkuart.h
> new file mode 100644
> index 0000000..4c2c24e
> --- /dev/null
> +++ b/drivers/bluetooth/btmtkuart.h
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + *
> + * Bluetooth support for MediaTek serial devices
> + *
> + * Author: Sean Wang <sean.wang@mediatek.com>
> + *
> + */
> +
> +#define FIRMWARE_MT7622		"mediatek/mt7622pr2h.bin"
> +
> +#define MTK_STP_TLR_SIZE	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 mtk_stp_splitter {
> +	u8	pad[6];
> +	u8	cursor;
> +	u16	dlen;
> +};
> +
> +struct mtk_bt_dev {
> +	struct clk *clk;
> +	struct completion wmt_cmd;
> +	struct mtk_stp_splitter *sp;
> +};
> +
> +static inline void
> +mtk_make_stp_hdr(struct mtk_stp_hdr *hdr, u8 type, u32 dlen)
> +{
> +	u8 *p = (u8 *)hdr;
> +
> +	hdr->prefix = 0x80;
> +	hdr->dlen1 = (dlen & 0xf00) >> 8;
> +	hdr->type = type;
> +	hdr->dlen2 = dlen & 0xff;
> +	hdr->cs = p[0] + p[1] + p[2];
> +}
> +
> +static inline void
> +mtk_make_wmt_hdr(struct mtk_wmt_hdr *hdr, u8 op, u16 plen, u8 flag)
> +{
> +	hdr->dir = 1;
> +	hdr->op = op;
> +	hdr->dlen = cpu_to_le16(plen + 1);
> +	hdr->flag = flag;
> +}
> +
> +#if IS_ENABLED(CONFIG_BT_HCIBTUART_MTK)
> +
> +void *mtk_btuart_init(struct device *dev);
> +int mtk_btuart_setup(struct hci_dev *hdev);
> +int mtk_btuart_shutdown(struct hci_dev *hdev);
> +int mtk_btuart_send(struct hci_dev *hdev, struct sk_buff *skb);
> +int mtk_btuart_hci_frame(struct hci_dev *hdev, struct sk_buff *skb);
> +int mtk_btuart_recv(struct hci_dev *hdev, const u8 *data, size_t count);
> +
> +#else
> +
> +static void *mtk_btuart_init(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static inline int mtk_btuart_setup(struct hci_dev *hdev)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int mtk_btuart_shutdown(struct hci_dev *hdev)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int mtk_btuart_send(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int mtk_btuart_hci_frame(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int mtk_btuart_recv(struct hci_dev *hdev, const u8 *data,
> +				  size_t count)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +#endif
> diff --git a/drivers/bluetooth/btuart.c b/drivers/bluetooth/btuart.c
> index 65d0086..2e715a5 100644
> --- a/drivers/bluetooth/btuart.c
> +++ b/drivers/bluetooth/btuart.c
> @@ -35,6 +35,7 @@
> #include "h4_recv.h"
> #include "btuart.h"
> #include "btbcm.h"
> +#include "btmtkuart.h"
> 
> #define VERSION "1.0"
> 
> @@ -396,6 +397,12 @@ static const struct h4_recv_pkt bcm_recv_pkts[] = {
> 	{ BCM_RECV_NULL,    .recv = hci_recv_diag  },
> };
> 
> +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 = mtk_btuart_hci_frame },
> +};
> +
> static const struct btuart_vnd bcm_vnd = {
> 	.recv_pkts	= bcm_recv_pkts,
> 	.recv_pkts_cnt	= ARRAY_SIZE(bcm_recv_pkts),
> @@ -403,6 +410,16 @@ static const struct btuart_vnd bcm_vnd = {
> 	.setup		= bcm_setup,
> };
> 
> +static const struct btuart_vnd mtk_vnd = {
> +	.recv_pkts	= mtk_recv_pkts,
> +	.recv_pkts_cnt	= ARRAY_SIZE(mtk_recv_pkts),
> +	.init		= mtk_btuart_init,
> +	.setup		= mtk_btuart_setup,
> +	.shutdown	= mtk_btuart_shutdown,
> +	.send		= mtk_btuart_send,
> +	.recv		= mtk_btuart_recv,
> +};
> +
> static const struct h4_recv_pkt default_recv_pkts[] = {
> 	{ H4_RECV_ACL,      .recv = hci_recv_frame },
> 	{ H4_RECV_SCO,      .recv = hci_recv_frame },
> @@ -487,6 +504,7 @@ static void btuart_remove(struct serdev_device *serdev)
> #ifdef CONFIG_OF
> static const struct of_device_id btuart_of_match_table[] = {
> 	{ .compatible = "brcm,bcm43438-bt", .data = &bcm_vnd },
> +	{ .compatible = "mediatek,mt7622-bluetooth", .data = &mtk_vnd },
> 	{ }
> };
> MODULE_DEVICE_TABLE(of, btuart_of_match_table);

Regards

Marcel

[PATCH v5 4/7] Bluetooth: Add new quirk for non-persistent setup settings

[Marcel Holtmann]

Hi Sean,

> Add a new quirk HCI_QUIRK_NON_PERSISTENT_SETUP allowing that a quirk that
> runs setup() after every open() and not just after the first open().
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
> include/net/bluetooth/hci.h | 9 +++++++++
> net/bluetooth/hci_core.c    | 3 ++-
> 2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 73e48be..d3ec5b2a8 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -183,6 +183,15 @@ enum {
> 	 * during the hdev->setup vendor callback.
> 	 */
> 	HCI_QUIRK_NON_PERSISTENT_DIAG,
> +
> +	/* When this quirk is set, setup() would be run after every
> +	 * open() and not just after the first open().
> +	 *
> +	 * This quirk can be set before hci_register_dev is called or
> +	 * during the hdev->setup vendor callback.
> +	 *
> +	 */
> +	HCI_QUIRK_NON_PERSISTENT_SETUP,
> };
> 
> /* HCI device flags */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index f5c21004..0111280 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1396,7 +1396,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> 	atomic_set(&hdev->cmd_cnt, 1);
> 	set_bit(HCI_INIT, &hdev->flags);
> 
> -	if (hci_dev_test_flag(hdev, HCI_SETUP)) {
> +	if (hci_dev_test_flag(hdev, HCI_SETUP) ||
> +	    test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
> 		hci_sock_dev_event(hdev, HCI_DEV_SETUP);

can you include the extract of btmon on how the HCI_DEV_SETUP event shows up in the traces? I remember that I asked about something like that.

Regards

Marcel

[PATCH v5 5/7] Bluetooth: Extend btuart driver for join more vendor devices

[Marcel Holtmann]

Hi Sean,

> Adding an independent btuart.h header allows these essential definitions
> can be reused in vendor driver. Also, struct btuart_vnd is extended with
> additional callbacks such as .init initializing vendor data, .shtudown,
> .recv and .send supporting SoC specific framing for that btuart can
> simply adapt to various Bluetooth uart-based devices.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
> drivers/bluetooth/btuart.c | 73 ++++++++++++++++++++++++----------------------
> drivers/bluetooth/btuart.h | 30 +++++++++++++++++++
> 2 files changed, 68 insertions(+), 35 deletions(-)
> create mode 100644 drivers/bluetooth/btuart.h
> 
> diff --git a/drivers/bluetooth/btuart.c b/drivers/bluetooth/btuart.c
> index a900aac..65d0086 100644
> --- a/drivers/bluetooth/btuart.c
> +++ b/drivers/bluetooth/btuart.c
> @@ -33,35 +33,11 @@
> #include <net/bluetooth/hci_core.h>
> 
> #include "h4_recv.h"
> +#include "btuart.h"
> #include "btbcm.h"
> 
> #define VERSION "1.0"
> 
> -struct btuart_vnd {
> -	const struct h4_recv_pkt *recv_pkts;
> -	int recv_pkts_cnt;
> -	unsigned int manufacturer;
> -	int (*open)(struct hci_dev *hdev);
> -	int (*close)(struct hci_dev *hdev);
> -	int (*setup)(struct hci_dev *hdev);
> -};
> -
> -struct btuart_dev {
> -	struct hci_dev *hdev;
> -	struct serdev_device *serdev;
> -
> -	struct work_struct tx_work;
> -	unsigned long tx_state;
> -	struct sk_buff_head txq;
> -
> -	struct sk_buff *rx_skb;
> -
> -	const struct btuart_vnd *vnd;
> -};

I really like to avoid this since it is not clean. Frankly I prefer to keep the btuart.c driver for drivers that really just use H:4 as transport protocol. If the protocol is only H:4 alike and has extra headers, then it should be a separate driver.

The common H:4 handling is abstracted in h4_recv.h already anyway and we can add more pieces if needed. However I also wonder since you have extra framing that the complex H:4 state keeping might be not needed at all. So it could be simplified.

Regards

Marcel

[PATCH v5 1/7] dt-bindings: net: bluetooth: Add mediatek-bluetooth

[Sean Wang]

On Sat, 2018-07-14 at 18:26 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> > 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..1335429
> > --- /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 one of
> > +		  "mediatek,mt7622-bluetooth"": for MT7622 SoC
> 
> this does not match with the example below. And one of, should be normally be a list.
> 

Thanks! I'll remove the words "one of" from the compatible description,
and the extra " being added accidentally.

And the below example fully shows the bluetooth device and its attached
bus (mediatek,mt7622-btif) to let people know clearly how to enable the
bluetooth device.

The current document just describes the bluetooth device and as for the
attached bus, it is already present at
Documentation/devicetree/bindings/serial/8250.txt.

	Sean


> > +- 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 at 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";
> > +		};
> > +	};
> 
> Regards
> 
> Marcel
> 

[SPAM]Re: [PATCH v5 2/7] serdev: add dev_pm_domain_attach|detach()

[Sean Wang]

On Sat, 2018-07-14 at 18:27 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> > In order to open up the required power gate before any operation can be
> > effectively performed over the serial bus between CPU and serdev, it's
> > clearly essential to add common attach functions for PM domains to serdev
> > at the probe phase.
> > 
> > Similarly, the relevant dettach function for the PM domains should be
> > properly and reversely added at the remove phase.
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jiri Slaby <jslaby@suse.com>
> > Cc: linux-serial at vger.kernel.org
> > ---
> > drivers/tty/serdev/core.c | 15 ++++++++++++++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> 
> can we take this through the serial subsystem? Or should I just take it when the driver is ready to be included?
> 
> Regards
> 
> Marcel
> 

I think it's better if the change is taken through serial subsystem
first.

Hi, Rob

do you have any comment? 

	Sean

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

[PATCH v5 6/7] Bluetooth: mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Sat, 2018-07-14 at 18:32 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> > This adds a driver to run on the top of btuart 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 | 352 ++++++++++++++++++++++++++++++++++++++++++
> > drivers/bluetooth/btmtkuart.h | 116 ++++++++++++++
> > drivers/bluetooth/btuart.c    |  18 +++
> > 5 files changed, 499 insertions(+)
> > create mode 100644 drivers/bluetooth/btmtkuart.c
> > create mode 100644 drivers/bluetooth/btmtkuart.h
> > 
> > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > index 00fdf5f..4d7d640 100644
> > --- a/drivers/bluetooth/Kconfig
> > +++ b/drivers/bluetooth/Kconfig
> > @@ -85,6 +85,17 @@ config BT_HCIBTUART
> > 	  Say Y here to compile support for Bluetooth UART devices into the
> > 	  kernel or say M to compile it as module (btuart).
> > 
> > +config BT_HCIBTUART_MTK
> > +	tristate "MediaTek HCI UART driver"
> > +	depends on BT_HCIBTUART
> > +	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 60a19cb..c9a8926 100644
> > --- a/drivers/bluetooth/Makefile
> > +++ b/drivers/bluetooth/Makefile
> > @@ -26,6 +26,8 @@ obj-$(CONFIG_BT_BCM)		+= btbcm.o
> > obj-$(CONFIG_BT_RTL)		+= btrtl.o
> > obj-$(CONFIG_BT_QCA)		+= btqca.o
> > 
> > +obj-$(CONFIG_BT_HCIBTUART_MTK)	+= 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..9eed21c
> > --- /dev/null
> > +++ b/drivers/bluetooth/btmtkuart.c
> > @@ -0,0 +1,352 @@
> > +// 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/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/serdev.h>
> > +
> > +#include <net/bluetooth/bluetooth.h>
> > +#include <net/bluetooth/hci_core.h>
> > +
> > +#include "btmtkuart.h"
> > +#include "btuart.h"
> > +#include "h4_recv.h"
> > +
> > +static void mtk_stp_reset(struct mtk_stp_splitter *sp)
> > +{
> > +	sp->cursor = 2;
> > +	sp->dlen = 0;
> > +}
> > +
> > +static const unsigned char *
> > +mtk_stp_split(struct btuart_dev *bdev, struct mtk_stp_splitter *sp,
> > +	      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 (!sp->dlen && sp->cursor >= 6)
> > +		sp->cursor = 0;
> > +
> > +	/* Filling pad until all STP info is obtained. */
> > +	while (sp->cursor < 6 && count > 0) {
> > +		sp->pad[sp->cursor] = *data;
> > +		sp->cursor++;
> > +		data++;
> > +		count--;
> > +	}
> > +
> > +	/* Retrieve STP info and have a sanity check. */
> > +	if (!sp->dlen && sp->cursor >= 6) {
> > +		shdr = (struct mtk_stp_hdr *)&sp->pad[2];
> > +		sp->dlen = shdr->dlen1 << 8 | shdr->dlen2;
> > +
> > +		/* Resync STP when unexpected data is being read. */
> > +		if (shdr->prefix != 0x80 || sp->dlen > 2048) {
> > +			bt_dev_err(bdev->hdev, "stp format unexpect (%d, %d)",
> > +				   shdr->prefix, sp->dlen);
> > +			mtk_stp_reset(sp);
> > +		}
> > +	}
> > +
> > +	/* 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, sp->dlen);
> > +	/* Update the remaining size of STP packet. */
> > +	sp->dlen -= *sz_h4;
> > +
> > +	/* Data points to STP payload which can be handled by H4. */
> > +	return data;
> > +}
> > +
> > +static int mtk_stp_send(struct btuart_dev *bdev, struct sk_buff *skb)
> > +{
> > +	struct mtk_stp_hdr *shdr;
> > +	struct sk_buff *new_skb;
> > +	int dlen;
> > +
> > +	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));
> > +	mtk_make_stp_hdr(shdr, 0, dlen);
> > +	skb_put_zero(skb, MTK_STP_TLR_SIZE);
> > +
> > +	skb_queue_tail(&bdev->txq, skb);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 opcode, 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;
> > +	mtk_make_wmt_hdr(hdr, opcode, plen, 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);
> 
> you have two spaces between = and __hci..

thanks! it'll be fixed in the next version.


> > +
> > +	if (IS_ERR(skb)) {
> > +		int err = PTR_ERR(skb);
> > +
> > +		bt_dev_err(hdev, "Failed to send wmt cmd (%d)\n", err);
> 
> No \n here since bt_dev_err already adds it.
> 

\n will be removed in the next version

> > +		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;
> > +
> > +	while (fw_size > 0) {
> > +		dlen = min_t(int, 250, fw_size);
> > +
> > +		/* Tell deivice the position in sequence. */
> > +		flag = (fw_size - dlen <= 0) ? 3 :
> > +		       (fw_size < fw->size - 30) ? 2 : 1;
> 
> Use an if statement here. It is easier to read.
> 

thanks, the if statement would be used instead in the next version.

> > +
> > +		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;
> > +}
> > +
> > +void *mtk_btuart_init(struct device *dev)
> > +{
> > +	struct mtk_bt_dev *soc;
> > +
> > +	soc = devm_kzalloc(dev, sizeof(*soc), GFP_KERNEL);
> > +	if (!soc)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	soc->sp = devm_kzalloc(dev, sizeof(*soc->sp), GFP_KERNEL);
> > +	if (!soc->sp)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	soc->clk = devm_clk_get(dev, "ref");
> > +	if (IS_ERR(soc->clk))
> > +		return ERR_CAST(soc->clk);
> > +
> > +	return soc;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_btuart_init);
> > +
> > +int mtk_btuart_send(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +	struct btuart_dev *bdev = hci_get_drvdata(hdev);
> > +
> > +	return mtk_stp_send(bdev, skb);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_btuart_send);
> > +
> > +int mtk_btuart_hci_frame(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);
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_btuart_hci_frame);
> > +
> > +int mtk_btuart_recv(struct hci_dev *hdev, const u8 *data, size_t count)
> > +{
> > +	struct btuart_dev *bdev = hci_get_drvdata(hdev);
> > +	const unsigned char *p_left = data, *p_h4;
> > +	const struct btuart_vnd *vnd = bdev->vnd;
> > +	struct mtk_bt_dev *soc = bdev->data;
> > +	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 don'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 and whose length recorded in STP header can
> > +		 *  shows up the most length the H:4 engine can handle in one
> > +		 *  time.
> > +		 */
> > +
> > +		p_h4 = mtk_stp_split(bdev, soc->sp, 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, vnd->recv_pkts,
> > +					   vnd->recv_pkts_cnt);
> > +		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;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_btuart_recv);
> > +
> > +int mtk_btuart_setup(struct hci_dev *hdev)
> > +{
> > +	struct btuart_dev *bdev = hci_get_drvdata(hdev);
> > +	struct mtk_bt_dev *soc = bdev->data;
> > +	struct device *dev;
> > +	u8 param = 0x1;
> > +	int err = 0;
> > +
> > +	dev = &bdev->serdev->dev;
> > +
> > +	mtk_stp_reset(soc->sp);
> > +
> > +	/* 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(soc->clk);
> > +	if (err < 0)
> > +		goto err_put_rpm;
> > +
> > +	/* Setup a firmware which the device definitely requires. */
> > +	err = mtk_setup_fw(hdev);
> > +	if (err < 0)
> > +		goto err_clk;
> > +
> > +	/* Activate funciton the firmware providing to. */
> > +	err = mtk_hci_wmt_sync(hdev, MTK_WMT_RST, 0x4, 0, 0);
> > +	if (err < 0)
> > +		goto err_clk;
> > +
> > +	/* Enable Bluetooth protocol. */
> > +	err = mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param),
> > +			       &param);
> > +	if (err < 0)
> > +		goto err_clk;
> > +
> > +	set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
> > +
> > +	return 0;
> > +err_clk:
> > +	clk_disable_unprepare(soc->clk);
> > +err_put_rpm:
> > +	pm_runtime_put_sync(dev);
> > +err_disable_rpm:
> > +	pm_runtime_disable(dev);
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_btuart_setup);
> > +
> > +int mtk_btuart_shutdown(struct hci_dev *hdev)
> > +{
> > +	struct btuart_dev *bdev = hci_get_drvdata(hdev);
> > +	struct device *dev = &bdev->serdev->dev;
> > +	struct mtk_bt_dev *soc = bdev->data;
> > +	u8 param = 0x0;
> > +
> > +	/* Disable the device. */
> > +	mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param), &param);
> > +
> > +	/* Shutdown the clock and power domain the device requires. */
> > +	clk_disable_unprepare(soc->clk);
> > +	pm_runtime_put_sync(dev);
> > +	pm_runtime_disable(dev);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_btuart_shutdown);
> > +
> > +MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
> > +MODULE_DESCRIPTION("Bluetooth Support for MediaTek Serial Devices");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/bluetooth/btmtkuart.h b/drivers/bluetooth/btmtkuart.h
> > new file mode 100644
> > index 0000000..4c2c24e
> > --- /dev/null
> > +++ b/drivers/bluetooth/btmtkuart.h
> > @@ -0,0 +1,116 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + *
> > + * Bluetooth support for MediaTek serial devices
> > + *
> > + * Author: Sean Wang <sean.wang@mediatek.com>
> > + *
> > + */
> > +
> > +#define FIRMWARE_MT7622		"mediatek/mt7622pr2h.bin"
> > +
> > +#define MTK_STP_TLR_SIZE	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 mtk_stp_splitter {
> > +	u8	pad[6];
> > +	u8	cursor;
> > +	u16	dlen;
> > +};
> > +
> > +struct mtk_bt_dev {
> > +	struct clk *clk;
> > +	struct completion wmt_cmd;
> > +	struct mtk_stp_splitter *sp;
> > +};
> > +
> > +static inline void
> > +mtk_make_stp_hdr(struct mtk_stp_hdr *hdr, u8 type, u32 dlen)
> > +{
> > +	u8 *p = (u8 *)hdr;
> > +
> > +	hdr->prefix = 0x80;
> > +	hdr->dlen1 = (dlen & 0xf00) >> 8;
> > +	hdr->type = type;
> > +	hdr->dlen2 = dlen & 0xff;
> > +	hdr->cs = p[0] + p[1] + p[2];
> > +}
> > +
> > +static inline void
> > +mtk_make_wmt_hdr(struct mtk_wmt_hdr *hdr, u8 op, u16 plen, u8 flag)
> > +{
> > +	hdr->dir = 1;
> > +	hdr->op = op;
> > +	hdr->dlen = cpu_to_le16(plen + 1);
> > +	hdr->flag = flag;
> > +}
> > +
> > +#if IS_ENABLED(CONFIG_BT_HCIBTUART_MTK)
> > +
> > +void *mtk_btuart_init(struct device *dev);
> > +int mtk_btuart_setup(struct hci_dev *hdev);
> > +int mtk_btuart_shutdown(struct hci_dev *hdev);
> > +int mtk_btuart_send(struct hci_dev *hdev, struct sk_buff *skb);
> > +int mtk_btuart_hci_frame(struct hci_dev *hdev, struct sk_buff *skb);
> > +int mtk_btuart_recv(struct hci_dev *hdev, const u8 *data, size_t count);
> > +
> > +#else
> > +
> > +static void *mtk_btuart_init(struct device *dev)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline int mtk_btuart_setup(struct hci_dev *hdev)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int mtk_btuart_shutdown(struct hci_dev *hdev)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int mtk_btuart_send(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static int mtk_btuart_hci_frame(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int mtk_btuart_recv(struct hci_dev *hdev, const u8 *data,
> > +				  size_t count)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +#endif
> > diff --git a/drivers/bluetooth/btuart.c b/drivers/bluetooth/btuart.c
> > index 65d0086..2e715a5 100644
> > --- a/drivers/bluetooth/btuart.c
> > +++ b/drivers/bluetooth/btuart.c
> > @@ -35,6 +35,7 @@
> > #include "h4_recv.h"
> > #include "btuart.h"
> > #include "btbcm.h"
> > +#include "btmtkuart.h"
> > 
> > #define VERSION "1.0"
> > 
> > @@ -396,6 +397,12 @@ static const struct h4_recv_pkt bcm_recv_pkts[] = {
> > 	{ BCM_RECV_NULL,    .recv = hci_recv_diag  },
> > };
> > 
> > +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 = mtk_btuart_hci_frame },
> > +};
> > +
> > static const struct btuart_vnd bcm_vnd = {
> > 	.recv_pkts	= bcm_recv_pkts,
> > 	.recv_pkts_cnt	= ARRAY_SIZE(bcm_recv_pkts),
> > @@ -403,6 +410,16 @@ static const struct btuart_vnd bcm_vnd = {
> > 	.setup		= bcm_setup,
> > };
> > 
> > +static const struct btuart_vnd mtk_vnd = {
> > +	.recv_pkts	= mtk_recv_pkts,
> > +	.recv_pkts_cnt	= ARRAY_SIZE(mtk_recv_pkts),
> > +	.init		= mtk_btuart_init,
> > +	.setup		= mtk_btuart_setup,
> > +	.shutdown	= mtk_btuart_shutdown,
> > +	.send		= mtk_btuart_send,
> > +	.recv		= mtk_btuart_recv,
> > +};
> > +
> > static const struct h4_recv_pkt default_recv_pkts[] = {
> > 	{ H4_RECV_ACL,      .recv = hci_recv_frame },
> > 	{ H4_RECV_SCO,      .recv = hci_recv_frame },
> > @@ -487,6 +504,7 @@ static void btuart_remove(struct serdev_device *serdev)
> > #ifdef CONFIG_OF
> > static const struct of_device_id btuart_of_match_table[] = {
> > 	{ .compatible = "brcm,bcm43438-bt", .data = &bcm_vnd },
> > +	{ .compatible = "mediatek,mt7622-bluetooth", .data = &mtk_vnd },
> > 	{ }
> > };
> > MODULE_DEVICE_TABLE(of, btuart_of_match_table);
> 
> Regards
> 
> Marcel
> 

[PATCH v5 5/7] Bluetooth: Extend btuart driver for join more vendor devices

[Sean Wang]

On Sat, 2018-07-14 at 18:44 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> > Adding an independent btuart.h header allows these essential definitions
> > can be reused in vendor driver. Also, struct btuart_vnd is extended with
> > additional callbacks such as .init initializing vendor data, .shtudown,
> > .recv and .send supporting SoC specific framing for that btuart can
> > simply adapt to various Bluetooth uart-based devices.
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> > drivers/bluetooth/btuart.c | 73 ++++++++++++++++++++++++----------------------
> > drivers/bluetooth/btuart.h | 30 +++++++++++++++++++
> > 2 files changed, 68 insertions(+), 35 deletions(-)
> > create mode 100644 drivers/bluetooth/btuart.h
> > 
> > diff --git a/drivers/bluetooth/btuart.c b/drivers/bluetooth/btuart.c
> > index a900aac..65d0086 100644
> > --- a/drivers/bluetooth/btuart.c
> > +++ b/drivers/bluetooth/btuart.c
> > @@ -33,35 +33,11 @@
> > #include <net/bluetooth/hci_core.h>
> > 
> > #include "h4_recv.h"
> > +#include "btuart.h"
> > #include "btbcm.h"
> > 
> > #define VERSION "1.0"
> > 
> > -struct btuart_vnd {
> > -	const struct h4_recv_pkt *recv_pkts;
> > -	int recv_pkts_cnt;
> > -	unsigned int manufacturer;
> > -	int (*open)(struct hci_dev *hdev);
> > -	int (*close)(struct hci_dev *hdev);
> > -	int (*setup)(struct hci_dev *hdev);
> > -};
> > -
> > -struct btuart_dev {
> > -	struct hci_dev *hdev;
> > -	struct serdev_device *serdev;
> > -
> > -	struct work_struct tx_work;
> > -	unsigned long tx_state;
> > -	struct sk_buff_head txq;
> > -
> > -	struct sk_buff *rx_skb;
> > -
> > -	const struct btuart_vnd *vnd;
> > -};
> 
> I really like to avoid this since it is not clean. Frankly I prefer to keep the btuart.c driver for drivers that really just use H:4 as transport protocol. If the protocol is only H:4 alike and has extra headers, then it should be a separate driver.
> 

thanks for letting me know your concern. I think I'm a little over in reusing these existing methods and break something generic and its beauty.

I'll make the driver be a separate one based on btuart in the next version.


> The common H:4 handling is abstracted in h4_recv.h already anyway and we can add more pieces if needed. However I also wonder since you have extra framing that the complex H:4 state keeping might be not needed at all. So it could be simplified.
> 

okay, I will get rid of h4_recv.h and consider to use more simplified logic to handle the extra framing and its payload.

> Regards
> 
> Marcel


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

[SPAM]Re: [PATCH v5 2/7] serdev: add dev_pm_domain_attach|detach()

[Greg Kroah-Hartman]

On Sun, Jul 15, 2018 at 01:29:55PM +0800, Sean Wang wrote:
> On Sat, 2018-07-14 at 18:27 +0200, Marcel Holtmann wrote:
> > Hi Sean,
> > 
> > > In order to open up the required power gate before any operation can be
> > > effectively performed over the serial bus between CPU and serdev, it's
> > > clearly essential to add common attach functions for PM domains to serdev
> > > at the probe phase.
> > > 
> > > Similarly, the relevant dettach function for the PM domains should be
> > > properly and reversely added at the remove phase.
> > > 
> > > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > Cc: Rob Herring <robh@kernel.org>
> > > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Jiri Slaby <jslaby@suse.com>
> > > Cc: linux-serial at vger.kernel.org
> > > ---
> > > drivers/tty/serdev/core.c | 15 ++++++++++++++-
> > > 1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > can we take this through the serial subsystem? Or should I just take it when the driver is ready to be included?
> > 
> > Regards
> > 
> > Marcel
> > 
> 
> I think it's better if the change is taken through serial subsystem
> first.
> 
> Hi, Rob
> 
> do you have any comment? 

Yeah, I've been ignoring it to wait to get an ack from someone...

greg k-h

[PATCH v5 2/7] serdev: add dev_pm_domain_attach|detach()

[Johan Hovold]

On Mon, Jul 09, 2018 at 11:56:58PM +0800, sean.wang at mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> In order to open up the required power gate before any operation can be
> effectively performed over the serial bus between CPU and serdev, it's
> clearly essential to add common attach functions for PM domains to serdev
> at the probe phase.
> 
> Similarly, the relevant dettach function for the PM domains should be
> properly and reversely added at the remove phase.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: linux-serial at vger.kernel.org

Reviewed-by: Johan Hovold <johan@kernel.org>

[PATCH v5 2/7] serdev: add dev_pm_domain_attach|detach()

[Greg Kroah-Hartman]

On Sun, Jul 15, 2018 at 10:56:28AM +0200, Johan Hovold wrote:
> On Mon, Jul 09, 2018 at 11:56:58PM +0800, sean.wang at mediatek.com wrote:
> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > In order to open up the required power gate before any operation can be
> > effectively performed over the serial bus between CPU and serdev, it's
> > clearly essential to add common attach functions for PM domains to serdev
> > at the probe phase.
> > 
> > Similarly, the relevant dettach function for the PM domains should be
> > properly and reversely added at the remove phase.
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Ulf Hansson <ulf.hansson@linaro.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jiri Slaby <jslaby@suse.com>
> > Cc: linux-serial at vger.kernel.org
> 
> Reviewed-by: Johan Hovold <johan@kernel.org>

Thanks, now applied.

greg k-h

[PATCH v5 4/7] Bluetooth: Add new quirk for non-persistent setup settings

[Marcel Holtmann]

Hi Sean,

>>> Add a new quirk HCI_QUIRK_NON_PERSISTENT_SETUP allowing that a quirk that
>>> runs setup() after every open() and not just after the first open().
>>> 
>>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
>>> ---
>>> include/net/bluetooth/hci.h | 9 +++++++++
>>> net/bluetooth/hci_core.c    | 3 ++-
>>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>> index 73e48be..d3ec5b2a8 100644
>>> --- a/include/net/bluetooth/hci.h
>>> +++ b/include/net/bluetooth/hci.h
>>> @@ -183,6 +183,15 @@ enum {
>>> 	 * during the hdev->setup vendor callback.
>>> 	 */
>>> 	HCI_QUIRK_NON_PERSISTENT_DIAG,
>>> +
>>> +	/* When this quirk is set, setup() would be run after every
>>> +	 * open() and not just after the first open().
>>> +	 *
>>> +	 * This quirk can be set before hci_register_dev is called or
>>> +	 * during the hdev->setup vendor callback.
>>> +	 *
>>> +	 */
>>> +	HCI_QUIRK_NON_PERSISTENT_SETUP,
>>> };
>>> 
>>> /* HCI device flags */
>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>> index f5c21004..0111280 100644
>>> --- a/net/bluetooth/hci_core.c
>>> +++ b/net/bluetooth/hci_core.c
>>> @@ -1396,7 +1396,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
>>> 	atomic_set(&hdev->cmd_cnt, 1);
>>> 	set_bit(HCI_INIT, &hdev->flags);
>>> 
>>> -	if (hci_dev_test_flag(hdev, HCI_SETUP)) {
>>> +	if (hci_dev_test_flag(hdev, HCI_SETUP) ||
>>> +	    test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>>> 		hci_sock_dev_event(hdev, HCI_DEV_SETUP);
>> 
>> can you include the extract of btmon on how the HCI_DEV_SETUP event shows up in the traces? I remember that I asked about something like that.
>> 
>> Regards
>> 
>> Marcel
>> 
> 
> No, I cannot see any event about HCI_DEV_SETUP in btmon trace, the trace I posted as below is doing some rounds of power off and then on

it will result in this:

        case HCI_DEV_SETUP:
                if (hdev->manufacturer == 0xffff)
                        return NULL;
                                                                                 
        case HCI_DEV_UP:
                skb = bt_skb_alloc(HCI_MON_INDEX_INFO_SIZE, GFP_ATOMIC);

So we will see extra index info events, but I assume that is just fine this we also see them on DEV_UP. They also do not hurt as long as not magically the manufacturer information changes.

I do wonder though if this quirk is set, then hdev->manufacturer needs to be reset and allow hdev->setup() to reset it. This goes with a log of other information as well. Maybe just a look if there are no races here.

Regards

Marcel

[PATCH v5 5/7] Bluetooth: Extend btuart driver for join more vendor devices

[Marcel Holtmann]

Hi Sean,

>>> Adding an independent btuart.h header allows these essential definitions
>>> can be reused in vendor driver. Also, struct btuart_vnd is extended with
>>> additional callbacks such as .init initializing vendor data, .shtudown,
>>> .recv and .send supporting SoC specific framing for that btuart can
>>> simply adapt to various Bluetooth uart-based devices.
>>> 
>>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
>>> ---
>>> drivers/bluetooth/btuart.c | 73 ++++++++++++++++++++++++----------------------
>>> drivers/bluetooth/btuart.h | 30 +++++++++++++++++++
>>> 2 files changed, 68 insertions(+), 35 deletions(-)
>>> create mode 100644 drivers/bluetooth/btuart.h
>>> 
>>> diff --git a/drivers/bluetooth/btuart.c b/drivers/bluetooth/btuart.c
>>> index a900aac..65d0086 100644
>>> --- a/drivers/bluetooth/btuart.c
>>> +++ b/drivers/bluetooth/btuart.c
>>> @@ -33,35 +33,11 @@
>>> #include <net/bluetooth/hci_core.h>
>>> 
>>> #include "h4_recv.h"
>>> +#include "btuart.h"
>>> #include "btbcm.h"
>>> 
>>> #define VERSION "1.0"
>>> 
>>> -struct btuart_vnd {
>>> -	const struct h4_recv_pkt *recv_pkts;
>>> -	int recv_pkts_cnt;
>>> -	unsigned int manufacturer;
>>> -	int (*open)(struct hci_dev *hdev);
>>> -	int (*close)(struct hci_dev *hdev);
>>> -	int (*setup)(struct hci_dev *hdev);
>>> -};
>>> -
>>> -struct btuart_dev {
>>> -	struct hci_dev *hdev;
>>> -	struct serdev_device *serdev;
>>> -
>>> -	struct work_struct tx_work;
>>> -	unsigned long tx_state;
>>> -	struct sk_buff_head txq;
>>> -
>>> -	struct sk_buff *rx_skb;
>>> -
>>> -	const struct btuart_vnd *vnd;
>>> -};
>> 
>> I really like to avoid this since it is not clean. Frankly I prefer to keep the btuart.c driver for drivers that really just use H:4 as transport protocol. If the protocol is only H:4 alike and has extra headers, then it should be a separate driver.
>> 
> 
> thanks for letting me know your concern. I think I'm a little over in reusing these existing methods and break something generic and its beauty.
> 
> I'll make the driver be a separate one based on btuart in the next version.
> 
> 
>> The common H:4 handling is abstracted in h4_recv.h already anyway and we can add more pieces if needed. However I also wonder since you have extra framing that the complex H:4 state keeping might be not needed at all. So it could be simplified.
>> 
> 
> okay, I will get rid of h4_recv.h and consider to use more simplified logic to handle the extra framing and its payload.

only if it is already framed by your extra header, but I seem to recall that it will frame it. For example the bfusb.c old driver does frame H:4 as well and then no extra state keeping for H:4 is needed. Just pass the frames up into the core via hci_recv_frame (or hci_recv_diag for vendor diagnostic) and move on with it.

You can of course use h4_recv.h if you need to, but only do that if you are true H:4 stream and have no idea where a frame ends and starts.

Regards

Marcel

[PATCH v5 5/7] Bluetooth: Extend btuart driver for join more vendor devices

[Sean Wang]

On Mon, 2018-07-16 at 14:59 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>> Adding an independent btuart.h header allows these essential definitions
> >>> can be reused in vendor driver. Also, struct btuart_vnd is extended with
> >>> additional callbacks such as .init initializing vendor data, .shtudown,
> >>> .recv and .send supporting SoC specific framing for that btuart can
> >>> simply adapt to various Bluetooth uart-based devices.
> >>> 
> >>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> >>> ---
> >>> drivers/bluetooth/btuart.c | 73 ++++++++++++++++++++++++----------------------
> >>> drivers/bluetooth/btuart.h | 30 +++++++++++++++++++
> >>> 2 files changed, 68 insertions(+), 35 deletions(-)
> >>> create mode 100644 drivers/bluetooth/btuart.h
> >>> 
> >>> diff --git a/drivers/bluetooth/btuart.c b/drivers/bluetooth/btuart.c
> >>> index a900aac..65d0086 100644
> >>> --- a/drivers/bluetooth/btuart.c
> >>> +++ b/drivers/bluetooth/btuart.c
> >>> @@ -33,35 +33,11 @@
> >>> #include <net/bluetooth/hci_core.h>
> >>> 
> >>> #include "h4_recv.h"
> >>> +#include "btuart.h"
> >>> #include "btbcm.h"
> >>> 
> >>> #define VERSION "1.0"
> >>> 
> >>> -struct btuart_vnd {
> >>> -	const struct h4_recv_pkt *recv_pkts;
> >>> -	int recv_pkts_cnt;
> >>> -	unsigned int manufacturer;
> >>> -	int (*open)(struct hci_dev *hdev);
> >>> -	int (*close)(struct hci_dev *hdev);
> >>> -	int (*setup)(struct hci_dev *hdev);
> >>> -};
> >>> -
> >>> -struct btuart_dev {
> >>> -	struct hci_dev *hdev;
> >>> -	struct serdev_device *serdev;
> >>> -
> >>> -	struct work_struct tx_work;
> >>> -	unsigned long tx_state;
> >>> -	struct sk_buff_head txq;
> >>> -
> >>> -	struct sk_buff *rx_skb;
> >>> -
> >>> -	const struct btuart_vnd *vnd;
> >>> -};
> >> 
> >> I really like to avoid this since it is not clean. Frankly I prefer to keep the btuart.c driver for drivers that really just use H:4 as transport protocol. If the protocol is only H:4 alike and has extra headers, then it should be a separate driver.
> >> 
> > 
> > thanks for letting me know your concern. I think I'm a little over in reusing these existing methods and break something generic and its beauty.
> > 
> > I'll make the driver be a separate one based on btuart in the next version.
> > 
> > 
> >> The common H:4 handling is abstracted in h4_recv.h already anyway and we can add more pieces if needed. However I also wonder since you have extra framing that the complex H:4 state keeping might be not needed at all. So it could be simplified.
> >> 
> > 
> > okay, I will get rid of h4_recv.h and consider to use more simplified logic to handle the extra framing and its payload.
> 
> only if it is already framed by your extra header, but I seem to recall that it will frame it. For example the bfusb.c old driver does frame H:4 as well and then no extra state keeping for H:4 is needed. Just pass the frames up into the core via hci_recv_frame (or hci_recv_diag for vendor diagnostic) and move on with it.
> 
> You can of course use h4_recv.h if you need to, but only do that if you are true H:4 stream and have no idea where a frame ends and starts.
> 
> Regards
> 
> Marcel
> 

Thanks for the explanation, I didn't show what the extra header is, that causes some misunderstanding.

The mtk extra header doesn't provide any idea where a frame ends and starts in the bluetooth stream. 

It is just totally a legacy stuff used by mtk combo devices sharing a serial transport, such as BT/GPS/FM running via a shared UART, to
let the host know what type of radio the following bytes is for and how long it's. 

The extra header is really useful for a combo device, splitting flow and fitting in a single serial transport, but for a single device such as
mt7622 Bluetooth I made here, it seems to be useless.

Because the extra header doesn't provide any details about this stream and each radio stack still needs to be in charge of their stream
parsing. That is why I still want to use recv_h4.h instead of coding my own parser.

	Sean

[PATCH v5 4/7] Bluetooth: Add new quirk for non-persistent setup settings

[Sean Wang]

On Mon, 2018-07-16 at 14:56 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>> Add a new quirk HCI_QUIRK_NON_PERSISTENT_SETUP allowing that a quirk that
> >>> runs setup() after every open() and not just after the first open().
> >>> 
> >>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> >>> ---
> >>> include/net/bluetooth/hci.h | 9 +++++++++
> >>> net/bluetooth/hci_core.c    | 3 ++-
> >>> 2 files changed, 11 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> >>> index 73e48be..d3ec5b2a8 100644
> >>> --- a/include/net/bluetooth/hci.h
> >>> +++ b/include/net/bluetooth/hci.h
> >>> @@ -183,6 +183,15 @@ enum {
> >>> 	 * during the hdev->setup vendor callback.
> >>> 	 */
> >>> 	HCI_QUIRK_NON_PERSISTENT_DIAG,
> >>> +
> >>> +	/* When this quirk is set, setup() would be run after every
> >>> +	 * open() and not just after the first open().
> >>> +	 *
> >>> +	 * This quirk can be set before hci_register_dev is called or
> >>> +	 * during the hdev->setup vendor callback.
> >>> +	 *
> >>> +	 */
> >>> +	HCI_QUIRK_NON_PERSISTENT_SETUP,
> >>> };
> >>> 
> >>> /* HCI device flags */
> >>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >>> index f5c21004..0111280 100644
> >>> --- a/net/bluetooth/hci_core.c
> >>> +++ b/net/bluetooth/hci_core.c
> >>> @@ -1396,7 +1396,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> >>> 	atomic_set(&hdev->cmd_cnt, 1);
> >>> 	set_bit(HCI_INIT, &hdev->flags);
> >>> 
> >>> -	if (hci_dev_test_flag(hdev, HCI_SETUP)) {
> >>> +	if (hci_dev_test_flag(hdev, HCI_SETUP) ||
> >>> +	    test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
> >>> 		hci_sock_dev_event(hdev, HCI_DEV_SETUP);
> >> 
> >> can you include the extract of btmon on how the HCI_DEV_SETUP event shows up in the traces? I remember that I asked about something like that.
> >> 
> >> Regards
> >> 
> >> Marcel
> >> 
> > 
> > No, I cannot see any event about HCI_DEV_SETUP in btmon trace, the trace I posted as below is doing some rounds of power off and then on
> 
> it will result in this:
> 
>         case HCI_DEV_SETUP:
>                 if (hdev->manufacturer == 0xffff)
>                         return NULL;
>                                                                                  
>         case HCI_DEV_UP:
>                 skb = bt_skb_alloc(HCI_MON_INDEX_INFO_SIZE, GFP_ATOMIC);
> 
> So we will see extra index info events, but I assume that is just fine this we also see them on DEV_UP. They also do not hurt as long as not magically the manufacturer information changes.
> 
> I do wonder though if this quirk is set, then hdev->manufacturer needs to be reset and allow hdev->setup() to reset it. This goes with a log of other information as well. Maybe just a look if there are no races here.
> 
> Regards
> 
> Marcel
> 

I didn't set a value to hdev->manufacture.

Should I set it in hdev->setup() ? MediaTek is 0x46 according to [1].

[1]
https://www.bluetooth.com/specifications/assigned-numbers/company-identifiers


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

[SPAM]Re: [PATCH v5 4/7] Bluetooth: Add new quirk for non-persistent setup settings

[Sean Wang]

On Tue, 2018-07-17 at 00:05 +0800, Sean Wang wrote:
> On Mon, 2018-07-16 at 14:56 +0200, Marcel Holtmann wrote:
> > Hi Sean,
> > 
> > >>> Add a new quirk HCI_QUIRK_NON_PERSISTENT_SETUP allowing that a quirk that
> > >>> runs setup() after every open() and not just after the first open().
> > >>> 
> > >>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > >>> ---
> > >>> include/net/bluetooth/hci.h | 9 +++++++++
> > >>> net/bluetooth/hci_core.c    | 3 ++-
> > >>> 2 files changed, 11 insertions(+), 1 deletion(-)
> > >>> 
> > >>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > >>> index 73e48be..d3ec5b2a8 100644
> > >>> --- a/include/net/bluetooth/hci.h
> > >>> +++ b/include/net/bluetooth/hci.h
> > >>> @@ -183,6 +183,15 @@ enum {
> > >>> 	 * during the hdev->setup vendor callback.
> > >>> 	 */
> > >>> 	HCI_QUIRK_NON_PERSISTENT_DIAG,
> > >>> +
> > >>> +	/* When this quirk is set, setup() would be run after every
> > >>> +	 * open() and not just after the first open().
> > >>> +	 *
> > >>> +	 * This quirk can be set before hci_register_dev is called or
> > >>> +	 * during the hdev->setup vendor callback.
> > >>> +	 *
> > >>> +	 */
> > >>> +	HCI_QUIRK_NON_PERSISTENT_SETUP,
> > >>> };
> > >>> 
> > >>> /* HCI device flags */
> > >>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > >>> index f5c21004..0111280 100644
> > >>> --- a/net/bluetooth/hci_core.c
> > >>> +++ b/net/bluetooth/hci_core.c
> > >>> @@ -1396,7 +1396,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> > >>> 	atomic_set(&hdev->cmd_cnt, 1);
> > >>> 	set_bit(HCI_INIT, &hdev->flags);
> > >>> 
> > >>> -	if (hci_dev_test_flag(hdev, HCI_SETUP)) {
> > >>> +	if (hci_dev_test_flag(hdev, HCI_SETUP) ||
> > >>> +	    test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
> > >>> 		hci_sock_dev_event(hdev, HCI_DEV_SETUP);
> > >> 
> > >> can you include the extract of btmon on how the HCI_DEV_SETUP event shows up in the traces? I remember that I asked about something like that.
> > >> 
> > >> Regards
> > >> 
> > >> Marcel
> > >> 
> > > 
> > > No, I cannot see any event about HCI_DEV_SETUP in btmon trace, the trace I posted as below is doing some rounds of power off and then on
> > 
> > it will result in this:
> > 
> >         case HCI_DEV_SETUP:
> >                 if (hdev->manufacturer == 0xffff)
> >                         return NULL;
> >                                                                                  
> >         case HCI_DEV_UP:
> >                 skb = bt_skb_alloc(HCI_MON_INDEX_INFO_SIZE, GFP_ATOMIC);
> > 
> > So we will see extra index info events, but I assume that is just fine this we also see them on DEV_UP. They also do not hurt as long as not magically the manufacturer information changes.
> > 
> > I do wonder though if this quirk is set, then hdev->manufacturer needs to be reset and allow hdev->setup() to reset it. This goes with a log of other information as well. Maybe just a look if there are no races here.
> > 
> > Regards
> > 
> > Marcel
> > 
> 
> I didn't set a value to hdev->manufacture.
> 
> Should I set it in hdev->setup() ? MediaTek is 0x46 according to [1].

sorry, something needs be corrected.

what I means is that should I set hdev->manufacture as 0x46 at probe ? 

> [1]
> https://www.bluetooth.com/specifications/assigned-numbers/company-identifiers
> 
> 
> > 
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v5 4/7] Bluetooth: Add new quirk for non-persistent setup settings

[Marcel Holtmann]

Hi Sean,

>>>>> Add a new quirk HCI_QUIRK_NON_PERSISTENT_SETUP allowing that a quirk that
>>>>> runs setup() after every open() and not just after the first open().
>>>>> 
>>>>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
>>>>> ---
>>>>> include/net/bluetooth/hci.h | 9 +++++++++
>>>>> net/bluetooth/hci_core.c    | 3 ++-
>>>>> 2 files changed, 11 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>>>> index 73e48be..d3ec5b2a8 100644
>>>>> --- a/include/net/bluetooth/hci.h
>>>>> +++ b/include/net/bluetooth/hci.h
>>>>> @@ -183,6 +183,15 @@ enum {
>>>>> 	 * during the hdev->setup vendor callback.
>>>>> 	 */
>>>>> 	HCI_QUIRK_NON_PERSISTENT_DIAG,
>>>>> +
>>>>> +	/* When this quirk is set, setup() would be run after every
>>>>> +	 * open() and not just after the first open().
>>>>> +	 *
>>>>> +	 * This quirk can be set before hci_register_dev is called or
>>>>> +	 * during the hdev->setup vendor callback.
>>>>> +	 *
>>>>> +	 */
>>>>> +	HCI_QUIRK_NON_PERSISTENT_SETUP,
>>>>> };
>>>>> 
>>>>> /* HCI device flags */
>>>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>>>> index f5c21004..0111280 100644
>>>>> --- a/net/bluetooth/hci_core.c
>>>>> +++ b/net/bluetooth/hci_core.c
>>>>> @@ -1396,7 +1396,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
>>>>> 	atomic_set(&hdev->cmd_cnt, 1);
>>>>> 	set_bit(HCI_INIT, &hdev->flags);
>>>>> 
>>>>> -	if (hci_dev_test_flag(hdev, HCI_SETUP)) {
>>>>> +	if (hci_dev_test_flag(hdev, HCI_SETUP) ||
>>>>> +	    test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>>>>> 		hci_sock_dev_event(hdev, HCI_DEV_SETUP);
>>>> 
>>>> can you include the extract of btmon on how the HCI_DEV_SETUP event shows up in the traces? I remember that I asked about something like that.
>>>> 
>>>> Regards
>>>> 
>>>> Marcel
>>>> 
>>> 
>>> No, I cannot see any event about HCI_DEV_SETUP in btmon trace, the trace I posted as below is doing some rounds of power off and then on
>> 
>> it will result in this:
>> 
>>        case HCI_DEV_SETUP:
>>                if (hdev->manufacturer == 0xffff)
>>                        return NULL;
>> 
>>        case HCI_DEV_UP:
>>                skb = bt_skb_alloc(HCI_MON_INDEX_INFO_SIZE, GFP_ATOMIC);
>> 
>> So we will see extra index info events, but I assume that is just fine this we also see them on DEV_UP. They also do not hurt as long as not magically the manufacturer information changes.
>> 
>> I do wonder though if this quirk is set, then hdev->manufacturer needs to be reset and allow hdev->setup() to reset it. This goes with a log of other information as well. Maybe just a look if there are no races here.
>> 
>> Regards
>> 
>> Marcel
>> 
> 
> I didn't set a value to hdev->manufacture.
> 
> Should I set it in hdev->setup() ? MediaTek is 0x46 according to [1].

it is generally useful so that it gets reported out in btmon and thus it can assign the correct vendor specific decoder. However you better set hdev->manufacturer = 70 in the probe() routine before calling hci_register_dev. You have a dedicated driver and thus will always know it.

The core tries to figure this out at some point, but then you are missing btmon decoding for the earlier messages. See btusb.c and how other drivers set it early on.

Regards

Marcel

[PATCH v5 5/7] Bluetooth: Extend btuart driver for join more vendor devices

[Marcel Holtmann]

Hi Sean,

>>>>> Adding an independent btuart.h header allows these essential definitions
>>>>> can be reused in vendor driver. Also, struct btuart_vnd is extended with
>>>>> additional callbacks such as .init initializing vendor data, .shtudown,
>>>>> .recv and .send supporting SoC specific framing for that btuart can
>>>>> simply adapt to various Bluetooth uart-based devices.
>>>>> 
>>>>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
>>>>> ---
>>>>> drivers/bluetooth/btuart.c | 73 ++++++++++++++++++++++++----------------------
>>>>> drivers/bluetooth/btuart.h | 30 +++++++++++++++++++
>>>>> 2 files changed, 68 insertions(+), 35 deletions(-)
>>>>> create mode 100644 drivers/bluetooth/btuart.h
>>>>> 
>>>>> diff --git a/drivers/bluetooth/btuart.c b/drivers/bluetooth/btuart.c
>>>>> index a900aac..65d0086 100644
>>>>> --- a/drivers/bluetooth/btuart.c
>>>>> +++ b/drivers/bluetooth/btuart.c
>>>>> @@ -33,35 +33,11 @@
>>>>> #include <net/bluetooth/hci_core.h>
>>>>> 
>>>>> #include "h4_recv.h"
>>>>> +#include "btuart.h"
>>>>> #include "btbcm.h"
>>>>> 
>>>>> #define VERSION "1.0"
>>>>> 
>>>>> -struct btuart_vnd {
>>>>> -	const struct h4_recv_pkt *recv_pkts;
>>>>> -	int recv_pkts_cnt;
>>>>> -	unsigned int manufacturer;
>>>>> -	int (*open)(struct hci_dev *hdev);
>>>>> -	int (*close)(struct hci_dev *hdev);
>>>>> -	int (*setup)(struct hci_dev *hdev);
>>>>> -};
>>>>> -
>>>>> -struct btuart_dev {
>>>>> -	struct hci_dev *hdev;
>>>>> -	struct serdev_device *serdev;
>>>>> -
>>>>> -	struct work_struct tx_work;
>>>>> -	unsigned long tx_state;
>>>>> -	struct sk_buff_head txq;
>>>>> -
>>>>> -	struct sk_buff *rx_skb;
>>>>> -
>>>>> -	const struct btuart_vnd *vnd;
>>>>> -};
>>>> 
>>>> I really like to avoid this since it is not clean. Frankly I prefer to keep the btuart.c driver for drivers that really just use H:4 as transport protocol. If the protocol is only H:4 alike and has extra headers, then it should be a separate driver.
>>>> 
>>> 
>>> thanks for letting me know your concern. I think I'm a little over in reusing these existing methods and break something generic and its beauty.
>>> 
>>> I'll make the driver be a separate one based on btuart in the next version.
>>> 
>>> 
>>>> The common H:4 handling is abstracted in h4_recv.h already anyway and we can add more pieces if needed. However I also wonder since you have extra framing that the complex H:4 state keeping might be not needed at all. So it could be simplified.
>>>> 
>>> 
>>> okay, I will get rid of h4_recv.h and consider to use more simplified logic to handle the extra framing and its payload.
>> 
>> only if it is already framed by your extra header, but I seem to recall that it will frame it. For example the bfusb.c old driver does frame H:4 as well and then no extra state keeping for H:4 is needed. Just pass the frames up into the core via hci_recv_frame (or hci_recv_diag for vendor diagnostic) and move on with it.
>> 
>> You can of course use h4_recv.h if you need to, but only do that if you are true H:4 stream and have no idea where a frame ends and starts.
>> 
>> Regards
>> 
>> Marcel
>> 
> 
> Thanks for the explanation, I didn't show what the extra header is, that causes some misunderstanding.
> 
> The mtk extra header doesn't provide any idea where a frame ends and starts in the bluetooth stream. 
> 
> It is just totally a legacy stuff used by mtk combo devices sharing a serial transport, such as BT/GPS/FM running via a shared UART, to
> let the host know what type of radio the following bytes is for and how long it's. 

but that means it is framed. You know ahead of time how long the H:4 packet will be. That is the important part. Unless you tell me that it can fragment a H:4 frame over multiple MTK specific frames.

> The extra header is really useful for a combo device, splitting flow and fitting in a single serial transport, but for a single device such as
> mt7622 Bluetooth I made here, it seems to be useless.

Useless or not depends on what you are going to do with the device. In theory you could write a serdev driver that just handles the multiplexing (since that is what you have) and then hands down the frames to individual drivers. Frankly something like btqcomsmd.c seems to be what you really want here. However first you would needed this multiplex driver that takes the serial stream and breaks it up.

> Because the extra header doesn't provide any details about this stream and each radio stack still needs to be in charge of their stream
> parsing. That is why I still want to use recv_h4.h instead of coding my own parser.

As I said above, if the header + length always indicates a full H:4 frame, then you do not need h4_recv.h since you know the packet size. If it doesn't (and it means things can fragment), then you do.

However I have to note that a serial stream from your multiplexer protocol also needs some state handling since it can be interrupted at any point. If you want this clean, then you actually do that anyway. Essentially you have two protocols layered and want to process the independently.

If you post details about the multiplexing protocol for your serial stream, then I can help you design a driver for it. With serdev that is actually simple. And then you could hook up GPS etc. at some point once you want to run this on hardware that has the combo chip.

Having a Linux Bluetooth driver is useful for Android as well btw. We have HCI_CHANNEL_USER and a generic Android driver for using it. So enabling the chip in Linux upstream will enable it for Android as well.

Regards

Marcel

[PATCH v5 4/7] Bluetooth: Add new quirk for non-persistent setup settings

[Sean Wang]

On Wed, 2018-07-18 at 14:14 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>>>> Add a new quirk HCI_QUIRK_NON_PERSISTENT_SETUP allowing that a quirk that
> >>>>> runs setup() after every open() and not just after the first open().
> >>>>> 
> >>>>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> >>>>> ---
> >>>>> include/net/bluetooth/hci.h | 9 +++++++++
> >>>>> net/bluetooth/hci_core.c    | 3 ++-
> >>>>> 2 files changed, 11 insertions(+), 1 deletion(-)
> >>>>> 
> >>>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> >>>>> index 73e48be..d3ec5b2a8 100644
> >>>>> --- a/include/net/bluetooth/hci.h
> >>>>> +++ b/include/net/bluetooth/hci.h
> >>>>> @@ -183,6 +183,15 @@ enum {
> >>>>> 	 * during the hdev->setup vendor callback.
> >>>>> 	 */
> >>>>> 	HCI_QUIRK_NON_PERSISTENT_DIAG,
> >>>>> +
> >>>>> +	/* When this quirk is set, setup() would be run after every
> >>>>> +	 * open() and not just after the first open().
> >>>>> +	 *
> >>>>> +	 * This quirk can be set before hci_register_dev is called or
> >>>>> +	 * during the hdev->setup vendor callback.
> >>>>> +	 *
> >>>>> +	 */
> >>>>> +	HCI_QUIRK_NON_PERSISTENT_SETUP,
> >>>>> };
> >>>>> 
> >>>>> /* HCI device flags */
> >>>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >>>>> index f5c21004..0111280 100644
> >>>>> --- a/net/bluetooth/hci_core.c
> >>>>> +++ b/net/bluetooth/hci_core.c
> >>>>> @@ -1396,7 +1396,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> >>>>> 	atomic_set(&hdev->cmd_cnt, 1);
> >>>>> 	set_bit(HCI_INIT, &hdev->flags);
> >>>>> 
> >>>>> -	if (hci_dev_test_flag(hdev, HCI_SETUP)) {
> >>>>> +	if (hci_dev_test_flag(hdev, HCI_SETUP) ||
> >>>>> +	    test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
> >>>>> 		hci_sock_dev_event(hdev, HCI_DEV_SETUP);
> >>>> 
> >>>> can you include the extract of btmon on how the HCI_DEV_SETUP event shows up in the traces? I remember that I asked about something like that.
> >>>> 
> >>>> Regards
> >>>> 
> >>>> Marcel
> >>>> 
> >>> 
> >>> No, I cannot see any event about HCI_DEV_SETUP in btmon trace, the trace I posted as below is doing some rounds of power off and then on
> >> 
> >> it will result in this:
> >> 
> >>        case HCI_DEV_SETUP:
> >>                if (hdev->manufacturer == 0xffff)
> >>                        return NULL;
> >> 
> >>        case HCI_DEV_UP:
> >>                skb = bt_skb_alloc(HCI_MON_INDEX_INFO_SIZE, GFP_ATOMIC);
> >> 
> >> So we will see extra index info events, but I assume that is just fine this we also see them on DEV_UP. They also do not hurt as long as not magically the manufacturer information changes.
> >> 
> >> I do wonder though if this quirk is set, then hdev->manufacturer needs to be reset and allow hdev->setup() to reset it. This goes with a log of other information as well. Maybe just a look if there are no races here.
> >> 
> >> Regards
> >> 
> >> Marcel
> >> 
> > 
> > I didn't set a value to hdev->manufacture.
> > 
> > Should I set it in hdev->setup() ? MediaTek is 0x46 according to [1].
> 
> it is generally useful so that it gets reported out in btmon and thus it can assign the correct vendor specific decoder. However you better set hdev->manufacturer = 70 in the probe() routine before calling hci_register_dev. You have a dedicated driver and thus will always know it.
> 
> The core tries to figure this out at some point, but then you are missing btmon decoding for the earlier messages. See btusb.c and how other drivers set it early on.
> 

thank for the point up, I will set hdev->manufacturer as 70 in probe
routine before calling hci_register_dev in the next version.

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

[PATCH v5 5/7] Bluetooth: Extend btuart driver for join more vendor devices

[Sean Wang]

On Wed, 2018-07-18 at 14:23 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>>>> Adding an independent btuart.h header allows these essential definitions
> >>>>> can be reused in vendor driver. Also, struct btuart_vnd is extended with
> >>>>> additional callbacks such as .init initializing vendor data, .shtudown,
> >>>>> .recv and .send supporting SoC specific framing for that btuart can
> >>>>> simply adapt to various Bluetooth uart-based devices.
> >>>>> 
> >>>>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> >>>>> ---
> >>>>> drivers/bluetooth/btuart.c | 73 ++++++++++++++++++++++++----------------------
> >>>>> drivers/bluetooth/btuart.h | 30 +++++++++++++++++++
> >>>>> 2 files changed, 68 insertions(+), 35 deletions(-)
> >>>>> create mode 100644 drivers/bluetooth/btuart.h
> >>>>> 
> >>>>> diff --git a/drivers/bluetooth/btuart.c b/drivers/bluetooth/btuart.c
> >>>>> index a900aac..65d0086 100644
> >>>>> --- a/drivers/bluetooth/btuart.c
> >>>>> +++ b/drivers/bluetooth/btuart.c
> >>>>> @@ -33,35 +33,11 @@
> >>>>> #include <net/bluetooth/hci_core.h>
> >>>>> 
> >>>>> #include "h4_recv.h"
> >>>>> +#include "btuart.h"
> >>>>> #include "btbcm.h"
> >>>>> 
> >>>>> #define VERSION "1.0"
> >>>>> 
> >>>>> -struct btuart_vnd {
> >>>>> -	const struct h4_recv_pkt *recv_pkts;
> >>>>> -	int recv_pkts_cnt;
> >>>>> -	unsigned int manufacturer;
> >>>>> -	int (*open)(struct hci_dev *hdev);
> >>>>> -	int (*close)(struct hci_dev *hdev);
> >>>>> -	int (*setup)(struct hci_dev *hdev);
> >>>>> -};
> >>>>> -
> >>>>> -struct btuart_dev {
> >>>>> -	struct hci_dev *hdev;
> >>>>> -	struct serdev_device *serdev;
> >>>>> -
> >>>>> -	struct work_struct tx_work;
> >>>>> -	unsigned long tx_state;
> >>>>> -	struct sk_buff_head txq;
> >>>>> -
> >>>>> -	struct sk_buff *rx_skb;
> >>>>> -
> >>>>> -	const struct btuart_vnd *vnd;
> >>>>> -};
> >>>> 
> >>>> I really like to avoid this since it is not clean. Frankly I prefer to keep the btuart.c driver for drivers that really just use H:4 as transport protocol. If the protocol is only H:4 alike and has extra headers, then it should be a separate driver.
> >>>> 
> >>> 
> >>> thanks for letting me know your concern. I think I'm a little over in reusing these existing methods and break something generic and its beauty.
> >>> 
> >>> I'll make the driver be a separate one based on btuart in the next version.
> >>> 
> >>> 
> >>>> The common H:4 handling is abstracted in h4_recv.h already anyway and we can add more pieces if needed. However I also wonder since you have extra framing that the complex H:4 state keeping might be not needed at all. So it could be simplified.
> >>>> 
> >>> 
> >>> okay, I will get rid of h4_recv.h and consider to use more simplified logic to handle the extra framing and its payload.
> >> 
> >> only if it is already framed by your extra header, but I seem to recall that it will frame it. For example the bfusb.c old driver does frame H:4 as well and then no extra state keeping for H:4 is needed. Just pass the frames up into the core via hci_recv_frame (or hci_recv_diag for vendor diagnostic) and move on with it.
> >> 
> >> You can of course use h4_recv.h if you need to, but only do that if you are true H:4 stream and have no idea where a frame ends and starts.
> >> 
> >> Regards
> >> 
> >> Marcel
> >> 
> > 
> > Thanks for the explanation, I didn't show what the extra header is, that causes some misunderstanding.
> > 
> > The mtk extra header doesn't provide any idea where a frame ends and starts in the bluetooth stream. 
> > 
> > It is just totally a legacy stuff used by mtk combo devices sharing a serial transport, such as BT/GPS/FM running via a shared UART, to
> > let the host know what type of radio the following bytes is for and how long it's. 
> 
> but that means it is framed. You know ahead of time how long the H:4 packet will be. That is the important part. Unless you tell me that it can fragment a H:4 frame over multiple MTK specific frames.
> 
> > The extra header is really useful for a combo device, splitting flow and fitting in a single serial transport, but for a single device such as
> > mt7622 Bluetooth I made here, it seems to be useless.
> 
> Useless or not depends on what you are going to do with the device. In theory you could write a serdev driver that just handles the multiplexing (since that is what you have) and then hands down the frames to individual drivers. Frankly something like btqcomsmd.c seems to be what you really want here. However first you would needed this multiplex driver that takes the serial stream and breaks it up.
> 
> > Because the extra header doesn't provide any details about this stream and each radio stack still needs to be in charge of their stream
> > parsing. That is why I still want to use recv_h4.h instead of coding my own parser.
> 
> As I said above, if the header + length always indicates a full H:4 frame, then you do not need h4_recv.h since you know the packet size. If it doesn't (and it means things can fragment), then you do.
> 

My case is the extra header + length doesn't indicate a full H:4 frame,
things can fragment

> However I have to note that a serial stream from your multiplexer protocol also needs some state handling since it can be interrupted at any point. If you want this clean, then you actually do that anyway. Essentially you have two protocols layered and want to process the independently.

Yes, I also agree that it makes better and cleaner if there is another
driver in charge of the multiplexer protocol and the framing.

But, could you accept that I postpone the target into the next stage,
I just like to consider BT single device, not for multiplexer protocol
case, in the current stage.

To be honest, my next step is to add mt7688 btusb and then want to have
an integration with btmtkuart. mt7688 btusb doesn't have extra framing
for multiplexer protocol, so it can allow me to make the concentration
more on pure bt protocol and pushing the latest mtk bluetooth devices
being supported on the bluez driver.

> If you post details about the multiplexing protocol for your serial stream, then I can help you design a driver for it. With serdev that is actually simple. And then you could hook up GPS etc. at some point once you want to run this on hardware that has the combo chip.
> 

okay, really thanks for your help. I also have an interest on this part.
now how does bluez receive and sent packet from/to a virtual device ( a
serdev handling multiplexer protocol)? It seems current bluez device all
handling packet from/to physical bus device. or I was missing something?

> Having a Linux Bluetooth driver is useful for Android as well btw. We have HCI_CHANNEL_USER and a generic Android driver for using it. So enabling the chip in Linux upstream will enable it for Android as well.
> 
it really save more time as I knew many vendors do two driver separately
for bluez and bluedroid. where could I find the resource for
HCI_CHANNEL_USER and generic android driver ? Is it still the part of
bluez or run by another project ?

> Regards
> 
> Marcel
> 

[SPAM]Re: [PATCH v5 5/7] Bluetooth: Extend btuart driver for join more vendor devices

[Sean Wang]

On Wed, 2018-07-18 at 22:26 +0800, Sean Wang wrote:
> On Wed, 2018-07-18 at 14:23 +0200, Marcel Holtmann wrote:
> > Hi Sean,
> > 

[ ... ]

> > > Because the extra header doesn't provide any details about this stream and each radio stack still needs to be in charge of their stream
> > > parsing. That is why I still want to use recv_h4.h instead of coding my own parser.
> > 
> > As I said above, if the header + length always indicates a full H:4 frame, then you do not need h4_recv.h since you know the packet size. If it doesn't (and it means things can fragment), then you do.
> > 
> 
> My case is the extra header + length doesn't indicate a full H:4 frame,
> things can fragment
> 
> > However I have to note that a serial stream from your multiplexer protocol also needs some state handling since it can be interrupted at any point. If you want this clean, then you actually do that anyway. Essentially you have two protocols layered and want to process the independently.
> 
> Yes, I also agree that it makes better and cleaner if there is another
> driver in charge of the multiplexer protocol and the framing.
> 
> But, could you accept that I postpone the target into the next stage,
> I just like to consider BT single device, not for multiplexer protocol
> case, in the current stage.
> 
> To be honest, my next step is to add mt7688 btusb and then want to have
> an integration with btmtkuart. mt7688 btusb doesn't have extra framing

something needs to be fixed, I mean mt7668 instead of mt7688

> for multiplexer protocol, so it can allow me to make the concentration
> more on pure bt protocol and pushing the latest mtk bluetooth devices
> being supported on the bluez driver.
> 
> > If you post details about the multiplexing protocol for your serial stream, then I can help you design a driver for it. With serdev that is actually simple. And then you could hook up GPS etc. at some point once you want to run this on hardware that has the combo chip.
> > 
> 
> okay, really thanks for your help. I also have an interest on this part.
> now how does bluez receive and sent packet from/to a virtual device ( a
> serdev handling multiplexer protocol)? It seems current bluez device all
> handling packet from/to physical bus device. or I was missing something?
> 
> > Having a Linux Bluetooth driver is useful for Android as well btw. We have HCI_CHANNEL_USER and a generic Android driver for using it. So enabling the chip in Linux upstream will enable it for Android as well.
> > 
> it really save more time as I knew many vendors do two driver separately
> for bluez and bluedroid. where could I find the resource for
> HCI_CHANNEL_USER and generic android driver ? Is it still the part of
> bluez or run by another project ?
> 
> > Regards
> > 
> > Marcel
> > 
> 
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek