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

2019-01-23 Thread Balakrishna Godavarthi 

Hi Matthias,

On 2019-01-23 03:23, Matthias Kaehlcke wrote:

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

Hi Matthias,

On 2019-01-19 06:01, Matthias Kaehlcke wrote:
> On Fri, Jan 18, 2019 at 10:44:16AM +0100, Johan Hovold wrote:
> > On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:
> >
> > > 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.



[Bala]: ok sure. will note this.


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


--
Regards
Balakrishna.

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

2019-01-22 Thread Matthias Kaehlcke 
On Mon, Jan 21, 2019 at 08:11:39PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2019-01-19 06:01, Matthias Kaehlcke wrote:
> > On Fri, Jan 18, 2019 at 10:44:16AM +0100, Johan Hovold wrote:
> > > On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:
> > > 
> > > > 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

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

2019-01-22 Thread Matthias Kaehlcke 
On Mon, Jan 21, 2019 at 09:56:13AM +0100, Johan Hovold wrote:
> On Fri, Jan 18, 2019 at 04:31:09PM -0800, Matthias Kaehlcke wrote:
> > On Fri, Jan 18, 2019 at 10:44:16AM +0100, Johan Hovold wrote:
> > > On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:
> > > 
> > > > 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/
> 
> Nice (I did mean set_mctrl() above, as I think you noticed).
> 
> > Balakrishna, the following (applied on top of your patch) works for me
> > with the UART patch above:
> > 
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index 9d5e41f159c78f..60bfdf01f72841 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -1080,7 +1080,7 @@ static int qca_set_speed(struct hci_uart *hu, enum 
> > qca_speed_type speed_type)
> >  {
> > unsigned int speed, qca_baudrate;
> > struct qca_serdev *qcadev;
> > -   int ret;
> > +   int ret = 0;
> >  
> > if (speed_type == QCA_INIT_SPEED) {
> > speed = qca_get_speed(hu, QCA_INIT_SPEED);
> > @@ -1097,22 +1097,27 @@ static int qca_set_speed(struct hci_uart *hu, enum 
> > qca_speed_type speed_type)
> >  * the old speed.
> >  */
> > qcadev = serdev_device_get_drvdata(hu->serdev);
> > -   if (qcadev->btsoc_type == QCA_WCN3990)
> > +   if (qcadev->btsoc_type == QCA_WCN3990) {
> > +   hci_uart_set_flow_control(hu, true);
> 
> Wow, yeah, this parameter inversion is indeed confusing...
> 
> > serdev_device_set_rts(hu->serdev, false);
> > +   }
> 
> But looking at hci_uart_set_flow_control() now, it actually also
> deasserts RTS. So all you need here is the hci_uart_set_flow_control()
> call.  

Great, thanks for pointing that out!

> And that makes the inversion make a bit more sense too, even if the
> naming is a bit unfortunate with respect to
> serdev_device_set_flow_control() at least.

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

2019-01-21 Thread Balakrishna Godavarthi 

Hi Matthias,

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

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

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

> 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.



diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 9d5e41f159c78f..60bfdf01f72841 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1080,7 +1080,7 @@ static int qca_set_speed(struct hci_uart *hu,
enum qca_speed_type speed_type)
 {
unsigned int speed, qca_baudrate;
struct qca_serdev *qcadev;
-   int ret;
+   int ret = 0;

if (speed_type == QCA_INIT_SPEED) {
speed = qca_get_speed(hu, QCA_INIT_SPEED);
@@ -1097,22 +1097,27 @@ static int qca_set_speed(struct hci_uart *hu,
enum qca_speed_type speed_type)
 * the old speed.
 */
qcadev = serdev_device_get_drvdata(hu->serdev);
-   if (qcadev->btsoc_type == QCA_WCN3990)
+   if (qcadev->btsoc_type == QCA_WCN3990) {
+   hci_uart_set_flow_control(hu, true);
serdev_device_set_rts(hu->serdev, false);
+   }

qca_baudrate = qca_get_baudrate_value(speed);
bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
ret = qca_set_baudrate(hu->hdev, qca_baudrate);
if (ret)
-   return ret;
+   goto out;

host_set_baudrate(hu, speed);

-   if (qcadev->btsoc_type == QCA_WCN3990)
+out:
+   if (qcadev->btsoc_type == QCA_WCN3990) {
+   hci_uart_set_flow_control(hu, false);
serdev_device_set_rts(hu->serdev, true);
+   }
}

-   return 0;
+   return ret;
 }

 static int qca_wcn3990_init(struct hci_uart *hu)


--
Regards
Balakrishna.

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

2019-01-21 Thread Johan Hovold 
On Fri, Jan 18, 2019 at 04:31:09PM -0800, Matthias Kaehlcke wrote:
> On Fri, Jan 18, 2019 at 10:44:16AM +0100, Johan Hovold wrote:
> > On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:
> > 
> > > 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/

Nice (I did mean set_mctrl() above, as I think you noticed).

> Balakrishna, the following (applied on top of your patch) works for me
> with the UART patch above:
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 9d5e41f159c78f..60bfdf01f72841 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1080,7 +1080,7 @@ static int qca_set_speed(struct hci_uart *hu, enum 
> qca_speed_type speed_type)
>  {
>   unsigned int speed, qca_baudrate;
>   struct qca_serdev *qcadev;
> - int ret;
> + int ret = 0;
>  
>   if (speed_type == QCA_INIT_SPEED) {
>   speed = qca_get_speed(hu, QCA_INIT_SPEED);
> @@ -1097,22 +1097,27 @@ static int qca_set_speed(struct hci_uart *hu, enum 
> qca_speed_type speed_type)
>* the old speed.
>*/
>   qcadev = serdev_device_get_drvdata(hu->serdev);
> - if (qcadev->btsoc_type == QCA_WCN3990)
> + if (qcadev->btsoc_type == QCA_WCN3990) {
> + hci_uart_set_flow_control(hu, true);

Wow, yeah, this parameter inversion is indeed confusing...

>   serdev_device_set_rts(hu->serdev, false);
> + }

But looking at hci_uart_set_flow_control() now, it actually also
deasserts RTS. So all you need here is the hci_uart_set_flow_control()
call.  

And that makes the inversion make a bit more sense too, even if the
naming is a bit unfortunate with respect to
serdev_device_set_flow_control() at least.

>   qca_baudrate = qca_get_baudrate_value(speed);
>   bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
>   ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>   if (ret)
> - return ret;
> + goto out;
>  
>   host_set_baudrate(hu, speed);
>  
> - if (qcadev->btsoc_type == QCA_WCN3990)
> +out:
> + if (qcadev->btsoc_type == QCA_WCN3990) {
> + hci_uart_set_flow_control(hu, false);
>   serdev_device_set_rts(hu->serdev, true);
> + }

And same here.

Johan

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

2019-01-18 Thread Matthias Kaehlcke 
On Fri, Jan 18, 2019 at 10:44:16AM +0100, Johan Hovold wrote:
> On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:
> 
> > 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:

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 9d5e41f159c78f..60bfdf01f72841 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1080,7 +1080,7 @@ static int qca_set_speed(struct hci_uart *hu, enum 
qca_speed_type speed_type)
 {
unsigned int speed, qca_baudrate;
struct qca_serdev *qcadev;
-   int ret;
+   int ret = 0;
 
if (speed_type == QCA_INIT_SPEED) {
speed = qca_get_speed(hu, QCA_INIT_SPEED);
@@ -1097,22 +1097,27 @@ static int qca_set_speed(struct hci_uart *hu, enum 
qca_speed_type speed_type)
 * the old speed.
 */
qcadev = serdev_device_get_drvdata(hu->serdev);
-   if (qcadev->btsoc_type == QCA_WCN3990)
+   if (qcadev->btsoc_type == QCA_WCN3990) {
+   hci_uart_set_flow_control(hu, true);
serdev_device_set_rts(hu->serdev, false);
+   }
 
qca_baudrate = qca_get_baudrate_value(speed);
bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
ret = qca_set_baudrate(hu->hdev, qca_baudrate);
if (ret)
-   return ret;
+   goto out;
 
host_set_baudrate(hu, speed);
 
-   if (qcadev->btsoc_type == QCA_WCN3990)
+out:
+   if (qcadev->btsoc_type == QCA_WCN3990) {
+   hci_uart_set_flow_control(hu, false);
serdev_device_set_rts(hu->serdev, true);
+   }
}
 
-   return 0;
+   return ret;
 }
 
 static int qca_wcn3990_init(struct hci_uart *hu)

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

2019-01-18 Thread Johan Hovold 
On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:

> 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.

Johan

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

2019-01-17 Thread Matthias Kaehlcke 
On Thu, Jan 17, 2019 at 05:09:44PM +0100, Johan Hovold wrote:
> On Tue, Jan 15, 2019 at 03:46:28PM -0800, Matthias Kaehlcke wrote:
> 
> > Using RTS seems|ed like a nice solutions, since it's the native way to
> > prevent the controller from sending data, instead of doing some custom
> > hack. However Johan seems to be fairly convinced that flow control and
> > manual toggling of RTS can be problematic, and we have seen that
> > disabling flow control has its own problems. Maybe it's time to
> > consider other solutions like the DISCARD_RX flag we discussed
> > earlier. Not that I really liked this custom solution, but in the end
> > it might be a more robust way to address the issue.
> > 
> > Johan/Marcel/others: Do you have any further ideas or input on this?
> 
> I don't see why deasserting RTS wouldn't work, well at least as long as
> the RTS signal is wired correctly.
> 
> My point was simply that calling serdev_device_set_rts() will generally
> not work unless you first disable automatic (i.e. hw-managed) flow
> control using serdev_device_set_flow_control(). The exact behaviour is
> serial-controller dependent, but I assume the driver needs to be
> platform agnostic.

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.

Cheers

Matthias

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

2019-01-17 Thread Johan Hovold 
On Tue, Jan 15, 2019 at 03:46:28PM -0800, Matthias Kaehlcke wrote:

> Using RTS seems|ed like a nice solutions, since it's the native way to
> prevent the controller from sending data, instead of doing some custom
> hack. However Johan seems to be fairly convinced that flow control and
> manual toggling of RTS can be problematic, and we have seen that
> disabling flow control has its own problems. Maybe it's time to
> consider other solutions like the DISCARD_RX flag we discussed
> earlier. Not that I really liked this custom solution, but in the end
> it might be a more robust way to address the issue.
> 
> Johan/Marcel/others: Do you have any further ideas or input on this?

I don't see why deasserting RTS wouldn't work, well at least as long as
the RTS signal is wired correctly.

My point was simply that calling serdev_device_set_rts() will generally
not work unless you first disable automatic (i.e. hw-managed) flow
control using serdev_device_set_flow_control(). The exact behaviour is
serial-controller dependent, but I assume the driver needs to be
platform agnostic.

Johan

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

2019-01-15 Thread Matthias Kaehlcke 
On Mon, Jan 14, 2019 at 08:22:12PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2019-01-12 05:26, Matthias Kaehlcke wrote:
> > On Fri, Jan 11, 2019 at 08:37:12PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > On 2019-01-11 07:07, Matthias Kaehlcke wrote:
> > > > On Thu, Jan 10, 2019 at 08:22:12PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Johan,
> > > > >
> > > > > On 2019-01-10 20:09, Johan Hovold wrote:
> > > > > > On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi 
> > > > > > wrote:
> > > > > > > Hi Johan,
> > > > > > >
> > > > > > > On 2019-01-09 20:22, Johan Hovold wrote:
> > > > > > > > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna 
> > > > > > > > Godavarthi wrote:
> > > > > > > >> This patch will help to stop frame reassembly errors while 
> > > > > > > >> changing
> > > > > > > >> the baudrate. This is because host send a change baudrate 
> > > > > > > >> request
> > > > > > > >> command to the chip with 115200 bps, Whereas chip will change 
> > > > > > > >> their
> > > > > > > >> UART clocks to the enable for new baudrate and sends the 
> > > > > > > >> response
> > > > > > > >> for the change request command with newer baudrate, On host 
> > > > > > > >> side
> > > > > > > >> we are still operating in 115200 bps which results of reading 
> > > > > > > >> garbage
> > > > > > > >> data. Here we are pulling RTS line, so that chip we will wait 
> > > > > > > >> to send
> > > > > > > >> data
> > > > > > > >> to host until host change its baudrate.
> > > > > >
> > > > > > > >> +  /* Deassert RTS while changing the baudrate of 
> > > > > > > >> chip and host.
> > > > > > > >> +   * This will prevent chip from transmitting its 
> > > > > > > >> response with
> > > > > > > >> +   * the new baudrate while the host port is 
> > > > > > > >> still operating at
> > > > > > > >> +   * the old speed.
> > > > > > > >> +   */
> > > > > > > >> +  qcadev = serdev_device_get_drvdata(hu->serdev);
> > > > > > > >> +  if (qcadev->btsoc_type == QCA_WCN3990)
> > > > > > > >> +  serdev_device_set_rts(hu->serdev, 
> > > > > > > >> false);
> > > > > > > >> +
> > > > > > > >
> > > > > > > > This may not do what you want unless you also disable hardware 
> > > > > > > > flow
> > > > > > > > control.
> > > > > >
> > > > > > > Here my requirement here is to block the chip to send its data 
> > > > > > > before
> > > > > > > HOST changes it is baudrate. So if i disable flow control lines of
> > > > > > > HOST which will be in low state.  so that the chip will send it 
> > > > > > > data
> > > > > > > before HOST change the baudrate of HOST. which results in frame
> > > > > > > reassembly error.
> > > > > >
> > > > > > Not sure I understand what you're trying to say above. My point is 
> > > > > > that
> > > > > > you cannot reliable control RTS when you have automatic flow control
> > > > > > enabled (i.e. it is managed by hardware and it's state reflects 
> > > > > > whether
> > > > > > there's room in the UART receive FIFO).
> > > > > >
> > > > > > Johan
> > > > >
> > > > > [Bala]: Yes i got your point, but our driver
> > > >
> > > > I suppose with "our driver" you refer to a Qualcomm UART driver like
> > > > qcom_geni_serial.c. Unless the Bluetooth controller is really tied to
> > > > some specific SoC (e.g. because it is on-chip) you shouldn't make
> > > > assumptions about the UART driver or hardware beyond standard
> > > > behavior.
> > > >
> > > > But even if we assume that the driver you mention is used, I think you
> > > > are rather confirming Johan's concern than dispersing it:
> > > >
> > > 
> > > [Bala]: now understood the point.
> > > 
> > > > > will not support automatic flow control (based on the FIFO status)
> > > > > unless we explicitly enabled via software. i.e. if we enable the
> > > > > flow, hardware will look for it else it will not looks for CTS or
> > > > > RTS Line.
> > > >
> > > > So we agree that the UART hardware may change RTS if hardware flow
> > > > control is enabled?
> > > >
> > > > static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
> > > > {
> > > >   ...
> > > >   hci_uart_set_flow_control(hu, false);
> > > >   ...
> > > > }
> > > >
> > > > I still find it utterly confusing that set_flow_control(false) enables
> > > > flow control, but that's what it does, hence after
> > > > qca_send_power_pulse() flow control is (re-)enabled.
> > > >
> > > > So far I haven't seen problems with qcom_geni_serial.c overriding the
> > > > level set with serdev_device_set_rts(), but I tend to agree with Johan
> > > > that this could be a problem (if not with this UART (driver) then with
> > > > another). I'm not keen about adding more flow control on/off clutter,
> > > > but if that is needed for the driver to operate reliably across
> > > > platforms so be it.
> > > >
> > > > Cheers
> > > >
> > > > Matthias
> > > 
> > > [Bala]:

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

2019-01-14 Thread Balakrishna Godavarthi 

Hi Matthias,

On 2019-01-12 05:26, Matthias Kaehlcke wrote:

On Fri, Jan 11, 2019 at 08:37:12PM +0530, Balakrishna Godavarthi wrote:

Hi Matthias,

On 2019-01-11 07:07, Matthias Kaehlcke wrote:
> On Thu, Jan 10, 2019 at 08:22:12PM +0530, Balakrishna Godavarthi wrote:
> > Hi Johan,
> >
> > On 2019-01-10 20:09, Johan Hovold wrote:
> > > On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:
> > > > Hi Johan,
> > > >
> > > > On 2019-01-09 20:22, Johan Hovold wrote:
> > > > > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi 
wrote:
> > > > >> This patch will help to stop frame reassembly errors while changing
> > > > >> the baudrate. This is because host send a change baudrate request
> > > > >> command to the chip with 115200 bps, Whereas chip will change their
> > > > >> UART clocks to the enable for new baudrate and sends the response
> > > > >> for the change request command with newer baudrate, On host side
> > > > >> we are still operating in 115200 bps which results of reading garbage
> > > > >> data. Here we are pulling RTS line, so that chip we will wait to send
> > > > >> data
> > > > >> to host until host change its baudrate.
> > >
> > > > >> +  /* Deassert RTS while changing the baudrate of chip and 
host.
> > > > >> +   * This will prevent chip from transmitting its response 
with
> > > > >> +   * the new baudrate while the host port is still 
operating at
> > > > >> +   * the old speed.
> > > > >> +   */
> > > > >> +  qcadev = serdev_device_get_drvdata(hu->serdev);
> > > > >> +  if (qcadev->btsoc_type == QCA_WCN3990)
> > > > >> +  serdev_device_set_rts(hu->serdev, false);
> > > > >> +
> > > > >
> > > > > This may not do what you want unless you also disable hardware flow
> > > > > control.
> > >
> > > > Here my requirement here is to block the chip to send its data before
> > > > HOST changes it is baudrate. So if i disable flow control lines of
> > > > HOST which will be in low state.  so that the chip will send it data
> > > > before HOST change the baudrate of HOST. which results in frame
> > > > reassembly error.
> > >
> > > Not sure I understand what you're trying to say above. My point is that
> > > you cannot reliable control RTS when you have automatic flow control
> > > enabled (i.e. it is managed by hardware and it's state reflects whether
> > > there's room in the UART receive FIFO).
> > >
> > > Johan
> >
> > [Bala]: Yes i got your point, but our driver
>
> I suppose with "our driver" you refer to a Qualcomm UART driver like
> qcom_geni_serial.c. Unless the Bluetooth controller is really tied to
> some specific SoC (e.g. because it is on-chip) you shouldn't make
> assumptions about the UART driver or hardware beyond standard
> behavior.
>
> But even if we assume that the driver you mention is used, I think you
> are rather confirming Johan's concern than dispersing it:
>

[Bala]: now understood the point.

> > will not support automatic flow control (based on the FIFO status)
> > unless we explicitly enabled via software. i.e. if we enable the
> > flow, hardware will look for it else it will not looks for CTS or
> > RTS Line.
>
> So we agree that the UART hardware may change RTS if hardware flow
> control is enabled?
>
> static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
> {
>   ...
>   hci_uart_set_flow_control(hu, false);
>   ...
> }
>
> I still find it utterly confusing that set_flow_control(false) enables
> flow control, but that's what it does, hence after
> qca_send_power_pulse() flow control is (re-)enabled.
>
> So far I haven't seen problems with qcom_geni_serial.c overriding the
> level set with serdev_device_set_rts(), but I tend to agree with Johan
> that this could be a problem (if not with this UART (driver) then with
> another). I'm not keen about adding more flow control on/off clutter,
> but if that is needed for the driver to operate reliably across
> platforms so be it.
>
> Cheers
>
> Matthias

[Bala]: previously we have disabling the flow control, that is not 
pulling

the RTS line if it disabled.
so that the reason we are explicilty pulling it by calling 
set_rts()

with false.

Johan concern can be fixed either of two ways.

1. disable the flow control, but the uart driver should pull 
the RTS

line high. as the line is unused
2. disable the flow control and call set_rts with false that 
will

helps us to pull the RTS line.


I don't think you can rely on 1. You might succeed to convince a
specific UART driver/hardware to do this, however you'd have to ensure
the same behavior on all other types of UARTs that could be used in
combination with the chip, which doesn't seem feasible.
In case the hardware completely relinquishes control of the RTS pin
upon disabling flow control the state of the signal could depend on
the pin configuration, i.e. whether Linux (or the bootloader) enables
a pull-up/down, which may vary

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

2019-01-11 Thread Matthias Kaehlcke 
On Fri, Jan 11, 2019 at 08:37:12PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2019-01-11 07:07, Matthias Kaehlcke wrote:
> > On Thu, Jan 10, 2019 at 08:22:12PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Johan,
> > > 
> > > On 2019-01-10 20:09, Johan Hovold wrote:
> > > > On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Johan,
> > > > >
> > > > > On 2019-01-09 20:22, Johan Hovold wrote:
> > > > > > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi 
> > > > > > wrote:
> > > > > >> This patch will help to stop frame reassembly errors while changing
> > > > > >> the baudrate. This is because host send a change baudrate request
> > > > > >> command to the chip with 115200 bps, Whereas chip will change their
> > > > > >> UART clocks to the enable for new baudrate and sends the response
> > > > > >> for the change request command with newer baudrate, On host side
> > > > > >> we are still operating in 115200 bps which results of reading 
> > > > > >> garbage
> > > > > >> data. Here we are pulling RTS line, so that chip we will wait to 
> > > > > >> send
> > > > > >> data
> > > > > >> to host until host change its baudrate.
> > > >
> > > > > >> +  /* Deassert RTS while changing the baudrate of chip and 
> > > > > >> host.
> > > > > >> +   * This will prevent chip from transmitting its 
> > > > > >> response with
> > > > > >> +   * the new baudrate while the host port is still 
> > > > > >> operating at
> > > > > >> +   * the old speed.
> > > > > >> +   */
> > > > > >> +  qcadev = serdev_device_get_drvdata(hu->serdev);
> > > > > >> +  if (qcadev->btsoc_type == QCA_WCN3990)
> > > > > >> +  serdev_device_set_rts(hu->serdev, false);
> > > > > >> +
> > > > > >
> > > > > > This may not do what you want unless you also disable hardware flow
> > > > > > control.
> > > >
> > > > > Here my requirement here is to block the chip to send its data before
> > > > > HOST changes it is baudrate. So if i disable flow control lines of
> > > > > HOST which will be in low state.  so that the chip will send it data
> > > > > before HOST change the baudrate of HOST. which results in frame
> > > > > reassembly error.
> > > >
> > > > Not sure I understand what you're trying to say above. My point is that
> > > > you cannot reliable control RTS when you have automatic flow control
> > > > enabled (i.e. it is managed by hardware and it's state reflects whether
> > > > there's room in the UART receive FIFO).
> > > >
> > > > Johan
> > > 
> > > [Bala]: Yes i got your point, but our driver
> > 
> > I suppose with "our driver" you refer to a Qualcomm UART driver like
> > qcom_geni_serial.c. Unless the Bluetooth controller is really tied to
> > some specific SoC (e.g. because it is on-chip) you shouldn't make
> > assumptions about the UART driver or hardware beyond standard
> > behavior.
> > 
> > But even if we assume that the driver you mention is used, I think you
> > are rather confirming Johan's concern than dispersing it:
> > 
> 
> [Bala]: now understood the point.
> 
> > > will not support automatic flow control (based on the FIFO status)
> > > unless we explicitly enabled via software. i.e. if we enable the
> > > flow, hardware will look for it else it will not looks for CTS or
> > > RTS Line.
> > 
> > So we agree that the UART hardware may change RTS if hardware flow
> > control is enabled?
> > 
> > static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
> > {
> >   ...
> >   hci_uart_set_flow_control(hu, false);
> >   ...
> > }
> > 
> > I still find it utterly confusing that set_flow_control(false) enables
> > flow control, but that's what it does, hence after
> > qca_send_power_pulse() flow control is (re-)enabled.
> > 
> > So far I haven't seen problems with qcom_geni_serial.c overriding the
> > level set with serdev_device_set_rts(), but I tend to agree with Johan
> > that this could be a problem (if not with this UART (driver) then with
> > another). I'm not keen about adding more flow control on/off clutter,
> > but if that is needed for the driver to operate reliably across
> > platforms so be it.
> > 
> > Cheers
> > 
> > Matthias
> 
> [Bala]: previously we have disabling the flow control, that is not pulling
> the RTS line if it disabled.
> so that the reason we are explicilty pulling it by calling set_rts()
> with false.
> 
> Johan concern can be fixed either of two ways.
> 
> 1. disable the flow control, but the uart driver should pull the RTS
> line high. as the line is unused
> 2. disable the flow control and call set_rts with false that will
> helps us to pull the RTS line.

I don't think you can rely on 1. You might succeed to convince a
specific UART driver/hardware to do this, however you'd have to ensure
the same behavior on all other types of UARTs that could be used in
combination with the chip, which doesn't seem feasible.
In case the

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

2019-01-11 Thread Balakrishna Godavarthi 

Hi Matthias,

On 2019-01-11 07:07, Matthias Kaehlcke wrote:

On Thu, Jan 10, 2019 at 08:22:12PM +0530, Balakrishna Godavarthi wrote:

Hi Johan,

On 2019-01-10 20:09, Johan Hovold wrote:
> On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:
> > Hi Johan,
> >
> > On 2019-01-09 20:22, Johan Hovold wrote:
> > > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> > >> This patch will help to stop frame reassembly errors while changing
> > >> the baudrate. This is because host send a change baudrate request
> > >> command to the chip with 115200 bps, Whereas chip will change their
> > >> UART clocks to the enable for new baudrate and sends the response
> > >> for the change request command with newer baudrate, On host side
> > >> we are still operating in 115200 bps which results of reading garbage
> > >> data. Here we are pulling RTS line, so that chip we will wait to send
> > >> data
> > >> to host until host change its baudrate.
>
> > >> +/* Deassert RTS while changing the baudrate of chip and 
host.
> > >> + * This will prevent chip from transmitting its response 
with
> > >> + * the new baudrate while the host port is still operating 
at
> > >> + * the old speed.
> > >> + */
> > >> +qcadev = serdev_device_get_drvdata(hu->serdev);
> > >> +if (qcadev->btsoc_type == QCA_WCN3990)
> > >> +serdev_device_set_rts(hu->serdev, false);
> > >> +
> > >
> > > This may not do what you want unless you also disable hardware flow
> > > control.
>
> > Here my requirement here is to block the chip to send its data before
> > HOST changes it is baudrate. So if i disable flow control lines of
> > HOST which will be in low state.  so that the chip will send it data
> > before HOST change the baudrate of HOST. which results in frame
> > reassembly error.
>
> Not sure I understand what you're trying to say above. My point is that
> you cannot reliable control RTS when you have automatic flow control
> enabled (i.e. it is managed by hardware and it's state reflects whether
> there's room in the UART receive FIFO).
>
> Johan

[Bala]: Yes i got your point, but our driver


I suppose with "our driver" you refer to a Qualcomm UART driver like
qcom_geni_serial.c. Unless the Bluetooth controller is really tied to
some specific SoC (e.g. because it is on-chip) you shouldn't make
assumptions about the UART driver or hardware beyond standard
behavior.

But even if we assume that the driver you mention is used, I think you
are rather confirming Johan's concern than dispersing it:



[Bala]: now understood the point.


will not support automatic flow control (based on the FIFO status)
unless we explicitly enabled via software. i.e. if we enable the
flow, hardware will look for it else it will not looks for CTS or
RTS Line.


So we agree that the UART hardware may change RTS if hardware flow
control is enabled?

static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
{
  ...
  hci_uart_set_flow_control(hu, false);
  ...
}

I still find it utterly confusing that set_flow_control(false) enables
flow control, but that's what it does, hence after
qca_send_power_pulse() flow control is (re-)enabled.

So far I haven't seen problems with qcom_geni_serial.c overriding the
level set with serdev_device_set_rts(), but I tend to agree with Johan
that this could be a problem (if not with this UART (driver) then with
another). I'm not keen about adding more flow control on/off clutter,
but if that is needed for the driver to operate reliably across
platforms so be it.

Cheers

Matthias


[Bala]: previously we have disabling the flow control, that is not 
pulling the RTS line if it disabled.
so that the reason we are explicilty pulling it by calling 
set_rts() with false.


Johan concern can be fixed either of two ways.

1. disable the flow control, but the uart driver should pull the 
RTS line high. as the line is unused
2. disable the flow control and call set_rts with false that 
will helps us to pull the RTS line.


will experiment more on this and post the change.


--
Regards
Balakrishna.

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

2019-01-10 Thread Matthias Kaehlcke 
On Thu, Jan 10, 2019 at 08:22:12PM +0530, Balakrishna Godavarthi wrote:
> Hi Johan,
> 
> On 2019-01-10 20:09, Johan Hovold wrote:
> > On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Johan,
> > > 
> > > On 2019-01-09 20:22, Johan Hovold wrote:
> > > > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> > > >> This patch will help to stop frame reassembly errors while changing
> > > >> the baudrate. This is because host send a change baudrate request
> > > >> command to the chip with 115200 bps, Whereas chip will change their
> > > >> UART clocks to the enable for new baudrate and sends the response
> > > >> for the change request command with newer baudrate, On host side
> > > >> we are still operating in 115200 bps which results of reading garbage
> > > >> data. Here we are pulling RTS line, so that chip we will wait to send
> > > >> data
> > > >> to host until host change its baudrate.
> > 
> > > >> +  /* Deassert RTS while changing the baudrate of chip and 
> > > >> host.
> > > >> +   * This will prevent chip from transmitting its 
> > > >> response with
> > > >> +   * the new baudrate while the host port is still 
> > > >> operating at
> > > >> +   * the old speed.
> > > >> +   */
> > > >> +  qcadev = serdev_device_get_drvdata(hu->serdev);
> > > >> +  if (qcadev->btsoc_type == QCA_WCN3990)
> > > >> +  serdev_device_set_rts(hu->serdev, false);
> > > >> +
> > > >
> > > > This may not do what you want unless you also disable hardware flow
> > > > control.
> > 
> > > Here my requirement here is to block the chip to send its data before
> > > HOST changes it is baudrate. So if i disable flow control lines of
> > > HOST which will be in low state.  so that the chip will send it data
> > > before HOST change the baudrate of HOST. which results in frame
> > > reassembly error.
> > 
> > Not sure I understand what you're trying to say above. My point is that
> > you cannot reliable control RTS when you have automatic flow control
> > enabled (i.e. it is managed by hardware and it's state reflects whether
> > there's room in the UART receive FIFO).
> > 
> > Johan
> 
> [Bala]: Yes i got your point, but our driver

I suppose with "our driver" you refer to a Qualcomm UART driver like
qcom_geni_serial.c. Unless the Bluetooth controller is really tied to
some specific SoC (e.g. because it is on-chip) you shouldn't make
assumptions about the UART driver or hardware beyond standard
behavior.

But even if we assume that the driver you mention is used, I think you
are rather confirming Johan's concern than dispersing it:

> will not support automatic flow control (based on the FIFO status)
> unless we explicitly enabled via software. i.e. if we enable the
> flow, hardware will look for it else it will not looks for CTS or
> RTS Line.

So we agree that the UART hardware may change RTS if hardware flow
control is enabled?

static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
{
  ...
  hci_uart_set_flow_control(hu, false);
  ...
}

I still find it utterly confusing that set_flow_control(false) enables
flow control, but that's what it does, hence after
qca_send_power_pulse() flow control is (re-)enabled.

So far I haven't seen problems with qcom_geni_serial.c overriding the
level set with serdev_device_set_rts(), but I tend to agree with Johan
that this could be a problem (if not with this UART (driver) then with
another). I'm not keen about adding more flow control on/off clutter,
but if that is needed for the driver to operate reliably across
platforms so be it.

Cheers

Matthias

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

2019-01-10 Thread Balakrishna Godavarthi 

Hi Johan,

On 2019-01-10 20:09, Johan Hovold wrote:

On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:

Hi Johan,

On 2019-01-09 20:22, Johan Hovold wrote:
> On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
>> This patch will help to stop frame reassembly errors while changing
>> the baudrate. This is because host send a change baudrate request
>> command to the chip with 115200 bps, Whereas chip will change their
>> UART clocks to the enable for new baudrate and sends the response
>> for the change request command with newer baudrate, On host side
>> we are still operating in 115200 bps which results of reading garbage
>> data. Here we are pulling RTS line, so that chip we will wait to send
>> data
>> to host until host change its baudrate.



>> +  /* Deassert RTS while changing the baudrate of chip and host.
>> +   * This will prevent chip from transmitting its response with
>> +   * the new baudrate while the host port is still operating at
>> +   * the old speed.
>> +   */
>> +  qcadev = serdev_device_get_drvdata(hu->serdev);
>> +  if (qcadev->btsoc_type == QCA_WCN3990)
>> +  serdev_device_set_rts(hu->serdev, false);
>> +
>
> This may not do what you want unless you also disable hardware flow
> control.



Here my requirement here is to block the chip to send its data before
HOST changes it is baudrate. So if i disable flow control lines of
HOST which will be in low state.  so that the chip will send it data
before HOST change the baudrate of HOST. which results in frame
reassembly error.


Not sure I understand what you're trying to say above. My point is that
you cannot reliable control RTS when you have automatic flow control
enabled (i.e. it is managed by hardware and it's state reflects whether
there's room in the UART receive FIFO).

Johan


[Bala]: Yes i got your point, but our driver will not support automatic 
flow control (based on the FIFO status)
unless we explicitly enabled via software. i.e. if we enable the 
flow, hardware will look for it.

else it will not looks for CTS or RTS Line.

--
Regards
Balakrishna.

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

2019-01-10 Thread Johan Hovold 
On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:
> Hi Johan,
> 
> On 2019-01-09 20:22, Johan Hovold wrote:
> > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> >> This patch will help to stop frame reassembly errors while changing
> >> the baudrate. This is because host send a change baudrate request
> >> command to the chip with 115200 bps, Whereas chip will change their
> >> UART clocks to the enable for new baudrate and sends the response
> >> for the change request command with newer baudrate, On host side
> >> we are still operating in 115200 bps which results of reading garbage
> >> data. Here we are pulling RTS line, so that chip we will wait to send 
> >> data
> >> to host until host change its baudrate.

> >> +  /* Deassert RTS while changing the baudrate of chip and host.
> >> +   * This will prevent chip from transmitting its response with
> >> +   * the new baudrate while the host port is still operating at
> >> +   * the old speed.
> >> +   */
> >> +  qcadev = serdev_device_get_drvdata(hu->serdev);
> >> +  if (qcadev->btsoc_type == QCA_WCN3990)
> >> +  serdev_device_set_rts(hu->serdev, false);
> >> +
> > 
> > This may not do what you want unless you also disable hardware flow
> > control.

> Here my requirement here is to block the chip to send its data before
> HOST changes it is baudrate. So if i disable flow control lines of
> HOST which will be in low state.  so that the chip will send it data
> before HOST change the baudrate of HOST. which results in frame
> reassembly error.

Not sure I understand what you're trying to say above. My point is that
you cannot reliable control RTS when you have automatic flow control
enabled (i.e. it is managed by hardware and it's state reflects whether
there's room in the UART receive FIFO).

Johan

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

2019-01-10 Thread Balakrishna Godavarthi 

Hi Johan,

On 2019-01-09 20:22, Johan Hovold wrote:

On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:

This patch will help to stop frame reassembly errors while changing
the baudrate. This is because host send a change baudrate request
command to the chip with 115200 bps, Whereas chip will change their
UART clocks to the enable for new baudrate and sends the response
for the change request command with newer baudrate, On host side
we are still operating in 115200 bps which results of reading garbage
data. Here we are pulling RTS line, so that chip we will wait to send 
data

to host until host change its baudrate.

Signed-off-by: Balakrishna Godavarthi 
Tested-by: Matthias Kaehlcke 
Reviewed-by: Matthias Kaehlcke 
---
 drivers/bluetooth/hci_qca.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 5a07c2370289..1680ead6cc3d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
uint8_t baudrate)

struct hci_uart *hu = hci_get_drvdata(hdev);
struct qca_data *qca = hu->priv;
struct sk_buff *skb;
-   struct qca_serdev *qcadev;
u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };

if (baudrate > QCA_BAUDRATE_320)
@@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
uint8_t baudrate)

return -ENOMEM;
}

-   /* Disabling hardware flow control is mandatory while
-* sending change baudrate request to wcn3990 SoC.
-*/
-   qcadev = serdev_device_get_drvdata(hu->serdev);
-   if (qcadev->btsoc_type == QCA_WCN3990)
-   hci_uart_set_flow_control(hu, true);
-
/* Assign commands to change baudrate and packet type. */
skb_put_data(skb, cmd, sizeof(cmd));
hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
@@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
uint8_t baudrate)

schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
set_current_state(TASK_RUNNING);

-   if (qcadev->btsoc_type == QCA_WCN3990)
-   hci_uart_set_flow_control(hu, false);
-
return 0;
 }

@@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
 static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type 
speed_type)

 {
unsigned int speed, qca_baudrate;
+   struct qca_serdev *qcadev;
int ret;

if (speed_type == QCA_INIT_SPEED) {
@@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu, 
enum qca_speed_type speed_type)

if (!speed)
return 0;

+   /* Deassert RTS while changing the baudrate of chip and host.
+* This will prevent chip from transmitting its response with
+* the new baudrate while the host port is still operating at
+* the old speed.
+*/
+   qcadev = serdev_device_get_drvdata(hu->serdev);
+   if (qcadev->btsoc_type == QCA_WCN3990)
+   serdev_device_set_rts(hu->serdev, false);
+


This may not do what you want unless you also disable hardware flow
control.

Johan



Here my requirement here is to block the chip to send its data before 
HOST changes
it is baudrate. So if i disable flow control lines of HOST which will be 
in low state.
so that the chip will send it data before HOST change the baudrate of 
HOST. which results in

frame reassembly error.

--
Regards
Balakrishna.

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

2019-01-09 Thread Johan Hovold 
On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> This patch will help to stop frame reassembly errors while changing
> the baudrate. This is because host send a change baudrate request
> command to the chip with 115200 bps, Whereas chip will change their
> UART clocks to the enable for new baudrate and sends the response
> for the change request command with newer baudrate, On host side
> we are still operating in 115200 bps which results of reading garbage
> data. Here we are pulling RTS line, so that chip we will wait to send data
> to host until host change its baudrate.
> 
> Signed-off-by: Balakrishna Godavarthi 
> Tested-by: Matthias Kaehlcke 
> Reviewed-by: Matthias Kaehlcke 
> ---
>  drivers/bluetooth/hci_qca.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 5a07c2370289..1680ead6cc3d 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t 
> baudrate)
>   struct hci_uart *hu = hci_get_drvdata(hdev);
>   struct qca_data *qca = hu->priv;
>   struct sk_buff *skb;
> - struct qca_serdev *qcadev;
>   u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
>  
>   if (baudrate > QCA_BAUDRATE_320)
> @@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
> uint8_t baudrate)
>   return -ENOMEM;
>   }
>  
> - /* Disabling hardware flow control is mandatory while
> -  * sending change baudrate request to wcn3990 SoC.
> -  */
> - qcadev = serdev_device_get_drvdata(hu->serdev);
> - if (qcadev->btsoc_type == QCA_WCN3990)
> - hci_uart_set_flow_control(hu, true);
> -
>   /* Assign commands to change baudrate and packet type. */
>   skb_put_data(skb, cmd, sizeof(cmd));
>   hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> @@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t 
> baudrate)
>   schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
>   set_current_state(TASK_RUNNING);
>  
> - if (qcadev->btsoc_type == QCA_WCN3990)
> - hci_uart_set_flow_control(hu, false);
> -
>   return 0;
>  }
>  
> @@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  {
>   unsigned int speed, qca_baudrate;
> + struct qca_serdev *qcadev;
>   int ret;
>  
>   if (speed_type == QCA_INIT_SPEED) {
> @@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu, enum 
> qca_speed_type speed_type)
>   if (!speed)
>   return 0;
>  
> + /* Deassert RTS while changing the baudrate of chip and host.
> +  * This will prevent chip from transmitting its response with
> +  * the new baudrate while the host port is still operating at
> +  * the old speed.
> +  */
> + qcadev = serdev_device_get_drvdata(hu->serdev);
> + if (qcadev->btsoc_type == QCA_WCN3990)
> + serdev_device_set_rts(hu->serdev, false);
> +

This may not do what you want unless you also disable hardware flow
control.

Johan

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

2018-12-26 Thread Balakrishna Godavarthi 

Hi Matthias,

On 2018-12-27 01:55, Matthias Kaehlcke wrote:

Hi Balakrishna,

On Wed, Dec 26, 2018 at 11:15:30AM +0530, Balakrishna Godavarthi wrote:

Hi Matthias,

On 2018-12-22 06:01, Matthias Kaehlcke wrote:
> On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> > This patch will help to stop frame reassembly errors while changing
> > the baudrate. This is because host send a change baudrate request
> > command to the chip with 115200 bps, Whereas chip will change their
> > UART clocks to the enable for new baudrate and sends the response
> > for the change request command with newer baudrate, On host side
> > we are still operating in 115200 bps which results of reading garbage
> > data. Here we are pulling RTS line, so that chip we will wait to
> > send data
> > to host until host change its baudrate.
> >
> > Signed-off-by: Balakrishna Godavarthi 
> > Tested-by: Matthias Kaehlcke 
> > Reviewed-by: Matthias Kaehlcke 
> > ---
> >  drivers/bluetooth/hci_qca.c | 24 +---
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index 5a07c2370289..1680ead6cc3d 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev
> > *hdev, uint8_t baudrate)
> >   struct hci_uart *hu = hci_get_drvdata(hdev);
> >   struct qca_data *qca = hu->priv;
> >   struct sk_buff *skb;
> > - struct qca_serdev *qcadev;
> >   u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
> >
> >   if (baudrate > QCA_BAUDRATE_320)
> > @@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev
> > *hdev, uint8_t baudrate)
> >   return -ENOMEM;
> >   }
> >
> > - /* Disabling hardware flow control is mandatory while
> > -  * sending change baudrate request to wcn3990 SoC.
> > -  */
> > - qcadev = serdev_device_get_drvdata(hu->serdev);
> > - if (qcadev->btsoc_type == QCA_WCN3990)
> > - hci_uart_set_flow_control(hu, true);
> > -
> >   /* Assign commands to change baudrate and packet type. */
> >   skb_put_data(skb, cmd, sizeof(cmd));
> >   hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> > @@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev
> > *hdev, uint8_t baudrate)
> >   schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
> >   set_current_state(TASK_RUNNING);
> >
> > - if (qcadev->btsoc_type == QCA_WCN3990)
> > - hci_uart_set_flow_control(hu, false);
> > -
> >   return 0;
> >  }
> >
> > @@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
> >  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type
> > speed_type)
> >  {
> >   unsigned int speed, qca_baudrate;
> > + struct qca_serdev *qcadev;
> >   int ret;
> >
> >   if (speed_type == QCA_INIT_SPEED) {
> > @@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu,
> > enum qca_speed_type speed_type)
> >   if (!speed)
> >   return 0;
> >
> > + /* Deassert RTS while changing the baudrate of chip and host.
> > +  * This will prevent chip from transmitting its response with
> > +  * the new baudrate while the host port is still operating at
> > +  * the old speed.
> > +  */
> > + qcadev = serdev_device_get_drvdata(hu->serdev);
> > + if (qcadev->btsoc_type == QCA_WCN3990)
> > + serdev_device_set_rts(hu->serdev, false);
> > +
> >   qca_baudrate = qca_get_baudrate_value(speed);
> >   bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
> >   ret = qca_set_baudrate(hu->hdev, qca_baudrate);
> > @@ -1104,6 +1103,9 @@ static int qca_set_speed(struct hci_uart *hu,
> > enum qca_speed_type speed_type)
> >   return ret;
> >
> >   host_set_baudrate(hu, speed);
> > +
> > + if (qcadev->btsoc_type == QCA_WCN3990)
> > + serdev_device_set_rts(hu->serdev, true);
> >   }
> >
> >   return 0;
>
> I looked for ways to do without this change, but didn't find a good
> solution. There are several possible problems with baudrate changes:
>
> 1) send request to BT controller to change the baudrate
>
>   this is an asynchronous operation, the actual baudrate change can
>   be delayed for multiple reasons, e.g.:
>
>   - request sits in the BT driver's TX queue
>
> this could be worked around by checking skb_queue_empty()
>
>   - request sits in the UART buffer
>
> a workaround for this could be calling
> serdev_device_wait_until_sent() (only available with serdev though)
>
>   - the request sits in the UART FIFO
>
> will be sent out 'immediately'. no neat solution available AFAIK,
> a short sleep could be an effective workaround
>
>   - the controller may have a short delay to

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

2018-12-26 Thread Matthias Kaehlcke 
Hi Balakrishna,

On Wed, Dec 26, 2018 at 11:15:30AM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-12-22 06:01, Matthias Kaehlcke wrote:
> > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> > > This patch will help to stop frame reassembly errors while changing
> > > the baudrate. This is because host send a change baudrate request
> > > command to the chip with 115200 bps, Whereas chip will change their
> > > UART clocks to the enable for new baudrate and sends the response
> > > for the change request command with newer baudrate, On host side
> > > we are still operating in 115200 bps which results of reading garbage
> > > data. Here we are pulling RTS line, so that chip we will wait to
> > > send data
> > > to host until host change its baudrate.
> > > 
> > > Signed-off-by: Balakrishna Godavarthi 
> > > Tested-by: Matthias Kaehlcke 
> > > Reviewed-by: Matthias Kaehlcke 
> > > ---
> > >  drivers/bluetooth/hci_qca.c | 24 +---
> > >  1 file changed, 13 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index 5a07c2370289..1680ead6cc3d 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > > @@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev
> > > *hdev, uint8_t baudrate)
> > >   struct hci_uart *hu = hci_get_drvdata(hdev);
> > >   struct qca_data *qca = hu->priv;
> > >   struct sk_buff *skb;
> > > - struct qca_serdev *qcadev;
> > >   u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
> > > 
> > >   if (baudrate > QCA_BAUDRATE_320)
> > > @@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev
> > > *hdev, uint8_t baudrate)
> > >   return -ENOMEM;
> > >   }
> > > 
> > > - /* Disabling hardware flow control is mandatory while
> > > -  * sending change baudrate request to wcn3990 SoC.
> > > -  */
> > > - qcadev = serdev_device_get_drvdata(hu->serdev);
> > > - if (qcadev->btsoc_type == QCA_WCN3990)
> > > - hci_uart_set_flow_control(hu, true);
> > > -
> > >   /* Assign commands to change baudrate and packet type. */
> > >   skb_put_data(skb, cmd, sizeof(cmd));
> > >   hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> > > @@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev
> > > *hdev, uint8_t baudrate)
> > >   schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
> > >   set_current_state(TASK_RUNNING);
> > > 
> > > - if (qcadev->btsoc_type == QCA_WCN3990)
> > > - hci_uart_set_flow_control(hu, false);
> > > -
> > >   return 0;
> > >  }
> > > 
> > > @@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
> > >  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type
> > > speed_type)
> > >  {
> > >   unsigned int speed, qca_baudrate;
> > > + struct qca_serdev *qcadev;
> > >   int ret;
> > > 
> > >   if (speed_type == QCA_INIT_SPEED) {
> > > @@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu,
> > > enum qca_speed_type speed_type)
> > >   if (!speed)
> > >   return 0;
> > > 
> > > + /* Deassert RTS while changing the baudrate of chip and host.
> > > +  * This will prevent chip from transmitting its response with
> > > +  * the new baudrate while the host port is still operating at
> > > +  * the old speed.
> > > +  */
> > > + qcadev = serdev_device_get_drvdata(hu->serdev);
> > > + if (qcadev->btsoc_type == QCA_WCN3990)
> > > + serdev_device_set_rts(hu->serdev, false);
> > > +
> > >   qca_baudrate = qca_get_baudrate_value(speed);
> > >   bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
> > >   ret = qca_set_baudrate(hu->hdev, qca_baudrate);
> > > @@ -1104,6 +1103,9 @@ static int qca_set_speed(struct hci_uart *hu,
> > > enum qca_speed_type speed_type)
> > >   return ret;
> > > 
> > >   host_set_baudrate(hu, speed);
> > > +
> > > + if (qcadev->btsoc_type == QCA_WCN3990)
> > > + serdev_device_set_rts(hu->serdev, true);
> > >   }
> > > 
> > >   return 0;
> > 
> > I looked for ways to do without this change, but didn't find a good
> > solution. There are several possible problems with baudrate changes:
> > 
> > 1) send request to BT controller to change the baudrate
> > 
> >   this is an asynchronous operation, the actual baudrate change can
> >   be delayed for multiple reasons, e.g.:
> > 
> >   - request sits in the BT driver's TX queue
> > 
> > this could be worked around by checking skb_queue_empty()
> > 
> >   - request sits in the UART buffer
> > 
> > a workaround for this could be calling
> > serdev_device_wait_until_sent() (only available with serdev though)
> > 
> >   - the request sits in the UART FIFO
> > 
> > will be sent out 'immediately'. no neat solution available AFAIK,
> > a short sleep could be an effective workaround
> > 
> >   - the controller may

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

2018-12-25 Thread Balakrishna Godavarthi 

Hi Matthias,

On 2018-12-22 06:01, Matthias Kaehlcke wrote:

On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:

This patch will help to stop frame reassembly errors while changing
the baudrate. This is because host send a change baudrate request
command to the chip with 115200 bps, Whereas chip will change their
UART clocks to the enable for new baudrate and sends the response
for the change request command with newer baudrate, On host side
we are still operating in 115200 bps which results of reading garbage
data. Here we are pulling RTS line, so that chip we will wait to send 
data

to host until host change its baudrate.

Signed-off-by: Balakrishna Godavarthi 
Tested-by: Matthias Kaehlcke 
Reviewed-by: Matthias Kaehlcke 
---
 drivers/bluetooth/hci_qca.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 5a07c2370289..1680ead6cc3d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
uint8_t baudrate)

struct hci_uart *hu = hci_get_drvdata(hdev);
struct qca_data *qca = hu->priv;
struct sk_buff *skb;
-   struct qca_serdev *qcadev;
u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };

if (baudrate > QCA_BAUDRATE_320)
@@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
uint8_t baudrate)

return -ENOMEM;
}

-   /* Disabling hardware flow control is mandatory while
-* sending change baudrate request to wcn3990 SoC.
-*/
-   qcadev = serdev_device_get_drvdata(hu->serdev);
-   if (qcadev->btsoc_type == QCA_WCN3990)
-   hci_uart_set_flow_control(hu, true);
-
/* Assign commands to change baudrate and packet type. */
skb_put_data(skb, cmd, sizeof(cmd));
hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
@@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
uint8_t baudrate)

schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
set_current_state(TASK_RUNNING);

-   if (qcadev->btsoc_type == QCA_WCN3990)
-   hci_uart_set_flow_control(hu, false);
-
return 0;
 }

@@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
 static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type 
speed_type)

 {
unsigned int speed, qca_baudrate;
+   struct qca_serdev *qcadev;
int ret;

if (speed_type == QCA_INIT_SPEED) {
@@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu, 
enum qca_speed_type speed_type)

if (!speed)
return 0;

+   /* Deassert RTS while changing the baudrate of chip and host.
+* This will prevent chip from transmitting its response with
+* the new baudrate while the host port is still operating at
+* the old speed.
+*/
+   qcadev = serdev_device_get_drvdata(hu->serdev);
+   if (qcadev->btsoc_type == QCA_WCN3990)
+   serdev_device_set_rts(hu->serdev, false);
+
qca_baudrate = qca_get_baudrate_value(speed);
bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
ret = qca_set_baudrate(hu->hdev, qca_baudrate);
@@ -1104,6 +1103,9 @@ static int qca_set_speed(struct hci_uart *hu, 
enum qca_speed_type speed_type)

return ret;

host_set_baudrate(hu, speed);
+
+   if (qcadev->btsoc_type == QCA_WCN3990)
+   serdev_device_set_rts(hu->serdev, true);
}

return 0;


I looked for ways to do without this change, but didn't find a good
solution. There are several possible problems with baudrate changes:

1) send request to BT controller to change the baudrate

  this is an asynchronous operation, the actual baudrate change can
  be delayed for multiple reasons, e.g.:

  - request sits in the BT driver's TX queue

this could be worked around by checking skb_queue_empty()

  - request sits in the UART buffer

a workaround for this could be calling
serdev_device_wait_until_sent() (only available with serdev though)

  - the request sits in the UART FIFO

will be sent out 'immediately'. no neat solution available AFAIK,
a short sleep could be an effective workaround

  - the controller may have a short delay to apply the change

Also no neat solution here. A/the same short sleep could work
around this

2) change baudrate of the host UART
  - this must not happen before the baudrate change request has been
sent to the BT controller, otherwise things are messed up
seriously

Ideally set_termios would make sure all pending data is sent
before the change is applied, some UART drivers do this, others
don't, so we can't rely

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

2018-12-21 Thread Matthias Kaehlcke 
On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> This patch will help to stop frame reassembly errors while changing
> the baudrate. This is because host send a change baudrate request
> command to the chip with 115200 bps, Whereas chip will change their
> UART clocks to the enable for new baudrate and sends the response
> for the change request command with newer baudrate, On host side
> we are still operating in 115200 bps which results of reading garbage
> data. Here we are pulling RTS line, so that chip we will wait to send data
> to host until host change its baudrate.
> 
> Signed-off-by: Balakrishna Godavarthi 
> Tested-by: Matthias Kaehlcke 
> Reviewed-by: Matthias Kaehlcke 
> ---
>  drivers/bluetooth/hci_qca.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 5a07c2370289..1680ead6cc3d 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t 
> baudrate)
>   struct hci_uart *hu = hci_get_drvdata(hdev);
>   struct qca_data *qca = hu->priv;
>   struct sk_buff *skb;
> - struct qca_serdev *qcadev;
>   u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
>  
>   if (baudrate > QCA_BAUDRATE_320)
> @@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
> uint8_t baudrate)
>   return -ENOMEM;
>   }
>  
> - /* Disabling hardware flow control is mandatory while
> -  * sending change baudrate request to wcn3990 SoC.
> -  */
> - qcadev = serdev_device_get_drvdata(hu->serdev);
> - if (qcadev->btsoc_type == QCA_WCN3990)
> - hci_uart_set_flow_control(hu, true);
> -
>   /* Assign commands to change baudrate and packet type. */
>   skb_put_data(skb, cmd, sizeof(cmd));
>   hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> @@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t 
> baudrate)
>   schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
>   set_current_state(TASK_RUNNING);
>  
> - if (qcadev->btsoc_type == QCA_WCN3990)
> - hci_uart_set_flow_control(hu, false);
> -
>   return 0;
>  }
>  
> @@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  {
>   unsigned int speed, qca_baudrate;
> + struct qca_serdev *qcadev;
>   int ret;
>  
>   if (speed_type == QCA_INIT_SPEED) {
> @@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu, enum 
> qca_speed_type speed_type)
>   if (!speed)
>   return 0;
>  
> + /* Deassert RTS while changing the baudrate of chip and host.
> +  * This will prevent chip from transmitting its response with
> +  * the new baudrate while the host port is still operating at
> +  * the old speed.
> +  */
> + qcadev = serdev_device_get_drvdata(hu->serdev);
> + if (qcadev->btsoc_type == QCA_WCN3990)
> + serdev_device_set_rts(hu->serdev, false);
> +
>   qca_baudrate = qca_get_baudrate_value(speed);
>   bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
>   ret = qca_set_baudrate(hu->hdev, qca_baudrate);
> @@ -1104,6 +1103,9 @@ static int qca_set_speed(struct hci_uart *hu, enum 
> qca_speed_type speed_type)
>   return ret;
>  
>   host_set_baudrate(hu, speed);
> +
> + if (qcadev->btsoc_type == QCA_WCN3990)
> + serdev_device_set_rts(hu->serdev, true);
>   }
>  
>   return 0;

I looked for ways to do without this change, but didn't find a good
solution. There are several possible problems with baudrate changes:

1) send request to BT controller to change the baudrate

  this is an asynchronous operation, the actual baudrate change can
  be delayed for multiple reasons, e.g.:

  - request sits in the BT driver's TX queue

this could be worked around by checking skb_queue_empty()

  - request sits in the UART buffer

a workaround for this could be calling
serdev_device_wait_until_sent() (only available with serdev though)

  - the request sits in the UART FIFO

will be sent out 'immediately'. no neat solution available AFAIK,
a short sleep could be an effective workaround

  - the controller may have a short delay to apply the change

Also no neat solution here. A/the same short sleep could work
around this

2) change baudrate of the host UART
  - this must not happen before the baudrate change request has been
sent to the BT controller, otherwise things are messed up
seriously

Ideally set_termios would make sure all pending data is sent
before the change is applied, some UART drivers do this, others

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

2018-12-20 Thread Balakrishna Godavarthi 
This patch will help to stop frame reassembly errors while changing
the baudrate. This is because host send a change baudrate request
command to the chip with 115200 bps, Whereas chip will change their
UART clocks to the enable for new baudrate and sends the response
for the change request command with newer baudrate, On host side
we are still operating in 115200 bps which results of reading garbage
data. Here we are pulling RTS line, so that chip we will wait to send data
to host until host change its baudrate.

Signed-off-by: Balakrishna Godavarthi 
Tested-by: Matthias Kaehlcke 
Reviewed-by: Matthias Kaehlcke 
---
 drivers/bluetooth/hci_qca.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 5a07c2370289..1680ead6cc3d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t 
baudrate)
struct hci_uart *hu = hci_get_drvdata(hdev);
struct qca_data *qca = hu->priv;
struct sk_buff *skb;
-   struct qca_serdev *qcadev;
u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
 
if (baudrate > QCA_BAUDRATE_320)
@@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t 
baudrate)
return -ENOMEM;
}
 
-   /* Disabling hardware flow control is mandatory while
-* sending change baudrate request to wcn3990 SoC.
-*/
-   qcadev = serdev_device_get_drvdata(hu->serdev);
-   if (qcadev->btsoc_type == QCA_WCN3990)
-   hci_uart_set_flow_control(hu, true);
-
/* Assign commands to change baudrate and packet type. */
skb_put_data(skb, cmd, sizeof(cmd));
hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
@@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t 
baudrate)
schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
set_current_state(TASK_RUNNING);
 
-   if (qcadev->btsoc_type == QCA_WCN3990)
-   hci_uart_set_flow_control(hu, false);
-
return 0;
 }
 
@@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
 static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 {
unsigned int speed, qca_baudrate;
+   struct qca_serdev *qcadev;
int ret;
 
if (speed_type == QCA_INIT_SPEED) {
@@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu, enum 
qca_speed_type speed_type)
if (!speed)
return 0;
 
+   /* Deassert RTS while changing the baudrate of chip and host.
+* This will prevent chip from transmitting its response with
+* the new baudrate while the host port is still operating at
+* the old speed.
+*/
+   qcadev = serdev_device_get_drvdata(hu->serdev);
+   if (qcadev->btsoc_type == QCA_WCN3990)
+   serdev_device_set_rts(hu->serdev, false);
+
qca_baudrate = qca_get_baudrate_value(speed);
bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
ret = qca_set_baudrate(hu->hdev, qca_baudrate);
@@ -1104,6 +1103,9 @@ static int qca_set_speed(struct hci_uart *hu, enum 
qca_speed_type speed_type)
return ret;
 
host_set_baudrate(hu, speed);
+
+   if (qcadev->btsoc_type == QCA_WCN3990)
+   serdev_device_set_rts(hu->serdev, true);
}
 
return 0;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project