| Message ID | 20230209020916.6475-3-steev@kali.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add WCN6855 Bluetooth support | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
On Wed, Feb 08, 2023 at 08:09:14PM -0600, Steev Klimaszewski wrote: > Added regulators,GPIOs and changes required to power on/off wcn6855. > Added support for firmware download for wcn6855. > > Signed-off-by: Steev Klimaszewski <steev@kali.org> Reviewed-by: Bjorn Andersson <andersson@kernel.org> Tested-by: Bjorn Andersson <andersson@kernel.org> Regards, Bjorn > --- > Changes since v4: > * Remove unused firmware check because we don't have mbn firmware. > * Set qcadev->init_speed if it hasn't been set. > > Changes since v3: > * drop unused regulators > > Changes since v2: > * drop unnecessary commit info > > Changes since v1: > * None > > drivers/bluetooth/btqca.c | 9 ++++++- > drivers/bluetooth/btqca.h | 10 ++++++++ > drivers/bluetooth/hci_qca.c | 50 ++++++++++++++++++++++++++++--------- > 3 files changed, 56 insertions(+), 13 deletions(-) > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c > index c9064d34d830..2f9d8bd27c38 100644 > --- a/drivers/bluetooth/btqca.c > +++ b/drivers/bluetooth/btqca.c > @@ -614,6 +614,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > config.type = ELF_TYPE_PATCH; > snprintf(config.fwname, sizeof(config.fwname), > "qca/msbtfw%02x.mbn", rom_ver); > + } else if (soc_type == QCA_WCN6855) { > + snprintf(config.fwname, sizeof(config.fwname), > + "qca/hpbtfw%02x.tlv", rom_ver); > } else { > snprintf(config.fwname, sizeof(config.fwname), > "qca/rampatch_%08x.bin", soc_ver); > @@ -648,6 +651,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > else if (soc_type == QCA_WCN6750) > snprintf(config.fwname, sizeof(config.fwname), > "qca/msnv%02x.bin", rom_ver); > + else if (soc_type == QCA_WCN6855) > + snprintf(config.fwname, sizeof(config.fwname), > + "qca/hpnv%02x.bin", rom_ver); > else > snprintf(config.fwname, sizeof(config.fwname), > "qca/nvm_%08x.bin", soc_ver); > @@ -672,6 +678,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > case QCA_WCN3991: > case QCA_WCN3998: > case QCA_WCN6750: > + case QCA_WCN6855: > hci_set_msft_opcode(hdev, 0xFD70); > break; > default: > @@ -685,7 +692,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > return err; > } > > - if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750) { > + if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750 || soc_type == QCA_WCN6855) { > /* get fw build info */ > err = qca_read_fw_build_info(hdev); > if (err < 0) > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h > index 61e9a50e66ae..b884095bcd9d 100644 > --- a/drivers/bluetooth/btqca.h > +++ b/drivers/bluetooth/btqca.h > @@ -147,6 +147,7 @@ enum qca_btsoc_type { > QCA_WCN3991, > QCA_QCA6390, > QCA_WCN6750, > + QCA_WCN6855, > }; > > #if IS_ENABLED(CONFIG_BT_QCA) > @@ -168,6 +169,10 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type) > { > return soc_type == QCA_WCN6750; > } > +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type) > +{ > + return soc_type == QCA_WCN6855; > +} > > #else > > @@ -206,6 +211,11 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type) > return false; > } > > +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type) > +{ > + return false; > +} > + > static inline int qca_send_pre_shutdown_cmd(struct hci_dev *hdev) > { > return -EOPNOTSUPP; > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index 3df8c3606e93..efc1c0306b4e 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -605,8 +605,7 @@ static int qca_open(struct hci_uart *hu) > if (hu->serdev) { > qcadev = serdev_device_get_drvdata(hu->serdev); > > - if (qca_is_wcn399x(qcadev->btsoc_type) || > - qca_is_wcn6750(qcadev->btsoc_type)) > + if (!(qcadev->init_speed)) > hu->init_speed = qcadev->init_speed; > > if (qcadev->oper_speed) > @@ -1317,7 +1316,8 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate) > > /* Give the controller time to process the request */ > if (qca_is_wcn399x(qca_soc_type(hu)) || > - qca_is_wcn6750(qca_soc_type(hu))) > + qca_is_wcn6750(qca_soc_type(hu)) || > + qca_is_wcn6855(qca_soc_type(hu))) > usleep_range(1000, 10000); > else > msleep(300); > @@ -1394,7 +1394,8 @@ static unsigned int qca_get_speed(struct hci_uart *hu, > static int qca_check_speeds(struct hci_uart *hu) > { > if (qca_is_wcn399x(qca_soc_type(hu)) || > - qca_is_wcn6750(qca_soc_type(hu))) { > + qca_is_wcn6750(qca_soc_type(hu)) || > + qca_is_wcn6855(qca_soc_type(hu))) { > if (!qca_get_speed(hu, QCA_INIT_SPEED) && > !qca_get_speed(hu, QCA_OPER_SPEED)) > return -EINVAL; > @@ -1682,7 +1683,8 @@ static int qca_power_on(struct hci_dev *hdev) > return 0; > > if (qca_is_wcn399x(soc_type) || > - qca_is_wcn6750(soc_type)) { > + qca_is_wcn6750(soc_type) || > + qca_is_wcn6855(soc_type)) { > ret = qca_regulator_init(hu); > } else { > qcadev = serdev_device_get_drvdata(hu->serdev); > @@ -1723,7 +1725,8 @@ static int qca_setup(struct hci_uart *hu) > > bt_dev_info(hdev, "setting up %s", > qca_is_wcn399x(soc_type) ? "wcn399x" : > - (soc_type == QCA_WCN6750) ? "wcn6750" : "ROME/QCA6390"); > + (soc_type == QCA_WCN6750) ? "wcn6750" : > + (soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390"); > > qca->memdump_state = QCA_MEMDUMP_IDLE; > > @@ -1735,7 +1738,8 @@ static int qca_setup(struct hci_uart *hu) > clear_bit(QCA_SSR_TRIGGERED, &qca->flags); > > if (qca_is_wcn399x(soc_type) || > - qca_is_wcn6750(soc_type)) { > + qca_is_wcn6750(soc_type) || > + qca_is_wcn6855(soc_type)) { > set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks); > hci_set_aosp_capable(hdev); > > @@ -1757,7 +1761,8 @@ static int qca_setup(struct hci_uart *hu) > } > > if (!(qca_is_wcn399x(soc_type) || > - qca_is_wcn6750(soc_type))) { > + qca_is_wcn6750(soc_type) || > + qca_is_wcn6855(soc_type))) { > /* Get QCA version information */ > ret = qca_read_soc_version(hdev, &ver, soc_type); > if (ret) > @@ -1883,6 +1888,20 @@ static const struct qca_device_data qca_soc_data_wcn6750 = { > .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, > }; > > +static const struct qca_device_data qca_soc_data_wcn6855 = { > + .soc_type = QCA_WCN6855, > + .vregs = (struct qca_vreg []) { > + { "vddio", 5000 }, > + { "vddbtcxmx", 126000 }, > + { "vddrfacmn", 12500 }, > + { "vddrfa0p8", 102000 }, > + { "vddrfa1p7", 302000 }, > + { "vddrfa1p2", 257000 }, > + }, > + .num_vregs = 6, > + .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, > +}; > + > static void qca_power_shutdown(struct hci_uart *hu) > { > struct qca_serdev *qcadev; > @@ -2047,7 +2066,8 @@ static int qca_serdev_probe(struct serdev_device *serdev) > > if (data && > (qca_is_wcn399x(data->soc_type) || > - qca_is_wcn6750(data->soc_type))) { > + qca_is_wcn6750(data->soc_type) || > + qca_is_wcn6855(data->soc_type))) { > qcadev->btsoc_type = data->soc_type; > qcadev->bt_power = devm_kzalloc(&serdev->dev, > sizeof(struct qca_power), > @@ -2067,14 +2087,18 @@ static int qca_serdev_probe(struct serdev_device *serdev) > > qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable", > GPIOD_OUT_LOW); > - if (IS_ERR_OR_NULL(qcadev->bt_en) && data->soc_type == QCA_WCN6750) { > + if (IS_ERR_OR_NULL(qcadev->bt_en) > + && (data->soc_type == QCA_WCN6750 || > + data->soc_type == QCA_WCN6855)) { > dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n"); > power_ctrl_enabled = false; > } > > qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl", > GPIOD_IN); > - if (IS_ERR_OR_NULL(qcadev->sw_ctrl) && data->soc_type == QCA_WCN6750) > + if (IS_ERR_OR_NULL(qcadev->sw_ctrl) > + && (data->soc_type == QCA_WCN6750 || > + data->soc_type == QCA_WCN6855)) > dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n"); > > qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL); > @@ -2150,7 +2174,8 @@ static void qca_serdev_remove(struct serdev_device *serdev) > struct qca_power *power = qcadev->bt_power; > > if ((qca_is_wcn399x(qcadev->btsoc_type) || > - qca_is_wcn6750(qcadev->btsoc_type)) && > + qca_is_wcn6750(qcadev->btsoc_type) || > + qca_is_wcn6855(qcadev->btsoc_type)) && > power->vregs_on) > qca_power_shutdown(&qcadev->serdev_hu); > else if (qcadev->susclk) > @@ -2335,6 +2360,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = { > { .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991}, > { .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998}, > { .compatible = "qcom,wcn6750-bt", .data = &qca_soc_data_wcn6750}, > + { .compatible = "qcom,wcn6855-bt", .data = &qca_soc_data_wcn6855}, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match); > -- > 2.39.1 >
On Wed, Feb 08, 2023 at 08:09:14PM -0600, Steev Klimaszewski wrote: > Added regulators,GPIOs and changes required to power on/off wcn6855. > Added support for firmware download for wcn6855. > > Signed-off-by: Steev Klimaszewski <steev@kali.org> > --- > drivers/bluetooth/btqca.c | 9 ++++++- > drivers/bluetooth/btqca.h | 10 ++++++++ > drivers/bluetooth/hci_qca.c | 50 ++++++++++++++++++++++++++++--------- > 3 files changed, 56 insertions(+), 13 deletions(-) > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c > index c9064d34d830..2f9d8bd27c38 100644 > --- a/drivers/bluetooth/btqca.c > +++ b/drivers/bluetooth/btqca.c > +static const struct qca_device_data qca_soc_data_wcn6855 = { > + .soc_type = QCA_WCN6855, > + .vregs = (struct qca_vreg []) { > + { "vddio", 5000 }, > + { "vddbtcxmx", 126000 }, > + { "vddrfacmn", 12500 }, > + { "vddrfa0p8", 102000 }, > + { "vddrfa1p7", 302000 }, > + { "vddrfa1p2", 257000 }, > + }, > + .num_vregs = 6, > + .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, > +}; > + > static void qca_power_shutdown(struct hci_uart *hu) > { > struct qca_serdev *qcadev; As I just mentioned IRC, you forgot to update qca_power_shutdown() here so the regulators are currently never disabled (e.g. when closing the device or on module unload). > @@ -2047,7 +2066,8 @@ static int qca_serdev_probe(struct serdev_device *serdev) > > if (data && > (qca_is_wcn399x(data->soc_type) || > - qca_is_wcn6750(data->soc_type))) { > + qca_is_wcn6750(data->soc_type) || > + qca_is_wcn6855(data->soc_type))) { > qcadev->btsoc_type = data->soc_type; > qcadev->bt_power = devm_kzalloc(&serdev->dev, > sizeof(struct qca_power), > @@ -2150,7 +2174,8 @@ static void qca_serdev_remove(struct serdev_device *serdev) > struct qca_power *power = qcadev->bt_power; > > if ((qca_is_wcn399x(qcadev->btsoc_type) || > - qca_is_wcn6750(qcadev->btsoc_type)) && > + qca_is_wcn6750(qcadev->btsoc_type) || > + qca_is_wcn6855(qcadev->btsoc_type)) && > power->vregs_on) > qca_power_shutdown(&qcadev->serdev_hu); > else if (qcadev->susclk) Johan
On Wed, Feb 08, 2023 at 08:09:14PM -0600, Steev Klimaszewski wrote: > Added regulators,GPIOs and changes required to power on/off wcn6855. > Added support for firmware download for wcn6855. > > Signed-off-by: Steev Klimaszewski <steev@kali.org> > --- > Changes since v4: > * Remove unused firmware check because we don't have mbn firmware. > * Set qcadev->init_speed if it hasn't been set. > > Changes since v3: > * drop unused regulators > > Changes since v2: > * drop unnecessary commit info > > Changes since v1: > * None > > drivers/bluetooth/btqca.c | 9 ++++++- > drivers/bluetooth/btqca.h | 10 ++++++++ > drivers/bluetooth/hci_qca.c | 50 ++++++++++++++++++++++++++++--------- > 3 files changed, 56 insertions(+), 13 deletions(-) > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c > index c9064d34d830..2f9d8bd27c38 100644 > --- a/drivers/bluetooth/btqca.c > +++ b/drivers/bluetooth/btqca.c > @@ -614,6 +614,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > config.type = ELF_TYPE_PATCH; > snprintf(config.fwname, sizeof(config.fwname), > "qca/msbtfw%02x.mbn", rom_ver); > + } else if (soc_type == QCA_WCN6855) { > + snprintf(config.fwname, sizeof(config.fwname), > + "qca/hpbtfw%02x.tlv", rom_ver); > } else { > snprintf(config.fwname, sizeof(config.fwname), > "qca/rampatch_%08x.bin", soc_ver); > @@ -648,6 +651,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > else if (soc_type == QCA_WCN6750) > snprintf(config.fwname, sizeof(config.fwname), > "qca/msnv%02x.bin", rom_ver); > + else if (soc_type == QCA_WCN6855) > + snprintf(config.fwname, sizeof(config.fwname), > + "qca/hpnv%02x.bin", rom_ver); > else > snprintf(config.fwname, sizeof(config.fwname), > "qca/nvm_%08x.bin", soc_ver); > @@ -672,6 +678,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > case QCA_WCN3991: > case QCA_WCN3998: > case QCA_WCN6750: > + case QCA_WCN6855: Did you actually verify the microsoft extensions need this, or you are assuming it works as 6750? > hci_set_msft_opcode(hdev, 0xFD70); > break; > default: > @@ -685,7 +692,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > return err; > } > > - if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750) { > + if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750 || soc_type == QCA_WCN6855) { Line is now over 80 columns which is still the preferred limit. Perhaps this should now be a switch statement instead? > /* get fw build info */ > err = qca_read_fw_build_info(hdev); > if (err < 0) > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h > index 61e9a50e66ae..b884095bcd9d 100644 > --- a/drivers/bluetooth/btqca.h > +++ b/drivers/bluetooth/btqca.h > @@ -147,6 +147,7 @@ enum qca_btsoc_type { > QCA_WCN3991, > QCA_QCA6390, > QCA_WCN6750, > + QCA_WCN6855, > }; > > #if IS_ENABLED(CONFIG_BT_QCA) > @@ -168,6 +169,10 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type) > { > return soc_type == QCA_WCN6750; > } > +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type) > +{ > + return soc_type == QCA_WCN6855; > +} > > #else > > @@ -206,6 +211,11 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type) > return false; > } > > +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type) > +{ > + return false; > +} > + > static inline int qca_send_pre_shutdown_cmd(struct hci_dev *hdev) > { > return -EOPNOTSUPP; > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index 3df8c3606e93..efc1c0306b4e 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -605,8 +605,7 @@ static int qca_open(struct hci_uart *hu) > if (hu->serdev) { > qcadev = serdev_device_get_drvdata(hu->serdev); > > - if (qca_is_wcn399x(qcadev->btsoc_type) || > - qca_is_wcn6750(qcadev->btsoc_type)) > + if (!(qcadev->init_speed)) > hu->init_speed = qcadev->init_speed; This change makes no sense. In fact, it seems the driver never sets init_speed anywhere. Either way, it should not be needed for wcn6855. > > if (qcadev->oper_speed) > @@ -1317,7 +1316,8 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate) > > /* Give the controller time to process the request */ > if (qca_is_wcn399x(qca_soc_type(hu)) || > - qca_is_wcn6750(qca_soc_type(hu))) > + qca_is_wcn6750(qca_soc_type(hu)) || > + qca_is_wcn6855(qca_soc_type(hu))) > usleep_range(1000, 10000); > else > msleep(300); > @@ -1394,7 +1394,8 @@ static unsigned int qca_get_speed(struct hci_uart *hu, > static int qca_check_speeds(struct hci_uart *hu) > { > if (qca_is_wcn399x(qca_soc_type(hu)) || > - qca_is_wcn6750(qca_soc_type(hu))) { > + qca_is_wcn6750(qca_soc_type(hu)) || > + qca_is_wcn6855(qca_soc_type(hu))) { > if (!qca_get_speed(hu, QCA_INIT_SPEED) && > !qca_get_speed(hu, QCA_OPER_SPEED)) > return -EINVAL; > @@ -1682,7 +1683,8 @@ static int qca_power_on(struct hci_dev *hdev) > return 0; > > if (qca_is_wcn399x(soc_type) || > - qca_is_wcn6750(soc_type)) { > + qca_is_wcn6750(soc_type) || > + qca_is_wcn6855(soc_type)) { > ret = qca_regulator_init(hu); > } else { > qcadev = serdev_device_get_drvdata(hu->serdev); > @@ -1723,7 +1725,8 @@ static int qca_setup(struct hci_uart *hu) > > bt_dev_info(hdev, "setting up %s", > qca_is_wcn399x(soc_type) ? "wcn399x" : > - (soc_type == QCA_WCN6750) ? "wcn6750" : "ROME/QCA6390"); > + (soc_type == QCA_WCN6750) ? "wcn6750" : > + (soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390"); This is hideous, but not your fault... > > qca->memdump_state = QCA_MEMDUMP_IDLE; > > @@ -1735,7 +1738,8 @@ static int qca_setup(struct hci_uart *hu) > clear_bit(QCA_SSR_TRIGGERED, &qca->flags); > > if (qca_is_wcn399x(soc_type) || > - qca_is_wcn6750(soc_type)) { > + qca_is_wcn6750(soc_type) || > + qca_is_wcn6855(soc_type)) { > set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks); > hci_set_aosp_capable(hdev); > > @@ -1757,7 +1761,8 @@ static int qca_setup(struct hci_uart *hu) > } > > if (!(qca_is_wcn399x(soc_type) || > - qca_is_wcn6750(soc_type))) { > + qca_is_wcn6750(soc_type) || > + qca_is_wcn6855(soc_type))) { Perhaps you can add a leading space while changing this so that the open-parenthesis alignment makes sense. > /* Get QCA version information */ > ret = qca_read_soc_version(hdev, &ver, soc_type); > if (ret) > @@ -1883,6 +1888,20 @@ static const struct qca_device_data qca_soc_data_wcn6750 = { > .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, > }; > > +static const struct qca_device_data qca_soc_data_wcn6855 = { > + .soc_type = QCA_WCN6855, > + .vregs = (struct qca_vreg []) { > + { "vddio", 5000 }, > + { "vddbtcxmx", 126000 }, > + { "vddrfacmn", 12500 }, > + { "vddrfa0p8", 102000 }, > + { "vddrfa1p7", 302000 }, > + { "vddrfa1p2", 257000 }, Hmm. More random regulator load values. I really think we should get rid of this but that's a separate discussion. > + }, > + .num_vregs = 6, > + .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, > +}; > + > static void qca_power_shutdown(struct hci_uart *hu) > { > struct qca_serdev *qcadev; As I mentioned elsewhere, you need to update also this function so that wcn6855 can be powered down. > @@ -2047,7 +2066,8 @@ static int qca_serdev_probe(struct serdev_device *serdev) > > if (data && > (qca_is_wcn399x(data->soc_type) || > - qca_is_wcn6750(data->soc_type))) { > + qca_is_wcn6750(data->soc_type) || > + qca_is_wcn6855(data->soc_type))) { Perhaps you fix the alignment here too. > qcadev->btsoc_type = data->soc_type; > qcadev->bt_power = devm_kzalloc(&serdev->dev, > sizeof(struct qca_power), > @@ -2067,14 +2087,18 @@ static int qca_serdev_probe(struct serdev_device *serdev) > > qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable", > GPIOD_OUT_LOW); > - if (IS_ERR_OR_NULL(qcadev->bt_en) && data->soc_type == QCA_WCN6750) { > + if (IS_ERR_OR_NULL(qcadev->bt_en) > + && (data->soc_type == QCA_WCN6750 || && operator should go on the previous line before the line break. > + data->soc_type == QCA_WCN6855)) { > dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n"); > power_ctrl_enabled = false; > } > > qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl", > GPIOD_IN); > - if (IS_ERR_OR_NULL(qcadev->sw_ctrl) && data->soc_type == QCA_WCN6750) > + if (IS_ERR_OR_NULL(qcadev->sw_ctrl) > + && (data->soc_type == QCA_WCN6750 || Same here. > + data->soc_type == QCA_WCN6855)) > dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n"); > > qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL); > @@ -2150,7 +2174,8 @@ static void qca_serdev_remove(struct serdev_device *serdev) > struct qca_power *power = qcadev->bt_power; > > if ((qca_is_wcn399x(qcadev->btsoc_type) || > - qca_is_wcn6750(qcadev->btsoc_type)) && > + qca_is_wcn6750(qcadev->btsoc_type) || > + qca_is_wcn6855(qcadev->btsoc_type)) && > power->vregs_on) > qca_power_shutdown(&qcadev->serdev_hu); > else if (qcadev->susclk) > @@ -2335,6 +2360,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = { > { .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991}, > { .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998}, > { .compatible = "qcom,wcn6750-bt", .data = &qca_soc_data_wcn6750}, > + { .compatible = "qcom,wcn6855-bt", .data = &qca_soc_data_wcn6855}, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match); With power-off handling fixed, this seems to work as quite well on my X13s with 6.3-rc1. Nice job! Btw, apart from the frame reassembly error, I'm also seeing: Bluetooth: Received HCI_IBS_WAKE_ACK in tx state 0 during probe. Johan
On Thu, Mar 9, 2023 at 11:08 AM Johan Hovold <johan@kernel.org> wrote: > > On Wed, Feb 08, 2023 at 08:09:14PM -0600, Steev Klimaszewski wrote: > > Added regulators,GPIOs and changes required to power on/off wcn6855. > > Added support for firmware download for wcn6855. > > > > Signed-off-by: Steev Klimaszewski <steev@kali.org> > > --- > > Changes since v4: > > * Remove unused firmware check because we don't have mbn firmware. > > * Set qcadev->init_speed if it hasn't been set. > > > > Changes since v3: > > * drop unused regulators > > > > Changes since v2: > > * drop unnecessary commit info > > > > Changes since v1: > > * None > > > > drivers/bluetooth/btqca.c | 9 ++++++- > > drivers/bluetooth/btqca.h | 10 ++++++++ > > drivers/bluetooth/hci_qca.c | 50 ++++++++++++++++++++++++++++--------- > > 3 files changed, 56 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c > > index c9064d34d830..2f9d8bd27c38 100644 > > --- a/drivers/bluetooth/btqca.c > > +++ b/drivers/bluetooth/btqca.c > > @@ -614,6 +614,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > > config.type = ELF_TYPE_PATCH; > > snprintf(config.fwname, sizeof(config.fwname), > > "qca/msbtfw%02x.mbn", rom_ver); > > + } else if (soc_type == QCA_WCN6855) { > > + snprintf(config.fwname, sizeof(config.fwname), > > + "qca/hpbtfw%02x.tlv", rom_ver); > > } else { > > snprintf(config.fwname, sizeof(config.fwname), > > "qca/rampatch_%08x.bin", soc_ver); > > @@ -648,6 +651,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > > else if (soc_type == QCA_WCN6750) > > snprintf(config.fwname, sizeof(config.fwname), > > "qca/msnv%02x.bin", rom_ver); > > + else if (soc_type == QCA_WCN6855) > > + snprintf(config.fwname, sizeof(config.fwname), > > + "qca/hpnv%02x.bin", rom_ver); > > else > > snprintf(config.fwname, sizeof(config.fwname), > > "qca/nvm_%08x.bin", soc_ver); > > @@ -672,6 +678,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > > case QCA_WCN3991: > > case QCA_WCN3998: > > case QCA_WCN6750: > > + case QCA_WCN6855: > > Did you actually verify the microsoft extensions need this, or you are > assuming it works as 6750? > It was 100% an assumption that since the 6750 does it, the 6855 does too. I should know better than to assume since I used to work at a device manufacturer but high hopes things have changed a bit in the past 12 years ;) > > hci_set_msft_opcode(hdev, 0xFD70); > > break; > > default: > > @@ -685,7 +692,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > > return err; > > } > > > > - if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750) { > > + if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750 || soc_type == QCA_WCN6855) { > > Line is now over 80 columns which is still the preferred limit. > > Perhaps this should now be a switch statement instead? > switch statement might work, I'll give it a shot here. > > /* get fw build info */ > > err = qca_read_fw_build_info(hdev); > > if (err < 0) > > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h > > index 61e9a50e66ae..b884095bcd9d 100644 > > --- a/drivers/bluetooth/btqca.h > > +++ b/drivers/bluetooth/btqca.h > > @@ -147,6 +147,7 @@ enum qca_btsoc_type { > > QCA_WCN3991, > > QCA_QCA6390, > > QCA_WCN6750, > > + QCA_WCN6855, > > }; > > > > #if IS_ENABLED(CONFIG_BT_QCA) > > @@ -168,6 +169,10 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type) > > { > > return soc_type == QCA_WCN6750; > > } > > +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type) > > +{ > > + return soc_type == QCA_WCN6855; > > +} > > > > #else > > > > @@ -206,6 +211,11 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type) > > return false; > > } > > > > +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type) > > +{ > > + return false; > > +} > > + > > static inline int qca_send_pre_shutdown_cmd(struct hci_dev *hdev) > > { > > return -EOPNOTSUPP; > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > > index 3df8c3606e93..efc1c0306b4e 100644 > > --- a/drivers/bluetooth/hci_qca.c > > +++ b/drivers/bluetooth/hci_qca.c > > @@ -605,8 +605,7 @@ static int qca_open(struct hci_uart *hu) > > if (hu->serdev) { > > qcadev = serdev_device_get_drvdata(hu->serdev); > > > > - if (qca_is_wcn399x(qcadev->btsoc_type) || > > - qca_is_wcn6750(qcadev->btsoc_type)) > > + if (!(qcadev->init_speed)) > > hu->init_speed = qcadev->init_speed; > > This change makes no sense. > > In fact, it seems the driver never sets init_speed anywhere. > > Either way, it should not be needed for wcn6855. > So, that was a request from an earlier review, but if it's not needed for 6855, I'll just drop it, and then I don't need to do any of those changes :D > > > > if (qcadev->oper_speed) > > @@ -1317,7 +1316,8 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate) > > > > /* Give the controller time to process the request */ > > if (qca_is_wcn399x(qca_soc_type(hu)) || > > - qca_is_wcn6750(qca_soc_type(hu))) > > + qca_is_wcn6750(qca_soc_type(hu)) || > > + qca_is_wcn6855(qca_soc_type(hu))) > > usleep_range(1000, 10000); > > else > > msleep(300); > > @@ -1394,7 +1394,8 @@ static unsigned int qca_get_speed(struct hci_uart *hu, > > static int qca_check_speeds(struct hci_uart *hu) > > { > > if (qca_is_wcn399x(qca_soc_type(hu)) || > > - qca_is_wcn6750(qca_soc_type(hu))) { > > + qca_is_wcn6750(qca_soc_type(hu)) || > > + qca_is_wcn6855(qca_soc_type(hu))) { > > if (!qca_get_speed(hu, QCA_INIT_SPEED) && > > !qca_get_speed(hu, QCA_OPER_SPEED)) > > return -EINVAL; > > @@ -1682,7 +1683,8 @@ static int qca_power_on(struct hci_dev *hdev) > > return 0; > > > > if (qca_is_wcn399x(soc_type) || > > - qca_is_wcn6750(soc_type)) { > > + qca_is_wcn6750(soc_type) || > > + qca_is_wcn6855(soc_type)) { > > ret = qca_regulator_init(hu); > > } else { > > qcadev = serdev_device_get_drvdata(hu->serdev); > > @@ -1723,7 +1725,8 @@ static int qca_setup(struct hci_uart *hu) > > > > bt_dev_info(hdev, "setting up %s", > > qca_is_wcn399x(soc_type) ? "wcn399x" : > > - (soc_type == QCA_WCN6750) ? "wcn6750" : "ROME/QCA6390"); > > + (soc_type == QCA_WCN6750) ? "wcn6750" : > > + (soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390"); > > This is hideous, but not your fault... > It is, and, I'm not entirely sure we need it? I mean, it's nice to show that it's now starting to set up, but it isn't particularly helpful for end users or making sure things are working? > > > > qca->memdump_state = QCA_MEMDUMP_IDLE; > > > > @@ -1735,7 +1738,8 @@ static int qca_setup(struct hci_uart *hu) > > clear_bit(QCA_SSR_TRIGGERED, &qca->flags); > > > > if (qca_is_wcn399x(soc_type) || > > - qca_is_wcn6750(soc_type)) { > > + qca_is_wcn6750(soc_type) || > > + qca_is_wcn6855(soc_type)) { > > set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks); > > hci_set_aosp_capable(hdev); > > > > @@ -1757,7 +1761,8 @@ static int qca_setup(struct hci_uart *hu) > > } > > > > if (!(qca_is_wcn399x(soc_type) || > > - qca_is_wcn6750(soc_type))) { > > + qca_is_wcn6750(soc_type) || > > + qca_is_wcn6855(soc_type))) { > > Perhaps you can add a leading space while changing this so that the > open-parenthesis alignment makes sense. > > > /* Get QCA version information */ > > ret = qca_read_soc_version(hdev, &ver, soc_type); > > if (ret) > > @@ -1883,6 +1888,20 @@ static const struct qca_device_data qca_soc_data_wcn6750 = { > > .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, > > }; > > > > +static const struct qca_device_data qca_soc_data_wcn6855 = { > > + .soc_type = QCA_WCN6855, > > + .vregs = (struct qca_vreg []) { > > + { "vddio", 5000 }, > > + { "vddbtcxmx", 126000 }, > > + { "vddrfacmn", 12500 }, > > + { "vddrfa0p8", 102000 }, > > + { "vddrfa1p7", 302000 }, > > + { "vddrfa1p2", 257000 }, > > Hmm. More random regulator load values. I really think we should get rid > of this but that's a separate discussion. > Bjorn specifically requested that he wanted me to leave them in. I'm not married to them, and don't care one way or the other, I just wanted working bluetooth since audio wasn't quite ready yet :) > > + }, > > + .num_vregs = 6, > > + .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, > > +}; > > + > > static void qca_power_shutdown(struct hci_uart *hu) > > { > > struct qca_serdev *qcadev; > > As I mentioned elsewhere, you need to update also this function so that > wcn6855 can be powered down. Sorry, I do have that locally, I just haven't pushed a v6 as I was looking at Tim's v2 of the qca2066 and was wondering if I should or shouldn't continue working on my version of the driver? > > > @@ -2047,7 +2066,8 @@ static int qca_serdev_probe(struct serdev_device *serdev) > > > > if (data && > > (qca_is_wcn399x(data->soc_type) || > > - qca_is_wcn6750(data->soc_type))) { > > + qca_is_wcn6750(data->soc_type) || > > + qca_is_wcn6855(data->soc_type))) { > > Perhaps you fix the alignment here too. > > > qcadev->btsoc_type = data->soc_type; > > qcadev->bt_power = devm_kzalloc(&serdev->dev, > > sizeof(struct qca_power), > > @@ -2067,14 +2087,18 @@ static int qca_serdev_probe(struct serdev_device *serdev) > > > > qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable", > > GPIOD_OUT_LOW); > > - if (IS_ERR_OR_NULL(qcadev->bt_en) && data->soc_type == QCA_WCN6750) { > > + if (IS_ERR_OR_NULL(qcadev->bt_en) > > + && (data->soc_type == QCA_WCN6750 || > > && operator should go on the previous line before the line break. > > > + data->soc_type == QCA_WCN6855)) { > > dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n"); > > power_ctrl_enabled = false; > > } > > > > qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl", > > GPIOD_IN); > > - if (IS_ERR_OR_NULL(qcadev->sw_ctrl) && data->soc_type == QCA_WCN6750) > > + if (IS_ERR_OR_NULL(qcadev->sw_ctrl) > > + && (data->soc_type == QCA_WCN6750 || > > Same here. > > > + data->soc_type == QCA_WCN6855)) > > dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n"); > > > > qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL); > > @@ -2150,7 +2174,8 @@ static void qca_serdev_remove(struct serdev_device *serdev) > > struct qca_power *power = qcadev->bt_power; > > > > if ((qca_is_wcn399x(qcadev->btsoc_type) || > > - qca_is_wcn6750(qcadev->btsoc_type)) && > > + qca_is_wcn6750(qcadev->btsoc_type) || > > + qca_is_wcn6855(qcadev->btsoc_type)) && > > power->vregs_on) > > qca_power_shutdown(&qcadev->serdev_hu); > > else if (qcadev->susclk) > > @@ -2335,6 +2360,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = { > > { .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991}, > > { .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998}, > > { .compatible = "qcom,wcn6750-bt", .data = &qca_soc_data_wcn6750}, > > + { .compatible = "qcom,wcn6855-bt", .data = &qca_soc_data_wcn6855}, > > { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match); > > With power-off handling fixed, this seems to work as quite well on my > X13s with 6.3-rc1. Nice job! > > Btw, apart from the frame reassembly error, I'm also seeing: > > Bluetooth: Received HCI_IBS_WAKE_ACK in tx state 0 > > during probe. > I'm still not sure where the frame reassembly error comes from, and I don't know how to get more info to figure it out either, if anyone happens to have any guidance for that, I would love some. Additionally, it doesn't always happen. It seems to happen on the first load of the module, however, running modprobe -r && modprobe in a loop (with the powerdown properly modified so the log isn't full of splats), it doesn't seem to occur every time. Likewise for the WAKE_ACK. > Johan
On Thu, Mar 09, 2023 at 02:24:57PM -0600, Steev Klimaszewski wrote: > On Thu, Mar 9, 2023 at 11:08 AM Johan Hovold <johan@kernel.org> wrote: > > On Wed, Feb 08, 2023 at 08:09:14PM -0600, Steev Klimaszewski wrote: > > > @@ -672,6 +678,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > > > case QCA_WCN3991: > > > case QCA_WCN3998: > > > case QCA_WCN6750: > > > + case QCA_WCN6855: > > > > Did you actually verify the microsoft extensions need this, or you are > > assuming it works as 6750? > > > It was 100% an assumption that since the 6750 does it, the 6855 does > too. I should know better than to assume since I used to work at a > device manufacturer but high hopes things have changed a bit in the > past 12 years ;) Heh. Thanks for confirming. :) > > > hci_set_msft_opcode(hdev, 0xFD70); > > > break; > > > default: > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > > > index 3df8c3606e93..efc1c0306b4e 100644 > > > --- a/drivers/bluetooth/hci_qca.c > > > +++ b/drivers/bluetooth/hci_qca.c > > > @@ -605,8 +605,7 @@ static int qca_open(struct hci_uart *hu) > > > if (hu->serdev) { > > > qcadev = serdev_device_get_drvdata(hu->serdev); > > > > > > - if (qca_is_wcn399x(qcadev->btsoc_type) || > > > - qca_is_wcn6750(qcadev->btsoc_type)) > > > + if (!(qcadev->init_speed)) > > > hu->init_speed = qcadev->init_speed; > > > > This change makes no sense. > > > > In fact, it seems the driver never sets init_speed anywhere. > > > > Either way, it should not be needed for wcn6855. > > So, that was a request from an earlier review, but if it's not needed > for 6855, I'll just drop it, and then I don't need to do any of those > changes :D Note that with the above, init_speed is only is set if qcadev->init_speed is *not* set, so if this was needed the test would need to be negated. But as I mentioned above, this also looks broken as qcadev->init_speed is never set anywhere. You could investigate and clean up this code before or after adding support for wcn6855, but the above really doesn't look right and would at least need to go in a separate patch with a proper explanation. > > > > > > if (qcadev->oper_speed) > > > @@ -1723,7 +1725,8 @@ static int qca_setup(struct hci_uart *hu) > > > > > > bt_dev_info(hdev, "setting up %s", > > > qca_is_wcn399x(soc_type) ? "wcn399x" : > > > - (soc_type == QCA_WCN6750) ? "wcn6750" : "ROME/QCA6390"); > > > + (soc_type == QCA_WCN6750) ? "wcn6750" : > > > + (soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390"); > > > > This is hideous, but not your fault... > > > It is, and, I'm not entirely sure we need it? I mean, it's nice to > show that it's now starting to set up, but it isn't particularly > helpful for end users or making sure things are working? This is driver is already spamming the logs, while generally we should only be logging when things go wrong. Those info messages can probably be changed to dev_dbg(), but that's a separate issue and not something that's needed to add support for wcn6855. And same here, you can keep the above as is, but it seems like someone will soon need to clean up the type handling as all these conditionals are getting a bit unwieldy. > > > @@ -1883,6 +1888,20 @@ static const struct qca_device_data qca_soc_data_wcn6750 = { > > > .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, > > > }; > > > > > > +static const struct qca_device_data qca_soc_data_wcn6855 = { > > > + .soc_type = QCA_WCN6855, > > > + .vregs = (struct qca_vreg []) { > > > + { "vddio", 5000 }, > > > + { "vddbtcxmx", 126000 }, > > > + { "vddrfacmn", 12500 }, > > > + { "vddrfa0p8", 102000 }, > > > + { "vddrfa1p7", 302000 }, > > > + { "vddrfa1p2", 257000 }, > > > > Hmm. More random regulator load values. I really think we should get rid > > of this but that's a separate discussion. > > > Bjorn specifically requested that he wanted me to leave them in. I'm > not married to them, and don't care one way or the other, I just > wanted working bluetooth since audio wasn't quite ready yet :) You can them in for now, but we had a discussion last fall about whether these made up numbers really belong in the kernel. > > > + }, > > > + .num_vregs = 6, > > > + .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, > > > +}; > > > + > > > static void qca_power_shutdown(struct hci_uart *hu) > > > { > > > struct qca_serdev *qcadev; > > > > As I mentioned elsewhere, you need to update also this function so that > > wcn6855 can be powered down. > > Sorry, I do have that locally, I just haven't pushed a v6 as I was > looking at Tim's v2 of the qca2066 and was wondering if I should or > shouldn't continue working on my version of the driver? I only skimmed that patch a while ago, but that ones not strictly needed for wcn6855, right? Things seems to work well here with just this series applied. > > With power-off handling fixed, this seems to work as quite well on my > > X13s with 6.3-rc1. Nice job! > > > > Btw, apart from the frame reassembly error, I'm also seeing: > > > > Bluetooth: Received HCI_IBS_WAKE_ACK in tx state 0 > > > > during probe. > > > I'm still not sure where the frame reassembly error comes from, and I > don't know how to get more info to figure it out either, if anyone > happens to have any guidance for that, I would love some. > Additionally, it doesn't always happen. It seems to happen on the > first load of the module, however, running modprobe -r && modprobe in > a loop (with the powerdown properly modified so the log isn't full of > splats), it doesn't seem to occur every time. Likewise for the > WAKE_ACK. Ok. Looks like the Chromium team tried to suppress these errors when switching line speed by toggling rts, but the frame-assembly error I get appears to happen before that. Johan
Hi Johan, <SNIP> > > > As I mentioned elsewhere, you need to update also this function so that > > > wcn6855 can be powered down. > > > > Sorry, I do have that locally, I just haven't pushed a v6 as I was > > looking at Tim's v2 of the qca2066 and was wondering if I should or > > shouldn't continue working on my version of the driver? > > I only skimmed that patch a while ago, but that ones not strictly needed > for wcn6855, right? Things seems to work well here with just this series > applied. Works, but, not quite well, and with the nvm bits from Tim's patch, we end up getting closer? I think that is the best way to put it. With what we currently have, we end up loading hpnv21.bin for our nvm patch file, however, we actually want (at least on my Thinkpad X13s) the .b8c file from the Windows partition for our nvm patch; With the b8c file symlinked to .bin with just my patch set, I am able to connect a pair of Air Pods Gen1 to the ThinkPad and play back audio, as well as use them for input. With the .bin file that comes from linux-firmware, they will still connect, however, they will randomly disconnect, as well as the audio output is all garbled. I think, ideally, we get v6+ in, and then we can figure out what to do about the bits that Tim's patch adds. I've tried them locally, but I'm not confident enough in my knowledge to address the issues that are brought up in the code review there. > > > With power-off handling fixed, this seems to work as quite well on my > > > X13s with 6.3-rc1. Nice job! > > > > > > Btw, apart from the frame reassembly error, I'm also seeing: > > > > > > Bluetooth: Received HCI_IBS_WAKE_ACK in tx state 0 > > > > > > during probe. > > > > > I'm still not sure where the frame reassembly error comes from, and I > > don't know how to get more info to figure it out either, if anyone > > happens to have any guidance for that, I would love some. > > Additionally, it doesn't always happen. It seems to happen on the > > first load of the module, however, running modprobe -r && modprobe in > > a loop (with the powerdown properly modified so the log isn't full of > > splats), it doesn't seem to occur every time. Likewise for the > > WAKE_ACK. > > Ok. Looks like the Chromium team tried to suppress these errors when > switching line speed by toggling rts, but the frame-assembly error I get > appears to happen before that. I am still trying to figure it out here as well, but I want to get v6 out there. > Johan
On Sun, Mar 12, 2023 at 10:18:48PM -0500, Steev Klimaszewski wrote: > Hi Johan, > > <SNIP> > > > > As I mentioned elsewhere, you need to update also this function so that > > > > wcn6855 can be powered down. > > > > > > Sorry, I do have that locally, I just haven't pushed a v6 as I was > > > looking at Tim's v2 of the qca2066 and was wondering if I should or > > > shouldn't continue working on my version of the driver? > > > > I only skimmed that patch a while ago, but that ones not strictly needed > > for wcn6855, right? Things seems to work well here with just this series > > applied. > > Works, but, not quite well, and with the nvm bits from Tim's patch, we > end up getting closer? I think that is the best way to put it. With > what we currently have, we end up loading hpnv21.bin for our nvm patch > file, however, we actually want (at least on my Thinkpad X13s) the > .b8c file from the Windows partition for our nvm patch; With the b8c > file symlinked to .bin with just my patch set, I am able to connect a > pair of Air Pods Gen1 to the ThinkPad and play back audio, as well as > use them for input. With the .bin file that comes from > linux-firmware, they will still connect, however, they will randomly > disconnect, as well as the audio output is all garbled. Hmm. Ok, but then we need to ask Lenovo and Qualcomm to release the firmware files we need for the X13s. Until then using your patch and "hpnv21.bin" at least works to some extent. I could connect to one bluetooth speaker without noticing any problems, but I did indeed get some garbled output when connecting to another. I have not tried the .b8c file yet though, so this could possibly be some other incompatibility issue. > I think, > ideally, we get v6+ in, and then we can figure out what to do about > the bits that Tim's patch adds. I've tried them locally, but I'm not > confident enough in my knowledge to address the issues that are > brought up in the code review there. Yes, that seems reasonable. Your patch is more complete in that it adds supports for managing power. Adding support for more fine grained loading of "NVM configuration" files could be done on top. > > > > With power-off handling fixed, this seems to work as quite well on my > > > > X13s with 6.3-rc1. Nice job! > > > > > > > > Btw, apart from the frame reassembly error, I'm also seeing: > > > > > > > > Bluetooth: Received HCI_IBS_WAKE_ACK in tx state 0 > > > > > > > > during probe. > > > > > > > I'm still not sure where the frame reassembly error comes from, and I > > > don't know how to get more info to figure it out either, if anyone > > > happens to have any guidance for that, I would love some. > > > Additionally, it doesn't always happen. It seems to happen on the > > > first load of the module, however, running modprobe -r && modprobe in > > > a loop (with the powerdown properly modified so the log isn't full of > > > splats), it doesn't seem to occur every time. Likewise for the > > > WAKE_ACK. > > > > Ok. Looks like the Chromium team tried to suppress these errors when > > switching line speed by toggling rts, but the frame-assembly error I get > > appears to happen before that. > > I am still trying to figure it out here as well, but I want to get v6 > out there. Yeah, I don't think that message during probe should be a show stopper here. Johan
On Tue, Mar 14, 2023 at 11:44:14AM +0100, Johan Hovold wrote: > On Sun, Mar 12, 2023 at 10:18:48PM -0500, Steev Klimaszewski wrote: > > Works, but, not quite well, and with the nvm bits from Tim's patch, we > > end up getting closer? I think that is the best way to put it. With > > what we currently have, we end up loading hpnv21.bin for our nvm patch > > file, however, we actually want (at least on my Thinkpad X13s) the > > .b8c file from the Windows partition for our nvm patch; With the b8c > > file symlinked to .bin with just my patch set, I am able to connect a > > pair of Air Pods Gen1 to the ThinkPad and play back audio, as well as > > use them for input. With the .bin file that comes from > > linux-firmware, they will still connect, however, they will randomly > > disconnect, as well as the audio output is all garbled. > > Hmm. Ok, but then we need to ask Lenovo and Qualcomm to release the > firmware files we need for the X13s. Until then using your patch and > "hpnv21.bin" at least works to some extent. > > I could connect to one bluetooth speaker without noticing any problems, > but I did indeed get some garbled output when connecting to another. I > have not tried the .b8c file yet though, so this could possibly be some > other incompatibility issue. I just tried with the hpnv21.b8c from the (somewhat old) windows installation on my x13s, but the bluetooth speaker that produced garbled output with the firmware from linux-firmware still does so with the windows file. Johan
diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c index c9064d34d830..2f9d8bd27c38 100644 --- a/drivers/bluetooth/btqca.c +++ b/drivers/bluetooth/btqca.c @@ -614,6 +614,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, config.type = ELF_TYPE_PATCH; snprintf(config.fwname, sizeof(config.fwname), "qca/msbtfw%02x.mbn", rom_ver); + } else if (soc_type == QCA_WCN6855) { + snprintf(config.fwname, sizeof(config.fwname), + "qca/hpbtfw%02x.tlv", rom_ver); } else { snprintf(config.fwname, sizeof(config.fwname), "qca/rampatch_%08x.bin", soc_ver); @@ -648,6 +651,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, else if (soc_type == QCA_WCN6750) snprintf(config.fwname, sizeof(config.fwname), "qca/msnv%02x.bin", rom_ver); + else if (soc_type == QCA_WCN6855) + snprintf(config.fwname, sizeof(config.fwname), + "qca/hpnv%02x.bin", rom_ver); else snprintf(config.fwname, sizeof(config.fwname), "qca/nvm_%08x.bin", soc_ver); @@ -672,6 +678,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, case QCA_WCN3991: case QCA_WCN3998: case QCA_WCN6750: + case QCA_WCN6855: hci_set_msft_opcode(hdev, 0xFD70); break; default: @@ -685,7 +692,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, return err; } - if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750) { + if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750 || soc_type == QCA_WCN6855) { /* get fw build info */ err = qca_read_fw_build_info(hdev); if (err < 0) diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h index 61e9a50e66ae..b884095bcd9d 100644 --- a/drivers/bluetooth/btqca.h +++ b/drivers/bluetooth/btqca.h @@ -147,6 +147,7 @@ enum qca_btsoc_type { QCA_WCN3991, QCA_QCA6390, QCA_WCN6750, + QCA_WCN6855, }; #if IS_ENABLED(CONFIG_BT_QCA) @@ -168,6 +169,10 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type) { return soc_type == QCA_WCN6750; } +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type) +{ + return soc_type == QCA_WCN6855; +} #else @@ -206,6 +211,11 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type) return false; } +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type) +{ + return false; +} + static inline int qca_send_pre_shutdown_cmd(struct hci_dev *hdev) { return -EOPNOTSUPP; diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index 3df8c3606e93..efc1c0306b4e 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -605,8 +605,7 @@ static int qca_open(struct hci_uart *hu) if (hu->serdev) { qcadev = serdev_device_get_drvdata(hu->serdev); - if (qca_is_wcn399x(qcadev->btsoc_type) || - qca_is_wcn6750(qcadev->btsoc_type)) + if (!(qcadev->init_speed)) hu->init_speed = qcadev->init_speed; if (qcadev->oper_speed) @@ -1317,7 +1316,8 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate) /* Give the controller time to process the request */ if (qca_is_wcn399x(qca_soc_type(hu)) || - qca_is_wcn6750(qca_soc_type(hu))) + qca_is_wcn6750(qca_soc_type(hu)) || + qca_is_wcn6855(qca_soc_type(hu))) usleep_range(1000, 10000); else msleep(300); @@ -1394,7 +1394,8 @@ static unsigned int qca_get_speed(struct hci_uart *hu, static int qca_check_speeds(struct hci_uart *hu) { if (qca_is_wcn399x(qca_soc_type(hu)) || - qca_is_wcn6750(qca_soc_type(hu))) { + qca_is_wcn6750(qca_soc_type(hu)) || + qca_is_wcn6855(qca_soc_type(hu))) { if (!qca_get_speed(hu, QCA_INIT_SPEED) && !qca_get_speed(hu, QCA_OPER_SPEED)) return -EINVAL; @@ -1682,7 +1683,8 @@ static int qca_power_on(struct hci_dev *hdev) return 0; if (qca_is_wcn399x(soc_type) || - qca_is_wcn6750(soc_type)) { + qca_is_wcn6750(soc_type) || + qca_is_wcn6855(soc_type)) { ret = qca_regulator_init(hu); } else { qcadev = serdev_device_get_drvdata(hu->serdev); @@ -1723,7 +1725,8 @@ static int qca_setup(struct hci_uart *hu) bt_dev_info(hdev, "setting up %s", qca_is_wcn399x(soc_type) ? "wcn399x" : - (soc_type == QCA_WCN6750) ? "wcn6750" : "ROME/QCA6390"); + (soc_type == QCA_WCN6750) ? "wcn6750" : + (soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390"); qca->memdump_state = QCA_MEMDUMP_IDLE; @@ -1735,7 +1738,8 @@ static int qca_setup(struct hci_uart *hu) clear_bit(QCA_SSR_TRIGGERED, &qca->flags); if (qca_is_wcn399x(soc_type) || - qca_is_wcn6750(soc_type)) { + qca_is_wcn6750(soc_type) || + qca_is_wcn6855(soc_type)) { set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks); hci_set_aosp_capable(hdev); @@ -1757,7 +1761,8 @@ static int qca_setup(struct hci_uart *hu) } if (!(qca_is_wcn399x(soc_type) || - qca_is_wcn6750(soc_type))) { + qca_is_wcn6750(soc_type) || + qca_is_wcn6855(soc_type))) { /* Get QCA version information */ ret = qca_read_soc_version(hdev, &ver, soc_type); if (ret) @@ -1883,6 +1888,20 @@ static const struct qca_device_data qca_soc_data_wcn6750 = { .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, }; +static const struct qca_device_data qca_soc_data_wcn6855 = { + .soc_type = QCA_WCN6855, + .vregs = (struct qca_vreg []) { + { "vddio", 5000 }, + { "vddbtcxmx", 126000 }, + { "vddrfacmn", 12500 }, + { "vddrfa0p8", 102000 }, + { "vddrfa1p7", 302000 }, + { "vddrfa1p2", 257000 }, + }, + .num_vregs = 6, + .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES, +}; + static void qca_power_shutdown(struct hci_uart *hu) { struct qca_serdev *qcadev; @@ -2047,7 +2066,8 @@ static int qca_serdev_probe(struct serdev_device *serdev) if (data && (qca_is_wcn399x(data->soc_type) || - qca_is_wcn6750(data->soc_type))) { + qca_is_wcn6750(data->soc_type) || + qca_is_wcn6855(data->soc_type))) { qcadev->btsoc_type = data->soc_type; qcadev->bt_power = devm_kzalloc(&serdev->dev, sizeof(struct qca_power), @@ -2067,14 +2087,18 @@ static int qca_serdev_probe(struct serdev_device *serdev) qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable", GPIOD_OUT_LOW); - if (IS_ERR_OR_NULL(qcadev->bt_en) && data->soc_type == QCA_WCN6750) { + if (IS_ERR_OR_NULL(qcadev->bt_en) + && (data->soc_type == QCA_WCN6750 || + data->soc_type == QCA_WCN6855)) { dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n"); power_ctrl_enabled = false; } qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl", GPIOD_IN); - if (IS_ERR_OR_NULL(qcadev->sw_ctrl) && data->soc_type == QCA_WCN6750) + if (IS_ERR_OR_NULL(qcadev->sw_ctrl) + && (data->soc_type == QCA_WCN6750 || + data->soc_type == QCA_WCN6855)) dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n"); qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL); @@ -2150,7 +2174,8 @@ static void qca_serdev_remove(struct serdev_device *serdev) struct qca_power *power = qcadev->bt_power; if ((qca_is_wcn399x(qcadev->btsoc_type) || - qca_is_wcn6750(qcadev->btsoc_type)) && + qca_is_wcn6750(qcadev->btsoc_type) || + qca_is_wcn6855(qcadev->btsoc_type)) && power->vregs_on) qca_power_shutdown(&qcadev->serdev_hu); else if (qcadev->susclk) @@ -2335,6 +2360,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = { { .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991}, { .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998}, { .compatible = "qcom,wcn6750-bt", .data = &qca_soc_data_wcn6750}, + { .compatible = "qcom,wcn6855-bt", .data = &qca_soc_data_wcn6855}, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
Added regulators,GPIOs and changes required to power on/off wcn6855. Added support for firmware download for wcn6855. Signed-off-by: Steev Klimaszewski <steev@kali.org> --- Changes since v4: * Remove unused firmware check because we don't have mbn firmware. * Set qcadev->init_speed if it hasn't been set. Changes since v3: * drop unused regulators Changes since v2: * drop unnecessary commit info Changes since v1: * None drivers/bluetooth/btqca.c | 9 ++++++- drivers/bluetooth/btqca.h | 10 ++++++++ drivers/bluetooth/hci_qca.c | 50 ++++++++++++++++++++++++++++--------- 3 files changed, 56 insertions(+), 13 deletions(-)