Message-ID: <33b964734ec255f75482475e7cb064c2@codeaurora.org>
Date:   Thu, 27 Dec 2018 08:50:49 +0530
From:   Balakrishna Godavarthi <bgodavar@...eaurora.org>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     marcel@...tmann.org, johan.hedberg@...il.com, johan@...nel.org,
        linux-kernel@...r.kernel.org, linux-bluetooth@...r.kernel.org,
        hemantg@...eaurora.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate
 change command

Hi Matthias,

On 2018-12-27 01:55, Matthias Kaehlcke wrote:
> 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@...eaurora.org>
>> > > Tested-by: Matthias Kaehlcke <mka@...omium.org>
>> > > Reviewed-by: Matthias Kaehlcke <mka@...omium.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?

[Bala]: i am not sure about other controller will check that. but for 
wcn3990 we can go
         with the 100ms.

> 
> Cheers
> 
> Matthias


-- 
Regards
Balakrishna.

Powered by blists - more mailing lists