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

[Matthias Kaehlcke <mka@chromium.org>]

Hi Balakrishna,

On Wed, Dec 26, 2018 at 11:15:30AM +0530, Balakrishna Godavarthi wrote:
> 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.

Thanks!

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

100 ms is definitely better than 300 ms if that's not really
needed. Did you see the need for a 100 ms delay with the WCN3990 or
some other QCA controller?

Cheers

Matthias