diff mbox series

[RFC] bluetooth: qca: generate nvm fw name from boardid for WCN6855

Message ID 20241003-bt-nvm-firmware-v1-1-79028931214f@oldschoolsolutions.biz (mailing list archive)
State New, archived
Headers show
Series [RFC] bluetooth: qca: generate nvm fw name from boardid for WCN6855 | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch warning WARNING: Non-standard signature: Co-authored-by: #103: Co-authored-by: Steev Klimaszewski <steev@kali.org> total: 0 errors, 1 warnings, 58 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13821456.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
tedd_an/GitLint fail WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 33: B2 Line has trailing whitespace: "name format for the WCN6855 into account. The hpnv* firmware files " 34: B2 Line has trailing whitespace: "can be found on the Windows partition ofthe device. They usually " 35: B2 Line has trailing whitespace: "work better with the specific hardware. If the file is not found it "
tedd_an/SubjectPrefix fail "Bluetooth: " prefix is not specified in the subject
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS

Commit Message

Jens Glathe via B4 Relay Oct. 3, 2024, 7:21 p.m. UTC 
From: Steev Klimaszewski <steev@kali.org>

This is based on the 2066 work, which the 6855 is basically the same
thing.

The already existing function qca_generate_hsp_nvm_name() appears to do
the right steps to generate the hpnv file name. For WCN6855, the suffix
seems to be the board id with prefix b, though.

Add specific masking for boardid to generate the full name.

boardid == 0 -> use 0 as parameter
boardid <  0x100 -> add 0x0b00 to it, otherwise add 0xb000

This generates correct hpnv* file names for the files on the Windows
partition that appear to work better with the hardware than the default
.bin.
Tested on Lenovo Thinkpad X13s, Microsoft WDK2023, and HP Omnibook X14.

The specific firmware is found on the Windows partition, and is supposed
to work a little bit better than the default .bin.

Co-authored-by: Steev Klimaszewski <steev@kali.org>
Signed-off-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
Signed-off-by: Steev Klimaszewski <steev@kali.org>
---
This is based on the 2066 work, which the 6855 is basically the same
thing.

It generates the fw name to load from the board id, taking the file
name format for the WCN6855 into account. The hpnv* firmware files 
can be found on the Windows partition ofthe device. They usually 
work better with the specific hardware. If the file is not found it 
retries with the default name.

This has been tested on:

Lenovo Thinkpad X13s
Microsoft Windows Dev Kit 2023 (Blackrock)
HP Omnibook X14
---
 drivers/bluetooth/btqca.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)


---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20241003-bt-nvm-firmware-47e4d1b70a99

Best regards,

Comments

quic_zijuhu Nov. 14, 2024, 6:17 a.m. UTC | #1 
On 10/4/2024 3:21 AM, Jens Glathe wrote:
> From: Steev Klimaszewski <steev@kali.org>
> 
> This is based on the 2066 work, which the 6855 is basically the same
> thing.
> 
> The already existing function qca_generate_hsp_nvm_name() appears to do
> the right steps to generate the hpnv file name. For WCN6855, the suffix
> seems to be the board id with prefix b, though.
> 
> Add specific masking for boardid to generate the full name.
> 
> boardid == 0 -> use 0 as parameter
> boardid <  0x100 -> add 0x0b00 to it, otherwise add 0xb000
> 
> This generates correct hpnv* file names for the files on the Windows
> partition that appear to work better with the hardware than the default
> .bin.
> Tested on Lenovo Thinkpad X13s, Microsoft WDK2023, and HP Omnibook X14.
> 
> The specific firmware is found on the Windows partition, and is supposed
> to work a little bit better than the default .bin.
> 
> Co-authored-by: Steev Klimaszewski <steev@kali.org>
> Signed-off-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
> Signed-off-by: Steev Klimaszewski <steev@kali.org>
> ---
> This is based on the 2066 work, which the 6855 is basically the same
> thing.
> 
> It generates the fw name to load from the board id, taking the file
> name format for the WCN6855 into account. The hpnv* firmware files 
> can be found on the Windows partition ofthe device. They usually 
> work better with the specific hardware. If the file is not found it 
> retries with the default name.
> 
> This has been tested on:
> 
> Lenovo Thinkpad X13s
> Microsoft Windows Dev Kit 2023 (Blackrock)
> HP Omnibook X14
> ---
>  drivers/bluetooth/btqca.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index dfbbac92242a8..ffc75680b322b 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -564,6 +564,21 @@ static int qca_download_firmware(struct hci_dev *hdev,
>  					   config->fwname, ret);
>  				return ret;
>  			}
> +			/* For WCN6855, if Windows firmware file isn't in place
> +			 * then use the default .bin file.
> +			 */
> +		} else if (soc_type == QCA_WCN6855) {
> +			bt_dev_dbg(hdev, "QCA Failed to request file: %s (%d)",
> +				   config->fwname, ret);
> +			snprintf(config->fwname, sizeof(config->fwname),
> +				 "qca/hpnv%02x.bin", rom_ver);
> +			bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
> +			ret = request_firmware(&fw, config->fwname, &hdev->dev);
> +			if (ret) {
> +				bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
> +					   config->fwname, ret);
> +				return ret;
> +			}

NO

1)
qca_download_firmware() is  also called to download *PATCH* hpbtfw*, if
downloading PATCH failed, your logic will wrongly download *NVM* hpnv*
instead, right ?

2)
if board ID  is available, must use board id specific NVM, must not
fallback to the default NVM is not for QCA_WCN6855.

>  		} else {
>  			bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
>  				   config->fwname, ret);
> @@ -773,6 +788,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  			 "qca/apbtfw%02x.tlv", rom_ver);
>  		break;
>  	case QCA_QCA2066:
> +	case QCA_WCN6855:
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/hpbtfw%02x.tlv", rom_ver);

>  		break;
> @@ -788,10 +804,6 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  		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);
> @@ -810,7 +822,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	/* Give the controller some time to get ready to receive the NVM */
>  	msleep(10);
>  
> -	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
> +	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850 || soc_type == QCA_WCN6855)
>  		qca_read_fw_board_id(hdev, &boardid);

a switch since SOC types will become more and more
>  
>  	/* Download NVM configuration */
> @@ -848,8 +860,10 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  				 "qca/msnv%02x.bin", rom_ver);
>  			break;
>  		case QCA_WCN6855:
> -			snprintf(config.fwname, sizeof(config.fwname),
> -				 "qca/hpnv%02x.bin", rom_ver);
> +			qca_generate_hsp_nvm_name(config.fwname,
> +				sizeof(config.fwname), ver, rom_ver,
> +				boardid == 0 ? boardid : (boardid < 0x0100 ?
> +				(boardid & 0x00ff)|0x0b00 : (boardid & 0x0fff)|0xb000));
>  			break;

why not to refer to existing qca_get_nvm_name_generic().

i will post a patch and cc you.

>  		case QCA_WCN7850:
>  			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);
> 
> ---
> base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
> change-id: 20241003-bt-nvm-firmware-47e4d1b70a99
> 
> Best regards,
Jens Glathe Nov. 15, 2024, 6:49 a.m. UTC | #2 
On 14.11.24 07:17, quic_zijuhu wrote:
> On 10/4/2024 3:21 AM, Jens Glathe wrote:
> why not to refer to existing qca_get_nvm_name_generic().
>
> i will post a patch and cc you.
>
Hi Ziju,

tested the patch [1] and works nicely. It is the cleaner solution
(having dedicated fw name generator functions) than the one I proposed.
Future-proof.

with best regards

Jens

[1]
https://lore.kernel.org/all/20241113-x13s_wcn6855_fix-v1-1-15af0aa2549c@quicinc.com/
diff mbox series

Patch

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index dfbbac92242a8..ffc75680b322b 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -564,6 +564,21 @@  static int qca_download_firmware(struct hci_dev *hdev,
 					   config->fwname, ret);
 				return ret;
 			}
+			/* For WCN6855, if Windows firmware file isn't in place
+			 * then use the default .bin file.
+			 */
+		} else if (soc_type == QCA_WCN6855) {
+			bt_dev_dbg(hdev, "QCA Failed to request file: %s (%d)",
+				   config->fwname, ret);
+			snprintf(config->fwname, sizeof(config->fwname),
+				 "qca/hpnv%02x.bin", rom_ver);
+			bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
+			ret = request_firmware(&fw, config->fwname, &hdev->dev);
+			if (ret) {
+				bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
+					   config->fwname, ret);
+				return ret;
+			}
 		} else {
 			bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
 				   config->fwname, ret);
@@ -773,6 +788,7 @@  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 			 "qca/apbtfw%02x.tlv", rom_ver);
 		break;
 	case QCA_QCA2066:
+	case QCA_WCN6855:
 		snprintf(config.fwname, sizeof(config.fwname),
 			 "qca/hpbtfw%02x.tlv", rom_ver);
 		break;
@@ -788,10 +804,6 @@  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 		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);
@@ -810,7 +822,7 @@  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	/* Give the controller some time to get ready to receive the NVM */
 	msleep(10);
 
-	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
+	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850 || soc_type == QCA_WCN6855)
 		qca_read_fw_board_id(hdev, &boardid);
 
 	/* Download NVM configuration */
@@ -848,8 +860,10 @@  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 				 "qca/msnv%02x.bin", rom_ver);
 			break;
 		case QCA_WCN6855:
-			snprintf(config.fwname, sizeof(config.fwname),
-				 "qca/hpnv%02x.bin", rom_ver);
+			qca_generate_hsp_nvm_name(config.fwname,
+				sizeof(config.fwname), ver, rom_ver,
+				boardid == 0 ? boardid : (boardid < 0x0100 ?
+				(boardid & 0x00ff)|0x0b00 : (boardid & 0x0fff)|0xb000));
 			break;
 		case QCA_WCN7850:
 			qca_get_nvm_name_generic(&config, "hmt", rom_ver, boardid);