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

[Balakrishna Godavarthi]

Hi Matthias,

Sorry for the late reply i was on vacation.

On 2019-03-08 05:00, Matthias Kaehlcke wrote:
> 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
>             ~~ ~~
[Bala]: can you share me the command sent and event recevied.
 I see that we receive a command complete event for the baud rate change 
command.

command sent: 01 48 fc 01 11
vendor specific event: 04 ff 02 92 01
command complete event: 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.
[Bala]: fw download approach is different where we use __hci_cmd_sync() 
where as here we use hci_uart_tx_wakeup()
        which directly calls the hci_uart_write_work(). so even we send 
an valid opcode or not for baudrate change will bot matter.

> 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)?

[Bala]: will take this forward to the SoC teams.

--
Regards
Balakrishna.