Bug fixes for Qualcomm BT chip wcn3990.

wcn3990 requires a power pulse to turn ON/OFF along with regulators. Sometimes we are observing the power pulses are sent out with some time delay, due to queuing these commands. This is causing synchronization issues with chip, which intern delay the chip setup or may end up with communication issues.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
Changes in v7:
* updated the wait time to 5 ms after sending power pulses.

Changes in v6:
* added serdev_device_write_flush() in qca_send_power_pulse instead during the power off pulse.

Changes in v5:
* added serdev_device_write_flush() in qca_power_off().

---
 drivers/bluetooth/hci_qca.c | 43 ++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

drivers/bluetooth/hci_qca.c +9 -13

index f036c8f98ea3..2aab34b3e696 100644
@@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
 		hci_uart_set_baudrate(hu, speed);
 }
 
-static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
+static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
 {
-	struct hci_uart *hu = hci_get_drvdata(hdev);
-	struct qca_data *qca = hu->priv;
-	struct sk_buff *skb;
+	int ret;
 
 	/* These power pulses are single byte command which are sent
 	 * at required baudrate to wcn3990. On wcn3990, we have an external

@@ -1029,22 +1027,23 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
 	 * save power. Disabling hardware flow control is mandatory while
 	 * sending power pulses to SoC.
 	 */
-	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
-
-	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
-	if (!skb)
-		return -ENOMEM;
+	bt_dev_dbg(hu->hdev, "sending power pulse %02x to controller", cmd);
 
+	serdev_device_write_flush(hu->serdev);
 	hci_uart_set_flow_control(hu, true);
+	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
+	if (ret < 0) {
+		bt_dev_err(hu->hdev, "failed to send power pulse %02x", cmd);
+		return ret;
+	}
 
-	skb_put_u8(skb, cmd);
-	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
-
-	skb_queue_tail(&qca->txq, skb);

drivers/bluetooth/hci_qca.c +2 -4

 	hci_uart_set_flow_control(hu, false);
 
 	return 0;

@@ -1116,7 +1115,6 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 
 static int qca_wcn3990_init(struct hci_uart *hu)
 {
-	struct hci_dev *hdev = hu->hdev;
 	struct qca_serdev *qcadev;
 	int ret;
 

@@ -1139,12 +1137,12 @@ static int qca_wcn3990_init(struct hci_uart *hu)
 
 	/* Forcefully enable wcn3990 to enter in to boot mode. */
 	host_set_baudrate(hu, 2400);
-	ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE);
+	ret = qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE);
 	if (ret)
 		return ret;
 
 	qca_set_speed(hu, QCA_INIT_SPEED);
-	ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
+	ret = qca_send_power_pulse(hu, QCA_WCN3990_POWERON_PULSE);
 	if (ret)
 		return ret;
 

@@ -1274,13 +1272,8 @@ static const struct qca_vreg_data qca_soc_data = {
 
 static void qca_power_shutdown(struct hci_uart *hu)
 {
-	struct serdev_device *serdev = hu->serdev;

During hci down we observed IBS sleep commands are queued in the Tx buffer and hci_uart_write_work is sending data to the chip which is not required as the chip is powered off. This patch will disable IBS and flush the Tx buffer before we turn off the chip.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
 drivers/bluetooth/hci_qca.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/bluetooth/hci_qca.c +0 -0

index b79ef901ab8b..9d5e41f159c7 100644

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
 drivers/bluetooth/btqca.c | 39 ++++++++++++++++++++++++++++++++++++++-
 drivers/bluetooth/btqca.h |  3 +++
 2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +38 -1

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev *hdev, int seg_size,
 	return err;
 }
 
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;
+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int qca_download_firmware(struct hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 
 		segment += segsize;
 	}
 
+	/* Latest qualcomm chipsets are not sending a command complete event
+	 * for every fw packet sent. They only respond with a vendor specific
+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will inject a command
+	 * complete event to avoid a command timeout error message.
+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+
+out:
 	release_firmware(fw);
 
 	return ret;

drivers/bluetooth/btqca.h +2 -0

index 0c01f375fe83..5c8fc54133e3 100644
@@ -40,6 +40,8 @@
 #define QCA_WCN3990_POWERON_PULSE	0xFC
 #define QCA_WCN3990_POWEROFF_PULSE	0xC0
 
+#define QCA_HCI_CC_SUCCESS		0x00
+
 enum qca_bardrate {
 	QCA_BAUDRATE_115200 	= 0,
 	QCA_BAUDRATE_57600,

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39 ++++++++++++++++++++++++++++++++++++++- drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +27 -1

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev *hdev, int seg_size,
 	return err;
 }
 
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;
+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int qca_download_firmware(struct hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 
 		segment += segsize;
 	}
 

Hi Balakrishna,

Quoted 71 lines

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39 ++++++++++++++++++++++++++++++++++++++- drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +1 -0

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct rome_config 
 *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 
 		BT_DBG("Total Length           : %d bytes",

		       le32_to_cpu(tlv_patch->total_size));

drivers/bluetooth/btqca.c +25 -0

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev 
 *hdev, int seg_size,
 	return err;
 }
 
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;
+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)

{

drivers/bluetooth/btqca.c +1 -1

@@ -297,11 +323,22 @@ static int qca_download_firmware(struct hci_dev 
 *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 
 		segment += segsize;
 	}
 

drivers/bluetooth/btqca.c static int qca_download_firmware(struct hci_dev *hdev, · line 340

Quoted 12 lines

drivers/bluetooth/btqca.c +2 -0

+	/* Latest qualcomm chipsets are not sending a command complete event
+	 * for every fw packet sent. They only respond with a vendor 

specific

drivers/bluetooth/btqca.c +2 -0

+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will inject a 

command

drivers/bluetooth/btqca.c +6 -0

+	 * complete event to avoid a command timeout error message.
+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

Regards

Marcel

Quoted 67 lines

Quoted 67 lines

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39 ++++++++++++++++++++++++++++++++++++++- drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +32 -1

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev *hdev, int seg_size,
 	return err;
 }
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;
+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int qca_download_firmware(struct hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 		segment += segsize;
 	}
+	/* Latest qualcomm chipsets are not sending a command complete event
+	 * for every fw packet sent. They only respond with a vendor specific
+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will inject a command
+	 * complete event to avoid a command timeout error message.

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_download_firmware(struct hci_dev *hdev, · line 340

Quoted 5 lines

drivers/bluetooth/btqca.c +5 -0

+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event. but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

Hi Balakrishna,

Quoted 82 lines

Quoted 74 lines

Quoted 74 lines

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +28 -1

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct 
 rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev 
 *hdev, int seg_size,
 	return err;
 }
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;
+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int qca_download_firmware(struct 
 hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 		segment += segsize;
 	}
+	/* Latest qualcomm chipsets are not sending a command complete 

event

drivers/bluetooth/btqca.c +1 -0

+	 * for every fw packet sent. They only respond with a vendor 

specific

drivers/bluetooth/btqca.c +2 -0

+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will inject a 

command

drivers/bluetooth/btqca.c +1 -0

+	 * complete event to avoid a command timeout error message.

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_download_firmware(struct hci_dev *hdev, · line 340

Quoted 5 lines

drivers/bluetooth/btqca.c +5 -0

+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

Quoted 6 lines

[Bala]: thanks for reviewing the change.

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event. but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

Hi Marcel,

Quoted 97 lines · Marcel Holtmann

Hi Balakrishna,

Quoted 82 lines

Quoted 74 lines

Quoted 74 lines

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +28 -1

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct 
 rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev 
 *hdev, int seg_size,
 	return err;
 }
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;
+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int qca_download_firmware(struct 
 hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 		segment += segsize;
 	}
+	/* Latest qualcomm chipsets are not sending a command complete 

event

drivers/bluetooth/btqca.c +1 -0

+	 * for every fw packet sent. They only respond with a vendor 

specific

drivers/bluetooth/btqca.c +2 -0

+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will inject a 

command

drivers/bluetooth/btqca.c +1 -0

+	 * complete event to avoid a command timeout error message.

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_download_firmware(struct hci_dev *hdev, · line 340

Quoted 5 lines

drivers/bluetooth/btqca.c +5 -0

+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

Quoted 6 lines

[Bala]: thanks for reviewing the change.

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event. but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

Quoted 7 lines

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

[Bala]: here is the use case, when ever we download the fw packets i.e. RAM image, for every command sent(i.e. fw packet) from the host chip will respond with an vendor specific event and command complete event.

the above is taking more time to setup the BT device. then we came up with solution where we enable flags in fw file (i.e. RAM image header) whether to wait for event to be received or sent the total packets and wait for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for event for all packet or wait for an event for the last packet.

but in the second case i.e. wait for event for the last packet sent, we are only receiving an vendor specific event from chip which holds the status of fw download.

so as __hci_cmd_sync_ev() requires an command complete event. so we are injecting it after the vendor specific event received for the last packet.

This helps to overcome 0xfc00 timeout error logging on console.

Hi Marcel,

Quoted 41 lines · Marcel Holtmann

Hi Balakrishna,

Quoted 39 lines

Quoted 39 lines

Quoted 39 lines

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific
event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +10 -0

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
 rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
 hci_dev *hdev, int seg_size,
 	return err;
 }
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_tlv_send_segment(struct hci_dev *hdev, int seg_size, · line 284

Quoted 8 lines

drivers/bluetooth/btqca.c +8 -0

+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;

Quoted 52 lines

Quoted 38 lines

Quoted 28 lines

Quoted 28 lines

drivers/bluetooth/btqca.c +10 -1

+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int
 qca_download_firmware(struct hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 		segment += segsize;
 	}
+	/* Latest qualcomm chipsets are not sending a command

complete event

drivers/bluetooth/btqca.c +1 -0

+	 * for every fw packet sent. They only respond with a

vendor specific

drivers/bluetooth/btqca.c +2 -0

+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will

inject a command

drivers/bluetooth/btqca.c +1 -0

+	 * complete event to avoid a command timeout error message.

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_download_firmware(struct hci_dev *hdev, · line 340

Quoted 5 lines

drivers/bluetooth/btqca.c +5 -0

+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

Quoted 7 lines

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

Quoted 7 lines

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

[Bala]: here is the use case, when ever we download the fw packets i.e. RAM image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command complete event.

the above is taking more time to setup the BT device. then we came up with solution where we enable flags in fw file (i.e. RAM image header) whether to wait for event to be received or sent the total packets and wait for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for event for all packet or wait for an event for the last packet.

but in the second case i.e. wait for event for the last packet sent, we are only receiving an vendor specific event from chip which holds the status of fw download.

so as __hci_cmd_sync_ev() requires an command complete event. so we are injecting it after the vendor specific event received for the last packet.

This helps to overcome 0xfc00 timeout error logging on console.

Quoted 44 lines · Balakrishna Godavarthi

Hi Marcel,

Quoted 41 lines · Marcel Holtmann

Hi Balakrishna,

Quoted 39 lines

Quoted 39 lines

Quoted 39 lines

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific
event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +10 -0

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
 rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
 hci_dev *hdev, int seg_size,
 	return err;
 }
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_tlv_send_segment(struct hci_dev *hdev, int seg_size, · line 284

Quoted 8 lines

drivers/bluetooth/btqca.c +8 -0

+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;

After looking a bit more at it I realize HCI_OP_NOP is not a good value in this case:

static void hci_cmd_complete_evt(...)
{
...

  if (*opcode != HCI_OP_NOP)
    cancel_delayed_work(&hdev->cmd_timer);

...
}

https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351

Cancelling the command timeout is precisely what we want. Not sure why the patch with HCI_OP_NOP makes the timeouts go away in most cases (but not e.g. when inserting an msleep(1000) after downloading the NVM.

I suggest to pass the opcode of the command to be completed.

Quoted 73 lines

Quoted 52 lines

Quoted 38 lines

Quoted 28 lines

Quoted 28 lines

drivers/bluetooth/btqca.c +10 -1

+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int
 qca_download_firmware(struct hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 		segment += segsize;
 	}
+	/* Latest qualcomm chipsets are not sending a command

complete event

drivers/bluetooth/btqca.c +1 -0

+	 * for every fw packet sent. They only respond with a

vendor specific

drivers/bluetooth/btqca.c +2 -0

+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will

inject a command

drivers/bluetooth/btqca.c +1 -0

+	 * complete event to avoid a command timeout error message.

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_download_firmware(struct hci_dev *hdev, · line 340

Quoted 5 lines

drivers/bluetooth/btqca.c +5 -0

+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

Quoted 7 lines

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

Quoted 7 lines

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

[Bala]: here is the use case, when ever we download the fw packets i.e. RAM image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command complete event.

the above is taking more time to setup the BT device. then we came up with solution where we enable flags in fw file (i.e. RAM image header) whether to wait for event to be received or sent the total packets and wait for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for event for all packet or wait for an event for the last packet.

Quoted 8 lines

but in the second case i.e. wait for event for the last packet sent, we are only receiving an vendor specific event from chip which holds the status of fw download.

so as __hci_cmd_sync_ev() requires an command complete event. so we are injecting it after the vendor specific event received for the last packet.

This helps to overcome 0xfc00 timeout error logging on console.

Some more details:

The timeout error is actually from reading the 'SoC version', which uses the same command code as the firmware download (EDL_PATCH_CMD_OPCODE). Without reading the 'SoC version' it would be from the command to write the first firmware segment.

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download, though I expect Marcel wouldn't be overly happy with that, since it would affect btmon even more.

Regards

Matthias

Hi Matthias,

Quoted 181 lines · Matthias Kaehlcke

Quoted 44 lines · Balakrishna Godavarthi

Hi Marcel,

Quoted 41 lines · Marcel Holtmann

Hi Balakrishna,

Quoted 39 lines

Quoted 39 lines

Quoted 39 lines

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific
event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +10 -0

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
 rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
 hci_dev *hdev, int seg_size,
 	return err;
 }
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_tlv_send_segment(struct · line 284

Quoted 8 lines

drivers/bluetooth/btqca.c +8 -0

+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;

After looking a bit more at it I realize HCI_OP_NOP is not a good value in this case:

static void hci_cmd_complete_evt(...)
{
...

  if (*opcode != HCI_OP_NOP)
    cancel_delayed_work(&hdev->cmd_timer);

...
}

https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351

Cancelling the command timeout is precisely what we want. Not sure why the patch with HCI_OP_NOP makes the timeouts go away in most cases (but not e.g. when inserting an msleep(1000) after downloading the NVM.

I suggest to pass the opcode of the command to be completed.

Quoted 83 lines

Quoted 52 lines

Quoted 38 lines

Quoted 28 lines

Quoted 28 lines

drivers/bluetooth/btqca.c +10 -1

+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int
 qca_download_firmware(struct hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 		segment += segsize;
 	}
+	/* Latest qualcomm chipsets are not sending a command

complete event

drivers/bluetooth/btqca.c +1 -0

+	 * for every fw packet sent. They only respond with a

vendor specific

drivers/bluetooth/btqca.c +2 -0

+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will

inject a command

drivers/bluetooth/btqca.c +1 -0

+	 * complete event to avoid a command timeout error message.

Quoted 8 lines

drivers/bluetooth/btqca.c

Quoted 5 lines

drivers/bluetooth/btqca.c +5 -0

+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

Quoted 7 lines

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

Quoted 7 lines

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

[Bala]: here is the use case, when ever we download the fw packets i.e. RAM
image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command complete event.

the above is taking more time to setup the BT device. then we came up with
solution where we enable flags in fw file (i.e. RAM image header) whether to wait for event to be received or sent the total packets and wait
for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for event
for all packet or wait for an event for the last packet.

but in the second case i.e. wait for event for the last packet sent, we are
only receiving an vendor specific event from chip which holds the status of
fw download.

Quoted 6 lines

so as __hci_cmd_sync_ev() requires an command complete event. so we are
injecting it after the vendor specific event received for the last packet.

This helps to overcome 0xfc00 timeout error logging on console.

Some more details:

The timeout error is actually from reading the 'SoC version', which uses the same command code as the firmware download (EDL_PATCH_CMD_OPCODE). Without reading the 'SoC version' it would be from the command to write the first firmware segment.

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download, though I expect Marcel wouldn't be overly happy with that, since it would affect btmon even more.

Regards

Quoted 1 line

Matthias

[Bala]: Basically every vendor specific command we sent to chip, chip should respond with an vendor specific event followed by an command complete event
or some times it will only respond with an command complete event. but in any case command complete event is mandatory to all the command we sent to the chip.

In our case, we have an two fw files i.e. *.tlv and *.bin.
tlv is an RAM image of the chip where as bin is an nvm image of the chip. so tlv will be of
more size which require an lot more time to dump the file in to chip, while dumping the tlv, we divide tlv as packet of size 245 bytes and send them as an command packet to the chip. chip should respond with an command complete event.
then only we will send the next packet. but size of the tlv is large, to optimize this we will
not wait for the either an vendor specific event or an command complete event.

But as we need to be on the sync, i.e. whether are we sending an correct packets or not,
for the last fw packet we sent to the chip.. chip will to do an CRC check for the total no of packets received and respond with an vendor specific event.

We decode the vendor specific event and decide whether the fw download is success or not. here we send an fw packet as command so stack expects an command complete event.
where this is missing from the chip. this is expected behavior from chip.

So currently i am inject an command complete event after receiving an vendor event for the last packet of the tlv.

This we inject only once for the last command packet sent to the chip. i don't think this will effect the btmon.

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download.

Hi Balakrishna,

Quoted 184 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 181 lines · Matthias Kaehlcke

Quoted 44 lines · Balakrishna Godavarthi

Hi Marcel,

Quoted 41 lines · Marcel Holtmann

Hi Balakrishna,

Quoted 39 lines

Quoted 39 lines

Quoted 39 lines

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific
event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +10 -0

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
 rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
 hci_dev *hdev, int seg_size,
 	return err;
 }
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_tlv_send_segment(struct · line 284

Quoted 8 lines

drivers/bluetooth/btqca.c +8 -0

+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;

After looking a bit more at it I realize HCI_OP_NOP is not a good value in this case:

static void hci_cmd_complete_evt(...)
{
...

  if (*opcode != HCI_OP_NOP)
    cancel_delayed_work(&hdev->cmd_timer);

...
}

https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351

Cancelling the command timeout is precisely what we want. Not sure why the patch with HCI_OP_NOP makes the timeouts go away in most cases (but not e.g. when inserting an msleep(1000) after downloading the NVM.

I suggest to pass the opcode of the command to be completed.

Quoted 83 lines

Quoted 52 lines

Quoted 38 lines

Quoted 28 lines

Quoted 28 lines

drivers/bluetooth/btqca.c +10 -1

+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int
 qca_download_firmware(struct hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 		segment += segsize;
 	}
+	/* Latest qualcomm chipsets are not sending a command

complete event

drivers/bluetooth/btqca.c +1 -0

+	 * for every fw packet sent. They only respond with a

vendor specific

drivers/bluetooth/btqca.c +2 -0

+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will

inject a command

drivers/bluetooth/btqca.c +1 -0

+	 * complete event to avoid a command timeout error message.

Quoted 8 lines

drivers/bluetooth/btqca.c

Quoted 5 lines

drivers/bluetooth/btqca.c +5 -0

+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

Quoted 7 lines

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

Quoted 7 lines

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

[Bala]: here is the use case, when ever we download the fw packets i.e. RAM
image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command complete event.

the above is taking more time to setup the BT device. then we came up with
solution where we enable flags in fw file (i.e. RAM image header) whether to wait for event to be received or sent the total packets and wait
for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for event
for all packet or wait for an event for the last packet.

but in the second case i.e. wait for event for the last packet sent, we are
only receiving an vendor specific event from chip which holds the status of
fw download.

Quoted 6 lines

so as __hci_cmd_sync_ev() requires an command complete event. so we are
injecting it after the vendor specific event received for the last packet.

This helps to overcome 0xfc00 timeout error logging on console.

Some more details:

The timeout error is actually from reading the 'SoC version', which uses the same command code as the firmware download (EDL_PATCH_CMD_OPCODE). Without reading the 'SoC version' it would be from the command to write the first firmware segment.

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download, though I expect Marcel wouldn't be overly happy with that, since it would affect btmon even more.

Regards

Quoted 8 lines

Quoted 1 line

Matthias

[Bala]: Basically every vendor specific command we sent to chip, chip should respond with an vendor specific event followed by an command complete event
or some times it will only respond with an command complete event. but in any case command complete event is mandatory to all the command we sent to the chip.

Is this ("command complete event is mandatory to all the command we sent to the chip") a description of what the chip actually does, or what it should be doing according to the spec?

As mentioned earlier, the timeout we see originates from reading the SoC version:

drivers/bluetooth/btqca.c +4 -0

index 0b533f65f652fc..1e484e61799571 100644
@@ -400,6 +400,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t 
 baudrate,
                return err;
        }
 
+       printk("DBG: ZZZzzzzzzz\n");
+       msleep(2500);
+       printk("DBG: good morning!\n");
+
        /* Download NVM configuration */
        config.type = TLV_TYPE_NVM;

        if (soc_type == QCA_WCN3990)

When you boot with this patch you'll see something like this:

[   15.531365] DBG: reading SoC version
[   15.544963] DBG: ZZZzzzzzzz
[   17.590282] Bluetooth: hci0: command 0xfc00 tx timeout
[   18.099110] DBG: good morning!

Quoted 11 lines

In our case, we have an two fw files i.e. *.tlv and *.bin.
tlv is an RAM image of the chip where as bin is an nvm image of the chip. so tlv will be of
more size which require an lot more time to dump the file in to chip, while dumping the tlv, we divide tlv as packet of size 245 bytes and send them as an command packet to the chip. chip should respond with an command complete event.
then only we will send the next packet. but size of the tlv is large, to optimize this we will
not wait for the either an vendor specific event or an command complete event.

Let's make sure we have an accurate picture, which of the following is correct:

1. the chip sends a command complete event after each packet, as an optimization the BT driver doesn't wait for it

2. as an optimization the chip does not send a command complete event and the driver has to deal with it

My understanding is that it's 2), but the wording above seems to describe 1)

Quoted 6 lines

But as we need to be on the sync, i.e. whether are we sending an correct packets or not,
for the last fw packet we sent to the chip.. chip will to do an CRC check for the total no of packets received and respond with an vendor specific event.

Quoted 7 lines

We decode the vendor specific event and decide whether the fw download is success or not. here we send an fw packet as command so stack expects an command complete event.
where this is missing from the chip. this is expected behavior from chip.

So currently i am inject an command complete event after receiving an vendor event for the last packet of the tlv.

And the same for the .bin if I'm not mistaken.

Quoted 2 lines

This we inject only once for the last command packet sent to the chip. i don't think this will effect the btmon.

I don't know enough about btmon to comment on that, in any case Marcel raised concerns.

And I think my comment that triggered this disucssion remains true:

Quoted 3 lines

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download.

Not sure it's a likely case, it might be an issue with larger firmware files and/or slower UART speeds.

Thanks

Matthias

Hi Matthias,

Quoted 222 lines · Matthias Kaehlcke

Hi Balakrishna,

Quoted 186 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 179 lines · Matthias Kaehlcke

Quoted 44 lines · Balakrishna Godavarthi

Hi Marcel,

Quoted 41 lines · Marcel Holtmann

Hi Balakrishna,

Quoted 39 lines

Quoted 39 lines

Quoted 39 lines

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific
event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +10 -0

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
 rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
 hci_dev *hdev, int seg_size,
 	return err;
 }
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_tlv_send_segment(struct · line 284

Quoted 8 lines

drivers/bluetooth/btqca.c +8 -0

+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;

After looking a bit more at it I realize HCI_OP_NOP is not a good value in this case:

static void hci_cmd_complete_evt(...)
{
...

  if (*opcode != HCI_OP_NOP)
    cancel_delayed_work(&hdev->cmd_timer);

...
}

https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351

Cancelling the command timeout is precisely what we want. Not sure why the patch with HCI_OP_NOP makes the timeouts go away in most cases (but not e.g. when inserting an msleep(1000) after downloading the NVM.

I suggest to pass the opcode of the command to be completed.

Quoted 83 lines

Quoted 52 lines

Quoted 38 lines

Quoted 28 lines

Quoted 28 lines

drivers/bluetooth/btqca.c +10 -1

+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int
 qca_download_firmware(struct hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 		segment += segsize;
 	}
+	/* Latest qualcomm chipsets are not sending a command

complete event

drivers/bluetooth/btqca.c +1 -0

+	 * for every fw packet sent. They only respond with a

vendor specific

drivers/bluetooth/btqca.c +2 -0

+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will

inject a command

drivers/bluetooth/btqca.c +1 -0

+	 * complete event to avoid a command timeout error message.

Quoted 8 lines

drivers/bluetooth/btqca.c

Quoted 5 lines

drivers/bluetooth/btqca.c +5 -0

+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

Quoted 7 lines

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

Quoted 7 lines

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

[Bala]: here is the use case, when ever we download the fw packets i.e. RAM
image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command complete event.

the above is taking more time to setup the BT device. then we came up with
solution where we enable flags in fw file (i.e. RAM image header) whether to wait for event to be received or sent the total packets and wait
for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for event
for all packet or wait for an event for the last packet.

but in the second case i.e. wait for event for the last packet sent, we are
only receiving an vendor specific event from chip which holds the status of
fw download.

Quoted 6 lines

so as __hci_cmd_sync_ev() requires an command complete event. so we are
injecting it after the vendor specific event received for the last packet.

This helps to overcome 0xfc00 timeout error logging on console.

Some more details:

The timeout error is actually from reading the 'SoC version', which uses the same command code as the firmware download (EDL_PATCH_CMD_OPCODE). Without reading the 'SoC version' it would be from the command to write the first firmware segment.

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download, though I expect Marcel wouldn't be overly happy with that, since it would affect btmon even more.

Quoted 3 lines

Regards

Matthias

Quoted 8 lines

[Bala]: Basically every vendor specific command we sent to chip, chip should respond with an vendor specific event followed by an command
complete event
or some times it will only respond with an command complete event. but in any case command complete event is mandatory to all the command we
sent to the chip.

Is this ("command complete event is mandatory to all the command we sent to the chip") a description of what the chip actually does, or what it should be doing according to the spec?

As mentioned earlier, the timeout we see originates from reading the SoC version:

drivers/bluetooth/btqca.c +4 -0

index 0b533f65f652fc..1e484e61799571 100644
@@ -400,6 +400,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t
 baudrate,
                return err;
        }
 
+       printk("DBG: ZZZzzzzzzz\n");
+       msleep(2500);
+       printk("DBG: good morning!\n");
+
        /* Download NVM configuration */
        config.type = TLV_TYPE_NVM;

        if (soc_type == QCA_WCN3990)

Quoted 7 lines

When you boot with this patch you'll see something like this:

[   15.531365] DBG: reading SoC version
[   15.544963] DBG: ZZZzzzzzzz
[   17.590282] Bluetooth: hci0: command 0xfc00 tx timeout
[   18.099110] DBG: good morning!

[Bala]: my previous analysis were wrong. thanks for pointing me to the correct issue.
i am able to see timeout after version command.(used some sleep) here is the reason for it.

01 00 fc 01 19 (we send the command to chip to request version) 04 ff 0e 00 02 0a 00 00 00 01 00 01 02 14 02 01 40(chip will respond
with the vendor specific event payload will be chip version)
04 0e 04 01 00 00 00(command complete event)

issue is with command complete event. ideally command complete event
payload holds the command for which chip sends command complete event.

breaking the command

04 : event packet (fixed)
0e : command complete event(fixed)
04 : size of the payload,

01 : no of commands in the payload
00 : OCF (opocde lsb)
00 : OGF (opcode msb)
00 : status of command executed,

payload is form byte 4 to byte 7. which hold the command i.e opcode form the above response opcode is 0x0000. ideally this should be
0xfc00
because the command complete response from the chip is for the command
0xfc00.

Checked with Chip firmware dev team for the reason, it was an
limitation or an bug in the chip ROM firmware
in the current chipset, which is fixed in the coming chipset.

so we expect the command 0xfc00 but recevuies 0x00. so that is the
reason we see an timeouts.

      for all the vendor commands we receive 0x0000 in the command complete

event.

      if comes a new question how are we not seeing an 0xfc00 when we inject

an command complete event.
i experimented this patch, when i inject the command complete event
from the soc version still i can see the command timeouts.

The reason for no timeouts error, we are injecting an command complete
after last packet sent out, so this injection helps
.bin file to speed out it download part, so as the bin is dumped
fastly we send an HCI RESET Comamnd where we recevie an command complete event
with non zero opcode which is cancelling the command timeout timer.

when we inject command complete event, in short we are completing
setup process in less than 2 seconds(command timeout timer value),
so before 2 seconds we are sending an HCI RESET command whose command
complete event opcode value is an non zero (which cancels the command timeout timer)

Quoted 23 lines

Quoted 8 lines

In our case, we have an two fw files i.e. *.tlv and *.bin. tlv is an RAM image of the chip where as bin is an nvm image of the chip. so
tlv will be of
more size which require an lot more time to dump the file in to chip, while dumping the tlv, we divide tlv as packet of size 245 bytes and send
them as an command packet to the chip. chip should respond with an

Quoted 8 lines

command
complete event.
then only we will send the next packet. but size of the tlv is large, to
optimize this we will
not wait for the either an vendor specific event or an command complete
event.

Let's make sure we have an accurate picture, which of the following is correct:

1. the chip sends a command complete event after each packet, as an optimization the BT driver doesn't wait for it

Quoted 6 lines

2. as an optimization the chip does not send a command complete event and the driver has to deal with it

My understanding is that it's 2), but the wording above seems to describe 1)

[Bala]: point 2 is true.

Quoted 38 lines

Quoted 17 lines

But as we need to be on the sync, i.e. whether are we sending an correct
packets or not,
for the last fw packet we sent to the chip.. chip will to do an CRC check
for the total no of packets received and respond with an vendor specific
event.

We decode the vendor specific event and decide whether the fw download is
success or not. here we send an fw packet as command so stack expects an
command complete event.
where this is missing from the chip. this is expected behavior from chip.

Quoted 3 lines

So currently i am inject an command complete event after receiving an vendor
event for the last packet of the tlv.

And the same for the .bin if I'm not mistaken.

Quoted 2 lines

This we inject only once for the last command packet sent to the chip. i don't think this will effect the btmon.

I don't know enough about btmon to comment on that, in any case Marcel raised concerns.

And I think my comment that triggered this disucssion remains true:

Quoted 3 lines

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download.

Not sure it's a likely case, it might be an issue with larger firmware files and/or slower UART speeds.

Quoted 3 lines

Thanks

Matthias

[Bala]: i don't think it is a good idea to handle this kind of limitations in the HOST driver.

Quoted 273 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 222 lines · Matthias Kaehlcke

Hi Balakrishna,

Quoted 186 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 179 lines · Matthias Kaehlcke

Quoted 44 lines · Balakrishna Godavarthi

Hi Marcel,

Quoted 41 lines · Marcel Holtmann

Hi Balakrishna,

Quoted 39 lines

Quoted 39 lines

Quoted 39 lines

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific
event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +10 -0

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
 rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
 hci_dev *hdev, int seg_size,
 	return err;
 }
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_tlv_send_segment(struct · line 284

Quoted 8 lines

drivers/bluetooth/btqca.c +8 -0

+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;

After looking a bit more at it I realize HCI_OP_NOP is not a good value in this case:

static void hci_cmd_complete_evt(...)
{
...

  if (*opcode != HCI_OP_NOP)
    cancel_delayed_work(&hdev->cmd_timer);

...
}

https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351

Cancelling the command timeout is precisely what we want. Not sure why the patch with HCI_OP_NOP makes the timeouts go away in most cases (but not e.g. when inserting an msleep(1000) after downloading the NVM.

I suggest to pass the opcode of the command to be completed.

Quoted 83 lines

Quoted 52 lines

Quoted 38 lines

Quoted 28 lines

Quoted 28 lines

drivers/bluetooth/btqca.c +10 -1

+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int
 qca_download_firmware(struct hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 		segment += segsize;
 	}
+	/* Latest qualcomm chipsets are not sending a command

complete event

drivers/bluetooth/btqca.c +1 -0

+	 * for every fw packet sent. They only respond with a

vendor specific

drivers/bluetooth/btqca.c +2 -0

+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will

inject a command

drivers/bluetooth/btqca.c +1 -0

+	 * complete event to avoid a command timeout error message.

Quoted 8 lines

drivers/bluetooth/btqca.c

Quoted 5 lines

drivers/bluetooth/btqca.c +5 -0

+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

Quoted 7 lines

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

Quoted 7 lines

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

[Bala]: here is the use case, when ever we download the fw packets i.e. RAM
image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command complete event.

the above is taking more time to setup the BT device. then we came up with
solution where we enable flags in fw file (i.e. RAM image header) whether to wait for event to be received or sent the total packets and wait
for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for event
for all packet or wait for an event for the last packet.

but in the second case i.e. wait for event for the last packet sent, we are
only receiving an vendor specific event from chip which holds the status of
fw download.

Quoted 6 lines

so as __hci_cmd_sync_ev() requires an command complete event. so we are
injecting it after the vendor specific event received for the last packet.

This helps to overcome 0xfc00 timeout error logging on console.

Some more details:

The timeout error is actually from reading the 'SoC version', which uses the same command code as the firmware download (EDL_PATCH_CMD_OPCODE). Without reading the 'SoC version' it would be from the command to write the first firmware segment.

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download, though I expect Marcel wouldn't be overly happy with that, since it would affect btmon even more.

Quoted 3 lines

Regards

Matthias

Quoted 8 lines

[Bala]: Basically every vendor specific command we sent to chip, chip should respond with an vendor specific event followed by an command
complete event
or some times it will only respond with an command complete event. but in any case command complete event is mandatory to all the command we
sent to the chip.

Is this ("command complete event is mandatory to all the command we sent to the chip") a description of what the chip actually does, or what it should be doing according to the spec?

As mentioned earlier, the timeout we see originates from reading the SoC version:

drivers/bluetooth/btqca.c +4 -0

index 0b533f65f652fc..1e484e61799571 100644
@@ -400,6 +400,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t
 baudrate,
                return err;
        }
 
+       printk("DBG: ZZZzzzzzzz\n");
+       msleep(2500);
+       printk("DBG: good morning!\n");
+
        /* Download NVM configuration */
        config.type = TLV_TYPE_NVM;

        if (soc_type == QCA_WCN3990)

Quoted 7 lines

When you boot with this patch you'll see something like this:

[   15.531365] DBG: reading SoC version
[   15.544963] DBG: ZZZzzzzzzz
[   17.590282] Bluetooth: hci0: command 0xfc00 tx timeout
[   18.099110] DBG: good morning!

[Bala]: my previous analysis were wrong. thanks for pointing me to the correct issue.
i am able to see timeout after version command.(used some
sleep)
here is the reason for it.

01 00 fc 01 19 (we send the command to chip to request
version)

        04 ff 0e 00 02 0a 00 00 00 01 00 01 02 14 02 01 40(chip will 

respond
with the vendor specific event payload will be chip version)
04 0e 04 01 00 00 00(command complete event)

issue is with command complete event. ideally command complete
event
payload holds the command for which chip sends command complete event.

breaking the command

04 : event packet (fixed)
0e : command complete event(fixed)
04 : size of the payload,

01 : no of commands in the payload
00 : OCF (opocde lsb)
00 : OGF (opcode msb)
00 : status of command executed,

payload is form byte 4 to byte 7. which hold the command i.e
opcode

      form the above response opcode is 0x0000. ideally this should be

0xfc00
because the command complete response from the chip is for the
command
0xfc00.

Checked with Chip firmware dev team for the reason, it was an
limitation or an bug in the chip ROM firmware
in the current chipset, which is fixed in the coming chipset.

Quoted 6 lines

so we expect the command 0xfc00 but recevuies 0x00. so that is
the
reason we see an timeouts.

      for all the vendor commands we receive 0x0000 in the command 

complete
event.

Thanks for your analysis.

It is unfortunate (though not unexpected) that this is a problem in the ROM were we can't fix it, but at least the FW team is aware of it and fixed it for future hardware.

Quoted 6 lines

      if comes a new question how are we not seeing an 0xfc00 when we 

inject
an command complete event.
i experimented this patch, when i inject the command complete
event
from the soc version still i can see the command timeouts.

I don't see command timeouts when injecting a command complete event after reading the SoC version. I suspect you still use patch which injects a command complete event with HCI_OP_NOP, which is precisely what the chip does ...

Quoted 9 lines

The reason for no timeouts error, we are injecting an command
complete
after last packet sent out, so this injection helps
.bin file to speed out it download part, so as the bin is dumped
fastly we send an HCI RESET Comamnd where we recevie an command complete event
with non zero opcode which is cancelling the command timeout
timer.

Quoted 7 lines

when we inject command complete event, in short we are
completing
setup process in less than 2 seconds(command timeout timer value),
so before 2 seconds we are sending an HCI RESET command whose
command
complete event opcode value is an non zero (which cancels the command timeout timer)

I agree that HCI_RESET cancels the command timeout timer if it is sent before the 2s timout expires, but I have doubts on the part about command complete events.

In my observations there are no command timeouts if a command complete (with opcode != HCI_OP_NOP) is injected after the firmware download, even if a 3s sleep is added before sending the HCI_RESET.

Quoted 23 lines

Quoted 23 lines

Quoted 8 lines

In our case, we have an two fw files i.e. *.tlv and *.bin. tlv is an RAM image of the chip where as bin is an nvm image of the chip. so
tlv will be of
more size which require an lot more time to dump the file in to chip, while dumping the tlv, we divide tlv as packet of size 245 bytes and send
them as an command packet to the chip. chip should respond with an

Quoted 8 lines

command
complete event.
then only we will send the next packet. but size of the tlv is large, to
optimize this we will
not wait for the either an vendor specific event or an command complete
event.

Let's make sure we have an accurate picture, which of the following is correct:

1. the chip sends a command complete event after each packet, as an optimization the BT driver doesn't wait for it

Quoted 7 lines

Quoted 6 lines

2. as an optimization the chip does not send a command complete event and the driver has to deal with it

My understanding is that it's 2), but the wording above seems to describe 1)

[Bala]: point 2 is true.

Thanks for the confirmation

Quoted 38 lines

Quoted 38 lines

Quoted 17 lines

But as we need to be on the sync, i.e. whether are we sending an correct
packets or not,
for the last fw packet we sent to the chip.. chip will to do an CRC check
for the total no of packets received and respond with an vendor specific
event.

We decode the vendor specific event and decide whether the fw download is
success or not. here we send an fw packet as command so stack expects an
command complete event.
where this is missing from the chip. this is expected behavior from chip.

Quoted 3 lines

So currently i am inject an command complete event after receiving an vendor
event for the last packet of the tlv.

And the same for the .bin if I'm not mistaken.

Quoted 2 lines

This we inject only once for the last command packet sent to the chip. i don't think this will effect the btmon.

I don't know enough about btmon to comment on that, in any case Marcel raised concerns.

And I think my comment that triggered this disucssion remains true:

Quoted 3 lines

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download.

Not sure it's a likely case, it might be an issue with larger firmware files and/or slower UART speeds.

Quoted 6 lines

Quoted 3 lines

Thanks

Matthias

[Bala]: i don't think it is a good idea to handle this kind of limitations in the HOST driver.

Where do you intend to handle it then? Ideally it would be fixed in the ROM FW, but that doesn't seem a viable option.

Cheers

Matthias

Quoted 294 lines · Matthias Kaehlcke

Quoted 273 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 222 lines · Matthias Kaehlcke

Hi Balakrishna,

Quoted 186 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 179 lines · Matthias Kaehlcke

Quoted 44 lines · Balakrishna Godavarthi

Hi Marcel,

Quoted 41 lines · Marcel Holtmann

Hi Balakrishna,

Quoted 39 lines

Quoted 39 lines

Quoted 39 lines

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific
event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +10 -0

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
 rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
 hci_dev *hdev, int seg_size,
 	return err;
 }
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_tlv_send_segment(struct · line 284

Quoted 8 lines

drivers/bluetooth/btqca.c +8 -0

+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;

After looking a bit more at it I realize HCI_OP_NOP is not a good value in this case:

static void hci_cmd_complete_evt(...)
{
...

  if (*opcode != HCI_OP_NOP)
    cancel_delayed_work(&hdev->cmd_timer);

...
}

https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351

Cancelling the command timeout is precisely what we want. Not sure why the patch with HCI_OP_NOP makes the timeouts go away in most cases (but not e.g. when inserting an msleep(1000) after downloading the NVM.

I suggest to pass the opcode of the command to be completed.

Quoted 83 lines

Quoted 52 lines

Quoted 38 lines

Quoted 28 lines

Quoted 28 lines

drivers/bluetooth/btqca.c +10 -1

+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int
 qca_download_firmware(struct hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 		segment += segsize;
 	}
+	/* Latest qualcomm chipsets are not sending a command

complete event

drivers/bluetooth/btqca.c +1 -0

+	 * for every fw packet sent. They only respond with a

vendor specific

drivers/bluetooth/btqca.c +2 -0

+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will

inject a command

drivers/bluetooth/btqca.c +1 -0

+	 * complete event to avoid a command timeout error message.

Quoted 8 lines

drivers/bluetooth/btqca.c

Quoted 5 lines

drivers/bluetooth/btqca.c +5 -0

+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

Quoted 7 lines

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

Quoted 7 lines

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

[Bala]: here is the use case, when ever we download the fw packets i.e. RAM
image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command complete event.

the above is taking more time to setup the BT device. then we came up with
solution where we enable flags in fw file (i.e. RAM image header) whether to wait for event to be received or sent the total packets and wait
for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for event
for all packet or wait for an event for the last packet.

but in the second case i.e. wait for event for the last packet sent, we are
only receiving an vendor specific event from chip which holds the status of
fw download.

Quoted 6 lines

so as __hci_cmd_sync_ev() requires an command complete event. so we are
injecting it after the vendor specific event received for the last packet.

This helps to overcome 0xfc00 timeout error logging on console.

Some more details:

The timeout error is actually from reading the 'SoC version', which uses the same command code as the firmware download (EDL_PATCH_CMD_OPCODE). Without reading the 'SoC version' it would be from the command to write the first firmware segment.

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download, though I expect Marcel wouldn't be overly happy with that, since it would affect btmon even more.

Quoted 3 lines

Regards

Matthias

Quoted 8 lines

[Bala]: Basically every vendor specific command we sent to chip, chip should respond with an vendor specific event followed by an command
complete event
or some times it will only respond with an command complete event. but in any case command complete event is mandatory to all the command we
sent to the chip.

Is this ("command complete event is mandatory to all the command we sent to the chip") a description of what the chip actually does, or what it should be doing according to the spec?

As mentioned earlier, the timeout we see originates from reading the SoC version:

drivers/bluetooth/btqca.c +4 -0

index 0b533f65f652fc..1e484e61799571 100644
@@ -400,6 +400,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t
 baudrate,
                return err;
        }
 
+       printk("DBG: ZZZzzzzzzz\n");
+       msleep(2500);
+       printk("DBG: good morning!\n");
+
        /* Download NVM configuration */
        config.type = TLV_TYPE_NVM;

        if (soc_type == QCA_WCN3990)

Quoted 7 lines

When you boot with this patch you'll see something like this:

[   15.531365] DBG: reading SoC version
[   15.544963] DBG: ZZZzzzzzzz
[   17.590282] Bluetooth: hci0: command 0xfc00 tx timeout
[   18.099110] DBG: good morning!

[Bala]: my previous analysis were wrong. thanks for pointing me to the correct issue.
i am able to see timeout after version command.(used some
sleep)
here is the reason for it.

01 00 fc 01 19 (we send the command to chip to request
version)

        04 ff 0e 00 02 0a 00 00 00 01 00 01 02 14 02 01 40(chip will

respond
with the vendor specific event payload will be chip version)
04 0e 04 01 00 00 00(command complete event)

issue is with command complete event. ideally command
complete event
payload holds the command for which chip sends command complete event.

breaking the command

04 : event packet (fixed)
0e : command complete event(fixed)
04 : size of the payload,

01 : no of commands in the payload
00 : OCF (opocde lsb)
00 : OGF (opcode msb)
00 : status of command executed,

payload is form byte 4 to byte 7. which hold the command i.e
opcode

      form the above response opcode is 0x0000. ideally this should be

0xfc00
because the command complete response from the chip is for the
command
0xfc00.

Checked with Chip firmware dev team for the reason, it was an
limitation or an bug in the chip ROM firmware
in the current chipset, which is fixed in the coming chipset.

Quoted 6 lines

so we expect the command 0xfc00 but recevuies 0x00. so that is
the
reason we see an timeouts.

      for all the vendor commands we receive 0x0000 in the command

complete
event.

Thanks for your analysis.

It is unfortunate (though not unexpected) that this is a problem in the ROM were we can't fix it, but at least the FW team is aware of it and fixed it for future hardware.

Quoted 6 lines

      if comes a new question how are we not seeing an 0xfc00 when

we inject
an command complete event.
i experimented this patch, when i inject the command complete
event
from the soc version still i can see the command timeouts.

Quoted 5 lines

I don't see command timeouts when injecting a command complete event after reading the SoC version. I suspect you still use patch which injects a command complete event with HCI_OP_NOP, which is precisely what the chip does ...

[Bala]: i am seeing command timeout after injecting cc event after read version request.

Quoted 22 lines

Quoted 10 lines

The reason for no timeouts error, we are injecting an command
complete
after last packet sent out, so this injection helps
.bin file to speed out it download part, so as the bin is dumped
fastly we send an HCI RESET Comamnd where we recevie an command complete
event
with non zero opcode which is cancelling the command timeout
timer.

Quoted 7 lines

when we inject command complete event, in short we are
completing
setup process in less than 2 seconds(command timeout timer value),
so before 2 seconds we are sending an HCI RESET command whose
command
complete event opcode value is an non zero (which cancels the command timeout timer)

I agree that HCI_RESET cancels the command timeout timer if it is sent before the 2s timout expires, but I have doubts on the part about command complete events.

Quoted 4 lines

In my observations there are no command timeouts if a command complete (with opcode != HCI_OP_NOP) is injected after the firmware download, even if a 3s sleep is added before sending the HCI_RESET.

[Bala]: add sleep before sending HCI reset, oxfc00 timeout observed.

Quoted 330 lines · Balakrishna Godavarthi

Quoted 294 lines · Matthias Kaehlcke

Quoted 273 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 222 lines · Matthias Kaehlcke

Hi Balakrishna,

Quoted 186 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 179 lines · Matthias Kaehlcke

Quoted 44 lines · Balakrishna Godavarthi

Hi Marcel,

Quoted 41 lines · Marcel Holtmann

Hi Balakrishna,

Quoted 39 lines

Quoted 39 lines

Quoted 39 lines

Latest qualcomm chips are not sending an command complete event for every firmware packet sent to chip. They only respond with a vendor specific event for the last firmware packet. This optimization will decrease the BT ON time. Due to this we are seeing a timeout error message logs on the console during firmware download. Now we are injecting a command complete event once we receive an vendor specific
event for the last RAM firmware packet.

Signed-off-by Balakrishna Godavarthi <[email protected]>

---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++

2 files changed, 41 insertions(+), 1 deletion(-)

drivers/bluetooth/btqca.c +10 -0

index ec9e03a6b778..0b533f65f652 100644
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
 rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));

@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
 hci_dev *hdev, int seg_size,
 	return err;
 }
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/bluetooth/btqca.c static int qca_tlv_send_segment(struct · line 284

Quoted 8 lines

drivers/bluetooth/btqca.c +8 -0

+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;

After looking a bit more at it I realize HCI_OP_NOP is not a good value in this case:

static void hci_cmd_complete_evt(...)
{
...

  if (*opcode != HCI_OP_NOP)
    cancel_delayed_work(&hdev->cmd_timer);

...
}

https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351

Cancelling the command timeout is precisely what we want. Not sure why the patch with HCI_OP_NOP makes the timeouts go away in most cases (but not e.g. when inserting an msleep(1000) after downloading the NVM.

I suggest to pass the opcode of the command to be completed.

Quoted 83 lines

Quoted 52 lines

Quoted 38 lines

Quoted 28 lines

Quoted 28 lines

drivers/bluetooth/btqca.c +10 -1

+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {

@@ -297,11 +323,22 @@ static int
 qca_download_firmware(struct hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 		segment += segsize;
 	}
+	/* Latest qualcomm chipsets are not sending a command

complete event

drivers/bluetooth/btqca.c +1 -0

+	 * for every fw packet sent. They only respond with a

vendor specific

drivers/bluetooth/btqca.c +2 -0

+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will

inject a command

drivers/bluetooth/btqca.c +1 -0

+	 * complete event to avoid a command timeout error message.

Quoted 8 lines

drivers/bluetooth/btqca.c

Quoted 5 lines

drivers/bluetooth/btqca.c +5 -0

+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

Quoted 7 lines

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

Quoted 7 lines

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

[Bala]: here is the use case, when ever we download the fw packets i.e. RAM
image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command complete event.

the above is taking more time to setup the BT device. then we came up with
solution where we enable flags in fw file (i.e. RAM image header) whether to wait for event to be received or sent the total packets and wait
for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for event
for all packet or wait for an event for the last packet.

but in the second case i.e. wait for event for the last packet sent, we are
only receiving an vendor specific event from chip which holds the status of
fw download.

Quoted 6 lines

so as __hci_cmd_sync_ev() requires an command complete event. so we are
injecting it after the vendor specific event received for the last packet.

This helps to overcome 0xfc00 timeout error logging on console.

Some more details:

The timeout error is actually from reading the 'SoC version', which uses the same command code as the firmware download (EDL_PATCH_CMD_OPCODE). Without reading the 'SoC version' it would be from the command to write the first firmware segment.

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download, though I expect Marcel wouldn't be overly happy with that, since it would affect btmon even more.

Quoted 3 lines

Regards

Matthias

Quoted 8 lines

[Bala]: Basically every vendor specific command we sent to chip, chip should respond with an vendor specific event followed by an command
complete event
or some times it will only respond with an command complete event. but in any case command complete event is mandatory to all the command we
sent to the chip.

Is this ("command complete event is mandatory to all the command we sent to the chip") a description of what the chip actually does, or what it should be doing according to the spec?

As mentioned earlier, the timeout we see originates from reading the SoC version:

drivers/bluetooth/btqca.c +4 -0

index 0b533f65f652fc..1e484e61799571 100644
@@ -400,6 +400,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t
 baudrate,
                return err;
        }
 
+       printk("DBG: ZZZzzzzzzz\n");
+       msleep(2500);
+       printk("DBG: good morning!\n");
+
        /* Download NVM configuration */
        config.type = TLV_TYPE_NVM;

        if (soc_type == QCA_WCN3990)

Quoted 7 lines

When you boot with this patch you'll see something like this:

[   15.531365] DBG: reading SoC version
[   15.544963] DBG: ZZZzzzzzzz
[   17.590282] Bluetooth: hci0: command 0xfc00 tx timeout
[   18.099110] DBG: good morning!

[Bala]: my previous analysis were wrong. thanks for pointing me to the correct issue.
i am able to see timeout after version command.(used some
sleep)
here is the reason for it.

01 00 fc 01 19 (we send the command to chip to request
version)

        04 ff 0e 00 02 0a 00 00 00 01 00 01 02 14 02 01 40(chip will

respond
with the vendor specific event payload will be chip version)
04 0e 04 01 00 00 00(command complete event)

issue is with command complete event. ideally command
complete event
payload holds the command for which chip sends command complete event.

breaking the command

04 : event packet (fixed)
0e : command complete event(fixed)
04 : size of the payload,

01 : no of commands in the payload
00 : OCF (opocde lsb)
00 : OGF (opcode msb)
00 : status of command executed,

payload is form byte 4 to byte 7. which hold the command i.e
opcode

      form the above response opcode is 0x0000. ideally this should be

0xfc00
because the command complete response from the chip is for the
command
0xfc00.

Checked with Chip firmware dev team for the reason, it was an
limitation or an bug in the chip ROM firmware
in the current chipset, which is fixed in the coming chipset.

Quoted 6 lines

so we expect the command 0xfc00 but recevuies 0x00. so that is
the
reason we see an timeouts.

      for all the vendor commands we receive 0x0000 in the command

complete
event.

Thanks for your analysis.

It is unfortunate (though not unexpected) that this is a problem in the ROM were we can't fix it, but at least the FW team is aware of it and fixed it for future hardware.

Quoted 6 lines

      if comes a new question how are we not seeing an 0xfc00 when

we inject
an command complete event.
i experimented this patch, when i inject the command complete
event
from the soc version still i can see the command timeouts.

Quoted 5 lines

I don't see command timeouts when injecting a command complete event after reading the SoC version. I suspect you still use patch which injects a command complete event with HCI_OP_NOP, which is precisely what the chip does ...

[Bala]: i am seeing command timeout after injecting cc event after read version request.

Quoted 26 lines

Quoted 10 lines

The reason for no timeouts error, we are injecting an command
complete
after last packet sent out, so this injection helps
.bin file to speed out it download part, so as the bin is dumped
fastly we send an HCI RESET Comamnd where we recevie an command complete
event
with non zero opcode which is cancelling the command timeout
timer.

Quoted 7 lines

when we inject command complete event, in short we are
completing
setup process in less than 2 seconds(command timeout timer value),
so before 2 seconds we are sending an HCI RESET command whose
command
complete event opcode value is an non zero (which cancels the command timeout timer)

I agree that HCI_RESET cancels the command timeout timer if it is sent before the 2s timout expires, but I have doubts on the part about command complete events.

In my observations there are no command timeouts if a command complete (with opcode != HCI_OP_NOP) is injected after the firmware download, even if a 3s sleep is added before sending the HCI_RESET.

Quoted 1 line

[Bala]: add sleep before sending HCI reset, oxfc00 timeout observed.

Please double check your setup. Are you 100% sure you are not still sending HCI_OP_NOP as opcode?

This is the diff I use for debugging on top of your patchset:

drivers/bluetooth/btqca.c +7 -0

index 0b533f65f652fc..5bb9e0ca7c348b 100644
@@ -27,6 +27,8 @@
 
 #define VERSION "0.1"
 
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev);
+
 int qca_read_soc_version(struct hci_dev *hdev, u32 *soc_version)
 {
 	struct sk_buff *skb;

@@ -84,6 +86,11 @@ int qca_read_soc_version(struct hci_dev *hdev, u32
 *soc_version)
 	if (*soc_version == 0)
 		err = -EILSEQ;
 
+	qca_inject_cmd_complete_event(hdev);
+	printk("DBG: ZZZzzzzzzz\n");
+	msleep(2500);
+	printk("DBG: good morning!\n");
+
 out:
 	kfree_skb(skb);

drivers/bluetooth/btqca.c +1 -1

 	if (err)

@@ -281,7 +288,7 @@ static int qca_inject_cmd_complete_event(struct
 hci_dev *hdev)
 
 	evt = skb_put(skb, sizeof(*evt));
 	evt->ncmd = 1;
-	evt->opcode = HCI_OP_NOP;
+	evt->opcode = 0x42;
 
 	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);

net/bluetooth/hci_event.c +4 -1

index f12555f23a49a0..c87ac1823439ab 100644
@@ -3348,8 +3348,11 @@ static void hci_cmd_complete_evt(struct hci_dev
 *hdev, struct sk_buff *skb,
 		break;
 	}
 
-	if (*opcode != HCI_OP_NOP)
+	if (*opcode != HCI_OP_NOP) {
+		printk("DBG: cancelling command timer (opcode = 0x%02x)\n",
+		       *opcode);
 		cancel_delayed_work(&hdev->cmd_timer);
+	}
 
 	if (ev->ncmd && !test_bit(HCI_RESET, &hdev->flags))

atomic_set(&hdev->cmd_cnt, 1);

With that I see:

[   18.725417] Bluetooth: hci0: setting up wcn3990
[   18.860381] DBG: ZZZzzzzzzz
[   18.863332] DBG: cancelling command timer (opcode = 0x42)
[   21.427085] DBG: good morning!

The timer is clearly cancelled by the injected command (opcode 0x42) and no timeout occurs. I wouldn't expect you to see anything vastly different.

Cheers

Matthias

Quoted 334 lines · Matthias Kaehlcke

Quoted 332 lines · Balakrishna Godavarthi

Quoted 294 lines · Matthias Kaehlcke

Quoted 273 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 222 lines · Matthias Kaehlcke

Hi Balakrishna,

Quoted 186 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 179 lines · Matthias Kaehlcke

Quoted 44 lines · Balakrishna Godavarthi

Hi Marcel,

Quoted 41 lines · Marcel Holtmann

Hi Balakrishna,

Quoted 39 lines

Quoted 39 lines

Quoted 39 lines

Latest qualcomm chips are not sending an command complete event for
every firmware packet sent to chip. They only respond with a vendor
specific event for the last firmware packet. This optimization will
decrease the BT ON time. Due to this we are seeing a timeout error
message logs on the console during firmware download. Now we are
injecting a command complete event once we receive an vendor
specific
event for the last RAM firmware packet.
Signed-off-by: Balakrishna Godavarthi <[email protected]>
---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index ec9e03a6b778..0b533f65f652 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
rome_config *config,
* In case VSE is skipped, only the last segment is acked.
*/
config->dnld_mode = tlv_patch->download_mode;
+ config->dnld_type = config->dnld_mode;
BT_DBG("Total Length : %d bytes",
le32_to_cpu(tlv_patch->total_size));
@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
hci_dev *hdev, int seg_size,
return err;
}
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+ struct hci_event_hdr *hdr;
+ struct hci_ev_cmd_complete *evt;
+ struct sk_buff *skb;
+
+ skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+ if (!skb)
+ return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

+
+ hdr = skb_put(skb, sizeof(*hdr));
+ hdr->evt = HCI_EV_CMD_COMPLETE;
+ hdr->plen = sizeof(*evt) + 1;
+
+ evt = skb_put(skb, sizeof(*evt));
+ evt->ncmd = 1;
+ evt->opcode = HCI_OP_NOP;

After looking a bit more at it I realize HCI_OP_NOP is not a good value in this case:

static void hci_cmd_complete_evt(...)
{
...

  if (*opcode != HCI_OP_NOP)
    cancel_delayed_work(&hdev->cmd_timer);

...
}

https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351

Cancelling the command timeout is precisely what we want. Not sure why the patch with HCI_OP_NOP makes the timeouts go away in most cases (but not e.g. when inserting an msleep(1000) after downloading the NVM.

I suggest to pass the opcode of the command to be completed.

Quoted 83 lines

Quoted 52 lines

Quoted 38 lines

Quoted 28 lines

Quoted 28 lines

+
+ skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+ hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+ return hci_recv_frame(hdev, skb);
+}
+
static int qca_download_firmware(struct hci_dev *hdev,
struct rome_config *config)
{
@@ -297,11 +323,22 @@ static int
qca_download_firmware(struct hci_dev *hdev,
ret = qca_tlv_send_segment(hdev, segsize, segment,
config->dnld_mode);
if (ret)
- break;
+ goto out;
segment += segsize;
}
+ /* Latest qualcomm chipsets are not sending a command
complete event
+ * for every fw packet sent. They only respond with a
vendor specific
+ * event for the last packet. This optimization in the chip will
+ * decrease the BT in initialization time. Here we will
inject a command
+ * complete event to avoid a command timeout error message.

Quoted 8 lines

Quoted 5 lines

+ */
+ if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+ config->dnld_type == ROME_SKIP_EVT_VSE))
+ return qca_inject_cmd_complete_event(hdev);
+

have you actually considered using __hci_cmd_send in that case. It is allowed for vendor OGF to use that command. I see you actually do use it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

Quoted 7 lines

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

Quoted 7 lines

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

[Bala]: here is the use case, when ever we download the fw packets i.e. RAM
image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command complete event.

the above is taking more time to setup the BT device. then we came up with
solution where we enable flags in fw file (i.e. RAM image header) whether to wait for event to be received or sent the total packets and wait
for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for event
for all packet or wait for an event for the last packet.

but in the second case i.e. wait for event for the last packet sent, we are
only receiving an vendor specific event from chip which holds the status of
fw download.

Quoted 6 lines

so as __hci_cmd_sync_ev() requires an command complete event. so we are
injecting it after the vendor specific event received for the last packet.

This helps to overcome 0xfc00 timeout error logging on console.

Some more details:

The timeout error is actually from reading the 'SoC version', which uses the same command code as the firmware download (EDL_PATCH_CMD_OPCODE). Without reading the 'SoC version' it would be from the command to write the first firmware segment.

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download, though I expect Marcel wouldn't be overly happy with that, since it would affect btmon even more.

Quoted 3 lines

Regards

Matthias

Quoted 8 lines

[Bala]: Basically every vendor specific command we sent to chip, chip should respond with an vendor specific event followed by an command
complete event
or some times it will only respond with an command complete event. but in any case command complete event is mandatory to all the command we
sent to the chip.

Is this ("command complete event is mandatory to all the command we sent to the chip") a description of what the chip actually does, or what it should be doing according to the spec?

As mentioned earlier, the timeout we see originates from reading the SoC version:

drivers/bluetooth/btqca.c +4 -0

index 0b533f65f652fc..1e484e61799571 100644
@@ -400,6 +400,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t
 baudrate,
                return err;
        }
 
+       printk("DBG: ZZZzzzzzzz\n");
+       msleep(2500);
+       printk("DBG: good morning!\n");
+
        /* Download NVM configuration */
        config.type = TLV_TYPE_NVM;

        if (soc_type == QCA_WCN3990)

Quoted 7 lines

When you boot with this patch you'll see something like this:

[   15.531365] DBG: reading SoC version
[   15.544963] DBG: ZZZzzzzzzz
[   17.590282] Bluetooth: hci0: command 0xfc00 tx timeout
[   18.099110] DBG: good morning!

[Bala]: my previous analysis were wrong. thanks for pointing me to the correct issue.
i am able to see timeout after version command.(used some
sleep)
here is the reason for it.

01 00 fc 01 19 (we send the command to chip to request
version)

        04 ff 0e 00 02 0a 00 00 00 01 00 01 02 14 02 01 40(chip will

respond
with the vendor specific event payload will be chip version)
04 0e 04 01 00 00 00(command complete event)

issue is with command complete event. ideally command
complete event
payload holds the command for which chip sends command complete event.

breaking the command

04 : event packet (fixed)
0e : command complete event(fixed)
04 : size of the payload,

01 : no of commands in the payload
00 : OCF (opocde lsb)
00 : OGF (opcode msb)
00 : status of command executed,

payload is form byte 4 to byte 7. which hold the command i.e
opcode

      form the above response opcode is 0x0000. ideally this should be

0xfc00
because the command complete response from the chip is for the
command
0xfc00.

Checked with Chip firmware dev team for the reason, it was an
limitation or an bug in the chip ROM firmware
in the current chipset, which is fixed in the coming chipset.

Quoted 6 lines

so we expect the command 0xfc00 but recevuies 0x00. so that is
the
reason we see an timeouts.

      for all the vendor commands we receive 0x0000 in the command

complete
event.

Thanks for your analysis.

It is unfortunate (though not unexpected) that this is a problem in the ROM were we can't fix it, but at least the FW team is aware of it and fixed it for future hardware.

Quoted 6 lines

      if comes a new question how are we not seeing an 0xfc00 when

we inject
an command complete event.
i experimented this patch, when i inject the command complete
event
from the soc version still i can see the command timeouts.

Quoted 5 lines

I don't see command timeouts when injecting a command complete event after reading the SoC version. I suspect you still use patch which injects a command complete event with HCI_OP_NOP, which is precisely what the chip does ...

[Bala]: i am seeing command timeout after injecting cc event after read
version request.

Quoted 18 lines

Quoted 10 lines

The reason for no timeouts error, we are injecting an command
complete
after last packet sent out, so this injection helps
.bin file to speed out it download part, so as the bin is dumped
fastly we send an HCI RESET Comamnd where we recevie an command complete
event
with non zero opcode which is cancelling the command timeout
timer.

Quoted 7 lines

when we inject command complete event, in short we are
completing
setup process in less than 2 seconds(command timeout timer value),
so before 2 seconds we are sending an HCI RESET command whose
command
complete event opcode value is an non zero (which cancels the command timeout timer)

Quoted 8 lines

I agree that HCI_RESET cancels the command timeout timer if it is sent before the 2s timout expires, but I have doubts on the part about command complete events.

In my observations there are no command timeouts if a command complete (with opcode != HCI_OP_NOP) is injected after the firmware download, even if a 3s sleep is added before sending the HCI_RESET.

[Bala]: add sleep before sending HCI reset, oxfc00 timeout observed.

Quoted 3 lines

Please double check your setup. Are you 100% sure you are not still sending HCI_OP_NOP as opcode?

[Bala]: sorry i am injecting the HCI_OP_NOP, so that is the reason we have an timeouts.

        yes if the opcode 0x42 injected, no timeouts are observed.

Quoted 72 lines

This is the diff I use for debugging on top of your patchset:

drivers/bluetooth/btqca.c +7 -0

index 0b533f65f652fc..5bb9e0ca7c348b 100644
@@ -27,6 +27,8 @@
 
 #define VERSION "0.1"
 
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev);
+
 int qca_read_soc_version(struct hci_dev *hdev, u32 *soc_version)
 {
 	struct sk_buff *skb;

@@ -84,6 +86,11 @@ int qca_read_soc_version(struct hci_dev *hdev, u32
 *soc_version)
 	if (*soc_version == 0)
 		err = -EILSEQ;
 
+	qca_inject_cmd_complete_event(hdev);
+	printk("DBG: ZZZzzzzzzz\n");
+	msleep(2500);
+	printk("DBG: good morning!\n");
+
 out:
 	kfree_skb(skb);

drivers/bluetooth/btqca.c +1 -1

 	if (err)

@@ -281,7 +288,7 @@ static int qca_inject_cmd_complete_event(struct
 hci_dev *hdev)
 
 	evt = skb_put(skb, sizeof(*evt));
 	evt->ncmd = 1;
-	evt->opcode = HCI_OP_NOP;
+	evt->opcode = 0x42;
 
 	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);

net/bluetooth/hci_event.c +4 -1

index f12555f23a49a0..c87ac1823439ab 100644
@@ -3348,8 +3348,11 @@ static void hci_cmd_complete_evt(struct hci_dev
 *hdev, struct sk_buff *skb,
 		break;
 	}
 
-	if (*opcode != HCI_OP_NOP)
+	if (*opcode != HCI_OP_NOP) {
+		printk("DBG: cancelling command timer (opcode = 0x%02x)\n",
+		       *opcode);
 		cancel_delayed_work(&hdev->cmd_timer);
+	}
 
 	if (ev->ncmd && !test_bit(HCI_RESET, &hdev->flags))

atomic_set(&hdev->cmd_cnt, 1);

With that I see:

[   18.725417] Bluetooth: hci0: setting up wcn3990
[   18.860381] DBG: ZZZzzzzzzz
[   18.863332] DBG: cancelling command timer (opcode = 0x42)
[   21.427085] DBG: good morning!

The timer is clearly cancelled by the injected command (opcode 0x42) and no timeout occurs. I wouldn't expect you to see anything vastly different.

Cheers

Quoted 1 line

Matthias

[Bala]: I think it would be better if we inject a command complete event after we
receive the version data. instead of injecting in between firmware
download.
this change will be only for wcn3990.

Quoted 335 lines · Balakrishna Godavarthi

Quoted 334 lines · Matthias Kaehlcke

Quoted 332 lines · Balakrishna Godavarthi

Quoted 294 lines · Matthias Kaehlcke

Quoted 273 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 222 lines · Matthias Kaehlcke

Hi Balakrishna,

Quoted 186 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 179 lines · Matthias Kaehlcke

Quoted 44 lines · Balakrishna Godavarthi

Hi Marcel,

Quoted 41 lines · Marcel Holtmann

Hi Balakrishna,

Quoted 39 lines

Quoted 39 lines

Latest qualcomm chips are not sending an command complete event for
every firmware packet sent to chip. They only respond with a vendor
specific event for the last firmware packet. This optimization will
decrease the BT ON time. Due to this we are seeing a timeout error
message logs on the console during firmware download. Now we are
injecting a command complete event once we receive an vendor
specific
event for the last RAM firmware packet.
Signed-off-by: Balakrishna Godavarthi <[email protected]>
---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index ec9e03a6b778..0b533f65f652 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
rome_config *config,
* In case VSE is skipped, only the last segment is acked.
*/
config->dnld_mode = tlv_patch->download_mode;
+ config->dnld_type = config->dnld_mode;
BT_DBG("Total Length : %d bytes",
le32_to_cpu(tlv_patch->total_size));
@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
hci_dev *hdev, int seg_size,
return err;
}
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+ struct hci_event_hdr *hdr;
+ struct hci_ev_cmd_complete *evt;
+ struct sk_buff *skb;
+
+ skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+ if (!skb)
+ return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

+
+ hdr = skb_put(skb, sizeof(*hdr));
+ hdr->evt = HCI_EV_CMD_COMPLETE;
+ hdr->plen = sizeof(*evt) + 1;
+
+ evt = skb_put(skb, sizeof(*evt));
+ evt->ncmd = 1;
+ evt->opcode = HCI_OP_NOP;

After looking a bit more at it I realize HCI_OP_NOP is not a good value in this case:

static void hci_cmd_complete_evt(...)
{
...

  if (*opcode != HCI_OP_NOP)
    cancel_delayed_work(&hdev->cmd_timer);

...
}

https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351

Cancelling the command timeout is precisely what we want. Not sure why the patch with HCI_OP_NOP makes the timeouts go away in most cases (but not e.g. when inserting an msleep(1000) after downloading the NVM.

I suggest to pass the opcode of the command to be completed.

Quoted 83 lines

Quoted 52 lines

Quoted 38 lines

Quoted 36 lines

+
+ skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+ hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+ return hci_recv_frame(hdev, skb);
+}
+
static int qca_download_firmware(struct hci_dev *hdev,
struct rome_config *config)
{
@@ -297,11 +323,22 @@ static int
qca_download_firmware(struct hci_dev *hdev,
ret = qca_tlv_send_segment(hdev, segsize, segment,
config->dnld_mode);
if (ret)
- break;
+ goto out;
segment += segsize;
}
+ /* Latest qualcomm chipsets are not sending a command
complete event
+ * for every fw packet sent. They only respond with a
vendor specific
+ * event for the last packet. This optimization in the chip will
+ * decrease the BT in initialization time. Here we will
inject a command
+ * complete event to avoid a command timeout error message.
+ */
+ if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+ config->dnld_type == ROME_SKIP_EVT_VSE))
+ return qca_inject_cmd_complete_event(hdev);
+
have you actually considered using __hci_cmd_send in that case. It is
allowed for vendor OGF to use that command. I see you actually do use
it and now I am failing to understand what this is for.

[Bala]: thanks for reviewing the change.

Quoted 7 lines

__hci_cmd_send() can be used only to send the command to the chip. it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

Quoted 7 lines

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

[Bala]: here is the use case, when ever we download the fw packets i.e. RAM
image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command complete event.

the above is taking more time to setup the BT device. then we came up with
solution where we enable flags in fw file (i.e. RAM image header) whether to wait for event to be received or sent the total packets and wait
for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for event
for all packet or wait for an event for the last packet.

but in the second case i.e. wait for event for the last packet sent, we are
only receiving an vendor specific event from chip which holds the status of
fw download.

Quoted 6 lines

so as __hci_cmd_sync_ev() requires an command complete event. so we are
injecting it after the vendor specific event received for the last packet.

This helps to overcome 0xfc00 timeout error logging on console.

Some more details:

The timeout error is actually from reading the 'SoC version', which uses the same command code as the firmware download (EDL_PATCH_CMD_OPCODE). Without reading the 'SoC version' it would be from the command to write the first firmware segment.

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download, though I expect Marcel wouldn't be overly happy with that, since it would affect btmon even more.

Quoted 3 lines

Regards

Matthias

Quoted 8 lines

[Bala]: Basically every vendor specific command we sent to chip, chip should respond with an vendor specific event followed by an command
complete event
or some times it will only respond with an command complete event. but in any case command complete event is mandatory to all the command we
sent to the chip.

Is this ("command complete event is mandatory to all the command we sent to the chip") a description of what the chip actually does, or what it should be doing according to the spec?

As mentioned earlier, the timeout we see originates from reading the SoC version:

drivers/bluetooth/btqca.c +4 -0

index 0b533f65f652fc..1e484e61799571 100644
@@ -400,6 +400,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t
 baudrate,
                return err;
        }
 
+       printk("DBG: ZZZzzzzzzz\n");
+       msleep(2500);
+       printk("DBG: good morning!\n");
+
        /* Download NVM configuration */
        config.type = TLV_TYPE_NVM;

        if (soc_type == QCA_WCN3990)

Quoted 7 lines

When you boot with this patch you'll see something like this:

[   15.531365] DBG: reading SoC version
[   15.544963] DBG: ZZZzzzzzzz
[   17.590282] Bluetooth: hci0: command 0xfc00 tx timeout
[   18.099110] DBG: good morning!

[Bala]: my previous analysis were wrong. thanks for pointing me to the correct issue.
i am able to see timeout after version command.(used some
sleep)
here is the reason for it.

01 00 fc 01 19 (we send the command to chip to request
version)

        04 ff 0e 00 02 0a 00 00 00 01 00 01 02 14 02 01 40(chip will

respond
with the vendor specific event payload will be chip version)
04 0e 04 01 00 00 00(command complete event)

issue is with command complete event. ideally command
complete event
payload holds the command for which chip sends command complete event.

breaking the command

04 : event packet (fixed)
0e : command complete event(fixed)
04 : size of the payload,

01 : no of commands in the payload
00 : OCF (opocde lsb)
00 : OGF (opcode msb)
00 : status of command executed,

payload is form byte 4 to byte 7. which hold the command i.e
opcode

      form the above response opcode is 0x0000. ideally this should be

0xfc00
because the command complete response from the chip is for the
command
0xfc00.

Checked with Chip firmware dev team for the reason, it was an
limitation or an bug in the chip ROM firmware
in the current chipset, which is fixed in the coming chipset.

Quoted 6 lines

so we expect the command 0xfc00 but recevuies 0x00. so that is
the
reason we see an timeouts.

      for all the vendor commands we receive 0x0000 in the command

complete
event.

Thanks for your analysis.

It is unfortunate (though not unexpected) that this is a problem in the ROM were we can't fix it, but at least the FW team is aware of it and fixed it for future hardware.

Quoted 6 lines

      if comes a new question how are we not seeing an 0xfc00 when

we inject
an command complete event.
i experimented this patch, when i inject the command complete
event
from the soc version still i can see the command timeouts.

Quoted 5 lines

I don't see command timeouts when injecting a command complete event after reading the SoC version. I suspect you still use patch which injects a command complete event with HCI_OP_NOP, which is precisely what the chip does ...

[Bala]: i am seeing command timeout after injecting cc event after read
version request.

Quoted 18 lines

Quoted 10 lines

The reason for no timeouts error, we are injecting an command
complete
after last packet sent out, so this injection helps
.bin file to speed out it download part, so as the bin is dumped
fastly we send an HCI RESET Comamnd where we recevie an command complete
event
with non zero opcode which is cancelling the command timeout
timer.

Quoted 7 lines

when we inject command complete event, in short we are
completing
setup process in less than 2 seconds(command timeout timer value),
so before 2 seconds we are sending an HCI RESET command whose
command
complete event opcode value is an non zero (which cancels the command timeout timer)

Quoted 8 lines

I agree that HCI_RESET cancels the command timeout timer if it is sent before the 2s timout expires, but I have doubts on the part about command complete events.

In my observations there are no command timeouts if a command complete (with opcode != HCI_OP_NOP) is injected after the firmware download, even if a 3s sleep is added before sending the HCI_RESET.

[Bala]: add sleep before sending HCI reset, oxfc00 timeout observed.

Quoted 7 lines

Quoted 3 lines

Please double check your setup. Are you 100% sure you are not still sending HCI_OP_NOP as opcode?

[Bala]: sorry i am injecting the HCI_OP_NOP, so that is the reason we have an timeouts.

        yes if the opcode 0x42 injected, no timeouts are observed.

Please don't use 0x42 (at least not as literal) when you respin, I only chose this value to illustrate that it's really the injected command which cancels the timer. You might want to add a QCA_HCI_OP_DUMMY or similar, to make clear that the value doesn't matter (as long as it isn't an actual opcode). With a define I don't really care if you use 0x01, 0x42 or any other non-opcode value.

Quoted 72 lines

Quoted 72 lines

This is the diff I use for debugging on top of your patchset:

drivers/bluetooth/btqca.c +7 -0

index 0b533f65f652fc..5bb9e0ca7c348b 100644
@@ -27,6 +27,8 @@
 
 #define VERSION "0.1"
 
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev);
+
 int qca_read_soc_version(struct hci_dev *hdev, u32 *soc_version)
 {
 	struct sk_buff *skb;

@@ -84,6 +86,11 @@ int qca_read_soc_version(struct hci_dev *hdev, u32
 *soc_version)
 	if (*soc_version == 0)
 		err = -EILSEQ;
 
+	qca_inject_cmd_complete_event(hdev);
+	printk("DBG: ZZZzzzzzzz\n");
+	msleep(2500);
+	printk("DBG: good morning!\n");
+
 out:
 	kfree_skb(skb);

drivers/bluetooth/btqca.c +1 -1

 	if (err)

@@ -281,7 +288,7 @@ static int qca_inject_cmd_complete_event(struct
 hci_dev *hdev)
 
 	evt = skb_put(skb, sizeof(*evt));
 	evt->ncmd = 1;
-	evt->opcode = HCI_OP_NOP;
+	evt->opcode = 0x42;
 
 	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);

net/bluetooth/hci_event.c +4 -1

index f12555f23a49a0..c87ac1823439ab 100644
@@ -3348,8 +3348,11 @@ static void hci_cmd_complete_evt(struct hci_dev
 *hdev, struct sk_buff *skb,
 		break;
 	}
 
-	if (*opcode != HCI_OP_NOP)
+	if (*opcode != HCI_OP_NOP) {
+		printk("DBG: cancelling command timer (opcode = 0x%02x)\n",
+		       *opcode);
 		cancel_delayed_work(&hdev->cmd_timer);
+	}
 
 	if (ev->ncmd && !test_bit(HCI_RESET, &hdev->flags))

atomic_set(&hdev->cmd_cnt, 1);

With that I see:

[   18.725417] Bluetooth: hci0: setting up wcn3990
[   18.860381] DBG: ZZZzzzzzzz
[   18.863332] DBG: cancelling command timer (opcode = 0x42)
[   21.427085] DBG: good morning!

The timer is clearly cancelled by the injected command (opcode 0x42) and no timeout occurs. I wouldn't expect you to see anything vastly different.

Cheers

Quoted 8 lines

Quoted 1 line

Matthias

[Bala]: I think it would be better if we inject a command complete event after we
receive the version data. instead of injecting in between
firmware
download.
this change will be only for wcn3990.

At first sight that doesn't sound like a bad idea if we want to inject a single command complete event. However the NVM download times out unless the command(s) of the TLV download is completed:

[   21.789452] Bluetooth: hci0: QCA Downloading qca/crnv21.bin
[   21.797675] Bluetooth: hci0: Send segment 0, size 243
[   23.605597] Bluetooth: hci0: command 0xfc00 tx timeout

So we should inject one command complete event after reading the SoC version and another after downloading the TLV file. An event after downloading the NVM file seems optional, since the HCI reset immediately after will cancel the timer anyway.

Cheers

Matthias

Quoted 341 lines

Quoted 333 lines · Matthias Kaehlcke

Quoted 332 lines · Balakrishna Godavarthi

Quoted 331 lines · Matthias Kaehlcke

Quoted 294 lines · Balakrishna Godavarthi

Quoted 273 lines

Hi Matthias,

Quoted 222 lines · Matthias Kaehlcke

Hi Balakrishna,

Quoted 186 lines · Balakrishna Godavarthi

Hi Matthias,

Quoted 179 lines · Matthias Kaehlcke

Quoted 44 lines · Balakrishna Godavarthi

Hi Marcel,

Quoted 41 lines · Marcel Holtmann

Hi Balakrishna,

Quoted 39 lines

Latest qualcomm chips are not sending an command complete event for
every firmware packet sent to chip. They only respond with a vendor
specific event for the last firmware packet. This optimization will
decrease the BT ON time. Due to this we are seeing a timeout error
message logs on the console during firmware download. Now we are
injecting a command complete event once we receive an vendor
specific
event for the last RAM firmware packet.
Signed-off-by: Balakrishna Godavarthi <[email protected]>
---
drivers/bluetooth/btqca.c | 39
++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btqca.h | 3 +++
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index ec9e03a6b778..0b533f65f652 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct
rome_config *config,
* In case VSE is skipped, only the last segment is acked.
*/
config->dnld_mode = tlv_patch->download_mode;
+ config->dnld_type = config->dnld_mode;
BT_DBG("Total Length : %d bytes",
le32_to_cpu(tlv_patch->total_size));
@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct
hci_dev *hdev, int seg_size,
return err;
}
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+ struct hci_event_hdr *hdr;
+ struct hci_ev_cmd_complete *evt;
+ struct sk_buff *skb;
+
+ skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+ if (!skb)
+ return -ENOMEM;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

+
+ hdr = skb_put(skb, sizeof(*hdr));
+ hdr->evt = HCI_EV_CMD_COMPLETE;
+ hdr->plen = sizeof(*evt) + 1;
+
+ evt = skb_put(skb, sizeof(*evt));
+ evt->ncmd = 1;
+ evt->opcode = HCI_OP_NOP;

After looking a bit more at it I realize HCI_OP_NOP is not a good value in this case:

static void hci_cmd_complete_evt(...)
{
...

  if (*opcode != HCI_OP_NOP)
    cancel_delayed_work(&hdev->cmd_timer);

...
}

https://elixir.bootlin.com/linux/v4.19/source/net/bluetooth/hci_event.c#L3351

Cancelling the command timeout is precisely what we want. Not sure why the patch with HCI_OP_NOP makes the timeouts go away in most cases (but not e.g. when inserting an msleep(1000) after downloading the NVM.

I suggest to pass the opcode of the command to be completed.

Quoted 83 lines

Quoted 52 lines

Quoted 45 lines

+
+ skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+ hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+ return hci_recv_frame(hdev, skb);
+}
+
static int qca_download_firmware(struct hci_dev *hdev,
struct rome_config *config)
{
@@ -297,11 +323,22 @@ static int
qca_download_firmware(struct hci_dev *hdev,
ret = qca_tlv_send_segment(hdev, segsize, segment,
config->dnld_mode);
if (ret)
- break;
+ goto out;
segment += segsize;
}
+ /* Latest qualcomm chipsets are not sending a command
complete event
+ * for every fw packet sent. They only respond with a
vendor specific
+ * event for the last packet. This optimization in the chip will
+ * decrease the BT in initialization time. Here we will
inject a command
+ * complete event to avoid a command timeout error message.
+ */
+ if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+ config->dnld_type == ROME_SKIP_EVT_VSE))
+ return qca_inject_cmd_complete_event(hdev);
+
have you actually considered using __hci_cmd_send in that case. It is
allowed for vendor OGF to use that command. I see you actually do use
it and now I am failing to understand what this is for.
[Bala]: thanks for reviewing the change.

__hci_cmd_send() can be used only to send the command to the chip.
it will not wait for the response for the command sent.

as you know that every vendor command sent to chip will respond with
vendor specific event and command complete event.
but in our case chip will only respond with vendor specific event
only. so we are injecting command complete event.

and __hci_cmd_sync_ev is also not working for you? However since you are not waiting for the vendor event anyway and just injecting cmd_complete, I wonder what’s the difference in just using __hci_cmd_send and not bothering to wait or inject at all. I am failing to see where this injection makes a difference.

Quoted 7 lines

For me it is a big difference if we are injecting one event like in the case of Intel compared to injecting one for every command. It will show a wrong picture in btmon and that is a bad idea.

Regards

Marcel

[Bala]: here is the use case, when ever we download the fw packets i.e. RAM
image, for every command sent(i.e. fw packet) from
the host chip will respond with an vendor specific event and command complete event.

the above is taking more time to setup the BT device. then we came up with
solution where we enable flags in fw file (i.e. RAM image header) whether to wait for event to be received or sent the total packets and wait
for the events for the last packet.

So currently we are handling both the cases in the code. i.e wait for event
for all packet or wait for an event for the last packet.

but in the second case i.e. wait for event for the last packet sent, we are
only receiving an vendor specific event from chip which holds the status of
fw download.

Quoted 6 lines

so as __hci_cmd_sync_ev() requires an command complete event. so we are
injecting it after the vendor specific event received for the last packet.

This helps to overcome 0xfc00 timeout error logging on console.

Some more details:

The timeout error is actually from reading the 'SoC version', which uses the same command code as the firmware download (EDL_PATCH_CMD_OPCODE). Without reading the 'SoC version' it would be from the command to write the first firmware segment.

If the download of a firmware binary takes >= 2s (HCI_CMD_TIMEOUT) the timeout would still occur. If necessary this could be mitigated by injecting some command complete events during the firmware download, though I expect Marcel wouldn't be overly happy with that, since it would affect btmon even more.

Quoted 3 lines

Regards

Matthias

Quoted 8 lines

[Bala]: Basically every vendor specific command we sent to chip, chip should respond with an vendor specific event followed by an command
complete event
or some times it will only respond with an command complete event. but in any case command complete event is mandatory to all the command we
sent to the chip.

Is this ("command complete event is mandatory to all the command we sent to the chip") a description of what the chip actually does, or what it should be doing according to the spec?

As mentioned earlier, the timeout we see originates from reading the SoC version:

drivers/bluetooth/btqca.c +4 -0

index 0b533f65f652fc..1e484e61799571 100644
@@ -400,6 +400,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t
 baudrate,
                return err;
        }
 
+       printk("DBG: ZZZzzzzzzz\n");
+       msleep(2500);
+       printk("DBG: good morning!\n");
+
        /* Download NVM configuration */
        config.type = TLV_TYPE_NVM;

        if (soc_type == QCA_WCN3990)

Quoted 7 lines

When you boot with this patch you'll see something like this:

[   15.531365] DBG: reading SoC version
[   15.544963] DBG: ZZZzzzzzzz
[   17.590282] Bluetooth: hci0: command 0xfc00 tx timeout
[   18.099110] DBG: good morning!

[Bala]: my previous analysis were wrong. thanks for pointing me to the correct issue.
i am able to see timeout after version command.(used some
sleep)
here is the reason for it.

01 00 fc 01 19 (we send the command to chip to request
version)

        04 ff 0e 00 02 0a 00 00 00 01 00 01 02 14 02 01 40(chip will

respond
with the vendor specific event payload will be chip version)
04 0e 04 01 00 00 00(command complete event)

issue is with command complete event. ideally command
complete event
payload holds the command for which chip sends command complete event.

breaking the command

04 : event packet (fixed)
0e : command complete event(fixed)
04 : size of the payload,

01 : no of commands in the payload
00 : OCF (opocde lsb)
00 : OGF (opcode msb)
00 : status of command executed,

payload is form byte 4 to byte 7. which hold the command i.e
opcode

      form the above response opcode is 0x0000. ideally this should be

0xfc00
because the command complete response from the chip is for the
command
0xfc00.

Checked with Chip firmware dev team for the reason, it was an
limitation or an bug in the chip ROM firmware
in the current chipset, which is fixed in the coming chipset.

Quoted 6 lines

so we expect the command 0xfc00 but recevuies 0x00. so that is
the
reason we see an timeouts.

      for all the vendor commands we receive 0x0000 in the command

complete
event.

Thanks for your analysis.

It is unfortunate (though not unexpected) that this is a problem in the ROM were we can't fix it, but at least the FW team is aware of it and fixed it for future hardware.

Quoted 6 lines

      if comes a new question how are we not seeing an 0xfc00 when

we inject
an command complete event.
i experimented this patch, when i inject the command complete
event
from the soc version still i can see the command timeouts.

Quoted 5 lines

I don't see command timeouts when injecting a command complete event after reading the SoC version. I suspect you still use patch which injects a command complete event with HCI_OP_NOP, which is precisely what the chip does ...

[Bala]: i am seeing command timeout after injecting cc event after read
version request.

Quoted 26 lines

Quoted 10 lines

The reason for no timeouts error, we are injecting an command
complete
after last packet sent out, so this injection helps
.bin file to speed out it download part, so as the bin is dumped
fastly we send an HCI RESET Comamnd where we recevie an command complete
event
with non zero opcode which is cancelling the command timeout
timer.

Quoted 7 lines

when we inject command complete event, in short we are
completing
setup process in less than 2 seconds(command timeout timer value),
so before 2 seconds we are sending an HCI RESET command whose
command
complete event opcode value is an non zero (which cancels the command timeout timer)

I agree that HCI_RESET cancels the command timeout timer if it is sent before the 2s timout expires, but I have doubts on the part about command complete events.

In my observations there are no command timeouts if a command complete (with opcode != HCI_OP_NOP) is injected after the firmware download, even if a 3s sleep is added before sending the HCI_RESET.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

[Bala]: add sleep before sending HCI reset, oxfc00 timeout observed.

Please double check your setup. Are you 100% sure you are not still sending HCI_OP_NOP as opcode?

[Bala]: sorry i am injecting the HCI_OP_NOP, so that is the reason we have an timeouts.

       yes if the opcode 0x42 injected, no timeouts are observed.

Quoted 6 lines

Please don't use 0x42 (at least not as literal) when you respin, I only chose this value to illustrate that it's really the injected command which cancels the timer. You might want to add a QCA_HCI_OP_DUMMY or similar, to make clear that the value doesn't matter (as long as it isn't an actual opcode). With a define I don't really care if you use 0x01, 0x42 or any other non-opcode value.

[Bala]: will not use 0x42, will use an opcode 0xfc00.

Quoted 84 lines

Quoted 70 lines

Quoted 70 lines

This is the diff I use for debugging on top of your patchset:

drivers/bluetooth/btqca.c +7 -0

index 0b533f65f652fc..5bb9e0ca7c348b 100644
@@ -27,6 +27,8 @@
 
 #define VERSION "0.1"
 
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev);
+
 int qca_read_soc_version(struct hci_dev *hdev, u32 *soc_version)
 {
 	struct sk_buff *skb;

@@ -84,6 +86,11 @@ int qca_read_soc_version(struct hci_dev *hdev, u32
 *soc_version)
 	if (*soc_version == 0)
 		err = -EILSEQ;
 
+	qca_inject_cmd_complete_event(hdev);
+	printk("DBG: ZZZzzzzzzz\n");
+	msleep(2500);
+	printk("DBG: good morning!\n");
+
 out:
 	kfree_skb(skb);

drivers/bluetooth/btqca.c +1 -1

 	if (err)

@@ -281,7 +288,7 @@ static int qca_inject_cmd_complete_event(struct
 hci_dev *hdev)
 
 	evt = skb_put(skb, sizeof(*evt));
 	evt->ncmd = 1;
-	evt->opcode = HCI_OP_NOP;
+	evt->opcode = 0x42;
 
 	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);

net/bluetooth/hci_event.c +4 -1

index f12555f23a49a0..c87ac1823439ab 100644
@@ -3348,8 +3348,11 @@ static void hci_cmd_complete_evt(struct hci_dev
 *hdev, struct sk_buff *skb,
 		break;
 	}
 
-	if (*opcode != HCI_OP_NOP)
+	if (*opcode != HCI_OP_NOP) {
+		printk("DBG: cancelling command timer (opcode = 0x%02x)\n",
+		       *opcode);
 		cancel_delayed_work(&hdev->cmd_timer);
+	}
 
 	if (ev->ncmd && !test_bit(HCI_RESET, &hdev->flags))

atomic_set(&hdev->cmd_cnt, 1);

With that I see:

[   18.725417] Bluetooth: hci0: setting up wcn3990
[   18.860381] DBG: ZZZzzzzzzz
[   18.863332] DBG: cancelling command timer (opcode = 0x42)
[   21.427085] DBG: good morning!

The timer is clearly cancelled by the injected command (opcode 0x42) and no timeout occurs. I wouldn't expect you to see anything vastly different.

Quoted 8 lines

Quoted 3 lines

Cheers

Matthias

[Bala]: I think it would be better if we inject a command complete event after we
receive the version data. instead of injecting in between firmware
download.
this change will be only for wcn3990.

At first sight that doesn't sound like a bad idea if we want to inject a single command complete event. However the NVM download times out unless the command(s) of the TLV download is completed:

[   21.789452] Bluetooth: hci0: QCA Downloading qca/crnv21.bin
[   21.797675] Bluetooth: hci0: Send segment 0, size 243
[   23.605597] Bluetooth: hci0: command 0xfc00 tx timeout

Quoted 6 lines

So we should inject one command complete event after reading the SoC version and another after downloading the TLV file. An event after downloading the NVM file seems optional, since the HCI reset immediately after will cancel the timer anyway.
Cheers
Matthias