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

[Matthias Kaehlcke]

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


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.

Cheers

Matthias