[PATCH v1 0/1] Cold on & off support for Qualcomm BT chip wcn3990.

[PATCH v1 0/1] Cold on & off support for Qualcomm BT chip wcn3990.

[Balakrishna Godavarthi]

This patch will power off regulators while hci0 down and turn on them 
back on hci0 up. Every time we power off we call proto specific close
function, so that we will free the memory of Qualcomm BT specific buffers.
Will call proto open to assign the memory and turn on the regulators, before
calling qualcomm setup.

Balakrishna Godavarthi (1):
  Bluetooth: hci_qca: Add poweroff support during hci down for wcn3990

 drivers/bluetooth/hci_qca.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

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

[PATCH v1 1/1] Bluetooth: hci_qca: Add poweroff support during hci down for wcn3990

[Balakrishna Godavarthi]

This patch enables power off support for hci down and power on support
for hci up. As wcn3990 power sources are ignited by regulators, we will
turn off them during hci down, i.e. an complete power off of wcn3990.
So while hci up, we will call vendor specific open/close and setup which
will turn on the regulators, requests BT chip version and download the
firmware.

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

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index e182f6019f68..98d33c6b8909 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -595,6 +595,9 @@ static int qca_close(struct hci_uart *hu)
 	struct qca_serdev *qcadev;
 	struct qca_data *qca = hu->priv;
 
+	if (!qca)
+		return 0;
+
 	BT_DBG("hu %p qca close", hu);
 
 	serial_clock_vote(HCI_IBS_VOTE_STATS_UPDATE, hu);
@@ -1098,6 +1101,32 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 	return 0;
 }
 
+static int hci_uart_open(struct hci_dev *hdev)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+
+	BT_DBG("%s %p", hdev->name, hdev);
+	set_bit(HCI_UART_PROTO_READY, &hu->flags);
+
+	return qca_open(hu);
+}
+
+static int hci_uart_close(struct hci_dev *hdev)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+
+	BT_DBG("%s %p", hdev->name, hdev);
+
+	/* After this step, we should not allow Tx and Rx work
+	 * function to execute. As we are freeing qca tx and rx
+	 * buffers which may cause kernel crash.
+	 */
+	clear_bit(HCI_UART_PROTO_READY, &hu->flags);
+	qca_close(hu);
+
+	return 0;
+}
+
 static int qca_wcn3990_init(struct hci_uart *hu)
 {
 	struct hci_dev *hdev = hu->hdev;
@@ -1154,6 +1183,11 @@ static int qca_setup(struct hci_uart *hu)
 
 	if (qcadev->btsoc_type == QCA_WCN3990) {
 		bt_dev_info(hdev, "setting up wcn3990");
+
+		hdev->open  = hci_uart_open;
+		hdev->close = hci_uart_close;
+		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+
 		ret = qca_wcn3990_init(hu);
 		if (ret)
 			return ret;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 1/1] Bluetooth: hci_qca: Add poweroff support during hci down for wcn3990

[Stephen Boyd]

Quoting Balakrishna Godavarthi (2018-08-23 04:29:35)
> This patch enables power off support for hci down and power on support
> for hci up. As wcn3990 power sources are ignited by regulators, we will
> turn off them during hci down, i.e. an complete power off of wcn3990.
> So while hci up, we will call vendor specific open/close and setup which
> will turn on the regulators, requests BT chip version and download the
> firmware.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
>  drivers/bluetooth/hci_qca.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index e182f6019f68..98d33c6b8909 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -595,6 +595,9 @@ static int qca_close(struct hci_uart *hu)
>         struct qca_serdev *qcadev;
>         struct qca_data *qca = hu->priv;
>  
> +       if (!qca)
> +               return 0;

Does this happen? If it does it seems like a failure in the caller to
know what's going on.

Re: [PATCH v1 1/1] Bluetooth: hci_qca: Add poweroff support during hci down for wcn3990

[Balakrishna Godavarthi]

Hi Stephen,

On 2018-08-24 12:17, Stephen Boyd wrote:
> Quoting Balakrishna Godavarthi (2018-08-23 04:29:35)
>> This patch enables power off support for hci down and power on support
>> for hci up. As wcn3990 power sources are ignited by regulators, we 
>> will
>> turn off them during hci down, i.e. an complete power off of wcn3990.
>> So while hci up, we will call vendor specific open/close and setup 
>> which
>> will turn on the regulators, requests BT chip version and download the
>> firmware.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>>  drivers/bluetooth/hci_qca.c | 34 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index e182f6019f68..98d33c6b8909 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -595,6 +595,9 @@ static int qca_close(struct hci_uart *hu)
>>         struct qca_serdev *qcadev;
>>         struct qca_data *qca = hu->priv;
>> 
>> +       if (!qca)
>> +               return 0;
> 
> Does this happen? If it does it seems like a failure in the caller to
> know what's going on.

[Bala]: yes qca_close() function will execute twice i.e. when we remove 
the BT module.

         while we remove the module,hci_dev_doc_close() will call 
hdev->close() i.e. hci_uart_close() which is Qualcomm specific close.
         in hci_uart_close() we will call qca_close() which will free the 
memory.
         after that proto close will also call qca_close().
         Here hci_uart_close and proto close are assigned to same 
function pointer i.e. qca_close().

-- 
Regards
Balakrishna.

Re: [PATCH v1 1/1] Bluetooth: hci_qca: Add poweroff support during hci down for wcn3990

[Marcel Holtmann]

Hi Balakrishna,

> This patch enables power off support for hci down and power on support
> for hci up. As wcn3990 power sources are ignited by regulators, we will
> turn off them during hci down, i.e. an complete power off of wcn3990.
> So while hci up, we will call vendor specific open/close and setup which
> will turn on the regulators, requests BT chip version and download the
> firmware.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> drivers/bluetooth/hci_qca.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index e182f6019f68..98d33c6b8909 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -595,6 +595,9 @@ static int qca_close(struct hci_uart *hu)
> 	struct qca_serdev *qcadev;
> 	struct qca_data *qca = hu->priv;
> 
> +	if (!qca)
> +		return 0;
> +
> 	BT_DBG("hu %p qca close", hu);
> 
> 	serial_clock_vote(HCI_IBS_VOTE_STATS_UPDATE, hu);
> @@ -1098,6 +1101,32 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
> 	return 0;
> }
> 
> +static int hci_uart_open(struct hci_dev *hdev)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +
> +	BT_DBG("%s %p", hdev->name, hdev);
> +	set_bit(HCI_UART_PROTO_READY, &hu->flags);
> +
> +	return qca_open(hu);
> +}
> +
> +static int hci_uart_close(struct hci_dev *hdev)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +
> +	BT_DBG("%s %p", hdev->name, hdev);
> +
> +	/* After this step, we should not allow Tx and Rx work
> +	 * function to execute. As we are freeing qca tx and rx
> +	 * buffers which may cause kernel crash.
> +	 */
> +	clear_bit(HCI_UART_PROTO_READY, &hu->flags);
> +	qca_close(hu);
> +
> +	return 0;
> +}
> +
> static int qca_wcn3990_init(struct hci_uart *hu)
> {
> 	struct hci_dev *hdev = hu->hdev;
> @@ -1154,6 +1183,11 @@ static int qca_setup(struct hci_uart *hu)
> 
> 	if (qcadev->btsoc_type == QCA_WCN3990) {
> 		bt_dev_info(hdev, "setting up wcn3990");
> +
> +		hdev->open  = hci_uart_open;
> +		hdev->close = hci_uart_close;
> +		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
> +

I do not get this change and I am also against it. Once hdev->open and hdev->close are assigned, don’t re-assign them. You called hci_register_dev and you are not changing the basic transport open/close functions after that. If you need some vendor specific things to open and close the transport, then do it properly.

When I posted the btuart.c serdev driver, it would be a lot easier to add vendor specific things then to hci_serdev.c which is still kinda hacked onto a line discipline driver.

Regards

Marcel

Re: [PATCH v1 1/1] Bluetooth: hci_qca: Add poweroff support during hci down for wcn3990

[Balakrishna Godavarthi]

Hi Marcel,

On 2018-08-25 00:20, Marcel Holtmann wrote:
> Hi Balakrishna,
> 
>> This patch enables power off support for hci down and power on support
>> for hci up. As wcn3990 power sources are ignited by regulators, we 
>> will
>> turn off them during hci down, i.e. an complete power off of wcn3990.
>> So while hci up, we will call vendor specific open/close and setup 
>> which
>> will turn on the regulators, requests BT chip version and download the
>> firmware.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> drivers/bluetooth/hci_qca.c | 34 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index e182f6019f68..98d33c6b8909 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -595,6 +595,9 @@ static int qca_close(struct hci_uart *hu)
>> 	struct qca_serdev *qcadev;
>> 	struct qca_data *qca = hu->priv;
>> 
>> +	if (!qca)
>> +		return 0;
>> +
>> 	BT_DBG("hu %p qca close", hu);
>> 
>> 	serial_clock_vote(HCI_IBS_VOTE_STATS_UPDATE, hu);
>> @@ -1098,6 +1101,32 @@ static int qca_set_speed(struct hci_uart *hu, 
>> enum qca_speed_type speed_type)
>> 	return 0;
>> }
>> 
>> +static int hci_uart_open(struct hci_dev *hdev)
>> +{
>> +	struct hci_uart *hu = hci_get_drvdata(hdev);
>> +
>> +	BT_DBG("%s %p", hdev->name, hdev);
>> +	set_bit(HCI_UART_PROTO_READY, &hu->flags);
>> +
>> +	return qca_open(hu);
>> +}
>> +
>> +static int hci_uart_close(struct hci_dev *hdev)
>> +{
>> +	struct hci_uart *hu = hci_get_drvdata(hdev);
>> +
>> +	BT_DBG("%s %p", hdev->name, hdev);
>> +
>> +	/* After this step, we should not allow Tx and Rx work
>> +	 * function to execute. As we are freeing qca tx and rx
>> +	 * buffers which may cause kernel crash.
>> +	 */
>> +	clear_bit(HCI_UART_PROTO_READY, &hu->flags);
>> +	qca_close(hu);
>> +
>> +	return 0;
>> +}
>> +
>> static int qca_wcn3990_init(struct hci_uart *hu)
>> {
>> 	struct hci_dev *hdev = hu->hdev;
>> @@ -1154,6 +1183,11 @@ static int qca_setup(struct hci_uart *hu)
>> 
>> 	if (qcadev->btsoc_type == QCA_WCN3990) {
>> 		bt_dev_info(hdev, "setting up wcn3990");
>> +
>> +		hdev->open  = hci_uart_open;
>> +		hdev->close = hci_uart_close;
>> +		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>> +
> 
> I do not get this change and I am also against it. Once hdev->open and
> hdev->close are assigned, don’t re-assign them. You called
> hci_register_dev and you are not changing the basic transport
> open/close functions after that. If you need some vendor specific
> things to open and close the transport, then do it properly.
> 

[Bala]: the reason to re-assign hdev->close & hdev->open is, when we do 
an hci0 down we are
         shutting down the regulators to turn off wcn3990 and closing the 
vendor proto i.e.clearing the memory too.,
         So we are re-assigning the hdev->close to proto close. Here is 
the use case of the above change wrt mobile handset.
         we will setup BT during bootup, i.e. we enable the clocks or 
regulators to turn on BT chip connected.
         let us assume that we turn off the BT chip i.e. hci0 down. so to 
current implementation we will down the HCI0,
         but still we have our uart clocks or BT clocks are regulators 
are enabled. This is causing power and memory consumption.

> When I posted the btuart.c serdev driver, it would be a lot easier to
> add vendor specific things then to hci_serdev.c which is still kinda
> hacked onto a line discipline driver.
> 

[Bala]: when can we expect btuart.c serdev driver.

> Regards
> 
> Marcel

-- 
Regards
Balakrishna.