Re: [PATCH 2/2] Bluetooth: hci_qca: wcn3990: Drop baudrate change vendor event

[Matthias Kaehlcke]

On Thu, Mar 07, 2019 at 10:20:09AM -0800, Matthias Kaehlcke wrote:
> Hi Balakrishna,
> 
> On Thu, Mar 07, 2019 at 10:35:08AM +0530, Balakrishna Godavarthi wrote:
> > hi Matthias,
> > 
> > On 2019-03-07 06:10, Matthias Kaehlcke wrote:
> > > Firmware download to the WCN3990 often fails with a 'TLV response size
> > > mismatch' error:
> > > 
> > > [  133.064659] Bluetooth: hci0: setting up wcn3990
> > > [  133.489150] Bluetooth: hci0: QCA controller version 0x02140201
> > > [  133.495245] Bluetooth: hci0: QCA Downloading qca/crbtfw21.tlv
> > > [  133.507214] Bluetooth: hci0: QCA TLV response size mismatch
> > > [  133.513265] Bluetooth: hci0: QCA Failed to download patch (-84)
> > > 
> > > This is caused by a vendor event that corresponds to an earlier command
> > > to change the baudrate. The event is not processed in the context of the
> > > baudrate change and later interpreted as response to the firmware
> > > download command (which is also a vendor command), but the driver
> > > detects
> > > that the event doesn't have the expected amount of associated data.
> > > 
> > > More details:
> > > 
> > > For the WCN3990 the vendor command for a baudrate change isn't sent as
> > > synchronous HCI command, because the controller sends the corresponding
> > > vendor event with the new baudrate. The event is received and decoded
> > > after the baudrate change of the host port.
> > > 
> > > Identify the 'unused' event when it is received and don't add it to
> > > the queue of RX frames.
> > > 
> > > Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> > > ---
> >
> > ...
> >
> > Can you test by reverting this change "94d6671473924".
> 
> The issue is still reproducible.
> 
> > We need at least 15ms minimum delay for the soc to change its baud rate and
> > respond to the with command complete event.
> 
> The baudrate change has clearly been successful when the problem is
> observed, since the host receives the vendor event with the new
> baudrate.

I forgot to mention this earlier: the controller doesn't send a
command complete event for the command, or at least not a correct
one.

That's the data that is received:

04 0e 04 01 00 00 00
            ~~ ~~

This is *a* command complete event, but the opcode is 0x0000 instead
of the earlier command. The same happens for the firmware
download/read version command, which is the reason why the command
complete injection mess
(https://lore.kernel.org/patchwork/patch/1027955/) is needed in one
way or another.

I wished Qualcomm FW developers would get their act together and:

- send actual command complete events
- acknowledge a baudrate change request using the current baudrate
  like Broadcom and Intel chips apparently do

this would have saved countless hours of debugging and implementing
quirky workarounds ...

Maybe there is hope for future chips (hint, hint)?