[PATCH 1/2] Bluetooth: Add __hci_cmd_sync_noev function

[PATCH 1/2] Bluetooth: Add __hci_cmd_sync_noev function

[Loic Poulain]

This function allows to send a HCI command without expecting any
controller event/response in return.

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_core.c         | 17 +++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index b619a19..d48a7df 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1393,6 +1393,8 @@ struct sk_buff *__hci_cmd_sync(struct hci_dev *hdev, u16 opcode, u32 plen,
 			       const void *param, u32 timeout);
 struct sk_buff *__hci_cmd_sync_ev(struct hci_dev *hdev, u16 opcode, u32 plen,
 				  const void *param, u8 event, u32 timeout);
+int __hci_cmd_sync_noev(struct hci_dev *hdev, u16 opcode, u32 plen,
+			const void *param);
 
 int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen,
 		 const void *param);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 40d260f..2831c4e 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3459,6 +3459,23 @@ struct sk_buff *hci_cmd_sync(struct hci_dev *hdev, u16 opcode, u32 plen,
 }
 EXPORT_SYMBOL(hci_cmd_sync);
 
+int __hci_cmd_sync_noev(struct hci_dev *hdev, u16 opcode, u32 plen,
+			const void *param)
+{
+	struct sk_buff *skb = hci_prepare_cmd(hdev, opcode, plen, param);
+
+	if (!skb) {
+		bt_dev_err(hdev, "no memory for command (opcode 0x%4.4x)",
+			   opcode);
+		return -ENOMEM;
+	}
+
+	hci_send_frame(hdev, skb);
+
+	return 0;
+}
+EXPORT_SYMBOL(__hci_cmd_sync_noev);
+
 /* Send ACL data */
 static void hci_add_acl_hdr(struct sk_buff *skb, __u16 handle, __u16 flags)
 {
-- 
2.7.4

[PATCH 2/2] Bluetooth: btqca: Add AR3002 rampatch support

[Loic Poulain]

This patch adds rampatch download compatibility for ROME >= 3.2.
Starting with ROME 3.2, the 'download mode' field of the rampatch
header indicates if the controller acknowledges (or not) the received
rampatch segments. If not, we need to send all the segments without
expecting any event from the controller (except for the last segment).
Goal is (I assume) to speed-up rampatch download.

This fixes BT on Dragonboard-600c P2 which includes the following BT
controller:

hci0: ROME Patch Version Request
hci0: Product:0x00000008
hci0: Patch  :0x00000111
hci0: ROM    :0x00000302
hci0: SOC    :0x00000023

Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bluetooth/btqca.c | 106 ++++++++++++++++++++++------------------------
 drivers/bluetooth/btqca.h |  11 ++++-
 2 files changed, 61 insertions(+), 56 deletions(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index 2793d41..129779e 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -127,28 +127,42 @@ static void rome_tlv_check_data(struct rome_config *config,
 	BT_DBG("TLV Type\t\t : 0x%x", type_len & 0x000000ff);
 	BT_DBG("Length\t\t : %d bytes", length);
 
+	config->dnld_mode = ROME_SKIP_EVT_NONE;
+
 	switch (config->type) {
 	case TLV_TYPE_PATCH:
 		tlv_patch = (struct tlv_type_patch *)tlv->data;
-		BT_DBG("Total Length\t\t : %d bytes",
+
+		/* For Rome version 1.1 to 3.1, all segment commands
+		 * are acked by a vendor specific event (VSE).
+		 * For Rome >= 3.2, the download mode field indicates
+		 * if VSE is skipped by the controller.
+		 * In case VSE is skipped, only the last segment is acked.
+		 */
+		if (le16_to_cpu(tlv_patch->rom_build) >= 0x0302)
+			config->dnld_mode = tlv_patch->download_mode;
+
+		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));
-		BT_DBG("Patch Data Length\t : %d bytes",
+		BT_DBG("Patch Data Length      : %d bytes",
 		       le32_to_cpu(tlv_patch->data_length));
 		BT_DBG("Signing Format Version : 0x%x",
 		       tlv_patch->format_version);
-		BT_DBG("Signature Algorithm\t : 0x%x",
+		BT_DBG("Signature Algorithm    : 0x%x",
 		       tlv_patch->signature);
-		BT_DBG("Reserved\t\t : 0x%x",
-		       le16_to_cpu(tlv_patch->reserved1));
-		BT_DBG("Product ID\t\t : 0x%04x",
+		BT_DBG("Download mode          : 0x%x",
+		       tlv_patch->download_mode);
+		BT_DBG("Reserved               : 0x%x",
+		       tlv_patch->reserved1);
+		BT_DBG("Product ID             : 0x%04x",
 		       le16_to_cpu(tlv_patch->product_id));
-		BT_DBG("Rom Build Version\t : 0x%04x",
+		BT_DBG("Rom Build Version      : 0x%04x",
 		       le16_to_cpu(tlv_patch->rom_build));
-		BT_DBG("Patch Version\t\t : 0x%04x",
+		BT_DBG("Patch Version          : 0x%04x",
 		       le16_to_cpu(tlv_patch->patch_version));
-		BT_DBG("Reserved\t\t : 0x%x",
+		BT_DBG("Reserved               : 0x%x",
 		       le16_to_cpu(tlv_patch->reserved2));
-		BT_DBG("Patch Entry Address\t : 0x%x",
+		BT_DBG("Patch Entry Address    : 0x%x",
 		       le32_to_cpu(tlv_patch->entry));
 		break;
 
@@ -194,8 +208,8 @@ static void rome_tlv_check_data(struct rome_config *config,
 	}
 }
 
-static int rome_tlv_send_segment(struct hci_dev *hdev, int idx, int seg_size,
-				 const u8 *data)
+static int rome_tlv_send_segment(struct hci_dev *hdev, int seg_size,
+				 const u8 *data, enum rome_tlv_dnld_mode mode)
 {
 	struct sk_buff *skb;
 	struct edl_event_hdr *edl;
@@ -203,12 +217,15 @@ static int rome_tlv_send_segment(struct hci_dev *hdev, int idx, int seg_size,
 	u8 cmd[MAX_SIZE_PER_TLV_SEGMENT + 2];
 	int err = 0;
 
-	BT_DBG("%s: Download segment #%d size %d", hdev->name, idx, seg_size);
-
 	cmd[0] = EDL_PATCH_TLV_REQ_CMD;
 	cmd[1] = seg_size;
 	memcpy(cmd + 2, data, seg_size);
 
+	if (mode == ROME_SKIP_EVT_VSE_CC || mode == ROME_SKIP_EVT_VSE) {
+		return __hci_cmd_sync_noev(hdev, EDL_PATCH_CMD_OPCODE,
+					   seg_size + 2, cmd);
+	}
+
 	skb = __hci_cmd_sync_ev(hdev, EDL_PATCH_CMD_OPCODE, seg_size + 2, cmd,
 				HCI_VENDOR_PKT, HCI_INIT_TIMEOUT);
 	if (IS_ERR(skb)) {
@@ -245,47 +262,12 @@ static int rome_tlv_send_segment(struct hci_dev *hdev, int idx, int seg_size,
 	return err;
 }
 
-static int rome_tlv_download_request(struct hci_dev *hdev,
-				     const struct firmware *fw)
-{
-	const u8 *buffer, *data;
-	int total_segment, remain_size;
-	int ret, i;
-
-	if (!fw || !fw->data)
-		return -EINVAL;
-
-	total_segment = fw->size / MAX_SIZE_PER_TLV_SEGMENT;
-	remain_size = fw->size % MAX_SIZE_PER_TLV_SEGMENT;
-
-	BT_DBG("%s: Total segment num %d remain size %d total size %zu",
-	       hdev->name, total_segment, remain_size, fw->size);
-
-	data = fw->data;
-	for (i = 0; i < total_segment; i++) {
-		buffer = data + i * MAX_SIZE_PER_TLV_SEGMENT;
-		ret = rome_tlv_send_segment(hdev, i, MAX_SIZE_PER_TLV_SEGMENT,
-					    buffer);
-		if (ret < 0)
-			return -EIO;
-	}
-
-	if (remain_size) {
-		buffer = data + total_segment * MAX_SIZE_PER_TLV_SEGMENT;
-		ret = rome_tlv_send_segment(hdev, total_segment, remain_size,
-					    buffer);
-		if (ret < 0)
-			return -EIO;
-	}
-
-	return 0;
-}
-
 static int rome_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {
 	const struct firmware *fw;
-	int ret;
+	const u8 *segment;
+	int ret, remain, i = 0;
 
 	bt_dev_info(hdev, "ROME Downloading %s", config->fwname);
 
@@ -298,10 +280,24 @@ static int rome_download_firmware(struct hci_dev *hdev,
 
 	rome_tlv_check_data(config, fw);
 
-	ret = rome_tlv_download_request(hdev, fw);
-	if (ret) {
-		BT_ERR("%s: Failed to download file: %s (%d)", hdev->name,
-		       config->fwname, ret);
+	segment = fw->data;
+	remain = fw->size;
+	while (remain > 0) {
+		int segsize = min(MAX_SIZE_PER_TLV_SEGMENT, remain);
+
+		bt_dev_dbg(hdev, "Send segment %d, size %d", i++, segsize);
+
+		remain -= segsize;
+		/* The last segment is always acked regardless download mode */
+		if (!remain || segsize < MAX_SIZE_PER_TLV_SEGMENT)
+			config->dnld_mode = ROME_SKIP_EVT_NONE;
+
+		ret = rome_tlv_send_segment(hdev, segsize, segment,
+					    config->dnld_mode);
+		if (ret)
+			break;
+
+		segment += segsize;
 	}
 
 	release_firmware(fw);
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index 65e994b..13d77fd 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -61,6 +61,13 @@ enum qca_bardrate {
 	QCA_BAUDRATE_RESERVED
 };
 
+enum rome_tlv_dnld_mode {
+	ROME_SKIP_EVT_NONE,
+	ROME_SKIP_EVT_VSE,
+	ROME_SKIP_EVT_CC,
+	ROME_SKIP_EVT_VSE_CC
+};
+
 enum rome_tlv_type {
 	TLV_TYPE_PATCH = 1,
 	TLV_TYPE_NVM
@@ -70,6 +77,7 @@ struct rome_config {
 	u8 type;
 	char fwname[64];
 	uint8_t user_baud_rate;
+	enum rome_tlv_dnld_mode dnld_mode;
 };
 
 struct edl_event_hdr {
@@ -94,7 +102,8 @@ struct tlv_type_patch {
 	__le32 data_length;
 	__u8   format_version;
 	__u8   signature;
-	__le16 reserved1;
+	__u8   download_mode;
+	__u8   reserved1;
 	__le16 product_id;
 	__le16 rom_build;
 	__le16 patch_version;
-- 
2.7.4

Re: [PATCH 1/2] Bluetooth: Add __hci_cmd_sync_noev function

[Marcel Holtmann]

Hi Loic,

> This function allows to send a HCI command without expecting any
> controller event/response in return.
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
> include/net/bluetooth/hci_core.h |  2 ++
> net/bluetooth/hci_core.c         | 17 +++++++++++++++++
> 2 files changed, 19 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index b619a19..d48a7df 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1393,6 +1393,8 @@ struct sk_buff *__hci_cmd_sync(struct hci_dev *hdev, u16 opcode, u32 plen,
> 			       const void *param, u32 timeout);
> struct sk_buff *__hci_cmd_sync_ev(struct hci_dev *hdev, u16 opcode, u32 plen,
> 				  const void *param, u8 event, u32 timeout);
> +int __hci_cmd_sync_noev(struct hci_dev *hdev, u16 opcode, u32 plen,
> +			const void *param);
> 
> int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen,
> 		 const void *param);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 40d260f..2831c4e 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3459,6 +3459,23 @@ struct sk_buff *hci_cmd_sync(struct hci_dev *hdev, u16 opcode, u32 plen,
> }
> EXPORT_SYMBOL(hci_cmd_sync);
> 
> +int __hci_cmd_sync_noev(struct hci_dev *hdev, u16 opcode, u32 plen,
> +			const void *param)
> +{
> +	struct sk_buff *skb = hci_prepare_cmd(hdev, opcode, plen, param);
> +
> +	if (!skb) {
> +		bt_dev_err(hdev, "no memory for command (opcode 0x%4.4x)",
> +			   opcode);
> +		return -ENOMEM;
> +	}
> +
> +	hci_send_frame(hdev, skb);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(__hci_cmd_sync_noev);

so this is not a _sync command. You are not waiting for the completion. So this is more like __hci_cmd_send or __hci_cmd_submit. I am also debating if this should be limited to vendor OGF only, meaning it should throw an error for all others.

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: btqca: Add AR3002 rampatch support

[Marcel Holtmann]

Hi Loic,

> This patch adds rampatch download compatibility for ROME >= 3.2.
> Starting with ROME 3.2, the 'download mode' field of the rampatch
> header indicates if the controller acknowledges (or not) the received
> rampatch segments. If not, we need to send all the segments without
> expecting any event from the controller (except for the last segment).
> Goal is (I assume) to speed-up rampatch download.

WHYYYYYYYYYY ???

I have done the measurement with the Intel chips and it is insignificant on Linux. The Linux USB subsystem is a lot better than the one from Windows. Is there any chance we can just switch this back on and keep waiting for the event?

> This fixes BT on Dragonboard-600c P2 which includes the following BT
> controller:
> 
> hci0: ROME Patch Version Request
> hci0: Product:0x00000008
> hci0: Patch  :0x00000111
> hci0: ROM    :0x00000302
> hci0: SOC    :0x00000023
> 
> Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> ---
> drivers/bluetooth/btqca.c | 106 ++++++++++++++++++++++------------------------
> drivers/bluetooth/btqca.h |  11 ++++-
> 2 files changed, 61 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index 2793d41..129779e 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -127,28 +127,42 @@ static void rome_tlv_check_data(struct rome_config *config,
> 	BT_DBG("TLV Type\t\t : 0x%x", type_len & 0x000000ff);
> 	BT_DBG("Length\t\t : %d bytes", length);
> 
> +	config->dnld_mode = ROME_SKIP_EVT_NONE;
> +
> 	switch (config->type) {
> 	case TLV_TYPE_PATCH:
> 		tlv_patch = (struct tlv_type_patch *)tlv->data;
> -		BT_DBG("Total Length\t\t : %d bytes",
> +
> +		/* For Rome version 1.1 to 3.1, all segment commands
> +		 * are acked by a vendor specific event (VSE).
> +		 * For Rome >= 3.2, the download mode field indicates
> +		 * if VSE is skipped by the controller.
> +		 * In case VSE is skipped, only the last segment is acked.
> +		 */
> +		if (le16_to_cpu(tlv_patch->rom_build) >= 0x0302)
> +			config->dnld_mode = tlv_patch->download_mode;
> +
> +		BT_DBG("Total Length           : %d bytes",
> 		       le32_to_cpu(tlv_patch->total_size));
> -		BT_DBG("Patch Data Length\t : %d bytes",
> +		BT_DBG("Patch Data Length      : %d bytes",
> 		       le32_to_cpu(tlv_patch->data_length));
> 		BT_DBG("Signing Format Version : 0x%x",
> 		       tlv_patch->format_version);
> -		BT_DBG("Signature Algorithm\t : 0x%x",
> +		BT_DBG("Signature Algorithm    : 0x%x",
> 		       tlv_patch->signature);
> -		BT_DBG("Reserved\t\t : 0x%x",
> -		       le16_to_cpu(tlv_patch->reserved1));
> -		BT_DBG("Product ID\t\t : 0x%04x",
> +		BT_DBG("Download mode          : 0x%x",
> +		       tlv_patch->download_mode);
> +		BT_DBG("Reserved               : 0x%x",
> +		       tlv_patch->reserved1);
> +		BT_DBG("Product ID             : 0x%04x",
> 		       le16_to_cpu(tlv_patch->product_id));
> -		BT_DBG("Rom Build Version\t : 0x%04x",
> +		BT_DBG("Rom Build Version      : 0x%04x",
> 		       le16_to_cpu(tlv_patch->rom_build));
> -		BT_DBG("Patch Version\t\t : 0x%04x",
> +		BT_DBG("Patch Version          : 0x%04x",
> 		       le16_to_cpu(tlv_patch->patch_version));
> -		BT_DBG("Reserved\t\t : 0x%x",
> +		BT_DBG("Reserved               : 0x%x",
> 		       le16_to_cpu(tlv_patch->reserved2));
> -		BT_DBG("Patch Entry Address\t : 0x%x",
> +		BT_DBG("Patch Entry Address    : 0x%x",
> 		       le32_to_cpu(tlv_patch->entry));
> 		break;
> 
> @@ -194,8 +208,8 @@ static void rome_tlv_check_data(struct rome_config *config,
> 	}
> }
> 
> -static int rome_tlv_send_segment(struct hci_dev *hdev, int idx, int seg_size,
> -				 const u8 *data)
> +static int rome_tlv_send_segment(struct hci_dev *hdev, int seg_size,
> +				 const u8 *data, enum rome_tlv_dnld_mode mode)
> {
> 	struct sk_buff *skb;
> 	struct edl_event_hdr *edl;
> @@ -203,12 +217,15 @@ static int rome_tlv_send_segment(struct hci_dev *hdev, int idx, int seg_size,
> 	u8 cmd[MAX_SIZE_PER_TLV_SEGMENT + 2];
> 	int err = 0;
> 
> -	BT_DBG("%s: Download segment #%d size %d", hdev->name, idx, seg_size);
> -
> 	cmd[0] = EDL_PATCH_TLV_REQ_CMD;
> 	cmd[1] = seg_size;
> 	memcpy(cmd + 2, data, seg_size);
> 
> +	if (mode == ROME_SKIP_EVT_VSE_CC || mode == ROME_SKIP_EVT_VSE) {
> +		return __hci_cmd_sync_noev(hdev, EDL_PATCH_CMD_OPCODE,
> +					   seg_size + 2, cmd);
> +	}
> +

No need for {} here.

> 	skb = __hci_cmd_sync_ev(hdev, EDL_PATCH_CMD_OPCODE, seg_size + 2, cmd,
> 				HCI_VENDOR_PKT, HCI_INIT_TIMEOUT);
> 	if (IS_ERR(skb)) {
> @@ -245,47 +262,12 @@ static int rome_tlv_send_segment(struct hci_dev *hdev, int idx, int seg_size,
> 	return err;
> }
> 
> -static int rome_tlv_download_request(struct hci_dev *hdev,
> -				     const struct firmware *fw)
> -{
> -	const u8 *buffer, *data;
> -	int total_segment, remain_size;
> -	int ret, i;
> -
> -	if (!fw || !fw->data)
> -		return -EINVAL;
> -
> -	total_segment = fw->size / MAX_SIZE_PER_TLV_SEGMENT;
> -	remain_size = fw->size % MAX_SIZE_PER_TLV_SEGMENT;
> -
> -	BT_DBG("%s: Total segment num %d remain size %d total size %zu",
> -	       hdev->name, total_segment, remain_size, fw->size);
> -
> -	data = fw->data;
> -	for (i = 0; i < total_segment; i++) {
> -		buffer = data + i * MAX_SIZE_PER_TLV_SEGMENT;
> -		ret = rome_tlv_send_segment(hdev, i, MAX_SIZE_PER_TLV_SEGMENT,
> -					    buffer);
> -		if (ret < 0)
> -			return -EIO;
> -	}
> -
> -	if (remain_size) {
> -		buffer = data + total_segment * MAX_SIZE_PER_TLV_SEGMENT;
> -		ret = rome_tlv_send_segment(hdev, total_segment, remain_size,
> -					    buffer);
> -		if (ret < 0)
> -			return -EIO;
> -	}
> -
> -	return 0;
> -}
> -
> static int rome_download_firmware(struct hci_dev *hdev,
> 				  struct rome_config *config)
> {
> 	const struct firmware *fw;
> -	int ret;
> +	const u8 *segment;
> +	int ret, remain, i = 0;
> 
> 	bt_dev_info(hdev, "ROME Downloading %s", config->fwname);
> 
> @@ -298,10 +280,24 @@ static int rome_download_firmware(struct hci_dev *hdev,
> 
> 	rome_tlv_check_data(config, fw);
> 
> -	ret = rome_tlv_download_request(hdev, fw);
> -	if (ret) {
> -		BT_ERR("%s: Failed to download file: %s (%d)", hdev->name,
> -		       config->fwname, ret);
> +	segment = fw->data;
> +	remain = fw->size;
> +	while (remain > 0) {
> +		int segsize = min(MAX_SIZE_PER_TLV_SEGMENT, remain);
> +
> +		bt_dev_dbg(hdev, "Send segment %d, size %d", i++, segsize);
> +
> +		remain -= segsize;
> +		/* The last segment is always acked regardless download mode */
> +		if (!remain || segsize < MAX_SIZE_PER_TLV_SEGMENT)
> +			config->dnld_mode = ROME_SKIP_EVT_NONE;
> +
> +		ret = rome_tlv_send_segment(hdev, segsize, segment,
> +					    config->dnld_mode);
> +		if (ret)
> +			break;
> +
> +		segment += segsize;
> 	}
> 
> 	release_firmware(fw);
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index 65e994b..13d77fd 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -61,6 +61,13 @@ enum qca_bardrate {
> 	QCA_BAUDRATE_RESERVED
> };
> 
> +enum rome_tlv_dnld_mode {
> +	ROME_SKIP_EVT_NONE,
> +	ROME_SKIP_EVT_VSE,
> +	ROME_SKIP_EVT_CC,
> +	ROME_SKIP_EVT_VSE_CC
> +};
> +
> enum rome_tlv_type {
> 	TLV_TYPE_PATCH = 1,
> 	TLV_TYPE_NVM
> @@ -70,6 +77,7 @@ struct rome_config {
> 	u8 type;
> 	char fwname[64];
> 	uint8_t user_baud_rate;
> +	enum rome_tlv_dnld_mode dnld_mode;
> };
> 
> struct edl_event_hdr {
> @@ -94,7 +102,8 @@ struct tlv_type_patch {
> 	__le32 data_length;
> 	__u8   format_version;
> 	__u8   signature;
> -	__le16 reserved1;
> +	__u8   download_mode;
> +	__u8   reserved1;
> 	__le16 product_id;
> 	__le16 rom_build;
> 	__le16 patch_version;

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: btqca: Add AR3002 rampatch support

[Loic Poulain]

Hi Marcel,

+Balakrishna Godavarthi, I've just seen that his recent patch for
wcn3990 support deals with the same download issue.

On 24 April 2018 at 17:13, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Loic,
>
>> This patch adds rampatch download compatibility for ROME >= 3.2.
>> Starting with ROME 3.2, the 'download mode' field of the rampatch
>> header indicates if the controller acknowledges (or not) the received
>> rampatch segments. If not, we need to send all the segments without
>> expecting any event from the controller (except for the last segment).
>> Goal is (I assume) to speed-up rampatch download.
>
> WHYYYYYYYYYY ???
>
> I have done the measurement with the Intel chips and it is insignificant on Linux. The Linux USB subsystem is a lot better than the one from Windows.

To be honest, I have no much information (so maybe Windows
optimization is the point), I mainly extracted info from a Yocto
hciattach patch:
https://github.com/boundarydevices/meta-boundary/blob/krogoth/recipes-connectivity/bluez5/bluez5/0001-hciattach-add-QCA9377-Tuffello-support.patch

My module version use HCI UART as transport layer.


> Is there any chance we can just switch this back on and keep waiting for the event?

I tried to change the fw/rampatch header on  the fly to 'standard
download mode' before starting flashing.
It seems to be supported since pretty all segments are correctly sent
and acked by the vendor event.
However the last segment is nacked:

    Bluetooth: hci0: TLV with error stat 0x0 rtype 0x4 (0x3)

I suppose the rampatch contains a checksum which is verified once
download is complete, hacking the header would cause a corruption of
the rampatch, leading to this NACK.
So in theory, the download behavior seems to depend only on the
rampatch file(header) and is not hard-coded on controller side.

Since rampatchs are not under our control, I think it's good to
support this download mode anyway.

Regards,
Loic

Re: [PATCH 1/2] Bluetooth: Add __hci_cmd_sync_noev function

[Loic Poulain]

Hi Marcel,

>
> so this is not a _sync command. You are not waiting for the completion. So this is more like __hci_cmd_send or __hci_cmd_submit. I am also debating if this should be limited to vendor OGF only, meaning it should throw an error for all others.

This makes sense, I agree we should not allow standard HCI command to
be unresponded and strictly follow the specification.
I think __hci_cmd_send/submit is a bit too generic and we already have
a hci_send_cmd which has a different behavior, don't want to cause
confusion. So what about __hci_cmd_noresp or __hci_cmd_vendor...?

Regards,
Loic

Re: [PATCH 1/2] Bluetooth: Add __hci_cmd_sync_noev function

[Marcel Holtmann]

Hi Loic,

>> so this is not a _sync command. You are not waiting for the completion. So this is more like __hci_cmd_send or __hci_cmd_submit. I am also debating if this should be limited to vendor OGF only, meaning it should throw an error for all others.
> 
> This makes sense, I agree we should not allow standard HCI command to
> be unresponded and strictly follow the specification.
> I think __hci_cmd_send/submit is a bit too generic and we already have
> a hci_send_cmd which has a different behavior, don't want to cause
> confusion. So what about __hci_cmd_noresp or __hci_cmd_vendor...?

so perfect would be that we have at least a “sync” command in that sense that we know the controller has send it to the hardware. However from a driver framework point of view we do not have this concept. And we don’t have it, because that kind of “flow control” is not needed since we have HCI command and also ACL packet flow control that should be used.

My concern is that you can easily overload the controller since there are no checks and balances from a HCI point of view. The transport might have some, but otherwise you push things in a queue and hope that all goes well.

Anyway, I am against __hci_cmd_vendor since that is a bit misleading and the _noresp seems a bit off as well. We could do __hci_cmd_dispatch or __hci_cmd_post. I am not convinced that __hci_cmd_send would be actually that bad. It is essentially the just that (except it bypasses the hdev->cmd_q.

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: btqca: Add AR3002 rampatch support

[bgodavar]

Hi Loic,

On 2018-04-25 13:28, Loic Poulain wrote:
> Hi Marcel,
> 
> +Balakrishna Godavarthi, I've just seen that his recent patch for
> wcn3990 support deals with the same download issue.
> 
> On 24 April 2018 at 17:13, Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi Loic,
>> 
>>> This patch adds rampatch download compatibility for ROME >= 3.2.
>>> Starting with ROME 3.2, the 'download mode' field of the rampatch
>>> header indicates if the controller acknowledges (or not) the received
>>> rampatch segments. If not, we need to send all the segments without
>>> expecting any event from the controller (except for the last 
>>> segment).
>>> Goal is (I assume) to speed-up rampatch download.
>> 
>> WHYYYYYYYYYY ???
>> 
>> I have done the measurement with the Intel chips and it is 
>> insignificant on Linux. The Linux USB subsystem is a lot better than 
>> the one from Windows.
> 
> To be honest, I have no much information (so maybe Windows
> optimization is the point), I mainly extracted info from a Yocto
> hciattach patch:
> https://github.com/boundarydevices/meta-boundary/blob/krogoth/recipes-connectivity/bluez5/bluez5/0001-hciattach-add-QCA9377-Tuffello-support.patch
> 
> My module version use HCI UART as transport layer.
> 
> 
>> Is there any chance we can just switch this back on and keep waiting 
>> for the event?
> 
> I tried to change the fw/rampatch header on  the fly to 'standard
> download mode' before starting flashing.
> It seems to be supported since pretty all segments are correctly sent
> and acked by the vendor event.
> However the last segment is nacked:
> 
>     Bluetooth: hci0: TLV with error stat 0x0 rtype 0x4 (0x3)
> 
> I suppose the rampatch contains a checksum which is verified once
> download is complete, hacking the header would cause a corruption of
> the rampatch, leading to this NACK.
> So in theory, the download behavior seems to depend only on the
> rampatch file(header) and is not hard-coded on controller side.
> 
> Since rampatchs are not under our control, I think it's good to
> support this download mode anyway.
> 
> Regards,
> Loic

Thank you for adding me to loop, is the header size of RAM patch file 
remains same for ROME < 3.2 and ROME >= 3.2, what about the header 
format, do we have same header format for different ROME versions, even 
mismatch of header will not break any execution, but it may show wrong 
data in kernel logs.


Regards
Balakrishna.

Re: [PATCH 1/2] Bluetooth: Add __hci_cmd_sync_noev function

[Loic Poulain]

Hi Marcel,

On 25 April 2018 at 14:10, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Loic,
>
>>> so this is not a _sync command. You are not waiting for the completion.=
 So this is more like __hci_cmd_send or __hci_cmd_submit. I am also debatin=
g if this should be limited to vendor OGF only, meaning it should throw an =
error for all others.
>>
>> This makes sense, I agree we should not allow standard HCI command to
>> be unresponded and strictly follow the specification.
>> I think __hci_cmd_send/submit is a bit too generic and we already have
>> a hci_send_cmd which has a different behavior, don't want to cause
>> confusion. So what about __hci_cmd_noresp or __hci_cmd_vendor...?
>
> so perfect would be that we have at least a =E2=80=9Csync=E2=80=9D comman=
d in that sense that we know the controller has send it to the hardware. Ho=
wever from a driver framework point of view we do not have this concept. An=
d we don=E2=80=99t have it, because that kind of =E2=80=9Cflow control=E2=
=80=9D is not needed since we have HCI command and also ACL packet flow con=
trol that should be used.
>
> My concern is that you can easily overload the controller since there are=
 no checks and balances from a HCI point of view. The transport might have =
some, but otherwise you push things in a queue and hope that all goes well.

Yes, we are out of spec here and the only overload protection is UART
hardware flow control in this case.

> Anyway, I am against __hci_cmd_vendor since that is a bit misleading and =
the _noresp seems a bit off as well. We could do __hci_cmd_dispatch or __hc=
i_cmd_post. I am not convinced that __hci_cmd_send would be actually that b=
ad. It is essentially the just that (except it bypasses the hdev->cmd_q.

Fair enough.

Regards,
Loic

Re: [PATCH 2/2] Bluetooth: btqca: Add AR3002 rampatch support

[Loic Poulain]

Hi Balakrishna,

On 25 April 2018 at 14:16,  <bgodavar@codeaurora.org> wrote:
>
> Thank you for adding me to loop, is the header size of RAM patch file
> remains same for ROME < 3.2 and ROME >= 3.2, what about the header format,
> do we have same header format for different ROME versions, even mismatch of
> header will not break any execution, but it may show wrong data in kernel
> logs.

Yes, at least in my case, we just use byte 7 which was previously 'reserved':

 struct edl_event_hdr {
@@ -94,7 +102,8 @@ struct tlv_type_patch {
        __le32 data_length;
        __u8   format_version;
        __u8   signature;
-       __le16 reserved1;
+       __u8   download_mode;
+       __u8   reserved1;
        __le16 product_id;
        __le16 rom_build;
        __le16 patch_version;

Regards,
Loic

Re: [PATCH 2/2] Bluetooth: btqca: Add AR3002 rampatch support

[bgodavar]

Hi Loic,

On 2018-04-25 19:07, Loic Poulain wrote:
> Hi Balakrishna,
> 
> On 25 April 2018 at 14:16,  <bgodavar@codeaurora.org> wrote:
>> 
>> Thank you for adding me to loop, is the header size of RAM patch file
>> remains same for ROME < 3.2 and ROME >= 3.2, what about the header 
>> format,
>> do we have same header format for different ROME versions, even 
>> mismatch of
>> header will not break any execution, but it may show wrong data in 
>> kernel
>> logs.
> 
> Yes, at least in my case, we just use byte 7 which was previously 
> 'reserved':
> 
>  struct edl_event_hdr {
> @@ -94,7 +102,8 @@ struct tlv_type_patch {
>         __le32 data_length;
>         __u8   format_version;
>         __u8   signature;
> -       __le16 reserved1;
> +       __u8   download_mode;
> +       __u8   reserved1;
>         __le16 product_id;
>         __le16 rom_build;
>         __le16 patch_version;
> 
> Regards,
> Loic

A small suggestion from my side, suppose in near future ROME <v3.2 
wanted to go with new format i.e. acknowledging for last segment of RAM 
patch.
then checking rom_build field may not work.
i.e.
if (le16_to_cpu(tlv_patch->rom_build) >= 0x0302)
config->dnld_mode = tlv_patch->download_mode;

i suggest you to check the byte download_mode directly and decide to 
wait for ACK from SoC, for every RAM patch segment sent.
i think byte download_mode is explicitly for mode of downloading the 
patch i.e. with ACK or NACK. (pls double confirm about this byte).

This is how we are doing for wcn3990 (in Add support wcn3990 patch).

Regards
Balakrishna.

Re: [PATCH 2/2] Bluetooth: btqca: Add AR3002 rampatch support

[Loic Poulain]

Hi Balakrishna,

> A small suggestion from my side, suppose in near future ROME <v3.2 wanted to
> go with new format i.e. acknowledging for last segment of RAM patch.
> then checking rom_build field may not work.
> i.e.
> if (le16_to_cpu(tlv_patch->rom_build) >= 0x0302)
> config->dnld_mode = tlv_patch->download_mode;
>
> i suggest you to check the byte download_mode directly and decide to wait
> for ACK from SoC, for every RAM patch segment sent.
> i think byte download_mode is explicitly for mode of downloading the patch
> i.e. with ACK or NACK. (pls double confirm about this byte).

Yes AFAIK, this byte is for download mode or reserved (0). So yes we
can unconditionally check it to determine the mode.

Regards,
Loic