From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B8287C43387 for ; Thu, 27 Dec 2018 03:21:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 756CA2148E for ; Thu, 27 Dec 2018 03:21:04 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="key not found in DNS" (0-bit key) header.d=codeaurora.org header.i=@codeaurora.org header.b="Rkfaju7D"; dkim=fail reason="key not found in DNS" (0-bit key) header.d=codeaurora.org header.i=@codeaurora.org header.b="AHj6pals" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728406AbeL0DVA (ORCPT ); Wed, 26 Dec 2018 22:21:00 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:33480 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726720AbeL0DVA (ORCPT ); Wed, 26 Dec 2018 22:21:00 -0500 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id E7EFF607F1; Thu, 27 Dec 2018 03:20:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1545880858; bh=a1z6d1hl4bhsyXWF2E7inQkMB0X0cHgtUVgzwRbBGTA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Rkfaju7DpkgYZK/5zLk3v+epnTxEbf3ric4IrIJnsUa/RIPdVcO/U559MxxMo1pBK 2kenJ4zneHG8cGBeNJbegBYQBpFKvie63No3Nud7Si67LL8MBCNwEiIE99dmwFKqQk SWmZ9FNg3+NkGI2WvtkIteMtX8/gI/vtGDXP09QQ= Received: from mail.codeaurora.org (localhost.localdomain [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id 619D960559; Thu, 27 Dec 2018 03:20:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1545880849; bh=a1z6d1hl4bhsyXWF2E7inQkMB0X0cHgtUVgzwRbBGTA=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=AHj6palsHcVskezFe3U+C4KuRjb1nOHpHzY7Uo31ktcJkI3US77D72raojo+rH3Vq ZFmqzfYHmaa0J/SDbE0RgcaDMbZeuVrVM9MjaSYKVxssGHmYir605tVJd9EDiKuvU+ W0/qRKjZNqLw7pq/t9cRGtdSCzLYBniqlfEWV19U= MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 27 Dec 2018 08:50:49 +0530 From: Balakrishna Godavarthi To: Matthias Kaehlcke Cc: marcel@holtmann.org, johan.hedberg@gmail.com, johan@kernel.org, linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org, hemantg@codeaurora.org, linux-arm-msm@vger.kernel.org Subject: Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command In-Reply-To: <20181226202501.GH261387@google.com> References: <20181220144639.15928-1-bgodavar@codeaurora.org> <20181220144639.15928-3-bgodavar@codeaurora.org> <20181222003116.GE261387@google.com> <20181226202501.GH261387@google.com> Message-ID: <33b964734ec255f75482475e7cb064c2@codeaurora.org> X-Sender: bgodavar@codeaurora.org User-Agent: Roundcube Webmail/1.2.5 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> > > Tested-by: Matthias Kaehlcke >> > > Reviewed-by: Matthias Kaehlcke >> > > --- >> > > 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.