| Message ID | 20191114081430.25427-1-bgodavar@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] Bluetooth: hci_qca: Enable clocks required for BT SOC | expand |
Hi Balakrishna, > 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 <bgodavar@codeaurora.org> > --- > drivers/bluetooth/hci_qca.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index f10bdf8e1fc5..dc95e378574b 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -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. > 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) { 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
Quoting Balakrishna Godavarthi (2019-11-14 00:14:30) > @@ -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(). > + err = clk_prepare_enable(qcadev->susclk); > + if (err) > + return err; > + }
On 2019-11-14 14:25, Marcel Holtmann wrote: > Hi Balakrishna, > >> 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 <bgodavar@codeaurora.org> >> --- >> drivers/bluetooth/hci_qca.c | 31 +++++++++++++++++++++++++++++-- >> 1 file changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index f10bdf8e1fc5..dc95e378574b 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -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. > [Bala]: agree, will work on this change. >> 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) { > > 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. > Regards > > Marcel
Hi Stephen, On 2019-11-14 22:49, Stephen Boyd wrote: > Quoting Balakrishna Godavarthi (2019-11-14 00:14:30) >> @@ -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(). > >> + err = clk_prepare_enable(qcadev->susclk); >> + if (err) >> + return err; >> + } [Bala]: will update. Regards Balakrishna
On Thu, Nov 14, 2019 at 01:44:30PM +0530, Balakrishna Godavarthi wrote: > 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 <bgodavar@codeaurora.org> > --- > drivers/bluetooth/hci_qca.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index f10bdf8e1fc5..dc95e378574b 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -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); > + } 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. On 2019-12-12 23:13, Matthias Kaehlcke wrote: > On Thu, Nov 14, 2019 at 01:44:30PM +0530, Balakrishna Godavarthi wrote: >> 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 <bgodavar@codeaurora.org> >> --- >> drivers/bluetooth/hci_qca.c | 31 +++++++++++++++++++++++++++++-- >> 1 file changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index f10bdf8e1fc5..dc95e378574b 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -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); >> + } > > 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: > [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. > 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";
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index f10bdf8e1fc5..dc95e378574b 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -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)
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 <bgodavar@codeaurora.org> --- drivers/bluetooth/hci_qca.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)