Messages in this thread | | | Date | Thu, 27 Dec 2018 08:50:49 +0530 | From | Balakrishna Godavarthi <> | 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@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?
[Bala]: i am not sure about other controller will check that. but for wcn3990 we can go with the 100ms.
> > Cheers > > Matthias
-- Regards Balakrishna.
| |