Hi Alex,
> On Sep 25, 2020, at 16:23, 陆朱伟 <alex_lu@xxxxxxxxxxxxxx> wrote:
>
> Hi Kai-Heng,
>
>> On September 25, 2020 at 15:56, Kai-Heng Feng wrote:
>>
>> Hi Alex,
>>
>>> On Sep 25, 2020, at 15:42, 陆朱伟 <alex_lu@xxxxxxxxxxxxxx> wrote:
>>>
>>> Hi Kai-Heng,
>>>
>>>> On 25 September 2020 at 15:14, Kai-Heng Feng wrote:
>>>>
>>>> Hi Alex,
>>
>> [snipped]
>>
>>>> Apparently for my case, RTL8821CE, firmware was kept without setting
>>>> remote wakeup.
>>>
>>> So you got the btusb disconnect and reprobe sequence after resume, and "
>> Bluetooth: hci0: command 0x1001 tx timeout " before firmware downloading ?
>>
>> USB power wasn't lost, but it got USB warm reset because btusb driver
>> explicitly flagged "reset_resume = 1".
>> Then the issue appeared as "Bluetooth: hci0: command 0x1001 tx timeout",
>> before downloading firmware.
>>
>>>
>>>> Is it okay to also set remote wakeup for global suspend to retain the
>>>> firmware?
>>>
>>> Yes, it's ok.
>>
>> Abhishek, does setting remote wakeup during global suspend works for you?
>
> It depends on your desire on power consumption during global suspend.
> The BT controller takes less power if firmware was lost during global suspend.
For my case, the firmware is retained after S3, despite of "reset_resume = 1":
[ 30.164036] ACPI: Waking up from system sleep state S3
[ 30.167913] ACPI: EC: interrupt unblocked
[ 31.284138] ACPI: EC: event unblocked
...
[ 31.467484] usb 1-14: reset full-speed USB device number 3 using xhci_hcd
...
[ 32.732934] Bluetooth: hci0: RTL: examining hci_ver=08 hci_rev=826c lmp_ver=08 lmp_subver=a99e
[ 32.732937] Bluetooth: hci0: RTL: unknown IC info, lmp subver a99e, hci rev 826c, hci ver 0008
[ 32.732937] Bluetooth: hci0: RTL: assuming no firmware upload needed
Kai-Heng
>
>>
>>>
>>>> If firmware was retained, does USB warm reset affect BT controller in
>>>> anyway?
>>>
>>> USB warm reset shouldn't affect BT controller.
>>> But hci device will not work after resume, because btrtl will find "unknown
>> IC info, lmp subvert ..." and return error when hci device setup is called.
>>> Tips: The lmp subver in controller changes after firmware downloading.
>> And driver will find " unknown IC info, lmp subver ..." when setup is called
>> with firmware retained.
>>
>> This should already be fixed by "Bluetooth: btrtl: Restore old logic to assume
>> firmware is already loaded".
>>
>> Kai-Heng
>>
>>>
>>>>
>>>> Kai-Heng
>>>>
>>>>>
>>>>>>
>>>>>> Kai-Heng
>>>>>>
>>>>>>>
>>>>>>> @Alex -- What is the common behavior for Realtek controllers?
>> Should
>>>>>>> we set BTUSB_WAKEUP_DISABLE only on RTL8822CE or should we
>> unset
>>>> it
>>>>>>> only on RTL8821CE?
>>>>>>>
>>>>>>>>>
>>>>>>>>> I would prefer this doesn't get accepted in its current state.
>>>>>>>>
>>>>>>>> Of course.
>>>>>>>> I think we need to find the root cause for your case before applying
>> this
>>>>>> one.
>>>>>>>>
>>>>>>>> Kai-Heng
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Abhishek
>>>>>>>>>
>>>>>>>>> On Wed, Sep 23, 2020 at 10:56 AM Kai-Heng Feng
>>>>>>>>> <kai.heng.feng@xxxxxxxxxxxxx> wrote:
>>>>>>>>>>
>>>>>>>>>> Realtek bluetooth controller may fail to work after system sleep:
>>>>>>>>>> [ 1272.707670] Bluetooth: hci0: command 0x1001 tx timeout
>>>>>>>>>> [ 1280.835712] Bluetooth: hci0: RTL:
>> HCI_OP_READ_LOCAL_VERSION
>>>>>> failed (-110)
>>>>>>>>>>
>>>>>>>>>> If platform firmware doesn't cut power off during suspend, the
>>>>>> firmware
>>>>>>>>>> is considered retained in controller but the driver is still asking USB
>>>>>>>>>> core to perform a reset-resume. This can make bluetooth
>> controller
>>>>>>>>>> unusable.
>>>>>>>>>>
>>>>>>>>>> So avoid unnecessary reset to resolve the issue.
>>>>>>>>>>
>>>>>>>>>> For devices that really lose power during suspend, USB core will
>>>> detect
>>>>>>>>>> and handle reset-resume correctly.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
>>>>>>>>>> ---
>>>>>>>>>> drivers/bluetooth/btusb.c | 8 +++-----
>>>>>>>>>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>>>>>>> index 8d2608ddfd08..de86ef4388f9 100644
>>>>>>>>>> --- a/drivers/bluetooth/btusb.c
>>>>>>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>>>>>>> @@ -4255,17 +4255,15 @@ 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 loaded fw
>>>>>>>>>> - * in them. But for autosuspend, firmware should remain.
>>>>>>>>>> - * Actually, it depends on whether the usb host sends
>>>>>>>>>> + /* For global suspend, Realtek devices lose the loaded fw in
>>>> them
>>>>>> if
>>>>>>>>>> + * platform firmware cut power off. But for autosuspend,
>>>>>> firmware
>>>>>>>>>> + * should remain. Actually, it depends on whether the usb
>> host
>>>>>> sends
>>>>>>>>>> * set feature (enable wakeup) or not.
>>>>>>>>>> */
>>>>>>>>>> if (test_bit(BTUSB_WAKEUP_DISABLE, &data->flags)) {
>>>>>>>>>> if (PMSG_IS_AUTO(message) &&
>>>>>>>>>> device_can_wakeup(&data->udev->dev))
>>>>>>>>>> data->udev->do_remote_wakeup = 1;
>>>>>>>>>> - else if (!PMSG_IS_AUTO(message))
>>>>>>>>>> - data->udev->reset_resume = 1;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> return 0;
>>>>>>>>>> --
>>>>>>>>>> 2.17.1
>>>>>>
>>>>>>
>>>>>> ------Please consider the environment before printing this e-mail.
>
