[PATCH RFC 0/4] Add WCN3988 Bluetooth support for Fairphone 4

[PATCH RFC 0/4] Add WCN3988 Bluetooth support for Fairphone 4

[Luca Weiss]

Just to start with the important part why this is an RFC:

While Bluetooth chip init works totally fine and bluez seems to be
fairly happy with it, there's a (major) problem with scanning, as shown
with this bluetoothctl snippet and dmesg snippet:

  [bluetooth]# scan on
  Failed to start discovery: org.bluez.Error.InProgress

  [  202.371374] Bluetooth: hci0: Opcode 0x200b failed: -16

This opcode should be the following:

  include/net/bluetooth/hci.h:#define HCI_OP_LE_SET_SCAN_PARAM    0x200b

Unfortunately trying various existing code branches in the Bluetooth
driver doesn't show any sign of making this work and I don't really know
where to look to debug this further.

On the other hand "discoverable on" makes the device show up on other
devices during scanning , so the RF parts of the Bluetooth chip are
generally functional for sure.

Any ideas are welcome.

@Bjorn: Patch "arm64: dts: qcom: sm6350: add uart1 node" should be fine
to take regardless the RFC status, I don't think the problem is caused
there.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
Luca Weiss (4):
      dt-bindings: net: qualcomm: Add WCN3988
      Bluetooth: btqca: Add WCN3988 support
      arm64: dts: qcom: sm6350: add uart1 node
      arm64: dts: qcom: sm7225-fairphone-fp4: Add Bluetooth

 .../bindings/net/bluetooth/qualcomm-bluetooth.yaml |  2 +
 arch/arm64/boot/dts/qcom/sm6350.dtsi               | 63 ++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts  | 17 ++++++
 drivers/bluetooth/btqca.c                          | 13 ++++-
 drivers/bluetooth/btqca.h                          | 12 ++++-
 drivers/bluetooth/hci_qca.c                        | 12 +++++
 6 files changed, 115 insertions(+), 4 deletions(-)
---
base-commit: cf4c0112a0350cfe8a63b5eb3377e2366f57545b
change-id: 20230421-fp4-bluetooth-b36a0e87b9c8

Best regards,
-- 
Luca Weiss <luca.weiss@fairphone.com>

[PATCH RFC 1/4] dt-bindings: net: qualcomm: Add WCN3988

[Luca Weiss]

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

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 Documentation/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 68f78b90d23a..7a53e05ae50d 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,qca6174-bt
       - qcom,qca9377-bt
+      - qcom,wcn3988-bt
       - qcom,wcn3990-bt
       - qcom,wcn3991-bt
       - qcom,wcn3998-bt
@@ -106,6 +107,7 @@ allOf:
         compatible:
           contains:
             enum:
+              - qcom,wcn3988-bt
               - qcom,wcn3990-bt
               - qcom,wcn3991-bt
               - qcom,wcn3998-bt

-- 
2.40.0

[PATCH RFC 2/4] Bluetooth: btqca: Add WCN3988 support

[Luca Weiss]

Add support for the Bluetooth chip codenamed APACHE which is part of
WCN3988.

The firmware for this chip has a slightly different naming scheme
compared to most others. For ROM Version 0x0200 we need to use
apbtfw10.tlv + apnv10.bin and for ROM version 0x201 apbtfw11.tlv +
apnv11.bin

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 drivers/bluetooth/btqca.c   | 13 +++++++++++--
 drivers/bluetooth/btqca.h   | 12 ++++++++++--
 drivers/bluetooth/hci_qca.c | 12 ++++++++++++
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index fd0941fe8608..3ee1ef88a640 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -594,14 +594,20 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	/* Firmware files to download are based on ROM version.
 	 * ROM version is derived from last two bytes of soc_ver.
 	 */
-	rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
+	if (soc_type == QCA_WCN3988)
+		rom_ver = ((soc_ver & 0x00000f00) >> 0x05) | (soc_ver & 0x0000000f);
+	else
+		rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
 
 	if (soc_type == QCA_WCN6750)
 		qca_send_patch_config_cmd(hdev);
 
 	/* Download rampatch file */
 	config.type = TLV_TYPE_PATCH;
-	if (qca_is_wcn399x(soc_type)) {
+	if (soc_type == QCA_WCN3988) {
+		snprintf(config.fwname, sizeof(config.fwname),
+			 "qca/apbtfw%02x.tlv", rom_ver);
+	} else if (qca_is_wcn399x(soc_type)) {
 		snprintf(config.fwname, sizeof(config.fwname),
 			 "qca/crbtfw%02x.tlv", rom_ver);
 	} else if (soc_type == QCA_QCA6390) {
@@ -636,6 +642,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	if (firmware_name)
 		snprintf(config.fwname, sizeof(config.fwname),
 			 "qca/%s", firmware_name);
+	else if (soc_type == QCA_WCN3988)
+		snprintf(config.fwname, sizeof(config.fwname),
+			 "qca/apnv%02x.bin", rom_ver);
 	else if (qca_is_wcn399x(soc_type)) {
 		if (ver.soc_id == QCA_WCN3991_SOC_ID) {
 			snprintf(config.fwname, sizeof(config.fwname),
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index b884095bcd9d..fc6cf314eb0e 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -142,6 +142,7 @@ enum qca_btsoc_type {
 	QCA_INVALID = -1,
 	QCA_AR3002,
 	QCA_ROME,
+	QCA_WCN3988,
 	QCA_WCN3990,
 	QCA_WCN3998,
 	QCA_WCN3991,
@@ -162,8 +163,15 @@ int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
 int qca_send_pre_shutdown_cmd(struct hci_dev *hdev);
 static inline bool qca_is_wcn399x(enum qca_btsoc_type soc_type)
 {
-	return soc_type == QCA_WCN3990 || soc_type == QCA_WCN3991 ||
-	       soc_type == QCA_WCN3998;
+	switch (soc_type) {
+	case QCA_WCN3988:
+	case QCA_WCN3990:
+	case QCA_WCN3991:
+	case QCA_WCN3998:
+		return true;
+	default:
+		return false;
+	}
 }
 static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
 {
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 1597797ff169..96b837410a6b 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1835,6 +1835,17 @@ static const struct hci_uart_proto qca_proto = {
 	.dequeue	= qca_dequeue,
 };
 
+static const struct qca_device_data qca_soc_data_wcn3988 __maybe_unused = {
+	.soc_type = QCA_WCN3988,
+	.vregs = (struct qca_vreg []) {
+		{ "vddio", 15000  },
+		{ "vddxo", 80000  },
+		{ "vddrf", 300000 },
+		{ "vddch0", 450000 },
+	},
+	.num_vregs = 4,
+};
+
 static const struct qca_device_data qca_soc_data_wcn3990 __maybe_unused = {
 	.soc_type = QCA_WCN3990,
 	.vregs = (struct qca_vreg []) {
@@ -2359,6 +2370,7 @@ 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,qca9377-bt" },
+	{ .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988},
 	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990},
 	{ .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991},
 	{ .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998},

-- 
2.40.0

[PATCH RFC 3/4] arm64: dts: qcom: sm6350: add uart1 node

[Luca Weiss]

Add the node describing uart1 incl. opp table and pinctrl.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
index 18c4616848ce..16c5e9a6c98a 100644
--- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
@@ -378,6 +378,25 @@ opp-2073600000 {
 		};
 	};
 
+	qup_opp_table: opp-table-qup {
+		compatible = "operating-points-v2";
+
+		opp-75000000 {
+			opp-hz = /bits/ 64 <75000000>;
+			required-opps = <&rpmhpd_opp_low_svs>;
+		};
+
+		opp-100000000 {
+			opp-hz = /bits/ 64 <100000000>;
+			required-opps = <&rpmhpd_opp_svs>;
+		};
+
+		opp-128000000 {
+			opp-hz = /bits/ 64 <128000000>;
+			required-opps = <&rpmhpd_opp_nom>;
+		};
+	};
+
 	pmu {
 		compatible = "arm,armv8-pmuv3";
 		interrupts = <GIC_PPI 5 IRQ_TYPE_LEVEL_LOW>;
@@ -741,6 +760,22 @@ i2c0: i2c@880000 {
 				status = "disabled";
 			};
 
+			uart1: serial@884000 {
+				compatible = "qcom,geni-uart";
+				reg = <0 0x00884000 0 0x4000>;
+				clock-names = "se";
+				clocks = <&gcc GCC_QUPV3_WRAP0_S1_CLK>;
+				pinctrl-names = "default";
+				pinctrl-0 = <&qup_uart1_cts>, <&qup_uart1_rts>, <&qup_uart1_tx>, <&qup_uart1_rx>;
+				interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SM6350_CX>;
+				operating-points-v2 = <&qup_opp_table>;
+				interconnects = <&clk_virt MASTER_QUP_CORE_0 0 &clk_virt SLAVE_QUP_CORE_0 0>,
+						<&aggre1_noc MASTER_QUP_0 0 &clk_virt SLAVE_EBI_CH0 0>;
+				interconnect-names = "qup-core", "qup-config";
+				status = "disabled";
+			};
+
 			i2c2: i2c@888000 {
 				compatible = "qcom,geni-i2c";
 				reg = <0 0x00888000 0 0x4000>;
@@ -1726,6 +1761,34 @@ qup_i2c10_default: qup-i2c10-default-state {
 				drive-strength = <2>;
 				bias-pull-up;
 			};
+
+			qup_uart1_cts: qup-uart1-cts-default-state {
+				pins = "gpio61";
+				function = "qup01";
+				drive-strength = <2>;
+				bias-disable;
+			};
+
+			qup_uart1_rts: qup-uart1-rts-default-state {
+				pins = "gpio62";
+				function = "qup01";
+				drive-strength = <2>;
+				bias-pull-down;
+			};
+
+			qup_uart1_tx: qup-uart1-tx-default-state {
+				pins = "gpio63";
+				function = "qup01";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
+
+			qup_uart1_rx: qup-uart1-rx-default-state {
+				pins = "gpio64";
+				function = "qup01";
+				drive-strength = <2>;
+				bias-disable;
+			};
 		};
 
 		apps_smmu: iommu@15000000 {

-- 
2.40.0

[PATCH RFC 4/4] arm64: dts: qcom: sm7225-fairphone-fp4: Add Bluetooth

[Luca Weiss]

The device has a WCN3988 chip for WiFi and Bluetooth. Configure the
Bluetooth node and enable the UART it is connected to.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
index 7ae6aba5d2ec..35e2889c5439 100644
--- a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
+++ b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
@@ -31,6 +31,7 @@ / {
 
 	aliases {
 		serial0 = &uart9;
+		serial1 = &uart1;
 	};
 
 	chosen {
@@ -563,6 +564,22 @@ &tlmm {
 	gpio-reserved-ranges = <13 4>, <56 2>;
 };
 
+&uart1 {
+	status = "okay";
+
+	bluetooth {
+		compatible = "qcom,wcn3988-bt";
+
+		vddio-supply = <&vreg_l11a>;
+		vddxo-supply = <&vreg_l7a>;
+		vddrf-supply = <&vreg_l2e>;
+		vddch0-supply = <&vreg_l10e>;
+		swctrl-gpios = <&tlmm 69 GPIO_ACTIVE_HIGH>;
+
+		max-speed = <3200000>;
+	};
+};
+
 &uart9 {
 	status = "okay";
 };

-- 
2.40.0

Re: [PATCH RFC 3/4] arm64: dts: qcom: sm6350: add uart1 node

[Steev Klimaszewski]

On Fri, Apr 21, 2023 at 9:12 AM Luca Weiss <luca.weiss@fairphone.com> wrote:
>
> Add the node describing uart1 incl. opp table and pinctrl.
>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> index 18c4616848ce..16c5e9a6c98a 100644
> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> @@ -378,6 +378,25 @@ opp-2073600000 {
>                 };
>         };
>
> +       qup_opp_table: opp-table-qup {
> +               compatible = "operating-points-v2";
> +
> +               opp-75000000 {
> +                       opp-hz = /bits/ 64 <75000000>;
> +                       required-opps = <&rpmhpd_opp_low_svs>;
> +               };
> +
> +               opp-100000000 {
> +                       opp-hz = /bits/ 64 <100000000>;
> +                       required-opps = <&rpmhpd_opp_svs>;
> +               };
> +
> +               opp-128000000 {
> +                       opp-hz = /bits/ 64 <128000000>;
> +                       required-opps = <&rpmhpd_opp_nom>;
> +               };
> +       };
> +
>         pmu {
>                 compatible = "arm,armv8-pmuv3";
>                 interrupts = <GIC_PPI 5 IRQ_TYPE_LEVEL_LOW>;
> @@ -741,6 +760,22 @@ i2c0: i2c@880000 {
>                                 status = "disabled";
>                         };
>
> +                       uart1: serial@884000 {
> +                               compatible = "qcom,geni-uart";
> +                               reg = <0 0x00884000 0 0x4000>;
> +                               clock-names = "se";
> +                               clocks = <&gcc GCC_QUPV3_WRAP0_S1_CLK>;
> +                               pinctrl-names = "default";
> +                               pinctrl-0 = <&qup_uart1_cts>, <&qup_uart1_rts>, <&qup_uart1_tx>, <&qup_uart1_rx>;
> +                               interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>;
> +                               power-domains = <&rpmhpd SM6350_CX>;
> +                               operating-points-v2 = <&qup_opp_table>;
> +                               interconnects = <&clk_virt MASTER_QUP_CORE_0 0 &clk_virt SLAVE_QUP_CORE_0 0>,
> +                                               <&aggre1_noc MASTER_QUP_0 0 &clk_virt SLAVE_EBI_CH0 0>;
> +                               interconnect-names = "qup-core", "qup-config";
> +                               status = "disabled";
> +                       };
> +
>                         i2c2: i2c@888000 {
>                                 compatible = "qcom,geni-i2c";
>                                 reg = <0 0x00888000 0 0x4000>;
> @@ -1726,6 +1761,34 @@ qup_i2c10_default: qup-i2c10-default-state {
>                                 drive-strength = <2>;
>                                 bias-pull-up;
>                         };
> +
> +                       qup_uart1_cts: qup-uart1-cts-default-state {
> +                               pins = "gpio61";
> +                               function = "qup01";
> +                               drive-strength = <2>;
> +                               bias-disable;
> +                       };
> +
> +                       qup_uart1_rts: qup-uart1-rts-default-state {
> +                               pins = "gpio62";
> +                               function = "qup01";
> +                               drive-strength = <2>;
> +                               bias-pull-down;
> +                       };
> +
> +                       qup_uart1_tx: qup-uart1-tx-default-state {
> +                               pins = "gpio63";
> +                               function = "qup01";
> +                               drive-strength = <2>;
> +                               bias-pull-up;
> +                       };
> +
tx should come after the rx, this caught me too when I was doing my
bluetooth driver, it goes by name, not gpio#.

> +                       qup_uart1_rx: qup-uart1-rx-default-state {
> +                               pins = "gpio64";
> +                               function = "qup01";
> +                               drive-strength = <2>;
> +                               bias-disable;
> +                       };
>                 };
>
>                 apps_smmu: iommu@15000000 {
>
> --
> 2.40.0
>

Re: [PATCH RFC 0/4] Add WCN3988 Bluetooth support for Fairphone 4

[Konrad Dybcio]

On 21.04.2023 16:11, Luca Weiss wrote:
> Just to start with the important part why this is an RFC:
> 
> While Bluetooth chip init works totally fine and bluez seems to be
> fairly happy with it, there's a (major) problem with scanning, as shown
> with this bluetoothctl snippet and dmesg snippet:
> 
>   [bluetooth]# scan on
>   Failed to start discovery: org.bluez.Error.InProgress
> 
>   [  202.371374] Bluetooth: hci0: Opcode 0x200b failed: -16
> 
> This opcode should be the following:
> 
>   include/net/bluetooth/hci.h:#define HCI_OP_LE_SET_SCAN_PARAM    0x200b
Not a bluetooth expert or anything, but does that thing support
bluetooth LE?

Konrad
> 
> Unfortunately trying various existing code branches in the Bluetooth
> driver doesn't show any sign of making this work and I don't really know
> where to look to debug this further.
> 
> On the other hand "discoverable on" makes the device show up on other
> devices during scanning , so the RF parts of the Bluetooth chip are
> generally functional for sure.
> 
> Any ideas are welcome.
> 
> @Bjorn: Patch "arm64: dts: qcom: sm6350: add uart1 node" should be fine
> to take regardless the RFC status, I don't think the problem is caused
> there.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
> Luca Weiss (4):
>       dt-bindings: net: qualcomm: Add WCN3988
>       Bluetooth: btqca: Add WCN3988 support
>       arm64: dts: qcom: sm6350: add uart1 node
>       arm64: dts: qcom: sm7225-fairphone-fp4: Add Bluetooth
> 
>  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml |  2 +
>  arch/arm64/boot/dts/qcom/sm6350.dtsi               | 63 ++++++++++++++++++++++
>  arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts  | 17 ++++++
>  drivers/bluetooth/btqca.c                          | 13 ++++-
>  drivers/bluetooth/btqca.h                          | 12 ++++-
>  drivers/bluetooth/hci_qca.c                        | 12 +++++
>  6 files changed, 115 insertions(+), 4 deletions(-)
> ---
> base-commit: cf4c0112a0350cfe8a63b5eb3377e2366f57545b
> change-id: 20230421-fp4-bluetooth-b36a0e87b9c8
> 
> Best regards,

Re: [PATCH RFC 1/4] dt-bindings: net: qualcomm: Add WCN3988

[Krzysztof Kozlowski]

On 21/04/2023 16:11, Luca Weiss wrote:
> Add the compatible for the Bluetooth part of the Qualcomm WCN3988
> chipset.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH RFC 3/4] arm64: dts: qcom: sm6350: add uart1 node

[Krzysztof Kozlowski]

On 21/04/2023 16:11, Luca Weiss wrote:
> Add the node describing uart1 incl. opp table and pinctrl.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)

Please do not send DTS patches for net-next. DTS must go via Qualcomm
SoC. Split the series and mention where is the bindings change in DTS
patchset.


Best regards,
Krzysztof

Re: [PATCH RFC 0/4] Add WCN3988 Bluetooth support for Fairphone 4

[Luca Weiss]

On Sat Apr 22, 2023 at 2:03 PM CEST, Konrad Dybcio wrote:
>
>
> On 21.04.2023 16:11, Luca Weiss wrote:
> > Just to start with the important part why this is an RFC:
> > 
> > While Bluetooth chip init works totally fine and bluez seems to be
> > fairly happy with it, there's a (major) problem with scanning, as shown
> > with this bluetoothctl snippet and dmesg snippet:
> > 
> >   [bluetooth]# scan on
> >   Failed to start discovery: org.bluez.Error.InProgress
> > 
> >   [  202.371374] Bluetooth: hci0: Opcode 0x200b failed: -16
> > 
> > This opcode should be the following:
> > 
> >   include/net/bluetooth/hci.h:#define HCI_OP_LE_SET_SCAN_PARAM    0x200b
> Not a bluetooth expert or anything, but does that thing support
> bluetooth LE?

I don't know too much about Bluetooth details either, but hasn't
Bluetooth LE been a consistently supported thing since like 10 years?

All the info I can easily find just states SM7225 SoC supports
"Bluetooth 5.1".

Regards
Luca

>
> Konrad
> > 
> > Unfortunately trying various existing code branches in the Bluetooth
> > driver doesn't show any sign of making this work and I don't really know
> > where to look to debug this further.
> > 
> > On the other hand "discoverable on" makes the device show up on other
> > devices during scanning , so the RF parts of the Bluetooth chip are
> > generally functional for sure.
> > 
> > Any ideas are welcome.
> > 
> > @Bjorn: Patch "arm64: dts: qcom: sm6350: add uart1 node" should be fine
> > to take regardless the RFC status, I don't think the problem is caused
> > there.
> > 
> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > ---
> > Luca Weiss (4):
> >       dt-bindings: net: qualcomm: Add WCN3988
> >       Bluetooth: btqca: Add WCN3988 support
> >       arm64: dts: qcom: sm6350: add uart1 node
> >       arm64: dts: qcom: sm7225-fairphone-fp4: Add Bluetooth
> > 
> >  .../bindings/net/bluetooth/qualcomm-bluetooth.yaml |  2 +
> >  arch/arm64/boot/dts/qcom/sm6350.dtsi               | 63 ++++++++++++++++++++++
> >  arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts  | 17 ++++++
> >  drivers/bluetooth/btqca.c                          | 13 ++++-
> >  drivers/bluetooth/btqca.h                          | 12 ++++-
> >  drivers/bluetooth/hci_qca.c                        | 12 +++++
> >  6 files changed, 115 insertions(+), 4 deletions(-)
> > ---
> > base-commit: cf4c0112a0350cfe8a63b5eb3377e2366f57545b
> > change-id: 20230421-fp4-bluetooth-b36a0e87b9c8
> > 
> > Best regards,

Re: [PATCH RFC 2/4] Bluetooth: btqca: Add WCN3988 support

[Simon Horman]

On Fri, Apr 21, 2023 at 04:11:39PM +0200, Luca Weiss wrote:
> Add support for the Bluetooth chip codenamed APACHE which is part of
> WCN3988.
> 
> The firmware for this chip has a slightly different naming scheme
> compared to most others. For ROM Version 0x0200 we need to use
> apbtfw10.tlv + apnv10.bin and for ROM version 0x201 apbtfw11.tlv +
> apnv11.bin
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  drivers/bluetooth/btqca.c   | 13 +++++++++++--
>  drivers/bluetooth/btqca.h   | 12 ++++++++++--
>  drivers/bluetooth/hci_qca.c | 12 ++++++++++++
>  3 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index fd0941fe8608..3ee1ef88a640 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -594,14 +594,20 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	/* Firmware files to download are based on ROM version.
>  	 * ROM version is derived from last two bytes of soc_ver.
>  	 */
> -	rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
> +	if (soc_type == QCA_WCN3988)
> +		rom_ver = ((soc_ver & 0x00000f00) >> 0x05) | (soc_ver & 0x0000000f);
> +	else
> +		rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);

Hi Luca,

perhaps it's just me. But I was wondering if this can be improved on a little.

* Move the common portion outside of the conditional
* And also, I think it's normal to use decimal for shift values.

e.g.
	unsigned shift;
	...

	shift = soc_type == QCA_WCN3988 ? 5 : 4;
	rom_ver = ((soc_ver & 0x00000f00) >> shift) | (soc_ver & 0x0000000f);

Using some helpers such as GENMASK and FIELD_PREP might also be nice.

>  
>  	if (soc_type == QCA_WCN6750)
>  		qca_send_patch_config_cmd(hdev);
>  
>  	/* Download rampatch file */
>  	config.type = TLV_TYPE_PATCH;
> -	if (qca_is_wcn399x(soc_type)) {
> +	if (soc_type == QCA_WCN3988) {
> +		snprintf(config.fwname, sizeof(config.fwname),
> +			 "qca/apbtfw%02x.tlv", rom_ver);
> +	} else if (qca_is_wcn399x(soc_type)) {
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/crbtfw%02x.tlv", rom_ver);
>  	} else if (soc_type == QCA_QCA6390) {
> @@ -636,6 +642,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	if (firmware_name)
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/%s", firmware_name);
> +	else if (soc_type == QCA_WCN3988)
> +		snprintf(config.fwname, sizeof(config.fwname),
> +			 "qca/apnv%02x.bin", rom_ver);
>  	else if (qca_is_wcn399x(soc_type)) {
>  		if (ver.soc_id == QCA_WCN3991_SOC_ID) {

Not strictly related to this patch, but while reviewing this I noticed that
ver.soc_id is __le32 but QCA_WCN3991_SOC_ID is in host byteorder.

Perhaps a cpu_to_le32() or le32_to_cpu() call is in order here?

>  			snprintf(config.fwname, sizeof(config.fwname),

...

Re: [PATCH RFC 2/4] Bluetooth: btqca: Add WCN3988 support

[Luca Weiss]

Hi Simon,

On Mon May 1, 2023 at 3:11 PM CEST, Simon Horman wrote:
> On Fri, Apr 21, 2023 at 04:11:39PM +0200, Luca Weiss wrote:
> > Add support for the Bluetooth chip codenamed APACHE which is part of
> > WCN3988.
> > 
> > The firmware for this chip has a slightly different naming scheme
> > compared to most others. For ROM Version 0x0200 we need to use
> > apbtfw10.tlv + apnv10.bin and for ROM version 0x201 apbtfw11.tlv +
> > apnv11.bin
> > 
> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > ---
> >  drivers/bluetooth/btqca.c   | 13 +++++++++++--
> >  drivers/bluetooth/btqca.h   | 12 ++++++++++--
> >  drivers/bluetooth/hci_qca.c | 12 ++++++++++++
> >  3 files changed, 33 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > index fd0941fe8608..3ee1ef88a640 100644
> > --- a/drivers/bluetooth/btqca.c
> > +++ b/drivers/bluetooth/btqca.c
> > @@ -594,14 +594,20 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >  	/* Firmware files to download are based on ROM version.
> >  	 * ROM version is derived from last two bytes of soc_ver.
> >  	 */
> > -	rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
> > +	if (soc_type == QCA_WCN3988)
> > +		rom_ver = ((soc_ver & 0x00000f00) >> 0x05) | (soc_ver & 0x0000000f);
> > +	else
> > +		rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
>
> Hi Luca,
>
> perhaps it's just me. But I was wondering if this can be improved on a little.
>
> * Move the common portion outside of the conditional
> * And also, I think it's normal to use decimal for shift values.
>
> e.g.
> 	unsigned shift;
> 	...
>
> 	shift = soc_type == QCA_WCN3988 ? 5 : 4;
> 	rom_ver = ((soc_ver & 0x00000f00) >> shift) | (soc_ver & 0x0000000f);
>
> Using some helpers such as GENMASK and FIELD_PREP might also be nice.

While I'm not opposed to the idea, I'm not sure it's worth making
beautiful macros for this since - to my eyes - how the mapping of
soc_ver to firmware name works is rather obscure since the sources from
Qualcomm just have a static lookup table of soc_ver to firmware name so
doing this dynamically like here is different.

And I haven't looked at other chips that are covered there to see if
there's a pattern to this, for the most part it seems the original
formula works for most chips and the one I added works for WCN3988 (and
the other "APACHE" chips, whatever they are).

If a third way is added then I would say for sure this line should be
made nicer but for now I think it's easier to keep this as I sent it
because we don't know what the future will hold.

>
> >  
> >  	if (soc_type == QCA_WCN6750)
> >  		qca_send_patch_config_cmd(hdev);
> >  
> >  	/* Download rampatch file */
> >  	config.type = TLV_TYPE_PATCH;
> > -	if (qca_is_wcn399x(soc_type)) {
> > +	if (soc_type == QCA_WCN3988) {
> > +		snprintf(config.fwname, sizeof(config.fwname),
> > +			 "qca/apbtfw%02x.tlv", rom_ver);
> > +	} else if (qca_is_wcn399x(soc_type)) {
> >  		snprintf(config.fwname, sizeof(config.fwname),
> >  			 "qca/crbtfw%02x.tlv", rom_ver);
> >  	} else if (soc_type == QCA_QCA6390) {
> > @@ -636,6 +642,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >  	if (firmware_name)
> >  		snprintf(config.fwname, sizeof(config.fwname),
> >  			 "qca/%s", firmware_name);
> > +	else if (soc_type == QCA_WCN3988)
> > +		snprintf(config.fwname, sizeof(config.fwname),
> > +			 "qca/apnv%02x.bin", rom_ver);
> >  	else if (qca_is_wcn399x(soc_type)) {
> >  		if (ver.soc_id == QCA_WCN3991_SOC_ID) {
>
> Not strictly related to this patch, but while reviewing this I noticed that
> ver.soc_id is __le32 but QCA_WCN3991_SOC_ID is in host byteorder.
>
> Perhaps a cpu_to_le32() or le32_to_cpu() call is in order here?

Good catch, as you've seen I sent a patch separately to fix that. :)

Regards
Luca

>
> >  			snprintf(config.fwname, sizeof(config.fwname),
>
> ...

Re: [PATCH RFC 3/4] arm64: dts: qcom: sm6350: add uart1 node

[Luca Weiss]

On Sun Apr 23, 2023 at 12:51 PM CEST, Krzysztof Kozlowski wrote:
> On 21/04/2023 16:11, Luca Weiss wrote:
> > Add the node describing uart1 incl. opp table and pinctrl.
> > 
> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > ---
> >  arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
>
> Please do not send DTS patches for net-next. DTS must go via Qualcomm
> SoC. Split the series and mention where is the bindings change in DTS
> patchset.

Sorry, just saw now after already sending v2.

Is this a special rule for linux-bluetooth@ / netdev@? Isn't it easier
to keep it together so the status of series can be assessed easier? I've
always submitted patches by topic, like input patches + dts patches and
it was never mentioned.

Regards
Luca

>
>
> Best regards,
> Krzysztof

Re: [PATCH RFC 3/4] arm64: dts: qcom: sm6350: add uart1 node

[Krzysztof Kozlowski]

On 12/05/2023 16:30, Luca Weiss wrote:
> On Sun Apr 23, 2023 at 12:51 PM CEST, Krzysztof Kozlowski wrote:
>> On 21/04/2023 16:11, Luca Weiss wrote:
>>> Add the node describing uart1 incl. opp table and pinctrl.
>>>
>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>> ---
>>>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 63 insertions(+)
>>
>> Please do not send DTS patches for net-next. DTS must go via Qualcomm
>> SoC. Split the series and mention where is the bindings change in DTS
>> patchset.
> 
> Sorry, just saw now after already sending v2.
> 
> Is this a special rule for linux-bluetooth@ / netdev@? Isn't it easier
> to keep it together so the status of series can be assessed easier? I've
> always submitted patches by topic, like input patches + dts patches and
> it was never mentioned.

The rule that DTS must go via Qualcomm SoC (arm-soc) was there always,
but other maintainers often do not pay attention to this. I don't blame
them, don't get me wrong. I am just stating the observed actions.
Usually netdev folks and Greg will take everything you throw at them, so
for these subsystems it is recommended to split DTS to different patchset.

For other maintainers it is usually also more useful to split, because
then they can apply entire patchset with one command, instead of picking
up specific patches (omitting DTS).

Best regards,
Krzysztof

Re: [PATCH RFC 2/4] Bluetooth: btqca: Add WCN3988 support

[Simon Horman]

On Fri, May 12, 2023 at 01:14:18PM +0200, Luca Weiss wrote:
> Hi Simon,
> 
> On Mon May 1, 2023 at 3:11 PM CEST, Simon Horman wrote:
> > On Fri, Apr 21, 2023 at 04:11:39PM +0200, Luca Weiss wrote:
> > > Add support for the Bluetooth chip codenamed APACHE which is part of
> > > WCN3988.
> > > 
> > > The firmware for this chip has a slightly different naming scheme
> > > compared to most others. For ROM Version 0x0200 we need to use
> > > apbtfw10.tlv + apnv10.bin and for ROM version 0x201 apbtfw11.tlv +
> > > apnv11.bin
> > > 
> > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > > ---
> > >  drivers/bluetooth/btqca.c   | 13 +++++++++++--
> > >  drivers/bluetooth/btqca.h   | 12 ++++++++++--
> > >  drivers/bluetooth/hci_qca.c | 12 ++++++++++++
> > >  3 files changed, 33 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > > index fd0941fe8608..3ee1ef88a640 100644
> > > --- a/drivers/bluetooth/btqca.c
> > > +++ b/drivers/bluetooth/btqca.c
> > > @@ -594,14 +594,20 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> > >  	/* Firmware files to download are based on ROM version.
> > >  	 * ROM version is derived from last two bytes of soc_ver.
> > >  	 */
> > > -	rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
> > > +	if (soc_type == QCA_WCN3988)
> > > +		rom_ver = ((soc_ver & 0x00000f00) >> 0x05) | (soc_ver & 0x0000000f);
> > > +	else
> > > +		rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f);
> >
> > Hi Luca,
> >
> > perhaps it's just me. But I was wondering if this can be improved on a little.
> >
> > * Move the common portion outside of the conditional
> > * And also, I think it's normal to use decimal for shift values.
> >
> > e.g.
> > 	unsigned shift;
> > 	...
> >
> > 	shift = soc_type == QCA_WCN3988 ? 5 : 4;
> > 	rom_ver = ((soc_ver & 0x00000f00) >> shift) | (soc_ver & 0x0000000f);
> >
> > Using some helpers such as GENMASK and FIELD_PREP might also be nice.
> 
> While I'm not opposed to the idea, I'm not sure it's worth making
> beautiful macros for this since - to my eyes - how the mapping of
> soc_ver to firmware name works is rather obscure since the sources from
> Qualcomm just have a static lookup table of soc_ver to firmware name so
> doing this dynamically like here is different.
> 
> And I haven't looked at other chips that are covered there to see if
> there's a pattern to this, for the most part it seems the original
> formula works for most chips and the one I added works for WCN3988 (and
> the other "APACHE" chips, whatever they are).
> 
> If a third way is added then I would say for sure this line should be
> made nicer but for now I think it's easier to keep this as I sent it
> because we don't know what the future will hold.

Thanks. My feeling is that my suggestion mainly makes sense
if it lease to improved readability and maintainability.
It sounds like that might not be the case here.

> > >  	if (soc_type == QCA_WCN6750)
> > >  		qca_send_patch_config_cmd(hdev);
> > >  
> > >  	/* Download rampatch file */
> > >  	config.type = TLV_TYPE_PATCH;
> > > -	if (qca_is_wcn399x(soc_type)) {
> > > +	if (soc_type == QCA_WCN3988) {
> > > +		snprintf(config.fwname, sizeof(config.fwname),
> > > +			 "qca/apbtfw%02x.tlv", rom_ver);
> > > +	} else if (qca_is_wcn399x(soc_type)) {
> > >  		snprintf(config.fwname, sizeof(config.fwname),
> > >  			 "qca/crbtfw%02x.tlv", rom_ver);
> > >  	} else if (soc_type == QCA_QCA6390) {
> > > @@ -636,6 +642,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> > >  	if (firmware_name)
> > >  		snprintf(config.fwname, sizeof(config.fwname),
> > >  			 "qca/%s", firmware_name);
> > > +	else if (soc_type == QCA_WCN3988)
> > > +		snprintf(config.fwname, sizeof(config.fwname),
> > > +			 "qca/apnv%02x.bin", rom_ver);
> > >  	else if (qca_is_wcn399x(soc_type)) {
> > >  		if (ver.soc_id == QCA_WCN3991_SOC_ID) {
> >
> > Not strictly related to this patch, but while reviewing this I noticed that
> > ver.soc_id is __le32 but QCA_WCN3991_SOC_ID is in host byteorder.
> >
> > Perhaps a cpu_to_le32() or le32_to_cpu() call is in order here?
> 
> Good catch, as you've seen I sent a patch separately to fix that. :)

Thanks!