Bug fixes for Qualcomm BT chip wcn3990
v6 cover Bug fixes for Qualcomm BT chip wcn3990
The below issues are found in the recent testing.
1. Observed device is not going into off state or not responding. As wcn3990 require a power pulses to turn on the irrespctive of igniting regulators, it was observed that power on or power off pulses are not in sync with respective to chip.
The below patch will help us to wait until byte is pushed on to wires.
* Bluetooth: hci_qca: use wait_until_sent() for power pulses
2. Observed Chip responding when we are in sleep.
This is due to turn on flow control during change baudrate request. The below patch will only pull the RTS line high instead of turning off the flow.
* Bluetooth: hci_qca: Pull RTS line high for baudrate change command.
3. Frame reassembly errors splashing on console.
wcn3990 requires will use multiple baudrates during booting stage. i.e. 2400 bps while sending power off pulse
115200 bps while sending power on pulse
port close
port open
set baudrate to 115200 request the chip version.
during above process, we are seeing some stray bytes coming up on the UART Rx FIFO it could be due to frequent baudrate change.
This patch will stop the frame reassembly errors.
* Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990
4. Disable IBS state machine and flush Tx buffer
We are disabling IBS and flushing the Tx buffer before turning off the chip.
This is due to IBS state machine is active when we turn off the chip. This will stop queuing IBS protocol bytes.
* Bluetooth: hci_qca: Disable IBS state machine and flush Tx buffer
5. hci_qca: Update baudrate change wait time for wcn3990
This patch will help to decrease the BT on time by 200ms. we updated the baudrate change wait time to 100ms instead of 300ms.
6. btqca: inject command complete event during fw download
Qualcomm latest chip will not send an command complete event for last packet of the fw dump sent, so here we are inject an command complete event.
* Bluetooth: btqca: inject command complete event during fw download
Changes in v6:
* added serdev_device_write_flush in qca_send_power_pulse().
* added new patch to update the baudrate change wait time.
Changes in V5:
* added serdev_device_write_flush before sending the power off pulse during shutdown.
Changes in v4:
* used serdev_device_write_buf() instead of serdev_device_write().
* added new patch to stop logging of 0xfc00 timeout on console.
Changes in v3:
* moved IBS & qca_flush to different patch
* updated comments in code fo Deassert RTS patch
Balakrishna Godavarthi (6):
Bluetooth: hci_qca: use wait_until_sent() for power pulses
Bluetooth: hci_qca: Deassert RTS while baudrate change command
Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990
Bluetooth: hci_qca: Disable IBS state machine and flush Tx buffer
Bluetooth: hci_qca: Update baudrate change wait time for wcn3990
Bluetooth: btqca: inject command complete event during fw download
drivers/bluetooth/btqca.c | 39 +++++++++++++++- drivers/bluetooth/btqca.h | 3 ++ drivers/bluetooth/hci_qca.c | 88 ++++++++++++++++++++++--------------- 3 files changed, 94 insertions(+), 36 deletions(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v6 1/6 Bluetooth: hci_qca: use wait_until_sent() for power pulses
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.
---
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 | 38 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 24 deletions(-)
drivers/bluetooth/hci_qca.c +14 -24
@@ -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,19 +1027,17 @@ 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 ; - +serdev_device_write_flush (hu->serdev); +bt_dev_dbg (hu->hdev,"sending power pulse %02x to SoC" , cmd);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 to SoC" , + cmd); +return ret; + } -skb_put_u8 (skb, cmd); -hci_skb_pkt_type (skb) =HCI_COMMAND_PKT ; - -skb_queue_tail (&qca->txq, skb); -hci_uart_tx_wakeup (hu); +serdev_device_wait_until_sent (hu->serdev,0 );/* Wait for 100 uS for SoC to settle down */ usleep_range (100 ,200 );
@@ -1116,7 +1112,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 +1134,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 +1269,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; -unsigned char cmd =QCA_WCN3990_POWEROFF_PULSE ; -host_set_baudrate (hu,2400 ); -hci_uart_set_flow_control (hu, true); -serdev_device_write_buf (serdev, &cmd,sizeof (cmd)); -hci_uart_set_flow_control (hu, false); +qca_send_power_pulse (hu,QCA_WCN3990_POWEROFF_PULSE );qca_power_setup (hu, false); }
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v6 1/6] Bluetooth: hci_qca: use wait_until_sent() for power pulses
Quoted 39 lines · Balakrishna Godavarthi
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 v6:
* addedserdev_device_write_flush()inqca_send_power_pulseinstead during the power off pulse.Changes in v5:
* addedserdev_device_write_flush()inqca_power_off().--- drivers/bluetooth/hci_qca.c | 38 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 24 deletions(-)drivers/bluetooth/hci_qca.c +2
-4@@ -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,19 +1027,17 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)* save power. Disabling hardware flow control is mandatory while
drivers/bluetooth/hci_qca.c static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd) · line 1031
Quoted 10 lines
drivers/bluetooth/hci_qca.c +2
-6* 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 ; - +serdev_device_write_flush (hu->serdev); +bt_dev_dbg (hu->hdev,"sending power pulse %02x to SoC" , cmd);
nit: why clutter the code flow by putting the log statement in the middle of code that is actually doing something with the serial interface?
In case you respin anyway I suggest to structure it like this:
bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
hci_uart_set_flow_control(hu, true); serdev_device_write_flush(hu->serdev); ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
drivers/bluetooth/hci_qca.c static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd) · line 1035
Quoted 4 lines
drivers/bluetooth/hci_qca.c +3
-0hci_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 to SoC",
nit: especially on 'embedded' devices 'SoC' is typically associated with the CPU running Linux, you might want to change it to 'controller'.
drivers/bluetooth/hci_qca.c static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd) · line 1043
Quoted 13 lines
drivers/bluetooth/hci_qca.c +4
-5+ cmd); +return ret; + } -skb_put_u8 (skb, cmd); -hci_skb_pkt_type (skb) =HCI_COMMAND_PKT ; - -skb_queue_tail (&qca->txq, skb); -hci_uart_tx_wakeup (hu); +serdev_device_wait_until_sent (hu->serdev,0 );/* Wait for 100 uS for SoC to settle down */ usleep_range (100 ,200 );
I said earlier the delay here should be enough to ensure that the byte gets transferred from a hardware buffer/FIFO to the controller, however that didn't take into account that the power pulses are sent with a baudrate of 2400. That translates to ~240 bytes/s, hence a delay of 5 ms is needed to be on the safe side.
In case you change the delay please also update the comment to make clear this is not only time for the BT controller to settle, but also to guarantee that the command was actually sent to the controller.
So far it seems no problems have been observed, though this could be thanks to the 100 ms delay in qca_wcn3990_init().
Cheers
Matthias
Re: [PATCH v6 1/6] Bluetooth: hci_qca: use wait_until_sent() for power pulses
HI Matthias,
Quoted 67 lines · Matthias Kaehlcke
Quoted 42 lines · Balakrishna Godavarthi
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 v6:
* addedserdev_device_write_flush()inqca_send_power_pulseinstead during the power off pulse.Changes in v5:
* addedserdev_device_write_flush()inqca_power_off().--- drivers/bluetooth/hci_qca.c | 38 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 24 deletions(-)drivers/bluetooth/hci_qca.c +2
-4@@ -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 drivers/bluetooth/hci_qca.c +0
-0* at required baudrate to wcn3990. On wcn3990, we have an external @@ -1029,19 +1027,17 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd) * save power. Disabling hardware flow control is mandatory whiledrivers/bluetooth/hci_qca.c static int qca_send_power_pulse(struct hci_dev · line 1032
Quoted 10 lines
drivers/bluetooth/hci_qca.c +2
-6* 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 ; - +serdev_device_write_flush (hu->serdev); +bt_dev_dbg (hu->hdev,"sending power pulse %02x to SoC" , cmd);nit: why clutter the code flow by putting the log statement in the middle of code that is actually doing something with the serial interface?
In case you respin anyway I suggest to structure it like this:
bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);hci_uart_set_flow_control(hu, true); serdev_device_write_flush(hu->serdev); ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
Quoted 9 lines
drivers/bluetooth/hci_qca.c static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd) · line 1035
Quoted 4 lines
drivers/bluetooth/hci_qca.c +3
-0hci_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 to SoC",nit: especially on 'embedded' devices 'SoC' is typically associated with the CPU running Linux, you might want to change it to 'controller'.
[Bala]: will update.
Quoted 11 lines
Quoted 11 lines
drivers/bluetooth/hci_qca.c +4
-5+ cmd); +return ret; + } -skb_put_u8 (skb, cmd); -hci_skb_pkt_type (skb) =HCI_COMMAND_PKT ; - -skb_queue_tail (&qca->txq, skb); -hci_uart_tx_wakeup (hu); +serdev_device_wait_until_sent (hu->serdev,0 );
Quoted 9 lines
Quoted 2 lines
drivers/bluetooth/hci_qca.c +0
-0/* Wait for 100 uS for SoC to settle down */
usleep_range(100, 200);I said earlier the delay here should be enough to ensure that the byte gets transferred from a hardware buffer/FIFO to the controller, however that didn't take into account that the power pulses are sent with a baudrate of 2400. That translates to ~240 bytes/s, hence a delay of 5 ms is needed to be on the safe side.
[Bala]: sure will update the delay of 5 ms.
Quoted 4 lines
In case you change the delay please also update the comment to make clear this is not only time for the BT controller to settle, but also to guarantee that the command was actually sent to the controller.
[Bala]: will update the comment.
Quoted 6 lines
So far it seems no problems have been observed, though this could be thanks to the 100 ms delay in
qca_wcn3990_init().Cheers
Matthias
Signature · 2 lines
Regards Balakrishna.
PATCH v6 2/6 Bluetooth: hci_qca: Deassert RTS while baudrate change command
In reply to Balakrishna GodavarthiThis patch will help to stop frame reassembly errors while changing the baudrate. This is because host send a change baudrate request command to the chip with 115200 bps, Whereas chip will change their UART clocks to the enable for new baudrate and sends the response for the change request command with newer baudrate, On host side we are still operating in 115200 bps which results of reading garbage data. Here we are pulling RTS line, so that chip we will wait to send data to host until host change its baudrate.
--- drivers/bluetooth/hci_qca.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)
drivers/bluetooth/hci_qca.c +13 -11
@@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)struct hci_uart *hu =hci_get_drvdata (hdev);struct qca_data *qca = hu->priv;struct sk_buff *skb; -struct qca_serdev *qcadev;u8 cmd[] = {0x01 ,0x48 ,0xFC ,0x01 ,0x00 };if (baudrate >QCA_BAUDRATE_3200000 )
@@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)return -ENOMEM ; } -/* Disabling hardware flow control is mandatory while -* sending change baudrate request to wcn3990 SoC. -*/ - qcadev =serdev_device_get_drvdata (hu->serdev); -if (qcadev->btsoc_type ==QCA_WCN3990 ) -hci_uart_set_flow_control (hu, true); -/* Assign commands to change baudrate and packet type. */ skb_put_data (skb, cmd,sizeof (cmd));hci_skb_pkt_type (skb) =HCI_COMMAND_PKT ;
@@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)schedule_timeout (msecs_to_jiffies (BAUDRATE_SETTLE_TIMEOUT_MS ));set_current_state (TASK_RUNNING ); -if (qcadev->btsoc_type ==QCA_WCN3990 ) -hci_uart_set_flow_control (hu, false); -return 0 ; }
@@ -1087,6 +1076,7 @@ static int qca_check_speeds(struct hci_uart *hu)static int qca_set_speed (struct hci_uart *hu,enum qca_speed_type speed_type) {unsigned int speed, qca_baudrate; +struct qca_serdev *qcadev;int ret;if (speed_type ==QCA_INIT_SPEED ) {
@@ -1098,6 +1088,15 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)if (!speed)return 0 ; +/* Deassert RTS while changing the baudrate of chip and host. +* This will prevent chip from transmitting its response with +* the new baudrate while the host port is still operating at +* the old speed. +*/ + qcadev =serdev_device_get_drvdata (hu->serdev); +if (qcadev->btsoc_type ==QCA_WCN3990 ) +serdev_device_set_rts (hu->serdev, false); + qca_baudrate =qca_get_baudrate_value (speed);bt_dev_dbg (hu->hdev,"Set UART speed to %d" , speed); ret =qca_set_baudrate (hu->hdev, qca_baudrate);
@@ -1105,6 +1104,9 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)return ret;host_set_baudrate (hu, speed); + +if (qcadev->btsoc_type ==QCA_WCN3990 ) +serdev_device_set_rts (hu->serdev, true); }return 0 ;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v6 3/6 Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990
In reply to Balakrishna GodavarthiDuring initalization of wcn3990, we observed UART is reading some stray bytes on the Rx line. This is logging Frame reassembly errors on the serial console. This could be because of tristate of Tx line of wcn3990 during boot up.
[ 176.929612] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84) [ 176.945734] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84) [ 176.953298] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84) [ 177.010660] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84) [ 177.067633] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84)
Now we enable a flag during bootup to stop executing proto receive function and clear it back once the initialization is done.
--- drivers/bluetooth/hci_qca.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
drivers/bluetooth/hci_qca.c +16 -0
@@ -56,6 +56,7 @@/* Controller states */ #define STATE_IN_BAND_SLEEP_ENABLED 1 +#define STATE_DISCARD_RX 2#define IBS_WAKE_RETRANS_TIMEOUT_MS 100#define IBS_TX_IDLE_TIMEOUT_MS 2000
@@ -511,6 +512,7 @@ static int qca_open(struct hci_uart *hu) }else { hu->init_speed = qcadev->init_speed; hu->oper_speed = qcadev->oper_speed; +set_bit (STATE_DISCARD_RX , &qca->flags); ret =qca_power_setup (hu, true);if (ret) {destroy_workqueue (qca->workqueue);
@@ -903,6 +905,13 @@ static int qca_recv(struct hci_uart *hu, const void *data, int count)if (!test_bit (HCI_UART_REGISTERED , &hu->flags))return -EUNATCH ; +/* We discard Rx data received while device is in booting +* stage, This is because of BT chip Tx line is in tristate. +* Due to this we read some garbage data on UART Rx. +*/ +if (test_bit (STATE_DISCARD_RX , &qca->flags)) +return 0 ; + qca->rx_skb =h4_recv_buf (hu->hdev, qca->rx_skb, data, count, qca_recv_pkts,ARRAY_SIZE (qca_recv_pkts));if (IS_ERR (qca->rx_skb)) {
@@ -1195,6 +1204,7 @@ static int qca_setup(struct hci_uart *hu)if (ret)return ret; +clear_bit (STATE_DISCARD_RX , &qca->flags); ret =qca_read_soc_version (hdev, &soc_ver);if (ret)return ret;
@@ -1271,6 +1281,12 @@ static const struct qca_vreg_data qca_soc_data = {static void qca_power_shutdown (struct hci_uart *hu) { +struct qca_data *qca = hu->priv; + +/* From this point we go into power off state. But serial port is +* still open, discard all the garbage data received on the Rx line. +*/ +set_bit (STATE_DISCARD_RX , &qca->flags);host_set_baudrate (hu,2400 );qca_send_power_pulse (hu,QCA_WCN3990_POWEROFF_PULSE );qca_power_setup (hu, false);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v6 3/6] Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990
Quoted 4 lines · Balakrishna Godavarthi
During initalization of wcn3990, we observed UART is reading some stray bytes on the Rx line. This is logging Frame reassembly errors on the serial console. This could be because of tristate of Tx line of wcn3990 during boot up.
My testing suggests that this change is not needed if the Rx line of the SoC/AP is configured with a pull-up. We'd probably all prefer not to have this change if there's a neater way to address the garbage data. Could you test with adding the pull-up and dropping this patch on your side?
Thanks
Matthias
Re: [PATCH v6 3/6] Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990
Hi Matthias,
Quoted 12 lines · Matthias Kaehlcke
Quoted 4 lines · Balakrishna Godavarthi
During initalization of wcn3990, we observed UART is reading some stray bytes on the Rx line. This is logging Frame reassembly errors on the serial console. This could be because of tristate of Tx line of wcn3990 during boot up.
My testing suggests that this change is not needed if the Rx line of the SoC/AP is configured with a pull-up. We'd probably all prefer not to have this change if there's a neater way to address the garbage data. Could you test with adding the pull-up and dropping this patch on your side?
Quoted 3 lines
Thanks
Matthias
Thanks a lot it worked to me. will drop this patch.
Signature · 2 lines
Regards Balakrishna.
PATCH v6 4/6 Bluetooth: hci_qca: Disable IBS state machine and flush Tx buffer
In reply to Balakrishna GodavarthiDuring 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.
--- drivers/bluetooth/hci_qca.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/bluetooth/hci_qca.c +2 -0
@@ -1287,6 +1287,8 @@ static void qca_power_shutdown(struct hci_uart *hu)* still open, discard all the garbage data received on the Rx line. */ set_bit (STATE_DISCARD_RX , &qca->flags); +clear_bit (STATE_IN_BAND_SLEEP_ENABLED , &qca->flags); +qca_flush (hu);host_set_baudrate (hu,2400 );qca_send_power_pulse (hu,QCA_WCN3990_POWEROFF_PULSE );qca_power_setup (hu, false);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v6 5/6 Bluetooth: hci_qca: Update baudrate change wait time for wcn3990
In reply to Balakrishna GodavarthiThis patch will update the baudrate change request wait time from 300 ms to 100 ms. When host sends the change baudrate request to the controller, controller sets its clock and wait until the clocks settle down. Here the Wait time is required for both host and controller to be on sync.
---
Changes in v6:
* intial patch.
--- drivers/bluetooth/hci_qca.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/bluetooth/hci_qca.c +10 -2
@@ -60,7 +60,8 @@#define IBS_WAKE_RETRANS_TIMEOUT_MS 100#define IBS_TX_IDLE_TIMEOUT_MS 2000 -#define BAUDRATE_SETTLE_TIMEOUT_MS 300 +#define ROME_BD_SETTLE_TIMEOUT_MS 300 +#define WCN3990_BD_SETTLE_TIMEOUT_MS 100/* susclk rate */ #define SUSCLK_RATE_32KHZ 32768
@@ -972,8 +973,11 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)struct hci_uart *hu =hci_get_drvdata (hdev);struct qca_data *qca = hu->priv;struct sk_buff *skb; +struct qca_serdev *qcadev; +unsigned int bd_settling_timeout;u8 cmd[] = {0x01 ,0x48 ,0xFC ,0x01 ,0x00 }; + qcadev =serdev_device_get_drvdata (hu->serdev);if (baudrate >QCA_BAUDRATE_3200000 )return -EINVAL ;
@@ -996,8 +1000,12 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)* controller will come back after they receive this HCI command * then host can communicate with new baudrate to controller */ + bd_settling_timeout = qcadev->btsoc_type ==QCA_WCN3990 ? +WCN3990_BD_SETTLE_TIMEOUT_MS : +ROME_BD_SETTLE_TIMEOUT_MS ; +set_current_state (TASK_UNINTERRUPTIBLE ); -schedule_timeout (msecs_to_jiffies (BAUDRATE_SETTLE_TIMEOUT_MS )); +schedule_timeout (msecs_to_jiffies (bd_settling_timeout));set_current_state (TASK_RUNNING );return 0 ;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v6 5/6] Bluetooth: hci_qca: Update baudrate change wait time for wcn3990
Quoted 5 lines · Balakrishna Godavarthi
This patch will update the baudrate change request wait time from 300 ms to 100 ms. When host sends the change baudrate request to the controller, controller sets its clock and wait until the clocks settle down. Here the Wait time is required for both host and controller to be on sync.
Ultimately up to you, but I would advise against adding 'improvement' changes to this series. The scope of the series is already fairly vague ("Bug fixes for Qualcomm BT chip wcn3990"), with at least some patches that could be sent individually, which would reduce churn when respinning.
Quoted 15 lines
Signed-off-by Balakrishna Godavarthi <[email protected]>---
Changes in v6:
* intial patch.--- drivers/bluetooth/hci_qca.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)drivers/bluetooth/hci_qca.c +0
-0
drivers/bluetooth/hci_qca.c line 64
Quoted 6 lines
drivers/bluetooth/hci_qca.c +2
-1@@ -60,7 +60,8 @@#define IBS_WAKE_RETRANS_TIMEOUT_MS 100#define IBS_TX_IDLE_TIMEOUT_MS 2000 -#define BAUDRATE_SETTLE_TIMEOUT_MS 300 +#define ROME_BD_SETTLE_TIMEOUT_MS 300 +#define WCN3990_BD_SETTLE_TIMEOUT_MS 100
My testing suggests that even the lower 100 ms delay isn't needed, if different parts that require a delay are addressed more specifically, instead of using a single long 'settle' delay
(https://lore.kernel.org/patchwork/patch/1026795/#1212816 has some details).
My idea was to sent a patch after this series has landed to avoid interfering with it.
Cheers
Matthias
Re: [PATCH v6 5/6] Bluetooth: hci_qca: Update baudrate change wait time for wcn3990
Hi Matthias,
Quoted 7 lines · Matthias Kaehlcke
Quoted 5 lines · Balakrishna Godavarthi
This patch will update the baudrate change request wait time from 300 ms to 100 ms. When host sends the change baudrate request to the controller, controller sets its clock and wait until the clocks settle down. Here the Wait time is required for both host and controller to be on sync.
Quoted 5 lines
Ultimately up to you, but I would advise against adding 'improvement' changes to this series. The scope of the series is already fairly vague ("Bug fixes for Qualcomm BT chip wcn3990"), with at least some patches that could be sent individually, which would reduce churn when respinning.
[Bala]: yes your correct will send this as improvement patch once the series is merged.
Quoted 17 lines
Quoted 17 lines
Signed-off-by Balakrishna Godavarthi <[email protected]>---
Changes in v6:
* intial patch.--- drivers/bluetooth/hci_qca.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)drivers/bluetooth/hci_qca.c +0
-0@@ -60,7 +60,8 @@
Quoted 12 lines
drivers/bluetooth/hci_qca.c line 64
Quoted 5 lines
drivers/bluetooth/hci_qca.c +2
-1#define IBS_WAKE_RETRANS_TIMEOUT_MS 100#define IBS_TX_IDLE_TIMEOUT_MS 2000 -#define BAUDRATE_SETTLE_TIMEOUT_MS 300 +#define ROME_BD_SETTLE_TIMEOUT_MS 300 +#define WCN3990_BD_SETTLE_TIMEOUT_MS 100My testing suggests that even the lower 100 ms delay isn't needed, if different parts that require a delay are addressed more specifically, instead of using a single long 'settle' delay
(https://lore.kernel.org/patchwork/patch/1026795/#1212816 has some details).
[Bala]: i to agree with you. but from the point 1, we have many chances of getting
baudrate change delay. after lot of stress test and experiments
we found 100 ms is reasonable to handle all
of them together.
Quoted 3 lines
My idea was to sent a patch after this series has landed to avoid interfering with it.
[Bala]: sure will drop this change from the series.
Quoted 3 lines
Cheers
Matthias
Signature · 2 lines
Regards Balakrishna.
PATCH v6 6/6 Bluetooth: btqca: inject command complete event during fw download
In reply to Balakrishna GodavarthiLatest 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.
--- drivers/bluetooth/btqca.c | 39 ++++++++++++++++++++++++++++++++++++++- drivers/bluetooth/btqca.h | 3 +++ 2 files changed, 41 insertions(+), 1 deletion(-)
drivers/bluetooth/btqca.c +38 -1
@@ -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 +3 -0
@@ -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 ,
@@ -81,6 +83,7 @@ struct rome_config {char fwname[64 ];uint8_t user_baud_rate;enum rome_tlv_dnld_mode dnld_mode; +enum rome_tlv_dnld_mode dnld_type; };struct edl_event_hdr {
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project