Bug fixes for Qualcomm BT chip wcn3990

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. Observed stray bytes observed after hdev->shutdown.
   Qualcomm bt chip wcn3990, will turn of regulators when hci down.
   This is via hdev->shutdown(), once hdev->shutdown() is called,
   it is not required to send reset command.

   * Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag.

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

Note: 
   This patches are on top of below patches posted in patch work for hci_qca
   https://lore.kernel.org/patchwork/patch/1005631/
   https://lore.kernel.org/patchwork/patch/1004372/
    

Balakrishna Godavarthi (4):
  Bluetooth: hci_qca: use wait_until_sent() for power pulses
  Bluetooth: hci_qca: Pull RTS line high for baudrate change command
  Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag
  Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990

 drivers/bluetooth/hci_qca.c | 66 +++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 25 deletions(-)

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

[PATCH v1 1/4] 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 | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index f72ded4ec9ae..051f081d1835 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1016,8 +1016,7 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
 static int qca_send_power_pulse(struct hci_dev *hdev, 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
@@ -1030,18 +1029,14 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
 	 * 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;
-
 	hci_uart_set_flow_control(hu, true);
+	ret = serdev_device_write(hu->serdev, &cmd, sizeof(cmd), 0);
+	if (ret < 0) {
+		bt_dev_err(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);
@@ -1283,7 +1278,8 @@ static void qca_power_shutdown(struct hci_uart *hu)
 
 	host_set_baudrate(hu, 2400);
 	hci_uart_set_flow_control(hu, true);
-	serdev_device_write_buf(serdev, &cmd, sizeof(cmd));
+	serdev_device_write(serdev, &cmd, sizeof(cmd), 0);
+	serdev_device_wait_until_sent(serdev, 0);
 	hci_uart_set_flow_control(hu, false);
 	qca_power_setup(hu, false);
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 2/4] Bluetooth: hci_qca: Pull RTS line high for baudrate change command

[Balakrishna Godavarthi]

This patch will pull the RTS line high instead of turning off the
flow control, while changing baudrate of host and chip.

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

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 051f081d1835..8301663f0004 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,14 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 		if (!speed)
 			return 0;
 
+		/* Pulling RTS line to high is mandatory while sending change
+		 * baudrate request to SoC and also while setting the host
+		 * baudrate.
+		 */
+		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 +1102,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

[PATCH v1 3/4] Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag

[Balakrishna Godavarthi]

During hci down we are sending reset command to chip, which
is not required for wcn3990, as hdev->shutdown() will turn off the
regulators.

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

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 8301663f0004..97b57e0f4725 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1190,6 +1190,7 @@ static int qca_setup(struct hci_uart *hu)
 		 */
 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
+		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
 		hu->hdev->shutdown = qca_power_off;
 		ret = qca_wcn3990_init(hu);
 		if (ret)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v1 4/4] Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990

[Balakrishna Godavarthi]

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

The above errors log on console due to receiving of stray bytes
when wcn3990 boot up's i.e. when during initial setup procedure.

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

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 97b57e0f4725..341f80606574 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 due multiple baudrate switch is causing
+	 * UART to read some garbage data.
+	 */
+	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)) {
@@ -1192,10 +1201,12 @@ static int qca_setup(struct hci_uart *hu)
 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
 		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
 		hu->hdev->shutdown = qca_power_off;
+
 		ret = qca_wcn3990_init(hu);
 		if (ret)
 			return ret;
 
+		clear_bit(STATE_DISCARD_RX, &qca->flags);
 		ret = qca_read_soc_version(hdev, &soc_ver);
 		if (ret)
 			return ret;
@@ -1278,8 +1289,15 @@ static void qca_power_shutdown(struct hci_uart *hu)
 	struct serdev_device *serdev = hu->serdev;
 	unsigned char cmd = QCA_WCN3990_POWEROFF_PULSE;
 
-	host_set_baudrate(hu, 2400);
+	/* From this point we go into power off state,
+	 * disable IBS and discard all the queued data.
+	 */
+	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
+	set_bit(STATE_DISCARD_RX, &qca->flags);
+	qca_flush(hu);
+
 	hci_uart_set_flow_control(hu, true);
+	host_set_baudrate(hu, 2400);
 	serdev_device_write(serdev, &cmd, sizeof(cmd), 0);
 	serdev_device_wait_until_sent(serdev, 0);
 	hci_uart_set_flow_control(hu, false);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 3/4] Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag

[Marcel Holtmann]

Hi Balakrishna,

> During hci down we are sending reset command to chip, which
> is not required for wcn3990, as hdev->shutdown() will turn off the
> regulators.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> drivers/bluetooth/hci_qca.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 8301663f0004..97b57e0f4725 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1190,6 +1190,7 @@ static int qca_setup(struct hci_uart *hu)
> 		 */
> 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
> 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
> +		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
> 		hu->hdev->shutdown = qca_power_off;
> 		ret = qca_wcn3990_init(hu);
> 		if (ret)

I am pretty certain that you didn’t want this quirk:

        /* When this quirk is set, the HCI Reset command is send when            
         * closing the transport instead of when opening it.

This quirk is for Bluetooth 1.0b devices where the HCI_Reset behavior was not clear or for devices that actually misbehave with the initial HCI_Reset.

In addition, you commit message is totally misleading. That is not what is happening with this quirk.

Regards

Marcel

Re: [PATCH v1 3/4] Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag

[Balakrishna Godavarthi]

Hi Marcel,

On 2018-11-06 18:03, Marcel Holtmann wrote:
> Hi Balakrishna,
> 
>> During hci down we are sending reset command to chip, which
>> is not required for wcn3990, as hdev->shutdown() will turn off the
>> regulators.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> drivers/bluetooth/hci_qca.c | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 8301663f0004..97b57e0f4725 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1190,6 +1190,7 @@ static int qca_setup(struct hci_uart *hu)
>> 		 */
>> 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>> 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>> +		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
>> 		hu->hdev->shutdown = qca_power_off;
>> 		ret = qca_wcn3990_init(hu);
>> 		if (ret)
> 
> I am pretty certain that you didn’t want this quirk:
> 
>         /* When this quirk is set, the HCI Reset command is send when
> 
>          * closing the transport instead of when opening it.
> 
> This quirk is for Bluetooth 1.0b devices where the HCI_Reset behavior
> was not clear or for devices that actually misbehave with the initial
> HCI_Reset.
> 
> In addition, you commit message is totally misleading. That is not
> what is happening with this quirk.
> 
> Regards
> 
> Marcel

My intention was reset command is not required when we do an hci down.
this is because of hdev->shutdown will turn off the regulators.
It is like turning off the chip. sending reset command after turning off 
the chip is not required.

I understand the usage of the quirk, will update the commit text.


-- 
Regards
Balakrishna.

Re: [PATCH v1 3/4] Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag

[Marcel Holtmann]

Hi Balakrishna,

>>> During hci down we are sending reset command to chip, which
>>> is not required for wcn3990, as hdev->shutdown() will turn off the
>>> regulators.
>>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>>> ---
>>> drivers/bluetooth/hci_qca.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>> index 8301663f0004..97b57e0f4725 100644
>>> --- a/drivers/bluetooth/hci_qca.c
>>> +++ b/drivers/bluetooth/hci_qca.c
>>> @@ -1190,6 +1190,7 @@ static int qca_setup(struct hci_uart *hu)
>>> 		 */
>>> 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>>> 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>>> +		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
>>> 		hu->hdev->shutdown = qca_power_off;
>>> 		ret = qca_wcn3990_init(hu);
>>> 		if (ret)
>> I am pretty certain that you didn’t want this quirk:
>>        /* When this quirk is set, the HCI Reset command is send when
>>         * closing the transport instead of when opening it.
>> This quirk is for Bluetooth 1.0b devices where the HCI_Reset behavior
>> was not clear or for devices that actually misbehave with the initial
>> HCI_Reset.
>> In addition, you commit message is totally misleading. That is not
>> what is happening with this quirk.
>> Regards
>> Marcel
> 
> My intention was reset command is not required when we do an hci down.
> this is because of hdev->shutdown will turn off the regulators.
> It is like turning off the chip. sending reset command after turning off the chip is not required.
> 
> I understand the usage of the quirk, will update the commit text.

you are papering over the issue. Actually hci_serdev.c:hci_uart_register_device() is the culprit with the legacy code copied over from hci_ldisc.c:hci_uart_register_dev(). I think there is no point doing all this legacy line discipline quirk handling until it is really needed. The serdev drivers are all for recent hardware.

That said, having moved over to a btuart.c approach and killed the whole hci_serdev.c thing would have been a lot better here. You will keep running in weird situations where 18 year old code keeps surprising you.

Regards

Marcel

Re: [PATCH v1 3/4] Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag

[Balakrishna Godavarthi]

Hi Marcel,

On 2018-11-06 18:32, Marcel Holtmann wrote:
> Hi Balakrishna,
> 
>>>> During hci down we are sending reset command to chip, which
>>>> is not required for wcn3990, as hdev->shutdown() will turn off the
>>>> regulators.
>>>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>>>> ---
>>>> drivers/bluetooth/hci_qca.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>> diff --git a/drivers/bluetooth/hci_qca.c 
>>>> b/drivers/bluetooth/hci_qca.c
>>>> index 8301663f0004..97b57e0f4725 100644
>>>> --- a/drivers/bluetooth/hci_qca.c
>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>> @@ -1190,6 +1190,7 @@ static int qca_setup(struct hci_uart *hu)
>>>> 		 */
>>>> 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>>>> 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>>>> +		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
>>>> 		hu->hdev->shutdown = qca_power_off;
>>>> 		ret = qca_wcn3990_init(hu);
>>>> 		if (ret)
>>> I am pretty certain that you didn’t want this quirk:
>>>        /* When this quirk is set, the HCI Reset command is send when
>>>         * closing the transport instead of when opening it.
>>> This quirk is for Bluetooth 1.0b devices where the HCI_Reset behavior
>>> was not clear or for devices that actually misbehave with the initial
>>> HCI_Reset.
>>> In addition, you commit message is totally misleading. That is not
>>> what is happening with this quirk.
>>> Regards
>>> Marcel
>> 
>> My intention was reset command is not required when we do an hci down.
>> this is because of hdev->shutdown will turn off the regulators.
>> It is like turning off the chip. sending reset command after turning 
>> off the chip is not required.
>> 
>> I understand the usage of the quirk, will update the commit text.
> 
> you are papering over the issue. Actually
> hci_serdev.c:hci_uart_register_device() is the culprit with the legacy
> code copied over from hci_ldisc.c:hci_uart_register_dev(). I think
> there is no point doing all this legacy line discipline quirk handling
> until it is really needed. The serdev drivers are all for recent
> hardware.
> 
> That said, having moved over to a btuart.c approach and killed the
> whole hci_serdev.c thing would have been a lot better here. You will
> keep running in weird situations where 18 year old code keeps
> surprising you.

[Bala]: even i feel the same. they are lot such kind of HACK's we need 
to do with current arch.
         when can we expect btuart.c merged to bt-next. i think having 
btuart will helps us to have the control of
         vendor porto's call's like in btusb.c

> 
> Regards
> 
> Marcel

-- 
Regards
Balakrishna.

Re: [PATCH v1 3/4] Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag

[Balakrishna Godavarthi]

Hi Marcel,

On 2018-11-06 18:44, Balakrishna Godavarthi wrote:
> Hi Marcel,
> 
> On 2018-11-06 18:32, Marcel Holtmann wrote:
>> Hi Balakrishna,
>> 
>>>>> During hci down we are sending reset command to chip, which
>>>>> is not required for wcn3990, as hdev->shutdown() will turn off the
>>>>> regulators.
>>>>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>>>>> ---
>>>>> drivers/bluetooth/hci_qca.c | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>> diff --git a/drivers/bluetooth/hci_qca.c 
>>>>> b/drivers/bluetooth/hci_qca.c
>>>>> index 8301663f0004..97b57e0f4725 100644
>>>>> --- a/drivers/bluetooth/hci_qca.c
>>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>>> @@ -1190,6 +1190,7 @@ static int qca_setup(struct hci_uart *hu)
>>>>> 		 */
>>>>> 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>>>>> 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>>>>> +		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
>>>>> 		hu->hdev->shutdown = qca_power_off;
>>>>> 		ret = qca_wcn3990_init(hu);
>>>>> 		if (ret)
>>>> I am pretty certain that you didn’t want this quirk:
>>>>        /* When this quirk is set, the HCI Reset command is send when
>>>>         * closing the transport instead of when opening it.
>>>> This quirk is for Bluetooth 1.0b devices where the HCI_Reset 
>>>> behavior
>>>> was not clear or for devices that actually misbehave with the 
>>>> initial
>>>> HCI_Reset.
>>>> In addition, you commit message is totally misleading. That is not
>>>> what is happening with this quirk.
>>>> Regards
>>>> Marcel
>>> 
>>> My intention was reset command is not required when we do an hci 
>>> down.
>>> this is because of hdev->shutdown will turn off the regulators.
>>> It is like turning off the chip. sending reset command after turning 
>>> off the chip is not required.
>>> 
>>> I understand the usage of the quirk, will update the commit text.
>> 
>> you are papering over the issue. Actually
>> hci_serdev.c:hci_uart_register_device() is the culprit with the legacy
>> code copied over from hci_ldisc.c:hci_uart_register_dev(). I think
>> there is no point doing all this legacy line discipline quirk handling
>> until it is really needed. The serdev drivers are all for recent
>> hardware.
>> 
>> That said, having moved over to a btuart.c approach and killed the
>> whole hci_serdev.c thing would have been a lot better here. You will
>> keep running in weird situations where 18 year old code keeps
>> surprising you.
> 
> [Bala]: even i feel the same. they are lot such kind of HACK's we need
> to do with current arch.
>         when can we expect btuart.c merged to bt-next. i think having
> btuart will helps us to have the control of
>         vendor porto's call's like in btusb.c
> 
>> 
>> Regards
>> 
>> Marcel

I need some clarification, do you expect some thing like this 
https://github.com/torvalds/linux/blob/master/drivers/bluetooth/btmtkuart.c 
for Qualcomm BT chip too.
it looks it is completely avoided hci_serdev.c interface.

-- 
Regards
Balakrishna.

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

[Matthias Kaehlcke]

On Tue, Nov 06, 2018 at 05:35:25PM +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 | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index f72ded4ec9ae..051f081d1835 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1016,8 +1016,7 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
>  static int qca_send_power_pulse(struct hci_dev *hdev, 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
> @@ -1030,18 +1029,14 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>  	 * 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;
> -
>  	hci_uart_set_flow_control(hu, true);
> +	ret = serdev_device_write(hu->serdev, &cmd, sizeof(cmd), 0);
> +	if (ret < 0) {
> +		bt_dev_err(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);

Is the delay still needed now that we wait for the pulse to be sent? I
didn't observe any problems without it in a few dozens of iterations.

> @@ -1283,7 +1278,8 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  
>  	host_set_baudrate(hu, 2400);
>  	hci_uart_set_flow_control(hu, true);
> -	serdev_device_write_buf(serdev, &cmd, sizeof(cmd));
> +	serdev_device_write(serdev, &cmd, sizeof(cmd), 0);
> +	serdev_device_wait_until_sent(serdev, 0);
>  	hci_uart_set_flow_control(hu, false);

You could call qca_send_power_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE)
instead, as an earlier patch set did before skbs were used to send the
power pulse.

You can also consider to set the baudrate in qca_send_power_pulse()
depending on the power pulse. On the plus side this would reduce a bit
of clutter in the callers of qca_send_power_pulse(), on the negative
side it would be harder to follow when baudrate changes occur (not
sure this is a problem). Up to you, just an idea.

Thanks

Matthias

Re: [PATCH v1 2/4] Bluetooth: hci_qca: Pull RTS line high for baudrate change command

[Matthias Kaehlcke]

On Tue, Nov 06, 2018 at 05:35:26PM +0530, Balakrishna Godavarthi wrote:
> This patch will pull the RTS line high instead of turning off the
> flow control, while changing baudrate of host and chip.

Please don't only describe what is changed, but also why this change
is necessary.

IIUC the BT chip honors flow control during baudrate changes, however
it sometimes sends 'garbage' which results in "Bluetooth: hci0: Frame
reassembly failed (-84)" messages.

> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
>  drivers/bluetooth/hci_qca.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 051f081d1835..8301663f0004 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,14 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  		if (!speed)
>  			return 0;
>  
> +		/* Pulling RTS line to high is mandatory while sending change
> +		 * baudrate request to SoC and also while setting the host
> +		 * baudrate.
> +		 */

Instead of just stating that this is 'mandatory' explain why it is
needed. Also better say 'Deassert RTS' instead of the more lower level
'Pulling RTS line to high'. It could be something like 'Deassert RTS
to prevent the BT controller from sending garbage during the baudrate
change.'

> +		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 +1102,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;

Tested-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH v1 3/4] Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag

[Matthias Kaehlcke]

On Tue, Nov 06, 2018 at 06:44:07PM +0530, Balakrishna Godavarthi wrote:
> Hi Marcel,
> 
> On 2018-11-06 18:32, Marcel Holtmann wrote:
> > Hi Balakrishna,
> > 
> > > > > During hci down we are sending reset command to chip, which
> > > > > is not required for wcn3990, as hdev->shutdown() will turn off the
> > > > > regulators.
> > > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > > > ---
> > > > > drivers/bluetooth/hci_qca.c | 1 +
> > > > > 1 file changed, 1 insertion(+)
> > > > > diff --git a/drivers/bluetooth/hci_qca.c
> > > > > b/drivers/bluetooth/hci_qca.c
> > > > > index 8301663f0004..97b57e0f4725 100644
> > > > > --- a/drivers/bluetooth/hci_qca.c
> > > > > +++ b/drivers/bluetooth/hci_qca.c
> > > > > @@ -1190,6 +1190,7 @@ static int qca_setup(struct hci_uart *hu)
> > > > > 		 */
> > > > > 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
> > > > > 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
> > > > > +		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);

This patch doesn't apply cleanly against bluetooth-next, looks like
you have the unrelated "Bluetooth: hci_qca: Set
HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990"
(https://lore.kernel.org/patchwork/patch/1004372/) in your tree.

> > > > > 		hu->hdev->shutdown = qca_power_off;
> > > > > 		ret = qca_wcn3990_init(hu);
> > > > > 		if (ret)
> > > > I am pretty certain that you didn’t want this quirk:
> > > >        /* When this quirk is set, the HCI Reset command is send when
> > > >         * closing the transport instead of when opening it.
> > > > This quirk is for Bluetooth 1.0b devices where the HCI_Reset behavior
> > > > was not clear or for devices that actually misbehave with the initial
> > > > HCI_Reset.
> > > > In addition, you commit message is totally misleading. That is not
> > > > what is happening with this quirk.
> > > > Regards
> > > > Marcel
> > > 
> > > My intention was reset command is not required when we do an hci down.
> > > this is because of hdev->shutdown will turn off the regulators.
> > > It is like turning off the chip. sending reset command after turning
> > > off the chip is not required.
> > > 
> > > I understand the usage of the quirk, will update the commit text.
> > 
> > you are papering over the issue. Actually
> > hci_serdev.c:hci_uart_register_device() is the culprit with the legacy
> > code copied over from hci_ldisc.c:hci_uart_register_dev(). I think
> > there is no point doing all this legacy line discipline quirk handling
> > until it is really needed. The serdev drivers are all for recent
> > hardware.
> > 
> > That said, having moved over to a btuart.c approach and killed the
> > whole hci_serdev.c thing would have been a lot better here. You will
> > keep running in weird situations where 18 year old code keeps
> > surprising you.
> 
> [Bala]: even i feel the same. they are lot such kind of HACK's we need to do
> with current arch.
>         when can we expect btuart.c merged to bt-next. i think having btuart
> will helps us to have the control of
>         vendor porto's call's like in btusb.c

btuart was initially part of the 'add support for Bluetooth on MT7622
SoC' series (https://lore.kernel.org/patchwork/patch/960806/), but was
dropped with v6
(https://lore.kernel.org/patchwork/project/lkml/list/?series=360046)
upon Marcel's request: 'Frankly I prefer to keep the btuart.c driver
for drivers that really just use H:4 as transport protocol. If the
protocol is only H:4 alike and has extra headers, then it should be a
separate driver.' (https://lore.kernel.org/patchwork/patch/960806/#1148426).

Cheers

Matthias

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

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-11-14 05:47, Matthias Kaehlcke wrote:
> On Tue, Nov 06, 2018 at 05:35:25PM +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 | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index f72ded4ec9ae..051f081d1835 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1016,8 +1016,7 @@ static inline void host_set_baudrate(struct 
>> hci_uart *hu, unsigned int speed)
>>  static int qca_send_power_pulse(struct hci_dev *hdev, 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
>> @@ -1030,18 +1029,14 @@ static int qca_send_power_pulse(struct hci_dev 
>> *hdev, u8 cmd)
>>  	 * 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;
>> -
>>  	hci_uart_set_flow_control(hu, true);
>> +	ret = serdev_device_write(hu->serdev, &cmd, sizeof(cmd), 0);
>> +	if (ret < 0) {
>> +		bt_dev_err(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);
> 
> Is the delay still needed now that we wait for the pulse to be sent? I
> didn't observe any problems without it in a few dozens of iterations.
> 
[Bala]: chip require some time to boot up
         so this delay will helps us to be in sync with the chip. for now
         we will go with this delay, if really required we can change 
100us to some where
         around 10us.

>> @@ -1283,7 +1278,8 @@ static void qca_power_shutdown(struct hci_uart 
>> *hu)
>> 
>>  	host_set_baudrate(hu, 2400);
>>  	hci_uart_set_flow_control(hu, true);
>> -	serdev_device_write_buf(serdev, &cmd, sizeof(cmd));
>> +	serdev_device_write(serdev, &cmd, sizeof(cmd), 0);
>> +	serdev_device_wait_until_sent(serdev, 0);
>>  	hci_uart_set_flow_control(hu, false);
> 
> You could call qca_send_power_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE)
> instead, as an earlier patch set did before skbs were used to send the
> power pulse.
> 

[Bala]: will update.

> You can also consider to set the baudrate in qca_send_power_pulse()
> depending on the power pulse. On the plus side this would reduce a bit
> of clutter in the callers of qca_send_power_pulse(), on the negative
> side it would be harder to follow when baudrate changes occur (not
> sure this is a problem). Up to you, just an idea.
> 
[Bala]:  moving bd change request to power_pulse has both plus & minus 
side.
          but my opinion is let us we leave qca_send_power_pulse() to be 
generic
          might be feature chip will use the same function with an 
different bd.

> Thanks
> 
> Matthias

-- 
Regards
Balakrishna.

Re: [PATCH v1 2/4] Bluetooth: hci_qca: Pull RTS line high for baudrate change command

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-11-14 07:25, Matthias Kaehlcke wrote:
> On Tue, Nov 06, 2018 at 05:35:26PM +0530, Balakrishna Godavarthi wrote:
>> This patch will pull the RTS line high instead of turning off the
>> flow control, while changing baudrate of host and chip.
> 
> Please don't only describe what is changed, but also why this change
> is necessary.
> 
> IIUC the BT chip honors flow control during baudrate changes, however
> it sometimes sends 'garbage' which results in "Bluetooth: hci0: Frame
> reassembly failed (-84)" messages.
> 

[Bala]: will update the commit text.

>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>>  drivers/bluetooth/hci_qca.c | 23 ++++++++++++-----------
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 051f081d1835..8301663f0004 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,14 @@ static int qca_set_speed(struct hci_uart *hu, 
>> enum qca_speed_type speed_type)
>>  		if (!speed)
>>  			return 0;
>> 
>> +		/* Pulling RTS line to high is mandatory while sending change
>> +		 * baudrate request to SoC and also while setting the host
>> +		 * baudrate.
>> +		 */
> 
> Instead of just stating that this is 'mandatory' explain why it is
> needed. Also better say 'Deassert RTS' instead of the more lower level
> 'Pulling RTS line to high'. It could be something like 'Deassert RTS
> to prevent the BT controller from sending garbage during the baudrate
> change.'
> 

[Bala]: will update the comment.

>> +		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 +1102,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;
> 
> Tested-by: Matthias Kaehlcke <mka@chromium.org>

Thanks for testing.

-- 
Regards
Balakrishna.

Re: [PATCH v1 3/4] Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-11-14 07:44, Matthias Kaehlcke wrote:
> On Tue, Nov 06, 2018 at 06:44:07PM +0530, Balakrishna Godavarthi wrote:
>> Hi Marcel,
>> 
>> On 2018-11-06 18:32, Marcel Holtmann wrote:
>> > Hi Balakrishna,
>> >
>> > > > > During hci down we are sending reset command to chip, which
>> > > > > is not required for wcn3990, as hdev->shutdown() will turn off the
>> > > > > regulators.
>> > > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > > > > ---
>> > > > > drivers/bluetooth/hci_qca.c | 1 +
>> > > > > 1 file changed, 1 insertion(+)
>> > > > > diff --git a/drivers/bluetooth/hci_qca.c
>> > > > > b/drivers/bluetooth/hci_qca.c
>> > > > > index 8301663f0004..97b57e0f4725 100644
>> > > > > --- a/drivers/bluetooth/hci_qca.c
>> > > > > +++ b/drivers/bluetooth/hci_qca.c
>> > > > > @@ -1190,6 +1190,7 @@ static int qca_setup(struct hci_uart *hu)
>> > > > > 		 */
>> > > > > 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>> > > > > 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>> > > > > +		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
> 
> This patch doesn't apply cleanly against bluetooth-next, looks like
> you have the unrelated "Bluetooth: hci_qca: Set
> HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990"
> (https://lore.kernel.org/patchwork/patch/1004372/) in your tree.
> 
[Bala]: this patch is on top of 
https://lore.kernel.org/patchwork/patch/1004372/.
         will remove the dependency and update the patch.

>> > > > > 		hu->hdev->shutdown = qca_power_off;
>> > > > > 		ret = qca_wcn3990_init(hu);
>> > > > > 		if (ret)
>> > > > I am pretty certain that you didn’t want this quirk:
>> > > >        /* When this quirk is set, the HCI Reset command is send when
>> > > >         * closing the transport instead of when opening it.
>> > > > This quirk is for Bluetooth 1.0b devices where the HCI_Reset behavior
>> > > > was not clear or for devices that actually misbehave with the initial
>> > > > HCI_Reset.
>> > > > In addition, you commit message is totally misleading. That is not
>> > > > what is happening with this quirk.
>> > > > Regards
>> > > > Marcel
>> > >
>> > > My intention was reset command is not required when we do an hci down.
>> > > this is because of hdev->shutdown will turn off the regulators.
>> > > It is like turning off the chip. sending reset command after turning
>> > > off the chip is not required.
>> > >
>> > > I understand the usage of the quirk, will update the commit text.
>> >
>> > you are papering over the issue. Actually
>> > hci_serdev.c:hci_uart_register_device() is the culprit with the legacy
>> > code copied over from hci_ldisc.c:hci_uart_register_dev(). I think
>> > there is no point doing all this legacy line discipline quirk handling
>> > until it is really needed. The serdev drivers are all for recent
>> > hardware.
>> >
>> > That said, having moved over to a btuart.c approach and killed the
>> > whole hci_serdev.c thing would have been a lot better here. You will
>> > keep running in weird situations where 18 year old code keeps
>> > surprising you.
>> 
>> [Bala]: even i feel the same. they are lot such kind of HACK's we need 
>> to do
>> with current arch.
>>         when can we expect btuart.c merged to bt-next. i think having 
>> btuart
>> will helps us to have the control of
>>         vendor porto's call's like in btusb.c
> 
> btuart was initially part of the 'add support for Bluetooth on MT7622
> SoC' series (https://lore.kernel.org/patchwork/patch/960806/), but was
> dropped with v6
> (https://lore.kernel.org/patchwork/project/lkml/list/?series=360046)
> upon Marcel's request: 'Frankly I prefer to keep the btuart.c driver
> for drivers that really just use H:4 as transport protocol. If the
> protocol is only H:4 alike and has extra headers, then it should be a
> separate driver.' 
> (https://lore.kernel.org/patchwork/patch/960806/#1148426).
> 
> Cheers
> 
> Matthias

[Bala]: currently our main moto is to make this driver to be stable for 
both rome and wcn3990.
         we will look into working on btuart.c type driver for future 
qualcomm BT devices.

-- 
Regards
Balakrishna.

Re: [PATCH v1 3/4] Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag

[Marcel Holtmann]

Hi Balakrishna,

>>>>>> During hci down we are sending reset command to chip, which
>>>>>> is not required for wcn3990, as hdev->shutdown() will turn off the
>>>>>> regulators.
>>>>>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>>>>>> ---
>>>>>> drivers/bluetooth/hci_qca.c | 1 +
>>>>>> 1 file changed, 1 insertion(+)
>>>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>>>> index 8301663f0004..97b57e0f4725 100644
>>>>>> --- a/drivers/bluetooth/hci_qca.c
>>>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>>>> @@ -1190,6 +1190,7 @@ static int qca_setup(struct hci_uart *hu)
>>>>>> 		 */
>>>>>> 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>>>>>> 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>>>>>> +		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
>>>>>> 		hu->hdev->shutdown = qca_power_off;
>>>>>> 		ret = qca_wcn3990_init(hu);
>>>>>> 		if (ret)
>>>>> I am pretty certain that you didn’t want this quirk:
>>>>>       /* When this quirk is set, the HCI Reset command is send when
>>>>>        * closing the transport instead of when opening it.
>>>>> This quirk is for Bluetooth 1.0b devices where the HCI_Reset behavior
>>>>> was not clear or for devices that actually misbehave with the initial
>>>>> HCI_Reset.
>>>>> In addition, you commit message is totally misleading. That is not
>>>>> what is happening with this quirk.
>>>>> Regards
>>>>> Marcel
>>>> My intention was reset command is not required when we do an hci down.
>>>> this is because of hdev->shutdown will turn off the regulators.
>>>> It is like turning off the chip. sending reset command after turning off the chip is not required.
>>>> I understand the usage of the quirk, will update the commit text.
>>> you are papering over the issue. Actually
>>> hci_serdev.c:hci_uart_register_device() is the culprit with the legacy
>>> code copied over from hci_ldisc.c:hci_uart_register_dev(). I think
>>> there is no point doing all this legacy line discipline quirk handling
>>> until it is really needed. The serdev drivers are all for recent
>>> hardware.
>>> That said, having moved over to a btuart.c approach and killed the
>>> whole hci_serdev.c thing would have been a lot better here. You will
>>> keep running in weird situations where 18 year old code keeps
>>> surprising you.
>> [Bala]: even i feel the same. they are lot such kind of HACK's we need
>> to do with current arch.
>>        when can we expect btuart.c merged to bt-next. i think having
>> btuart will helps us to have the control of
>>        vendor porto's call's like in btusb.c
>>> Regards
>>> Marcel
> 
> I need some clarification, do you expect some thing like this https://github.com/torvalds/linux/blob/master/drivers/bluetooth/btmtkuart.c for Qualcomm BT chip too.
> it looks it is completely avoided hci_serdev.c interface.

you tell me actually. Are you using the H:4 transport or do you have an extra protocol layer / framing below it. If you do, then use your own driver, but if the transport is H:4 with vendor packets and vendor setup, then btuart.c (which is not yet upstream) should be your target.

For the MTK hardware it was obvious that it was better served as a separate driver. For QCA serial it really depends on how much extra protocol you have to run. So this might be an exercise in trying QCA serial as a separate driver and then go from there.

It is clear that the baggage from hci_ldisc.c etc is in the way for serdev based systems.

Regards

Marcel

Re: [PATCH v1 3/4] Bluetooth: hci_qca: clear HCI_QUIRK_RESET_ON_CLOSE flag

[Balakrishna Godavarthi]

Hi Marcel,

On 2018-11-14 13:18, Marcel Holtmann wrote:
> Hi Balakrishna,
> 
>>>>>>> During hci down we are sending reset command to chip, which
>>>>>>> is not required for wcn3990, as hdev->shutdown() will turn off 
>>>>>>> the
>>>>>>> regulators.
>>>>>>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>>>>>>> ---
>>>>>>> drivers/bluetooth/hci_qca.c | 1 +
>>>>>>> 1 file changed, 1 insertion(+)
>>>>>>> diff --git a/drivers/bluetooth/hci_qca.c 
>>>>>>> b/drivers/bluetooth/hci_qca.c
>>>>>>> index 8301663f0004..97b57e0f4725 100644
>>>>>>> --- a/drivers/bluetooth/hci_qca.c
>>>>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>>>>> @@ -1190,6 +1190,7 @@ static int qca_setup(struct hci_uart *hu)
>>>>>>> 		 */
>>>>>>> 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>>>>>>> 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>>>>>>> +		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
>>>>>>> 		hu->hdev->shutdown = qca_power_off;
>>>>>>> 		ret = qca_wcn3990_init(hu);
>>>>>>> 		if (ret)
>>>>>> I am pretty certain that you didn’t want this quirk:
>>>>>>       /* When this quirk is set, the HCI Reset command is send 
>>>>>> when
>>>>>>        * closing the transport instead of when opening it.
>>>>>> This quirk is for Bluetooth 1.0b devices where the HCI_Reset 
>>>>>> behavior
>>>>>> was not clear or for devices that actually misbehave with the 
>>>>>> initial
>>>>>> HCI_Reset.
>>>>>> In addition, you commit message is totally misleading. That is not
>>>>>> what is happening with this quirk.
>>>>>> Regards
>>>>>> Marcel
>>>>> My intention was reset command is not required when we do an hci 
>>>>> down.
>>>>> this is because of hdev->shutdown will turn off the regulators.
>>>>> It is like turning off the chip. sending reset command after 
>>>>> turning off the chip is not required.
>>>>> I understand the usage of the quirk, will update the commit text.
>>>> you are papering over the issue. Actually
>>>> hci_serdev.c:hci_uart_register_device() is the culprit with the 
>>>> legacy
>>>> code copied over from hci_ldisc.c:hci_uart_register_dev(). I think
>>>> there is no point doing all this legacy line discipline quirk 
>>>> handling
>>>> until it is really needed. The serdev drivers are all for recent
>>>> hardware.
>>>> That said, having moved over to a btuart.c approach and killed the
>>>> whole hci_serdev.c thing would have been a lot better here. You will
>>>> keep running in weird situations where 18 year old code keeps
>>>> surprising you.
>>> [Bala]: even i feel the same. they are lot such kind of HACK's we 
>>> need
>>> to do with current arch.
>>>        when can we expect btuart.c merged to bt-next. i think having
>>> btuart will helps us to have the control of
>>>        vendor porto's call's like in btusb.c
>>>> Regards
>>>> Marcel
>> 
>> I need some clarification, do you expect some thing like this 
>> https://github.com/torvalds/linux/blob/master/drivers/bluetooth/btmtkuart.c 
>> for Qualcomm BT chip too.
>> it looks it is completely avoided hci_serdev.c interface.
> 
> you tell me actually. Are you using the H:4 transport or do you have
> an extra protocol layer / framing below it. If you do, then use your
> own driver, but if the transport is H:4 with vendor packets and vendor
> setup, then btuart.c (which is not yet upstream) should be your
> target.
> 
> For the MTK hardware it was obvious that it was better served as a
> separate driver. For QCA serial it really depends on how much extra
> protocol you have to run. So this might be an exercise in trying QCA
> serial as a separate driver and then go from there.
> 
> It is clear that the baggage from hci_ldisc.c etc is in the way for
> serdev based systems.
> 
> Regards
> 
> Marcel

[Bala]:
     From my experience of serdev and ldisc, I prefer to go as MTK, here 
are the reasons
     1.If btuart maintains same generic way as hci_serdev, hardware 
vendors will not have control on of
       port opening and close. Because qca want to close port when we do 
hci down where as hci_serdev it is not possible to do.
       This is an important feature for power saving of the device.
    2. I think btuart will have common recv_handle to handle all the 
received data irrespective to the vendors.
       but in our case,  we will receive debug logs as ACL packet which 
is not possible to bypass as diagnostic packet.

      The same with the susb system restart events too. So, I feel that 
having an different driver as same as MTK will help us.

-- 
Regards
Balakrishna.

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

[Johan Hovold]

On Tue, Nov 06, 2018 at 05:35:25PM +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 | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index f72ded4ec9ae..051f081d1835 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1016,8 +1016,7 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
>  static int qca_send_power_pulse(struct hci_dev *hdev, 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
> @@ -1030,18 +1029,14 @@ static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>  	 * 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;
> -
>  	hci_uart_set_flow_control(hu, true);
> +	ret = serdev_device_write(hu->serdev, &cmd, sizeof(cmd), 0);

Don't you want a non-zero timeout here? Otherwise you might as well call
serdev_device_write() directly.

Also have you made sure that serdev_device_write_wakeup() is called in
the drivers write-wakeup callback as serdev_device_write() requires?

See this series in case what you really wanted was to wait indefinitely
(you can use MAX_SCHEDULE_TIMEOUT):

	https://lkml.kernel.org/r/<20181114150904.19653-1-johan@kernel.org>

> +	if (ret < 0) {
> +		bt_dev_err(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);
> @@ -1283,7 +1278,8 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  
>  	host_set_baudrate(hu, 2400);
>  	hci_uart_set_flow_control(hu, true);
> -	serdev_device_write_buf(serdev, &cmd, sizeof(cmd));
> +	serdev_device_write(serdev, &cmd, sizeof(cmd), 0);

Same here.

> +	serdev_device_wait_until_sent(serdev, 0);
>  	hci_uart_set_flow_control(hu, false);
>  	qca_power_setup(hu, false);
>  }

Johan

Re: [PATCH v1 4/4] Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990

[Matthias Kaehlcke]

On Tue, Nov 06, 2018 at 05:35:28PM +0530, Balakrishna Godavarthi wrote:
> [  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)
> 
> The above errors log on console due to receiving of stray bytes
> when wcn3990 boot up's i.e. when during initial setup procedure.

Please shortly introduce the topic instead of starting with the log
messages.

> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
>  drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 97b57e0f4725..341f80606574 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 due multiple baudrate switch is causing
> +	 * UART to read some garbage data.
> +	 */

This isn't entirely correct. I saw frame reassembly errors before
qca_wcn3990_init() is called, i.e. no baudrate changes. Some of the
garbage seems to be sent shortly after powering on the controller.

> +	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)) {
> @@ -1192,10 +1201,12 @@ static int qca_setup(struct hci_uart *hu)
>  		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>  		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
>  		hu->hdev->shutdown = qca_power_off;
> +
>  		ret = qca_wcn3990_init(hu);
>  		if (ret)
>  			return ret;
>  
> +		clear_bit(STATE_DISCARD_RX, &qca->flags);
>  		ret = qca_read_soc_version(hdev, &soc_ver);
>  		if (ret)
>  			return ret;
> @@ -1278,8 +1289,15 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  	struct serdev_device *serdev = hu->serdev;
>  	unsigned char cmd = QCA_WCN3990_POWEROFF_PULSE;
>  
> -	host_set_baudrate(hu, 2400);
> +	/* From this point we go into power off state,
> +	 * disable IBS and discard all the queued data.
> +	 */
> +	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> +	set_bit(STATE_DISCARD_RX, &qca->flags);
> +	qca_flush(hu);
> +
>  	hci_uart_set_flow_control(hu, true);
> +	host_set_baudrate(hu, 2400);
>  	serdev_device_write(serdev, &cmd, sizeof(cmd), 0);
>  	serdev_device_wait_until_sent(serdev, 0);
>  	hci_uart_set_flow_control(hu, false);

This change won't win a beauty price, but it seems it is needed to
suppress the 'frame reassembly' spam, unless the controller can be
convinced to stop sending garbage in the first place.

Tested-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH v1 4/4] Bluetooth: hci_qca: Fix frame reassembly errors for wcn3990

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-11-15 01:06, Matthias Kaehlcke wrote:
> On Tue, Nov 06, 2018 at 05:35:28PM +0530, Balakrishna Godavarthi wrote:
>> [  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)
>> 
>> The above errors log on console due to receiving of stray bytes
>> when wcn3990 boot up's i.e. when during initial setup procedure.
> 
> Please shortly introduce the topic instead of starting with the log
> messages.
> 
[Bala]:  will update.

>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>>  drivers/bluetooth/hci_qca.c | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 97b57e0f4725..341f80606574 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 due multiple baudrate switch is causing
>> +	 * UART to read some garbage data.
>> +	 */
> 
> This isn't entirely correct. I saw frame reassembly errors before
> qca_wcn3990_init() is called, i.e. no baudrate changes. Some of the
> garbage seems to be sent shortly after powering on the controller.
> 

[Bala]: yes your correct, will update the comment.

>> +	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)) {
>> @@ -1192,10 +1201,12 @@ static int qca_setup(struct hci_uart *hu)
>>  		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>>  		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
>>  		hu->hdev->shutdown = qca_power_off;
>> +
>>  		ret = qca_wcn3990_init(hu);
>>  		if (ret)
>>  			return ret;
>> 
>> +		clear_bit(STATE_DISCARD_RX, &qca->flags);
>>  		ret = qca_read_soc_version(hdev, &soc_ver);
>>  		if (ret)
>>  			return ret;
>> @@ -1278,8 +1289,15 @@ static void qca_power_shutdown(struct hci_uart 
>> *hu)
>>  	struct serdev_device *serdev = hu->serdev;
>>  	unsigned char cmd = QCA_WCN3990_POWEROFF_PULSE;
>> 
>> -	host_set_baudrate(hu, 2400);
>> +	/* From this point we go into power off state,
>> +	 * disable IBS and discard all the queued data.
>> +	 */
>> +	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> +	set_bit(STATE_DISCARD_RX, &qca->flags);
>> +	qca_flush(hu);
>> +
>>  	hci_uart_set_flow_control(hu, true);
>> +	host_set_baudrate(hu, 2400);
>>  	serdev_device_write(serdev, &cmd, sizeof(cmd), 0);
>>  	serdev_device_wait_until_sent(serdev, 0);
>>  	hci_uart_set_flow_control(hu, false);
> 
> This change won't win a beauty price, but it seems it is needed to
> suppress the 'frame reassembly' spam, unless the controller can be
> convinced to stop sending garbage in the first place.
> 
[Bala]: This is due to serdev_open function call before we trun on the 
regulators.
         where we don't have control over serdev open and close calls.

> Tested-by: Matthias Kaehlcke <mka@chromium.org>

[Bala]: Thanks for testing.

-- 
Regards
Balakrishna.

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

[Balakrishna Godavarthi]

Hi Johan,

On 2018-11-14 20:57, Johan Hovold wrote:
> On Tue, Nov 06, 2018 at 05:35:25PM +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 | 22 +++++++++-------------
>>  1 file changed, 9 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index f72ded4ec9ae..051f081d1835 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1016,8 +1016,7 @@ static inline void host_set_baudrate(struct 
>> hci_uart *hu, unsigned int speed)
>>  static int qca_send_power_pulse(struct hci_dev *hdev, 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
>> @@ -1030,18 +1029,14 @@ static int qca_send_power_pulse(struct hci_dev 
>> *hdev, u8 cmd)
>>  	 * 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;
>> -
>>  	hci_uart_set_flow_control(hu, true);
>> +	ret = serdev_device_write(hu->serdev, &cmd, sizeof(cmd), 0);
> 
> Don't you want a non-zero timeout here? Otherwise you might as well 
> call
> serdev_device_write() directly.
> 

[Bala]: we need an in definite timeout for these commands. will use 
HCI_UART_TX_WAKEUP.

> Also have you made sure that serdev_device_write_wakeup() is called in
> the drivers write-wakeup callback as serdev_device_write() requires?
> 
> See this series in case what you really wanted was to wait indefinitely
> (you can use MAX_SCHEDULE_TIMEOUT):
> 
> 	https://lkml.kernel.org/r/<20181114150904.19653-1-johan@kernel.org>
> 

[Bala]: thanks for pointers. from 
https://lore.kernel.org/patchwork/patch/1012358/
         it was clear to me that i need call below functions in order to 
wait until full data is sent.

         serdev_device_write_wakeup()
         serdev_device_write()
         serdev_device_wait_until_sent()

         correct me whether my understanding is correct.

>> +	if (ret < 0) {
>> +		bt_dev_err(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);
>> @@ -1283,7 +1278,8 @@ static void qca_power_shutdown(struct hci_uart 
>> *hu)
>> 
>>  	host_set_baudrate(hu, 2400);
>>  	hci_uart_set_flow_control(hu, true);
>> -	serdev_device_write_buf(serdev, &cmd, sizeof(cmd));
>> +	serdev_device_write(serdev, &cmd, sizeof(cmd), 0);
> 
> Same here.
> 
>> +	serdev_device_wait_until_sent(serdev, 0);
>>  	hci_uart_set_flow_control(hu, false);
>>  	qca_power_setup(hu, false);
>>  }
> 
> Johan

-- 
Regards
Balakrishna.

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

[Johan Hovold]

On Thu, Nov 15, 2018 at 08:04:29PM +0530, Balakrishna Godavarthi wrote:
> Hi Johan,
> 
> On 2018-11-14 20:57, Johan Hovold wrote:
> > On Tue, Nov 06, 2018 at 05:35:25PM +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 | 22 +++++++++-------------
> >>  1 file changed, 9 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> >> index f72ded4ec9ae..051f081d1835 100644
> >> --- a/drivers/bluetooth/hci_qca.c
> >> +++ b/drivers/bluetooth/hci_qca.c
> >> @@ -1016,8 +1016,7 @@ static inline void host_set_baudrate(struct 
> >> hci_uart *hu, unsigned int speed)
> >>  static int qca_send_power_pulse(struct hci_dev *hdev, 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
> >> @@ -1030,18 +1029,14 @@ static int qca_send_power_pulse(struct hci_dev 
> >> *hdev, u8 cmd)
> >>  	 * 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;
> >> -
> >>  	hci_uart_set_flow_control(hu, true);
> >> +	ret = serdev_device_write(hu->serdev, &cmd, sizeof(cmd), 0);
> > 
> > Don't you want a non-zero timeout here? Otherwise you might as well
> > call serdev_device_write() directly.
> 
> [Bala]: we need an in definite timeout for these commands. will use 
> HCI_UART_TX_WAKEUP.
> 
> > Also have you made sure that serdev_device_write_wakeup() is called in
> > the drivers write-wakeup callback as serdev_device_write() requires?
> > 
> > See this series in case what you really wanted was to wait indefinitely
> > (you can use MAX_SCHEDULE_TIMEOUT):
> > 
> > 	https://lkml.kernel.org/r/<20181114150904.19653-1-johan@kernel.org>
> 
> [Bala]: thanks for pointers. from 
> https://lore.kernel.org/patchwork/patch/1012358/
>          it was clear to me that i need call below functions in order to 
> wait until full data is sent.
> 
>          serdev_device_write_wakeup()
>          serdev_device_write()
>          serdev_device_wait_until_sent()
> 
>          correct me whether my understanding is correct.

No, serdev_device_write_wakeup() needs to be called from the serdev
driver's write_wakeup() callback (as defined in struct struct
serdev_device_ops).

Either use serdev_device_write_wakeup() as the callback, or call it from
your driver's custom write_wakeup() callback.

Hmm, I see now that the callbacks are set in hci_uart_register_device()
to hci_serdev_client_ops, which is shared by all serdev BT drivers. You
may be able to just add the call to the callback defined there so that
all Bluetooth drivers can use serdev_device_write().

Johan