From: Balakrishna Godavarthi <bgodavar@codeaurora.org>
To: Matthias Kaehlcke <mka@chromium.org>
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
Date: Thu, 27 Dec 2018 08:50:49 +0530 [thread overview]
Message-ID: <33b964734ec255f75482475e7cb064c2@codeaurora.org> (raw)
In-Reply-To: <20181226202501.GH261387@google.com>
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.
next prev parent reply other threads:[~2018-12-27 3:21 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-20 14:46 [PATCH v5 0/5] Bug fixes for Qualcomm BT chip wcn3990 Balakrishna Godavarthi
2018-12-20 14:46 ` [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses Balakrishna Godavarthi
2018-12-22 1:59 ` Matthias Kaehlcke
2018-12-26 6:31 ` Balakrishna Godavarthi
2018-12-26 22:21 ` Matthias Kaehlcke
2018-12-27 3:23 ` Balakrishna Godavarthi
2019-01-09 14:38 ` Johan Hovold
2019-01-10 14:48 ` Balakrishna Godavarthi
2019-01-11 0:55 ` Matthias Kaehlcke
2019-01-11 14:32 ` Balakrishna Godavarthi
2019-01-11 23:38 ` Matthias Kaehlcke
2019-01-14 10:25 ` Balakrishna Godavarthi
2018-12-20 14:46 ` [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command Balakrishna Godavarthi
2018-12-22 0:31 ` Matthias Kaehlcke
2018-12-26 5:45 ` Balakrishna Godavarthi
2018-12-26 20:25 ` Matthias Kaehlcke
2018-12-27 3:20 ` Balakrishna Godavarthi [this message]
2019-01-09 14:52 ` Johan Hovold
2019-01-10 14:34 ` Balakrishna Godavarthi
2019-01-10 14:39 ` Johan Hovold
2019-01-10 14:52 ` Balakrishna Godavarthi
2019-01-11 1:37 ` Matthias Kaehlcke
2019-01-11 15:07 ` Balakrishna Godavarthi
2019-01-11 23:56 ` Matthias Kaehlcke
2019-01-14 14:52 ` Balakrishna Godavarthi
2019-01-15 23:46 ` Matthias Kaehlcke
2019-01-17 16:09 ` Johan Hovold
2019-01-17 17:21 ` Matthias Kaehlcke
2019-01-18 9:44 ` Johan Hovold
2019-01-19 0:31 ` Matthias Kaehlcke
2019-01-21 8:56 ` Johan Hovold
2019-01-22 21:39 ` Matthias Kaehlcke
2019-01-21 14:41 ` Balakrishna Godavarthi
2019-01-22 21:53 ` Matthias Kaehlcke
2019-01-23 12:57 ` Balakrishna Godavarthi
2018-12-20 14:46 ` [PATCH v5 3/5] Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990 Balakrishna Godavarthi
2018-12-20 14:46 ` [PATCH v5 4/5] Bluetooth: hci_qca: Disable IBS state machine and flush Tx buffer Balakrishna Godavarthi
2018-12-20 14:46 ` [PATCH v5 5/5] Bluetooth: btqca: inject command complete event during fw download Balakrishna Godavarthi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=33b964734ec255f75482475e7cb064c2@codeaurora.org \
--to=bgodavar@codeaurora.org \
--cc=hemantg@codeaurora.org \
--cc=johan.hedberg@gmail.com \
--cc=johan@kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marcel@holtmann.org \
--cc=mka@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).