[PATCH v5] Bluetooth: Add support for QCA ROME chipset family

[PATCH v5] Bluetooth: Add support for QCA ROME chipset family

[Kim, Ben Young Tae]

This patch supports ROME Bluetooth family from Qualcomm Atheros,
e.g. QCA61x4 or QCA6574.

New chipset have similar firmware downloading sequences to previous
chipset from Atheros, however, it doesn't support vid/pid switching
after downloading the patch so that firmware needs to be handled by
btusb module directly.

ROME chipset can be differentiated from previous version by reading
ROM version.

T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 16 Spd=12   MxCh= 0
D:  Ver= 1.10 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=0cf3 ProdID=e300 Rev= 0.01
C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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

T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#=  8 Spd=12   MxCh= 0
D:  Ver= 2.01 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=0cf3 ProdID=e360 Rev= 0.01
C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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

Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
---
 drivers/bluetooth/btusb.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 265 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index b876888..45fe262 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -52,6 +52,7 @@ static struct usb_driver btusb_driver;
 #define BTUSB_SWAVE		0x1000
 #define BTUSB_INTEL_NEW		0x2000
 #define BTUSB_AMP		0x4000
+#define BTUSB_QCA_ROME		0x8000
 
 static const struct usb_device_id btusb_table[] = {
 	/* Generic Bluetooth USB device */
@@ -212,6 +213,10 @@ static const struct usb_device_id blacklist_table[] = {
 	{ USB_DEVICE(0x0489, 0xe036), .driver_info = BTUSB_ATH3012 },
 	{ USB_DEVICE(0x0489, 0xe03c), .driver_info = BTUSB_ATH3012 },
 
+	/* QCA ROME chipset */
+	{ USB_DEVICE(0x0cf3, 0xe300), .driver_info = BTUSB_QCA_ROME},
+	{ USB_DEVICE(0x0cf3, 0xe360), .driver_info = BTUSB_QCA_ROME},
+
 	/* Broadcom BCM2035 */
 	{ USB_DEVICE(0x0a5c, 0x2009), .driver_info = BTUSB_BCM92035 },
 	{ USB_DEVICE(0x0a5c, 0x200a), .driver_info = BTUSB_WRONG_SCO_MTU },
@@ -336,6 +341,8 @@ struct btusb_data {
 
 	int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
 	int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
+
+	int (*setup_patch_usb)(struct hci_dev *hdev);
 };
 
 static int btusb_wait_on_bit_timeout(void *word, int bit, unsigned long timeout,
@@ -887,6 +894,15 @@ static int btusb_open(struct hci_dev *hdev)
 
 	BT_DBG("%s", hdev->name);
 
+	/* Patching USB firmware files prior to starting any URBs of HCI path
+	 * It is more safe to use USB bulk channel for downloading USB patch
+	 */
+	if (data->setup_patch_usb) {
+		err = data->setup_patch_usb(hdev);
+		if (err <0)
+			return err;
+	}
+
 	err = usb_autopm_get_interface(data->intf);
 	if (err < 0)
 		return err;
@@ -2584,6 +2600,250 @@ static int btusb_set_bdaddr_ath3012(struct hci_dev *hdev,
 	return 0;
 }
 
+#define QCA_DFU_PACKET_LEN	4096
+
+#define QCA_GET_TARGET_VERSION	0x09
+#define QCA_CHECK_STATUS	0x05
+#define QCA_DFU_DOWNLOAD	0x01
+
+#define QCA_SYSCFG_UPDATED	0x40
+#define QCA_PATCH_UPDATED	0x80
+#define QCA_DFU_TIMEOUT		3000
+
+struct qca_version {
+	__le32	rom_version;
+	__le32	patch_version;
+	__le32	ram_version;
+	__le32	ref_clock;
+	__u8	reserved[4];
+} __packed;
+
+struct qca_rampatch_version {
+	__le16	rom_version;
+	__le16	patch_version;
+} __packed;
+
+struct qca_device_info {
+	__le32	rom_version;
+	__u8	rampatch_hdr;	/* length of header in rampatch */
+	__u8	nvm_hdr;	/* length of header in NVM */
+	__u8	ver_offset;	/* offset of version structure in rampatch */
+};
+
+static const struct qca_device_info qca_devices_table[] = {
+	{ 0x00000100, 20, 4, 10 }, /* Rome 1.0 */
+	{ 0x00000101, 20, 4, 10 }, /* Rome 1.1 */
+	{ 0x00000201, 28, 4, 18 }, /* Rome 2.1 */
+	{ 0x00000300, 28, 4, 18 }, /* Rome 3.0 */
+	{ 0x00000302, 28, 4, 18 }, /* Rome 3.2 */
+};
+
+static int btusb_qca_send_vendor_req(struct hci_dev *hdev, u8 request,
+				     void *data, u16 size)
+{
+	struct btusb_data *btdata = hci_get_drvdata(hdev);
+	struct usb_device *udev = btdata->udev;
+	int pipe, err;
+	u8 *buf;
+
+	buf = kmalloc(size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	/* Found some of USB hosts have IOT issues with ours so that we should
+	 * not wait until HCI layer is ready.
+	 */
+	pipe = usb_rcvctrlpipe(udev, 0);
+	err = usb_control_msg(udev, pipe, request, USB_TYPE_VENDOR | USB_DIR_IN,
+			      0, 0, buf, size, USB_CTRL_SET_TIMEOUT);
+	if (err < 0) {
+		BT_ERR("%s: Failed to access otp area (%d)", hdev->name, err);
+		goto done;
+	}
+
+	memcpy(data, buf, size);
+
+done:
+	kfree(buf);
+
+	return err;
+}
+
+static int btusb_setup_qca_download_fw(struct hci_dev *hdev,
+				       const struct firmware *firmware,
+				       size_t hdr_size)
+{
+	struct btusb_data *btdata = hci_get_drvdata(hdev);
+	struct usb_device *udev = btdata->udev;
+	size_t count, size, sent = 0;
+	int pipe, len, err;
+	u8 *buf;
+
+	buf = kmalloc(QCA_DFU_PACKET_LEN, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	count = firmware->size;
+
+	size = min_t(size_t, count, hdr_size);
+	memcpy(buf, firmware->data, size);
+
+	/* USB patches should go down to controller through USB path
+	 * because binary format fits to go down through USB channel.
+	 * USB control path is for patching headers and USB bulk is for
+	 * patch body.
+	 */
+	pipe = usb_sndctrlpipe(udev, 0);
+	err = usb_control_msg(udev, pipe, QCA_DFU_DOWNLOAD, USB_TYPE_VENDOR,
+			      0, 0, buf, size, USB_CTRL_SET_TIMEOUT);
+	if (err < 0) {
+		BT_ERR("%s: Failed to send headers (%d)", hdev->name, err);
+		goto done;
+	}
+
+	sent += size;
+	count -= size;
+
+	while (count) {
+		size = min_t(size_t, count, QCA_DFU_PACKET_LEN);
+
+		memcpy(buf, firmware->data + sent, size);
+
+		pipe = usb_sndbulkpipe(udev, 0x02);
+		err = usb_bulk_msg(udev, pipe, buf, size, &len,
+				   QCA_DFU_TIMEOUT);
+		if (err < 0) {
+			BT_ERR("%s: Failed to send body at %zd of %zd (%d)",
+			       hdev->name, sent, firmware->size, err);
+			break;
+		}
+
+		if (size != len) {
+			BT_ERR("%s: Failed to get bulk buffer", hdev->name);
+			err = -EILSEQ;
+			break;
+		}
+
+		sent  += size;
+		count -= size;
+	}
+
+done:
+	kfree(buf);
+	return err;
+}
+
+static int btusb_setup_qca_load_rampatch(struct hci_dev *hdev,
+					 struct qca_version *ver,
+					 const struct qca_device_info *info)
+{
+	struct qca_rampatch_version *rver;
+	const struct firmware *fw;
+	char fwname[64];
+	int err;
+
+	snprintf(fwname, sizeof(fwname), "qca/rampatch_usb_%08x.bin",
+		 le32_to_cpu(ver->rom_version));
+
+	err = request_firmware(&fw, fwname, &hdev->dev);
+	if (err) {
+		BT_ERR("%s: failed to request rampatch file: %s (%d)",
+		       hdev->name, fwname, err);
+		return err;
+	}
+
+	BT_INFO("%s: using rampatch file: %s", hdev->name, fwname);
+	rver = (struct qca_rampatch_version *)(fw->data + info->ver_offset);
+	BT_INFO("%s: QCA: patch rome 0x%x build 0x%x, firmware rome 0x%x "
+		"build 0x%x", hdev->name, le16_to_cpu(rver->rom_version),
+		le16_to_cpu(rver->patch_version), le32_to_cpu(ver->rom_version),
+		le32_to_cpu(ver->patch_version));
+
+	if (rver->rom_version != ver->rom_version ||
+	    rver->patch_version <= ver->patch_version) {
+		BT_ERR("%s: rampatch file version did not match with firmware",
+		       hdev->name);
+		err = -EINVAL;
+		goto done;
+	}
+
+	err = btusb_setup_qca_download_fw(hdev, fw, info->rampatch_hdr);
+
+done:
+	release_firmware(fw);
+
+	return err;
+}
+
+static int btusb_setup_qca_load_nvm(struct hci_dev *hdev,
+				    struct qca_version *ver,
+				    const struct qca_device_info *info)
+{
+	const struct firmware *fw;
+	char fwname[64];
+	int err;
+
+	snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
+		 le32_to_cpu(ver->rom_version));
+
+	err = request_firmware(&fw, fwname, &hdev->dev);
+	if (err) {
+		BT_ERR("%s: failed to request NVM file: %s (%d)",
+		       hdev->name, fwname, err);
+		return err;
+	}
+
+	BT_INFO("%s: using NVM file: %s", hdev->name, fwname);
+
+	err = btusb_setup_qca_download_fw(hdev, fw, info->nvm_hdr);
+
+	release_firmware(fw);
+
+	return err;
+}
+
+static int btusb_setup_qca(struct hci_dev *hdev)
+{
+	const struct qca_device_info *info = NULL;
+	struct qca_version ver;
+	u8 status;
+	int i, err;
+
+	err = btusb_qca_send_vendor_req(hdev, QCA_GET_TARGET_VERSION, &ver,
+				        sizeof(ver));
+	if (err < 0)
+		return err;
+
+	for (i = 0; i < ARRAY_SIZE(qca_devices_table); i++) {
+		if (ver.rom_version == qca_devices_table[i].rom_version)
+			info = &qca_devices_table[i];
+	}
+	if (!info) {
+		BT_ERR("%s: don't support firmware rome 0x%x", hdev->name,
+		       le32_to_cpu(ver.rom_version));
+		return -ENODEV;
+	}
+
+	err = btusb_qca_send_vendor_req(hdev, QCA_CHECK_STATUS, &status,
+					sizeof(status));
+	if (err < 0)
+		return err;
+
+	if (!(status & QCA_PATCH_UPDATED)) {
+		err = btusb_setup_qca_load_rampatch(hdev, &ver, info);
+		if (err < 0)
+			return err;
+	}
+
+	if (!(status & QCA_SYSCFG_UPDATED)) {
+		err = btusb_setup_qca_load_nvm(hdev, &ver, info);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
 static int btusb_probe(struct usb_interface *intf,
 		       const struct usb_device_id *id)
 {
@@ -2736,6 +2996,11 @@ static int btusb_probe(struct usb_interface *intf,
 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
 	}
 
+	if (id->driver_info & BTUSB_QCA_ROME) {
+		data->setup_patch_usb = btusb_setup_qca;
+		hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
+	}
+
 	if (id->driver_info & BTUSB_AMP) {
 		/* AMP controllers do not support SCO packets */
 		data->isoc = NULL;
-- 
1.9.1

Re: [PATCH v5] Bluetooth: Add support for QCA ROME chipset family

[Kim, Ben Young Tae]

PING..

On Feb 5, 2015, at 10:40 AM, Kim, Ben Young Tae <ytkim@qca.qualcomm.com> wrote:

> This patch supports ROME Bluetooth family from Qualcomm Atheros,
> e.g. QCA61x4 or QCA6574.
> 
> New chipset have similar firmware downloading sequences to previous
> chipset from Atheros, however, it doesn't support vid/pid switching
> after downloading the patch so that firmware needs to be handled by
> btusb module directly.
> 
> ROME chipset can be differentiated from previous version by reading
> ROM version.
> 
> T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 16 Spd=12   MxCh= 0
> D:  Ver= 1.10 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
> P:  Vendor=0cf3 ProdID=e300 Rev= 0.01
> C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
> I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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
> 
> T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#=  8 Spd=12   MxCh= 0
> D:  Ver= 2.01 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
> P:  Vendor=0cf3 ProdID=e360 Rev= 0.01
> C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
> I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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
> 
> Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
> ---
> drivers/bluetooth/btusb.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 265 insertions(+)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index b876888..45fe262 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -52,6 +52,7 @@ static struct usb_driver btusb_driver;
> #define BTUSB_SWAVE		0x1000
> #define BTUSB_INTEL_NEW		0x2000
> #define BTUSB_AMP		0x4000
> +#define BTUSB_QCA_ROME		0x8000
> 
> static const struct usb_device_id btusb_table[] = {
> 	/* Generic Bluetooth USB device */
> @@ -212,6 +213,10 @@ static const struct usb_device_id blacklist_table[] = {
> 	{ USB_DEVICE(0x0489, 0xe036), .driver_info = BTUSB_ATH3012 },
> 	{ USB_DEVICE(0x0489, 0xe03c), .driver_info = BTUSB_ATH3012 },
> 
> +	/* QCA ROME chipset */
> +	{ USB_DEVICE(0x0cf3, 0xe300), .driver_info = BTUSB_QCA_ROME},
> +	{ USB_DEVICE(0x0cf3, 0xe360), .driver_info = BTUSB_QCA_ROME},
> +
> 	/* Broadcom BCM2035 */
> 	{ USB_DEVICE(0x0a5c, 0x2009), .driver_info = BTUSB_BCM92035 },
> 	{ USB_DEVICE(0x0a5c, 0x200a), .driver_info = BTUSB_WRONG_SCO_MTU },
> @@ -336,6 +341,8 @@ struct btusb_data {
> 
> 	int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> 	int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> +
> +	int (*setup_patch_usb)(struct hci_dev *hdev);
> };
> 
> static int btusb_wait_on_bit_timeout(void *word, int bit, unsigned long timeout,
> @@ -887,6 +894,15 @@ static int btusb_open(struct hci_dev *hdev)
> 
> 	BT_DBG("%s", hdev->name);
> 
> +	/* Patching USB firmware files prior to starting any URBs of HCI path
> +	 * It is more safe to use USB bulk channel for downloading USB patch
> +	 */
> +	if (data->setup_patch_usb) {
> +		err = data->setup_patch_usb(hdev);
> +		if (err <0)
> +			return err;
> +	}
> +
> 	err = usb_autopm_get_interface(data->intf);
> 	if (err < 0)
> 		return err;
> @@ -2584,6 +2600,250 @@ static int btusb_set_bdaddr_ath3012(struct hci_dev *hdev,
> 	return 0;
> }
> 
> +#define QCA_DFU_PACKET_LEN	4096
> +
> +#define QCA_GET_TARGET_VERSION	0x09
> +#define QCA_CHECK_STATUS	0x05
> +#define QCA_DFU_DOWNLOAD	0x01
> +
> +#define QCA_SYSCFG_UPDATED	0x40
> +#define QCA_PATCH_UPDATED	0x80
> +#define QCA_DFU_TIMEOUT		3000
> +
> +struct qca_version {
> +	__le32	rom_version;
> +	__le32	patch_version;
> +	__le32	ram_version;
> +	__le32	ref_clock;
> +	__u8	reserved[4];
> +} __packed;
> +
> +struct qca_rampatch_version {
> +	__le16	rom_version;
> +	__le16	patch_version;
> +} __packed;
> +
> +struct qca_device_info {
> +	__le32	rom_version;
> +	__u8	rampatch_hdr;	/* length of header in rampatch */
> +	__u8	nvm_hdr;	/* length of header in NVM */
> +	__u8	ver_offset;	/* offset of version structure in rampatch */
> +};
> +
> +static const struct qca_device_info qca_devices_table[] = {
> +	{ 0x00000100, 20, 4, 10 }, /* Rome 1.0 */
> +	{ 0x00000101, 20, 4, 10 }, /* Rome 1.1 */
> +	{ 0x00000201, 28, 4, 18 }, /* Rome 2.1 */
> +	{ 0x00000300, 28, 4, 18 }, /* Rome 3.0 */
> +	{ 0x00000302, 28, 4, 18 }, /* Rome 3.2 */
> +};
> +
> +static int btusb_qca_send_vendor_req(struct hci_dev *hdev, u8 request,
> +				     void *data, u16 size)
> +{
> +	struct btusb_data *btdata = hci_get_drvdata(hdev);
> +	struct usb_device *udev = btdata->udev;
> +	int pipe, err;
> +	u8 *buf;
> +
> +	buf = kmalloc(size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* Found some of USB hosts have IOT issues with ours so that we should
> +	 * not wait until HCI layer is ready.
> +	 */
> +	pipe = usb_rcvctrlpipe(udev, 0);
> +	err = usb_control_msg(udev, pipe, request, USB_TYPE_VENDOR | USB_DIR_IN,
> +			      0, 0, buf, size, USB_CTRL_SET_TIMEOUT);
> +	if (err < 0) {
> +		BT_ERR("%s: Failed to access otp area (%d)", hdev->name, err);
> +		goto done;
> +	}
> +
> +	memcpy(data, buf, size);
> +
> +done:
> +	kfree(buf);
> +
> +	return err;
> +}
> +
> +static int btusb_setup_qca_download_fw(struct hci_dev *hdev,
> +				       const struct firmware *firmware,
> +				       size_t hdr_size)
> +{
> +	struct btusb_data *btdata = hci_get_drvdata(hdev);
> +	struct usb_device *udev = btdata->udev;
> +	size_t count, size, sent = 0;
> +	int pipe, len, err;
> +	u8 *buf;
> +
> +	buf = kmalloc(QCA_DFU_PACKET_LEN, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	count = firmware->size;
> +
> +	size = min_t(size_t, count, hdr_size);
> +	memcpy(buf, firmware->data, size);
> +
> +	/* USB patches should go down to controller through USB path
> +	 * because binary format fits to go down through USB channel.
> +	 * USB control path is for patching headers and USB bulk is for
> +	 * patch body.
> +	 */
> +	pipe = usb_sndctrlpipe(udev, 0);
> +	err = usb_control_msg(udev, pipe, QCA_DFU_DOWNLOAD, USB_TYPE_VENDOR,
> +			      0, 0, buf, size, USB_CTRL_SET_TIMEOUT);
> +	if (err < 0) {
> +		BT_ERR("%s: Failed to send headers (%d)", hdev->name, err);
> +		goto done;
> +	}
> +
> +	sent += size;
> +	count -= size;
> +
> +	while (count) {
> +		size = min_t(size_t, count, QCA_DFU_PACKET_LEN);
> +
> +		memcpy(buf, firmware->data + sent, size);
> +
> +		pipe = usb_sndbulkpipe(udev, 0x02);
> +		err = usb_bulk_msg(udev, pipe, buf, size, &len,
> +				   QCA_DFU_TIMEOUT);
> +		if (err < 0) {
> +			BT_ERR("%s: Failed to send body at %zd of %zd (%d)",
> +			       hdev->name, sent, firmware->size, err);
> +			break;
> +		}
> +
> +		if (size != len) {
> +			BT_ERR("%s: Failed to get bulk buffer", hdev->name);
> +			err = -EILSEQ;
> +			break;
> +		}
> +
> +		sent  += size;
> +		count -= size;
> +	}
> +
> +done:
> +	kfree(buf);
> +	return err;
> +}
> +
> +static int btusb_setup_qca_load_rampatch(struct hci_dev *hdev,
> +					 struct qca_version *ver,
> +					 const struct qca_device_info *info)
> +{
> +	struct qca_rampatch_version *rver;
> +	const struct firmware *fw;
> +	char fwname[64];
> +	int err;
> +
> +	snprintf(fwname, sizeof(fwname), "qca/rampatch_usb_%08x.bin",
> +		 le32_to_cpu(ver->rom_version));
> +
> +	err = request_firmware(&fw, fwname, &hdev->dev);
> +	if (err) {
> +		BT_ERR("%s: failed to request rampatch file: %s (%d)",
> +		       hdev->name, fwname, err);
> +		return err;
> +	}
> +
> +	BT_INFO("%s: using rampatch file: %s", hdev->name, fwname);
> +	rver = (struct qca_rampatch_version *)(fw->data + info->ver_offset);
> +	BT_INFO("%s: QCA: patch rome 0x%x build 0x%x, firmware rome 0x%x "
> +		"build 0x%x", hdev->name, le16_to_cpu(rver->rom_version),
> +		le16_to_cpu(rver->patch_version), le32_to_cpu(ver->rom_version),
> +		le32_to_cpu(ver->patch_version));
> +
> +	if (rver->rom_version != ver->rom_version ||
> +	    rver->patch_version <= ver->patch_version) {
> +		BT_ERR("%s: rampatch file version did not match with firmware",
> +		       hdev->name);
> +		err = -EINVAL;
> +		goto done;
> +	}
> +
> +	err = btusb_setup_qca_download_fw(hdev, fw, info->rampatch_hdr);
> +
> +done:
> +	release_firmware(fw);
> +
> +	return err;
> +}
> +
> +static int btusb_setup_qca_load_nvm(struct hci_dev *hdev,
> +				    struct qca_version *ver,
> +				    const struct qca_device_info *info)
> +{
> +	const struct firmware *fw;
> +	char fwname[64];
> +	int err;
> +
> +	snprintf(fwname, sizeof(fwname), "qca/nvm_usb_%08x.bin",
> +		 le32_to_cpu(ver->rom_version));
> +
> +	err = request_firmware(&fw, fwname, &hdev->dev);
> +	if (err) {
> +		BT_ERR("%s: failed to request NVM file: %s (%d)",
> +		       hdev->name, fwname, err);
> +		return err;
> +	}
> +
> +	BT_INFO("%s: using NVM file: %s", hdev->name, fwname);
> +
> +	err = btusb_setup_qca_download_fw(hdev, fw, info->nvm_hdr);
> +
> +	release_firmware(fw);
> +
> +	return err;
> +}
> +
> +static int btusb_setup_qca(struct hci_dev *hdev)
> +{
> +	const struct qca_device_info *info = NULL;
> +	struct qca_version ver;
> +	u8 status;
> +	int i, err;
> +
> +	err = btusb_qca_send_vendor_req(hdev, QCA_GET_TARGET_VERSION, &ver,
> +				        sizeof(ver));
> +	if (err < 0)
> +		return err;
> +
> +	for (i = 0; i < ARRAY_SIZE(qca_devices_table); i++) {
> +		if (ver.rom_version == qca_devices_table[i].rom_version)
> +			info = &qca_devices_table[i];
> +	}
> +	if (!info) {
> +		BT_ERR("%s: don't support firmware rome 0x%x", hdev->name,
> +		       le32_to_cpu(ver.rom_version));
> +		return -ENODEV;
> +	}
> +
> +	err = btusb_qca_send_vendor_req(hdev, QCA_CHECK_STATUS, &status,
> +					sizeof(status));
> +	if (err < 0)
> +		return err;
> +
> +	if (!(status & QCA_PATCH_UPDATED)) {
> +		err = btusb_setup_qca_load_rampatch(hdev, &ver, info);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	if (!(status & QCA_SYSCFG_UPDATED)) {
> +		err = btusb_setup_qca_load_nvm(hdev, &ver, info);
> +		if (err < 0)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> static int btusb_probe(struct usb_interface *intf,
> 		       const struct usb_device_id *id)
> {
> @@ -2736,6 +2996,11 @@ static int btusb_probe(struct usb_interface *intf,
> 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
> 	}
> 
> +	if (id->driver_info & BTUSB_QCA_ROME) {
> +		data->setup_patch_usb = btusb_setup_qca;
> +		hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
> +	}
> +
> 	if (id->driver_info & BTUSB_AMP) {
> 		/* AMP controllers do not support SCO packets */
> 		data->isoc = NULL;
> -- 
> 1.9.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] Bluetooth: Add support for QCA ROME chipset family

[Marcel Holtmann]

Hi Ben,

> This patch supports ROME Bluetooth family from Qualcomm Atheros,
> e.g. QCA61x4 or QCA6574.
> 
> New chipset have similar firmware downloading sequences to previous
> chipset from Atheros, however, it doesn't support vid/pid switching
> after downloading the patch so that firmware needs to be handled by
> btusb module directly.
> 
> ROME chipset can be differentiated from previous version by reading
> ROM version.
> 
> T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 16 Spd=12   MxCh= 0
> D:  Ver= 1.10 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
> P:  Vendor=0cf3 ProdID=e300 Rev= 0.01
> C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
> I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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
> 
> T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#=  8 Spd=12   MxCh= 0
> D:  Ver= 2.01 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
> P:  Vendor=0cf3 ProdID=e360 Rev= 0.01
> C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
> I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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
> 
> Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
> ---
> drivers/bluetooth/btusb.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 265 insertions(+)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index b876888..45fe262 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -52,6 +52,7 @@ static struct usb_driver btusb_driver;
> #define BTUSB_SWAVE		0x1000
> #define BTUSB_INTEL_NEW		0x2000
> #define BTUSB_AMP		0x4000
> +#define BTUSB_QCA_ROME		0x8000
> 
> static const struct usb_device_id btusb_table[] = {
> 	/* Generic Bluetooth USB device */
> @@ -212,6 +213,10 @@ static const struct usb_device_id blacklist_table[] = {
> 	{ USB_DEVICE(0x0489, 0xe036), .driver_info = BTUSB_ATH3012 },
> 	{ USB_DEVICE(0x0489, 0xe03c), .driver_info = BTUSB_ATH3012 },
> 
> +	/* QCA ROME chipset */
> +	{ USB_DEVICE(0x0cf3, 0xe300), .driver_info = BTUSB_QCA_ROME},
> +	{ USB_DEVICE(0x0cf3, 0xe360), .driver_info = BTUSB_QCA_ROME},
> +
> 	/* Broadcom BCM2035 */
> 	{ USB_DEVICE(0x0a5c, 0x2009), .driver_info = BTUSB_BCM92035 },
> 	{ USB_DEVICE(0x0a5c, 0x200a), .driver_info = BTUSB_WRONG_SCO_MTU },
> @@ -336,6 +341,8 @@ struct btusb_data {
> 
> 	int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> 	int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> +
> +	int (*setup_patch_usb)(struct hci_dev *hdev);
> };

I prefer to do this in two patches. One that add the extra callback handling. And then the other adding QCA support.

However I do wonder if setup_patch_usb is the right name here. I current fail to come up with a better name. So I am open for ideas.

One other thing I am wondering is if this should take a btusb_data pointer. Since we are bypassing hci_dev anyway. We do not have to do that, but I am wondering if that makes sense.

The overall patch looks fine to me and if it works for you I am fine doing it. I think we just need to make sure this is clean and all angles are thought through before I merge it.

Regards

Marcel

Re: [PATCH v5] Bluetooth: Add support for QCA ROME chipset family

[Kim, Ben Young Tae]

Hi Marcel,

On Feb 12, 2015, at 11:35 AM, Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Ben,
> 
>> This patch supports ROME Bluetooth family from Qualcomm Atheros,
>> e.g. QCA61x4 or QCA6574.
>> 
>> New chipset have similar firmware downloading sequences to previous
>> chipset from Atheros, however, it doesn't support vid/pid switching
>> after downloading the patch so that firmware needs to be handled by
>> btusb module directly.
>> 
>> ROME chipset can be differentiated from previous version by reading
>> ROM version.
>> 
>> T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 16 Spd=12   MxCh= 0
>> D:  Ver= 1.10 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
>> P:  Vendor=0cf3 ProdID=e300 Rev= 0.01
>> C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
>> I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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
>> 
>> T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#=  8 Spd=12   MxCh= 0
>> D:  Ver= 2.01 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
>> P:  Vendor=0cf3 ProdID=e360 Rev= 0.01
>> C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
>> I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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
>> 
>> Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
>> ---
>> drivers/bluetooth/btusb.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 265 insertions(+)
>> 
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index b876888..45fe262 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -52,6 +52,7 @@ static struct usb_driver btusb_driver;
>> #define BTUSB_SWAVE		0x1000
>> #define BTUSB_INTEL_NEW		0x2000
>> #define BTUSB_AMP		0x4000
>> +#define BTUSB_QCA_ROME		0x8000
>> 
>> static const struct usb_device_id btusb_table[] = {
>> 	/* Generic Bluetooth USB device */
>> @@ -212,6 +213,10 @@ static const struct usb_device_id blacklist_table[] = {
>> 	{ USB_DEVICE(0x0489, 0xe036), .driver_info = BTUSB_ATH3012 },
>> 	{ USB_DEVICE(0x0489, 0xe03c), .driver_info = BTUSB_ATH3012 },
>> 
>> +	/* QCA ROME chipset */
>> +	{ USB_DEVICE(0x0cf3, 0xe300), .driver_info = BTUSB_QCA_ROME},
>> +	{ USB_DEVICE(0x0cf3, 0xe360), .driver_info = BTUSB_QCA_ROME},
>> +
>> 	/* Broadcom BCM2035 */
>> 	{ USB_DEVICE(0x0a5c, 0x2009), .driver_info = BTUSB_BCM92035 },
>> 	{ USB_DEVICE(0x0a5c, 0x200a), .driver_info = BTUSB_WRONG_SCO_MTU },
>> @@ -336,6 +341,8 @@ struct btusb_data {
>> 
>> 	int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
>> 	int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
>> +
>> +	int (*setup_patch_usb)(struct hci_dev *hdev);
>> };
> 
> I prefer to do this in two patches. One that add the extra callback handling. And then the other adding QCA support.
> 
> However I do wonder if setup_patch_usb is the right name here. I current fail to come up with a better name. So I am open for ideas.
> 
> One other thing I am wondering is if this should take a btusb_data pointer. Since we are bypassing hci_dev anyway. We do not have to do that, but I am wondering if that makes sense.

If we’re okay to touch hci_dev structure, I would prefer to add this function pointer in hci_dev. The reason why btusb_data struct was chosen is it doesn’t need to touch outside of header or files except btusb.c file. If possible, I’ll add setup function pointer in hci_dev as ’setup_on_usb’. That is more clean & easily readable.

> 
> The overall patch looks fine to me and if it works for you I am fine doing it. I think we just need to make sure this is clean and all angles are thought through before I merge it.

My testing has been good & got couple of testing feedbacks said this patch is good to go.

> 
> Regards
> 
> Marcel

Re: [PATCH v5] Bluetooth: Add support for QCA ROME chipset family

[Marcel Holtmann]

Hi Ben,

>>> This patch supports ROME Bluetooth family from Qualcomm Atheros,
>>> e.g. QCA61x4 or QCA6574.
>>> 
>>> New chipset have similar firmware downloading sequences to previous
>>> chipset from Atheros, however, it doesn't support vid/pid switching
>>> after downloading the patch so that firmware needs to be handled by
>>> btusb module directly.
>>> 
>>> ROME chipset can be differentiated from previous version by reading
>>> ROM version.
>>> 
>>> T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 16 Spd=12   MxCh= 0
>>> D:  Ver= 1.10 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
>>> P:  Vendor=0cf3 ProdID=e300 Rev= 0.01
>>> C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
>>> I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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
>>> 
>>> T:  Bus=03 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#=  8 Spd=12   MxCh= 0
>>> D:  Ver= 2.01 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
>>> P:  Vendor=0cf3 ProdID=e360 Rev= 0.01
>>> C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
>>> I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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=e0(wlcon) 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
>>> 
>>> Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
>>> ---
>>> drivers/bluetooth/btusb.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 265 insertions(+)
>>> 
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index b876888..45fe262 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -52,6 +52,7 @@ static struct usb_driver btusb_driver;
>>> #define BTUSB_SWAVE		0x1000
>>> #define BTUSB_INTEL_NEW		0x2000
>>> #define BTUSB_AMP		0x4000
>>> +#define BTUSB_QCA_ROME		0x8000
>>> 
>>> static const struct usb_device_id btusb_table[] = {
>>> 	/* Generic Bluetooth USB device */
>>> @@ -212,6 +213,10 @@ static const struct usb_device_id blacklist_table[] = {
>>> 	{ USB_DEVICE(0x0489, 0xe036), .driver_info = BTUSB_ATH3012 },
>>> 	{ USB_DEVICE(0x0489, 0xe03c), .driver_info = BTUSB_ATH3012 },
>>> 
>>> +	/* QCA ROME chipset */
>>> +	{ USB_DEVICE(0x0cf3, 0xe300), .driver_info = BTUSB_QCA_ROME},
>>> +	{ USB_DEVICE(0x0cf3, 0xe360), .driver_info = BTUSB_QCA_ROME},
>>> +
>>> 	/* Broadcom BCM2035 */
>>> 	{ USB_DEVICE(0x0a5c, 0x2009), .driver_info = BTUSB_BCM92035 },
>>> 	{ USB_DEVICE(0x0a5c, 0x200a), .driver_info = BTUSB_WRONG_SCO_MTU },
>>> @@ -336,6 +341,8 @@ struct btusb_data {
>>> 
>>> 	int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
>>> 	int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
>>> +
>>> +	int (*setup_patch_usb)(struct hci_dev *hdev);
>>> };
>> 
>> I prefer to do this in two patches. One that add the extra callback handling. And then the other adding QCA support.
>> 
>> However I do wonder if setup_patch_usb is the right name here. I current fail to come up with a better name. So I am open for ideas.
>> 
>> One other thing I am wondering is if this should take a btusb_data pointer. Since we are bypassing hci_dev anyway. We do not have to do that, but I am wondering if that makes sense.
> 
> If we’re okay to touch hci_dev structure, I would prefer to add this function pointer in hci_dev. The reason why btusb_data struct was chosen is it doesn’t need to touch outside of header or files except btusb.c file. If possible, I’ll add setup function pointer in hci_dev as ’setup_on_usb’. That is more clean & easily readable.

we are not okay with that at all. This is btusb.c internal detail on how to handle the USB transport. That is why I asked about doing this over HCI. Then we could involve the core layer, but for USB specific details we can never ever involve the Bluetooth core layer.

>> 
>> The overall patch looks fine to me and if it works for you I am fine doing it. I think we just need to make sure this is clean and all angles are thought through before I merge it.
> 
> My testing has been good & got couple of testing feedbacks said this patch is good to go.


Yes. It is just cosmetics at this point.

Regards

Marcel

Re: [PATCH v5] Bluetooth: Add support for QCA ROME chipset family

[Kim, Ben Young Tae]

Hi Marcel,

On Feb 14, 2015, at 3:35 PM, Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Ben,
>=20
>>>> This patch supports ROME Bluetooth family from Qualcomm Atheros,
>>>> e.g. QCA61x4 or QCA6574.
>>>>=20
>>>> New chipset have similar firmware downloading sequences to previous
>>>> chipset from Atheros, however, it doesn't support vid/pid switching
>>>> after downloading the patch so that firmware needs to be handled by
>>>> btusb module directly.
>>>>=20
>>>> ROME chipset can be differentiated from previous version by reading
>>>> ROM version.
>>>>=20
>>>> T:  Bus=3D03 Lev=3D01 Prnt=3D01 Port=3D01 Cnt=3D01 Dev#=3D 16 Spd=3D12=
   MxCh=3D 0
>>>> D:  Ver=3D 1.10 Cls=3De0(wlcon) Sub=3D01 Prot=3D01 MxPS=3D64 #Cfgs=3D =
 1
>>>> P:  Vendor=3D0cf3 ProdID=3De300 Rev=3D 0.01
>>>> C:* #Ifs=3D 2 Cfg#=3D 1 Atr=3De0 MxPwr=3D100mA
>>>> I:* If#=3D 0 Alt=3D 0 #EPs=3D 3 Cls=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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
>>>>=20
>>>> T:  Bus=3D03 Lev=3D01 Prnt=3D01 Port=3D01 Cnt=3D01 Dev#=3D  8 Spd=3D12=
   MxCh=3D 0
>>>> D:  Ver=3D 2.01 Cls=3De0(wlcon) Sub=3D01 Prot=3D01 MxPS=3D64 #Cfgs=3D =
 1
>>>> P:  Vendor=3D0cf3 ProdID=3De360 Rev=3D 0.01
>>>> C:* #Ifs=3D 2 Cfg#=3D 1 Atr=3De0 MxPwr=3D100mA
>>>> I:* If#=3D 0 Alt=3D 0 #EPs=3D 3 Cls=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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=3De0(wlcon) Sub=3D01 Prot=3D01 Dri=
ver=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
>>>>=20
>>>> Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
>>>> ---
>>>> drivers/bluetooth/btusb.c | 265 ++++++++++++++++++++++++++++++++++++++=
++++++++
>>>> 1 file changed, 265 insertions(+)
>>>>=20
>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>> index b876888..45fe262 100644
>>>> --- a/drivers/bluetooth/btusb.c
>>>> +++ b/drivers/bluetooth/btusb.c
>>>> @@ -52,6 +52,7 @@ static struct usb_driver btusb_driver;
>>>> #define BTUSB_SWAVE		0x1000
>>>> #define BTUSB_INTEL_NEW		0x2000
>>>> #define BTUSB_AMP		0x4000
>>>> +#define BTUSB_QCA_ROME		0x8000
>>>>=20
>>>> static const struct usb_device_id btusb_table[] =3D {
>>>> 	/* Generic Bluetooth USB device */
>>>> @@ -212,6 +213,10 @@ static const struct usb_device_id blacklist_table=
[] =3D {
>>>> 	{ USB_DEVICE(0x0489, 0xe036), .driver_info =3D BTUSB_ATH3012 },
>>>> 	{ USB_DEVICE(0x0489, 0xe03c), .driver_info =3D BTUSB_ATH3012 },
>>>>=20
>>>> +	/* QCA ROME chipset */
>>>> +	{ USB_DEVICE(0x0cf3, 0xe300), .driver_info =3D BTUSB_QCA_ROME},
>>>> +	{ USB_DEVICE(0x0cf3, 0xe360), .driver_info =3D BTUSB_QCA_ROME},
>>>> +
>>>> 	/* Broadcom BCM2035 */
>>>> 	{ USB_DEVICE(0x0a5c, 0x2009), .driver_info =3D BTUSB_BCM92035 },
>>>> 	{ USB_DEVICE(0x0a5c, 0x200a), .driver_info =3D BTUSB_WRONG_SCO_MTU },
>>>> @@ -336,6 +341,8 @@ struct btusb_data {
>>>>=20
>>>> 	int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
>>>> 	int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
>>>> +
>>>> +	int (*setup_patch_usb)(struct hci_dev *hdev);
>>>> };
>>>=20
>>> I prefer to do this in two patches. One that add the extra callback han=
dling. And then the other adding QCA support.
>>>=20
>>> However I do wonder if setup_patch_usb is the right name here. I curren=
t fail to come up with a better name. So I am open for ideas.
>>>=20
>>> One other thing I am wondering is if this should take a btusb_data poin=
ter. Since we are bypassing hci_dev anyway. We do not have to do that, but =
I am wondering if that makes sense.
>>=20
>> If we=92re okay to touch hci_dev structure, I would prefer to add this f=
unction pointer in hci_dev. The reason why btusb_data struct was chosen is =
it doesn=92t need to touch outside of header or files except btusb.c file. =
If possible, I=92ll add setup function pointer in hci_dev as =92setup_on_us=
b=92. That is more clean & easily readable.
>=20
> we are not okay with that at all. This is btusb.c internal detail on how =
to handle the USB transport. That is why I asked about doing this over HCI.=
 Then we could involve the core layer, but for USB specific details we can =
never ever involve the Bluetooth core layer.
>=20

So only option here is putting =91setup_on_usb=92 pointer in btusb_data str=
ucture and invoke it from btusb_open function I guess. If you have better i=
dea to deal with USB operation in btusb loader, please let me know.

>>>=20
>>> The overall patch looks fine to me and if it works for you I am fine do=
ing it. I think we just need to make sure this is clean and all angles are =
thought through before I merge it.
>>=20
>> My testing has been good & got couple of testing feedbacks said this pat=
ch is good to go.
>=20
>=20
> Yes. It is just cosmetics at this point.
>=20
> Regards
>=20
> Marcel

[PATCH] Bluetooth:btusb:fix incorrect type in initializer

[Kim, Ben Young Tae]

This fixes the following sparse warning:

drivers/bluetooth/btusb.c:2677:11: warning: incorrect type in initializer (=
different base types)
drivers/bluetooth/btusb.c:2677:11:    expected restricted __le32 [usertype]=
 rom_version
drivers/bluetooth/btusb.c:2677:11:    got int
drivers/bluetooth/btusb.c:2678:11: warning: incorrect type in initializer (=
different base types)
drivers/bluetooth/btusb.c:2678:11:    expected restricted __le32 [usertype]=
 rom_version
drivers/bluetooth/btusb.c:2678:11:    got int
drivers/bluetooth/btusb.c:2679:11: warning: incorrect type in initializer (=
different base types)
drivers/bluetooth/btusb.c:2679:11:    expected restricted __le32 [usertype]=
 rom_version
drivers/bluetooth/btusb.c:2679:11:    got int
drivers/bluetooth/btusb.c:2680:11: warning: incorrect type in initializer (=
different base types)
drivers/bluetooth/btusb.c:2680:11:    expected restricted __le32 [usertype]=
 rom_version
drivers/bluetooth/btusb.c:2680:11:    got int
drivers/bluetooth/btusb.c:2681:11: warning: incorrect type in initializer (=
different base types)
drivers/bluetooth/btusb.c:2681:11:    expected restricted __le32 [usertype]=
 rom_version
drivers/bluetooth/btusb.c:2681:11:    got int
drivers/bluetooth/btusb.c:2805:17: warning: restricted __le16 degrades to i=
nteger
drivers/bluetooth/btusb.c:2805:37: warning: restricted __le32 degrades to i=
nteger
drivers/bluetooth/btusb.c:2806:17: warning: restricted __le16 degrades to i=
nteger
drivers/bluetooth/btusb.c:2806:39: warning: restricted __le32 degrades to i=
nteger

Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
---
 drivers/bluetooth/btusb.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 0833054..f65cd44 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2674,11 +2674,11 @@ struct qca_device_info {
 };
=20
 static const struct qca_device_info qca_devices_table[] =3D {
-	{ 0x00000100, 20, 4, 10 }, /* Rome 1.0 */
-	{ 0x00000101, 20, 4, 10 }, /* Rome 1.1 */
-	{ 0x00000201, 28, 4, 18 }, /* Rome 2.1 */
-	{ 0x00000300, 28, 4, 18 }, /* Rome 3.0 */
-	{ 0x00000302, 28, 4, 18 }, /* Rome 3.2 */
+	{ cpu_to_le32(0x00000100), 20, 4, 10 }, /* Rome 1.0 */
+	{ cpu_to_le32(0x00000101), 20, 4, 10 }, /* Rome 1.1 */
+	{ cpu_to_le32(0x00000201), 28, 4, 18 }, /* Rome 2.1 */
+	{ cpu_to_le32(0x00000300), 28, 4, 18 }, /* Rome 3.0 */
+	{ cpu_to_le32(0x00000302), 28, 4, 18 }, /* Rome 3.2 */
 };
=20
 static int btusb_qca_send_vendor_req(struct hci_dev *hdev, u8 request,
@@ -2802,8 +2802,8 @@ static int btusb_setup_qca_load_rampatch(struct hci_d=
ev *hdev,
 		le16_to_cpu(rver->patch_version), le32_to_cpu(ver->rom_version),
 		le32_to_cpu(ver->patch_version));
=20
-	if (rver->rom_version !=3D ver->rom_version ||
-	    rver->patch_version <=3D ver->patch_version) {
+	if (le16_to_cpu(rver->rom_version) !=3D le32_to_cpu(ver->rom_version) ||
+	    le16_to_cpu(rver->patch_version) < le32_to_cpu(ver->patch_version)) {
 		BT_ERR("%s: rampatch file version did not match with firmware",
 		       hdev->name);
 		err =3D -EINVAL;
--=20
1.8.1.5

Re: [PATCH] Bluetooth:btusb:fix incorrect type in initializer

[Johan Hedberg]

Hi,

On Tue, Mar 03, 2015, Kim, Ben Young Tae wrote:
> This fixes the following sparse warning:
> 
> drivers/bluetooth/btusb.c:2677:11: warning: incorrect type in initializer (different base types)
> drivers/bluetooth/btusb.c:2677:11:    expected restricted __le32 [usertype] rom_version
> drivers/bluetooth/btusb.c:2677:11:    got int
> drivers/bluetooth/btusb.c:2678:11: warning: incorrect type in initializer (different base types)
> drivers/bluetooth/btusb.c:2678:11:    expected restricted __le32 [usertype] rom_version
> drivers/bluetooth/btusb.c:2678:11:    got int
> drivers/bluetooth/btusb.c:2679:11: warning: incorrect type in initializer (different base types)
> drivers/bluetooth/btusb.c:2679:11:    expected restricted __le32 [usertype] rom_version
> drivers/bluetooth/btusb.c:2679:11:    got int
> drivers/bluetooth/btusb.c:2680:11: warning: incorrect type in initializer (different base types)
> drivers/bluetooth/btusb.c:2680:11:    expected restricted __le32 [usertype] rom_version
> drivers/bluetooth/btusb.c:2680:11:    got int
> drivers/bluetooth/btusb.c:2681:11: warning: incorrect type in initializer (different base types)
> drivers/bluetooth/btusb.c:2681:11:    expected restricted __le32 [usertype] rom_version
> drivers/bluetooth/btusb.c:2681:11:    got int
> drivers/bluetooth/btusb.c:2805:17: warning: restricted __le16 degrades to integer
> drivers/bluetooth/btusb.c:2805:37: warning: restricted __le32 degrades to integer
> drivers/bluetooth/btusb.c:2806:17: warning: restricted __le16 degrades to integer
> drivers/bluetooth/btusb.c:2806:39: warning: restricted __le32 degrades to integer
> 
> Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
> ---
>  drivers/bluetooth/btusb.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 0833054..f65cd44 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2674,11 +2674,11 @@ struct qca_device_info {
>  };
>  
>  static const struct qca_device_info qca_devices_table[] = {
> -	{ 0x00000100, 20, 4, 10 }, /* Rome 1.0 */
> -	{ 0x00000101, 20, 4, 10 }, /* Rome 1.1 */
> -	{ 0x00000201, 28, 4, 18 }, /* Rome 2.1 */
> -	{ 0x00000300, 28, 4, 18 }, /* Rome 3.0 */
> -	{ 0x00000302, 28, 4, 18 }, /* Rome 3.2 */
> +	{ cpu_to_le32(0x00000100), 20, 4, 10 }, /* Rome 1.0 */
> +	{ cpu_to_le32(0x00000101), 20, 4, 10 }, /* Rome 1.1 */
> +	{ cpu_to_le32(0x00000201), 28, 4, 18 }, /* Rome 2.1 */
> +	{ cpu_to_le32(0x00000300), 28, 4, 18 }, /* Rome 3.0 */
> +	{ cpu_to_le32(0x00000302), 28, 4, 18 }, /* Rome 3.2 */
>  };

Since "struct qca_device_info" doesn't describe raw protocol data I
don't think it should contain protocol endianness values, i.e. the
rom_version member should be u32 instead of __le32.

Johan

Re: [PATCH] Bluetooth:btusb:fix incorrect type in initializer

[Kim, Ben Young Tae]

Hi Johan

> On Mar 6, 2015, at 5:55 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote=
:
>=20
> Hi,
>=20
>> On Tue, Mar 03, 2015, Kim, Ben Young Tae wrote:
>> This fixes the following sparse warning:
>>=20
>> drivers/bluetooth/btusb.c:2677:11: warning: incorrect type in initialize=
r (different base types)
>> drivers/bluetooth/btusb.c:2677:11: expected restricted __le32 [usertype]=
 rom_version
>> drivers/bluetooth/btusb.c:2677:11: got int
>> drivers/bluetooth/btusb.c:2678:11: warning: incorrect type in initialize=
r (different base types)
>> drivers/bluetooth/btusb.c:2678:11: expected restricted __le32 [usertype]=
 rom_version
>> drivers/bluetooth/btusb.c:2678:11: got int
>> drivers/bluetooth/btusb.c:2679:11: warning: incorrect type in initialize=
r (different base types)
>> drivers/bluetooth/btusb.c:2679:11: expected restricted __le32 [usertype]=
 rom_version
>> drivers/bluetooth/btusb.c:2679:11: got int
>> drivers/bluetooth/btusb.c:2680:11: warning: incorrect type in initialize=
r (different base types)
>> drivers/bluetooth/btusb.c:2680:11: expected restricted __le32 [usertype]=
 rom_version
>> drivers/bluetooth/btusb.c:2680:11: got int
>> drivers/bluetooth/btusb.c:2681:11: warning: incorrect type in initialize=
r (different base types)
>> drivers/bluetooth/btusb.c:2681:11: expected restricted __le32 [usertype]=
 rom_version
>> drivers/bluetooth/btusb.c:2681:11: got int
>> drivers/bluetooth/btusb.c:2805:17: warning: restricted __le16 degrades t=
o integer
>> drivers/bluetooth/btusb.c:2805:37: warning: restricted __le32 degrades t=
o integer
>> drivers/bluetooth/btusb.c:2806:17: warning: restricted __le16 degrades t=
o integer
>> drivers/bluetooth/btusb.c:2806:39: warning: restricted __le32 degrades t=
o integer
>>=20
>> Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
>> ---
>> drivers/bluetooth/btusb.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>=20
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 0833054..f65cd44 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -2674,11 +2674,11 @@ struct qca_device_info {
>> };
>>=20
>> static const struct qca_device_info qca_devices_table[] =3D {
>> -    { 0x00000100, 20, 4, 10 }, /* Rome 1.0 */
>> -    { 0x00000101, 20, 4, 10 }, /* Rome 1.1 */
>> -    { 0x00000201, 28, 4, 18 }, /* Rome 2.1 */
>> -    { 0x00000300, 28, 4, 18 }, /* Rome 3.0 */
>> -    { 0x00000302, 28, 4, 18 }, /* Rome 3.2 */
>> +    { cpu_to_le32(0x00000100), 20, 4, 10 }, /* Rome 1.0 */
>> +    { cpu_to_le32(0x00000101), 20, 4, 10 }, /* Rome 1.1 */
>> +    { cpu_to_le32(0x00000201), 28, 4, 18 }, /* Rome 2.1 */
>> +    { cpu_to_le32(0x00000300), 28, 4, 18 }, /* Rome 3.0 */
>> +    { cpu_to_le32(0x00000302), 28, 4, 18 }, /* Rome 3.2 */
>> };
>=20
> Since "struct qca_device_info" doesn't describe raw protocol data I
> don't think it should contain protocol endianness values, i.e. the
> rom_version member should be u32 instead of __le32.
>=20

The rom_version will be coming from chipset when we ask chip version to soc=
 at the first time and we are comparing to this during initialization. I th=
ink this should be _le32 type.

> Johan

Re: [PATCH] Bluetooth:btusb:fix incorrect type in initializer

[Johan Hedberg]

Hi,

On Fri, Mar 06, 2015, Kim, Ben Young Tae wrote:
> >> On Tue, Mar 03, 2015, Kim, Ben Young Tae wrote:
> >> This fixes the following sparse warning:
> >> 
> >> drivers/bluetooth/btusb.c:2677:11: warning: incorrect type in initializer (different base types)
> >> drivers/bluetooth/btusb.c:2677:11: expected restricted __le32 [usertype] rom_version
> >> drivers/bluetooth/btusb.c:2677:11: got int
> >> drivers/bluetooth/btusb.c:2678:11: warning: incorrect type in initializer (different base types)
> >> drivers/bluetooth/btusb.c:2678:11: expected restricted __le32 [usertype] rom_version
> >> drivers/bluetooth/btusb.c:2678:11: got int
> >> drivers/bluetooth/btusb.c:2679:11: warning: incorrect type in initializer (different base types)
> >> drivers/bluetooth/btusb.c:2679:11: expected restricted __le32 [usertype] rom_version
> >> drivers/bluetooth/btusb.c:2679:11: got int
> >> drivers/bluetooth/btusb.c:2680:11: warning: incorrect type in initializer (different base types)
> >> drivers/bluetooth/btusb.c:2680:11: expected restricted __le32 [usertype] rom_version
> >> drivers/bluetooth/btusb.c:2680:11: got int
> >> drivers/bluetooth/btusb.c:2681:11: warning: incorrect type in initializer (different base types)
> >> drivers/bluetooth/btusb.c:2681:11: expected restricted __le32 [usertype] rom_version
> >> drivers/bluetooth/btusb.c:2681:11: got int
> >> drivers/bluetooth/btusb.c:2805:17: warning: restricted __le16 degrades to integer
> >> drivers/bluetooth/btusb.c:2805:37: warning: restricted __le32 degrades to integer
> >> drivers/bluetooth/btusb.c:2806:17: warning: restricted __le16 degrades to integer
> >> drivers/bluetooth/btusb.c:2806:39: warning: restricted __le32 degrades to integer
> >> 
> >> Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
> >> ---
> >> drivers/bluetooth/btusb.c | 14 +++++++-------
> >> 1 file changed, 7 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> >> index 0833054..f65cd44 100644
> >> --- a/drivers/bluetooth/btusb.c
> >> +++ b/drivers/bluetooth/btusb.c
> >> @@ -2674,11 +2674,11 @@ struct qca_device_info {
> >> };
> >> 
> >> static const struct qca_device_info qca_devices_table[] = {
> >> -    { 0x00000100, 20, 4, 10 }, /* Rome 1.0 */
> >> -    { 0x00000101, 20, 4, 10 }, /* Rome 1.1 */
> >> -    { 0x00000201, 28, 4, 18 }, /* Rome 2.1 */
> >> -    { 0x00000300, 28, 4, 18 }, /* Rome 3.0 */
> >> -    { 0x00000302, 28, 4, 18 }, /* Rome 3.2 */
> >> +    { cpu_to_le32(0x00000100), 20, 4, 10 }, /* Rome 1.0 */
> >> +    { cpu_to_le32(0x00000101), 20, 4, 10 }, /* Rome 1.1 */
> >> +    { cpu_to_le32(0x00000201), 28, 4, 18 }, /* Rome 2.1 */
> >> +    { cpu_to_le32(0x00000300), 28, 4, 18 }, /* Rome 3.0 */
> >> +    { cpu_to_le32(0x00000302), 28, 4, 18 }, /* Rome 3.2 */
> >> };
> > 
> > Since "struct qca_device_info" doesn't describe raw protocol data I
> > don't think it should contain protocol endianness values, i.e. the
> > rom_version member should be u32 instead of __le32.
> > 
> 
> The rom_version will be coming from chipset when we ask chip version
> to soc at the first time and we are comparing to this during
> initialization. I think this should be _le32 type.

Understood, but the general principle we try to follow is that only
structs that contain raw protocol data are in protocol endianness.
Anything else should be host endianness. It's also a bit awkward to
define a const struct with values that need to be evaluated through a
function call (i.e. cpu_to_le32). I'd assume that the compiler needs to
do extra tricks in this case to have the resulting data evaluated at
runtime instead of compile time.

Johan

Re: [PATCH] Bluetooth:btusb:fix incorrect type in initializer

[Marcel Holtmann]

Hi Ben,

>>> On Tue, Mar 03, 2015, Kim, Ben Young Tae wrote:
>>> This fixes the following sparse warning:
>>> 
>>> drivers/bluetooth/btusb.c:2677:11: warning: incorrect type in initializer (different base types)
>>> drivers/bluetooth/btusb.c:2677:11: expected restricted __le32 [usertype] rom_version
>>> drivers/bluetooth/btusb.c:2677:11: got int
>>> drivers/bluetooth/btusb.c:2678:11: warning: incorrect type in initializer (different base types)
>>> drivers/bluetooth/btusb.c:2678:11: expected restricted __le32 [usertype] rom_version
>>> drivers/bluetooth/btusb.c:2678:11: got int
>>> drivers/bluetooth/btusb.c:2679:11: warning: incorrect type in initializer (different base types)
>>> drivers/bluetooth/btusb.c:2679:11: expected restricted __le32 [usertype] rom_version
>>> drivers/bluetooth/btusb.c:2679:11: got int
>>> drivers/bluetooth/btusb.c:2680:11: warning: incorrect type in initializer (different base types)
>>> drivers/bluetooth/btusb.c:2680:11: expected restricted __le32 [usertype] rom_version
>>> drivers/bluetooth/btusb.c:2680:11: got int
>>> drivers/bluetooth/btusb.c:2681:11: warning: incorrect type in initializer (different base types)
>>> drivers/bluetooth/btusb.c:2681:11: expected restricted __le32 [usertype] rom_version
>>> drivers/bluetooth/btusb.c:2681:11: got int
>>> drivers/bluetooth/btusb.c:2805:17: warning: restricted __le16 degrades to integer
>>> drivers/bluetooth/btusb.c:2805:37: warning: restricted __le32 degrades to integer
>>> drivers/bluetooth/btusb.c:2806:17: warning: restricted __le16 degrades to integer
>>> drivers/bluetooth/btusb.c:2806:39: warning: restricted __le32 degrades to integer
>>> 
>>> Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
>>> ---
>>> drivers/bluetooth/btusb.c | 14 +++++++-------
>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 0833054..f65cd44 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -2674,11 +2674,11 @@ struct qca_device_info {
>>> };
>>> 
>>> static const struct qca_device_info qca_devices_table[] = {
>>> -    { 0x00000100, 20, 4, 10 }, /* Rome 1.0 */
>>> -    { 0x00000101, 20, 4, 10 }, /* Rome 1.1 */
>>> -    { 0x00000201, 28, 4, 18 }, /* Rome 2.1 */
>>> -    { 0x00000300, 28, 4, 18 }, /* Rome 3.0 */
>>> -    { 0x00000302, 28, 4, 18 }, /* Rome 3.2 */
>>> +    { cpu_to_le32(0x00000100), 20, 4, 10 }, /* Rome 1.0 */
>>> +    { cpu_to_le32(0x00000101), 20, 4, 10 }, /* Rome 1.1 */
>>> +    { cpu_to_le32(0x00000201), 28, 4, 18 }, /* Rome 2.1 */
>>> +    { cpu_to_le32(0x00000300), 28, 4, 18 }, /* Rome 3.0 */
>>> +    { cpu_to_le32(0x00000302), 28, 4, 18 }, /* Rome 3.2 */
>>> };
>> 
>> Since "struct qca_device_info" doesn't describe raw protocol data I
>> don't think it should contain protocol endianness values, i.e. the
>> rom_version member should be u32 instead of __le32.
>> 
> 
> The rom_version will be coming from chipset when we ask chip version to soc at the first time and we are comparing to this during initialization. I think this should be _le32 type.

I made a mistake in the review actually that I only now realized it. And this means that you really have not tested anything on big-endian architectures or just got lucky with the current values.

So lets keep qca_device_info in host endian. It is a host data structure. It is not a wire data structure.

This is actually the broken one:

        if (rver->rom_version != ver->rom_version ||
            rver->patch_version <= ver->patch_version) {

You are now comparing little-endian values. That works all fine on a little-endian system, but on a big-endian system the <= comparison might not give you the result you are hoping for. So this all needs to be done with host endian values.

Move qca_device_info to host endian and then go through the code and then do the little-endian transformation of data from the the hardware. You can easily check with spares (C=2) if you are doing this correctly.

Regards

Marcel

Re: [PATCH] Bluetooth:btusb:fix incorrect type in initializer

[Kim, Ben Young Tae]

Hi Marcel/Johan,

On Mar 6, 2015, at 7:49 AM, Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Ben,
>=20
>>>> On Tue, Mar 03, 2015, Kim, Ben Young Tae wrote:
>>>> This fixes the following sparse warning:
>>>>=20
>>>> drivers/bluetooth/btusb.c:2677:11: warning: incorrect type in initiali=
zer (different base types)
>>>> drivers/bluetooth/btusb.c:2677:11: expected restricted __le32 [usertyp=
e] rom_version
>>>> drivers/bluetooth/btusb.c:2677:11: got int
>>>> drivers/bluetooth/btusb.c:2678:11: warning: incorrect type in initiali=
zer (different base types)
>>>> drivers/bluetooth/btusb.c:2678:11: expected restricted __le32 [usertyp=
e] rom_version
>>>> drivers/bluetooth/btusb.c:2678:11: got int
>>>> drivers/bluetooth/btusb.c:2679:11: warning: incorrect type in initiali=
zer (different base types)
>>>> drivers/bluetooth/btusb.c:2679:11: expected restricted __le32 [usertyp=
e] rom_version
>>>> drivers/bluetooth/btusb.c:2679:11: got int
>>>> drivers/bluetooth/btusb.c:2680:11: warning: incorrect type in initiali=
zer (different base types)
>>>> drivers/bluetooth/btusb.c:2680:11: expected restricted __le32 [usertyp=
e] rom_version
>>>> drivers/bluetooth/btusb.c:2680:11: got int
>>>> drivers/bluetooth/btusb.c:2681:11: warning: incorrect type in initiali=
zer (different base types)
>>>> drivers/bluetooth/btusb.c:2681:11: expected restricted __le32 [usertyp=
e] rom_version
>>>> drivers/bluetooth/btusb.c:2681:11: got int
>>>> drivers/bluetooth/btusb.c:2805:17: warning: restricted __le16 degrades=
 to integer
>>>> drivers/bluetooth/btusb.c:2805:37: warning: restricted __le32 degrades=
 to integer
>>>> drivers/bluetooth/btusb.c:2806:17: warning: restricted __le16 degrades=
 to integer
>>>> drivers/bluetooth/btusb.c:2806:39: warning: restricted __le32 degrades=
 to integer
>>>>=20
>>>> Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
>>>> ---
>>>> drivers/bluetooth/btusb.c | 14 +++++++-------
>>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>>>=20
>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>> index 0833054..f65cd44 100644
>>>> --- a/drivers/bluetooth/btusb.c
>>>> +++ b/drivers/bluetooth/btusb.c
>>>> @@ -2674,11 +2674,11 @@ struct qca_device_info {
>>>> };
>>>>=20
>>>> static const struct qca_device_info qca_devices_table[] =3D {
>>>> -    { 0x00000100, 20, 4, 10 }, /* Rome 1.0 */
>>>> -    { 0x00000101, 20, 4, 10 }, /* Rome 1.1 */
>>>> -    { 0x00000201, 28, 4, 18 }, /* Rome 2.1 */
>>>> -    { 0x00000300, 28, 4, 18 }, /* Rome 3.0 */
>>>> -    { 0x00000302, 28, 4, 18 }, /* Rome 3.2 */
>>>> +    { cpu_to_le32(0x00000100), 20, 4, 10 }, /* Rome 1.0 */
>>>> +    { cpu_to_le32(0x00000101), 20, 4, 10 }, /* Rome 1.1 */
>>>> +    { cpu_to_le32(0x00000201), 28, 4, 18 }, /* Rome 2.1 */
>>>> +    { cpu_to_le32(0x00000300), 28, 4, 18 }, /* Rome 3.0 */
>>>> +    { cpu_to_le32(0x00000302), 28, 4, 18 }, /* Rome 3.2 */
>>>> };
>>>=20
>>> Since "struct qca_device_info" doesn't describe raw protocol data I
>>> don't think it should contain protocol endianness values, i.e. the
>>> rom_version member should be u32 instead of __le32.
>>>=20
>>=20
>> The rom_version will be coming from chipset when we ask chip version to =
soc at the first time and we are comparing to this during initialization. I=
 think this should be _le32 type.
>=20
> I made a mistake in the review actually that I only now realized it. And =
this means that you really have not tested anything on big-endian architect=
ures or just got lucky with the current values.
>=20
> So lets keep qca_device_info in host endian. It is a host data structure.=
 It is not a wire data structure.
>=20
> This is actually the broken one:
>=20
>        if (rver->rom_version !=3D ver->rom_version ||
>            rver->patch_version <=3D ver->patch_version) {
>=20
> You are now comparing little-endian values. That works all fine on a litt=
le-endian system, but on a big-endian system the <=3D comparison might not =
give you the result you are hoping for. So this all needs to be done with h=
ost endian values.
>=20
> Move qca_device_info to host endian and then go through the code and then=
 do the little-endian transformation of data from the the hardware. You can=
 easily check with spares (C=3D2) if you are doing this correctly.
>=20

Does host endian mean to use u32/u8 type instead of __le or __be? If yes, I=
=92ll work on this and send you the patch soon after verifying it with C=3D=
2 option.

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

Thanks
=97 Ben Kim=

Re: [PATCH] Bluetooth:btusb:fix incorrect type in initializer

[Marcel Holtmann]

Hi Ben,

>>>>> On Tue, Mar 03, 2015, Kim, Ben Young Tae wrote:
>>>>> This fixes the following sparse warning:
>>>>> 
>>>>> drivers/bluetooth/btusb.c:2677:11: warning: incorrect type in initializer (different base types)
>>>>> drivers/bluetooth/btusb.c:2677:11: expected restricted __le32 [usertype] rom_version
>>>>> drivers/bluetooth/btusb.c:2677:11: got int
>>>>> drivers/bluetooth/btusb.c:2678:11: warning: incorrect type in initializer (different base types)
>>>>> drivers/bluetooth/btusb.c:2678:11: expected restricted __le32 [usertype] rom_version
>>>>> drivers/bluetooth/btusb.c:2678:11: got int
>>>>> drivers/bluetooth/btusb.c:2679:11: warning: incorrect type in initializer (different base types)
>>>>> drivers/bluetooth/btusb.c:2679:11: expected restricted __le32 [usertype] rom_version
>>>>> drivers/bluetooth/btusb.c:2679:11: got int
>>>>> drivers/bluetooth/btusb.c:2680:11: warning: incorrect type in initializer (different base types)
>>>>> drivers/bluetooth/btusb.c:2680:11: expected restricted __le32 [usertype] rom_version
>>>>> drivers/bluetooth/btusb.c:2680:11: got int
>>>>> drivers/bluetooth/btusb.c:2681:11: warning: incorrect type in initializer (different base types)
>>>>> drivers/bluetooth/btusb.c:2681:11: expected restricted __le32 [usertype] rom_version
>>>>> drivers/bluetooth/btusb.c:2681:11: got int
>>>>> drivers/bluetooth/btusb.c:2805:17: warning: restricted __le16 degrades to integer
>>>>> drivers/bluetooth/btusb.c:2805:37: warning: restricted __le32 degrades to integer
>>>>> drivers/bluetooth/btusb.c:2806:17: warning: restricted __le16 degrades to integer
>>>>> drivers/bluetooth/btusb.c:2806:39: warning: restricted __le32 degrades to integer
>>>>> 
>>>>> Signed-off-by: Ben Young Tae Kim <ytkim@qca.qualcomm.com>
>>>>> ---
>>>>> drivers/bluetooth/btusb.c | 14 +++++++-------
>>>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>> index 0833054..f65cd44 100644
>>>>> --- a/drivers/bluetooth/btusb.c
>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>> @@ -2674,11 +2674,11 @@ struct qca_device_info {
>>>>> };
>>>>> 
>>>>> static const struct qca_device_info qca_devices_table[] = {
>>>>> -    { 0x00000100, 20, 4, 10 }, /* Rome 1.0 */
>>>>> -    { 0x00000101, 20, 4, 10 }, /* Rome 1.1 */
>>>>> -    { 0x00000201, 28, 4, 18 }, /* Rome 2.1 */
>>>>> -    { 0x00000300, 28, 4, 18 }, /* Rome 3.0 */
>>>>> -    { 0x00000302, 28, 4, 18 }, /* Rome 3.2 */
>>>>> +    { cpu_to_le32(0x00000100), 20, 4, 10 }, /* Rome 1.0 */
>>>>> +    { cpu_to_le32(0x00000101), 20, 4, 10 }, /* Rome 1.1 */
>>>>> +    { cpu_to_le32(0x00000201), 28, 4, 18 }, /* Rome 2.1 */
>>>>> +    { cpu_to_le32(0x00000300), 28, 4, 18 }, /* Rome 3.0 */
>>>>> +    { cpu_to_le32(0x00000302), 28, 4, 18 }, /* Rome 3.2 */
>>>>> };
>>>> 
>>>> Since "struct qca_device_info" doesn't describe raw protocol data I
>>>> don't think it should contain protocol endianness values, i.e. the
>>>> rom_version member should be u32 instead of __le32.
>>>> 
>>> 
>>> The rom_version will be coming from chipset when we ask chip version to soc at the first time and we are comparing to this during initialization. I think this should be _le32 type.
>> 
>> I made a mistake in the review actually that I only now realized it. And this means that you really have not tested anything on big-endian architectures or just got lucky with the current values.
>> 
>> So lets keep qca_device_info in host endian. It is a host data structure. It is not a wire data structure.
>> 
>> This is actually the broken one:
>> 
>>       if (rver->rom_version != ver->rom_version ||
>>           rver->patch_version <= ver->patch_version) {
>> 
>> You are now comparing little-endian values. That works all fine on a little-endian system, but on a big-endian system the <= comparison might not give you the result you are hoping for. So this all needs to be done with host endian values.
>> 
>> Move qca_device_info to host endian and then go through the code and then do the little-endian transformation of data from the the hardware. You can easily check with spares (C=2) if you are doing this correctly.
>> 
> 
> Does host endian mean to use u32/u8 type instead of __le or __be? If yes, I’ll work on this and send you the patch soon after verifying it with C=2 option.

yes, host endian means that you use u32 etc. Only for types that a represented on the wire, you use __le32 or __be32 depending on what they are.

Regards

Marcel