Bug fixes for Qualcomm BT chip wcn3990

PATCH Superseded Balakrishna Godavarthi 2 participants 12 messages Nov 30, 2018 – Dec 14, 2018

Subsystems (3)

Arm Qualcomm mailing list Bluetooth drivers Qualcomm Bluetooth driver

lore

Share & tools

lore

Balakrishna Godavarthi

author

2018-11-30 15:02 UTC #0 lore raw

v3 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

Changes in v3:
* moved IBS & qca_flush to different patch
* updated comments in code fo Deassert RTS patch

Changes in v2:
* Removed Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag patch will be sending as separate series.
Balakrishna Godavarthi (4):
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

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

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Balakrishna Godavarthi

author

2018-11-30 15:02 UTC #1 lore raw

PATCH v3 1/4 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.

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

---
v3:
* no change.
v2:
* Updated function qca_send_power_pulse()
* addressed reviewer comments.

v1:
* initial patch

---
 drivers/bluetooth/hci_qca.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

drivers/bluetooth/hci_qca.c +13 ~~-24~~

index f036c8f98ea3..f5dd323c1967 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,19 +1027,16 @@ 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 SoC", cmd);
 	hci_uart_set_flow_control(hu, true);
+	ret = serdev_device_write(hu->serdev, &cmd, sizeof(cmd), 0);
+	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 +1111,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 +1133,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 +1268,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

Johan Hovold

2018-12-05 06:25 UTC #2 lore raw

Re: [PATCH v3 1/4] Bluetooth: hci_qca: use wait_until_sent() for power pulses

Quoted 6 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.

Quoted 7 lines

Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v3:
* no change.
v2:
* Updated function qca_send_power_pulse()
* addressed reviewer comments.

Please make sure to include reviewers on CC when resending, and as someone else already mentioned, be a bit more specific about what changes you actually made in response to the review feedback you received.

Quoted 27 lines

v1:
* initial patch

---
 drivers/bluetooth/hci_qca.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

drivers/bluetooth/hci_qca.c +2 -4

index f036c8f98ea3..f5dd323c1967 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,19 +1027,16 @@ 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.

drivers/bluetooth/hci_qca.c static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd) · line 1032

Quoted 10 lines

drivers/bluetooth/hci_qca.c +2 -6

 	 */
-	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 SoC", cmd);
 	hci_uart_set_flow_control(hu, true);
+	ret = serdev_device_write(hu->serdev, &cmd, sizeof(cmd), 0);

You're still using 0 as a timeout here which is broken, as I already told you.

Quoted 1 line

From 4.21 this will result in an indefinite timeout, but currently

implies not to wait for a full write buffer to drain at all.

As I also mentioned, you need to to make sure to call serdev_device_write_wakeup() in the write_wakup() path if you are going to use serdev_device_write() at all.

Johan

Balakrishna Godavarthi

author

2018-12-06 10:40 UTC #3 lore raw

Re: [PATCH v3 1/4] Bluetooth: hci_qca: use wait_until_sent() for power pulses

Hi Johan,

Quoted 15 lines · Johan Hovold

Quoted 6 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.
Quoted 7 lines
Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v3:
* no change.
v2:
* Updated function qca_send_power_pulse()
* addressed reviewer comments.

Quoted 5 lines

Please make sure to include reviewers on CC when resending, and as someone else already mentioned, be a bit more specific about what changes you actually made in response to the review feedback you received.

[Bala]: sure will add and provide more info in version change history.

Quoted 35 lines

v1:
* initial patch

---
 drivers/bluetooth/hci_qca.c | 37 
+++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

drivers/bluetooth/hci_qca.c +2 -4

index f036c8f98ea3..f5dd323c1967 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

drivers/bluetooth/hci_qca.c +0 -4

 	 * at required baudrate to wcn3990. On wcn3990, we have an external

@@ -1029,19 +1027,16 @@ 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)

Quoted 9 lines

drivers/bluetooth/hci_qca.c static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd) · line 1032
Quoted 5 lines
drivers/bluetooth/hci_qca.c +2 -2
-		return -ENOMEM;
-
+	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
 	hci_uart_set_flow_control(hu, true);
+	ret = serdev_device_write(hu->serdev, &cmd, sizeof(cmd), 0);
You're still using 0 as a timeout here which is broken, as I already told you.

[Bala]: got the change now will update to timeout to non zero value.

Quoted 7 lines

From 4.21 this will result in an indefinite timeout, but currently implies not to wait for a full write buffer to drain at all.
As I also mentioned, you need to to make sure to call serdev_device_write_wakeup() in the write_wakup() path if you are going to use serdev_device_write() at all.

[Bala]: this where i am confused.
calling serdev_device_write is calling an wakeup internally.
below is the flow

ttyport_write_buf:
* calling serdev_device_write() will call write_buf() in
this call we are enabling bit "TTY_DO_WRITE_WAKEUP" and calling write()
i.e. uart_write() where we call in start_tx. this will
go to the vendor specific write where in isr we call uart_write_wakeup()

https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/qcom_geni_serial.c#L756

uart_write_wakeup()->ttyport_write_wakeup()->serdev_controller_write_wakeup()->hci_uart_write_wakeup()->hci_uart_tx_wakeup()

the above is flow when serdev_device_write() is called, it is
indirectly calling serdev_write_wakeup().

Why actual we need to call an serdev_write_wakeup() is this
wakeup related to the UART port or for the BT chip.

Quoted 1 line

Johan

Signature · 2 lines

Regards
Balakrishna.

Balakrishna Godavarthi

author

2018-12-11 15:42 UTC #4 lore raw

Re: [PATCH v3 1/4] Bluetooth: hci_qca: use wait_until_sent() for power pulses

Hi Johan,

Quoted 96 lines · Balakrishna Godavarthi

Hi Johan,
Quoted 15 lines · Johan Hovold
Quoted 6 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.
Quoted 7 lines
Signed-off-by Balakrishna Godavarthi <[email protected]>
---
v3:
* no change.
v2:
* Updated function qca_send_power_pulse()
* addressed reviewer comments.
Quoted 5 lines
Please make sure to include reviewers on CC when resending, and as someone else already mentioned, be a bit more specific about what changes you actually made in response to the review feedback you received.
[Bala]: sure will add and provide more info in version change history.
Quoted 35 lines
Quoted 35 lines
v1:
* initial patch
---
 drivers/bluetooth/hci_qca.c | 37 
+++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)
drivers/bluetooth/hci_qca.c +2 -4
index f036c8f98ea3..f5dd323c1967 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
drivers/bluetooth/hci_qca.c +0 -4
 	 * at required baudrate to wcn3990. On wcn3990, we have an external
@@ -1029,19 +1027,16 @@ 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)
Quoted 9 lines
drivers/bluetooth/hci_qca.c static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd) · line 1032
Quoted 5 lines
drivers/bluetooth/hci_qca.c +2 -2
-		return -ENOMEM;
-
+	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
 	hci_uart_set_flow_control(hu, true);
+	ret = serdev_device_write(hu->serdev, &cmd, sizeof(cmd), 0);
You're still using 0 as a timeout here which is broken, as I already told you.
[Bala]: got the change now will update to timeout to non zero value.
Quoted 7 lines
From 4.21 this will result in an indefinite timeout, but currently implies not to wait for a full write buffer to drain at all.
As I also mentioned, you need to to make sure to call serdev_device_write_wakeup() in the write_wakup() path if you are going to use serdev_device_write() at all.
[Bala]: this where i am confused.
calling serdev_device_write is calling an wakeup internally.
below is the flow
ttyport_write_buf:
* calling serdev_device_write() will call write_buf() in
this call we are enabling bit "TTY_DO_WRITE_WAKEUP" and calling write()
i.e. uart_write() where we call in start_tx. this will
go to the vendor specific write where in isr we call uart_write_wakeup()
https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/qcom_geni_serial.c#L756

Quoted 9 lines

uart_write_wakeup()->ttyport_write_wakeup()->serdev_controller_write_wakeup()->hci_uart_write_wakeup()->hci_uart_tx_wakeup()
the above is flow when serdev_device_write() is called, it is
indirectly calling serdev_write_wakeup().
Why actual we need to call an serdev_write_wakeup() is this
wakeup related to the UART port or for the BT chip.
Quoted 1 line
Johan

Can you help me to understand, whether my understating is correct wrt serdev_wakeup().

Signature · 2 lines

Regards
Balakrishna.

Johan Hovold

2018-12-12 16:42 UTC #5 lore raw

Re: [PATCH v3 1/4] Bluetooth: hci_qca: use wait_until_sent() for power pulses

In reply to Balakrishna Godavarthi

Quoted 29 lines · Balakrishna Godavarthi

Hi Johan,
Quoted 6 lines · Johan Hovold
drivers/bluetooth/hci_qca.c static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd) · line 1032
Quoted 1 line · Balakrishna Godavarthi
drivers/bluetooth/hci_qca.c +1 -0
+	ret = serdev_device_write(hu->serdev, &cmd, sizeof(cmd), 0);
You're still using 0 as a timeout here which is broken, as I already told you.
[Bala]: got the change now will update to timeout to non zero value.
Quoted 6 lines
From 4.21 this will result in an indefinite timeout, but currently implies not to wait for a full write buffer to drain at all.
As I also mentioned, you need to to make sure to call serdev_device_write_wakeup() in the write_wakup() path if you are going to use serdev_device_write() at all.
[Bala]: this where i am confused.
calling serdev_device_write is calling an wakeup internally.
below is the flow
ttyport_write_buf:
* calling serdev_device_write() will call write_buf() in
this call we are enabling bit "TTY_DO_WRITE_WAKEUP" and calling write()
i.e. uart_write() where we call in start_tx. this will
go to the vendor specific write where in isr we call uart_write_wakeup()

Quoted 7 lines

https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/qcom_geni_serial.c#L756
uart_write_wakeup()->ttyport_write_wakeup()->serdev_controller_write_wakeup()->hci_uart_write_wakeup()->hci_uart_tx_wakeup()
the above is flow when serdev_device_write() is called, it is
indirectly calling serdev_write_wakeup().

No, serdev_device_write_wakeup() is currently not called in this path, which means you cannot use serdev_device_write().

Quoted 2 lines

Why actual we need to call an serdev_write_wakeup() is this
wakeup related to the UART port or for the BT chip.

serdev_device_write_wakeup() is where a writer blocked on a full write buffer in serdev_device_write() is woken up.

Johan

Balakrishna Godavarthi

author

2018-12-14 12:32 UTC #6 lore raw

Re: [PATCH v3 1/4] Bluetooth: hci_qca: use wait_until_sent() for power pulses

Hi Johan,

Quoted 43 lines · Johan Hovold

Quoted 31 lines · Balakrishna Godavarthi
Hi Johan,
Quoted 6 lines · Johan Hovold
drivers/bluetooth/hci_qca.c static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd) · line 1032
Quoted 1 line · Balakrishna Godavarthi
drivers/bluetooth/hci_qca.c +1 -0
+	ret = serdev_device_write(hu->serdev, &cmd, sizeof(cmd), 0);
You're still using 0 as a timeout here which is broken, as I already told you.
[Bala]: got the change now will update to timeout to non zero value.
Quoted 6 lines
From 4.21 this will result in an indefinite timeout, but currently implies not to wait for a full write buffer to drain at all.
As I also mentioned, you need to to make sure to call serdev_device_write_wakeup() in the write_wakup() path if you are going to use serdev_device_write() at all.
[Bala]: this where i am confused.
calling serdev_device_write is calling an wakeup internally.
below is the flow
ttyport_write_buf:
* calling serdev_device_write() will call write_buf()
in
this call we are enabling bit "TTY_DO_WRITE_WAKEUP" and calling write()
i.e. uart_write() where we call in start_tx. this
will
go to the vendor specific write where in isr we call uart_write_wakeup()
Quoted 7 lines
https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/qcom_geni_serial.c#L756
uart_write_wakeup()->ttyport_write_wakeup()->serdev_controller_write_wakeup()->hci_uart_write_wakeup()->hci_uart_tx_wakeup()
the above is flow when serdev_device_write() is called, it is
indirectly calling serdev_write_wakeup().
No, serdev_device_write_wakeup() is currently not called in this path, which means you cannot use serdev_device_write().

Quoted 7 lines

Quoted 2 lines
Why actual we need to call an serdev_write_wakeup() is this
wakeup related to the UART port or for the BT chip.
serdev_device_write_wakeup() is where a writer blocked on a full write buffer in serdev_device_write() is woken up.
Johan

Is it preferred to use and serdev_device_write_buf() followed by serdev_device_wait_until_sent() or do we required an write_wakeup() called before writing into serdev_device_write_buf()

Signature · 2 lines

Regards
Balakrishna.

Johan Hovold

2018-12-14 13:16 UTC #7 lore raw

Re: [PATCH v3 1/4] Bluetooth: hci_qca: use wait_until_sent() for power pulses

Quoted 16 lines · Balakrishna Godavarthi

Hi Johan,
Quoted 13 lines · Johan Hovold
Quoted 4 lines · Balakrishna Godavarthi
uart_write_wakeup()->ttyport_write_wakeup()->serdev_controller_write_wakeup()->hci_uart_write_wakeup()->hci_uart_tx_wakeup()
the above is flow when serdev_device_write() is called, it is
indirectly calling serdev_write_wakeup().
No, serdev_device_write_wakeup() is currently not called in this path, which means you cannot use serdev_device_write().
Quoted 2 lines
Why actual we need to call an serdev_write_wakeup() is this
wakeup related to the UART port or for the BT chip.

Quoted 7 lines

Quoted 4 lines
serdev_device_write_wakeup() is where a writer blocked on a full write buffer in serdev_device_write() is woken up.
Johan
Is it preferred to use and serdev_device_write_buf() followed by serdev_device_wait_until_sent()

That's up to the driver author and depends on what you want to achieve.

If you want to do something synchronously, you always have to call serdev_device_wait_until_sent() after, as both serdev_device_write_buf() and serdev_device_write() only buffer the data.

If you use serdev_device_write_buf() you also have to make sure that you can handle incomplete buffering yourself (e.g. when the underlying tty driver buffer is getting full).

serdev_device_write() was added as a convenience helper for this, but depends on serdev_device_write_wakeup() being called in the write_wakeup path to make forward progress.

Quoted 2 lines

or do we required an write_wakeup() called before writing into serdev_device_write_buf()

No, that doesn't make any sense. serdev_device_write_wakeup() needs to be called in the write-wakeup path, but only if you use serdev_device_write().

Please see the documentation of these function that I added to linux-next (and that I linked to earlier).

Johan

Balakrishna Godavarthi

author

2018-12-14 13:41 UTC #8 lore raw

Re: [PATCH v3 1/4] Bluetooth: hci_qca: use wait_until_sent() for power pulses

Hi Johan,

Quoted 20 lines · Johan Hovold

Quoted 19 lines · Balakrishna Godavarthi
Hi Johan,
Quoted 16 lines · Johan Hovold
Quoted 4 lines · Balakrishna Godavarthi
uart_write_wakeup()->ttyport_write_wakeup()->serdev_controller_write_wakeup()->hci_uart_write_wakeup()->hci_uart_tx_wakeup()
the above is flow when serdev_device_write() is called, it is
indirectly calling serdev_write_wakeup().
No, serdev_device_write_wakeup() is currently not called in this path, which means you cannot use serdev_device_write().
Quoted 2 lines
Why actual we need to call an serdev_write_wakeup() is this
wakeup related to the UART port or for the BT chip.
serdev_device_write_wakeup() is where a writer blocked on a full write buffer in serdev_device_write() is woken up.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Johan
Is it preferred to use and serdev_device_write_buf() followed by serdev_device_wait_until_sent()
That's up to the driver author and depends on what you want to achieve.

[Bala]: During the BT on & of, we need to send the commands to the controller
at a specific baudrate. as the data is getting queued in the buffer we see some times the data is transmitting on the newer baudrate instead of the actual baudrate. we want to achieve an synchronous data transfer for some commands to over come byte corruptions.

Quoted 4 lines

If you want to do something synchronously, you always have to call serdev_device_wait_until_sent() after, as both serdev_device_write_buf() and serdev_device_write() only buffer the data.

Quoted 8 lines

If you use serdev_device_write_buf() you also have to make sure that you can handle incomplete buffering yourself (e.g. when the underlying tty driver buffer is getting full).
serdev_device_write() was added as a convenience helper for this, but depends on serdev_device_write_wakeup() being called in the write_wakeup path to make forward progress.

[Bala]: Thanks for the info, now i understood the usage of the both write() and write_buf().

Quoted 7 lines

Quoted 2 lines
or do we required an write_wakeup() called before writing into serdev_device_write_buf()
No, that doesn't make any sense. serdev_device_write_wakeup() needs to be called in the write-wakeup path, but only if you use serdev_device_write().

Quoted 5 lines

Please see the documentation of these function that I added to linux-next (and that I linked to earlier).
Johan

Thanks Johan for the detailed info.

Signature · 2 lines

Regards
Balakrishna.

Balakrishna Godavarthi

author

2018-11-30 15:02 UTC #9 lore raw

PATCH v3 2/4 Bluetooth: hci_qca: Deassert RTS while baudrate change command

In reply to Balakrishna Godavarthi

This 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.

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

Tested-by Matthias Kaehlcke <[email protected]>

Reviewed-by Matthias Kaehlcke <[email protected]>

---
v3:
* Updated comments in code to be more specific.
v2:
* updated commit text and comments.
v1:
* initial patch

---
 drivers/bluetooth/hci_qca.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

drivers/bluetooth/hci_qca.c +13 ~~-11~~

index f5dd323c1967..253b5bae30c2 100644
@@ -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;
 }

@@ -1086,6 +1075,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) {

@@ -1097,6 +1087,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);

@@ -1104,6 +1103,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

Balakrishna Godavarthi

author

2018-11-30 15:02 UTC #10 lore raw

PATCH v3 3/4 Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990

In reply to 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.

[  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.

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

Tested-by Matthias Kaehlcke <[email protected]>

---
v3:
* moved ibs disable and qca_flush to different patch
* updated commnents in code.
v2:
* Updated commit text & comments.
v1:
* initial patch

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

drivers/bluetooth/hci_qca.c +16 -0

index 253b5bae30c2..d595a82cb4b9 100644
@@ -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)) {

@@ -1194,6 +1203,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;

@@ -1270,6 +1280,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

Balakrishna Godavarthi

author

2018-11-30 15:02 UTC #11 lore raw

PATCH v3 4/4 Bluetooth: hci_qca: Disable IBS state machine and flush Tx buffer

In reply to Balakrishna Godavarthi

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]>

---
v3:
* initial patch

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

drivers/bluetooth/hci_qca.c +2 -0

index d595a82cb4b9..6f2a4bf7ba17 100644
@@ -1286,6 +1286,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