2023-02-08 09:41:14

by Tim Jiang

[permalink] [raw]
Subject: [PATCH v1] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC QCA2066

This patch adds support for QCA2066, including the devicetree
and patch/nvm downloading.

Signed-off-by: Tim Jiang <[email protected]>
---
drivers/bluetooth/btqca.c | 85 +++++++++++++++++++++++++++++++++++++
drivers/bluetooth/btqca.h | 2 +
drivers/bluetooth/hci_qca.c | 6 +++
3 files changed, 93 insertions(+)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index c9064d34d830..7d00df3a1d61 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -205,6 +205,55 @@ static int qca_send_reset(struct hci_dev *hdev)
return 0;
}

+static int qca_read_fw_board_id(struct hci_dev *hdev, u8 *bid)
+{
+ u8 cmd;
+ struct sk_buff *skb;
+ struct edl_event_hdr *edl;
+ int err;
+ int bid_len;
+
+ bt_dev_dbg(hdev, "QCA read board ID");
+
+ cmd = EDL_GET_BID_REQ_CMD;
+ skb = __hci_cmd_sync_ev(hdev, EDL_PATCH_CMD_OPCODE, EDL_PATCH_CMD_LEN,
+ &cmd, 0, HCI_INIT_TIMEOUT);
+ if (IS_ERR(skb)) {
+ err = PTR_ERR(skb);
+ bt_dev_err(hdev, "Reading QCA board ID failed (%d)",
+ err);
+ return err;
+ }
+
+ edl = (struct edl_event_hdr *)(skb->data);
+ if (!edl) {
+ bt_dev_err(hdev, "QCA read board ID with no header");
+ err = -EILSEQ;
+ goto out;
+ }
+
+ if (edl->cresp != EDL_CMD_REQ_RES_EVT ||
+ edl->rtype != EDL_GET_BID_REQ_CMD) {
+ bt_dev_err(hdev, "QCA Wrong packet received %d %d", edl->cresp,
+ edl->rtype);
+ err = -EIO;
+ goto out;
+ }
+
+ bid_len = edl->data[0];
+ bid[0] = edl->data[1];
+ bid[1] = edl->data[2];
+ bt_dev_info(hdev, "QCA board ID len %d,id = %x %x", bid_len, bid[0], bid[1]);
+
+
+ err = 0;
+
+out:
+ kfree_skb(skb);
+ return err;
+}
+
+
int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
{
struct sk_buff *skb;
@@ -574,6 +623,32 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
}
EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);

+static void qca_generate_nvm_name(struct hci_dev *hdev, char *fwname,
+ size_t max_size, struct qca_btsoc_version ver, u8 *bid)
+{
+ u8 rom_ver = 0;
+ u32 soc_ver;
+ const char *variant;
+
+ soc_ver = get_soc_ver(ver.soc_id, ver.rom_ver);
+ rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
+ bt_dev_info(hdev, "bid == 0x%02x 0x%02x", bid[0], bid[1]);
+
+ if ((ver.soc_id & 0x0000ff00) == 0x1200) /*hsp gf chip*/
+ variant = "g";
+ else
+ variant = "";
+
+ if (bid[0] == 0 && bid[1] == 0) {
+ snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant);
+ } else {
+ snprintf(fwname, max_size, "qca/hpnv%02x%s.%x%02x",
+ rom_ver, variant, bid[0], bid[1]);
+ }
+ bt_dev_info(hdev, "%s: %s", __func__, fwname);
+
+}
+
int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
const char *firmware_name)
@@ -582,6 +657,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
int err;
u8 rom_ver = 0;
u32 soc_ver;
+ u8 bid[2] = {0};

bt_dev_dbg(hdev, "QCA setup on UART");

@@ -607,6 +683,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
} else if (soc_type == QCA_QCA6390) {
snprintf(config.fwname, sizeof(config.fwname),
"qca/htbtfw%02x.tlv", rom_ver);
+ } else if (soc_type == QCA_QCA2066) {
+ snprintf(config.fwname, sizeof(config.fwname),
+ "qca/hpbtfw%02x.tlv", rom_ver);
} else if (soc_type == QCA_WCN6750) {
/* Choose mbn file by default.If mbn file is not found
* then choose tlv file
@@ -628,6 +707,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
/* Give the controller some time to get ready to receive the NVM */
msleep(10);

+ if (soc_type == QCA_QCA2066)
+ qca_read_fw_board_id(hdev, bid);
+
+
/* Download NVM configuration */
config.type = TLV_TYPE_NVM;
if (firmware_name)
@@ -645,6 +728,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
else if (soc_type == QCA_QCA6390)
snprintf(config.fwname, sizeof(config.fwname),
"qca/htnv%02x.bin", rom_ver);
+ else if (soc_type == QCA_QCA2066)
+ qca_generate_nvm_name(hdev, config.fwname, sizeof(config.fwname), ver, bid);
else if (soc_type == QCA_WCN6750)
snprintf(config.fwname, sizeof(config.fwname),
"qca/msnv%02x.bin", rom_ver);
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index 61e9a50e66ae..e762c403284a 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -13,6 +13,7 @@
#define EDL_PATCH_TLV_REQ_CMD (0x1E)
#define EDL_GET_BUILD_INFO_CMD (0x20)
#define EDL_NVM_ACCESS_SET_REQ_CMD (0x01)
+#define EDL_GET_BID_REQ_CMD (0x23)
#define EDL_PATCH_CONFIG_CMD (0x28)
#define MAX_SIZE_PER_TLV_SEGMENT (243)
#define QCA_PRE_SHUTDOWN_CMD (0xFC08)
@@ -147,6 +148,7 @@ enum qca_btsoc_type {
QCA_WCN3991,
QCA_QCA6390,
QCA_WCN6750,
+ QCA_QCA2066,
};

#if IS_ENABLED(CONFIG_BT_QCA)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 3df9e692756a..596f730ff375 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1859,6 +1859,11 @@ static const struct qca_device_data qca_soc_data_qca6390 = {
.num_vregs = 0,
};

+static const struct qca_device_data qca_soc_data_qca2066 = {
+ .soc_type = QCA_QCA2066,
+ .num_vregs = 0,
+};
+
static const struct qca_device_data qca_soc_data_wcn6750 = {
.soc_type = QCA_WCN6750,
.vregs = (struct qca_vreg []) {
@@ -2316,6 +2321,7 @@ static SIMPLE_DEV_PM_OPS(qca_pm_ops, qca_suspend, qca_resume);
static const struct of_device_id qca_bluetooth_of_match[] = {
{ .compatible = "qcom,qca6174-bt" },
{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
+ { .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
{ .compatible = "qcom,qca9377-bt" },
{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
{ .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991},
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project



2023-02-08 10:36:14

by bluez.test.bot

[permalink] [raw]
Subject: RE: [v1] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC QCA2066

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=719878

---Test result---

Test Summary:
CheckPatch FAIL 1.55 seconds
GitLint PASS 0.27 seconds
SubjectPrefix PASS 0.09 seconds
BuildKernel PASS 31.36 seconds
CheckAllWarning PASS 34.50 seconds
CheckSparse WARNING 38.42 seconds
CheckSmatch PASS 107.08 seconds
BuildKernel32 PASS 29.93 seconds
TestRunnerSetup PASS 433.09 seconds
TestRunner_l2cap-tester PASS 16.29 seconds
TestRunner_iso-tester PASS 16.63 seconds
TestRunner_bnep-tester PASS 5.41 seconds
TestRunner_mgmt-tester PASS 109.34 seconds
TestRunner_rfcomm-tester PASS 8.78 seconds
TestRunner_sco-tester PASS 8.01 seconds
TestRunner_ioctl-tester PASS 9.36 seconds
TestRunner_mesh-tester PASS 6.88 seconds
TestRunner_smp-tester PASS 7.94 seconds
TestRunner_userchan-tester PASS 5.67 seconds
IncrementalBuild PASS 28.19 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v1] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC QCA2066
WARNING: DT compatible string "qcom,qca2066-bt" appears un-documented -- check ./Documentation/devicetree/bindings/
#271: FILE: drivers/bluetooth/hci_qca.c:2324:
+ { .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},

total: 0 errors, 1 warnings, 153 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13132788.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
drivers/bluetooth/btqca.c:637:17: warning: restricted __le32 degrades to integerdrivers/bluetooth/btqca.c:720:24: warning: restricted __le32 degrades to integer


---
Regards,
Linux Bluetooth

2023-02-08 19:04:08

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH v1] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC QCA2066

Hi Tim,

On Wed, Feb 8, 2023 at 1:44 AM Tim Jiang <[email protected]> wrote:
>
> This patch adds support for QCA2066, including the devicetree
> and patch/nvm downloading.
>
> Signed-off-by: Tim Jiang <[email protected]>
> ---
> drivers/bluetooth/btqca.c | 85 +++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btqca.h | 2 +
> drivers/bluetooth/hci_qca.c | 6 +++
> 3 files changed, 93 insertions(+)
>
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index c9064d34d830..7d00df3a1d61 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -205,6 +205,55 @@ static int qca_send_reset(struct hci_dev *hdev)
> return 0;
> }
>
> +static int qca_read_fw_board_id(struct hci_dev *hdev, u8 *bid)
> +{
> + u8 cmd;
> + struct sk_buff *skb;
> + struct edl_event_hdr *edl;
> + int err;
> + int bid_len;
> +
> + bt_dev_dbg(hdev, "QCA read board ID");
> +
> + cmd = EDL_GET_BID_REQ_CMD;
> + skb = __hci_cmd_sync_ev(hdev, EDL_PATCH_CMD_OPCODE, EDL_PATCH_CMD_LEN,
> + &cmd, 0, HCI_INIT_TIMEOUT);
> + if (IS_ERR(skb)) {
> + err = PTR_ERR(skb);
> + bt_dev_err(hdev, "Reading QCA board ID failed (%d)",
> + err);
> + return err;
> + }
> +
> + edl = (struct edl_event_hdr *)(skb->data);

Please use skb_pull_data that checks if the skb has enough data.

> + if (!edl) {
> + bt_dev_err(hdev, "QCA read board ID with no header");
> + err = -EILSEQ;
> + goto out;
> + }
> +
> + if (edl->cresp != EDL_CMD_REQ_RES_EVT ||
> + edl->rtype != EDL_GET_BID_REQ_CMD) {
> + bt_dev_err(hdev, "QCA Wrong packet received %d %d", edl->cresp,
> + edl->rtype);
> + err = -EIO;
> + goto out;
> + }
> +
> + bid_len = edl->data[0];
> + bid[0] = edl->data[1];
> + bid[1] = edl->data[2];
> + bt_dev_info(hdev, "QCA board ID len %d,id = %x %x", bid_len, bid[0], bid[1]);
> +
> +
> + err = 0;
> +
> +out:
> + kfree_skb(skb);
> + return err;
> +}
> +
> +
> int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
> {
> struct sk_buff *skb;
> @@ -574,6 +623,32 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> }
> EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
>
> +static void qca_generate_nvm_name(struct hci_dev *hdev, char *fwname,
> + size_t max_size, struct qca_btsoc_version ver, u8 *bid)
> +{
> + u8 rom_ver = 0;
> + u32 soc_ver;
> + const char *variant;
> +
> + soc_ver = get_soc_ver(ver.soc_id, ver.rom_ver);
> + rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
> + bt_dev_info(hdev, "bid == 0x%02x 0x%02x", bid[0], bid[1]);
> +
> + if ((ver.soc_id & 0x0000ff00) == 0x1200) /*hsp gf chip*/
> + variant = "g";
> + else
> + variant = "";
> +
> + if (bid[0] == 0 && bid[1] == 0) {
> + snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant);
> + } else {
> + snprintf(fwname, max_size, "qca/hpnv%02x%s.%x%02x",
> + rom_ver, variant, bid[0], bid[1]);
> + }
> + bt_dev_info(hdev, "%s: %s", __func__, fwname);
> +
> +}
> +
> int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
> const char *firmware_name)
> @@ -582,6 +657,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> int err;
> u8 rom_ver = 0;
> u32 soc_ver;
> + u8 bid[2] = {0};
>
> bt_dev_dbg(hdev, "QCA setup on UART");
>
> @@ -607,6 +683,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> } else if (soc_type == QCA_QCA6390) {
> snprintf(config.fwname, sizeof(config.fwname),
> "qca/htbtfw%02x.tlv", rom_ver);
> + } else if (soc_type == QCA_QCA2066) {
> + snprintf(config.fwname, sizeof(config.fwname),
> + "qca/hpbtfw%02x.tlv", rom_ver);
> } else if (soc_type == QCA_WCN6750) {
> /* Choose mbn file by default.If mbn file is not found
> * then choose tlv file
> @@ -628,6 +707,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> /* Give the controller some time to get ready to receive the NVM */
> msleep(10);
>
> + if (soc_type == QCA_QCA2066)
> + qca_read_fw_board_id(hdev, bid);
> +
> +
> /* Download NVM configuration */
> config.type = TLV_TYPE_NVM;
> if (firmware_name)
> @@ -645,6 +728,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> else if (soc_type == QCA_QCA6390)
> snprintf(config.fwname, sizeof(config.fwname),
> "qca/htnv%02x.bin", rom_ver);
> + else if (soc_type == QCA_QCA2066)
> + qca_generate_nvm_name(hdev, config.fwname, sizeof(config.fwname), ver, bid);
> else if (soc_type == QCA_WCN6750)
> snprintf(config.fwname, sizeof(config.fwname),
> "qca/msnv%02x.bin", rom_ver);
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index 61e9a50e66ae..e762c403284a 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -13,6 +13,7 @@
> #define EDL_PATCH_TLV_REQ_CMD (0x1E)
> #define EDL_GET_BUILD_INFO_CMD (0x20)
> #define EDL_NVM_ACCESS_SET_REQ_CMD (0x01)
> +#define EDL_GET_BID_REQ_CMD (0x23)
> #define EDL_PATCH_CONFIG_CMD (0x28)
> #define MAX_SIZE_PER_TLV_SEGMENT (243)
> #define QCA_PRE_SHUTDOWN_CMD (0xFC08)
> @@ -147,6 +148,7 @@ enum qca_btsoc_type {
> QCA_WCN3991,
> QCA_QCA6390,
> QCA_WCN6750,
> + QCA_QCA2066,
> };
>
> #if IS_ENABLED(CONFIG_BT_QCA)
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 3df9e692756a..596f730ff375 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1859,6 +1859,11 @@ static const struct qca_device_data qca_soc_data_qca6390 = {
> .num_vregs = 0,
> };
>
> +static const struct qca_device_data qca_soc_data_qca2066 = {
> + .soc_type = QCA_QCA2066,
> + .num_vregs = 0,
> +};
> +
> static const struct qca_device_data qca_soc_data_wcn6750 = {
> .soc_type = QCA_WCN6750,
> .vregs = (struct qca_vreg []) {
> @@ -2316,6 +2321,7 @@ static SIMPLE_DEV_PM_OPS(qca_pm_ops, qca_suspend, qca_resume);
> static const struct of_device_id qca_bluetooth_of_match[] = {
> { .compatible = "qcom,qca6174-bt" },
> { .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
> + { .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},

Ive been reviewing this changes and I must say I don't like this
pattern of setting a data specific for each soc and then still having
to check what type it is everywhere, it is probably a lot cleaner to
have fields that are soc specific added to qca_device_data directly so
when adding new soc type one just have to update this table.

> { .compatible = "qcom,qca9377-bt" },
> { .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
> { .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991},
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
>


--
Luiz Augusto von Dentz

2023-02-08 21:18:09

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH v1] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC QCA2066

On Wed, Feb 08, 2023 at 05:40:38PM +0800, Tim Jiang wrote:
> This patch adds support for QCA2066, including the devicetree

What do you mean with "devicetree downloading"?

> and patch/nvm downloading.
>
> Signed-off-by: Tim Jiang <[email protected]>
> ---
> drivers/bluetooth/btqca.c | 85 +++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btqca.h | 2 +
> drivers/bluetooth/hci_qca.c | 6 +++
> 3 files changed, 93 insertions(+)
>
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index c9064d34d830..7d00df3a1d61 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -205,6 +205,55 @@ static int qca_send_reset(struct hci_dev *hdev)
> return 0;
> }
>
> +static int qca_read_fw_board_id(struct hci_dev *hdev, u8 *bid)
> +{
> + u8 cmd;
> + struct sk_buff *skb;
> + struct edl_event_hdr *edl;
> + int err;
> + int bid_len;
> +
> + bt_dev_dbg(hdev, "QCA read board ID");
> +
> + cmd = EDL_GET_BID_REQ_CMD;
> + skb = __hci_cmd_sync_ev(hdev, EDL_PATCH_CMD_OPCODE, EDL_PATCH_CMD_LEN,
> + &cmd, 0, HCI_INIT_TIMEOUT);
> + if (IS_ERR(skb)) {
> + err = PTR_ERR(skb);
> + bt_dev_err(hdev, "Reading QCA board ID failed (%d)",
> + err);
> + return err;
> + }
> +
> + edl = (struct edl_event_hdr *)(skb->data);
> + if (!edl) {
> + bt_dev_err(hdev, "QCA read board ID with no header");
> + err = -EILSEQ;
> + goto out;
> + }
> +
> + if (edl->cresp != EDL_CMD_REQ_RES_EVT ||
> + edl->rtype != EDL_GET_BID_REQ_CMD) {
> + bt_dev_err(hdev, "QCA Wrong packet received %d %d", edl->cresp,
> + edl->rtype);
> + err = -EIO;
> + goto out;
> + }
> +
> + bid_len = edl->data[0];
> + bid[0] = edl->data[1];
> + bid[1] = edl->data[2];
> + bt_dev_info(hdev, "QCA board ID len %d,id = %x %x", bid_len, bid[0], bid[1]);
> +
> +

Please clean up the double empty lines..

> + err = 0;

Why not zero-initialize err from the start?

> +
> +out:
> + kfree_skb(skb);
> + return err;
> +}
> +
> +
> int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
> {
> struct sk_buff *skb;
> @@ -574,6 +623,32 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> }
> EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
>
> +static void qca_generate_nvm_name(struct hci_dev *hdev, char *fwname,
> + size_t max_size, struct qca_btsoc_version ver, u8 *bid)
> +{
> + u8 rom_ver = 0;
> + u32 soc_ver;
> + const char *variant;
> +
> + soc_ver = get_soc_ver(ver.soc_id, ver.rom_ver);
> + rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
> + bt_dev_info(hdev, "bid == 0x%02x 0x%02x", bid[0], bid[1]);

Is this really useful information to put in the log every time the
device comes up?

> +
> + if ((ver.soc_id & 0x0000ff00) == 0x1200) /*hsp gf chip*/
> + variant = "g";
> + else
> + variant = "";
> +
> + if (bid[0] == 0 && bid[1] == 0) {
> + snprintf(fwname, max_size, "qca/hpnv%02x%s.bin", rom_ver, variant);
> + } else {
> + snprintf(fwname, max_size, "qca/hpnv%02x%s.%x%02x",
> + rom_ver, variant, bid[0], bid[1]);
> + }
> + bt_dev_info(hdev, "%s: %s", __func__, fwname);

Please write proper and useful informational prints in the log.

> +
> +}
> +
> int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> enum qca_btsoc_type soc_type, struct qca_btsoc_version ver,
> const char *firmware_name)
> @@ -582,6 +657,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> int err;
> u8 rom_ver = 0;
> u32 soc_ver;
> + u8 bid[2] = {0};

Seems that storing this as a u16 would make the patch cleaner.

>
> bt_dev_dbg(hdev, "QCA setup on UART");
>
> @@ -607,6 +683,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> } else if (soc_type == QCA_QCA6390) {
> snprintf(config.fwname, sizeof(config.fwname),
> "qca/htbtfw%02x.tlv", rom_ver);
> + } else if (soc_type == QCA_QCA2066) {
> + snprintf(config.fwname, sizeof(config.fwname),
> + "qca/hpbtfw%02x.tlv", rom_ver);
> } else if (soc_type == QCA_WCN6750) {
> /* Choose mbn file by default.If mbn file is not found
> * then choose tlv file
> @@ -628,6 +707,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> /* Give the controller some time to get ready to receive the NVM */
> msleep(10);
>
> + if (soc_type == QCA_QCA2066)
> + qca_read_fw_board_id(hdev, bid);
> +
> +

Double empty lines...

> /* Download NVM configuration */
> config.type = TLV_TYPE_NVM;
> if (firmware_name)
> @@ -645,6 +728,8 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> else if (soc_type == QCA_QCA6390)
> snprintf(config.fwname, sizeof(config.fwname),
> "qca/htnv%02x.bin", rom_ver);
> + else if (soc_type == QCA_QCA2066)
> + qca_generate_nvm_name(hdev, config.fwname, sizeof(config.fwname), ver, bid);
> else if (soc_type == QCA_WCN6750)
> snprintf(config.fwname, sizeof(config.fwname),
> "qca/msnv%02x.bin", rom_ver);
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index 61e9a50e66ae..e762c403284a 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -13,6 +13,7 @@
> #define EDL_PATCH_TLV_REQ_CMD (0x1E)
> #define EDL_GET_BUILD_INFO_CMD (0x20)
> #define EDL_NVM_ACCESS_SET_REQ_CMD (0x01)
> +#define EDL_GET_BID_REQ_CMD (0x23)
> #define EDL_PATCH_CONFIG_CMD (0x28)
> #define MAX_SIZE_PER_TLV_SEGMENT (243)
> #define QCA_PRE_SHUTDOWN_CMD (0xFC08)
> @@ -147,6 +148,7 @@ enum qca_btsoc_type {
> QCA_WCN3991,
> QCA_QCA6390,
> QCA_WCN6750,
> + QCA_QCA2066,
> };
>
> #if IS_ENABLED(CONFIG_BT_QCA)
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 3df9e692756a..596f730ff375 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1859,6 +1859,11 @@ static const struct qca_device_data qca_soc_data_qca6390 = {
> .num_vregs = 0,
> };
>
> +static const struct qca_device_data qca_soc_data_qca2066 = {
> + .soc_type = QCA_QCA2066,
> + .num_vregs = 0,
> +};
> +
> static const struct qca_device_data qca_soc_data_wcn6750 = {
> .soc_type = QCA_WCN6750,
> .vregs = (struct qca_vreg []) {
> @@ -2316,6 +2321,7 @@ static SIMPLE_DEV_PM_OPS(qca_pm_ops, qca_suspend, qca_resume);
> static const struct of_device_id qca_bluetooth_of_match[] = {
> { .compatible = "qcom,qca6174-bt" },
> { .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
> + { .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},

These entries seems alphabetically sorted, please maintain that.

Regards,
Bjorn

> { .compatible = "qcom,qca9377-bt" },
> { .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
> { .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991},
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
>

2023-02-09 00:59:39

by Steev Klimaszewski

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] Bluetooth: hci_qca: Add support for QTI Bluetooth chip wcn6855

>Hi Steev,
>
>On Mon, Feb 6, 2023 at 9:28 PM Steev Klimaszewski <[email protected]> wrote:
>>
>> Added regulators,GPIOs and changes required to power on/off wcn6855.
>> Added support for firmware download for wcn6855.
>>
>> Signed-off-by: Steev Klimaszewski <[email protected]>
>> ---
>>
>> Changes since v3:
>> * drop unused regulators
>>
>> Changes since v2:
>> * drop unnecessary commit info
>>
>> Changes since v1:
>> * None
>> ---
>> drivers/bluetooth/btqca.c | 24 ++++++++++++++--
>> drivers/bluetooth/btqca.h | 10 +++++++
>> drivers/bluetooth/hci_qca.c | 56 ++++++++++++++++++++++++++++---------
>> 3 files changed, 75 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index c9064d34d830..456b961b9554 100644
>> --- a/drivers/bluetooth/btqca.c
>> +++ b/drivers/bluetooth/btqca.c
>> @@ -472,6 +472,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
>> config->fwname, ret);
>> return ret;
>> }
>> + } else if (soc_type == QCA_WCN6855 && config->type == ELF_TYPE_PATCH) {
>> + bt_dev_dbg(hdev, "QCA Failed to request file: %s (%d)",
>> + config->fwname, ret);
>
>I suspect the above message is not really needed since you didn't even
>request the firmware at this point.
>

You are correct, I was copying from the 6750's work, where they have a .mbn file
available, and as far as I could tell looking through the Windows firmware
folders, we do not; so we shouldn't ever hit that. I've removed it.

>> + config->type = TLV_TYPE_PATCH;
>> + snprintf(config->fwname, sizeof(config->fwname),
>> + "qca/hpbtfw%02x.tlv", rom_ver);
>> + bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
>> + ret = request_firmware(&fw, config->fwname, &hdev->dev);
>> + if (ret) {
>> + bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>> + config->fwname, ret);
>> + return ret;
>> + }
>> } else {
>> bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>> config->fwname, ret);
>> @@ -596,7 +609,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>> */
>> rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
>>
>> - if (soc_type == QCA_WCN6750)
>> + if (soc_type == QCA_WCN6750 || soc_type == QCA_WCN6855)
>> qca_send_patch_config_cmd(hdev);
>>
>> /* Download rampatch file */
>> @@ -614,6 +627,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 +664,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 +691,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 +705,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..a35612ba23b2 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -606,7 +606,8 @@ static int qca_open(struct hci_uart *hu)
>> qcadev = serdev_device_get_drvdata(hu->serdev);
>>
>> 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))
>> hu->init_speed = qcadev->init_speed;
>
>This is really ugly I must say, why don't we just check if
>qcadev->init_speed has been set then we set it as hu->init_speed?
>
I think I've addressed this in v5, and hopefully I've done it right.

>> if (qcadev->oper_speed)
>> @@ -1317,7 +1318,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 +1396,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;
>> @@ -1428,7 +1431,8 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>> * changing the baudrate of chip and host.
>> */
>> if (qca_is_wcn399x(soc_type) ||
>> - qca_is_wcn6750(soc_type))
>> + qca_is_wcn6750(soc_type) ||
>> + qca_is_wcn6855(soc_type))
>> hci_uart_set_flow_control(hu, true);
>>
>> if (soc_type == QCA_WCN3990) {
>> @@ -1446,7 +1450,8 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>>
>> error:
>> if (qca_is_wcn399x(soc_type) ||
>> - qca_is_wcn6750(soc_type))
>> + qca_is_wcn6750(soc_type) ||
>> + qca_is_wcn6855(soc_type))
>> hci_uart_set_flow_control(hu, false);
>>
>> if (soc_type == QCA_WCN3990) {
>> @@ -1682,7 +1687,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 +1729,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 +1742,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 +1765,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 +1892,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,
>
>Since it appears there is already a struct like this for each soc type
>we could perhaps just extend with the data needed instead of checking
>the soc_type over and over again.

I'm definitely new to all of this, and if you mean something like a
quirks/feature set instead, that sounds like a good idea, however, it's quite a
bit over my head.

Additionally, I noticed your similar comments in
https://lore.kernel.org/all/CABBYNZLaOcYU-2HsSuJy4E6-OyfffmZdfK5mxWY=Wto4G84ySA@mail.gmail.com/
which is Tim's patch for the QCA2066. I actually think both the WCN6855 and the
QCA2066 might be the same chip? I'm not at Qualcomm, just someone who wanted to
get bluetooth working on my Thinkpad X13s and so I started poking at it until
something worked. If someone from Qualcomm could confirm or say, one way or the
other, that would be great. The QCA2066 and my patch do seem to use the same
firmware at least.

>> +};
>> +
>> static void qca_power_shutdown(struct hci_uart *hu)
>> {
>> struct qca_serdev *qcadev;
>> @@ -2047,7 +2070,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 +2091,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 +2178,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 +2364,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.0
>>
>
>
>--
>Luiz Augusto von Dentz

Thank you!
-- steev

2023-02-09 01:57:44

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH v4 2/4] Bluetooth: hci_qca: Add support for QTI Bluetooth chip wcn6855

Hi Steev,

On Wed, Feb 8, 2023 at 4:59 PM Steev Klimaszewski <[email protected]> wrote:
>
> >Hi Steev,
> >
> >On Mon, Feb 6, 2023 at 9:28 PM Steev Klimaszewski <[email protected]> wrote:
> >>
> >> Added regulators,GPIOs and changes required to power on/off wcn6855.
> >> Added support for firmware download for wcn6855.
> >>
> >> Signed-off-by: Steev Klimaszewski <[email protected]>
> >> ---
> >>
> >> Changes since v3:
> >> * drop unused regulators
> >>
> >> Changes since v2:
> >> * drop unnecessary commit info
> >>
> >> Changes since v1:
> >> * None
> >> ---
> >> drivers/bluetooth/btqca.c | 24 ++++++++++++++--
> >> drivers/bluetooth/btqca.h | 10 +++++++
> >> drivers/bluetooth/hci_qca.c | 56 ++++++++++++++++++++++++++++---------
> >> 3 files changed, 75 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> >> index c9064d34d830..456b961b9554 100644
> >> --- a/drivers/bluetooth/btqca.c
> >> +++ b/drivers/bluetooth/btqca.c
> >> @@ -472,6 +472,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
> >> config->fwname, ret);
> >> return ret;
> >> }
> >> + } else if (soc_type == QCA_WCN6855 && config->type == ELF_TYPE_PATCH) {
> >> + bt_dev_dbg(hdev, "QCA Failed to request file: %s (%d)",
> >> + config->fwname, ret);
> >
> >I suspect the above message is not really needed since you didn't even
> >request the firmware at this point.
> >
>
> You are correct, I was copying from the 6750's work, where they have a .mbn file
> available, and as far as I could tell looking through the Windows firmware
> folders, we do not; so we shouldn't ever hit that. I've removed it.
>
> >> + config->type = TLV_TYPE_PATCH;
> >> + snprintf(config->fwname, sizeof(config->fwname),
> >> + "qca/hpbtfw%02x.tlv", rom_ver);
> >> + bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
> >> + ret = request_firmware(&fw, config->fwname, &hdev->dev);
> >> + if (ret) {
> >> + bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> >> + config->fwname, ret);
> >> + return ret;
> >> + }
> >> } else {
> >> bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> >> config->fwname, ret);
> >> @@ -596,7 +609,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >> */
> >> rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
> >>
> >> - if (soc_type == QCA_WCN6750)
> >> + if (soc_type == QCA_WCN6750 || soc_type == QCA_WCN6855)
> >> qca_send_patch_config_cmd(hdev);
> >>
> >> /* Download rampatch file */
> >> @@ -614,6 +627,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 +664,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 +691,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 +705,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..a35612ba23b2 100644
> >> --- a/drivers/bluetooth/hci_qca.c
> >> +++ b/drivers/bluetooth/hci_qca.c
> >> @@ -606,7 +606,8 @@ static int qca_open(struct hci_uart *hu)
> >> qcadev = serdev_device_get_drvdata(hu->serdev);
> >>
> >> 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))
> >> hu->init_speed = qcadev->init_speed;
> >
> >This is really ugly I must say, why don't we just check if
> >qcadev->init_speed has been set then we set it as hu->init_speed?
> >
> I think I've addressed this in v5, and hopefully I've done it right.
>
> >> if (qcadev->oper_speed)
> >> @@ -1317,7 +1318,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 +1396,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;
> >> @@ -1428,7 +1431,8 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
> >> * changing the baudrate of chip and host.
> >> */
> >> if (qca_is_wcn399x(soc_type) ||
> >> - qca_is_wcn6750(soc_type))
> >> + qca_is_wcn6750(soc_type) ||
> >> + qca_is_wcn6855(soc_type))
> >> hci_uart_set_flow_control(hu, true);
> >>
> >> if (soc_type == QCA_WCN3990) {
> >> @@ -1446,7 +1450,8 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
> >>
> >> error:
> >> if (qca_is_wcn399x(soc_type) ||
> >> - qca_is_wcn6750(soc_type))
> >> + qca_is_wcn6750(soc_type) ||
> >> + qca_is_wcn6855(soc_type))
> >> hci_uart_set_flow_control(hu, false);
> >>
> >> if (soc_type == QCA_WCN3990) {
> >> @@ -1682,7 +1687,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 +1729,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 +1742,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 +1765,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 +1892,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,
> >
> >Since it appears there is already a struct like this for each soc type
> >we could perhaps just extend with the data needed instead of checking
> >the soc_type over and over again.
>
> I'm definitely new to all of this, and if you mean something like a
> quirks/feature set instead, that sounds like a good idea, however, it's quite a
> bit over my head.
>
> Additionally, I noticed your similar comments in
> https://lore.kernel.org/all/CABBYNZLaOcYU-2HsSuJy4E6-OyfffmZdfK5mxWY=Wto4G84ySA@mail.gmail.com/
> which is Tim's patch for the QCA2066. I actually think both the WCN6855 and the
> QCA2066 might be the same chip? I'm not at Qualcomm, just someone who wanted to
> get bluetooth working on my Thinkpad X13s and so I started poking at it until
> something worked. If someone from Qualcomm could confirm or say, one way or the
> other, that would be great. The QCA2066 and my patch do seem to use the same
> firmware at least.

It would be great if you could reach out to Tim if this is the case
and then work together so we don't have different sets attempting to
do the same which increases the time upstreams maintainers have to
spend on them.

> >> +};
> >> +
> >> static void qca_power_shutdown(struct hci_uart *hu)
> >> {
> >> struct qca_serdev *qcadev;
> >> @@ -2047,7 +2070,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 +2091,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 +2178,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 +2364,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.0
> >>
> >
> >
> >--
> >Luiz Augusto von Dentz
>
> Thank you!
> -- steev



--
Luiz Augusto von Dentz