Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command

On Mon, Jan 21, 2019 at 08:11:39PM +0530, Balakrishna Godavarthi wrote:

Hi Matthias,

On 2019-01-19 06:01, Matthias Kaehlcke wrote:

quoted

On Fri, Jan 18, 2019 at 10:44:16AM +0100, Johan Hovold wrote:

quoted

On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:

quoted

I observed that the qcom_geni_serial driver doesn't raise RTS with
flow control disabled. It seems we have to investigate why that's the
case. I agree that the driver should be platform agnostic.

Sounds like a driver bug, unless the hardware is just "odd". The
driver implementation of this looks very non-standard judging from a
quick peek.

In fact, qcom_geni_serial_get_mctrl() is currently a no-op if hardware
flow control is not enabled:

	if (uart_console(uport) || !uart_cts_enabled(uport))
		return;

Perhaps dropping the !uart_cts_enabled() test is sufficient.

Thanks for taking a look Johan, that was indeed the problem (also
in set_mctrl()). I posted a fix:
https://lore.kernel.org/patchwork/patch/1033611/

Balakrishna, the following (applied on top of your patch) works for me
with the UART patch above:

[Bala]: will test and update BT patch accordingly.

Note that Johan pointed out that hci_uart_set_flow_control() deasserts
RTS when flow control is disabled, so the _set_rts() calls can be
removed. With that only a single action needs to be undone in case of
an error, you can consider to keep the return instead of using the
goto introduced by my patch.

Please keep/adapt the "Deassert RTS while changing the baudrate ..."
comment to leave it clear to posterity why flow control is disabled
during baudrate changes. It's fairly obvious once you understand the
problem and that disabling flow control deasserts RTS, but it took us
a while to get there, initially we only had a comment saying
"disabling flow control is mandatory" (I recall I inquired about this
multiple times during the initial review of the wcn3990 patches ;-)

Thanks

Matthias