[PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

From: Ganapathi Bhat <gbhat@marvell.com>

This patch implement firmware download feature for
Marvell Bluetooth devices. If firmware is already
downloaded, it will skip downloading.

Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: Fixed compilation warning reported by kbuild test robot
v3: Addressed review comments from Marcel Holtmann
    a) Removed vendor specific code from hci_ldisc.c
    b) Get rid of static forward declaration
    c) Removed unnecessary heavy nesting
    d) Git rid of module parameter and global variables
    e) Add logic to pick right firmware image
v4: Addresses review comments from Alan
    a) Use existing kernel helper APIs instead of writing own.
    b) Replace mdelay() with msleep()
v5: Addresses review comments from Loic Poulain
    a) Use bt_dev_err/warn/dbg helpers insted of BT_ERR/WARN/DBG
    b) Used static functions where required and removed forward delcarations
    c) Edited comments for the function hci_uart_recv_data
    d) Made HCI_UART_DNLD_FW flag a part of driver private data
v6: Addresses review comments from Loic Poulain
    a) Used skb instead of array to store firmware data during download
    b) Used hci_uart_tx_wakeup and enqueued packets instead of tty write
    c) Used GFP_KERNEL instead of GFP_ATOMIC
v7: Edited Kconfig to add dependency for BT_HCIUART_H4. The change resolves
    errors reported by kbuild test robot.
---
 drivers/bluetooth/Kconfig     |  11 +
 drivers/bluetooth/Makefile    |   1 +
 drivers/bluetooth/btmrvl.h    |  43 ++++
 drivers/bluetooth/hci_ldisc.c |   6 +
 drivers/bluetooth/hci_mrvl.c  | 568 ++++++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/hci_uart.h  |   8 +-
 6 files changed, 636 insertions(+), 1 deletion(-)
 create mode 100644 drivers/bluetooth/btmrvl.h
 create mode 100644 drivers/bluetooth/hci_mrvl.c

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index cf50fd2..daafd0c 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -180,6 +180,17 @@ config BT_HCIUART_AG6XX
 
 	  Say Y here to compile support for Intel AG6XX protocol.
 
+config BT_HCIUART_MRVL
+	bool "Marvell protocol support"
+	depends on BT_HCIUART
+	select BT_HCIUART_H4
+	help
+	  Marvell is serial protocol for communication between Bluetooth
+	  device and host. This protocol is required for most Marvell Bluetooth
+	  devices with UART interface.
+
+	  Say Y here to compile support for HCI MRVL protocol.
+
 config BT_HCIBCM203X
 	tristate "HCI BCM203x USB driver"
 	depends on USB
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 9c18939..364dbb6 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)	+= hci_intel.o
 hci_uart-$(CONFIG_BT_HCIUART_BCM)	+= hci_bcm.o
 hci_uart-$(CONFIG_BT_HCIUART_QCA)	+= hci_qca.o
 hci_uart-$(CONFIG_BT_HCIUART_AG6XX)	+= hci_ag6xx.o
+hci_uart-$(CONFIG_BT_HCIUART_MRVL)	+= hci_mrvl.o
 hci_uart-objs				:= $(hci_uart-y)
 
 ccflags-y += -D__CHECK_ENDIAN__
diff --git a/drivers/bluetooth/btmrvl.h b/drivers/bluetooth/btmrvl.h
new file mode 100644
index 0000000..66855de
--- /dev/null
+++ b/drivers/bluetooth/btmrvl.h
@@ -0,0 +1,43 @@
+/* Bluetooth support for Marvell devices
+ *
+ * Copyright (C) 2016, Marvell International Ltd.
+ *
+ * This software file (the "File") is distributed by Marvell International
+ * Ltd. under the terms of the GNU General Public License Version 2, June 1991
+ * (the "License").  You may use, redistribute and/or modify this File in
+ * accordance with the terms and conditions of the License, a copy of which
+ * is available on the worldwide web at
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
+ *
+ * THE FILE IS DISTRIBUTED AS-IS, WITHOUT WARRANTY OF ANY KIND, AND THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE
+ * ARE EXPRESSLY DISCLAIMED.  The License provides additional details about
+ * this warranty disclaimer.
+ */
+
+struct fw_data {
+	wait_queue_head_t init_wait_q __aligned(4);
+	u8 wait_fw;
+	int next_len;
+	u8 five_bytes[5];
+	u8 next_index;
+	u8 last_ack;
+	u8 expected_ack;
+	struct ktermios old_termios;
+	u8 chip_id;
+	u8 chip_rev;
+	struct sk_buff *skb;
+};
+
+#define MRVL_HELPER_NAME	"mrvl/helper_uart_3000000.bin"
+#define MRVL_8997_FW_NAME	"mrvl/uart8997_bt.bin"
+#define MRVL_WAIT_TIMEOUT	12000
+#define MRVL_MAX_FW_BLOCK_SIZE	1024
+#define MRVL_MAX_RETRY_SEND	12
+#define MRVL_DNLD_DELAY		100
+#define MRVL_ACK		0x5A
+#define MRVL_NAK		0xBF
+#define MRVL_HDR_REQ_FW		0xA5
+#define MRVL_HDR_CHIP_VER	0xAA
+#define MRVL_HCI_OP_SET_BAUD	0xFC09
+#define MRVL_FW_HDR_LEN		5
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index c00168a..b858758 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -807,6 +807,9 @@ static int __init hci_uart_init(void)
 #ifdef CONFIG_BT_HCIUART_AG6XX
 	ag6xx_init();
 #endif
+#ifdef CONFIG_BT_HCIUART_MRVL
+	mrvl_init();
+#endif
 
 	return 0;
 }
@@ -842,6 +845,9 @@ static void __exit hci_uart_exit(void)
 #ifdef CONFIG_BT_HCIUART_AG6XX
 	ag6xx_deinit();
 #endif
+#ifdef CONFIG_BT_HCIUART_MRVL
+	mrvl_deinit();
+#endif
 
 	/* Release tty registration of line discipline */
 	err = tty_unregister_ldisc(N_HCI);
diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
new file mode 100644
index 0000000..bca47fd
--- /dev/null
+++ b/drivers/bluetooth/hci_mrvl.c
@@ -0,0 +1,568 @@
+/* Bluetooth HCI UART driver for Marvell devices
+ *
+ * Copyright (C) 2016, Marvell International Ltd.
+ *
+ *  Acknowledgements:
+ *  This file is based on hci_h4.c, which was written
+ *  by Maxim Krasnyansky and Marcel Holtmann.
+ *
+ * This software file (the "File") is distributed by Marvell International
+ * Ltd. under the terms of the GNU General Public License Version 2, June 1991
+ * (the "License").  You may use, redistribute and/or modify this File in
+ * accordance with the terms and conditions of the License, a copy of which
+ * is available on the worldwide web at
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
+ *
+ * THE FILE IS DISTRIBUTED AS-IS, WITHOUT WARRANTY OF ANY KIND, AND THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE
+ * ARE EXPRESSLY DISCLAIMED.  The License provides additional details about
+ * this warranty disclaimer.
+ */
+
+#include <linux/module.h>
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/fcntl.h>
+#include <linux/interrupt.h>
+#include <linux/ptrace.h>
+#include <linux/poll.h>
+#include <linux/firmware.h>
+#include <linux/version.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/signal.h>
+#include <linux/ioctl.h>
+#include <linux/skbuff.h>
+#include <asm/unaligned.h>
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include "hci_uart.h"
+#include "btmrvl.h"
+
+#define HCI_UART_DNLD_FW	2
+
+struct mrvl_data {
+	struct sk_buff *rx_skb;
+	struct sk_buff_head txq;
+	struct fw_data *fwdata;
+	unsigned long flags;
+};
+
+static int get_cts(struct hci_uart *hu)
+{
+	struct tty_struct *tty = hu->tty;
+	u32 state;
+
+	if (tty->ops->tiocmget) {
+		state = tty->ops->tiocmget(tty);
+		if (state & TIOCM_CTS) {
+			bt_dev_dbg(hu->hdev, "CTS is low");
+			return 1;
+		}
+		bt_dev_dbg(hu->hdev, "CTS is high");
+		return 0;
+	}
+	return -1;
+}
+
+/* Initialize protocol */
+static int mrvl_open(struct hci_uart *hu)
+{
+	struct mrvl_data *mrvl;
+
+	bt_dev_dbg(hu->hdev, "hu %p", hu);
+
+	mrvl = kzalloc(sizeof(*mrvl), GFP_KERNEL);
+	if (!mrvl)
+		return -ENOMEM;
+
+	skb_queue_head_init(&mrvl->txq);
+	hu->priv = mrvl;
+
+	return 0;
+}
+
+/* Flush protocol data */
+static int mrvl_flush(struct hci_uart *hu)
+{
+	struct mrvl_data *mrvl = hu->priv;
+
+	bt_dev_dbg(hu->hdev, "hu %p", hu);
+
+	skb_queue_purge(&mrvl->txq);
+
+	return 0;
+}
+
+/* Close protocol */
+static int mrvl_close(struct hci_uart *hu)
+{
+	struct mrvl_data *mrvl = hu->priv;
+
+	bt_dev_dbg(hu->hdev, "hu %p", hu);
+
+	skb_queue_purge(&mrvl->txq);
+	kfree_skb(mrvl->rx_skb);
+	kfree(mrvl->fwdata);
+	hu->priv = NULL;
+	kfree(mrvl);
+
+	return 0;
+}
+
+/* Enqueue frame for transmittion (padding, crc, etc) */
+static int mrvl_enqueue(struct hci_uart *hu, struct sk_buff *skb)
+{
+	struct mrvl_data *mrvl = hu->priv;
+
+	if (test_bit(HCI_UART_DNLD_FW, &mrvl->flags))
+		return -EBUSY;
+
+	bt_dev_dbg(hu->hdev, "hu %p skb %p", hu, skb);
+
+	/* Prepend skb with frame type */
+	memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
+	skb_queue_tail(&mrvl->txq, skb);
+
+	return 0;
+}
+
+static const struct h4_recv_pkt mrvl_recv_pkts[] = {
+	{ H4_RECV_ACL,   .recv = hci_recv_frame },
+	{ H4_RECV_SCO,   .recv = hci_recv_frame },
+	{ H4_RECV_EVENT, .recv = hci_recv_frame },
+};
+
+/* Send ACK/NAK to the device */
+static void mrvl_send_ack(struct hci_uart *hu, unsigned char ack)
+{
+	struct tty_struct *tty = hu->tty;
+
+	tty->ops->write(tty, &ack, sizeof(ack));
+}
+
+/* Validate the feedback data from device */
+static void mrvl_pkt_complete(struct hci_uart *hu, struct sk_buff *skb)
+{
+	struct mrvl_data *mrvl = hu->priv;
+	struct fw_data *fw_data = mrvl->fwdata;
+	u8 buf[MRVL_FW_HDR_LEN];
+	u16 lhs, rhs;
+
+	memcpy(buf, skb->data, skb->len);
+
+	lhs = le16_to_cpu(*((__le16 *)(&buf[1])));
+	rhs = le16_to_cpu(*((__le16 *)(&buf[3])));
+	if ((lhs ^ rhs) == 0xffff) {
+		mrvl_send_ack(hu, MRVL_ACK);
+		fw_data->wait_fw = 1;
+		fw_data->next_len = lhs;
+		/*firmware download is done, send the last ack*/
+		if (!lhs)
+			fw_data->last_ack = 1;
+
+		if (unlikely(fw_data->expected_ack == MRVL_HDR_CHIP_VER)) {
+			fw_data->chip_id = *((u8 *)(&buf[1]));
+			fw_data->chip_rev = *((u8 *)(&buf[2]));
+		}
+		wake_up_interruptible(&fw_data->init_wait_q);
+	} else {
+		mrvl_send_ack(hu, MRVL_NAK);
+	}
+}
+
+/* This function receives data from the uart device during firmware download.
+ * Driver expects 5 bytes of data as per the protocal in the below format:
+ * <HEADER><BYTE_1><BYTE_2><BYTE_3><BYTE_4>
+ * BYTE_3 and BYTE_4 are compliment of BYTE_1 an BYTE_2. Data can come in chunks
+ * of any length. If length received is < 5, accumulate the data in an array,
+ * until we have a sequence of 5 bytes, starting with the expected HEADER. If
+ * the length received is > 5  bytes, then get the first 5 bytes, starting with
+ * the HEADER and process the same, ignoring the rest of the bytes as per the
+ * protocal.
+ */
+static struct sk_buff *mrvl_process_fw_data(struct hci_uart *hu,
+					    struct sk_buff *skb,
+					    u8 *buf, int count)
+{
+	struct mrvl_data *mrvl = hu->priv;
+	struct fw_data *fw_data = mrvl->fwdata;
+	int i = 0, len;
+
+	if (!skb) {
+		while (buf[i] != fw_data->expected_ack && i < count)
+			i++;
+		if (i == count)
+			return ERR_PTR(-EILSEQ);
+
+		skb = bt_skb_alloc(MRVL_FW_HDR_LEN, GFP_KERNEL);
+	}
+
+	if (!skb)
+		return ERR_PTR(-ENOMEM);
+
+	len = count - i;
+	memcpy(skb_put(skb, len), &buf[i], len);
+
+	if (skb->len == MRVL_FW_HDR_LEN) {
+		mrvl_pkt_complete(hu, skb);
+		kfree_skb(skb);
+		skb = NULL;
+	}
+
+	return skb;
+}
+
+/* Receive data */
+static int mrvl_recv(struct hci_uart *hu, const void *data, int count)
+{
+	struct mrvl_data *mrvl = hu->priv;
+
+	if (test_bit(HCI_UART_DNLD_FW, &mrvl->flags)) {
+		mrvl->fwdata->skb = mrvl_process_fw_data(hu, mrvl->fwdata->skb,
+							 (u8 *)data, count);
+		if (IS_ERR(mrvl->fwdata->skb)) {
+			int err = PTR_ERR(mrvl->fwdata->skb);
+
+			bt_dev_err(hu->hdev,
+				   "Receive firmware data failed (%d)", err);
+			mrvl->fwdata->skb = NULL;
+			return err;
+		}
+		return 0;
+	}
+
+	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
+		return -EUNATCH;
+
+	mrvl->rx_skb = h4_recv_buf(hu->hdev, mrvl->rx_skb, data, count,
+				   mrvl_recv_pkts, ARRAY_SIZE(mrvl_recv_pkts));
+	if (IS_ERR(mrvl->rx_skb)) {
+		int err = PTR_ERR(mrvl->rx_skb);
+
+		bt_dev_err(hu->hdev, "Frame reassembly failed (%d)", err);
+		mrvl->rx_skb = NULL;
+		return err;
+	}
+
+	return count;
+}
+
+static struct sk_buff *mrvl_dequeue(struct hci_uart *hu)
+{
+	struct mrvl_data *mrvl = hu->priv;
+
+	return skb_dequeue(&mrvl->txq);
+}
+
+static int mrvl_init_fw_data(struct hci_uart *hu)
+{
+	struct fw_data *fwdata;
+	struct mrvl_data *mrvl = hu->priv;
+
+	fwdata = kzalloc(sizeof(*fwdata), GFP_KERNEL);
+
+	if (!fwdata)
+		return -ENOMEM;
+
+	mrvl->fwdata = fwdata;
+	init_waitqueue_head(&fwdata->init_wait_q);
+
+	return 0;
+}
+
+/* Wait for the header from device */
+static int mrvl_wait_for_hdr(struct hci_uart *hu, u8 header)
+{
+	struct mrvl_data *mrvl = hu->priv;
+	struct fw_data *fw_data = mrvl->fwdata;
+
+	fw_data->expected_ack = header;
+	fw_data->wait_fw = 0;
+
+	if (!wait_event_interruptible_timeout(fw_data->init_wait_q,
+					      fw_data->wait_fw,
+					((MRVL_WAIT_TIMEOUT) * HZ / 1000))) {
+		bt_dev_err(hu->hdev, "TIMEOUT, waiting for:0x%x",
+			   fw_data->expected_ack);
+		return -1;
+	}
+
+	return 0;
+}
+
+/* Send bytes to device */
+static int mrvl_send_data(struct hci_uart *hu, struct sk_buff *skb)
+{
+	struct mrvl_data *mrvl = hu->priv;
+
+	skb_queue_head(&mrvl->txq, skb);
+	hci_uart_tx_wakeup(hu);
+
+	if (mrvl_wait_for_hdr(hu, MRVL_HDR_REQ_FW) == -1)
+		return -1;
+
+	return 0;
+}
+
+static struct sk_buff *mrvl_alloc_skb(struct hci_dev *hdev)
+{
+	struct sk_buff *skb = NULL;
+
+	skb = bt_skb_alloc(MRVL_MAX_FW_BLOCK_SIZE, GFP_KERNEL);
+	if (!skb)
+		bt_dev_err(hdev, "cannot allocate memory for skb");
+	else
+		skb->dev = (void *)hdev;
+
+	return skb;
+}
+
+/* Download firmware to the device */
+static int mrvl_dnld_fw(struct hci_uart *hu, const char *file_name)
+{
+	struct hci_dev *hdev = hu->hdev;
+	const struct firmware *fw = NULL;
+	struct sk_buff *skb = NULL;
+	int offset = 0;
+	int ret, tx_len;
+	struct mrvl_data *mrvl = hu->priv;
+	struct fw_data *fw_data = mrvl->fwdata;
+
+	ret = request_firmware(&fw, file_name, &hdev->dev);
+	if (ret < 0) {
+		bt_dev_err(hu->hdev, "request_firmware() failed");
+		return -1;
+	}
+
+	bt_dev_info(hu->hdev, "Downloading FW (%d bytes)", (u16)fw->size);
+
+	fw_data->last_ack = 0;
+
+	do {
+		if ((offset >= fw->size) || (fw_data->last_ack))
+			break;
+		tx_len = fw_data->next_len;
+		if ((fw->size - offset) < tx_len)
+			tx_len = fw->size - offset;
+
+		skb = mrvl_alloc_skb(hdev);
+		if (!skb) {
+			ret = -1;
+			goto done;
+		}
+
+		memcpy(skb->data, &fw->data[offset], tx_len);
+		skb_put(skb, tx_len);
+		if (mrvl_send_data(hu, skb) != 0) {
+			bt_dev_err(hu->hdev, "fail to download firmware");
+			ret = -1;
+			goto done;
+		}
+		offset += tx_len;
+	} while (1);
+
+	bt_dev_info(hu->hdev, "downloaded %d byte firmware", offset);
+done:
+	release_firmware(fw);
+
+	return ret;
+}
+
+/* Set the baud rate */
+static int mrvl_set_dev_baud(struct hci_uart *hu)
+{
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+	static const u8 baud_param[] = { 0xc0, 0xc6, 0x2d, 0x00 };
+	int err;
+
+	skb = __hci_cmd_sync(hdev, MRVL_HCI_OP_SET_BAUD, sizeof(baud_param),
+			     baud_param, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		err = PTR_ERR(skb);
+		bt_dev_err(hu->hdev, "Set device baudrate failed (%d)", err);
+		return err;
+	}
+	kfree_skb(skb);
+
+	return 0;
+}
+
+/* Reset device */
+static int mrvl_reset(struct hci_uart *hu)
+{
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+	int err;
+
+	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_CMD_TIMEOUT);
+	if (IS_ERR(skb)) {
+		err = PTR_ERR(skb);
+		bt_dev_err(hu->hdev, "Reset device failed (%d)", err);
+		return err;
+	}
+	kfree_skb(skb);
+
+	return 0;
+}
+
+static int mrvl_set_baud(struct hci_uart *hu)
+{
+	int err;
+
+	hci_uart_set_baudrate(hu, 115200);
+	hci_uart_set_flow_control(hu, false);
+
+	err = mrvl_reset(hu);
+	if (err)
+		goto set_term_baud;
+	else
+		goto set_dev_baud;
+
+set_dev_baud:
+	err = mrvl_set_dev_baud(hu);
+	if (err)
+		return -1;
+
+set_term_baud:
+	hci_uart_set_baudrate(hu, 3000000);
+	hci_uart_set_flow_control(hu, false);
+
+	return 0;
+}
+
+static int mrvl_get_fw_name(struct hci_uart *hu, char *fw_name)
+{
+	int ret;
+	struct mrvl_data *mrvl = hu->priv;
+	struct fw_data *fw_data = mrvl->fwdata;
+
+	ret = mrvl_wait_for_hdr(hu, MRVL_HDR_CHIP_VER);
+	if (ret) {
+		ret = -1;
+		bt_dev_err(hu->hdev, "Could not read chip id and revision");
+		goto fail;
+	}
+
+	bt_dev_dbg(hu->hdev, "chip_id=0x%x, chip_rev=0x%x",
+		   fw_data->chip_id, fw_data->chip_rev);
+
+	if (fw_data->chip_id == 0x50) {
+		memcpy(fw_name, MRVL_8997_FW_NAME, sizeof(MRVL_8997_FW_NAME));
+	} else {
+		ret = -1;
+		bt_dev_err(hu->hdev, "Invalid chip id");
+	}
+fail:
+	return ret;
+}
+
+/* Download helper and firmare to device */
+static int hci_uart_dnld_fw(struct hci_uart *hu)
+{
+	struct tty_struct *tty = hu->tty;
+	struct ktermios new_termios;
+	struct ktermios old_termios;
+	struct mrvl_data *mrvl = hu->priv;
+	char fw_name[128];
+	int ret;
+
+	old_termios = tty->termios;
+
+	if (get_cts(hu)) {
+		bt_dev_info(hu->hdev, "fw is running");
+		clear_bit(HCI_UART_DNLD_FW, &mrvl->flags);
+		goto set_baud;
+	}
+
+	hci_uart_set_baudrate(hu, 115200);
+	hci_uart_set_flow_control(hu, true);
+
+	ret = mrvl_wait_for_hdr(hu, MRVL_HDR_REQ_FW);
+	if (ret)
+		goto fail;
+
+	ret = mrvl_dnld_fw(hu, MRVL_HELPER_NAME);
+	if (ret)
+		goto fail;
+
+	msleep(MRVL_DNLD_DELAY);
+
+	hci_uart_set_baudrate(hu, 3000000);
+	hci_uart_set_flow_control(hu, false);
+
+	ret = mrvl_get_fw_name(hu, fw_name);
+	if (ret)
+		goto fail;
+
+	ret = mrvl_wait_for_hdr(hu, MRVL_HDR_REQ_FW);
+	if (ret)
+		goto fail;
+
+	ret = mrvl_dnld_fw(hu, fw_name);
+	if (ret)
+		goto fail;
+
+	msleep(MRVL_DNLD_DELAY);
+	/* restore uart settings */
+	new_termios = tty->termios;
+	tty->termios.c_cflag = old_termios.c_cflag;
+	tty_set_termios(tty, &new_termios);
+	clear_bit(HCI_UART_DNLD_FW, &mrvl->flags);
+
+set_baud:
+	ret = mrvl_set_baud(hu);
+	if (ret)
+		goto fail;
+
+	msleep(MRVL_DNLD_DELAY);
+
+	return ret;
+fail:
+	/* restore uart settings */
+	new_termios = tty->termios;
+	tty->termios.c_cflag = old_termios.c_cflag;
+	tty_set_termios(tty, &new_termios);
+	clear_bit(HCI_UART_DNLD_FW, &mrvl->flags);
+
+	return ret;
+}
+
+static int mrvl_setup(struct hci_uart *hu)
+{
+	struct mrvl_data *mrvl = hu->priv;
+
+	mrvl_init_fw_data(hu);
+	set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
+
+	return hci_uart_dnld_fw(hu);
+}
+
+static const struct hci_uart_proto mrvlp = {
+	.id		= HCI_UART_MRVL,
+	.name		= "MRVL",
+	.open		= mrvl_open,
+	.close		= mrvl_close,
+	.recv		= mrvl_recv,
+	.enqueue	= mrvl_enqueue,
+	.dequeue	= mrvl_dequeue,
+	.flush		= mrvl_flush,
+	.setup		= mrvl_setup,
+};
+
+int __init mrvl_init(void)
+{
+	return hci_uart_register_proto(&mrvlp);
+}
+
+int __exit mrvl_deinit(void)
+{
+	return hci_uart_unregister_proto(&mrvlp);
+}
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 4814ff0..8b1f744 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -35,7 +35,7 @@
 #define HCIUARTGETFLAGS		_IOR('U', 204, int)
 
 /* UART protocols */
-#define HCI_UART_MAX_PROTO	10
+#define HCI_UART_MAX_PROTO	11
 
 #define HCI_UART_H4	0
 #define HCI_UART_BCSP	1
@@ -47,6 +47,7 @@
 #define HCI_UART_BCM	7
 #define HCI_UART_QCA	8
 #define HCI_UART_AG6XX	9
+#define HCI_UART_MRVL	10
 
 #define HCI_UART_RAW_DEVICE	0
 #define HCI_UART_RESET_ON_INIT	1
@@ -188,3 +189,8 @@ int qca_deinit(void);
 int ag6xx_init(void);
 int ag6xx_deinit(void);
 #endif
+
+#ifdef CONFIG_BT_HCIUART_MRVL
+int mrvl_init(void);
+int mrvl_deinit(void);
+#endif
-- 
1.9.1

RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

Hi Marcel,

> From: Amitkumar Karwar [mailto:akarwar@marvell.com]
> Sent: Tuesday, April 05, 2016 2:04 PM
> To: linux-bluetooth@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Ganapathi Bhat; Amitkumar Karwar
> Subject: [PATCH v7] Bluetooth: hci_uart: Support firmware download for
> Marvell
> 
> From: Ganapathi Bhat <gbhat@marvell.com>
> 
> This patch implement firmware download feature for Marvell Bluetooth
> devices. If firmware is already downloaded, it will skip downloading.
> 
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
> v2: Fixed compilation warning reported by kbuild test robot
> v3: Addressed review comments from Marcel Holtmann
>     a) Removed vendor specific code from hci_ldisc.c
>     b) Get rid of static forward declaration
>     c) Removed unnecessary heavy nesting
>     d) Git rid of module parameter and global variables
>     e) Add logic to pick right firmware image
> v4: Addresses review comments from Alan
>     a) Use existing kernel helper APIs instead of writing own.
>     b) Replace mdelay() with msleep()
> v5: Addresses review comments from Loic Poulain
>     a) Use bt_dev_err/warn/dbg helpers insted of BT_ERR/WARN/DBG
>     b) Used static functions where required and removed forward
> delcarations
>     c) Edited comments for the function hci_uart_recv_data
>     d) Made HCI_UART_DNLD_FW flag a part of driver private data
> v6: Addresses review comments from Loic Poulain
>     a) Used skb instead of array to store firmware data during download
>     b) Used hci_uart_tx_wakeup and enqueued packets instead of tty write
>     c) Used GFP_KERNEL instead of GFP_ATOMIC
> v7: Edited Kconfig to add dependency for BT_HCIUART_H4. The change
> resolves
>     errors reported by kbuild test robot.
> ---
>  drivers/bluetooth/Kconfig     |  11 +
>  drivers/bluetooth/Makefile    |   1 +
>  drivers/bluetooth/btmrvl.h    |  43 ++++
>  drivers/bluetooth/hci_ldisc.c |   6 +
>  drivers/bluetooth/hci_mrvl.c  | 568
> ++++++++++++++++++++++++++++++++++++++++++
>  drivers/bluetooth/hci_uart.h  |   8 +-
>  6 files changed, 636 insertions(+), 1 deletion(-)  create mode 100644
> drivers/bluetooth/btmrvl.h  create mode 100644
> drivers/bluetooth/hci_mrvl.c
> 

Could you please take this patch if there are no further comments/suggestions?

Regards,
Amitkumar

Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Marcel Holtmann]

Hi Amitkumar,

>> From: Amitkumar Karwar [mailto:akarwar@marvell.com]
>> Sent: Tuesday, April 05, 2016 2:04 PM
>> To: linux-bluetooth@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org; Ganapathi Bhat; Amitkumar Karwar
>> Subject: [PATCH v7] Bluetooth: hci_uart: Support firmware download for
>> Marvell
>> 
>> From: Ganapathi Bhat <gbhat@marvell.com>
>> 
>> This patch implement firmware download feature for Marvell Bluetooth
>> devices. If firmware is already downloaded, it will skip downloading.
>> 
>> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
>> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
>> ---
>> v2: Fixed compilation warning reported by kbuild test robot
>> v3: Addressed review comments from Marcel Holtmann
>>    a) Removed vendor specific code from hci_ldisc.c
>>    b) Get rid of static forward declaration
>>    c) Removed unnecessary heavy nesting
>>    d) Git rid of module parameter and global variables
>>    e) Add logic to pick right firmware image
>> v4: Addresses review comments from Alan
>>    a) Use existing kernel helper APIs instead of writing own.
>>    b) Replace mdelay() with msleep()
>> v5: Addresses review comments from Loic Poulain
>>    a) Use bt_dev_err/warn/dbg helpers insted of BT_ERR/WARN/DBG
>>    b) Used static functions where required and removed forward
>> delcarations
>>    c) Edited comments for the function hci_uart_recv_data
>>    d) Made HCI_UART_DNLD_FW flag a part of driver private data
>> v6: Addresses review comments from Loic Poulain
>>    a) Used skb instead of array to store firmware data during download
>>    b) Used hci_uart_tx_wakeup and enqueued packets instead of tty write
>>    c) Used GFP_KERNEL instead of GFP_ATOMIC
>> v7: Edited Kconfig to add dependency for BT_HCIUART_H4. The change
>> resolves
>>    errors reported by kbuild test robot.
>> ---
>> drivers/bluetooth/Kconfig     |  11 +
>> drivers/bluetooth/Makefile    |   1 +
>> drivers/bluetooth/btmrvl.h    |  43 ++++
>> drivers/bluetooth/hci_ldisc.c |   6 +
>> drivers/bluetooth/hci_mrvl.c  | 568
>> ++++++++++++++++++++++++++++++++++++++++++
>> drivers/bluetooth/hci_uart.h  |   8 +-
>> 6 files changed, 636 insertions(+), 1 deletion(-)  create mode 100644
>> drivers/bluetooth/btmrvl.h  create mode 100644
>> drivers/bluetooth/hci_mrvl.c
>> 
> 
> Could you please take this patch if there are no further comments/suggestions?

didn't I see a kbot build warning for this version as well? Or was that the previous version?

Regards

Marcel

RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

Hi Marcel,

> From: Marcel Holtmann [mailto:marcel@holtmann.org]
> Sent: Tuesday, April 12, 2016 9:48 PM
> To: Amitkumar Karwar
> Cc: linux-bluetooth@vger.kernel.org; Ganapathi Bhat
> Subject: Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download
> for Marvell
>=20
> Hi Amitkumar,
>=20
> >> From: Amitkumar Karwar [mailto:akarwar@marvell.com]
> >> Sent: Tuesday, April 05, 2016 2:04 PM
> >> To: linux-bluetooth@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org; Ganapathi Bhat; Amitkumar Karwar
> >> Subject: [PATCH v7] Bluetooth: hci_uart: Support firmware download
> >> for Marvell
> >>
> >> From: Ganapathi Bhat <gbhat@marvell.com>
> >>
> >> This patch implement firmware download feature for Marvell Bluetooth
> >> devices. If firmware is already downloaded, it will skip downloading.
> >>
> >> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> >> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> >> ---
> >> v2: Fixed compilation warning reported by kbuild test robot
> >> v3: Addressed review comments from Marcel Holtmann
> >>    a) Removed vendor specific code from hci_ldisc.c
> >>    b) Get rid of static forward declaration
> >>    c) Removed unnecessary heavy nesting
> >>    d) Git rid of module parameter and global variables
> >>    e) Add logic to pick right firmware image
> >> v4: Addresses review comments from Alan
> >>    a) Use existing kernel helper APIs instead of writing own.
> >>    b) Replace mdelay() with msleep()
> >> v5: Addresses review comments from Loic Poulain
> >>    a) Use bt_dev_err/warn/dbg helpers insted of BT_ERR/WARN/DBG
> >>    b) Used static functions where required and removed forward
> >> delcarations
> >>    c) Edited comments for the function hci_uart_recv_data
> >>    d) Made HCI_UART_DNLD_FW flag a part of driver private data
> >> v6: Addresses review comments from Loic Poulain
> >>    a) Used skb instead of array to store firmware data during
> download
> >>    b) Used hci_uart_tx_wakeup and enqueued packets instead of tty
> write
> >>    c) Used GFP_KERNEL instead of GFP_ATOMIC
> >> v7: Edited Kconfig to add dependency for BT_HCIUART_H4. The change
> >> resolves
> >>    errors reported by kbuild test robot.
> >> ---
> >> drivers/bluetooth/Kconfig     |  11 +
> >> drivers/bluetooth/Makefile    |   1 +
> >> drivers/bluetooth/btmrvl.h    |  43 ++++
> >> drivers/bluetooth/hci_ldisc.c |   6 +
> >> drivers/bluetooth/hci_mrvl.c  | 568
> >> ++++++++++++++++++++++++++++++++++++++++++
> >> drivers/bluetooth/hci_uart.h  |   8 +-
> >> 6 files changed, 636 insertions(+), 1 deletion(-)  create mode 100644
> >> drivers/bluetooth/btmrvl.h  create mode 100644
> >> drivers/bluetooth/hci_mrvl.c
> >>
> >
> > Could you please take this patch if there are no further
> comments/suggestions?
>=20
> didn't I see a kbot build warning for this version as well? Or was that
> the previous version?
>=20

Last kbot build warning was for V6 patch. It has been fixed in this version=
.
https://lkml.org/lkml/2016/4/2/102

Regards,
Amitkumar

RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

Hi Marcel,

> From: Amitkumar Karwar
> Sent: Tuesday, April 12, 2016 10:23 PM
> To: 'Marcel Holtmann'
> Cc: linux-bluetooth@vger.kernel.org; Ganapathi Bhat
> Subject: RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download
> for Marvell
>=20
> Hi Marcel,
>=20
> > From: Marcel Holtmann [mailto:marcel@holtmann.org]
> > Sent: Tuesday, April 12, 2016 9:48 PM
> > To: Amitkumar Karwar
> > Cc: linux-bluetooth@vger.kernel.org; Ganapathi Bhat
> > Subject: Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download
> > for Marvell
> >
> > Hi Amitkumar,
> >
> > >> From: Amitkumar Karwar [mailto:akarwar@marvell.com]
> > >> Sent: Tuesday, April 05, 2016 2:04 PM
> > >> To: linux-bluetooth@vger.kernel.org
> > >> Cc: linux-kernel@vger.kernel.org; Ganapathi Bhat; Amitkumar Karwar
> > >> Subject: [PATCH v7] Bluetooth: hci_uart: Support firmware download
> > >> for Marvell
> > >>
> > >> From: Ganapathi Bhat <gbhat@marvell.com>
> > >>
> > >> This patch implement firmware download feature for Marvell
> > >> Bluetooth devices. If firmware is already downloaded, it will skip
> downloading.
> > >>
> > >> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> > >> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > >> ---
> > >> v2: Fixed compilation warning reported by kbuild test robot
> > >> v3: Addressed review comments from Marcel Holtmann
> > >>    a) Removed vendor specific code from hci_ldisc.c
> > >>    b) Get rid of static forward declaration
> > >>    c) Removed unnecessary heavy nesting
> > >>    d) Git rid of module parameter and global variables
> > >>    e) Add logic to pick right firmware image
> > >> v4: Addresses review comments from Alan
> > >>    a) Use existing kernel helper APIs instead of writing own.
> > >>    b) Replace mdelay() with msleep()
> > >> v5: Addresses review comments from Loic Poulain
> > >>    a) Use bt_dev_err/warn/dbg helpers insted of BT_ERR/WARN/DBG
> > >>    b) Used static functions where required and removed forward
> > >> delcarations
> > >>    c) Edited comments for the function hci_uart_recv_data
> > >>    d) Made HCI_UART_DNLD_FW flag a part of driver private data
> > >> v6: Addresses review comments from Loic Poulain
> > >>    a) Used skb instead of array to store firmware data during
> > download
> > >>    b) Used hci_uart_tx_wakeup and enqueued packets instead of tty
> > write
> > >>    c) Used GFP_KERNEL instead of GFP_ATOMIC
> > >> v7: Edited Kconfig to add dependency for BT_HCIUART_H4. The change
> > >> resolves
> > >>    errors reported by kbuild test robot.
> > >> ---
> > >> drivers/bluetooth/Kconfig     |  11 +
> > >> drivers/bluetooth/Makefile    |   1 +
> > >> drivers/bluetooth/btmrvl.h    |  43 ++++
> > >> drivers/bluetooth/hci_ldisc.c |   6 +
> > >> drivers/bluetooth/hci_mrvl.c  | 568
> > >> ++++++++++++++++++++++++++++++++++++++++++
> > >> drivers/bluetooth/hci_uart.h  |   8 +-
> > >> 6 files changed, 636 insertions(+), 1 deletion(-)  create mode
> > >> 100644 drivers/bluetooth/btmrvl.h  create mode 100644
> > >> drivers/bluetooth/hci_mrvl.c
> > >>
> > >
> > > Could you please take this patch if there are no further
> > comments/suggestions?
> >
> > didn't I see a kbot build warning for this version as well? Or was
> > that the previous version?
> >
>=20
> Last kbot build warning was for V6 patch. It has been fixed in this
> version.
> https://lkml.org/lkml/2016/4/2/102
>=20

The warning had got fixed in latest version. Please let me know if there ar=
e any issues merging the patch.

Regards,
Amitkumar

Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Marcel Holtmann]

Hi Amitkumar,

> This patch implement firmware download feature for
> Marvell Bluetooth devices. If firmware is already
> downloaded, it will skip downloading.
> 
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> ---
> v2: Fixed compilation warning reported by kbuild test robot
> v3: Addressed review comments from Marcel Holtmann
>    a) Removed vendor specific code from hci_ldisc.c
>    b) Get rid of static forward declaration
>    c) Removed unnecessary heavy nesting
>    d) Git rid of module parameter and global variables
>    e) Add logic to pick right firmware image
> v4: Addresses review comments from Alan
>    a) Use existing kernel helper APIs instead of writing own.
>    b) Replace mdelay() with msleep()
> v5: Addresses review comments from Loic Poulain
>    a) Use bt_dev_err/warn/dbg helpers insted of BT_ERR/WARN/DBG
>    b) Used static functions where required and removed forward delcarations
>    c) Edited comments for the function hci_uart_recv_data
>    d) Made HCI_UART_DNLD_FW flag a part of driver private data
> v6: Addresses review comments from Loic Poulain
>    a) Used skb instead of array to store firmware data during download
>    b) Used hci_uart_tx_wakeup and enqueued packets instead of tty write
>    c) Used GFP_KERNEL instead of GFP_ATOMIC
> v7: Edited Kconfig to add dependency for BT_HCIUART_H4. The change resolves
>    errors reported by kbuild test robot.
> ---
> drivers/bluetooth/Kconfig     |  11 +
> drivers/bluetooth/Makefile    |   1 +
> drivers/bluetooth/btmrvl.h    |  43 ++++
> drivers/bluetooth/hci_ldisc.c |   6 +
> drivers/bluetooth/hci_mrvl.c  | 568 ++++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/hci_uart.h  |   8 +-
> 6 files changed, 636 insertions(+), 1 deletion(-)
> create mode 100644 drivers/bluetooth/btmrvl.h
> create mode 100644 drivers/bluetooth/hci_mrvl.c
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index cf50fd2..daafd0c 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -180,6 +180,17 @@ config BT_HCIUART_AG6XX
> 
> 	  Say Y here to compile support for Intel AG6XX protocol.
> 
> +config BT_HCIUART_MRVL
> +	bool "Marvell protocol support"
> +	depends on BT_HCIUART
> +	select BT_HCIUART_H4
> +	help
> +	  Marvell is serial protocol for communication between Bluetooth
> +	  device and host. This protocol is required for most Marvell Bluetooth
> +	  devices with UART interface.
> +
> +	  Say Y here to compile support for HCI MRVL protocol.
> +
> config BT_HCIBCM203X
> 	tristate "HCI BCM203x USB driver"
> 	depends on USB
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index 9c18939..364dbb6 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)	+= hci_intel.o
> hci_uart-$(CONFIG_BT_HCIUART_BCM)	+= hci_bcm.o
> hci_uart-$(CONFIG_BT_HCIUART_QCA)	+= hci_qca.o
> hci_uart-$(CONFIG_BT_HCIUART_AG6XX)	+= hci_ag6xx.o
> +hci_uart-$(CONFIG_BT_HCIUART_MRVL)	+= hci_mrvl.o
> hci_uart-objs				:= $(hci_uart-y)
> 
> ccflags-y += -D__CHECK_ENDIAN__
> diff --git a/drivers/bluetooth/btmrvl.h b/drivers/bluetooth/btmrvl.h
> new file mode 100644
> index 0000000..66855de
> --- /dev/null
> +++ b/drivers/bluetooth/btmrvl.h
> @@ -0,0 +1,43 @@
> +/* Bluetooth support for Marvell devices
> + *
> + * Copyright (C) 2016, Marvell International Ltd.
> + *
> + * This software file (the "File") is distributed by Marvell International
> + * Ltd. under the terms of the GNU General Public License Version 2, June 1991
> + * (the "License").  You may use, redistribute and/or modify this File in
> + * accordance with the terms and conditions of the License, a copy of which
> + * is available on the worldwide web at
> + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
> + *
> + * THE FILE IS DISTRIBUTED AS-IS, WITHOUT WARRANTY OF ANY KIND, AND THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE
> + * ARE EXPRESSLY DISCLAIMED.  The License provides additional details about
> + * this warranty disclaimer.
> + */
> +
> +struct fw_data {
> +	wait_queue_head_t init_wait_q __aligned(4);

why does this need to be aligned?

> +	u8 wait_fw;
> +	int next_len;
> +	u8 five_bytes[5];
> +	u8 next_index;
> +	u8 last_ack;
> +	u8 expected_ack;
> +	struct ktermios old_termios;
> +	u8 chip_id;
> +	u8 chip_rev;
> +	struct sk_buff *skb;
> +};
> +
> +#define MRVL_HELPER_NAME	"mrvl/helper_uart_3000000.bin"
> +#define MRVL_8997_FW_NAME	"mrvl/uart8997_bt.bin"
> +#define MRVL_WAIT_TIMEOUT	12000
> +#define MRVL_MAX_FW_BLOCK_SIZE	1024
> +#define MRVL_MAX_RETRY_SEND	12
> +#define MRVL_DNLD_DELAY		100
> +#define MRVL_ACK		0x5A
> +#define MRVL_NAK		0xBF
> +#define MRVL_HDR_REQ_FW		0xA5
> +#define MRVL_HDR_CHIP_VER	0xAA
> +#define MRVL_HCI_OP_SET_BAUD	0xFC09
> +#define MRVL_FW_HDR_LEN		5

And if this is all hci_marvl.c specific, why do we bother with a generic header file. I would first try to keep it all in the C file and only split it out once it is actually used outside the driver.

> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index c00168a..b858758 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -807,6 +807,9 @@ static int __init hci_uart_init(void)
> #ifdef CONFIG_BT_HCIUART_AG6XX
> 	ag6xx_init();
> #endif
> +#ifdef CONFIG_BT_HCIUART_MRVL
> +	mrvl_init();
> +#endif
> 
> 	return 0;
> }
> @@ -842,6 +845,9 @@ static void __exit hci_uart_exit(void)
> #ifdef CONFIG_BT_HCIUART_AG6XX
> 	ag6xx_deinit();
> #endif
> +#ifdef CONFIG_BT_HCIUART_MRVL
> +	mrvl_deinit();
> +#endif
> 
> 	/* Release tty registration of line discipline */
> 	err = tty_unregister_ldisc(N_HCI);
> diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
> new file mode 100644
> index 0000000..bca47fd
> --- /dev/null
> +++ b/drivers/bluetooth/hci_mrvl.c
> @@ -0,0 +1,568 @@
> +/* Bluetooth HCI UART driver for Marvell devices
> + *
> + * Copyright (C) 2016, Marvell International Ltd.
> + *
> + *  Acknowledgements:
> + *  This file is based on hci_h4.c, which was written
> + *  by Maxim Krasnyansky and Marcel Holtmann.
> + *
> + * This software file (the "File") is distributed by Marvell International
> + * Ltd. under the terms of the GNU General Public License Version 2, June 1991
> + * (the "License").  You may use, redistribute and/or modify this File in
> + * accordance with the terms and conditions of the License, a copy of which
> + * is available on the worldwide web at
> + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
> + *
> + * THE FILE IS DISTRIBUTED AS-IS, WITHOUT WARRANTY OF ANY KIND, AND THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE
> + * ARE EXPRESSLY DISCLAIMED.  The License provides additional details about
> + * this warranty disclaimer.
> + */
> +
> +#include <linux/module.h>
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/fcntl.h>
> +#include <linux/interrupt.h>
> +#include <linux/ptrace.h>
> +#include <linux/poll.h>
> +#include <linux/firmware.h>
> +#include <linux/version.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/signal.h>

Are you sure all of these are needed? Some of them sound not like they are actually used.

> +#include <linux/ioctl.h>
> +#include <linux/skbuff.h>
> +#include <asm/unaligned.h>
> +
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include "hci_uart.h"
> +#include "btmrvl.h"
> +
> +#define HCI_UART_DNLD_FW	2

Why is this 2?

> +
> +struct mrvl_data {
> +	struct sk_buff *rx_skb;
> +	struct sk_buff_head txq;
> +	struct fw_data *fwdata;
> +	unsigned long flags;
> +};
> +
> +static int get_cts(struct hci_uart *hu)
> +{
> +	struct tty_struct *tty = hu->tty;
> +	u32 state;
> +
> +	if (tty->ops->tiocmget) {
> +		state = tty->ops->tiocmget(tty);
> +		if (state & TIOCM_CTS) {
> +			bt_dev_dbg(hu->hdev, "CTS is low");
> +			return 1;
> +		}
> +		bt_dev_dbg(hu->hdev, "CTS is high");
> +		return 0;
> +	}
> +	return -1;
> +}

Why is this a tristate and not just bool?

> +
> +/* Initialize protocol */
> +static int mrvl_open(struct hci_uart *hu)
> +{
> +	struct mrvl_data *mrvl;
> +
> +	bt_dev_dbg(hu->hdev, "hu %p", hu);
> +
> +	mrvl = kzalloc(sizeof(*mrvl), GFP_KERNEL);
> +	if (!mrvl)
> +		return -ENOMEM;
> +
> +	skb_queue_head_init(&mrvl->txq);
> +	hu->priv = mrvl;
> +
> +	return 0;
> +}
> +
> +/* Flush protocol data */
> +static int mrvl_flush(struct hci_uart *hu)
> +{
> +	struct mrvl_data *mrvl = hu->priv;
> +
> +	bt_dev_dbg(hu->hdev, "hu %p", hu);
> +
> +	skb_queue_purge(&mrvl->txq);
> +
> +	return 0;
> +}
> +
> +/* Close protocol */
> +static int mrvl_close(struct hci_uart *hu)
> +{
> +	struct mrvl_data *mrvl = hu->priv;
> +
> +	bt_dev_dbg(hu->hdev, "hu %p", hu);
> +
> +	skb_queue_purge(&mrvl->txq);
> +	kfree_skb(mrvl->rx_skb);
> +	kfree(mrvl->fwdata);
> +	hu->priv = NULL;
> +	kfree(mrvl);
> +
> +	return 0;
> +}
> +
> +/* Enqueue frame for transmittion (padding, crc, etc) */
> +static int mrvl_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> +{
> +	struct mrvl_data *mrvl = hu->priv;
> +
> +	if (test_bit(HCI_UART_DNLD_FW, &mrvl->flags))
> +		return -EBUSY;
> +
> +	bt_dev_dbg(hu->hdev, "hu %p skb %p", hu, skb);
> +
> +	/* Prepend skb with frame type */
> +	memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
> +	skb_queue_tail(&mrvl->txq, skb);
> +
> +	return 0;
> +}
> +
> +static const struct h4_recv_pkt mrvl_recv_pkts[] = {
> +	{ H4_RECV_ACL,   .recv = hci_recv_frame },
> +	{ H4_RECV_SCO,   .recv = hci_recv_frame },
> +	{ H4_RECV_EVENT, .recv = hci_recv_frame },
> +};
> +
> +/* Send ACK/NAK to the device */
> +static void mrvl_send_ack(struct hci_uart *hu, unsigned char ack)
> +{
> +	struct tty_struct *tty = hu->tty;
> +
> +	tty->ops->write(tty, &ack, sizeof(ack));
> +}
> +
> +/* Validate the feedback data from device */
> +static void mrvl_pkt_complete(struct hci_uart *hu, struct sk_buff *skb)
> +{
> +	struct mrvl_data *mrvl = hu->priv;
> +	struct fw_data *fw_data = mrvl->fwdata;
> +	u8 buf[MRVL_FW_HDR_LEN];
> +	u16 lhs, rhs;
> +
> +	memcpy(buf, skb->data, skb->len);
> +
> +	lhs = le16_to_cpu(*((__le16 *)(&buf[1])));
> +	rhs = le16_to_cpu(*((__le16 *)(&buf[3])));
> +	if ((lhs ^ rhs) == 0xffff) {
> +		mrvl_send_ack(hu, MRVL_ACK);
> +		fw_data->wait_fw = 1;
> +		fw_data->next_len = lhs;
> +		/*firmware download is done, send the last ack*/
> +		if (!lhs)
> +			fw_data->last_ack = 1;
> +
> +		if (unlikely(fw_data->expected_ack == MRVL_HDR_CHIP_VER)) {
> +			fw_data->chip_id = *((u8 *)(&buf[1]));
> +			fw_data->chip_rev = *((u8 *)(&buf[2]));
> +		}
> +		wake_up_interruptible(&fw_data->init_wait_q);
> +	} else {
> +		mrvl_send_ack(hu, MRVL_NAK);
> +	}
> +}
> +
> +/* This function receives data from the uart device during firmware download.
> + * Driver expects 5 bytes of data as per the protocal in the below format:
> + * <HEADER><BYTE_1><BYTE_2><BYTE_3><BYTE_4>
> + * BYTE_3 and BYTE_4 are compliment of BYTE_1 an BYTE_2. Data can come in chunks
> + * of any length. If length received is < 5, accumulate the data in an array,
> + * until we have a sequence of 5 bytes, starting with the expected HEADER. If
> + * the length received is > 5  bytes, then get the first 5 bytes, starting with
> + * the HEADER and process the same, ignoring the rest of the bytes as per the
> + * protocal.
> + */
> +static struct sk_buff *mrvl_process_fw_data(struct hci_uart *hu,
> +					    struct sk_buff *skb,
> +					    u8 *buf, int count)
> +{
> +	struct mrvl_data *mrvl = hu->priv;
> +	struct fw_data *fw_data = mrvl->fwdata;
> +	int i = 0, len;
> +
> +	if (!skb) {
> +		while (buf[i] != fw_data->expected_ack && i < count)
> +			i++;
> +		if (i == count)
> +			return ERR_PTR(-EILSEQ);
> +
> +		skb = bt_skb_alloc(MRVL_FW_HDR_LEN, GFP_KERNEL);
> +	}
> +
> +	if (!skb)
> +		return ERR_PTR(-ENOMEM);
> +
> +	len = count - i;
> +	memcpy(skb_put(skb, len), &buf[i], len);
> +
> +	if (skb->len == MRVL_FW_HDR_LEN) {
> +		mrvl_pkt_complete(hu, skb);
> +		kfree_skb(skb);
> +		skb = NULL;
> +	}
> +
> +	return skb;
> +}
> +
> +/* Receive data */
> +static int mrvl_recv(struct hci_uart *hu, const void *data, int count)
> +{
> +	struct mrvl_data *mrvl = hu->priv;
> +
> +	if (test_bit(HCI_UART_DNLD_FW, &mrvl->flags)) {
> +		mrvl->fwdata->skb = mrvl_process_fw_data(hu, mrvl->fwdata->skb,
> +							 (u8 *)data, count);
> +		if (IS_ERR(mrvl->fwdata->skb)) {
> +			int err = PTR_ERR(mrvl->fwdata->skb);
> +
> +			bt_dev_err(hu->hdev,
> +				   "Receive firmware data failed (%d)", err);
> +			mrvl->fwdata->skb = NULL;
> +			return err;
> +		}
> +		return 0;
> +	}

This part actually worries me a bit. Yes, we can do it this way, but in general it sounds a bit more like we need a generic approach in hci_ldisc.c to handle pre-HCI firmware loading and/or setup.

In the btusb.c driver we added ->setup_on_usb callback. And for it sounds like we need something similar here. So that hci_ldisc.c can handle most of the basic TX/RX.

> +
> +	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
> +		return -EUNATCH;
> +
> +	mrvl->rx_skb = h4_recv_buf(hu->hdev, mrvl->rx_skb, data, count,
> +				   mrvl_recv_pkts, ARRAY_SIZE(mrvl_recv_pkts));
> +	if (IS_ERR(mrvl->rx_skb)) {
> +		int err = PTR_ERR(mrvl->rx_skb);
> +
> +		bt_dev_err(hu->hdev, "Frame reassembly failed (%d)", err);
> +		mrvl->rx_skb = NULL;
> +		return err;
> +	}
> +
> +	return count;
> +}
> +
> +static struct sk_buff *mrvl_dequeue(struct hci_uart *hu)
> +{
> +	struct mrvl_data *mrvl = hu->priv;
> +
> +	return skb_dequeue(&mrvl->txq);
> +}
> +
> +static int mrvl_init_fw_data(struct hci_uart *hu)
> +{
> +	struct fw_data *fwdata;
> +	struct mrvl_data *mrvl = hu->priv;
> +
> +	fwdata = kzalloc(sizeof(*fwdata), GFP_KERNEL);
> +

This empty line here is not needed.

> +	if (!fwdata)
> +		return -ENOMEM;
> +
> +	mrvl->fwdata = fwdata;
> +	init_waitqueue_head(&fwdata->init_wait_q);
> +
> +	return 0;
> +}
> +
> +/* Wait for the header from device */
> +static int mrvl_wait_for_hdr(struct hci_uart *hu, u8 header)
> +{
> +	struct mrvl_data *mrvl = hu->priv;
> +	struct fw_data *fw_data = mrvl->fwdata;
> +
> +	fw_data->expected_ack = header;
> +	fw_data->wait_fw = 0;
> +
> +	if (!wait_event_interruptible_timeout(fw_data->init_wait_q,
> +					      fw_data->wait_fw,
> +					((MRVL_WAIT_TIMEOUT) * HZ / 1000))) {

Don't use HZ magic, use the proper conversation macros and define a new one specific for this value.

> +		bt_dev_err(hu->hdev, "TIMEOUT, waiting for:0x%x",
> +			   fw_data->expected_ack);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/* Send bytes to device */
> +static int mrvl_send_data(struct hci_uart *hu, struct sk_buff *skb)
> +{
> +	struct mrvl_data *mrvl = hu->priv;
> +
> +	skb_queue_head(&mrvl->txq, skb);
> +	hci_uart_tx_wakeup(hu);
> +
> +	if (mrvl_wait_for_hdr(hu, MRVL_HDR_REQ_FW) == -1)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static struct sk_buff *mrvl_alloc_skb(struct hci_dev *hdev)
> +{
> +	struct sk_buff *skb = NULL;

This assignment is useless.

> +
> +	skb = bt_skb_alloc(MRVL_MAX_FW_BLOCK_SIZE, GFP_KERNEL);
> +	if (!skb)
> +		bt_dev_err(hdev, "cannot allocate memory for skb");

	return NULL here please.

And you might want to check if we not have removed all the memory allocation error messages. I think the kernel will print appropriate errors by itself. Please follow general practice.

> +	else
> +		skb->dev = (void *)hdev;
> +
> +	return skb;
> +}
> +
> +/* Download firmware to the device */
> +static int mrvl_dnld_fw(struct hci_uart *hu, const char *file_name)
> +{
> +	struct hci_dev *hdev = hu->hdev;
> +	const struct firmware *fw = NULL;
> +	struct sk_buff *skb = NULL;
> +	int offset = 0;
> +	int ret, tx_len;
> +	struct mrvl_data *mrvl = hu->priv;
> +	struct fw_data *fw_data = mrvl->fwdata;
> +
> +	ret = request_firmware(&fw, file_name, &hdev->dev);
> +	if (ret < 0) {
> +		bt_dev_err(hu->hdev, "request_firmware() failed");
> +		return -1;
> +	}
> +
> +	bt_dev_info(hu->hdev, "Downloading FW (%d bytes)", (u16)fw->size);
> +
> +	fw_data->last_ack = 0;
> +
> +	do {
> +		if ((offset >= fw->size) || (fw_data->last_ack))
> +			break;
> +		tx_len = fw_data->next_len;
> +		if ((fw->size - offset) < tx_len)
> +			tx_len = fw->size - offset;
> +
> +		skb = mrvl_alloc_skb(hdev);
> +		if (!skb) {
> +			ret = -1;
> +			goto done;
> +		}
> +
> +		memcpy(skb->data, &fw->data[offset], tx_len);
> +		skb_put(skb, tx_len);
> +		if (mrvl_send_data(hu, skb) != 0) {
> +			bt_dev_err(hu->hdev, "fail to download firmware");
> +			ret = -1;
> +			goto done;
> +		}
> +		offset += tx_len;
> +	} while (1);
> +
> +	bt_dev_info(hu->hdev, "downloaded %d byte firmware", offset);
> +done:
> +	release_firmware(fw);
> +
> +	return ret;
> +}
> +
> +/* Set the baud rate */
> +static int mrvl_set_dev_baud(struct hci_uart *hu)
> +{
> +	struct hci_dev *hdev = hu->hdev;
> +	struct sk_buff *skb;
> +	static const u8 baud_param[] = { 0xc0, 0xc6, 0x2d, 0x00 };
> +	int err;
> +
> +	skb = __hci_cmd_sync(hdev, MRVL_HCI_OP_SET_BAUD, sizeof(baud_param),
> +			     baud_param, HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		err = PTR_ERR(skb);
> +		bt_dev_err(hu->hdev, "Set device baudrate failed (%d)", err);
> +		return err;
> +	}
> +	kfree_skb(skb);
> +
> +	return 0;
> +}
> +
> +/* Reset device */
> +static int mrvl_reset(struct hci_uart *hu)
> +{
> +	struct hci_dev *hdev = hu->hdev;
> +	struct sk_buff *skb;
> +	int err;
> +
> +	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_CMD_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		err = PTR_ERR(skb);
> +		bt_dev_err(hu->hdev, "Reset device failed (%d)", err);
> +		return err;
> +	}
> +	kfree_skb(skb);
> +
> +	return 0;
> +}
> +
> +static int mrvl_set_baud(struct hci_uart *hu)
> +{
> +	int err;
> +
> +	hci_uart_set_baudrate(hu, 115200);
> +	hci_uart_set_flow_control(hu, false);
> +
> +	err = mrvl_reset(hu);
> +	if (err)
> +		goto set_term_baud;
> +	else
> +		goto set_dev_baud;
> +
> +set_dev_baud:
> +	err = mrvl_set_dev_baud(hu);
> +	if (err)
> +		return -1;
> +
> +set_term_baud:
> +	hci_uart_set_baudrate(hu, 3000000);
> +	hci_uart_set_flow_control(hu, false);
> +
> +	return 0;
> +}
> +
> +static int mrvl_get_fw_name(struct hci_uart *hu, char *fw_name)
> +{
> +	int ret;
> +	struct mrvl_data *mrvl = hu->priv;
> +	struct fw_data *fw_data = mrvl->fwdata;
> +
> +	ret = mrvl_wait_for_hdr(hu, MRVL_HDR_CHIP_VER);
> +	if (ret) {
> +		ret = -1;
> +		bt_dev_err(hu->hdev, "Could not read chip id and revision");
> +		goto fail;
> +	}
> +
> +	bt_dev_dbg(hu->hdev, "chip_id=0x%x, chip_rev=0x%x",
> +		   fw_data->chip_id, fw_data->chip_rev);
> +
> +	if (fw_data->chip_id == 0x50) {
> +		memcpy(fw_name, MRVL_8997_FW_NAME, sizeof(MRVL_8997_FW_NAME));
> +	} else {
> +		ret = -1;
> +		bt_dev_err(hu->hdev, "Invalid chip id");
> +	}

Not a big fine of these else clauses. Check for the error and exit cleanly.

> +fail:
> +	return ret;
> +}
> +
> +/* Download helper and firmare to device */
> +static int hci_uart_dnld_fw(struct hci_uart *hu)
> +{
> +	struct tty_struct *tty = hu->tty;
> +	struct ktermios new_termios;
> +	struct ktermios old_termios;
> +	struct mrvl_data *mrvl = hu->priv;
> +	char fw_name[128];
> +	int ret;
> +
> +	old_termios = tty->termios;
> +
> +	if (get_cts(hu)) {
> +		bt_dev_info(hu->hdev, "fw is running");
> +		clear_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> +		goto set_baud;
> +	}
> +
> +	hci_uart_set_baudrate(hu, 115200);
> +	hci_uart_set_flow_control(hu, true);
> +
> +	ret = mrvl_wait_for_hdr(hu, MRVL_HDR_REQ_FW);
> +	if (ret)
> +		goto fail;
> +
> +	ret = mrvl_dnld_fw(hu, MRVL_HELPER_NAME);
> +	if (ret)
> +		goto fail;
> +
> +	msleep(MRVL_DNLD_DELAY);
> +
> +	hci_uart_set_baudrate(hu, 3000000);
> +	hci_uart_set_flow_control(hu, false);
> +
> +	ret = mrvl_get_fw_name(hu, fw_name);
> +	if (ret)
> +		goto fail;
> +
> +	ret = mrvl_wait_for_hdr(hu, MRVL_HDR_REQ_FW);
> +	if (ret)
> +		goto fail;
> +
> +	ret = mrvl_dnld_fw(hu, fw_name);
> +	if (ret)
> +		goto fail;
> +
> +	msleep(MRVL_DNLD_DELAY);
> +	/* restore uart settings */
> +	new_termios = tty->termios;
> +	tty->termios.c_cflag = old_termios.c_cflag;
> +	tty_set_termios(tty, &new_termios);
> +	clear_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> +
> +set_baud:
> +	ret = mrvl_set_baud(hu);
> +	if (ret)
> +		goto fail;
> +
> +	msleep(MRVL_DNLD_DELAY);
> +
> +	return ret;
> +fail:
> +	/* restore uart settings */
> +	new_termios = tty->termios;
> +	tty->termios.c_cflag = old_termios.c_cflag;
> +	tty_set_termios(tty, &new_termios);
> +	clear_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> +
> +	return ret;
> +}
> +
> +static int mrvl_setup(struct hci_uart *hu)
> +{
> +	struct mrvl_data *mrvl = hu->priv;
> +
> +	mrvl_init_fw_data(hu);
> +	set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> +
> +	return hci_uart_dnld_fw(hu);
> +}

So this is clearly the wrong spot. When ->setup is called it is expected that HCI is ready. You are misusing it here.

> +
> +static const struct hci_uart_proto mrvlp = {
> +	.id		= HCI_UART_MRVL,
> +	.name		= "MRVL",
> +	.open		= mrvl_open,
> +	.close		= mrvl_close,
> +	.recv		= mrvl_recv,
> +	.enqueue	= mrvl_enqueue,
> +	.dequeue	= mrvl_dequeue,
> +	.flush		= mrvl_flush,
> +	.setup		= mrvl_setup,
> +};
> +
> +int __init mrvl_init(void)
> +{
> +	return hci_uart_register_proto(&mrvlp);
> +}
> +
> +int __exit mrvl_deinit(void)
> +{
> +	return hci_uart_unregister_proto(&mrvlp);
> +}
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index 4814ff0..8b1f744 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -35,7 +35,7 @@
> #define HCIUARTGETFLAGS		_IOR('U', 204, int)
> 
> /* UART protocols */
> -#define HCI_UART_MAX_PROTO	10
> +#define HCI_UART_MAX_PROTO	11
> 
> #define HCI_UART_H4	0
> #define HCI_UART_BCSP	1
> @@ -47,6 +47,7 @@
> #define HCI_UART_BCM	7
> #define HCI_UART_QCA	8
> #define HCI_UART_AG6XX	9
> +#define HCI_UART_MRVL	10
> 
> #define HCI_UART_RAW_DEVICE	0
> #define HCI_UART_RESET_ON_INIT	1
> @@ -188,3 +189,8 @@ int qca_deinit(void);
> int ag6xx_init(void);
> int ag6xx_deinit(void);
> #endif
> +
> +#ifdef CONFIG_BT_HCIUART_MRVL
> +int mrvl_init(void);
> +int mrvl_deinit(void);
> +#endif

Regards

Marcel

RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

Hi Marcel,

Thanks for your review.
We will address these comments in updated version.

> > +
> > +/* Receive data */
> > +static int mrvl_recv(struct hci_uart *hu, const void *data, int
> > +count) {
> > +	struct mrvl_data *mrvl = hu->priv;
> > +
> > +	if (test_bit(HCI_UART_DNLD_FW, &mrvl->flags)) {
> > +		mrvl->fwdata->skb = mrvl_process_fw_data(hu, mrvl->fwdata-
> >skb,
> > +							 (u8 *)data, count);
> > +		if (IS_ERR(mrvl->fwdata->skb)) {
> > +			int err = PTR_ERR(mrvl->fwdata->skb);
> > +
> > +			bt_dev_err(hu->hdev,
> > +				   "Receive firmware data failed (%d)", err);
> > +			mrvl->fwdata->skb = NULL;
> > +			return err;
> > +		}
> > +		return 0;
> > +	}
> 
> This part actually worries me a bit. Yes, we can do it this way, but in
> general it sounds a bit more like we need a generic approach in
> hci_ldisc.c to handle pre-HCI firmware loading and/or setup.
> 
> In the btusb.c driver we added ->setup_on_usb callback. And for it
> sounds like we need something similar here. So that hci_ldisc.c can
> handle most of the basic TX/RX.

Even if we added "->setup_on_usb" in hci_ldisc.c, we will need protocol specific changes in receive path during firmware download.
With this patch, those changes are smoothly handled in hci_mrvl.c file.
[hci_uart_tty_receive] -> [proto->recv] -> [mrvl_recv] -> [normal Rx path/FW download Rx handling]

> > +
> > +static int mrvl_setup(struct hci_uart *hu) {
> > +	struct mrvl_data *mrvl = hu->priv;
> > +
> > +	mrvl_init_fw_data(hu);
> > +	set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> > +
> > +	return hci_uart_dnld_fw(hu);
> > +}
> 
> So this is clearly the wrong spot. When ->setup is called it is expected
> that HCI is ready. You are misusing it here.
> 

Sure. We will move this to mrvl_open() where HCI is not yet initialized.

Regards,
Amitkumar

RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

Hi Marcel,

Thanks for your review.
We will address these comments in updated version.

> > +
> > +/* Receive data */
> > +static int mrvl_recv(struct hci_uart *hu, const void *data, int
> > +count) {
> > +	struct mrvl_data *mrvl =3D hu->priv;
> > +
> > +	if (test_bit(HCI_UART_DNLD_FW, &mrvl->flags)) {
> > +		mrvl->fwdata->skb =3D mrvl_process_fw_data(hu, mrvl->fwdata-
> >skb,
> > +							 (u8 *)data, count);
> > +		if (IS_ERR(mrvl->fwdata->skb)) {
> > +			int err =3D PTR_ERR(mrvl->fwdata->skb);
> > +
> > +			bt_dev_err(hu->hdev,
> > +				   "Receive firmware data failed (%d)", err);
> > +			mrvl->fwdata->skb =3D NULL;
> > +			return err;
> > +		}
> > +		return 0;
> > +	}
>=20
> This part actually worries me a bit. Yes, we can do it this way, but in
> general it sounds a bit more like we need a generic approach in
> hci_ldisc.c to handle pre-HCI firmware loading and/or setup.
>=20
> In the btusb.c driver we added ->setup_on_usb callback. And for it
> sounds like we need something similar here. So that hci_ldisc.c can
> handle most of the basic TX/RX.

Even if we added "->setup_on_usb" in hci_ldisc.c, we will need protocol spe=
cific changes in receive path during firmware download.
With this patch, those changes are smoothly handled in hci_mrvl.c file.
[hci_uart_tty_receive] -> [proto->recv] -> [mrvl_recv] -> [normal Rx path/F=
W download Rx handling]

> > +
> > +static int mrvl_setup(struct hci_uart *hu) {
> > +	struct mrvl_data *mrvl =3D hu->priv;
> > +
> > +	mrvl_init_fw_data(hu);
> > +	set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> > +
> > +	return hci_uart_dnld_fw(hu);
> > +}
>=20
> So this is clearly the wrong spot. When ->setup is called it is expected
> that HCI is ready. You are misusing it here.
>=20

Sure. We will move this to mrvl_open() where HCI is not yet initialized.

Regards,
Amitkumar

Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Marcel Holtmann]

Hi Amitkumar,

> Thanks for your review.
> We will address these comments in updated version.
> 
>>> +
>>> +/* Receive data */
>>> +static int mrvl_recv(struct hci_uart *hu, const void *data, int
>>> +count) {
>>> +	struct mrvl_data *mrvl = hu->priv;
>>> +
>>> +	if (test_bit(HCI_UART_DNLD_FW, &mrvl->flags)) {
>>> +		mrvl->fwdata->skb = mrvl_process_fw_data(hu, mrvl->fwdata-
>>> skb,
>>> +							 (u8 *)data, count);
>>> +		if (IS_ERR(mrvl->fwdata->skb)) {
>>> +			int err = PTR_ERR(mrvl->fwdata->skb);
>>> +
>>> +			bt_dev_err(hu->hdev,
>>> +				   "Receive firmware data failed (%d)", err);
>>> +			mrvl->fwdata->skb = NULL;
>>> +			return err;
>>> +		}
>>> +		return 0;
>>> +	}
>> 
>> This part actually worries me a bit. Yes, we can do it this way, but in
>> general it sounds a bit more like we need a generic approach in
>> hci_ldisc.c to handle pre-HCI firmware loading and/or setup.
>> 
>> In the btusb.c driver we added ->setup_on_usb callback. And for it
>> sounds like we need something similar here. So that hci_ldisc.c can
>> handle most of the basic TX/RX.
> 
> Even if we added "->setup_on_usb" in hci_ldisc.c, we will need protocol specific changes in receive path during firmware download.
> With this patch, those changes are smoothly handled in hci_mrvl.c file.
> [hci_uart_tty_receive] -> [proto->recv] -> [mrvl_recv] -> [normal Rx path/FW download Rx handling]

we might need to find a better callback name, but yes, we want to make this generic in hci_ldisc.c. I do not have a perfect solution or quick answer, but I think this needs a bit more generic framework.

Regards

Marcel

RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

Hi Marcel,

> 
> > > +
> > > +static int mrvl_setup(struct hci_uart *hu) {
> > > +	struct mrvl_data *mrvl = hu->priv;
> > > +
> > > +	mrvl_init_fw_data(hu);
> > > +	set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> > > +
> > > +	return hci_uart_dnld_fw(hu);
> > > +}
> >
> > So this is clearly the wrong spot. When ->setup is called it is
> > expected that HCI is ready. You are misusing it here.
> >
> 
> Sure. We will move this to mrvl_open() where HCI is not yet initialized.

We tried moving firmware download to mrvl_open(), but it's not feasible. "hu->proto" is not yet initialized at that time. So when the data/ack is received during firmware download, we can't have Marvell specific handling. Also, I can see other vendor's (broadcomm, Intel) have done firmware download in setup handler.

I will send V8 patch shortly. Please check.

Regards,
Amitkumar

RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

Hi Marcel,

>=20
> > > +
> > > +static int mrvl_setup(struct hci_uart *hu) {
> > > +	struct mrvl_data *mrvl =3D hu->priv;
> > > +
> > > +	mrvl_init_fw_data(hu);
> > > +	set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> > > +
> > > +	return hci_uart_dnld_fw(hu);
> > > +}
> >
> > So this is clearly the wrong spot. When ->setup is called it is
> > expected that HCI is ready. You are misusing it here.
> >
>=20
> Sure. We will move this to mrvl_open() where HCI is not yet initialized.

We tried moving firmware download to mrvl_open(), but it's not feasible. "h=
u->proto" is not yet initialized at that time. So when the data/ack is rece=
ived during firmware download, we can't have Marvell specific handling. Als=
o, I can see other vendor's (broadcomm, Intel) have done firmware download =
in setup handler.

I will send V8 patch shortly. Please check.

Regards,
Amitkumar

Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Marcel Holtmann]

Hi Amitkumar,

>>>> +
>>>> +static int mrvl_setup(struct hci_uart *hu) {
>>>> +	struct mrvl_data *mrvl = hu->priv;
>>>> +
>>>> +	mrvl_init_fw_data(hu);
>>>> +	set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
>>>> +
>>>> +	return hci_uart_dnld_fw(hu);
>>>> +}
>>> 
>>> So this is clearly the wrong spot. When ->setup is called it is
>>> expected that HCI is ready. You are misusing it here.
>>> 
>> 
>> Sure. We will move this to mrvl_open() where HCI is not yet initialized.
> 
> We tried moving firmware download to mrvl_open(), but it's not feasible. "hu->proto" is not yet initialized at that time. So when the data/ack is received during firmware download, we can't have Marvell specific handling. Also, I can see other vendor's (broadcomm, Intel) have done firmware download in setup handler.

firmware download in ->setup() is fine as long as it uses HCI commands. If it does not use HCI commands, then we need to come up with something new.

The problem here is that for all intense and purposes once ->setup() is called, then assumption is that you are in an HCI capable transport and it is ready.

Regards

Marcel

RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

Hi Marcel,

> From: Marcel Holtmann [mailto:marcel@holtmann.org]
> Sent: Friday, April 22, 2016 6:19 PM
> To: Amitkumar Karwar
> Cc: Linux Bluetooth; LKML; Ganapathi Bhat; Cathy Luo
> Subject: Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download
> for Marvell
> 
> Hi Amitkumar,
> 
> >>>> +
> >>>> +static int mrvl_setup(struct hci_uart *hu) {
> >>>> +	struct mrvl_data *mrvl = hu->priv;
> >>>> +
> >>>> +	mrvl_init_fw_data(hu);
> >>>> +	set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> >>>> +
> >>>> +	return hci_uart_dnld_fw(hu);
> >>>> +}
> >>>
> >>> So this is clearly the wrong spot. When ->setup is called it is
> >>> expected that HCI is ready. You are misusing it here.
> >>>
> >>
> >> Sure. We will move this to mrvl_open() where HCI is not yet
> initialized.
> >
> > We tried moving firmware download to mrvl_open(), but it's not
> feasible. "hu->proto" is not yet initialized at that time. So when the
> data/ack is received during firmware download, we can't have Marvell
> specific handling. Also, I can see other vendor's (broadcomm, Intel)
> have done firmware download in setup handler.
> 
> firmware download in ->setup() is fine as long as it uses HCI commands.
> If it does not use HCI commands, then we need to come up with something
> new.
> 
> The problem here is that for all intense and purposes once ->setup() is
> called, then assumption is that you are in an HCI capable transport and
> it is ready.
> 

We have maintained a flag(in mrvl->flags) which remains on until firmware download gets completed. We drop HCI frames in "->enqueue" if firmware download is in progress. I don't see any possible issues if firmware download is included in ->setup().

request_firmware() API expects a device pointer (hdev->dev). We won't get valid pointer until HCI is ready.
Also, we are downloading firmware data chunks through normal Tx path(scheduling hu->write_work etc) where hdev comes into picture.

Let me know your thoughts.

Regards,
Amitkumar

RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

Hi Marcel,

> From: Marcel Holtmann [mailto:marcel@holtmann.org]
> Sent: Friday, April 22, 2016 6:19 PM
> To: Amitkumar Karwar
> Cc: Linux Bluetooth; LKML; Ganapathi Bhat; Cathy Luo
> Subject: Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download
> for Marvell
>=20
> Hi Amitkumar,
>=20
> >>>> +
> >>>> +static int mrvl_setup(struct hci_uart *hu) {
> >>>> +	struct mrvl_data *mrvl =3D hu->priv;
> >>>> +
> >>>> +	mrvl_init_fw_data(hu);
> >>>> +	set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> >>>> +
> >>>> +	return hci_uart_dnld_fw(hu);
> >>>> +}
> >>>
> >>> So this is clearly the wrong spot. When ->setup is called it is
> >>> expected that HCI is ready. You are misusing it here.
> >>>
> >>
> >> Sure. We will move this to mrvl_open() where HCI is not yet
> initialized.
> >
> > We tried moving firmware download to mrvl_open(), but it's not
> feasible. "hu->proto" is not yet initialized at that time. So when the
> data/ack is received during firmware download, we can't have Marvell
> specific handling. Also, I can see other vendor's (broadcomm, Intel)
> have done firmware download in setup handler.
>=20
> firmware download in ->setup() is fine as long as it uses HCI commands.
> If it does not use HCI commands, then we need to come up with something
> new.
>=20
> The problem here is that for all intense and purposes once ->setup() is
> called, then assumption is that you are in an HCI capable transport and
> it is ready.
>=20

We have maintained a flag(in mrvl->flags) which remains on until firmware d=
ownload gets completed. We drop HCI frames in "->enqueue" if firmware downl=
oad is in progress. I don't see any possible issues if firmware download is=
 included in ->setup().

request_firmware() API expects a device pointer (hdev->dev). We won't get v=
alid pointer until HCI is ready.
Also, we are downloading firmware data chunks through normal Tx path(schedu=
ling hu->write_work etc) where hdev comes into picture.

Let me know your thoughts.

Regards,
Amitkumar

RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

Hi Marcel,

> From: Marcel Holtmann [mailto:marcel@holtmann.org]
> Sent: Friday, April 22, 2016 6:19 PM
> To: Amitkumar Karwar
> Cc: Linux Bluetooth; LKML; Ganapathi Bhat; Cathy Luo
> Subject: Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download
> for Marvell
> 
> Hi Amitkumar,
> 
> >>>> +
> >>>> +static int mrvl_setup(struct hci_uart *hu) {
> >>>> +	struct mrvl_data *mrvl = hu->priv;
> >>>> +
> >>>> +	mrvl_init_fw_data(hu);
> >>>> +	set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> >>>> +
> >>>> +	return hci_uart_dnld_fw(hu);
> >>>> +}
> >>>
> >>> So this is clearly the wrong spot. When ->setup is called it is
> >>> expected that HCI is ready. You are misusing it here.
> >>>
> >>
> >> Sure. We will move this to mrvl_open() where HCI is not yet
> initialized.
> >
> > We tried moving firmware download to mrvl_open(), but it's not
> feasible. "hu->proto" is not yet initialized at that time. So when the
> data/ack is received during firmware download, we can't have Marvell
> specific handling. Also, I can see other vendor's (broadcomm, Intel)
> have done firmware download in setup handler.
> 
> firmware download in ->setup() is fine as long as it uses HCI commands.
> If it does not use HCI commands, then we need to come up with something
> new.
> 
> The problem here is that for all intense and purposes once ->setup() is
> called, then assumption is that you are in an HCI capable transport and
> it is ready.
> 

I understand your concern. We are now planning to add new handler called "->prepare" where we can do firmware download. We are working on the change. Meanwhile let us know if you have any suggestion.

---------hci_ldisc.c---------
@@ -641,6 +641,10 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id)
        hu->proto = p;
        set_bit(HCI_UART_PROTO_READY, &hu->flags);

+       err = p->prepare(hu);
+       if (err)
+               return err;
+
        err = hci_uart_register_dev(hu);
        if (err) {
---------------------------

Regards,
Amitkumar

RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Amitkumar Karwar]

Hi Marcel,

> From: Marcel Holtmann [mailto:marcel@holtmann.org]
> Sent: Friday, April 22, 2016 6:19 PM
> To: Amitkumar Karwar
> Cc: Linux Bluetooth; LKML; Ganapathi Bhat; Cathy Luo
> Subject: Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download
> for Marvell
>=20
> Hi Amitkumar,
>=20
> >>>> +
> >>>> +static int mrvl_setup(struct hci_uart *hu) {
> >>>> +	struct mrvl_data *mrvl =3D hu->priv;
> >>>> +
> >>>> +	mrvl_init_fw_data(hu);
> >>>> +	set_bit(HCI_UART_DNLD_FW, &mrvl->flags);
> >>>> +
> >>>> +	return hci_uart_dnld_fw(hu);
> >>>> +}
> >>>
> >>> So this is clearly the wrong spot. When ->setup is called it is
> >>> expected that HCI is ready. You are misusing it here.
> >>>
> >>
> >> Sure. We will move this to mrvl_open() where HCI is not yet
> initialized.
> >
> > We tried moving firmware download to mrvl_open(), but it's not
> feasible. "hu->proto" is not yet initialized at that time. So when the
> data/ack is received during firmware download, we can't have Marvell
> specific handling. Also, I can see other vendor's (broadcomm, Intel)
> have done firmware download in setup handler.
>=20
> firmware download in ->setup() is fine as long as it uses HCI commands.
> If it does not use HCI commands, then we need to come up with something
> new.
>=20
> The problem here is that for all intense and purposes once ->setup() is
> called, then assumption is that you are in an HCI capable transport and
> it is ready.
>=20

I understand your concern. We are now planning to add new handler called "-=
>prepare" where we can do firmware download. We are working on the change. =
Meanwhile let us know if you have any suggestion.

---------hci_ldisc.c---------
@@ -641,6 +641,10 @@ static int hci_uart_set_proto(struct hci_uart *hu, int=
 id)
        hu->proto =3D p;
        set_bit(HCI_UART_PROTO_READY, &hu->flags);

+       err =3D p->prepare(hu);
+       if (err)
+               return err;
+
        err =3D hci_uart_register_dev(hu);
        if (err) {
---------------------------

Regards,
Amitkumar