Bluetooth: hci_qca: Enable clocks required for BT SOC
PATCH 1/1 Bluetooth: hci_qca: Enable clocks required for BT SOC
Instead of relying on other subsytem to turn ON clocks required for BT SoC to operate, voting them from the driver.
--- drivers/bluetooth/hci_qca.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
drivers/bluetooth/hci_qca.c +29 -2
@@ -164,6 +164,7 @@ struct qca_serdev { };static int qca_regulator_enable (struct qca_serdev *qcadev); +static int qca_power_on (struct qca_serdev *qcadev);static void qca_regulator_disable (struct qca_serdev *qcadev);static void qca_power_shutdown (struct hci_uart *hu);static int qca_power_off (struct hci_dev *hdev);
@@ -528,7 +529,7 @@ static int qca_open(struct hci_uart *hu) }else { hu->init_speed = qcadev->init_speed; hu->oper_speed = qcadev->oper_speed; - ret =qca_regulator_enable (qcadev); + ret =qca_power_on (qcadev);if (ret) {destroy_workqueue (qca->workqueue);kfree_skb (qca->rx_skb);
@@ -1214,7 +1215,7 @@ static int qca_wcn3990_init(struct hci_uart *hu) qcadev =serdev_device_get_drvdata (hu->serdev);if (!qcadev->bt_power->vregs_on) {serdev_device_close (hu->serdev); - ret =qca_regulator_enable (qcadev); + ret =qca_power_on (qcadev);if (ret)return ret;
@@ -1408,6 +1409,9 @@ static void qca_power_shutdown(struct hci_uart *hu)host_set_baudrate (hu,2400 );qca_send_power_pulse (hu, false);qca_regulator_disable (qcadev); + +if (qcadev->susclk) +clk_disable_unprepare (qcadev->susclk); }static int qca_power_off (struct hci_dev *hdev)
@@ -1423,6 +1427,20 @@ static int qca_power_off(struct hci_dev *hdev)return 0 ; } +static int qca_power_on (struct qca_serdev *qcadev) +{ +int err; + +if (qcadev->susclk) { + err =clk_prepare_enable (qcadev->susclk); +if (err) +return err; + } + +qca_regulator_enable (qcadev); +return 0 ; +} +static int qca_regulator_enable (struct qca_serdev *qcadev) {struct qca_power *power = qcadev->bt_power;
@@ -1523,6 +1541,15 @@ static int qca_serdev_probe(struct serdev_device *serdev) qcadev->bt_power->vregs_on = false; +if (qcadev->btsoc_type ==QCA_WCN3990 || + qcadev->btsoc_type ==QCA_WCN3991 ) { + qcadev->susclk =devm_clk_get (&serdev->dev,NULL ); +if (IS_ERR (qcadev->susclk)) { +dev_err (&serdev->dev,"failed to acquire clk\n" ); +return PTR_ERR (qcadev->susclk); + } + } +device_property_read_u32 (&serdev->dev,"max-speed" , &qcadev->oper_speed);if (!qcadev->oper_speed)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Marcel Holtmann
maintainerHi Balakrishna,
Quoted 13 lines
Instead of relying on other subsytem to turn ON clocks required for BT SoC to operate, voting them from the driver.
Signed-off-by Balakrishna Godavarthi <[email protected]>---
drivers/bluetooth/hci_qca.c| 31 +++++++++++++++++++++++++++++--1 file changed, 29 insertions(+), 2 deletions(-)drivers/bluetooth/hci_qca.c +0
-0
drivers/bluetooth/hci_qca.c struct qca_serdev { · line 167
Quoted 4 lines
drivers/bluetooth/hci_qca.c +1
-0@@ -164,6 +164,7 @@ struct qca_serdev { };static int qca_regulator_enable (struct qca_serdev *qcadev); +static int qca_power_on (struct qca_serdev *qcadev);
I really dislike forward declaration. Only use them if they are really really needed. That said, this driver might actually need cleanups since I just realized it has tons of forward declarations.
Quoted 53 lines
drivers/bluetooth/hci_qca.c +19
-2static void qca_regulator_disable (struct qca_serdev *qcadev);static void qca_power_shutdown (struct hci_uart *hu);static int qca_power_off (struct hci_dev *hdev);@@ -528,7 +529,7 @@ static int qca_open(struct hci_uart *hu) }else { hu->init_speed = qcadev->init_speed; hu->oper_speed = qcadev->oper_speed; - ret =qca_regulator_enable (qcadev); + ret =qca_power_on (qcadev);if (ret) {destroy_workqueue (qca->workqueue);kfree_skb (qca->rx_skb);@@ -1214,7 +1215,7 @@ static int qca_wcn3990_init(struct hci_uart *hu) qcadev =serdev_device_get_drvdata (hu->serdev);if (!qcadev->bt_power->vregs_on) {serdev_device_close (hu->serdev); - ret =qca_regulator_enable (qcadev); + ret =qca_power_on (qcadev);if (ret)return ret;@@ -1408,6 +1409,9 @@ static void qca_power_shutdown(struct hci_uart *hu)host_set_baudrate (hu,2400 );qca_send_power_pulse (hu, false);qca_regulator_disable (qcadev); + +if (qcadev->susclk) +clk_disable_unprepare (qcadev->susclk); }static int qca_power_off (struct hci_dev *hdev)@@ -1423,6 +1427,20 @@ static int qca_power_off(struct hci_dev *hdev)return 0 ; } +static int qca_power_on (struct qca_serdev *qcadev) +{ +int err; + +if (qcadev->susclk) { + err =clk_prepare_enable (qcadev->susclk); +if (err) +return err; + } + +qca_regulator_enable (qcadev); +return 0 ; +} +static int qca_regulator_enable (struct qca_serdev *qcadev) {struct qca_power *power = qcadev->bt_power;
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 1545
Quoted 5 lines
drivers/bluetooth/hci_qca.c +2
-0@@ -1523,6 +1541,15 @@ static int qca_serdev_probe(struct serdev_device *serdev) qcadev->bt_power->vregs_on = false; +if (qcadev->btsoc_type ==QCA_WCN3990 || + qcadev->btsoc_type ==QCA_WCN3991 ) {
There comes a point when using a switch statement is a lot easier to read. See if that has been reached and if there are other places that would benefit from a cleanup patch.
Regards
Marcel
Quoted 17 lines · Marcel Holtmann
Hi Balakrishna,
Quoted 15 lines
Instead of relying on other subsytem to turn ON clocks required for BT SoC to operate, voting them from the driver.
Signed-off-by Balakrishna Godavarthi <[email protected]>---
drivers/bluetooth/hci_qca.c| 31 +++++++++++++++++++++++++++++--1 file changed, 29 insertions(+), 2 deletions(-)drivers/bluetooth/hci_qca.c +0
-0@@ -164,6 +164,7 @@ struct qca_serdev { };
Quoted 7 lines
drivers/bluetooth/hci_qca.c struct qca_serdev { · line 167
Quoted 2 lines
drivers/bluetooth/hci_qca.c +1
-0static int qca_regulator_enable (struct qca_serdev *qcadev); +static int qca_power_on (struct qca_serdev *qcadev);I really dislike forward declaration. Only use them if they are really really needed. That said, this driver might actually need cleanups since I just realized it has tons of forward declarations.
[Bala]: agree, will work on this change.
Quoted 56 lines
Quoted 56 lines
drivers/bluetooth/hci_qca.c +5
-2static void qca_regulator_disable (struct qca_serdev *qcadev);static void qca_power_shutdown (struct hci_uart *hu);static int qca_power_off (struct hci_dev *hdev);@@ -528,7 +529,7 @@ static int qca_open(struct hci_uart *hu) }else { hu->init_speed = qcadev->init_speed; hu->oper_speed = qcadev->oper_speed; - ret =qca_regulator_enable (qcadev); + ret =qca_power_on (qcadev);if (ret) {destroy_workqueue (qca->workqueue);kfree_skb (qca->rx_skb);@@ -1214,7 +1215,7 @@ static int qca_wcn3990_init(struct hci_uart *hu) qcadev =serdev_device_get_drvdata (hu->serdev);if (!qcadev->bt_power->vregs_on) {serdev_device_close (hu->serdev); - ret =qca_regulator_enable (qcadev); + ret =qca_power_on (qcadev);if (ret)return ret;@@ -1408,6 +1409,9 @@ static void qca_power_shutdown(struct hci_uart *hu)host_set_baudrate (hu,2400 );qca_send_power_pulse (hu, false);qca_regulator_disable (qcadev); + +if (qcadev->susclk) +clk_disable_unprepare (qcadev->susclk); }static int
qca_power_off(struct hci_dev *hdev)drivers/bluetooth/hci_qca.c +14
-0@@ -1423,6 +1427,20 @@ static int qca_power_off(struct hci_dev *hdev)return 0 ; } +static int qca_power_on (struct qca_serdev *qcadev) +{ +int err; + +if (qcadev->susclk) { + err =clk_prepare_enable (qcadev->susclk); +if (err) +return err; + } + +qca_regulator_enable (qcadev); +return 0 ; +} +static int qca_regulator_enable (struct qca_serdev *qcadev) {struct qca_power *power = qcadev->bt_power;@@ -1523,6 +1541,15 @@ static int qca_serdev_probe(struct serdev_device *serdev)
Quoted 9 lines
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 1545
Quoted 4 lines
drivers/bluetooth/hci_qca.c +2
-0qcadev->bt_power->vregs_on = false; +if (qcadev->btsoc_type ==QCA_WCN3990 || + qcadev->btsoc_type ==QCA_WCN3991 ) {There comes a point when using a switch statement is a lot easier to read. See if that has been reached and if there are other places that would benefit from a cleanup patch.
[Bala] : will work on this.
Quoted 3 lines
Regards
Marcel
Quoting Balakrishna Godavarthi (2019-11-14 00:14:30)
drivers/bluetooth/hci_qca.c static int qca_power_off(struct hci_dev *hdev) · line 1434
Quoted 9 lines
drivers/bluetooth/hci_qca.c +5
-0@@ -1423,6 +1427,20 @@ static int qca_power_off(struct hci_dev *hdev)return 0 ; } +static int qca_power_on (struct qca_serdev *qcadev) +{ +int err; + +if (qcadev->susclk) {
clk_prepare_enable() shouldn't return anything besides 0 when passed a NULL pointer. Please drop this if condition in addition to the one on the clk_disable_unprepare().
Quoted 4 lines
drivers/bluetooth/hci_qca.c +4
-0+ err =clk_prepare_enable (qcadev->susclk); +if (err) +return err; + }
Hi Stephen,
Quoted 5 lines · Stephen Boyd
Quoting Balakrishna Godavarthi (2019-11-14 00:14:30)
Quoted 4 lines
drivers/bluetooth/hci_qca.c +0
-0@@ -1423,6 +1427,20 @@ static int qca_power_off(struct hci_dev *hdev)return 0 ; }
Quoted 14 lines
drivers/bluetooth/hci_qca.c static int qca_power_off(struct hci_dev *hdev) · line 1434
Quoted 5 lines
drivers/bluetooth/hci_qca.c +5
-0+static int qca_power_on (struct qca_serdev *qcadev) +{ +int err; + +if (qcadev->susclk) {
clk_prepare_enable()shouldn't return anything besides 0 when passed a NULL pointer. Please drop this if condition in addition to the one on theclk_disable_unprepare().Quoted 4 lines
drivers/bluetooth/hci_qca.c +4
-0+ err =clk_prepare_enable (qcadev->susclk); +if (err) +return err; + }
[Bala]: will update.
Regards
Balakrishna
Quoted 70 lines · Balakrishna Godavarthi
Instead of relying on other subsytem to turn ON clocks required for BT SoC to operate, voting them from the driver.
Signed-off-by Balakrishna Godavarthi <[email protected]>--- drivers/bluetooth/hci_qca.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)drivers/bluetooth/hci_qca.c +20
-2@@ -164,6 +164,7 @@ struct qca_serdev { };static int qca_regulator_enable (struct qca_serdev *qcadev); +static int qca_power_on (struct qca_serdev *qcadev);static void qca_regulator_disable (struct qca_serdev *qcadev);static void qca_power_shutdown (struct hci_uart *hu);static int qca_power_off (struct hci_dev *hdev);@@ -528,7 +529,7 @@ static int qca_open(struct hci_uart *hu) }else { hu->init_speed = qcadev->init_speed; hu->oper_speed = qcadev->oper_speed; - ret =qca_regulator_enable (qcadev); + ret =qca_power_on (qcadev);if (ret) {destroy_workqueue (qca->workqueue);kfree_skb (qca->rx_skb);@@ -1214,7 +1215,7 @@ static int qca_wcn3990_init(struct hci_uart *hu) qcadev =serdev_device_get_drvdata (hu->serdev);if (!qcadev->bt_power->vregs_on) {serdev_device_close (hu->serdev); - ret =qca_regulator_enable (qcadev); + ret =qca_power_on (qcadev);if (ret)return ret;@@ -1408,6 +1409,9 @@ static void qca_power_shutdown(struct hci_uart *hu)host_set_baudrate (hu,2400 );qca_send_power_pulse (hu, false);qca_regulator_disable (qcadev); + +if (qcadev->susclk) +clk_disable_unprepare (qcadev->susclk); }static int qca_power_off (struct hci_dev *hdev)@@ -1423,6 +1427,20 @@ static int qca_power_off(struct hci_dev *hdev)return 0 ; } +static int qca_power_on (struct qca_serdev *qcadev) +{ +int err; + +if (qcadev->susclk) { + err =clk_prepare_enable (qcadev->susclk); +if (err) +return err; + } + +qca_regulator_enable (qcadev); +return 0 ; +} +static int qca_regulator_enable (struct qca_serdev *qcadev) {struct qca_power *power = qcadev->bt_power;
drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct serdev_device *serdev) · line 1550
Quoted 10 lines
drivers/bluetooth/hci_qca.c +7
-0@@ -1523,6 +1541,15 @@ static int qca_serdev_probe(struct serdev_device *serdev) qcadev->bt_power->vregs_on = false; +if (qcadev->btsoc_type ==QCA_WCN3990 || + qcadev->btsoc_type ==QCA_WCN3991 ) { + qcadev->susclk =devm_clk_get (&serdev->dev,NULL ); +if (IS_ERR (qcadev->susclk)) { +dev_err (&serdev->dev,"failed to acquire clk\n" ); +return PTR_ERR (qcadev->susclk); + }
This will break existing users. Use devm_clk_get_optional() and at most raise a warning if the clock doesn't exist.
It would also be nice to add the clock to the affected devices in the tree if possible:
arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi: compatible = "qcom,wcn3990-bt";
arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi: compatible = "qcom,wcn3990-bt"; arch/arm64/boot/dts/qcom/qcs404-evb.dtsi: compatible = "qcom,wcn3990-bt"; arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi: compatible = "qcom,wcn3990-bt"; arch/arm64/boot/dts/qcom/sdm845-db845c.dts: compatible = "qcom,wcn3990-bt";
arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts: compatible = "qcom,wcn3990-bt";
Hi Matthias,
sorry missed you mail.
Quoted 87 lines · Matthias Kaehlcke
Quoted 72 lines · Balakrishna Godavarthi
Instead of relying on other subsytem to turn ON clocks required for BT SoC to operate, voting them from the driver.
Signed-off-by Balakrishna Godavarthi <[email protected]>--- drivers/bluetooth/hci_qca.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)drivers/bluetooth/hci_qca.c +6
-2@@ -164,6 +164,7 @@ struct qca_serdev { };static int qca_regulator_enable (struct qca_serdev *qcadev); +static int qca_power_on (struct qca_serdev *qcadev);static void qca_regulator_disable (struct qca_serdev *qcadev);static void qca_power_shutdown (struct hci_uart *hu);static int qca_power_off (struct hci_dev *hdev);@@ -528,7 +529,7 @@ static int qca_open(struct hci_uart *hu) }else { hu->init_speed = qcadev->init_speed; hu->oper_speed = qcadev->oper_speed; - ret =qca_regulator_enable (qcadev); + ret =qca_power_on (qcadev);if (ret) {destroy_workqueue (qca->workqueue);kfree_skb (qca->rx_skb);@@ -1214,7 +1215,7 @@ static int qca_wcn3990_init(struct hci_uart *hu) qcadev =serdev_device_get_drvdata (hu->serdev);if (!qcadev->bt_power->vregs_on) {serdev_device_close (hu->serdev); - ret =qca_regulator_enable (qcadev); + ret =qca_power_on (qcadev);if (ret)return ret;@@ -1408,6 +1409,9 @@ static void qca_power_shutdown(struct hci_uart *hu)host_set_baudrate (hu,2400 );qca_send_power_pulse (hu, false);qca_regulator_disable (qcadev); + +if (qcadev->susclk) +clk_disable_unprepare (qcadev->susclk); }static int
qca_power_off(struct hci_dev *hdev)drivers/bluetooth/hci_qca.c +14
-0@@ -1423,6 +1427,20 @@ static int qca_power_off(struct hci_dev *hdev)return 0 ; } +static int qca_power_on (struct qca_serdev *qcadev) +{ +int err; + +if (qcadev->susclk) { + err =clk_prepare_enable (qcadev->susclk); +if (err) +return err; + } + +qca_regulator_enable (qcadev); +return 0 ; +} +static int qca_regulator_enable (struct qca_serdev *qcadev) {struct qca_power *power = qcadev->bt_power;@@ -1523,6 +1541,15 @@ static int qca_serdev_probe(struct serdev_device *serdev)drivers/bluetooth/hci_qca.c static int qca_serdev_probe(struct · line 1551
Quoted 10 lines
drivers/bluetooth/hci_qca.c +7
-0qcadev->bt_power->vregs_on = false; +if (qcadev->btsoc_type ==QCA_WCN3990 || + qcadev->btsoc_type ==QCA_WCN3991 ) { + qcadev->susclk =devm_clk_get (&serdev->dev,NULL ); +if (IS_ERR (qcadev->susclk)) { +dev_err (&serdev->dev,"failed to acquire clk\n" ); +return PTR_ERR (qcadev->susclk); + }This will break existing users. Use
devm_clk_get_optional()and at most raise a warning if the clock doesn't exist.
Quoted 3 lines
It would also be nice to add the clock to the affected devices in the tree if possible:
[Bala]: Sure we will use devm_clk_get_optional() in the next patch.
We will check the effected areas and update the necessary as i see some projects are not existing.
Quoted 12 lines
arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi:
compatible = "qcom,wcn3990-bt";arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi: compatible =
"qcom,wcn3990-bt";arch/arm64/boot/dts/qcom/qcs404-evb.dtsi: compatible =
"qcom,wcn3990-bt";arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi: compatible =
"qcom,wcn3990-bt";arch/arm64/boot/dts/qcom/sdm845-db845c.dts: compatible =
"qcom,wcn3990-bt";arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts:
compatible = "qcom,wcn3990-bt";