Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command

From: Balakrishna Godavarthi
Date: Wed Dec 26 2018 - 00:45:45 EST

Hi Matthias,

On 2018-12-22 06:01, Matthias Kaehlcke wrote:
On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
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 <bgodavar@xxxxxxxxxxxxxx>
Tested-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
Reviewed-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
---
drivers/bluetooth/hci_qca.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 5a07c2370289..1680ead6cc3d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -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;

I looked for ways to do without this change, but didn't find a good
solution. There are several possible problems with baudrate changes:

1) send request to BT controller to change the baudrate

this is an asynchronous operation, the actual baudrate change can
be delayed for multiple reasons, e.g.:

- request sits in the BT driver's TX queue

this could be worked around by checking skb_queue_empty()

- request sits in the UART buffer

a workaround for this could be calling
serdev_device_wait_until_sent() (only available with serdev though)

- the request sits in the UART FIFO

will be sent out 'immediately'. no neat solution available AFAIK,
a short sleep could be an effective workaround

- the controller may have a short delay to apply the change

Also no neat solution here. A/the same short sleep could work
around this

2) change baudrate of the host UART
- this must not happen before the baudrate change request has been
sent to the BT controller, otherwise things are messed up
seriously

Ideally set_termios would make sure all pending data is sent
before the change is applied, some UART drivers do this, others
don't, so we can't rely on this.

3) BT controller sends data after baudrate change

a few ms after a baudrate change the BT controller sends data
(4, 255, 2, 146, 1, 4, 14, 4, 1, 0, 0, 0) with the new baudrate

- dunno what the data stands for, but the BT stack/driver appears to
be fine with it, as long as the host UART operates at the new
baudrate when the data is received.

- if the data is received before the baudrate of the host UART is
changes we see 'frame reassembly' errors


[Bala]: the data is an vendor specific event and command complete event,
4, 255, 2, 146, 1, : vendor specific event
4, 14, 4, 1, 0, 0, 0: command complete event.

In summary, I think it should be feasible to guarantee that the
baudrate change of the host UART is always done after the controller
changed it's baudrate, however we can't guarantee at the same time
that the baudrate change of the host controller is completed before
the BT controller sends its 'response'.

Using the RTS signal seems a reasonable way to delay the controller
data until the host is ready, the only thing I don't like too much
is that in this patch set we currently have two mechanisms to
suppress/delay unwanted data. Unfortunately the RTS method isn't
effective at initialization time.

Not the scope of this patch set, but I really dislike the 300 ms delay
(BAUDRATE_SETTLE_TIMEOUT_MS) in qca_set_baudrate(), and wonder if it
is actually needed (I seriously doubt that it takes the BT controller
300 ms to change its baudrate). I guess it's more a combination of what I
described above in 1), once we are done with this series I might try
to improve this, unless somebody is really, really convinced that such
a gigantic delay is actually needed.

[Bala]: Thanks for detail analysis.
even i feel the same whether is it really required to have an delay of 300ms.
But during our testing we found the it depends on the controller clock settling time.
all observations are less than 100 ms. will update this change in separate patch series.

Cheers

Matthias

--
Regards
Balakrishna.