[PATCH 0/2] broadcom patchram firmware loader

[PATCH 0/2] broadcom patchram firmware loader

[Jesse Sung]

From: Wen-chien Jesse Sung <jesse.sung@canonical.com>

There is an user space firmware loading tool which can be found at
http://marc.info/?l=linux-bluetooth&m=132039175324993&w=2

This tool requires hci device to do the firmware loading, but this
may cause some race condition between patchram tool and bluetoothd
or something that also works on hci interface.

Also it needs some hooks to make firmware loads after bootup, s3,
s4, rfkill, and device hotplug events. Implement this loader in kernel
module would make things more easier.

These patches are sent to the mailing list for comment on Aug-15:
http://marc.info/?t=134495475100008&r=1&w=2
No further comment since then. I've rebased these patches against
current bluetooth git tree and would like to propose them for merging.

Wen-chien Jesse Sung (2):
  Implement broadcom patchram firmware loader
  Cache firmware images for later use

 drivers/bluetooth/btusb.c |  148 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 143 insertions(+), 5 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] Implement broadcom patchram firmware loader

[Jesse Sung]

From: Wen-chien Jesse Sung <jesse.sung@canonical.com>


Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
---
 drivers/bluetooth/btusb.c |  103 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 654e248..7189fed 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -23,6 +23,8 @@
 
 #include <linux/module.h>
 #include <linux/usb.h>
+#include <linux/delay.h>
+#include <linux/firmware.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -47,6 +49,7 @@ static struct usb_driver btusb_driver;
 #define BTUSB_BROKEN_ISOC	0x20
 #define BTUSB_WRONG_SCO_MTU	0x40
 #define BTUSB_ATH3012		0x80
+#define BTUSB_BCM_PATCHRAM	0x100
 
 static struct usb_device_id btusb_table[] = {
 	/* Generic Bluetooth USB device */
@@ -96,14 +99,15 @@ static struct usb_device_id btusb_table[] = {
 	{ USB_DEVICE(0x0c10, 0x0000) },
 
 	/* Broadcom BCM20702A0 */
+	{ USB_DEVICE(0x0489, 0xe031), .driver_info = BTUSB_BCM_PATCHRAM },
 	{ USB_DEVICE(0x0489, 0xe042) },
-	{ USB_DEVICE(0x413c, 0x8197) },
+	{ USB_DEVICE(0x413c, 0x8197), .driver_info = BTUSB_BCM_PATCHRAM },
 
 	/* Foxconn - Hon Hai */
 	{ USB_DEVICE(0x0489, 0xe033) },
 
 	/*Broadcom devices with vendor specific id */
-	{ USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01) },
+	{ USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01), .driver_info = BTUSB_BCM_PATCHRAM },
 
 	{ }	/* Terminating entry */
 };
@@ -197,6 +201,37 @@ static struct usb_device_id blacklist_table[] = {
 	{ }	/* Terminating entry */
 };
 
+#define PATCHRAM_TIMEOUT	1000
+#define FW_0489_E031		"fw-0489_e031.hcd"
+#define FW_0A5C_21D3		"fw-0a5c_21d3.hcd"
+#define FW_0A5C_21D7		"fw-0a5c_21d7.hcd"
+#define FW_0A5C_21E6		"fw-0a5c_21e6.hcd"
+#define FW_0A5C_21F3		"fw-0a5c_21f3.hcd"
+#define FW_0A5C_21F4		"fw-0a5c_21f4.hcd"
+#define FW_413C_8197		"fw-413c_8197.hcd"
+
+MODULE_FIRMWARE(FW_0489_E031);
+MODULE_FIRMWARE(FW_0A5C_21D3);
+MODULE_FIRMWARE(FW_0A5C_21D7);
+MODULE_FIRMWARE(FW_0A5C_21E6);
+MODULE_FIRMWARE(FW_0A5C_21F3);
+MODULE_FIRMWARE(FW_0A5C_21F4);
+MODULE_FIRMWARE(FW_413C_8197);
+
+static struct usb_device_id patchram_table[] = {
+	/* Dell DW1704 */
+	{ USB_DEVICE(0x0a5c, 0x21d3), .driver_info = (kernel_ulong_t) FW_0A5C_21D3 },
+	{ USB_DEVICE(0x0a5c, 0x21d7), .driver_info = (kernel_ulong_t) FW_0A5C_21D7 },
+	/* Dell DW380 */
+	{ USB_DEVICE(0x413c, 0x8197), .driver_info = (kernel_ulong_t) FW_413C_8197 },
+	/* FoxConn Hon Hai */
+	{ USB_DEVICE(0x0489, 0xe031), .driver_info = (kernel_ulong_t) FW_0489_E031 },
+	/* Lenovo */
+	{ USB_DEVICE(0x0a5c, 0x21e6), .driver_info = (kernel_ulong_t) FW_0A5C_21E6 },
+	{ USB_DEVICE(0x0a5c, 0x21f3), .driver_info = (kernel_ulong_t) FW_0A5C_21F3 },
+	{ USB_DEVICE(0x0a5c, 0x21f4), .driver_info = (kernel_ulong_t) FW_0A5C_21F4 },
+};
+
 #define BTUSB_MAX_ISOC_FRAMES	10
 
 #define BTUSB_INTR_RUNNING	0
@@ -914,6 +949,55 @@ static void btusb_waker(struct work_struct *work)
 	usb_autopm_put_interface(data->intf);
 }
 
+static inline void load_patchram_fw(struct usb_device *udev, const struct usb_device_id *id)
+{
+	size_t pos = 0;
+	int err = 0;
+	const struct firmware *fw;
+
+	unsigned char reset_cmd[] = { 0x03, 0x0c, 0x00 };
+	unsigned char download_cmd[] = { 0x2e, 0xfc, 0x00 };
+
+	if (request_firmware(&fw, (const char *) id->driver_info, &udev->dev) < 0) {
+		BT_INFO("can't load firmware, may not work correctly");
+		return;
+	}
+
+	if (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
+		reset_cmd, sizeof(reset_cmd), PATCHRAM_TIMEOUT) < 0) {
+		err = -1;
+		goto out;
+	}
+	msleep(300);
+
+	if (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
+		download_cmd, sizeof(download_cmd), PATCHRAM_TIMEOUT) < 0) {
+		err = -1;
+		goto out;
+	}
+	msleep(300);
+
+	while (pos < fw->size) {
+		size_t len;
+		len = fw->data[pos + 2] + 3;
+		if ((pos + len > fw->size) ||
+			(usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0,
+			USB_TYPE_CLASS, 0, 0, (void *)fw->data + pos, len,
+			PATCHRAM_TIMEOUT) < 0)) {
+			err = -1;
+			goto out;
+		}
+		pos += len;
+	}
+
+	err = (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
+		reset_cmd, sizeof(reset_cmd), PATCHRAM_TIMEOUT) < 0);
+out:
+	if (err)
+		BT_INFO("fail to load firmware, may not work correctly");
+	release_firmware(fw);
+}
+
 static int btusb_probe(struct usb_interface *intf,
 				const struct usb_device_id *id)
 {
@@ -1078,15 +1162,26 @@ static int btusb_probe(struct usb_interface *intf,
 		}
 	}
 
+	usb_set_intfdata(intf, data);
+
+	if (id->driver_info & BTUSB_BCM_PATCHRAM) {
+		const struct usb_device_id *match;
+		match = usb_match_id(intf, patchram_table);
+		if (match) {
+			btusb_open(hdev);
+			load_patchram_fw(interface_to_usbdev(intf), match);
+			btusb_close(hdev);
+		}
+	}
+
 	err = hci_register_dev(hdev);
 	if (err < 0) {
 		hci_free_dev(hdev);
+		usb_set_intfdata(intf, NULL);
 		kfree(data);
 		return err;
 	}
 
-	usb_set_intfdata(intf, data);
-
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 2/2] Cache firmware images for later use

[Jesse Sung]

From: Wen-chien Jesse Sung <jesse.sung@canonical.com>

Since request_firmware() may fail when resume from suspend, store
used firmware image in ram to make sure that we can have what we need
on resume.

Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
---
 drivers/bluetooth/btusb.c |   77 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 60 insertions(+), 17 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7189fed..8bf9e89 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -218,18 +218,34 @@ MODULE_FIRMWARE(FW_0A5C_21F3);
 MODULE_FIRMWARE(FW_0A5C_21F4);
 MODULE_FIRMWARE(FW_413C_8197);
 
+struct firmware_cache {
+	const char *filename;
+	u8* data;
+	size_t size;
+};
+
+static struct firmware_cache firmware[] = {
+	{ .filename = FW_0A5C_21D3, },
+	{ .filename = FW_0A5C_21D7, },
+	{ .filename = FW_413C_8197, },
+	{ .filename = FW_0489_E031, },
+	{ .filename = FW_0A5C_21E6, },
+	{ .filename = FW_0A5C_21F3, },
+	{ .filename = FW_0A5C_21F4, },
+};
+
 static struct usb_device_id patchram_table[] = {
 	/* Dell DW1704 */
-	{ USB_DEVICE(0x0a5c, 0x21d3), .driver_info = (kernel_ulong_t) FW_0A5C_21D3 },
-	{ USB_DEVICE(0x0a5c, 0x21d7), .driver_info = (kernel_ulong_t) FW_0A5C_21D7 },
+	{ USB_DEVICE(0x0a5c, 0x21d3), .driver_info = (kernel_ulong_t) &firmware[0] },
+	{ USB_DEVICE(0x0a5c, 0x21d7), .driver_info = (kernel_ulong_t) &firmware[1] },
 	/* Dell DW380 */
-	{ USB_DEVICE(0x413c, 0x8197), .driver_info = (kernel_ulong_t) FW_413C_8197 },
+	{ USB_DEVICE(0x413c, 0x8197), .driver_info = (kernel_ulong_t) &firmware[2] },
 	/* FoxConn Hon Hai */
-	{ USB_DEVICE(0x0489, 0xe031), .driver_info = (kernel_ulong_t) FW_0489_E031 },
+	{ USB_DEVICE(0x0489, 0xe031), .driver_info = (kernel_ulong_t) &firmware[3] },
 	/* Lenovo */
-	{ USB_DEVICE(0x0a5c, 0x21e6), .driver_info = (kernel_ulong_t) FW_0A5C_21E6 },
-	{ USB_DEVICE(0x0a5c, 0x21f3), .driver_info = (kernel_ulong_t) FW_0A5C_21F3 },
-	{ USB_DEVICE(0x0a5c, 0x21f4), .driver_info = (kernel_ulong_t) FW_0A5C_21F4 },
+	{ USB_DEVICE(0x0a5c, 0x21e6), .driver_info = (kernel_ulong_t) &firmware[4] },
+	{ USB_DEVICE(0x0a5c, 0x21f3), .driver_info = (kernel_ulong_t) &firmware[5] },
+	{ USB_DEVICE(0x0a5c, 0x21f4), .driver_info = (kernel_ulong_t) &firmware[6] },
 };
 
 #define BTUSB_MAX_ISOC_FRAMES	10
@@ -953,14 +969,27 @@ static inline void load_patchram_fw(struct usb_device *udev, const struct usb_de
 {
 	size_t pos = 0;
 	int err = 0;
-	const struct firmware *fw;
+	struct firmware_cache *fwcache;
 
 	unsigned char reset_cmd[] = { 0x03, 0x0c, 0x00 };
 	unsigned char download_cmd[] = { 0x2e, 0xfc, 0x00 };
 
-	if (request_firmware(&fw, (const char *) id->driver_info, &udev->dev) < 0) {
-		BT_INFO("can't load firmware, may not work correctly");
-		return;
+	fwcache = (struct firmware_cache *)id->driver_info;
+	if (!fwcache->data) {
+		const struct firmware *fw;
+		if (request_firmware(&fw, fwcache->filename, &udev->dev) < 0) {
+			BT_INFO("can't load firmware, may not work correctly");
+			return;
+		}
+		fwcache->data = kmalloc(fw->size, GFP_KERNEL);
+		if (!fwcache->data) {
+			BT_INFO("OOM");
+			release_firmware(fw);
+			return;
+		}
+		fwcache->size = fw->size;
+		memcpy(fwcache->data, fw->data, fwcache->size);
+		release_firmware(fw);
 	}
 
 	if (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
@@ -977,12 +1006,12 @@ static inline void load_patchram_fw(struct usb_device *udev, const struct usb_de
 	}
 	msleep(300);
 
-	while (pos < fw->size) {
+	while (pos < fwcache->size) {
 		size_t len;
-		len = fw->data[pos + 2] + 3;
-		if ((pos + len > fw->size) ||
+		len = fwcache->data[pos + 2] + 3;
+		if ((pos + len > fwcache->size) ||
 			(usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0,
-			USB_TYPE_CLASS, 0, 0, (void *)fw->data + pos, len,
+			USB_TYPE_CLASS, 0, 0, (void *)(fwcache->data + pos), len,
 			PATCHRAM_TIMEOUT) < 0)) {
 			err = -1;
 			goto out;
@@ -995,7 +1024,6 @@ static inline void load_patchram_fw(struct usb_device *udev, const struct usb_de
 out:
 	if (err)
 		BT_INFO("fail to load firmware, may not work correctly");
-	release_firmware(fw);
 }
 
 static int btusb_probe(struct usb_interface *intf,
@@ -1326,7 +1354,22 @@ static struct usb_driver btusb_driver = {
 	.disable_hub_initiated_lpm = 1,
 };
 
-module_usb_driver(btusb_driver);
+static int __init btusb_init(void)
+{
+	return usb_register(&btusb_driver);
+}
+
+static void __exit btusb_exit(void)
+{
+	int i;
+	for (i = 0; i < sizeof(firmware) / sizeof(firmware[0]); i++)
+		if (firmware[i].data)
+			kfree(firmware[i].data);
+	usb_deregister(&btusb_driver);
+}
+
+module_init(btusb_init);
+module_exit(btusb_exit);
 
 module_param(ignore_dga, bool, 0644);
 MODULE_PARM_DESC(ignore_dga, "Ignore devices with id 08fd:0001");
-- 
1.7.9.5

Re: [PATCH 1/2] Implement broadcom patchram firmware loader

[Marcel Holtmann]

Hi Jesse,

> From: Wen-chien Jesse Sung <jesse.sung@canonical.com>
> 
> 
> Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>

please learn on how to write commit messages. I want a full blown commit
messages with /sys/kernel/debug/usb/devices output (before and after)
and something that explains what is actually done.

> ---
>  drivers/bluetooth/btusb.c |  103 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 99 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 654e248..7189fed 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -23,6 +23,8 @@
>  
>  #include <linux/module.h>
>  #include <linux/usb.h>
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
>  
>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
> @@ -47,6 +49,7 @@ static struct usb_driver btusb_driver;
>  #define BTUSB_BROKEN_ISOC	0x20
>  #define BTUSB_WRONG_SCO_MTU	0x40
>  #define BTUSB_ATH3012		0x80
> +#define BTUSB_BCM_PATCHRAM	0x100
>  
>  static struct usb_device_id btusb_table[] = {
>  	/* Generic Bluetooth USB device */
> @@ -96,14 +99,15 @@ static struct usb_device_id btusb_table[] = {
>  	{ USB_DEVICE(0x0c10, 0x0000) },
>  
>  	/* Broadcom BCM20702A0 */
> +	{ USB_DEVICE(0x0489, 0xe031), .driver_info = BTUSB_BCM_PATCHRAM },
>  	{ USB_DEVICE(0x0489, 0xe042) },
> -	{ USB_DEVICE(0x413c, 0x8197) },
> +	{ USB_DEVICE(0x413c, 0x8197), .driver_info = BTUSB_BCM_PATCHRAM },

These drivers did work without firmware before. So why is this change
required? Or is that Broadcom wide?
 
>  	/* Foxconn - Hon Hai */
>  	{ USB_DEVICE(0x0489, 0xe033) },
>  
>  	/*Broadcom devices with vendor specific id */
> -	{ USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01) },
> +	{ USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01), .driver_info = BTUSB_BCM_PATCHRAM },
>  
>  	{ }	/* Terminating entry */
>  };
> @@ -197,6 +201,37 @@ static struct usb_device_id blacklist_table[] = {
>  	{ }	/* Terminating entry */
>  };
>  
> +#define PATCHRAM_TIMEOUT	1000
> +#define FW_0489_E031		"fw-0489_e031.hcd"
> +#define FW_0A5C_21D3		"fw-0a5c_21d3.hcd"
> +#define FW_0A5C_21D7		"fw-0a5c_21d7.hcd"
> +#define FW_0A5C_21E6		"fw-0a5c_21e6.hcd"
> +#define FW_0A5C_21F3		"fw-0a5c_21f3.hcd"
> +#define FW_0A5C_21F4		"fw-0a5c_21f4.hcd"
> +#define FW_413C_8197		"fw-413c_8197.hcd"
> +
> +MODULE_FIRMWARE(FW_0489_E031);
> +MODULE_FIRMWARE(FW_0A5C_21D3);
> +MODULE_FIRMWARE(FW_0A5C_21D7);
> +MODULE_FIRMWARE(FW_0A5C_21E6);
> +MODULE_FIRMWARE(FW_0A5C_21F3);
> +MODULE_FIRMWARE(FW_0A5C_21F4);
> +MODULE_FIRMWARE(FW_413C_8197);
> +
> +static struct usb_device_id patchram_table[] = {
> +	/* Dell DW1704 */
> +	{ USB_DEVICE(0x0a5c, 0x21d3), .driver_info = (kernel_ulong_t) FW_0A5C_21D3 },
> +	{ USB_DEVICE(0x0a5c, 0x21d7), .driver_info = (kernel_ulong_t) FW_0A5C_21D7 },
> +	/* Dell DW380 */
> +	{ USB_DEVICE(0x413c, 0x8197), .driver_info = (kernel_ulong_t) FW_413C_8197 },
> +	/* FoxConn Hon Hai */
> +	{ USB_DEVICE(0x0489, 0xe031), .driver_info = (kernel_ulong_t) FW_0489_E031 },
> +	/* Lenovo */
> +	{ USB_DEVICE(0x0a5c, 0x21e6), .driver_info = (kernel_ulong_t) FW_0A5C_21E6 },
> +	{ USB_DEVICE(0x0a5c, 0x21f3), .driver_info = (kernel_ulong_t) FW_0A5C_21F3 },
> +	{ USB_DEVICE(0x0a5c, 0x21f4), .driver_info = (kernel_ulong_t) FW_0A5C_21F4 },
> +};
> +

And this looks like totally wasted details to me. Either build the
firmware name from USB VID:PID or only include the ones that we are
actually supporting.

>  #define BTUSB_MAX_ISOC_FRAMES	10
>  
>  #define BTUSB_INTR_RUNNING	0
> @@ -914,6 +949,55 @@ static void btusb_waker(struct work_struct *work)
>  	usb_autopm_put_interface(data->intf);
>  }
>  
> +static inline void load_patchram_fw(struct usb_device *udev, const struct usb_device_id *id)
> +{
> +	size_t pos = 0;
> +	int err = 0;
> +	const struct firmware *fw;
> +
> +	unsigned char reset_cmd[] = { 0x03, 0x0c, 0x00 };
> +	unsigned char download_cmd[] = { 0x2e, 0xfc, 0x00 };
> +
> +	if (request_firmware(&fw, (const char *) id->driver_info, &udev->dev) < 0) {
> +		BT_INFO("can't load firmware, may not work correctly");
> +		return;
> +	}
> +
> +	if (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
> +		reset_cmd, sizeof(reset_cmd), PATCHRAM_TIMEOUT) < 0) {
> +		err = -1;
> +		goto out;
> +	}
> +	msleep(300);
> +
> +	if (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
> +		download_cmd, sizeof(download_cmd), PATCHRAM_TIMEOUT) < 0) {
> +		err = -1;
> +		goto out;
> +	}
> +	msleep(300);
> +
> +	while (pos < fw->size) {
> +		size_t len;
> +		len = fw->data[pos + 2] + 3;
> +		if ((pos + len > fw->size) ||
> +			(usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0,
> +			USB_TYPE_CLASS, 0, 0, (void *)fw->data + pos, len,
> +			PATCHRAM_TIMEOUT) < 0)) {
> +			err = -1;
> +			goto out;
> +		}
> +		pos += len;
> +	}
> +
> +	err = (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
> +		reset_cmd, sizeof(reset_cmd), PATCHRAM_TIMEOUT) < 0);
> +out:
> +	if (err)
> +		BT_INFO("fail to load firmware, may not work correctly");
> +	release_firmware(fw);
> +}
> +
>  static int btusb_probe(struct usb_interface *intf,
>  				const struct usb_device_id *id)
>  {
> @@ -1078,15 +1162,26 @@ static int btusb_probe(struct usb_interface *intf,
>  		}
>  	}
>  
> +	usb_set_intfdata(intf, data);
> +
> +	if (id->driver_info & BTUSB_BCM_PATCHRAM) {
> +		const struct usb_device_id *match;
> +		match = usb_match_id(intf, patchram_table);
> +		if (match) {
> +			btusb_open(hdev);
> +			load_patchram_fw(interface_to_usbdev(intf), match);
> +			btusb_close(hdev);
> +		}
> +	}
> +

So we are now blocking every other USB devices on that bus here? I
actually do not like this idea very much.

Also the call of btusb_open() before hdev is actually registered is
kinda fishy to me. I am not even sure that works how you think it would.

And why can't Broadcom just change the PID once the patchram has been
loaded to something else. That way we can nicely iterate through this.

This also does not really belong in a standard driver. Quirks fine, but
a complete ROM patches procedure that is vendor specific.

As I said above, I want to see the /sys/kernel/debug/usb/devices from
before and after first. Until then NAK.

Regards

Marcel

Re: [PATCH 1/2] Implement broadcom patchram firmware loader

[Jesse Sung]

Hi Marcel,

2012/9/1 Marcel Holtmann <marcel@holtmann.org>:
> Hi Jesse,
>
>> From: Wen-chien Jesse Sung <jesse.sung@canonical.com>
>>
>>
>> Signed-off-by: Wen-chien Jesse Sung <jesse.sung@canonical.com>
>
> please learn on how to write commit messages. I want a full blown commit
> messages with /sys/kernel/debug/usb/devices output (before and after)
> and something that explains what is actually done.

Sorry, I'll try to do that better next time.

>> ---
>>  drivers/bluetooth/btusb.c |  103 +++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 99 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 654e248..7189fed 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -23,6 +23,8 @@
>>
>>  #include <linux/module.h>
>>  #include <linux/usb.h>
>> +#include <linux/delay.h>
>> +#include <linux/firmware.h>
>>
>>  #include <net/bluetooth/bluetooth.h>
>>  #include <net/bluetooth/hci_core.h>
>> @@ -47,6 +49,7 @@ static struct usb_driver btusb_driver;
>>  #define BTUSB_BROKEN_ISOC    0x20
>>  #define BTUSB_WRONG_SCO_MTU  0x40
>>  #define BTUSB_ATH3012                0x80
>> +#define BTUSB_BCM_PATCHRAM   0x100
>>
>>  static struct usb_device_id btusb_table[] = {
>>       /* Generic Bluetooth USB device */
>> @@ -96,14 +99,15 @@ static struct usb_device_id btusb_table[] = {
>>       { USB_DEVICE(0x0c10, 0x0000) },
>>
>>       /* Broadcom BCM20702A0 */
>> +     { USB_DEVICE(0x0489, 0xe031), .driver_info = BTUSB_BCM_PATCHRAM },
>>       { USB_DEVICE(0x0489, 0xe042) },
>> -     { USB_DEVICE(0x413c, 0x8197) },
>> +     { USB_DEVICE(0x413c, 0x8197), .driver_info = BTUSB_BCM_PATCHRAM },
>
> These drivers did work without firmware before. So why is this change
> required? Or is that Broadcom wide?

Currently patchram applies to BCM43142 and BCM20702 modules. Dunno if
there will be
more or not.

Some of these modules have PROM or something like that holding the
needed images.
For them, adding IDs to this table is enough for them to work. But as
what we have found,
these images may have bugs and need an updated image to be loaded to
fix the bugs.
Since failed to load firmware may not trigger an error while doing
probe, these kind
of modules will still work even if there's no firmware provided.

For other modules that do not have images with them, patchram is
needed to bring them
into a working state.

>>       /* Foxconn - Hon Hai */
>>       { USB_DEVICE(0x0489, 0xe033) },
>>
>>       /*Broadcom devices with vendor specific id */
>> -     { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01) },
>> +     { USB_VENDOR_AND_INTERFACE_INFO(0x0a5c, 0xff, 0x01, 0x01), .driver_info = BTUSB_BCM_PATCHRAM },
>>
>>       { }     /* Terminating entry */
>>  };
>> @@ -197,6 +201,37 @@ static struct usb_device_id blacklist_table[] = {
>>       { }     /* Terminating entry */
>>  };
>>
>> +#define PATCHRAM_TIMEOUT     1000
>> +#define FW_0489_E031         "fw-0489_e031.hcd"
>> +#define FW_0A5C_21D3         "fw-0a5c_21d3.hcd"
>> +#define FW_0A5C_21D7         "fw-0a5c_21d7.hcd"
>> +#define FW_0A5C_21E6         "fw-0a5c_21e6.hcd"
>> +#define FW_0A5C_21F3         "fw-0a5c_21f3.hcd"
>> +#define FW_0A5C_21F4         "fw-0a5c_21f4.hcd"
>> +#define FW_413C_8197         "fw-413c_8197.hcd"
>> +
>> +MODULE_FIRMWARE(FW_0489_E031);
>> +MODULE_FIRMWARE(FW_0A5C_21D3);
>> +MODULE_FIRMWARE(FW_0A5C_21D7);
>> +MODULE_FIRMWARE(FW_0A5C_21E6);
>> +MODULE_FIRMWARE(FW_0A5C_21F3);
>> +MODULE_FIRMWARE(FW_0A5C_21F4);
>> +MODULE_FIRMWARE(FW_413C_8197);
>> +
>> +static struct usb_device_id patchram_table[] = {
>> +     /* Dell DW1704 */
>> +     { USB_DEVICE(0x0a5c, 0x21d3), .driver_info = (kernel_ulong_t) FW_0A5C_21D3 },
>> +     { USB_DEVICE(0x0a5c, 0x21d7), .driver_info = (kernel_ulong_t) FW_0A5C_21D7 },
>> +     /* Dell DW380 */
>> +     { USB_DEVICE(0x413c, 0x8197), .driver_info = (kernel_ulong_t) FW_413C_8197 },
>> +     /* FoxConn Hon Hai */
>> +     { USB_DEVICE(0x0489, 0xe031), .driver_info = (kernel_ulong_t) FW_0489_E031 },
>> +     /* Lenovo */
>> +     { USB_DEVICE(0x0a5c, 0x21e6), .driver_info = (kernel_ulong_t) FW_0A5C_21E6 },
>> +     { USB_DEVICE(0x0a5c, 0x21f3), .driver_info = (kernel_ulong_t) FW_0A5C_21F3 },
>> +     { USB_DEVICE(0x0a5c, 0x21f4), .driver_info = (kernel_ulong_t) FW_0A5C_21F4 },
>> +};
>> +
>
> And this looks like totally wasted details to me. Either build the
> firmware name from USB VID:PID or only include the ones that we are
> actually supporting.

I'll get the firmware name from VID and PID instead.

>>  #define BTUSB_MAX_ISOC_FRAMES        10
>>
>>  #define BTUSB_INTR_RUNNING   0
>> @@ -914,6 +949,55 @@ static void btusb_waker(struct work_struct *work)
>>       usb_autopm_put_interface(data->intf);
>>  }
>>
>> +static inline void load_patchram_fw(struct usb_device *udev, const struct usb_device_id *id)
>> +{
>> +     size_t pos = 0;
>> +     int err = 0;
>> +     const struct firmware *fw;
>> +
>> +     unsigned char reset_cmd[] = { 0x03, 0x0c, 0x00 };
>> +     unsigned char download_cmd[] = { 0x2e, 0xfc, 0x00 };
>> +
>> +     if (request_firmware(&fw, (const char *) id->driver_info, &udev->dev) < 0) {
>> +             BT_INFO("can't load firmware, may not work correctly");
>> +             return;
>> +     }
>> +
>> +     if (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
>> +             reset_cmd, sizeof(reset_cmd), PATCHRAM_TIMEOUT) < 0) {
>> +             err = -1;
>> +             goto out;
>> +     }
>> +     msleep(300);
>> +
>> +     if (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
>> +             download_cmd, sizeof(download_cmd), PATCHRAM_TIMEOUT) < 0) {
>> +             err = -1;
>> +             goto out;
>> +     }
>> +     msleep(300);
>> +
>> +     while (pos < fw->size) {
>> +             size_t len;
>> +             len = fw->data[pos + 2] + 3;
>> +             if ((pos + len > fw->size) ||
>> +                     (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0,
>> +                     USB_TYPE_CLASS, 0, 0, (void *)fw->data + pos, len,
>> +                     PATCHRAM_TIMEOUT) < 0)) {
>> +                     err = -1;
>> +                     goto out;
>> +             }
>> +             pos += len;
>> +     }
>> +
>> +     err = (usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0, USB_TYPE_CLASS, 0, 0,
>> +             reset_cmd, sizeof(reset_cmd), PATCHRAM_TIMEOUT) < 0);
>> +out:
>> +     if (err)
>> +             BT_INFO("fail to load firmware, may not work correctly");
>> +     release_firmware(fw);
>> +}
>> +
>>  static int btusb_probe(struct usb_interface *intf,
>>                               const struct usb_device_id *id)
>>  {
>> @@ -1078,15 +1162,26 @@ static int btusb_probe(struct usb_interface *intf,
>>               }
>>       }
>>
>> +     usb_set_intfdata(intf, data);
>> +
>> +     if (id->driver_info & BTUSB_BCM_PATCHRAM) {
>> +             const struct usb_device_id *match;
>> +             match = usb_match_id(intf, patchram_table);
>> +             if (match) {
>> +                     btusb_open(hdev);
>> +                     load_patchram_fw(interface_to_usbdev(intf), match);
>> +                     btusb_close(hdev);
>> +             }
>> +     }
>> +
>
> So we are now blocking every other USB devices on that bus here? I
> actually do not like this idea very much.

Do you mean the usleep() in load_patchram_fw()? These only affects people
who have this kind of device, so the impact should be limited.

> Also the call of btusb_open() before hdev is actually registered is
> kinda fishy to me. I am not even sure that works how you think it would.

Humm.. If there are concerns about calling btusb_open() and btusb_close(),
I'll try to find another way to hook usb callbacks.

> And why can't Broadcom just change the PID once the patchram has been
> loaded to something else. That way we can nicely iterate through this.

Unfortunately that's how they do it now, and these modules are already in
users' machines.

> This also does not really belong in a standard driver. Quirks fine, but
> a complete ROM patches procedure that is vendor specific.
>
> As I said above, I want to see the /sys/kernel/debug/usb/devices from
> before and after first. Until then NAK.

before:
T:  Bus=01 Lev=02 Prnt=02 Port=04 Cnt=02 Dev#=  4 Spd=12   MxCh= 0
D:  Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=0a5c ProdID=21d3 Rev= 1.12
S:  Manufacturer=Broadcom Corp
S:  Product=BCM43142A0
S:  SerialNumber=3859F9D6199A
C:* #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr=  0mA
I:* If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=01 Prot=01 Driver=(none)
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=(none)
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=(none)
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=(none)
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=(none)
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=(none)
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=(none)
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)

after:
T:  Bus=01 Lev=02 Prnt=02 Port=04 Cnt=02 Dev#=  4 Spd=12   MxCh= 0
D:  Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=0a5c ProdID=21d3 Rev= 1.12
S:  Manufacturer=Broadcom Corp
S:  Product=BCM43142A0
S:  SerialNumber=3859F9D6199A
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)

Thanks,
Jesse

Re: [PATCH 1/2] Implement broadcom patchram firmware loader

[Marcel Holtmann]

Hi Jesse,

> >>  static int btusb_probe(struct usb_interface *intf,
> >>                               const struct usb_device_id *id)
> >>  {
> >> @@ -1078,15 +1162,26 @@ static int btusb_probe(struct usb_interface *intf,
> >>               }
> >>       }
> >>
> >> +     usb_set_intfdata(intf, data);
> >> +
> >> +     if (id->driver_info & BTUSB_BCM_PATCHRAM) {
> >> +             const struct usb_device_id *match;
> >> +             match = usb_match_id(intf, patchram_table);
> >> +             if (match) {
> >> +                     btusb_open(hdev);
> >> +                     load_patchram_fw(interface_to_usbdev(intf), match);
> >> +                     btusb_close(hdev);
> >> +             }
> >> +     }
> >> +
> >
> > So we are now blocking every other USB devices on that bus here? I
> > actually do not like this idea very much.
> 
> Do you mean the usleep() in load_patchram_fw()? These only affects people
> who have this kind of device, so the impact should be limited.

and what about the global USB lock that is hold when probing devices.

> > Also the call of btusb_open() before hdev is actually registered is
> > kinda fishy to me. I am not even sure that works how you think it would.
> 
> Humm.. If there are concerns about calling btusb_open() and btusb_close(),
> I'll try to find another way to hook usb callbacks.

They are callbacks exposed from hci_dev and not for direct calling.

I have been telling this before multiple times. If you require a
firmware patching stage, then integrate that into hci_dev directly and
use the core for driving this properly.

> > And why can't Broadcom just change the PID once the patchram has been
> > loaded to something else. That way we can nicely iterate through this.
> 
> Unfortunately that's how they do it now, and these modules are already in
> users' machines.

Some companies never ever learn. Most funny is that we had the bcm203x
driver that was doing this correctly. And 8 years later they have
forgotten all about it.

Regards

Marcel