| Message ID | 20180705165515.13340-8-bgodavar@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Thu, Jul 05, 2018 at 10:25:15PM +0530, Balakrishna Godavarthi wrote: > Add support to set voltage/current of various regulators > to power up/down Bluetooth chip wcn3990. > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> > --- > changes in v9: > * moved flow control to vendor and set_baudarte functions. > * removed parent regs. > > changes in v8: > * closing qca buffer, if qca_power_setup fails > * chnaged ibs start timer function call location. > * updated review comments. > > changes in v7: > * addressed review comments. > > changes in v6: > * Hooked up qca_power to qca_serdev. > * renamed all the naming inconsistency functions with qca_* > * leveraged common code of ROME for wcn3990. > * created wrapper functions for re-usable blocks. > * updated function of _*regulator_enable and _*regualtor_disable. > * removed redundant comments and functions. > * addressed review comments. > > Changes in v5: > * updated regulator vddpa min_uV to 1304000. > * addressed review comments. > > Changes in v4: > * Segregated the changes of btqca from hci_qca > * rebased all changes on top of bluetooth-next. > * addressed review comments. > > --- > drivers/bluetooth/btqca.h | 3 + > drivers/bluetooth/hci_qca.c | 387 +++++++++++++++++++++++++++++++----- > 2 files changed, 345 insertions(+), 45 deletions(-) > > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h > index a9c2779f3e07..0c01f375fe83 100644 > --- a/drivers/bluetooth/btqca.h > +++ b/drivers/bluetooth/btqca.h > @@ -37,6 +37,9 @@ > #define EDL_TAG_ID_HCI (17) > #define EDL_TAG_ID_DEEP_SLEEP (27) > > +#define QCA_WCN3990_POWERON_PULSE 0xFC > +#define QCA_WCN3990_POWEROFF_PULSE 0xC0 > + > enum qca_bardrate { > QCA_BAUDRATE_115200 = 0, > QCA_BAUDRATE_57600, > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index 7ebfaa0edf3f..d62c7785a618 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -5,7 +5,7 @@ > * protocol extension to H4. > * > * Copyright (C) 2007 Texas Instruments, Inc. > - * Copyright (c) 2010, 2012 The Linux Foundation. All rights reserved. > + * Copyright (c) 2010, 2012, 2018 The Linux Foundation. All rights reserved. > * > * Acknowledgements: > * This file is based on hci_ll.c, which was... > @@ -31,9 +31,14 @@ > #include <linux/kernel.h> > #include <linux/clk.h> > #include <linux/debugfs.h> > +#include <linux/delay.h> > +#include <linux/device.h> > #include <linux/gpio/consumer.h> > #include <linux/mod_devicetable.h> > #include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/consumer.h> > #include <linux/serdev.h> > > #include <net/bluetooth/bluetooth.h> > @@ -124,12 +129,46 @@ enum qca_speed_type { > QCA_OPER_SPEED > }; > > +/* > + * Voltage regulator information required for configuring the > + * QCA Bluetooth chipset > + */ > +struct qca_vreg { > + const char *name; > + unsigned int min_uV; > + unsigned int max_uV; > + unsigned int load_uA; > +}; > + > +struct qca_vreg_data { > + enum qca_btsoc_type soc_type; > + struct qca_vreg *vregs; > + size_t num_vregs; > +}; > + > +/* > + * Platform data for the QCA Bluetooth power driver. > + */ > +struct qca_power { > + struct device *dev; > + const struct qca_vreg_data *vreg_data; > + struct regulator_bulk_data *vreg_bulk; > + bool vregs_on; > +}; > + > struct qca_serdev { > struct hci_uart serdev_hu; > struct gpio_desc *bt_en; > struct clk *susclk; > + enum qca_btsoc_type btsoc_type; > + struct qca_power *bt_power; > + u32 init_speed; > + u32 oper_speed; > }; > > +static int qca_power_setup(struct hci_uart *hu, bool on); > +static void qca_power_shutdown(struct hci_uart *hu); > + > static void __serial_clock_on(struct tty_struct *tty) > { > /* TODO: Some chipset requires to enable UART clock on client > @@ -407,6 +446,7 @@ static int qca_open(struct hci_uart *hu) > { > struct qca_serdev *qcadev; > struct qca_data *qca; > + int ret; > > BT_DBG("hu %p qca_open", hu); > > @@ -458,19 +498,32 @@ static int qca_open(struct hci_uart *hu) > > hu->priv = qca; > > - timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, 0); > - qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; > - > - timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); > - qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; > - > if (hu->serdev) { > serdev_device_open(hu->serdev); > > qcadev = serdev_device_get_drvdata(hu->serdev); > - gpiod_set_value_cansleep(qcadev->bt_en, 1); > + if (qcadev->btsoc_type != QCA_WCN3990) { > + gpiod_set_value_cansleep(qcadev->bt_en, 1); > + } else { > + hu->init_speed = qcadev->init_speed; > + hu->oper_speed = qcadev->oper_speed; > + ret = qca_power_setup(hu, true); > + if (ret) { > + destroy_workqueue(qca->workqueue); > + kfree_skb(qca->rx_skb); > + hu->priv = NULL; > + kfree(qca); > + return ret; > + } > + } > } > > + timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, 0); > + qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; > + > + timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); > + qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; > + > BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u", > qca->tx_idle_delay, qca->wake_retrans); > > @@ -554,10 +607,13 @@ static int qca_close(struct hci_uart *hu) > qca->hu = NULL; > > if (hu->serdev) { > - serdev_device_close(hu->serdev); > - > qcadev = serdev_device_get_drvdata(hu->serdev); > - gpiod_set_value_cansleep(qcadev->bt_en, 0); > + if (qcadev->btsoc_type == QCA_WCN3990) > + qca_power_shutdown(hu); > + else > + gpiod_set_value_cansleep(qcadev->bt_en, 0); > + > + serdev_device_close(hu->serdev); > } > > kfree_skb(qca->rx_skb); > @@ -891,6 +947,7 @@ 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) > @@ -904,6 +961,13 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate) > return -ENOMEM; > } > > + /* Disabling hardware flow control is mandate while nit: s/mandate/mandatory|required/ > + * 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; > @@ -919,6 +983,9 @@ 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; > } > > @@ -930,6 +997,36 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed) > hci_uart_set_baudrate(hu, speed); > } > > +static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd) > +{ > + struct hci_uart *hu = hci_get_drvdata(hdev); > + struct qca_data *qca = hu->priv; > + struct sk_buff *skb; > + > + bt_dev_dbg(hdev, "sending command %02x to SoC", cmd); > + > + skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL); > + if (!skb) > + return -ENOMEM; > + > + /* Disabling hardware flow control is mandate while Same nit as above > + * sending vendor power on and off pulse to SoC. > + */ The function is called qca_send_vendor_cmd(), the comment about power on/off pulses seems misplaced here. Are there other 'vendor commands' that require a different flow control behavior? Perhaps the function should have a different name or we need another wrapper. > + hci_uart_set_flow_control(hu, true); Should the changing of the flow control be limited to wcn3990? As of now the function is only called for wcn3990, however this is not stated as a requirement and might change in the future. > + skb_put_u8(skb, cmd); > + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT; > + > + skb_queue_tail(&qca->txq, skb); > + hci_uart_tx_wakeup(hu); > + > + /* Wait for 100 uS for SoC to settle down */ > + usleep_range(100, 200); Is this needed for any 'vendor command' or directly related with the power on/off pulses? > + hci_uart_set_flow_control(hu, false); > + > + return 0; > +} > + > static unsigned int qca_get_speed(struct hci_uart *hu, > enum qca_speed_type speed_type) > { > @@ -952,10 +1049,19 @@ static unsigned int qca_get_speed(struct hci_uart *hu, > > static int qca_check_speeds(struct hci_uart *hu) > { > - /* One or the other speeds should be non zero. */ > - if (!qca_get_speed(hu, QCA_INIT_SPEED) && > - !qca_get_speed(hu, QCA_OPER_SPEED)) > + struct qca_serdev *qcadev; > + > + qcadev = serdev_device_get_drvdata(hu->serdev); > + if ((qcadev->btsoc_type == QCA_WCN3990 && > + !qca_get_speed(hu, QCA_INIT_SPEED)) || > + !qca_get_speed(hu, QCA_OPER_SPEED)) { > + /* Both INIT and OPER speed should be non zero. */ > return -EINVAL; > + } else if (!qca_get_speed(hu, QCA_INIT_SPEED) && > + !qca_get_speed(hu, QCA_OPER_SPEED)) { > + /* One or the other speeds should be non zero. */ > + return -EINVAL; > + } It is questionable if the comments are useful, they are basically stating the same as the conditions. if-else statements with a single statement in all branches shouldn't use curly braces. Personally I don't dislike them in this case with the multi-line conditions, but in principle they shoulnd't be there. nit: this would be easier to read with nested if statements: if (qcadev->btsoc_type == QCA_WCN3990) if (!qca_get_speed(hu, QCA_INIT_SPEED)) || !qca_get_speed(hu, QCA_OPER_SPEED)) return -EINVAL; else if (!qca_get_speed(hu, QCA_INIT_SPEED) && !qca_get_speed(hu, QCA_OPER_SPEED)) { return -EINVAL; > return 0; > } > @@ -986,15 +1092,40 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type) > return 0; > } > > +static int qca_wcn3990_init(struct hci_uart *hu) > +{ > + int ret; > + > + ret = qca_send_vendor_cmd(hu->hdev, QCA_WCN3990_POWERON_PULSE); > + if (ret) > + return ret; > + > + /* Wait for 100 ms for SoC to boot up */ > + msleep(100); > + serdev_device_close(hu->serdev); > + ret = serdev_device_open(hu->serdev); > + if (ret) { > + bt_dev_err(hu->hdev, "failed to open port"); > + return ret; > + } > + > + /* Setup initial baudrate */ > + qca_set_speed(hu, QCA_INIT_SPEED); The comment is a bit redundant. > + hci_uart_set_flow_control(hu, false); > + > + return 0; > +} > + > static int qca_setup(struct hci_uart *hu) > { > struct hci_dev *hdev = hu->hdev; > struct qca_data *qca = hu->priv; > unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; > + struct qca_serdev *qcadev; > int ret; > int soc_ver = 0; > > - bt_dev_info(hdev, "ROME setup"); > + qcadev = serdev_device_get_drvdata(hu->serdev); > > ret = qca_check_speeds(hu); > if (ret) > @@ -1006,6 +1137,21 @@ static int qca_setup(struct hci_uart *hu) > /* Setup initial baudrate */ > qca_set_speed(hu, QCA_INIT_SPEED); > > + if (qcadev->btsoc_type == QCA_WCN3990) { > + bt_dev_dbg(hdev, "setting up wcn3990"); Seems like this should be bt_dev_info() for consistency with Rome. > + ret = qca_wcn3990_init(hu); > + if (ret) > + return ret; > + > + ret = qca_read_soc_version(hdev, &soc_ver); > + if (ret < 0 || soc_ver == 0) { > + bt_dev_err(hdev, "Failed to get version %d", ret); > + return ret; > + } > + } else { > + bt_dev_info(hdev, "ROME setup"); > + } > + > /* Setup user speed if needed */ > speed = qca_get_speed(hu, QCA_OPER_SPEED); > if (speed) { > @@ -1016,16 +1162,18 @@ static int qca_setup(struct hci_uart *hu) > qca_baudrate = qca_get_baudrate_value(speed); > } > > - /* Get QCA version information */ > - ret = qca_read_soc_version(hdev, &soc_ver); > - if (ret < 0 || soc_ver == 0) { > - bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); > - return ret; > + if (!soc_ver) { The following would probably be clearer: if (qcadev->btsoc_type != QCA_WCN3990) { > + /* Get QCA version information */ > + ret = qca_read_soc_version(hdev, &soc_ver); > + if (ret < 0 || soc_ver == 0) { > + bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); > + return ret; > + } > + bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); Couldn't we hide some of this in qca_read_soc_version()? qca_read_soc_version() could at least do the logging in the error case (the 'soc_ver == 0' case could be handled there as well), which would leave us with: ret = qca_read_soc_version(hdev, &soc_ver); if (ret) return ret; And the same above. The controller version can be logged outside of the branch for both wcn3990 and Rome. > } > - bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); > > /* Setup patch / NVM configurations */ > - ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver); > + ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, soc_ver); > if (!ret) { > set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); > qca_debugfs_init(hdev); > @@ -1061,9 +1209,123 @@ static struct hci_uart_proto qca_proto = { > .dequeue = qca_dequeue, > }; > > +static const struct qca_vreg_data qca_soc_data = { > + .soc_type = QCA_WCN3990, > + .vregs = (struct qca_vreg []) { > + { "vddio", 1800000, 1800000, 0 }, > + { "vddxo", 1800000, 1800000, 1 }, > + { "vddrf", 1304000, 1304000, 1 }, > + { "vddch0", 3000000, 3312000, 1 }, > + }, > + .num_vregs = 4, > +}; I didn't chime in earlier in the discussion with Stephen on the regulators (https://patchwork.kernel.org/patch/10467911/), however I agree with him that specifying at load of 1uA doesn't seem to make much sense. What would happen if the load remained unspecified (or 0)? -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Matthias, On 2018-07-07 03:51, Matthias Kaehlcke wrote: > On Thu, Jul 05, 2018 at 10:25:15PM +0530, Balakrishna Godavarthi wrote: >> Add support to set voltage/current of various regulators >> to power up/down Bluetooth chip wcn3990. >> >> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> >> --- >> changes in v9: >> * moved flow control to vendor and set_baudarte functions. >> * removed parent regs. >> >> changes in v8: >> * closing qca buffer, if qca_power_setup fails >> * chnaged ibs start timer function call location. >> * updated review comments. >> >> changes in v7: >> * addressed review comments. >> >> changes in v6: >> * Hooked up qca_power to qca_serdev. >> * renamed all the naming inconsistency functions with qca_* >> * leveraged common code of ROME for wcn3990. >> * created wrapper functions for re-usable blocks. >> * updated function of _*regulator_enable and _*regualtor_disable. >> * removed redundant comments and functions. >> * addressed review comments. >> >> Changes in v5: >> * updated regulator vddpa min_uV to 1304000. >> * addressed review comments. >> >> Changes in v4: >> * Segregated the changes of btqca from hci_qca >> * rebased all changes on top of bluetooth-next. >> * addressed review comments. >> >> --- >> drivers/bluetooth/btqca.h | 3 + >> drivers/bluetooth/hci_qca.c | 387 >> +++++++++++++++++++++++++++++++----- >> 2 files changed, 345 insertions(+), 45 deletions(-) >> >> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h >> index a9c2779f3e07..0c01f375fe83 100644 >> --- a/drivers/bluetooth/btqca.h >> +++ b/drivers/bluetooth/btqca.h >> @@ -37,6 +37,9 @@ >> #define EDL_TAG_ID_HCI (17) >> #define EDL_TAG_ID_DEEP_SLEEP (27) >> >> +#define QCA_WCN3990_POWERON_PULSE 0xFC >> +#define QCA_WCN3990_POWEROFF_PULSE 0xC0 >> + >> enum qca_bardrate { >> QCA_BAUDRATE_115200 = 0, >> QCA_BAUDRATE_57600, >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index 7ebfaa0edf3f..d62c7785a618 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -5,7 +5,7 @@ >> * protocol extension to H4. >> * >> * Copyright (C) 2007 Texas Instruments, Inc. >> - * Copyright (c) 2010, 2012 The Linux Foundation. All rights >> reserved. >> + * Copyright (c) 2010, 2012, 2018 The Linux Foundation. All rights >> reserved. >> * >> * Acknowledgements: >> * This file is based on hci_ll.c, which was... >> @@ -31,9 +31,14 @@ >> #include <linux/kernel.h> >> #include <linux/clk.h> >> #include <linux/debugfs.h> >> +#include <linux/delay.h> >> +#include <linux/device.h> >> #include <linux/gpio/consumer.h> >> #include <linux/mod_devicetable.h> >> #include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/regulator/consumer.h> >> #include <linux/serdev.h> >> >> #include <net/bluetooth/bluetooth.h> >> @@ -124,12 +129,46 @@ enum qca_speed_type { >> QCA_OPER_SPEED >> }; >> >> +/* >> + * Voltage regulator information required for configuring the >> + * QCA Bluetooth chipset >> + */ >> +struct qca_vreg { >> + const char *name; >> + unsigned int min_uV; >> + unsigned int max_uV; >> + unsigned int load_uA; >> +}; >> + >> +struct qca_vreg_data { >> + enum qca_btsoc_type soc_type; >> + struct qca_vreg *vregs; >> + size_t num_vregs; >> +}; >> + >> +/* >> + * Platform data for the QCA Bluetooth power driver. >> + */ >> +struct qca_power { >> + struct device *dev; >> + const struct qca_vreg_data *vreg_data; >> + struct regulator_bulk_data *vreg_bulk; >> + bool vregs_on; >> +}; >> + >> struct qca_serdev { >> struct hci_uart serdev_hu; >> struct gpio_desc *bt_en; >> struct clk *susclk; >> + enum qca_btsoc_type btsoc_type; >> + struct qca_power *bt_power; >> + u32 init_speed; >> + u32 oper_speed; >> }; >> >> +static int qca_power_setup(struct hci_uart *hu, bool on); >> +static void qca_power_shutdown(struct hci_uart *hu); >> + >> static void __serial_clock_on(struct tty_struct *tty) >> { >> /* TODO: Some chipset requires to enable UART clock on client >> @@ -407,6 +446,7 @@ static int qca_open(struct hci_uart *hu) >> { >> struct qca_serdev *qcadev; >> struct qca_data *qca; >> + int ret; >> >> BT_DBG("hu %p qca_open", hu); >> >> @@ -458,19 +498,32 @@ static int qca_open(struct hci_uart *hu) >> >> hu->priv = qca; >> >> - timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, >> 0); >> - qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; >> - >> - timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); >> - qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; >> - >> if (hu->serdev) { >> serdev_device_open(hu->serdev); >> >> qcadev = serdev_device_get_drvdata(hu->serdev); >> - gpiod_set_value_cansleep(qcadev->bt_en, 1); >> + if (qcadev->btsoc_type != QCA_WCN3990) { >> + gpiod_set_value_cansleep(qcadev->bt_en, 1); >> + } else { >> + hu->init_speed = qcadev->init_speed; >> + hu->oper_speed = qcadev->oper_speed; >> + ret = qca_power_setup(hu, true); >> + if (ret) { >> + destroy_workqueue(qca->workqueue); >> + kfree_skb(qca->rx_skb); >> + hu->priv = NULL; >> + kfree(qca); >> + return ret; >> + } >> + } >> } >> >> + timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, >> 0); >> + qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; >> + >> + timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); >> + qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; >> + >> BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u", >> qca->tx_idle_delay, qca->wake_retrans); >> >> @@ -554,10 +607,13 @@ static int qca_close(struct hci_uart *hu) >> qca->hu = NULL; >> >> if (hu->serdev) { >> - serdev_device_close(hu->serdev); >> - >> qcadev = serdev_device_get_drvdata(hu->serdev); >> - gpiod_set_value_cansleep(qcadev->bt_en, 0); >> + if (qcadev->btsoc_type == QCA_WCN3990) >> + qca_power_shutdown(hu); >> + else >> + gpiod_set_value_cansleep(qcadev->bt_en, 0); >> + >> + serdev_device_close(hu->serdev); >> } >> >> kfree_skb(qca->rx_skb); >> @@ -891,6 +947,7 @@ 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) >> @@ -904,6 +961,13 @@ static int qca_set_baudrate(struct hci_dev *hdev, >> uint8_t baudrate) >> return -ENOMEM; >> } >> >> + /* Disabling hardware flow control is mandate while > > nit: s/mandate/mandatory|required/ > [Bala]: will update. >> + * 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; >> @@ -919,6 +983,9 @@ 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; >> } >> >> @@ -930,6 +997,36 @@ static inline void host_set_baudrate(struct >> hci_uart *hu, unsigned int speed) >> hci_uart_set_baudrate(hu, speed); >> } >> >> +static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd) >> +{ >> + struct hci_uart *hu = hci_get_drvdata(hdev); >> + struct qca_data *qca = hu->priv; >> + struct sk_buff *skb; >> + >> + bt_dev_dbg(hdev, "sending command %02x to SoC", cmd); >> + >> + skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL); >> + if (!skb) >> + return -ENOMEM; >> + >> + /* Disabling hardware flow control is mandate while > > Same nit as above > [Bala]: will update. >> + * sending vendor power on and off pulse to SoC. >> + */ > > The function is called qca_send_vendor_cmd(), the comment about power > on/off pulses seems misplaced here. Are there other 'vendor commands' > that require a different flow control behavior? > > Perhaps the function should have a different name or we need another > wrapper. > [Bala]: this function is used to send single byte vendor commands. like power on and off pulse. and all single byte vendor commands require an flow control off and on. so might be function name change is required. instead of qca_send_vendor_cmd(). i want to change function name to qca_send_pulse() as this will be generic. >> + hci_uart_set_flow_control(hu, true); > > Should the changing of the flow control be limited to wcn3990? As of > now the function is only called for wcn3990, however this is not > stated as a requirement and might change in the future. > >> + skb_put_u8(skb, cmd); >> + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT; >> + >> + skb_queue_tail(&qca->txq, skb); >> + hci_uart_tx_wakeup(hu); >> + >> + /* Wait for 100 uS for SoC to settle down */ >> + usleep_range(100, 200); > > Is this needed for any 'vendor command' or directly related with the > power on/off pulses? > [Bala] : flow control off/on is needed for power on and off pulses along with command to set baudrate.. this is common across all the present chips and chips which are in development stages. might be function name is confusing will update the function name. >> + hci_uart_set_flow_control(hu, false); >> + >> + return 0; >> +} >> + >> static unsigned int qca_get_speed(struct hci_uart *hu, >> enum qca_speed_type speed_type) >> { >> @@ -952,10 +1049,19 @@ static unsigned int qca_get_speed(struct >> hci_uart *hu, >> >> static int qca_check_speeds(struct hci_uart *hu) >> { >> - /* One or the other speeds should be non zero. */ >> - if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> - !qca_get_speed(hu, QCA_OPER_SPEED)) >> + struct qca_serdev *qcadev; >> + >> + qcadev = serdev_device_get_drvdata(hu->serdev); >> + if ((qcadev->btsoc_type == QCA_WCN3990 && >> + !qca_get_speed(hu, QCA_INIT_SPEED)) || >> + !qca_get_speed(hu, QCA_OPER_SPEED)) { >> + /* Both INIT and OPER speed should be non zero. */ >> return -EINVAL; >> + } else if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> + !qca_get_speed(hu, QCA_OPER_SPEED)) { >> + /* One or the other speeds should be non zero. */ >> + return -EINVAL; >> + } > > It is questionable if the comments are useful, they are basically > stating the same as the conditions. > > if-else statements with a single statement in all branches shouldn't > use curly braces. Personally I don't dislike them in this case with > the multi-line conditions, but in principle they shoulnd't be there. > > nit: this would be easier to read with nested if statements: > [Bala]: will update > if (qcadev->btsoc_type == QCA_WCN3990) > if (!qca_get_speed(hu, QCA_INIT_SPEED)) || > !qca_get_speed(hu, QCA_OPER_SPEED)) > return -EINVAL; > else > if (!qca_get_speed(hu, QCA_INIT_SPEED) && > !qca_get_speed(hu, QCA_OPER_SPEED)) { > return -EINVAL; > >> return 0; >> } >> @@ -986,15 +1092,40 @@ static int qca_set_speed(struct hci_uart *hu, >> enum qca_speed_type speed_type) >> return 0; >> } >> >> +static int qca_wcn3990_init(struct hci_uart *hu) >> +{ >> + int ret; >> + >> + ret = qca_send_vendor_cmd(hu->hdev, QCA_WCN3990_POWERON_PULSE); >> + if (ret) >> + return ret; >> + >> + /* Wait for 100 ms for SoC to boot up */ >> + msleep(100); >> + serdev_device_close(hu->serdev); >> + ret = serdev_device_open(hu->serdev); >> + if (ret) { >> + bt_dev_err(hu->hdev, "failed to open port"); >> + return ret; >> + } >> + >> + /* Setup initial baudrate */ >> + qca_set_speed(hu, QCA_INIT_SPEED); > > The comment is a bit redundant. > [Bala]: will update >> + hci_uart_set_flow_control(hu, false); >> + >> + return 0; >> +} >> + >> static int qca_setup(struct hci_uart *hu) >> { >> struct hci_dev *hdev = hu->hdev; >> struct qca_data *qca = hu->priv; >> unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; >> + struct qca_serdev *qcadev; >> int ret; >> int soc_ver = 0; >> >> - bt_dev_info(hdev, "ROME setup"); >> + qcadev = serdev_device_get_drvdata(hu->serdev); >> >> ret = qca_check_speeds(hu); >> if (ret) >> @@ -1006,6 +1137,21 @@ static int qca_setup(struct hci_uart *hu) >> /* Setup initial baudrate */ >> qca_set_speed(hu, QCA_INIT_SPEED); >> >> + if (qcadev->btsoc_type == QCA_WCN3990) { >> + bt_dev_dbg(hdev, "setting up wcn3990"); > > Seems like this should be bt_dev_info() for consistency with Rome. > [Bala]: will update >> + ret = qca_wcn3990_init(hu); >> + if (ret) >> + return ret; >> + >> + ret = qca_read_soc_version(hdev, &soc_ver); >> + if (ret < 0 || soc_ver == 0) { >> + bt_dev_err(hdev, "Failed to get version %d", ret); >> + return ret; >> + } >> + } else { >> + bt_dev_info(hdev, "ROME setup"); >> + } >> + >> /* Setup user speed if needed */ >> speed = qca_get_speed(hu, QCA_OPER_SPEED); >> if (speed) { >> @@ -1016,16 +1162,18 @@ static int qca_setup(struct hci_uart *hu) >> qca_baudrate = qca_get_baudrate_value(speed); >> } >> >> - /* Get QCA version information */ >> - ret = qca_read_soc_version(hdev, &soc_ver); >> - if (ret < 0 || soc_ver == 0) { >> - bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); >> - return ret; >> + if (!soc_ver) { > > > The following would probably be clearer: > > if (qcadev->btsoc_type != QCA_WCN3990) { > >> + /* Get QCA version information */ >> + ret = qca_read_soc_version(hdev, &soc_ver); >> + if (ret < 0 || soc_ver == 0) { >> + bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); >> + return ret; >> + } >> + bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); > > Couldn't we hide some of this in qca_read_soc_version()? > [Bala]: we can hide soc_ver==0 in qca_read_soc_version() will update. > qca_read_soc_version() could at least do the logging in the error case > (the 'soc_ver == 0' case could be handled there as well), which would > leave us with: > > > ret = qca_read_soc_version(hdev, &soc_ver); > if (ret) > return ret; > > And the same above. > > The controller version can be logged outside of the branch for both > wcn3990 and Rome. > [Bala]: will update. >> } >> - bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); >> >> /* Setup patch / NVM configurations */ >> - ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver); >> + ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, >> soc_ver); >> if (!ret) { >> set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); >> qca_debugfs_init(hdev); >> @@ -1061,9 +1209,123 @@ static struct hci_uart_proto qca_proto = { >> .dequeue = qca_dequeue, >> }; >> >> +static const struct qca_vreg_data qca_soc_data = { >> + .soc_type = QCA_WCN3990, >> + .vregs = (struct qca_vreg []) { >> + { "vddio", 1800000, 1800000, 0 }, >> + { "vddxo", 1800000, 1800000, 1 }, >> + { "vddrf", 1304000, 1304000, 1 }, >> + { "vddch0", 3000000, 3312000, 1 }, >> + }, >> + .num_vregs = 4, >> +}; > > I didn't chime in earlier in the discussion with Stephen on the > regulators (https://patchwork.kernel.org/patch/10467911/), however I > agree with him that specifying at load of 1uA doesn't seem to make > much sense. What would happen if the load remained unspecified (or 0)? [Bala]: will test and get back to you on this.
Hi Bala, On Tue, Jul 10, 2018 at 06:22:02PM +0530, Balakrishna Godavarthi wrote: > Hi Matthias, > > On 2018-07-07 03:51, Matthias Kaehlcke wrote: > > On Thu, Jul 05, 2018 at 10:25:15PM +0530, Balakrishna Godavarthi wrote: > > > + * sending vendor power on and off pulse to SoC. > > > + */ > > > > The function is called qca_send_vendor_cmd(), the comment about power > > on/off pulses seems misplaced here. Are there other 'vendor commands' > > that require a different flow control behavior? > > > > Perhaps the function should have a different name or we need another > > wrapper. > > > > [Bala]: this function is used to send single byte vendor commands. like > power on and off pulse. > and all single byte vendor commands require an flow control off and > on. > so might be function name change is required. > instead of qca_send_vendor_cmd(). i want to change function name to > qca_send_pulse() as this will be generic. 'pulse' covers power on/off pulses, is it also applicable to other single byte commands? How are these commands named in the QCA documentation? In any case the comment about 'vendor power on and off pulse' should be updated to refer to 'vendor commands'/'pulses' or whatever terminology we decide to use. > > > + hci_uart_set_flow_control(hu, true); > > > > Should the changing of the flow control be limited to wcn3990? As of > > now the function is only called for wcn3990, however this is not > > stated as a requirement and might change in the future. > > > > > + skb_put_u8(skb, cmd); > > > + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT; > > > + > > > + skb_queue_tail(&qca->txq, skb); > > > + hci_uart_tx_wakeup(hu); > > > + > > > + /* Wait for 100 uS for SoC to settle down */ > > > + usleep_range(100, 200); > > > > Is this needed for any 'vendor command' or directly related with the > > power on/off pulses? > > > > [Bala] : flow control off/on is needed for power on and off pulses along > with > command to set baudrate.. this is common across all the present > chips and chips which are in development stages. > might be function name is confusing will update the function name. The current name *might* be ok, depending on if we come up with something better. If the QCA docs refer to them as 'vendor commands' and there are no other multi-byte 'vendor commands' I'm fine with it. Is the settling down of 100us specific to the power on/off pulses or also applicable to other commands? Thanks Matthias -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Matthias, On 2018-07-07 03:51, Matthias Kaehlcke wrote: > On Thu, Jul 05, 2018 at 10:25:15PM +0530, Balakrishna Godavarthi wrote: >> Add support to set voltage/current of various regulators >> to power up/down Bluetooth chip wcn3990. >> >> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> >> --- >> changes in v9: >> * moved flow control to vendor and set_baudarte functions. >> * removed parent regs. >> >> changes in v8: >> * closing qca buffer, if qca_power_setup fails >> * chnaged ibs start timer function call location. >> * updated review comments. >> >> changes in v7: >> * addressed review comments. >> >> changes in v6: >> * Hooked up qca_power to qca_serdev. >> * renamed all the naming inconsistency functions with qca_* >> * leveraged common code of ROME for wcn3990. >> * created wrapper functions for re-usable blocks. >> * updated function of _*regulator_enable and _*regualtor_disable. >> * removed redundant comments and functions. >> * addressed review comments. >> >> Changes in v5: >> * updated regulator vddpa min_uV to 1304000. >> * addressed review comments. >> >> Changes in v4: >> * Segregated the changes of btqca from hci_qca >> * rebased all changes on top of bluetooth-next. >> * addressed review comments. >> >> --- >> drivers/bluetooth/btqca.h | 3 + >> drivers/bluetooth/hci_qca.c | 387 >> +++++++++++++++++++++++++++++++----- >> 2 files changed, 345 insertions(+), 45 deletions(-) >> >> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h >> index a9c2779f3e07..0c01f375fe83 100644 >> --- a/drivers/bluetooth/btqca.h >> +++ b/drivers/bluetooth/btqca.h >> @@ -37,6 +37,9 @@ >> #define EDL_TAG_ID_HCI (17) >> #define EDL_TAG_ID_DEEP_SLEEP (27) >> >> +#define QCA_WCN3990_POWERON_PULSE 0xFC >> +#define QCA_WCN3990_POWEROFF_PULSE 0xC0 >> + >> enum qca_bardrate { >> QCA_BAUDRATE_115200 = 0, >> QCA_BAUDRATE_57600, >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index 7ebfaa0edf3f..d62c7785a618 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -5,7 +5,7 @@ >> * protocol extension to H4. >> * >> * Copyright (C) 2007 Texas Instruments, Inc. >> - * Copyright (c) 2010, 2012 The Linux Foundation. All rights >> reserved. >> + * Copyright (c) 2010, 2012, 2018 The Linux Foundation. All rights >> reserved. >> * >> * Acknowledgements: >> * This file is based on hci_ll.c, which was... >> @@ -31,9 +31,14 @@ >> #include <linux/kernel.h> >> #include <linux/clk.h> >> #include <linux/debugfs.h> >> +#include <linux/delay.h> >> +#include <linux/device.h> >> #include <linux/gpio/consumer.h> >> #include <linux/mod_devicetable.h> >> #include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/regulator/consumer.h> >> #include <linux/serdev.h> >> >> #include <net/bluetooth/bluetooth.h> >> @@ -124,12 +129,46 @@ enum qca_speed_type { >> QCA_OPER_SPEED >> }; >> >> +/* >> + * Voltage regulator information required for configuring the >> + * QCA Bluetooth chipset >> + */ >> +struct qca_vreg { >> + const char *name; >> + unsigned int min_uV; >> + unsigned int max_uV; >> + unsigned int load_uA; >> +}; >> + >> +struct qca_vreg_data { >> + enum qca_btsoc_type soc_type; >> + struct qca_vreg *vregs; >> + size_t num_vregs; >> +}; >> + >> +/* >> + * Platform data for the QCA Bluetooth power driver. >> + */ >> +struct qca_power { >> + struct device *dev; >> + const struct qca_vreg_data *vreg_data; >> + struct regulator_bulk_data *vreg_bulk; >> + bool vregs_on; >> +}; >> + >> struct qca_serdev { >> struct hci_uart serdev_hu; >> struct gpio_desc *bt_en; >> struct clk *susclk; >> + enum qca_btsoc_type btsoc_type; >> + struct qca_power *bt_power; >> + u32 init_speed; >> + u32 oper_speed; >> }; >> >> +static int qca_power_setup(struct hci_uart *hu, bool on); >> +static void qca_power_shutdown(struct hci_uart *hu); >> + >> static void __serial_clock_on(struct tty_struct *tty) >> { >> /* TODO: Some chipset requires to enable UART clock on client >> @@ -407,6 +446,7 @@ static int qca_open(struct hci_uart *hu) >> { >> struct qca_serdev *qcadev; >> struct qca_data *qca; >> + int ret; >> >> BT_DBG("hu %p qca_open", hu); >> >> @@ -458,19 +498,32 @@ static int qca_open(struct hci_uart *hu) >> >> hu->priv = qca; >> >> - timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, >> 0); >> - qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; >> - >> - timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); >> - qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; >> - >> if (hu->serdev) { >> serdev_device_open(hu->serdev); >> >> qcadev = serdev_device_get_drvdata(hu->serdev); >> - gpiod_set_value_cansleep(qcadev->bt_en, 1); >> + if (qcadev->btsoc_type != QCA_WCN3990) { >> + gpiod_set_value_cansleep(qcadev->bt_en, 1); >> + } else { >> + hu->init_speed = qcadev->init_speed; >> + hu->oper_speed = qcadev->oper_speed; >> + ret = qca_power_setup(hu, true); >> + if (ret) { >> + destroy_workqueue(qca->workqueue); >> + kfree_skb(qca->rx_skb); >> + hu->priv = NULL; >> + kfree(qca); >> + return ret; >> + } >> + } >> } >> >> + timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, >> 0); >> + qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; >> + >> + timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); >> + qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; >> + >> BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u", >> qca->tx_idle_delay, qca->wake_retrans); >> >> @@ -554,10 +607,13 @@ static int qca_close(struct hci_uart *hu) >> qca->hu = NULL; >> >> if (hu->serdev) { >> - serdev_device_close(hu->serdev); >> - >> qcadev = serdev_device_get_drvdata(hu->serdev); >> - gpiod_set_value_cansleep(qcadev->bt_en, 0); >> + if (qcadev->btsoc_type == QCA_WCN3990) >> + qca_power_shutdown(hu); >> + else >> + gpiod_set_value_cansleep(qcadev->bt_en, 0); >> + >> + serdev_device_close(hu->serdev); >> } >> >> kfree_skb(qca->rx_skb); >> @@ -891,6 +947,7 @@ 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) >> @@ -904,6 +961,13 @@ static int qca_set_baudrate(struct hci_dev *hdev, >> uint8_t baudrate) >> return -ENOMEM; >> } >> >> + /* Disabling hardware flow control is mandate while > > nit: s/mandate/mandatory|required/ > >> + * 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; >> @@ -919,6 +983,9 @@ 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; >> } >> >> @@ -930,6 +997,36 @@ static inline void host_set_baudrate(struct >> hci_uart *hu, unsigned int speed) >> hci_uart_set_baudrate(hu, speed); >> } >> >> +static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd) >> +{ >> + struct hci_uart *hu = hci_get_drvdata(hdev); >> + struct qca_data *qca = hu->priv; >> + struct sk_buff *skb; >> + >> + bt_dev_dbg(hdev, "sending command %02x to SoC", cmd); >> + >> + skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL); >> + if (!skb) >> + return -ENOMEM; >> + >> + /* Disabling hardware flow control is mandate while > > Same nit as above > >> + * sending vendor power on and off pulse to SoC. >> + */ > > The function is called qca_send_vendor_cmd(), the comment about power > on/off pulses seems misplaced here. Are there other 'vendor commands' > that require a different flow control behavior? > > Perhaps the function should have a different name or we need another > wrapper. > >> + hci_uart_set_flow_control(hu, true); > > Should the changing of the flow control be limited to wcn3990? As of > now the function is only called for wcn3990, however this is not > stated as a requirement and might change in the future. > >> + skb_put_u8(skb, cmd); >> + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT; >> + >> + skb_queue_tail(&qca->txq, skb); >> + hci_uart_tx_wakeup(hu); >> + >> + /* Wait for 100 uS for SoC to settle down */ >> + usleep_range(100, 200); > > Is this needed for any 'vendor command' or directly related with the > power on/off pulses? > >> + hci_uart_set_flow_control(hu, false); >> + >> + return 0; >> +} >> + >> static unsigned int qca_get_speed(struct hci_uart *hu, >> enum qca_speed_type speed_type) >> { >> @@ -952,10 +1049,19 @@ static unsigned int qca_get_speed(struct >> hci_uart *hu, >> >> static int qca_check_speeds(struct hci_uart *hu) >> { >> - /* One or the other speeds should be non zero. */ >> - if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> - !qca_get_speed(hu, QCA_OPER_SPEED)) >> + struct qca_serdev *qcadev; >> + >> + qcadev = serdev_device_get_drvdata(hu->serdev); >> + if ((qcadev->btsoc_type == QCA_WCN3990 && >> + !qca_get_speed(hu, QCA_INIT_SPEED)) || >> + !qca_get_speed(hu, QCA_OPER_SPEED)) { >> + /* Both INIT and OPER speed should be non zero. */ >> return -EINVAL; >> + } else if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> + !qca_get_speed(hu, QCA_OPER_SPEED)) { >> + /* One or the other speeds should be non zero. */ >> + return -EINVAL; >> + } > > It is questionable if the comments are useful, they are basically > stating the same as the conditions. > > if-else statements with a single statement in all branches shouldn't > use curly braces. Personally I don't dislike them in this case with > the multi-line conditions, but in principle they shoulnd't be there. > > nit: this would be easier to read with nested if statements: > > if (qcadev->btsoc_type == QCA_WCN3990) > if (!qca_get_speed(hu, QCA_INIT_SPEED)) || > !qca_get_speed(hu, QCA_OPER_SPEED)) > return -EINVAL; > else > if (!qca_get_speed(hu, QCA_INIT_SPEED) && > !qca_get_speed(hu, QCA_OPER_SPEED)) { > return -EINVAL; > >> return 0; >> } >> @@ -986,15 +1092,40 @@ static int qca_set_speed(struct hci_uart *hu, >> enum qca_speed_type speed_type) >> return 0; >> } >> >> +static int qca_wcn3990_init(struct hci_uart *hu) >> +{ >> + int ret; >> + >> + ret = qca_send_vendor_cmd(hu->hdev, QCA_WCN3990_POWERON_PULSE); >> + if (ret) >> + return ret; >> + >> + /* Wait for 100 ms for SoC to boot up */ >> + msleep(100); >> + serdev_device_close(hu->serdev); >> + ret = serdev_device_open(hu->serdev); >> + if (ret) { >> + bt_dev_err(hu->hdev, "failed to open port"); >> + return ret; >> + } >> + >> + /* Setup initial baudrate */ >> + qca_set_speed(hu, QCA_INIT_SPEED); > > The comment is a bit redundant. > >> + hci_uart_set_flow_control(hu, false); >> + >> + return 0; >> +} >> + >> static int qca_setup(struct hci_uart *hu) >> { >> struct hci_dev *hdev = hu->hdev; >> struct qca_data *qca = hu->priv; >> unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; >> + struct qca_serdev *qcadev; >> int ret; >> int soc_ver = 0; >> >> - bt_dev_info(hdev, "ROME setup"); >> + qcadev = serdev_device_get_drvdata(hu->serdev); >> >> ret = qca_check_speeds(hu); >> if (ret) >> @@ -1006,6 +1137,21 @@ static int qca_setup(struct hci_uart *hu) >> /* Setup initial baudrate */ >> qca_set_speed(hu, QCA_INIT_SPEED); >> >> + if (qcadev->btsoc_type == QCA_WCN3990) { >> + bt_dev_dbg(hdev, "setting up wcn3990"); > > Seems like this should be bt_dev_info() for consistency with Rome. > >> + ret = qca_wcn3990_init(hu); >> + if (ret) >> + return ret; >> + >> + ret = qca_read_soc_version(hdev, &soc_ver); >> + if (ret < 0 || soc_ver == 0) { >> + bt_dev_err(hdev, "Failed to get version %d", ret); >> + return ret; >> + } >> + } else { >> + bt_dev_info(hdev, "ROME setup"); >> + } >> + >> /* Setup user speed if needed */ >> speed = qca_get_speed(hu, QCA_OPER_SPEED); >> if (speed) { >> @@ -1016,16 +1162,18 @@ static int qca_setup(struct hci_uart *hu) >> qca_baudrate = qca_get_baudrate_value(speed); >> } >> >> - /* Get QCA version information */ >> - ret = qca_read_soc_version(hdev, &soc_ver); >> - if (ret < 0 || soc_ver == 0) { >> - bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); >> - return ret; >> + if (!soc_ver) { > > > The following would probably be clearer: > > if (qcadev->btsoc_type != QCA_WCN3990) { > >> + /* Get QCA version information */ >> + ret = qca_read_soc_version(hdev, &soc_ver); >> + if (ret < 0 || soc_ver == 0) { >> + bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); >> + return ret; >> + } >> + bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); > > Couldn't we hide some of this in qca_read_soc_version()? > > qca_read_soc_version() could at least do the logging in the error case > (the 'soc_ver == 0' case could be handled there as well), which would > leave us with: > > > ret = qca_read_soc_version(hdev, &soc_ver); > if (ret) > return ret; > > And the same above. > > The controller version can be logged outside of the branch for both > wcn3990 and Rome. > >> } >> - bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); >> >> /* Setup patch / NVM configurations */ >> - ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver); >> + ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, >> soc_ver); >> if (!ret) { >> set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); >> qca_debugfs_init(hdev); >> @@ -1061,9 +1209,123 @@ static struct hci_uart_proto qca_proto = { >> .dequeue = qca_dequeue, >> }; >> >> +static const struct qca_vreg_data qca_soc_data = { >> + .soc_type = QCA_WCN3990, >> + .vregs = (struct qca_vreg []) { >> + { "vddio", 1800000, 1800000, 0 }, >> + { "vddxo", 1800000, 1800000, 1 }, >> + { "vddrf", 1304000, 1304000, 1 }, >> + { "vddch0", 3000000, 3312000, 1 }, >> + }, >> + .num_vregs = 4, >> +}; > > I didn't chime in earlier in the discussion with Stephen on the > regulators (https://patchwork.kernel.org/patch/10467911/), however I > agree with him that specifying at load of 1uA doesn't seem to make > much sense. What would happen if the load remained unspecified (or 0)? [Bala]: On RPMh based designs, calling a regulator_set_load with a non-zero value moves the regulator to NPM (high power mode) which is required for BT communication. and call with 0uA moves it to low-power-mode (vote from APPS). Basically whenever BT wants these in NPM it calls set-load with 1.
On Mon, Jul 16, 2018 at 07:21:56PM +0530, Balakrishna Godavarthi wrote: > Hi Matthias, > > On 2018-07-07 03:51, Matthias Kaehlcke wrote: > > On Thu, Jul 05, 2018 at 10:25:15PM +0530, Balakrishna Godavarthi wrote: > > > Add support to set voltage/current of various regulators > > > to power up/down Bluetooth chip wcn3990. > > > > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> > > > --- > > > changes in v9: > > > * moved flow control to vendor and set_baudarte functions. > > > * removed parent regs. > > > > > > changes in v8: > > > * closing qca buffer, if qca_power_setup fails > > > * chnaged ibs start timer function call location. > > > * updated review comments. > > > > > > changes in v7: > > > * addressed review comments. > > > > > > changes in v6: > > > * Hooked up qca_power to qca_serdev. > > > * renamed all the naming inconsistency functions with qca_* > > > * leveraged common code of ROME for wcn3990. > > > * created wrapper functions for re-usable blocks. > > > * updated function of _*regulator_enable and _*regualtor_disable. > > > * removed redundant comments and functions. > > > * addressed review comments. > > > > > > Changes in v5: > > > * updated regulator vddpa min_uV to 1304000. > > > * addressed review comments. > > > > > > Changes in v4: > > > * Segregated the changes of btqca from hci_qca > > > * rebased all changes on top of bluetooth-next. > > > * addressed review comments. > > > > > > --- > > > drivers/bluetooth/btqca.h | 3 + > > > drivers/bluetooth/hci_qca.c | 387 > > > +++++++++++++++++++++++++++++++----- > > > 2 files changed, 345 insertions(+), 45 deletions(-) > > > > > > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h > > > index a9c2779f3e07..0c01f375fe83 100644 > > > --- a/drivers/bluetooth/btqca.h > > > +++ b/drivers/bluetooth/btqca.h > > > @@ -37,6 +37,9 @@ > > > #define EDL_TAG_ID_HCI (17) > > > #define EDL_TAG_ID_DEEP_SLEEP (27) > > > > > > +#define QCA_WCN3990_POWERON_PULSE 0xFC > > > +#define QCA_WCN3990_POWEROFF_PULSE 0xC0 > > > + > > > enum qca_bardrate { > > > QCA_BAUDRATE_115200 = 0, > > > QCA_BAUDRATE_57600, > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > > > index 7ebfaa0edf3f..d62c7785a618 100644 > > > --- a/drivers/bluetooth/hci_qca.c > > > +++ b/drivers/bluetooth/hci_qca.c > > > @@ -5,7 +5,7 @@ > > > * protocol extension to H4. > > > * > > > * Copyright (C) 2007 Texas Instruments, Inc. > > > - * Copyright (c) 2010, 2012 The Linux Foundation. All rights > > > reserved. > > > + * Copyright (c) 2010, 2012, 2018 The Linux Foundation. All rights > > > reserved. > > > * > > > * Acknowledgements: > > > * This file is based on hci_ll.c, which was... > > > @@ -31,9 +31,14 @@ > > > #include <linux/kernel.h> > > > #include <linux/clk.h> > > > #include <linux/debugfs.h> > > > +#include <linux/delay.h> > > > +#include <linux/device.h> > > > #include <linux/gpio/consumer.h> > > > #include <linux/mod_devicetable.h> > > > #include <linux/module.h> > > > +#include <linux/of_device.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/regulator/consumer.h> > > > #include <linux/serdev.h> > > > > > > #include <net/bluetooth/bluetooth.h> > > > @@ -124,12 +129,46 @@ enum qca_speed_type { > > > QCA_OPER_SPEED > > > }; > > > > > > +/* > > > + * Voltage regulator information required for configuring the > > > + * QCA Bluetooth chipset > > > + */ > > > +struct qca_vreg { > > > + const char *name; > > > + unsigned int min_uV; > > > + unsigned int max_uV; > > > + unsigned int load_uA; > > > +}; > > > + > > > +struct qca_vreg_data { > > > + enum qca_btsoc_type soc_type; > > > + struct qca_vreg *vregs; > > > + size_t num_vregs; > > > +}; > > > + > > > +/* > > > + * Platform data for the QCA Bluetooth power driver. > > > + */ > > > +struct qca_power { > > > + struct device *dev; > > > + const struct qca_vreg_data *vreg_data; > > > + struct regulator_bulk_data *vreg_bulk; > > > + bool vregs_on; > > > +}; > > > + > > > struct qca_serdev { > > > struct hci_uart serdev_hu; > > > struct gpio_desc *bt_en; > > > struct clk *susclk; > > > + enum qca_btsoc_type btsoc_type; > > > + struct qca_power *bt_power; > > > + u32 init_speed; > > > + u32 oper_speed; > > > }; > > > > > > +static int qca_power_setup(struct hci_uart *hu, bool on); > > > +static void qca_power_shutdown(struct hci_uart *hu); > > > + > > > static void __serial_clock_on(struct tty_struct *tty) > > > { > > > /* TODO: Some chipset requires to enable UART clock on client > > > @@ -407,6 +446,7 @@ static int qca_open(struct hci_uart *hu) > > > { > > > struct qca_serdev *qcadev; > > > struct qca_data *qca; > > > + int ret; > > > > > > BT_DBG("hu %p qca_open", hu); > > > > > > @@ -458,19 +498,32 @@ static int qca_open(struct hci_uart *hu) > > > > > > hu->priv = qca; > > > > > > - timer_setup(&qca->wake_retrans_timer, > > > hci_ibs_wake_retrans_timeout, 0); > > > - qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; > > > - > > > - timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); > > > - qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; > > > - > > > if (hu->serdev) { > > > serdev_device_open(hu->serdev); > > > > > > qcadev = serdev_device_get_drvdata(hu->serdev); > > > - gpiod_set_value_cansleep(qcadev->bt_en, 1); > > > + if (qcadev->btsoc_type != QCA_WCN3990) { > > > + gpiod_set_value_cansleep(qcadev->bt_en, 1); > > > + } else { > > > + hu->init_speed = qcadev->init_speed; > > > + hu->oper_speed = qcadev->oper_speed; > > > + ret = qca_power_setup(hu, true); > > > + if (ret) { > > > + destroy_workqueue(qca->workqueue); > > > + kfree_skb(qca->rx_skb); > > > + hu->priv = NULL; > > > + kfree(qca); > > > + return ret; > > > + } > > > + } > > > } > > > > > > + timer_setup(&qca->wake_retrans_timer, > > > hci_ibs_wake_retrans_timeout, 0); > > > + qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; > > > + > > > + timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); > > > + qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; > > > + > > > BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u", > > > qca->tx_idle_delay, qca->wake_retrans); > > > > > > @@ -554,10 +607,13 @@ static int qca_close(struct hci_uart *hu) > > > qca->hu = NULL; > > > > > > if (hu->serdev) { > > > - serdev_device_close(hu->serdev); > > > - > > > qcadev = serdev_device_get_drvdata(hu->serdev); > > > - gpiod_set_value_cansleep(qcadev->bt_en, 0); > > > + if (qcadev->btsoc_type == QCA_WCN3990) > > > + qca_power_shutdown(hu); > > > + else > > > + gpiod_set_value_cansleep(qcadev->bt_en, 0); > > > + > > > + serdev_device_close(hu->serdev); > > > } > > > > > > kfree_skb(qca->rx_skb); > > > @@ -891,6 +947,7 @@ 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) > > > @@ -904,6 +961,13 @@ static int qca_set_baudrate(struct hci_dev > > > *hdev, uint8_t baudrate) > > > return -ENOMEM; > > > } > > > > > > + /* Disabling hardware flow control is mandate while > > > > nit: s/mandate/mandatory|required/ > > > > > + * 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; > > > @@ -919,6 +983,9 @@ 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; > > > } > > > > > > @@ -930,6 +997,36 @@ static inline void host_set_baudrate(struct > > > hci_uart *hu, unsigned int speed) > > > hci_uart_set_baudrate(hu, speed); > > > } > > > > > > +static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd) > > > +{ > > > + struct hci_uart *hu = hci_get_drvdata(hdev); > > > + struct qca_data *qca = hu->priv; > > > + struct sk_buff *skb; > > > + > > > + bt_dev_dbg(hdev, "sending command %02x to SoC", cmd); > > > + > > > + skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL); > > > + if (!skb) > > > + return -ENOMEM; > > > + > > > + /* Disabling hardware flow control is mandate while > > > > Same nit as above > > > > > + * sending vendor power on and off pulse to SoC. > > > + */ > > > > The function is called qca_send_vendor_cmd(), the comment about power > > on/off pulses seems misplaced here. Are there other 'vendor commands' > > that require a different flow control behavior? > > > > Perhaps the function should have a different name or we need another > > wrapper. > > > > > + hci_uart_set_flow_control(hu, true); > > > > Should the changing of the flow control be limited to wcn3990? As of > > now the function is only called for wcn3990, however this is not > > stated as a requirement and might change in the future. > > > > > + skb_put_u8(skb, cmd); > > > + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT; > > > + > > > + skb_queue_tail(&qca->txq, skb); > > > + hci_uart_tx_wakeup(hu); > > > + > > > + /* Wait for 100 uS for SoC to settle down */ > > > + usleep_range(100, 200); > > > > Is this needed for any 'vendor command' or directly related with the > > power on/off pulses? > > > > > + hci_uart_set_flow_control(hu, false); > > > + > > > + return 0; > > > +} > > > + > > > static unsigned int qca_get_speed(struct hci_uart *hu, > > > enum qca_speed_type speed_type) > > > { > > > @@ -952,10 +1049,19 @@ static unsigned int qca_get_speed(struct > > > hci_uart *hu, > > > > > > static int qca_check_speeds(struct hci_uart *hu) > > > { > > > - /* One or the other speeds should be non zero. */ > > > - if (!qca_get_speed(hu, QCA_INIT_SPEED) && > > > - !qca_get_speed(hu, QCA_OPER_SPEED)) > > > + struct qca_serdev *qcadev; > > > + > > > + qcadev = serdev_device_get_drvdata(hu->serdev); > > > + if ((qcadev->btsoc_type == QCA_WCN3990 && > > > + !qca_get_speed(hu, QCA_INIT_SPEED)) || > > > + !qca_get_speed(hu, QCA_OPER_SPEED)) { > > > + /* Both INIT and OPER speed should be non zero. */ > > > return -EINVAL; > > > + } else if (!qca_get_speed(hu, QCA_INIT_SPEED) && > > > + !qca_get_speed(hu, QCA_OPER_SPEED)) { > > > + /* One or the other speeds should be non zero. */ > > > + return -EINVAL; > > > + } > > > > It is questionable if the comments are useful, they are basically > > stating the same as the conditions. > > > > if-else statements with a single statement in all branches shouldn't > > use curly braces. Personally I don't dislike them in this case with > > the multi-line conditions, but in principle they shoulnd't be there. > > > > nit: this would be easier to read with nested if statements: > > > > if (qcadev->btsoc_type == QCA_WCN3990) > > if (!qca_get_speed(hu, QCA_INIT_SPEED)) || > > !qca_get_speed(hu, QCA_OPER_SPEED)) > > return -EINVAL; > > else > > if (!qca_get_speed(hu, QCA_INIT_SPEED) && > > !qca_get_speed(hu, QCA_OPER_SPEED)) { > > return -EINVAL; > > > > > return 0; > > > } > > > @@ -986,15 +1092,40 @@ static int qca_set_speed(struct hci_uart *hu, > > > enum qca_speed_type speed_type) > > > return 0; > > > } > > > > > > +static int qca_wcn3990_init(struct hci_uart *hu) > > > +{ > > > + int ret; > > > + > > > + ret = qca_send_vendor_cmd(hu->hdev, QCA_WCN3990_POWERON_PULSE); > > > + if (ret) > > > + return ret; > > > + > > > + /* Wait for 100 ms for SoC to boot up */ > > > + msleep(100); > > > + serdev_device_close(hu->serdev); > > > + ret = serdev_device_open(hu->serdev); > > > + if (ret) { > > > + bt_dev_err(hu->hdev, "failed to open port"); > > > + return ret; > > > + } > > > + > > > + /* Setup initial baudrate */ > > > + qca_set_speed(hu, QCA_INIT_SPEED); > > > > The comment is a bit redundant. > > > > > + hci_uart_set_flow_control(hu, false); > > > + > > > + return 0; > > > +} > > > + > > > static int qca_setup(struct hci_uart *hu) > > > { > > > struct hci_dev *hdev = hu->hdev; > > > struct qca_data *qca = hu->priv; > > > unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; > > > + struct qca_serdev *qcadev; > > > int ret; > > > int soc_ver = 0; > > > > > > - bt_dev_info(hdev, "ROME setup"); > > > + qcadev = serdev_device_get_drvdata(hu->serdev); > > > > > > ret = qca_check_speeds(hu); > > > if (ret) > > > @@ -1006,6 +1137,21 @@ static int qca_setup(struct hci_uart *hu) > > > /* Setup initial baudrate */ > > > qca_set_speed(hu, QCA_INIT_SPEED); > > > > > > + if (qcadev->btsoc_type == QCA_WCN3990) { > > > + bt_dev_dbg(hdev, "setting up wcn3990"); > > > > Seems like this should be bt_dev_info() for consistency with Rome. > > > > > + ret = qca_wcn3990_init(hu); > > > + if (ret) > > > + return ret; > > > + > > > + ret = qca_read_soc_version(hdev, &soc_ver); > > > + if (ret < 0 || soc_ver == 0) { > > > + bt_dev_err(hdev, "Failed to get version %d", ret); > > > + return ret; > > > + } > > > + } else { > > > + bt_dev_info(hdev, "ROME setup"); > > > + } > > > + > > > /* Setup user speed if needed */ > > > speed = qca_get_speed(hu, QCA_OPER_SPEED); > > > if (speed) { > > > @@ -1016,16 +1162,18 @@ static int qca_setup(struct hci_uart *hu) > > > qca_baudrate = qca_get_baudrate_value(speed); > > > } > > > > > > - /* Get QCA version information */ > > > - ret = qca_read_soc_version(hdev, &soc_ver); > > > - if (ret < 0 || soc_ver == 0) { > > > - bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); > > > - return ret; > > > + if (!soc_ver) { > > > > > > The following would probably be clearer: > > > > if (qcadev->btsoc_type != QCA_WCN3990) { > > > > > + /* Get QCA version information */ > > > + ret = qca_read_soc_version(hdev, &soc_ver); > > > + if (ret < 0 || soc_ver == 0) { > > > + bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); > > > + return ret; > > > + } > > > + bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); > > > > Couldn't we hide some of this in qca_read_soc_version()? > > > > qca_read_soc_version() could at least do the logging in the error case > > (the 'soc_ver == 0' case could be handled there as well), which would > > leave us with: > > > > > > ret = qca_read_soc_version(hdev, &soc_ver); > > if (ret) > > return ret; > > > > And the same above. > > > > The controller version can be logged outside of the branch for both > > wcn3990 and Rome. > > > > > } > > > - bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); > > > > > > /* Setup patch / NVM configurations */ > > > - ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver); > > > + ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, > > > soc_ver); > > > if (!ret) { > > > set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); > > > qca_debugfs_init(hdev); > > > @@ -1061,9 +1209,123 @@ static struct hci_uart_proto qca_proto = { > > > .dequeue = qca_dequeue, > > > }; > > > > > > +static const struct qca_vreg_data qca_soc_data = { > > > + .soc_type = QCA_WCN3990, > > > + .vregs = (struct qca_vreg []) { > > > + { "vddio", 1800000, 1800000, 0 }, > > > + { "vddxo", 1800000, 1800000, 1 }, > > > + { "vddrf", 1304000, 1304000, 1 }, > > > + { "vddch0", 3000000, 3312000, 1 }, > > > + }, > > > + .num_vregs = 4, > > > +}; > > > > I didn't chime in earlier in the discussion with Stephen on the > > regulators (https://patchwork.kernel.org/patch/10467911/), however I > > agree with him that specifying at load of 1uA doesn't seem to make > > much sense. What would happen if the load remained unspecified (or 0)? > > [Bala]: On RPMh based designs, calling a regulator_set_load with a non-zero > value moves the regulator to NPM (high power mode) which is required for BT > communication. > and call with 0uA moves it to low-power-mode (vote from APPS). > Basically whenever BT wants these in NPM it calls set-load with 1. This assumes that the chip is powered by RPMh regulators, which depending on the system may or may not be true. On systems with other regulators you are telling the regulator that the max load is 1uA, and the regulator might comply, going in a low power mode that does not provide enough current for the chip to operate properly. Doesn't the datasheet of the chip specify actual max currents for these consumers that could be used instead of the bogus 1uA? If not you could ask the designers of the chip for an approximate number (preferably erring on the upper side). -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Matthias, On 2018-07-16 21:35, Matthias Kaehlcke wrote: > On Mon, Jul 16, 2018 at 07:21:56PM +0530, Balakrishna Godavarthi wrote: >> Hi Matthias, >> >> On 2018-07-07 03:51, Matthias Kaehlcke wrote: >> > On Thu, Jul 05, 2018 at 10:25:15PM +0530, Balakrishna Godavarthi wrote: >> > > Add support to set voltage/current of various regulators >> > > to power up/down Bluetooth chip wcn3990. >> > > >> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> >> > > --- >> > > changes in v9: >> > > * moved flow control to vendor and set_baudarte functions. >> > > * removed parent regs. >> > > >> > > changes in v8: >> > > * closing qca buffer, if qca_power_setup fails >> > > * chnaged ibs start timer function call location. >> > > * updated review comments. >> > > >> > > changes in v7: >> > > * addressed review comments. >> > > >> > > changes in v6: >> > > * Hooked up qca_power to qca_serdev. >> > > * renamed all the naming inconsistency functions with qca_* >> > > * leveraged common code of ROME for wcn3990. >> > > * created wrapper functions for re-usable blocks. >> > > * updated function of _*regulator_enable and _*regualtor_disable. >> > > * removed redundant comments and functions. >> > > * addressed review comments. >> > > >> > > Changes in v5: >> > > * updated regulator vddpa min_uV to 1304000. >> > > * addressed review comments. >> > > >> > > Changes in v4: >> > > * Segregated the changes of btqca from hci_qca >> > > * rebased all changes on top of bluetooth-next. >> > > * addressed review comments. >> > > >> > > --- >> > > drivers/bluetooth/btqca.h | 3 + >> > > drivers/bluetooth/hci_qca.c | 387 >> > > +++++++++++++++++++++++++++++++----- >> > > 2 files changed, 345 insertions(+), 45 deletions(-) >> > > >> > > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h >> > > index a9c2779f3e07..0c01f375fe83 100644 >> > > --- a/drivers/bluetooth/btqca.h >> > > +++ b/drivers/bluetooth/btqca.h >> > > @@ -37,6 +37,9 @@ >> > > #define EDL_TAG_ID_HCI (17) >> > > #define EDL_TAG_ID_DEEP_SLEEP (27) >> > > >> > > +#define QCA_WCN3990_POWERON_PULSE 0xFC >> > > +#define QCA_WCN3990_POWEROFF_PULSE 0xC0 >> > > + >> > > enum qca_bardrate { >> > > QCA_BAUDRATE_115200 = 0, >> > > QCA_BAUDRATE_57600, >> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> > > index 7ebfaa0edf3f..d62c7785a618 100644 >> > > --- a/drivers/bluetooth/hci_qca.c >> > > +++ b/drivers/bluetooth/hci_qca.c >> > > @@ -5,7 +5,7 @@ >> > > * protocol extension to H4. >> > > * >> > > * Copyright (C) 2007 Texas Instruments, Inc. >> > > - * Copyright (c) 2010, 2012 The Linux Foundation. All rights >> > > reserved. >> > > + * Copyright (c) 2010, 2012, 2018 The Linux Foundation. All rights >> > > reserved. >> > > * >> > > * Acknowledgements: >> > > * This file is based on hci_ll.c, which was... >> > > @@ -31,9 +31,14 @@ >> > > #include <linux/kernel.h> >> > > #include <linux/clk.h> >> > > #include <linux/debugfs.h> >> > > +#include <linux/delay.h> >> > > +#include <linux/device.h> >> > > #include <linux/gpio/consumer.h> >> > > #include <linux/mod_devicetable.h> >> > > #include <linux/module.h> >> > > +#include <linux/of_device.h> >> > > +#include <linux/platform_device.h> >> > > +#include <linux/regulator/consumer.h> >> > > #include <linux/serdev.h> >> > > >> > > #include <net/bluetooth/bluetooth.h> >> > > @@ -124,12 +129,46 @@ enum qca_speed_type { >> > > QCA_OPER_SPEED >> > > }; >> > > >> > > +/* >> > > + * Voltage regulator information required for configuring the >> > > + * QCA Bluetooth chipset >> > > + */ >> > > +struct qca_vreg { >> > > + const char *name; >> > > + unsigned int min_uV; >> > > + unsigned int max_uV; >> > > + unsigned int load_uA; >> > > +}; >> > > + >> > > +struct qca_vreg_data { >> > > + enum qca_btsoc_type soc_type; >> > > + struct qca_vreg *vregs; >> > > + size_t num_vregs; >> > > +}; >> > > + >> > > +/* >> > > + * Platform data for the QCA Bluetooth power driver. >> > > + */ >> > > +struct qca_power { >> > > + struct device *dev; >> > > + const struct qca_vreg_data *vreg_data; >> > > + struct regulator_bulk_data *vreg_bulk; >> > > + bool vregs_on; >> > > +}; >> > > + >> > > struct qca_serdev { >> > > struct hci_uart serdev_hu; >> > > struct gpio_desc *bt_en; >> > > struct clk *susclk; >> > > + enum qca_btsoc_type btsoc_type; >> > > + struct qca_power *bt_power; >> > > + u32 init_speed; >> > > + u32 oper_speed; >> > > }; >> > > >> > > +static int qca_power_setup(struct hci_uart *hu, bool on); >> > > +static void qca_power_shutdown(struct hci_uart *hu); >> > > + >> > > static void __serial_clock_on(struct tty_struct *tty) >> > > { >> > > /* TODO: Some chipset requires to enable UART clock on client >> > > @@ -407,6 +446,7 @@ static int qca_open(struct hci_uart *hu) >> > > { >> > > struct qca_serdev *qcadev; >> > > struct qca_data *qca; >> > > + int ret; >> > > >> > > BT_DBG("hu %p qca_open", hu); >> > > >> > > @@ -458,19 +498,32 @@ static int qca_open(struct hci_uart *hu) >> > > >> > > hu->priv = qca; >> > > >> > > - timer_setup(&qca->wake_retrans_timer, >> > > hci_ibs_wake_retrans_timeout, 0); >> > > - qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; >> > > - >> > > - timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); >> > > - qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; >> > > - >> > > if (hu->serdev) { >> > > serdev_device_open(hu->serdev); >> > > >> > > qcadev = serdev_device_get_drvdata(hu->serdev); >> > > - gpiod_set_value_cansleep(qcadev->bt_en, 1); >> > > + if (qcadev->btsoc_type != QCA_WCN3990) { >> > > + gpiod_set_value_cansleep(qcadev->bt_en, 1); >> > > + } else { >> > > + hu->init_speed = qcadev->init_speed; >> > > + hu->oper_speed = qcadev->oper_speed; >> > > + ret = qca_power_setup(hu, true); >> > > + if (ret) { >> > > + destroy_workqueue(qca->workqueue); >> > > + kfree_skb(qca->rx_skb); >> > > + hu->priv = NULL; >> > > + kfree(qca); >> > > + return ret; >> > > + } >> > > + } >> > > } >> > > >> > > + timer_setup(&qca->wake_retrans_timer, >> > > hci_ibs_wake_retrans_timeout, 0); >> > > + qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; >> > > + >> > > + timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); >> > > + qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; >> > > + >> > > BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u", >> > > qca->tx_idle_delay, qca->wake_retrans); >> > > >> > > @@ -554,10 +607,13 @@ static int qca_close(struct hci_uart *hu) >> > > qca->hu = NULL; >> > > >> > > if (hu->serdev) { >> > > - serdev_device_close(hu->serdev); >> > > - >> > > qcadev = serdev_device_get_drvdata(hu->serdev); >> > > - gpiod_set_value_cansleep(qcadev->bt_en, 0); >> > > + if (qcadev->btsoc_type == QCA_WCN3990) >> > > + qca_power_shutdown(hu); >> > > + else >> > > + gpiod_set_value_cansleep(qcadev->bt_en, 0); >> > > + >> > > + serdev_device_close(hu->serdev); >> > > } >> > > >> > > kfree_skb(qca->rx_skb); >> > > @@ -891,6 +947,7 @@ 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) >> > > @@ -904,6 +961,13 @@ static int qca_set_baudrate(struct hci_dev >> > > *hdev, uint8_t baudrate) >> > > return -ENOMEM; >> > > } >> > > >> > > + /* Disabling hardware flow control is mandate while >> > >> > nit: s/mandate/mandatory|required/ >> > >> > > + * 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; >> > > @@ -919,6 +983,9 @@ 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; >> > > } >> > > >> > > @@ -930,6 +997,36 @@ static inline void host_set_baudrate(struct >> > > hci_uart *hu, unsigned int speed) >> > > hci_uart_set_baudrate(hu, speed); >> > > } >> > > >> > > +static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd) >> > > +{ >> > > + struct hci_uart *hu = hci_get_drvdata(hdev); >> > > + struct qca_data *qca = hu->priv; >> > > + struct sk_buff *skb; >> > > + >> > > + bt_dev_dbg(hdev, "sending command %02x to SoC", cmd); >> > > + >> > > + skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL); >> > > + if (!skb) >> > > + return -ENOMEM; >> > > + >> > > + /* Disabling hardware flow control is mandate while >> > >> > Same nit as above >> > >> > > + * sending vendor power on and off pulse to SoC. >> > > + */ >> > >> > The function is called qca_send_vendor_cmd(), the comment about power >> > on/off pulses seems misplaced here. Are there other 'vendor commands' >> > that require a different flow control behavior? >> > >> > Perhaps the function should have a different name or we need another >> > wrapper. >> > >> > > + hci_uart_set_flow_control(hu, true); >> > >> > Should the changing of the flow control be limited to wcn3990? As of >> > now the function is only called for wcn3990, however this is not >> > stated as a requirement and might change in the future. >> > >> > > + skb_put_u8(skb, cmd); >> > > + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT; >> > > + >> > > + skb_queue_tail(&qca->txq, skb); >> > > + hci_uart_tx_wakeup(hu); >> > > + >> > > + /* Wait for 100 uS for SoC to settle down */ >> > > + usleep_range(100, 200); >> > >> > Is this needed for any 'vendor command' or directly related with the >> > power on/off pulses? >> > >> > > + hci_uart_set_flow_control(hu, false); >> > > + >> > > + return 0; >> > > +} >> > > + >> > > static unsigned int qca_get_speed(struct hci_uart *hu, >> > > enum qca_speed_type speed_type) >> > > { >> > > @@ -952,10 +1049,19 @@ static unsigned int qca_get_speed(struct >> > > hci_uart *hu, >> > > >> > > static int qca_check_speeds(struct hci_uart *hu) >> > > { >> > > - /* One or the other speeds should be non zero. */ >> > > - if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> > > - !qca_get_speed(hu, QCA_OPER_SPEED)) >> > > + struct qca_serdev *qcadev; >> > > + >> > > + qcadev = serdev_device_get_drvdata(hu->serdev); >> > > + if ((qcadev->btsoc_type == QCA_WCN3990 && >> > > + !qca_get_speed(hu, QCA_INIT_SPEED)) || >> > > + !qca_get_speed(hu, QCA_OPER_SPEED)) { >> > > + /* Both INIT and OPER speed should be non zero. */ >> > > return -EINVAL; >> > > + } else if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> > > + !qca_get_speed(hu, QCA_OPER_SPEED)) { >> > > + /* One or the other speeds should be non zero. */ >> > > + return -EINVAL; >> > > + } >> > >> > It is questionable if the comments are useful, they are basically >> > stating the same as the conditions. >> > >> > if-else statements with a single statement in all branches shouldn't >> > use curly braces. Personally I don't dislike them in this case with >> > the multi-line conditions, but in principle they shoulnd't be there. >> > >> > nit: this would be easier to read with nested if statements: >> > >> > if (qcadev->btsoc_type == QCA_WCN3990) >> > if (!qca_get_speed(hu, QCA_INIT_SPEED)) || >> > !qca_get_speed(hu, QCA_OPER_SPEED)) >> > return -EINVAL; >> > else >> > if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> > !qca_get_speed(hu, QCA_OPER_SPEED)) { >> > return -EINVAL; >> > >> > > return 0; >> > > } >> > > @@ -986,15 +1092,40 @@ static int qca_set_speed(struct hci_uart *hu, >> > > enum qca_speed_type speed_type) >> > > return 0; >> > > } >> > > >> > > +static int qca_wcn3990_init(struct hci_uart *hu) >> > > +{ >> > > + int ret; >> > > + >> > > + ret = qca_send_vendor_cmd(hu->hdev, QCA_WCN3990_POWERON_PULSE); >> > > + if (ret) >> > > + return ret; >> > > + >> > > + /* Wait for 100 ms for SoC to boot up */ >> > > + msleep(100); >> > > + serdev_device_close(hu->serdev); >> > > + ret = serdev_device_open(hu->serdev); >> > > + if (ret) { >> > > + bt_dev_err(hu->hdev, "failed to open port"); >> > > + return ret; >> > > + } >> > > + >> > > + /* Setup initial baudrate */ >> > > + qca_set_speed(hu, QCA_INIT_SPEED); >> > >> > The comment is a bit redundant. >> > >> > > + hci_uart_set_flow_control(hu, false); >> > > + >> > > + return 0; >> > > +} >> > > + >> > > static int qca_setup(struct hci_uart *hu) >> > > { >> > > struct hci_dev *hdev = hu->hdev; >> > > struct qca_data *qca = hu->priv; >> > > unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; >> > > + struct qca_serdev *qcadev; >> > > int ret; >> > > int soc_ver = 0; >> > > >> > > - bt_dev_info(hdev, "ROME setup"); >> > > + qcadev = serdev_device_get_drvdata(hu->serdev); >> > > >> > > ret = qca_check_speeds(hu); >> > > if (ret) >> > > @@ -1006,6 +1137,21 @@ static int qca_setup(struct hci_uart *hu) >> > > /* Setup initial baudrate */ >> > > qca_set_speed(hu, QCA_INIT_SPEED); >> > > >> > > + if (qcadev->btsoc_type == QCA_WCN3990) { >> > > + bt_dev_dbg(hdev, "setting up wcn3990"); >> > >> > Seems like this should be bt_dev_info() for consistency with Rome. >> > >> > > + ret = qca_wcn3990_init(hu); >> > > + if (ret) >> > > + return ret; >> > > + >> > > + ret = qca_read_soc_version(hdev, &soc_ver); >> > > + if (ret < 0 || soc_ver == 0) { >> > > + bt_dev_err(hdev, "Failed to get version %d", ret); >> > > + return ret; >> > > + } >> > > + } else { >> > > + bt_dev_info(hdev, "ROME setup"); >> > > + } >> > > + >> > > /* Setup user speed if needed */ >> > > speed = qca_get_speed(hu, QCA_OPER_SPEED); >> > > if (speed) { >> > > @@ -1016,16 +1162,18 @@ static int qca_setup(struct hci_uart *hu) >> > > qca_baudrate = qca_get_baudrate_value(speed); >> > > } >> > > >> > > - /* Get QCA version information */ >> > > - ret = qca_read_soc_version(hdev, &soc_ver); >> > > - if (ret < 0 || soc_ver == 0) { >> > > - bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); >> > > - return ret; >> > > + if (!soc_ver) { >> > >> > >> > The following would probably be clearer: >> > >> > if (qcadev->btsoc_type != QCA_WCN3990) { >> > >> > > + /* Get QCA version information */ >> > > + ret = qca_read_soc_version(hdev, &soc_ver); >> > > + if (ret < 0 || soc_ver == 0) { >> > > + bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); >> > > + return ret; >> > > + } >> > > + bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); >> > >> > Couldn't we hide some of this in qca_read_soc_version()? >> > >> > qca_read_soc_version() could at least do the logging in the error case >> > (the 'soc_ver == 0' case could be handled there as well), which would >> > leave us with: >> > >> > >> > ret = qca_read_soc_version(hdev, &soc_ver); >> > if (ret) >> > return ret; >> > >> > And the same above. >> > >> > The controller version can be logged outside of the branch for both >> > wcn3990 and Rome. >> > >> > > } >> > > - bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); >> > > >> > > /* Setup patch / NVM configurations */ >> > > - ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver); >> > > + ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, >> > > soc_ver); >> > > if (!ret) { >> > > set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); >> > > qca_debugfs_init(hdev); >> > > @@ -1061,9 +1209,123 @@ static struct hci_uart_proto qca_proto = { >> > > .dequeue = qca_dequeue, >> > > }; >> > > >> > > +static const struct qca_vreg_data qca_soc_data = { >> > > + .soc_type = QCA_WCN3990, >> > > + .vregs = (struct qca_vreg []) { >> > > + { "vddio", 1800000, 1800000, 0 }, >> > > + { "vddxo", 1800000, 1800000, 1 }, >> > > + { "vddrf", 1304000, 1304000, 1 }, >> > > + { "vddch0", 3000000, 3312000, 1 }, >> > > + }, >> > > + .num_vregs = 4, >> > > +}; >> > >> > I didn't chime in earlier in the discussion with Stephen on the >> > regulators (https://patchwork.kernel.org/patch/10467911/), however I >> > agree with him that specifying at load of 1uA doesn't seem to make >> > much sense. What would happen if the load remained unspecified (or 0)? >> >> [Bala]: On RPMh based designs, calling a regulator_set_load with a >> non-zero >> value moves the regulator to NPM (high power mode) which is required >> for BT >> communication. >> and call with 0uA moves it to low-power-mode (vote from >> APPS). >> Basically whenever BT wants these in NPM it calls set-load with 1. > > This assumes that the chip is powered by RPMh regulators, which > depending on the system may or may not be true. On systems with other > regulators you are telling the regulator that the max load is 1uA, and > the regulator might comply, going in a low power mode that does not > provide enough current for the chip to operate properly. > > Doesn't the datasheet of the chip specify actual max currents for > these consumers that could be used instead of the bogus 1uA? If not > you could ask the designers of the chip for an approximate number > (preferably erring on the upper side). [Bala]: will check and update you on this.
Hi Matthias, On 2018-07-10 22:09, Matthias Kaehlcke wrote: > Hi Bala, > > On Tue, Jul 10, 2018 at 06:22:02PM +0530, Balakrishna Godavarthi wrote: >> Hi Matthias, >> >> On 2018-07-07 03:51, Matthias Kaehlcke wrote: >> > On Thu, Jul 05, 2018 at 10:25:15PM +0530, Balakrishna Godavarthi wrote: >> > > + * sending vendor power on and off pulse to SoC. >> > > + */ >> > >> > The function is called qca_send_vendor_cmd(), the comment about power >> > on/off pulses seems misplaced here. Are there other 'vendor commands' >> > that require a different flow control behavior? >> > >> > Perhaps the function should have a different name or we need another >> > wrapper. >> > >> >> [Bala]: this function is used to send single byte vendor commands. >> like >> power on and off pulse. >> and all single byte vendor commands require an flow control >> off and >> on. >> so might be function name change is required. >> instead of qca_send_vendor_cmd(). i want to change function >> name to >> qca_send_pulse() as this will be generic. > > 'pulse' covers power on/off pulses, is it also applicable to other > single byte commands? How are these commands named in the QCA > documentation? > [Bala]: in QCA wcn3990 for these two bytes we will turn off the flow control while sending command and turn on back once it is enabled. in documentation it was mentioned as power on and power off pulse. for all other single bytes we need flow control enable. > In any case the comment about 'vendor power on and off pulse' should > be updated to refer to 'vendor commands'/'pulses' or whatever > terminology we decide to use. [Bala]: will update the the function name. > >> > > + hci_uart_set_flow_control(hu, true); >> > >> > Should the changing of the flow control be limited to wcn3990? As of >> > now the function is only called for wcn3990, however this is not >> > stated as a requirement and might change in the future. >> > >> > > + skb_put_u8(skb, cmd); >> > > + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT; >> > > + >> > > + skb_queue_tail(&qca->txq, skb); >> > > + hci_uart_tx_wakeup(hu); >> > > + >> > > + /* Wait for 100 uS for SoC to settle down */ >> > > + usleep_range(100, 200); >> > >> > Is this needed for any 'vendor command' or directly related with the >> > power on/off pulses? >> > >> >> [Bala] : flow control off/on is needed for power on and off pulses >> along >> with >> command to set baudrate.. this is common across all the >> present >> chips and chips which are in development stages. >> might be function name is confusing will update the function >> name. > > The current name *might* be ok, depending on if we come up with > something better. If the QCA docs refer to them as 'vendor commands' > and there are no other multi-byte 'vendor commands' I'm fine with it. > > Is the settling down of 100us specific to the power on/off pulses or > also applicable to other commands? > [Bala]: 100 us is specific to power on and off pulses. > Thanks > > Matthias
Hi Matthias, On 2018-07-16 21:35, Matthias Kaehlcke wrote: > On Mon, Jul 16, 2018 at 07:21:56PM +0530, Balakrishna Godavarthi wrote: >> Hi Matthias, >> >> On 2018-07-07 03:51, Matthias Kaehlcke wrote: >> > On Thu, Jul 05, 2018 at 10:25:15PM +0530, Balakrishna Godavarthi wrote: >> > > Add support to set voltage/current of various regulators >> > > to power up/down Bluetooth chip wcn3990. >> > > >> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> >> > > --- >> > > changes in v9: >> > > * moved flow control to vendor and set_baudarte functions. >> > > * removed parent regs. >> > > >> > > changes in v8: >> > > * closing qca buffer, if qca_power_setup fails >> > > * chnaged ibs start timer function call location. >> > > * updated review comments. >> > > >> > > changes in v7: >> > > * addressed review comments. >> > > >> > > changes in v6: >> > > * Hooked up qca_power to qca_serdev. >> > > * renamed all the naming inconsistency functions with qca_* >> > > * leveraged common code of ROME for wcn3990. >> > > * created wrapper functions for re-usable blocks. >> > > * updated function of _*regulator_enable and _*regualtor_disable. >> > > * removed redundant comments and functions. >> > > * addressed review comments. >> > > >> > > Changes in v5: >> > > * updated regulator vddpa min_uV to 1304000. >> > > * addressed review comments. >> > > >> > > Changes in v4: >> > > * Segregated the changes of btqca from hci_qca >> > > * rebased all changes on top of bluetooth-next. >> > > * addressed review comments. >> > > >> > > --- >> > > drivers/bluetooth/btqca.h | 3 + >> > > drivers/bluetooth/hci_qca.c | 387 >> > > +++++++++++++++++++++++++++++++----- >> > > 2 files changed, 345 insertions(+), 45 deletions(-) >> > > >> > > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h >> > > index a9c2779f3e07..0c01f375fe83 100644 >> > > --- a/drivers/bluetooth/btqca.h >> > > +++ b/drivers/bluetooth/btqca.h >> > > @@ -37,6 +37,9 @@ >> > > #define EDL_TAG_ID_HCI (17) >> > > #define EDL_TAG_ID_DEEP_SLEEP (27) >> > > >> > > +#define QCA_WCN3990_POWERON_PULSE 0xFC >> > > +#define QCA_WCN3990_POWEROFF_PULSE 0xC0 >> > > + >> > > enum qca_bardrate { >> > > QCA_BAUDRATE_115200 = 0, >> > > QCA_BAUDRATE_57600, >> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> > > index 7ebfaa0edf3f..d62c7785a618 100644 >> > > --- a/drivers/bluetooth/hci_qca.c >> > > +++ b/drivers/bluetooth/hci_qca.c >> > > @@ -5,7 +5,7 @@ >> > > * protocol extension to H4. >> > > * >> > > * Copyright (C) 2007 Texas Instruments, Inc. >> > > - * Copyright (c) 2010, 2012 The Linux Foundation. All rights >> > > reserved. >> > > + * Copyright (c) 2010, 2012, 2018 The Linux Foundation. All rights >> > > reserved. >> > > * >> > > * Acknowledgements: >> > > * This file is based on hci_ll.c, which was... >> > > @@ -31,9 +31,14 @@ >> > > #include <linux/kernel.h> >> > > #include <linux/clk.h> >> > > #include <linux/debugfs.h> >> > > +#include <linux/delay.h> >> > > +#include <linux/device.h> >> > > #include <linux/gpio/consumer.h> >> > > #include <linux/mod_devicetable.h> >> > > #include <linux/module.h> >> > > +#include <linux/of_device.h> >> > > +#include <linux/platform_device.h> >> > > +#include <linux/regulator/consumer.h> >> > > #include <linux/serdev.h> >> > > >> > > #include <net/bluetooth/bluetooth.h> >> > > @@ -124,12 +129,46 @@ enum qca_speed_type { >> > > QCA_OPER_SPEED >> > > }; >> > > >> > > +/* >> > > + * Voltage regulator information required for configuring the >> > > + * QCA Bluetooth chipset >> > > + */ >> > > +struct qca_vreg { >> > > + const char *name; >> > > + unsigned int min_uV; >> > > + unsigned int max_uV; >> > > + unsigned int load_uA; >> > > +}; >> > > + >> > > +struct qca_vreg_data { >> > > + enum qca_btsoc_type soc_type; >> > > + struct qca_vreg *vregs; >> > > + size_t num_vregs; >> > > +}; >> > > + >> > > +/* >> > > + * Platform data for the QCA Bluetooth power driver. >> > > + */ >> > > +struct qca_power { >> > > + struct device *dev; >> > > + const struct qca_vreg_data *vreg_data; >> > > + struct regulator_bulk_data *vreg_bulk; >> > > + bool vregs_on; >> > > +}; >> > > + >> > > struct qca_serdev { >> > > struct hci_uart serdev_hu; >> > > struct gpio_desc *bt_en; >> > > struct clk *susclk; >> > > + enum qca_btsoc_type btsoc_type; >> > > + struct qca_power *bt_power; >> > > + u32 init_speed; >> > > + u32 oper_speed; >> > > }; >> > > >> > > +static int qca_power_setup(struct hci_uart *hu, bool on); >> > > +static void qca_power_shutdown(struct hci_uart *hu); >> > > + >> > > static void __serial_clock_on(struct tty_struct *tty) >> > > { >> > > /* TODO: Some chipset requires to enable UART clock on client >> > > @@ -407,6 +446,7 @@ static int qca_open(struct hci_uart *hu) >> > > { >> > > struct qca_serdev *qcadev; >> > > struct qca_data *qca; >> > > + int ret; >> > > >> > > BT_DBG("hu %p qca_open", hu); >> > > >> > > @@ -458,19 +498,32 @@ static int qca_open(struct hci_uart *hu) >> > > >> > > hu->priv = qca; >> > > >> > > - timer_setup(&qca->wake_retrans_timer, >> > > hci_ibs_wake_retrans_timeout, 0); >> > > - qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; >> > > - >> > > - timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); >> > > - qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; >> > > - >> > > if (hu->serdev) { >> > > serdev_device_open(hu->serdev); >> > > >> > > qcadev = serdev_device_get_drvdata(hu->serdev); >> > > - gpiod_set_value_cansleep(qcadev->bt_en, 1); >> > > + if (qcadev->btsoc_type != QCA_WCN3990) { >> > > + gpiod_set_value_cansleep(qcadev->bt_en, 1); >> > > + } else { >> > > + hu->init_speed = qcadev->init_speed; >> > > + hu->oper_speed = qcadev->oper_speed; >> > > + ret = qca_power_setup(hu, true); >> > > + if (ret) { >> > > + destroy_workqueue(qca->workqueue); >> > > + kfree_skb(qca->rx_skb); >> > > + hu->priv = NULL; >> > > + kfree(qca); >> > > + return ret; >> > > + } >> > > + } >> > > } >> > > >> > > + timer_setup(&qca->wake_retrans_timer, >> > > hci_ibs_wake_retrans_timeout, 0); >> > > + qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; >> > > + >> > > + timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); >> > > + qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; >> > > + >> > > BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u", >> > > qca->tx_idle_delay, qca->wake_retrans); >> > > >> > > @@ -554,10 +607,13 @@ static int qca_close(struct hci_uart *hu) >> > > qca->hu = NULL; >> > > >> > > if (hu->serdev) { >> > > - serdev_device_close(hu->serdev); >> > > - >> > > qcadev = serdev_device_get_drvdata(hu->serdev); >> > > - gpiod_set_value_cansleep(qcadev->bt_en, 0); >> > > + if (qcadev->btsoc_type == QCA_WCN3990) >> > > + qca_power_shutdown(hu); >> > > + else >> > > + gpiod_set_value_cansleep(qcadev->bt_en, 0); >> > > + >> > > + serdev_device_close(hu->serdev); >> > > } >> > > >> > > kfree_skb(qca->rx_skb); >> > > @@ -891,6 +947,7 @@ 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) >> > > @@ -904,6 +961,13 @@ static int qca_set_baudrate(struct hci_dev >> > > *hdev, uint8_t baudrate) >> > > return -ENOMEM; >> > > } >> > > >> > > + /* Disabling hardware flow control is mandate while >> > >> > nit: s/mandate/mandatory|required/ >> > >> > > + * 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; >> > > @@ -919,6 +983,9 @@ 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; >> > > } >> > > >> > > @@ -930,6 +997,36 @@ static inline void host_set_baudrate(struct >> > > hci_uart *hu, unsigned int speed) >> > > hci_uart_set_baudrate(hu, speed); >> > > } >> > > >> > > +static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd) >> > > +{ >> > > + struct hci_uart *hu = hci_get_drvdata(hdev); >> > > + struct qca_data *qca = hu->priv; >> > > + struct sk_buff *skb; >> > > + >> > > + bt_dev_dbg(hdev, "sending command %02x to SoC", cmd); >> > > + >> > > + skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL); >> > > + if (!skb) >> > > + return -ENOMEM; >> > > + >> > > + /* Disabling hardware flow control is mandate while >> > >> > Same nit as above >> > >> > > + * sending vendor power on and off pulse to SoC. >> > > + */ >> > >> > The function is called qca_send_vendor_cmd(), the comment about power >> > on/off pulses seems misplaced here. Are there other 'vendor commands' >> > that require a different flow control behavior? >> > >> > Perhaps the function should have a different name or we need another >> > wrapper. >> > >> > > + hci_uart_set_flow_control(hu, true); >> > >> > Should the changing of the flow control be limited to wcn3990? As of >> > now the function is only called for wcn3990, however this is not >> > stated as a requirement and might change in the future. >> > >> > > + skb_put_u8(skb, cmd); >> > > + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT; >> > > + >> > > + skb_queue_tail(&qca->txq, skb); >> > > + hci_uart_tx_wakeup(hu); >> > > + >> > > + /* Wait for 100 uS for SoC to settle down */ >> > > + usleep_range(100, 200); >> > >> > Is this needed for any 'vendor command' or directly related with the >> > power on/off pulses? >> > >> > > + hci_uart_set_flow_control(hu, false); >> > > + >> > > + return 0; >> > > +} >> > > + >> > > static unsigned int qca_get_speed(struct hci_uart *hu, >> > > enum qca_speed_type speed_type) >> > > { >> > > @@ -952,10 +1049,19 @@ static unsigned int qca_get_speed(struct >> > > hci_uart *hu, >> > > >> > > static int qca_check_speeds(struct hci_uart *hu) >> > > { >> > > - /* One or the other speeds should be non zero. */ >> > > - if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> > > - !qca_get_speed(hu, QCA_OPER_SPEED)) >> > > + struct qca_serdev *qcadev; >> > > + >> > > + qcadev = serdev_device_get_drvdata(hu->serdev); >> > > + if ((qcadev->btsoc_type == QCA_WCN3990 && >> > > + !qca_get_speed(hu, QCA_INIT_SPEED)) || >> > > + !qca_get_speed(hu, QCA_OPER_SPEED)) { >> > > + /* Both INIT and OPER speed should be non zero. */ >> > > return -EINVAL; >> > > + } else if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> > > + !qca_get_speed(hu, QCA_OPER_SPEED)) { >> > > + /* One or the other speeds should be non zero. */ >> > > + return -EINVAL; >> > > + } >> > >> > It is questionable if the comments are useful, they are basically >> > stating the same as the conditions. >> > >> > if-else statements with a single statement in all branches shouldn't >> > use curly braces. Personally I don't dislike them in this case with >> > the multi-line conditions, but in principle they shoulnd't be there. >> > >> > nit: this would be easier to read with nested if statements: >> > >> > if (qcadev->btsoc_type == QCA_WCN3990) >> > if (!qca_get_speed(hu, QCA_INIT_SPEED)) || >> > !qca_get_speed(hu, QCA_OPER_SPEED)) >> > return -EINVAL; >> > else >> > if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> > !qca_get_speed(hu, QCA_OPER_SPEED)) { >> > return -EINVAL; >> > >> > > return 0; >> > > } >> > > @@ -986,15 +1092,40 @@ static int qca_set_speed(struct hci_uart *hu, >> > > enum qca_speed_type speed_type) >> > > return 0; >> > > } >> > > >> > > +static int qca_wcn3990_init(struct hci_uart *hu) >> > > +{ >> > > + int ret; >> > > + >> > > + ret = qca_send_vendor_cmd(hu->hdev, QCA_WCN3990_POWERON_PULSE); >> > > + if (ret) >> > > + return ret; >> > > + >> > > + /* Wait for 100 ms for SoC to boot up */ >> > > + msleep(100); >> > > + serdev_device_close(hu->serdev); >> > > + ret = serdev_device_open(hu->serdev); >> > > + if (ret) { >> > > + bt_dev_err(hu->hdev, "failed to open port"); >> > > + return ret; >> > > + } >> > > + >> > > + /* Setup initial baudrate */ >> > > + qca_set_speed(hu, QCA_INIT_SPEED); >> > >> > The comment is a bit redundant. >> > >> > > + hci_uart_set_flow_control(hu, false); >> > > + >> > > + return 0; >> > > +} >> > > + >> > > static int qca_setup(struct hci_uart *hu) >> > > { >> > > struct hci_dev *hdev = hu->hdev; >> > > struct qca_data *qca = hu->priv; >> > > unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; >> > > + struct qca_serdev *qcadev; >> > > int ret; >> > > int soc_ver = 0; >> > > >> > > - bt_dev_info(hdev, "ROME setup"); >> > > + qcadev = serdev_device_get_drvdata(hu->serdev); >> > > >> > > ret = qca_check_speeds(hu); >> > > if (ret) >> > > @@ -1006,6 +1137,21 @@ static int qca_setup(struct hci_uart *hu) >> > > /* Setup initial baudrate */ >> > > qca_set_speed(hu, QCA_INIT_SPEED); >> > > >> > > + if (qcadev->btsoc_type == QCA_WCN3990) { >> > > + bt_dev_dbg(hdev, "setting up wcn3990"); >> > >> > Seems like this should be bt_dev_info() for consistency with Rome. >> > >> > > + ret = qca_wcn3990_init(hu); >> > > + if (ret) >> > > + return ret; >> > > + >> > > + ret = qca_read_soc_version(hdev, &soc_ver); >> > > + if (ret < 0 || soc_ver == 0) { >> > > + bt_dev_err(hdev, "Failed to get version %d", ret); >> > > + return ret; >> > > + } >> > > + } else { >> > > + bt_dev_info(hdev, "ROME setup"); >> > > + } >> > > + >> > > /* Setup user speed if needed */ >> > > speed = qca_get_speed(hu, QCA_OPER_SPEED); >> > > if (speed) { >> > > @@ -1016,16 +1162,18 @@ static int qca_setup(struct hci_uart *hu) >> > > qca_baudrate = qca_get_baudrate_value(speed); >> > > } >> > > >> > > - /* Get QCA version information */ >> > > - ret = qca_read_soc_version(hdev, &soc_ver); >> > > - if (ret < 0 || soc_ver == 0) { >> > > - bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); >> > > - return ret; >> > > + if (!soc_ver) { >> > >> > >> > The following would probably be clearer: >> > >> > if (qcadev->btsoc_type != QCA_WCN3990) { >> > >> > > + /* Get QCA version information */ >> > > + ret = qca_read_soc_version(hdev, &soc_ver); >> > > + if (ret < 0 || soc_ver == 0) { >> > > + bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); >> > > + return ret; >> > > + } >> > > + bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); >> > >> > Couldn't we hide some of this in qca_read_soc_version()? >> > >> > qca_read_soc_version() could at least do the logging in the error case >> > (the 'soc_ver == 0' case could be handled there as well), which would >> > leave us with: >> > >> > >> > ret = qca_read_soc_version(hdev, &soc_ver); >> > if (ret) >> > return ret; >> > >> > And the same above. >> > >> > The controller version can be logged outside of the branch for both >> > wcn3990 and Rome. >> > >> > > } >> > > - bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); >> > > >> > > /* Setup patch / NVM configurations */ >> > > - ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver); >> > > + ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, >> > > soc_ver); >> > > if (!ret) { >> > > set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); >> > > qca_debugfs_init(hdev); >> > > @@ -1061,9 +1209,123 @@ static struct hci_uart_proto qca_proto = { >> > > .dequeue = qca_dequeue, >> > > }; >> > > >> > > +static const struct qca_vreg_data qca_soc_data = { >> > > + .soc_type = QCA_WCN3990, >> > > + .vregs = (struct qca_vreg []) { >> > > + { "vddio", 1800000, 1800000, 0 }, >> > > + { "vddxo", 1800000, 1800000, 1 }, >> > > + { "vddrf", 1304000, 1304000, 1 }, >> > > + { "vddch0", 3000000, 3312000, 1 }, >> > > + }, >> > > + .num_vregs = 4, >> > > +}; >> > >> > I didn't chime in earlier in the discussion with Stephen on the >> > regulators (https://patchwork.kernel.org/patch/10467911/), however I >> > agree with him that specifying at load of 1uA doesn't seem to make >> > much sense. What would happen if the load remained unspecified (or 0)? >> >> [Bala]: On RPMh based designs, calling a regulator_set_load with a >> non-zero >> value moves the regulator to NPM (high power mode) which is required >> for BT >> communication. >> and call with 0uA moves it to low-power-mode (vote from >> APPS). >> Basically whenever BT wants these in NPM it calls set-load with 1. > > This assumes that the chip is powered by RPMh regulators, which > depending on the system may or may not be true. On systems with other > regulators you are telling the regulator that the max load is 1uA, and > the regulator might comply, going in a low power mode that does not > provide enough current for the chip to operate properly. > > Doesn't the datasheet of the chip specify actual max currents for > these consumers that could be used instead of the bogus 1uA? If not > you could ask the designers of the chip for an approximate number > (preferably erring on the upper side). [Bala]: yes your true, now current values are aligned according to the RPMH regs packages. this is not our goal. what ever the package we use wcn3990 should be platform independent. I propose an idea that, we will have default current value in driver i.e current required by regs to turn on wcn3990 and also we will provide interface in driver to read current from dts tree. i.e. if we have prop for current to the reg in the dts, we will read the value assigned to prop else we will take default current <code snippets> qca->vreg_data->vregs[i].name = current; // holds the default current value taken from data provided from deb. this will value remains unchanged. if (device_property_read_bool(dev, prop_name)) // if we have prop name enabled then read the value. device_property_read_u32(dev, prop_name, &vregs->load_uA);
On Wed, Jul 18, 2018 at 09:03:08PM +0530, Balakrishna Godavarthi wrote: > Hi Matthias, > > On 2018-07-16 21:35, Matthias Kaehlcke wrote: > > On Mon, Jul 16, 2018 at 07:21:56PM +0530, Balakrishna Godavarthi wrote: > > > Hi Matthias, > > > > > > On 2018-07-07 03:51, Matthias Kaehlcke wrote: > > > > On Thu, Jul 05, 2018 at 10:25:15PM +0530, Balakrishna Godavarthi wrote: > > > > > Add support to set voltage/current of various regulators > > > > > to power up/down Bluetooth chip wcn3990. > > > > > > > > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> > > > > > --- > > > > > changes in v9: > > > > > * moved flow control to vendor and set_baudarte functions. > > > > > * removed parent regs. > > > > > > > > > > changes in v8: > > > > > * closing qca buffer, if qca_power_setup fails > > > > > * chnaged ibs start timer function call location. > > > > > * updated review comments. > > > > > > > > > > changes in v7: > > > > > * addressed review comments. > > > > > > > > > > changes in v6: > > > > > * Hooked up qca_power to qca_serdev. > > > > > * renamed all the naming inconsistency functions with qca_* > > > > > * leveraged common code of ROME for wcn3990. > > > > > * created wrapper functions for re-usable blocks. > > > > > * updated function of _*regulator_enable and _*regualtor_disable. > > > > > * removed redundant comments and functions. > > > > > * addressed review comments. > > > > > > > > > > Changes in v5: > > > > > * updated regulator vddpa min_uV to 1304000. > > > > > * addressed review comments. > > > > > > > > > > Changes in v4: > > > > > * Segregated the changes of btqca from hci_qca > > > > > * rebased all changes on top of bluetooth-next. > > > > > * addressed review comments. > > > > > > > > > > --- > > > > > drivers/bluetooth/btqca.h | 3 + > > > > > drivers/bluetooth/hci_qca.c | 387 > > > > > +++++++++++++++++++++++++++++++----- > > > > > 2 files changed, 345 insertions(+), 45 deletions(-) > > > > > > > > > > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h > > > > > index a9c2779f3e07..0c01f375fe83 100644 > > > > > --- a/drivers/bluetooth/btqca.h > > > > > +++ b/drivers/bluetooth/btqca.h > > > > > @@ -37,6 +37,9 @@ > > > > > #define EDL_TAG_ID_HCI (17) > > > > > #define EDL_TAG_ID_DEEP_SLEEP (27) > > > > > > > > > > +#define QCA_WCN3990_POWERON_PULSE 0xFC > > > > > +#define QCA_WCN3990_POWEROFF_PULSE 0xC0 > > > > > + > > > > > enum qca_bardrate { > > > > > QCA_BAUDRATE_115200 = 0, > > > > > QCA_BAUDRATE_57600, > > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > > > > > index 7ebfaa0edf3f..d62c7785a618 100644 > > > > > --- a/drivers/bluetooth/hci_qca.c > > > > > +++ b/drivers/bluetooth/hci_qca.c > > > > > @@ -5,7 +5,7 @@ > > > > > * protocol extension to H4. > > > > > * > > > > > * Copyright (C) 2007 Texas Instruments, Inc. > > > > > - * Copyright (c) 2010, 2012 The Linux Foundation. All rights > > > > > reserved. > > > > > + * Copyright (c) 2010, 2012, 2018 The Linux Foundation. All rights > > > > > reserved. > > > > > * > > > > > * Acknowledgements: > > > > > * This file is based on hci_ll.c, which was... > > > > > @@ -31,9 +31,14 @@ > > > > > #include <linux/kernel.h> > > > > > #include <linux/clk.h> > > > > > #include <linux/debugfs.h> > > > > > +#include <linux/delay.h> > > > > > +#include <linux/device.h> > > > > > #include <linux/gpio/consumer.h> > > > > > #include <linux/mod_devicetable.h> > > > > > #include <linux/module.h> > > > > > +#include <linux/of_device.h> > > > > > +#include <linux/platform_device.h> > > > > > +#include <linux/regulator/consumer.h> > > > > > #include <linux/serdev.h> > > > > > > > > > > #include <net/bluetooth/bluetooth.h> > > > > > @@ -124,12 +129,46 @@ enum qca_speed_type { > > > > > QCA_OPER_SPEED > > > > > }; > > > > > > > > > > +/* > > > > > + * Voltage regulator information required for configuring the > > > > > + * QCA Bluetooth chipset > > > > > + */ > > > > > +struct qca_vreg { > > > > > + const char *name; > > > > > + unsigned int min_uV; > > > > > + unsigned int max_uV; > > > > > + unsigned int load_uA; > > > > > +}; > > > > > + > > > > > +struct qca_vreg_data { > > > > > + enum qca_btsoc_type soc_type; > > > > > + struct qca_vreg *vregs; > > > > > + size_t num_vregs; > > > > > +}; > > > > > + > > > > > +/* > > > > > + * Platform data for the QCA Bluetooth power driver. > > > > > + */ > > > > > +struct qca_power { > > > > > + struct device *dev; > > > > > + const struct qca_vreg_data *vreg_data; > > > > > + struct regulator_bulk_data *vreg_bulk; > > > > > + bool vregs_on; > > > > > +}; > > > > > + > > > > > struct qca_serdev { > > > > > struct hci_uart serdev_hu; > > > > > struct gpio_desc *bt_en; > > > > > struct clk *susclk; > > > > > + enum qca_btsoc_type btsoc_type; > > > > > + struct qca_power *bt_power; > > > > > + u32 init_speed; > > > > > + u32 oper_speed; > > > > > }; > > > > > > > > > > +static int qca_power_setup(struct hci_uart *hu, bool on); > > > > > +static void qca_power_shutdown(struct hci_uart *hu); > > > > > + > > > > > static void __serial_clock_on(struct tty_struct *tty) > > > > > { > > > > > /* TODO: Some chipset requires to enable UART clock on client > > > > > @@ -407,6 +446,7 @@ static int qca_open(struct hci_uart *hu) > > > > > { > > > > > struct qca_serdev *qcadev; > > > > > struct qca_data *qca; > > > > > + int ret; > > > > > > > > > > BT_DBG("hu %p qca_open", hu); > > > > > > > > > > @@ -458,19 +498,32 @@ static int qca_open(struct hci_uart *hu) > > > > > > > > > > hu->priv = qca; > > > > > > > > > > - timer_setup(&qca->wake_retrans_timer, > > > > > hci_ibs_wake_retrans_timeout, 0); > > > > > - qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; > > > > > - > > > > > - timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); > > > > > - qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; > > > > > - > > > > > if (hu->serdev) { > > > > > serdev_device_open(hu->serdev); > > > > > > > > > > qcadev = serdev_device_get_drvdata(hu->serdev); > > > > > - gpiod_set_value_cansleep(qcadev->bt_en, 1); > > > > > + if (qcadev->btsoc_type != QCA_WCN3990) { > > > > > + gpiod_set_value_cansleep(qcadev->bt_en, 1); > > > > > + } else { > > > > > + hu->init_speed = qcadev->init_speed; > > > > > + hu->oper_speed = qcadev->oper_speed; > > > > > + ret = qca_power_setup(hu, true); > > > > > + if (ret) { > > > > > + destroy_workqueue(qca->workqueue); > > > > > + kfree_skb(qca->rx_skb); > > > > > + hu->priv = NULL; > > > > > + kfree(qca); > > > > > + return ret; > > > > > + } > > > > > + } > > > > > } > > > > > > > > > > + timer_setup(&qca->wake_retrans_timer, > > > > > hci_ibs_wake_retrans_timeout, 0); > > > > > + qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; > > > > > + > > > > > + timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); > > > > > + qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; > > > > > + > > > > > BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u", > > > > > qca->tx_idle_delay, qca->wake_retrans); > > > > > > > > > > @@ -554,10 +607,13 @@ static int qca_close(struct hci_uart *hu) > > > > > qca->hu = NULL; > > > > > > > > > > if (hu->serdev) { > > > > > - serdev_device_close(hu->serdev); > > > > > - > > > > > qcadev = serdev_device_get_drvdata(hu->serdev); > > > > > - gpiod_set_value_cansleep(qcadev->bt_en, 0); > > > > > + if (qcadev->btsoc_type == QCA_WCN3990) > > > > > + qca_power_shutdown(hu); > > > > > + else > > > > > + gpiod_set_value_cansleep(qcadev->bt_en, 0); > > > > > + > > > > > + serdev_device_close(hu->serdev); > > > > > } > > > > > > > > > > kfree_skb(qca->rx_skb); > > > > > @@ -891,6 +947,7 @@ 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) > > > > > @@ -904,6 +961,13 @@ static int qca_set_baudrate(struct hci_dev > > > > > *hdev, uint8_t baudrate) > > > > > return -ENOMEM; > > > > > } > > > > > > > > > > + /* Disabling hardware flow control is mandate while > > > > > > > > nit: s/mandate/mandatory|required/ > > > > > > > > > + * 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; > > > > > @@ -919,6 +983,9 @@ 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; > > > > > } > > > > > > > > > > @@ -930,6 +997,36 @@ static inline void host_set_baudrate(struct > > > > > hci_uart *hu, unsigned int speed) > > > > > hci_uart_set_baudrate(hu, speed); > > > > > } > > > > > > > > > > +static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd) > > > > > +{ > > > > > + struct hci_uart *hu = hci_get_drvdata(hdev); > > > > > + struct qca_data *qca = hu->priv; > > > > > + struct sk_buff *skb; > > > > > + > > > > > + bt_dev_dbg(hdev, "sending command %02x to SoC", cmd); > > > > > + > > > > > + skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL); > > > > > + if (!skb) > > > > > + return -ENOMEM; > > > > > + > > > > > + /* Disabling hardware flow control is mandate while > > > > > > > > Same nit as above > > > > > > > > > + * sending vendor power on and off pulse to SoC. > > > > > + */ > > > > > > > > The function is called qca_send_vendor_cmd(), the comment about power > > > > on/off pulses seems misplaced here. Are there other 'vendor commands' > > > > that require a different flow control behavior? > > > > > > > > Perhaps the function should have a different name or we need another > > > > wrapper. > > > > > > > > > + hci_uart_set_flow_control(hu, true); > > > > > > > > Should the changing of the flow control be limited to wcn3990? As of > > > > now the function is only called for wcn3990, however this is not > > > > stated as a requirement and might change in the future. > > > > > > > > > + skb_put_u8(skb, cmd); > > > > > + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT; > > > > > + > > > > > + skb_queue_tail(&qca->txq, skb); > > > > > + hci_uart_tx_wakeup(hu); > > > > > + > > > > > + /* Wait for 100 uS for SoC to settle down */ > > > > > + usleep_range(100, 200); > > > > > > > > Is this needed for any 'vendor command' or directly related with the > > > > power on/off pulses? > > > > > > > > > + hci_uart_set_flow_control(hu, false); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > static unsigned int qca_get_speed(struct hci_uart *hu, > > > > > enum qca_speed_type speed_type) > > > > > { > > > > > @@ -952,10 +1049,19 @@ static unsigned int qca_get_speed(struct > > > > > hci_uart *hu, > > > > > > > > > > static int qca_check_speeds(struct hci_uart *hu) > > > > > { > > > > > - /* One or the other speeds should be non zero. */ > > > > > - if (!qca_get_speed(hu, QCA_INIT_SPEED) && > > > > > - !qca_get_speed(hu, QCA_OPER_SPEED)) > > > > > + struct qca_serdev *qcadev; > > > > > + > > > > > + qcadev = serdev_device_get_drvdata(hu->serdev); > > > > > + if ((qcadev->btsoc_type == QCA_WCN3990 && > > > > > + !qca_get_speed(hu, QCA_INIT_SPEED)) || > > > > > + !qca_get_speed(hu, QCA_OPER_SPEED)) { > > > > > + /* Both INIT and OPER speed should be non zero. */ > > > > > return -EINVAL; > > > > > + } else if (!qca_get_speed(hu, QCA_INIT_SPEED) && > > > > > + !qca_get_speed(hu, QCA_OPER_SPEED)) { > > > > > + /* One or the other speeds should be non zero. */ > > > > > + return -EINVAL; > > > > > + } > > > > > > > > It is questionable if the comments are useful, they are basically > > > > stating the same as the conditions. > > > > > > > > if-else statements with a single statement in all branches shouldn't > > > > use curly braces. Personally I don't dislike them in this case with > > > > the multi-line conditions, but in principle they shoulnd't be there. > > > > > > > > nit: this would be easier to read with nested if statements: > > > > > > > > if (qcadev->btsoc_type == QCA_WCN3990) > > > > if (!qca_get_speed(hu, QCA_INIT_SPEED)) || > > > > !qca_get_speed(hu, QCA_OPER_SPEED)) > > > > return -EINVAL; > > > > else > > > > if (!qca_get_speed(hu, QCA_INIT_SPEED) && > > > > !qca_get_speed(hu, QCA_OPER_SPEED)) { > > > > return -EINVAL; > > > > > > > > > return 0; > > > > > } > > > > > @@ -986,15 +1092,40 @@ static int qca_set_speed(struct hci_uart *hu, > > > > > enum qca_speed_type speed_type) > > > > > return 0; > > > > > } > > > > > > > > > > +static int qca_wcn3990_init(struct hci_uart *hu) > > > > > +{ > > > > > + int ret; > > > > > + > > > > > + ret = qca_send_vendor_cmd(hu->hdev, QCA_WCN3990_POWERON_PULSE); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + /* Wait for 100 ms for SoC to boot up */ > > > > > + msleep(100); > > > > > + serdev_device_close(hu->serdev); > > > > > + ret = serdev_device_open(hu->serdev); > > > > > + if (ret) { > > > > > + bt_dev_err(hu->hdev, "failed to open port"); > > > > > + return ret; > > > > > + } > > > > > + > > > > > + /* Setup initial baudrate */ > > > > > + qca_set_speed(hu, QCA_INIT_SPEED); > > > > > > > > The comment is a bit redundant. > > > > > > > > > + hci_uart_set_flow_control(hu, false); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > static int qca_setup(struct hci_uart *hu) > > > > > { > > > > > struct hci_dev *hdev = hu->hdev; > > > > > struct qca_data *qca = hu->priv; > > > > > unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; > > > > > + struct qca_serdev *qcadev; > > > > > int ret; > > > > > int soc_ver = 0; > > > > > > > > > > - bt_dev_info(hdev, "ROME setup"); > > > > > + qcadev = serdev_device_get_drvdata(hu->serdev); > > > > > > > > > > ret = qca_check_speeds(hu); > > > > > if (ret) > > > > > @@ -1006,6 +1137,21 @@ static int qca_setup(struct hci_uart *hu) > > > > > /* Setup initial baudrate */ > > > > > qca_set_speed(hu, QCA_INIT_SPEED); > > > > > > > > > > + if (qcadev->btsoc_type == QCA_WCN3990) { > > > > > + bt_dev_dbg(hdev, "setting up wcn3990"); > > > > > > > > Seems like this should be bt_dev_info() for consistency with Rome. > > > > > > > > > + ret = qca_wcn3990_init(hu); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + ret = qca_read_soc_version(hdev, &soc_ver); > > > > > + if (ret < 0 || soc_ver == 0) { > > > > > + bt_dev_err(hdev, "Failed to get version %d", ret); > > > > > + return ret; > > > > > + } > > > > > + } else { > > > > > + bt_dev_info(hdev, "ROME setup"); > > > > > + } > > > > > + > > > > > /* Setup user speed if needed */ > > > > > speed = qca_get_speed(hu, QCA_OPER_SPEED); > > > > > if (speed) { > > > > > @@ -1016,16 +1162,18 @@ static int qca_setup(struct hci_uart *hu) > > > > > qca_baudrate = qca_get_baudrate_value(speed); > > > > > } > > > > > > > > > > - /* Get QCA version information */ > > > > > - ret = qca_read_soc_version(hdev, &soc_ver); > > > > > - if (ret < 0 || soc_ver == 0) { > > > > > - bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); > > > > > - return ret; > > > > > + if (!soc_ver) { > > > > > > > > > > > > The following would probably be clearer: > > > > > > > > if (qcadev->btsoc_type != QCA_WCN3990) { > > > > > > > > > + /* Get QCA version information */ > > > > > + ret = qca_read_soc_version(hdev, &soc_ver); > > > > > + if (ret < 0 || soc_ver == 0) { > > > > > + bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); > > > > > + return ret; > > > > > + } > > > > > + bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); > > > > > > > > Couldn't we hide some of this in qca_read_soc_version()? > > > > > > > > qca_read_soc_version() could at least do the logging in the error case > > > > (the 'soc_ver == 0' case could be handled there as well), which would > > > > leave us with: > > > > > > > > > > > > ret = qca_read_soc_version(hdev, &soc_ver); > > > > if (ret) > > > > return ret; > > > > > > > > And the same above. > > > > > > > > The controller version can be logged outside of the branch for both > > > > wcn3990 and Rome. > > > > > > > > > } > > > > > - bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); > > > > > > > > > > /* Setup patch / NVM configurations */ > > > > > - ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver); > > > > > + ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, > > > > > soc_ver); > > > > > if (!ret) { > > > > > set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); > > > > > qca_debugfs_init(hdev); > > > > > @@ -1061,9 +1209,123 @@ static struct hci_uart_proto qca_proto = { > > > > > .dequeue = qca_dequeue, > > > > > }; > > > > > > > > > > +static const struct qca_vreg_data qca_soc_data = { > > > > > + .soc_type = QCA_WCN3990, > > > > > + .vregs = (struct qca_vreg []) { > > > > > + { "vddio", 1800000, 1800000, 0 }, > > > > > + { "vddxo", 1800000, 1800000, 1 }, > > > > > + { "vddrf", 1304000, 1304000, 1 }, > > > > > + { "vddch0", 3000000, 3312000, 1 }, > > > > > + }, > > > > > + .num_vregs = 4, > > > > > +}; > > > > > > > > I didn't chime in earlier in the discussion with Stephen on the > > > > regulators (https://patchwork.kernel.org/patch/10467911/), however I > > > > agree with him that specifying at load of 1uA doesn't seem to make > > > > much sense. What would happen if the load remained unspecified (or 0)? > > > > > > [Bala]: On RPMh based designs, calling a regulator_set_load with a > > > non-zero > > > value moves the regulator to NPM (high power mode) which is required > > > for BT > > > communication. > > > and call with 0uA moves it to low-power-mode (vote from > > > APPS). > > > Basically whenever BT wants these in NPM it calls set-load with 1. > > > > This assumes that the chip is powered by RPMh regulators, which > > depending on the system may or may not be true. On systems with other > > regulators you are telling the regulator that the max load is 1uA, and > > the regulator might comply, going in a low power mode that does not > > provide enough current for the chip to operate properly. > > > > Doesn't the datasheet of the chip specify actual max currents for > > these consumers that could be used instead of the bogus 1uA? If not > > you could ask the designers of the chip for an approximate number > > (preferably erring on the upper side). > > [Bala]: yes your true, now current values are aligned according to the RPMH > regs packages. > this is not our goal. what ever the package we use wcn3990 should be > platform independent. > I propose an idea that, we will have default current value in driver > i.e current required by regs to turn on wcn3990 and also we will provide > interface in driver > to read current from dts tree. > > i.e. if we have prop for current to the reg in the dts, we will read > the value assigned to prop else we will take default current > > <code snippets> > > qca->vreg_data->vregs[i].name = current; // holds the default current I suppose this should be 'qca->vreg_data->vregs[i].load_uA' > value taken from data provided from deb. this will value remains unchanged. > if (device_property_read_bool(dev, prop_name)) // if we have prop > name enabled then read the value. > device_property_read_u32(dev, prop_name, &vregs->load_uA); It's still not clear why this would be needed. Is 1uA a magic value that is somehow used by RPMh regulators? Earlier you said: > > > [Bala]: On RPMh based designs, calling a regulator_set_load with a > > > non-zero > > > value moves the regulator to NPM (high power mode) which is required > > > for BT > > > communication. > > > and call with 0uA moves it to low-power-mode (vote from > > > APPS). > > > Basically whenever BT wants these in NPM it calls set-load with 1. This is the ->set_load function of the RPMh regulator: static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA) { struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); unsigned int mode; if (load_uA >= vreg->hw_data->hpm_min_load_uA) mode = REGULATOR_MODE_NORMAL; else mode = REGULATOR_MODE_IDLE; return rpmh_regulator_vrm_set_mode(rdev, mode); } and REGULATOR_MODE_NORMAL translates to HPM for PMIC4 LDOs: static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = { [REGULATOR_MODE_INVALID] = -EINVAL, [REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION, [REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM, [REGULATOR_MODE_NORMAL] = PMIC4_LDO_MODE_HPM, [REGULATOR_MODE_FAST] = -EINVAL, }; https://patchwork.kernel.org/patch/10524299/ I don't see how a load value of 1uA would cause the regulator to enter HPM and another higher value wouldn't. HPM is entered as long as 'load_uA >= vreg->hw_data->hpm_min_load_uA', or am I missing something here? -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Matthias, On 2018-07-18 22:43, Matthias Kaehlcke wrote: > On Wed, Jul 18, 2018 at 09:03:08PM +0530, Balakrishna Godavarthi wrote: >> Hi Matthias, >> >> On 2018-07-16 21:35, Matthias Kaehlcke wrote: >> > On Mon, Jul 16, 2018 at 07:21:56PM +0530, Balakrishna Godavarthi wrote: >> > > Hi Matthias, >> > > >> > > On 2018-07-07 03:51, Matthias Kaehlcke wrote: >> > > > On Thu, Jul 05, 2018 at 10:25:15PM +0530, Balakrishna Godavarthi wrote: >> > > > > Add support to set voltage/current of various regulators >> > > > > to power up/down Bluetooth chip wcn3990. >> > > > > >> > > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> >> > > > > --- >> > > > > changes in v9: >> > > > > * moved flow control to vendor and set_baudarte functions. >> > > > > * removed parent regs. >> > > > > >> > > > > changes in v8: >> > > > > * closing qca buffer, if qca_power_setup fails >> > > > > * chnaged ibs start timer function call location. >> > > > > * updated review comments. >> > > > > >> > > > > changes in v7: >> > > > > * addressed review comments. >> > > > > >> > > > > changes in v6: >> > > > > * Hooked up qca_power to qca_serdev. >> > > > > * renamed all the naming inconsistency functions with qca_* >> > > > > * leveraged common code of ROME for wcn3990. >> > > > > * created wrapper functions for re-usable blocks. >> > > > > * updated function of _*regulator_enable and _*regualtor_disable. >> > > > > * removed redundant comments and functions. >> > > > > * addressed review comments. >> > > > > >> > > > > Changes in v5: >> > > > > * updated regulator vddpa min_uV to 1304000. >> > > > > * addressed review comments. >> > > > > >> > > > > Changes in v4: >> > > > > * Segregated the changes of btqca from hci_qca >> > > > > * rebased all changes on top of bluetooth-next. >> > > > > * addressed review comments. >> > > > > >> > > > > --- >> > > > > drivers/bluetooth/btqca.h | 3 + >> > > > > drivers/bluetooth/hci_qca.c | 387 >> > > > > +++++++++++++++++++++++++++++++----- >> > > > > 2 files changed, 345 insertions(+), 45 deletions(-) >> > > > > >> > > > > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h >> > > > > index a9c2779f3e07..0c01f375fe83 100644 >> > > > > --- a/drivers/bluetooth/btqca.h >> > > > > +++ b/drivers/bluetooth/btqca.h >> > > > > @@ -37,6 +37,9 @@ >> > > > > #define EDL_TAG_ID_HCI (17) >> > > > > #define EDL_TAG_ID_DEEP_SLEEP (27) >> > > > > >> > > > > +#define QCA_WCN3990_POWERON_PULSE 0xFC >> > > > > +#define QCA_WCN3990_POWEROFF_PULSE 0xC0 >> > > > > + >> > > > > enum qca_bardrate { >> > > > > QCA_BAUDRATE_115200 = 0, >> > > > > QCA_BAUDRATE_57600, >> > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> > > > > index 7ebfaa0edf3f..d62c7785a618 100644 >> > > > > --- a/drivers/bluetooth/hci_qca.c >> > > > > +++ b/drivers/bluetooth/hci_qca.c >> > > > > @@ -5,7 +5,7 @@ >> > > > > * protocol extension to H4. >> > > > > * >> > > > > * Copyright (C) 2007 Texas Instruments, Inc. >> > > > > - * Copyright (c) 2010, 2012 The Linux Foundation. All rights >> > > > > reserved. >> > > > > + * Copyright (c) 2010, 2012, 2018 The Linux Foundation. All rights >> > > > > reserved. >> > > > > * >> > > > > * Acknowledgements: >> > > > > * This file is based on hci_ll.c, which was... >> > > > > @@ -31,9 +31,14 @@ >> > > > > #include <linux/kernel.h> >> > > > > #include <linux/clk.h> >> > > > > #include <linux/debugfs.h> >> > > > > +#include <linux/delay.h> >> > > > > +#include <linux/device.h> >> > > > > #include <linux/gpio/consumer.h> >> > > > > #include <linux/mod_devicetable.h> >> > > > > #include <linux/module.h> >> > > > > +#include <linux/of_device.h> >> > > > > +#include <linux/platform_device.h> >> > > > > +#include <linux/regulator/consumer.h> >> > > > > #include <linux/serdev.h> >> > > > > >> > > > > #include <net/bluetooth/bluetooth.h> >> > > > > @@ -124,12 +129,46 @@ enum qca_speed_type { >> > > > > QCA_OPER_SPEED >> > > > > }; >> > > > > >> > > > > +/* >> > > > > + * Voltage regulator information required for configuring the >> > > > > + * QCA Bluetooth chipset >> > > > > + */ >> > > > > +struct qca_vreg { >> > > > > + const char *name; >> > > > > + unsigned int min_uV; >> > > > > + unsigned int max_uV; >> > > > > + unsigned int load_uA; >> > > > > +}; >> > > > > + >> > > > > +struct qca_vreg_data { >> > > > > + enum qca_btsoc_type soc_type; >> > > > > + struct qca_vreg *vregs; >> > > > > + size_t num_vregs; >> > > > > +}; >> > > > > + >> > > > > +/* >> > > > > + * Platform data for the QCA Bluetooth power driver. >> > > > > + */ >> > > > > +struct qca_power { >> > > > > + struct device *dev; >> > > > > + const struct qca_vreg_data *vreg_data; >> > > > > + struct regulator_bulk_data *vreg_bulk; >> > > > > + bool vregs_on; >> > > > > +}; >> > > > > + >> > > > > struct qca_serdev { >> > > > > struct hci_uart serdev_hu; >> > > > > struct gpio_desc *bt_en; >> > > > > struct clk *susclk; >> > > > > + enum qca_btsoc_type btsoc_type; >> > > > > + struct qca_power *bt_power; >> > > > > + u32 init_speed; >> > > > > + u32 oper_speed; >> > > > > }; >> > > > > >> > > > > +static int qca_power_setup(struct hci_uart *hu, bool on); >> > > > > +static void qca_power_shutdown(struct hci_uart *hu); >> > > > > + >> > > > > static void __serial_clock_on(struct tty_struct *tty) >> > > > > { >> > > > > /* TODO: Some chipset requires to enable UART clock on client >> > > > > @@ -407,6 +446,7 @@ static int qca_open(struct hci_uart *hu) >> > > > > { >> > > > > struct qca_serdev *qcadev; >> > > > > struct qca_data *qca; >> > > > > + int ret; >> > > > > >> > > > > BT_DBG("hu %p qca_open", hu); >> > > > > >> > > > > @@ -458,19 +498,32 @@ static int qca_open(struct hci_uart *hu) >> > > > > >> > > > > hu->priv = qca; >> > > > > >> > > > > - timer_setup(&qca->wake_retrans_timer, >> > > > > hci_ibs_wake_retrans_timeout, 0); >> > > > > - qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; >> > > > > - >> > > > > - timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); >> > > > > - qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; >> > > > > - >> > > > > if (hu->serdev) { >> > > > > serdev_device_open(hu->serdev); >> > > > > >> > > > > qcadev = serdev_device_get_drvdata(hu->serdev); >> > > > > - gpiod_set_value_cansleep(qcadev->bt_en, 1); >> > > > > + if (qcadev->btsoc_type != QCA_WCN3990) { >> > > > > + gpiod_set_value_cansleep(qcadev->bt_en, 1); >> > > > > + } else { >> > > > > + hu->init_speed = qcadev->init_speed; >> > > > > + hu->oper_speed = qcadev->oper_speed; >> > > > > + ret = qca_power_setup(hu, true); >> > > > > + if (ret) { >> > > > > + destroy_workqueue(qca->workqueue); >> > > > > + kfree_skb(qca->rx_skb); >> > > > > + hu->priv = NULL; >> > > > > + kfree(qca); >> > > > > + return ret; >> > > > > + } >> > > > > + } >> > > > > } >> > > > > >> > > > > + timer_setup(&qca->wake_retrans_timer, >> > > > > hci_ibs_wake_retrans_timeout, 0); >> > > > > + qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; >> > > > > + >> > > > > + timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); >> > > > > + qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; >> > > > > + >> > > > > BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u", >> > > > > qca->tx_idle_delay, qca->wake_retrans); >> > > > > >> > > > > @@ -554,10 +607,13 @@ static int qca_close(struct hci_uart *hu) >> > > > > qca->hu = NULL; >> > > > > >> > > > > if (hu->serdev) { >> > > > > - serdev_device_close(hu->serdev); >> > > > > - >> > > > > qcadev = serdev_device_get_drvdata(hu->serdev); >> > > > > - gpiod_set_value_cansleep(qcadev->bt_en, 0); >> > > > > + if (qcadev->btsoc_type == QCA_WCN3990) >> > > > > + qca_power_shutdown(hu); >> > > > > + else >> > > > > + gpiod_set_value_cansleep(qcadev->bt_en, 0); >> > > > > + >> > > > > + serdev_device_close(hu->serdev); >> > > > > } >> > > > > >> > > > > kfree_skb(qca->rx_skb); >> > > > > @@ -891,6 +947,7 @@ 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) >> > > > > @@ -904,6 +961,13 @@ static int qca_set_baudrate(struct hci_dev >> > > > > *hdev, uint8_t baudrate) >> > > > > return -ENOMEM; >> > > > > } >> > > > > >> > > > > + /* Disabling hardware flow control is mandate while >> > > > >> > > > nit: s/mandate/mandatory|required/ >> > > > >> > > > > + * 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; >> > > > > @@ -919,6 +983,9 @@ 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; >> > > > > } >> > > > > >> > > > > @@ -930,6 +997,36 @@ static inline void host_set_baudrate(struct >> > > > > hci_uart *hu, unsigned int speed) >> > > > > hci_uart_set_baudrate(hu, speed); >> > > > > } >> > > > > >> > > > > +static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd) >> > > > > +{ >> > > > > + struct hci_uart *hu = hci_get_drvdata(hdev); >> > > > > + struct qca_data *qca = hu->priv; >> > > > > + struct sk_buff *skb; >> > > > > + >> > > > > + bt_dev_dbg(hdev, "sending command %02x to SoC", cmd); >> > > > > + >> > > > > + skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL); >> > > > > + if (!skb) >> > > > > + return -ENOMEM; >> > > > > + >> > > > > + /* Disabling hardware flow control is mandate while >> > > > >> > > > Same nit as above >> > > > >> > > > > + * sending vendor power on and off pulse to SoC. >> > > > > + */ >> > > > >> > > > The function is called qca_send_vendor_cmd(), the comment about power >> > > > on/off pulses seems misplaced here. Are there other 'vendor commands' >> > > > that require a different flow control behavior? >> > > > >> > > > Perhaps the function should have a different name or we need another >> > > > wrapper. >> > > > >> > > > > + hci_uart_set_flow_control(hu, true); >> > > > >> > > > Should the changing of the flow control be limited to wcn3990? As of >> > > > now the function is only called for wcn3990, however this is not >> > > > stated as a requirement and might change in the future. >> > > > >> > > > > + skb_put_u8(skb, cmd); >> > > > > + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT; >> > > > > + >> > > > > + skb_queue_tail(&qca->txq, skb); >> > > > > + hci_uart_tx_wakeup(hu); >> > > > > + >> > > > > + /* Wait for 100 uS for SoC to settle down */ >> > > > > + usleep_range(100, 200); >> > > > >> > > > Is this needed for any 'vendor command' or directly related with the >> > > > power on/off pulses? >> > > > >> > > > > + hci_uart_set_flow_control(hu, false); >> > > > > + >> > > > > + return 0; >> > > > > +} >> > > > > + >> > > > > static unsigned int qca_get_speed(struct hci_uart *hu, >> > > > > enum qca_speed_type speed_type) >> > > > > { >> > > > > @@ -952,10 +1049,19 @@ static unsigned int qca_get_speed(struct >> > > > > hci_uart *hu, >> > > > > >> > > > > static int qca_check_speeds(struct hci_uart *hu) >> > > > > { >> > > > > - /* One or the other speeds should be non zero. */ >> > > > > - if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> > > > > - !qca_get_speed(hu, QCA_OPER_SPEED)) >> > > > > + struct qca_serdev *qcadev; >> > > > > + >> > > > > + qcadev = serdev_device_get_drvdata(hu->serdev); >> > > > > + if ((qcadev->btsoc_type == QCA_WCN3990 && >> > > > > + !qca_get_speed(hu, QCA_INIT_SPEED)) || >> > > > > + !qca_get_speed(hu, QCA_OPER_SPEED)) { >> > > > > + /* Both INIT and OPER speed should be non zero. */ >> > > > > return -EINVAL; >> > > > > + } else if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> > > > > + !qca_get_speed(hu, QCA_OPER_SPEED)) { >> > > > > + /* One or the other speeds should be non zero. */ >> > > > > + return -EINVAL; >> > > > > + } >> > > > >> > > > It is questionable if the comments are useful, they are basically >> > > > stating the same as the conditions. >> > > > >> > > > if-else statements with a single statement in all branches shouldn't >> > > > use curly braces. Personally I don't dislike them in this case with >> > > > the multi-line conditions, but in principle they shoulnd't be there. >> > > > >> > > > nit: this would be easier to read with nested if statements: >> > > > >> > > > if (qcadev->btsoc_type == QCA_WCN3990) >> > > > if (!qca_get_speed(hu, QCA_INIT_SPEED)) || >> > > > !qca_get_speed(hu, QCA_OPER_SPEED)) >> > > > return -EINVAL; >> > > > else >> > > > if (!qca_get_speed(hu, QCA_INIT_SPEED) && >> > > > !qca_get_speed(hu, QCA_OPER_SPEED)) { >> > > > return -EINVAL; >> > > > >> > > > > return 0; >> > > > > } >> > > > > @@ -986,15 +1092,40 @@ static int qca_set_speed(struct hci_uart *hu, >> > > > > enum qca_speed_type speed_type) >> > > > > return 0; >> > > > > } >> > > > > >> > > > > +static int qca_wcn3990_init(struct hci_uart *hu) >> > > > > +{ >> > > > > + int ret; >> > > > > + >> > > > > + ret = qca_send_vendor_cmd(hu->hdev, QCA_WCN3990_POWERON_PULSE); >> > > > > + if (ret) >> > > > > + return ret; >> > > > > + >> > > > > + /* Wait for 100 ms for SoC to boot up */ >> > > > > + msleep(100); >> > > > > + serdev_device_close(hu->serdev); >> > > > > + ret = serdev_device_open(hu->serdev); >> > > > > + if (ret) { >> > > > > + bt_dev_err(hu->hdev, "failed to open port"); >> > > > > + return ret; >> > > > > + } >> > > > > + >> > > > > + /* Setup initial baudrate */ >> > > > > + qca_set_speed(hu, QCA_INIT_SPEED); >> > > > >> > > > The comment is a bit redundant. >> > > > >> > > > > + hci_uart_set_flow_control(hu, false); >> > > > > + >> > > > > + return 0; >> > > > > +} >> > > > > + >> > > > > static int qca_setup(struct hci_uart *hu) >> > > > > { >> > > > > struct hci_dev *hdev = hu->hdev; >> > > > > struct qca_data *qca = hu->priv; >> > > > > unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; >> > > > > + struct qca_serdev *qcadev; >> > > > > int ret; >> > > > > int soc_ver = 0; >> > > > > >> > > > > - bt_dev_info(hdev, "ROME setup"); >> > > > > + qcadev = serdev_device_get_drvdata(hu->serdev); >> > > > > >> > > > > ret = qca_check_speeds(hu); >> > > > > if (ret) >> > > > > @@ -1006,6 +1137,21 @@ static int qca_setup(struct hci_uart *hu) >> > > > > /* Setup initial baudrate */ >> > > > > qca_set_speed(hu, QCA_INIT_SPEED); >> > > > > >> > > > > + if (qcadev->btsoc_type == QCA_WCN3990) { >> > > > > + bt_dev_dbg(hdev, "setting up wcn3990"); >> > > > >> > > > Seems like this should be bt_dev_info() for consistency with Rome. >> > > > >> > > > > + ret = qca_wcn3990_init(hu); >> > > > > + if (ret) >> > > > > + return ret; >> > > > > + >> > > > > + ret = qca_read_soc_version(hdev, &soc_ver); >> > > > > + if (ret < 0 || soc_ver == 0) { >> > > > > + bt_dev_err(hdev, "Failed to get version %d", ret); >> > > > > + return ret; >> > > > > + } >> > > > > + } else { >> > > > > + bt_dev_info(hdev, "ROME setup"); >> > > > > + } >> > > > > + >> > > > > /* Setup user speed if needed */ >> > > > > speed = qca_get_speed(hu, QCA_OPER_SPEED); >> > > > > if (speed) { >> > > > > @@ -1016,16 +1162,18 @@ static int qca_setup(struct hci_uart *hu) >> > > > > qca_baudrate = qca_get_baudrate_value(speed); >> > > > > } >> > > > > >> > > > > - /* Get QCA version information */ >> > > > > - ret = qca_read_soc_version(hdev, &soc_ver); >> > > > > - if (ret < 0 || soc_ver == 0) { >> > > > > - bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); >> > > > > - return ret; >> > > > > + if (!soc_ver) { >> > > > >> > > > >> > > > The following would probably be clearer: >> > > > >> > > > if (qcadev->btsoc_type != QCA_WCN3990) { >> > > > >> > > > > + /* Get QCA version information */ >> > > > > + ret = qca_read_soc_version(hdev, &soc_ver); >> > > > > + if (ret < 0 || soc_ver == 0) { >> > > > > + bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); >> > > > > + return ret; >> > > > > + } >> > > > > + bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); >> > > > >> > > > Couldn't we hide some of this in qca_read_soc_version()? >> > > > >> > > > qca_read_soc_version() could at least do the logging in the error case >> > > > (the 'soc_ver == 0' case could be handled there as well), which would >> > > > leave us with: >> > > > >> > > > >> > > > ret = qca_read_soc_version(hdev, &soc_ver); >> > > > if (ret) >> > > > return ret; >> > > > >> > > > And the same above. >> > > > >> > > > The controller version can be logged outside of the branch for both >> > > > wcn3990 and Rome. >> > > > >> > > > > } >> > > > > - bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); >> > > > > >> > > > > /* Setup patch / NVM configurations */ >> > > > > - ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver); >> > > > > + ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, >> > > > > soc_ver); >> > > > > if (!ret) { >> > > > > set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); >> > > > > qca_debugfs_init(hdev); >> > > > > @@ -1061,9 +1209,123 @@ static struct hci_uart_proto qca_proto = { >> > > > > .dequeue = qca_dequeue, >> > > > > }; >> > > > > >> > > > > +static const struct qca_vreg_data qca_soc_data = { >> > > > > + .soc_type = QCA_WCN3990, >> > > > > + .vregs = (struct qca_vreg []) { >> > > > > + { "vddio", 1800000, 1800000, 0 }, >> > > > > + { "vddxo", 1800000, 1800000, 1 }, >> > > > > + { "vddrf", 1304000, 1304000, 1 }, >> > > > > + { "vddch0", 3000000, 3312000, 1 }, >> > > > > + }, >> > > > > + .num_vregs = 4, >> > > > > +}; >> > > > >> > > > I didn't chime in earlier in the discussion with Stephen on the >> > > > regulators (https://patchwork.kernel.org/patch/10467911/), however I >> > > > agree with him that specifying at load of 1uA doesn't seem to make >> > > > much sense. What would happen if the load remained unspecified (or 0)? >> > > >> > > [Bala]: On RPMh based designs, calling a regulator_set_load with a >> > > non-zero >> > > value moves the regulator to NPM (high power mode) which is required >> > > for BT >> > > communication. >> > > and call with 0uA moves it to low-power-mode (vote from >> > > APPS). >> > > Basically whenever BT wants these in NPM it calls set-load with 1. >> > >> > This assumes that the chip is powered by RPMh regulators, which >> > depending on the system may or may not be true. On systems with other >> > regulators you are telling the regulator that the max load is 1uA, and >> > the regulator might comply, going in a low power mode that does not >> > provide enough current for the chip to operate properly. >> > >> > Doesn't the datasheet of the chip specify actual max currents for >> > these consumers that could be used instead of the bogus 1uA? If not >> > you could ask the designers of the chip for an approximate number >> > (preferably erring on the upper side). >> >> [Bala]: yes your true, now current values are aligned according to the >> RPMH >> regs packages. >> this is not our goal. what ever the package we use wcn3990 >> should be >> platform independent. >> I propose an idea that, we will have default current value in >> driver >> i.e current required by regs to turn on wcn3990 and also we will >> provide >> interface in driver >> to read current from dts tree. >> >> i.e. if we have prop for current to the reg in the dts, we will >> read >> the value assigned to prop else we will take default current >> >> <code snippets> >> >> qca->vreg_data->vregs[i].name = current; // holds the default >> current > > I suppose this should be 'qca->vreg_data->vregs[i].load_uA' > >> value taken from data provided from deb. this will value remains >> unchanged. >> if (device_property_read_bool(dev, prop_name)) // if we have >> prop >> name enabled then read the value. >> device_property_read_u32(dev, prop_name, >> &vregs->load_uA); > > It's still not clear why this would be needed. Is 1uA a magic value > that is somehow used by RPMh regulators? > > Earlier you said: > >> > > [Bala]: On RPMh based designs, calling a regulator_set_load with a >> > > non-zero >> > > value moves the regulator to NPM (high power mode) which is required >> > > for BT >> > > communication. >> > > and call with 0uA moves it to low-power-mode (vote from >> > > APPS). >> > > Basically whenever BT wants these in NPM it calls set-load with 1. > > This is the ->set_load function of the RPMh regulator: > > static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int > load_uA) > { > struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); > unsigned int mode; > > if (load_uA >= vreg->hw_data->hpm_min_load_uA) > mode = REGULATOR_MODE_NORMAL; > else > mode = REGULATOR_MODE_IDLE; > > return rpmh_regulator_vrm_set_mode(rdev, mode); > } > > and REGULATOR_MODE_NORMAL translates to HPM for PMIC4 LDOs: > > static const int pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = > { > [REGULATOR_MODE_INVALID] = -EINVAL, > [REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION, > [REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM, > [REGULATOR_MODE_NORMAL] = PMIC4_LDO_MODE_HPM, > [REGULATOR_MODE_FAST] = -EINVAL, > }; > > https://patchwork.kernel.org/patch/10524299/ > > I don't see how a load value of 1uA would cause the regulator to enter > HPM and another higher value wouldn't. HPM is entered as long as > 'load_uA >= vreg->hw_data->hpm_min_load_uA', or am I missing > something here? [Bala]: yes you are correct, both 0 and 1 uA are are entering into HPM mode. i have asked info from PMIC vendor to address this query. Mean time i have removed 1 uA current from the code and added the required current for turning on of WCN3990.
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h index a9c2779f3e07..0c01f375fe83 100644 --- a/drivers/bluetooth/btqca.h +++ b/drivers/bluetooth/btqca.h @@ -37,6 +37,9 @@ #define EDL_TAG_ID_HCI (17) #define EDL_TAG_ID_DEEP_SLEEP (27) +#define QCA_WCN3990_POWERON_PULSE 0xFC +#define QCA_WCN3990_POWEROFF_PULSE 0xC0 + enum qca_bardrate { QCA_BAUDRATE_115200 = 0, QCA_BAUDRATE_57600, diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index 7ebfaa0edf3f..d62c7785a618 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -5,7 +5,7 @@ * protocol extension to H4. * * Copyright (C) 2007 Texas Instruments, Inc. - * Copyright (c) 2010, 2012 The Linux Foundation. All rights reserved. + * Copyright (c) 2010, 2012, 2018 The Linux Foundation. All rights reserved. * * Acknowledgements: * This file is based on hci_ll.c, which was... @@ -31,9 +31,14 @@ #include <linux/kernel.h> #include <linux/clk.h> #include <linux/debugfs.h> +#include <linux/delay.h> +#include <linux/device.h> #include <linux/gpio/consumer.h> #include <linux/mod_devicetable.h> #include <linux/module.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> #include <linux/serdev.h> #include <net/bluetooth/bluetooth.h> @@ -124,12 +129,46 @@ enum qca_speed_type { QCA_OPER_SPEED }; +/* + * Voltage regulator information required for configuring the + * QCA Bluetooth chipset + */ +struct qca_vreg { + const char *name; + unsigned int min_uV; + unsigned int max_uV; + unsigned int load_uA; +}; + +struct qca_vreg_data { + enum qca_btsoc_type soc_type; + struct qca_vreg *vregs; + size_t num_vregs; +}; + +/* + * Platform data for the QCA Bluetooth power driver. + */ +struct qca_power { + struct device *dev; + const struct qca_vreg_data *vreg_data; + struct regulator_bulk_data *vreg_bulk; + bool vregs_on; +}; + struct qca_serdev { struct hci_uart serdev_hu; struct gpio_desc *bt_en; struct clk *susclk; + enum qca_btsoc_type btsoc_type; + struct qca_power *bt_power; + u32 init_speed; + u32 oper_speed; }; +static int qca_power_setup(struct hci_uart *hu, bool on); +static void qca_power_shutdown(struct hci_uart *hu); + static void __serial_clock_on(struct tty_struct *tty) { /* TODO: Some chipset requires to enable UART clock on client @@ -407,6 +446,7 @@ static int qca_open(struct hci_uart *hu) { struct qca_serdev *qcadev; struct qca_data *qca; + int ret; BT_DBG("hu %p qca_open", hu); @@ -458,19 +498,32 @@ static int qca_open(struct hci_uart *hu) hu->priv = qca; - timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, 0); - qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; - - timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); - qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; - if (hu->serdev) { serdev_device_open(hu->serdev); qcadev = serdev_device_get_drvdata(hu->serdev); - gpiod_set_value_cansleep(qcadev->bt_en, 1); + if (qcadev->btsoc_type != QCA_WCN3990) { + gpiod_set_value_cansleep(qcadev->bt_en, 1); + } else { + hu->init_speed = qcadev->init_speed; + hu->oper_speed = qcadev->oper_speed; + ret = qca_power_setup(hu, true); + if (ret) { + destroy_workqueue(qca->workqueue); + kfree_skb(qca->rx_skb); + hu->priv = NULL; + kfree(qca); + return ret; + } + } } + timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, 0); + qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS; + + timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); + qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; + BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u", qca->tx_idle_delay, qca->wake_retrans); @@ -554,10 +607,13 @@ static int qca_close(struct hci_uart *hu) qca->hu = NULL; if (hu->serdev) { - serdev_device_close(hu->serdev); - qcadev = serdev_device_get_drvdata(hu->serdev); - gpiod_set_value_cansleep(qcadev->bt_en, 0); + if (qcadev->btsoc_type == QCA_WCN3990) + qca_power_shutdown(hu); + else + gpiod_set_value_cansleep(qcadev->bt_en, 0); + + serdev_device_close(hu->serdev); } kfree_skb(qca->rx_skb); @@ -891,6 +947,7 @@ 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) @@ -904,6 +961,13 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate) return -ENOMEM; } + /* Disabling hardware flow control is mandate 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; @@ -919,6 +983,9 @@ 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; } @@ -930,6 +997,36 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed) hci_uart_set_baudrate(hu, speed); } +static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd) +{ + struct hci_uart *hu = hci_get_drvdata(hdev); + struct qca_data *qca = hu->priv; + struct sk_buff *skb; + + bt_dev_dbg(hdev, "sending command %02x to SoC", cmd); + + skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL); + if (!skb) + return -ENOMEM; + + /* Disabling hardware flow control is mandate while + * sending vendor power on and off pulse to SoC. + */ + hci_uart_set_flow_control(hu, true); + + skb_put_u8(skb, cmd); + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT; + + skb_queue_tail(&qca->txq, skb); + hci_uart_tx_wakeup(hu); + + /* Wait for 100 uS for SoC to settle down */ + usleep_range(100, 200); + hci_uart_set_flow_control(hu, false); + + return 0; +} + static unsigned int qca_get_speed(struct hci_uart *hu, enum qca_speed_type speed_type) { @@ -952,10 +1049,19 @@ static unsigned int qca_get_speed(struct hci_uart *hu, static int qca_check_speeds(struct hci_uart *hu) { - /* One or the other speeds should be non zero. */ - if (!qca_get_speed(hu, QCA_INIT_SPEED) && - !qca_get_speed(hu, QCA_OPER_SPEED)) + struct qca_serdev *qcadev; + + qcadev = serdev_device_get_drvdata(hu->serdev); + if ((qcadev->btsoc_type == QCA_WCN3990 && + !qca_get_speed(hu, QCA_INIT_SPEED)) || + !qca_get_speed(hu, QCA_OPER_SPEED)) { + /* Both INIT and OPER speed should be non zero. */ return -EINVAL; + } else if (!qca_get_speed(hu, QCA_INIT_SPEED) && + !qca_get_speed(hu, QCA_OPER_SPEED)) { + /* One or the other speeds should be non zero. */ + return -EINVAL; + } return 0; } @@ -986,15 +1092,40 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type) return 0; } +static int qca_wcn3990_init(struct hci_uart *hu) +{ + int ret; + + ret = qca_send_vendor_cmd(hu->hdev, QCA_WCN3990_POWERON_PULSE); + if (ret) + return ret; + + /* Wait for 100 ms for SoC to boot up */ + msleep(100); + serdev_device_close(hu->serdev); + ret = serdev_device_open(hu->serdev); + if (ret) { + bt_dev_err(hu->hdev, "failed to open port"); + return ret; + } + + /* Setup initial baudrate */ + qca_set_speed(hu, QCA_INIT_SPEED); + hci_uart_set_flow_control(hu, false); + + return 0; +} + static int qca_setup(struct hci_uart *hu) { struct hci_dev *hdev = hu->hdev; struct qca_data *qca = hu->priv; unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; + struct qca_serdev *qcadev; int ret; int soc_ver = 0; - bt_dev_info(hdev, "ROME setup"); + qcadev = serdev_device_get_drvdata(hu->serdev); ret = qca_check_speeds(hu); if (ret) @@ -1006,6 +1137,21 @@ static int qca_setup(struct hci_uart *hu) /* Setup initial baudrate */ qca_set_speed(hu, QCA_INIT_SPEED); + if (qcadev->btsoc_type == QCA_WCN3990) { + bt_dev_dbg(hdev, "setting up wcn3990"); + ret = qca_wcn3990_init(hu); + if (ret) + return ret; + + ret = qca_read_soc_version(hdev, &soc_ver); + if (ret < 0 || soc_ver == 0) { + bt_dev_err(hdev, "Failed to get version %d", ret); + return ret; + } + } else { + bt_dev_info(hdev, "ROME setup"); + } + /* Setup user speed if needed */ speed = qca_get_speed(hu, QCA_OPER_SPEED); if (speed) { @@ -1016,16 +1162,18 @@ static int qca_setup(struct hci_uart *hu) qca_baudrate = qca_get_baudrate_value(speed); } - /* Get QCA version information */ - ret = qca_read_soc_version(hdev, &soc_ver); - if (ret < 0 || soc_ver == 0) { - bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); - return ret; + if (!soc_ver) { + /* Get QCA version information */ + ret = qca_read_soc_version(hdev, &soc_ver); + if (ret < 0 || soc_ver == 0) { + bt_dev_err(hdev, "QCA Failed to get version (%d)", ret); + return ret; + } + bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); } - bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver); /* Setup patch / NVM configurations */ - ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver); + ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, soc_ver); if (!ret) { set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); qca_debugfs_init(hdev); @@ -1061,9 +1209,123 @@ static struct hci_uart_proto qca_proto = { .dequeue = qca_dequeue, }; +static const struct qca_vreg_data qca_soc_data = { + .soc_type = QCA_WCN3990, + .vregs = (struct qca_vreg []) { + { "vddio", 1800000, 1800000, 0 }, + { "vddxo", 1800000, 1800000, 1 }, + { "vddrf", 1304000, 1304000, 1 }, + { "vddch0", 3000000, 3312000, 1 }, + }, + .num_vregs = 4, +}; + +static void qca_power_shutdown(struct hci_uart *hu) +{ + struct hci_dev *hdev = hu->hdev; + + host_set_baudrate(hu, 2400); + qca_send_vendor_cmd(hdev, QCA_WCN3990_POWEROFF_PULSE); + qca_power_setup(hu, false); +} + +static int qca_enable_regulator(struct qca_vreg vregs, + struct regulator *regulator) +{ + int ret; + + ret = regulator_set_voltage(regulator, vregs.min_uV, + vregs.max_uV); + if (ret) + return ret; + + if (vregs.load_uA) + ret = regulator_set_load(regulator, + vregs.load_uA); + + if (ret) + return ret; + + return regulator_enable(regulator); + +} + +static void qca_disable_regulator(struct qca_vreg vregs, + struct regulator *regulator) +{ + regulator_disable(regulator); + regulator_set_voltage(regulator, 0, vregs.max_uV); + if (vregs.load_uA) + regulator_set_load(regulator, 0); + +} + +static int qca_power_setup(struct hci_uart *hu, bool on) +{ + struct qca_vreg *vregs; + struct regulator_bulk_data *vreg_bulk; + struct qca_serdev *qcadev; + int i, num_vregs, ret = 0; + + qcadev = serdev_device_get_drvdata(hu->serdev); + if (!qcadev || !qcadev->bt_power || !qcadev->bt_power->vreg_data || + !qcadev->bt_power->vreg_bulk) + return -EINVAL; + + vregs = qcadev->bt_power->vreg_data->vregs; + vreg_bulk = qcadev->bt_power->vreg_bulk; + num_vregs = qcadev->bt_power->vreg_data->num_vregs; + BT_DBG("on: %d", on); + if (on && !qcadev->bt_power->vregs_on) { + for (i = 0; i < num_vregs; i++) { + ret = qca_enable_regulator(vregs[i], + vreg_bulk[i].consumer); + if (ret) + break; + } + + if (ret) { + BT_ERR("failed to enable regulator:%s", vregs[i].name); + /* turn off regulators which are enabled */ + for (i = i - 1; i >= 0; i--) + qca_disable_regulator(vregs[i], + vreg_bulk[i].consumer); + } else { + qcadev->bt_power->vregs_on = true; + } + } else if (!on && qcadev->bt_power->vregs_on) { + /* turn off regulator in reverse order */ + i = qcadev->bt_power->vreg_data->num_vregs - 1; + for ( ; i >= 0; i--) + qca_disable_regulator(vregs[i], vreg_bulk[i].consumer); + + qcadev->bt_power->vregs_on = false; + } + + return ret; +} + +static int qca_init_regulators(struct qca_power *qca, + const struct qca_vreg *vregs, size_t num_vregs) +{ + int i; + + qca->vreg_bulk = devm_kzalloc(qca->dev, num_vregs * + sizeof(struct regulator_bulk_data), + GFP_KERNEL); + if (!qca->vreg_bulk) + return -ENOMEM; + + for (i = 0; i < num_vregs; i++) + qca->vreg_bulk[i].supply = vregs[i].name; + + return devm_regulator_bulk_get(qca->dev, num_vregs, qca->vreg_bulk); +} + static int qca_serdev_probe(struct serdev_device *serdev) { struct qca_serdev *qcadev; + const struct qca_vreg_data *data; int err; qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL); @@ -1071,47 +1333,82 @@ static int qca_serdev_probe(struct serdev_device *serdev) return -ENOMEM; qcadev->serdev_hu.serdev = serdev; + data = of_device_get_match_data(&serdev->dev); serdev_device_set_drvdata(serdev, qcadev); + if (data && data->soc_type == QCA_WCN3990) { + qcadev->btsoc_type = QCA_WCN3990; + qcadev->bt_power = devm_kzalloc(&serdev->dev, + sizeof(struct qca_power), + GFP_KERNEL); + if (!qcadev->bt_power) + return -ENOMEM; + + qcadev->bt_power->dev = &serdev->dev; + qcadev->bt_power->vreg_data = data; + err = qca_init_regulators(qcadev->bt_power, data->vregs, + data->num_vregs); + if (err) { + BT_ERR("Failed to init regulators:%d", err); + goto out; + } - qcadev->bt_en = devm_gpiod_get(&serdev->dev, "enable", - GPIOD_OUT_LOW); - if (IS_ERR(qcadev->bt_en)) { - dev_err(&serdev->dev, "failed to acquire enable gpio\n"); - return PTR_ERR(qcadev->bt_en); - } + qcadev->bt_power->vregs_on = false; + device_property_read_u32(&serdev->dev, "max-speed", + &qcadev->oper_speed); + if (!qcadev->oper_speed) + BT_INFO("UART will pick default operating speed"); + err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto); + if (err) { + BT_ERR("wcn3990 serdev registration failed"); + goto out; + } + } else { + qcadev->btsoc_type = QCA_ROME; + qcadev->bt_en = devm_gpiod_get(&serdev->dev, "enable", + GPIOD_OUT_LOW); + if (IS_ERR(qcadev->bt_en)) { + dev_err(&serdev->dev, "failed to acquire enable gpio\n"); + return PTR_ERR(qcadev->bt_en); + } - 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); - } + 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); + } - err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ); - if (err) - return err; + err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ); + if (err) + return err; - err = clk_prepare_enable(qcadev->susclk); - if (err) - return err; + err = clk_prepare_enable(qcadev->susclk); + if (err) + return err; - err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto); - if (err) - clk_disable_unprepare(qcadev->susclk); + err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto); + if (err) + clk_disable_unprepare(qcadev->susclk); + } + +out: return err; - return err; } static void qca_serdev_remove(struct serdev_device *serdev) { struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev); - hci_uart_unregister_device(&qcadev->serdev_hu); + if (qcadev->btsoc_type == QCA_WCN3990) + qca_power_shutdown(&qcadev->serdev_hu); + else + clk_disable_unprepare(qcadev->susclk); - clk_disable_unprepare(qcadev->susclk); + hci_uart_unregister_device(&qcadev->serdev_hu); } static const struct of_device_id qca_bluetooth_of_match[] = { { .compatible = "qcom,qca6174-bt" }, + { .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data}, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
Add support to set voltage/current of various regulators to power up/down Bluetooth chip wcn3990. Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org> --- changes in v9: * moved flow control to vendor and set_baudarte functions. * removed parent regs. changes in v8: * closing qca buffer, if qca_power_setup fails * chnaged ibs start timer function call location. * updated review comments. changes in v7: * addressed review comments. changes in v6: * Hooked up qca_power to qca_serdev. * renamed all the naming inconsistency functions with qca_* * leveraged common code of ROME for wcn3990. * created wrapper functions for re-usable blocks. * updated function of _*regulator_enable and _*regualtor_disable. * removed redundant comments and functions. * addressed review comments. Changes in v5: * updated regulator vddpa min_uV to 1304000. * addressed review comments. Changes in v4: * Segregated the changes of btqca from hci_qca * rebased all changes on top of bluetooth-next. * addressed review comments. --- drivers/bluetooth/btqca.h | 3 + drivers/bluetooth/hci_qca.c | 387 +++++++++++++++++++++++++++++++----- 2 files changed, 345 insertions(+), 45 deletions(-)