From: Juerg Haefliger <juerg.haefliger@canonical.com>
To: <max.chou@realtek.com>
Cc: <marcel@holtmann.org>, <johan.hedberg@gmail.com>,
<luiz.dentz@gmail.com>, <linux-bluetooth@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <alex_lu@realsil.com.cn>,
<hildawu@realtek.com>, <karenhsu@realtek.com>,
<kidman@realtek.com>, <vicamo.yang@canonical.com>,
<Riley.Kao@dell.com>
Subject: Re: [PATCH] Bluetooth: btrtl: Load FW v2 otherwise FW v1 for RTL8852C
Date: Fri, 4 Aug 2023 10:51:22 +0200 [thread overview]
Message-ID: <20230804105122.6f41882c@gollum> (raw)
In-Reply-To: <20230804055426.6806-1-max.chou@realtek.com>
[-- Attachment #1: Type: text/plain, Size: 12391 bytes --]
Thanks for this Max,
Comments inlined.
> From: Max Chou <max.chou@realtek.com>
>
> In the commit of linux-firmware project, rtl8852cu_fw.bin is updated as
> FW v2 format[1]. Consider the case that if driver did not be updated for
> FW v2 supported[2], it can not use FW v2.
> By Canonical's suggestion, older driver should be able to load FW v1,
> so rtl8852cu_fw.bin will be revert to the previous commit as FW v1 and
> add rtl8852cu_fw_v2.bin as FW v2. This item will be started on
> linux-firmware project.
>
> In this commit, the driver prefers to load FW v2 if available. Fallback to
> FW v1 otherwise.
>
> To do on linux-firmware project.
> rtl_bt/rtl8852cu_fw.bin: FW v1 (stay at ver. 0xD7B8_FABF)
> rtl_bt/rtl8852cu_fw_v2.bin: FW v2 (to be maintained)
>
> [1]'9a24ce5e29b1 ("Bluetooth: btrtl: Firmware format v2 support")'
> [2]'55e7448533e7 ("rtl_bt: Update RTL8852C BT USB firmware
> to 0x040D_7225")'
>
> Suggested-by: Juerg Haefliger <juerg.haefliger@canonical.com>
> Tested-by: Hilda Wu <hildawu@realtek.com>
> Signed-off-by: Max Chou <max.chou@realtek.com>
> ---
> drivers/bluetooth/btrtl.c | 68 +++++++++++++++++++++++++--------------
> 1 file changed, 44 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index ddae6524106d..8bfa86dd12f7 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -104,7 +104,7 @@ static const struct id_table ic_id_table[] = {
> { IC_INFO(RTL_ROM_LMP_8723A, 0xb, 0x6, HCI_USB),
> .config_needed = false,
> .has_rom_version = false,
> - .fw_name = "rtl_bt/rtl8723a_fw.bin",
> + .fw_name = "rtl_bt/rtl8723a_fw",
> .cfg_name = NULL,
> .hw_info = "rtl8723au" },
>
> @@ -112,7 +112,7 @@ static const struct id_table ic_id_table[] = {
> { IC_INFO(RTL_ROM_LMP_8723B, 0xb, 0x6, HCI_UART),
> .config_needed = true,
> .has_rom_version = true,
> - .fw_name = "rtl_bt/rtl8723bs_fw.bin",
> + .fw_name = "rtl_bt/rtl8723bs_fw",
> .cfg_name = "rtl_bt/rtl8723bs_config",
> .hw_info = "rtl8723bs" },
>
> @@ -120,7 +120,7 @@ static const struct id_table ic_id_table[] = {
> { IC_INFO(RTL_ROM_LMP_8723B, 0xb, 0x6, HCI_USB),
> .config_needed = false,
> .has_rom_version = true,
> - .fw_name = "rtl_bt/rtl8723b_fw.bin",
> + .fw_name = "rtl_bt/rtl8723b_fw",
> .cfg_name = "rtl_bt/rtl8723b_config",
> .hw_info = "rtl8723bu" },
>
> @@ -132,7 +132,7 @@ static const struct id_table ic_id_table[] = {
> .hci_bus = HCI_UART,
> .config_needed = true,
> .has_rom_version = true,
> - .fw_name = "rtl_bt/rtl8723cs_cg_fw.bin",
> + .fw_name = "rtl_bt/rtl8723cs_cg_fw",
> .cfg_name = "rtl_bt/rtl8723cs_cg_config",
> .hw_info = "rtl8723cs-cg" },
>
> @@ -144,7 +144,7 @@ static const struct id_table ic_id_table[] = {
> .hci_bus = HCI_UART,
> .config_needed = true,
> .has_rom_version = true,
> - .fw_name = "rtl_bt/rtl8723cs_vf_fw.bin",
> + .fw_name = "rtl_bt/rtl8723cs_vf_fw",
> .cfg_name = "rtl_bt/rtl8723cs_vf_config",
> .hw_info = "rtl8723cs-vf" },
>
> @@ -156,7 +156,7 @@ static const struct id_table ic_id_table[] = {
> .hci_bus = HCI_UART,
> .config_needed = true,
> .has_rom_version = true,
> - .fw_name = "rtl_bt/rtl8723cs_xx_fw.bin",
> + .fw_name = "rtl_bt/rtl8723cs_xx_fw",
> .cfg_name = "rtl_bt/rtl8723cs_xx_config",
> .hw_info = "rtl8723cs" },
>
> @@ -164,7 +164,7 @@ static const struct id_table ic_id_table[] = {
> { IC_INFO(RTL_ROM_LMP_8723B, 0xd, 0x8, HCI_USB),
> .config_needed = true,
> .has_rom_version = true,
> - .fw_name = "rtl_bt/rtl8723d_fw.bin",
> + .fw_name = "rtl_bt/rtl8723d_fw",
> .cfg_name = "rtl_bt/rtl8723d_config",
> .hw_info = "rtl8723du" },
>
> @@ -172,7 +172,7 @@ static const struct id_table ic_id_table[] = {
> { IC_INFO(RTL_ROM_LMP_8723B, 0xd, 0x8, HCI_UART),
> .config_needed = true,
> .has_rom_version = true,
> - .fw_name = "rtl_bt/rtl8723ds_fw.bin",
> + .fw_name = "rtl_bt/rtl8723ds_fw",
> .cfg_name = "rtl_bt/rtl8723ds_config",
> .hw_info = "rtl8723ds" },
>
> @@ -180,7 +180,7 @@ static const struct id_table ic_id_table[] = {
> { IC_INFO(RTL_ROM_LMP_8821A, 0xa, 0x6, HCI_USB),
> .config_needed = false,
> .has_rom_version = true,
> - .fw_name = "rtl_bt/rtl8821a_fw.bin",
> + .fw_name = "rtl_bt/rtl8821a_fw",
> .cfg_name = "rtl_bt/rtl8821a_config",
> .hw_info = "rtl8821au" },
>
> @@ -189,7 +189,7 @@ static const struct id_table ic_id_table[] = {
> .config_needed = false,
> .has_rom_version = true,
> .has_msft_ext = true,
> - .fw_name = "rtl_bt/rtl8821c_fw.bin",
> + .fw_name = "rtl_bt/rtl8821c_fw",
> .cfg_name = "rtl_bt/rtl8821c_config",
> .hw_info = "rtl8821cu" },
>
> @@ -198,7 +198,7 @@ static const struct id_table ic_id_table[] = {
> .config_needed = true,
> .has_rom_version = true,
> .has_msft_ext = true,
> - .fw_name = "rtl_bt/rtl8821cs_fw.bin",
> + .fw_name = "rtl_bt/rtl8821cs_fw",
> .cfg_name = "rtl_bt/rtl8821cs_config",
> .hw_info = "rtl8821cs" },
>
> @@ -206,7 +206,7 @@ static const struct id_table ic_id_table[] = {
> { IC_INFO(RTL_ROM_LMP_8761A, 0xa, 0x6, HCI_USB),
> .config_needed = false,
> .has_rom_version = true,
> - .fw_name = "rtl_bt/rtl8761a_fw.bin",
> + .fw_name = "rtl_bt/rtl8761a_fw",
> .cfg_name = "rtl_bt/rtl8761a_config",
> .hw_info = "rtl8761au" },
>
> @@ -215,7 +215,7 @@ static const struct id_table ic_id_table[] = {
> .config_needed = false,
> .has_rom_version = true,
> .has_msft_ext = true,
> - .fw_name = "rtl_bt/rtl8761b_fw.bin",
> + .fw_name = "rtl_bt/rtl8761b_fw",
> .cfg_name = "rtl_bt/rtl8761b_config",
> .hw_info = "rtl8761btv" },
>
> @@ -223,7 +223,7 @@ static const struct id_table ic_id_table[] = {
> { IC_INFO(RTL_ROM_LMP_8761A, 0xb, 0xa, HCI_USB),
> .config_needed = false,
> .has_rom_version = true,
> - .fw_name = "rtl_bt/rtl8761bu_fw.bin",
> + .fw_name = "rtl_bt/rtl8761bu_fw",
> .cfg_name = "rtl_bt/rtl8761bu_config",
> .hw_info = "rtl8761bu" },
>
> @@ -232,7 +232,7 @@ static const struct id_table ic_id_table[] = {
> .config_needed = true,
> .has_rom_version = true,
> .has_msft_ext = true,
> - .fw_name = "rtl_bt/rtl8822cs_fw.bin",
> + .fw_name = "rtl_bt/rtl8822cs_fw",
> .cfg_name = "rtl_bt/rtl8822cs_config",
> .hw_info = "rtl8822cs" },
>
> @@ -241,7 +241,7 @@ static const struct id_table ic_id_table[] = {
> .config_needed = true,
> .has_rom_version = true,
> .has_msft_ext = true,
> - .fw_name = "rtl_bt/rtl8822cs_fw.bin",
> + .fw_name = "rtl_bt/rtl8822cs_fw",
> .cfg_name = "rtl_bt/rtl8822cs_config",
> .hw_info = "rtl8822cs" },
>
> @@ -250,7 +250,7 @@ static const struct id_table ic_id_table[] = {
> .config_needed = false,
> .has_rom_version = true,
> .has_msft_ext = true,
> - .fw_name = "rtl_bt/rtl8822cu_fw.bin",
> + .fw_name = "rtl_bt/rtl8822cu_fw",
> .cfg_name = "rtl_bt/rtl8822cu_config",
> .hw_info = "rtl8822cu" },
>
> @@ -259,7 +259,7 @@ static const struct id_table ic_id_table[] = {
> .config_needed = true,
> .has_rom_version = true,
> .has_msft_ext = true,
> - .fw_name = "rtl_bt/rtl8822b_fw.bin",
> + .fw_name = "rtl_bt/rtl8822b_fw",
> .cfg_name = "rtl_bt/rtl8822b_config",
> .hw_info = "rtl8822bu" },
>
> @@ -268,7 +268,7 @@ static const struct id_table ic_id_table[] = {
> .config_needed = false,
> .has_rom_version = true,
> .has_msft_ext = true,
> - .fw_name = "rtl_bt/rtl8852au_fw.bin",
> + .fw_name = "rtl_bt/rtl8852au_fw",
> .cfg_name = "rtl_bt/rtl8852au_config",
> .hw_info = "rtl8852au" },
>
> @@ -277,7 +277,7 @@ static const struct id_table ic_id_table[] = {
> .config_needed = true,
> .has_rom_version = true,
> .has_msft_ext = true,
> - .fw_name = "rtl_bt/rtl8852bs_fw.bin",
> + .fw_name = "rtl_bt/rtl8852bs_fw",
> .cfg_name = "rtl_bt/rtl8852bs_config",
> .hw_info = "rtl8852bs" },
>
> @@ -286,7 +286,7 @@ static const struct id_table ic_id_table[] = {
> .config_needed = false,
> .has_rom_version = true,
> .has_msft_ext = true,
> - .fw_name = "rtl_bt/rtl8852bu_fw.bin",
> + .fw_name = "rtl_bt/rtl8852bu_fw",
> .cfg_name = "rtl_bt/rtl8852bu_config",
> .hw_info = "rtl8852bu" },
>
> @@ -295,7 +295,7 @@ static const struct id_table ic_id_table[] = {
> .config_needed = false,
> .has_rom_version = true,
> .has_msft_ext = true,
> - .fw_name = "rtl_bt/rtl8852cu_fw.bin",
> + .fw_name = "rtl_bt/rtl8852cu_fw",
> .cfg_name = "rtl_bt/rtl8852cu_config",
> .hw_info = "rtl8852cu" },
>
> @@ -304,7 +304,7 @@ static const struct id_table ic_id_table[] = {
> .config_needed = false,
> .has_rom_version = true,
> .has_msft_ext = false,
> - .fw_name = "rtl_bt/rtl8851bu_fw.bin",
> + .fw_name = "rtl_bt/rtl8851bu_fw",
> .cfg_name = "rtl_bt/rtl8851bu_config",
> .hw_info = "rtl8851bu" },
> };
> @@ -1045,10 +1045,12 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
> struct sk_buff *skb;
> struct hci_rp_read_local_version *resp;
> struct hci_command_hdr *cmd;
> + char fw_name[40];
> char cfg_name[40];
> u16 hci_rev, lmp_subver;
> u8 hci_ver, lmp_ver, chip_type = 0;
> int ret;
> + int fw_load_retry = 0;
> u8 reg_val[2];
>
> btrtl_dev = kzalloc(sizeof(*btrtl_dev), GFP_KERNEL);
> @@ -1154,9 +1156,26 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
> goto err_free;
> }
>
> - btrtl_dev->fw_len = rtl_load_file(hdev, btrtl_dev->ic_info->fw_name,
> +fw_name_load:
> + if (btrtl_dev->ic_info->fw_name) {
> + if (lmp_subver == RTL_ROM_LMP_8852A && hci_rev == 0x000c &&
> + fw_load_retry == 0) {
> + fw_load_retry = 1;
> + snprintf(fw_name, sizeof(fw_name), "%s_v2.bin",
> + btrtl_dev->ic_info->fw_name);
> + } else {
> + fw_load_retry = 0;
> + snprintf(fw_name, sizeof(fw_name), "%s.bin",
> + btrtl_dev->ic_info->fw_name);
> + }
> + btrtl_dev->fw_len = rtl_load_file(hdev, fw_name,
> &btrtl_dev->fw_data);
> + }
> +
> if (btrtl_dev->fw_len < 0) {
> + if (fw_load_retry == 1)
> + goto fw_name_load;
> +
> rtl_dev_err(hdev, "firmware file %s not found",
> btrtl_dev->ic_info->fw_name);
> ret = btrtl_dev->fw_len;
This makes my head hurt ;-) Can we make this more readable, something like
(untested):
@@ -1154,8 +1155,26 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
goto err_free;
}
- btrtl_dev->fw_len = rtl_load_file(hdev, btrtl_dev->ic_info->fw_name,
- &btrtl_dev->fw_data);
+ if (!btrtl_dev->ic_info->fw_name) {
+ ret = -ENOMEM;
+ goto err_free;
+ }
+
+ btrtl_dev->fw_len = -EIO;
+ if (lmp_subver == RTL_ROM_LMP_8852A && hci_rev == 0x000c) {
+ snprintf(fw_name, sizeof(fw_name), "%s_v2.bin",
+ btrtl_dev->ic_info->fw_name);
+ btrtl_dev->fw_len = rtl_load_file(hdev, fw_name,
+ &btrtl_dev->fw_data);
+ }
+
+ if (btrtl_dev->fw_len < 0) {
+ snprintf(fw_name, sizeof(fw_name), "%s.bin",
+ btrtl_dev->ic_info->fw_name);
+ btrtl_dev->fw_len = rtl_load_file(hdev, fw_name,
+ &btrtl_dev->fw_data);
+ }
+
if (btrtl_dev->fw_len < 0) {
rtl_dev_err(hdev, "firmware file %s not found",
btrtl_dev->ic_info->fw_name);
...Juerg
> @@ -1491,4 +1510,5 @@ MODULE_FIRMWARE("rtl_bt/rtl8852bs_config.bin");
> MODULE_FIRMWARE("rtl_bt/rtl8852bu_fw.bin");
> MODULE_FIRMWARE("rtl_bt/rtl8852bu_config.bin");
> MODULE_FIRMWARE("rtl_bt/rtl8852cu_fw.bin");
> +MODULE_FIRMWARE("rtl_bt/rtl8852cu_fw_v2.bin");
> MODULE_FIRMWARE("rtl_bt/rtl8852cu_config.bin");
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2023-08-04 9:07 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-04 5:54 [PATCH] Bluetooth: btrtl: Load FW v2 otherwise FW v1 for RTL8852C max.chou
2023-08-04 6:42 ` bluez.test.bot
2023-08-04 8:51 ` Juerg Haefliger [this message]
2023-08-04 8:54 ` [PATCH] " Linux regression tracking (Thorsten Leemhuis)
2023-08-04 9:14 ` You-Sheng Yang
2023-08-04 9:30 ` Max Chou
[not found] ` <202308040950.3749oOuG5031679@rtits1.realtek.com.tw>
2023-08-04 10:15 ` Max Chou
2023-08-04 10:31 ` Linux regression tracking (Thorsten Leemhuis)
2023-08-04 10:28 ` Max Chou
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230804105122.6f41882c@gollum \
--to=juerg.haefliger@canonical.com \
--cc=Riley.Kao@dell.com \
--cc=alex_lu@realsil.com.cn \
--cc=hildawu@realtek.com \
--cc=johan.hedberg@gmail.com \
--cc=karenhsu@realtek.com \
--cc=kidman@realtek.com \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
--cc=marcel@holtmann.org \
--cc=max.chou@realtek.com \
--cc=vicamo.yang@canonical.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox