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.
