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.
