[v3] Bluetooth: qca: Fix poor RF performance for WCN6855

Commit Message

Zijun Hu Jan. 13, 2025, 2:43 p.m. UTC

From: Zijun Hu <[e-mail]>

For WCN6855, board ID specific NVM needs to be downloaded once board ID
is available, but the default NVM is always downloaded currently.

The wrong NVM causes poor RF performance, and effects user experience
for several types of laptop with WCN6855 on the market.

Fix by downloading board ID specific NVM if board ID is available.

Fixes: 095327fede00 ("Bluetooth: hci_qca: Add support for QTI Bluetooth chip wcn6855")
Cc: [e-mail] # 6.4
Signed-off-by: Zijun Hu <[e-mail]>
---
Changes in v3:
- Rework over tip of bluetooth-next tree.
- Remove both Reviewed-by and Tested-by tags.
- Link to v2: https://lore.kernel.org/r/[e-mail]

Changes in v2:
- Correct subject and commit message
- Temporarily add nvm fallback logic to speed up backport.
- Add fix/stable tags as suggested by Luiz and Johan
- Link to v1: https://lore.kernel.org/r/[e-mail]
---
 drivers/bluetooth/btqca.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


---
base-commit: a723753d039fd9a6c5998340ac65f4d9e2966ba8
change-id: 20250113-wcn6855_fix-036ca2fa5559

Best regards,

Comments

Johan Hovold Jan. 13, 2025, 3:04 p.m. UTC | #1

On Mon, Jan 13, 2025 at 10:43:23PM +0800, Zijun Hu wrote:
> From: Zijun Hu <[e-mail]>
> 
> For WCN6855, board ID specific NVM needs to be downloaded once board ID
> is available, but the default NVM is always downloaded currently.
> 
> The wrong NVM causes poor RF performance, and effects user experience
> for several types of laptop with WCN6855 on the market.
> 
> Fix by downloading board ID specific NVM if board ID is available.
> 
> Fixes: 095327fede00 ("Bluetooth: hci_qca: Add support for QTI Bluetooth chip wcn6855")
> Cc: [e-mail] # 6.4
> Signed-off-by: Zijun Hu <[e-mail]>
> ---
> Changes in v3:
> - Rework over tip of bluetooth-next tree.
> - Remove both Reviewed-by and Tested-by tags.
> - Link to v2: https://lore.kernel.org/r/[e-mail]

Thanks for the quick update.

I'm fine with dropping the fallback logic, but you should have mentioned
that here.

This still works fine on X13s and sc8280xp crd (hpnv21g.b8c and
hpnv20.b8c):

Tested-by: Johan Hovold <[e-mail]>

> Changes in v2:
> - Correct subject and commit message
> - Temporarily add nvm fallback logic to speed up backport.
> - Add fix/stable tags as suggested by Luiz and Johan
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  drivers/bluetooth/btqca.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index a6b53d1f23dbd4666b93e10635f5f154f38d80a5..cdf09d9a9ad27c080f27c5fe8d61d76085e1fd2c 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -909,8 +909,9 @@ 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_read_fw_board_id(hdev, &boardid);

For consistency, this should probably have been handled by amending the
conditional above the switch:

	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
		qca_read_fw_board_id(hdev, &boardid);

but long term that should probably be moved into
qca_get_nvm_name_by_board() to avoid sprinkling conditionals all over
the driver.

I'm fine with this as a stop gap unless you want to move the call to the
QCA2066/WCN7850 conditional:

Reviewed-by: Johan Hovold <[e-mail]>

> +			qca_get_nvm_name_by_board(config.fwname, sizeof(config.fwname),
> +						  "hpnv", soc_type, ver, rom_ver, boardid);
>  			break;
>  		case QCA_WCN7850:
>  			qca_get_nvm_name_by_board(config.fwname, sizeof(config.fwname),

Johan

Zijun Hu Jan. 13, 2025, 3:11 p.m. UTC | #2

On 2025/1/13 23:04, Johan Hovold wrote:
>> ---
>> Changes in v3:
>> - Rework over tip of bluetooth-next tree.
>> - Remove both Reviewed-by and Tested-by tags.
>> - Link to v2: https://lore.kernel.org/r/[e-mail]
> Thanks for the quick update.
> 
> I'm fine with dropping the fallback logic, but you should have mentioned
> that here.
> 

the fallback logic is still reserved.

look at drivers/bluetooth/btqca.c:qca_download_firmware() changes
introduce by:
Commit: ad3f4635a796 ("Bluetooth: qca: Update firmware-name to support
board specific nvm")

> This still works fine on X13s and sc8280xp crd (hpnv21g.b8c and
> hpnv20.b8c):
> 
> Tested-by: Johan Hovold <[e-mail]>

Zijun Hu Jan. 13, 2025, 3:15 p.m. UTC | #3

On 2025/1/13 23:04, Johan Hovold wrote:
>>  		case QCA_WCN6855:
>> -			snprintf(config.fwname, sizeof(config.fwname),
>> -				 "qca/hpnv%02x.bin", rom_ver);
>> +			qca_read_fw_board_id(hdev, &boardid);
> For consistency, this should probably have been handled by amending the
> conditional above the switch:
> 
> 	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
> 		qca_read_fw_board_id(hdev, &boardid);
> 
sorry for that not notice this comments.

qca_read_fw_board_id() may be invoked twice if adding reading board ID here

see below branch:

	config.type = TLV_TYPE_NVM;
	if (firmware_name) {
		/* The firmware name has an extension, use it directly */
		if (qca_filename_has_extension(firmware_name)) {
			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, soc_type, ver, 0, boardid);
		}

> but long term that should probably be moved into
> qca_get_nvm_name_by_board() to avoid sprinkling conditionals all over
> the driver.
> 
> I'm fine with this as a stop gap unless you want to move the call to the
> QCA2066/WCN7850 conditional:
> 
> Reviewed-by: Johan Hovold <[e-mail]>

Johan Hovold Jan. 13, 2025, 3:17 p.m. UTC | #4

On Mon, Jan 13, 2025 at 11:11:00PM +0800, Zijun Hu wrote:
> On 2025/1/13 23:04, Johan Hovold wrote:
> >> ---
> >> Changes in v3:
> >> - Rework over tip of bluetooth-next tree.
> >> - Remove both Reviewed-by and Tested-by tags.
> >> - Link to v2: https://lore.kernel.org/r/[e-mail]
> > Thanks for the quick update.
> > 
> > I'm fine with dropping the fallback logic, but you should have mentioned
> > that here.
> 
> the fallback logic is still reserved.
> 
> look at drivers/bluetooth/btqca.c:qca_download_firmware() changes
> introduce by:
> Commit: ad3f4635a796 ("Bluetooth: qca: Update firmware-name to support
> board specific nvm")

Ah, sorry, thanks for pointing that out.

> > This still works fine on X13s and sc8280xp crd (hpnv21g.b8c and
> > hpnv20.b8c):
> > 
> > Tested-by: Johan Hovold <[e-mail]>

Johan

Johan Hovold Jan. 13, 2025, 3:22 p.m. UTC | #5

On Mon, Jan 13, 2025 at 11:15:13PM +0800, Zijun Hu wrote:
> On 2025/1/13 23:04, Johan Hovold wrote:
> >>  		case QCA_WCN6855:
> >> -			snprintf(config.fwname, sizeof(config.fwname),
> >> -				 "qca/hpnv%02x.bin", rom_ver);
> >> +			qca_read_fw_board_id(hdev, &boardid);
> > For consistency, this should probably have been handled by amending the
> > conditional above the switch:
> > 
> > 	if (soc_type == QCA_QCA2066 || soc_type == QCA_WCN7850)
> > 		qca_read_fw_board_id(hdev, &boardid);

> qca_read_fw_board_id() may be invoked twice if adding reading board ID here
> 
> see below branch:
> 
> 	config.type = TLV_TYPE_NVM;
> 	if (firmware_name) {
> 		/* The firmware name has an extension, use it directly */
> 		if (qca_filename_has_extension(firmware_name)) {
> 			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, soc_type, ver, 0, boardid);
> 		}

Fair enough (but I guess that would be the case also for
QCA2066/WCN7850 currently).

But either way, this is not something that should block this fix.

> > but long term that should probably be moved into
> > qca_get_nvm_name_by_board() to avoid sprinkling conditionals all over
> > the driver.
> > 
> > I'm fine with this as a stop gap unless you want to move the call to the
> > QCA2066/WCN7850 conditional:
> > 
> > Reviewed-by: Johan Hovold <[e-mail]>

Johan

[e-mail] Jan. 13, 2025, 3:33 p.m. UTC | #6

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=924924

---Test result---

Test Summary:
CheckPatch                    PENDING   0.30 seconds
GitLint                       PENDING   0.23 seconds
SubjectPrefix                 PASS      0.89 seconds
BuildKernel                   PASS      23.92 seconds
CheckAllWarning               PASS      26.06 seconds
CheckSparse                   PASS      29.51 seconds
BuildKernel32                 PASS      23.70 seconds
TestRunnerSetup               PASS      428.03 seconds
TestRunner_l2cap-tester       PASS      21.28 seconds
TestRunner_iso-tester         PASS      36.48 seconds
TestRunner_bnep-tester        PASS      4.98 seconds
TestRunner_mgmt-tester        FAIL      124.64 seconds
TestRunner_rfcomm-tester      PASS      7.80 seconds
TestRunner_sco-tester         PASS      9.61 seconds
TestRunner_ioctl-tester       PASS      8.44 seconds
TestRunner_mesh-tester        PASS      6.25 seconds
TestRunner_smp-tester         PASS      7.18 seconds
TestRunner_userchan-tester    PASS      7.27 seconds
IncrementalBuild              PENDING   0.69 seconds

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

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

##############################
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 - Set Flags 3 (2 Devices to RL)           Failed       0.184 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Steev Klimaszewski Jan. 14, 2025, 4:10 a.m. UTC | #7

Hi Zijun,

On Mon, Jan 13, 2025 at 8:43 AM Zijun Hu <[e-mail]> wrote:
>
> From: Zijun Hu <[e-mail]>
>
> For WCN6855, board ID specific NVM needs to be downloaded once board ID
> is available, but the default NVM is always downloaded currently.
>
> The wrong NVM causes poor RF performance, and effects user experience
> for several types of laptop with WCN6855 on the market.
>
> Fix by downloading board ID specific NVM if board ID is available.
>
> Fixes: 095327fede00 ("Bluetooth: hci_qca: Add support for QTI Bluetooth chip wcn6855")
> Cc: [e-mail] # 6.4
> Signed-off-by: Zijun Hu <[e-mail]>
> ---
> Changes in v3:
> - Rework over tip of bluetooth-next tree.
> - Remove both Reviewed-by and Tested-by tags.
> - Link to v2: https://lore.kernel.org/r/[e-mail]
>
> Changes in v2:
> - Correct subject and commit message
> - Temporarily add nvm fallback logic to speed up backport.
> - Add fix/stable tags as suggested by Luiz and Johan
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  drivers/bluetooth/btqca.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index a6b53d1f23dbd4666b93e10635f5f154f38d80a5..cdf09d9a9ad27c080f27c5fe8d61d76085e1fd2c 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -909,8 +909,9 @@ 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_read_fw_board_id(hdev, &boardid);
> +                       qca_get_nvm_name_by_board(config.fwname, sizeof(config.fwname),
> +                                                 "hpnv", soc_type, ver, rom_ver, boardid);
>                         break;
>                 case QCA_WCN7850:
>                         qca_get_nvm_name_by_board(config.fwname, sizeof(config.fwname),
>
> ---
> base-commit: a723753d039fd9a6c5998340ac65f4d9e2966ba8
> change-id: 20250113-wcn6855_fix-036ca2fa5559
>
> Best regards,
> --
> Zijun Hu <[e-mail]>
>

Tested on my X13s and working great here, thank you for this!
Tested-by: Steev Klimaszewski <[e-mail]> #Thinkpad X13s

[e-mail] Jan. 14, 2025, 5:20 p.m. UTC | #8

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Mon, 13 Jan 2025 22:43:23 +0800 you wrote:
> From: Zijun Hu <[e-mail]>
> 
> For WCN6855, board ID specific NVM needs to be downloaded once board ID
> is available, but the default NVM is always downloaded currently.
> 
> The wrong NVM causes poor RF performance, and effects user experience
> for several types of laptop with WCN6855 on the market.
> 
> [...]

Here is the summary with links:
  - [v3] Bluetooth: qca: Fix poor RF performance for WCN6855
    https://git.kernel.org/bluetooth/bluetooth-next/c/67f8711aae4b

You are awesome, thank you!

Patch

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index a6b53d1f23dbd4666b93e10635f5f154f38d80a5..cdf09d9a9ad27c080f27c5fe8d61d76085e1fd2c 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -909,8 +909,9 @@  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_read_fw_board_id(hdev, &boardid);
+			qca_get_nvm_name_by_board(config.fwname, sizeof(config.fwname),
+						  "hpnv", soc_type, ver, rom_ver, boardid);
 			break;
 		case QCA_WCN7850:
 			qca_get_nvm_name_by_board(config.fwname, sizeof(config.fwname),