[PATCH v1 0/3] bluetooth: qca: Add QCA6698 Bluetooth chip

[PATCH v1 0/3] bluetooth: qca: Add QCA6698 Bluetooth chip

[Cheng Jiang]

QCA6698 chipset shares the same IP core as the WCN6855. However,
it has different RF components and RAM sizes, so new firmware
is needed.

This change allows driver to distinguish it from the WCN6855
and load the specific firmware. As the RF performance of
QCA6698 chip from different foundries may vary. Therefore use
different NVM to configure them based on board ID.

Cheng Jiang (3):
  arm64: dts: qcom: sa8775p-ride: Change the BT node
  dt-bindings: net: Add QCA6698 Bluetooth
  Bluetooth: btqca: Add QCA6698 support

 .../net/bluetooth/qualcomm-bluetooth.yaml     |  2 +
 arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi    |  2 +-
 drivers/bluetooth/btqca.c                     | 47 ++++++++++++++++++-
 drivers/bluetooth/btqca.h                     |  1 +
 drivers/bluetooth/hci_qca.c                   | 36 +++++++++++++-
 5 files changed, 84 insertions(+), 4 deletions(-)


base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
--
2.25.1

[PATCH v1 1/3] arm64: dts: qcom: sa8775p-ride: Change the BT node

[Cheng Jiang]

The SA8775P-Ride uses the QCA6698 chipset, which shares the same IP core
as the WCN6855. However, it has different RF components and RAM sizes,
so new firmware is needed.  This change allows driver to distinguish it
from the WCN6855 and load the specific firmware.

Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
index 3fc62e123..f95e709bd 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
@@ -856,7 +856,7 @@ &uart17 {
 	status = "okay";
 
 	bluetooth {
-		compatible = "qcom,wcn6855-bt";
+		compatible = "qcom,qca6698-bt";
 
 		vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
 		vddaon-supply = <&vreg_pmu_aon_0p59>;
-- 
2.25.1

[PATCH v1 2/3] dt-bindings: net: Add QCA6698 Bluetooth

[Cheng Jiang]

Add the compatible for the Bluetooth part of the Qualcomm QCA6698 chipset.

Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
---
 .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
index 7bb68311c..82105382a 100644
--- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
+++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
@@ -18,6 +18,7 @@ properties:
     enum:
       - qcom,qca2066-bt
       - qcom,qca6174-bt
+      - qcom,qca6698-bt
       - qcom,qca9377-bt
       - qcom,wcn3988-bt
       - qcom,wcn3990-bt
@@ -170,6 +171,7 @@ allOf:
           contains:
             enum:
               - qcom,wcn6855-bt
+              - qcom,qca6698-bt
     then:
       required:
         - vddrfacmn-supply
-- 
2.25.1

[PATCH v1 3/3] Bluetooth: btqca: Add QCA6698 support

[Cheng Jiang]

Add support for the QCA6698 Bluetooth chip, which shares the same IP core
as the WCN6855. However, it has different RF components and RAM sizes,
requiring new firmware files. This patch adds support for loading QCA6698
rampatch and NVM from a different directory.

Due to variations in RF performance of QCA6698 chips from different
foundries, different NVM configurations are used based on board ID.

Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
---
 drivers/bluetooth/btqca.c   | 47 ++++++++++++++++++++++++++++++++++++-
 drivers/bluetooth/btqca.h   |  1 +
 drivers/bluetooth/hci_qca.c | 36 ++++++++++++++++++++++++++--
 3 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index dfbbac922..24bf00cac 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -700,6 +700,21 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co
 	return 0;
 }
 
+int qca_check_firmware_exists(const char *name, struct hci_dev *hdev)
+{
+	const struct firmware *fw;
+	int ret;
+
+	ret = firmware_request_nowarn(&fw, name, &hdev->dev);
+	if (ret) {
+		bt_dev_warn(hdev, "Firmware %s does not exist. Use default", name);
+		return 0;
+	}
+
+	release_firmware(fw);
+	return 1;
+}
+
 static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
 		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
 {
@@ -730,6 +745,26 @@ static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
 			 "qca/%snv%02x.b%02x", stem, rom_ver, bid);
 }
 
+static void qca_get_qca6698_nvm_name(struct hci_dev *hdev, char *fwname,
+		size_t max_size, struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
+{
+	const char *variant;
+
+	/* hsp gf chip */
+	if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
+		variant = "g";
+	else
+		variant = "";
+
+	if (bid != 0x0)
+		snprintf(fwname, max_size, "qca/QCA6698/hpnv%02x%s.b%04x", rom_ver,
+			 variant, bid);
+
+	/* if board id is 0 or the nvm file doesn't exisit, use the default */
+	if (bid == 0x0 || !qca_check_firmware_exists(fwname, hdev))
+		snprintf(fwname, max_size, "qca/QCA6698/hpnv%02x%s.bin", rom_ver, variant);
+}
+
 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)
@@ -796,6 +831,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 		snprintf(config.fwname, sizeof(config.fwname),
 			 "qca/hmtbtfw%02x.tlv", rom_ver);
 		break;
+	case QCA_QCA6698:
+		snprintf(config.fwname, sizeof(config.fwname),
+			 "qca/QCA6698/hpbtfw%02x.tlv", rom_ver);
+		break;
 	default:
 		snprintf(config.fwname, sizeof(config.fwname),
 			 "qca/rampatch_%08x.bin", soc_ver);
@@ -810,7 +849,7 @@ 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 || soc_type == QCA_WCN7850)
+	if (soc_type == QCA_QCA2066 || soc_type == QCA_QCA6698)
 		qca_read_fw_board_id(hdev, &boardid);
 
 	/* Download NVM configuration */
@@ -854,6 +893,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 		case QCA_WCN7850:
 			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
 			break;
+		case QCA_QCA6698:
+			qca_get_qca6698_nvm_name(hdev, config.fwname,
+				sizeof(config.fwname), ver, rom_ver, boardid);
+			break;
 
 		default:
 			snprintf(config.fwname, sizeof(config.fwname),
@@ -874,6 +917,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		err = qca_disable_soc_logging(hdev);
 		if (err < 0)
 			return err;
@@ -909,6 +953,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		/* 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 bb5207d7a..67c16d8f2 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -151,6 +151,7 @@ enum qca_btsoc_type {
 	QCA_WCN3991,
 	QCA_QCA2066,
 	QCA_QCA6390,
+	QCA_QCA6698,
 	QCA_WCN6750,
 	QCA_WCN6855,
 	QCA_WCN7850,
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 37129e6cb..70bdc046c 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1361,6 +1361,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		usleep_range(1000, 10000);
 		break;
 
@@ -1447,6 +1448,7 @@ static int qca_check_speeds(struct hci_uart *hu)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
 		    !qca_get_speed(hu, QCA_OPER_SPEED))
 			return -EINVAL;
@@ -1489,6 +1491,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 		case QCA_WCN6750:
 		case QCA_WCN6855:
 		case QCA_WCN7850:
+		case QCA_QCA6698:
 			hci_uart_set_flow_control(hu, true);
 			break;
 
@@ -1523,6 +1526,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 		case QCA_WCN6750:
 		case QCA_WCN6855:
 		case QCA_WCN7850:
+		case QCA_QCA6698:
 			hci_uart_set_flow_control(hu, false);
 			break;
 
@@ -1803,6 +1807,7 @@ static int qca_power_on(struct hci_dev *hdev)
 	case QCA_WCN6855:
 	case QCA_WCN7850:
 	case QCA_QCA6390:
+	case QCA_QCA6698:
 		ret = qca_regulator_init(hu);
 		break;
 
@@ -1878,6 +1883,10 @@ static int qca_setup(struct hci_uart *hu)
 		soc_name = "qca2066";
 		break;
 
+	case QCA_QCA6698:
+		soc_name = "qca6698";
+		break;
+
 	case QCA_WCN3988:
 	case QCA_WCN3990:
 	case QCA_WCN3991:
@@ -1919,6 +1928,7 @@ static int qca_setup(struct hci_uart *hu)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		qcadev = serdev_device_get_drvdata(hu->serdev);
 		if (qcadev->bdaddr_property_broken)
 			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
@@ -1952,6 +1962,7 @@ static int qca_setup(struct hci_uart *hu)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		break;
 
 	default:
@@ -2089,6 +2100,20 @@ static const struct qca_device_data qca_soc_data_qca6390 __maybe_unused = {
 	.num_vregs = 0,
 };
 
+static const struct qca_device_data qca_soc_data_qca6698 __maybe_unused = {
+	.soc_type = QCA_QCA6698,
+	.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 const struct qca_device_data qca_soc_data_wcn6750 __maybe_unused = {
 	.soc_type = QCA_WCN6750,
 	.vregs = (struct qca_vreg []) {
@@ -2179,6 +2204,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
 
 	case QCA_WCN6750:
 	case QCA_WCN6855:
+	case QCA_QCA6698:
 		gpiod_set_value_cansleep(qcadev->bt_en, 0);
 		msleep(100);
 		qca_regulator_disable(qcadev);
@@ -2333,6 +2359,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 	case QCA_WCN6855:
 	case QCA_WCN7850:
 	case QCA_QCA6390:
+	case QCA_QCA6698:
 		qcadev->bt_power = devm_kzalloc(&serdev->dev,
 						sizeof(struct qca_power),
 						GFP_KERNEL);
@@ -2346,6 +2373,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 	switch (qcadev->btsoc_type) {
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		if (!device_property_present(&serdev->dev, "enable-gpios")) {
 			/*
 			 * Backward compatibility with old DT sources. If the
@@ -2380,7 +2408,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 					       GPIOD_OUT_LOW);
 		if (IS_ERR(qcadev->bt_en) &&
 		    (data->soc_type == QCA_WCN6750 ||
-		     data->soc_type == QCA_WCN6855)) {
+		     data->soc_type == QCA_WCN6855 ||
+		     data->soc_type == QCA_QCA6698)) {
 			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
 			return PTR_ERR(qcadev->bt_en);
 		}
@@ -2393,7 +2422,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		if (IS_ERR(qcadev->sw_ctrl) &&
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855 ||
-		     data->soc_type == QCA_WCN7850)) {
+		     data->soc_type == QCA_WCN7850 ||
+		     data->soc_type == QCA_QCA6698)) {
 			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
 			return PTR_ERR(qcadev->sw_ctrl);
 		}
@@ -2475,6 +2505,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
 	case QCA_WCN6750:
 	case QCA_WCN6855:
 	case QCA_WCN7850:
+	case QCA_QCA6698:
 		if (power->vregs_on)
 			qca_power_shutdown(&qcadev->serdev_hu);
 		break;
@@ -2669,6 +2700,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
 	{ .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
 	{ .compatible = "qcom,qca6174-bt" },
 	{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
+	{ .compatible = "qcom,qca6698-bt", .data = &qca_soc_data_qca6698},
 	{ .compatible = "qcom,qca9377-bt" },
 	{ .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988},
 	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
-- 
2.25.1

Re: [PATCH v1 0/3] bluetooth: qca: Add QCA6698 Bluetooth chip

[Dmitry Baryshkov]

On Thu, Nov 28, 2024 at 08:09:19PM +0800, Cheng Jiang wrote:
> QCA6698 chipset shares the same IP core as the WCN6855. However,
> it has different RF components and RAM sizes, so new firmware
> is needed.
> 
> This change allows driver to distinguish it from the WCN6855
> and load the specific firmware. As the RF performance of
> QCA6698 chip from different foundries may vary. Therefore use
> different NVM to configure them based on board ID.
> 
> Cheng Jiang (3):
>   arm64: dts: qcom: sa8775p-ride: Change the BT node
>   dt-bindings: net: Add QCA6698 Bluetooth
>   Bluetooth: btqca: Add QCA6698 support

Order is totally incorrect:
- dt bindings
- driver changes
- DTS

> 
>  .../net/bluetooth/qualcomm-bluetooth.yaml     |  2 +
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi    |  2 +-
>  drivers/bluetooth/btqca.c                     | 47 ++++++++++++++++++-
>  drivers/bluetooth/btqca.h                     |  1 +
>  drivers/bluetooth/hci_qca.c                   | 36 +++++++++++++-
>  5 files changed, 84 insertions(+), 4 deletions(-)
> 
> 
> base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
> --
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] dt-bindings: net: Add QCA6698 Bluetooth

[Dmitry Baryshkov]

On Thu, Nov 28, 2024 at 08:09:21PM +0800, Cheng Jiang wrote:
> Add the compatible for the Bluetooth part of the Qualcomm QCA6698 chipset.

... 
And you have misssed to explain why do you need to add it and how it is
different from WCN6855.

> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>  .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> index 7bb68311c..82105382a 100644
> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> @@ -18,6 +18,7 @@ properties:
>      enum:
>        - qcom,qca2066-bt
>        - qcom,qca6174-bt
> +      - qcom,qca6698-bt
>        - qcom,qca9377-bt
>        - qcom,wcn3988-bt
>        - qcom,wcn3990-bt
> @@ -170,6 +171,7 @@ allOf:
>            contains:
>              enum:
>                - qcom,wcn6855-bt
> +              - qcom,qca6698-bt
>      then:
>        required:
>          - vddrfacmn-supply
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 3/3] Bluetooth: btqca: Add QCA6698 support

[Dmitry Baryshkov]

On Thu, Nov 28, 2024 at 08:09:22PM +0800, Cheng Jiang wrote:
> Add support for the QCA6698 Bluetooth chip, which shares the same IP core
> as the WCN6855. However, it has different RF components and RAM sizes,
> requiring new firmware files. This patch adds support for loading QCA6698
> rampatch and NVM from a different directory.
> 
> Due to variations in RF performance of QCA6698 chips from different
> foundries, different NVM configurations are used based on board ID.
> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>  drivers/bluetooth/btqca.c   | 47 ++++++++++++++++++++++++++++++++++++-
>  drivers/bluetooth/btqca.h   |  1 +
>  drivers/bluetooth/hci_qca.c | 36 ++++++++++++++++++++++++++--
>  3 files changed, 81 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index dfbbac922..24bf00cac 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -700,6 +700,21 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co
>  	return 0;
>  }
>  
> +int qca_check_firmware_exists(const char *name, struct hci_dev *hdev)
> +{
> +	const struct firmware *fw;
> +	int ret;
> +
> +	ret = firmware_request_nowarn(&fw, name, &hdev->dev);
> +	if (ret) {
> +		bt_dev_warn(hdev, "Firmware %s does not exist. Use default", name);

No useless warnings

> +		return 0;
> +	}
> +
> +	release_firmware(fw);
> +	return 1;
> +}
> +
>  static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
>  		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
>  {
> @@ -730,6 +745,26 @@ static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
>  			 "qca/%snv%02x.b%02x", stem, rom_ver, bid);
>  }
>  
> +static void qca_get_qca6698_nvm_name(struct hci_dev *hdev, char *fwname,
> +		size_t max_size, struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
> +{
> +	const char *variant;
> +
> +	/* hsp gf chip */
> +	if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
> +		variant = "g";
> +	else
> +		variant = "";
> +
> +	if (bid != 0x0)
> +		snprintf(fwname, max_size, "qca/QCA6698/hpnv%02x%s.b%04x", rom_ver,
> +			 variant, bid);
> +
> +	/* if board id is 0 or the nvm file doesn't exisit, use the default */
> +	if (bid == 0x0 || !qca_check_firmware_exists(fwname, hdev))
> +		snprintf(fwname, max_size, "qca/QCA6698/hpnv%02x%s.bin", rom_ver, variant);

So, do you want to load the same firmware twice? You've asked for it
already, if it is present, use it.

Anyway, if you have followed previous discussions, you'd have known that
it has been recommended to use DT's firmware-name instead of pushing
everything to the driver.

> +}
> +
>  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)
> @@ -796,6 +831,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/hmtbtfw%02x.tlv", rom_ver);
>  		break;
> +	case QCA_QCA6698:
> +		snprintf(config.fwname, sizeof(config.fwname),
> +			 "qca/QCA6698/hpbtfw%02x.tlv", rom_ver);
> +		break;
>  	default:
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/rampatch_%08x.bin", soc_ver);
> @@ -810,7 +849,7 @@ 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 || soc_type == QCA_WCN7850)
> +	if (soc_type == QCA_QCA2066 || soc_type == QCA_QCA6698)
>  		qca_read_fw_board_id(hdev, &boardid);
>  
>  	/* Download NVM configuration */
> @@ -854,6 +893,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  		case QCA_WCN7850:
>  			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
>  			break;
> +		case QCA_QCA6698:
> +			qca_get_qca6698_nvm_name(hdev, config.fwname,
> +				sizeof(config.fwname), ver, rom_ver, boardid);
> +			break;
>  
>  		default:
>  			snprintf(config.fwname, sizeof(config.fwname),
> @@ -874,6 +917,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		err = qca_disable_soc_logging(hdev);
>  		if (err < 0)
>  			return err;
> @@ -909,6 +953,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		/* 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 bb5207d7a..67c16d8f2 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -151,6 +151,7 @@ enum qca_btsoc_type {
>  	QCA_WCN3991,
>  	QCA_QCA2066,
>  	QCA_QCA6390,
> +	QCA_QCA6698,
>  	QCA_WCN6750,
>  	QCA_WCN6855,
>  	QCA_WCN7850,
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 37129e6cb..70bdc046c 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1361,6 +1361,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		usleep_range(1000, 10000);
>  		break;
>  
> @@ -1447,6 +1448,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
>  		    !qca_get_speed(hu, QCA_OPER_SPEED))
>  			return -EINVAL;
> @@ -1489,6 +1491,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  		case QCA_WCN6750:
>  		case QCA_WCN6855:
>  		case QCA_WCN7850:
> +		case QCA_QCA6698:
>  			hci_uart_set_flow_control(hu, true);
>  			break;
>  
> @@ -1523,6 +1526,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  		case QCA_WCN6750:
>  		case QCA_WCN6855:
>  		case QCA_WCN7850:
> +		case QCA_QCA6698:
>  			hci_uart_set_flow_control(hu, false);
>  			break;
>  
> @@ -1803,6 +1807,7 @@ static int qca_power_on(struct hci_dev *hdev)
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
>  	case QCA_QCA6390:
> +	case QCA_QCA6698:
>  		ret = qca_regulator_init(hu);
>  		break;
>  
> @@ -1878,6 +1883,10 @@ static int qca_setup(struct hci_uart *hu)
>  		soc_name = "qca2066";
>  		break;
>  
> +	case QCA_QCA6698:
> +		soc_name = "qca6698";
> +		break;
> +
>  	case QCA_WCN3988:
>  	case QCA_WCN3990:
>  	case QCA_WCN3991:
> @@ -1919,6 +1928,7 @@ static int qca_setup(struct hci_uart *hu)
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		qcadev = serdev_device_get_drvdata(hu->serdev);
>  		if (qcadev->bdaddr_property_broken)
>  			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
> @@ -1952,6 +1962,7 @@ static int qca_setup(struct hci_uart *hu)
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		break;
>  
>  	default:
> @@ -2089,6 +2100,20 @@ static const struct qca_device_data qca_soc_data_qca6390 __maybe_unused = {
>  	.num_vregs = 0,
>  };
>  
> +static const struct qca_device_data qca_soc_data_qca6698 __maybe_unused = {
> +	.soc_type = QCA_QCA6698,

No. It's the same as WCN6855. You don't need extra SoC type for it.

> +	.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 const struct qca_device_data qca_soc_data_wcn6750 __maybe_unused = {
>  	.soc_type = QCA_WCN6750,
>  	.vregs = (struct qca_vreg []) {
> @@ -2179,6 +2204,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
> +	case QCA_QCA6698:
>  		gpiod_set_value_cansleep(qcadev->bt_en, 0);
>  		msleep(100);
>  		qca_regulator_disable(qcadev);
> @@ -2333,6 +2359,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
>  	case QCA_QCA6390:
> +	case QCA_QCA6698:
>  		qcadev->bt_power = devm_kzalloc(&serdev->dev,
>  						sizeof(struct qca_power),
>  						GFP_KERNEL);
> @@ -2346,6 +2373,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  	switch (qcadev->btsoc_type) {
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		if (!device_property_present(&serdev->dev, "enable-gpios")) {
>  			/*
>  			 * Backward compatibility with old DT sources. If the
> @@ -2380,7 +2408,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  					       GPIOD_OUT_LOW);
>  		if (IS_ERR(qcadev->bt_en) &&
>  		    (data->soc_type == QCA_WCN6750 ||
> -		     data->soc_type == QCA_WCN6855)) {
> +		     data->soc_type == QCA_WCN6855 ||
> +		     data->soc_type == QCA_QCA6698)) {
>  			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>  			return PTR_ERR(qcadev->bt_en);
>  		}
> @@ -2393,7 +2422,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		if (IS_ERR(qcadev->sw_ctrl) &&
>  		    (data->soc_type == QCA_WCN6750 ||
>  		     data->soc_type == QCA_WCN6855 ||
> -		     data->soc_type == QCA_WCN7850)) {
> +		     data->soc_type == QCA_WCN7850 ||
> +		     data->soc_type == QCA_QCA6698)) {
>  			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>  			return PTR_ERR(qcadev->sw_ctrl);
>  		}
> @@ -2475,6 +2505,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
>  	case QCA_WCN6750:
>  	case QCA_WCN6855:
>  	case QCA_WCN7850:
> +	case QCA_QCA6698:
>  		if (power->vregs_on)
>  			qca_power_shutdown(&qcadev->serdev_hu);
>  		break;
> @@ -2669,6 +2700,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
>  	{ .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
>  	{ .compatible = "qcom,qca6174-bt" },
>  	{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
> +	{ .compatible = "qcom,qca6698-bt", .data = &qca_soc_data_qca6698},
>  	{ .compatible = "qcom,qca9377-bt" },
>  	{ .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988},
>  	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 1/3] arm64: dts: qcom: sa8775p-ride: Change the BT node

[Krzysztof Kozlowski]

On 28/11/2024 13:09, Cheng Jiang wrote:
> The SA8775P-Ride uses the QCA6698 chipset, which shares the same IP core
> as the WCN6855. However, it has different RF components and RAM sizes,
> so new firmware is needed.  This change allows driver to distinguish it
> from the WCN6855 and load the specific firmware.
> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> index 3fc62e123..f95e709bd 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> @@ -856,7 +856,7 @@ &uart17 {
>  	status = "okay";
>  
>  	bluetooth {
> -		compatible = "qcom,wcn6855-bt";
> +		compatible = "qcom,qca6698-bt";

This breaks users without mentioning it, without really justifying the
impact. Also it is not clear for me whether devices are or are not
compatible.

Best regards,
Krzysztof

Re: [PATCH v1 2/3] dt-bindings: net: Add QCA6698 Bluetooth

[Krzysztof Kozlowski]

On 28/11/2024 13:09, Cheng Jiang wrote:
> Add the compatible for the Bluetooth part of the Qualcomm QCA6698 chipset.

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

Respond to the comment and then implement it.

Also, version your patches correct and provide changelog. This is v2,
not v1.

Best regards,
Krzysztof

Re: [PATCH v1 2/3] dt-bindings: net: Add QCA6698 Bluetooth

[Krzysztof Kozlowski]

On 28/11/2024 15:41, Krzysztof Kozlowski wrote:
> On 28/11/2024 13:09, Cheng Jiang wrote:
>> Add the compatible for the Bluetooth part of the Qualcomm QCA6698 chipset.
> 
> <form letter>
> This is a friendly reminder during the review process.
> 
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
> 
> Thank you.
> </form letter>
> 
> Respond to the comment and then implement it.
> 
> Also, version your patches correct and provide changelog. This is v2,
> not v1.

Wait, no, it's even v3 or v4. You just ask us to the same work twice,
don't you?

Best regards,
Krzysztof

Re: [PATCH v1 3/3] Bluetooth: btqca: Add QCA6698 support

[kernel test robot]

Hi Cheng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on f486c8aa16b8172f63bddc70116a0c897a7f3f02]

url:    https://github.com/intel-lab-lkp/linux/commits/Cheng-Jiang/arm64-dts-qcom-sa8775p-ride-Change-the-BT-node/20241128-201156
base:   f486c8aa16b8172f63bddc70116a0c897a7f3f02
patch link:    https://lore.kernel.org/r/20241128120922.3518582-4-quic_chejiang%40quicinc.com
patch subject: [PATCH v1 3/3] Bluetooth: btqca: Add QCA6698 support
config: arc-randconfig-002-20241129 (https://download.01.org/0day-ci/archive/20241129/202411290858.nBlfcX9C-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241129/202411290858.nBlfcX9C-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411290858.nBlfcX9C-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/bluetooth/btqca.c:703:5: warning: no previous prototype for 'qca_check_firmware_exists' [-Wmissing-prototypes]
     703 | int qca_check_firmware_exists(const char *name, struct hci_dev *hdev)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~


vim +/qca_check_firmware_exists +703 drivers/bluetooth/btqca.c

   702	
 > 703	int qca_check_firmware_exists(const char *name, struct hci_dev *hdev)
   704	{
   705		const struct firmware *fw;
   706		int ret;
   707	
   708		ret = firmware_request_nowarn(&fw, name, &hdev->dev);
   709		if (ret) {
   710			bt_dev_warn(hdev, "Firmware %s does not exist. Use default", name);
   711			return 0;
   712		}
   713	
   714		release_firmware(fw);
   715		return 1;
   716	}
   717	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 0/3] bluetooth: qca: Add QCA6698 Bluetooth chip

[Cheng Jiang (IOE)]

On 11/28/2024 8:57 PM, Dmitry Baryshkov wrote:
> On Thu, Nov 28, 2024 at 08:09:19PM +0800, Cheng Jiang wrote:
>> QCA6698 chipset shares the same IP core as the WCN6855. However,
>> it has different RF components and RAM sizes, so new firmware
>> is needed.
>>
>> This change allows driver to distinguish it from the WCN6855
>> and load the specific firmware. As the RF performance of
>> QCA6698 chip from different foundries may vary. Therefore use
>> different NVM to configure them based on board ID.
>>
>> Cheng Jiang (3):
>>   arm64: dts: qcom: sa8775p-ride: Change the BT node
>>   dt-bindings: net: Add QCA6698 Bluetooth
>>   Bluetooth: btqca: Add QCA6698 support
> 
> Order is totally incorrect:
> - dt bindings
> - driver changes
> - DTS
> 
Ack, will follow this order later
>>
>>  .../net/bluetooth/qualcomm-bluetooth.yaml     |  2 +
>>  arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi    |  2 +-
>>  drivers/bluetooth/btqca.c                     | 47 ++++++++++++++++++-
>>  drivers/bluetooth/btqca.h                     |  1 +
>>  drivers/bluetooth/hci_qca.c                   | 36 +++++++++++++-
>>  5 files changed, 84 insertions(+), 4 deletions(-)
>>
>>
>> base-commit: f486c8aa16b8172f63bddc70116a0c897a7f3f02
>> --
>> 2.25.1
>>
> 

Re: [PATCH v1 3/3] Bluetooth: btqca: Add QCA6698 support

[Cheng Jiang (IOE)]

Hi Dmitry,

On 11/28/2024 9:02 PM, Dmitry Baryshkov wrote:
> On Thu, Nov 28, 2024 at 08:09:22PM +0800, Cheng Jiang wrote:
>> Add support for the QCA6698 Bluetooth chip, which shares the same IP core
>> as the WCN6855. However, it has different RF components and RAM sizes,
>> requiring new firmware files. This patch adds support for loading QCA6698
>> rampatch and NVM from a different directory.
>>
>> Due to variations in RF performance of QCA6698 chips from different
>> foundries, different NVM configurations are used based on board ID.
>>
>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>> ---
>>  drivers/bluetooth/btqca.c   | 47 ++++++++++++++++++++++++++++++++++++-
>>  drivers/bluetooth/btqca.h   |  1 +
>>  drivers/bluetooth/hci_qca.c | 36 ++++++++++++++++++++++++++--
>>  3 files changed, 81 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index dfbbac922..24bf00cac 100644
>> --- a/drivers/bluetooth/btqca.c
>> +++ b/drivers/bluetooth/btqca.c
>> @@ -700,6 +700,21 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co
>>  	return 0;
>>  }
>>  
>> +int qca_check_firmware_exists(const char *name, struct hci_dev *hdev)
>> +{
>> +	const struct firmware *fw;
>> +	int ret;
>> +
>> +	ret = firmware_request_nowarn(&fw, name, &hdev->dev);
>> +	if (ret) {
>> +		bt_dev_warn(hdev, "Firmware %s does not exist. Use default", name);
> 
> No useless warnings
Ack.
> 
>> +		return 0;
>> +	}
>> +
>> +	release_firmware(fw);
>> +	return 1;
>> +}
>> +
>>  static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
>>  		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
>>  {
>> @@ -730,6 +745,26 @@ static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
>>  			 "qca/%snv%02x.b%02x", stem, rom_ver, bid);
>>  }
>>  
>> +static void qca_get_qca6698_nvm_name(struct hci_dev *hdev, char *fwname,
>> +		size_t max_size, struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
>> +{
>> +	const char *variant;
>> +
>> +	/* hsp gf chip */
>> +	if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
>> +		variant = "g";
>> +	else
>> +		variant = "";
>> +
>> +	if (bid != 0x0)
>> +		snprintf(fwname, max_size, "qca/QCA6698/hpnv%02x%s.b%04x", rom_ver,
>> +			 variant, bid);
>> +
>> +	/* if board id is 0 or the nvm file doesn't exisit, use the default */
>> +	if (bid == 0x0 || !qca_check_firmware_exists(fwname, hdev))
>> +		snprintf(fwname, max_size, "qca/QCA6698/hpnv%02x%s.bin", rom_ver, variant);
> 
> So, do you want to load the same firmware twice? You've asked for it
> already, if it is present, use it.
This is only used to check the nvm exists or not. Yes, it's loaded twice if the nvm exist.
It's just to avoid too much changes of the firmware download on the current driver. 
> 
> Anyway, if you have followed previous discussions, you'd have known that
> it has been recommended to use DT's firmware-name instead of pushing
> everything to the driver.
Sorry, I misunderstand the comment here. I thought it was to add a compact string. 
"qcom,qca6698-bt", since both the meothods can solve our requriment. If use DT's
firmware-name is perfered, I can provide a change based on it. 

"
Need because of the product needs or need because of the existing
firmware not working with the chip?
Wait... your WiFi colleagues were more helpful and they wrote that "it
has different RF,
IPA, thermal, RAM size and etc, so new firmware files used." ([1]).
Please include that information in your commit messages too to let
reviewers understand  what is going on.

[1] https://lore.kernel.org/linux-arm-msm/20241024002514.92290-1-quic_miaoqing@quicinc.com/

> Let me check if using
> "firmware-name" allows us to omit the new soc type.
> From the driver's perspective, the only change is the need to load a
> different firmware.

If you want to emphasise that it is not just WCN6855, extend schema to
use fallback compatibles:
compat = "qcom,qca6698-bt", "qcom,wcn6855-bt"; No driver changes are
necessary with this approach.
"
> 
>> +}
>> +
>>  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)
>> @@ -796,6 +831,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  		snprintf(config.fwname, sizeof(config.fwname),
>>  			 "qca/hmtbtfw%02x.tlv", rom_ver);
>>  		break;
>> +	case QCA_QCA6698:
>> +		snprintf(config.fwname, sizeof(config.fwname),
>> +			 "qca/QCA6698/hpbtfw%02x.tlv", rom_ver);
>> +		break;
>>  	default:
>>  		snprintf(config.fwname, sizeof(config.fwname),
>>  			 "qca/rampatch_%08x.bin", soc_ver);
>> @@ -810,7 +849,7 @@ 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 || soc_type == QCA_WCN7850)
>> +	if (soc_type == QCA_QCA2066 || soc_type == QCA_QCA6698)
>>  		qca_read_fw_board_id(hdev, &boardid);
>>  
>>  	/* Download NVM configuration */
>> @@ -854,6 +893,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  		case QCA_WCN7850:
>>  			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
>>  			break;
>> +		case QCA_QCA6698:
>> +			qca_get_qca6698_nvm_name(hdev, config.fwname,
>> +				sizeof(config.fwname), ver, rom_ver, boardid);
>> +			break;
>>  
>>  		default:
>>  			snprintf(config.fwname, sizeof(config.fwname),
>> @@ -874,6 +917,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		err = qca_disable_soc_logging(hdev);
>>  		if (err < 0)
>>  			return err;
>> @@ -909,6 +953,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		/* 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 bb5207d7a..67c16d8f2 100644
>> --- a/drivers/bluetooth/btqca.h
>> +++ b/drivers/bluetooth/btqca.h
>> @@ -151,6 +151,7 @@ enum qca_btsoc_type {
>>  	QCA_WCN3991,
>>  	QCA_QCA2066,
>>  	QCA_QCA6390,
>> +	QCA_QCA6698,
>>  	QCA_WCN6750,
>>  	QCA_WCN6855,
>>  	QCA_WCN7850,
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 37129e6cb..70bdc046c 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1361,6 +1361,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		usleep_range(1000, 10000);
>>  		break;
>>  
>> @@ -1447,6 +1448,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
>>  		    !qca_get_speed(hu, QCA_OPER_SPEED))
>>  			return -EINVAL;
>> @@ -1489,6 +1491,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>>  		case QCA_WCN6750:
>>  		case QCA_WCN6855:
>>  		case QCA_WCN7850:
>> +		case QCA_QCA6698:
>>  			hci_uart_set_flow_control(hu, true);
>>  			break;
>>  
>> @@ -1523,6 +1526,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>>  		case QCA_WCN6750:
>>  		case QCA_WCN6855:
>>  		case QCA_WCN7850:
>> +		case QCA_QCA6698:
>>  			hci_uart_set_flow_control(hu, false);
>>  			break;
>>  
>> @@ -1803,6 +1807,7 @@ static int qca_power_on(struct hci_dev *hdev)
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>>  	case QCA_QCA6390:
>> +	case QCA_QCA6698:
>>  		ret = qca_regulator_init(hu);
>>  		break;
>>  
>> @@ -1878,6 +1883,10 @@ static int qca_setup(struct hci_uart *hu)
>>  		soc_name = "qca2066";
>>  		break;
>>  
>> +	case QCA_QCA6698:
>> +		soc_name = "qca6698";
>> +		break;
>> +
>>  	case QCA_WCN3988:
>>  	case QCA_WCN3990:
>>  	case QCA_WCN3991:
>> @@ -1919,6 +1928,7 @@ static int qca_setup(struct hci_uart *hu)
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		qcadev = serdev_device_get_drvdata(hu->serdev);
>>  		if (qcadev->bdaddr_property_broken)
>>  			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
>> @@ -1952,6 +1962,7 @@ static int qca_setup(struct hci_uart *hu)
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		break;
>>  
>>  	default:
>> @@ -2089,6 +2100,20 @@ static const struct qca_device_data qca_soc_data_qca6390 __maybe_unused = {
>>  	.num_vregs = 0,
>>  };
>>  
>> +static const struct qca_device_data qca_soc_data_qca6698 __maybe_unused = {
>> +	.soc_type = QCA_QCA6698,
> 
> No. It's the same as WCN6855. You don't need extra SoC type for it.
> 
>> +	.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 const struct qca_device_data qca_soc_data_wcn6750 __maybe_unused = {
>>  	.soc_type = QCA_WCN6750,
>>  	.vregs = (struct qca_vreg []) {
>> @@ -2179,6 +2204,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>>  
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>> +	case QCA_QCA6698:
>>  		gpiod_set_value_cansleep(qcadev->bt_en, 0);
>>  		msleep(100);
>>  		qca_regulator_disable(qcadev);
>> @@ -2333,6 +2359,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>>  	case QCA_QCA6390:
>> +	case QCA_QCA6698:
>>  		qcadev->bt_power = devm_kzalloc(&serdev->dev,
>>  						sizeof(struct qca_power),
>>  						GFP_KERNEL);
>> @@ -2346,6 +2373,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>  	switch (qcadev->btsoc_type) {
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		if (!device_property_present(&serdev->dev, "enable-gpios")) {
>>  			/*
>>  			 * Backward compatibility with old DT sources. If the
>> @@ -2380,7 +2408,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>  					       GPIOD_OUT_LOW);
>>  		if (IS_ERR(qcadev->bt_en) &&
>>  		    (data->soc_type == QCA_WCN6750 ||
>> -		     data->soc_type == QCA_WCN6855)) {
>> +		     data->soc_type == QCA_WCN6855 ||
>> +		     data->soc_type == QCA_QCA6698)) {
>>  			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>>  			return PTR_ERR(qcadev->bt_en);
>>  		}
>> @@ -2393,7 +2422,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>  		if (IS_ERR(qcadev->sw_ctrl) &&
>>  		    (data->soc_type == QCA_WCN6750 ||
>>  		     data->soc_type == QCA_WCN6855 ||
>> -		     data->soc_type == QCA_WCN7850)) {
>> +		     data->soc_type == QCA_WCN7850 ||
>> +		     data->soc_type == QCA_QCA6698)) {
>>  			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>>  			return PTR_ERR(qcadev->sw_ctrl);
>>  		}
>> @@ -2475,6 +2505,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
>>  	case QCA_WCN6750:
>>  	case QCA_WCN6855:
>>  	case QCA_WCN7850:
>> +	case QCA_QCA6698:
>>  		if (power->vregs_on)
>>  			qca_power_shutdown(&qcadev->serdev_hu);
>>  		break;
>> @@ -2669,6 +2700,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
>>  	{ .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
>>  	{ .compatible = "qcom,qca6174-bt" },
>>  	{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
>> +	{ .compatible = "qcom,qca6698-bt", .data = &qca_soc_data_qca6698},
>>  	{ .compatible = "qcom,qca9377-bt" },
>>  	{ .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988},
>>  	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH v1 2/3] dt-bindings: net: Add QCA6698 Bluetooth

[Cheng Jiang (IOE)]

Hi Krzysztof,

On 11/28/2024 10:41 PM, Krzysztof Kozlowski wrote:
> On 28/11/2024 15:41, Krzysztof Kozlowski wrote:
>> On 28/11/2024 13:09, Cheng Jiang wrote:
>>> Add the compatible for the Bluetooth part of the Qualcomm QCA6698 chipset.
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>> </form letter>
>>
>> Respond to the comment and then implement it.
>>
>> Also, version your patches correct and provide changelog. This is v2,
>> not v1.
> 
> Wait, no, it's even v3 or v4. You just ask us to the same work twice,
> don't you?
Sorry for this. So the version number should be increased even 
solution/implementation is changed? Will follow the rule.  
> 
> Best regards,
> Krzysztof

Re: [PATCH v1 2/3] dt-bindings: net: Add QCA6698 Bluetooth

[Cheng Jiang (IOE)]

Hi Krzysztof,

On 11/28/2024 10:41 PM, Krzysztof Kozlowski wrote:
> On 28/11/2024 13:09, Cheng Jiang wrote:
>> Add the compatible for the Bluetooth part of the Qualcomm QCA6698 chipset.
> 
> <form letter>
> This is a friendly reminder during the review process.
> 
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
> 
> Thank you.
> </form letter>
> 
> Respond to the comment and then implement it.
> 
> Also, version your patches correct and provide changelog. This is v2,
> not v1.
> 
Thank you for the guidance. 

> Best regards,
> Krzysztof

Re: [PATCH v1 2/3] dt-bindings: net: Add QCA6698 Bluetooth

[Cheng Jiang (IOE)]

Hi Dmitry,

On 11/28/2024 8:58 PM, Dmitry Baryshkov wrote:
> On Thu, Nov 28, 2024 at 08:09:21PM +0800, Cheng Jiang wrote:
>> Add the compatible for the Bluetooth part of the Qualcomm QCA6698 chipset.
> 
> ... 
> And you have misssed to explain why do you need to add it and how it is
> different from WCN6855.
> 
Got it. I just explain in the dts/driver change, forget to explain here. 

If use the firmware-name solution, do we still need add the new compatible
string for qcom,qca6698-bt here? The driver may not use this string.   
>>
>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>> ---
>>  .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>> index 7bb68311c..82105382a 100644
>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>> @@ -18,6 +18,7 @@ properties:
>>      enum:
>>        - qcom,qca2066-bt
>>        - qcom,qca6174-bt
>> +      - qcom,qca6698-bt
>>        - qcom,qca9377-bt
>>        - qcom,wcn3988-bt
>>        - qcom,wcn3990-bt
>> @@ -170,6 +171,7 @@ allOf:
>>            contains:
>>              enum:
>>                - qcom,wcn6855-bt
>> +              - qcom,qca6698-bt
>>      then:
>>        required:
>>          - vddrfacmn-supply
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH v1 1/3] arm64: dts: qcom: sa8775p-ride: Change the BT node

[Cheng Jiang (IOE)]

Hi Krzysztof,

On 11/28/2024 10:39 PM, Krzysztof Kozlowski wrote:
> On 28/11/2024 13:09, Cheng Jiang wrote:
>> The SA8775P-Ride uses the QCA6698 chipset, which shares the same IP core
>> as the WCN6855. However, it has different RF components and RAM sizes,
>> so new firmware is needed.  This change allows driver to distinguish it
>> from the WCN6855 and load the specific firmware.
>>
>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>> ---
>>  arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
>> index 3fc62e123..f95e709bd 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
>> @@ -856,7 +856,7 @@ &uart17 {
>>  	status = "okay";
>>  
>>  	bluetooth {
>> -		compatible = "qcom,wcn6855-bt";
>> +		compatible = "qcom,qca6698-bt";
> 
> This breaks users without mentioning it, without really justifying the
> impact. Also it is not clear for me whether devices are or are not
> compatible.
QCA6698 can use the wcn6855 firmware, but the performance is not good as expect. 
To avoid breaking existing projects, we plan to upstream a new firmware for this
chip. 
If use the 'firmware-name' to specify the firmware, we can keep the compatible
here unchanged. Is it OK?
> 
> Best regards,
> Krzysztof

Re: [PATCH v1 3/3] Bluetooth: btqca: Add QCA6698 support

[kernel test robot]

Hi Cheng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on f486c8aa16b8172f63bddc70116a0c897a7f3f02]

url:    https://github.com/intel-lab-lkp/linux/commits/Cheng-Jiang/arm64-dts-qcom-sa8775p-ride-Change-the-BT-node/20241128-201156
base:   f486c8aa16b8172f63bddc70116a0c897a7f3f02
patch link:    https://lore.kernel.org/r/20241128120922.3518582-4-quic_chejiang%40quicinc.com
patch subject: [PATCH v1 3/3] Bluetooth: btqca: Add QCA6698 support
config: arm-randconfig-001-20241129 (https://download.01.org/0day-ci/archive/20241129/202411291150.ngHlIQve-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241129/202411291150.ngHlIQve-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411291150.ngHlIQve-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/bluetooth/btqca.c:703:5: warning: no previous prototype for function 'qca_check_firmware_exists' [-Wmissing-prototypes]
   int qca_check_firmware_exists(const char *name, struct hci_dev *hdev)
       ^
   drivers/bluetooth/btqca.c:703:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int qca_check_firmware_exists(const char *name, struct hci_dev *hdev)
   ^
   static 
   1 warning generated.


vim +/qca_check_firmware_exists +703 drivers/bluetooth/btqca.c

   702	
 > 703	int qca_check_firmware_exists(const char *name, struct hci_dev *hdev)
   704	{
   705		const struct firmware *fw;
   706		int ret;
   707	
   708		ret = firmware_request_nowarn(&fw, name, &hdev->dev);
   709		if (ret) {
   710			bt_dev_warn(hdev, "Firmware %s does not exist. Use default", name);
   711			return 0;
   712		}
   713	
   714		release_firmware(fw);
   715		return 1;
   716	}
   717	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 2/3] dt-bindings: net: Add QCA6698 Bluetooth

[Krzysztof Kozlowski]

On 29/11/2024 03:13, Cheng Jiang (IOE) wrote:
> Hi Krzysztof,
> 
> On 11/28/2024 10:41 PM, Krzysztof Kozlowski wrote:
>> On 28/11/2024 15:41, Krzysztof Kozlowski wrote:
>>> On 28/11/2024 13:09, Cheng Jiang wrote:
>>>> Add the compatible for the Bluetooth part of the Qualcomm QCA6698 chipset.
>>>
>>> <form letter>
>>> This is a friendly reminder during the review process.
>>>
>>> It seems my or other reviewer's previous comments were not fully
>>> addressed. Maybe the feedback got lost between the quotes, maybe you
>>> just forgot to apply it. Please go back to the previous discussion and
>>> either implement all requested changes or keep discussing them.
>>>
>>> Thank you.
>>> </form letter>
>>>
>>> Respond to the comment and then implement it.
>>>
>>> Also, version your patches correct and provide changelog. This is v2,
>>> not v1.
>>
>> Wait, no, it's even v3 or v4. You just ask us to the same work twice,
>> don't you?
> Sorry for this. So the version number should be increased even 
> solution/implementation is changed? Will follow the rule.  

Yes, because you don't want to send the same for review over and over again.

Best regards,
Krzysztof

Re: [PATCH v1 2/3] dt-bindings: net: Add QCA6698 Bluetooth

[Dmitry Baryshkov]

On Fri, Nov 29, 2024 at 10:30:00AM +0800, Cheng Jiang (IOE) wrote:
> Hi Dmitry,
> 
> On 11/28/2024 8:58 PM, Dmitry Baryshkov wrote:
> > On Thu, Nov 28, 2024 at 08:09:21PM +0800, Cheng Jiang wrote:
> >> Add the compatible for the Bluetooth part of the Qualcomm QCA6698 chipset.
> > 
> > ... 
> > And you have misssed to explain why do you need to add it and how it is
> > different from WCN6855.
> > 
> Got it. I just explain in the dts/driver change, forget to explain here. 
> 
> If use the firmware-name solution, do we still need add the new compatible
> string for qcom,qca6698-bt here? The driver may not use this string.   

DT describes the hardware. If you want, you can still add new string
_and_ use old one as a fallback compatible: "qcom,qca6698-bt",
"qcom,wcn6855-bt".

> >>
> >> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >> ---
> >>  .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >> index 7bb68311c..82105382a 100644
> >> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
> >> @@ -18,6 +18,7 @@ properties:
> >>      enum:
> >>        - qcom,qca2066-bt
> >>        - qcom,qca6174-bt
> >> +      - qcom,qca6698-bt
> >>        - qcom,qca9377-bt
> >>        - qcom,wcn3988-bt
> >>        - qcom,wcn3990-bt
> >> @@ -170,6 +171,7 @@ allOf:
> >>            contains:
> >>              enum:
> >>                - qcom,wcn6855-bt
> >> +              - qcom,qca6698-bt
> >>      then:
> >>        required:
> >>          - vddrfacmn-supply
> >> -- 
> >> 2.25.1
> >>
> > 
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 3/3] Bluetooth: btqca: Add QCA6698 support

[Dmitry Baryshkov]

On Fri, Nov 29, 2024 at 10:05:30AM +0800, Cheng Jiang (IOE) wrote:
> Hi Dmitry,
> 
> On 11/28/2024 9:02 PM, Dmitry Baryshkov wrote:
> > On Thu, Nov 28, 2024 at 08:09:22PM +0800, Cheng Jiang wrote:
> >> Add support for the QCA6698 Bluetooth chip, which shares the same IP core
> >> as the WCN6855. However, it has different RF components and RAM sizes,
> >> requiring new firmware files. This patch adds support for loading QCA6698
> >> rampatch and NVM from a different directory.
> >>
> >> Due to variations in RF performance of QCA6698 chips from different
> >> foundries, different NVM configurations are used based on board ID.
> >>
> >> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >> ---
> >>  drivers/bluetooth/btqca.c   | 47 ++++++++++++++++++++++++++++++++++++-
> >>  drivers/bluetooth/btqca.h   |  1 +
> >>  drivers/bluetooth/hci_qca.c | 36 ++++++++++++++++++++++++++--
> >>  3 files changed, 81 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> >> index dfbbac922..24bf00cac 100644
> >> --- a/drivers/bluetooth/btqca.c
> >> +++ b/drivers/bluetooth/btqca.c
> >> @@ -700,6 +700,21 @@ static int qca_check_bdaddr(struct hci_dev *hdev, const struct qca_fw_config *co
> >>  	return 0;
> >>  }
> >>  
> >> +int qca_check_firmware_exists(const char *name, struct hci_dev *hdev)
> >> +{
> >> +	const struct firmware *fw;
> >> +	int ret;
> >> +
> >> +	ret = firmware_request_nowarn(&fw, name, &hdev->dev);
> >> +	if (ret) {
> >> +		bt_dev_warn(hdev, "Firmware %s does not exist. Use default", name);
> > 
> > No useless warnings
> Ack.
> > 
> >> +		return 0;
> >> +	}
> >> +
> >> +	release_firmware(fw);
> >> +	return 1;
> >> +}
> >> +
> >>  static void qca_generate_hsp_nvm_name(char *fwname, size_t max_size,
> >>  		struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
> >>  {
> >> @@ -730,6 +745,26 @@ static inline void qca_get_nvm_name_generic(struct qca_fw_config *cfg,
> >>  			 "qca/%snv%02x.b%02x", stem, rom_ver, bid);
> >>  }
> >>  
> >> +static void qca_get_qca6698_nvm_name(struct hci_dev *hdev, char *fwname,
> >> +		size_t max_size, struct qca_btsoc_version ver, u8 rom_ver, u16 bid)
> >> +{
> >> +	const char *variant;
> >> +
> >> +	/* hsp gf chip */
> >> +	if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
> >> +		variant = "g";
> >> +	else
> >> +		variant = "";
> >> +
> >> +	if (bid != 0x0)
> >> +		snprintf(fwname, max_size, "qca/QCA6698/hpnv%02x%s.b%04x", rom_ver,
> >> +			 variant, bid);
> >> +
> >> +	/* if board id is 0 or the nvm file doesn't exisit, use the default */
> >> +	if (bid == 0x0 || !qca_check_firmware_exists(fwname, hdev))
> >> +		snprintf(fwname, max_size, "qca/QCA6698/hpnv%02x%s.bin", rom_ver, variant);
> > 
> > So, do you want to load the same firmware twice? You've asked for it
> > already, if it is present, use it.
> This is only used to check the nvm exists or not. Yes, it's loaded twice if the nvm exist.
> It's just to avoid too much changes of the firmware download on the current driver. 
> > 
> > Anyway, if you have followed previous discussions, you'd have known that
> > it has been recommended to use DT's firmware-name instead of pushing
> > everything to the driver.
> Sorry, I misunderstand the comment here. I thought it was to add a compact string. 
> "qcom,qca6698-bt", since both the meothods can solve our requriment. If use DT's
> firmware-name is perfered, I can provide a change based on it. 
> 
> "
> Need because of the product needs or need because of the existing
> firmware not working with the chip?
> Wait... your WiFi colleagues were more helpful and they wrote that "it
> has different RF,
> IPA, thermal, RAM size and etc, so new firmware files used." ([1]).
> Please include that information in your commit messages too to let
> reviewers understand  what is going on.
> 
> [1] https://lore.kernel.org/linux-arm-msm/20241024002514.92290-1-quic_miaoqing@quicinc.com/
> 
> > Let me check if using
> > "firmware-name" allows us to omit the new soc type.
> > From the driver's perspective, the only change is the need to load a
> > different firmware.
> 
> If you want to emphasise that it is not just WCN6855, extend schema to
> use fallback compatibles:
> compat = "qcom,qca6698-bt", "qcom,wcn6855-bt"; No driver changes are
> necessary with this approach.
> "

Please learn how to quote messages. Anyway, it clearly says to use
firmware-name and two compat strings. If you have questions, please ask
them _before_  sending new iteration, not after.

> > 
> >> +}
> >> +
> >>  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)
> >> @@ -796,6 +831,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  		snprintf(config.fwname, sizeof(config.fwname),
> >>  			 "qca/hmtbtfw%02x.tlv", rom_ver);
> >>  		break;
> >> +	case QCA_QCA6698:
> >> +		snprintf(config.fwname, sizeof(config.fwname),
> >> +			 "qca/QCA6698/hpbtfw%02x.tlv", rom_ver);
> >> +		break;
> >>  	default:
> >>  		snprintf(config.fwname, sizeof(config.fwname),
> >>  			 "qca/rampatch_%08x.bin", soc_ver);
> >> @@ -810,7 +849,7 @@ 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 || soc_type == QCA_WCN7850)
> >> +	if (soc_type == QCA_QCA2066 || soc_type == QCA_QCA6698)
> >>  		qca_read_fw_board_id(hdev, &boardid);
> >>  
> >>  	/* Download NVM configuration */
> >> @@ -854,6 +893,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  		case QCA_WCN7850:
> >>  			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
> >>  			break;
> >> +		case QCA_QCA6698:
> >> +			qca_get_qca6698_nvm_name(hdev, config.fwname,
> >> +				sizeof(config.fwname), ver, rom_ver, boardid);
> >> +			break;
> >>  
> >>  		default:
> >>  			snprintf(config.fwname, sizeof(config.fwname),
> >> @@ -874,6 +917,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		err = qca_disable_soc_logging(hdev);
> >>  		if (err < 0)
> >>  			return err;
> >> @@ -909,6 +953,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		/* 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 bb5207d7a..67c16d8f2 100644
> >> --- a/drivers/bluetooth/btqca.h
> >> +++ b/drivers/bluetooth/btqca.h
> >> @@ -151,6 +151,7 @@ enum qca_btsoc_type {
> >>  	QCA_WCN3991,
> >>  	QCA_QCA2066,
> >>  	QCA_QCA6390,
> >> +	QCA_QCA6698,
> >>  	QCA_WCN6750,
> >>  	QCA_WCN6855,
> >>  	QCA_WCN7850,
> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> >> index 37129e6cb..70bdc046c 100644
> >> --- a/drivers/bluetooth/hci_qca.c
> >> +++ b/drivers/bluetooth/hci_qca.c
> >> @@ -1361,6 +1361,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		usleep_range(1000, 10000);
> >>  		break;
> >>  
> >> @@ -1447,6 +1448,7 @@ static int qca_check_speeds(struct hci_uart *hu)
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
> >>  		    !qca_get_speed(hu, QCA_OPER_SPEED))
> >>  			return -EINVAL;
> >> @@ -1489,6 +1491,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
> >>  		case QCA_WCN6750:
> >>  		case QCA_WCN6855:
> >>  		case QCA_WCN7850:
> >> +		case QCA_QCA6698:
> >>  			hci_uart_set_flow_control(hu, true);
> >>  			break;
> >>  
> >> @@ -1523,6 +1526,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
> >>  		case QCA_WCN6750:
> >>  		case QCA_WCN6855:
> >>  		case QCA_WCN7850:
> >> +		case QCA_QCA6698:
> >>  			hci_uart_set_flow_control(hu, false);
> >>  			break;
> >>  
> >> @@ -1803,6 +1807,7 @@ static int qca_power_on(struct hci_dev *hdev)
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >>  	case QCA_QCA6390:
> >> +	case QCA_QCA6698:
> >>  		ret = qca_regulator_init(hu);
> >>  		break;
> >>  
> >> @@ -1878,6 +1883,10 @@ static int qca_setup(struct hci_uart *hu)
> >>  		soc_name = "qca2066";
> >>  		break;
> >>  
> >> +	case QCA_QCA6698:
> >> +		soc_name = "qca6698";
> >> +		break;
> >> +
> >>  	case QCA_WCN3988:
> >>  	case QCA_WCN3990:
> >>  	case QCA_WCN3991:
> >> @@ -1919,6 +1928,7 @@ static int qca_setup(struct hci_uart *hu)
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		qcadev = serdev_device_get_drvdata(hu->serdev);
> >>  		if (qcadev->bdaddr_property_broken)
> >>  			set_bit(HCI_QUIRK_BDADDR_PROPERTY_BROKEN, &hdev->quirks);
> >> @@ -1952,6 +1962,7 @@ static int qca_setup(struct hci_uart *hu)
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		break;
> >>  
> >>  	default:
> >> @@ -2089,6 +2100,20 @@ static const struct qca_device_data qca_soc_data_qca6390 __maybe_unused = {
> >>  	.num_vregs = 0,
> >>  };
> >>  
> >> +static const struct qca_device_data qca_soc_data_qca6698 __maybe_unused = {
> >> +	.soc_type = QCA_QCA6698,
> > 
> > No. It's the same as WCN6855. You don't need extra SoC type for it.
> > 
> >> +	.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 const struct qca_device_data qca_soc_data_wcn6750 __maybe_unused = {
> >>  	.soc_type = QCA_WCN6750,
> >>  	.vregs = (struct qca_vreg []) {
> >> @@ -2179,6 +2204,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
> >>  
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >> +	case QCA_QCA6698:
> >>  		gpiod_set_value_cansleep(qcadev->bt_en, 0);
> >>  		msleep(100);
> >>  		qca_regulator_disable(qcadev);
> >> @@ -2333,6 +2359,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >>  	case QCA_QCA6390:
> >> +	case QCA_QCA6698:
> >>  		qcadev->bt_power = devm_kzalloc(&serdev->dev,
> >>  						sizeof(struct qca_power),
> >>  						GFP_KERNEL);
> >> @@ -2346,6 +2373,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>  	switch (qcadev->btsoc_type) {
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		if (!device_property_present(&serdev->dev, "enable-gpios")) {
> >>  			/*
> >>  			 * Backward compatibility with old DT sources. If the
> >> @@ -2380,7 +2408,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>  					       GPIOD_OUT_LOW);
> >>  		if (IS_ERR(qcadev->bt_en) &&
> >>  		    (data->soc_type == QCA_WCN6750 ||
> >> -		     data->soc_type == QCA_WCN6855)) {
> >> +		     data->soc_type == QCA_WCN6855 ||
> >> +		     data->soc_type == QCA_QCA6698)) {
> >>  			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
> >>  			return PTR_ERR(qcadev->bt_en);
> >>  		}
> >> @@ -2393,7 +2422,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>  		if (IS_ERR(qcadev->sw_ctrl) &&
> >>  		    (data->soc_type == QCA_WCN6750 ||
> >>  		     data->soc_type == QCA_WCN6855 ||
> >> -		     data->soc_type == QCA_WCN7850)) {
> >> +		     data->soc_type == QCA_WCN7850 ||
> >> +		     data->soc_type == QCA_QCA6698)) {
> >>  			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
> >>  			return PTR_ERR(qcadev->sw_ctrl);
> >>  		}
> >> @@ -2475,6 +2505,7 @@ static void qca_serdev_remove(struct serdev_device *serdev)
> >>  	case QCA_WCN6750:
> >>  	case QCA_WCN6855:
> >>  	case QCA_WCN7850:
> >> +	case QCA_QCA6698:
> >>  		if (power->vregs_on)
> >>  			qca_power_shutdown(&qcadev->serdev_hu);
> >>  		break;
> >> @@ -2669,6 +2700,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
> >>  	{ .compatible = "qcom,qca2066-bt", .data = &qca_soc_data_qca2066},
> >>  	{ .compatible = "qcom,qca6174-bt" },
> >>  	{ .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390},
> >> +	{ .compatible = "qcom,qca6698-bt", .data = &qca_soc_data_qca6698},
> >>  	{ .compatible = "qcom,qca9377-bt" },
> >>  	{ .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988},
> >>  	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
> >> -- 
> >> 2.25.1
> >>
> > 
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] dt-bindings: net: Add QCA6698 Bluetooth

[Cheng Jiang (IOE)]

Hi Dmitry,

On 11/30/2024 1:13 AM, Dmitry Baryshkov wrote:
> On Fri, Nov 29, 2024 at 10:30:00AM +0800, Cheng Jiang (IOE) wrote:
>> Hi Dmitry,
>>
>> On 11/28/2024 8:58 PM, Dmitry Baryshkov wrote:
>>> On Thu, Nov 28, 2024 at 08:09:21PM +0800, Cheng Jiang wrote:
>>>> Add the compatible for the Bluetooth part of the Qualcomm QCA6698 chipset.
>>>
>>> ... 
>>> And you have misssed to explain why do you need to add it and how it is
>>> different from WCN6855.
>>>
>> Got it. I just explain in the dts/driver change, forget to explain here. 
>>
>> If use the firmware-name solution, do we still need add the new compatible
>> string for qcom,qca6698-bt here? The driver may not use this string.   
> 
> DT describes the hardware. If you want, you can still add new string
> _and_ use old one as a fallback compatible: "qcom,qca6698-bt",
> "qcom,wcn6855-bt".
Got it. Thanks! 
> 
>>>>
>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>> ---
>>>>  .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> index 7bb68311c..82105382a 100644
>>>> --- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
>>>> @@ -18,6 +18,7 @@ properties:
>>>>      enum:
>>>>        - qcom,qca2066-bt
>>>>        - qcom,qca6174-bt
>>>> +      - qcom,qca6698-bt
>>>>        - qcom,qca9377-bt
>>>>        - qcom,wcn3988-bt
>>>>        - qcom,wcn3990-bt
>>>> @@ -170,6 +171,7 @@ allOf:
>>>>            contains:
>>>>              enum:
>>>>                - qcom,wcn6855-bt
>>>> +              - qcom,qca6698-bt
>>>>      then:
>>>>        required:
>>>>          - vddrfacmn-supply
>>>> -- 
>>>> 2.25.1
>>>>
>>>
>>
>