Re: [PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

Re: [PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

[Jesse Sung]

Hi Marcel,

Sorry for creating a new thread instead since I didn't subscribe this 
mailing list.

>>>> Interestingly, now that I look at that page more closely, Ubuntu 12.04
>>>> LTS with kernel 3.8 supports this already? Has Ubuntu made a patch
>>>> that bluetooth-next doesn't have?
>>>
>>> That is Ubuntu for you. They just push patches into their kernel and never give them back to upstream or make sure they get merged into bluetooth-next. I really dislike that behavior. It is always the easy way out instead of trying to do the right thing.
>>>
>>
>> See this page: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1065400
>
> It is a stupid userspace version that is racy as hell. Even the bug mentions that it should be done in the kernel. And I am pretty sure that was my initial comment as well. Just posting the userspace patch for reference.

No it's not the stupid userspace loader you're referring to.

And no, these patches are not pushed into Ubuntu kernel only.

These stupid patches were sent to this mailing list for review, for 
three or four times, and all of them were rejected by you. The latest 
version of patches that got rejected should be quite similar to Petri's.

Regards,
Jesse

Re: [PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

[Marcel Holtmann]

Hi Jesse,

> Sorry for creating a new thread instead since I didn't subscribe this mailing list.
> 
>>>>> Interestingly, now that I look at that page more closely, Ubuntu 12.04
>>>>> LTS with kernel 3.8 supports this already? Has Ubuntu made a patch
>>>>> that bluetooth-next doesn't have?
>>>> 
>>>> That is Ubuntu for you. They just push patches into their kernel and never give them back to upstream or make sure they get merged into bluetooth-next. I really dislike that behavior. It is always the easy way out instead of trying to do the right thing.
>>>> 
>>> 
>>> See this page: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1065400
>> 
>> It is a stupid userspace version that is racy as hell. Even the bug mentions that it should be done in the kernel. And I am pretty sure that was my initial comment as well. Just posting the userspace patch for reference.
> 
> No it's not the stupid userspace loader you're referring to.
> 
> And no, these patches are not pushed into Ubuntu kernel only.
> 
> These stupid patches were sent to this mailing list for review, for three or four times, and all of them were rejected by you. The latest version of patches that got rejected should be quite similar to Petri's.

so do you have answers to all the questions I raised with Petri? I can go through the archive and read my own comments, but if I rejected the patches, then there was a reason for that.

Regards

Marcel

Re: [PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

[Jesse Sung]

Hi Marcel,

2014-04-22 2:19 GMT+08:00 Marcel Holtmann <marcel@holtmann.org>:
> Hi Jesse,
>
>> Sorry for creating a new thread instead since I didn't subscribe this ma=
iling list.
>>
>>>>>> Interestingly, now that I look at that page more closely, Ubuntu 12.=
04
>>>>>> LTS with kernel 3.8 supports this already? Has Ubuntu made a patch
>>>>>> that bluetooth-next doesn't have?
>>>>>
>>>>> That is Ubuntu for you. They just push patches into their kernel and =
never give them back to upstream or make sure they get merged into bluetoot=
h-next. I really dislike that behavior. It is always the easy way out inste=
ad of trying to do the right thing.
>>>>>
>>>>
>>>> See this page: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/10=
65400
>>>
>>> It is a stupid userspace version that is racy as hell. Even the bug men=
tions that it should be done in the kernel. And I am pretty sure that was m=
y initial comment as well. Just posting the userspace patch for reference.
>>
>> No it's not the stupid userspace loader you're referring to.
>>
>> And no, these patches are not pushed into Ubuntu kernel only.
>>
>> These stupid patches were sent to this mailing list for review, for thre=
e or four times, and all of them were rejected by you. The latest version o=
f patches that got rejected should be quite similar to Petri's.
>
> so do you have answers to all the questions I raised with Petri? I can go=
 through the archive and read my own comments, but if I rejected the patche=
s, then there was a reason for that.

You did have a reason, and I don't complain about the rejection at all.

The reason I reply is that Petri asked why there's a patch in Ubuntu
and not in upstream, and, with all due respect, the answer of "That is
Ubuntu for you. They just push patches into their kernel and never
give them back to upstream or make sure they get merged into
bluetooth-next." doesn't reflect what happened. The launchpad link
Petri mentioned is all about patching btusb instead of using userspace
loader, but guess you're too busy to read the short description.

For Petri's question, patches have been rejected for several times.
Since these patchram modules are equipped on more and more devices
recently, making them supported is better than just leaving them
unsupported, even with a solution that does not perfectly match the
expectation of upstream IMHO. That's why I tried to push these
rejected patches into Ubuntu kernel. The Ubuntu kernel team accepted
them though they do have concerns about these patches are not accepted
by upstream. I'm still working on a solution when I have time to do
so.

And as I didn't subscribe to this mailing list, would you mind to show
me the questions you're referring to? Most likely I won't have answers
to all of the questions, after all, all the clue I have is the
userspace loader. But I'd be more than happy to give it a try.

Regards,
Jesse

[PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

[Petri Gynther]

After hardware reset, BCM20702 obtains its initial firmware from a PROM chip.
Once this initial firmware is running, the firmware can be further upgraded
over HCI interface with .hcd files provided by Broadcom. This is also known
as "patch RAM" support. This change implements that.

If the .hcd file is not found in /lib/firmware, BCM20702 continues to operate
with the initial firmware. Sample kernel log:
  hotplug: sys=firmware act=add fw=brcm/bcm20702-0a5c-22be.hcd dev=...
  Bluetooth: hci0: BCM20702: patch brcm/bcm20702-0a5c-22be.hcd not found
  Bluetooth: hci0: BCM20702: firmware hci_ver=06 hci_rev=1000 lmp_ver=06 lmp_subver=220e

If the .hcd file is found, btusb driver pushes it to BCM20702, and BCM20702
starts using the new firmware. Sample kernel log:
  hotplug: sys=firmware act=add fw=brcm/bcm20702-0a5c-22be.hcd dev=...
  Bluetooth: hci0: BCM20702: patching hci_ver=06 hci_rev=1000 lmp_ver=06 lmp_subver=220e
  Bluetooth: hci0: BCM20702: firmware hci_ver=06 hci_rev=1389 lmp_ver=06 lmp_subver=220e

Above, we can see that hci_rev goes from 1000 to 1389 as a result of the upgrade.

Signed-off-by: Petri Gynther <pgynther@google.com>
---
 drivers/bluetooth/btusb.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index f338b0c..371d7e9 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -49,6 +49,7 @@ static struct usb_driver btusb_driver;
 #define BTUSB_WRONG_SCO_MTU	0x40
 #define BTUSB_ATH3012		0x80
 #define BTUSB_INTEL		0x100
+#define BTUSB_BCM20702_PATCHRAM	0x200
 
 static const struct usb_device_id btusb_table[] = {
 	/* Generic Bluetooth USB device */
@@ -103,6 +104,7 @@ static const struct usb_device_id btusb_table[] = {
 	/* Broadcom BCM20702A0 */
 	{ USB_DEVICE(0x0489, 0xe042) },
 	{ USB_DEVICE(0x04ca, 0x2003) },
+	{ USB_DEVICE(0x0a5c, 0x22be), .driver_info = BTUSB_BCM20702_PATCHRAM },
 	{ USB_DEVICE(0x0b05, 0x17b5) },
 	{ USB_DEVICE(0x0b05, 0x17cb) },
 	{ USB_DEVICE(0x413c, 0x8197) },
@@ -1380,6 +1382,140 @@ exit_mfg_deactivate:
 	return 0;
 }
 
+static int btusb_setup_bcm20702(struct hci_dev *hdev)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+	struct usb_device *udev = data->udev;
+	char fw_name[64];
+	const struct firmware *fw;
+	const u8 *fw_ptr;
+	size_t fw_size;
+	const struct hci_command_hdr *cmd;
+	const u8 *cmd_param;
+	u16 opcode;
+	struct sk_buff *skb;
+	struct hci_rp_read_local_version *ver;
+	long ret;
+
+	snprintf(fw_name, sizeof(fw_name), "brcm/bcm20702-%04x-%04x.hcd",
+		 le16_to_cpu(udev->descriptor.idVendor),
+		 le16_to_cpu(udev->descriptor.idProduct));
+
+	ret = request_firmware(&fw, fw_name, &hdev->dev);
+	if (ret < 0) {
+		BT_INFO("%s: BCM20702: patch %s not found", hdev->name,
+			fw_name);
+		ret = 0;
+		goto get_fw_ver;
+	}
+
+	/* Reset */
+	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		ret = PTR_ERR(skb);
+		BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret);
+		goto reset_fw;
+	}
+	kfree_skb(skb);
+
+	/* Read Local Version Info */
+	skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
+			     HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		ret = PTR_ERR(skb);
+		BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
+			hdev->name, ret);
+		goto reset_fw;
+	}
+
+	ver = (struct hci_rp_read_local_version *) skb->data;
+	BT_INFO("%s: BCM20702: patching hci_ver=%02x hci_rev=%04x lmp_ver=%02x "
+		"lmp_subver=%04x", hdev->name, ver->hci_ver, ver->hci_rev,
+		ver->lmp_ver, ver->lmp_subver);
+	kfree_skb(skb);
+
+	/* Start Download */
+	skb = __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		ret = PTR_ERR(skb);
+		BT_ERR("%s: BCM20702: Download Minidrv command failed (%ld)",
+			hdev->name, ret);
+		goto reset_fw;
+	}
+	kfree_skb(skb);
+
+	/* 50 msec delay after Download Minidrv completes */
+	msleep(50);
+
+	fw_ptr = fw->data;
+	fw_size = fw->size;
+
+	while (fw_size >= sizeof(*cmd)) {
+		cmd = (struct hci_command_hdr *) fw_ptr;
+		fw_ptr += sizeof(*cmd);
+		fw_size -= sizeof(*cmd);
+
+		if (fw_size < cmd->plen) {
+			BT_ERR("%s: BCM20702: patch %s is corrupted",
+				hdev->name, fw_name);
+			ret = -EINVAL;
+			goto reset_fw;
+		}
+
+		cmd_param = fw_ptr;
+		fw_ptr += cmd->plen;
+		fw_size -= cmd->plen;
+
+		opcode = le16_to_cpu(cmd->opcode);
+
+		skb = __hci_cmd_sync(hdev, opcode, cmd->plen, cmd_param,
+				     HCI_INIT_TIMEOUT);
+		if (IS_ERR(skb)) {
+			ret = PTR_ERR(skb);
+			BT_ERR("%s: BCM20702: patch command %04x failed (%ld)",
+				hdev->name, opcode, ret);
+			goto reset_fw;
+		}
+		kfree_skb(skb);
+	}
+
+	/* 250 msec delay after Launch Ram completes */
+	msleep(250);
+
+reset_fw:
+	/* Reset */
+	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		ret = PTR_ERR(skb);
+		BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret);
+		goto done;
+	}
+	kfree_skb(skb);
+
+get_fw_ver:
+	/* Read Local Version Info */
+	skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
+			     HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		ret = PTR_ERR(skb);
+		BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
+			hdev->name, ret);
+		goto done;
+	}
+
+	ver = (struct hci_rp_read_local_version *) skb->data;
+	BT_INFO("%s: BCM20702: firmware hci_ver=%02x hci_rev=%04x lmp_ver=%02x "
+		"lmp_subver=%04x", hdev->name, ver->hci_ver, ver->hci_rev,
+		ver->lmp_ver, ver->lmp_subver);
+	kfree_skb(skb);
+
+done:
+	if (fw)
+		release_firmware(fw);
+
+	return ret;
+}
+
 static int btusb_probe(struct usb_interface *intf,
 				const struct usb_device_id *id)
 {
@@ -1485,6 +1621,9 @@ static int btusb_probe(struct usb_interface *intf,
 	if (id->driver_info & BTUSB_BCM92035)
 		hdev->setup = btusb_setup_bcm92035;
 
+	if (id->driver_info & BTUSB_BCM20702_PATCHRAM)
+		hdev->setup = btusb_setup_bcm20702;
+
 	if (id->driver_info & BTUSB_INTEL) {
 		usb_enable_autosuspend(data->udev);
 		hdev->setup = btusb_setup_intel;
-- 
1.9.1.423.g4596e3a

Re: [PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

[Marcel Holtmann]

Hi Petri,

> After hardware reset, BCM20702 obtains its initial firmware from a PROM chip.
> Once this initial firmware is running, the firmware can be further upgraded
> over HCI interface with .hcd files provided by Broadcom. This is also known
> as "patch RAM" support. This change implements that.
> 
> If the .hcd file is not found in /lib/firmware, BCM20702 continues to operate
> with the initial firmware. Sample kernel log:
>  hotplug: sys=firmware act=add fw=brcm/bcm20702-0a5c-22be.hcd dev=...
>  Bluetooth: hci0: BCM20702: patch brcm/bcm20702-0a5c-22be.hcd not found
>  Bluetooth: hci0: BCM20702: firmware hci_ver=06 hci_rev=1000 lmp_ver=06 lmp_subver=220e
> 
> If the .hcd file is found, btusb driver pushes it to BCM20702, and BCM20702
> starts using the new firmware. Sample kernel log:
>  hotplug: sys=firmware act=add fw=brcm/bcm20702-0a5c-22be.hcd dev=...
>  Bluetooth: hci0: BCM20702: patching hci_ver=06 hci_rev=1000 lmp_ver=06 lmp_subver=220e
>  Bluetooth: hci0: BCM20702: firmware hci_ver=06 hci_rev=1389 lmp_ver=06 lmp_subver=220e
> 
> Above, we can see that hci_rev goes from 1000 to 1389 as a result of the upgrade.

this is a nice feature to have. Where are these hcd files are coming from. Are they available somewhere or do we have them available in linux-firmware.git tree.

I also wonder if there is a bit more general naming convention for the files. For example with a quick search I found references to BCM2045B2_002.002.011.0348.0349.hcd and similar. They indicate that the naming is not based on the USB vendor and product ID.

For example for the Intel ROM patching, we used internal versioning to pick the right file. That way we were able to run it for all of our devices. For the Broadcom devices, it might make sense to run the patchram routine for all devices not just this one specific one.

> Signed-off-by: Petri Gynther <pgynther@google.com>
> ---
> drivers/bluetooth/btusb.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 139 insertions(+)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index f338b0c..371d7e9 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -49,6 +49,7 @@ static struct usb_driver btusb_driver;
> #define BTUSB_WRONG_SCO_MTU	0x40
> #define BTUSB_ATH3012		0x80
> #define BTUSB_INTEL		0x100
> +#define BTUSB_BCM20702_PATCHRAM	0x200
> 
> static const struct usb_device_id btusb_table[] = {
> 	/* Generic Bluetooth USB device */
> @@ -103,6 +104,7 @@ static const struct usb_device_id btusb_table[] = {
> 	/* Broadcom BCM20702A0 */
> 	{ USB_DEVICE(0x0489, 0xe042) },
> 	{ USB_DEVICE(0x04ca, 0x2003) },
> +	{ USB_DEVICE(0x0a5c, 0x22be), .driver_info = BTUSB_BCM20702_PATCHRAM },
> 	{ USB_DEVICE(0x0b05, 0x17b5) },
> 	{ USB_DEVICE(0x0b05, 0x17cb) },
> 	{ USB_DEVICE(0x413c, 0x8197) },
> @@ -1380,6 +1382,140 @@ exit_mfg_deactivate:
> 	return 0;
> }
> 
> +static int btusb_setup_bcm20702(struct hci_dev *hdev)
> +{
> +	struct btusb_data *data = hci_get_drvdata(hdev);
> +	struct usb_device *udev = data->udev;
> +	char fw_name[64];
> +	const struct firmware *fw;
> +	const u8 *fw_ptr;
> +	size_t fw_size;
> +	const struct hci_command_hdr *cmd;
> +	const u8 *cmd_param;
> +	u16 opcode;
> +	struct sk_buff *skb;
> +	struct hci_rp_read_local_version *ver;
> +	long ret;
> +
> +	snprintf(fw_name, sizeof(fw_name), "brcm/bcm20702-%04x-%04x.hcd",
> +		 le16_to_cpu(udev->descriptor.idVendor),
> +		 le16_to_cpu(udev->descriptor.idProduct));
> +
> +	ret = request_firmware(&fw, fw_name, &hdev->dev);
> +	if (ret < 0) {
> +		BT_INFO("%s: BCM20702: patch %s not found", hdev->name,
> +			fw_name);
> +		ret = 0;
> +		goto get_fw_ver;
> +	}
> +
> +	/* Reset */
> +	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		ret = PTR_ERR(skb);
> +		BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret);
> +		goto reset_fw;
> +	}
> +	kfree_skb(skb);
> +
> +	/* Read Local Version Info */
> +	skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
> +			     HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		ret = PTR_ERR(skb);
> +		BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
> +			hdev->name, ret);
> +		goto reset_fw;
> +	}
> +

You need a length check here to ensure that resulting buffer is the correct length. Trusting the hardware is not a good idea. I have seen this go wrong in some cases.

> +	ver = (struct hci_rp_read_local_version *) skb->data;
> +	BT_INFO("%s: BCM20702: patching hci_ver=%02x hci_rev=%04x lmp_ver=%02x "
> +		"lmp_subver=%04x", hdev->name, ver->hci_ver, ver->hci_rev,
> +		ver->lmp_ver, ver->lmp_subver);
> +	kfree_skb(skb);
> +
> +	/* Start Download */
> +	skb = __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		ret = PTR_ERR(skb);
> +		BT_ERR("%s: BCM20702: Download Minidrv command failed (%ld)",
> +			hdev->name, ret);
> +		goto reset_fw;
> +	}
> +	kfree_skb(skb);
> +
> +	/* 50 msec delay after Download Minidrv completes */
> +	msleep(50);
> +
> +	fw_ptr = fw->data;
> +	fw_size = fw->size;
> +
> +	while (fw_size >= sizeof(*cmd)) {
> +		cmd = (struct hci_command_hdr *) fw_ptr;
> +		fw_ptr += sizeof(*cmd);
> +		fw_size -= sizeof(*cmd);
> +
> +		if (fw_size < cmd->plen) {
> +			BT_ERR("%s: BCM20702: patch %s is corrupted",
> +				hdev->name, fw_name);
> +			ret = -EINVAL;
> +			goto reset_fw;
> +		}
> +
> +		cmd_param = fw_ptr;
> +		fw_ptr += cmd->plen;
> +		fw_size -= cmd->plen;
> +
> +		opcode = le16_to_cpu(cmd->opcode);
> +
> +		skb = __hci_cmd_sync(hdev, opcode, cmd->plen, cmd_param,
> +				     HCI_INIT_TIMEOUT);
> +		if (IS_ERR(skb)) {
> +			ret = PTR_ERR(skb);
> +			BT_ERR("%s: BCM20702: patch command %04x failed (%ld)",
> +				hdev->name, opcode, ret);
> +			goto reset_fw;
> +		}
> +		kfree_skb(skb);
> +	}
> +
> +	/* 250 msec delay after Launch Ram completes */
> +	msleep(250);
> +
> +reset_fw:
> +	/* Reset */
> +	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		ret = PTR_ERR(skb);
> +		BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret);
> +		goto done;
> +	}
> +	kfree_skb(skb);
> +
> +get_fw_ver:
> +	/* Read Local Version Info */
> +	skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
> +			     HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		ret = PTR_ERR(skb);
> +		BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
> +			hdev->name, ret);
> +		goto done;
> +	}
> +
> +	ver = (struct hci_rp_read_local_version *) skb->data;
> +	BT_INFO("%s: BCM20702: firmware hci_ver=%02x hci_rev=%04x lmp_ver=%02x "
> +		"lmp_subver=%04x", hdev->name, ver->hci_ver, ver->hci_rev,
> +		ver->lmp_ver, ver->lmp_subver);
> +	kfree_skb(skb);

Is this HCI_Reset and HCI_Read_Local_Version_Info really needed. The standard init procedure will do exactly that anyway. I have no problem in having this in here. I am just saying that it will be run anyway again after ->setup() completed.

You can check for yourself when running btmon before plugging this device or manually loading btusb.

> +
> +done:
> +	if (fw)
> +		release_firmware(fw);
> +
> +	return ret;
> +}
> +
> static int btusb_probe(struct usb_interface *intf,
> 				const struct usb_device_id *id)
> {
> @@ -1485,6 +1621,9 @@ static int btusb_probe(struct usb_interface *intf,
> 	if (id->driver_info & BTUSB_BCM92035)
> 		hdev->setup = btusb_setup_bcm92035;
> 
> +	if (id->driver_info & BTUSB_BCM20702_PATCHRAM)
> +		hdev->setup = btusb_setup_bcm20702;
> +

If we are somehow able to just do BTUSB_BROADCOM and always trigger the patchram loading based on the hardware found that would be super great.

> 	if (id->driver_info & BTUSB_INTEL) {
> 		usb_enable_autosuspend(data->udev);
> 		hdev->setup = btusb_setup_intel;

Regards

Marcel

Re: [PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

[Petri Gynther]

Hi Marcel,

On Tue, Apr 8, 2014 at 6:36 PM, Marcel Holtmann <marcel@holtmann.org> wrote=
:
> Hi Petri,
>
>> After hardware reset, BCM20702 obtains its initial firmware from a PROM =
chip.
>> Once this initial firmware is running, the firmware can be further upgra=
ded
>> over HCI interface with .hcd files provided by Broadcom. This is also kn=
own
>> as "patch RAM" support. This change implements that.
>>
>> If the .hcd file is not found in /lib/firmware, BCM20702 continues to op=
erate
>> with the initial firmware. Sample kernel log:
>>  hotplug: sys=3Dfirmware act=3Dadd fw=3Dbrcm/bcm20702-0a5c-22be.hcd dev=
=3D...
>>  Bluetooth: hci0: BCM20702: patch brcm/bcm20702-0a5c-22be.hcd not found
>>  Bluetooth: hci0: BCM20702: firmware hci_ver=3D06 hci_rev=3D1000 lmp_ver=
=3D06 lmp_subver=3D220e
>>
>> If the .hcd file is found, btusb driver pushes it to BCM20702, and BCM20=
702
>> starts using the new firmware. Sample kernel log:
>>  hotplug: sys=3Dfirmware act=3Dadd fw=3Dbrcm/bcm20702-0a5c-22be.hcd dev=
=3D...
>>  Bluetooth: hci0: BCM20702: patching hci_ver=3D06 hci_rev=3D1000 lmp_ver=
=3D06 lmp_subver=3D220e
>>  Bluetooth: hci0: BCM20702: firmware hci_ver=3D06 hci_rev=3D1389 lmp_ver=
=3D06 lmp_subver=3D220e
>>
>> Above, we can see that hci_rev goes from 1000 to 1389 as a result of the=
 upgrade.
>
> this is a nice feature to have. Where are these hcd files are coming from=
. Are they available somewhere or do we have them available in linux-firmwa=
re.git tree.

In our case, we get these files directly from the vendor. I haven't
seen these in linux-firmware.git tree. Some searching reveals that
Windows drivers contain either .hex or .hcd files, and there appears
to be a tool hex2hcd to convert to .hcd. See page
http://swiesmann.de/?p=3D30.

Interestingly, now that I look at that page more closely, Ubuntu 12.04
LTS with kernel 3.8 supports this already? Has Ubuntu made a patch
that bluetooth-next doesn't have?

>
> I also wonder if there is a bit more general naming convention for the fi=
les. For example with a quick search I found references to BCM2045B2_002.00=
2.011.0348.0349.hcd and similar. They indicate that the naming is not based=
 on the USB vendor and product ID.

Some version numbering scheme is applied by Broadcom. However, at
least initially, I'd prefer using USB vid/pid in the .hcd filename. I
know that the .hcd file that I have works on our specific vid/pid of
BCM20702, but I have no idea whether it would work on other BCM20702
variants.

>
> For example for the Intel ROM patching, we used internal versioning to pi=
ck the right file. That way we were able to run it for all of our devices. =
For the Broadcom devices, it might make sense to run the patchram routine f=
or all devices not just this one specific one.

It would make sense, but I have no way of testing it. I only feel
comfortable introducing this to the single device that I've been able
to test with. It can later be extended to other BCM variants as people
test this.

>
>> Signed-off-by: Petri Gynther <pgynther@google.com>
>> ---
>> drivers/bluetooth/btusb.c | 139 ++++++++++++++++++++++++++++++++++++++++=
++++++
>> 1 file changed, 139 insertions(+)
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index f338b0c..371d7e9 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -49,6 +49,7 @@ static struct usb_driver btusb_driver;
>> #define BTUSB_WRONG_SCO_MTU   0x40
>> #define BTUSB_ATH3012         0x80
>> #define BTUSB_INTEL           0x100
>> +#define BTUSB_BCM20702_PATCHRAM      0x200
>>
>> static const struct usb_device_id btusb_table[] =3D {
>>       /* Generic Bluetooth USB device */
>> @@ -103,6 +104,7 @@ static const struct usb_device_id btusb_table[] =3D =
{
>>       /* Broadcom BCM20702A0 */
>>       { USB_DEVICE(0x0489, 0xe042) },
>>       { USB_DEVICE(0x04ca, 0x2003) },
>> +     { USB_DEVICE(0x0a5c, 0x22be), .driver_info =3D BTUSB_BCM20702_PATC=
HRAM },
>>       { USB_DEVICE(0x0b05, 0x17b5) },
>>       { USB_DEVICE(0x0b05, 0x17cb) },
>>       { USB_DEVICE(0x413c, 0x8197) },
>> @@ -1380,6 +1382,140 @@ exit_mfg_deactivate:
>>       return 0;
>> }
>>
>> +static int btusb_setup_bcm20702(struct hci_dev *hdev)
>> +{
>> +     struct btusb_data *data =3D hci_get_drvdata(hdev);
>> +     struct usb_device *udev =3D data->udev;
>> +     char fw_name[64];
>> +     const struct firmware *fw;
>> +     const u8 *fw_ptr;
>> +     size_t fw_size;
>> +     const struct hci_command_hdr *cmd;
>> +     const u8 *cmd_param;
>> +     u16 opcode;
>> +     struct sk_buff *skb;
>> +     struct hci_rp_read_local_version *ver;
>> +     long ret;
>> +
>> +     snprintf(fw_name, sizeof(fw_name), "brcm/bcm20702-%04x-%04x.hcd",
>> +              le16_to_cpu(udev->descriptor.idVendor),
>> +              le16_to_cpu(udev->descriptor.idProduct));
>> +
>> +     ret =3D request_firmware(&fw, fw_name, &hdev->dev);
>> +     if (ret < 0) {
>> +             BT_INFO("%s: BCM20702: patch %s not found", hdev->name,
>> +                     fw_name);
>> +             ret =3D 0;
>> +             goto get_fw_ver;
>> +     }
>> +
>> +     /* Reset */
>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEO=
UT);
>> +     if (IS_ERR(skb)) {
>> +             ret =3D PTR_ERR(skb);
>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret);
>> +             goto reset_fw;
>> +     }
>> +     kfree_skb(skb);
>> +
>> +     /* Read Local Version Info */
>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
>> +                          HCI_INIT_TIMEOUT);
>> +     if (IS_ERR(skb)) {
>> +             ret =3D PTR_ERR(skb);
>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>> +                     hdev->name, ret);
>> +             goto reset_fw;
>> +     }
>> +
>
> You need a length check here to ensure that resulting buffer is the corre=
ct length. Trusting the hardware is not a good idea. I have seen this go wr=
ong in some cases.

Will do. I'll send revised diff for this.

>
>> +     ver =3D (struct hci_rp_read_local_version *) skb->data;
>> +     BT_INFO("%s: BCM20702: patching hci_ver=3D%02x hci_rev=3D%04x lmp_=
ver=3D%02x "
>> +             "lmp_subver=3D%04x", hdev->name, ver->hci_ver, ver->hci_re=
v,
>> +             ver->lmp_ver, ver->lmp_subver);
>> +     kfree_skb(skb);
>> +
>> +     /* Start Download */
>> +     skb =3D __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT);
>> +     if (IS_ERR(skb)) {
>> +             ret =3D PTR_ERR(skb);
>> +             BT_ERR("%s: BCM20702: Download Minidrv command failed (%ld=
)",
>> +                     hdev->name, ret);
>> +             goto reset_fw;
>> +     }
>> +     kfree_skb(skb);
>> +
>> +     /* 50 msec delay after Download Minidrv completes */
>> +     msleep(50);
>> +
>> +     fw_ptr =3D fw->data;
>> +     fw_size =3D fw->size;
>> +
>> +     while (fw_size >=3D sizeof(*cmd)) {
>> +             cmd =3D (struct hci_command_hdr *) fw_ptr;
>> +             fw_ptr +=3D sizeof(*cmd);
>> +             fw_size -=3D sizeof(*cmd);
>> +
>> +             if (fw_size < cmd->plen) {
>> +                     BT_ERR("%s: BCM20702: patch %s is corrupted",
>> +                             hdev->name, fw_name);
>> +                     ret =3D -EINVAL;
>> +                     goto reset_fw;
>> +             }
>> +
>> +             cmd_param =3D fw_ptr;
>> +             fw_ptr +=3D cmd->plen;
>> +             fw_size -=3D cmd->plen;
>> +
>> +             opcode =3D le16_to_cpu(cmd->opcode);
>> +
>> +             skb =3D __hci_cmd_sync(hdev, opcode, cmd->plen, cmd_param,
>> +                                  HCI_INIT_TIMEOUT);
>> +             if (IS_ERR(skb)) {
>> +                     ret =3D PTR_ERR(skb);
>> +                     BT_ERR("%s: BCM20702: patch command %04x failed (%=
ld)",
>> +                             hdev->name, opcode, ret);
>> +                     goto reset_fw;
>> +             }
>> +             kfree_skb(skb);
>> +     }
>> +
>> +     /* 250 msec delay after Launch Ram completes */
>> +     msleep(250);
>> +
>> +reset_fw:
>> +     /* Reset */
>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEO=
UT);
>> +     if (IS_ERR(skb)) {
>> +             ret =3D PTR_ERR(skb);
>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret);
>> +             goto done;
>> +     }
>> +     kfree_skb(skb);
>> +
>> +get_fw_ver:
>> +     /* Read Local Version Info */
>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
>> +                          HCI_INIT_TIMEOUT);
>> +     if (IS_ERR(skb)) {
>> +             ret =3D PTR_ERR(skb);
>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>> +                     hdev->name, ret);
>> +             goto done;
>> +     }
>> +
>> +     ver =3D (struct hci_rp_read_local_version *) skb->data;
>> +     BT_INFO("%s: BCM20702: firmware hci_ver=3D%02x hci_rev=3D%04x lmp_=
ver=3D%02x "
>> +             "lmp_subver=3D%04x", hdev->name, ver->hci_ver, ver->hci_re=
v,
>> +             ver->lmp_ver, ver->lmp_subver);
>> +     kfree_skb(skb);
>
> Is this HCI_Reset and HCI_Read_Local_Version_Info really needed. The stan=
dard init procedure will do exactly that anyway. I have no problem in havin=
g this in here. I am just saying that it will be run anyway again after ->s=
etup() completed.
>
> You can check for yourself when running btmon before plugging this device=
 or manually loading btusb.

The reset is important because it activates the new firmware. And, I
do like seeing the local version info in kernel log because it makes
it clear what FW version was before and is after the upgrade.

>
>> +
>> +done:
>> +     if (fw)
>> +             release_firmware(fw);
>> +
>> +     return ret;
>> +}
>> +
>> static int btusb_probe(struct usb_interface *intf,
>>                               const struct usb_device_id *id)
>> {
>> @@ -1485,6 +1621,9 @@ static int btusb_probe(struct usb_interface *intf,
>>       if (id->driver_info & BTUSB_BCM92035)
>>               hdev->setup =3D btusb_setup_bcm92035;
>>
>> +     if (id->driver_info & BTUSB_BCM20702_PATCHRAM)
>> +             hdev->setup =3D btusb_setup_bcm20702;
>> +
>
> If we are somehow able to just do BTUSB_BROADCOM and always trigger the p=
atchram loading based on the hardware found that would be super great.

It would be nice, but I don't have enough resources to test it. I'd
say let's start with this and extend from there.

-- Petri

>
>>       if (id->driver_info & BTUSB_INTEL) {
>>               usb_enable_autosuspend(data->udev);
>>               hdev->setup =3D btusb_setup_intel;
>
> Regards
>
> Marcel
>

Re: [PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

[Marcel Holtmann]

Hi Petri,

>>> After hardware reset, BCM20702 obtains its initial firmware from a PROM chip.
>>> Once this initial firmware is running, the firmware can be further upgraded
>>> over HCI interface with .hcd files provided by Broadcom. This is also known
>>> as "patch RAM" support. This change implements that.
>>> 
>>> If the .hcd file is not found in /lib/firmware, BCM20702 continues to operate
>>> with the initial firmware. Sample kernel log:
>>> hotplug: sys=firmware act=add fw=brcm/bcm20702-0a5c-22be.hcd dev=...
>>> Bluetooth: hci0: BCM20702: patch brcm/bcm20702-0a5c-22be.hcd not found
>>> Bluetooth: hci0: BCM20702: firmware hci_ver=06 hci_rev=1000 lmp_ver=06 lmp_subver=220e
>>> 
>>> If the .hcd file is found, btusb driver pushes it to BCM20702, and BCM20702
>>> starts using the new firmware. Sample kernel log:
>>> hotplug: sys=firmware act=add fw=brcm/bcm20702-0a5c-22be.hcd dev=...
>>> Bluetooth: hci0: BCM20702: patching hci_ver=06 hci_rev=1000 lmp_ver=06 lmp_subver=220e
>>> Bluetooth: hci0: BCM20702: firmware hci_ver=06 hci_rev=1389 lmp_ver=06 lmp_subver=220e
>>> 
>>> Above, we can see that hci_rev goes from 1000 to 1389 as a result of the upgrade.
>> 
>> this is a nice feature to have. Where are these hcd files are coming from. Are they available somewhere or do we have them available in linux-firmware.git tree.
> 
> In our case, we get these files directly from the vendor. I haven't
> seen these in linux-firmware.git tree. Some searching reveals that
> Windows drivers contain either .hex or .hcd files, and there appears
> to be a tool hex2hcd to convert to .hcd. See page
> http://swiesmann.de/?p=30.

what is the firmware name that the files from the vendor have. My problem with introducing a VID+PID based naming is that it might causes problems when the VID+PID is re-used for a different SKU or revision of the same board and we should have picked the firmware based on the board version.

And the HEX vs HCD is most likely the same as Intel’s seq vs bseq. Just one is a text file and the other a binary version of the same data. In all cases these are HCI commands anyway.

> Interestingly, now that I look at that page more closely, Ubuntu 12.04
> LTS with kernel 3.8 supports this already? Has Ubuntu made a patch
> that bluetooth-next doesn't have?

That is Ubuntu for you. They just push patches into their kernel and never give them back to upstream or make sure they get merged into bluetooth-next. I really dislike that behavior. It is always the easy way out instead of trying to do the right thing.

>> I also wonder if there is a bit more general naming convention for the files. For example with a quick search I found references to BCM2045B2_002.002.011.0348.0349.hcd and similar. They indicate that the naming is not based on the USB vendor and product ID.
> 
> Some version numbering scheme is applied by Broadcom. However, at
> least initially, I'd prefer using USB vid/pid in the .hcd filename. I
> know that the .hcd file that I have works on our specific vid/pid of
> BCM20702, but I have no idea whether it would work on other BCM20702
> variants.

I am worried to back us into a corner. Reuse of PIDs is plenty. Especially with the cheaper dongle manufactures.

>> For example for the Intel ROM patching, we used internal versioning to pick the right file. That way we were able to run it for all of our devices. For the Broadcom devices, it might make sense to run the patchram routine for all devices not just this one specific one.
> 
> It would make sense, but I have no way of testing it. I only feel
> comfortable introducing this to the single device that I've been able
> to test with. It can later be extended to other BCM variants as people
> test this.

I have a bunch of Broadcom USB dongles around here. I might not get to it this week, but worth while trying to see how this works out.

At least the product string could be taken from the USB descriptors. Seems that Broadcom devices are pretty good in that regard no matter what. So quickly checking with mine, they give you BCM20702A0 and from the looks of it, the firmware files are prefix with exactly that.

>>> Signed-off-by: Petri Gynther <pgynther@google.com>
>>> ---
>>> drivers/bluetooth/btusb.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 139 insertions(+)
>>> 
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index f338b0c..371d7e9 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -49,6 +49,7 @@ static struct usb_driver btusb_driver;
>>> #define BTUSB_WRONG_SCO_MTU   0x40
>>> #define BTUSB_ATH3012         0x80
>>> #define BTUSB_INTEL           0x100
>>> +#define BTUSB_BCM20702_PATCHRAM      0x200
>>> 
>>> static const struct usb_device_id btusb_table[] = {
>>>      /* Generic Bluetooth USB device */
>>> @@ -103,6 +104,7 @@ static const struct usb_device_id btusb_table[] = {
>>>      /* Broadcom BCM20702A0 */
>>>      { USB_DEVICE(0x0489, 0xe042) },
>>>      { USB_DEVICE(0x04ca, 0x2003) },
>>> +     { USB_DEVICE(0x0a5c, 0x22be), .driver_info = BTUSB_BCM20702_PATCHRAM },
>>>      { USB_DEVICE(0x0b05, 0x17b5) },
>>>      { USB_DEVICE(0x0b05, 0x17cb) },
>>>      { USB_DEVICE(0x413c, 0x8197) },
>>> @@ -1380,6 +1382,140 @@ exit_mfg_deactivate:
>>>      return 0;
>>> }
>>> 
>>> +static int btusb_setup_bcm20702(struct hci_dev *hdev)
>>> +{
>>> +     struct btusb_data *data = hci_get_drvdata(hdev);
>>> +     struct usb_device *udev = data->udev;
>>> +     char fw_name[64];
>>> +     const struct firmware *fw;
>>> +     const u8 *fw_ptr;
>>> +     size_t fw_size;
>>> +     const struct hci_command_hdr *cmd;
>>> +     const u8 *cmd_param;
>>> +     u16 opcode;
>>> +     struct sk_buff *skb;
>>> +     struct hci_rp_read_local_version *ver;
>>> +     long ret;
>>> +
>>> +     snprintf(fw_name, sizeof(fw_name), "brcm/bcm20702-%04x-%04x.hcd",
>>> +              le16_to_cpu(udev->descriptor.idVendor),
>>> +              le16_to_cpu(udev->descriptor.idProduct));
>>> +
>>> +     ret = request_firmware(&fw, fw_name, &hdev->dev);
>>> +     if (ret < 0) {
>>> +             BT_INFO("%s: BCM20702: patch %s not found", hdev->name,
>>> +                     fw_name);
>>> +             ret = 0;
>>> +             goto get_fw_ver;
>>> +     }
>>> +
>>> +     /* Reset */
>>> +     skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>>> +     if (IS_ERR(skb)) {
>>> +             ret = PTR_ERR(skb);
>>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret);
>>> +             goto reset_fw;
>>> +     }
>>> +     kfree_skb(skb);
>>> +
>>> +     /* Read Local Version Info */
>>> +     skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
>>> +                          HCI_INIT_TIMEOUT);
>>> +     if (IS_ERR(skb)) {
>>> +             ret = PTR_ERR(skb);
>>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>>> +                     hdev->name, ret);
>>> +             goto reset_fw;
>>> +     }
>>> +
>> 
>> You need a length check here to ensure that resulting buffer is the correct length. Trusting the hardware is not a good idea. I have seen this go wrong in some cases.
> 
> Will do. I'll send revised diff for this.
> 
>> 
>>> +     ver = (struct hci_rp_read_local_version *) skb->data;
>>> +     BT_INFO("%s: BCM20702: patching hci_ver=%02x hci_rev=%04x lmp_ver=%02x "
>>> +             "lmp_subver=%04x", hdev->name, ver->hci_ver, ver->hci_rev,
>>> +             ver->lmp_ver, ver->lmp_subver);
>>> +     kfree_skb(skb);
>>> +
>>> +     /* Start Download */
>>> +     skb = __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT);
>>> +     if (IS_ERR(skb)) {
>>> +             ret = PTR_ERR(skb);
>>> +             BT_ERR("%s: BCM20702: Download Minidrv command failed (%ld)",
>>> +                     hdev->name, ret);
>>> +             goto reset_fw;
>>> +     }
>>> +     kfree_skb(skb);
>>> +
>>> +     /* 50 msec delay after Download Minidrv completes */
>>> +     msleep(50);
>>> +
>>> +     fw_ptr = fw->data;
>>> +     fw_size = fw->size;
>>> +
>>> +     while (fw_size >= sizeof(*cmd)) {
>>> +             cmd = (struct hci_command_hdr *) fw_ptr;
>>> +             fw_ptr += sizeof(*cmd);
>>> +             fw_size -= sizeof(*cmd);
>>> +
>>> +             if (fw_size < cmd->plen) {
>>> +                     BT_ERR("%s: BCM20702: patch %s is corrupted",
>>> +                             hdev->name, fw_name);
>>> +                     ret = -EINVAL;
>>> +                     goto reset_fw;
>>> +             }
>>> +
>>> +             cmd_param = fw_ptr;
>>> +             fw_ptr += cmd->plen;
>>> +             fw_size -= cmd->plen;
>>> +
>>> +             opcode = le16_to_cpu(cmd->opcode);
>>> +
>>> +             skb = __hci_cmd_sync(hdev, opcode, cmd->plen, cmd_param,
>>> +                                  HCI_INIT_TIMEOUT);
>>> +             if (IS_ERR(skb)) {
>>> +                     ret = PTR_ERR(skb);
>>> +                     BT_ERR("%s: BCM20702: patch command %04x failed (%ld)",
>>> +                             hdev->name, opcode, ret);
>>> +                     goto reset_fw;
>>> +             }
>>> +             kfree_skb(skb);
>>> +     }
>>> +
>>> +     /* 250 msec delay after Launch Ram completes */
>>> +     msleep(250);
>>> +
>>> +reset_fw:
>>> +     /* Reset */
>>> +     skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>>> +     if (IS_ERR(skb)) {
>>> +             ret = PTR_ERR(skb);
>>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret);
>>> +             goto done;
>>> +     }
>>> +     kfree_skb(skb);
>>> +
>>> +get_fw_ver:
>>> +     /* Read Local Version Info */
>>> +     skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
>>> +                          HCI_INIT_TIMEOUT);
>>> +     if (IS_ERR(skb)) {
>>> +             ret = PTR_ERR(skb);
>>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>>> +                     hdev->name, ret);
>>> +             goto done;
>>> +     }
>>> +
>>> +     ver = (struct hci_rp_read_local_version *) skb->data;
>>> +     BT_INFO("%s: BCM20702: firmware hci_ver=%02x hci_rev=%04x lmp_ver=%02x "
>>> +             "lmp_subver=%04x", hdev->name, ver->hci_ver, ver->hci_rev,
>>> +             ver->lmp_ver, ver->lmp_subver);
>>> +     kfree_skb(skb);
>> 
>> Is this HCI_Reset and HCI_Read_Local_Version_Info really needed. The standard init procedure will do exactly that anyway. I have no problem in having this in here. I am just saying that it will be run anyway again after ->setup() completed.
>> 
>> You can check for yourself when running btmon before plugging this device or manually loading btusb.
> 
> The reset is important because it activates the new firmware. And, I
> do like seeing the local version info in kernel log because it makes
> it clear what FW version was before and is after the upgrade.

I get that. What I am saying is that after you go through ->setup() the kernel will call these commands anyway.

So what you are getting now is this:

	setup()
		HCI_Reset
		HCI_Read_Local_Version_Info
		.. load HCD
		HCI_Reset
		HCI_Read_Local_Version_Info
	init()
		HCI_Reset
		HCI_Read_Local_Features
		HCI_Read_Local_Version_Info
		HCI_Read_BD_Address

The init is always executed right after the setup. Unload btusb, start btmon and reload btusb. You will see.

>>> +
>>> +done:
>>> +     if (fw)
>>> +             release_firmware(fw);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> static int btusb_probe(struct usb_interface *intf,
>>>                              const struct usb_device_id *id)
>>> {
>>> @@ -1485,6 +1621,9 @@ static int btusb_probe(struct usb_interface *intf,
>>>      if (id->driver_info & BTUSB_BCM92035)
>>>              hdev->setup = btusb_setup_bcm92035;
>>> 
>>> +     if (id->driver_info & BTUSB_BCM20702_PATCHRAM)
>>> +             hdev->setup = btusb_setup_bcm20702;
>>> +
>> 
>> If we are somehow able to just do BTUSB_BROADCOM and always trigger the patchram loading based on the hardware found that would be super great.
> 
> It would be nice, but I don't have enough resources to test it. I'd
> say let's start with this and extend from there.

We can not easily go back and revert this change though. Can you include /sys/kernel/debug/usb/devices part for this device.

Btw. we have seen devices that pretend to be Broadcom dongles or CSR dongles, but they are not. They also have a split personality where USB descriptors say they are one thing and HCI_Read_Local_Version tells us they are another. They are essentially cheap knockoffs from China.

Regards

Marcel

Re: [PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

[Petri Gynther]

Hi Marcel,

On Wed, Apr 9, 2014 at 10:01 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Petri,
>
>>>> After hardware reset, BCM20702 obtains its initial firmware from a PRO=
M chip.
>>>> Once this initial firmware is running, the firmware can be further upg=
raded
>>>> over HCI interface with .hcd files provided by Broadcom. This is also =
known
>>>> as "patch RAM" support. This change implements that.
>>>>
>>>> If the .hcd file is not found in /lib/firmware, BCM20702 continues to =
operate
>>>> with the initial firmware. Sample kernel log:
>>>> hotplug: sys=3Dfirmware act=3Dadd fw=3Dbrcm/bcm20702-0a5c-22be.hcd dev=
=3D...
>>>> Bluetooth: hci0: BCM20702: patch brcm/bcm20702-0a5c-22be.hcd not found
>>>> Bluetooth: hci0: BCM20702: firmware hci_ver=3D06 hci_rev=3D1000 lmp_ve=
r=3D06 lmp_subver=3D220e
>>>>
>>>> If the .hcd file is found, btusb driver pushes it to BCM20702, and BCM=
20702
>>>> starts using the new firmware. Sample kernel log:
>>>> hotplug: sys=3Dfirmware act=3Dadd fw=3Dbrcm/bcm20702-0a5c-22be.hcd dev=
=3D...
>>>> Bluetooth: hci0: BCM20702: patching hci_ver=3D06 hci_rev=3D1000 lmp_ve=
r=3D06 lmp_subver=3D220e
>>>> Bluetooth: hci0: BCM20702: firmware hci_ver=3D06 hci_rev=3D1389 lmp_ve=
r=3D06 lmp_subver=3D220e
>>>>
>>>> Above, we can see that hci_rev goes from 1000 to 1389 as a result of t=
he upgrade.
>>>
>>> this is a nice feature to have. Where are these hcd files are coming fr=
om. Are they available somewhere or do we have them available in linux-firm=
ware.git tree.
>>
>> In our case, we get these files directly from the vendor. I haven't
>> seen these in linux-firmware.git tree. Some searching reveals that
>> Windows drivers contain either .hex or .hcd files, and there appears
>> to be a tool hex2hcd to convert to .hcd. See page
>> http://swiesmann.de/?p=3D30.
>
> what is the firmware name that the files from the vendor have. My problem=
 with introducing a VID+PID based naming is that it might causes problems w=
hen the VID+PID is re-used for a different SKU or revision of the same boar=
d and we should have picked the firmware based on the board version.

The firmware files seem to have a naming pattern
BCM20702A0_aaa.bbb.ccc.dddd.eeee. I don't know where
aaa.bbb.ccc.dddd.eeee are derived from. Some version number, I guess.

>
> And the HEX vs HCD is most likely the same as Intel=E2=80=99s seq vs bseq=
. Just one is a text file and the other a binary version of the same data. =
In all cases these are HCI commands anyway.
>
>> Interestingly, now that I look at that page more closely, Ubuntu 12.04
>> LTS with kernel 3.8 supports this already? Has Ubuntu made a patch
>> that bluetooth-next doesn't have?
>
> That is Ubuntu for you. They just push patches into their kernel and neve=
r give them back to upstream or make sure they get merged into bluetooth-ne=
xt. I really dislike that behavior. It is always the easy way out instead o=
f trying to do the right thing.
>

See this page: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1065400

>>> I also wonder if there is a bit more general naming convention for the =
files. For example with a quick search I found references to BCM2045B2_002.=
002.011.0348.0349.hcd and similar. They indicate that the naming is not bas=
ed on the USB vendor and product ID.
>>
>> Some version numbering scheme is applied by Broadcom. However, at
>> least initially, I'd prefer using USB vid/pid in the .hcd filename. I
>> know that the .hcd file that I have works on our specific vid/pid of
>> BCM20702, but I have no idea whether it would work on other BCM20702
>> variants.
>
> I am worried to back us into a corner. Reuse of PIDs is plenty. Especiall=
y with the cheaper dongle manufactures.
>
>>> For example for the Intel ROM patching, we used internal versioning to =
pick the right file. That way we were able to run it for all of our devices=
. For the Broadcom devices, it might make sense to run the patchram routine=
 for all devices not just this one specific one.
>>
>> It would make sense, but I have no way of testing it. I only feel
>> comfortable introducing this to the single device that I've been able
>> to test with. It can later be extended to other BCM variants as people
>> test this.
>
> I have a bunch of Broadcom USB dongles around here. I might not get to it=
 this week, but worth while trying to see how this works out.
>
> At least the product string could be taken from the USB descriptors. Seem=
s that Broadcom devices are pretty good in that regard no matter what. So q=
uickly checking with mine, they give you BCM20702A0 and from the looks of i=
t, the firmware files are prefix with exactly that.

Yes, using the product string as the FW filename prefix sounds good.
But, we would need to hardcode the rest, which appears to be some kind
of version number.

>
>>>> Signed-off-by: Petri Gynther <pgynther@google.com>
>>>> ---
>>>> drivers/bluetooth/btusb.c | 139 ++++++++++++++++++++++++++++++++++++++=
++++++++
>>>> 1 file changed, 139 insertions(+)
>>>>
>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>> index f338b0c..371d7e9 100644
>>>> --- a/drivers/bluetooth/btusb.c
>>>> +++ b/drivers/bluetooth/btusb.c
>>>> @@ -49,6 +49,7 @@ static struct usb_driver btusb_driver;
>>>> #define BTUSB_WRONG_SCO_MTU   0x40
>>>> #define BTUSB_ATH3012         0x80
>>>> #define BTUSB_INTEL           0x100
>>>> +#define BTUSB_BCM20702_PATCHRAM      0x200
>>>>
>>>> static const struct usb_device_id btusb_table[] =3D {
>>>>      /* Generic Bluetooth USB device */
>>>> @@ -103,6 +104,7 @@ static const struct usb_device_id btusb_table[] =
=3D {
>>>>      /* Broadcom BCM20702A0 */
>>>>      { USB_DEVICE(0x0489, 0xe042) },
>>>>      { USB_DEVICE(0x04ca, 0x2003) },
>>>> +     { USB_DEVICE(0x0a5c, 0x22be), .driver_info =3D BTUSB_BCM20702_PA=
TCHRAM },
>>>>      { USB_DEVICE(0x0b05, 0x17b5) },
>>>>      { USB_DEVICE(0x0b05, 0x17cb) },
>>>>      { USB_DEVICE(0x413c, 0x8197) },
>>>> @@ -1380,6 +1382,140 @@ exit_mfg_deactivate:
>>>>      return 0;
>>>> }
>>>>
>>>> +static int btusb_setup_bcm20702(struct hci_dev *hdev)
>>>> +{
>>>> +     struct btusb_data *data =3D hci_get_drvdata(hdev);
>>>> +     struct usb_device *udev =3D data->udev;
>>>> +     char fw_name[64];
>>>> +     const struct firmware *fw;
>>>> +     const u8 *fw_ptr;
>>>> +     size_t fw_size;
>>>> +     const struct hci_command_hdr *cmd;
>>>> +     const u8 *cmd_param;
>>>> +     u16 opcode;
>>>> +     struct sk_buff *skb;
>>>> +     struct hci_rp_read_local_version *ver;
>>>> +     long ret;
>>>> +
>>>> +     snprintf(fw_name, sizeof(fw_name), "brcm/bcm20702-%04x-%04x.hcd"=
,
>>>> +              le16_to_cpu(udev->descriptor.idVendor),
>>>> +              le16_to_cpu(udev->descriptor.idProduct));
>>>> +
>>>> +     ret =3D request_firmware(&fw, fw_name, &hdev->dev);
>>>> +     if (ret < 0) {
>>>> +             BT_INFO("%s: BCM20702: patch %s not found", hdev->name,
>>>> +                     fw_name);
>>>> +             ret =3D 0;
>>>> +             goto get_fw_ver;
>>>> +     }
>>>> +
>>>> +     /* Reset */
>>>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIM=
EOUT);
>>>> +     if (IS_ERR(skb)) {
>>>> +             ret =3D PTR_ERR(skb);
>>>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret)=
;
>>>> +             goto reset_fw;
>>>> +     }
>>>> +     kfree_skb(skb);
>>>> +
>>>> +     /* Read Local Version Info */
>>>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
>>>> +                          HCI_INIT_TIMEOUT);
>>>> +     if (IS_ERR(skb)) {
>>>> +             ret =3D PTR_ERR(skb);
>>>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>>>> +                     hdev->name, ret);
>>>> +             goto reset_fw;
>>>> +     }
>>>> +
>>>
>>> You need a length check here to ensure that resulting buffer is the cor=
rect length. Trusting the hardware is not a good idea. I have seen this go =
wrong in some cases.
>>
>> Will do. I'll send revised diff for this.
>>
>>>
>>>> +     ver =3D (struct hci_rp_read_local_version *) skb->data;
>>>> +     BT_INFO("%s: BCM20702: patching hci_ver=3D%02x hci_rev=3D%04x lm=
p_ver=3D%02x "
>>>> +             "lmp_subver=3D%04x", hdev->name, ver->hci_ver, ver->hci_=
rev,
>>>> +             ver->lmp_ver, ver->lmp_subver);
>>>> +     kfree_skb(skb);
>>>> +
>>>> +     /* Start Download */
>>>> +     skb =3D __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT);
>>>> +     if (IS_ERR(skb)) {
>>>> +             ret =3D PTR_ERR(skb);
>>>> +             BT_ERR("%s: BCM20702: Download Minidrv command failed (%=
ld)",
>>>> +                     hdev->name, ret);
>>>> +             goto reset_fw;
>>>> +     }
>>>> +     kfree_skb(skb);
>>>> +
>>>> +     /* 50 msec delay after Download Minidrv completes */
>>>> +     msleep(50);
>>>> +
>>>> +     fw_ptr =3D fw->data;
>>>> +     fw_size =3D fw->size;
>>>> +
>>>> +     while (fw_size >=3D sizeof(*cmd)) {
>>>> +             cmd =3D (struct hci_command_hdr *) fw_ptr;
>>>> +             fw_ptr +=3D sizeof(*cmd);
>>>> +             fw_size -=3D sizeof(*cmd);
>>>> +
>>>> +             if (fw_size < cmd->plen) {
>>>> +                     BT_ERR("%s: BCM20702: patch %s is corrupted",
>>>> +                             hdev->name, fw_name);
>>>> +                     ret =3D -EINVAL;
>>>> +                     goto reset_fw;
>>>> +             }
>>>> +
>>>> +             cmd_param =3D fw_ptr;
>>>> +             fw_ptr +=3D cmd->plen;
>>>> +             fw_size -=3D cmd->plen;
>>>> +
>>>> +             opcode =3D le16_to_cpu(cmd->opcode);
>>>> +
>>>> +             skb =3D __hci_cmd_sync(hdev, opcode, cmd->plen, cmd_para=
m,
>>>> +                                  HCI_INIT_TIMEOUT);
>>>> +             if (IS_ERR(skb)) {
>>>> +                     ret =3D PTR_ERR(skb);
>>>> +                     BT_ERR("%s: BCM20702: patch command %04x failed =
(%ld)",
>>>> +                             hdev->name, opcode, ret);
>>>> +                     goto reset_fw;
>>>> +             }
>>>> +             kfree_skb(skb);
>>>> +     }
>>>> +
>>>> +     /* 250 msec delay after Launch Ram completes */
>>>> +     msleep(250);
>>>> +
>>>> +reset_fw:
>>>> +     /* Reset */
>>>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIM=
EOUT);
>>>> +     if (IS_ERR(skb)) {
>>>> +             ret =3D PTR_ERR(skb);
>>>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret)=
;
>>>> +             goto done;
>>>> +     }
>>>> +     kfree_skb(skb);
>>>> +
>>>> +get_fw_ver:
>>>> +     /* Read Local Version Info */
>>>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
>>>> +                          HCI_INIT_TIMEOUT);
>>>> +     if (IS_ERR(skb)) {
>>>> +             ret =3D PTR_ERR(skb);
>>>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>>>> +                     hdev->name, ret);
>>>> +             goto done;
>>>> +     }
>>>> +
>>>> +     ver =3D (struct hci_rp_read_local_version *) skb->data;
>>>> +     BT_INFO("%s: BCM20702: firmware hci_ver=3D%02x hci_rev=3D%04x lm=
p_ver=3D%02x "
>>>> +             "lmp_subver=3D%04x", hdev->name, ver->hci_ver, ver->hci_=
rev,
>>>> +             ver->lmp_ver, ver->lmp_subver);
>>>> +     kfree_skb(skb);
>>>
>>> Is this HCI_Reset and HCI_Read_Local_Version_Info really needed. The st=
andard init procedure will do exactly that anyway. I have no problem in hav=
ing this in here. I am just saying that it will be run anyway again after -=
>setup() completed.
>>>
>>> You can check for yourself when running btmon before plugging this devi=
ce or manually loading btusb.
>>
>> The reset is important because it activates the new firmware. And, I
>> do like seeing the local version info in kernel log because it makes
>> it clear what FW version was before and is after the upgrade.
>
> I get that. What I am saying is that after you go through ->setup() the k=
ernel will call these commands anyway.
>
> So what you are getting now is this:
>
>         setup()
>                 HCI_Reset
>                 HCI_Read_Local_Version_Info
>                 .. load HCD
>                 HCI_Reset
>                 HCI_Read_Local_Version_Info
>         init()
>                 HCI_Reset
>                 HCI_Read_Local_Features
>                 HCI_Read_Local_Version_Info
>                 HCI_Read_BD_Address
>
> The init is always executed right after the setup. Unload btusb, start bt=
mon and reload btusb. You will see.

I understand. I looked at the code in hci_dev_do_open() and
__hci_init(). I'm OK to remove the first HCI_Read_Local_Version_Info
before loading HCD. But, I'd like to keep the rest, so setup() becomes
reset + load HCD + reset + read local version info. I'd like to see in
kernel log what the FW version is after the HCD push.

>
>>>> +
>>>> +done:
>>>> +     if (fw)
>>>> +             release_firmware(fw);
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>> static int btusb_probe(struct usb_interface *intf,
>>>>                              const struct usb_device_id *id)
>>>> {
>>>> @@ -1485,6 +1621,9 @@ static int btusb_probe(struct usb_interface *int=
f,
>>>>      if (id->driver_info & BTUSB_BCM92035)
>>>>              hdev->setup =3D btusb_setup_bcm92035;
>>>>
>>>> +     if (id->driver_info & BTUSB_BCM20702_PATCHRAM)
>>>> +             hdev->setup =3D btusb_setup_bcm20702;
>>>> +
>>>
>>> If we are somehow able to just do BTUSB_BROADCOM and always trigger the=
 patchram loading based on the hardware found that would be super great.
>>
>> It would be nice, but I don't have enough resources to test it. I'd
>> say let's start with this and extend from there.
>
> We can not easily go back and revert this change though. Can you include =
/sys/kernel/debug/usb/devices part for this device.

T:  Bus=3D05 Lev=3D01 Prnt=3D01 Port=3D00 Cnt=3D01 Dev#=3D  2 Spd=3D12   Mx=
Ch=3D 0
D:  Ver=3D 2.00 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 MxPS=3D64 #Cfgs=3D  1
P:  Vendor=3D0a5c ProdID=3D22be Rev=3D 1.12
S:  Manufacturer=3DBroadcom Corp
S:  Product=3DBCM20702A0
S:  SerialNumber=3D000000000000
C:* #Ifs=3D 4 Cfg#=3D 1 Atr=3De0 MxPwr=3D  0mA
I:* If#=3D 0 Alt=3D 0 #EPs=3D 3 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Driver=
=3Dbtusb
E:  Ad=3D81(I) Atr=3D03(Int.) MxPS=3D  16 Ivl=3D1ms
E:  Ad=3D82(I) Atr=3D02(Bulk) MxPS=3D  64 Ivl=3D0ms
E:  Ad=3D02(O) Atr=3D02(Bulk) MxPS=3D  64 Ivl=3D0ms
I:* If#=3D 1 Alt=3D 0 #EPs=3D 2 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Driver=
=3Dbtusb
E:  Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D   0 Ivl=3D1ms
E:  Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D   0 Ivl=3D1ms
I:  If#=3D 1 Alt=3D 1 #EPs=3D 2 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Driver=
=3Dbtusb
E:  Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D   9 Ivl=3D1ms
E:  Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D   9 Ivl=3D1ms
I:  If#=3D 1 Alt=3D 2 #EPs=3D 2 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Driver=
=3Dbtusb
E:  Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D  17 Ivl=3D1ms
E:  Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D  17 Ivl=3D1ms
I:  If#=3D 1 Alt=3D 3 #EPs=3D 2 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Driver=
=3Dbtusb
E:  Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D  25 Ivl=3D1ms
E:  Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D  25 Ivl=3D1ms
I:  If#=3D 1 Alt=3D 4 #EPs=3D 2 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Driver=
=3Dbtusb
E:  Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D  33 Ivl=3D1ms
E:  Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D  33 Ivl=3D1ms
I:  If#=3D 1 Alt=3D 5 #EPs=3D 2 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Driver=
=3Dbtusb
E:  Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D  49 Ivl=3D1ms
E:  Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D  49 Ivl=3D1ms
I:* If#=3D 2 Alt=3D 0 #EPs=3D 2 Cls=3Dff(vend.) Sub=3Dff Prot=3Dff Driver=
=3D(none)
E:  Ad=3D84(I) Atr=3D02(Bulk) MxPS=3D  32 Ivl=3D0ms
E:  Ad=3D04(O) Atr=3D02(Bulk) MxPS=3D  32 Ivl=3D0ms
I:* If#=3D 3 Alt=3D 0 #EPs=3D 0 Cls=3Dfe(app. ) Sub=3D01 Prot=3D01 Driver=
=3D(none)


>
> Btw. we have seen devices that pretend to be Broadcom dongles or CSR dong=
les, but they are not. They also have a split personality where USB descrip=
tors say they are one thing and HCI_Read_Local_Version tells us they are an=
other. They are essentially cheap knockoffs from China.
>
> Regards
>
> Marcel
>

Re: [PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

[Marcel Holtmann]

Hi Petri,

>>>>> After hardware reset, BCM20702 obtains its initial firmware from a PROM chip.
>>>>> Once this initial firmware is running, the firmware can be further upgraded
>>>>> over HCI interface with .hcd files provided by Broadcom. This is also known
>>>>> as "patch RAM" support. This change implements that.
>>>>> 
>>>>> If the .hcd file is not found in /lib/firmware, BCM20702 continues to operate
>>>>> with the initial firmware. Sample kernel log:
>>>>> hotplug: sys=firmware act=add fw=brcm/bcm20702-0a5c-22be.hcd dev=...
>>>>> Bluetooth: hci0: BCM20702: patch brcm/bcm20702-0a5c-22be.hcd not found
>>>>> Bluetooth: hci0: BCM20702: firmware hci_ver=06 hci_rev=1000 lmp_ver=06 lmp_subver=220e
>>>>> 
>>>>> If the .hcd file is found, btusb driver pushes it to BCM20702, and BCM20702
>>>>> starts using the new firmware. Sample kernel log:
>>>>> hotplug: sys=firmware act=add fw=brcm/bcm20702-0a5c-22be.hcd dev=...
>>>>> Bluetooth: hci0: BCM20702: patching hci_ver=06 hci_rev=1000 lmp_ver=06 lmp_subver=220e
>>>>> Bluetooth: hci0: BCM20702: firmware hci_ver=06 hci_rev=1389 lmp_ver=06 lmp_subver=220e
>>>>> 
>>>>> Above, we can see that hci_rev goes from 1000 to 1389 as a result of the upgrade.
>>>> 
>>>> this is a nice feature to have. Where are these hcd files are coming from. Are they available somewhere or do we have them available in linux-firmware.git tree.
>>> 
>>> In our case, we get these files directly from the vendor. I haven't
>>> seen these in linux-firmware.git tree. Some searching reveals that
>>> Windows drivers contain either .hex or .hcd files, and there appears
>>> to be a tool hex2hcd to convert to .hcd. See page
>>> http://swiesmann.de/?p=30.
>> 
>> what is the firmware name that the files from the vendor have. My problem with introducing a VID+PID based naming is that it might causes problems when the VID+PID is re-used for a different SKU or revision of the same board and we should have picked the firmware based on the board version.
> 
> The firmware files seem to have a naming pattern
> BCM20702A0_aaa.bbb.ccc.dddd.eeee. I don't know where
> aaa.bbb.ccc.dddd.eeee are derived from. Some version number, I guess.

I am trying to figure out what they mean. Any chance you can ask your vendor for help.

>> And the HEX vs HCD is most likely the same as Intel’s seq vs bseq. Just one is a text file and the other a binary version of the same data. In all cases these are HCI commands anyway.
>> 
>>> Interestingly, now that I look at that page more closely, Ubuntu 12.04
>>> LTS with kernel 3.8 supports this already? Has Ubuntu made a patch
>>> that bluetooth-next doesn't have?
>> 
>> That is Ubuntu for you. They just push patches into their kernel and never give them back to upstream or make sure they get merged into bluetooth-next. I really dislike that behavior. It is always the easy way out instead of trying to do the right thing.
>> 
> 
> See this page: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1065400

It is a stupid userspace version that is racy as hell. Even the bug mentions that it should be done in the kernel. And I am pretty sure that was my initial comment as well. Just posting the userspace patch for reference.

>>>> I also wonder if there is a bit more general naming convention for the files. For example with a quick search I found references to BCM2045B2_002.002.011.0348.0349.hcd and similar. They indicate that the naming is not based on the USB vendor and product ID.
>>> 
>>> Some version numbering scheme is applied by Broadcom. However, at
>>> least initially, I'd prefer using USB vid/pid in the .hcd filename. I
>>> know that the .hcd file that I have works on our specific vid/pid of
>>> BCM20702, but I have no idea whether it would work on other BCM20702
>>> variants.
>> 
>> I am worried to back us into a corner. Reuse of PIDs is plenty. Especially with the cheaper dongle manufactures.
>> 
>>>> For example for the Intel ROM patching, we used internal versioning to pick the right file. That way we were able to run it for all of our devices. For the Broadcom devices, it might make sense to run the patchram routine for all devices not just this one specific one.
>>> 
>>> It would make sense, but I have no way of testing it. I only feel
>>> comfortable introducing this to the single device that I've been able
>>> to test with. It can later be extended to other BCM variants as people
>>> test this.
>> 
>> I have a bunch of Broadcom USB dongles around here. I might not get to it this week, but worth while trying to see how this works out.
>> 
>> At least the product string could be taken from the USB descriptors. Seems that Broadcom devices are pretty good in that regard no matter what. So quickly checking with mine, they give you BCM20702A0 and from the looks of it, the firmware files are prefix with exactly that.
> 
> Yes, using the product string as the FW filename prefix sounds good.
> But, we would need to hardcode the rest, which appears to be some kind
> of version number.

With the Intel firmware loading we fall back to a generic file in case the proper ROM patch file is not found. Maybe we should do something similar here as well. The more we can follow standard Broadcom procedure for these patch files, the better of course. It makes it a lot easier for any new devices.

>>>>> Signed-off-by: Petri Gynther <pgynther@google.com>
>>>>> ---
>>>>> drivers/bluetooth/btusb.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 139 insertions(+)
>>>>> 
>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>> index f338b0c..371d7e9 100644
>>>>> --- a/drivers/bluetooth/btusb.c
>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>> @@ -49,6 +49,7 @@ static struct usb_driver btusb_driver;
>>>>> #define BTUSB_WRONG_SCO_MTU   0x40
>>>>> #define BTUSB_ATH3012         0x80
>>>>> #define BTUSB_INTEL           0x100
>>>>> +#define BTUSB_BCM20702_PATCHRAM      0x200
>>>>> 
>>>>> static const struct usb_device_id btusb_table[] = {
>>>>>     /* Generic Bluetooth USB device */
>>>>> @@ -103,6 +104,7 @@ static const struct usb_device_id btusb_table[] = {
>>>>>     /* Broadcom BCM20702A0 */
>>>>>     { USB_DEVICE(0x0489, 0xe042) },
>>>>>     { USB_DEVICE(0x04ca, 0x2003) },
>>>>> +     { USB_DEVICE(0x0a5c, 0x22be), .driver_info = BTUSB_BCM20702_PATCHRAM },
>>>>>     { USB_DEVICE(0x0b05, 0x17b5) },
>>>>>     { USB_DEVICE(0x0b05, 0x17cb) },
>>>>>     { USB_DEVICE(0x413c, 0x8197) },
>>>>> @@ -1380,6 +1382,140 @@ exit_mfg_deactivate:
>>>>>     return 0;
>>>>> }
>>>>> 
>>>>> +static int btusb_setup_bcm20702(struct hci_dev *hdev)
>>>>> +{
>>>>> +     struct btusb_data *data = hci_get_drvdata(hdev);
>>>>> +     struct usb_device *udev = data->udev;
>>>>> +     char fw_name[64];
>>>>> +     const struct firmware *fw;
>>>>> +     const u8 *fw_ptr;
>>>>> +     size_t fw_size;
>>>>> +     const struct hci_command_hdr *cmd;
>>>>> +     const u8 *cmd_param;
>>>>> +     u16 opcode;
>>>>> +     struct sk_buff *skb;
>>>>> +     struct hci_rp_read_local_version *ver;
>>>>> +     long ret;
>>>>> +
>>>>> +     snprintf(fw_name, sizeof(fw_name), "brcm/bcm20702-%04x-%04x.hcd",
>>>>> +              le16_to_cpu(udev->descriptor.idVendor),
>>>>> +              le16_to_cpu(udev->descriptor.idProduct));
>>>>> +
>>>>> +     ret = request_firmware(&fw, fw_name, &hdev->dev);
>>>>> +     if (ret < 0) {
>>>>> +             BT_INFO("%s: BCM20702: patch %s not found", hdev->name,
>>>>> +                     fw_name);
>>>>> +             ret = 0;
>>>>> +             goto get_fw_ver;
>>>>> +     }
>>>>> +
>>>>> +     /* Reset */
>>>>> +     skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>>>>> +     if (IS_ERR(skb)) {
>>>>> +             ret = PTR_ERR(skb);
>>>>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret);
>>>>> +             goto reset_fw;
>>>>> +     }
>>>>> +     kfree_skb(skb);
>>>>> +
>>>>> +     /* Read Local Version Info */
>>>>> +     skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
>>>>> +                          HCI_INIT_TIMEOUT);
>>>>> +     if (IS_ERR(skb)) {
>>>>> +             ret = PTR_ERR(skb);
>>>>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>>>>> +                     hdev->name, ret);
>>>>> +             goto reset_fw;
>>>>> +     }
>>>>> +
>>>> 
>>>> You need a length check here to ensure that resulting buffer is the correct length. Trusting the hardware is not a good idea. I have seen this go wrong in some cases.
>>> 
>>> Will do. I'll send revised diff for this.
>>> 
>>>> 
>>>>> +     ver = (struct hci_rp_read_local_version *) skb->data;
>>>>> +     BT_INFO("%s: BCM20702: patching hci_ver=%02x hci_rev=%04x lmp_ver=%02x "
>>>>> +             "lmp_subver=%04x", hdev->name, ver->hci_ver, ver->hci_rev,
>>>>> +             ver->lmp_ver, ver->lmp_subver);
>>>>> +     kfree_skb(skb);
>>>>> +
>>>>> +     /* Start Download */
>>>>> +     skb = __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT);
>>>>> +     if (IS_ERR(skb)) {
>>>>> +             ret = PTR_ERR(skb);
>>>>> +             BT_ERR("%s: BCM20702: Download Minidrv command failed (%ld)",
>>>>> +                     hdev->name, ret);
>>>>> +             goto reset_fw;
>>>>> +     }
>>>>> +     kfree_skb(skb);
>>>>> +
>>>>> +     /* 50 msec delay after Download Minidrv completes */
>>>>> +     msleep(50);
>>>>> +
>>>>> +     fw_ptr = fw->data;
>>>>> +     fw_size = fw->size;
>>>>> +
>>>>> +     while (fw_size >= sizeof(*cmd)) {
>>>>> +             cmd = (struct hci_command_hdr *) fw_ptr;
>>>>> +             fw_ptr += sizeof(*cmd);
>>>>> +             fw_size -= sizeof(*cmd);
>>>>> +
>>>>> +             if (fw_size < cmd->plen) {
>>>>> +                     BT_ERR("%s: BCM20702: patch %s is corrupted",
>>>>> +                             hdev->name, fw_name);
>>>>> +                     ret = -EINVAL;
>>>>> +                     goto reset_fw;
>>>>> +             }
>>>>> +
>>>>> +             cmd_param = fw_ptr;
>>>>> +             fw_ptr += cmd->plen;
>>>>> +             fw_size -= cmd->plen;
>>>>> +
>>>>> +             opcode = le16_to_cpu(cmd->opcode);
>>>>> +
>>>>> +             skb = __hci_cmd_sync(hdev, opcode, cmd->plen, cmd_param,
>>>>> +                                  HCI_INIT_TIMEOUT);
>>>>> +             if (IS_ERR(skb)) {
>>>>> +                     ret = PTR_ERR(skb);
>>>>> +                     BT_ERR("%s: BCM20702: patch command %04x failed (%ld)",
>>>>> +                             hdev->name, opcode, ret);
>>>>> +                     goto reset_fw;
>>>>> +             }
>>>>> +             kfree_skb(skb);
>>>>> +     }
>>>>> +
>>>>> +     /* 250 msec delay after Launch Ram completes */
>>>>> +     msleep(250);
>>>>> +
>>>>> +reset_fw:
>>>>> +     /* Reset */
>>>>> +     skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>>>>> +     if (IS_ERR(skb)) {
>>>>> +             ret = PTR_ERR(skb);
>>>>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret);
>>>>> +             goto done;
>>>>> +     }
>>>>> +     kfree_skb(skb);
>>>>> +
>>>>> +get_fw_ver:
>>>>> +     /* Read Local Version Info */
>>>>> +     skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
>>>>> +                          HCI_INIT_TIMEOUT);
>>>>> +     if (IS_ERR(skb)) {
>>>>> +             ret = PTR_ERR(skb);
>>>>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>>>>> +                     hdev->name, ret);
>>>>> +             goto done;
>>>>> +     }
>>>>> +
>>>>> +     ver = (struct hci_rp_read_local_version *) skb->data;
>>>>> +     BT_INFO("%s: BCM20702: firmware hci_ver=%02x hci_rev=%04x lmp_ver=%02x "
>>>>> +             "lmp_subver=%04x", hdev->name, ver->hci_ver, ver->hci_rev,
>>>>> +             ver->lmp_ver, ver->lmp_subver);
>>>>> +     kfree_skb(skb);
>>>> 
>>>> Is this HCI_Reset and HCI_Read_Local_Version_Info really needed. The standard init procedure will do exactly that anyway. I have no problem in having this in here. I am just saying that it will be run anyway again after ->setup() completed.
>>>> 
>>>> You can check for yourself when running btmon before plugging this device or manually loading btusb.
>>> 
>>> The reset is important because it activates the new firmware. And, I
>>> do like seeing the local version info in kernel log because it makes
>>> it clear what FW version was before and is after the upgrade.
>> 
>> I get that. What I am saying is that after you go through ->setup() the kernel will call these commands anyway.
>> 
>> So what you are getting now is this:
>> 
>>        setup()
>>                HCI_Reset
>>                HCI_Read_Local_Version_Info
>>                .. load HCD
>>                HCI_Reset
>>                HCI_Read_Local_Version_Info
>>        init()
>>                HCI_Reset
>>                HCI_Read_Local_Features
>>                HCI_Read_Local_Version_Info
>>                HCI_Read_BD_Address
>> 
>> The init is always executed right after the setup. Unload btusb, start btmon and reload btusb. You will see.
> 
> I understand. I looked at the code in hci_dev_do_open() and
> __hci_init(). I'm OK to remove the first HCI_Read_Local_Version_Info
> before loading HCD. But, I'd like to keep the rest, so setup() becomes
> reset + load HCD + reset + read local version info. I'd like to see in
> kernel log what the FW version is after the HCD push.

We can keep it as well. I do not care much either way since ->setup() is only run once. I was more curious if there is some specific reason for it.

>>>>> +
>>>>> +done:
>>>>> +     if (fw)
>>>>> +             release_firmware(fw);
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> static int btusb_probe(struct usb_interface *intf,
>>>>>                             const struct usb_device_id *id)
>>>>> {
>>>>> @@ -1485,6 +1621,9 @@ static int btusb_probe(struct usb_interface *intf,
>>>>>     if (id->driver_info & BTUSB_BCM92035)
>>>>>             hdev->setup = btusb_setup_bcm92035;
>>>>> 
>>>>> +     if (id->driver_info & BTUSB_BCM20702_PATCHRAM)
>>>>> +             hdev->setup = btusb_setup_bcm20702;
>>>>> +
>>>> 
>>>> If we are somehow able to just do BTUSB_BROADCOM and always trigger the patchram loading based on the hardware found that would be super great.
>>> 
>>> It would be nice, but I don't have enough resources to test it. I'd
>>> say let's start with this and extend from there.
>> 
>> We can not easily go back and revert this change though. Can you include /sys/kernel/debug/usb/devices part for this device.
> 
> T:  Bus=05 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  2 Spd=12   MxCh= 0
> D:  Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
> P:  Vendor=0a5c ProdID=22be Rev= 1.12
> S:  Manufacturer=Broadcom Corp
> S:  Product=BCM20702A0
> S:  SerialNumber=000000000000

Did you block out the serial number on purpose? Normally that is your BD_ADDR. Or do you happen to have a different kind of module and the firmware loading will actually set the address. If so, then careful that the firmware not program the same BD_ADDR for all devices.

> C:* #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr=  0mA
> I:* If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E:  Ad=81(I) Atr=03(Int.) MxPS=  16 Ivl=1ms
> E:  Ad=82(I) Atr=02(Bulk) MxPS=  64 Ivl=0ms
> E:  Ad=02(O) Atr=02(Bulk) MxPS=  64 Ivl=0ms
> I:* If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E:  Ad=83(I) Atr=01(Isoc) MxPS=   0 Ivl=1ms
> E:  Ad=03(O) Atr=01(Isoc) MxPS=   0 Ivl=1ms
> I:  If#= 1 Alt= 1 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E:  Ad=83(I) Atr=01(Isoc) MxPS=   9 Ivl=1ms
> E:  Ad=03(O) Atr=01(Isoc) MxPS=   9 Ivl=1ms
> I:  If#= 1 Alt= 2 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E:  Ad=83(I) Atr=01(Isoc) MxPS=  17 Ivl=1ms
> E:  Ad=03(O) Atr=01(Isoc) MxPS=  17 Ivl=1ms
> I:  If#= 1 Alt= 3 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E:  Ad=83(I) Atr=01(Isoc) MxPS=  25 Ivl=1ms
> E:  Ad=03(O) Atr=01(Isoc) MxPS=  25 Ivl=1ms
> I:  If#= 1 Alt= 4 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E:  Ad=83(I) Atr=01(Isoc) MxPS=  33 Ivl=1ms
> E:  Ad=03(O) Atr=01(Isoc) MxPS=  33 Ivl=1ms
> I:  If#= 1 Alt= 5 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E:  Ad=83(I) Atr=01(Isoc) MxPS=  49 Ivl=1ms
> E:  Ad=03(O) Atr=01(Isoc) MxPS=  49 Ivl=1ms
> I:* If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
> E:  Ad=84(I) Atr=02(Bulk) MxPS=  32 Ivl=0ms
> E:  Ad=04(O) Atr=02(Bulk) MxPS=  32 Ivl=0ms
> I:* If#= 3 Alt= 0 #EPs= 0 Cls=fe(app. ) Sub=01 Prot=01 Driver=(none)

Regards

Marcel

Re: [PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

[Petri Gynther]

Hi Marcel,

On Wed, Apr 9, 2014 at 1:57 PM, Marcel Holtmann <marcel@holtmann.org> wrote=
:
> Hi Petri,
>
>>>>>> After hardware reset, BCM20702 obtains its initial firmware from a P=
ROM chip.
>>>>>> Once this initial firmware is running, the firmware can be further u=
pgraded
>>>>>> over HCI interface with .hcd files provided by Broadcom. This is als=
o known
>>>>>> as "patch RAM" support. This change implements that.
>>>>>>
>>>>>> If the .hcd file is not found in /lib/firmware, BCM20702 continues t=
o operate
>>>>>> with the initial firmware. Sample kernel log:
>>>>>> hotplug: sys=3Dfirmware act=3Dadd fw=3Dbrcm/bcm20702-0a5c-22be.hcd d=
ev=3D...
>>>>>> Bluetooth: hci0: BCM20702: patch brcm/bcm20702-0a5c-22be.hcd not fou=
nd
>>>>>> Bluetooth: hci0: BCM20702: firmware hci_ver=3D06 hci_rev=3D1000 lmp_=
ver=3D06 lmp_subver=3D220e
>>>>>>
>>>>>> If the .hcd file is found, btusb driver pushes it to BCM20702, and B=
CM20702
>>>>>> starts using the new firmware. Sample kernel log:
>>>>>> hotplug: sys=3Dfirmware act=3Dadd fw=3Dbrcm/bcm20702-0a5c-22be.hcd d=
ev=3D...
>>>>>> Bluetooth: hci0: BCM20702: patching hci_ver=3D06 hci_rev=3D1000 lmp_=
ver=3D06 lmp_subver=3D220e
>>>>>> Bluetooth: hci0: BCM20702: firmware hci_ver=3D06 hci_rev=3D1389 lmp_=
ver=3D06 lmp_subver=3D220e
>>>>>>
>>>>>> Above, we can see that hci_rev goes from 1000 to 1389 as a result of=
 the upgrade.
>>>>>
>>>>> this is a nice feature to have. Where are these hcd files are coming =
from. Are they available somewhere or do we have them available in linux-fi=
rmware.git tree.
>>>>
>>>> In our case, we get these files directly from the vendor. I haven't
>>>> seen these in linux-firmware.git tree. Some searching reveals that
>>>> Windows drivers contain either .hex or .hcd files, and there appears
>>>> to be a tool hex2hcd to convert to .hcd. See page
>>>> http://swiesmann.de/?p=3D30.
>>>
>>> what is the firmware name that the files from the vendor have. My probl=
em with introducing a VID+PID based naming is that it might causes problems=
 when the VID+PID is re-used for a different SKU or revision of the same bo=
ard and we should have picked the firmware based on the board version.
>>
>> The firmware files seem to have a naming pattern
>> BCM20702A0_aaa.bbb.ccc.dddd.eeee. I don't know where
>> aaa.bbb.ccc.dddd.eeee are derived from. Some version number, I guess.
>
> I am trying to figure out what they mean. Any chance you can ask your ven=
dor for help.
>

I've looked a bit more into this. It turns out that there are cases
where BCM A0 chip might actually use BCM A1 firmware file. So, I don't
think we can reliably use the product string (e.g. BCM20702A0) as a
prefix.

So, any problem going with VID-PID approach here? That is easy for the
user, as they can figure out the VID-PID pair with lsusb, and then
place properly named file to /lib/firmware/brcm/

And, if the firmware file is missing, no problem. The device will
operate with the default firmware from OTPROM.

>>> And the HEX vs HCD is most likely the same as Intel=E2=80=99s seq vs bs=
eq. Just one is a text file and the other a binary version of the same data=
. In all cases these are HCI commands anyway.
>>>
>>>> Interestingly, now that I look at that page more closely, Ubuntu 12.04
>>>> LTS with kernel 3.8 supports this already? Has Ubuntu made a patch
>>>> that bluetooth-next doesn't have?
>>>
>>> That is Ubuntu for you. They just push patches into their kernel and ne=
ver give them back to upstream or make sure they get merged into bluetooth-=
next. I really dislike that behavior. It is always the easy way out instead=
 of trying to do the right thing.
>>>
>>
>> See this page: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1065=
400
>
> It is a stupid userspace version that is racy as hell. Even the bug menti=
ons that it should be done in the kernel. And I am pretty sure that was my =
initial comment as well. Just posting the userspace patch for reference.
>
>>>>> I also wonder if there is a bit more general naming convention for th=
e files. For example with a quick search I found references to BCM2045B2_00=
2.002.011.0348.0349.hcd and similar. They indicate that the naming is not b=
ased on the USB vendor and product ID.
>>>>
>>>> Some version numbering scheme is applied by Broadcom. However, at
>>>> least initially, I'd prefer using USB vid/pid in the .hcd filename. I
>>>> know that the .hcd file that I have works on our specific vid/pid of
>>>> BCM20702, but I have no idea whether it would work on other BCM20702
>>>> variants.
>>>
>>> I am worried to back us into a corner. Reuse of PIDs is plenty. Especia=
lly with the cheaper dongle manufactures.
>>>
>>>>> For example for the Intel ROM patching, we used internal versioning t=
o pick the right file. That way we were able to run it for all of our devic=
es. For the Broadcom devices, it might make sense to run the patchram routi=
ne for all devices not just this one specific one.
>>>>
>>>> It would make sense, but I have no way of testing it. I only feel
>>>> comfortable introducing this to the single device that I've been able
>>>> to test with. It can later be extended to other BCM variants as people
>>>> test this.
>>>
>>> I have a bunch of Broadcom USB dongles around here. I might not get to =
it this week, but worth while trying to see how this works out.
>>>
>>> At least the product string could be taken from the USB descriptors. Se=
ems that Broadcom devices are pretty good in that regard no matter what. So=
 quickly checking with mine, they give you BCM20702A0 and from the looks of=
 it, the firmware files are prefix with exactly that.
>>
>> Yes, using the product string as the FW filename prefix sounds good.
>> But, we would need to hardcode the rest, which appears to be some kind
>> of version number.
>
> With the Intel firmware loading we fall back to a generic file in case th=
e proper ROM patch file is not found. Maybe we should do something similar =
here as well. The more we can follow standard Broadcom procedure for these =
patch files, the better of course. It makes it a lot easier for any new dev=
ices.
>

We don't need a fallback as the FW file is optional. The device
operates fine with OTPROM FW.

>>>>>> Signed-off-by: Petri Gynther <pgynther@google.com>
>>>>>> ---
>>>>>> drivers/bluetooth/btusb.c | 139 ++++++++++++++++++++++++++++++++++++=
++++++++++
>>>>>> 1 file changed, 139 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>>> index f338b0c..371d7e9 100644
>>>>>> --- a/drivers/bluetooth/btusb.c
>>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>>> @@ -49,6 +49,7 @@ static struct usb_driver btusb_driver;
>>>>>> #define BTUSB_WRONG_SCO_MTU   0x40
>>>>>> #define BTUSB_ATH3012         0x80
>>>>>> #define BTUSB_INTEL           0x100
>>>>>> +#define BTUSB_BCM20702_PATCHRAM      0x200
>>>>>>
>>>>>> static const struct usb_device_id btusb_table[] =3D {
>>>>>>     /* Generic Bluetooth USB device */
>>>>>> @@ -103,6 +104,7 @@ static const struct usb_device_id btusb_table[] =
=3D {
>>>>>>     /* Broadcom BCM20702A0 */
>>>>>>     { USB_DEVICE(0x0489, 0xe042) },
>>>>>>     { USB_DEVICE(0x04ca, 0x2003) },
>>>>>> +     { USB_DEVICE(0x0a5c, 0x22be), .driver_info =3D BTUSB_BCM20702_=
PATCHRAM },
>>>>>>     { USB_DEVICE(0x0b05, 0x17b5) },
>>>>>>     { USB_DEVICE(0x0b05, 0x17cb) },
>>>>>>     { USB_DEVICE(0x413c, 0x8197) },
>>>>>> @@ -1380,6 +1382,140 @@ exit_mfg_deactivate:
>>>>>>     return 0;
>>>>>> }
>>>>>>
>>>>>> +static int btusb_setup_bcm20702(struct hci_dev *hdev)
>>>>>> +{
>>>>>> +     struct btusb_data *data =3D hci_get_drvdata(hdev);
>>>>>> +     struct usb_device *udev =3D data->udev;
>>>>>> +     char fw_name[64];
>>>>>> +     const struct firmware *fw;
>>>>>> +     const u8 *fw_ptr;
>>>>>> +     size_t fw_size;
>>>>>> +     const struct hci_command_hdr *cmd;
>>>>>> +     const u8 *cmd_param;
>>>>>> +     u16 opcode;
>>>>>> +     struct sk_buff *skb;
>>>>>> +     struct hci_rp_read_local_version *ver;
>>>>>> +     long ret;
>>>>>> +
>>>>>> +     snprintf(fw_name, sizeof(fw_name), "brcm/bcm20702-%04x-%04x.hc=
d",
>>>>>> +              le16_to_cpu(udev->descriptor.idVendor),
>>>>>> +              le16_to_cpu(udev->descriptor.idProduct));
>>>>>> +
>>>>>> +     ret =3D request_firmware(&fw, fw_name, &hdev->dev);
>>>>>> +     if (ret < 0) {
>>>>>> +             BT_INFO("%s: BCM20702: patch %s not found", hdev->name=
,
>>>>>> +                     fw_name);
>>>>>> +             ret =3D 0;
>>>>>> +             goto get_fw_ver;
>>>>>> +     }
>>>>>> +
>>>>>> +     /* Reset */
>>>>>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_T=
IMEOUT);
>>>>>> +     if (IS_ERR(skb)) {
>>>>>> +             ret =3D PTR_ERR(skb);
>>>>>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, re=
t);
>>>>>> +             goto reset_fw;
>>>>>> +     }
>>>>>> +     kfree_skb(skb);
>>>>>> +
>>>>>> +     /* Read Local Version Info */
>>>>>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NUL=
L,
>>>>>> +                          HCI_INIT_TIMEOUT);
>>>>>> +     if (IS_ERR(skb)) {
>>>>>> +             ret =3D PTR_ERR(skb);
>>>>>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>>>>>> +                     hdev->name, ret);
>>>>>> +             goto reset_fw;
>>>>>> +     }
>>>>>> +
>>>>>
>>>>> You need a length check here to ensure that resulting buffer is the c=
orrect length. Trusting the hardware is not a good idea. I have seen this g=
o wrong in some cases.
>>>>
>>>> Will do. I'll send revised diff for this.
>>>>
>>>>>
>>>>>> +     ver =3D (struct hci_rp_read_local_version *) skb->data;
>>>>>> +     BT_INFO("%s: BCM20702: patching hci_ver=3D%02x hci_rev=3D%04x =
lmp_ver=3D%02x "
>>>>>> +             "lmp_subver=3D%04x", hdev->name, ver->hci_ver, ver->hc=
i_rev,
>>>>>> +             ver->lmp_ver, ver->lmp_subver);
>>>>>> +     kfree_skb(skb);
>>>>>> +
>>>>>> +     /* Start Download */
>>>>>> +     skb =3D __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT=
);
>>>>>> +     if (IS_ERR(skb)) {
>>>>>> +             ret =3D PTR_ERR(skb);
>>>>>> +             BT_ERR("%s: BCM20702: Download Minidrv command failed =
(%ld)",
>>>>>> +                     hdev->name, ret);
>>>>>> +             goto reset_fw;
>>>>>> +     }
>>>>>> +     kfree_skb(skb);
>>>>>> +
>>>>>> +     /* 50 msec delay after Download Minidrv completes */
>>>>>> +     msleep(50);
>>>>>> +
>>>>>> +     fw_ptr =3D fw->data;
>>>>>> +     fw_size =3D fw->size;
>>>>>> +
>>>>>> +     while (fw_size >=3D sizeof(*cmd)) {
>>>>>> +             cmd =3D (struct hci_command_hdr *) fw_ptr;
>>>>>> +             fw_ptr +=3D sizeof(*cmd);
>>>>>> +             fw_size -=3D sizeof(*cmd);
>>>>>> +
>>>>>> +             if (fw_size < cmd->plen) {
>>>>>> +                     BT_ERR("%s: BCM20702: patch %s is corrupted",
>>>>>> +                             hdev->name, fw_name);
>>>>>> +                     ret =3D -EINVAL;
>>>>>> +                     goto reset_fw;
>>>>>> +             }
>>>>>> +
>>>>>> +             cmd_param =3D fw_ptr;
>>>>>> +             fw_ptr +=3D cmd->plen;
>>>>>> +             fw_size -=3D cmd->plen;
>>>>>> +
>>>>>> +             opcode =3D le16_to_cpu(cmd->opcode);
>>>>>> +
>>>>>> +             skb =3D __hci_cmd_sync(hdev, opcode, cmd->plen, cmd_pa=
ram,
>>>>>> +                                  HCI_INIT_TIMEOUT);
>>>>>> +             if (IS_ERR(skb)) {
>>>>>> +                     ret =3D PTR_ERR(skb);
>>>>>> +                     BT_ERR("%s: BCM20702: patch command %04x faile=
d (%ld)",
>>>>>> +                             hdev->name, opcode, ret);
>>>>>> +                     goto reset_fw;
>>>>>> +             }
>>>>>> +             kfree_skb(skb);
>>>>>> +     }
>>>>>> +
>>>>>> +     /* 250 msec delay after Launch Ram completes */
>>>>>> +     msleep(250);
>>>>>> +
>>>>>> +reset_fw:
>>>>>> +     /* Reset */
>>>>>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_T=
IMEOUT);
>>>>>> +     if (IS_ERR(skb)) {
>>>>>> +             ret =3D PTR_ERR(skb);
>>>>>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, re=
t);
>>>>>> +             goto done;
>>>>>> +     }
>>>>>> +     kfree_skb(skb);
>>>>>> +
>>>>>> +get_fw_ver:
>>>>>> +     /* Read Local Version Info */
>>>>>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NUL=
L,
>>>>>> +                          HCI_INIT_TIMEOUT);
>>>>>> +     if (IS_ERR(skb)) {
>>>>>> +             ret =3D PTR_ERR(skb);
>>>>>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>>>>>> +                     hdev->name, ret);
>>>>>> +             goto done;
>>>>>> +     }
>>>>>> +
>>>>>> +     ver =3D (struct hci_rp_read_local_version *) skb->data;
>>>>>> +     BT_INFO("%s: BCM20702: firmware hci_ver=3D%02x hci_rev=3D%04x =
lmp_ver=3D%02x "
>>>>>> +             "lmp_subver=3D%04x", hdev->name, ver->hci_ver, ver->hc=
i_rev,
>>>>>> +             ver->lmp_ver, ver->lmp_subver);
>>>>>> +     kfree_skb(skb);
>>>>>
>>>>> Is this HCI_Reset and HCI_Read_Local_Version_Info really needed. The =
standard init procedure will do exactly that anyway. I have no problem in h=
aving this in here. I am just saying that it will be run anyway again after=
 ->setup() completed.
>>>>>
>>>>> You can check for yourself when running btmon before plugging this de=
vice or manually loading btusb.
>>>>
>>>> The reset is important because it activates the new firmware. And, I
>>>> do like seeing the local version info in kernel log because it makes
>>>> it clear what FW version was before and is after the upgrade.
>>>
>>> I get that. What I am saying is that after you go through ->setup() the=
 kernel will call these commands anyway.
>>>
>>> So what you are getting now is this:
>>>
>>>        setup()
>>>                HCI_Reset
>>>                HCI_Read_Local_Version_Info
>>>                .. load HCD
>>>                HCI_Reset
>>>                HCI_Read_Local_Version_Info
>>>        init()
>>>                HCI_Reset
>>>                HCI_Read_Local_Features
>>>                HCI_Read_Local_Version_Info
>>>                HCI_Read_BD_Address
>>>
>>> The init is always executed right after the setup. Unload btusb, start =
btmon and reload btusb. You will see.
>>
>> I understand. I looked at the code in hci_dev_do_open() and
>> __hci_init(). I'm OK to remove the first HCI_Read_Local_Version_Info
>> before loading HCD. But, I'd like to keep the rest, so setup() becomes
>> reset + load HCD + reset + read local version info. I'd like to see in
>> kernel log what the FW version is after the HCD push.
>
> We can keep it as well. I do not care much either way since ->setup() is =
only run once. I was more curious if there is some specific reason for it.
>

OK.

>>>>>> +
>>>>>> +done:
>>>>>> +     if (fw)
>>>>>> +             release_firmware(fw);
>>>>>> +
>>>>>> +     return ret;
>>>>>> +}
>>>>>> +
>>>>>> static int btusb_probe(struct usb_interface *intf,
>>>>>>                             const struct usb_device_id *id)
>>>>>> {
>>>>>> @@ -1485,6 +1621,9 @@ static int btusb_probe(struct usb_interface *i=
ntf,
>>>>>>     if (id->driver_info & BTUSB_BCM92035)
>>>>>>             hdev->setup =3D btusb_setup_bcm92035;
>>>>>>
>>>>>> +     if (id->driver_info & BTUSB_BCM20702_PATCHRAM)
>>>>>> +             hdev->setup =3D btusb_setup_bcm20702;
>>>>>> +
>>>>>
>>>>> If we are somehow able to just do BTUSB_BROADCOM and always trigger t=
he patchram loading based on the hardware found that would be super great.
>>>>
>>>> It would be nice, but I don't have enough resources to test it. I'd
>>>> say let's start with this and extend from there.
>>>
>>> We can not easily go back and revert this change though. Can you includ=
e /sys/kernel/debug/usb/devices part for this device.
>>
>> T:  Bus=3D05 Lev=3D01 Prnt=3D01 Port=3D00 Cnt=3D01 Dev#=3D  2 Spd=3D12  =
 MxCh=3D 0
>> D:  Ver=3D 2.00 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 MxPS=3D64 #Cfgs=3D  1
>> P:  Vendor=3D0a5c ProdID=3D22be Rev=3D 1.12
>> S:  Manufacturer=3DBroadcom Corp
>> S:  Product=3DBCM20702A0
>> S:  SerialNumber=3D000000000000
>
> Did you block out the serial number on purpose? Normally that is your BD_=
ADDR. Or do you happen to have a different kind of module and the firmware =
loading will actually set the address. If so, then careful that the firmwar=
e not program the same BD_ADDR for all devices.
>

No, I didn't block the serial number. The device truly has
SerialNumber=3D0. Also, by default, it uses BD_ADDR =3D 00:20:70:02:A0:00.
The BD_ADDR needs to be reprogrammed at every boot (or at hotplug).

>> C:* #Ifs=3D 4 Cfg#=3D 1 Atr=3De0 MxPwr=3D  0mA
>> I:* If#=3D 0 Alt=3D 0 #EPs=3D 3 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Drive=
r=3Dbtusb
>> E:  Ad=3D81(I) Atr=3D03(Int.) MxPS=3D  16 Ivl=3D1ms
>> E:  Ad=3D82(I) Atr=3D02(Bulk) MxPS=3D  64 Ivl=3D0ms
>> E:  Ad=3D02(O) Atr=3D02(Bulk) MxPS=3D  64 Ivl=3D0ms
>> I:* If#=3D 1 Alt=3D 0 #EPs=3D 2 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Drive=
r=3Dbtusb
>> E:  Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D   0 Ivl=3D1ms
>> E:  Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D   0 Ivl=3D1ms
>> I:  If#=3D 1 Alt=3D 1 #EPs=3D 2 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Drive=
r=3Dbtusb
>> E:  Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D   9 Ivl=3D1ms
>> E:  Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D   9 Ivl=3D1ms
>> I:  If#=3D 1 Alt=3D 2 #EPs=3D 2 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Drive=
r=3Dbtusb
>> E:  Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D  17 Ivl=3D1ms
>> E:  Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D  17 Ivl=3D1ms
>> I:  If#=3D 1 Alt=3D 3 #EPs=3D 2 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Drive=
r=3Dbtusb
>> E:  Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D  25 Ivl=3D1ms
>> E:  Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D  25 Ivl=3D1ms
>> I:  If#=3D 1 Alt=3D 4 #EPs=3D 2 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Drive=
r=3Dbtusb
>> E:  Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D  33 Ivl=3D1ms
>> E:  Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D  33 Ivl=3D1ms
>> I:  If#=3D 1 Alt=3D 5 #EPs=3D 2 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 Drive=
r=3Dbtusb
>> E:  Ad=3D83(I) Atr=3D01(Isoc) MxPS=3D  49 Ivl=3D1ms
>> E:  Ad=3D03(O) Atr=3D01(Isoc) MxPS=3D  49 Ivl=3D1ms
>> I:* If#=3D 2 Alt=3D 0 #EPs=3D 2 Cls=3Dff(vend.) Sub=3Dff Prot=3Dff Drive=
r=3D(none)
>> E:  Ad=3D84(I) Atr=3D02(Bulk) MxPS=3D  32 Ivl=3D0ms
>> E:  Ad=3D04(O) Atr=3D02(Bulk) MxPS=3D  32 Ivl=3D0ms
>> I:* If#=3D 3 Alt=3D 0 #EPs=3D 0 Cls=3Dfe(app. ) Sub=3D01 Prot=3D01 Drive=
r=3D(none)
>
> Regards
>
> Marcel
>

Let me know what you would like to do with this patch.

-- Petri

Re: [PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

[Marcel Holtmann]

Hi Petri,

>>>>>>> After hardware reset, BCM20702 obtains its initial firmware from a PROM chip.
>>>>>>> Once this initial firmware is running, the firmware can be further upgraded
>>>>>>> over HCI interface with .hcd files provided by Broadcom. This is also known
>>>>>>> as "patch RAM" support. This change implements that.
>>>>>>> 
>>>>>>> If the .hcd file is not found in /lib/firmware, BCM20702 continues to operate
>>>>>>> with the initial firmware. Sample kernel log:
>>>>>>> hotplug: sys=firmware act=add fw=brcm/bcm20702-0a5c-22be.hcd dev=...
>>>>>>> Bluetooth: hci0: BCM20702: patch brcm/bcm20702-0a5c-22be.hcd not found
>>>>>>> Bluetooth: hci0: BCM20702: firmware hci_ver=06 hci_rev=1000 lmp_ver=06 lmp_subver=220e
>>>>>>> 
>>>>>>> If the .hcd file is found, btusb driver pushes it to BCM20702, and BCM20702
>>>>>>> starts using the new firmware. Sample kernel log:
>>>>>>> hotplug: sys=firmware act=add fw=brcm/bcm20702-0a5c-22be.hcd dev=...
>>>>>>> Bluetooth: hci0: BCM20702: patching hci_ver=06 hci_rev=1000 lmp_ver=06 lmp_subver=220e
>>>>>>> Bluetooth: hci0: BCM20702: firmware hci_ver=06 hci_rev=1389 lmp_ver=06 lmp_subver=220e
>>>>>>> 
>>>>>>> Above, we can see that hci_rev goes from 1000 to 1389 as a result of the upgrade.
>>>>>> 
>>>>>> this is a nice feature to have. Where are these hcd files are coming from. Are they available somewhere or do we have them available in linux-firmware.git tree.
>>>>> 
>>>>> In our case, we get these files directly from the vendor. I haven't
>>>>> seen these in linux-firmware.git tree. Some searching reveals that
>>>>> Windows drivers contain either .hex or .hcd files, and there appears
>>>>> to be a tool hex2hcd to convert to .hcd. See page
>>>>> http://swiesmann.de/?p=30.
>>>> 
>>>> what is the firmware name that the files from the vendor have. My problem with introducing a VID+PID based naming is that it might causes problems when the VID+PID is re-used for a different SKU or revision of the same board and we should have picked the firmware based on the board version.
>>> 
>>> The firmware files seem to have a naming pattern
>>> BCM20702A0_aaa.bbb.ccc.dddd.eeee. I don't know where
>>> aaa.bbb.ccc.dddd.eeee are derived from. Some version number, I guess.
>> 
>> I am trying to figure out what they mean. Any chance you can ask your vendor for help.
>> 
> 
> I've looked a bit more into this. It turns out that there are cases
> where BCM A0 chip might actually use BCM A1 firmware file. So, I don't
> think we can reliably use the product string (e.g. BCM20702A0) as a
> prefix.
> 
> So, any problem going with VID-PID approach here? That is easy for the
> user, as they can figure out the VID-PID pair with lsusb, and then
> place properly named file to /lib/firmware/brcm/
> 
> And, if the firmware file is missing, no problem. The device will
> operate with the default firmware from OTPROM.

in theory this all will apply to more than just the USB versions of the Broadcom chips, but that is what we might need to focus on first here. For the UART versions it can be easily done within hciattach.

So the first thing I would like to change is that we use USB_VENDOR_AND_INTERFACE_INFO() to identify the Broadcom device. That way the usb_device_id table gets smaller and we ensure that this works on every single one of the devices even if there is no firmware file present. The more expose this gets the better.

T:  Bus=05 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  2 Spd=12   MxCh= 0
D:  Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=0a5c ProdID=22be Rev= 1.12
S:  Manufacturer=Broadcom Corp
S:  Product=BCM20702A0
S:  SerialNumber=000000000000

Almost all newer Broadcom device identify themselves as vendor class, but then keep the subclass and protocol fields matching up with the H:2 specification. So lets focus on adding support to this entry:

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

And then we use brcm/{product}-%04x-%04x.hcd as your firmware filename. So getting the {product} string is something we should work out. Do you happen to know if we can issue a Broadcom vendor command to get the product identifier. We could use the UBS product string and just turn it into lower case. The appended a0 or b0 or whatever does not matter. However being able to just get the bcm20702 from the the device would be helpful.

As I side note here, we could check the serial number being zero or the default address being present to set an internal flag to tell the Bluetooth core that this device needs an address programmed before being fully operational. However that is a separate patch. Just a hint here.

>>>> And the HEX vs HCD is most likely the same as Intel’s seq vs bseq. Just one is a text file and the other a binary version of the same data. In all cases these are HCI commands anyway.
>>>> 
>>>>> Interestingly, now that I look at that page more closely, Ubuntu 12.04
>>>>> LTS with kernel 3.8 supports this already? Has Ubuntu made a patch
>>>>> that bluetooth-next doesn't have?
>>>> 
>>>> That is Ubuntu for you. They just push patches into their kernel and never give them back to upstream or make sure they get merged into bluetooth-next. I really dislike that behavior. It is always the easy way out instead of trying to do the right thing.
>>>> 
>>> 
>>> See this page: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1065400
>> 
>> It is a stupid userspace version that is racy as hell. Even the bug mentions that it should be done in the kernel. And I am pretty sure that was my initial comment as well. Just posting the userspace patch for reference.
>> 
>>>>>> I also wonder if there is a bit more general naming convention for the files. For example with a quick search I found references to BCM2045B2_002.002.011.0348.0349.hcd and similar. They indicate that the naming is not based on the USB vendor and product ID.
>>>>> 
>>>>> Some version numbering scheme is applied by Broadcom. However, at
>>>>> least initially, I'd prefer using USB vid/pid in the .hcd filename. I
>>>>> know that the .hcd file that I have works on our specific vid/pid of
>>>>> BCM20702, but I have no idea whether it would work on other BCM20702
>>>>> variants.
>>>> 
>>>> I am worried to back us into a corner. Reuse of PIDs is plenty. Especially with the cheaper dongle manufactures.
>>>> 
>>>>>> For example for the Intel ROM patching, we used internal versioning to pick the right file. That way we were able to run it for all of our devices. For the Broadcom devices, it might make sense to run the patchram routine for all devices not just this one specific one.
>>>>> 
>>>>> It would make sense, but I have no way of testing it. I only feel
>>>>> comfortable introducing this to the single device that I've been able
>>>>> to test with. It can later be extended to other BCM variants as people
>>>>> test this.
>>>> 
>>>> I have a bunch of Broadcom USB dongles around here. I might not get to it this week, but worth while trying to see how this works out.
>>>> 
>>>> At least the product string could be taken from the USB descriptors. Seems that Broadcom devices are pretty good in that regard no matter what. So quickly checking with mine, they give you BCM20702A0 and from the looks of it, the firmware files are prefix with exactly that.
>>> 
>>> Yes, using the product string as the FW filename prefix sounds good.
>>> But, we would need to hardcode the rest, which appears to be some kind
>>> of version number.
>> 
>> With the Intel firmware loading we fall back to a generic file in case the proper ROM patch file is not found. Maybe we should do something similar here as well. The more we can follow standard Broadcom procedure for these patch files, the better of course. It makes it a lot easier for any new devices.
>> 
> 
> We don't need a fallback as the FW file is optional. The device
> operates fine with OTPROM FW.

So there are no generic variables or configuration that should be set. If not, then this fine, we don’t need a fallback.

>>>>>>> Signed-off-by: Petri Gynther <pgynther@google.com>
>>>>>>> ---
>>>>>>> drivers/bluetooth/btusb.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>> 1 file changed, 139 insertions(+)
>>>>>>> 
>>>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>>>> index f338b0c..371d7e9 100644
>>>>>>> --- a/drivers/bluetooth/btusb.c
>>>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>>>> @@ -49,6 +49,7 @@ static struct usb_driver btusb_driver;
>>>>>>> #define BTUSB_WRONG_SCO_MTU   0x40
>>>>>>> #define BTUSB_ATH3012         0x80
>>>>>>> #define BTUSB_INTEL           0x100
>>>>>>> +#define BTUSB_BCM20702_PATCHRAM      0x200
>>>>>>> 
>>>>>>> static const struct usb_device_id btusb_table[] = {
>>>>>>>    /* Generic Bluetooth USB device */
>>>>>>> @@ -103,6 +104,7 @@ static const struct usb_device_id btusb_table[] = {
>>>>>>>    /* Broadcom BCM20702A0 */
>>>>>>>    { USB_DEVICE(0x0489, 0xe042) },
>>>>>>>    { USB_DEVICE(0x04ca, 0x2003) },
>>>>>>> +     { USB_DEVICE(0x0a5c, 0x22be), .driver_info = BTUSB_BCM20702_PATCHRAM },
>>>>>>>    { USB_DEVICE(0x0b05, 0x17b5) },
>>>>>>>    { USB_DEVICE(0x0b05, 0x17cb) },
>>>>>>>    { USB_DEVICE(0x413c, 0x8197) },
>>>>>>> @@ -1380,6 +1382,140 @@ exit_mfg_deactivate:
>>>>>>>    return 0;
>>>>>>> }
>>>>>>> 
>>>>>>> +static int btusb_setup_bcm20702(struct hci_dev *hdev)
>>>>>>> +{
>>>>>>> +     struct btusb_data *data = hci_get_drvdata(hdev);
>>>>>>> +     struct usb_device *udev = data->udev;
>>>>>>> +     char fw_name[64];
>>>>>>> +     const struct firmware *fw;
>>>>>>> +     const u8 *fw_ptr;
>>>>>>> +     size_t fw_size;
>>>>>>> +     const struct hci_command_hdr *cmd;
>>>>>>> +     const u8 *cmd_param;
>>>>>>> +     u16 opcode;
>>>>>>> +     struct sk_buff *skb;
>>>>>>> +     struct hci_rp_read_local_version *ver;
>>>>>>> +     long ret;
>>>>>>> +
>>>>>>> +     snprintf(fw_name, sizeof(fw_name), "brcm/bcm20702-%04x-%04x.hcd",
>>>>>>> +              le16_to_cpu(udev->descriptor.idVendor),
>>>>>>> +              le16_to_cpu(udev->descriptor.idProduct));
>>>>>>> +
>>>>>>> +     ret = request_firmware(&fw, fw_name, &hdev->dev);
>>>>>>> +     if (ret < 0) {
>>>>>>> +             BT_INFO("%s: BCM20702: patch %s not found", hdev->name,
>>>>>>> +                     fw_name);
>>>>>>> +             ret = 0;
>>>>>>> +             goto get_fw_ver;
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     /* Reset */
>>>>>>> +     skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>>>>>>> +     if (IS_ERR(skb)) {
>>>>>>> +             ret = PTR_ERR(skb);
>>>>>>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret);
>>>>>>> +             goto reset_fw;
>>>>>>> +     }
>>>>>>> +     kfree_skb(skb);
>>>>>>> +
>>>>>>> +     /* Read Local Version Info */
>>>>>>> +     skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
>>>>>>> +                          HCI_INIT_TIMEOUT);
>>>>>>> +     if (IS_ERR(skb)) {
>>>>>>> +             ret = PTR_ERR(skb);
>>>>>>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>>>>>>> +                     hdev->name, ret);
>>>>>>> +             goto reset_fw;
>>>>>>> +     }
>>>>>>> +
>>>>>> 
>>>>>> You need a length check here to ensure that resulting buffer is the correct length. Trusting the hardware is not a good idea. I have seen this go wrong in some cases.
>>>>> 
>>>>> Will do. I'll send revised diff for this.
>>>>> 
>>>>>> 
>>>>>>> +     ver = (struct hci_rp_read_local_version *) skb->data;
>>>>>>> +     BT_INFO("%s: BCM20702: patching hci_ver=%02x hci_rev=%04x lmp_ver=%02x "
>>>>>>> +             "lmp_subver=%04x", hdev->name, ver->hci_ver, ver->hci_rev,
>>>>>>> +             ver->lmp_ver, ver->lmp_subver);
>>>>>>> +     kfree_skb(skb);
>>>>>>> +
>>>>>>> +     /* Start Download */
>>>>>>> +     skb = __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEOUT);
>>>>>>> +     if (IS_ERR(skb)) {
>>>>>>> +             ret = PTR_ERR(skb);
>>>>>>> +             BT_ERR("%s: BCM20702: Download Minidrv command failed (%ld)",
>>>>>>> +                     hdev->name, ret);
>>>>>>> +             goto reset_fw;
>>>>>>> +     }
>>>>>>> +     kfree_skb(skb);
>>>>>>> +
>>>>>>> +     /* 50 msec delay after Download Minidrv completes */
>>>>>>> +     msleep(50);
>>>>>>> +
>>>>>>> +     fw_ptr = fw->data;
>>>>>>> +     fw_size = fw->size;
>>>>>>> +
>>>>>>> +     while (fw_size >= sizeof(*cmd)) {
>>>>>>> +             cmd = (struct hci_command_hdr *) fw_ptr;
>>>>>>> +             fw_ptr += sizeof(*cmd);
>>>>>>> +             fw_size -= sizeof(*cmd);
>>>>>>> +
>>>>>>> +             if (fw_size < cmd->plen) {
>>>>>>> +                     BT_ERR("%s: BCM20702: patch %s is corrupted",
>>>>>>> +                             hdev->name, fw_name);
>>>>>>> +                     ret = -EINVAL;
>>>>>>> +                     goto reset_fw;
>>>>>>> +             }
>>>>>>> +
>>>>>>> +             cmd_param = fw_ptr;
>>>>>>> +             fw_ptr += cmd->plen;
>>>>>>> +             fw_size -= cmd->plen;
>>>>>>> +
>>>>>>> +             opcode = le16_to_cpu(cmd->opcode);
>>>>>>> +
>>>>>>> +             skb = __hci_cmd_sync(hdev, opcode, cmd->plen, cmd_param,
>>>>>>> +                                  HCI_INIT_TIMEOUT);
>>>>>>> +             if (IS_ERR(skb)) {
>>>>>>> +                     ret = PTR_ERR(skb);
>>>>>>> +                     BT_ERR("%s: BCM20702: patch command %04x failed (%ld)",
>>>>>>> +                             hdev->name, opcode, ret);
>>>>>>> +                     goto reset_fw;
>>>>>>> +             }
>>>>>>> +             kfree_skb(skb);
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     /* 250 msec delay after Launch Ram completes */
>>>>>>> +     msleep(250);
>>>>>>> +
>>>>>>> +reset_fw:
>>>>>>> +     /* Reset */
>>>>>>> +     skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>>>>>>> +     if (IS_ERR(skb)) {
>>>>>>> +             ret = PTR_ERR(skb);
>>>>>>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, ret);
>>>>>>> +             goto done;
>>>>>>> +     }
>>>>>>> +     kfree_skb(skb);
>>>>>>> +
>>>>>>> +get_fw_ver:
>>>>>>> +     /* Read Local Version Info */
>>>>>>> +     skb = __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL,
>>>>>>> +                          HCI_INIT_TIMEOUT);
>>>>>>> +     if (IS_ERR(skb)) {
>>>>>>> +             ret = PTR_ERR(skb);
>>>>>>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>>>>>>> +                     hdev->name, ret);
>>>>>>> +             goto done;
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     ver = (struct hci_rp_read_local_version *) skb->data;
>>>>>>> +     BT_INFO("%s: BCM20702: firmware hci_ver=%02x hci_rev=%04x lmp_ver=%02x "
>>>>>>> +             "lmp_subver=%04x", hdev->name, ver->hci_ver, ver->hci_rev,
>>>>>>> +             ver->lmp_ver, ver->lmp_subver);
>>>>>>> +     kfree_skb(skb);
>>>>>> 
>>>>>> Is this HCI_Reset and HCI_Read_Local_Version_Info really needed. The standard init procedure will do exactly that anyway. I have no problem in having this in here. I am just saying that it will be run anyway again after ->setup() completed.
>>>>>> 
>>>>>> You can check for yourself when running btmon before plugging this device or manually loading btusb.
>>>>> 
>>>>> The reset is important because it activates the new firmware. And, I
>>>>> do like seeing the local version info in kernel log because it makes
>>>>> it clear what FW version was before and is after the upgrade.
>>>> 
>>>> I get that. What I am saying is that after you go through ->setup() the kernel will call these commands anyway.
>>>> 
>>>> So what you are getting now is this:
>>>> 
>>>>       setup()
>>>>               HCI_Reset
>>>>               HCI_Read_Local_Version_Info
>>>>               .. load HCD
>>>>               HCI_Reset
>>>>               HCI_Read_Local_Version_Info
>>>>       init()
>>>>               HCI_Reset
>>>>               HCI_Read_Local_Features
>>>>               HCI_Read_Local_Version_Info
>>>>               HCI_Read_BD_Address
>>>> 
>>>> The init is always executed right after the setup. Unload btusb, start btmon and reload btusb. You will see.
>>> 
>>> I understand. I looked at the code in hci_dev_do_open() and
>>> __hci_init(). I'm OK to remove the first HCI_Read_Local_Version_Info
>>> before loading HCD. But, I'd like to keep the rest, so setup() becomes
>>> reset + load HCD + reset + read local version info. I'd like to see in
>>> kernel log what the FW version is after the HCD push.
>> 
>> We can keep it as well. I do not care much either way since ->setup() is only run once. I was more curious if there is some specific reason for it.
>> 
> 
> OK.
> 
>>>>>>> +
>>>>>>> +done:
>>>>>>> +     if (fw)
>>>>>>> +             release_firmware(fw);
>>>>>>> +
>>>>>>> +     return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>> static int btusb_probe(struct usb_interface *intf,
>>>>>>>                            const struct usb_device_id *id)
>>>>>>> {
>>>>>>> @@ -1485,6 +1621,9 @@ static int btusb_probe(struct usb_interface *intf,
>>>>>>>    if (id->driver_info & BTUSB_BCM92035)
>>>>>>>            hdev->setup = btusb_setup_bcm92035;
>>>>>>> 
>>>>>>> +     if (id->driver_info & BTUSB_BCM20702_PATCHRAM)
>>>>>>> +             hdev->setup = btusb_setup_bcm20702;
>>>>>>> +
>>>>>> 
>>>>>> If we are somehow able to just do BTUSB_BROADCOM and always trigger the patchram loading based on the hardware found that would be super great.
>>>>> 
>>>>> It would be nice, but I don't have enough resources to test it. I'd
>>>>> say let's start with this and extend from there.
>>>> 
>>>> We can not easily go back and revert this change though. Can you include /sys/kernel/debug/usb/devices part for this device.
>>> 
>>> T:  Bus=05 Lev=01 Prnt=01 Port=00 Cnt=01 Dev#=  2 Spd=12   MxCh= 0
>>> D:  Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
>>> P:  Vendor=0a5c ProdID=22be Rev= 1.12
>>> S:  Manufacturer=Broadcom Corp
>>> S:  Product=BCM20702A0
>>> S:  SerialNumber=000000000000
>> 
>> Did you block out the serial number on purpose? Normally that is your BD_ADDR. Or do you happen to have a different kind of module and the firmware loading will actually set the address. If so, then careful that the firmware not program the same BD_ADDR for all devices.
>> 
> 
> No, I didn't block the serial number. The device truly has
> SerialNumber=0. Also, by default, it uses BD_ADDR = 00:20:70:02:A0:00.
> The BD_ADDR needs to be reprogrammed at every boot (or at hot plug).

After you changed the BD_ADDR, I was wondering if you manually re-read the USB string descriptor for the serial number that it now returns the actual address. Linux caches the descriptors, so it will most likely keep telling you it is 0. Might have to write a tiny libusb program to verify this.

Regards

Marcel

Re: [PATCH] Bluetooth: btusb: Add BCM20702 firmware upgrade support

[Petri Gynther]

Hi Marcel,

On Mon, May 5, 2014 at 11:01 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Petri,
>
>>>>>>>> After hardware reset, BCM20702 obtains its initial firmware from a=
 PROM chip.
>>>>>>>> Once this initial firmware is running, the firmware can be further=
 upgraded
>>>>>>>> over HCI interface with .hcd files provided by Broadcom. This is a=
lso known
>>>>>>>> as "patch RAM" support. This change implements that.
>>>>>>>>
>>>>>>>> If the .hcd file is not found in /lib/firmware, BCM20702 continues=
 to operate
>>>>>>>> with the initial firmware. Sample kernel log:
>>>>>>>> hotplug: sys=3Dfirmware act=3Dadd fw=3Dbrcm/bcm20702-0a5c-22be.hcd=
 dev=3D...
>>>>>>>> Bluetooth: hci0: BCM20702: patch brcm/bcm20702-0a5c-22be.hcd not f=
ound
>>>>>>>> Bluetooth: hci0: BCM20702: firmware hci_ver=3D06 hci_rev=3D1000 lm=
p_ver=3D06 lmp_subver=3D220e
>>>>>>>>
>>>>>>>> If the .hcd file is found, btusb driver pushes it to BCM20702, and=
 BCM20702
>>>>>>>> starts using the new firmware. Sample kernel log:
>>>>>>>> hotplug: sys=3Dfirmware act=3Dadd fw=3Dbrcm/bcm20702-0a5c-22be.hcd=
 dev=3D...
>>>>>>>> Bluetooth: hci0: BCM20702: patching hci_ver=3D06 hci_rev=3D1000 lm=
p_ver=3D06 lmp_subver=3D220e
>>>>>>>> Bluetooth: hci0: BCM20702: firmware hci_ver=3D06 hci_rev=3D1389 lm=
p_ver=3D06 lmp_subver=3D220e
>>>>>>>>
>>>>>>>> Above, we can see that hci_rev goes from 1000 to 1389 as a result =
of the upgrade.
>>>>>>>
>>>>>>> this is a nice feature to have. Where are these hcd files are comin=
g from. Are they available somewhere or do we have them available in linux-=
firmware.git tree.
>>>>>>
>>>>>> In our case, we get these files directly from the vendor. I haven't
>>>>>> seen these in linux-firmware.git tree. Some searching reveals that
>>>>>> Windows drivers contain either .hex or .hcd files, and there appears
>>>>>> to be a tool hex2hcd to convert to .hcd. See page
>>>>>> http://swiesmann.de/?p=3D30.
>>>>>
>>>>> what is the firmware name that the files from the vendor have. My pro=
blem with introducing a VID+PID based naming is that it might causes proble=
ms when the VID+PID is re-used for a different SKU or revision of the same =
board and we should have picked the firmware based on the board version.
>>>>
>>>> The firmware files seem to have a naming pattern
>>>> BCM20702A0_aaa.bbb.ccc.dddd.eeee. I don't know where
>>>> aaa.bbb.ccc.dddd.eeee are derived from. Some version number, I guess.
>>>
>>> I am trying to figure out what they mean. Any chance you can ask your v=
endor for help.
>>>
>>
>> I've looked a bit more into this. It turns out that there are cases
>> where BCM A0 chip might actually use BCM A1 firmware file. So, I don't
>> think we can reliably use the product string (e.g. BCM20702A0) as a
>> prefix.
>>
>> So, any problem going with VID-PID approach here? That is easy for the
>> user, as they can figure out the VID-PID pair with lsusb, and then
>> place properly named file to /lib/firmware/brcm/
>>
>> And, if the firmware file is missing, no problem. The device will
>> operate with the default firmware from OTPROM.
>
> in theory this all will apply to more than just the USB versions of the B=
roadcom chips, but that is what we might need to focus on first here. For t=
he UART versions it can be easily done within hciattach.
>
> So the first thing I would like to change is that we use USB_VENDOR_AND_I=
NTERFACE_INFO() to identify the Broadcom device. That way the usb_device_id=
 table gets smaller and we ensure that this works on every single one of th=
e devices even if there is no firmware file present. The more expose this g=
ets the better.
>
> T:  Bus=3D05 Lev=3D01 Prnt=3D01 Port=3D00 Cnt=3D01 Dev#=3D  2 Spd=3D12   =
MxCh=3D 0
> D:  Ver=3D 2.00 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 MxPS=3D64 #Cfgs=3D  1
> P:  Vendor=3D0a5c ProdID=3D22be Rev=3D 1.12
> S:  Manufacturer=3DBroadcom Corp
> S:  Product=3DBCM20702A0
> S:  SerialNumber=3D000000000000
>
> Almost all newer Broadcom device identify themselves as vendor class, but=
 then keep the subclass and protocol fields matching up with the H:2 specif=
ication. So lets focus on adding support to this entry:
>
>         /* Broadcom devices with vendor specific id */
>         { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01) },
>
> And then we use brcm/{product}-%04x-%04x.hcd as your firmware filename. S=
o getting the {product} string is something we should work out. Do you happ=
en to know if we can issue a Broadcom vendor command to get the product ide=
ntifier. We could use the UBS product string and just turn it into lower ca=
se. The appended a0 or b0 or whatever does not matter. However being able t=
o just get the bcm20702 from the the device would be helpful.
>
> As I side note here, we could check the serial number being zero or the d=
efault address being present to set an internal flag to tell the Bluetooth =
core that this device needs an address programmed before being fully operat=
ional. However that is a separate patch. Just a hint here.
>

New patch coming soon. I ended up using udev->product to obtain the
iProduct string "BCM20702A0". Kernel doesn't have strlwr(), so I left
it as-is uppercase. If you strongly prefer lowercase, we can add code
to do so.

-- Petri

>>>>> And the HEX vs HCD is most likely the same as Intel=E2=80=99s seq vs =
bseq. Just one is a text file and the other a binary version of the same da=
ta. In all cases these are HCI commands anyway.
>>>>>
>>>>>> Interestingly, now that I look at that page more closely, Ubuntu 12.=
04
>>>>>> LTS with kernel 3.8 supports this already? Has Ubuntu made a patch
>>>>>> that bluetooth-next doesn't have?
>>>>>
>>>>> That is Ubuntu for you. They just push patches into their kernel and =
never give them back to upstream or make sure they get merged into bluetoot=
h-next. I really dislike that behavior. It is always the easy way out inste=
ad of trying to do the right thing.
>>>>>
>>>>
>>>> See this page: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/10=
65400
>>>
>>> It is a stupid userspace version that is racy as hell. Even the bug men=
tions that it should be done in the kernel. And I am pretty sure that was m=
y initial comment as well. Just posting the userspace patch for reference.
>>>
>>>>>>> I also wonder if there is a bit more general naming convention for =
the files. For example with a quick search I found references to BCM2045B2_=
002.002.011.0348.0349.hcd and similar. They indicate that the naming is not=
 based on the USB vendor and product ID.
>>>>>>
>>>>>> Some version numbering scheme is applied by Broadcom. However, at
>>>>>> least initially, I'd prefer using USB vid/pid in the .hcd filename. =
I
>>>>>> know that the .hcd file that I have works on our specific vid/pid of
>>>>>> BCM20702, but I have no idea whether it would work on other BCM20702
>>>>>> variants.
>>>>>
>>>>> I am worried to back us into a corner. Reuse of PIDs is plenty. Espec=
ially with the cheaper dongle manufactures.
>>>>>
>>>>>>> For example for the Intel ROM patching, we used internal versioning=
 to pick the right file. That way we were able to run it for all of our dev=
ices. For the Broadcom devices, it might make sense to run the patchram rou=
tine for all devices not just this one specific one.
>>>>>>
>>>>>> It would make sense, but I have no way of testing it. I only feel
>>>>>> comfortable introducing this to the single device that I've been abl=
e
>>>>>> to test with. It can later be extended to other BCM variants as peop=
le
>>>>>> test this.
>>>>>
>>>>> I have a bunch of Broadcom USB dongles around here. I might not get t=
o it this week, but worth while trying to see how this works out.
>>>>>
>>>>> At least the product string could be taken from the USB descriptors. =
Seems that Broadcom devices are pretty good in that regard no matter what. =
So quickly checking with mine, they give you BCM20702A0 and from the looks =
of it, the firmware files are prefix with exactly that.
>>>>
>>>> Yes, using the product string as the FW filename prefix sounds good.
>>>> But, we would need to hardcode the rest, which appears to be some kind
>>>> of version number.
>>>
>>> With the Intel firmware loading we fall back to a generic file in case =
the proper ROM patch file is not found. Maybe we should do something simila=
r here as well. The more we can follow standard Broadcom procedure for thes=
e patch files, the better of course. It makes it a lot easier for any new d=
evices.
>>>
>>
>> We don't need a fallback as the FW file is optional. The device
>> operates fine with OTPROM FW.
>
> So there are no generic variables or configuration that should be set. If=
 not, then this fine, we don=E2=80=99t need a fallback.
>
>>>>>>>> Signed-off-by: Petri Gynther <pgynther@google.com>
>>>>>>>> ---
>>>>>>>> drivers/bluetooth/btusb.c | 139 ++++++++++++++++++++++++++++++++++=
++++++++++++
>>>>>>>> 1 file changed, 139 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>>>>> index f338b0c..371d7e9 100644
>>>>>>>> --- a/drivers/bluetooth/btusb.c
>>>>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>>>>> @@ -49,6 +49,7 @@ static struct usb_driver btusb_driver;
>>>>>>>> #define BTUSB_WRONG_SCO_MTU   0x40
>>>>>>>> #define BTUSB_ATH3012         0x80
>>>>>>>> #define BTUSB_INTEL           0x100
>>>>>>>> +#define BTUSB_BCM20702_PATCHRAM      0x200
>>>>>>>>
>>>>>>>> static const struct usb_device_id btusb_table[] =3D {
>>>>>>>>    /* Generic Bluetooth USB device */
>>>>>>>> @@ -103,6 +104,7 @@ static const struct usb_device_id btusb_table[=
] =3D {
>>>>>>>>    /* Broadcom BCM20702A0 */
>>>>>>>>    { USB_DEVICE(0x0489, 0xe042) },
>>>>>>>>    { USB_DEVICE(0x04ca, 0x2003) },
>>>>>>>> +     { USB_DEVICE(0x0a5c, 0x22be), .driver_info =3D BTUSB_BCM2070=
2_PATCHRAM },
>>>>>>>>    { USB_DEVICE(0x0b05, 0x17b5) },
>>>>>>>>    { USB_DEVICE(0x0b05, 0x17cb) },
>>>>>>>>    { USB_DEVICE(0x413c, 0x8197) },
>>>>>>>> @@ -1380,6 +1382,140 @@ exit_mfg_deactivate:
>>>>>>>>    return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> +static int btusb_setup_bcm20702(struct hci_dev *hdev)
>>>>>>>> +{
>>>>>>>> +     struct btusb_data *data =3D hci_get_drvdata(hdev);
>>>>>>>> +     struct usb_device *udev =3D data->udev;
>>>>>>>> +     char fw_name[64];
>>>>>>>> +     const struct firmware *fw;
>>>>>>>> +     const u8 *fw_ptr;
>>>>>>>> +     size_t fw_size;
>>>>>>>> +     const struct hci_command_hdr *cmd;
>>>>>>>> +     const u8 *cmd_param;
>>>>>>>> +     u16 opcode;
>>>>>>>> +     struct sk_buff *skb;
>>>>>>>> +     struct hci_rp_read_local_version *ver;
>>>>>>>> +     long ret;
>>>>>>>> +
>>>>>>>> +     snprintf(fw_name, sizeof(fw_name), "brcm/bcm20702-%04x-%04x.=
hcd",
>>>>>>>> +              le16_to_cpu(udev->descriptor.idVendor),
>>>>>>>> +              le16_to_cpu(udev->descriptor.idProduct));
>>>>>>>> +
>>>>>>>> +     ret =3D request_firmware(&fw, fw_name, &hdev->dev);
>>>>>>>> +     if (ret < 0) {
>>>>>>>> +             BT_INFO("%s: BCM20702: patch %s not found", hdev->na=
me,
>>>>>>>> +                     fw_name);
>>>>>>>> +             ret =3D 0;
>>>>>>>> +             goto get_fw_ver;
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     /* Reset */
>>>>>>>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT=
_TIMEOUT);
>>>>>>>> +     if (IS_ERR(skb)) {
>>>>>>>> +             ret =3D PTR_ERR(skb);
>>>>>>>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, =
ret);
>>>>>>>> +             goto reset_fw;
>>>>>>>> +     }
>>>>>>>> +     kfree_skb(skb);
>>>>>>>> +
>>>>>>>> +     /* Read Local Version Info */
>>>>>>>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, N=
ULL,
>>>>>>>> +                          HCI_INIT_TIMEOUT);
>>>>>>>> +     if (IS_ERR(skb)) {
>>>>>>>> +             ret =3D PTR_ERR(skb);
>>>>>>>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>>>>>>>> +                     hdev->name, ret);
>>>>>>>> +             goto reset_fw;
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>
>>>>>>> You need a length check here to ensure that resulting buffer is the=
 correct length. Trusting the hardware is not a good idea. I have seen this=
 go wrong in some cases.
>>>>>>
>>>>>> Will do. I'll send revised diff for this.
>>>>>>
>>>>>>>
>>>>>>>> +     ver =3D (struct hci_rp_read_local_version *) skb->data;
>>>>>>>> +     BT_INFO("%s: BCM20702: patching hci_ver=3D%02x hci_rev=3D%04=
x lmp_ver=3D%02x "
>>>>>>>> +             "lmp_subver=3D%04x", hdev->name, ver->hci_ver, ver->=
hci_rev,
>>>>>>>> +             ver->lmp_ver, ver->lmp_subver);
>>>>>>>> +     kfree_skb(skb);
>>>>>>>> +
>>>>>>>> +     /* Start Download */
>>>>>>>> +     skb =3D __hci_cmd_sync(hdev, 0xfc2e, 0, NULL, HCI_INIT_TIMEO=
UT);
>>>>>>>> +     if (IS_ERR(skb)) {
>>>>>>>> +             ret =3D PTR_ERR(skb);
>>>>>>>> +             BT_ERR("%s: BCM20702: Download Minidrv command faile=
d (%ld)",
>>>>>>>> +                     hdev->name, ret);
>>>>>>>> +             goto reset_fw;
>>>>>>>> +     }
>>>>>>>> +     kfree_skb(skb);
>>>>>>>> +
>>>>>>>> +     /* 50 msec delay after Download Minidrv completes */
>>>>>>>> +     msleep(50);
>>>>>>>> +
>>>>>>>> +     fw_ptr =3D fw->data;
>>>>>>>> +     fw_size =3D fw->size;
>>>>>>>> +
>>>>>>>> +     while (fw_size >=3D sizeof(*cmd)) {
>>>>>>>> +             cmd =3D (struct hci_command_hdr *) fw_ptr;
>>>>>>>> +             fw_ptr +=3D sizeof(*cmd);
>>>>>>>> +             fw_size -=3D sizeof(*cmd);
>>>>>>>> +
>>>>>>>> +             if (fw_size < cmd->plen) {
>>>>>>>> +                     BT_ERR("%s: BCM20702: patch %s is corrupted"=
,
>>>>>>>> +                             hdev->name, fw_name);
>>>>>>>> +                     ret =3D -EINVAL;
>>>>>>>> +                     goto reset_fw;
>>>>>>>> +             }
>>>>>>>> +
>>>>>>>> +             cmd_param =3D fw_ptr;
>>>>>>>> +             fw_ptr +=3D cmd->plen;
>>>>>>>> +             fw_size -=3D cmd->plen;
>>>>>>>> +
>>>>>>>> +             opcode =3D le16_to_cpu(cmd->opcode);
>>>>>>>> +
>>>>>>>> +             skb =3D __hci_cmd_sync(hdev, opcode, cmd->plen, cmd_=
param,
>>>>>>>> +                                  HCI_INIT_TIMEOUT);
>>>>>>>> +             if (IS_ERR(skb)) {
>>>>>>>> +                     ret =3D PTR_ERR(skb);
>>>>>>>> +                     BT_ERR("%s: BCM20702: patch command %04x fai=
led (%ld)",
>>>>>>>> +                             hdev->name, opcode, ret);
>>>>>>>> +                     goto reset_fw;
>>>>>>>> +             }
>>>>>>>> +             kfree_skb(skb);
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     /* 250 msec delay after Launch Ram completes */
>>>>>>>> +     msleep(250);
>>>>>>>> +
>>>>>>>> +reset_fw:
>>>>>>>> +     /* Reset */
>>>>>>>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT=
_TIMEOUT);
>>>>>>>> +     if (IS_ERR(skb)) {
>>>>>>>> +             ret =3D PTR_ERR(skb);
>>>>>>>> +             BT_ERR("%s: HCI_OP_RESET failed (%ld)", hdev->name, =
ret);
>>>>>>>> +             goto done;
>>>>>>>> +     }
>>>>>>>> +     kfree_skb(skb);
>>>>>>>> +
>>>>>>>> +get_fw_ver:
>>>>>>>> +     /* Read Local Version Info */
>>>>>>>> +     skb =3D __hci_cmd_sync(hdev, HCI_OP_READ_LOCAL_VERSION, 0, N=
ULL,
>>>>>>>> +                          HCI_INIT_TIMEOUT);
>>>>>>>> +     if (IS_ERR(skb)) {
>>>>>>>> +             ret =3D PTR_ERR(skb);
>>>>>>>> +             BT_ERR("%s: HCI_OP_READ_LOCAL_VERSION failed (%ld)",
>>>>>>>> +                     hdev->name, ret);
>>>>>>>> +             goto done;
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     ver =3D (struct hci_rp_read_local_version *) skb->data;
>>>>>>>> +     BT_INFO("%s: BCM20702: firmware hci_ver=3D%02x hci_rev=3D%04=
x lmp_ver=3D%02x "
>>>>>>>> +             "lmp_subver=3D%04x", hdev->name, ver->hci_ver, ver->=
hci_rev,
>>>>>>>> +             ver->lmp_ver, ver->lmp_subver);
>>>>>>>> +     kfree_skb(skb);
>>>>>>>
>>>>>>> Is this HCI_Reset and HCI_Read_Local_Version_Info really needed. Th=
e standard init procedure will do exactly that anyway. I have no problem in=
 having this in here. I am just saying that it will be run anyway again aft=
er ->setup() completed.
>>>>>>>
>>>>>>> You can check for yourself when running btmon before plugging this =
device or manually loading btusb.
>>>>>>
>>>>>> The reset is important because it activates the new firmware. And, I
>>>>>> do like seeing the local version info in kernel log because it makes
>>>>>> it clear what FW version was before and is after the upgrade.
>>>>>
>>>>> I get that. What I am saying is that after you go through ->setup() t=
he kernel will call these commands anyway.
>>>>>
>>>>> So what you are getting now is this:
>>>>>
>>>>>       setup()
>>>>>               HCI_Reset
>>>>>               HCI_Read_Local_Version_Info
>>>>>               .. load HCD
>>>>>               HCI_Reset
>>>>>               HCI_Read_Local_Version_Info
>>>>>       init()
>>>>>               HCI_Reset
>>>>>               HCI_Read_Local_Features
>>>>>               HCI_Read_Local_Version_Info
>>>>>               HCI_Read_BD_Address
>>>>>
>>>>> The init is always executed right after the setup. Unload btusb, star=
t btmon and reload btusb. You will see.
>>>>
>>>> I understand. I looked at the code in hci_dev_do_open() and
>>>> __hci_init(). I'm OK to remove the first HCI_Read_Local_Version_Info
>>>> before loading HCD. But, I'd like to keep the rest, so setup() becomes
>>>> reset + load HCD + reset + read local version info. I'd like to see in
>>>> kernel log what the FW version is after the HCD push.
>>>
>>> We can keep it as well. I do not care much either way since ->setup() i=
s only run once. I was more curious if there is some specific reason for it=
.
>>>
>>
>> OK.
>>
>>>>>>>> +
>>>>>>>> +done:
>>>>>>>> +     if (fw)
>>>>>>>> +             release_firmware(fw);
>>>>>>>> +
>>>>>>>> +     return ret;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> static int btusb_probe(struct usb_interface *intf,
>>>>>>>>                            const struct usb_device_id *id)
>>>>>>>> {
>>>>>>>> @@ -1485,6 +1621,9 @@ static int btusb_probe(struct usb_interface =
*intf,
>>>>>>>>    if (id->driver_info & BTUSB_BCM92035)
>>>>>>>>            hdev->setup =3D btusb_setup_bcm92035;
>>>>>>>>
>>>>>>>> +     if (id->driver_info & BTUSB_BCM20702_PATCHRAM)
>>>>>>>> +             hdev->setup =3D btusb_setup_bcm20702;
>>>>>>>> +
>>>>>>>
>>>>>>> If we are somehow able to just do BTUSB_BROADCOM and always trigger=
 the patchram loading based on the hardware found that would be super great=
.
>>>>>>
>>>>>> It would be nice, but I don't have enough resources to test it. I'd
>>>>>> say let's start with this and extend from there.
>>>>>
>>>>> We can not easily go back and revert this change though. Can you incl=
ude /sys/kernel/debug/usb/devices part for this device.
>>>>
>>>> T:  Bus=3D05 Lev=3D01 Prnt=3D01 Port=3D00 Cnt=3D01 Dev#=3D  2 Spd=3D12=
   MxCh=3D 0
>>>> D:  Ver=3D 2.00 Cls=3Dff(vend.) Sub=3D01 Prot=3D01 MxPS=3D64 #Cfgs=3D =
 1
>>>> P:  Vendor=3D0a5c ProdID=3D22be Rev=3D 1.12
>>>> S:  Manufacturer=3DBroadcom Corp
>>>> S:  Product=3DBCM20702A0
>>>> S:  SerialNumber=3D000000000000
>>>
>>> Did you block out the serial number on purpose? Normally that is your B=
D_ADDR. Or do you happen to have a different kind of module and the firmwar=
e loading will actually set the address. If so, then careful that the firmw=
are not program the same BD_ADDR for all devices.
>>>
>>
>> No, I didn't block the serial number. The device truly has
>> SerialNumber=3D0. Also, by default, it uses BD_ADDR =3D 00:20:70:02:A0:0=
0.
>> The BD_ADDR needs to be reprogrammed at every boot (or at hot plug).
>
> After you changed the BD_ADDR, I was wondering if you manually re-read th=
e USB string descriptor for the serial number that it now returns the actua=
l address. Linux caches the descriptors, so it will most likely keep tellin=
g you it is 0. Might have to write a tiny libusb program to verify this.
>
> Regards
>
> Marcel
>