[PATCH v5 0/5] Bug fixes for Qualcomm BT chip wcn3990

[PATCH v5 0/5] Bug fixes for Qualcomm BT chip wcn3990

[Balakrishna Godavarthi]

The below issues are found in the recent testing.

1. Observed device is not going into off state or not responding.
    As wcn3990 require a power pulses to turn on the irrespctive of
    igniting regulators, it was observed that power on or power off
    pulses are not in sync with respective to chip.
    The below patch will help us to wait until byte is pushed on to wires.          

    * Bluetooth: hci_qca: use wait_until_sent() for power pulses

2. Observed Chip responding when we are in sleep.
   This is due to turn on flow control during change baudrate request.
   The below patch will only pull the RTS line high instead of turning off
   the flow.

   * Bluetooth: hci_qca: Pull RTS line high for baudrate change command.

3. Frame reassembly errors splashing on console.
   wcn3990 requires will use multiple baudrates during booting stage.
   i.e. 2400 bps while sending power off pulse
    115200 bps while sending power on pulse
    port close
    port open
    set baudrate to 115200 request the chip version.

  during above process, we are seeing some stray bytes coming up
  on the UART Rx FIFO it could be due to frequent baudrate change.

  This patch will stop the frame reassembly errors.

  * Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990

4. Disable IBS state machine and flush Tx buffer
   We are disabling IBS and flushing the Tx buffer before turning off the chip.
  
   This is due to IBS state machine is active when we turn off the chip.
   This will stop queuing IBS protocol bytes.

   * Bluetooth: hci_qca: Disable IBS state machine and flush Tx buffer

5. btqca: inject command complete event during fw download
   
   Qualcomm latest chip will not send an command complete event
   for last packet of the fw dump sent, so here we are inject an command 
   complete event.

   * Bluetooth: btqca: inject command complete event during fw download

Changes in V5:
 * added serdev_device_wrute_flush before sending the power off pulse
   during shutdown.

Changes in v4:
 * used serdev_device_write_buf() instead of serdev_device_write().
 * added new patch to stop logging of 0xfc00 timeout on console.

Changes in v3:
 * moved IBS & qca_flush to different patch
 * updated comments in code fo Deassert RTS patch

Balakrishna Godavarthi (5):
  Bluetooth: hci_qca: use wait_until_sent() for power pulses
  Bluetooth: hci_qca: Deassert RTS while baudrate change command
  Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990
  Bluetooth: hci_qca: Disable IBS state machine and flush Tx buffer
  Bluetooth: btqca: inject command complete event during fw download

 drivers/bluetooth/btqca.c   | 39 ++++++++++++++++++-
 drivers/bluetooth/btqca.h   |  3 ++
 drivers/bluetooth/hci_qca.c | 78 +++++++++++++++++++++----------------
 3 files changed, 85 insertions(+), 35 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

[Balakrishna Godavarthi]

wcn3990 requires a power pulse to turn ON/OFF along with
regulators. Sometimes we are observing the power pulses are sent
out with some time delay, due to queuing these commands. This is
causing synchronization issues with chip, which intern delay the
chip setup or may end up with communication issues.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
 drivers/bluetooth/hci_qca.c | 38 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index f036c8f98ea3..5a07c2370289 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
 		hci_uart_set_baudrate(hu, speed);
 }
 
-static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
+static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
 {
-	struct hci_uart *hu = hci_get_drvdata(hdev);
-	struct qca_data *qca = hu->priv;
-	struct sk_buff *skb;
+	int ret;
 
 	/* These power pulses are single byte command which are sent
 	 * at required baudrate to wcn3990. On wcn3990, we have an external
@@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
 	 * save power. Disabling hardware flow control is mandatory while
 	 * sending power pulses to SoC.
 	 */
-	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
-
-	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
-	if (!skb)
-		return -ENOMEM;
-
+	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
 	hci_uart_set_flow_control(hu, true);
+	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
+	if (ret < 0) {
+		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
+			   cmd);
+		return ret;
+	}
 
-	skb_put_u8(skb, cmd);
-	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
-
-	skb_queue_tail(&qca->txq, skb);
-	hci_uart_tx_wakeup(hu);
+	serdev_device_wait_until_sent(hu->serdev, 0);
 
 	/* Wait for 100 uS for SoC to settle down */
 	usleep_range(100, 200);
@@ -1116,7 +1111,6 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 
 static int qca_wcn3990_init(struct hci_uart *hu)
 {
-	struct hci_dev *hdev = hu->hdev;
 	struct qca_serdev *qcadev;
 	int ret;
 
@@ -1139,12 +1133,12 @@ static int qca_wcn3990_init(struct hci_uart *hu)
 
 	/* Forcefully enable wcn3990 to enter in to boot mode. */
 	host_set_baudrate(hu, 2400);
-	ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE);
+	ret = qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE);
 	if (ret)
 		return ret;
 
 	qca_set_speed(hu, QCA_INIT_SPEED);
-	ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
+	ret = qca_send_power_pulse(hu, QCA_WCN3990_POWERON_PULSE);
 	if (ret)
 		return ret;
 
@@ -1274,13 +1268,9 @@ static const struct qca_vreg_data qca_soc_data = {
 
 static void qca_power_shutdown(struct hci_uart *hu)
 {
-	struct serdev_device *serdev = hu->serdev;
-	unsigned char cmd = QCA_WCN3990_POWEROFF_PULSE;
-
+	serdev_device_write_flush(hu->serdev);
 	host_set_baudrate(hu, 2400);
-	hci_uart_set_flow_control(hu, true);
-	serdev_device_write_buf(serdev, &cmd, sizeof(cmd));
-	hci_uart_set_flow_control(hu, false);
+	qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE);
 	qca_power_setup(hu, false);
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

[Matthias Kaehlcke]

On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi wrote:
> wcn3990 requires a power pulse to turn ON/OFF along with
> regulators. Sometimes we are observing the power pulses are sent
> out with some time delay, due to queuing these commands. This is
> causing synchronization issues with chip, which intern delay the
> chip setup or may end up with communication issues.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
>  drivers/bluetooth/hci_qca.c | 38 ++++++++++++++-----------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index f036c8f98ea3..5a07c2370289 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
>  		hci_uart_set_baudrate(hu, speed);
>  }
>  
> -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
> +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
>  {
> -	struct hci_uart *hu = hci_get_drvdata(hdev);
> -	struct qca_data *qca = hu->priv;
> -	struct sk_buff *skb;
> +	int ret;
>  
>  	/* These power pulses are single byte command which are sent
>  	 * at required baudrate to wcn3990. On wcn3990, we have an external
> @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>  	 * save power. Disabling hardware flow control is mandatory while
>  	 * sending power pulses to SoC.
>  	 */
> -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
> -
> -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
> -	if (!skb)
> -		return -ENOMEM;
> -
> +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
>  	hci_uart_set_flow_control(hu, true);
> +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
> +	if (ret < 0) {
> +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
> +			   cmd);
> +		return ret;
> +	}
>  
> -	skb_put_u8(skb, cmd);
> -	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> -
> -	skb_queue_tail(&qca->txq, skb);
> -	hci_uart_tx_wakeup(hu);
> +	serdev_device_wait_until_sent(hu->serdev, 0);

serdev_device_wait_until_sent() might only guarantee that the UART
circular buffer is empty (see
https://elixir.bootlin.com/linux/v4.19/source/drivers/tty/tty_ioctl.c#L225),
not that the data has actually sent (e.g. it might be sitting in
the UART FIFO). However with this:

>  	/* Wait for 100 uS for SoC to settle down */
>  	usleep_range(100, 200);

we should probably be fine, unless there's tons of data in the
FIFO.

You currently call serdev_device_write_flush() in
qca_power_shutdown(), I wonder if it would make sense to call it in
qca_send_power_pulse(), regardless of whether it's an on or off
pulse. In any case we don't care if the chip receives any 'pending'
data when we switch it on or off, right?

Cheers

Matthias

Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-12-22 07:29, Matthias Kaehlcke wrote:
> On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi wrote:
>> wcn3990 requires a power pulse to turn ON/OFF along with
>> regulators. Sometimes we are observing the power pulses are sent
>> out with some time delay, due to queuing these commands. This is
>> causing synchronization issues with chip, which intern delay the
>> chip setup or may end up with communication issues.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>>  drivers/bluetooth/hci_qca.c | 38 
>> ++++++++++++++-----------------------
>>  1 file changed, 14 insertions(+), 24 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index f036c8f98ea3..5a07c2370289 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct 
>> hci_uart *hu, unsigned int speed)
>>  		hci_uart_set_baudrate(hu, speed);
>>  }
>> 
>> -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>> +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
>>  {
>> -	struct hci_uart *hu = hci_get_drvdata(hdev);
>> -	struct qca_data *qca = hu->priv;
>> -	struct sk_buff *skb;
>> +	int ret;
>> 
>>  	/* These power pulses are single byte command which are sent
>>  	 * at required baudrate to wcn3990. On wcn3990, we have an external
>> @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct hci_dev 
>> *hdev, u8 cmd)
>>  	 * save power. Disabling hardware flow control is mandatory while
>>  	 * sending power pulses to SoC.
>>  	 */
>> -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
>> -
>> -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
>> -	if (!skb)
>> -		return -ENOMEM;
>> -
>> +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
>>  	hci_uart_set_flow_control(hu, true);
>> +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
>> +	if (ret < 0) {
>> +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
>> +			   cmd);
>> +		return ret;
>> +	}
>> 
>> -	skb_put_u8(skb, cmd);
>> -	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
>> -
>> -	skb_queue_tail(&qca->txq, skb);
>> -	hci_uart_tx_wakeup(hu);
>> +	serdev_device_wait_until_sent(hu->serdev, 0);
> 
> serdev_device_wait_until_sent() might only guarantee that the UART
> circular buffer is empty (see
> https://elixir.bootlin.com/linux/v4.19/source/drivers/tty/tty_ioctl.c#L225),
> not that the data has actually sent (e.g. it might be sitting in
> the UART FIFO). However with this:
> 
>>  	/* Wait for 100 uS for SoC to settle down */
>>  	usleep_range(100, 200);
> 
> we should probably be fine, unless there's tons of data in the
> FIFO.
> 
> You currently call serdev_device_write_flush() in
> qca_power_shutdown(), I wonder if it would make sense to call it in
> qca_send_power_pulse(), regardless of whether it's an on or off

[Bala]: during sending the ON pulse we will not have any data in the
         UART circular buffer as this is the first command to send and we 
are sending it as soon as we open the port.
         so i taught why should be flush the circular as it is already 
empty.

> pulse. In any case we don't care if the chip receives any 'pending'
> data when we switch it on or off, right?
> 

[Bala]: during on we freshly open port and i see that flush() called 
while port opening.

https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/serial_core.c#L207



> Cheers
> 
> Matthias


-- 
Regards
Balakrishna.

Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

[Matthias Kaehlcke]

On Wed, Dec 26, 2018 at 12:01:51PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-12-22 07:29, Matthias Kaehlcke wrote:
> > On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi wrote:
> > > wcn3990 requires a power pulse to turn ON/OFF along with
> > > regulators. Sometimes we are observing the power pulses are sent
> > > out with some time delay, due to queuing these commands. This is
> > > causing synchronization issues with chip, which intern delay the
> > > chip setup or may end up with communication issues.
> > > 
> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > ---
> > >  drivers/bluetooth/hci_qca.c | 38
> > > ++++++++++++++-----------------------
> > >  1 file changed, 14 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index f036c8f98ea3..5a07c2370289 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > > @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct
> > > hci_uart *hu, unsigned int speed)
> > >  		hci_uart_set_baudrate(hu, speed);
> > >  }
> > > 
> > > -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
> > > +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
> > >  {
> > > -	struct hci_uart *hu = hci_get_drvdata(hdev);
> > > -	struct qca_data *qca = hu->priv;
> > > -	struct sk_buff *skb;
> > > +	int ret;
> > > 
> > >  	/* These power pulses are single byte command which are sent
> > >  	 * at required baudrate to wcn3990. On wcn3990, we have an external
> > > @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct
> > > hci_dev *hdev, u8 cmd)
> > >  	 * save power. Disabling hardware flow control is mandatory while
> > >  	 * sending power pulses to SoC.
> > >  	 */
> > > -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
> > > -
> > > -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
> > > -	if (!skb)
> > > -		return -ENOMEM;
> > > -
> > > +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
> > >  	hci_uart_set_flow_control(hu, true);
> > > +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
> > > +	if (ret < 0) {
> > > +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
> > > +			   cmd);
> > > +		return ret;
> > > +	}
> > > 
> > > -	skb_put_u8(skb, cmd);
> > > -	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> > > -
> > > -	skb_queue_tail(&qca->txq, skb);
> > > -	hci_uart_tx_wakeup(hu);
> > > +	serdev_device_wait_until_sent(hu->serdev, 0);
> > 
> > serdev_device_wait_until_sent() might only guarantee that the UART
> > circular buffer is empty (see
> > https://elixir.bootlin.com/linux/v4.19/source/drivers/tty/tty_ioctl.c#L225),
> > not that the data has actually sent (e.g. it might be sitting in
> > the UART FIFO). However with this:
> > 
> > >  	/* Wait for 100 uS for SoC to settle down */
> > >  	usleep_range(100, 200);
> > 
> > we should probably be fine, unless there's tons of data in the
> > FIFO.
> > 
> > You currently call serdev_device_write_flush() in
> > qca_power_shutdown(), I wonder if it would make sense to call it in
> > qca_send_power_pulse(), regardless of whether it's an on or off
> 
> [Bala]: during sending the ON pulse we will not have any data in the
>         UART circular buffer as this is the first command to send and we are
> sending it as soon as we open the port.
>         so i taught why should be flush the circular as it is already empty.

> > pulse. In any case we don't care if the chip receives any 'pending'
> > data when we switch it on or off, right?
> > 
> 
> [Bala]: during on we freshly open port and i see that flush() called while
> port opening.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/serial_core.c#L207

I would argue that the serdev_device_write_flush() call in
qca_power_shutdown() is related with/needed for sending the power off
pulse, hence it should be part of qca_send_power_pulse(), unless it
adds a significant overhead and we really want to call it only in the
shutdown case.

Flushing the buffer should be fairly lightweight and power pulses are
only sent when the device is switched on or off, so the overhead
should be negligible. You *could* restrict the flush to the power off
pulse, assuming that the driver always re-opens the port in
qca_wcn3990_init() (tests with this patch set suggest this might not
be needed) and that serdev_device_open() flushes the buffer (which
seems a sane assumption). Yet given the minimal overhead I'd suggest
to not make assumptions about what happened previously in other code
and avoid the clutter of a condition that doesn't add much value.

Cheers

Matthias

Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-12-27 03:51, Matthias Kaehlcke wrote:
> On Wed, Dec 26, 2018 at 12:01:51PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-12-22 07:29, Matthias Kaehlcke wrote:
>> > On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi wrote:
>> > > wcn3990 requires a power pulse to turn ON/OFF along with
>> > > regulators. Sometimes we are observing the power pulses are sent
>> > > out with some time delay, due to queuing these commands. This is
>> > > causing synchronization issues with chip, which intern delay the
>> > > chip setup or may end up with communication issues.
>> > >
>> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > > ---
>> > >  drivers/bluetooth/hci_qca.c | 38
>> > > ++++++++++++++-----------------------
>> > >  1 file changed, 14 insertions(+), 24 deletions(-)
>> > >
>> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > index f036c8f98ea3..5a07c2370289 100644
>> > > --- a/drivers/bluetooth/hci_qca.c
>> > > +++ b/drivers/bluetooth/hci_qca.c
>> > > @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct
>> > > hci_uart *hu, unsigned int speed)
>> > >  		hci_uart_set_baudrate(hu, speed);
>> > >  }
>> > >
>> > > -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>> > > +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
>> > >  {
>> > > -	struct hci_uart *hu = hci_get_drvdata(hdev);
>> > > -	struct qca_data *qca = hu->priv;
>> > > -	struct sk_buff *skb;
>> > > +	int ret;
>> > >
>> > >  	/* These power pulses are single byte command which are sent
>> > >  	 * at required baudrate to wcn3990. On wcn3990, we have an external
>> > > @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct
>> > > hci_dev *hdev, u8 cmd)
>> > >  	 * save power. Disabling hardware flow control is mandatory while
>> > >  	 * sending power pulses to SoC.
>> > >  	 */
>> > > -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
>> > > -
>> > > -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
>> > > -	if (!skb)
>> > > -		return -ENOMEM;
>> > > -
>> > > +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
>> > >  	hci_uart_set_flow_control(hu, true);
>> > > +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
>> > > +	if (ret < 0) {
>> > > +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
>> > > +			   cmd);
>> > > +		return ret;
>> > > +	}
>> > >
>> > > -	skb_put_u8(skb, cmd);
>> > > -	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
>> > > -
>> > > -	skb_queue_tail(&qca->txq, skb);
>> > > -	hci_uart_tx_wakeup(hu);
>> > > +	serdev_device_wait_until_sent(hu->serdev, 0);
>> >
>> > serdev_device_wait_until_sent() might only guarantee that the UART
>> > circular buffer is empty (see
>> > https://elixir.bootlin.com/linux/v4.19/source/drivers/tty/tty_ioctl.c#L225),
>> > not that the data has actually sent (e.g. it might be sitting in
>> > the UART FIFO). However with this:
>> >
>> > >  	/* Wait for 100 uS for SoC to settle down */
>> > >  	usleep_range(100, 200);
>> >
>> > we should probably be fine, unless there's tons of data in the
>> > FIFO.
>> >
>> > You currently call serdev_device_write_flush() in
>> > qca_power_shutdown(), I wonder if it would make sense to call it in
>> > qca_send_power_pulse(), regardless of whether it's an on or off
>> 
>> [Bala]: during sending the ON pulse we will not have any data in the
>>         UART circular buffer as this is the first command to send and 
>> we are
>> sending it as soon as we open the port.
>>         so i taught why should be flush the circular as it is already 
>> empty.
> 
>> > pulse. In any case we don't care if the chip receives any 'pending'
>> > data when we switch it on or off, right?
>> >
>> 
>> [Bala]: during on we freshly open port and i see that flush() called 
>> while
>> port opening.
>> 
>> https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/serial_core.c#L207
> 
> I would argue that the serdev_device_write_flush() call in
> qca_power_shutdown() is related with/needed for sending the power off
> pulse, hence it should be part of qca_send_power_pulse(), unless it
> adds a significant overhead and we really want to call it only in the
> shutdown case.
> 
> Flushing the buffer should be fairly lightweight and power pulses are
> only sent when the device is switched on or off, so the overhead
> should be negligible. You *could* restrict the flush to the power off
> pulse, assuming that the driver always re-opens the port in
> qca_wcn3990_init() (tests with this patch set suggest this might not
> be needed) and that serdev_device_open() flushes the buffer (which
> seems a sane assumption). Yet given the minimal overhead I'd suggest
> to not make assumptions about what happened previously in other code
> and avoid the clutter of a condition that doesn't add much value.
> 
[Bala]: will call the flush() while sending the power pulses 
irrespective of the pulse type.

> Cheers
> 
> Matthias

-- 
Regards
Balakrishna.

Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

[Johan Hovold]

On Fri, Dec 21, 2018 at 05:59:47PM -0800, Matthias Kaehlcke wrote:
> On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi wrote:
> > wcn3990 requires a power pulse to turn ON/OFF along with
> > regulators. Sometimes we are observing the power pulses are sent
> > out with some time delay, due to queuing these commands. This is
> > causing synchronization issues with chip, which intern delay the
> > chip setup or may end up with communication issues.
> > 
> > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > ---
> >  drivers/bluetooth/hci_qca.c | 38 ++++++++++++++-----------------------
> >  1 file changed, 14 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index f036c8f98ea3..5a07c2370289 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
> >  		hci_uart_set_baudrate(hu, speed);
> >  }
> >  
> > -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
> > +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
> >  {
> > -	struct hci_uart *hu = hci_get_drvdata(hdev);
> > -	struct qca_data *qca = hu->priv;
> > -	struct sk_buff *skb;
> > +	int ret;
> >  
> >  	/* These power pulses are single byte command which are sent
> >  	 * at required baudrate to wcn3990. On wcn3990, we have an external
> > @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
> >  	 * save power. Disabling hardware flow control is mandatory while
> >  	 * sending power pulses to SoC.
> >  	 */
> > -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
> > -
> > -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
> > -	if (!skb)
> > -		return -ENOMEM;
> > -
> > +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
> >  	hci_uart_set_flow_control(hu, true);
> > +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
> > +	if (ret < 0) {
> > +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
> > +			   cmd);
> > +		return ret;
> > +	}
> >  
> > -	skb_put_u8(skb, cmd);
> > -	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> > -
> > -	skb_queue_tail(&qca->txq, skb);
> > -	hci_uart_tx_wakeup(hu);
> > +	serdev_device_wait_until_sent(hu->serdev, 0);

Again, do you really want to wait indefinitely here?

> serdev_device_wait_until_sent() might only guarantee that the UART
> circular buffer is empty (see
> https://elixir.bootlin.com/linux/v4.19/source/drivers/tty/tty_ioctl.c#L225),
> not that the data has actually sent (e.g. it might be sitting in
> the UART FIFO).

For serial core, uart_wait_until_sent() makes sure also the UART FIFO is
empty, although it may time out when using flow control (as then the
FIFO may never empty, and flow control appears to be enabled here).

Johan

Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

[Balakrishna Godavarthi]

Hi Johan,

On 2019-01-09 20:08, Johan Hovold wrote:
> On Fri, Dec 21, 2018 at 05:59:47PM -0800, Matthias Kaehlcke wrote:
>> On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi 
>> wrote:
>> > wcn3990 requires a power pulse to turn ON/OFF along with
>> > regulators. Sometimes we are observing the power pulses are sent
>> > out with some time delay, due to queuing these commands. This is
>> > causing synchronization issues with chip, which intern delay the
>> > chip setup or may end up with communication issues.
>> >
>> > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > ---
>> >  drivers/bluetooth/hci_qca.c | 38 ++++++++++++++-----------------------
>> >  1 file changed, 14 insertions(+), 24 deletions(-)
>> >
>> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > index f036c8f98ea3..5a07c2370289 100644
>> > --- a/drivers/bluetooth/hci_qca.c
>> > +++ b/drivers/bluetooth/hci_qca.c
>> > @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> >  		hci_uart_set_baudrate(hu, speed);
>> >  }
>> >
>> > -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>> > +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
>> >  {
>> > -	struct hci_uart *hu = hci_get_drvdata(hdev);
>> > -	struct qca_data *qca = hu->priv;
>> > -	struct sk_buff *skb;
>> > +	int ret;
>> >
>> >  	/* These power pulses are single byte command which are sent
>> >  	 * at required baudrate to wcn3990. On wcn3990, we have an external
>> > @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>> >  	 * save power. Disabling hardware flow control is mandatory while
>> >  	 * sending power pulses to SoC.
>> >  	 */
>> > -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
>> > -
>> > -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
>> > -	if (!skb)
>> > -		return -ENOMEM;
>> > -
>> > +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
>> >  	hci_uart_set_flow_control(hu, true);
>> > +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
>> > +	if (ret < 0) {
>> > +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
>> > +			   cmd);
>> > +		return ret;
>> > +	}
>> >
>> > -	skb_put_u8(skb, cmd);
>> > -	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
>> > -
>> > -	skb_queue_tail(&qca->txq, skb);
>> > -	hci_uart_tx_wakeup(hu);
>> > +	serdev_device_wait_until_sent(hu->serdev, 0);
> 
> Again, do you really want to wait indefinitely here?
> 
[Bala]: these commands are mandatory to turn ON or OFF the chip.
         so blocking to the max time is required.
         these commands are sent during the BT chip ON & OFF.
         in the latest series, i have flushed the uart before sending 
this commands
         so the uart FIFO(as just opened the port before calling this 
function) or the circular
         buffer will be empty and also i am disabling the flow control 
too.
        https://patchwork.kernel.org/patch/10744435/

>> serdev_device_wait_until_sent() might only guarantee that the UART
>> circular buffer is empty (see
>> https://elixir.bootlin.com/linux/v4.19/source/drivers/tty/tty_ioctl.c#L225),
>> not that the data has actually sent (e.g. it might be sitting in
>> the UART FIFO).
> 
> For serial core, uart_wait_until_sent() makes sure also the UART FIFO 
> is
> empty, although it may time out when using flow control (as then the
> FIFO may never empty, and flow control appears to be enabled here).
> 
> Johan


[Bala]: Pls refer the latest patch series 
https://patchwork.kernel.org/patch/10744435/
        where i am flushing the circular buffer before calling the 
qca_send_power_pulse().
        this how the flow

        1. open serial port (gurantess that UART FIFO is empty)
        2. flush the circular buffer
        3. disable the flow control.
        4. send the command byte.
        5. wait for the circular buffer is empty.

        the above is one time process,

-- 
Regards
Balakrishna.

Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

[Matthias Kaehlcke]

On Thu, Jan 10, 2019 at 08:18:37PM +0530, Balakrishna Godavarthi wrote:
> Hi Johan,
> 
> On 2019-01-09 20:08, Johan Hovold wrote:
> > On Fri, Dec 21, 2018 at 05:59:47PM -0800, Matthias Kaehlcke wrote:
> > > On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi
> > > wrote:
> > > > wcn3990 requires a power pulse to turn ON/OFF along with
> > > > regulators. Sometimes we are observing the power pulses are sent
> > > > out with some time delay, due to queuing these commands. This is
> > > > causing synchronization issues with chip, which intern delay the
> > > > chip setup or may end up with communication issues.
> > > >
> > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > > ---
> > > >  drivers/bluetooth/hci_qca.c | 38 ++++++++++++++-----------------------
> > > >  1 file changed, 14 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > > index f036c8f98ea3..5a07c2370289 100644
> > > > --- a/drivers/bluetooth/hci_qca.c
> > > > +++ b/drivers/bluetooth/hci_qca.c
> > > > @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
> > > >  		hci_uart_set_baudrate(hu, speed);
> > > >  }
> > > >
> > > > -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
> > > > +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
> > > >  {
> > > > -	struct hci_uart *hu = hci_get_drvdata(hdev);
> > > > -	struct qca_data *qca = hu->priv;
> > > > -	struct sk_buff *skb;
> > > > +	int ret;
> > > >
> > > >  	/* These power pulses are single byte command which are sent
> > > >  	 * at required baudrate to wcn3990. On wcn3990, we have an external
> > > > @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
> > > >  	 * save power. Disabling hardware flow control is mandatory while
> > > >  	 * sending power pulses to SoC.
> > > >  	 */
> > > > -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
> > > > -
> > > > -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
> > > > -	if (!skb)
> > > > -		return -ENOMEM;
> > > > -
> > > > +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
> > > >  	hci_uart_set_flow_control(hu, true);
> > > > +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
> > > > +	if (ret < 0) {
> > > > +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
> > > > +			   cmd);
> > > > +		return ret;
> > > > +	}
> > > >
> > > > -	skb_put_u8(skb, cmd);
> > > > -	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> > > > -
> > > > -	skb_queue_tail(&qca->txq, skb);
> > > > -	hci_uart_tx_wakeup(hu);
> > > > +	serdev_device_wait_until_sent(hu->serdev, 0);
> > 
> > Again, do you really want to wait indefinitely here?
> > 
> [Bala]: these commands are mandatory to turn ON or OFF the chip.
>         so blocking to the max time is required.
>         these commands are sent during the BT chip ON & OFF.
>         in the latest series, i have flushed the uart before sending this
> commands
>         so the uart FIFO(as just opened the port before calling this
> function) or the circular
>         buffer will be empty and also i am disabling the flow control too.
>        https://patchwork.kernel.org/patch/10744435/

The commands may be mandatory for switching the chip on or off, but
what is better if there is a problem with sending them (e.g. a buggy
UART driver):

1. wait a reasonable time, report an error
2. wait forever

?

If the single byte command couldn't be sent after a few milliseconds,
it likely never will, waiting forever doesn't fix that. An error
report at least provides some information about the problem and the
driver is in a not-hanging state.

Cheers

Matthias

Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

[Balakrishna Godavarthi]

On 2019-01-11 06:25, Matthias Kaehlcke wrote:
> On Thu, Jan 10, 2019 at 08:18:37PM +0530, Balakrishna Godavarthi wrote:
>> Hi Johan,
>> 
>> On 2019-01-09 20:08, Johan Hovold wrote:
>> > On Fri, Dec 21, 2018 at 05:59:47PM -0800, Matthias Kaehlcke wrote:
>> > > On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi
>> > > wrote:
>> > > > wcn3990 requires a power pulse to turn ON/OFF along with
>> > > > regulators. Sometimes we are observing the power pulses are sent
>> > > > out with some time delay, due to queuing these commands. This is
>> > > > causing synchronization issues with chip, which intern delay the
>> > > > chip setup or may end up with communication issues.
>> > > >
>> > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > > > ---
>> > > >  drivers/bluetooth/hci_qca.c | 38 ++++++++++++++-----------------------
>> > > >  1 file changed, 14 insertions(+), 24 deletions(-)
>> > > >
>> > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > > index f036c8f98ea3..5a07c2370289 100644
>> > > > --- a/drivers/bluetooth/hci_qca.c
>> > > > +++ b/drivers/bluetooth/hci_qca.c
>> > > > @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> > > >  		hci_uart_set_baudrate(hu, speed);
>> > > >  }
>> > > >
>> > > > -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>> > > > +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
>> > > >  {
>> > > > -	struct hci_uart *hu = hci_get_drvdata(hdev);
>> > > > -	struct qca_data *qca = hu->priv;
>> > > > -	struct sk_buff *skb;
>> > > > +	int ret;
>> > > >
>> > > >  	/* These power pulses are single byte command which are sent
>> > > >  	 * at required baudrate to wcn3990. On wcn3990, we have an external
>> > > > @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>> > > >  	 * save power. Disabling hardware flow control is mandatory while
>> > > >  	 * sending power pulses to SoC.
>> > > >  	 */
>> > > > -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
>> > > > -
>> > > > -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
>> > > > -	if (!skb)
>> > > > -		return -ENOMEM;
>> > > > -
>> > > > +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
>> > > >  	hci_uart_set_flow_control(hu, true);
>> > > > +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
>> > > > +	if (ret < 0) {
>> > > > +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
>> > > > +			   cmd);
>> > > > +		return ret;
>> > > > +	}
>> > > >
>> > > > -	skb_put_u8(skb, cmd);
>> > > > -	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
>> > > > -
>> > > > -	skb_queue_tail(&qca->txq, skb);
>> > > > -	hci_uart_tx_wakeup(hu);
>> > > > +	serdev_device_wait_until_sent(hu->serdev, 0);
>> >
>> > Again, do you really want to wait indefinitely here?
>> >
>> [Bala]: these commands are mandatory to turn ON or OFF the chip.
>>         so blocking to the max time is required.
>>         these commands are sent during the BT chip ON & OFF.
>>         in the latest series, i have flushed the uart before sending 
>> this
>> commands
>>         so the uart FIFO(as just opened the port before calling this
>> function) or the circular
>>         buffer will be empty and also i am disabling the flow control 
>> too.
>>        https://patchwork.kernel.org/patch/10744435/
> 
> The commands may be mandatory for switching the chip on or off, but
> what is better if there is a problem with sending them (e.g. a buggy
> UART driver):
> 
> 1. wait a reasonable time, report an error
> 2. wait forever
> 
> ?
> 
> If the single byte command couldn't be sent after a few milliseconds,
> it likely never will, waiting forever doesn't fix that. An error
> report at least provides some information about the problem and the
> driver is in a not-hanging state.
> 
> Cheers
> 
> Matthias

[Bala]: will update this with a bound TIMEOUT value. But 
wait_until_sent() is void return
         type how could we know that the data is sent out on the lines.


-- 
Regards
Balakrishna.

Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

[Matthias Kaehlcke]

On Fri, Jan 11, 2019 at 08:02:00PM +0530, Balakrishna Godavarthi wrote:
> On 2019-01-11 06:25, Matthias Kaehlcke wrote:
> > On Thu, Jan 10, 2019 at 08:18:37PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Johan,
> > > 
> > > On 2019-01-09 20:08, Johan Hovold wrote:
> > > > On Fri, Dec 21, 2018 at 05:59:47PM -0800, Matthias Kaehlcke wrote:
> > > > > On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi
> > > > > wrote:
> > > > > > wcn3990 requires a power pulse to turn ON/OFF along with
> > > > > > regulators. Sometimes we are observing the power pulses are sent
> > > > > > out with some time delay, due to queuing these commands. This is
> > > > > > causing synchronization issues with chip, which intern delay the
> > > > > > chip setup or may end up with communication issues.
> > > > > >
> > > > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > > > > ---
> > > > > >  drivers/bluetooth/hci_qca.c | 38 ++++++++++++++-----------------------
> > > > > >  1 file changed, 14 insertions(+), 24 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > > > > index f036c8f98ea3..5a07c2370289 100644
> > > > > > --- a/drivers/bluetooth/hci_qca.c
> > > > > > +++ b/drivers/bluetooth/hci_qca.c
> > > > > > @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
> > > > > >  		hci_uart_set_baudrate(hu, speed);
> > > > > >  }
> > > > > >
> > > > > > -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
> > > > > > +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
> > > > > >  {
> > > > > > -	struct hci_uart *hu = hci_get_drvdata(hdev);
> > > > > > -	struct qca_data *qca = hu->priv;
> > > > > > -	struct sk_buff *skb;
> > > > > > +	int ret;
> > > > > >
> > > > > >  	/* These power pulses are single byte command which are sent
> > > > > >  	 * at required baudrate to wcn3990. On wcn3990, we have an external
> > > > > > @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
> > > > > >  	 * save power. Disabling hardware flow control is mandatory while
> > > > > >  	 * sending power pulses to SoC.
> > > > > >  	 */
> > > > > > -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
> > > > > > -
> > > > > > -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
> > > > > > -	if (!skb)
> > > > > > -		return -ENOMEM;
> > > > > > -
> > > > > > +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
> > > > > >  	hci_uart_set_flow_control(hu, true);
> > > > > > +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
> > > > > > +	if (ret < 0) {
> > > > > > +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
> > > > > > +			   cmd);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > >
> > > > > > -	skb_put_u8(skb, cmd);
> > > > > > -	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> > > > > > -
> > > > > > -	skb_queue_tail(&qca->txq, skb);
> > > > > > -	hci_uart_tx_wakeup(hu);
> > > > > > +	serdev_device_wait_until_sent(hu->serdev, 0);
> > > >
> > > > Again, do you really want to wait indefinitely here?
> > > >
> > > [Bala]: these commands are mandatory to turn ON or OFF the chip.
> > >         so blocking to the max time is required.
> > >         these commands are sent during the BT chip ON & OFF.
> > >         in the latest series, i have flushed the uart before sending
> > > this
> > > commands
> > >         so the uart FIFO(as just opened the port before calling this
> > > function) or the circular
> > >         buffer will be empty and also i am disabling the flow
> > > control too.
> > >        https://patchwork.kernel.org/patch/10744435/
> > 
> > The commands may be mandatory for switching the chip on or off, but
> > what is better if there is a problem with sending them (e.g. a buggy
> > UART driver):
> > 
> > 1. wait a reasonable time, report an error
> > 2. wait forever
> > 
> > ?
> > 
> > If the single byte command couldn't be sent after a few milliseconds,
> > it likely never will, waiting forever doesn't fix that. An error
> > report at least provides some information about the problem and the
> > driver is in a not-hanging state.
> > 
> > Cheers
> > 
> > Matthias
> 
> [Bala]: will update this with a bound TIMEOUT value. But wait_until_sent()
> is void return
>         type how could we know that the data is sent out on the lines.

Good point, I didn't check and expected it to return an error. If you
feel really motivated and have maintainer support you could possibly
change the API, however it seems this would be a somewhat larger
change.

I guess the next best thing to do is to proceed as if all data was
sent and if there was a problem it will likely manifest through
another error (especially for the ON pulse), which still seems better
than a hanging driver.

Cheers

Matthias

Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

[Balakrishna Godavarthi]

Hi Matthias,

On 2019-01-12 05:08, Matthias Kaehlcke wrote:
> On Fri, Jan 11, 2019 at 08:02:00PM +0530, Balakrishna Godavarthi wrote:
>> On 2019-01-11 06:25, Matthias Kaehlcke wrote:
>> > On Thu, Jan 10, 2019 at 08:18:37PM +0530, Balakrishna Godavarthi wrote:
>> > > Hi Johan,
>> > >
>> > > On 2019-01-09 20:08, Johan Hovold wrote:
>> > > > On Fri, Dec 21, 2018 at 05:59:47PM -0800, Matthias Kaehlcke wrote:
>> > > > > On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi
>> > > > > wrote:
>> > > > > > wcn3990 requires a power pulse to turn ON/OFF along with
>> > > > > > regulators. Sometimes we are observing the power pulses are sent
>> > > > > > out with some time delay, due to queuing these commands. This is
>> > > > > > causing synchronization issues with chip, which intern delay the
>> > > > > > chip setup or may end up with communication issues.
>> > > > > >
>> > > > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > > > > > ---
>> > > > > >  drivers/bluetooth/hci_qca.c | 38 ++++++++++++++-----------------------
>> > > > > >  1 file changed, 14 insertions(+), 24 deletions(-)
>> > > > > >
>> > > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > > > > index f036c8f98ea3..5a07c2370289 100644
>> > > > > > --- a/drivers/bluetooth/hci_qca.c
>> > > > > > +++ b/drivers/bluetooth/hci_qca.c
>> > > > > > @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> > > > > >  		hci_uart_set_baudrate(hu, speed);
>> > > > > >  }
>> > > > > >
>> > > > > > -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>> > > > > > +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
>> > > > > >  {
>> > > > > > -	struct hci_uart *hu = hci_get_drvdata(hdev);
>> > > > > > -	struct qca_data *qca = hu->priv;
>> > > > > > -	struct sk_buff *skb;
>> > > > > > +	int ret;
>> > > > > >
>> > > > > >  	/* These power pulses are single byte command which are sent
>> > > > > >  	 * at required baudrate to wcn3990. On wcn3990, we have an external
>> > > > > > @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>> > > > > >  	 * save power. Disabling hardware flow control is mandatory while
>> > > > > >  	 * sending power pulses to SoC.
>> > > > > >  	 */
>> > > > > > -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
>> > > > > > -
>> > > > > > -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
>> > > > > > -	if (!skb)
>> > > > > > -		return -ENOMEM;
>> > > > > > -
>> > > > > > +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
>> > > > > >  	hci_uart_set_flow_control(hu, true);
>> > > > > > +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
>> > > > > > +	if (ret < 0) {
>> > > > > > +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
>> > > > > > +			   cmd);
>> > > > > > +		return ret;
>> > > > > > +	}
>> > > > > >
>> > > > > > -	skb_put_u8(skb, cmd);
>> > > > > > -	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
>> > > > > > -
>> > > > > > -	skb_queue_tail(&qca->txq, skb);
>> > > > > > -	hci_uart_tx_wakeup(hu);
>> > > > > > +	serdev_device_wait_until_sent(hu->serdev, 0);
>> > > >
>> > > > Again, do you really want to wait indefinitely here?
>> > > >
>> > > [Bala]: these commands are mandatory to turn ON or OFF the chip.
>> > >         so blocking to the max time is required.
>> > >         these commands are sent during the BT chip ON & OFF.
>> > >         in the latest series, i have flushed the uart before sending
>> > > this
>> > > commands
>> > >         so the uart FIFO(as just opened the port before calling this
>> > > function) or the circular
>> > >         buffer will be empty and also i am disabling the flow
>> > > control too.
>> > >        https://patchwork.kernel.org/patch/10744435/
>> >
>> > The commands may be mandatory for switching the chip on or off, but
>> > what is better if there is a problem with sending them (e.g. a buggy
>> > UART driver):
>> >
>> > 1. wait a reasonable time, report an error
>> > 2. wait forever
>> >
>> > ?
>> >
>> > If the single byte command couldn't be sent after a few milliseconds,
>> > it likely never will, waiting forever doesn't fix that. An error
>> > report at least provides some information about the problem and the
>> > driver is in a not-hanging state.
>> >
>> > Cheers
>> >
>> > Matthias
>> 
>> [Bala]: will update this with a bound TIMEOUT value. But 
>> wait_until_sent()
>> is void return
>>         type how could we know that the data is sent out on the lines.
> 
> Good point, I didn't check and expected it to return an error. If you
> feel really motivated and have maintainer support you could possibly
> change the API, however it seems this would be a somewhat larger
> change.
> 
> I guess the next best thing to do is to proceed as if all data was
> sent and if there was a problem it will likely manifest through
> another error (especially for the ON pulse), which still seems better
> than a hanging driver.
> 
> Cheers
> 
> Matthias

[Bala]: sure, will add the timeout to one second and if data didn't sent 
to the lines anyways
         we will get an version command timeouts errors.

-- 
Regards
Balakrishna.

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

[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 <bgodavar@codeaurora.org>
Tested-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 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_3200000)
@@ -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

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

[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 <bgodavar@codeaurora.org>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  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_3200000)
> @@ -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 on this.

3) BT controller sends data after baudrate change

  a few ms after a baudrate change the BT controller sends data
  (4, 255, 2, 146, 1, 4, 14, 4, 1, 0, 0, 0) with the new baudrate

  - dunno what the data stands for, but the BT stack/driver appears to
    be fine with it, as long as the host UART operates at the new
    baudrate when the data is received.

  - if the data is received before the baudrate of the host UART is
    changes we see 'frame reassembly' errors


In summary, I think it should be feasible to guarantee that the
baudrate change of the host UART is always done after the controller
changed it's baudrate, however we can't guarantee at the same time
that the baudrate change of the host controller is completed before
the BT controller sends its 'response'.

Using the RTS signal seems a reasonable way to delay the controller
data until the host is ready, the only thing I don't like too much
is that in this patch set we currently have two mechanisms to
suppress/delay unwanted data. Unfortunately the RTS method isn't
effective at initialization time.

Not the scope of this patch set, but I really dislike the 300 ms delay
(BAUDRATE_SETTLE_TIMEOUT_MS) in qca_set_baudrate(), and wonder if it
is actually needed (I seriously doubt that it takes the BT controller
300 ms to change its baudrate). I guess it's more a combination of what I
described above in 1), once we are done with this series I might try
to improve this, unless somebody is really, really convinced that such
a gigantic delay is actually needed.

Cheers

Matthias

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

[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 <bgodavar@codeaurora.org>
>> Tested-by: Matthias Kaehlcke <mka@chromium.org>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>  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_3200000)
>> @@ -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 on this.
> 
> 3) BT controller sends data after baudrate change
> 
>   a few ms after a baudrate change the BT controller sends data
>   (4, 255, 2, 146, 1, 4, 14, 4, 1, 0, 0, 0) with the new baudrate
> 
>   - dunno what the data stands for, but the BT stack/driver appears to
>     be fine with it, as long as the host UART operates at the new
>     baudrate when the data is received.
> 
>   - if the data is received before the baudrate of the host UART is
>     changes we see 'frame reassembly' errors
> 
> 
[Bala]: the data is an vendor specific event and command complete event,
          4, 255, 2, 146, 1, : vendor specific event
          4, 14, 4, 1, 0, 0, 0: command complete event.

> In summary, I think it should be feasible to guarantee that the
> baudrate change of the host UART is always done after the controller
> changed it's baudrate, however we can't guarantee at the same time
> that the baudrate change of the host controller is completed before
> the BT controller sends its 'response'.
> 
> Using the RTS signal seems a reasonable way to delay the controller
> data until the host is ready, the only thing I don't like too much
> is that in this patch set we currently have two mechanisms to
> suppress/delay unwanted data. Unfortunately the RTS method isn't
> effective at initialization time.
> 
> Not the scope of this patch set, but I really dislike the 300 ms delay
> (BAUDRATE_SETTLE_TIMEOUT_MS) in qca_set_baudrate(), and wonder if it
> is actually needed (I seriously doubt that it takes the BT controller
> 300 ms to change its baudrate). I guess it's more a combination of what 
> I
> described above in 1), once we are done with this series I might try
> to improve this, unless somebody is really, really convinced that such
> a gigantic delay is actually needed.
> 
[Bala]:  Thanks for detail analysis.
         even i feel the same whether is it really required to have an 
delay of 300ms.
         But during our testing we found the it depends on the controller 
clock settling time.
         all observations are less than 100 ms. will update this change 
in separate patch series.

> Cheers
> 
> Matthias

-- 
Regards
Balakrishna.

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

[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 <bgodavar@codeaurora.org>
> > > Tested-by: Matthias Kaehlcke <mka@chromium.org>
> > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > >  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_3200000)
> > > @@ -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 on this.
> > 
> > 3) BT controller sends data after baudrate change
> > 
> >   a few ms after a baudrate change the BT controller sends data
> >   (4, 255, 2, 146, 1, 4, 14, 4, 1, 0, 0, 0) with the new baudrate
> > 
> >   - dunno what the data stands for, but the BT stack/driver appears to
> >     be fine with it, as long as the host UART operates at the new
> >     baudrate when the data is received.
> > 
> >   - if the data is received before the baudrate of the host UART is
> >     changes we see 'frame reassembly' errors
> > 
> > 
> [Bala]: the data is an vendor specific event and command complete event,
>          4, 255, 2, 146, 1, : vendor specific event
>          4, 14, 4, 1, 0, 0, 0: command complete event.

Thanks!

> > In summary, I think it should be feasible to guarantee that the
> > baudrate change of the host UART is always done after the controller
> > changed it's baudrate, however we can't guarantee at the same time
> > that the baudrate change of the host controller is completed before
> > the BT controller sends its 'response'.
> > 
> > Using the RTS signal seems a reasonable way to delay the controller
> > data until the host is ready, the only thing I don't like too much
> > is that in this patch set we currently have two mechanisms to
> > suppress/delay unwanted data. Unfortunately the RTS method isn't
> > effective at initialization time.
> > 
> > Not the scope of this patch set, but I really dislike the 300 ms delay
> > (BAUDRATE_SETTLE_TIMEOUT_MS) in qca_set_baudrate(), and wonder if it
> > is actually needed (I seriously doubt that it takes the BT controller
> > 300 ms to change its baudrate). I guess it's more a combination of what
> > I
> > described above in 1), once we are done with this series I might try
> > to improve this, unless somebody is really, really convinced that such
> > a gigantic delay is actually needed.
> > 
> [Bala]:  Thanks for detail analysis.
>         even i feel the same whether is it really required to have an delay
> of 300ms.
>         But during our testing we found the it depends on the controller
> clock settling time.
>         all observations are less than 100 ms. will update this change in
> separate patch series.

100 ms is definitely better than 300 ms if that's not really
needed. Did you see the need for a 100 ms delay with the WCN3990 or
some other QCA controller?

Cheers

Matthias

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

[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 <bgodavar@codeaurora.org>
>> > > Tested-by: Matthias Kaehlcke <mka@chromium.org>
>> > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> > > ---
>> > >  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_3200000)
>> > > @@ -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 on this.
>> >
>> > 3) BT controller sends data after baudrate change
>> >
>> >   a few ms after a baudrate change the BT controller sends data
>> >   (4, 255, 2, 146, 1, 4, 14, 4, 1, 0, 0, 0) with the new baudrate
>> >
>> >   - dunno what the data stands for, but the BT stack/driver appears to
>> >     be fine with it, as long as the host UART operates at the new
>> >     baudrate when the data is received.
>> >
>> >   - if the data is received before the baudrate of the host UART is
>> >     changes we see 'frame reassembly' errors
>> >
>> >
>> [Bala]: the data is an vendor specific event and command complete 
>> event,
>>          4, 255, 2, 146, 1, : vendor specific event
>>          4, 14, 4, 1, 0, 0, 0: command complete event.
> 
> Thanks!
> 
>> > In summary, I think it should be feasible to guarantee that the
>> > baudrate change of the host UART is always done after the controller
>> > changed it's baudrate, however we can't guarantee at the same time
>> > that the baudrate change of the host controller is completed before
>> > the BT controller sends its 'response'.
>> >
>> > Using the RTS signal seems a reasonable way to delay the controller
>> > data until the host is ready, the only thing I don't like too much
>> > is that in this patch set we currently have two mechanisms to
>> > suppress/delay unwanted data. Unfortunately the RTS method isn't
>> > effective at initialization time.
>> >
>> > Not the scope of this patch set, but I really dislike the 300 ms delay
>> > (BAUDRATE_SETTLE_TIMEOUT_MS) in qca_set_baudrate(), and wonder if it
>> > is actually needed (I seriously doubt that it takes the BT controller
>> > 300 ms to change its baudrate). I guess it's more a combination of what
>> > I
>> > described above in 1), once we are done with this series I might try
>> > to improve this, unless somebody is really, really convinced that such
>> > a gigantic delay is actually needed.
>> >
>> [Bala]:  Thanks for detail analysis.
>>         even i feel the same whether is it really required to have an 
>> delay
>> of 300ms.
>>         But during our testing we found the it depends on the 
>> controller
>> clock settling time.
>>         all observations are less than 100 ms. will update this change 
>> in
>> separate patch series.
> 
> 100 ms is definitely better than 300 ms if that's not really
> needed. Did you see the need for a 100 ms delay with the WCN3990 or
> some other QCA controller?

[Bala]: i am not sure about other controller will check that. but for 
wcn3990 we can go
         with the 100ms.

> 
> Cheers
> 
> Matthias


-- 
Regards
Balakrishna.

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

[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 <bgodavar@codeaurora.org>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  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_3200000)
> @@ -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

[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 <bgodavar@codeaurora.org>
>> Tested-by: Matthias Kaehlcke <mka@chromium.org>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>  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_3200000)
>> @@ -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

[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

[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

[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

[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

[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 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 across boards, even if they use the
same SoC.

I think it will have to be the second option.

Cheers

Matthias

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

[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 across boards, even if they use the
> same SoC.
> 
> I think it will have to be the second option.
> 
> Cheers
> 
> Matthias

[Bala]: i have tried option 2, but sill i see frame reassembly errors.
         1. disabling flow control
         2. pull RTS

         But even we have dynamic flow control, we will not have any 
issue.
         let us assume we have an dynamic flow control and RTS line is 
pulled high.
         but the during this stage for sure our FIFO we not be full, 
because this is an init sequence.

         01 48 fc 01 11(command we send and wait here for 300 ms)
         04 ff 02 92 01(command specific event)
         04 0e 04 01 00 00 00(command complete event)

        so we will only have 5 bytes to be sent. i don't think dynamic 
flow control will not active.

        I have an doubt that if HOST supports dynamic flow control, how 
would it helps BT chip it may cause the byte corruption.
        here is the scenario, if the chip is not ready to accept any data 
from the HOST where as host as lot of data to sent,
        then enabling dynamic flow control will cause an data loss 
between BT chip and HOST>

-- 
Regards
Balakrishna.

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

[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]: 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 across boards, even if they use the
> > same SoC.
> > 
> > I think it will have to be the second option.
> > 
> > Cheers
> > 
> > Matthias
> 
> [Bala]: i have tried option 2, but sill i see frame reassembly errors.
>         1. disabling flow control
>         2. pull RTS

I can reproduce this. Apparently the geni serial port doesn't raise
RTS when flow control is disabled, even when told to do so. I don't
know if this is a bug or just undefined behavior when flow control is
disabled (e.g. the port might not even have a RTS signal
(configured)).

>         But even we have dynamic flow control, we will not have any issue.
>         let us assume we have an dynamic flow control and RTS line is pulled
> high.
>         but the during this stage for sure our FIFO we not be full, because
> this is an init sequence.
> 
>         01 48 fc 01 11(command we send and wait here for 300 ms)
>         04 ff 02 92 01(command specific event)
>         04 0e 04 01 00 00 00(command complete event)
> 
>        so we will only have 5 bytes to be sent. i don't think dynamic flow
> control will not active.

Flow control will always be active unless we disable it, I think you
are referring to the status of the RTS signal.

It seems you are talking about the TX FIFO ("5 bytes to be sent"),
however that shouldn't affect RTS.

I believe Johan is referring to the case where the port/driver asserts
RTS, because it is ready to receive more data.

>        I have an doubt that if HOST supports dynamic flow control, how would
> it helps BT chip it may cause the byte corruption.
>        here is the scenario, if the chip is not ready to accept any data
> from the HOST where as host as lot of data to sent,
>        then enabling dynamic flow control will cause an data loss between BT
> chip and HOST>

Enabled flow control is supposed to prevent that situation, I imagine
you refer to disabled flow control? That is a potential problem.

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?

Thanks

Matthias

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

[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

[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

[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

[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

[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

[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

[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

[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

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

[PATCH v5 3/5] Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990

[Balakrishna Godavarthi]

During initalization of wcn3990, we observed UART is reading some
stray bytes on the Rx line. This is logging Frame reassembly errors
on the serial console. This could be because of tristate of Tx line
of wcn3990 during boot up.

[  176.929612] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84)
[  176.945734] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84)
[  176.953298] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84)
[  177.010660] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84)
[  177.067633] Bluetooth: hci_qca.c:qca_recv() hci0: Frame reassembly failed (-84)

Now we enable a flag during bootup to stop executing proto receive
function and clear it back once the initialization is done.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
Tested-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/bluetooth/hci_qca.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 1680ead6cc3d..52de0b5a0620 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -56,6 +56,7 @@
 
 /* Controller states */
 #define STATE_IN_BAND_SLEEP_ENABLED	1
+#define STATE_DISCARD_RX		2
 
 #define IBS_WAKE_RETRANS_TIMEOUT_MS	100
 #define IBS_TX_IDLE_TIMEOUT_MS		2000
@@ -511,6 +512,7 @@ static int qca_open(struct hci_uart *hu)
 		} else {
 			hu->init_speed = qcadev->init_speed;
 			hu->oper_speed = qcadev->oper_speed;
+			set_bit(STATE_DISCARD_RX, &qca->flags);
 			ret = qca_power_setup(hu, true);
 			if (ret) {
 				destroy_workqueue(qca->workqueue);
@@ -903,6 +905,13 @@ static int qca_recv(struct hci_uart *hu, const void *data, int count)
 	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
 		return -EUNATCH;
 
+	/* We discard Rx data received while device is in booting
+	 * stage, This is because of BT chip Tx line is in tristate.
+	 * Due to this we read some garbage data on UART Rx.
+	 */
+	if (test_bit(STATE_DISCARD_RX, &qca->flags))
+		return 0;
+
 	qca->rx_skb = h4_recv_buf(hu->hdev, qca->rx_skb, data, count,
 				  qca_recv_pkts, ARRAY_SIZE(qca_recv_pkts));
 	if (IS_ERR(qca->rx_skb)) {
@@ -1194,6 +1203,7 @@ static int qca_setup(struct hci_uart *hu)
 		if (ret)
 			return ret;
 
+		clear_bit(STATE_DISCARD_RX, &qca->flags);
 		ret = qca_read_soc_version(hdev, &soc_ver);
 		if (ret)
 			return ret;
@@ -1270,6 +1280,12 @@ static const struct qca_vreg_data qca_soc_data = {
 
 static void qca_power_shutdown(struct hci_uart *hu)
 {
+	struct qca_data *qca = hu->priv;
+
+	/* From this point we go into power off state. But serial port is
+	 * still open, discard all the garbage data received on the Rx line.
+	 */
+	set_bit(STATE_DISCARD_RX, &qca->flags);
 	serdev_device_write_flush(hu->serdev);
 	host_set_baudrate(hu, 2400);
 	qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v5 4/5] Bluetooth: hci_qca: Disable IBS state machine and flush Tx buffer

[Balakrishna Godavarthi]

During hci down we observed IBS sleep commands are queued in the Tx
buffer and hci_uart_write_work is sending data to the chip which is
not required as the chip is powered off. This patch will disable IBS
and flush the Tx buffer before we turn off the chip.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
 drivers/bluetooth/hci_qca.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 52de0b5a0620..cccc9a525926 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1286,6 +1286,8 @@ static void qca_power_shutdown(struct hci_uart *hu)
 	 * still open, discard all the garbage data received on the Rx line.
 	 */
 	set_bit(STATE_DISCARD_RX, &qca->flags);
+	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
+	qca_flush(hu);
 	serdev_device_write_flush(hu->serdev);
 	host_set_baudrate(hu, 2400);
 	qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v5 5/5] Bluetooth: btqca: inject command complete event during fw download

[Balakrishna Godavarthi]

Latest qualcomm chips are not sending an command complete event for
every firmware packet sent to chip. They only respond with a vendor
specific event for the last firmware packet. This optimization will
decrease the BT ON time. Due to this we are seeing a timeout error
message logs on the console during firmware download. Now we are
injecting a command complete event once we receive an vendor specific
event for the last RAM firmware packet.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
 drivers/bluetooth/btqca.c | 39 ++++++++++++++++++++++++++++++++++++++-
 drivers/bluetooth/btqca.h |  3 +++
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index ec9e03a6b778..0b533f65f652 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -144,6 +144,7 @@ static void qca_tlv_check_data(struct rome_config *config,
 		 * In case VSE is skipped, only the last segment is acked.
 		 */
 		config->dnld_mode = tlv_patch->download_mode;
+		config->dnld_type = config->dnld_mode;
 
 		BT_DBG("Total Length           : %d bytes",
 		       le32_to_cpu(tlv_patch->total_size));
@@ -264,6 +265,31 @@ static int qca_tlv_send_segment(struct hci_dev *hdev, int seg_size,
 	return err;
 }
 
+static int qca_inject_cmd_complete_event(struct hci_dev *hdev)
+{
+	struct hci_event_hdr *hdr;
+	struct hci_ev_cmd_complete *evt;
+	struct sk_buff *skb;
+
+	skb = bt_skb_alloc(sizeof(*hdr) + sizeof(*evt) + 1, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hdr = skb_put(skb, sizeof(*hdr));
+	hdr->evt = HCI_EV_CMD_COMPLETE;
+	hdr->plen = sizeof(*evt) + 1;
+
+	evt = skb_put(skb, sizeof(*evt));
+	evt->ncmd = 1;
+	evt->opcode = HCI_OP_NOP;
+
+	skb_put_u8(skb, QCA_HCI_CC_SUCCESS);
+
+	hci_skb_pkt_type(skb) = HCI_EVENT_PKT;
+
+	return hci_recv_frame(hdev, skb);
+}
+
 static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {
@@ -297,11 +323,22 @@ static int qca_download_firmware(struct hci_dev *hdev,
 		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
-			break;
+			goto out;
 
 		segment += segsize;
 	}
 
+	/* Latest qualcomm chipsets are not sending a command complete event
+	 * for every fw packet sent. They only respond with a vendor specific
+	 * event for the last packet. This optimization in the chip will
+	 * decrease the BT in initialization time. Here we will inject a command
+	 * complete event to avoid a command timeout error message.
+	 */
+	if ((config->dnld_type == ROME_SKIP_EVT_VSE_CC ||
+	    config->dnld_type == ROME_SKIP_EVT_VSE))
+		return qca_inject_cmd_complete_event(hdev);
+
+out:
 	release_firmware(fw);
 
 	return ret;
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index 0c01f375fe83..5c8fc54133e3 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -40,6 +40,8 @@
 #define QCA_WCN3990_POWERON_PULSE	0xFC
 #define QCA_WCN3990_POWEROFF_PULSE	0xC0
 
+#define QCA_HCI_CC_SUCCESS		0x00
+
 enum qca_bardrate {
 	QCA_BAUDRATE_115200 	= 0,
 	QCA_BAUDRATE_57600,
@@ -81,6 +83,7 @@ struct rome_config {
 	char fwname[64];
 	uint8_t user_baud_rate;
 	enum rome_tlv_dnld_mode dnld_mode;
+	enum rome_tlv_dnld_mode dnld_type;
 };
 
 struct edl_event_hdr {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project