RE: [PATCH v12 3/3] Bluetooth: hci_uart: Support firmware download for Marvell

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Loic,

Thanks for your comments.

> -----Original Message-----
> From: Loic Poulain [mailto:loic.poulain@xxxxxxxxx]
> Sent: Tuesday, August 16, 2016 1:20 PM
> To: Amitkumar Karwar; linux-bluetooth@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-rockchip@xxxxxxxxxxxxxxxxxxx;
> marcel@xxxxxxxxxxxx; Ganapathi Bhat
> Subject: Re: [PATCH v12 3/3] Bluetooth: hci_uart: Support firmware
> download for Marvell
> 
> Hi Amit,
> 
> On 09/08/2016 18:32, Amitkumar Karwar wrote:
> > From: Ganapathi Bhat <gbhat@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Signed-off-by: Amitkumar Karwar <akarwar@xxxxxxxxxxx>
> > Tested-by: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
> > ---
> > 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.
> > v8: Addressed review comments from Marcel Holtmann
> >      a) Removed unnecessary memory allocation failure messages
> >      b) Get rid of btmrvl.h header file and add definitions in
> > hci_mrvl.c file
> > v9: Addressed review comments from Marcel Holtmann
> >      a) Moved firmware download code from setup to prepare handler.
> >      b) Change messages from bt_dev_*->BT_*, as hdev isn't available
> during firmware
> >       download.
> > v10: Addressed review comments from Marcel Holtmann
> >      a) Added new callback recv_for_prepare to receive data from
> device
> >       during prepare phase
> >      b) Avoided using private flags (HCI_UART_DNLD_FW) as new receive
> callback is
> >       added for the same purpose
> >      c) Used kernel API to handle unaligned data
> >      d) Moved mrvl_set_baud functionality inside setup callback
> > v11: Write data through ldisc in mrvl_send_ack() instead of directly
> calling
> >      write method(One Thousand Gnomes).
> > v12: a) Check for buffer length before copying to skb (Loic Poulain)
> >       b) Rearrange skb memory allocation check
> > ---
> >   drivers/bluetooth/Kconfig     |  11 +
> >   drivers/bluetooth/Makefile    |   1 +
> >   drivers/bluetooth/hci_ldisc.c |   6 +
> >   drivers/bluetooth/hci_mrvl.c  | 545
> ++++++++++++++++++++++++++++++++++++++++++
> >   drivers/bluetooth/hci_uart.h  |   8 +-
> >   5 files changed, 570 insertions(+), 1 deletion(-)
> >   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/hci_ldisc.c
> > b/drivers/bluetooth/hci_ldisc.c index 2c88586..ded13d3 100644
> > --- a/drivers/bluetooth/hci_ldisc.c
> > +++ b/drivers/bluetooth/hci_ldisc.c
> > @@ -821,6 +821,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;
> >   }
> > @@ -856,6 +859,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..c993c1b
> > --- /dev/null
> > +++ b/drivers/bluetooth/hci_mrvl.c
> > @@ -0,0 +1,545 @@
> > +/* 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/firmware.h>
> > +#include <linux/tty.h>
> > +#include <asm/unaligned.h>
> > +#include <net/bluetooth/bluetooth.h>
> > +#include <net/bluetooth/hci_core.h>
> > +#include "hci_uart.h"
> > +
> > +struct fw_data {
> > +	wait_queue_head_t init_wait_q;
> > +	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_CHIP_ID	0x50
> > +#define MRVL_8997_FW_NAME	"mrvl/uart8997_bt.bin"
> > +#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
> > +#define MRVL_WAIT_TIMEOUT	msecs_to_jiffies(12000)
> > +
> > +struct mrvl_data {
> > +	struct sk_buff *rx_skb;
> > +	struct sk_buff_head txq;
> > +	struct fw_data *fwdata;
> > +};
> > +
> > +static int get_cts(struct hci_uart *hu) {
> > +	struct tty_struct *tty = hu->tty;
> > +	u32 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;
> > +}
> > +
> > +/* 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);
> > +	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;
> > +
> > +	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 mrvl_data *mrvl = hu->priv;
> > +	struct sk_buff *skb = NULL;
> > +
> > +	skb = bt_skb_alloc(sizeof(ack), GFP_KERNEL);
> > +	if (!skb)
> > +		return;
> > +
> > +	memcpy(skb->data, &ack, sizeof(ack));
> > +	skb_put(skb, sizeof(ack));
> > +	skb_queue_head(&mrvl->txq, skb);
> > +	hci_uart_tx_wakeup(hu);
> > +}
> > +
> > +/* 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;
> > +	u16 lhs, rhs;
> > +
> > +	lhs = get_unaligned_le16(skb->data + 1);
> > +	rhs = get_unaligned_le16(skb->data + 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 (fw_data->expected_ack == MRVL_HDR_CHIP_VER) {
> > +			fw_data->chip_id = skb->data[1];
> > +			fw_data->chip_rev = skb->data[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 ((i < count) && buf[i] != fw_data->expected_ack)
> > +			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;
> > +	if ((len + skb->len) > MRVL_FW_HDR_LEN)
> > +		return ERR_PTR(-EILSEQ);
> > +
> > +	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 firmware feedback data */
> > +static int mrvl_recv_for_prepare(struct hci_uart *hu, const void
> *data,
> > +				 int count)
> > +{
> > +	struct mrvl_data *mrvl = hu->priv;
> > +
> > +	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;
> > +}
> 
> Don't understand why you need this 'prepare stuff', everything could be
> done in setup procedure.

We had our firmware download code in setup handler in initial versions of this patch. Marcel pointed out that firmware download in ->setup() is fine as long as it uses HCI commands. We don't use HCI commands for FW download. Assumption in ->setup handler is we are in an HCI capable transport and it is ready.

So we came up with prepare handler.

I have addressed your other comments in V13 patch. I will send it shortly.

Regards,
Amitkumar Karwar
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux