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

[Balakrishna Godavarthi <bgodavar@codeaurora.org>]

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@codeaurora.org>
>> Tested-by: Matthias Kaehlcke <mka@chromium.org>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>  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.