[PATCH v7 0/6] Add baudrate management for Bluetooth UART

[PATCH v7 0/6] Add baudrate management for Bluetooth UART

[Frederic Danis]

This patch set introduce baudrate management for Bluetooth UART
Controller setup and a first implementation for Broadcom BCM4324B3
used in Asus T100.

v6 -> v7:
- Fix setup when firmware is missing by moving
  request/release_firmware() out of btbcm_patchram().

v2 -> v6:
- Fixes for __hci_cmd_sync() has been integrated (patch v5)
- In btbcm_check_bdaddr(), merge controller address tests
- Fix declaration of speeds to use "unsigned int" type
- Rename default_speed to init_speed
- Rename operational_speed to oper_speed
- In case of error during vendor specific set_baudrate(), do not change
  host baudrate and continue without changing speed
- Discard changes to bcm_setup_patchram() and in btusb.c
- Introduce helper functions btbcm_initialize() and btbcm_finalize() to
  be used by the UART side of things.

Frederic Danis (6):
  Bluetooth: btbcm: Move request/release_firmware()
  Bluetooth: btbcm: Add BCM4324B3 UART device
  Bluetooth: hci_uart: Support operational speed during setup
  Bluetooth: btbcm: Add helper functions for UART setup
  Bluetooth: hci_uart: Update Broadcom UART setup
  Bluetooth: hci_uart: Add bcm_set_baudrate()

 drivers/bluetooth/btbcm.c     | 135 +++++++++++++++++++++++++++++++++---------
 drivers/bluetooth/btbcm.h     |  20 ++++++-
 drivers/bluetooth/hci_bcm.c   |  86 ++++++++++++++++++++++++++-
 drivers/bluetooth/hci_ldisc.c |  26 ++++++++
 drivers/bluetooth/hci_uart.h  |   4 ++
 5 files changed, 240 insertions(+), 31 deletions(-)

-- 
1.9.1

[PATCH v7 1/6] Bluetooth: btbcm: Move request/release_firmware()

[Frederic Danis]

Move request/release_firmware() out of btbcm_patchram().
This allows a better error management, if request_firmware() returns an
error then the controller will be used without firmware loading and 0 is
returned.
This will imply to change btbcm_patchram() to accept a firmware instead
of firmware name.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/btbcm.c | 26 ++++++++++++--------------
 drivers/bluetooth/btbcm.h |  6 ++++--
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 728fce3..c7aec97 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -89,21 +89,14 @@ int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 }
 EXPORT_SYMBOL_GPL(btbcm_set_bdaddr);
 
-int btbcm_patchram(struct hci_dev *hdev, const char *firmware)
+int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw)
 {
 	const struct hci_command_hdr *cmd;
-	const struct firmware *fw;
 	const u8 *fw_ptr;
 	size_t fw_size;
 	struct sk_buff *skb;
 	u16 opcode;
-	int err;
-
-	err = request_firmware(&fw, firmware, &hdev->dev);
-	if (err < 0) {
-		BT_INFO("%s: BCM: Patch %s not found", hdev->name, firmware);
-		return err;
-	}
+	int err = 0;
 
 	/* Start Download */
 	skb = __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT);
@@ -129,8 +122,7 @@ int btbcm_patchram(struct hci_dev *hdev, const char *firmware)
 		fw_size -= sizeof(*cmd);
 
 		if (fw_size < cmd->plen) {
-			BT_ERR("%s: BCM: Patch %s is corrupted", hdev->name,
-			       firmware);
+			BT_ERR("%s: BCM: Patch is corrupted", hdev->name);
 			err = -EINVAL;
 			goto done;
 		}
@@ -156,7 +148,6 @@ int btbcm_patchram(struct hci_dev *hdev, const char *firmware)
 	msleep(250);
 
 done:
-	release_firmware(fw);
 	return err;
 }
 EXPORT_SYMBOL(btbcm_patchram);
@@ -265,6 +256,7 @@ static const struct {
 int btbcm_setup_patchram(struct hci_dev *hdev)
 {
 	char fw_name[64];
+	const struct firmware *fw;
 	u16 subver, rev, pid, vid;
 	const char *hw_name = NULL;
 	struct sk_buff *skb;
@@ -335,9 +327,15 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
 		hw_name ? : "BCM", (subver & 0x7000) >> 13,
 		(subver & 0x1f00) >> 8, (subver & 0x00ff), rev & 0x0fff);
 
-	err = btbcm_patchram(hdev, fw_name);
-	if (err == -ENOENT)
+	err = request_firmware(&fw, fw_name, &hdev->dev);
+	if (err < 0) {
+		BT_INFO("%s: BCM: Patch %s not found", hdev->name, fw_name);
 		return 0;
+	}
+
+	btbcm_patchram(hdev, fw);
+
+	release_firmware(fw);
 
 	/* Reset */
 	err = btbcm_reset(hdev);
diff --git a/drivers/bluetooth/btbcm.h b/drivers/bluetooth/btbcm.h
index eb6ab5f..de11ace 100644
--- a/drivers/bluetooth/btbcm.h
+++ b/drivers/bluetooth/btbcm.h
@@ -21,11 +21,13 @@
  *
  */
 
+#include <linux/firmware.h>
+
 #if IS_ENABLED(CONFIG_BT_BCM)
 
 int btbcm_check_bdaddr(struct hci_dev *hdev);
 int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
-int btbcm_patchram(struct hci_dev *hdev, const char *firmware);
+int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw);
 
 int btbcm_setup_patchram(struct hci_dev *hdev);
 int btbcm_setup_apple(struct hci_dev *hdev);
@@ -42,7 +44,7 @@ static inline int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 	return -EOPNOTSUPP;
 }
 
-static inline int btbcm_patchram(struct hci_dev *hdev, const char *firmware)
+static inline int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw)
 {
 	return -EOPNOTSUPP;
 }
-- 
1.9.1

Re: [PATCH v7 1/6] Bluetooth: btbcm: Move request/release_firmware()

[Marcel Holtmann]

Hi Fred,

> Move request/release_firmware() out of btbcm_patchram().
> This allows a better error management, if request_firmware() returns an
> error then the controller will be used without firmware loading and 0 is
> returned.
> This will imply to change btbcm_patchram() to accept a firmware instead
> of firmware name.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/btbcm.c | 26 ++++++++++++--------------
> drivers/bluetooth/btbcm.h |  6 ++++--
> 2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> index 728fce3..c7aec97 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -89,21 +89,14 @@ int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> }
> EXPORT_SYMBOL_GPL(btbcm_set_bdaddr);
> 
> -int btbcm_patchram(struct hci_dev *hdev, const char *firmware)
> +int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw)
> {
> 	const struct hci_command_hdr *cmd;
> -	const struct firmware *fw;
> 	const u8 *fw_ptr;
> 	size_t fw_size;
> 	struct sk_buff *skb;
> 	u16 opcode;
> -	int err;
> -
> -	err = request_firmware(&fw, firmware, &hdev->dev);
> -	if (err < 0) {
> -		BT_INFO("%s: BCM: Patch %s not found", hdev->name, firmware);
> -		return err;
> -	}
> +	int err = 0;
> 
> 	/* Start Download */
> 	skb = __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT);
> @@ -129,8 +122,7 @@ int btbcm_patchram(struct hci_dev *hdev, const char *firmware)
> 		fw_size -= sizeof(*cmd);
> 
> 		if (fw_size < cmd->plen) {
> -			BT_ERR("%s: BCM: Patch %s is corrupted", hdev->name,
> -			       firmware);
> +			BT_ERR("%s: BCM: Patch is corrupted", hdev->name);
> 			err = -EINVAL;
> 			goto done;
> 		}
> @@ -156,7 +148,6 @@ int btbcm_patchram(struct hci_dev *hdev, const char *firmware)
> 	msleep(250);
> 
> done:
> -	release_firmware(fw);
> 	return err;
> }
> EXPORT_SYMBOL(btbcm_patchram);
> @@ -265,6 +256,7 @@ static const struct {
> int btbcm_setup_patchram(struct hci_dev *hdev)
> {
> 	char fw_name[64];
> +	const struct firmware *fw;
> 	u16 subver, rev, pid, vid;
> 	const char *hw_name = NULL;
> 	struct sk_buff *skb;
> @@ -335,9 +327,15 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
> 		hw_name ? : "BCM", (subver & 0x7000) >> 13,
> 		(subver & 0x1f00) >> 8, (subver & 0x00ff), rev & 0x0fff);
> 
> -	err = btbcm_patchram(hdev, fw_name);
> -	if (err == -ENOENT)
> +	err = request_firmware(&fw, fw_name, &hdev->dev);
> +	if (err < 0) {
> +		BT_INFO("%s: BCM: Patch %s not found", hdev->name, fw_name);
> 		return 0;
> +	}
> +
> +	btbcm_patchram(hdev, fw);
> +
> +	release_firmware(fw);
> 
> 	/* Reset */
> 	err = btbcm_reset(hdev);
> diff --git a/drivers/bluetooth/btbcm.h b/drivers/bluetooth/btbcm.h
> index eb6ab5f..de11ace 100644
> --- a/drivers/bluetooth/btbcm.h
> +++ b/drivers/bluetooth/btbcm.h
> @@ -21,11 +21,13 @@
>  *
>  */
> 
> +#include <linux/firmware.h>
> +

I removed this include and put it into the files that reference this.

Regards

Marcel

[PATCH v7 2/6] Bluetooth: btbcm: Add BCM4324B3 UART device

[Frederic Danis]

Add "waiting for configuration" address.
Add lmp_subver and firmware name for BCM4324B3 controller.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/btbcm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index c7aec97..e90e6b8 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -33,6 +33,7 @@
 #define VERSION "0.1"
 
 #define BDADDR_BCM20702A0 (&(bdaddr_t) {{0x00, 0xa0, 0x02, 0x70, 0x20, 0x00}})
+#define BDADDR_BCM4324B3 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb3, 0x24, 0x43}})
 
 int btbcm_check_bdaddr(struct hci_dev *hdev)
 {
@@ -56,10 +57,11 @@ int btbcm_check_bdaddr(struct hci_dev *hdev)
 
 	bda = (struct hci_rp_read_bd_addr *)skb->data;
 
-	/* The address 00:20:70:02:A0:00 indicates a BCM20702A0 controller
-	 * with no configured address.
+	/* Check if the address indicates a controller with no configured
+	 * address.
 	 */
-	if (!bacmp(&bda->bdaddr, BDADDR_BCM20702A0)) {
+	if (!bacmp(&bda->bdaddr, BDADDR_BCM20702A0) ||
+	    !bacmp(&bda->bdaddr, BDADDR_BCM4324B3)) {
 		BT_INFO("%s: BCM: Using default device address (%pMR)",
 			hdev->name, &bda->bdaddr);
 		set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
@@ -233,6 +235,7 @@ static const struct {
 	const char *name;
 } bcm_uart_subver_table[] = {
 	{ 0x410e, "BCM43341B0"	},	/* 002.001.014 */
+	{ 0x4406, "BCM4324B3"	},	/* 002.004.006 */
 	{ }
 };
 
@@ -288,6 +291,7 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
 
 	switch ((rev & 0xf000) >> 12) {
 	case 0:
+	case 3:
 		for (i = 0; bcm_uart_subver_table[i].name; i++) {
 			if (subver == bcm_uart_subver_table[i].subver) {
 				hw_name = bcm_uart_subver_table[i].name;
-- 
1.9.1

Re: [PATCH v7 2/6] Bluetooth: btbcm: Add BCM4324B3 UART device

[Marcel Holtmann]

Hi Fred,

> Add "waiting for configuration" address.
> Add lmp_subver and firmware name for BCM4324B3 controller.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/btbcm.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> index c7aec97..e90e6b8 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -33,6 +33,7 @@
> #define VERSION "0.1"
> 
> #define BDADDR_BCM20702A0 (&(bdaddr_t) {{0x00, 0xa0, 0x02, 0x70, 0x20, 0x00}})
> +#define BDADDR_BCM4324B3 (&(bdaddr_t) {{0x00, 0x00, 0x00, 0xb3, 0x24, 0x43}})
> 
> int btbcm_check_bdaddr(struct hci_dev *hdev)
> {
> @@ -56,10 +57,11 @@ int btbcm_check_bdaddr(struct hci_dev *hdev)
> 
> 	bda = (struct hci_rp_read_bd_addr *)skb->data;
> 
> -	/* The address 00:20:70:02:A0:00 indicates a BCM20702A0 controller
> -	 * with no configured address.
> +	/* Check if the address indicates a controller with no configured
> +	 * address.
> 	 */

I did like the comment that shows the address in human readable format. So I re-phrased this and mentioned both addresses. It is just a lot easier to grep for in case anybody ever wants to find this.

Regards

Marcel

[PATCH v7 3/6] Bluetooth: hci_uart: Support operational speed during setup

[Frederic Danis]

Add initial and operational speeds.
Change to operational speed as soon as possible. If controller
set_baudrate() fails, continue at initial speed.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_ldisc.c | 26 ++++++++++++++++++++++++++
 drivers/bluetooth/hci_uart.h  |  4 ++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 5c9a73f..a39e1ab 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -265,11 +265,37 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }
 
+void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
+{
+	struct tty_struct *tty = hu->tty;
+	struct ktermios ktermios;
+
+	ktermios = tty->termios;
+	ktermios.c_cflag &= ~CBAUD;
+	ktermios.c_cflag |= BOTHER;
+	tty_termios_encode_baud_rate(&ktermios, speed, speed);
+
+	/* tty_set_termios() return not checked as it is always 0 */
+	tty_set_termios(tty, &ktermios);
+
+	BT_DBG("%s: New tty speed: %d", hu->hdev->name, tty->termios.c_ispeed);
+}
+
 static int hci_uart_setup(struct hci_dev *hdev)
 {
 	struct hci_uart *hu = hci_get_drvdata(hdev);
 	struct hci_rp_read_local_version *ver;
 	struct sk_buff *skb;
+	int err;
+
+	if (hu->proto->init_speed)
+		hci_uart_set_baudrate(hu, hu->proto->init_speed);
+
+	if (hu->proto->set_baudrate && hu->proto->oper_speed) {
+		err = hu->proto->set_baudrate(hu, hu->proto->oper_speed);
+		if (!err)
+			hci_uart_set_baudrate(hu, hu->proto->oper_speed);
+	}
 
 	if (hu->proto->setup)
 		return hu->proto->setup(hu);
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 72120a5..e9f970c 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -58,10 +58,13 @@ struct hci_uart;
 struct hci_uart_proto {
 	unsigned int id;
 	const char *name;
+	unsigned int init_speed;
+	unsigned int oper_speed;
 	int (*open)(struct hci_uart *hu);
 	int (*close)(struct hci_uart *hu);
 	int (*flush)(struct hci_uart *hu);
 	int (*setup)(struct hci_uart *hu);
+	int (*set_baudrate)(struct hci_uart *hu, unsigned int speed);
 	int (*recv)(struct hci_uart *hu, const void *data, int len);
 	int (*enqueue)(struct hci_uart *hu, struct sk_buff *skb);
 	struct sk_buff *(*dequeue)(struct hci_uart *hu);
@@ -96,6 +99,7 @@ int hci_uart_register_proto(const struct hci_uart_proto *p);
 int hci_uart_unregister_proto(const struct hci_uart_proto *p);
 int hci_uart_tx_wakeup(struct hci_uart *hu);
 int hci_uart_init_ready(struct hci_uart *hu);
+void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed);
 
 #ifdef CONFIG_BT_HCIUART_H4
 int h4_init(void);
-- 
1.9.1

[PATCH v7 4/6] Bluetooth: btbcm: Add helper functions for UART setup

[Frederic Danis]

Firmware loading may reset the controller UART speed and needs to set
host UART speed back to init speed.

UART drivers setup is split in 3 parts:
- btbcm_initialize() resets the controller and returns the firmware
  name based on controller revision and sub_version.
- btbtcm_patchram() (already existing and public), which takes the
  firmware name as parameter, requests the firmware and loads it to
  the controller.
- btbcm_finalize() which resets the controller, reads local version
  and checks if the controller address is a default one or not.

Remove firmware name retrieval for UART controllers from
btbcm_setup_patchram().

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/btbcm.c | 101 ++++++++++++++++++++++++++++++++++++++++------
 drivers/bluetooth/btbcm.h |  14 +++++++
 2 files changed, 103 insertions(+), 12 deletions(-)

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index e90e6b8..d16878e 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -239,6 +239,95 @@ static const struct {
 	{ }
 };
 
+int btbcm_initialize(struct hci_dev *hdev, char *fw_name, size_t len)
+{
+	u16 subver, rev;
+	const char *hw_name = NULL;
+	struct sk_buff *skb;
+	struct hci_rp_read_local_version *ver;
+	int i, err;
+
+	/* Reset */
+	err = btbcm_reset(hdev);
+	if (err)
+		return err;
+
+	/* Read Local Version Info */
+	skb = btbcm_read_local_version(hdev);
+	if (IS_ERR(skb))
+		return PTR_ERR(skb);
+
+	ver = (struct hci_rp_read_local_version *)skb->data;
+	rev = le16_to_cpu(ver->hci_rev);
+	subver = le16_to_cpu(ver->lmp_subver);
+	kfree_skb(skb);
+
+	/* Read Verbose Config Version Info */
+	skb = btbcm_read_verbose_config(hdev);
+	if (IS_ERR(skb))
+		return PTR_ERR(skb);
+
+	BT_INFO("%s: BCM: chip id %u", hdev->name, skb->data[1]);
+	kfree_skb(skb);
+
+	switch ((rev & 0xf000) >> 12) {
+	case 0:
+	case 3:
+		for (i = 0; bcm_uart_subver_table[i].name; i++) {
+			if (subver == bcm_uart_subver_table[i].subver) {
+				hw_name = bcm_uart_subver_table[i].name;
+				break;
+			}
+		}
+
+		snprintf(fw_name, len, "brcm/%s.hcd", hw_name ? : "BCM");
+		break;
+	default:
+		return 0;
+	}
+
+	BT_INFO("%s: %s (%3.3u.%3.3u.%3.3u) build %4.4u", hdev->name,
+		hw_name ? : "BCM", (subver & 0x7000) >> 13,
+		(subver & 0x1f00) >> 8, (subver & 0x00ff), rev & 0x0fff);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(btbcm_initialize);
+
+int btbcm_finalize(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+	struct hci_rp_read_local_version *ver;
+	u16 subver, rev;
+	int err;
+
+	/* Reset */
+	err = btbcm_reset(hdev);
+	if (err)
+		return err;
+
+	/* Read Local Version Info */
+	skb = btbcm_read_local_version(hdev);
+	if (IS_ERR(skb))
+		return PTR_ERR(skb);
+
+	ver = (struct hci_rp_read_local_version *)skb->data;
+	rev = le16_to_cpu(ver->hci_rev);
+	subver = le16_to_cpu(ver->lmp_subver);
+	kfree_skb(skb);
+
+	BT_INFO("%s: BCM (%3.3u.%3.3u.%3.3u) build %4.4u", hdev->name,
+		(subver & 0x7000) >> 13, (subver & 0x1f00) >> 8,
+		(subver & 0x00ff), rev & 0x0fff);
+
+	btbcm_check_bdaddr(hdev);
+
+	set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(btbcm_finalize);
+
 static const struct {
 	u16 subver;
 	const char *name;
@@ -290,18 +379,6 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
 	kfree_skb(skb);
 
 	switch ((rev & 0xf000) >> 12) {
-	case 0:
-	case 3:
-		for (i = 0; bcm_uart_subver_table[i].name; i++) {
-			if (subver == bcm_uart_subver_table[i].subver) {
-				hw_name = bcm_uart_subver_table[i].name;
-				break;
-			}
-		}
-
-		snprintf(fw_name, sizeof(fw_name), "brcm/%s.hcd",
-			 hw_name ? : "BCM");
-		break;
 	case 1:
 	case 2:
 		/* Read USB Product Info */
diff --git a/drivers/bluetooth/btbcm.h b/drivers/bluetooth/btbcm.h
index de11ace..6f1e418 100644
--- a/drivers/bluetooth/btbcm.h
+++ b/drivers/bluetooth/btbcm.h
@@ -32,6 +32,9 @@ int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw);
 int btbcm_setup_patchram(struct hci_dev *hdev);
 int btbcm_setup_apple(struct hci_dev *hdev);
 
+int btbcm_initialize(struct hci_dev *hdev, char *fw_name, size_t len);
+int btbcm_finalize(struct hci_dev *hdev);
+
 #else
 
 static inline int btbcm_check_bdaddr(struct hci_dev *hdev)
@@ -59,4 +62,15 @@ static inline int btbcm_setup_apple(struct hci_dev *hdev)
 	return 0;
 }
 
+static inline int btbcm_initialize(struct hci_dev *hdev, char *fw_name,
+				   size_t len)
+{
+	return 0;
+}
+
+static inline int btbcm_finalize(struct hci_dev *hdev)
+{
+	return 0;
+}
+
 #endif
-- 
1.9.1

Re: [PATCH v7 4/6] Bluetooth: btbcm: Add helper functions for UART setup

[Marcel Holtmann]

Hi Fred,

> Firmware loading may reset the controller UART speed and needs to set
> host UART speed back to init speed.
> 
> UART drivers setup is split in 3 parts:
> - btbcm_initialize() resets the controller and returns the firmware
>  name based on controller revision and sub_version.
> - btbtcm_patchram() (already existing and public), which takes the
>  firmware name as parameter, requests the firmware and loads it to
>  the controller.
> - btbcm_finalize() which resets the controller, reads local version
>  and checks if the controller address is a default one or not.
> 
> Remove firmware name retrieval for UART controllers from
> btbcm_setup_patchram().
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/btbcm.c | 101 ++++++++++++++++++++++++++++++++++++++++------
> drivers/bluetooth/btbcm.h |  14 +++++++
> 2 files changed, 103 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> index e90e6b8..d16878e 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -239,6 +239,95 @@ static const struct {
> 	{ }
> };
> 
> +int btbcm_initialize(struct hci_dev *hdev, char *fw_name, size_t len)
> +{
> +	u16 subver, rev;
> +	const char *hw_name = NULL;
> +	struct sk_buff *skb;
> +	struct hci_rp_read_local_version *ver;
> +	int i, err;
> +
> +	/* Reset */
> +	err = btbcm_reset(hdev);
> +	if (err)
> +		return err;
> +
> +	/* Read Local Version Info */
> +	skb = btbcm_read_local_version(hdev);
> +	if (IS_ERR(skb))
> +		return PTR_ERR(skb);
> +
> +	ver = (struct hci_rp_read_local_version *)skb->data;
> +	rev = le16_to_cpu(ver->hci_rev);
> +	subver = le16_to_cpu(ver->lmp_subver);
> +	kfree_skb(skb);
> +
> +	/* Read Verbose Config Version Info */
> +	skb = btbcm_read_verbose_config(hdev);
> +	if (IS_ERR(skb))
> +		return PTR_ERR(skb);
> +
> +	BT_INFO("%s: BCM: chip id %u", hdev->name, skb->data[1]);
> +	kfree_skb(skb);
> +
> +	switch ((rev & 0xf000) >> 12) {
> +	case 0:
> +	case 3:
> +		for (i = 0; bcm_uart_subver_table[i].name; i++) {
> +			if (subver == bcm_uart_subver_table[i].subver) {
> +				hw_name = bcm_uart_subver_table[i].name;
> +				break;
> +			}
> +		}
> +
> +		snprintf(fw_name, len, "brcm/%s.hcd", hw_name ? : "BCM");
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	BT_INFO("%s: %s (%3.3u.%3.3u.%3.3u) build %4.4u", hdev->name,
> +		hw_name ? : "BCM", (subver & 0x7000) >> 13,
> +		(subver & 0x1f00) >> 8, (subver & 0x00ff), rev & 0x0fff);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(btbcm_initialize);
> +
> +int btbcm_finalize(struct hci_dev *hdev)
> +{
> +	struct sk_buff *skb;
> +	struct hci_rp_read_local_version *ver;
> +	u16 subver, rev;
> +	int err;
> +
> +	/* Reset */
> +	err = btbcm_reset(hdev);
> +	if (err)
> +		return err;
> +
> +	/* Read Local Version Info */
> +	skb = btbcm_read_local_version(hdev);
> +	if (IS_ERR(skb))
> +		return PTR_ERR(skb);
> +
> +	ver = (struct hci_rp_read_local_version *)skb->data;
> +	rev = le16_to_cpu(ver->hci_rev);
> +	subver = le16_to_cpu(ver->lmp_subver);
> +	kfree_skb(skb);
> +
> +	BT_INFO("%s: BCM (%3.3u.%3.3u.%3.3u) build %4.4u", hdev->name,
> +		(subver & 0x7000) >> 13, (subver & 0x1f00) >> 8,
> +		(subver & 0x00ff), rev & 0x0fff);
> +
> +	btbcm_check_bdaddr(hdev);
> +
> +	set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(btbcm_finalize);
> +
> static const struct {
> 	u16 subver;
> 	const char *name;
> @@ -290,18 +379,6 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
> 	kfree_skb(skb);
> 
> 	switch ((rev & 0xf000) >> 12) {
> -	case 0:
> -	case 3:
> -		for (i = 0; bcm_uart_subver_table[i].name; i++) {
> -			if (subver == bcm_uart_subver_table[i].subver) {
> -				hw_name = bcm_uart_subver_table[i].name;
> -				break;
> -			}
> -		}
> -
> -		snprintf(fw_name, sizeof(fw_name), "brcm/%s.hcd",
> -			 hw_name ? : "BCM");
> -		break;

I took this hunk out since I am pretty sure I found an USB dongle that has this gotten wrong. Long term we will move over to a manifest file that will list modalias and firmware name. That way this becomes a detail of the linux-firmware tree and no longer a driver problem.

Regards

Marcel

[PATCH v7 5/6] Bluetooth: hci_uart: Update Broadcom UART setup

[Frederic Danis]

Use btbcm helpers to perform controller setup.
Perform host UART reset to init speed between btbcm_patchram() and
btbcm_finalize(). This may be need because firmware loading may have
reseted controller UART to init speed.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 1ec0b4a..6c34135 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -79,11 +79,39 @@ static int bcm_flush(struct hci_uart *hu)
 
 static int bcm_setup(struct hci_uart *hu)
 {
+	char fw_name[64];
+	const struct firmware *fw;
+	int err;
+
 	BT_DBG("hu %p", hu);
 
 	hu->hdev->set_bdaddr = btbcm_set_bdaddr;
 
-	return btbcm_setup_patchram(hu->hdev);
+	err = btbcm_initialize(hu->hdev, fw_name, sizeof(fw_name));
+	if (err)
+		return err;
+
+	err = request_firmware(&fw, fw_name, &hu->hdev->dev);
+	if (err < 0) {
+		BT_INFO("%s: BCM: Patch %s not found", hu->hdev->name, fw_name);
+		return 0;
+	}
+
+	err = btbcm_patchram(hu->hdev, fw);
+	if (err) {
+		BT_INFO("%s: BCM: Patch failed (%d)", hu->hdev->name, err);
+		goto finalize;
+	}
+
+	if (hu->proto->init_speed)
+		hci_uart_set_baudrate(hu, hu->proto->init_speed);
+
+finalize:
+	release_firmware(fw);
+
+	err = btbcm_finalize(hu->hdev);
+
+	return err;
 }
 
 static const struct h4_recv_pkt bcm_recv_pkts[] = {
-- 
1.9.1

[PATCH v7 6/6] Bluetooth: hci_uart: Add bcm_set_baudrate()

[Frederic Danis]

Add vendor specific command to change controller device speed.

Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
---
 drivers/bluetooth/hci_bcm.c | 56 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 6c34135..0cbf9d4 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -24,6 +24,7 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/skbuff.h>
+#include <linux/tty.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -31,11 +32,55 @@
 #include "btbcm.h"
 #include "hci_uart.h"
 
+#define BCM43XX_CLOCK_48 1
+#define BCM43XX_CLOCK_24 2
+
 struct bcm_data {
 	struct sk_buff *rx_skb;
 	struct sk_buff_head txq;
 };
 
+struct hci_cp_bcm_set_speed {
+	__le16   dummy;
+	__le32   speed;
+} __packed;
+
+static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
+{
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+	struct hci_cp_bcm_set_speed param = { 0, cpu_to_le32(speed) };
+
+	if (speed > 3000000) {
+		u8 clock = BCM43XX_CLOCK_48;
+
+		BT_DBG("%s: Set Controller clock (%d)", hdev->name, clock);
+
+		skb = __hci_cmd_sync(hdev, 0xfc45, 1, &clock, HCI_INIT_TIMEOUT);
+		if (IS_ERR(skb)) {
+			BT_ERR("%s: failed to write update clock command (%ld)",
+			       hdev->name, PTR_ERR(skb));
+			return PTR_ERR(skb);
+		}
+
+		kfree_skb(skb);
+	}
+
+	BT_DBG("%s: Set Controller UART speed to %d bit/s", hdev->name, speed);
+
+	skb = __hci_cmd_sync(hdev, 0xfc18, sizeof(param), &param,
+			     HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		BT_ERR("%s: failed to write update baudrate command (%ld)",
+		       hdev->name, PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+
+	kfree_skb(skb);
+
+	return 0;
+}
+
 static int bcm_open(struct hci_uart *hu)
 {
 	struct bcm_data *bcm;
@@ -106,6 +151,14 @@ static int bcm_setup(struct hci_uart *hu)
 	if (hu->proto->init_speed)
 		hci_uart_set_baudrate(hu, hu->proto->init_speed);
 
+	if (hu->proto->oper_speed) {
+		err = bcm_set_baudrate(hu, hu->proto->oper_speed);
+		if (!err)
+			/* hci_uart_set_baudrate() has no return value as
+			   tty_set_termios() return is always 0 */
+			hci_uart_set_baudrate(hu, hu->proto->oper_speed);
+	}
+
 finalize:
 	release_firmware(fw);
 
@@ -161,10 +214,13 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 static const struct hci_uart_proto bcm_proto = {
 	.id		= HCI_UART_BCM,
 	.name		= "BCM",
+	.init_speed	= 115200,
+	.oper_speed	= 4000000,
 	.open		= bcm_open,
 	.close		= bcm_close,
 	.flush		= bcm_flush,
 	.setup		= bcm_setup,
+	.set_baudrate	= bcm_set_baudrate,
 	.recv		= bcm_recv,
 	.enqueue	= bcm_enqueue,
 	.dequeue	= bcm_dequeue,
-- 
1.9.1

Re: [PATCH v7 6/6] Bluetooth: hci_uart: Add bcm_set_baudrate()

[Marcel Holtmann]

Hi Fred,

> Add vendor specific command to change controller device speed.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/hci_bcm.c | 56 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 6c34135..0cbf9d4 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -24,6 +24,7 @@
> #include <linux/kernel.h>
> #include <linux/errno.h>
> #include <linux/skbuff.h>
> +#include <linux/tty.h>
> 
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -31,11 +32,55 @@
> #include "btbcm.h"
> #include "hci_uart.h"
> 
> +#define BCM43XX_CLOCK_48 1
> +#define BCM43XX_CLOCK_24 2
> +
> struct bcm_data {
> 	struct sk_buff *rx_skb;
> 	struct sk_buff_head txq;
> };
> 
> +struct hci_cp_bcm_set_speed {
> +	__le16   dummy;
> +	__le32   speed;
> +} __packed;

I think this data structure should go into btbcm.h with a little bit better name. This should be similar to the btintel.h where I am slowly trying to get the proper data structures put in place.



> +
> +static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
> +{
> +	struct hci_dev *hdev = hu->hdev;
> +	struct sk_buff *skb;
> +	struct hci_cp_bcm_set_speed param = { 0, cpu_to_le32(speed) };

I would prefer if we assign this one by one and use cpu_lo_le16(0) here as well. Just to remember that the value is actually in little endian.

> +
> +	if (speed > 3000000) {
> +		u8 clock = BCM43XX_CLOCK_48;
> +
> +		BT_DBG("%s: Set Controller clock (%d)", hdev->name, clock);
> +

I prefer having a comment above the vendor commands explain on what they do and why this is correct. That is as close as it gets for someone with the proper documentation to figure out why we are doing things. I think the Intel support is a good example on how far I have gone with this.

> +		skb = __hci_cmd_sync(hdev, 0xfc45, 1, &clock, HCI_INIT_TIMEOUT);
> +		if (IS_ERR(skb)) {
> +			BT_ERR("%s: failed to write update clock command (%ld)”,

Lets try to be a bit consistent with the error messages. I think we prefixed most of it with BCM: as well. Overall, I try to make this really consistent.

> +			       hdev->name, PTR_ERR(skb));
> +			return PTR_ERR(skb);
> +		}
> +
> +		kfree_skb(skb);
> +	}
> +
> +	BT_DBG("%s: Set Controller UART speed to %d bit/s", hdev->name, speed);
> +

Same here. Comments should be put above and assignment of the values to param maybe better places above here so you know exactly where they are coming from.

> +	skb = __hci_cmd_sync(hdev, 0xfc18, sizeof(param), &param,
> +			     HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		BT_ERR("%s: failed to write update baudrate command (%ld)”,

Same as the comment above with the BCM: prefix.

> +		       hdev->name, PTR_ERR(skb));
> +		return PTR_ERR(skb);
> +	}
> +
> +	kfree_skb(skb);
> +
> +	return 0;
> +}
> +
> static int bcm_open(struct hci_uart *hu)
> {
> 	struct bcm_data *bcm;
> @@ -106,6 +151,14 @@ static int bcm_setup(struct hci_uart *hu)
> 	if (hu->proto->init_speed)
> 		hci_uart_set_baudrate(hu, hu->proto->init_speed);
> 
> +	if (hu->proto->oper_speed) {
> +		err = bcm_set_baudrate(hu, hu->proto->oper_speed);
> +		if (!err)
> +			/* hci_uart_set_baudrate() has no return value as
> +			   tty_set_termios() return is always 0 */

This comment is just duplicating it. Especially since above you do not comment it. So in this case I would remove it.

> +			hci_uart_set_baudrate(hu, hu->proto->oper_speed);
> +	}
> +
> finalize:
> 	release_firmware(fw);
> 
> @@ -161,10 +214,13 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
> static const struct hci_uart_proto bcm_proto = {
> 	.id		= HCI_UART_BCM,
> 	.name		= "BCM",
> +	.init_speed	= 115200,
> +	.oper_speed	= 4000000,
> 	.open		= bcm_open,
> 	.close		= bcm_close,
> 	.flush		= bcm_flush,
> 	.setup		= bcm_setup,
> +	.set_baudrate	= bcm_set_baudrate,
> 	.recv		= bcm_recv,
> 	.enqueue	= bcm_enqueue,
> 	.dequeue	= bcm_dequeue,

Regards

Marcel

Re: [PATCH v7 0/6] Add baudrate management for Bluetooth UART

[Marcel Holtmann]

Hi Fred,

> This patch set introduce baudrate management for Bluetooth UART
> Controller setup and a first implementation for Broadcom BCM4324B3
> used in Asus T100.
> 
> v6 -> v7:
> - Fix setup when firmware is missing by moving
>  request/release_firmware() out of btbcm_patchram().
> 
> v2 -> v6:
> - Fixes for __hci_cmd_sync() has been integrated (patch v5)
> - In btbcm_check_bdaddr(), merge controller address tests
> - Fix declaration of speeds to use "unsigned int" type
> - Rename default_speed to init_speed
> - Rename operational_speed to oper_speed
> - In case of error during vendor specific set_baudrate(), do not change
>  host baudrate and continue without changing speed
> - Discard changes to bcm_setup_patchram() and in btusb.c
> - Introduce helper functions btbcm_initialize() and btbcm_finalize() to
>  be used by the UART side of things.
> 
> Frederic Danis (6):
>  Bluetooth: btbcm: Move request/release_firmware()
>  Bluetooth: btbcm: Add BCM4324B3 UART device
>  Bluetooth: hci_uart: Support operational speed during setup
>  Bluetooth: btbcm: Add helper functions for UART setup
>  Bluetooth: hci_uart: Update Broadcom UART setup
>  Bluetooth: hci_uart: Add bcm_set_baudrate()
> 
> drivers/bluetooth/btbcm.c     | 135 +++++++++++++++++++++++++++++++++---------
> drivers/bluetooth/btbcm.h     |  20 ++++++-
> drivers/bluetooth/hci_bcm.c   |  86 ++++++++++++++++++++++++++-
> drivers/bluetooth/hci_ldisc.c |  26 ++++++++
> drivers/bluetooth/hci_uart.h  |   4 ++
> 5 files changed, 240 insertions(+), 31 deletions(-)

patches 1-5 have been applied to bluetooth-next tree.

However I did a few minor modifications as mentioned in the separate emails. They were mainly cosmetic and easy to do inline so that you do not have to worry about redoing the whole series. However patch 6 needs a bit of extra work.

Regards

Marcel