Re: [PATCH] Bluetooth: hci_qca: Add delay for wcn3990 stability

[Jeffrey Hugo]

On Fri, Oct 18, 2019 at 1:40 PM Matthias Kaehlcke <mka@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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.