| Message ID | 20180823112935.17342-2-bgodavar@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | Cold on & off support for Qualcomm BT chip wcn3990. | expand |
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.
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().
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
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
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;
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(+)