| Message ID | 20180616062718.29844-9-bgodavar@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
Quoting Balakrishna Godavarthi (2018-06-15 23:27:18) > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index 28ae6a17a595..1961e313aae7 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -1031,9 +1145,118 @@ static struct hci_uart_proto qca_proto = { > .dequeue = qca_dequeue, > }; > > +static const struct qca_vreg_data qca_cherokee_data = { > + .soc_type = QCA_WCN3990, > + .vregs = (struct qca_vreg []) { > + { "vddio", 1352000, 1352000, 0 }, > + { "vddxtal", 1904000, 2040000, 0 }, > + { "vddcore", 1800000, 1800000, 1 }, > + { "vddpa", 1304000, 1304000, 1 }, > + { "vddldo", 3000000, 3312000, 1 }, Load of 0 and 1 seems sort of odd. Are these made up to indicate "some load" vs. "no load"? > + }, > + .num_vregs = 5, > +}; > + > +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) > + goto out; Just return ret; > + > + if (vregs.load_uA) > + ret = regulator_set_load(regulator, > + vregs.load_uA); > + > + if (ret) > + goto out; > + Same. > + ret = regulator_enable(regulator); > + > +out: > + return ret; And make this return regulator_enable(regulator). > + > +} > + -- 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 Stephen, Please find my comments inline. On 2018-06-18 22:12, Stephen Boyd wrote: > Quoting Balakrishna Godavarthi (2018-06-15 23:27:18) >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index 28ae6a17a595..1961e313aae7 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -1031,9 +1145,118 @@ static struct hci_uart_proto qca_proto = { >> .dequeue = qca_dequeue, >> }; >> >> +static const struct qca_vreg_data qca_cherokee_data = { >> + .soc_type = QCA_WCN3990, >> + .vregs = (struct qca_vreg []) { >> + { "vddio", 1352000, 1352000, 0 }, >> + { "vddxtal", 1904000, 2040000, 0 }, >> + { "vddcore", 1800000, 1800000, 1 }, >> + { "vddpa", 1304000, 1304000, 1 }, >> + { "vddldo", 3000000, 3312000, 1 }, > > Load of 0 and 1 seems sort of odd. Are these made up to indicate "some > load" vs. "no load"? > [Bala]: this value specifies the output load current required to turn on the wcn3990. in struct defined vddio\vddxtal are smps, with fixed load. regs from vddcore/vddpa/vddldo are programmable line regulators, in which we need to set the basic load. >> + }, >> + .num_vregs = 5, >> +}; >> + >> +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) >> + goto out; > > Just return ret; [Bala]: will update. > >> + >> + if (vregs.load_uA) >> + ret = regulator_set_load(regulator, >> + vregs.load_uA); >> + >> + if (ret) >> + goto out; >> + > > Same. > [Bala]: will update. >> + ret = regulator_enable(regulator); >> + >> +out: >> + return ret; > > And make this return regulator_enable(regulator). > [Bala]: will update. >> + >> +} >> + Thanks for reviewing the patch.
On Sat, Jun 16, 2018 at 11:57:18AM +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> > --- > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index 28ae6a17a595..1961e313aae7 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -402,6 +441,7 @@ static int qca_open(struct hci_uart *hu) > { > struct qca_serdev *qcadev; > struct qca_data *qca; > + int ret = 0; > > BT_DBG("hu %p qca_open", hu); > > @@ -463,13 +503,19 @@ static int qca_open(struct hci_uart *hu) > 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) { > + hu->init_speed = qcadev->init_speed; > + hu->oper_speed = qcadev->oper_speed; > + ret = qca_btsoc_power_setup(hu, true); Better do this before starting the timers, otherwise you need to take care of stopping them in case of failure. If qca_btsoc_power_setup() fails you also have to free 'qca->workqueue' and 'qca'. > +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) { > + bt_dev_err(hdev, "Failed to allocate memory for skb packet"); I was told that custom OOM messages should not be used, since the mm code will complain loudly in this case. > +static int qca_btsoc_shutdown(struct hci_uart *hu) The return value is not evaluated by the only caller, so this should probably be void. > static int qca_setup(struct hci_uart *hu) > { > struct hci_dev *hdev = hu->hdev; > struct qca_data *qca = hu->priv; > + struct qca_serdev *qcadev; > unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; > int ret; > int soc_ver = 0; > > - bt_dev_info(hdev, "ROME setup"); > + qcadev = serdev_device_get_drvdata(hu->serdev); > > /* Patch downloading has to be done without IBS mode */ > clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); > - > - /* Setup initial baudrate */ > qca_set_init_speed(hu); > + > + if (qcadev->btsoc_type == QCA_WCN3990) { > + bt_dev_dbg(hdev, "setting up wcn3990"); > + hci_uart_set_flow_control(hu, true); > + ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE); > + if (ret) { > + bt_dev_err(hdev, "failed to send power on command"); > + return ret; > + } > + serdev_device_close(hu->serdev); > + ret = serdev_device_open(hu->serdev); > + if (ret) { > + bt_dev_err(hdev, "failed to open port"); > + return ret; > + } > + msleep(100); > + qca_set_init_speed(hu); > + hci_uart_set_flow_control(hu, false); > + ret = qca_read_soc_version(hdev, &soc_ver); > + if (ret < 0 || soc_ver == 0) { > + bt_dev_err(hdev, "Failed to get version %d", ret); serdev_device_close() ? Also applies to other error paths in this function. > 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); > @@ -1041,47 +1264,85 @@ static int qca_serdev_probe(struct serdev_device *serdev) > return -ENOMEM; > > qcadev->serdev_hu.serdev = serdev; > + data = of_device_get_match_data(&serdev->dev); > + if (data && data->soc_type == QCA_WCN3990) > + qcadev->btsoc_type = QCA_WCN3990; > + else > + qcadev->btsoc_type = QCA_ROME; > + > serdev_device_set_drvdata(serdev, qcadev); > + if (qcadev->btsoc_type == QCA_WCN3990) { nit: the double "if (soc_type == QCA_WCN3990)" is a bit odd. Consider changing this condition to "if (data && data->soc_type == QCA_WCN3990)" and assign qcadev->btsoc_type in the corresponding branch. -- 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-06-20 03:23, Matthias Kaehlcke wrote: > On Sat, Jun 16, 2018 at 11:57:18AM +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> >> --- >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index 28ae6a17a595..1961e313aae7 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -402,6 +441,7 @@ static int qca_open(struct hci_uart *hu) >> { >> struct qca_serdev *qcadev; >> struct qca_data *qca; >> + int ret = 0; >> >> BT_DBG("hu %p qca_open", hu); >> >> @@ -463,13 +503,19 @@ static int qca_open(struct hci_uart *hu) >> 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) { >> + hu->init_speed = qcadev->init_speed; >> + hu->oper_speed = qcadev->oper_speed; >> + ret = qca_btsoc_power_setup(hu, true); > > Better do this before starting the timers, otherwise you need to take > care of stopping them in case of failure. > > If qca_btsoc_power_setup() fails you also have to free > 'qca->workqueue' and 'qca'. > [Bala]: i missed this.will update >> +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) { >> + bt_dev_err(hdev, "Failed to allocate memory for skb packet"); > > I was told that custom OOM messages should not be used, since the > mm code will complain loudly in this case. > >> +static int qca_btsoc_shutdown(struct hci_uart *hu) > > The return value is not evaluated by the only caller, so this should > probably be void. > [Bala]: will update >> static int qca_setup(struct hci_uart *hu) >> { >> struct hci_dev *hdev = hu->hdev; >> struct qca_data *qca = hu->priv; >> + struct qca_serdev *qcadev; >> unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; >> int ret; >> int soc_ver = 0; >> >> - bt_dev_info(hdev, "ROME setup"); >> + qcadev = serdev_device_get_drvdata(hu->serdev); >> >> /* Patch downloading has to be done without IBS mode */ >> clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); >> - >> - /* Setup initial baudrate */ >> qca_set_init_speed(hu); >> + >> + if (qcadev->btsoc_type == QCA_WCN3990) { >> + bt_dev_dbg(hdev, "setting up wcn3990"); >> + hci_uart_set_flow_control(hu, true); >> + ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE); >> + if (ret) { >> + bt_dev_err(hdev, "failed to send power on command"); >> + return ret; >> + } >> + serdev_device_close(hu->serdev); >> + ret = serdev_device_open(hu->serdev); >> + if (ret) { >> + bt_dev_err(hdev, "failed to open port"); >> + return ret; >> + } >> + msleep(100); >> + qca_set_init_speed(hu); >> + hci_uart_set_flow_control(hu, false); >> + ret = qca_read_soc_version(hdev, &soc_ver); >> + if (ret < 0 || soc_ver == 0) { >> + bt_dev_err(hdev, "Failed to get version %d", ret); > > serdev_device_close() ? > > Also applies to other error paths in this function. > [Bala]: sorry, i didn't get you. >> 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); >> @@ -1041,47 +1264,85 @@ static int qca_serdev_probe(struct >> serdev_device *serdev) >> return -ENOMEM; >> >> qcadev->serdev_hu.serdev = serdev; >> + data = of_device_get_match_data(&serdev->dev); >> + if (data && data->soc_type == QCA_WCN3990) >> + qcadev->btsoc_type = QCA_WCN3990; >> + else >> + qcadev->btsoc_type = QCA_ROME; >> + >> serdev_device_set_drvdata(serdev, qcadev); >> + if (qcadev->btsoc_type == QCA_WCN3990) { > > nit: the double "if (soc_type == QCA_WCN3990)" is a bit odd. Consider > changing this condition to "if (data && data->soc_type == QCA_WCN3990)" > and assign qcadev->btsoc_type in the corresponding branch. [bala]: will update. I have idea of removing flow control from qca_setup() and use them in qca_set_speed() "i need to disable hardware flow control when sending change baudrate request to WCN3990. enabling it after setting host baudrate. this is only for wcn3990. " static int qca_set_speed(struct hci_uart *hu, unsigned int speed, enum qca_speed_type speed_type, enum qca_btsoc_type soc_type) { unsigned int qca_baudrate; int ret; if (speed_type == QCA_INIT_SPEED) goto change_host_baudrate; if (soc_type == QCA_WCN3990) hci_uart_set_flow_control(hu, true); qca_baudrate = qca_get_baudrate_value(speed); bt_dev_info(hu->hdev, "Set UART speed to %d", speed); ret = qca_set_baudrate(hu->hdev, qca_baudrate); if (ret) { bt_dev_err(hu->hdev, "Failed to change the baudrate (%d)", ret); return ret; } change_host_baudrate: host_set_baudrate(hu, speed); if (soc_type == QCA_WCN3990) hci_uart_set_flow_control(hu, false); return ret; } is it good idea?
Hi Balakrishna, On Thu, Jun 21, 2018 at 07:30:25PM +0530, Balakrishna Godavarthi wrote: > Hi Matthias, > > On 2018-06-20 03:23, Matthias Kaehlcke wrote: > > On Sat, Jun 16, 2018 at 11:57:18AM +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> > > > --- > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > > > index 28ae6a17a595..1961e313aae7 100644 > > > --- a/drivers/bluetooth/hci_qca.c > > > +++ b/drivers/bluetooth/hci_qca.c > > > > > > static int qca_setup(struct hci_uart *hu) > > > { > > > struct hci_dev *hdev = hu->hdev; > > > struct qca_data *qca = hu->priv; > > > + struct qca_serdev *qcadev; > > > unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; > > > int ret; > > > int soc_ver = 0; > > > > > > - bt_dev_info(hdev, "ROME setup"); > > > + qcadev = serdev_device_get_drvdata(hu->serdev); > > > > > > /* Patch downloading has to be done without IBS mode */ > > > clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); > > > - > > > - /* Setup initial baudrate */ > > > qca_set_init_speed(hu); > > > + > > > + if (qcadev->btsoc_type == QCA_WCN3990) { > > > + bt_dev_dbg(hdev, "setting up wcn3990"); > > > + hci_uart_set_flow_control(hu, true); > > > + ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE); > > > + if (ret) { > > > + bt_dev_err(hdev, "failed to send power on command"); > > > + return ret; > > > + } > > > + serdev_device_close(hu->serdev); > > > + ret = serdev_device_open(hu->serdev); > > > + if (ret) { > > > + bt_dev_err(hdev, "failed to open port"); > > > + return ret; > > > + } > > > + msleep(100); > > > + qca_set_init_speed(hu); > > > + hci_uart_set_flow_control(hu, false); > > > + ret = qca_read_soc_version(hdev, &soc_ver); > > > + if (ret < 0 || soc_ver == 0) { > > > + bt_dev_err(hdev, "Failed to get version %d", ret); > > > > serdev_device_close() ? > > > > Also applies to other error paths in this function. > > > [Bala]: sorry, i didn't get you. A few lines above serdev_device_open() is called, in the error paths the device should be closed. > > > 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); > > > @@ -1041,47 +1264,85 @@ static int qca_serdev_probe(struct > > > serdev_device *serdev) > > > return -ENOMEM; > > > > > > qcadev->serdev_hu.serdev = serdev; > > > + data = of_device_get_match_data(&serdev->dev); > > > + if (data && data->soc_type == QCA_WCN3990) > > > + qcadev->btsoc_type = QCA_WCN3990; > > > + else > > > + qcadev->btsoc_type = QCA_ROME; > > > + > > > serdev_device_set_drvdata(serdev, qcadev); > > > + if (qcadev->btsoc_type == QCA_WCN3990) { > > > > nit: the double "if (soc_type == QCA_WCN3990)" is a bit odd. Consider > > changing this condition to "if (data && data->soc_type == QCA_WCN3990)" > > and assign qcadev->btsoc_type in the corresponding branch. > > [bala]: will update. > > I have idea of removing flow control from qca_setup() and use them in > qca_set_speed() > "i need to disable hardware flow control when sending change baudrate > request to WCN3990. > enabling it after setting host baudrate. this is only for wcn3990. > " I definitely think it's positive to hide the hci_uart_set_flow_control() call in qca_set_speed(). A few comments inline. > static int qca_set_speed(struct hci_uart *hu, unsigned int speed, > enum qca_speed_type speed_type, > enum qca_btsoc_type soc_type) > { That's a lot of parameters, in particular 'soc_type' seems a bit off-topic in a function called qca_set_speed(). Passing struct qca_serdev instead of struct hci_uart would make the 'soc_type' parameter unnecessary. > unsigned int qca_baudrate; > int ret; > > if (speed_type == QCA_INIT_SPEED) > goto change_host_baudrate; If the order of setting host and chip baudrate can't be changed I think it's preferable to set the init speed right here in the if branch instead of doing the goto. I earlier mentioned the short cut of a return (if possible) to save a level of indentation, but a goto IMO doesn't improve readability and it's the first level of indentation anyway. > if (soc_type == QCA_WCN3990) > hci_uart_set_flow_control(hu, true); > > qca_baudrate = qca_get_baudrate_value(speed); > bt_dev_info(hu->hdev, "Set UART speed to %d", speed); > ret = qca_set_baudrate(hu->hdev, qca_baudrate); > if (ret) { > bt_dev_err(hu->hdev, "Failed to change the baudrate (%d)", > ret); > return ret; > } > > change_host_baudrate: > > host_set_baudrate(hu, speed); > if (soc_type == QCA_WCN3990) > hci_uart_set_flow_control(hu, false); > > return ret; > } > > > is it good idea? In general I think it is. -- 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
Quoting Balakrishna Godavarthi (2018-06-18 10:07:31) > Hi Stephen, > > Please find my comments inline. > > On 2018-06-18 22:12, Stephen Boyd wrote: > > Quoting Balakrishna Godavarthi (2018-06-15 23:27:18) > >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > >> index 28ae6a17a595..1961e313aae7 100644 > >> --- a/drivers/bluetooth/hci_qca.c > >> +++ b/drivers/bluetooth/hci_qca.c > >> @@ -1031,9 +1145,118 @@ static struct hci_uart_proto qca_proto = { > >> .dequeue = qca_dequeue, > >> }; > >> > >> +static const struct qca_vreg_data qca_cherokee_data = { > >> + .soc_type = QCA_WCN3990, > >> + .vregs = (struct qca_vreg []) { > >> + { "vddio", 1352000, 1352000, 0 }, > >> + { "vddxtal", 1904000, 2040000, 0 }, > >> + { "vddcore", 1800000, 1800000, 1 }, > >> + { "vddpa", 1304000, 1304000, 1 }, > >> + { "vddldo", 3000000, 3312000, 1 }, > > > > Load of 0 and 1 seems sort of odd. Are these made up to indicate "some > > load" vs. "no load"? > > > > [Bala]: this value specifies the output load current required to turn on > the wcn3990. > in struct defined vddio\vddxtal are smps, with fixed load. > regs from vddcore/vddpa/vddldo are programmable line regulators, > in which we need to set the basic load. I got that part, but a load of 1 still seems like a nonsensical value. Is it to workaround some issue with the regulator driver? It's also pretty weird to be setting regulator voltages here in the driver. Typically that's configured on the board and then the DT has the right voltage already. The only case when the voltage may need to be set by the driver is if the hardware can dynamically scale the voltage for things like DVFS. It looks like here that the voltage is always fixed, so probably we don't need to have any sort of voltage setting in this driver and can rely on the DTS file to get things right. -- 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-06-22 02:46, Matthias Kaehlcke wrote: > Hi Balakrishna, > > On Thu, Jun 21, 2018 at 07:30:25PM +0530, Balakrishna Godavarthi wrote: >> Hi Matthias, >> >> On 2018-06-20 03:23, Matthias Kaehlcke wrote: >> > On Sat, Jun 16, 2018 at 11:57:18AM +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> >> > > --- >> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> > > index 28ae6a17a595..1961e313aae7 100644 >> > > --- a/drivers/bluetooth/hci_qca.c >> > > +++ b/drivers/bluetooth/hci_qca.c >> > > >> > > static int qca_setup(struct hci_uart *hu) >> > > { >> > > struct hci_dev *hdev = hu->hdev; >> > > struct qca_data *qca = hu->priv; >> > > + struct qca_serdev *qcadev; >> > > unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; >> > > int ret; >> > > int soc_ver = 0; >> > > >> > > - bt_dev_info(hdev, "ROME setup"); >> > > + qcadev = serdev_device_get_drvdata(hu->serdev); >> > > >> > > /* Patch downloading has to be done without IBS mode */ >> > > clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); >> > > - >> > > - /* Setup initial baudrate */ >> > > qca_set_init_speed(hu); >> > > + >> > > + if (qcadev->btsoc_type == QCA_WCN3990) { >> > > + bt_dev_dbg(hdev, "setting up wcn3990"); >> > > + hci_uart_set_flow_control(hu, true); >> > > + ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE); >> > > + if (ret) { >> > > + bt_dev_err(hdev, "failed to send power on command"); >> > > + return ret; >> > > + } >> > > + serdev_device_close(hu->serdev); >> > > + ret = serdev_device_open(hu->serdev); >> > > + if (ret) { >> > > + bt_dev_err(hdev, "failed to open port"); >> > > + return ret; >> > > + } >> > > + msleep(100); >> > > + qca_set_init_speed(hu); >> > > + hci_uart_set_flow_control(hu, false); >> > > + ret = qca_read_soc_version(hdev, &soc_ver); >> > > + if (ret < 0 || soc_ver == 0) { >> > > + bt_dev_err(hdev, "Failed to get version %d", ret); >> > >> > serdev_device_close() ? >> > >> > Also applies to other error paths in this function. >> > >> [Bala]: sorry, i didn't get you. > > A few lines above serdev_device_open() is called, in the error paths > the device should be closed. > [Bala]: i don't think closing of port is required. qca_send_vendor_cmd() fails if skb_alloc() fails. that is not linked with serdev_close(). we need to close the port, if we have issues from reading or writing into the port. pls correct me if i am wrong. >> > > 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); >> > > @@ -1041,47 +1264,85 @@ static int qca_serdev_probe(struct >> > > serdev_device *serdev) >> > > return -ENOMEM; >> > > >> > > qcadev->serdev_hu.serdev = serdev; >> > > + data = of_device_get_match_data(&serdev->dev); >> > > + if (data && data->soc_type == QCA_WCN3990) >> > > + qcadev->btsoc_type = QCA_WCN3990; >> > > + else >> > > + qcadev->btsoc_type = QCA_ROME; >> > > + >> > > serdev_device_set_drvdata(serdev, qcadev); >> > > + if (qcadev->btsoc_type == QCA_WCN3990) { >> > >> > nit: the double "if (soc_type == QCA_WCN3990)" is a bit odd. Consider >> > changing this condition to "if (data && data->soc_type == QCA_WCN3990)" >> > and assign qcadev->btsoc_type in the corresponding branch. >> >> [bala]: will update. >> >> I have idea of removing flow control from qca_setup() and use them in >> qca_set_speed() >> "i need to disable hardware flow control when sending change baudrate >> request to WCN3990. >> enabling it after setting host baudrate. this is only for wcn3990. >> " > > I definitely think it's positive to hide the > hci_uart_set_flow_control() call in qca_set_speed(). A few comments > inline. > > >> static int qca_set_speed(struct hci_uart *hu, unsigned int speed, >> enum qca_speed_type speed_type, >> enum qca_btsoc_type soc_type) >> { > > That's a lot of parameters, in particular 'soc_type' seems a bit > off-topic in a function called qca_set_speed(). Passing struct > qca_serdev instead of struct hci_uart would make the 'soc_type' > parameter unnecessary. > [Bala]: will update. >> unsigned int qca_baudrate; >> int ret; >> >> if (speed_type == QCA_INIT_SPEED) >> goto change_host_baudrate; > > If the order of setting host and chip baudrate can't be changed I > think it's preferable to set the init speed right here in the if > branch instead of doing the goto. I earlier mentioned the short cut of > a return (if possible) to save a level of indentation, but a goto > IMO doesn't improve readability and it's the first level of > indentation anyway. > [Bala]: will update. >> if (soc_type == QCA_WCN3990) >> hci_uart_set_flow_control(hu, true); >> >> qca_baudrate = qca_get_baudrate_value(speed); >> bt_dev_info(hu->hdev, "Set UART speed to %d", speed); >> ret = qca_set_baudrate(hu->hdev, qca_baudrate); >> if (ret) { >> bt_dev_err(hu->hdev, "Failed to change the baudrate >> (%d)", >> ret); >> return ret; >> } >> >> change_host_baudrate: >> >> host_set_baudrate(hu, speed); >> if (soc_type == QCA_WCN3990) >> hci_uart_set_flow_control(hu, false); >> >> return ret; >> } >> >> >> is it good idea? > > In general I think it is.
Hi Stephen, On 2018-06-22 06:58, Stephen Boyd wrote: > Quoting Balakrishna Godavarthi (2018-06-18 10:07:31) >> Hi Stephen, >> >> Please find my comments inline. >> >> On 2018-06-18 22:12, Stephen Boyd wrote: >> > Quoting Balakrishna Godavarthi (2018-06-15 23:27:18) >> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> >> index 28ae6a17a595..1961e313aae7 100644 >> >> --- a/drivers/bluetooth/hci_qca.c >> >> +++ b/drivers/bluetooth/hci_qca.c >> >> @@ -1031,9 +1145,118 @@ static struct hci_uart_proto qca_proto = { >> >> .dequeue = qca_dequeue, >> >> }; >> >> >> >> +static const struct qca_vreg_data qca_cherokee_data = { >> >> + .soc_type = QCA_WCN3990, >> >> + .vregs = (struct qca_vreg []) { >> >> + { "vddio", 1352000, 1352000, 0 }, >> >> + { "vddxtal", 1904000, 2040000, 0 }, >> >> + { "vddcore", 1800000, 1800000, 1 }, >> >> + { "vddpa", 1304000, 1304000, 1 }, >> >> + { "vddldo", 3000000, 3312000, 1 }, >> > >> > Load of 0 and 1 seems sort of odd. Are these made up to indicate "some >> > load" vs. "no load"? >> > >> >> [Bala]: this value specifies the output load current required to turn >> on >> the wcn3990. >> in struct defined vddio\vddxtal are smps, with fixed load. >> regs from vddcore/vddpa/vddldo are programmable line >> regulators, >> in which we need to set the basic load. > > I got that part, but a load of 1 still seems like a nonsensical value. > Is it to workaround some issue with the regulator driver? > [Bala]: i don't think, it is issue with regulator driver. > It's also pretty weird to be setting regulator voltages here in the > driver. Typically that's configured on the board and then the DT has > the > right voltage already. The only case when the voltage may need to be > set > by the driver is if the hardware can dynamically scale the voltage for > things like DVFS. It looks like here that the voltage is always fixed, > so probably we don't need to have any sort of voltage setting in this > driver and can rely on the DTS file to get things right. [Bala]: voltage required by wcn3990 are fixed. now i am using a programmable line regulators. in future, if i have an platform where we have a variable voltage regulators used. i.e. an voltage can be scaled to the required voltage to wcn3990. in that case, we explicitly restrict the regulator output voltage to wcn3990 standards. let us take an example. i am using an programmable regulator, with varying output voltage ranges from 1v to 5v. but from wcn3990 we require an voltage 3.3v. while enabling the regulators.. so that output of regulator will be 3.3v fixed. one more case, every time when we have platform change.. we need to ensure that regulators voltage level should be matching with wcn3990. instead we expect platform vendor to do all these. i want to restrict the voltages in driver itself. please let me know, if cleared your query.
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h index c7a5d1754f47..d49c1d847e1a 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 CHEROKEE_POWERON_PULSE 0xFC +#define CHEROKEE_POWEROFF_PULSE 0xC0 + enum qca_baudrate { QCA_BAUDRATE_115200 = 0, QCA_BAUDRATE_57600, diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index 28ae6a17a595..1961e313aae7 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> @@ -119,12 +124,46 @@ struct qca_data { u64 votes_off; }; +/* + * 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_btsoc_power_setup(struct hci_uart *hu, bool on); +static int qca_btsoc_shutdown(struct hci_uart *hu); + static void __serial_clock_on(struct tty_struct *tty) { /* TODO: Some chipset requires to enable UART clock on client @@ -402,6 +441,7 @@ static int qca_open(struct hci_uart *hu) { struct qca_serdev *qcadev; struct qca_data *qca; + int ret = 0; BT_DBG("hu %p qca_open", hu); @@ -463,13 +503,19 @@ static int qca_open(struct hci_uart *hu) 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) { + hu->init_speed = qcadev->init_speed; + hu->oper_speed = qcadev->oper_speed; + ret = qca_btsoc_power_setup(hu, true); + } else { + gpiod_set_value_cansleep(qcadev->bt_en, 1); + } } BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u", qca->tx_idle_delay, qca->wake_retrans); - return 0; + return ret; } static void qca_debugfs_init(struct hci_dev *hdev) @@ -549,10 +595,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_btsoc_shutdown(hu); + else + gpiod_set_value_cansleep(qcadev->bt_en, 0); + + serdev_device_close(hu->serdev); } kfree_skb(qca->rx_skb); @@ -925,6 +974,32 @@ 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) { + bt_dev_err(hdev, "Failed to allocate memory for skb packet"); + return -ENOMEM; + } + + 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); + + return 0; +} + static void qca_set_init_speed(struct hci_uart *hu) { unsigned int speed = 0; @@ -971,21 +1046,60 @@ static int qca_set_oper_speed(struct hci_uart *hu) return ret; } +static int qca_btsoc_shutdown(struct hci_uart *hu) +{ + struct hci_dev *hdev = hu->hdev; + + host_set_baudrate(hu, 2400); + qca_send_vendor_cmd(hdev, CHEROKEE_POWEROFF_PULSE); + return qca_btsoc_power_setup(hu, false); +} + static int qca_setup(struct hci_uart *hu) { struct hci_dev *hdev = hu->hdev; struct qca_data *qca = hu->priv; + struct qca_serdev *qcadev; unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200; int ret; int soc_ver = 0; - bt_dev_info(hdev, "ROME setup"); + qcadev = serdev_device_get_drvdata(hu->serdev); /* Patch downloading has to be done without IBS mode */ clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags); - - /* Setup initial baudrate */ qca_set_init_speed(hu); + + if (qcadev->btsoc_type == QCA_WCN3990) { + bt_dev_dbg(hdev, "setting up wcn3990"); + hci_uart_set_flow_control(hu, true); + ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE); + if (ret) { + bt_dev_err(hdev, "failed to send power on command"); + return ret; + } + serdev_device_close(hu->serdev); + ret = serdev_device_open(hu->serdev); + if (ret) { + bt_dev_err(hdev, "failed to open port"); + return ret; + } + msleep(100); + qca_set_init_speed(hu); + hci_uart_set_flow_control(hu, false); + 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; + } + + bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver); + hci_uart_set_flow_control(hu, true); + + } else { + bt_dev_info(hdev, "ROME setup"); + } + /* Setup user speed if needed */ ret = qca_set_oper_speed(hu); if (ret) @@ -995,7 +1109,7 @@ static int qca_setup(struct hci_uart *hu) qca_baudrate = qca_get_baudrate_value(speed); /* 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); @@ -1031,9 +1145,118 @@ static struct hci_uart_proto qca_proto = { .dequeue = qca_dequeue, }; +static const struct qca_vreg_data qca_cherokee_data = { + .soc_type = QCA_WCN3990, + .vregs = (struct qca_vreg []) { + { "vddio", 1352000, 1352000, 0 }, + { "vddxtal", 1904000, 2040000, 0 }, + { "vddcore", 1800000, 1800000, 1 }, + { "vddpa", 1304000, 1304000, 1 }, + { "vddldo", 3000000, 3312000, 1 }, + }, + .num_vregs = 5, +}; + +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) + goto out; + + if (vregs.load_uA) + ret = regulator_set_load(regulator, + vregs.load_uA); + + if (ret) + goto out; + + ret = regulator_enable(regulator); + +out: + return ret; + +} + +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_btsoc_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); @@ -1041,47 +1264,85 @@ static int qca_serdev_probe(struct serdev_device *serdev) return -ENOMEM; qcadev->serdev_hu.serdev = serdev; + data = of_device_get_match_data(&serdev->dev); + if (data && data->soc_type == QCA_WCN3990) + qcadev->btsoc_type = QCA_WCN3990; + else + qcadev->btsoc_type = QCA_ROME; + serdev_device_set_drvdata(serdev, qcadev); + if (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->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_btsoc_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_cherokee_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 v6: * 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 | 327 ++++++++++++++++++++++++++++++++---- 2 files changed, 297 insertions(+), 33 deletions(-)