From: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: marcel@holtmann.org, johan.hedberg@gmail.com,
c-hbandi@codeaurora.org, bgodavar@codeaurora.org,
linux-bluetooth@vger.kernel.org,
MSM <linux-arm-msm@vger.kernel.org>,
lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] Bluetooth: hci_qca: Add delay for wcn3990 stability
Date: Fri, 18 Oct 2019 13:51:39 -0600 [thread overview]
Message-ID: <CAOCk7NqacfVLzKueTRTFQ6aWbLXFyMQaQNXeXENzLTyMNLSp9w@mail.gmail.com> (raw)
In-Reply-To: <20191018194039.GB20212@google.com>
On Fri, Oct 18, 2019 at 1:40 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Fri, Oct 18, 2019 at 12:30:09PM -0600, Jeffrey Hugo wrote:
> > On Fri, Oct 18, 2019 at 12:03 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > >
> > > On Thu, Oct 17, 2019 at 02:29:55PM -0700, Jeffrey Hugo wrote:
> > > > On the msm8998 mtp, the response to the baudrate change command is never
> > > > received. On the Lenovo Miix 630, the response to the baudrate change
> > > > command is corrupted - "Frame reassembly failed (-84)".
> > > >
> > > > Adding a 50ms delay before re-enabling flow to receive the baudrate change
> > > > command response from the wcn3990 addesses both issues, and allows
> > > > bluetooth to become functional.
> > >
> > > From my earlier debugging on sdm845 I don't think this is what happens.
> > > The problem is that the wcn3990 sends the response to the baudrate change
> > > command using the new baudrate, while the UART on the SoC still operates
> > > with the prior speed (for details see 2faa3f15fa2f ("Bluetooth: hci_qca:
> > > wcn3990: Drop baudrate change vendor event"))
> > >
> > > IIRC the 50ms delay causes the HCI core to discard the received data,
> > > which is why the "Frame reassembly failed" message disappears, not
> > > because the response was received. In theory commit 78e8fa2972e5
> > > ("Bluetooth: hci_qca: Deassert RTS while baudrate change command")
> > > should have fixed those messages, do you know if CTS/RTS are connected
> > > on the Bluetooth UART of the Lenovo Miix 630?
> >
> > I was testing with 5.4-rc1 which contains the indicated RTS fix.
> >
> > Yes, CTS/RTS are connected on the Lenovo Miix 630.
> >
> > I added debug statements which indicated that data was received,
> > however it was corrupt, and the packet type did not match what was
> > expected, hence the frame reassembly errors.
>
> Do you know if any data is received during the delay? In theory that
> shouldn't be the case since RTS is deasserted, just double-checking.
I don't think so, but I've run so many tests, I'm not 100% positive.
Let me go double check and get back to you.
>
> What happens if you add a longer delay (e.g. 1s) before/after setting
> the host baudrate?
Hmm, not exactly sure. I will test.
>
> > In response to this patch, Balakrishna pointed me to a bug report
> > which indicated that some of the UART GPIO lines need to have a bias
> > applied to prevent errant data from floating lines -
> >
> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1391888
>
> Yeah, that was another source of frame reassembly errors that we were
> seeing on SDM845.
>
> Balakrishna, please post these kind of replies on-list, so that
> everybody can benefit from possible solutions or contribute to the
> discussion.
>
> > It turns out this fix was never applied to msm8998. Applying the fix
> > does cause the the frame reassembly errors to go away, however then
> > the host SoC never receives the baud rate change response (I increased
> > the timeout from 2faa3f15fa2f ("Bluetooth: hci_qca: wcn3990: Drop
> > baudrate change vendor event") to 5 seconds). As of now, this patch
> > is still required.
>
> Interesting.
>
> FTR, this is the full UART pin configuration for cheza (SDM845):
>
> &qup_uart6_default {
> /* Change pinmux to all 4 pins since CTS and RTS are connected */
> pinmux {
> pins = "gpio45", "gpio46",
> "gpio47", "gpio48";
> };
>
> pinconf-cts {
> /*
> * Configure a pull-down on 45 (CTS) to match the pull of
> * the Bluetooth module.
> */
> pins = "gpio45";
> bias-pull-down;
> };
>
> pinconf-rts-tx {
> /* We'll drive 46 (RTS) and 47 (TX), so no pull */
> pins = "gpio46", "gpio47";
> drive-strength = <2>;
> bias-disable;
> };
>
> pinconf-rx {
> /*
> * Configure a pull-up on 48 (RX). This is needed to avoid
> * garbage data when the TX pin of the Bluetooth module is
> * in tri-state (module powered off or not driving the
> * signal yet).
> */
> pins = "gpio48";
> bias-pull-up;
> };
> };
>
> Does this correspond to what you tried on the Lenovo Miix 630?
Which GPIO maps to which pin is different -
45 - TX
46 - RX
47 - CTS
48 - RFR (RTS)
However, accounting for that, yes that corresponds to what I used.
>
> > I have no idea why the delay is required, and was hoping that posting
> > this patch would result in someone else providing some missing pieces
> > to determine the real root cause. I suspect that asserting RTS at the
> > wrong time may cause an issue for the wcn3990, but I have no data nor
> > documentation to support this guess. I welcome any further insights
> > you may have.
>
> Unfortunately I don't have a clear suggestion at this point, debugging
> the original problem which lead to 2faa3f15fa2f ("Bluetooth: hci_qca:
> wcn3990: Drop baudrate change vendor event") involved quite some time
> and hooking up a scope/logic analyzer ...
>
> I also suspect RTS is involved, and potentially the configuration of
> the pulls. It might be interesting to analyze the data that leads to
> the frame assembly error and determine if it is just noise (wrong
> pulls/drive strength?) or received with a non-matching baud-rate.
I don't have a scope/logic analyzer, but since I hooked up the
blsp_bam I'm able to see the raw data from the uart before it gets to
the HCI stack or anything. As a side note, having the bam or not
seemed to have no effect on the issue. Most of the time the data was
one byte (zero), some times it was a string of zero bytes. Rarely it
would be random data.
>
> The 50ms delay doesn't really cause any harm, but ideally we'd
> understand what exactly is going on.
Agreed.
next prev parent reply other threads:[~2019-10-18 19:51 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-17 21:29 [PATCH] Bluetooth: hci_qca: Add delay for wcn3990 stability Jeffrey Hugo
2019-10-18 7:57 ` Marcel Holtmann
2019-10-18 18:03 ` Matthias Kaehlcke
2019-10-18 18:30 ` Jeffrey Hugo
2019-10-18 19:40 ` Matthias Kaehlcke
2019-10-18 19:51 ` Jeffrey Hugo [this message]
2019-10-18 21:33 ` Matthias Kaehlcke
2019-10-18 22:36 ` Jeffrey Hugo
2019-10-18 23:15 ` Matthias Kaehlcke
2019-10-19 20:31 ` Jeffrey Hugo
2019-10-19 21:17 ` Marcel Holtmann
2019-10-21 19:18 ` Matthias Kaehlcke
2019-10-19 19:19 ` Marcel Holtmann
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=CAOCk7NqacfVLzKueTRTFQ6aWbLXFyMQaQNXeXENzLTyMNLSp9w@mail.gmail.com \
--to=jeffrey.l.hugo@gmail.com \
--cc=bgodavar@codeaurora.org \
--cc=c-hbandi@codeaurora.org \
--cc=johan.hedberg@gmail.com \
--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).