[PATCH v3 0/3] Expand firmware-name property to load specific

[PATCH v3 0/3] Expand firmware-name property to load specific

[Cheng Jiang]

Expand the firmware-name property to specify the names of NVM and
rampatch firmware to load.

This update will support loading specific firmware (nvm and rampatch)
for certain chips, like the QCA6698 Bluetooth chip, which shares the
same IP core as the WCN6855 but has different RF components and RAM
sizes, requiring new firmware files.

Different connectivity boards may be attached to the same platform. For
example, QCA6698-based boards can support either a two-antenna or
three-antenna solution, both of which work on the sa8775p-ride platform.
Due to differences in connectivity boards and variations in RF
performance from different foundries, different NVM configurations are
used based on the board ID.

So In firmware-name, if the NVM file has an extension, the NVM file will
be used. Otherwise, the system will first try the .bNN (board ID) file,
and if that fails, it will fall back to the .bin file.

Possible configurations:
firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
firmware-name = "QCA6698/hpnv21.bin";

---
v3:
  1.Expand firmware-name property to specify the nvm and rampatch to
  load.
  2.Change the driver to support two items in firmware-name and choose
  the NVM file according to board id if there is no extension in NVM
  file.
  3.Update the dts file to idendify the firmware for QCA6698.
---

Cheng Jiang (3):
  dt-bindings: net: bluetooth: qca: Expand firmware-name property
  Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch
  arm64: dts: qcom: sa8775p-ride: Add firmware-name in BT node

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


base-commit: c245a7a79602ccbee780c004c1e4abcda66aec32
-- 
2.25.1

[PATCH v3 1/3] dt-bindings: net: bluetooth: qca: Expand firmware-name property

[Cheng Jiang]

Expand the firmware-name property to specify the names of NVM and
rampatch firmware to load. This update will support loading specific
firmware (nvm and rampatch) for certain chips, like the QCA6698
Bluetooth chip, which shares the same IP core as the WCN6855 but has
different RF components and RAM sizes, requiring new firmware files.

We might use different connectivity boards on the same platform. For
example, QCA6698-based boards can support either a two-antenna or
three-antenna solution, both of which work on the sa8775p-ride platform.
Due to differences in connectivity boards and variations in RF
performance from different foundries, different NVM configurations are
used based on the board ID.

So In firmware-name, if the NVM file has an extension, the NVM file will
be used. Otherwise, the system will first try the .bNN (board ID) file,
and if that fails, it will fall back to the .bin file.

Possible configurations:
firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
firmware-name = "QCA6698/hpnv21.bin";

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..6d73de9cd 100644
--- a/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
+++ b/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
@@ -101,7 +101,9 @@ properties:
   max-speed: true
 
   firmware-name:
+    maxItems: 2
     description: specify the name of nvm firmware to load
+    description: specify the name of rampatch firmware to load
 
   local-bd-address: true
 
-- 
2.25.1

[PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Cheng Jiang]

The firmware-name property has been expanded to specify the names of NVM
and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
chip. Although it shares the same IP core as the WCN6855, the QCA6698
has different RF components and RAM sizes, necessitating new firmware
files. This change allows for the configuration of NVM and rampatch in
DT.

Different connectivity boards may be attached to the same platform. For
example, QCA6698-based boards can support either a two-antenna or
three-antenna solution, both of which work on the sa8775p-ride platform.
Due to differences in connectivity boards and variations in RF
performance from different foundries, different NVM configurations are
used based on the board ID.

Therefore, in the firmware-name property, if the NVM file has an
extension, the NVM file will be used. Otherwise, the system will first
try the .bNN (board ID) file, and if that fails, it will fall back to
the .bin file.

Possible configurations:
firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
firmware-name = "QCA6698/hpnv21.bin";

Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
---
 drivers/bluetooth/btqca.c   | 154 ++++++++++++++++++++++++++----------
 drivers/bluetooth/btqca.h   |   5 +-
 drivers/bluetooth/hci_qca.c |  21 ++++-
 3 files changed, 134 insertions(+), 46 deletions(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index dfbbac922..e8b89b8cc 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
 }
 EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
 
+static int qca_get_alt_nvm_path(char *path, size_t max_size)
+{
+	char fwname[64];
+	const char *suffix;
+
+	suffix = strrchr(path, '.');
+
+	if (!suffix)
+		return 0;
+
+	strscpy(fwname, path, strlen(path));
+	fwname[suffix - path] = 0;
+
+	snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
+
+	/* If nvm file is already the default one, return false to
+	 * skip the retry.
+	 */
+	if (strcmp(fwname, path) == 0)
+		return 0;
+
+	snprintf(path, max_size, "%s", fwname);
+	return 1;
+}
+
 static int qca_tlv_check_data(struct hci_dev *hdev,
 			       struct qca_fw_config *config,
 			       u8 *fw_data, size_t fw_size,
@@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
 					   config->fwname, ret);
 				return ret;
 			}
+		}
+		/* For nvm, if desired nvm file is not present and it's not the
+		 * default nvm file(ends with .bin), try to load the default nvm.
+		 */
+		else if (config->type == TLV_TYPE_NVM &&
+			 qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {
+			bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
+			ret = request_firmware(&fw, config->fwname, &hdev->dev);
+			if (ret) {
+				bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
+					   config->fwname, ret);
+				return ret;
+			}
 		} else {
 			bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
 				   config->fwname, ret);
@@ -730,15 +768,38 @@ 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_nvm_name_by_board(char *fwname, size_t max_size,
+		const char *firmware_name, struct qca_btsoc_version ver,
+		enum qca_btsoc_type soc_type, u16 bid)
+{
+	const char *variant;
+
+	/* Set the variant to empty by default */
+	variant = "";
+	/* hsp gf chip */
+	if (soc_type == QCA_WCN6855) {
+		if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
+			variant = "g";
+	}
+
+	if (bid == 0x0)
+		snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
+	else if (bid & 0xff00)
+		snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);
+	else
+		snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
+}
+
 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)
+		   const char *firmware_name, const char *rampatch_name)
 {
 	struct qca_fw_config config = {};
 	int err;
 	u8 rom_ver = 0;
 	u32 soc_ver;
 	u16 boardid = 0;
+	const char *suffix;
 
 	bt_dev_dbg(hdev, "QCA setup on UART");
 
@@ -761,44 +822,48 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 
 	/* Download rampatch file */
 	config.type = TLV_TYPE_PATCH;
-	switch (soc_type) {
-	case QCA_WCN3990:
-	case QCA_WCN3991:
-	case QCA_WCN3998:
-		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/crbtfw%02x.tlv", rom_ver);
-		break;
-	case QCA_WCN3988:
-		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/apbtfw%02x.tlv", rom_ver);
-		break;
-	case QCA_QCA2066:
-		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/hpbtfw%02x.tlv", rom_ver);
-		break;
-	case QCA_QCA6390:
-		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/htbtfw%02x.tlv", rom_ver);
-		break;
-	case QCA_WCN6750:
-		/* Choose mbn file by default.If mbn file is not found
-		 * then choose tlv file
-		 */
-		config.type = ELF_TYPE_PATCH;
-		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/msbtfw%02x.mbn", rom_ver);
-		break;
-	case QCA_WCN6855:
-		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/hpbtfw%02x.tlv", rom_ver);
-		break;
-	case QCA_WCN7850:
-		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/hmtbtfw%02x.tlv", rom_ver);
-		break;
-	default:
-		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/rampatch_%08x.bin", soc_ver);
+	if (rampatch_name) {
+		snprintf(config.fwname, sizeof(config.fwname), "qca/%s", rampatch_name);
+	} else {
+		switch (soc_type) {
+		case QCA_WCN3990:
+		case QCA_WCN3991:
+		case QCA_WCN3998:
+			snprintf(config.fwname, sizeof(config.fwname),
+				 "qca/crbtfw%02x.tlv", rom_ver);
+			break;
+		case QCA_WCN3988:
+			snprintf(config.fwname, sizeof(config.fwname),
+				 "qca/apbtfw%02x.tlv", rom_ver);
+			break;
+		case QCA_QCA2066:
+			snprintf(config.fwname, sizeof(config.fwname),
+				 "qca/hpbtfw%02x.tlv", rom_ver);
+			break;
+		case QCA_QCA6390:
+			snprintf(config.fwname, sizeof(config.fwname),
+				 "qca/htbtfw%02x.tlv", rom_ver);
+			break;
+		case QCA_WCN6750:
+			/* Choose mbn file by default.If mbn file is not found
+			 * then choose tlv file
+			 */
+			config.type = ELF_TYPE_PATCH;
+			snprintf(config.fwname, sizeof(config.fwname),
+				 "qca/msbtfw%02x.mbn", rom_ver);
+			break;
+		case QCA_WCN6855:
+			snprintf(config.fwname, sizeof(config.fwname),
+				 "qca/hpbtfw%02x.tlv", rom_ver);
+			break;
+		case QCA_WCN7850:
+			snprintf(config.fwname, sizeof(config.fwname),
+				 "qca/hmtbtfw%02x.tlv", rom_ver);
+			break;
+		default:
+			snprintf(config.fwname, sizeof(config.fwname),
+				 "qca/rampatch_%08x.bin", soc_ver);
+		}
 	}
 
 	err = qca_download_firmware(hdev, &config, soc_type, rom_ver);
@@ -816,8 +881,15 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	/* Download NVM configuration */
 	config.type = TLV_TYPE_NVM;
 	if (firmware_name) {
-		snprintf(config.fwname, sizeof(config.fwname),
-			 "qca/%s", firmware_name);
+		/* The firmware name has suffix, use it directly */
+		suffix = strrchr(firmware_name, '.');
+		if (suffix && *(suffix + 1) != '\0' && *(suffix + 1) != '.') {
+			snprintf(config.fwname, sizeof(config.fwname), "qca/%s", firmware_name);
+		} else {
+			qca_read_fw_board_id(hdev, &boardid);
+			qca_get_nvm_name_by_board(config.fwname, sizeof(config.fwname),
+				 firmware_name, ver, soc_type, boardid);
+		}
 	} else {
 		switch (soc_type) {
 		case QCA_WCN3990:
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index bb5207d7a..9d28c8800 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -161,7 +161,7 @@ enum qca_btsoc_type {
 int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
 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);
+		   const char *firmware_name, const char *rampatch_name);
 int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
 			 enum qca_btsoc_type);
 int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
@@ -176,7 +176,8 @@ static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdad
 static inline 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)
+				 const char *firmware_name,
+				 const char *rampatch_name)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 37129e6cb..c566d0bf2 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -229,6 +229,7 @@ struct qca_serdev {
 	u32 oper_speed;
 	bool bdaddr_property_broken;
 	const char *firmware_name;
+	const char *rampatch_name;
 };
 
 static int qca_regulator_enable(struct qca_serdev *qcadev);
@@ -264,6 +265,17 @@ static const char *qca_get_firmware_name(struct hci_uart *hu)
 	}
 }
 
+static const char *qca_get_rampatch_name(struct hci_uart *hu)
+{
+	if (hu->serdev) {
+		struct qca_serdev *qsd = serdev_device_get_drvdata(hu->serdev);
+
+		return qsd->rampatch_name;
+	} else {
+		return NULL;
+	}
+}
+
 static void __serial_clock_on(struct tty_struct *tty)
 {
 	/* TODO: Some chipset requires to enable UART clock on client
@@ -1855,6 +1867,7 @@ static int qca_setup(struct hci_uart *hu)
 	unsigned int retries = 0;
 	enum qca_btsoc_type soc_type = qca_soc_type(hu);
 	const char *firmware_name = qca_get_firmware_name(hu);
+	const char *rampatch_name = qca_get_rampatch_name(hu);
 	int ret;
 	struct qca_btsoc_version ver;
 	struct qca_serdev *qcadev;
@@ -1963,7 +1976,7 @@ static int qca_setup(struct hci_uart *hu)
 
 	/* Setup patch / NVM configurations */
 	ret = qca_uart_setup(hdev, qca_baudrate, soc_type, ver,
-			firmware_name);
+			firmware_name, rampatch_name);
 	if (!ret) {
 		clear_bit(QCA_IBS_DISABLED, &qca->flags);
 		qca_debugfs_init(hdev);
@@ -2309,8 +2322,10 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 	qcadev->serdev_hu.serdev = serdev;
 	data = device_get_match_data(&serdev->dev);
 	serdev_device_set_drvdata(serdev, qcadev);
-	device_property_read_string(&serdev->dev, "firmware-name",
-					 &qcadev->firmware_name);
+	of_property_read_string_index(serdev->dev.of_node, "firmware-name",
+					 0, &qcadev->firmware_name);
+	of_property_read_string_index(serdev->dev.of_node, "firmware-name",
+					 1, &qcadev->rampatch_name);
 	device_property_read_u32(&serdev->dev, "max-speed",
 				 &qcadev->oper_speed);
 	if (!qcadev->oper_speed)
-- 
2.25.1

[PATCH v3 3/3] arm64: dts: qcom: sa8775p-ride: Add firmware-name in BT node

[Cheng Jiang]

The sa8775p-ride platform uses the QCA6698 Bluetooth chip. While the
QCA6698 shares the same IP core as the WCN6855, it has different RF
components and RAM sizes, requiring new firmware files. Use the
firmware-name property to specify the NVM and rampatch firmware to load.

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

diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
index 3fc62e123..e7fe53d95 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
@@ -857,6 +857,7 @@ &uart17 {
 
 	bluetooth {
 		compatible = "qcom,wcn6855-bt";
+		firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
 
 		vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
 		vddaon-supply = <&vreg_pmu_aon_0p59>;
-- 
2.25.1

RE: Expand firmware-name property to load specific

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi:857
error: arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

Re: [PATCH v3 1/3] dt-bindings: net: bluetooth: qca: Expand firmware-name property

[Rob Herring (Arm)]

On Thu, 05 Dec 2024 18:22:11 +0800, Cheng Jiang wrote:
> Expand the firmware-name property to specify the names of NVM and
> rampatch firmware to load. This update will support loading specific
> firmware (nvm and rampatch) for certain chips, like the QCA6698
> Bluetooth chip, which shares the same IP core as the WCN6855 but has
> different RF components and RAM sizes, requiring new firmware files.
> 
> We might use different connectivity boards on the same platform. For
> example, QCA6698-based boards can support either a two-antenna or
> three-antenna solution, both of which work on the sa8775p-ride platform.
> Due to differences in connectivity boards and variations in RF
> performance from different foundries, different NVM configurations are
> used based on the board ID.
> 
> So In firmware-name, if the NVM file has an extension, the NVM file will
> be used. Otherwise, the system will first try the .bNN (board ID) file,
> and if that fails, it will fall back to the .bin file.
> 
> Possible configurations:
> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
> firmware-name = "QCA6698/hpnv21.bin";
> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>  .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
>  1 file changed, 2 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml:106:5: [error] duplication of key "description" in mapping (key-duplicates)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml: ignoring, error parsing file
./Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml:106:5: found duplicate key "description" with value "specify the name of rampatch firmware to load" (original value: "specify the name of nvm firmware to load")
make[2]: *** Deleting file 'Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.example.dts'
Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml:106:5: found duplicate key "description" with value "specify the name of rampatch firmware to load" (original value: "specify the name of nvm firmware to load")
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1506: dt_binding_check] Error 2
make: *** [Makefile:251: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241205102213.1281865-2-quic_chejiang@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Dmitry Baryshkov]

On Thu, Dec 05, 2024 at 06:22:12PM +0800, Cheng Jiang wrote:
> The firmware-name property has been expanded to specify the names of NVM
> and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
> chip. Although it shares the same IP core as the WCN6855, the QCA6698
> has different RF components and RAM sizes, necessitating new firmware
> files. This change allows for the configuration of NVM and rampatch in
> DT.
> 
> Different connectivity boards may be attached to the same platform. For
> example, QCA6698-based boards can support either a two-antenna or
> three-antenna solution, both of which work on the sa8775p-ride platform.
> Due to differences in connectivity boards and variations in RF
> performance from different foundries, different NVM configurations are
> used based on the board ID.

Two separate commits, one for NVM, another one for RAM patch.

> 
> Therefore, in the firmware-name property, if the NVM file has an
> extension, the NVM file will be used. Otherwise, the system will first
> try the .bNN (board ID) file, and if that fails, it will fall back to
> the .bin file.
> 
> Possible configurations:
> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
> firmware-name = "QCA6698/hpnv21.bin";
> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>  drivers/bluetooth/btqca.c   | 154 ++++++++++++++++++++++++++----------
>  drivers/bluetooth/btqca.h   |   5 +-
>  drivers/bluetooth/hci_qca.c |  21 ++++-
>  3 files changed, 134 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index dfbbac922..e8b89b8cc 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
>  }
>  EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
>  
> +static int qca_get_alt_nvm_path(char *path, size_t max_size)

int is usually for errors, the code suggests bool return type.

> +{
> +	char fwname[64];
> +	const char *suffix;
> +
> +	suffix = strrchr(path, '.');
> +
> +	if (!suffix)
> +		return 0;
> +
> +	strscpy(fwname, path, strlen(path));

64 bytes ought to be enough for anybody, correct?

> +	fwname[suffix - path] = 0;

with path = "qcom/sc7180/Oh.My.Device/name" this is broken.

> +
> +	snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
> +
> +	/* If nvm file is already the default one, return false to
> +	 * skip the retry.
> +	 */
> +	if (strcmp(fwname, path) == 0)
> +		return 0;
> +
> +	snprintf(path, max_size, "%s", fwname);
> +	return 1;
> +}
> +
>  static int qca_tlv_check_data(struct hci_dev *hdev,
>  			       struct qca_fw_config *config,
>  			       u8 *fw_data, size_t fw_size,
> @@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
>  					   config->fwname, ret);
>  				return ret;
>  			}
> +		}
> +		/* For nvm, if desired nvm file is not present and it's not the
> +		 * default nvm file(ends with .bin), try to load the default nvm.
> +		 */
> +		else if (config->type == TLV_TYPE_NVM &&
> +			 qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {

Please, don't rewrite the config. The file may be not present now, but
it will reappear later (e.g. when rootfs gets mounted).

> +			bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
> +			ret = request_firmware(&fw, config->fwname, &hdev->dev);
> +			if (ret) {
> +				bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> +					   config->fwname, ret);
> +				return ret;
> +			}
>  		} else {
>  			bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>  				   config->fwname, ret);
> @@ -730,15 +768,38 @@ 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_nvm_name_by_board(char *fwname, size_t max_size,
> +		const char *firmware_name, struct qca_btsoc_version ver,
> +		enum qca_btsoc_type soc_type, u16 bid)
> +{
> +	const char *variant;
> +
> +	/* Set the variant to empty by default */
> +	variant = "";
> +	/* hsp gf chip */
> +	if (soc_type == QCA_WCN6855) {
> +		if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
> +			variant = "g";

Didn't you get the 'set but unused' here?

> +	}
> +
> +	if (bid == 0x0)

0x0 or 0xff?

> +		snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
> +	else if (bid & 0xff00)
> +		snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);

Doesn't ".b%02x" work in this case too?

> +	else
> +		snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
> +}
> +
>  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)
> +		   const char *firmware_name, const char *rampatch_name)
>  {
>  	struct qca_fw_config config = {};
>  	int err;
>  	u8 rom_ver = 0;
>  	u32 soc_ver;
>  	u16 boardid = 0;
> +	const char *suffix;
>  
>  	bt_dev_dbg(hdev, "QCA setup on UART");
>  
> @@ -761,44 +822,48 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  
>  	/* Download rampatch file */
>  	config.type = TLV_TYPE_PATCH;
> -	switch (soc_type) {
> -	case QCA_WCN3990:
> -	case QCA_WCN3991:
> -	case QCA_WCN3998:
> -		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/crbtfw%02x.tlv", rom_ver);
> -		break;
> -	case QCA_WCN3988:
> -		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/apbtfw%02x.tlv", rom_ver);
> -		break;
> -	case QCA_QCA2066:
> -		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/hpbtfw%02x.tlv", rom_ver);
> -		break;
> -	case QCA_QCA6390:
> -		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/htbtfw%02x.tlv", rom_ver);
> -		break;
> -	case QCA_WCN6750:
> -		/* Choose mbn file by default.If mbn file is not found
> -		 * then choose tlv file
> -		 */
> -		config.type = ELF_TYPE_PATCH;
> -		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/msbtfw%02x.mbn", rom_ver);
> -		break;
> -	case QCA_WCN6855:
> -		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/hpbtfw%02x.tlv", rom_ver);
> -		break;
> -	case QCA_WCN7850:
> -		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/hmtbtfw%02x.tlv", rom_ver);
> -		break;
> -	default:
> -		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/rampatch_%08x.bin", soc_ver);
> +	if (rampatch_name) {
> +		snprintf(config.fwname, sizeof(config.fwname), "qca/%s", rampatch_name);
> +	} else {
> +		switch (soc_type) {
> +		case QCA_WCN3990:
> +		case QCA_WCN3991:
> +		case QCA_WCN3998:
> +			snprintf(config.fwname, sizeof(config.fwname),
> +				 "qca/crbtfw%02x.tlv", rom_ver);
> +			break;
> +		case QCA_WCN3988:
> +			snprintf(config.fwname, sizeof(config.fwname),
> +				 "qca/apbtfw%02x.tlv", rom_ver);
> +			break;
> +		case QCA_QCA2066:
> +			snprintf(config.fwname, sizeof(config.fwname),
> +				 "qca/hpbtfw%02x.tlv", rom_ver);
> +			break;
> +		case QCA_QCA6390:
> +			snprintf(config.fwname, sizeof(config.fwname),
> +				 "qca/htbtfw%02x.tlv", rom_ver);
> +			break;
> +		case QCA_WCN6750:
> +			/* Choose mbn file by default.If mbn file is not found
> +			 * then choose tlv file
> +			 */
> +			config.type = ELF_TYPE_PATCH;
> +			snprintf(config.fwname, sizeof(config.fwname),
> +				 "qca/msbtfw%02x.mbn", rom_ver);
> +			break;
> +		case QCA_WCN6855:
> +			snprintf(config.fwname, sizeof(config.fwname),
> +				 "qca/hpbtfw%02x.tlv", rom_ver);
> +			break;
> +		case QCA_WCN7850:
> +			snprintf(config.fwname, sizeof(config.fwname),
> +				 "qca/hmtbtfw%02x.tlv", rom_ver);
> +			break;
> +		default:
> +			snprintf(config.fwname, sizeof(config.fwname),
> +				 "qca/rampatch_%08x.bin", soc_ver);
> +		}
>  	}
>  
>  	err = qca_download_firmware(hdev, &config, soc_type, rom_ver);
> @@ -816,8 +881,15 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	/* Download NVM configuration */
>  	config.type = TLV_TYPE_NVM;
>  	if (firmware_name) {
> -		snprintf(config.fwname, sizeof(config.fwname),
> -			 "qca/%s", firmware_name);
> +		/* The firmware name has suffix, use it directly */
> +		suffix = strrchr(firmware_name, '.');
> +		if (suffix && *(suffix + 1) != '\0' && *(suffix + 1) != '.') {
> +			snprintf(config.fwname, sizeof(config.fwname), "qca/%s", firmware_name);
> +		} else {
> +			qca_read_fw_board_id(hdev, &boardid);
> +			qca_get_nvm_name_by_board(config.fwname, sizeof(config.fwname),
> +				 firmware_name, ver, soc_type, boardid);
> +		}
>  	} else {
>  		switch (soc_type) {
>  		case QCA_WCN3990:
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index bb5207d7a..9d28c8800 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -161,7 +161,7 @@ enum qca_btsoc_type {
>  int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>  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);
> +		   const char *firmware_name, const char *rampatch_name);
>  int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
>  			 enum qca_btsoc_type);
>  int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> @@ -176,7 +176,8 @@ static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdad
>  static inline 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)
> +				 const char *firmware_name,
> +				 const char *rampatch_name)
>  {
>  	return -EOPNOTSUPP;
>  }
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 37129e6cb..c566d0bf2 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -229,6 +229,7 @@ struct qca_serdev {
>  	u32 oper_speed;
>  	bool bdaddr_property_broken;
>  	const char *firmware_name;
> +	const char *rampatch_name;
>  };
>  
>  static int qca_regulator_enable(struct qca_serdev *qcadev);
> @@ -264,6 +265,17 @@ static const char *qca_get_firmware_name(struct hci_uart *hu)
>  	}
>  }
>  
> +static const char *qca_get_rampatch_name(struct hci_uart *hu)
> +{
> +	if (hu->serdev) {
> +		struct qca_serdev *qsd = serdev_device_get_drvdata(hu->serdev);
> +
> +		return qsd->rampatch_name;
> +	} else {
> +		return NULL;
> +	}
> +}
> +
>  static void __serial_clock_on(struct tty_struct *tty)
>  {
>  	/* TODO: Some chipset requires to enable UART clock on client
> @@ -1855,6 +1867,7 @@ static int qca_setup(struct hci_uart *hu)
>  	unsigned int retries = 0;
>  	enum qca_btsoc_type soc_type = qca_soc_type(hu);
>  	const char *firmware_name = qca_get_firmware_name(hu);
> +	const char *rampatch_name = qca_get_rampatch_name(hu);
>  	int ret;
>  	struct qca_btsoc_version ver;
>  	struct qca_serdev *qcadev;
> @@ -1963,7 +1976,7 @@ static int qca_setup(struct hci_uart *hu)
>  
>  	/* Setup patch / NVM configurations */
>  	ret = qca_uart_setup(hdev, qca_baudrate, soc_type, ver,
> -			firmware_name);
> +			firmware_name, rampatch_name);
>  	if (!ret) {
>  		clear_bit(QCA_IBS_DISABLED, &qca->flags);
>  		qca_debugfs_init(hdev);
> @@ -2309,8 +2322,10 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  	qcadev->serdev_hu.serdev = serdev;
>  	data = device_get_match_data(&serdev->dev);
>  	serdev_device_set_drvdata(serdev, qcadev);
> -	device_property_read_string(&serdev->dev, "firmware-name",
> -					 &qcadev->firmware_name);
> +	of_property_read_string_index(serdev->dev.of_node, "firmware-name",
> +					 0, &qcadev->firmware_name);
> +	of_property_read_string_index(serdev->dev.of_node, "firmware-name",
> +					 1, &qcadev->rampatch_name);
>  	device_property_read_u32(&serdev->dev, "max-speed",
>  				 &qcadev->oper_speed);
>  	if (!qcadev->oper_speed)
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 3/3] arm64: dts: qcom: sa8775p-ride: Add firmware-name in BT node

[Dmitry Baryshkov]

On Thu, Dec 05, 2024 at 06:22:13PM +0800, Cheng Jiang wrote:
> The sa8775p-ride platform uses the QCA6698 Bluetooth chip. While the
> QCA6698 shares the same IP core as the WCN6855, it has different RF
> components and RAM sizes, requiring new firmware files. Use the
> firmware-name property to specify the NVM and rampatch firmware to load.
> 
> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> index 3fc62e123..e7fe53d95 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
> @@ -857,6 +857,7 @@ &uart17 {
>  
>  	bluetooth {
>  		compatible = "qcom,wcn6855-bt";
> +		firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";

And the rampatch is not going to be board-specific?

>  
>  		vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
>  		vddaon-supply = <&vreg_pmu_aon_0p59>;
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[kernel test robot]

Hi Cheng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on c245a7a79602ccbee780c004c1e4abcda66aec32]

url:    https://github.com/intel-lab-lkp/linux/commits/Cheng-Jiang/dt-bindings-net-bluetooth-qca-Expand-firmware-name-property/20241205-182449
base:   c245a7a79602ccbee780c004c1e4abcda66aec32
patch link:    https://lore.kernel.org/r/20241205102213.1281865-3-quic_chejiang%40quicinc.com
patch subject: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch
config: arm-randconfig-002-20241205 (https://download.01.org/0day-ci/archive/20241205/202412052157.4fOQCv3u-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241205/202412052157.4fOQCv3u-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/202412052157.4fOQCv3u-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/bluetooth/btqca.c: In function 'qca_get_nvm_name_by_board':
>> drivers/bluetooth/btqca.c:775:21: warning: variable 'variant' set but not used [-Wunused-but-set-variable]
     775 |         const char *variant;
         |                     ^~~~~~~
   drivers/bluetooth/btqca.c: In function 'qca_get_alt_nvm_path.constprop':
>> drivers/bluetooth/btqca.c:288:45: warning: '.bin' directive output may be truncated writing 4 bytes into a region of size between 1 and 64 [-Wformat-truncation=]
     288 |         snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
         |                                             ^~~~
   drivers/bluetooth/btqca.c:288:9: note: 'snprintf' output between 5 and 68 bytes into a destination of size 64
     288 |         snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/bluetooth/btqca.c:288:9: warning: 'snprintf' argument 4 overlaps destination object 'fwname' [-Wrestrict]
   drivers/bluetooth/btqca.c:277:14: note: destination object referenced by 'restrict'-qualified argument 1 was declared here
     277 |         char fwname[64];
         |              ^~~~~~


vim +/variant +775 drivers/bluetooth/btqca.c

   770	
   771	static void qca_get_nvm_name_by_board(char *fwname, size_t max_size,
   772			const char *firmware_name, struct qca_btsoc_version ver,
   773			enum qca_btsoc_type soc_type, u16 bid)
   774	{
 > 775		const char *variant;
   776	
   777		/* Set the variant to empty by default */
   778		variant = "";
   779		/* hsp gf chip */
   780		if (soc_type == QCA_WCN6855) {
   781			if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
   782				variant = "g";
   783		}
   784	
   785		if (bid == 0x0)
   786			snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
   787		else if (bid & 0xff00)
   788			snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);
   789		else
   790			snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
   791	}
   792	

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

Re: [PATCH v3 1/3] dt-bindings: net: bluetooth: qca: Expand firmware-name property

[Krzysztof Kozlowski]

On 05/12/2024 11:22, Cheng Jiang wrote:
> Expand the firmware-name property to specify the names of NVM and
> rampatch firmware to load. This update will support loading specific
> firmware (nvm and rampatch) for certain chips, like the QCA6698
> Bluetooth chip, which shares the same IP core as the WCN6855 but has
> different RF components and RAM sizes, requiring new firmware files.
> 
> We might use different connectivity boards on the same platform. For
> example, QCA6698-based boards can support either a two-antenna or
> three-antenna solution, both of which work on the sa8775p-ride platform.
> Due to differences in connectivity boards and variations in RF
> performance from different foundries, different NVM configurations are
> used based on the board ID.
> 
> So In firmware-name, if the NVM file has an extension, the NVM file will
> be used. Otherwise, the system will first try the .bNN (board ID) file,
> and if that fails, it will fall back to the .bin file.
> 
> Possible configurations:
> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
> firmware-name = "QCA6698/hpnv21.bin";
This configuration will fail tests. Please test the binding and DTS
before v4.

Best regards,
Krzysztof

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Krzysztof Kozlowski]

On 05/12/2024 11:22, Cheng Jiang wrote:
> @@ -2309,8 +2322,10 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  	qcadev->serdev_hu.serdev = serdev;
>  	data = device_get_match_data(&serdev->dev);
>  	serdev_device_set_drvdata(serdev, qcadev);
> -	device_property_read_string(&serdev->dev, "firmware-name",
> -					 &qcadev->firmware_name);
> +	of_property_read_string_index(serdev->dev.of_node, "firmware-name",
> +					 0, &qcadev->firmware_name);
> +	of_property_read_string_index(serdev->dev.of_node, "firmware-name",
> +					 1, &qcadev->rampatch_name);
No, that's entirely different API. You are not supposed to drop here
device-API and revert to OF. It is not a related change, either.


Best regards,
Krzysztof

Re: [PATCH v3 3/3] arm64: dts: qcom: sa8775p-ride: Add firmware-name in BT node

[Cheng Jiang (IOE)]

Hi Dmitry,

On 12/5/2024 8:13 PM, Dmitry Baryshkov wrote:
> On Thu, Dec 05, 2024 at 06:22:13PM +0800, Cheng Jiang wrote:
>> The sa8775p-ride platform uses the QCA6698 Bluetooth chip. While the
>> QCA6698 shares the same IP core as the WCN6855, it has different RF
>> components and RAM sizes, requiring new firmware files. Use the
>> firmware-name property to specify the NVM and rampatch firmware to load.
>>
>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>> ---
>>  arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
>> index 3fc62e123..e7fe53d95 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi
>> @@ -857,6 +857,7 @@ &uart17 {
>>  
>>  	bluetooth {
>>  		compatible = "qcom,wcn6855-bt";
>> +		firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
> 
> And the rampatch is not going to be board-specific?
> 
Yes, rampatch is chip-specific. 
>>  
>>  		vddrfacmn-supply = <&vreg_pmu_rfa_cmn>;
>>  		vddaon-supply = <&vreg_pmu_aon_0p59>;
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Cheng Jiang (IOE)]

Hi Krzysztof,

On 12/5/2024 10:37 PM, Krzysztof Kozlowski wrote:
> On 05/12/2024 11:22, Cheng Jiang wrote:
>> @@ -2309,8 +2322,10 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>  	qcadev->serdev_hu.serdev = serdev;
>>  	data = device_get_match_data(&serdev->dev);
>>  	serdev_device_set_drvdata(serdev, qcadev);
>> -	device_property_read_string(&serdev->dev, "firmware-name",
>> -					 &qcadev->firmware_name);
>> +	of_property_read_string_index(serdev->dev.of_node, "firmware-name",
>> +					 0, &qcadev->firmware_name);
>> +	of_property_read_string_index(serdev->dev.of_node, "firmware-name",
>> +					 1, &qcadev->rampatch_name);
> No, that's entirely different API. You are not supposed to drop here
> device-API and revert to OF. It is not a related change, either.
> 
Ack, Will change this in next version.
> 
> Best regards,
> Krzysztof

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Cheng Jiang (IOE)]

Hi Dmitry,

On 12/5/2024 8:00 PM, Dmitry Baryshkov wrote:
> On Thu, Dec 05, 2024 at 06:22:12PM +0800, Cheng Jiang wrote:
>> The firmware-name property has been expanded to specify the names of NVM
>> and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
>> chip. Although it shares the same IP core as the WCN6855, the QCA6698
>> has different RF components and RAM sizes, necessitating new firmware
>> files. This change allows for the configuration of NVM and rampatch in
>> DT.
>>
>> Different connectivity boards may be attached to the same platform. For
>> example, QCA6698-based boards can support either a two-antenna or
>> three-antenna solution, both of which work on the sa8775p-ride platform.
>> Due to differences in connectivity boards and variations in RF
>> performance from different foundries, different NVM configurations are
>> used based on the board ID.
> 
> Two separate commits, one for NVM, another one for RAM patch.
> 
Ack.
>>
>> Therefore, in the firmware-name property, if the NVM file has an
>> extension, the NVM file will be used. Otherwise, the system will first
>> try the .bNN (board ID) file, and if that fails, it will fall back to
>> the .bin file.
>>
>> Possible configurations:
>> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
>> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
>> firmware-name = "QCA6698/hpnv21.bin";
>>
>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>> ---
>>  drivers/bluetooth/btqca.c   | 154 ++++++++++++++++++++++++++----------
>>  drivers/bluetooth/btqca.h   |   5 +-
>>  drivers/bluetooth/hci_qca.c |  21 ++++-
>>  3 files changed, 134 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index dfbbac922..e8b89b8cc 100644
>> --- a/drivers/bluetooth/btqca.c
>> +++ b/drivers/bluetooth/btqca.c
>> @@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
>>  }
>>  EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
>>  
>> +static int qca_get_alt_nvm_path(char *path, size_t max_size)
> 
> int is usually for errors, the code suggests bool return type.
> 
Ack.
>> +{
>> +	char fwname[64];
>> +	const char *suffix;
>> +
>> +	suffix = strrchr(path, '.');
>> +
>> +	if (!suffix)
>> +		return 0;
>> +
>> +	strscpy(fwname, path, strlen(path));
> 
> 64 bytes ought to be enough for anybody, correct?
> 
Yes, in current driver, the max f/w path length is 64.

>> +	fwname[suffix - path] = 0;
> 
> with path = "qcom/sc7180/Oh.My.Device/name" this is broken.
> 
Let me test this and fix in next patch.
>> +
>> +	snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
>> +
>> +	/* If nvm file is already the default one, return false to
>> +	 * skip the retry.
>> +	 */
>> +	if (strcmp(fwname, path) == 0)
>> +		return 0;
>> +
>> +	snprintf(path, max_size, "%s", fwname);
>> +	return 1;
>> +}
>> +
>>  static int qca_tlv_check_data(struct hci_dev *hdev,
>>  			       struct qca_fw_config *config,
>>  			       u8 *fw_data, size_t fw_size,
>> @@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
>>  					   config->fwname, ret);
>>  				return ret;
>>  			}
>> +		}
>> +		/* For nvm, if desired nvm file is not present and it's not the
>> +		 * default nvm file(ends with .bin), try to load the default nvm.
>> +		 */
>> +		else if (config->type == TLV_TYPE_NVM &&
>> +			 qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {
> 
> Please, don't rewrite the config. The file may be not present now, but
> it will reappear later (e.g. when rootfs gets mounted).
> 
This tries to load a default NVM file if the board-specific NVM is not found.
It is called when request_firmware fails. It's safe to rewrite the config->fwname
here since we have already tried to load the board-specific NVM. The config
is a local variable in qca_uart_setup and will return after downloading the NVM. 
>> +			bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
>> +			ret = request_firmware(&fw, config->fwname, &hdev->dev);
>> +			if (ret) {
>> +				bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>> +					   config->fwname, ret);
>> +				return ret;
>> +			}
>>  		} else {
>>  			bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>>  				   config->fwname, ret);
>> @@ -730,15 +768,38 @@ 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_nvm_name_by_board(char *fwname, size_t max_size,
>> +		const char *firmware_name, struct qca_btsoc_version ver,
>> +		enum qca_btsoc_type soc_type, u16 bid)
>> +{
>> +	const char *variant;
>> +
>> +	/* Set the variant to empty by default */
>> +	variant = "";
>> +	/* hsp gf chip */
>> +	if (soc_type == QCA_WCN6855) {
>> +		if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
>> +			variant = "g";
> 
> Didn't you get the 'set but unused' here?
> 
Yes, miss this part. Thank you!
>> +	}
>> +
>> +	if (bid == 0x0)
> 
> 0x0 or 0xff?
board is set to 0 by default, 0x0 means read board id fails, then we should use
the default one. 
> 
>> +		snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
>> +	else if (bid & 0xff00)
>> +		snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);
> 
> Doesn't ".b%02x" work in this case too?
> 
No, board id are two bytes, it coudl be 0x0206, then we need .b206. Or it is
0x000a, then we need .b0a. 
>> +	else
>> +		snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
>> +}
>> +
>>  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)
>> +		   const char *firmware_name, const char *rampatch_name)
>>  {
>>  	struct qca_fw_config config = {};
>>  	int err;
>>  	u8 rom_ver = 0;
>>  	u32 soc_ver;
>>  	u16 boardid = 0;
>> +	const char *suffix;
>>  
>>  	bt_dev_dbg(hdev, "QCA setup on UART");
>>  
>> @@ -761,44 +822,48 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  
>>  	/* Download rampatch file */
>>  	config.type = TLV_TYPE_PATCH;
>> -	switch (soc_type) {
>> -	case QCA_WCN3990:
>> -	case QCA_WCN3991:
>> -	case QCA_WCN3998:
>> -		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/crbtfw%02x.tlv", rom_ver);
>> -		break;
>> -	case QCA_WCN3988:
>> -		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/apbtfw%02x.tlv", rom_ver);
>> -		break;
>> -	case QCA_QCA2066:
>> -		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/hpbtfw%02x.tlv", rom_ver);
>> -		break;
>> -	case QCA_QCA6390:
>> -		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/htbtfw%02x.tlv", rom_ver);
>> -		break;
>> -	case QCA_WCN6750:
>> -		/* Choose mbn file by default.If mbn file is not found
>> -		 * then choose tlv file
>> -		 */
>> -		config.type = ELF_TYPE_PATCH;
>> -		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/msbtfw%02x.mbn", rom_ver);
>> -		break;
>> -	case QCA_WCN6855:
>> -		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/hpbtfw%02x.tlv", rom_ver);
>> -		break;
>> -	case QCA_WCN7850:
>> -		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/hmtbtfw%02x.tlv", rom_ver);
>> -		break;
>> -	default:
>> -		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/rampatch_%08x.bin", soc_ver);
>> +	if (rampatch_name) {
>> +		snprintf(config.fwname, sizeof(config.fwname), "qca/%s", rampatch_name);
>> +	} else {
>> +		switch (soc_type) {
>> +		case QCA_WCN3990:
>> +		case QCA_WCN3991:
>> +		case QCA_WCN3998:
>> +			snprintf(config.fwname, sizeof(config.fwname),
>> +				 "qca/crbtfw%02x.tlv", rom_ver);
>> +			break;
>> +		case QCA_WCN3988:
>> +			snprintf(config.fwname, sizeof(config.fwname),
>> +				 "qca/apbtfw%02x.tlv", rom_ver);
>> +			break;
>> +		case QCA_QCA2066:
>> +			snprintf(config.fwname, sizeof(config.fwname),
>> +				 "qca/hpbtfw%02x.tlv", rom_ver);
>> +			break;
>> +		case QCA_QCA6390:
>> +			snprintf(config.fwname, sizeof(config.fwname),
>> +				 "qca/htbtfw%02x.tlv", rom_ver);
>> +			break;
>> +		case QCA_WCN6750:
>> +			/* Choose mbn file by default.If mbn file is not found
>> +			 * then choose tlv file
>> +			 */
>> +			config.type = ELF_TYPE_PATCH;
>> +			snprintf(config.fwname, sizeof(config.fwname),
>> +				 "qca/msbtfw%02x.mbn", rom_ver);
>> +			break;
>> +		case QCA_WCN6855:
>> +			snprintf(config.fwname, sizeof(config.fwname),
>> +				 "qca/hpbtfw%02x.tlv", rom_ver);
>> +			break;
>> +		case QCA_WCN7850:
>> +			snprintf(config.fwname, sizeof(config.fwname),
>> +				 "qca/hmtbtfw%02x.tlv", rom_ver);
>> +			break;
>> +		default:
>> +			snprintf(config.fwname, sizeof(config.fwname),
>> +				 "qca/rampatch_%08x.bin", soc_ver);
>> +		}
>>  	}
>>  
>>  	err = qca_download_firmware(hdev, &config, soc_type, rom_ver);
>> @@ -816,8 +881,15 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>>  	/* Download NVM configuration */
>>  	config.type = TLV_TYPE_NVM;
>>  	if (firmware_name) {
>> -		snprintf(config.fwname, sizeof(config.fwname),
>> -			 "qca/%s", firmware_name);
>> +		/* The firmware name has suffix, use it directly */
>> +		suffix = strrchr(firmware_name, '.');
>> +		if (suffix && *(suffix + 1) != '\0' && *(suffix + 1) != '.') {
>> +			snprintf(config.fwname, sizeof(config.fwname), "qca/%s", firmware_name);
>> +		} else {
>> +			qca_read_fw_board_id(hdev, &boardid);
>> +			qca_get_nvm_name_by_board(config.fwname, sizeof(config.fwname),
>> +				 firmware_name, ver, soc_type, boardid);
>> +		}
>>  	} else {
>>  		switch (soc_type) {
>>  		case QCA_WCN3990:
>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> index bb5207d7a..9d28c8800 100644
>> --- a/drivers/bluetooth/btqca.h
>> +++ b/drivers/bluetooth/btqca.h
>> @@ -161,7 +161,7 @@ enum qca_btsoc_type {
>>  int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>>  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);
>> +		   const char *firmware_name, const char *rampatch_name);
>>  int qca_read_soc_version(struct hci_dev *hdev, struct qca_btsoc_version *ver,
>>  			 enum qca_btsoc_type);
>>  int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>> @@ -176,7 +176,8 @@ static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdad
>>  static inline 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)
>> +				 const char *firmware_name,
>> +				 const char *rampatch_name)
>>  {
>>  	return -EOPNOTSUPP;
>>  }
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 37129e6cb..c566d0bf2 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -229,6 +229,7 @@ struct qca_serdev {
>>  	u32 oper_speed;
>>  	bool bdaddr_property_broken;
>>  	const char *firmware_name;
>> +	const char *rampatch_name;
>>  };
>>  
>>  static int qca_regulator_enable(struct qca_serdev *qcadev);
>> @@ -264,6 +265,17 @@ static const char *qca_get_firmware_name(struct hci_uart *hu)
>>  	}
>>  }
>>  
>> +static const char *qca_get_rampatch_name(struct hci_uart *hu)
>> +{
>> +	if (hu->serdev) {
>> +		struct qca_serdev *qsd = serdev_device_get_drvdata(hu->serdev);
>> +
>> +		return qsd->rampatch_name;
>> +	} else {
>> +		return NULL;
>> +	}
>> +}
>> +
>>  static void __serial_clock_on(struct tty_struct *tty)
>>  {
>>  	/* TODO: Some chipset requires to enable UART clock on client
>> @@ -1855,6 +1867,7 @@ static int qca_setup(struct hci_uart *hu)
>>  	unsigned int retries = 0;
>>  	enum qca_btsoc_type soc_type = qca_soc_type(hu);
>>  	const char *firmware_name = qca_get_firmware_name(hu);
>> +	const char *rampatch_name = qca_get_rampatch_name(hu);
>>  	int ret;
>>  	struct qca_btsoc_version ver;
>>  	struct qca_serdev *qcadev;
>> @@ -1963,7 +1976,7 @@ static int qca_setup(struct hci_uart *hu)
>>  
>>  	/* Setup patch / NVM configurations */
>>  	ret = qca_uart_setup(hdev, qca_baudrate, soc_type, ver,
>> -			firmware_name);
>> +			firmware_name, rampatch_name);
>>  	if (!ret) {
>>  		clear_bit(QCA_IBS_DISABLED, &qca->flags);
>>  		qca_debugfs_init(hdev);
>> @@ -2309,8 +2322,10 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>  	qcadev->serdev_hu.serdev = serdev;
>>  	data = device_get_match_data(&serdev->dev);
>>  	serdev_device_set_drvdata(serdev, qcadev);
>> -	device_property_read_string(&serdev->dev, "firmware-name",
>> -					 &qcadev->firmware_name);
>> +	of_property_read_string_index(serdev->dev.of_node, "firmware-name",
>> +					 0, &qcadev->firmware_name);
>> +	of_property_read_string_index(serdev->dev.of_node, "firmware-name",
>> +					 1, &qcadev->rampatch_name);
>>  	device_property_read_u32(&serdev->dev, "max-speed",
>>  				 &qcadev->oper_speed);
>>  	if (!qcadev->oper_speed)
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH v3 1/3] dt-bindings: net: bluetooth: qca: Expand firmware-name property

[Cheng Jiang (IOE)]

Hi Krzysztof,

On 12/5/2024 10:36 PM, Krzysztof Kozlowski wrote:
> On 05/12/2024 11:22, Cheng Jiang wrote:
>> Expand the firmware-name property to specify the names of NVM and
>> rampatch firmware to load. This update will support loading specific
>> firmware (nvm and rampatch) for certain chips, like the QCA6698
>> Bluetooth chip, which shares the same IP core as the WCN6855 but has
>> different RF components and RAM sizes, requiring new firmware files.
>>
>> We might use different connectivity boards on the same platform. For
>> example, QCA6698-based boards can support either a two-antenna or
>> three-antenna solution, both of which work on the sa8775p-ride platform.
>> Due to differences in connectivity boards and variations in RF
>> performance from different foundries, different NVM configurations are
>> used based on the board ID.
>>
>> So In firmware-name, if the NVM file has an extension, the NVM file will
>> be used. Otherwise, the system will first try the .bNN (board ID) file,
>> and if that fails, it will fall back to the .bin file.
>>
>> Possible configurations:
>> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
>> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
>> firmware-name = "QCA6698/hpnv21.bin";
> This configuration will fail tests. Please test the binding and DTS
> before v4.
> 
Ack.
> Best regards,
> Krzysztof

Re: [PATCH v3 1/3] dt-bindings: net: bluetooth: qca: Expand firmware-name property

[Cheng Jiang (IOE)]

Hi Rob,

On 12/5/2024 7:18 PM, Rob Herring (Arm) wrote:
> 
> On Thu, 05 Dec 2024 18:22:11 +0800, Cheng Jiang wrote:
>> Expand the firmware-name property to specify the names of NVM and
>> rampatch firmware to load. This update will support loading specific
>> firmware (nvm and rampatch) for certain chips, like the QCA6698
>> Bluetooth chip, which shares the same IP core as the WCN6855 but has
>> different RF components and RAM sizes, requiring new firmware files.
>>
>> We might use different connectivity boards on the same platform. For
>> example, QCA6698-based boards can support either a two-antenna or
>> three-antenna solution, both of which work on the sa8775p-ride platform.
>> Due to differences in connectivity boards and variations in RF
>> performance from different foundries, different NVM configurations are
>> used based on the board ID.
>>
>> So In firmware-name, if the NVM file has an extension, the NVM file will
>> be used. Otherwise, the system will first try the .bNN (board ID) file,
>> and if that fails, it will fall back to the .bin file.
>>
>> Possible configurations:
>> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
>> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
>> firmware-name = "QCA6698/hpnv21.bin";
>>
>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>> ---
>>  .../devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml   | 2 ++
>>  1 file changed, 2 insertions(+)
>>
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml:106:5: [error] duplication of key "description" in mapping (key-duplicates)
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml: ignoring, error parsing file
> ./Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml:106:5: found duplicate key "description" with value "specify the name of rampatch firmware to load" (original value: "specify the name of nvm firmware to load")
> make[2]: *** Deleting file 'Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.example.dts'
> Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml:106:5: found duplicate key "description" with value "specify the name of rampatch firmware to load" (original value: "specify the name of nvm firmware to load")
> make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.example.dts] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1506: dt_binding_check] Error 2
> make: *** [Makefile:251: __sub-make] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241205102213.1281865-2-quic_chejiang@quicinc.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 
Thank you for the info. I will run the dt_binding_check before next patch. 

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Dmitry Baryshkov]

On Fri, 6 Dec 2024 at 05:05, Cheng Jiang (IOE)
<quic_chejiang@quicinc.com> wrote:
>
> Hi Dmitry,
>
> On 12/5/2024 8:00 PM, Dmitry Baryshkov wrote:
> > On Thu, Dec 05, 2024 at 06:22:12PM +0800, Cheng Jiang wrote:
> >> The firmware-name property has been expanded to specify the names of NVM
> >> and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
> >> chip. Although it shares the same IP core as the WCN6855, the QCA6698
> >> has different RF components and RAM sizes, necessitating new firmware
> >> files. This change allows for the configuration of NVM and rampatch in
> >> DT.
> >>
> >> Different connectivity boards may be attached to the same platform. For
> >> example, QCA6698-based boards can support either a two-antenna or
> >> three-antenna solution, both of which work on the sa8775p-ride platform.
> >> Due to differences in connectivity boards and variations in RF
> >> performance from different foundries, different NVM configurations are
> >> used based on the board ID.
> >
> > Two separate commits, one for NVM, another one for RAM patch.
> >
> Ack.
> >>
> >> Therefore, in the firmware-name property, if the NVM file has an
> >> extension, the NVM file will be used. Otherwise, the system will first
> >> try the .bNN (board ID) file, and if that fails, it will fall back to
> >> the .bin file.
> >>
> >> Possible configurations:
> >> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
> >> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
> >> firmware-name = "QCA6698/hpnv21.bin";
> >>
> >> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >> ---
> >>  drivers/bluetooth/btqca.c   | 154 ++++++++++++++++++++++++++----------
> >>  drivers/bluetooth/btqca.h   |   5 +-
> >>  drivers/bluetooth/hci_qca.c |  21 ++++-
> >>  3 files changed, 134 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> >> index dfbbac922..e8b89b8cc 100644
> >> --- a/drivers/bluetooth/btqca.c
> >> +++ b/drivers/bluetooth/btqca.c
> >> @@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
> >>  }
> >>  EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
> >>
> >> +static int qca_get_alt_nvm_path(char *path, size_t max_size)
> >
> > int is usually for errors, the code suggests bool return type.
> >
> Ack.
> >> +{
> >> +    char fwname[64];
> >> +    const char *suffix;
> >> +
> >> +    suffix = strrchr(path, '.');
> >> +
> >> +    if (!suffix)
> >> +            return 0;
> >> +
> >> +    strscpy(fwname, path, strlen(path));
> >
> > 64 bytes ought to be enough for anybody, correct?
> >
> Yes, in current driver, the max f/w path length is 64.
>
> >> +    fwname[suffix - path] = 0;
> >
> > with path = "qcom/sc7180/Oh.My.Device/name" this is broken.
> >
> Let me test this and fix in next patch.
> >> +
> >> +    snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
> >> +
> >> +    /* If nvm file is already the default one, return false to
> >> +     * skip the retry.
> >> +     */
> >> +    if (strcmp(fwname, path) == 0)
> >> +            return 0;
> >> +
> >> +    snprintf(path, max_size, "%s", fwname);
> >> +    return 1;
> >> +}
> >> +
> >>  static int qca_tlv_check_data(struct hci_dev *hdev,
> >>                             struct qca_fw_config *config,
> >>                             u8 *fw_data, size_t fw_size,
> >> @@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
> >>                                         config->fwname, ret);
> >>                              return ret;
> >>                      }
> >> +            }
> >> +            /* For nvm, if desired nvm file is not present and it's not the
> >> +             * default nvm file(ends with .bin), try to load the default nvm.
> >> +             */
> >> +            else if (config->type == TLV_TYPE_NVM &&
> >> +                     qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {
> >
> > Please, don't rewrite the config. The file may be not present now, but
> > it will reappear later (e.g. when rootfs gets mounted).
> >
> This tries to load a default NVM file if the board-specific NVM is not found.
> It is called when request_firmware fails. It's safe to rewrite the config->fwname
> here since we have already tried to load the board-specific NVM. The config
> is a local variable in qca_uart_setup and will return after downloading the NVM.

Please read my question before answering it.

> >> +                    bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
> >> +                    ret = request_firmware(&fw, config->fwname, &hdev->dev);
> >> +                    if (ret) {
> >> +                            bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> >> +                                       config->fwname, ret);
> >> +                            return ret;
> >> +                    }
> >>              } else {
> >>                      bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> >>                                 config->fwname, ret);
> >> @@ -730,15 +768,38 @@ 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_nvm_name_by_board(char *fwname, size_t max_size,
> >> +            const char *firmware_name, struct qca_btsoc_version ver,
> >> +            enum qca_btsoc_type soc_type, u16 bid)
> >> +{
> >> +    const char *variant;
> >> +
> >> +    /* Set the variant to empty by default */
> >> +    variant = "";
> >> +    /* hsp gf chip */
> >> +    if (soc_type == QCA_WCN6855) {
> >> +            if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
> >> +                    variant = "g";
> >
> > Didn't you get the 'set but unused' here?
> >
> Yes, miss this part. Thank you!
> >> +    }
> >> +
> >> +    if (bid == 0x0)
> >
> > 0x0 or 0xff?
> board is set to 0 by default, 0x0 means read board id fails, then we should use
> the default one.

What is the 'unprogrammed' board_id? On the WiFi side it's usually 0xff.

> >
> >> +            snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
> >> +    else if (bid & 0xff00)
> >> +            snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);
> >
> > Doesn't ".b%02x" work in this case too?
> >
> No, board id are two bytes, it coudl be 0x0206, then we need .b206. Or it is
> 0x000a, then we need .b0a.

What will ".b%02x" write in those two cases?

> >> +    else
> >> +            snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
> >> +}
> >> +


-- 
With best wishes
Dmitry

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Cheng Jiang (IOE)]

Hi Dmitry,

On 12/6/2024 4:34 PM, Dmitry Baryshkov wrote:
> On Fri, 6 Dec 2024 at 05:05, Cheng Jiang (IOE)
> <quic_chejiang@quicinc.com> wrote:
>>
>> Hi Dmitry,
>>
>> On 12/5/2024 8:00 PM, Dmitry Baryshkov wrote:
>>> On Thu, Dec 05, 2024 at 06:22:12PM +0800, Cheng Jiang wrote:
>>>> The firmware-name property has been expanded to specify the names of NVM
>>>> and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
>>>> chip. Although it shares the same IP core as the WCN6855, the QCA6698
>>>> has different RF components and RAM sizes, necessitating new firmware
>>>> files. This change allows for the configuration of NVM and rampatch in
>>>> DT.
>>>>
>>>> Different connectivity boards may be attached to the same platform. For
>>>> example, QCA6698-based boards can support either a two-antenna or
>>>> three-antenna solution, both of which work on the sa8775p-ride platform.
>>>> Due to differences in connectivity boards and variations in RF
>>>> performance from different foundries, different NVM configurations are
>>>> used based on the board ID.
>>>
>>> Two separate commits, one for NVM, another one for RAM patch.
>>>
>> Ack.
>>>>
>>>> Therefore, in the firmware-name property, if the NVM file has an
>>>> extension, the NVM file will be used. Otherwise, the system will first
>>>> try the .bNN (board ID) file, and if that fails, it will fall back to
>>>> the .bin file.
>>>>
>>>> Possible configurations:
>>>> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
>>>> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
>>>> firmware-name = "QCA6698/hpnv21.bin";
>>>>
>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>> ---
>>>>  drivers/bluetooth/btqca.c   | 154 ++++++++++++++++++++++++++----------
>>>>  drivers/bluetooth/btqca.h   |   5 +-
>>>>  drivers/bluetooth/hci_qca.c |  21 ++++-
>>>>  3 files changed, 134 insertions(+), 46 deletions(-)
>>>>
>>>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>>>> index dfbbac922..e8b89b8cc 100644
>>>> --- a/drivers/bluetooth/btqca.c
>>>> +++ b/drivers/bluetooth/btqca.c
>>>> @@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
>>>>
>>>> +static int qca_get_alt_nvm_path(char *path, size_t max_size)
>>>
>>> int is usually for errors, the code suggests bool return type.
>>>
>> Ack.
>>>> +{
>>>> +    char fwname[64];
>>>> +    const char *suffix;
>>>> +
>>>> +    suffix = strrchr(path, '.');
>>>> +
>>>> +    if (!suffix)
>>>> +            return 0;
>>>> +
>>>> +    strscpy(fwname, path, strlen(path));
>>>
>>> 64 bytes ought to be enough for anybody, correct?
>>>
>> Yes, in current driver, the max f/w path length is 64.
>>
>>>> +    fwname[suffix - path] = 0;
>>>
>>> with path = "qcom/sc7180/Oh.My.Device/name" this is broken.
>>>
>> Let me test this and fix in next patch.
>>>> +
>>>> +    snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
>>>> +
>>>> +    /* If nvm file is already the default one, return false to
>>>> +     * skip the retry.
>>>> +     */
>>>> +    if (strcmp(fwname, path) == 0)
>>>> +            return 0;
>>>> +
>>>> +    snprintf(path, max_size, "%s", fwname);
>>>> +    return 1;
>>>> +}
>>>> +
>>>>  static int qca_tlv_check_data(struct hci_dev *hdev,
>>>>                             struct qca_fw_config *config,
>>>>                             u8 *fw_data, size_t fw_size,
>>>> @@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
>>>>                                         config->fwname, ret);
>>>>                              return ret;
>>>>                      }
>>>> +            }
>>>> +            /* For nvm, if desired nvm file is not present and it's not the
>>>> +             * default nvm file(ends with .bin), try to load the default nvm.
>>>> +             */
>>>> +            else if (config->type == TLV_TYPE_NVM &&
>>>> +                     qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {
>>>
>>> Please, don't rewrite the config. The file may be not present now, but
>>> it will reappear later (e.g. when rootfs gets mounted).
>>>
>> This tries to load a default NVM file if the board-specific NVM is not found.
>> It is called when request_firmware fails. It's safe to rewrite the config->fwname
>> here since we have already tried to load the board-specific NVM. The config
>> is a local variable in qca_uart_setup and will return after downloading the NVM.
> 
> Please read my question before answering it.
> 
Sorry, I'm not clear about your question. Could you please explain it in more detail? 
I'm not quite sure how the situation you mentioned affects this code flow if you mean
not downloading another NVM file.

The board-specific NVM and the default NVM should be in the same folder and should
appear simultaneously.

From the Bluetooth firmware load flow perspective, the firmware is loaded either 
when the kernel module is inserted (insmod) or when Bluetooth is turned off and 
then on again via a user-space command. If the firmware is not found at this time, 
the ROM code is used instead. It does not attempt to load the firmware automatically,
even if the firmware appears later.

>>>> +                    bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
>>>> +                    ret = request_firmware(&fw, config->fwname, &hdev->dev);
>>>> +                    if (ret) {
>>>> +                            bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>>>> +                                       config->fwname, ret);
>>>> +                            return ret;
>>>> +                    }
>>>>              } else {
>>>>                      bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>>>>                                 config->fwname, ret);
>>>> @@ -730,15 +768,38 @@ 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_nvm_name_by_board(char *fwname, size_t max_size,
>>>> +            const char *firmware_name, struct qca_btsoc_version ver,
>>>> +            enum qca_btsoc_type soc_type, u16 bid)
>>>> +{
>>>> +    const char *variant;
>>>> +
>>>> +    /* Set the variant to empty by default */
>>>> +    variant = "";
>>>> +    /* hsp gf chip */
>>>> +    if (soc_type == QCA_WCN6855) {
>>>> +            if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
>>>> +                    variant = "g";
>>>
>>> Didn't you get the 'set but unused' here?
>>>
>> Yes, miss this part. Thank you!
>>>> +    }
>>>> +
>>>> +    if (bid == 0x0)
>>>
>>> 0x0 or 0xff?
>> board is set to 0 by default, 0x0 means read board id fails, then we should use
>> the default one.
> 
> What is the 'unprogrammed' board_id? On the WiFi side it's usually 0xff.
> 
Yes, the 'unprogrammed' board_id should be 0xffff. Then 0 and 0xffff should use the
default nvm.  
>>>
>>>> +            snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
>>>> +    else if (bid & 0xff00)
>>>> +            snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);
>>>
>>> Doesn't ".b%02x" work in this case too?
>>>
>> No, board id are two bytes, it coudl be 0x0206, then we need .b206. Or it is
>> 0x000a, then we need .b0a.
> 
> What will ".b%02x" write in those two cases?
> 
Yes, it works for both cases. Thanks! 
>>>> +    else
>>>> +            snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
>>>> +}
>>>> +
> 
> 

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Dmitry Baryshkov]

On Mon, Dec 09, 2024 at 05:03:55PM +0800, Cheng Jiang (IOE) wrote:
> Hi Dmitry,
> 
> On 12/6/2024 4:34 PM, Dmitry Baryshkov wrote:
> > On Fri, 6 Dec 2024 at 05:05, Cheng Jiang (IOE)
> > <quic_chejiang@quicinc.com> wrote:
> >>
> >> Hi Dmitry,
> >>
> >> On 12/5/2024 8:00 PM, Dmitry Baryshkov wrote:
> >>> On Thu, Dec 05, 2024 at 06:22:12PM +0800, Cheng Jiang wrote:
> >>>> The firmware-name property has been expanded to specify the names of NVM
> >>>> and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
> >>>> chip. Although it shares the same IP core as the WCN6855, the QCA6698
> >>>> has different RF components and RAM sizes, necessitating new firmware
> >>>> files. This change allows for the configuration of NVM and rampatch in
> >>>> DT.
> >>>>
> >>>> Different connectivity boards may be attached to the same platform. For
> >>>> example, QCA6698-based boards can support either a two-antenna or
> >>>> three-antenna solution, both of which work on the sa8775p-ride platform.
> >>>> Due to differences in connectivity boards and variations in RF
> >>>> performance from different foundries, different NVM configurations are
> >>>> used based on the board ID.
> >>>
> >>> Two separate commits, one for NVM, another one for RAM patch.
> >>>
> >> Ack.
> >>>>
> >>>> Therefore, in the firmware-name property, if the NVM file has an
> >>>> extension, the NVM file will be used. Otherwise, the system will first
> >>>> try the .bNN (board ID) file, and if that fails, it will fall back to
> >>>> the .bin file.
> >>>>
> >>>> Possible configurations:
> >>>> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
> >>>> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
> >>>> firmware-name = "QCA6698/hpnv21.bin";
> >>>>
> >>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >>>> ---
> >>>>  drivers/bluetooth/btqca.c   | 154 ++++++++++++++++++++++++++----------
> >>>>  drivers/bluetooth/btqca.h   |   5 +-
> >>>>  drivers/bluetooth/hci_qca.c |  21 ++++-
> >>>>  3 files changed, 134 insertions(+), 46 deletions(-)
> >>>>
> >>>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> >>>> index dfbbac922..e8b89b8cc 100644
> >>>> --- a/drivers/bluetooth/btqca.c
> >>>> +++ b/drivers/bluetooth/btqca.c
> >>>> @@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
> >>>>  }
> >>>>  EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
> >>>>
> >>>> +static int qca_get_alt_nvm_path(char *path, size_t max_size)
> >>>
> >>> int is usually for errors, the code suggests bool return type.
> >>>
> >> Ack.
> >>>> +{
> >>>> +    char fwname[64];
> >>>> +    const char *suffix;
> >>>> +
> >>>> +    suffix = strrchr(path, '.');
> >>>> +
> >>>> +    if (!suffix)
> >>>> +            return 0;
> >>>> +
> >>>> +    strscpy(fwname, path, strlen(path));
> >>>
> >>> 64 bytes ought to be enough for anybody, correct?
> >>>
> >> Yes, in current driver, the max f/w path length is 64.
> >>
> >>>> +    fwname[suffix - path] = 0;
> >>>
> >>> with path = "qcom/sc7180/Oh.My.Device/name" this is broken.
> >>>
> >> Let me test this and fix in next patch.
> >>>> +
> >>>> +    snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
> >>>> +
> >>>> +    /* If nvm file is already the default one, return false to
> >>>> +     * skip the retry.
> >>>> +     */
> >>>> +    if (strcmp(fwname, path) == 0)
> >>>> +            return 0;
> >>>> +
> >>>> +    snprintf(path, max_size, "%s", fwname);
> >>>> +    return 1;
> >>>> +}
> >>>> +
> >>>>  static int qca_tlv_check_data(struct hci_dev *hdev,
> >>>>                             struct qca_fw_config *config,
> >>>>                             u8 *fw_data, size_t fw_size,
> >>>> @@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
> >>>>                                         config->fwname, ret);
> >>>>                              return ret;
> >>>>                      }
> >>>> +            }
> >>>> +            /* For nvm, if desired nvm file is not present and it's not the
> >>>> +             * default nvm file(ends with .bin), try to load the default nvm.
> >>>> +             */
> >>>> +            else if (config->type == TLV_TYPE_NVM &&
> >>>> +                     qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {
> >>>
> >>> Please, don't rewrite the config. The file may be not present now, but
> >>> it will reappear later (e.g. when rootfs gets mounted).
> >>>
> >> This tries to load a default NVM file if the board-specific NVM is not found.
> >> It is called when request_firmware fails. It's safe to rewrite the config->fwname
> >> here since we have already tried to load the board-specific NVM. The config
> >> is a local variable in qca_uart_setup and will return after downloading the NVM.
> > 
> > Please read my question before answering it.
> > 
> Sorry, I'm not clear about your question. Could you please explain it in more detail? 
> I'm not quite sure how the situation you mentioned affects this code flow if you mean
> not downloading another NVM file.
> 
> The board-specific NVM and the default NVM should be in the same folder and should
> appear simultaneously.
> 
> From the Bluetooth firmware load flow perspective, the firmware is loaded either 
> when the kernel module is inserted (insmod) or when Bluetooth is turned off and 
> then on again via a user-space command. If the firmware is not found at this time, 
> the ROM code is used instead. It does not attempt to load the firmware automatically,
> even if the firmware appears later.

I was thinking about the following scenario:

- BT firmware is attempted to load during driver probe, /lib/firmware is
  not fully populated, so the config is rewritten to use the default
- rootfs is fully mounted and populated with the board-specific file
- BT interface is being turned on. It is expected that the
  board-specific file will be loaded, however because the config was
  changed in one of the previous steps, the driver still loads the
  default one.

That said, the driver should perform the fallback, etc, but the config
should stay intact even in the fallback case.

> 
> >>>> +                    bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
> >>>> +                    ret = request_firmware(&fw, config->fwname, &hdev->dev);
> >>>> +                    if (ret) {
> >>>> +                            bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> >>>> +                                       config->fwname, ret);
> >>>> +                            return ret;
> >>>> +                    }
> >>>>              } else {
> >>>>                      bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> >>>>                                 config->fwname, ret);
> >>>> @@ -730,15 +768,38 @@ 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_nvm_name_by_board(char *fwname, size_t max_size,
> >>>> +            const char *firmware_name, struct qca_btsoc_version ver,
> >>>> +            enum qca_btsoc_type soc_type, u16 bid)
> >>>> +{
> >>>> +    const char *variant;
> >>>> +
> >>>> +    /* Set the variant to empty by default */
> >>>> +    variant = "";
> >>>> +    /* hsp gf chip */
> >>>> +    if (soc_type == QCA_WCN6855) {
> >>>> +            if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
> >>>> +                    variant = "g";
> >>>
> >>> Didn't you get the 'set but unused' here?
> >>>
> >> Yes, miss this part. Thank you!
> >>>> +    }
> >>>> +
> >>>> +    if (bid == 0x0)
> >>>
> >>> 0x0 or 0xff?
> >> board is set to 0 by default, 0x0 means read board id fails, then we should use
> >> the default one.
> > 
> > What is the 'unprogrammed' board_id? On the WiFi side it's usually 0xff.
> > 
> Yes, the 'unprogrammed' board_id should be 0xffff. Then 0 and 0xffff should use the
> default nvm.  

Good. I think it's safe to safe board_id to 0xffff by default, then you
don't have to handle '0' specially.

> >>>
> >>>> +            snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
> >>>> +    else if (bid & 0xff00)
> >>>> +            snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);
> >>>
> >>> Doesn't ".b%02x" work in this case too?
> >>>
> >> No, board id are two bytes, it coudl be 0x0206, then we need .b206. Or it is
> >> 0x000a, then we need .b0a.
> > 
> > What will ".b%02x" write in those two cases?
> > 
> Yes, it works for both cases. Thanks! 

:-)

> >>>> +    else
> >>>> +            snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
> >>>> +}
> >>>> +
> > 
> > 
> 

-- 
With best wishes
Dmitry

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Cheng Jiang (IOE)]

Hi Dmitry,

On 12/9/2024 6:49 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 09, 2024 at 05:03:55PM +0800, Cheng Jiang (IOE) wrote:
>> Hi Dmitry,
>>
>> On 12/6/2024 4:34 PM, Dmitry Baryshkov wrote:
>>> On Fri, 6 Dec 2024 at 05:05, Cheng Jiang (IOE)
>>> <quic_chejiang@quicinc.com> wrote:
>>>>
>>>> Hi Dmitry,
>>>>
>>>> On 12/5/2024 8:00 PM, Dmitry Baryshkov wrote:
>>>>> On Thu, Dec 05, 2024 at 06:22:12PM +0800, Cheng Jiang wrote:
>>>>>> The firmware-name property has been expanded to specify the names of NVM
>>>>>> and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
>>>>>> chip. Although it shares the same IP core as the WCN6855, the QCA6698
>>>>>> has different RF components and RAM sizes, necessitating new firmware
>>>>>> files. This change allows for the configuration of NVM and rampatch in
>>>>>> DT.
>>>>>>
>>>>>> Different connectivity boards may be attached to the same platform. For
>>>>>> example, QCA6698-based boards can support either a two-antenna or
>>>>>> three-antenna solution, both of which work on the sa8775p-ride platform.
>>>>>> Due to differences in connectivity boards and variations in RF
>>>>>> performance from different foundries, different NVM configurations are
>>>>>> used based on the board ID.
>>>>>
>>>>> Two separate commits, one for NVM, another one for RAM patch.
>>>>>
>>>> Ack.
>>>>>>
>>>>>> Therefore, in the firmware-name property, if the NVM file has an
>>>>>> extension, the NVM file will be used. Otherwise, the system will first
>>>>>> try the .bNN (board ID) file, and if that fails, it will fall back to
>>>>>> the .bin file.
>>>>>>
>>>>>> Possible configurations:
>>>>>> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
>>>>>> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
>>>>>> firmware-name = "QCA6698/hpnv21.bin";
>>>>>>
>>>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>>>> ---
>>>>>>  drivers/bluetooth/btqca.c   | 154 ++++++++++++++++++++++++++----------
>>>>>>  drivers/bluetooth/btqca.h   |   5 +-
>>>>>>  drivers/bluetooth/hci_qca.c |  21 ++++-
>>>>>>  3 files changed, 134 insertions(+), 46 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>>>>>> index dfbbac922..e8b89b8cc 100644
>>>>>> --- a/drivers/bluetooth/btqca.c
>>>>>> +++ b/drivers/bluetooth/btqca.c
>>>>>> @@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
>>>>>>  }
>>>>>>  EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
>>>>>>
>>>>>> +static int qca_get_alt_nvm_path(char *path, size_t max_size)
>>>>>
>>>>> int is usually for errors, the code suggests bool return type.
>>>>>
>>>> Ack.
>>>>>> +{
>>>>>> +    char fwname[64];
>>>>>> +    const char *suffix;
>>>>>> +
>>>>>> +    suffix = strrchr(path, '.');
>>>>>> +
>>>>>> +    if (!suffix)
>>>>>> +            return 0;
>>>>>> +
>>>>>> +    strscpy(fwname, path, strlen(path));
>>>>>
>>>>> 64 bytes ought to be enough for anybody, correct?
>>>>>
>>>> Yes, in current driver, the max f/w path length is 64.
>>>>
>>>>>> +    fwname[suffix - path] = 0;
>>>>>
>>>>> with path = "qcom/sc7180/Oh.My.Device/name" this is broken.
>>>>>
>>>> Let me test this and fix in next patch.
>>>>>> +
>>>>>> +    snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
>>>>>> +
>>>>>> +    /* If nvm file is already the default one, return false to
>>>>>> +     * skip the retry.
>>>>>> +     */
>>>>>> +    if (strcmp(fwname, path) == 0)
>>>>>> +            return 0;
>>>>>> +
>>>>>> +    snprintf(path, max_size, "%s", fwname);
>>>>>> +    return 1;
>>>>>> +}
>>>>>> +
>>>>>>  static int qca_tlv_check_data(struct hci_dev *hdev,
>>>>>>                             struct qca_fw_config *config,
>>>>>>                             u8 *fw_data, size_t fw_size,
>>>>>> @@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
>>>>>>                                         config->fwname, ret);
>>>>>>                              return ret;
>>>>>>                      }
>>>>>> +            }
>>>>>> +            /* For nvm, if desired nvm file is not present and it's not the
>>>>>> +             * default nvm file(ends with .bin), try to load the default nvm.
>>>>>> +             */
>>>>>> +            else if (config->type == TLV_TYPE_NVM &&
>>>>>> +                     qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {
>>>>>
>>>>> Please, don't rewrite the config. The file may be not present now, but
>>>>> it will reappear later (e.g. when rootfs gets mounted).
>>>>>
>>>> This tries to load a default NVM file if the board-specific NVM is not found.
>>>> It is called when request_firmware fails. It's safe to rewrite the config->fwname
>>>> here since we have already tried to load the board-specific NVM. The config
>>>> is a local variable in qca_uart_setup and will return after downloading the NVM.
>>>
>>> Please read my question before answering it.
>>>
>> Sorry, I'm not clear about your question. Could you please explain it in more detail? 
>> I'm not quite sure how the situation you mentioned affects this code flow if you mean
>> not downloading another NVM file.
>>
>> The board-specific NVM and the default NVM should be in the same folder and should
>> appear simultaneously.
>>
>> From the Bluetooth firmware load flow perspective, the firmware is loaded either 
>> when the kernel module is inserted (insmod) or when Bluetooth is turned off and 
>> then on again via a user-space command. If the firmware is not found at this time, 
>> the ROM code is used instead. It does not attempt to load the firmware automatically,
>> even if the firmware appears later.
> 
> I was thinking about the following scenario:
> 
> - BT firmware is attempted to load during driver probe, /lib/firmware is
>   not fully populated, so the config is rewritten to use the default
> - rootfs is fully mounted and populated with the board-specific file
> - BT interface is being turned on. It is expected that the
>   board-specific file will be loaded, however because the config was
>   changed in one of the previous steps, the driver still loads the
>   default one.
> 
> That said, the driver should perform the fallback, etc, but the config
> should stay intact even in the fallback case.
> 
Thank you for the detail explanation. Current flow of BT enable in driver
likes this:

Enable the soc(Assert BT_EN) -->  read the SOC info --> Change baud rate --> 
get rampatch file name (based on soc info or dts) --> download rampatch --> 
get nvm file name(based on soc info or dts) --> download nvm file --> 
download default nvm (if the board-specific file not found).

Every time the driver probe or the BT interface is turned on, it follows the
flow described above. The rampatch and NVM file names are reconstructed by 
the SoC information each time, so the driver always attempts to download the 
board-specific file first.

Here is the log, there is no hpnv21.b206 and re-insmod the driver. 
[11850.644220] Bluetooth: HCI UART driver ver 2.3
[11850.644232] Bluetooth: HCI UART protocol H4 registered
[11850.644284] Bluetooth: HCI UART protocol LL registered
[11850.644314] Bluetooth: HCI UART protocol QCA registered
[11850.645055] Bluetooth: hci0: setting up wcn6855
[11850.706962] Bluetooth: hci0: QCA Product ID   :0x00000013
[11850.706975] Bluetooth: hci0: QCA SOC Version  :0x400c0210
[11850.706978] Bluetooth: hci0: QCA ROM Version  :0x00000201
[11850.706981] Bluetooth: hci0: QCA Patch Version:0x000038e6
[11850.714508] Bluetooth: hci0: QCA controller version 0x02100201
[11850.714518] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
[11851.406475] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
[11851.406515] bluetooth hci0: Direct firmware load for qca/QCA6698/hpnv21.b206 failed with error -2
[11851.406522] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.bin
[11851.570125] Bluetooth: hci0: QCA setup on UART is completed

hpnv21.b206 exists and then re-insmod the driver.
[11878.551494] Bluetooth: HCI UART driver ver 2.3
[11878.551505] Bluetooth: HCI UART protocol H4 registered
[11878.551553] Bluetooth: HCI UART protocol LL registered
[11878.551580] Bluetooth: HCI UART protocol QCA registered
[11878.552131] Bluetooth: hci0: setting up wcn6855
[11878.618865] Bluetooth: hci0: QCA Product ID   :0x00000013
[11878.618877] Bluetooth: hci0: QCA SOC Version  :0x400c0210
[11878.618881] Bluetooth: hci0: QCA ROM Version  :0x00000201
[11878.618884] Bluetooth: hci0: QCA Patch Version:0x000038e6
[11878.629674] Bluetooth: hci0: QCA controller version 0x02100201
[11878.629681] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
[11879.318475] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
[11879.482082] Bluetooth: hci0: QCA setup on UART is completed
[11879.505086] Bluetooth: MGMT ver 1.22

Turn on BT has the similar log.
>>
>>>>>> +                    bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
>>>>>> +                    ret = request_firmware(&fw, config->fwname, &hdev->dev);
>>>>>> +                    if (ret) {
>>>>>> +                            bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>>>>>> +                                       config->fwname, ret);
>>>>>> +                            return ret;
>>>>>> +                    }
>>>>>>              } else {
>>>>>>                      bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>>>>>>                                 config->fwname, ret);
>>>>>> @@ -730,15 +768,38 @@ 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_nvm_name_by_board(char *fwname, size_t max_size,
>>>>>> +            const char *firmware_name, struct qca_btsoc_version ver,
>>>>>> +            enum qca_btsoc_type soc_type, u16 bid)
>>>>>> +{
>>>>>> +    const char *variant;
>>>>>> +
>>>>>> +    /* Set the variant to empty by default */
>>>>>> +    variant = "";
>>>>>> +    /* hsp gf chip */
>>>>>> +    if (soc_type == QCA_WCN6855) {
>>>>>> +            if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
>>>>>> +                    variant = "g";
>>>>>
>>>>> Didn't you get the 'set but unused' here?
>>>>>
>>>> Yes, miss this part. Thank you!
>>>>>> +    }
>>>>>> +
>>>>>> +    if (bid == 0x0)
>>>>>
>>>>> 0x0 or 0xff?
>>>> board is set to 0 by default, 0x0 means read board id fails, then we should use
>>>> the default one.
>>>
>>> What is the 'unprogrammed' board_id? On the WiFi side it's usually 0xff.
>>>
>> Yes, the 'unprogrammed' board_id should be 0xffff. Then 0 and 0xffff should use the
>> default nvm.  
> 
> Good. I think it's safe to safe board_id to 0xffff by default, then you
> don't have to handle '0' specially.
> 
>>>>>
>>>>>> +            snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
>>>>>> +    else if (bid & 0xff00)
>>>>>> +            snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);
>>>>>
>>>>> Doesn't ".b%02x" work in this case too?
>>>>>
>>>> No, board id are two bytes, it coudl be 0x0206, then we need .b206. Or it is
>>>> 0x000a, then we need .b0a.
>>>
>>> What will ".b%02x" write in those two cases?
>>>
>> Yes, it works for both cases. Thanks! 
> 
> :-)
> 
>>>>>> +    else
>>>>>> +            snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
>>>>>> +}
>>>>>> +
>>>
>>>
>>
> 

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Dmitry Baryshkov]

On Mon, 9 Dec 2024 at 15:59, Cheng Jiang (IOE)
<quic_chejiang@quicinc.com> wrote:
>
> Hi Dmitry,
>
> On 12/9/2024 6:49 PM, Dmitry Baryshkov wrote:
> > On Mon, Dec 09, 2024 at 05:03:55PM +0800, Cheng Jiang (IOE) wrote:
> >> Hi Dmitry,
> >>
> >> On 12/6/2024 4:34 PM, Dmitry Baryshkov wrote:
> >>> On Fri, 6 Dec 2024 at 05:05, Cheng Jiang (IOE)
> >>> <quic_chejiang@quicinc.com> wrote:
> >>>>
> >>>> Hi Dmitry,
> >>>>
> >>>> On 12/5/2024 8:00 PM, Dmitry Baryshkov wrote:
> >>>>> On Thu, Dec 05, 2024 at 06:22:12PM +0800, Cheng Jiang wrote:
> >>>>>> The firmware-name property has been expanded to specify the names of NVM
> >>>>>> and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
> >>>>>> chip. Although it shares the same IP core as the WCN6855, the QCA6698
> >>>>>> has different RF components and RAM sizes, necessitating new firmware
> >>>>>> files. This change allows for the configuration of NVM and rampatch in
> >>>>>> DT.
> >>>>>>
> >>>>>> Different connectivity boards may be attached to the same platform. For
> >>>>>> example, QCA6698-based boards can support either a two-antenna or
> >>>>>> three-antenna solution, both of which work on the sa8775p-ride platform.
> >>>>>> Due to differences in connectivity boards and variations in RF
> >>>>>> performance from different foundries, different NVM configurations are
> >>>>>> used based on the board ID.
> >>>>>
> >>>>> Two separate commits, one for NVM, another one for RAM patch.
> >>>>>
> >>>> Ack.
> >>>>>>
> >>>>>> Therefore, in the firmware-name property, if the NVM file has an
> >>>>>> extension, the NVM file will be used. Otherwise, the system will first
> >>>>>> try the .bNN (board ID) file, and if that fails, it will fall back to
> >>>>>> the .bin file.
> >>>>>>
> >>>>>> Possible configurations:
> >>>>>> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
> >>>>>> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
> >>>>>> firmware-name = "QCA6698/hpnv21.bin";
> >>>>>>
> >>>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >>>>>> ---
> >>>>>>  drivers/bluetooth/btqca.c   | 154 ++++++++++++++++++++++++++----------
> >>>>>>  drivers/bluetooth/btqca.h   |   5 +-
> >>>>>>  drivers/bluetooth/hci_qca.c |  21 ++++-
> >>>>>>  3 files changed, 134 insertions(+), 46 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> >>>>>> index dfbbac922..e8b89b8cc 100644
> >>>>>> --- a/drivers/bluetooth/btqca.c
> >>>>>> +++ b/drivers/bluetooth/btqca.c
> >>>>>> @@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
> >>>>>>  }
> >>>>>>  EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
> >>>>>>
> >>>>>> +static int qca_get_alt_nvm_path(char *path, size_t max_size)
> >>>>>
> >>>>> int is usually for errors, the code suggests bool return type.
> >>>>>
> >>>> Ack.
> >>>>>> +{
> >>>>>> +    char fwname[64];
> >>>>>> +    const char *suffix;
> >>>>>> +
> >>>>>> +    suffix = strrchr(path, '.');
> >>>>>> +
> >>>>>> +    if (!suffix)
> >>>>>> +            return 0;
> >>>>>> +
> >>>>>> +    strscpy(fwname, path, strlen(path));
> >>>>>
> >>>>> 64 bytes ought to be enough for anybody, correct?
> >>>>>
> >>>> Yes, in current driver, the max f/w path length is 64.
> >>>>
> >>>>>> +    fwname[suffix - path] = 0;
> >>>>>
> >>>>> with path = "qcom/sc7180/Oh.My.Device/name" this is broken.
> >>>>>
> >>>> Let me test this and fix in next patch.
> >>>>>> +
> >>>>>> +    snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
> >>>>>> +
> >>>>>> +    /* If nvm file is already the default one, return false to
> >>>>>> +     * skip the retry.
> >>>>>> +     */
> >>>>>> +    if (strcmp(fwname, path) == 0)
> >>>>>> +            return 0;
> >>>>>> +
> >>>>>> +    snprintf(path, max_size, "%s", fwname);
> >>>>>> +    return 1;
> >>>>>> +}
> >>>>>> +
> >>>>>>  static int qca_tlv_check_data(struct hci_dev *hdev,
> >>>>>>                             struct qca_fw_config *config,
> >>>>>>                             u8 *fw_data, size_t fw_size,
> >>>>>> @@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
> >>>>>>                                         config->fwname, ret);
> >>>>>>                              return ret;
> >>>>>>                      }
> >>>>>> +            }
> >>>>>> +            /* For nvm, if desired nvm file is not present and it's not the
> >>>>>> +             * default nvm file(ends with .bin), try to load the default nvm.
> >>>>>> +             */
> >>>>>> +            else if (config->type == TLV_TYPE_NVM &&
> >>>>>> +                     qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {
> >>>>>
> >>>>> Please, don't rewrite the config. The file may be not present now, but
> >>>>> it will reappear later (e.g. when rootfs gets mounted).
> >>>>>
> >>>> This tries to load a default NVM file if the board-specific NVM is not found.
> >>>> It is called when request_firmware fails. It's safe to rewrite the config->fwname
> >>>> here since we have already tried to load the board-specific NVM. The config
> >>>> is a local variable in qca_uart_setup and will return after downloading the NVM.
> >>>
> >>> Please read my question before answering it.
> >>>
> >> Sorry, I'm not clear about your question. Could you please explain it in more detail?
> >> I'm not quite sure how the situation you mentioned affects this code flow if you mean
> >> not downloading another NVM file.
> >>
> >> The board-specific NVM and the default NVM should be in the same folder and should
> >> appear simultaneously.
> >>
> >> From the Bluetooth firmware load flow perspective, the firmware is loaded either
> >> when the kernel module is inserted (insmod) or when Bluetooth is turned off and
> >> then on again via a user-space command. If the firmware is not found at this time,
> >> the ROM code is used instead. It does not attempt to load the firmware automatically,
> >> even if the firmware appears later.
> >
> > I was thinking about the following scenario:
> >
> > - BT firmware is attempted to load during driver probe, /lib/firmware is
> >   not fully populated, so the config is rewritten to use the default
> > - rootfs is fully mounted and populated with the board-specific file
> > - BT interface is being turned on. It is expected that the
> >   board-specific file will be loaded, however because the config was
> >   changed in one of the previous steps, the driver still loads the
> >   default one.
> >
> > That said, the driver should perform the fallback, etc, but the config
> > should stay intact even in the fallback case.
> >
> Thank you for the detail explanation. Current flow of BT enable in driver
> likes this:
>
> Enable the soc(Assert BT_EN) -->  read the SOC info --> Change baud rate -->
> get rampatch file name (based on soc info or dts) --> download rampatch -->
> get nvm file name(based on soc info or dts) --> download nvm file -->
> download default nvm (if the board-specific file not found).
>
> Every time the driver probe or the BT interface is turned on, it follows the
> flow described above. The rampatch and NVM file names are reconstructed by
> the SoC information each time, so the driver always attempts to download the
> board-specific file first.
>
> Here is the log, there is no hpnv21.b206 and re-insmod the driver.

You are re-insmodding the driver. I was talking about a different scenario:
- there is no BDF
- modprobe the driver
- wait for the hci0 to become available
- hciconfig hci0 down
- provide BDF
- hciconfig hci0 up

Check the dmesg. If everything is implemented correctly, second
hciconfig command should load the firmware files again (because BT was
unpowered in between). Second time it should load the proper board
file instead of loading the default or falling back to the ROM.

> [11850.644220] Bluetooth: HCI UART driver ver 2.3
> [11850.644232] Bluetooth: HCI UART protocol H4 registered
> [11850.644284] Bluetooth: HCI UART protocol LL registered
> [11850.644314] Bluetooth: HCI UART protocol QCA registered
> [11850.645055] Bluetooth: hci0: setting up wcn6855
> [11850.706962] Bluetooth: hci0: QCA Product ID   :0x00000013
> [11850.706975] Bluetooth: hci0: QCA SOC Version  :0x400c0210
> [11850.706978] Bluetooth: hci0: QCA ROM Version  :0x00000201
> [11850.706981] Bluetooth: hci0: QCA Patch Version:0x000038e6
> [11850.714508] Bluetooth: hci0: QCA controller version 0x02100201
> [11850.714518] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
> [11851.406475] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
> [11851.406515] bluetooth hci0: Direct firmware load for qca/QCA6698/hpnv21.b206 failed with error -2
> [11851.406522] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.bin
> [11851.570125] Bluetooth: hci0: QCA setup on UART is completed
>
> hpnv21.b206 exists and then re-insmod the driver.
> [11878.551494] Bluetooth: HCI UART driver ver 2.3
> [11878.551505] Bluetooth: HCI UART protocol H4 registered
> [11878.551553] Bluetooth: HCI UART protocol LL registered
> [11878.551580] Bluetooth: HCI UART protocol QCA registered
> [11878.552131] Bluetooth: hci0: setting up wcn6855
> [11878.618865] Bluetooth: hci0: QCA Product ID   :0x00000013
> [11878.618877] Bluetooth: hci0: QCA SOC Version  :0x400c0210
> [11878.618881] Bluetooth: hci0: QCA ROM Version  :0x00000201
> [11878.618884] Bluetooth: hci0: QCA Patch Version:0x000038e6
> [11878.629674] Bluetooth: hci0: QCA controller version 0x02100201
> [11878.629681] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
> [11879.318475] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
> [11879.482082] Bluetooth: hci0: QCA setup on UART is completed
> [11879.505086] Bluetooth: MGMT ver 1.22
>
> Turn on BT has the similar log.
> >>
> >>>>>> +                    bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
> >>>>>> +                    ret = request_firmware(&fw, config->fwname, &hdev->dev);
> >>>>>> +                    if (ret) {
> >>>>>> +                            bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> >>>>>> +                                       config->fwname, ret);
> >>>>>> +                            return ret;
> >>>>>> +                    }
> >>>>>>              } else {
> >>>>>>                      bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> >>>>>>                                 config->fwname, ret);
> >>>>>> @@ -730,15 +768,38 @@ 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_nvm_name_by_board(char *fwname, size_t max_size,
> >>>>>> +            const char *firmware_name, struct qca_btsoc_version ver,
> >>>>>> +            enum qca_btsoc_type soc_type, u16 bid)
> >>>>>> +{
> >>>>>> +    const char *variant;
> >>>>>> +
> >>>>>> +    /* Set the variant to empty by default */
> >>>>>> +    variant = "";
> >>>>>> +    /* hsp gf chip */
> >>>>>> +    if (soc_type == QCA_WCN6855) {
> >>>>>> +            if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
> >>>>>> +                    variant = "g";
> >>>>>
> >>>>> Didn't you get the 'set but unused' here?
> >>>>>
> >>>> Yes, miss this part. Thank you!
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    if (bid == 0x0)
> >>>>>
> >>>>> 0x0 or 0xff?
> >>>> board is set to 0 by default, 0x0 means read board id fails, then we should use
> >>>> the default one.
> >>>
> >>> What is the 'unprogrammed' board_id? On the WiFi side it's usually 0xff.
> >>>
> >> Yes, the 'unprogrammed' board_id should be 0xffff. Then 0 and 0xffff should use the
> >> default nvm.
> >
> > Good. I think it's safe to safe board_id to 0xffff by default, then you
> > don't have to handle '0' specially.
> >
> >>>>>
> >>>>>> +            snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
> >>>>>> +    else if (bid & 0xff00)
> >>>>>> +            snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);
> >>>>>
> >>>>> Doesn't ".b%02x" work in this case too?
> >>>>>
> >>>> No, board id are two bytes, it coudl be 0x0206, then we need .b206. Or it is
> >>>> 0x000a, then we need .b0a.
> >>>
> >>> What will ".b%02x" write in those two cases?
> >>>
> >> Yes, it works for both cases. Thanks!
> >
> > :-)
> >
> >>>>>> +    else
> >>>>>> +            snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
> >>>>>> +}
> >>>>>> +
> >>>
> >>>
> >>
> >
>


-- 
With best wishes
Dmitry

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Cheng Jiang (IOE)]

Hi Dmitry,

On 12/10/2024 12:04 AM, Dmitry Baryshkov wrote:
> On Mon, 9 Dec 2024 at 15:59, Cheng Jiang (IOE)
> <quic_chejiang@quicinc.com> wrote:
>>
>> Hi Dmitry,
>>
>> On 12/9/2024 6:49 PM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 09, 2024 at 05:03:55PM +0800, Cheng Jiang (IOE) wrote:
>>>> Hi Dmitry,
>>>>
>>>> On 12/6/2024 4:34 PM, Dmitry Baryshkov wrote:
>>>>> On Fri, 6 Dec 2024 at 05:05, Cheng Jiang (IOE)
>>>>> <quic_chejiang@quicinc.com> wrote:
>>>>>>
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> On 12/5/2024 8:00 PM, Dmitry Baryshkov wrote:
>>>>>>> On Thu, Dec 05, 2024 at 06:22:12PM +0800, Cheng Jiang wrote:
>>>>>>>> The firmware-name property has been expanded to specify the names of NVM
>>>>>>>> and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
>>>>>>>> chip. Although it shares the same IP core as the WCN6855, the QCA6698
>>>>>>>> has different RF components and RAM sizes, necessitating new firmware
>>>>>>>> files. This change allows for the configuration of NVM and rampatch in
>>>>>>>> DT.
>>>>>>>>
>>>>>>>> Different connectivity boards may be attached to the same platform. For
>>>>>>>> example, QCA6698-based boards can support either a two-antenna or
>>>>>>>> three-antenna solution, both of which work on the sa8775p-ride platform.
>>>>>>>> Due to differences in connectivity boards and variations in RF
>>>>>>>> performance from different foundries, different NVM configurations are
>>>>>>>> used based on the board ID.
>>>>>>>
>>>>>>> Two separate commits, one for NVM, another one for RAM patch.
>>>>>>>
>>>>>> Ack.
>>>>>>>>
>>>>>>>> Therefore, in the firmware-name property, if the NVM file has an
>>>>>>>> extension, the NVM file will be used. Otherwise, the system will first
>>>>>>>> try the .bNN (board ID) file, and if that fails, it will fall back to
>>>>>>>> the .bin file.
>>>>>>>>
>>>>>>>> Possible configurations:
>>>>>>>> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
>>>>>>>> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
>>>>>>>> firmware-name = "QCA6698/hpnv21.bin";
>>>>>>>>
>>>>>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>>>>>> ---
>>>>>>>>  drivers/bluetooth/btqca.c   | 154 ++++++++++++++++++++++++++----------
>>>>>>>>  drivers/bluetooth/btqca.h   |   5 +-
>>>>>>>>  drivers/bluetooth/hci_qca.c |  21 ++++-
>>>>>>>>  3 files changed, 134 insertions(+), 46 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>>>>>>>> index dfbbac922..e8b89b8cc 100644
>>>>>>>> --- a/drivers/bluetooth/btqca.c
>>>>>>>> +++ b/drivers/bluetooth/btqca.c
>>>>>>>> @@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
>>>>>>>>  }
>>>>>>>>  EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
>>>>>>>>
>>>>>>>> +static int qca_get_alt_nvm_path(char *path, size_t max_size)
>>>>>>>
>>>>>>> int is usually for errors, the code suggests bool return type.
>>>>>>>
>>>>>> Ack.
>>>>>>>> +{
>>>>>>>> +    char fwname[64];
>>>>>>>> +    const char *suffix;
>>>>>>>> +
>>>>>>>> +    suffix = strrchr(path, '.');
>>>>>>>> +
>>>>>>>> +    if (!suffix)
>>>>>>>> +            return 0;
>>>>>>>> +
>>>>>>>> +    strscpy(fwname, path, strlen(path));
>>>>>>>
>>>>>>> 64 bytes ought to be enough for anybody, correct?
>>>>>>>
>>>>>> Yes, in current driver, the max f/w path length is 64.
>>>>>>
>>>>>>>> +    fwname[suffix - path] = 0;
>>>>>>>
>>>>>>> with path = "qcom/sc7180/Oh.My.Device/name" this is broken.
>>>>>>>
>>>>>> Let me test this and fix in next patch.
>>>>>>>> +
>>>>>>>> +    snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
>>>>>>>> +
>>>>>>>> +    /* If nvm file is already the default one, return false to
>>>>>>>> +     * skip the retry.
>>>>>>>> +     */
>>>>>>>> +    if (strcmp(fwname, path) == 0)
>>>>>>>> +            return 0;
>>>>>>>> +
>>>>>>>> +    snprintf(path, max_size, "%s", fwname);
>>>>>>>> +    return 1;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  static int qca_tlv_check_data(struct hci_dev *hdev,
>>>>>>>>                             struct qca_fw_config *config,
>>>>>>>>                             u8 *fw_data, size_t fw_size,
>>>>>>>> @@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
>>>>>>>>                                         config->fwname, ret);
>>>>>>>>                              return ret;
>>>>>>>>                      }
>>>>>>>> +            }
>>>>>>>> +            /* For nvm, if desired nvm file is not present and it's not the
>>>>>>>> +             * default nvm file(ends with .bin), try to load the default nvm.
>>>>>>>> +             */
>>>>>>>> +            else if (config->type == TLV_TYPE_NVM &&
>>>>>>>> +                     qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {
>>>>>>>
>>>>>>> Please, don't rewrite the config. The file may be not present now, but
>>>>>>> it will reappear later (e.g. when rootfs gets mounted).
>>>>>>>
>>>>>> This tries to load a default NVM file if the board-specific NVM is not found.
>>>>>> It is called when request_firmware fails. It's safe to rewrite the config->fwname
>>>>>> here since we have already tried to load the board-specific NVM. The config
>>>>>> is a local variable in qca_uart_setup and will return after downloading the NVM.
>>>>>
>>>>> Please read my question before answering it.
>>>>>
>>>> Sorry, I'm not clear about your question. Could you please explain it in more detail?
>>>> I'm not quite sure how the situation you mentioned affects this code flow if you mean
>>>> not downloading another NVM file.
>>>>
>>>> The board-specific NVM and the default NVM should be in the same folder and should
>>>> appear simultaneously.
>>>>
>>>> From the Bluetooth firmware load flow perspective, the firmware is loaded either
>>>> when the kernel module is inserted (insmod) or when Bluetooth is turned off and
>>>> then on again via a user-space command. If the firmware is not found at this time,
>>>> the ROM code is used instead. It does not attempt to load the firmware automatically,
>>>> even if the firmware appears later.
>>>
>>> I was thinking about the following scenario:
>>>
>>> - BT firmware is attempted to load during driver probe, /lib/firmware is
>>>   not fully populated, so the config is rewritten to use the default
>>> - rootfs is fully mounted and populated with the board-specific file
>>> - BT interface is being turned on. It is expected that the
>>>   board-specific file will be loaded, however because the config was
>>>   changed in one of the previous steps, the driver still loads the
>>>   default one.
>>>
>>> That said, the driver should perform the fallback, etc, but the config
>>> should stay intact even in the fallback case.
>>>
>> Thank you for the detail explanation. Current flow of BT enable in driver
>> likes this:
>>
>> Enable the soc(Assert BT_EN) -->  read the SOC info --> Change baud rate -->
>> get rampatch file name (based on soc info or dts) --> download rampatch -->
>> get nvm file name(based on soc info or dts) --> download nvm file -->
>> download default nvm (if the board-specific file not found).
>>
>> Every time the driver probe or the BT interface is turned on, it follows the
>> flow described above. The rampatch and NVM file names are reconstructed by
>> the SoC information each time, so the driver always attempts to download the
>> board-specific file first.
>>
>> Here is the log, there is no hpnv21.b206 and re-insmod the driver.
> 
> You are re-insmodding the driver. I was talking about a different scenario:
> - there is no BDF
> - modprobe the driver
> - wait for the hci0 to become available
> - hciconfig hci0 down
> - provide BDF
> - hciconfig hci0 up
> 
> Check the dmesg. If everything is implemented correctly, second
> hciconfig command should load the firmware files again (because BT was
> unpowered in between). Second time it should load the proper board
> file instead of loading the default or falling back to the ROM.
> 
Yes, the 'hciconfig hci0 up' will load the proper board file, since it also follows 
the flow described above. 

Here is the dmesg:

sh-5.1# mv hpnv21.b206 hpnv21.b2069                 -- Remove the board specific nvm
sh-5.1# rmmod hci_uart
sh-5.1# insmod /lib/modules/6.6.52-dirty/kernel/drivers/bluetooth/hci_uart.ko
sh-5.1# dmesg|grep -i bluetooth

[54781.019527] Bluetooth: HCI UART driver ver 2.3
[54781.019538] Bluetooth: HCI UART protocol H4 registered
[54781.019589] Bluetooth: HCI UART protocol LL registered
[54781.019612] Bluetooth: HCI UART protocol QCA registered
[54781.020893] Bluetooth: hci0: setting up wcn6855
[54781.087027] Bluetooth: hci0: QCA Product ID   :0x00000013
[54781.087037] Bluetooth: hci0: QCA SOC Version  :0x400c0210
[54781.087039] Bluetooth: hci0: QCA ROM Version  :0x00000201
[54781.087042] Bluetooth: hci0: QCA Patch Version:0x000038e6
[54781.104087] Bluetooth: hci0: QCA controller version 0x02100201
[54781.104097] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
[54781.794628] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
[54781.794671] bluetooth hci0: Direct firmware load for qca/QCA6698/hpnv21.b206 failed with error -2
[54781.794677] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.bin
[54781.958319] Bluetooth: hci0: QCA setup on UART is completed
[54781.981490] Bluetooth: MGMT ver 1.22

No board specific nvm found, use the default one.
Disable hci0 and add the board specific nvm, then enable hci0.

sh-5.1# hciconfig hci0 down
sh-5.1# mv hpnv21.b2069 hpnv21.b206
sh-5.1# hciconfig hci0 up
sh-5.1# dmesg|grep -i bluetooth
[54834.686170] Bluetooth: hci0: setting up wcn6855
[54834.750997] Bluetooth: hci0: QCA Product ID   :0x00000013
[54834.751006] Bluetooth: hci0: QCA SOC Version  :0x400c0210
[54834.751010] Bluetooth: hci0: QCA ROM Version  :0x00000201
[54834.751013] Bluetooth: hci0: QCA Patch Version:0x000038e6
[54834.761826] Bluetooth: hci0: QCA controller version 0x02100201
[54834.761833] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
[54835.450621] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
[54835.614015] Bluetooth: hci0: QCA setup on UART is completed

Load the board-specific nvm when enable hci0.

>> [11850.644220] Bluetooth: HCI UART driver ver 2.3
>> [11850.644232] Bluetooth: HCI UART protocol H4 registered
>> [11850.644284] Bluetooth: HCI UART protocol LL registered
>> [11850.644314] Bluetooth: HCI UART protocol QCA registered
>> [11850.645055] Bluetooth: hci0: setting up wcn6855
>> [11850.706962] Bluetooth: hci0: QCA Product ID   :0x00000013
>> [11850.706975] Bluetooth: hci0: QCA SOC Version  :0x400c0210
>> [11850.706978] Bluetooth: hci0: QCA ROM Version  :0x00000201
>> [11850.706981] Bluetooth: hci0: QCA Patch Version:0x000038e6
>> [11850.714508] Bluetooth: hci0: QCA controller version 0x02100201
>> [11850.714518] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
>> [11851.406475] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
>> [11851.406515] bluetooth hci0: Direct firmware load for qca/QCA6698/hpnv21.b206 failed with error -2
>> [11851.406522] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.bin
>> [11851.570125] Bluetooth: hci0: QCA setup on UART is completed
>>
>> hpnv21.b206 exists and then re-insmod the driver.
>> [11878.551494] Bluetooth: HCI UART driver ver 2.3
>> [11878.551505] Bluetooth: HCI UART protocol H4 registered
>> [11878.551553] Bluetooth: HCI UART protocol LL registered
>> [11878.551580] Bluetooth: HCI UART protocol QCA registered
>> [11878.552131] Bluetooth: hci0: setting up wcn6855
>> [11878.618865] Bluetooth: hci0: QCA Product ID   :0x00000013
>> [11878.618877] Bluetooth: hci0: QCA SOC Version  :0x400c0210
>> [11878.618881] Bluetooth: hci0: QCA ROM Version  :0x00000201
>> [11878.618884] Bluetooth: hci0: QCA Patch Version:0x000038e6
>> [11878.629674] Bluetooth: hci0: QCA controller version 0x02100201
>> [11878.629681] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
>> [11879.318475] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
>> [11879.482082] Bluetooth: hci0: QCA setup on UART is completed
>> [11879.505086] Bluetooth: MGMT ver 1.22
>>
>> Turn on BT has the similar log.
>>>>
>>>>>>>> +                    bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
>>>>>>>> +                    ret = request_firmware(&fw, config->fwname, &hdev->dev);
>>>>>>>> +                    if (ret) {
>>>>>>>> +                            bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>>>>>>>> +                                       config->fwname, ret);
>>>>>>>> +                            return ret;
>>>>>>>> +                    }
>>>>>>>>              } else {
>>>>>>>>                      bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>>>>>>>>                                 config->fwname, ret);
>>>>>>>> @@ -730,15 +768,38 @@ 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_nvm_name_by_board(char *fwname, size_t max_size,
>>>>>>>> +            const char *firmware_name, struct qca_btsoc_version ver,
>>>>>>>> +            enum qca_btsoc_type soc_type, u16 bid)
>>>>>>>> +{
>>>>>>>> +    const char *variant;
>>>>>>>> +
>>>>>>>> +    /* Set the variant to empty by default */
>>>>>>>> +    variant = "";
>>>>>>>> +    /* hsp gf chip */
>>>>>>>> +    if (soc_type == QCA_WCN6855) {
>>>>>>>> +            if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
>>>>>>>> +                    variant = "g";
>>>>>>>
>>>>>>> Didn't you get the 'set but unused' here?
>>>>>>>
>>>>>> Yes, miss this part. Thank you!
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    if (bid == 0x0)
>>>>>>>
>>>>>>> 0x0 or 0xff?
>>>>>> board is set to 0 by default, 0x0 means read board id fails, then we should use
>>>>>> the default one.
>>>>>
>>>>> What is the 'unprogrammed' board_id? On the WiFi side it's usually 0xff.
>>>>>
>>>> Yes, the 'unprogrammed' board_id should be 0xffff. Then 0 and 0xffff should use the
>>>> default nvm.
>>>
>>> Good. I think it's safe to safe board_id to 0xffff by default, then you
>>> don't have to handle '0' specially.
>>>
>>>>>>>
>>>>>>>> +            snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
>>>>>>>> +    else if (bid & 0xff00)
>>>>>>>> +            snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);
>>>>>>>
>>>>>>> Doesn't ".b%02x" work in this case too?
>>>>>>>
>>>>>> No, board id are two bytes, it coudl be 0x0206, then we need .b206. Or it is
>>>>>> 0x000a, then we need .b0a.
>>>>>
>>>>> What will ".b%02x" write in those two cases?
>>>>>
>>>> Yes, it works for both cases. Thanks!
>>>
>>> :-)
>>>
>>>>>>>> +    else
>>>>>>>> +            snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
>>>>>>>> +}
>>>>>>>> +
>>>>>
>>>>>
>>>>
>>>
>>
> 
> 

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Dmitry Baryshkov]

On Tue, Dec 10, 2024 at 10:00:51AM +0800, Cheng Jiang (IOE) wrote:
> Hi Dmitry,
> 
> On 12/10/2024 12:04 AM, Dmitry Baryshkov wrote:
> > On Mon, 9 Dec 2024 at 15:59, Cheng Jiang (IOE)
> > <quic_chejiang@quicinc.com> wrote:
> >>
> >> Hi Dmitry,
> >>
> >> On 12/9/2024 6:49 PM, Dmitry Baryshkov wrote:
> >>> On Mon, Dec 09, 2024 at 05:03:55PM +0800, Cheng Jiang (IOE) wrote:
> >>>> Hi Dmitry,
> >>>>
> >>>> On 12/6/2024 4:34 PM, Dmitry Baryshkov wrote:
> >>>>> On Fri, 6 Dec 2024 at 05:05, Cheng Jiang (IOE)
> >>>>> <quic_chejiang@quicinc.com> wrote:
> >>>>>>
> >>>>>> Hi Dmitry,
> >>>>>>
> >>>>>> On 12/5/2024 8:00 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Thu, Dec 05, 2024 at 06:22:12PM +0800, Cheng Jiang wrote:
> >>>>>>>> The firmware-name property has been expanded to specify the names of NVM
> >>>>>>>> and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
> >>>>>>>> chip. Although it shares the same IP core as the WCN6855, the QCA6698
> >>>>>>>> has different RF components and RAM sizes, necessitating new firmware
> >>>>>>>> files. This change allows for the configuration of NVM and rampatch in
> >>>>>>>> DT.
> >>>>>>>>
> >>>>>>>> Different connectivity boards may be attached to the same platform. For
> >>>>>>>> example, QCA6698-based boards can support either a two-antenna or
> >>>>>>>> three-antenna solution, both of which work on the sa8775p-ride platform.
> >>>>>>>> Due to differences in connectivity boards and variations in RF
> >>>>>>>> performance from different foundries, different NVM configurations are
> >>>>>>>> used based on the board ID.
> >>>>>>>
> >>>>>>> Two separate commits, one for NVM, another one for RAM patch.
> >>>>>>>
> >>>>>> Ack.
> >>>>>>>>
> >>>>>>>> Therefore, in the firmware-name property, if the NVM file has an
> >>>>>>>> extension, the NVM file will be used. Otherwise, the system will first
> >>>>>>>> try the .bNN (board ID) file, and if that fails, it will fall back to
> >>>>>>>> the .bin file.
> >>>>>>>>
> >>>>>>>> Possible configurations:
> >>>>>>>> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
> >>>>>>>> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
> >>>>>>>> firmware-name = "QCA6698/hpnv21.bin";
> >>>>>>>>
> >>>>>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
> >>>>>>>> ---
> >>>>>>>>  drivers/bluetooth/btqca.c   | 154 ++++++++++++++++++++++++++----------
> >>>>>>>>  drivers/bluetooth/btqca.h   |   5 +-
> >>>>>>>>  drivers/bluetooth/hci_qca.c |  21 ++++-
> >>>>>>>>  3 files changed, 134 insertions(+), 46 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> >>>>>>>> index dfbbac922..e8b89b8cc 100644
> >>>>>>>> --- a/drivers/bluetooth/btqca.c
> >>>>>>>> +++ b/drivers/bluetooth/btqca.c
> >>>>>>>> @@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
> >>>>>>>>  }
> >>>>>>>>  EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
> >>>>>>>>
> >>>>>>>> +static int qca_get_alt_nvm_path(char *path, size_t max_size)
> >>>>>>>
> >>>>>>> int is usually for errors, the code suggests bool return type.
> >>>>>>>
> >>>>>> Ack.
> >>>>>>>> +{
> >>>>>>>> +    char fwname[64];
> >>>>>>>> +    const char *suffix;
> >>>>>>>> +
> >>>>>>>> +    suffix = strrchr(path, '.');
> >>>>>>>> +
> >>>>>>>> +    if (!suffix)
> >>>>>>>> +            return 0;
> >>>>>>>> +
> >>>>>>>> +    strscpy(fwname, path, strlen(path));
> >>>>>>>
> >>>>>>> 64 bytes ought to be enough for anybody, correct?
> >>>>>>>
> >>>>>> Yes, in current driver, the max f/w path length is 64.
> >>>>>>
> >>>>>>>> +    fwname[suffix - path] = 0;
> >>>>>>>
> >>>>>>> with path = "qcom/sc7180/Oh.My.Device/name" this is broken.
> >>>>>>>
> >>>>>> Let me test this and fix in next patch.
> >>>>>>>> +
> >>>>>>>> +    snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
> >>>>>>>> +
> >>>>>>>> +    /* If nvm file is already the default one, return false to
> >>>>>>>> +     * skip the retry.
> >>>>>>>> +     */
> >>>>>>>> +    if (strcmp(fwname, path) == 0)
> >>>>>>>> +            return 0;
> >>>>>>>> +
> >>>>>>>> +    snprintf(path, max_size, "%s", fwname);
> >>>>>>>> +    return 1;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>>  static int qca_tlv_check_data(struct hci_dev *hdev,
> >>>>>>>>                             struct qca_fw_config *config,
> >>>>>>>>                             u8 *fw_data, size_t fw_size,
> >>>>>>>> @@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
> >>>>>>>>                                         config->fwname, ret);
> >>>>>>>>                              return ret;
> >>>>>>>>                      }
> >>>>>>>> +            }
> >>>>>>>> +            /* For nvm, if desired nvm file is not present and it's not the
> >>>>>>>> +             * default nvm file(ends with .bin), try to load the default nvm.
> >>>>>>>> +             */
> >>>>>>>> +            else if (config->type == TLV_TYPE_NVM &&
> >>>>>>>> +                     qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {
> >>>>>>>
> >>>>>>> Please, don't rewrite the config. The file may be not present now, but
> >>>>>>> it will reappear later (e.g. when rootfs gets mounted).
> >>>>>>>
> >>>>>> This tries to load a default NVM file if the board-specific NVM is not found.
> >>>>>> It is called when request_firmware fails. It's safe to rewrite the config->fwname
> >>>>>> here since we have already tried to load the board-specific NVM. The config
> >>>>>> is a local variable in qca_uart_setup and will return after downloading the NVM.
> >>>>>
> >>>>> Please read my question before answering it.
> >>>>>
> >>>> Sorry, I'm not clear about your question. Could you please explain it in more detail?
> >>>> I'm not quite sure how the situation you mentioned affects this code flow if you mean
> >>>> not downloading another NVM file.
> >>>>
> >>>> The board-specific NVM and the default NVM should be in the same folder and should
> >>>> appear simultaneously.
> >>>>
> >>>> From the Bluetooth firmware load flow perspective, the firmware is loaded either
> >>>> when the kernel module is inserted (insmod) or when Bluetooth is turned off and
> >>>> then on again via a user-space command. If the firmware is not found at this time,
> >>>> the ROM code is used instead. It does not attempt to load the firmware automatically,
> >>>> even if the firmware appears later.
> >>>
> >>> I was thinking about the following scenario:
> >>>
> >>> - BT firmware is attempted to load during driver probe, /lib/firmware is
> >>>   not fully populated, so the config is rewritten to use the default
> >>> - rootfs is fully mounted and populated with the board-specific file
> >>> - BT interface is being turned on. It is expected that the
> >>>   board-specific file will be loaded, however because the config was
> >>>   changed in one of the previous steps, the driver still loads the
> >>>   default one.
> >>>
> >>> That said, the driver should perform the fallback, etc, but the config
> >>> should stay intact even in the fallback case.
> >>>
> >> Thank you for the detail explanation. Current flow of BT enable in driver
> >> likes this:
> >>
> >> Enable the soc(Assert BT_EN) -->  read the SOC info --> Change baud rate -->
> >> get rampatch file name (based on soc info or dts) --> download rampatch -->
> >> get nvm file name(based on soc info or dts) --> download nvm file -->
> >> download default nvm (if the board-specific file not found).
> >>
> >> Every time the driver probe or the BT interface is turned on, it follows the
> >> flow described above. The rampatch and NVM file names are reconstructed by
> >> the SoC information each time, so the driver always attempts to download the
> >> board-specific file first.
> >>
> >> Here is the log, there is no hpnv21.b206 and re-insmod the driver.
> > 
> > You are re-insmodding the driver. I was talking about a different scenario:
> > - there is no BDF
> > - modprobe the driver
> > - wait for the hci0 to become available
> > - hciconfig hci0 down
> > - provide BDF
> > - hciconfig hci0 up
> > 
> > Check the dmesg. If everything is implemented correctly, second
> > hciconfig command should load the firmware files again (because BT was
> > unpowered in between). Second time it should load the proper board
> > file instead of loading the default or falling back to the ROM.
> > 
> Yes, the 'hciconfig hci0 up' will load the proper board file, since it also follows 
> the flow described above. 
> 
> Here is the dmesg:
> 
> sh-5.1# mv hpnv21.b206 hpnv21.b2069                 -- Remove the board specific nvm
> sh-5.1# rmmod hci_uart
> sh-5.1# insmod /lib/modules/6.6.52-dirty/kernel/drivers/bluetooth/hci_uart.ko
> sh-5.1# dmesg|grep -i bluetooth
> 
> [54781.019527] Bluetooth: HCI UART driver ver 2.3
> [54781.019538] Bluetooth: HCI UART protocol H4 registered
> [54781.019589] Bluetooth: HCI UART protocol LL registered
> [54781.019612] Bluetooth: HCI UART protocol QCA registered
> [54781.020893] Bluetooth: hci0: setting up wcn6855
> [54781.087027] Bluetooth: hci0: QCA Product ID   :0x00000013
> [54781.087037] Bluetooth: hci0: QCA SOC Version  :0x400c0210
> [54781.087039] Bluetooth: hci0: QCA ROM Version  :0x00000201
> [54781.087042] Bluetooth: hci0: QCA Patch Version:0x000038e6
> [54781.104087] Bluetooth: hci0: QCA controller version 0x02100201
> [54781.104097] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
> [54781.794628] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
> [54781.794671] bluetooth hci0: Direct firmware load for qca/QCA6698/hpnv21.b206 failed with error -2
> [54781.794677] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.bin
> [54781.958319] Bluetooth: hci0: QCA setup on UART is completed
> [54781.981490] Bluetooth: MGMT ver 1.22
> 
> No board specific nvm found, use the default one.
> Disable hci0 and add the board specific nvm, then enable hci0.
> 
> sh-5.1# hciconfig hci0 down
> sh-5.1# mv hpnv21.b2069 hpnv21.b206
> sh-5.1# hciconfig hci0 up
> sh-5.1# dmesg|grep -i bluetooth
> [54834.686170] Bluetooth: hci0: setting up wcn6855
> [54834.750997] Bluetooth: hci0: QCA Product ID   :0x00000013
> [54834.751006] Bluetooth: hci0: QCA SOC Version  :0x400c0210
> [54834.751010] Bluetooth: hci0: QCA ROM Version  :0x00000201
> [54834.751013] Bluetooth: hci0: QCA Patch Version:0x000038e6
> [54834.761826] Bluetooth: hci0: QCA controller version 0x02100201
> [54834.761833] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpbtfw21.tlv
> [54835.450621] Bluetooth: hci0: QCA Downloading qca/QCA6698/hpnv21.b206
> [54835.614015] Bluetooth: hci0: QCA setup on UART is completed
> 
> Load the board-specific nvm when enable hci0.

Ack, thanks for the confirmation.

Please post the next iteration, I'll R-B it.

-- 
With best wishes
Dmitry

Re: [PATCH v3 2/3] Bluetooth: qca: Expand firmware-name to load specific nvm and rampatch

[Cheng Jiang (IOE)]

Hi Dmitry,

On 12/9/2024 6:49 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 09, 2024 at 05:03:55PM +0800, Cheng Jiang (IOE) wrote:
>> Hi Dmitry,
>>
>> On 12/6/2024 4:34 PM, Dmitry Baryshkov wrote:
>>> On Fri, 6 Dec 2024 at 05:05, Cheng Jiang (IOE)
>>> <quic_chejiang@quicinc.com> wrote:
>>>>
>>>> Hi Dmitry,
>>>>
>>>> On 12/5/2024 8:00 PM, Dmitry Baryshkov wrote:
>>>>> On Thu, Dec 05, 2024 at 06:22:12PM +0800, Cheng Jiang wrote:
>>>>>> The firmware-name property has been expanded to specify the names of NVM
>>>>>> and rampatch firmware for certain chips, such as the QCA6698 Bluetooth
>>>>>> chip. Although it shares the same IP core as the WCN6855, the QCA6698
>>>>>> has different RF components and RAM sizes, necessitating new firmware
>>>>>> files. This change allows for the configuration of NVM and rampatch in
>>>>>> DT.
>>>>>>
>>>>>> Different connectivity boards may be attached to the same platform. For
>>>>>> example, QCA6698-based boards can support either a two-antenna or
>>>>>> three-antenna solution, both of which work on the sa8775p-ride platform.
>>>>>> Due to differences in connectivity boards and variations in RF
>>>>>> performance from different foundries, different NVM configurations are
>>>>>> used based on the board ID.
>>>>>
>>>>> Two separate commits, one for NVM, another one for RAM patch.
>>>>>
>>>> Ack.
>>>>>>
>>>>>> Therefore, in the firmware-name property, if the NVM file has an
>>>>>> extension, the NVM file will be used. Otherwise, the system will first
>>>>>> try the .bNN (board ID) file, and if that fails, it will fall back to
>>>>>> the .bin file.
>>>>>>
>>>>>> Possible configurations:
>>>>>> firmware-name = "QCA6698/hpnv21.bin", "QCA6698/hpbtfw21.tlv";
>>>>>> firmware-name = "QCA6698/hpnv21", "QCA6698/hpbtfw21.tlv";
>>>>>> firmware-name = "QCA6698/hpnv21.bin";
>>>>>>
>>>>>> Signed-off-by: Cheng Jiang <quic_chejiang@quicinc.com>
>>>>>> ---
>>>>>>  drivers/bluetooth/btqca.c   | 154 ++++++++++++++++++++++++++----------
>>>>>>  drivers/bluetooth/btqca.h   |   5 +-
>>>>>>  drivers/bluetooth/hci_qca.c |  21 ++++-
>>>>>>  3 files changed, 134 insertions(+), 46 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>>>>>> index dfbbac922..e8b89b8cc 100644
>>>>>> --- a/drivers/bluetooth/btqca.c
>>>>>> +++ b/drivers/bluetooth/btqca.c
>>>>>> @@ -272,6 +272,31 @@ int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
>>>>>>  }
>>>>>>  EXPORT_SYMBOL_GPL(qca_send_pre_shutdown_cmd);
>>>>>>
>>>>>> +static int qca_get_alt_nvm_path(char *path, size_t max_size)
>>>>>
>>>>> int is usually for errors, the code suggests bool return type.
>>>>>
>>>> Ack.
>>>>>> +{
>>>>>> +    char fwname[64];
>>>>>> +    const char *suffix;
>>>>>> +
>>>>>> +    suffix = strrchr(path, '.');
>>>>>> +
>>>>>> +    if (!suffix)
>>>>>> +            return 0;
>>>>>> +
>>>>>> +    strscpy(fwname, path, strlen(path));
>>>>>
>>>>> 64 bytes ought to be enough for anybody, correct?
>>>>>
>>>> Yes, in current driver, the max f/w path length is 64.
>>>>
>>>>>> +    fwname[suffix - path] = 0;
>>>>>
>>>>> with path = "qcom/sc7180/Oh.My.Device/name" this is broken.
>>>>>
>>>> Let me test this and fix in next patch.
>>>>>> +
>>>>>> +    snprintf(fwname, sizeof(fwname), "%s.bin", fwname);
>>>>>> +
>>>>>> +    /* If nvm file is already the default one, return false to
>>>>>> +     * skip the retry.
>>>>>> +     */
>>>>>> +    if (strcmp(fwname, path) == 0)
>>>>>> +            return 0;
>>>>>> +
>>>>>> +    snprintf(path, max_size, "%s", fwname);
>>>>>> +    return 1;
>>>>>> +}
>>>>>> +
>>>>>>  static int qca_tlv_check_data(struct hci_dev *hdev,
>>>>>>                             struct qca_fw_config *config,
>>>>>>                             u8 *fw_data, size_t fw_size,
>>>>>> @@ -564,6 +589,19 @@ static int qca_download_firmware(struct hci_dev *hdev,
>>>>>>                                         config->fwname, ret);
>>>>>>                              return ret;
>>>>>>                      }
>>>>>> +            }
>>>>>> +            /* For nvm, if desired nvm file is not present and it's not the
>>>>>> +             * default nvm file(ends with .bin), try to load the default nvm.
>>>>>> +             */
>>>>>> +            else if (config->type == TLV_TYPE_NVM &&
>>>>>> +                     qca_get_alt_nvm_path(config->fwname, sizeof(config->fwname))) {
>>>>>
>>>>> Please, don't rewrite the config. The file may be not present now, but
>>>>> it will reappear later (e.g. when rootfs gets mounted).
>>>>>
>>>> This tries to load a default NVM file if the board-specific NVM is not found.
>>>> It is called when request_firmware fails. It's safe to rewrite the config->fwname
>>>> here since we have already tried to load the board-specific NVM. The config
>>>> is a local variable in qca_uart_setup and will return after downloading the NVM.
>>>
>>> Please read my question before answering it.
>>>
>> Sorry, I'm not clear about your question. Could you please explain it in more detail? 
>> I'm not quite sure how the situation you mentioned affects this code flow if you mean
>> not downloading another NVM file.
>>
>> The board-specific NVM and the default NVM should be in the same folder and should
>> appear simultaneously.
>>
>> From the Bluetooth firmware load flow perspective, the firmware is loaded either 
>> when the kernel module is inserted (insmod) or when Bluetooth is turned off and 
>> then on again via a user-space command. If the firmware is not found at this time, 
>> the ROM code is used instead. It does not attempt to load the firmware automatically,
>> even if the firmware appears later.
> 
> I was thinking about the following scenario:
> 
> - BT firmware is attempted to load during driver probe, /lib/firmware is
>   not fully populated, so the config is rewritten to use the default
> - rootfs is fully mounted and populated with the board-specific file
> - BT interface is being turned on. It is expected that the
>   board-specific file will be loaded, however because the config was
>   changed in one of the previous steps, the driver still loads the
>   default one.
> 
> That said, the driver should perform the fallback, etc, but the config
> should stay intact even in the fallback case.
> 
>>
>>>>>> +                    bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
>>>>>> +                    ret = request_firmware(&fw, config->fwname, &hdev->dev);
>>>>>> +                    if (ret) {
>>>>>> +                            bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>>>>>> +                                       config->fwname, ret);
>>>>>> +                            return ret;
>>>>>> +                    }
>>>>>>              } else {
>>>>>>                      bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>>>>>>                                 config->fwname, ret);
>>>>>> @@ -730,15 +768,38 @@ 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_nvm_name_by_board(char *fwname, size_t max_size,
>>>>>> +            const char *firmware_name, struct qca_btsoc_version ver,
>>>>>> +            enum qca_btsoc_type soc_type, u16 bid)
>>>>>> +{
>>>>>> +    const char *variant;
>>>>>> +
>>>>>> +    /* Set the variant to empty by default */
>>>>>> +    variant = "";
>>>>>> +    /* hsp gf chip */
>>>>>> +    if (soc_type == QCA_WCN6855) {
>>>>>> +            if ((le32_to_cpu(ver.soc_id) & QCA_HSP_GF_SOC_MASK) == QCA_HSP_GF_SOC_ID)
>>>>>> +                    variant = "g";
>>>>>
>>>>> Didn't you get the 'set but unused' here?
>>>>>
>>>> Yes, miss this part. Thank you!
>>>>>> +    }
>>>>>> +
>>>>>> +    if (bid == 0x0)
>>>>>
>>>>> 0x0 or 0xff?
>>>> board is set to 0 by default, 0x0 means read board id fails, then we should use
>>>> the default one.
>>>
>>> What is the 'unprogrammed' board_id? On the WiFi side it's usually 0xff.
>>>
>> Yes, the 'unprogrammed' board_id should be 0xffff. Then 0 and 0xffff should use the
>> default nvm.  
> 
> Good. I think it's safe to safe board_id to 0xffff by default, then you
> don't have to handle '0' specially.
> 
Sorry, I missed this comment, we have read board id of 0 from some boards in other project.
So it's better to check both 0 and 0xffff. It aligns with current driver implementation. 
>>>>>
>>>>>> +            snprintf(fwname, max_size, "qca/%s.bin", firmware_name);
>>>>>> +    else if (bid & 0xff00)
>>>>>> +            snprintf(fwname, max_size, "qca/%s.b%x", firmware_name, bid);
>>>>>
>>>>> Doesn't ".b%02x" work in this case too?
>>>>>
>>>> No, board id are two bytes, it coudl be 0x0206, then we need .b206. Or it is
>>>> 0x000a, then we need .b0a.
>>>
>>> What will ".b%02x" write in those two cases?
>>>
>> Yes, it works for both cases. Thanks! 
> 
> :-)
> 
>>>>>> +    else
>>>>>> +            snprintf(fwname, max_size, "qca/%s.b%02x", firmware_name, bid);
>>>>>> +}
>>>>>> +
>>>
>>>
>>
> 

RE: Expand firmware-name property to load specific

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi:857
error: arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

RE: Expand firmware-name property to load specific

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi:857
error: arch/arm64/boot/dts/qcom/sa8775p-ride.dtsi: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

RE: Expand firmware-name property to load specific

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2460 bytes --]

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

Dear submitter,

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

---Test result---

Test Summary:
CheckPatch                    PENDING   0.24 seconds
GitLint                       PENDING   0.24 seconds
SubjectPrefix                 FAIL      0.60 seconds
BuildKernel                   PASS      24.81 seconds
CheckAllWarning               PASS      27.29 seconds
CheckSparse                   PASS      30.85 seconds
BuildKernel32                 PASS      24.89 seconds
TestRunnerSetup               PASS      435.80 seconds
TestRunner_l2cap-tester       PASS      20.49 seconds
TestRunner_iso-tester         PASS      32.72 seconds
TestRunner_bnep-tester        PASS      7.83 seconds
TestRunner_mgmt-tester        FAIL      119.87 seconds
TestRunner_rfcomm-tester      PASS      7.65 seconds
TestRunner_sco-tester         PASS      9.40 seconds
TestRunner_ioctl-tester       PASS      8.19 seconds
TestRunner_mesh-tester        FAIL      6.81 seconds
TestRunner_smp-tester         PASS      7.05 seconds
TestRunner_userchan-tester    PASS      5.03 seconds
IncrementalBuild              PENDING   0.57 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 484 (98.8%), Failed: 2, Not Run: 4

Failed Test Cases
LL Privacy - Add Device 3 (AL is full)               Failed       0.185 seconds
LL Privacy - Set Flags 1 (Add to RL)                 Failed       0.143 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 9 (90.0%), Failed: 1, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 2                               Failed       0.110 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

RE: Expand firmware-name property to load specific

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]

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

Dear submitter,

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

---Test result---

Test Summary:
CheckPatch                    PENDING   0.29 seconds
GitLint                       PENDING   0.21 seconds
SubjectPrefix                 FAIL      0.57 seconds
BuildKernel                   PASS      25.76 seconds
CheckAllWarning               PASS      27.54 seconds
CheckSparse                   PASS      31.11 seconds
BuildKernel32                 PASS      24.84 seconds
TestRunnerSetup               PASS      435.99 seconds
TestRunner_l2cap-tester       PASS      22.59 seconds
TestRunner_iso-tester         PASS      36.10 seconds
TestRunner_bnep-tester        PASS      4.81 seconds
TestRunner_mgmt-tester        FAIL      121.09 seconds
TestRunner_rfcomm-tester      PASS      7.53 seconds
TestRunner_sco-tester         PASS      9.35 seconds
TestRunner_ioctl-tester       PASS      8.12 seconds
TestRunner_mesh-tester        PASS      6.07 seconds
TestRunner_smp-tester         FAIL      9.27 seconds
TestRunner_userchan-tester    PASS      5.08 seconds
IncrementalBuild              PENDING   0.44 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 485 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
LL Privacy - Add Device 2 (2 Devices to AL)          Failed       0.183 seconds
##############################
Test: TestRunner_smp-tester - FAIL
Desc: Run smp-tester with test-runner
Output:
WARNING: CPU: 0 PID: 42 at net/bluetooth/hci_conn.c:567 hci_conn_timeout+0x14e/0x190
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth