[PATCH 1/7] Bluetooth: btbcm: Add BCM4324B3 UART device

[PATCH 1/7] Bluetooth: btbcm: Add BCM4324B3 UART device

[Frederic Danis]

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

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index d0741f3..47e0563 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)
 {
@@ -69,6 +70,10 @@ int btbcm_check_bdaddr(struct hci_dev *hdev)
 		BT_INFO("%s: BCM: Using default device address (%pMR)",
 			hdev->name, &bda->bdaddr);
 		set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
+	} else if (!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);
 	}
 
 	kfree_skb(skb);
@@ -176,6 +181,7 @@ static const struct {
 	const char *name;
 } bcm_uart_subver_table[] = {
 	{ 0x410e, "BCM43341B0"	},	/* 002.001.014 */
+	{ 0x4406, "BCM4324B3"	},	/* 002.004.006 */
 	{ }
 };
 
@@ -234,6 +240,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 2/7] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Frederic Danis]

This allows user space application to set final speed requested for UART
device. UART port is open at init speed by user space application.

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

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 5c9a73f..190a7f8 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -609,6 +609,12 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
 	case HCIUARTGETFLAGS:
 		return hu->hdev_flags;
 
+	case HCIUARTSETBAUDRATE:
+		if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
+			return -EBUSY;
+		hu->speed = arg;
+		break;
+
 	default:
 		err = n_tty_ioctl_helper(tty, file, cmd, arg);
 		break;
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 72120a5..09a47b4 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -33,6 +33,7 @@
 #define HCIUARTGETDEVICE	_IOR('U', 202, int)
 #define HCIUARTSETFLAGS		_IOW('U', 203, int)
 #define HCIUARTGETFLAGS		_IOR('U', 204, int)
+#define HCIUARTSETBAUDRATE	_IOW('U', 205, int)
 
 /* UART protocols */
 #define HCI_UART_MAX_PROTO	8
@@ -72,6 +73,7 @@ struct hci_uart {
 	struct hci_dev		*hdev;
 	unsigned long		flags;
 	unsigned long		hdev_flags;
+	unsigned long		speed;
 
 	struct work_struct	init_ready;
 	struct work_struct	write_work;
-- 
1.9.1

[PATCH 3/7] Bluetooth: hci_uart: Support final speed during setup

[Frederic Danis]

Change to full speed as soon as possible.
Vendor specific setup is splited into setup() and post_setup() functions
because setup() may have reseted Bluetooth controller speed to default
speed during firmware loading.
This implies to set UART speed back to default speed then set speed to
full speed again before calling post_setup().

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

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 190a7f8..6e33a14 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -265,14 +265,58 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }
 
+static void hci_uart_set_baudrate(struct hci_uart *hu, speed_t 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(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;
+	speed_t init_speed = hu->tty->termios.c_ispeed;
+	int err;
+
+	if (hu->proto->set_baudrate && hu->speed) {
+		err = hu->proto->set_baudrate(hu, hu->speed);
+		if (err)
+			return err;
+		hci_uart_set_baudrate(hu, hu->speed);
+	}
+
+	if (hu->proto->setup) {
+		err = hu->proto->setup(hu);
+		/* If there is no firmware (-ENOENT), discard the error and
+		 * continue */
+		if (err == -ENOENT)
+			return 0;
+
+		if (hu->proto->set_baudrate && hu->speed) {
+			/* Controller speed has been reset to default speed */
+			hci_uart_set_baudrate(hu, init_speed);
+
+			err = hu->proto->set_baudrate(hu, hu->speed);
+			if (err)
+				return err;
+			hci_uart_set_baudrate(hu, hu->speed);
+		}
+
+		if (hu->proto->post_setup)
+			err = hu->proto->post_setup(hu);
 
-	if (hu->proto->setup)
-		return hu->proto->setup(hu);
+		return err;
+	}
 
 	if (!test_bit(HCI_UART_VND_DETECT, &hu->hdev_flags))
 		return 0;
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 09a47b4..18050e0 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -63,6 +63,8 @@ struct hci_uart_proto {
 	int (*close)(struct hci_uart *hu);
 	int (*flush)(struct hci_uart *hu);
 	int (*setup)(struct hci_uart *hu);
+	int (*post_setup)(struct hci_uart *hu);
+	int (*set_baudrate)(struct hci_uart *hu, 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);
-- 
1.9.1

[PATCH 4/7] Bluetooth: btbcm: Split setup() function

[Frederic Danis]

BCM firmware loading will reset controller speed to default one.
So we need to split it in 2 functions done before and after this speed
change.

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

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 47e0563..f7802c4 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -283,7 +283,7 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
 	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;
+		return -ENOENT;
 	}
 
 	/* Start Download */
@@ -292,7 +292,7 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
 		err = PTR_ERR(skb);
 		BT_ERR("%s: BCM: Download Minidrv command failed (%d)",
 		       hdev->name, err);
-		goto reset;
+		goto done;
 	}
 	kfree_skb(skb);
 
@@ -313,7 +313,7 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
 			BT_ERR("%s: BCM: patch %s is corrupted", hdev->name,
 			       fw_name);
 			err = -EINVAL;
-			goto reset;
+			goto done;
 		}
 
 		cmd_param = fw_ptr;
@@ -328,7 +328,7 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
 			err = PTR_ERR(skb);
 			BT_ERR("%s: BCM: patch command %04x failed (%d)",
 			       hdev->name, opcode, err);
-			goto reset;
+			goto done;
 		}
 		kfree_skb(skb);
 	}
@@ -336,7 +336,20 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
 	/* 250 msec delay after Launch Ram completes */
 	msleep(250);
 
-reset:
+done:
+	release_firmware(fw);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(btbcm_setup_patchram);
+
+int btbcm_setup_post(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)
@@ -354,20 +367,18 @@ reset:
 	subver = le16_to_cpu(ver->lmp_subver);
 	kfree_skb(skb);
 
-	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);
+	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);
 
 done:
-	release_firmware(fw);
-
 	return err;
 }
-EXPORT_SYMBOL_GPL(btbcm_setup_patchram);
+EXPORT_SYMBOL_GPL(btbcm_setup_post);
 
 int btbcm_setup_apple(struct hci_dev *hdev)
 {
diff --git a/drivers/bluetooth/btbcm.h b/drivers/bluetooth/btbcm.h
index 34268ae..245fa30 100644
--- a/drivers/bluetooth/btbcm.h
+++ b/drivers/bluetooth/btbcm.h
@@ -27,6 +27,7 @@ int btbcm_check_bdaddr(struct hci_dev *hdev);
 int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
 
 int btbcm_setup_patchram(struct hci_dev *hdev);
+int btbcm_setup_post(struct hci_dev *hdev);
 int btbcm_setup_apple(struct hci_dev *hdev);
 
 #else
@@ -46,6 +47,11 @@ static inline int btbcm_setup_patchram(struct hci_dev *hdev)
 	return 0;
 }
 
+static inline int btbcm_setup_post(struct hci_dev *hdev)
+{
+	return 0;
+}
+
 static inline int btbcm_setup_apple(struct hci_dev *hdev)
 {
 	return 0;
-- 
1.9.1

[PATCH 5/7] Bluetooth: btusb: Use btbcm_setup_post()

[Frederic Danis]

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

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index de7b236..25d490d 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1309,6 +1309,22 @@ static int btusb_setup_bcm92035(struct hci_dev *hdev)
 	return 0;
 }
 
+#ifdef CONFIG_BT_HCIBTUSB_BCM
+static int btusb_setup_bcm(struct hci_dev *hdev)
+{
+	int err;
+
+	err = btbcm_setup_patchram(hdev);
+	/* If there is no firmware (-ENOENT), discard the error and continue */
+	if (err == -ENOENT)
+		return 0;
+
+	err = btbcm_setup_post(hdev);
+
+	return err;
+}
+#endif
+
 static int btusb_setup_csr(struct hci_dev *hdev)
 {
 	struct hci_rp_read_local_version *rp;
@@ -2730,7 +2746,7 @@ static int btusb_probe(struct usb_interface *intf,
 
 #ifdef CONFIG_BT_HCIBTUSB_BCM
 	if (id->driver_info & BTUSB_BCM_PATCHRAM) {
-		hdev->setup = btbcm_setup_patchram;
+		hdev->setup = btusb_setup_bcm;
 		hdev->set_bdaddr = btbcm_set_bdaddr;
 	}
 
-- 
1.9.1

[PATCH 6/7] Bluetooth: hci_uart: Use btbcm_setup_post()

[Frederic Danis]

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

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 1ec0b4a..25c9883 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -86,6 +86,13 @@ static int bcm_setup(struct hci_uart *hu)
 	return btbcm_setup_patchram(hu->hdev);
 }
 
+static int bcm_post_setup(struct hci_uart *hu)
+{
+	BT_DBG("hu %p", hu);
+
+	return btbcm_setup_post(hu->hdev);
+}
+
 static const struct h4_recv_pkt bcm_recv_pkts[] = {
 	{ H4_RECV_ACL,   .recv = hci_recv_frame },
 	{ H4_RECV_SCO,   .recv = hci_recv_frame },
@@ -137,6 +144,7 @@ static const struct hci_uart_proto bcm_proto = {
 	.close		= bcm_close,
 	.flush		= bcm_flush,
 	.setup		= bcm_setup,
+	.post_setup	= bcm_post_setup,
 	.recv		= bcm_recv,
 	.enqueue	= bcm_enqueue,
 	.dequeue	= bcm_dequeue,
-- 
1.9.1

[PATCH 7/7] Bluetooth: btbcm: 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 | 71 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 25c9883..7308cf4 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,80 @@
 #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;
 };
 
+static int bcm_set_clock(struct hci_uart *hu, unsigned char clock)
+{
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+
+	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);
+	}
+
+	if (skb->data[0]) {
+		u8 evt_status = skb->data[0];
+
+		BT_ERR("%s: write update clock event failed (%02x)",
+		       hdev->name, evt_status);
+		kfree_skb(skb);
+		return -bt_to_errno(evt_status);
+	}
+
+	kfree_skb(skb);
+
+	return 0;
+}
+
+struct hci_cp_bcm_set_speed {
+	__le16   dummy;
+	__le32   speed;
+} __packed;
+
+static int bcm_set_baudrate(struct hci_uart *hu, 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 && bcm_set_clock(hu, BCM43XX_CLOCK_48))
+		return -EINVAL;
+
+	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);
+	}
+
+	if (skb->data[0]) {
+		u8 evt_status = skb->data[0];
+
+		BT_ERR("%s: write update baudrate event failed (%02x)",
+		       hdev->name, evt_status);
+		kfree_skb(skb);
+		return -bt_to_errno(evt_status);
+	}
+
+	kfree_skb(skb);
+
+	return 0;
+}
+
 static int bcm_open(struct hci_uart *hu)
 {
 	struct bcm_data *bcm;
@@ -145,6 +215,7 @@ static const struct hci_uart_proto bcm_proto = {
 	.flush		= bcm_flush,
 	.setup		= bcm_setup,
 	.post_setup	= bcm_post_setup,
+	.set_baudrate	= bcm_set_baudrate,
 	.recv		= bcm_recv,
 	.enqueue	= bcm_enqueue,
 	.dequeue	= bcm_dequeue,
-- 
1.9.1

Re: [PATCH 2/7] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Peter Hurley]

On 04/10/2015 09:37 AM, Frederic Danis wrote:
> This allows user space application to set final speed requested for UART
> device. UART port is open at init speed by user space application.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
>  drivers/bluetooth/hci_ldisc.c | 6 ++++++
>  drivers/bluetooth/hci_uart.h  | 2 ++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 5c9a73f..190a7f8 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -609,6 +609,12 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
>  	case HCIUARTGETFLAGS:
>  		return hu->hdev_flags;
>  
> +	case HCIUARTSETBAUDRATE:
> +		if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
> +			return -EBUSY;
> +		hu->speed = arg;
> +		break;
> +

So now that the kernel can set line rate, why is an ioctl necessary
to determine what the line rate should be?


>  	default:
>  		err = n_tty_ioctl_helper(tty, file, cmd, arg);
>  		break;
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index 72120a5..09a47b4 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -33,6 +33,7 @@
>  #define HCIUARTGETDEVICE	_IOR('U', 202, int)
>  #define HCIUARTSETFLAGS		_IOW('U', 203, int)
>  #define HCIUARTGETFLAGS		_IOR('U', 204, int)
> +#define HCIUARTSETBAUDRATE	_IOW('U', 205, int)
>  
>  /* UART protocols */
>  #define HCI_UART_MAX_PROTO	8
> @@ -72,6 +73,7 @@ struct hci_uart {
>  	struct hci_dev		*hdev;
>  	unsigned long		flags;
>  	unsigned long		hdev_flags;
> +	unsigned long		speed;
>  
>  	struct work_struct	init_ready;
>  	struct work_struct	write_work;
> 

Re: [PATCH 2/7] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Marcel Holtmann]

Hi Peter,

>> This allows user space application to set final speed requested for UART
>> device. UART port is open at init speed by user space application.
>> 
>> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
>> ---
>> drivers/bluetooth/hci_ldisc.c | 6 ++++++
>> drivers/bluetooth/hci_uart.h  | 2 ++
>> 2 files changed, 8 insertions(+)
>> 
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index 5c9a73f..190a7f8 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -609,6 +609,12 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
>> 	case HCIUARTGETFLAGS:
>> 		return hu->hdev_flags;
>> 
>> +	case HCIUARTSETBAUDRATE:
>> +		if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
>> +			return -EBUSY;
>> +		hu->speed = arg;
>> +		break;
>> +
> 
> So now that the kernel can set line rate, why is an ioctl necessary
> to determine what the line rate should be?

actually I think we should skip this for now and only introduce it if it is really needed. For the chips we want to support initially we only need the default baudrate and the max baudrate. So the default baudrate is vendor specific and we would just hardcode that in the driver. The max baudrate is platform specific and most likely should come via ACPI or DT. Or some default value in case it is platform specific driver in the first place.

Just as background, this command is not for actually setting the baudrate at that point, it is informing the kernel about the max baudrate for operational use. It will be programmed way later. However as stated above, I think for now we should skip this and only introduce it if the kernel needs input from userspace on the max baudrate.

Fred, I think what we really want at this point is to add default and operational baudrate fields to hci_uart_proto.

Regards

Marcel

Re: [PATCH 2/7] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Peter Hurley]

On 04/10/2015 02:20 PM, Marcel Holtmann wrote:
> Hi Peter,
> 
>>> This allows user space application to set final speed requested for UART
>>> device. UART port is open at init speed by user space application.
>>>
>>> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
>>> ---
>>> drivers/bluetooth/hci_ldisc.c | 6 ++++++
>>> drivers/bluetooth/hci_uart.h  | 2 ++
>>> 2 files changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>>> index 5c9a73f..190a7f8 100644
>>> --- a/drivers/bluetooth/hci_ldisc.c
>>> +++ b/drivers/bluetooth/hci_ldisc.c
>>> @@ -609,6 +609,12 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
>>> 	case HCIUARTGETFLAGS:
>>> 		return hu->hdev_flags;
>>>
>>> +	case HCIUARTSETBAUDRATE:
>>> +		if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
>>> +			return -EBUSY;
>>> +		hu->speed = arg;
>>> +		break;
>>> +
>>
>> So now that the kernel can set line rate, why is an ioctl necessary
>> to determine what the line rate should be?
> 
> actually I think we should skip this for now and only introduce it if it is really needed. For the chips we want to support initially we only need the default baudrate and the max baudrate. So the default baudrate is vendor specific and we would just hardcode that in the driver. The max baudrate is platform specific and most likely should come via ACPI or DT. Or some default value in case it is platform specific driver in the first place.
> 
> Just as background, this command is not for actually setting the baudrate at that point, it is informing the kernel about the max baudrate for operational use. It will be programmed way later. However as stated above, I think for now we should skip this and only introduce it if the kernel needs input from userspace on the max baudrate.

Or through sysfs.

> Fred, I think what we really want at this point is to add default and operational baudrate fields to hci_uart_proto.
> 
> Regards
> 
> Marcel
> 

Re: [PATCH 3/7] Bluetooth: hci_uart: Support final speed during setup

[Marcel Holtmann]

Hi Fred,

> Change to full speed as soon as possible.
> Vendor specific setup is splited into setup() and post_setup() functions
> because setup() may have reseted Bluetooth controller speed to default
> speed during firmware loading.
> This implies to set UART speed back to default speed then set speed to
> full speed again before calling post_setup().
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 48 +++++++++++++++++++++++++++++++++++++++++--
> drivers/bluetooth/hci_uart.h  |  2 ++
> 2 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 190a7f8..6e33a14 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -265,14 +265,58 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> 	return 0;
> }
> 
> +static void hci_uart_set_baudrate(struct hci_uart *hu, speed_t 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(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;
> +	speed_t init_speed = hu->tty->termios.c_ispeed;
> +	int err;
> +
> +	if (hu->proto->set_baudrate && hu->speed) {
> +		err = hu->proto->set_baudrate(hu, hu->speed);
> +		if (err)
> +			return err;
> +		hci_uart_set_baudrate(hu, hu->speed);
> +	}

I would prefer if we split this into multiple patches. The first one should just set the operational baudrate. Since that is something that we need for all drivers that specify this baudrate.

You also need to set the default baudrate first. Then issue the HCI command and then set the operational one.

> +
> +	if (hu->proto->setup) {
> +		err = hu->proto->setup(hu);
> +		/* If there is no firmware (-ENOENT), discard the error and
> +		 * continue */
> +		if (err == -ENOENT)
> +			return 0;

This is actually a bug in the setup callback then. If there is no firmware and the hardware can operate without it, then it should just return 0.

And with that I think the vendor specific setup should trigger the baudrate change. The reason for is that we can not always assume that the setup will drop us back into default baud rate. Some controllers might stay in operational baudrate.

> +
> +		if (hu->proto->set_baudrate && hu->speed) {
> +			/* Controller speed has been reset to default speed */
> +			hci_uart_set_baudrate(hu, init_speed);
> +
> +			err = hu->proto->set_baudrate(hu, hu->speed);
> +			if (err)
> +				return err;
> +			hci_uart_set_baudrate(hu, hu->speed);
> +		}
> +
> +		if (hu->proto->post_setup)
> +			err = hu->proto->post_setup(hu);
> 
> -	if (hu->proto->setup)
> -		return hu->proto->setup(hu);
> +		return err;
> +	}
> 
> 	if (!test_bit(HCI_UART_VND_DETECT, &hu->hdev_flags))
> 		return 0;
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index 09a47b4..18050e0 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -63,6 +63,8 @@ struct hci_uart_proto {
> 	int (*close)(struct hci_uart *hu);
> 	int (*flush)(struct hci_uart *hu);
> 	int (*setup)(struct hci_uart *hu);
> +	int (*post_setup)(struct hci_uart *hu);
> +	int (*set_baudrate)(struct hci_uart *hu, int speed);

Lets keep the patch for baudrate support independent from any post setup support. Maybe introducint hci_uart_reset_baudrate that can be called from hu->setup will just work and we do not need a post_setup at all here.

> 	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);

Regards

Marcel

Re: [PATCH 4/7] Bluetooth: btbcm: Split setup() function

[Marcel Holtmann]

Hi Fred,

> BCM firmware loading will reset controller speed to default one.
> So we need to split it in 2 functions done before and after this speed
> change.
> 
> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
> ---
> drivers/bluetooth/btbcm.c | 33 ++++++++++++++++++++++-----------
> drivers/bluetooth/btbcm.h |  6 ++++++
> 2 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> index 47e0563..f7802c4 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -283,7 +283,7 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
> 	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;
> +		return -ENOENT;
> 	}

I really like to have it return 0 to indicate that all went fine. Using ENOENT error code seems odd to me. Especially since in some cases we will have required and not optional firmware download.

> 
> 	/* Start Download */
> @@ -292,7 +292,7 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
> 		err = PTR_ERR(skb);
> 		BT_ERR("%s: BCM: Download Minidrv command failed (%d)",
> 		       hdev->name, err);
> -		goto reset;
> +		goto done;
> 	}
> 	kfree_skb(skb);
> 
> @@ -313,7 +313,7 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
> 			BT_ERR("%s: BCM: patch %s is corrupted", hdev->name,
> 			       fw_name);
> 			err = -EINVAL;
> -			goto reset;
> +			goto done;
> 		}
> 
> 		cmd_param = fw_ptr;
> @@ -328,7 +328,7 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
> 			err = PTR_ERR(skb);
> 			BT_ERR("%s: BCM: patch command %04x failed (%d)",
> 			       hdev->name, opcode, err);
> -			goto reset;
> +			goto done;
> 		}
> 		kfree_skb(skb);
> 	}
> @@ -336,7 +336,20 @@ int btbcm_setup_patchram(struct hci_dev *hdev)
> 	/* 250 msec delay after Launch Ram completes */
> 	msleep(250);
> 
> -reset:
> +done:
> +	release_firmware(fw);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(btbcm_setup_patchram);
> +

I need to think about this a little bit, but I think we might want to actually call this btbcm_patchram since that is what this procedure is actually doing in the end.

We might actually even change the parameters of it to take hdev and the firmware name. In some cases we might get the firmware name from a different source and can not rely on our auto name detection.

So I would start with creating a btbcm_patchram that takes the firmware name and does just the write ram and launch ram handling. I can try to do this as a prepatch and test if this works out with the USB driver.

Coming to think about this, if we create a separate btbcm_patchram, then returning ENOENT is fine. Since the handling of the error would be Broadcom specific. We just can not do that inside the generic hdev->setup callback. It needs to be confined to the hu->setup.

> +int btbcm_setup_post(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)
> @@ -354,20 +367,18 @@ reset:
> 	subver = le16_to_cpu(ver->lmp_subver);
> 	kfree_skb(skb);
> 
> -	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);
> +	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);
> 
> done:
> -	release_firmware(fw);
> -
> 	return err;
> }
> -EXPORT_SYMBOL_GPL(btbcm_setup_patchram);
> +EXPORT_SYMBOL_GPL(btbcm_setup_post);

Since these will turn into just version number reading and printing it out, we might actually call it like that. And then it can be called from multiple drivers multiple times.

A little bit of code duplication in the setup routines from USB and UART drivers is fine. As long as we push the HCI command execution into the vendor specific module. That is where I really want it concentrated.

> 
> int btbcm_setup_apple(struct hci_dev *hdev)
> {
> diff --git a/drivers/bluetooth/btbcm.h b/drivers/bluetooth/btbcm.h
> index 34268ae..245fa30 100644
> --- a/drivers/bluetooth/btbcm.h
> +++ b/drivers/bluetooth/btbcm.h
> @@ -27,6 +27,7 @@ int btbcm_check_bdaddr(struct hci_dev *hdev);
> int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> 
> int btbcm_setup_patchram(struct hci_dev *hdev);
> +int btbcm_setup_post(struct hci_dev *hdev);
> int btbcm_setup_apple(struct hci_dev *hdev);
> 
> #else
> @@ -46,6 +47,11 @@ static inline int btbcm_setup_patchram(struct hci_dev *hdev)
> 	return 0;
> }
> 
> +static inline int btbcm_setup_post(struct hci_dev *hdev)
> +{
> +	return 0;
> +}
> +
> static inline int btbcm_setup_apple(struct hci_dev *hdev)
> {
> 	return 0;

Regards

Marcel

Re: [PATCH 7/7] Bluetooth: btbcm: 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 | 71 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 71 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 25c9883..7308cf4 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,80 @@
> #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;
> };
> 
> +static int bcm_set_clock(struct hci_uart *hu, unsigned char clock)
> +{
> +	struct hci_dev *hdev = hu->hdev;
> +	struct sk_buff *skb;
> +
> +	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);
> +	}
> +
> +	if (skb->data[0]) {
> +		u8 evt_status = skb->data[0];
> +
> +		BT_ERR("%s: write update clock event failed (%02x)",
> +		       hdev->name, evt_status);
> +		kfree_skb(skb);
> +		return -bt_to_errno(evt_status);
> +	}
> +
> +	kfree_skb(skb);
> +
> +	return 0;
> +}
> +
> +struct hci_cp_bcm_set_speed {
> +	__le16   dummy;
> +	__le32   speed;
> +} __packed;
> +
> +static int bcm_set_baudrate(struct hci_uart *hu, 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 && bcm_set_clock(hu, BCM43XX_CLOCK_48))
> +		return -EINVAL;

Please just fold this in to this function. I prefer if you can read the changes like a sequence of actions that have to happen.

Also curious is when we fallback to default baudrate, do we have to change the clock back to 24?

> +
> +	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);
> +	}
> +
> +	if (skb->data[0]) {
> +		u8 evt_status = skb->data[0];

This part seems a bit duplicated. I actually wonder if we should change __hci_cmd_sync to also handle command complete events and just extract the error/status for us. I have done it that way in userspace and the command complete return response parameters are guaranteed to have status as their first parameter. So it is doable. Do you mind looking into that.

> +
> +		BT_ERR("%s: write update baudrate event failed (%02x)",
> +		       hdev->name, evt_status);
> +		kfree_skb(skb);
> +		return -bt_to_errno(evt_status);
> +	}
> +
> +	kfree_skb(skb);
> +
> +	return 0;
> +}
> +
> static int bcm_open(struct hci_uart *hu)
> {
> 	struct bcm_data *bcm;
> @@ -145,6 +215,7 @@ static const struct hci_uart_proto bcm_proto = {
> 	.flush		= bcm_flush,
> 	.setup		= bcm_setup,
> 	.post_setup	= bcm_post_setup,
> +	.set_baudrate	= bcm_set_baudrate,
> 	.recv		= bcm_recv,
> 	.enqueue	= bcm_enqueue,
> 	.dequeue	= bcm_dequeue,

Regards

Marcel

Re: [PATCH 7/7] Bluetooth: btbcm: Add bcm_set_baudrate()

[Peter Hurley]

On 04/10/2015 04:19 PM, Marcel Holtmann wrote:
> 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 | 71 +++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 71 insertions(+)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index 25c9883..7308cf4 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,80 @@
>> #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;
>> };
>>
>> +static int bcm_set_clock(struct hci_uart *hu, unsigned char clock)
>> +{
>> +	struct hci_dev *hdev = hu->hdev;
>> +	struct sk_buff *skb;
>> +
>> +	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);
>> +	}
>> +
>> +	if (skb->data[0]) {
>> +		u8 evt_status = skb->data[0];
>> +
>> +		BT_ERR("%s: write update clock event failed (%02x)",
>> +		       hdev->name, evt_status);
>> +		kfree_skb(skb);
>> +		return -bt_to_errno(evt_status);
>> +	}
>> +
>> +	kfree_skb(skb);
>> +
>> +	return 0;
>> +}
>> +
>> +struct hci_cp_bcm_set_speed {
>> +	__le16   dummy;
>> +	__le32   speed;
>> +} __packed;
>> +
>> +static int bcm_set_baudrate(struct hci_uart *hu, 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 && bcm_set_clock(hu, BCM43XX_CLOCK_48))
>> +		return -EINVAL;
> 
> Please just fold this in to this function. I prefer if you can read the changes like a sequence of actions that have to happen.
> 
> Also curious is when we fallback to default baudrate, do we have to change the clock back to 24?
> 
>> +
>> +	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);
>> +	}
>> +
>> +	if (skb->data[0]) {
>> +		u8 evt_status = skb->data[0];
> 
> This part seems a bit duplicated. I actually wonder if we should change __hci_cmd_sync to also handle command complete events and just extract the error/status for us.

So the hci sends command complete at the old line rate?


> I have done it that way in userspace and the command complete return response parameters are guaranteed to have status as their first parameter. So it is doable. Do you mind looking into that.

Re: [PATCH 7/7] Bluetooth: btbcm: Add bcm_set_baudrate()

[Marcel Holtmann]

Hi Peter,

>>> Add vendor specific command to change controller device speed.
>>> 
>>> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
>>> ---
>>> drivers/bluetooth/hci_bcm.c | 71 +++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 71 insertions(+)
>>> 
>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>> index 25c9883..7308cf4 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,80 @@
>>> #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;
>>> };
>>> 
>>> +static int bcm_set_clock(struct hci_uart *hu, unsigned char clock)
>>> +{
>>> +	struct hci_dev *hdev = hu->hdev;
>>> +	struct sk_buff *skb;
>>> +
>>> +	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);
>>> +	}
>>> +
>>> +	if (skb->data[0]) {
>>> +		u8 evt_status = skb->data[0];
>>> +
>>> +		BT_ERR("%s: write update clock event failed (%02x)",
>>> +		       hdev->name, evt_status);
>>> +		kfree_skb(skb);
>>> +		return -bt_to_errno(evt_status);
>>> +	}
>>> +
>>> +	kfree_skb(skb);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +struct hci_cp_bcm_set_speed {
>>> +	__le16   dummy;
>>> +	__le32   speed;
>>> +} __packed;
>>> +
>>> +static int bcm_set_baudrate(struct hci_uart *hu, 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 && bcm_set_clock(hu, BCM43XX_CLOCK_48))
>>> +		return -EINVAL;
>> 
>> Please just fold this in to this function. I prefer if you can read the changes like a sequence of actions that have to happen.
>> 
>> Also curious is when we fallback to default baudrate, do we have to change the clock back to 24?
>> 
>>> +
>>> +	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);
>>> +	}
>>> +
>>> +	if (skb->data[0]) {
>>> +		u8 evt_status = skb->data[0];
>> 
>> This part seems a bit duplicated. I actually wonder if we should change __hci_cmd_sync to also handle command complete events and just extract the error/status for us.
> 
> So the hci sends command complete at the old line rate?

that is what I have seen so far. You receive the command complete and then change the baudrate. This also makes sense since otherwise you could not report errors.

Regards

Marcel

Re: [PATCH 7/7] Bluetooth: btbcm: Add bcm_set_baudrate()

[Frederic Danis]

Hello Marcel,

On 10/04/2015 22:19, Marcel Holtmann wrote:
<snip>
>> +static int bcm_set_clock(struct hci_uart *hu, unsigned char clock)
>> +{
>> +	struct hci_dev *hdev = hu->hdev;
>> +	struct sk_buff *skb;
>> +
>> +	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);
>> +	}
>> +
>> +	if (skb->data[0]) {
>> +		u8 evt_status = skb->data[0];
>> +
>> +		BT_ERR("%s: write update clock event failed (%02x)",
>> +		       hdev->name, evt_status);
>> +		kfree_skb(skb);
>> +		return -bt_to_errno(evt_status);
>> +	}
>> +
>> +	kfree_skb(skb);
>> +
>> +	return 0;
>> +}
>> +
>> +struct hci_cp_bcm_set_speed {
>> +	__le16   dummy;
>> +	__le32   speed;
>> +} __packed;
>> +
>> +static int bcm_set_baudrate(struct hci_uart *hu, 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 && bcm_set_clock(hu, BCM43XX_CLOCK_48))
>> +		return -EINVAL;
>
> Please just fold this in to this function. I prefer if you can read the changes like a sequence of actions that have to happen.

OK, I will move bcm_set_clock code to bcm_set_baudrate

> Also curious is when we fallback to default baudrate, do we have to change the clock back to 24?

Currently, controller speed is set back to default baudrate by firmware 
loading. hci_bcm do not set it explicitly.

>> +
>> +	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);
>> +	}
>> +
>> +	if (skb->data[0]) {
>> +		u8 evt_status = skb->data[0];
>
> This part seems a bit duplicated. I actually wonder if we should change __hci_cmd_sync to also handle command complete events and just extract the error/status for us. I have done it that way in userspace and the command complete return response parameters are guaranteed to have status as their first parameter. So it is doable. Do you mind looking into that.

OK, I will send a patch adding a status parameter to __hci_cmd_sync.

Regards

Fred

-- 
Frederic Danis                            Open Source Technology Center
frederic.danis@intel.com                              Intel Corporation

Re: [PATCH 2/7] Bluetooth: hci_uart: Add HCIUARTSETBAUDRATE ioctl

[Frederic Danis]

Hello Marcel,

On 10/04/2015 20:20, Marcel Holtmann wrote:
> Hi Peter,
>
>>> This allows user space application to set final speed requested for UART
>>> device. UART port is open at init speed by user space application.
>>>
>>> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
>>> ---
>>> drivers/bluetooth/hci_ldisc.c | 6 ++++++
>>> drivers/bluetooth/hci_uart.h  | 2 ++
>>> 2 files changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>>> index 5c9a73f..190a7f8 100644
>>> --- a/drivers/bluetooth/hci_ldisc.c
>>> +++ b/drivers/bluetooth/hci_ldisc.c
>>> @@ -609,6 +609,12 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
>>> 	case HCIUARTGETFLAGS:
>>> 		return hu->hdev_flags;
>>>
>>> +	case HCIUARTSETBAUDRATE:
>>> +		if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
>>> +			return -EBUSY;
>>> +		hu->speed = arg;
>>> +		break;
>>> +
>>
>> So now that the kernel can set line rate, why is an ioctl necessary
>> to determine what the line rate should be?
>
> actually I think we should skip this for now and only introduce it if it is really needed. For the chips we want to support initially we only need the default baudrate and the max baudrate. So the default baudrate is vendor specific and we would just hardcode that in the driver. The max baudrate is platform specific and most likely should come via ACPI or DT. Or some default value in case it is platform specific driver in the first place.
>
> Just as background, this command is not for actually setting the baudrate at that point, it is informing the kernel about the max baudrate for operational use. It will be programmed way later. However as stated above, I think for now we should skip this and only introduce it if the kernel needs input from userspace on the max baudrate.
>
> Fred, I think what we really want at this point is to add default and operational baudrate fields to hci_uart_proto.

OK, I will add default and operational speed to hci_uart_proto.
I tried to find the operational speed in T100's ACPI table but only find 
default speed. (first time I work on ACPI table so I may have missed it).

Regards

Fred

-- 
Frederic Danis                            Open Source Technology Center
frederic.danis@intel.com                              Intel Corporation