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