[PATCH v6 0/5] Add baudrate management for Bluetooth UART

[PATCH v6 0/5] 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.

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 (5):
  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     | 109 ++++++++++++++++++++++++++++++++++++------
 drivers/bluetooth/btbcm.h     |  14 ++++++
 drivers/bluetooth/hci_bcm.c   |  75 ++++++++++++++++++++++++++++-
 drivers/bluetooth/hci_ldisc.c |  26 ++++++++++
 drivers/bluetooth/hci_uart.h  |   4 ++
 5 files changed, 213 insertions(+), 15 deletions(-)

-- 
1.9.1

[PATCH v6 1/5] 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 728fce3..acb9171 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);
@@ -242,6 +244,7 @@ static const struct {
 	const char *name;
 } bcm_uart_subver_table[] = {
 	{ 0x410e, "BCM43341B0"	},	/* 002.001.014 */
+	{ 0x4406, "BCM4324B3"	},	/* 002.004.006 */
 	{ }
 };
 
@@ -296,6 +299,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

[PATCH v6 2/5] 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 v6 3/5] 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 acb9171..3e3250c 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -248,6 +248,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;
@@ -298,18 +387,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 eb6ab5f..3c35a06 100644
--- a/drivers/bluetooth/btbcm.h
+++ b/drivers/bluetooth/btbcm.h
@@ -30,6 +30,9 @@ int btbcm_patchram(struct hci_dev *hdev, const char *firmware);
 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)
@@ -57,4 +60,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

[PATCH v6 4/5] 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 | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 1ec0b4a..cede445 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
 
 static int bcm_setup(struct hci_uart *hu)
 {
+	char fw_name[64];
+	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 = btbcm_patchram(hu->hdev, fw_name);
+	/* If there is no firmware (-ENOENT), discard the error and continue */
+	if (err == -ENOENT)
+		return 0;
+
+	if (hu->proto->init_speed)
+		hci_uart_set_baudrate(hu, hu->proto->init_speed);
+
+	err = btbcm_finalize(hu->hdev);
+
+	return err;
 }
 
 static const struct h4_recv_pkt bcm_recv_pkts[] = {
-- 
1.9.1

[PATCH v6 5/5] 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 cede445..b4fd532 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;
@@ -98,6 +143,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);
+	}
+
 	err = btbcm_finalize(hu->hdev);
 
 	return err;
@@ -150,10 +203,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 v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Arend van Spriel]

On 05/20/15 17:46, Frederic Danis wrote:
> 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 | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 1ec0b4a..cede445 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>
>   static int bcm_setup(struct hci_uart *hu)
>   {
> +	char fw_name[64];
> +	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 = btbcm_patchram(hu->hdev, fw_name);
> +	/* If there is no firmware (-ENOENT), discard the error and continue */

I guess -ENOENT means no firmware is required and not a 
request_firmware() failure. Not sure what is meant here.

Regards,
Arend

> +	if (err == -ENOENT)
> +		return 0;
> +
> +	if (hu->proto->init_speed)
> +		hci_uart_set_baudrate(hu, hu->proto->init_speed);
> +
> +	err = btbcm_finalize(hu->hdev);
> +
> +	return err;
>   }
>
>   static const struct h4_recv_pkt bcm_recv_pkts[] = {

Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Frederic Danis]

Hello Arend,

On 21/05/2015 15:50, Arend van Spriel wrote:
> On 05/20/15 17:46, Frederic Danis wrote:
>> 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 | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index 1ec0b4a..cede445 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>
>>   static int bcm_setup(struct hci_uart *hu)
>>   {
>> +    char fw_name[64];
>> +    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 = btbcm_patchram(hu->hdev, fw_name);
>> +    /* If there is no firmware (-ENOENT), discard the error and
>> continue */
>
> I guess -ENOENT means no firmware is required and not a
> request_firmware() failure. Not sure what is meant here.
>
> Regards,
> Arend
>
>> +    if (err == -ENOENT)
>> +        return 0;
>> +
>> +    if (hu->proto->init_speed)
>> +        hci_uart_set_baudrate(hu, hu->proto->init_speed);
>> +
>> +    err = btbcm_finalize(hu->hdev);
>> +
>> +    return err;
>>   }
>>
>>   static const struct h4_recv_pkt bcm_recv_pkts[] = {

You're right, btbcm_patchram() return test does not work as I expected.

I can change this by performing uart speed change only if it returns no 
error.

     err = btbcm_patchram(hu->hdev, fw_name);
     if (!err) {
	/* Firmware loading may have reseted controller UART to init speed */
         if (hu->proto->init_speed)
             hci_uart_set_baudrate(hu, hu->proto->init_speed);
     }

     err = btbcm_finalize(hu->hdev);


Or I can move request_firmware() out of btbcm_patchram() and test it 
before calling btbcm_patchram(). This will imply to change 
btbcm_patchram() to accept a firmware instead of firmware name.

     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) {
	/* Firmware loading may have reseted controller UART to init speed */
         if (hu->proto->init_speed)
             hci_uart_set_baudrate(hu, hu->proto->init_speed);
     }

     err = btbcm_finalize(hu->hdev);

Arend, Marcel, any advice regarding this ?

Regard

Fred

Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Arend van Spriel]

On 05/22/15 14:50, Frederic Danis wrote:
> Hello Arend,
>
> On 21/05/2015 15:50, Arend van Spriel wrote:
>> On 05/20/15 17:46, Frederic Danis wrote:
>>> 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 | 19 ++++++++++++++++++-
>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>> index 1ec0b4a..cede445 100644
>>> --- a/drivers/bluetooth/hci_bcm.c
>>> +++ b/drivers/bluetooth/hci_bcm.c
>>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>>
>>> static int bcm_setup(struct hci_uart *hu)
>>> {
>>> + char fw_name[64];
>>> + 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 = btbcm_patchram(hu->hdev, fw_name);
>>> + /* If there is no firmware (-ENOENT), discard the error and
>>> continue */
>>
>> I guess -ENOENT means no firmware is required and not a
>> request_firmware() failure. Not sure what is meant here.
>>
>> Regards,
>> Arend
>>
>>> + if (err == -ENOENT)
>>> + return 0;
>>> +
>>> + if (hu->proto->init_speed)
>>> + hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>> +
>>> + err = btbcm_finalize(hu->hdev);
>>> +
>>> + return err;
>>> }
>>>
>>> static const struct h4_recv_pkt bcm_recv_pkts[] = {
>
> You're right, btbcm_patchram() return test does not work as I expected.
>
> I can change this by performing uart speed change only if it returns no
> error.
>
> err = btbcm_patchram(hu->hdev, fw_name);
> if (!err) {
> /* Firmware loading may have reseted controller UART to init speed */
> if (hu->proto->init_speed)
> hci_uart_set_baudrate(hu, hu->proto->init_speed);
> }
>
> err = btbcm_finalize(hu->hdev);
>
>
> Or I can move request_firmware() out of btbcm_patchram() and test it
> before calling btbcm_patchram(). This will imply to change
> btbcm_patchram() to accept a firmware instead of firmware name.
>
> 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) {
> /* Firmware loading may have reseted controller UART to init speed */
> if (hu->proto->init_speed)
> hci_uart_set_baudrate(hu, hu->proto->init_speed);
> }
>
> err = btbcm_finalize(hu->hdev);
>
> Arend, Marcel, any advice regarding this ?

Well, functionally it does not make a difference as the request_firmware 
is the first call done in btbcm_patchram(). So if it solves your problem 
I would go for option 2. Other question: is there a reason why the error 
code from request_firmware is not returned?

Regards,
Arend

Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Arend van Spriel]

On 05/22/15 15:27, Arend van Spriel wrote:
> On 05/22/15 14:50, Frederic Danis wrote:
>> Hello Arend,
>>
>> On 21/05/2015 15:50, Arend van Spriel wrote:
>>> On 05/20/15 17:46, Frederic Danis wrote:
>>>> 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 | 19 ++++++++++++++++++-
>>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>> index 1ec0b4a..cede445 100644
>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>>>
>>>> static int bcm_setup(struct hci_uart *hu)
>>>> {
>>>> + char fw_name[64];
>>>> + 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 = btbcm_patchram(hu->hdev, fw_name);
>>>> + /* If there is no firmware (-ENOENT), discard the error and
>>>> continue */
>>>
>>> I guess -ENOENT means no firmware is required and not a
>>> request_firmware() failure. Not sure what is meant here.
>>>
>>> Regards,
>>> Arend
>>>
>>>> + if (err == -ENOENT)
>>>> + return 0;
>>>> +
>>>> + if (hu->proto->init_speed)
>>>> + hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>> +
>>>> + err = btbcm_finalize(hu->hdev);
>>>> +
>>>> + return err;
>>>> }
>>>>
>>>> static const struct h4_recv_pkt bcm_recv_pkts[] = {
>>
>> You're right, btbcm_patchram() return test does not work as I expected.
>>
>> I can change this by performing uart speed change only if it returns no
>> error.
>>
>> err = btbcm_patchram(hu->hdev, fw_name);
>> if (!err) {
>> /* Firmware loading may have reseted controller UART to init speed */
>> if (hu->proto->init_speed)
>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>> }
>>
>> err = btbcm_finalize(hu->hdev);
>>
>>
>> Or I can move request_firmware() out of btbcm_patchram() and test it
>> before calling btbcm_patchram(). This will imply to change
>> btbcm_patchram() to accept a firmware instead of firmware name.
>>
>> 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) {
>> /* Firmware loading may have reseted controller UART to init speed */
>> if (hu->proto->init_speed)
>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>> }
>>
>> err = btbcm_finalize(hu->hdev);
>>
>> Arend, Marcel, any advice regarding this ?
>
> Well, functionally it does not make a difference as the request_firmware
> is the first call done in btbcm_patchram(). So if it solves your problem
> I would go for option 2. Other question: is there a reason why the error
> code from request_firmware is not returned?

Forgot to mention but if going for option 2) also move release_firmware 
call out of btbcm_patchram().

Regards,
Arend

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

Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Frederic Danis]

On 22/05/2015 15:27, Arend van Spriel wrote:
> On 05/22/15 14:50, Frederic Danis wrote:
>> Hello Arend,
>>
>> On 21/05/2015 15:50, Arend van Spriel wrote:
>>> On 05/20/15 17:46, Frederic Danis wrote:
>>>> 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 | 19 ++++++++++++++++++-
>>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>> index 1ec0b4a..cede445 100644
>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>>>
>>>> static int bcm_setup(struct hci_uart *hu)
>>>> {
>>>> + char fw_name[64];
>>>> + 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 = btbcm_patchram(hu->hdev, fw_name);
>>>> + /* If there is no firmware (-ENOENT), discard the error and
>>>> continue */
>>>
>>> I guess -ENOENT means no firmware is required and not a
>>> request_firmware() failure. Not sure what is meant here.
>>>
>>> Regards,
>>> Arend
>>>
>>>> + if (err == -ENOENT)
>>>> + return 0;
>>>> +
>>>> + if (hu->proto->init_speed)
>>>> + hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>> +
>>>> + err = btbcm_finalize(hu->hdev);
>>>> +
>>>> + return err;
>>>> }
>>>>
>>>> static const struct h4_recv_pkt bcm_recv_pkts[] = {
>>
>> You're right, btbcm_patchram() return test does not work as I expected.
>>
>> I can change this by performing uart speed change only if it returns no
>> error.
>>
>> err = btbcm_patchram(hu->hdev, fw_name);
>> if (!err) {
>> /* Firmware loading may have reseted controller UART to init speed */
>> if (hu->proto->init_speed)
>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>> }
>>
>> err = btbcm_finalize(hu->hdev);
>>
>>
>> Or I can move request_firmware() out of btbcm_patchram() and test it
>> before calling btbcm_patchram(). This will imply to change
>> btbcm_patchram() to accept a firmware instead of firmware name.
>>
>> 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) {
>> /* Firmware loading may have reseted controller UART to init speed */
>> if (hu->proto->init_speed)
>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>> }
>>
>> err = btbcm_finalize(hu->hdev);
>>
>> Arend, Marcel, any advice regarding this ?
>
> Well, functionally it does not make a difference as the request_firmware
> is the first call done in btbcm_patchram(). So if it solves your problem
> I would go for option 2. Other question: is there a reason why the error
> code from request_firmware is not returned?

If an error is returned it will stop Bluetooth setup.
So, when request_firmware() returns an error, the Bluetooth controller 
will be used with current firmware and 0 is returned as bcm_setup is 
completed.

Regards

Fred

RE: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Ilya Faenson]

Hi Fred,

________________________________________
From: Frederic Danis [frederic.danis@linux.intel.com]
Sent: Friday, May 22, 2015 9:49 AM
To: Arend Van Spriel
Cc: Marcel Holtmann; linux-bluetooth@vger.kernel.org; Ilya Faenson
Subject: Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

On 22/05/2015 15:27, Arend van Spriel wrote:
> On 05/22/15 14:50, Frederic Danis wrote:
>> Hello Arend,
>>
>> On 21/05/2015 15:50, Arend van Spriel wrote:
>>> On 05/20/15 17:46, Frederic Danis wrote:
>>>> 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 | 19 ++++++++++++++++++-
>>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>> index 1ec0b4a..cede445 100644
>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>>>
>>>> static int bcm_setup(struct hci_uart *hu)
>>>> {
>>>> + char fw_name[64];
>>>> + int err;
>>>> +
>>>> BT_DBG("hu %p", hu);
>>>>
>>>> hu->hdev->set_bdaddr =3D btbcm_set_bdaddr;
>>>>
>>>> - return btbcm_setup_patchram(hu->hdev);
>>>> + err =3D btbcm_initialize(hu->hdev, fw_name, sizeof(fw_name));
>>>> + if (err)
>>>> + return err;
>>>> +
>>>> + err =3D btbcm_patchram(hu->hdev, fw_name);
>>>> + /* If there is no firmware (-ENOENT), discard the error and
>>>> continue */
>>>
>>> I guess -ENOENT means no firmware is required and not a
>>> request_firmware() failure. Not sure what is meant here.
>>>
>>> Regards,
>>> Arend
>>>
>>>> + if (err =3D=3D -ENOENT)
>>>> + return 0;
>>>> +
>>>> + if (hu->proto->init_speed)
>>>> + hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>> +
>>>> + err =3D btbcm_finalize(hu->hdev);
>>>> +
>>>> + return err;
>>>> }
>>>>
>>>> static const struct h4_recv_pkt bcm_recv_pkts[] =3D {
>>
>> You're right, btbcm_patchram() return test does not work as I expected.
>>
>> I can change this by performing uart speed change only if it returns no
>> error.
>>
>> err =3D btbcm_patchram(hu->hdev, fw_name);
>> if (!err) {
>> /* Firmware loading may have reseted controller UART to init speed */
>> if (hu->proto->init_speed)
>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>> }
>>
>> err =3D btbcm_finalize(hu->hdev);
>>
>>
>> Or I can move request_firmware() out of btbcm_patchram() and test it
>> before calling btbcm_patchram(). This will imply to change
>> btbcm_patchram() to accept a firmware instead of firmware name.
>>
>> err =3D 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 =3D btbcm_patchram(hu->hdev, fw);
>> if (!err) {
>> /* Firmware loading may have reseted controller UART to init speed */
>> if (hu->proto->init_speed)
>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>> }
>>
>> err =3D btbcm_finalize(hu->hdev);
>>
>> Arend, Marcel, any advice regarding this ?
>
> Well, functionally it does not make a difference as the request_firmware
> is the first call done in btbcm_patchram(). So if it solves your problem
> I would go for option 2. Other question: is there a reason why the error
> code from request_firmware is not returned?

If an error is returned it will stop Bluetooth setup.
So, when request_firmware() returns an error, the Bluetooth controller
will be used with current firmware and 0 is returned as bcm_setup is
completed.

IF: There is generally no "current firmware" if the firmware download fails=
 or not attempted. The device would run out of the ROM then which is not te=
rribly useful if Bluetooth functionality is needed. Thanks, -Ilya

Regards

Fred=

Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Frederic Danis]

Hello Ilya,

On 22/05/2015 15:55, Ilya Faenson wrote:
> Hi Fred,
>
> ________________________________________
> From: Frederic Danis [frederic.danis@linux.intel.com]
> Sent: Friday, May 22, 2015 9:49 AM
> To: Arend Van Spriel
> Cc: Marcel Holtmann; linux-bluetooth@vger.kernel.org; Ilya Faenson
> Subject: Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup
>
> On 22/05/2015 15:27, Arend van Spriel wrote:
>> On 05/22/15 14:50, Frederic Danis wrote:
>>> Hello Arend,
>>>
>>> On 21/05/2015 15:50, Arend van Spriel wrote:
>>>> On 05/20/15 17:46, Frederic Danis wrote:
>>>>> 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 | 19 ++++++++++++++++++-
>>>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>>> index 1ec0b4a..cede445 100644
>>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>>>>
>>>>> static int bcm_setup(struct hci_uart *hu)
>>>>> {
>>>>> + char fw_name[64];
>>>>> + 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 = btbcm_patchram(hu->hdev, fw_name);
>>>>> + /* If there is no firmware (-ENOENT), discard the error and
>>>>> continue */
>>>>
>>>> I guess -ENOENT means no firmware is required and not a
>>>> request_firmware() failure. Not sure what is meant here.
>>>>
>>>> Regards,
>>>> Arend
>>>>
>>>>> + if (err == -ENOENT)
>>>>> + return 0;
>>>>> +
>>>>> + if (hu->proto->init_speed)
>>>>> + hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>> +
>>>>> + err = btbcm_finalize(hu->hdev);
>>>>> +
>>>>> + return err;
>>>>> }
>>>>>
>>>>> static const struct h4_recv_pkt bcm_recv_pkts[] = {
>>>
>>> You're right, btbcm_patchram() return test does not work as I expected.
>>>
>>> I can change this by performing uart speed change only if it returns no
>>> error.
>>>
>>> err = btbcm_patchram(hu->hdev, fw_name);
>>> if (!err) {
>>> /* Firmware loading may have reseted controller UART to init speed */
>>> if (hu->proto->init_speed)
>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>> }
>>>
>>> err = btbcm_finalize(hu->hdev);
>>>
>>>
>>> Or I can move request_firmware() out of btbcm_patchram() and test it
>>> before calling btbcm_patchram(). This will imply to change
>>> btbcm_patchram() to accept a firmware instead of firmware name.
>>>
>>> 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) {
>>> /* Firmware loading may have reseted controller UART to init speed */
>>> if (hu->proto->init_speed)
>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>> }
>>>
>>> err = btbcm_finalize(hu->hdev);
>>>
>>> Arend, Marcel, any advice regarding this ?
>>
>> Well, functionally it does not make a difference as the request_firmware
>> is the first call done in btbcm_patchram(). So if it solves your problem
>> I would go for option 2. Other question: is there a reason why the error
>> code from request_firmware is not returned?
>
> If an error is returned it will stop Bluetooth setup.
> So, when request_firmware() returns an error, the Bluetooth controller
> will be used with current firmware and 0 is returned as bcm_setup is
> completed.
>
> IF: There is generally no "current firmware" if the firmware download fails or not attempted. The device would run out of the ROM then which is not terribly useful if Bluetooth functionality is needed. Thanks, -Ilya

The BCM4324B3 embedded in T100 is able to operate without firmware 
loading, at least performing Bluetooth discovery.
I think it is better to run without firmware loading than not allowing 
to use Bluetooth controller at all.

regards

Fred

RE: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Ilya Faenson]

Hi Fred,

-----Original Message-----
From: Frederic Danis [mailto:frederic.danis@linux.intel.com]=20
Sent: Tuesday, May 26, 2015 8:42 AM
To: Ilya Faenson; Arend Van Spriel
Cc: Marcel Holtmann; linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

Hello Ilya,

On 22/05/2015 15:55, Ilya Faenson wrote:
> Hi Fred,
>
> ________________________________________
> From: Frederic Danis [frederic.danis@linux.intel.com]
> Sent: Friday, May 22, 2015 9:49 AM
> To: Arend Van Spriel
> Cc: Marcel Holtmann; linux-bluetooth@vger.kernel.org; Ilya Faenson
> Subject: Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART set=
up
>
> On 22/05/2015 15:27, Arend van Spriel wrote:
>> On 05/22/15 14:50, Frederic Danis wrote:
>>> Hello Arend,
>>>
>>> On 21/05/2015 15:50, Arend van Spriel wrote:
>>>> On 05/20/15 17:46, Frederic Danis wrote:
>>>>> 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 | 19 ++++++++++++++++++-
>>>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.=
c
>>>>> index 1ec0b4a..cede445 100644
>>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>>>>
>>>>> static int bcm_setup(struct hci_uart *hu)
>>>>> {
>>>>> + char fw_name[64];
>>>>> + int err;
>>>>> +
>>>>> BT_DBG("hu %p", hu);
>>>>>
>>>>> hu->hdev->set_bdaddr =3D btbcm_set_bdaddr;
>>>>>
>>>>> - return btbcm_setup_patchram(hu->hdev);
>>>>> + err =3D btbcm_initialize(hu->hdev, fw_name, sizeof(fw_name));
>>>>> + if (err)
>>>>> + return err;
>>>>> +
>>>>> + err =3D btbcm_patchram(hu->hdev, fw_name);
>>>>> + /* If there is no firmware (-ENOENT), discard the error and
>>>>> continue */
>>>>
>>>> I guess -ENOENT means no firmware is required and not a
>>>> request_firmware() failure. Not sure what is meant here.
>>>>
>>>> Regards,
>>>> Arend
>>>>
>>>>> + if (err =3D=3D -ENOENT)
>>>>> + return 0;
>>>>> +
>>>>> + if (hu->proto->init_speed)
>>>>> + hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>> +
>>>>> + err =3D btbcm_finalize(hu->hdev);
>>>>> +
>>>>> + return err;
>>>>> }
>>>>>
>>>>> static const struct h4_recv_pkt bcm_recv_pkts[] =3D {
>>>
>>> You're right, btbcm_patchram() return test does not work as I expected.
>>>
>>> I can change this by performing uart speed change only if it returns no
>>> error.
>>>
>>> err =3D btbcm_patchram(hu->hdev, fw_name);
>>> if (!err) {
>>> /* Firmware loading may have reseted controller UART to init speed */
>>> if (hu->proto->init_speed)
>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>> }
>>>
>>> err =3D btbcm_finalize(hu->hdev);
>>>
>>>
>>> Or I can move request_firmware() out of btbcm_patchram() and test it
>>> before calling btbcm_patchram(). This will imply to change
>>> btbcm_patchram() to accept a firmware instead of firmware name.
>>>
>>> err =3D 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 =3D btbcm_patchram(hu->hdev, fw);
>>> if (!err) {
>>> /* Firmware loading may have reseted controller UART to init speed */
>>> if (hu->proto->init_speed)
>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>> }
>>>
>>> err =3D btbcm_finalize(hu->hdev);
>>>
>>> Arend, Marcel, any advice regarding this ?
>>
>> Well, functionally it does not make a difference as the request_firmware
>> is the first call done in btbcm_patchram(). So if it solves your problem
>> I would go for option 2. Other question: is there a reason why the error
>> code from request_firmware is not returned?
>
> If an error is returned it will stop Bluetooth setup.
> So, when request_firmware() returns an error, the Bluetooth controller
> will be used with current firmware and 0 is returned as bcm_setup is
> completed.
>
> IF: There is generally no "current firmware" if the firmware download fai=
ls or not attempted. The device would run out of the ROM then which is not =
terribly useful if Bluetooth functionality is needed. Thanks, -Ilya

The BCM4324B3 embedded in T100 is able to operate without firmware=20
loading, at least performing Bluetooth discovery.
I think it is better to run without firmware loading than not allowing=20
to use Bluetooth controller at all.

IF: Broadcom software elsewhere generally fails device start in this case a=
s every Bluetooth feature may or may not work afterwards, with no guarantee=
s whatsoever. The decision here is obviously up to BlueZ maintainers.

Thanks,
 -Ilya

regards

Fred

Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Marcel Holtmann]

Hi Ilya,

>>>>>> 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 | 19 ++++++++++++++++++-
>>>>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>>>> index 1ec0b4a..cede445 100644
>>>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>>>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>>>>> 
>>>>>> static int bcm_setup(struct hci_uart *hu)
>>>>>> {
>>>>>> + char fw_name[64];
>>>>>> + 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 = btbcm_patchram(hu->hdev, fw_name);
>>>>>> + /* If there is no firmware (-ENOENT), discard the error and
>>>>>> continue */
>>>>> 
>>>>> I guess -ENOENT means no firmware is required and not a
>>>>> request_firmware() failure. Not sure what is meant here.
>>>>> 
>>>>> Regards,
>>>>> Arend
>>>>> 
>>>>>> + if (err == -ENOENT)
>>>>>> + return 0;
>>>>>> +
>>>>>> + if (hu->proto->init_speed)
>>>>>> + hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>> +
>>>>>> + err = btbcm_finalize(hu->hdev);
>>>>>> +
>>>>>> + return err;
>>>>>> }
>>>>>> 
>>>>>> static const struct h4_recv_pkt bcm_recv_pkts[] = {
>>>> 
>>>> You're right, btbcm_patchram() return test does not work as I expected.
>>>> 
>>>> I can change this by performing uart speed change only if it returns no
>>>> error.
>>>> 
>>>> err = btbcm_patchram(hu->hdev, fw_name);
>>>> if (!err) {
>>>> /* Firmware loading may have reseted controller UART to init speed */
>>>> if (hu->proto->init_speed)
>>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>> }
>>>> 
>>>> err = btbcm_finalize(hu->hdev);
>>>> 
>>>> 
>>>> Or I can move request_firmware() out of btbcm_patchram() and test it
>>>> before calling btbcm_patchram(). This will imply to change
>>>> btbcm_patchram() to accept a firmware instead of firmware name.
>>>> 
>>>> 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) {
>>>> /* Firmware loading may have reseted controller UART to init speed */
>>>> if (hu->proto->init_speed)
>>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>> }
>>>> 
>>>> err = btbcm_finalize(hu->hdev);
>>>> 
>>>> Arend, Marcel, any advice regarding this ?
>>> 
>>> Well, functionally it does not make a difference as the request_firmware
>>> is the first call done in btbcm_patchram(). So if it solves your problem
>>> I would go for option 2. Other question: is there a reason why the error
>>> code from request_firmware is not returned?
>> 
>> If an error is returned it will stop Bluetooth setup.
>> So, when request_firmware() returns an error, the Bluetooth controller
>> will be used with current firmware and 0 is returned as bcm_setup is
>> completed.
>> 
>> IF: There is generally no "current firmware" if the firmware download fails or not attempted. The device would run out of the ROM then which is not terribly useful if Bluetooth functionality is needed. Thanks, -Ilya
> 
> The BCM4324B3 embedded in T100 is able to operate without firmware 
> loading, at least performing Bluetooth discovery.
> I think it is better to run without firmware loading than not allowing 
> to use Bluetooth controller at all.
> 
> IF: Broadcom software elsewhere generally fails device start in this case as every Bluetooth feature may or may not work afterwards, with no guarantees whatsoever. The decision here is obviously up to BlueZ maintainers.

I am fine with actually doing exactly that once we have a bit better infrastructure in place. However this also requires a commitment from Broadcom to actually publish the firmware files in linux-firmware tree and maintain the manifest document I have been talking about.

Regards

Marcel

Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Arend van Spriel]

On 06/06/15 08:18, Marcel Holtmann wrote:
> Hi Ilya,
>
>>>>>>> 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 | 19 ++++++++++++++++++-
>>>>>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>>>>> index 1ec0b4a..cede445 100644
>>>>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>>>>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>>>>>>
>>>>>>> static int bcm_setup(struct hci_uart *hu)
>>>>>>> {
>>>>>>> + char fw_name[64];
>>>>>>> + 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 = btbcm_patchram(hu->hdev, fw_name);
>>>>>>> + /* If there is no firmware (-ENOENT), discard the error and
>>>>>>> continue */
>>>>>>
>>>>>> I guess -ENOENT means no firmware is required and not a
>>>>>> request_firmware() failure. Not sure what is meant here.
>>>>>>
>>>>>> Regards,
>>>>>> Arend
>>>>>>
>>>>>>> + if (err == -ENOENT)
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> + if (hu->proto->init_speed)
>>>>>>> + hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>>> +
>>>>>>> + err = btbcm_finalize(hu->hdev);
>>>>>>> +
>>>>>>> + return err;
>>>>>>> }
>>>>>>>
>>>>>>> static const struct h4_recv_pkt bcm_recv_pkts[] = {
>>>>>
>>>>> You're right, btbcm_patchram() return test does not work as I expected.
>>>>>
>>>>> I can change this by performing uart speed change only if it returns no
>>>>> error.
>>>>>
>>>>> err = btbcm_patchram(hu->hdev, fw_name);
>>>>> if (!err) {
>>>>> /* Firmware loading may have reseted controller UART to init speed */
>>>>> if (hu->proto->init_speed)
>>>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>> }
>>>>>
>>>>> err = btbcm_finalize(hu->hdev);
>>>>>
>>>>>
>>>>> Or I can move request_firmware() out of btbcm_patchram() and test it
>>>>> before calling btbcm_patchram(). This will imply to change
>>>>> btbcm_patchram() to accept a firmware instead of firmware name.
>>>>>
>>>>> 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) {
>>>>> /* Firmware loading may have reseted controller UART to init speed */
>>>>> if (hu->proto->init_speed)
>>>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>> }
>>>>>
>>>>> err = btbcm_finalize(hu->hdev);
>>>>>
>>>>> Arend, Marcel, any advice regarding this ?
>>>>
>>>> Well, functionally it does not make a difference as the request_firmware
>>>> is the first call done in btbcm_patchram(). So if it solves your problem
>>>> I would go for option 2. Other question: is there a reason why the error
>>>> code from request_firmware is not returned?
>>>
>>> If an error is returned it will stop Bluetooth setup.
>>> So, when request_firmware() returns an error, the Bluetooth controller
>>> will be used with current firmware and 0 is returned as bcm_setup is
>>> completed.
>>>
>>> IF: There is generally no "current firmware" if the firmware download fails or not attempted. The device would run out of the ROM then which is not terribly useful if Bluetooth functionality is needed. Thanks, -Ilya
>>
>> The BCM4324B3 embedded in T100 is able to operate without firmware
>> loading, at least performing Bluetooth discovery.
>> I think it is better to run without firmware loading than not allowing
>> to use Bluetooth controller at all.
>>
>> IF: Broadcom software elsewhere generally fails device start in this case as every Bluetooth feature may or may not work afterwards, with no guarantees whatsoever. The decision here is obviously up to BlueZ maintainers.
>
> I am fine with actually doing exactly that once we have a bit better infrastructure in place. However this also requires a commitment from Broadcom to actually publish the firmware files in linux-firmware tree and maintain the manifest document I have been talking about.

Hi Marcel,

What is the added value of having the manifest file? I can see it keeps 
mapping hardware info to firmware file out of the driver and as such 
keeps the driver from growing in size for new device support. Just 
wondering how big that issue is. It also poses other issues as it adds 
another interaction with the firmware api, which in my opinion is still 
a nasty api from driver perspective.

Regards,
Arend

Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Marcel Holtmann]

Hi Arend,

>>>>>>>> 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 | 19 ++++++++++++++++++-
>>>>>>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>>>>>> 
>>>>>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>>>>>> index 1ec0b4a..cede445 100644
>>>>>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>>>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>>>>>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>>>>>>> 
>>>>>>>> static int bcm_setup(struct hci_uart *hu)
>>>>>>>> {
>>>>>>>> + char fw_name[64];
>>>>>>>> + 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 = btbcm_patchram(hu->hdev, fw_name);
>>>>>>>> + /* If there is no firmware (-ENOENT), discard the error and
>>>>>>>> continue */
>>>>>>> 
>>>>>>> I guess -ENOENT means no firmware is required and not a
>>>>>>> request_firmware() failure. Not sure what is meant here.
>>>>>>> 
>>>>>>> Regards,
>>>>>>> Arend
>>>>>>> 
>>>>>>>> + if (err == -ENOENT)
>>>>>>>> + return 0;
>>>>>>>> +
>>>>>>>> + if (hu->proto->init_speed)
>>>>>>>> + hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>>>> +
>>>>>>>> + err = btbcm_finalize(hu->hdev);
>>>>>>>> +
>>>>>>>> + return err;
>>>>>>>> }
>>>>>>>> 
>>>>>>>> static const struct h4_recv_pkt bcm_recv_pkts[] = {
>>>>>> 
>>>>>> You're right, btbcm_patchram() return test does not work as I expected.
>>>>>> 
>>>>>> I can change this by performing uart speed change only if it returns no
>>>>>> error.
>>>>>> 
>>>>>> err = btbcm_patchram(hu->hdev, fw_name);
>>>>>> if (!err) {
>>>>>> /* Firmware loading may have reseted controller UART to init speed */
>>>>>> if (hu->proto->init_speed)
>>>>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>> }
>>>>>> 
>>>>>> err = btbcm_finalize(hu->hdev);
>>>>>> 
>>>>>> 
>>>>>> Or I can move request_firmware() out of btbcm_patchram() and test it
>>>>>> before calling btbcm_patchram(). This will imply to change
>>>>>> btbcm_patchram() to accept a firmware instead of firmware name.
>>>>>> 
>>>>>> 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) {
>>>>>> /* Firmware loading may have reseted controller UART to init speed */
>>>>>> if (hu->proto->init_speed)
>>>>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>> }
>>>>>> 
>>>>>> err = btbcm_finalize(hu->hdev);
>>>>>> 
>>>>>> Arend, Marcel, any advice regarding this ?
>>>>> 
>>>>> Well, functionally it does not make a difference as the request_firmware
>>>>> is the first call done in btbcm_patchram(). So if it solves your problem
>>>>> I would go for option 2. Other question: is there a reason why the error
>>>>> code from request_firmware is not returned?
>>>> 
>>>> If an error is returned it will stop Bluetooth setup.
>>>> So, when request_firmware() returns an error, the Bluetooth controller
>>>> will be used with current firmware and 0 is returned as bcm_setup is
>>>> completed.
>>>> 
>>>> IF: There is generally no "current firmware" if the firmware download fails or not attempted. The device would run out of the ROM then which is not terribly useful if Bluetooth functionality is needed. Thanks, -Ilya
>>> 
>>> The BCM4324B3 embedded in T100 is able to operate without firmware
>>> loading, at least performing Bluetooth discovery.
>>> I think it is better to run without firmware loading than not allowing
>>> to use Bluetooth controller at all.
>>> 
>>> IF: Broadcom software elsewhere generally fails device start in this case as every Bluetooth feature may or may not work afterwards, with no guarantees whatsoever. The decision here is obviously up to BlueZ maintainers.
>> 
>> I am fine with actually doing exactly that once we have a bit better infrastructure in place. However this also requires a commitment from Broadcom to actually publish the firmware files in linux-firmware tree and maintain the manifest document I have been talking about.
> 
> What is the added value of having the manifest file? I can see it keeps mapping hardware info to firmware file out of the driver and as such keeps the driver from growing in size for new device support. Just wondering how big that issue is. It also poses other issues as it adds another interaction with the firmware api, which in my opinion is still a nasty api from driver perspective.

so a recent version of the Broadcom Bluetooth driver for Windows contains 167+ firmware files. Which means there are plenty of different files. I count at least 100+ individual USB dongles listed. Inside the btusb.c driver we started to match these devices with interface specific information.

        /* Broadcom devices with vendor specific id */
        { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
          .driver_info = BTUSB_BCM_PATCHRAM },

Otherwise we would just go insane to attempt to update the driver for every new dongle manufacture that comes along. This way this is limited to a handful big ones.

The request_firmware() API allows to builtin the file into the kernel image. While I would not do that for the actual firmware file itself, but for the manifest it might make sense. However keeping it outside and allowing update via linux-firmware package update seems beneficial for everybody.

Regards

Marcel

Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Arend van Spriel]

On 06/06/15 09:24, Marcel Holtmann wrote:
> Hi Arend,
>
>>>>>>>>> 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 | 19 ++++++++++++++++++-
>>>>>>>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>>>>>>> index 1ec0b4a..cede445 100644
>>>>>>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>>>>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>>>>>>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>>>>>>>>
>>>>>>>>> static int bcm_setup(struct hci_uart *hu)
>>>>>>>>> {
>>>>>>>>> + char fw_name[64];
>>>>>>>>> + 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 = btbcm_patchram(hu->hdev, fw_name);
>>>>>>>>> + /* If there is no firmware (-ENOENT), discard the error and
>>>>>>>>> continue */
>>>>>>>>
>>>>>>>> I guess -ENOENT means no firmware is required and not a
>>>>>>>> request_firmware() failure. Not sure what is meant here.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Arend
>>>>>>>>
>>>>>>>>> + if (err == -ENOENT)
>>>>>>>>> + return 0;
>>>>>>>>> +
>>>>>>>>> + if (hu->proto->init_speed)
>>>>>>>>> + hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>>>>> +
>>>>>>>>> + err = btbcm_finalize(hu->hdev);
>>>>>>>>> +
>>>>>>>>> + return err;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> static const struct h4_recv_pkt bcm_recv_pkts[] = {
>>>>>>>
>>>>>>> You're right, btbcm_patchram() return test does not work as I expected.
>>>>>>>
>>>>>>> I can change this by performing uart speed change only if it returns no
>>>>>>> error.
>>>>>>>
>>>>>>> err = btbcm_patchram(hu->hdev, fw_name);
>>>>>>> if (!err) {
>>>>>>> /* Firmware loading may have reseted controller UART to init speed */
>>>>>>> if (hu->proto->init_speed)
>>>>>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>>> }
>>>>>>>
>>>>>>> err = btbcm_finalize(hu->hdev);
>>>>>>>
>>>>>>>
>>>>>>> Or I can move request_firmware() out of btbcm_patchram() and test it
>>>>>>> before calling btbcm_patchram(). This will imply to change
>>>>>>> btbcm_patchram() to accept a firmware instead of firmware name.
>>>>>>>
>>>>>>> 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) {
>>>>>>> /* Firmware loading may have reseted controller UART to init speed */
>>>>>>> if (hu->proto->init_speed)
>>>>>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>>> }
>>>>>>>
>>>>>>> err = btbcm_finalize(hu->hdev);
>>>>>>>
>>>>>>> Arend, Marcel, any advice regarding this ?
>>>>>>
>>>>>> Well, functionally it does not make a difference as the request_firmware
>>>>>> is the first call done in btbcm_patchram(). So if it solves your problem
>>>>>> I would go for option 2. Other question: is there a reason why the error
>>>>>> code from request_firmware is not returned?
>>>>>
>>>>> If an error is returned it will stop Bluetooth setup.
>>>>> So, when request_firmware() returns an error, the Bluetooth controller
>>>>> will be used with current firmware and 0 is returned as bcm_setup is
>>>>> completed.
>>>>>
>>>>> IF: There is generally no "current firmware" if the firmware download fails or not attempted. The device would run out of the ROM then which is not terribly useful if Bluetooth functionality is needed. Thanks, -Ilya
>>>>
>>>> The BCM4324B3 embedded in T100 is able to operate without firmware
>>>> loading, at least performing Bluetooth discovery.
>>>> I think it is better to run without firmware loading than not allowing
>>>> to use Bluetooth controller at all.
>>>>
>>>> IF: Broadcom software elsewhere generally fails device start in this case as every Bluetooth feature may or may not work afterwards, with no guarantees whatsoever. The decision here is obviously up to BlueZ maintainers.
>>>
>>> I am fine with actually doing exactly that once we have a bit better infrastructure in place. However this also requires a commitment from Broadcom to actually publish the firmware files in linux-firmware tree and maintain the manifest document I have been talking about.
>>
>> What is the added value of having the manifest file? I can see it keeps mapping hardware info to firmware file out of the driver and as such keeps the driver from growing in size for new device support. Just wondering how big that issue is. It also poses other issues as it adds another interaction with the firmware api, which in my opinion is still a nasty api from driver perspective.
>
> so a recent version of the Broadcom Bluetooth driver for Windows contains 167+ firmware files. Which means there are plenty of different files. I count at least 100+ individual USB dongles listed. Inside the btusb.c driver we started to match these devices with interface specific information.
>
>          /* Broadcom devices with vendor specific id */
>          { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
>            .driver_info = BTUSB_BCM_PATCHRAM },
>
> Otherwise we would just go insane to attempt to update the driver for every new dongle manufacture that comes along. This way this is limited to a handful big ones.
>
> The request_firmware() API allows to builtin the file into the kernel image. While I would not do that for the actual firmware file itself, but for the manifest it might make sense. However keeping it outside and allowing update via linux-firmware package update seems beneficial for everybody.

Hi Marcel,

I did not realize there were so many firmware variations in bluetooth. 
That's justification enough and having the manifest file built-in sounds 
like a viable plan. At least I like the idea and might explore it for 
our wifi drivers (sorry for being a bit single-minded ;-) ).

Thanks,
Arend

Re: [PATCH v6 4/5] Bluetooth: hci_uart: Update Broadcom UART setup

[Marcel Holtmann]

Hi Arend,

>>>>>>>>>> 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 | 19 ++++++++++++++++++-
>>>>>>>>>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>>>>>>>>> 
>>>>>>>>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>>>>>>>>> index 1ec0b4a..cede445 100644
>>>>>>>>>> --- a/drivers/bluetooth/hci_bcm.c
>>>>>>>>>> +++ b/drivers/bluetooth/hci_bcm.c
>>>>>>>>>> @@ -79,11 +79,28 @@ static int bcm_flush(struct hci_uart *hu)
>>>>>>>>>> 
>>>>>>>>>> static int bcm_setup(struct hci_uart *hu)
>>>>>>>>>> {
>>>>>>>>>> + char fw_name[64];
>>>>>>>>>> + 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 = btbcm_patchram(hu->hdev, fw_name);
>>>>>>>>>> + /* If there is no firmware (-ENOENT), discard the error and
>>>>>>>>>> continue */
>>>>>>>>> 
>>>>>>>>> I guess -ENOENT means no firmware is required and not a
>>>>>>>>> request_firmware() failure. Not sure what is meant here.
>>>>>>>>> 
>>>>>>>>> Regards,
>>>>>>>>> Arend
>>>>>>>>> 
>>>>>>>>>> + if (err == -ENOENT)
>>>>>>>>>> + return 0;
>>>>>>>>>> +
>>>>>>>>>> + if (hu->proto->init_speed)
>>>>>>>>>> + hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>>>>>> +
>>>>>>>>>> + err = btbcm_finalize(hu->hdev);
>>>>>>>>>> +
>>>>>>>>>> + return err;
>>>>>>>>>> }
>>>>>>>>>> 
>>>>>>>>>> static const struct h4_recv_pkt bcm_recv_pkts[] = {
>>>>>>>> 
>>>>>>>> You're right, btbcm_patchram() return test does not work as I expected.
>>>>>>>> 
>>>>>>>> I can change this by performing uart speed change only if it returns no
>>>>>>>> error.
>>>>>>>> 
>>>>>>>> err = btbcm_patchram(hu->hdev, fw_name);
>>>>>>>> if (!err) {
>>>>>>>> /* Firmware loading may have reseted controller UART to init speed */
>>>>>>>> if (hu->proto->init_speed)
>>>>>>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>>>> }
>>>>>>>> 
>>>>>>>> err = btbcm_finalize(hu->hdev);
>>>>>>>> 
>>>>>>>> 
>>>>>>>> Or I can move request_firmware() out of btbcm_patchram() and test it
>>>>>>>> before calling btbcm_patchram(). This will imply to change
>>>>>>>> btbcm_patchram() to accept a firmware instead of firmware name.
>>>>>>>> 
>>>>>>>> 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) {
>>>>>>>> /* Firmware loading may have reseted controller UART to init speed */
>>>>>>>> if (hu->proto->init_speed)
>>>>>>>> hci_uart_set_baudrate(hu, hu->proto->init_speed);
>>>>>>>> }
>>>>>>>> 
>>>>>>>> err = btbcm_finalize(hu->hdev);
>>>>>>>> 
>>>>>>>> Arend, Marcel, any advice regarding this ?
>>>>>>> 
>>>>>>> Well, functionally it does not make a difference as the request_firmware
>>>>>>> is the first call done in btbcm_patchram(). So if it solves your problem
>>>>>>> I would go for option 2. Other question: is there a reason why the error
>>>>>>> code from request_firmware is not returned?
>>>>>> 
>>>>>> If an error is returned it will stop Bluetooth setup.
>>>>>> So, when request_firmware() returns an error, the Bluetooth controller
>>>>>> will be used with current firmware and 0 is returned as bcm_setup is
>>>>>> completed.
>>>>>> 
>>>>>> IF: There is generally no "current firmware" if the firmware download fails or not attempted. The device would run out of the ROM then which is not terribly useful if Bluetooth functionality is needed. Thanks, -Ilya
>>>>> 
>>>>> The BCM4324B3 embedded in T100 is able to operate without firmware
>>>>> loading, at least performing Bluetooth discovery.
>>>>> I think it is better to run without firmware loading than not allowing
>>>>> to use Bluetooth controller at all.
>>>>> 
>>>>> IF: Broadcom software elsewhere generally fails device start in this case as every Bluetooth feature may or may not work afterwards, with no guarantees whatsoever. The decision here is obviously up to BlueZ maintainers.
>>>> 
>>>> I am fine with actually doing exactly that once we have a bit better infrastructure in place. However this also requires a commitment from Broadcom to actually publish the firmware files in linux-firmware tree and maintain the manifest document I have been talking about.
>>> 
>>> What is the added value of having the manifest file? I can see it keeps mapping hardware info to firmware file out of the driver and as such keeps the driver from growing in size for new device support. Just wondering how big that issue is. It also poses other issues as it adds another interaction with the firmware api, which in my opinion is still a nasty api from driver perspective.
>> 
>> so a recent version of the Broadcom Bluetooth driver for Windows contains 167+ firmware files. Which means there are plenty of different files. I count at least 100+ individual USB dongles listed. Inside the btusb.c driver we started to match these devices with interface specific information.
>> 
>>         /* Broadcom devices with vendor specific id */
>>         { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01),
>>           .driver_info = BTUSB_BCM_PATCHRAM },
>> 
>> Otherwise we would just go insane to attempt to update the driver for every new dongle manufacture that comes along. This way this is limited to a handful big ones.
>> 
>> The request_firmware() API allows to builtin the file into the kernel image. While I would not do that for the actual firmware file itself, but for the manifest it might make sense. However keeping it outside and allowing update via linux-firmware package update seems beneficial for everybody.
> 
> I did not realize there were so many firmware variations in bluetooth. That's justification enough and having the manifest file built-in sounds like a viable plan. At least I like the idea and might explore it for our wifi drivers (sorry for being a bit single-minded ;-) ).

actually keep in mind that in Bluetooth in most cases it is never a full firmware image. Only a few chips load a full firmware. A lot these chips are just patching their ROM. So for that obvious reason you have to pick the right firmware patch that matches your ROM.

Regards

Marcel