RE: [PATCH] Bluetooth: btrtl: Enable WBS for the specific Realtek devices
Hi! Abhishek,
My bad! I should check the code base is the latest and follow it.
I'll submit the v2 patch. Thanks for your review.
BRs,
Max
-Original Message-
From: Abhishek Pandit-Subedi
Sent: Tuesday, January 19, 2021 12:41 PM
To: Max Chou
Cc: Marcel Holtmann ; Johan Hedberg
; Luiz Augusto von Dentz ; Bluez
mailing list ; LKML
; alex...@realsil.com.cn; Hilda Wu
; KidmanLee ; Shyh-In Hwang
Subject: Re: [PATCH] Bluetooth: btrtl: Enable WBS for the specific Realtek
devices
Hi Max,
On Mon, Jan 18, 2021 at 3:28 AM wrote:
>
> From: Max Chou
>
> By this change, it will enable WBS supported on the specific Realtek
> BT devices, such as RTL8822C and RTL8852A.
> In the future, it's able to maintain what the Realtek devices support
> WBS here.
>
> Tested-by: Hilda Wu
> Signed-off-by: Max Chou
> ---
> drivers/bluetooth/btrtl.c | 26 +++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index 24f03a1f8d57..835819c47ae6 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -38,6 +38,19 @@
> .hci_ver = (hciv), \
> .hci_bus = (bus)
>
> +enum btrtl_chip_id {
> + CHIP_ID_8723A, /* index 0 for RTL8723A*/
> + CHIP_ID_8723B, /* index 1 for RTL8723B*/
> + CHIP_ID_8821A, /* index 2 for RTL8821A*/
> + CHIP_ID_8761A, /* index 3 for RTL8761A*/
> + CHIP_ID_8822B = 8, /* index 8 for RTL8822B */
> + CHIP_ID_8723D, /* index 9 for RTL8723D */
> + CHIP_ID_8821C, /* index 10 for RTL8821C */
> + CHIP_ID_8822C = 13, /* index 13 for RTL8822C */
> + CHIP_ID_8761B, /* index 14 for RTL8761B */
> + CHIP_ID_8852A = 18, /* index 18 for RTL8852A */
> +};
> +
> struct id_table {
> __u16 match_flags;
> __u16 lmp_subver;
> @@ -58,6 +71,7 @@ struct btrtl_device_info {
> u8 *cfg_data;
> int cfg_len;
> bool drop_fw;
> + int project_id;
> };
>
> static const struct id_table ic_id_table[] = { @@ -307,8 +321,10 @@
> static int rtlbt_parse_firmware(struct hci_dev *hdev,
>
> /* Find project_id in table */
> for (i = 0; i < ARRAY_SIZE(project_id_to_lmp_subver); i++) {
> - if (project_id == project_id_to_lmp_subver[i].id)
> + if (project_id == project_id_to_lmp_subver[i].id) {
> + btrtl_dev->project_id = project_id;
> break;
> + }
> }
>
> if (i >= ARRAY_SIZE(project_id_to_lmp_subver)) { @@ -725,12
> +741,16 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
> /* Enable central-peripheral role (able to create new connections with
> * an existing connection in slave role).
> */
> - switch (btrtl_dev->ic_info->lmp_subver) {
> - case RTL_ROM_LMP_8822B:
> + /* Enable WBS supported for the specific Realtek devices. */
> + switch (btrtl_dev->project_id) {
> + case CHIP_ID_8822C:
> + case CHIP_ID_8852A:
> set_bit(HCI_QUIRK_VALID_LE_STATES, &hdev->quirks);
> + set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED,
> + &hdev->quirks);
> break;
> default:
> rtl_dev_dbg(hdev, "Central-peripheral role not
> enabled.");
> + rtl_dev_dbg(hdev, "WBS supported not enabled.");
> break;
> }
There is a null-check on btrtl_dev->ic_info right above this switch statement
(https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=b649813eadbc062d8682f7a20aa025275707dd1f).
Is this still necessary with this change? (Sorry, I missed this during our
previous review).
>
> --
> 2.17.1
>
Abhishek
--Please consider the environment before printing this e-mail.
RE: [PATCH] Bluetooth: btusb: Add a Kconfig option to disable USB wakeup by default
// add Abhishek to CC list
BRs,
Max
-Original Message-
From: Max Chou
Sent: Wednesday, December 30, 2020 2:55 PM
To: mar...@holtmann.org; johan.hedb...@gmail.com; luiz.de...@gmail.com;
matthias@gmail.com
Cc: linux-blueto...@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-arm-ker...@lists.infradead.org; linux-media...@lists.infradead.org;
alex...@realsil.com.cn; Hilda Wu ; KidmanLee
; Max Chou
Subject: [PATCH] Bluetooth: btusb: Add a Kconfig option to disable USB wakeup
by default
From: Max Chou
For the original commit of 9e45524a011107a73bc2cdde8370c61e82e93a4d,
wakeup is always disabled for Realtek Bluetooth devices.
However, there's the capability for Realtek Bluetooth devices to apply USB
wakeup. Otherwise, there's the better power consumption without USB wakeup
during suspending.
In this commit, divide the original commit into two parts.
1. Redefine the feature that Realtek devices should be enabled wakeup on
auto-suspend as BTUSB_WAKEUP_AUTOSUSPEND.
2. Add a Kconfig option to switch disable_wakeup for Bluetooth USB devices by
default as CONFIG_BT_HCIBTUSB_DISABLEWAKEUP.
Signed-off-by: Max Chou
---
drivers/bluetooth/Kconfig | 11 +++ drivers/bluetooth/btusb.c | 41
++-
2 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig index
4e73a531b377..7af10897a248 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -41,6 +41,17 @@ config BT_HCIBTUSB_AUTOSUSPEND
This can be overridden by passing btusb.enable_autosuspend=[y|n]
on the kernel commandline.
+config BT_HCIBTUSB_DISABLEWAKEUP
+ bool "Disable USB wakeup for Bluetooth USB devices by default"
+ depends on BT_HCIBTUSB
+ default n
+ help
+ Say Y here to disable USB wakeup for Bluetooth USB devices by
+ default.
+
+ This can be overridden by passing btusb.disable_wakeup=[y|n]
+ on the kernel commandline.
+
config BT_HCIBTUSB_BCM
bool "Broadcom protocol support"
depends on BT_HCIBTUSB
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index
b630a1d54c02..5f55111849b5 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -30,6 +30,7 @@
static bool disable_scofix;
static bool force_scofix;
static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
+static bool disable_wakeup =
+IS_ENABLED(CONFIG_BT_HCIBTUSB_DISABLEWAKEUP);
static bool reset = true;
@@ -505,7 +506,7 @@ static const struct dmi_system_id
btusb_needs_reset_resume_table[] = {
#define BTUSB_OOB_WAKE_ENABLED 11
#define BTUSB_HW_RESET_ACTIVE 12
#define BTUSB_TX_WAIT_VND_EVT 13
-#define BTUSB_WAKEUP_DISABLE 14
+#define BTUSB_WAKEUP_AUTOSUSPEND 14
struct btusb_data {
struct hci_dev *hdev;
@@ -1330,7 +1331,7 @@ static int btusb_open(struct hci_dev *hdev)
* For Realtek chips, global suspend without
* SET_FEATURE (DEVICE_REMOTE_WAKEUP) can save more power in device.
*/
- if (test_bit(BTUSB_WAKEUP_DISABLE, &data->flags))
+ if (disable_wakeup)
device_wakeup_disable(&data->udev->dev);
if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags)) @@ -1399,7
+1400,7 @@ static int btusb_close(struct hci_dev *hdev)
data->intf->needs_remote_wakeup = 0;
/* Enable remote wake up for auto-suspend */
- if (test_bit(BTUSB_WAKEUP_DISABLE, &data->flags))
+ if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
data->intf->needs_remote_wakeup = 1;
usb_autopm_put_interface(data->intf);
@@ -4257,7 +4258,7 @@ static bool btusb_prevent_wake(struct hci_dev *hdev) {
struct btusb_data *data = hci_get_drvdata(hdev);
- if (test_bit(BTUSB_WAKEUP_DISABLE, &data->flags))
+ if (disable_wakeup)
return true;
return !device_may_wakeup(&data->udev->dev);
@@ -4557,11 +4558,8 @@ static int btusb_probe(struct usb_interface *intf,
hdev->shutdown = btrtl_shutdown_realtek;
hdev->cmd_timeout = btusb_rtl_cmd_timeout;
- /* Realtek devices lose their updated firmware over global
-* suspend that means host doesn't send SET_FEATURE
-* (DEVICE_REMOTE_WAKEUP)
-*/
- set_bit(BTUSB_WAKEUP_DISABLE, &data->flags);
+ /* Realtek devices need to set USB remote wakeup on
auto-suspend */
+ set_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags);
}
if (!reset)
@@ -4731,17 +4729,29 @@ static int btusb_suspend(struct usb_interface *intf,
pm_message_t message)
enable_irq(data->oob_wake_irq);
}
- /* For global suspend, Realtek devices lose the
RE: [PATCH v2] Bluetooth: btrtl: Ask 8821C to drop old firmware
Dear guys,
I agree this patch.
BRs,
Max
-Original Message-
From: Kai-Heng Feng
Sent: Monday, November 2, 2020 9:33 PM
To: Max Chou ; alex_lu
Cc: Marcel Holtmann ; Johan Hedberg
; open list:BLUETOOTH DRIVERS
; open list
Subject: Re: [PATCH v2] Bluetooth: btrtl: Ask 8821C to drop old firmware
> On Oct 26, 2020, at 16:28, Kai-Heng Feng wrote:
>
> Some platforms keep USB power even when they are powered off and in
> S5, this makes Realtek 8821C keep its firmware even after a cold boot,
> and make 8821C never load new firmware.
>
> So use vendor specific HCI command to ask 8821C drop its firmware
> after system shutdown.
>
> Newer firmware doesn't have this issue so we only use this trick for
> old 8821C firmware version.
>
> Suggested-by: Max Chou
> Signed-off-by: Kai-Heng Feng
Max and Alex,
Can you please ack or review the patch?
Kai-Heng
> ---
> v2:
> - Fix incorrect parAnthesis on le16_to_cpu.
> - Ensure firmware gets re-uploaded in initialization.
>
> drivers/bluetooth/btrtl.c | 46 +++
> 1 file changed, 46 insertions(+)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index 3a9afc905f24..37e24bbb2eb4 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -55,6 +55,7 @@ struct btrtl_device_info {
> int fw_len;
> u8 *cfg_data;
> int cfg_len;
> + bool drop_fw;
> };
>
> static const struct id_table ic_id_table[] = { @@ -563,6 +564,8 @@
> struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
> u16 hci_rev, lmp_subver;
> u8 hci_ver;
> int ret;
> + u16 opcode;
> + u8 cmd[2];
>
> btrtl_dev = kzalloc(sizeof(*btrtl_dev), GFP_KERNEL);
> if (!btrtl_dev) {
> @@ -584,6 +587,49 @@ struct btrtl_device_info *btrtl_initialize(struct
> hci_dev *hdev,
> hci_ver = resp->hci_ver;
> hci_rev = le16_to_cpu(resp->hci_rev);
> lmp_subver = le16_to_cpu(resp->lmp_subver);
> +
> + if (resp->hci_ver == 0x8 && le16_to_cpu(resp->hci_rev) == 0x826c &&
> + resp->lmp_ver == 0x8 && le16_to_cpu(resp->lmp_subver) == 0xa99e)
> + btrtl_dev->drop_fw = true;
> +
> + if (btrtl_dev->drop_fw) {
> + opcode = hci_opcode_pack(0x3f, 0x66);
> + cmd[0] = opcode & 0xff;
> + cmd[1] = opcode >> 8;
> +
> + skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
> + if (IS_ERR(skb))
> + goto out_free;
> +
> + skb_put_data(skb, cmd, sizeof(cmd));
> + hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> +
> + hdev->send(hdev, skb);
> +
> + /* Ensure the above vendor command is sent to controller and
> + * process has done.
> + */
> + msleep(200);
> +
> + /* Read the local version again. Expect to have the vanilla
> + * version as cold boot.
> + */
> + skb = btrtl_read_local_version(hdev);
> + if (IS_ERR(skb)) {
> + ret = PTR_ERR(skb);
> + goto err_free;
> + }
> +
> + resp = (struct hci_rp_read_local_version *)skb->data;
> + rtl_dev_info(hdev, "examining hci_ver=%02x hci_rev=%04x
> lmp_ver=%02x lmp_subver=%04x",
> + resp->hci_ver, resp->hci_rev,
> + resp->lmp_ver, resp->lmp_subver);
> +
> + hci_ver = resp->hci_ver;
> + hci_rev = le16_to_cpu(resp->hci_rev);
> + lmp_subver = le16_to_cpu(resp->lmp_subver);
> + }
> +out_free:
> kfree_skb(skb);
>
> btrtl_dev->ic_info = btrtl_match_ic(lmp_subver, hci_rev, hci_ver,
> --
> 2.17.1
>
--Please consider the environment before printing this e-mail.
回收: [PATCH] Bluetooth: btrtl: Fix an issue that failing to download the FW which size is over 32K bytes
Max Chou 希望回收這封郵件 [[PATCH] Bluetooth: btrtl: Fix an issue that failing to download the FW which size is over 32K bytes]。
RE: [PATCH v3] Bluetooth: btusb: Fix suspend issue for Realtek devices
Dear Kernel Maintainer Marcel, Sorry for the inconvenience. For the original target, this patch is edited for low power consumption hence controller should not receive DEVICE_REMOTE_WAKE_UP that it's able to save power in suspend mode because BT wake-up function is disabled. In upstream driver, there should be higher priority for function rather than performance. In other words, this patch can meet the low power consumption in suspend mode but will lose BT wake-up function. It is not a good idea for that. Please help to revert this modification. Thank you. BRs, Max -Original Message- From: Marcel Holtmann Sent: Wednesday, August 14, 2019 9:54 PM To: alex_lu Cc: Johan Hedberg ; linux-blueto...@vger.kernel.org; linux-kernel@vger.kernel.org; Max Chou Subject: Re: [PATCH v3] Bluetooth: btusb: Fix suspend issue for Realtek devices Hi Alex, > From the perspective of controller, global suspend means there is no > SET_FEATURE (DEVICE_REMOTE_WAKEUP) and controller would drop the > firmware. It would consume less power. So we should not send this kind > of SET_FEATURE when host goes to suspend state. > Otherwise, when making device enter selective suspend, host should > send SET_FEATURE to make sure the firmware remains. > > Signed-off-by: Alex Lu > --- > Changes in v3: > - Change to fit for bluetooth-next > Changes in v2: > - Change flag to be more descriptive > - Delete pointless #ifdef CONFIG_BT_HCIBTUSB_RTL and #endif > > drivers/bluetooth/btusb.c | 34 ++ > 1 file changed, 30 insertions(+), 4 deletions(-) patch has been applied to bluetooth-next tree. Regards Marcel --Please consider the environment before printing this e-mail.
