Hi Luiz,
>-----Original Message-----
>From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
>Sent: Friday, July 25, 2025 7:24 PM
>To: K, Kiran <kiran.k@xxxxxxxxx>
>Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Srivatsa, Ravishankar
><ravishankar.srivatsa@xxxxxxxxx>; Tumkur Narayan, Chethan
><chethan.tumkur.narayan@xxxxxxxxx>; bhelgaas@xxxxxxxxxx; linux-
>pci@xxxxxxxxxxxxxxx; Devegowda, Chandrashekar
><chandrashekar.devegowda@xxxxxxxxx>
>Subject: Re: [PATCH v6] Bluetooth: btintel_pcie: Add support for _suspend() /
>_resume()
>
>Hi Kiran,
>
>On Fri, Jul 25, 2025 at 4:45 AM Kiran K <kiran.k@xxxxxxxxx> wrote:
>>
>> From: Chandrashekar Devegowda <chandrashekar.devegowda@xxxxxxxxx>
>>
>> This patch implements _suspend() and _resume() functions for the
>> Bluetooth controller. When the system enters a suspended state, the
>> driver notifies the controller to perform necessary housekeeping tasks
>> by writing to the sleep control register and waits for an alive
>> interrupt. The firmware raises the alive interrupt when it has
>> transitioned to the D3 state. The same flow occurs when the system
>> resumes.
>>
>> Command to test host initiated wakeup after 60 seconds sudo rtcwake -m
>> mem -s 60
>>
>> dmesg log (tested on Whale Peak2 on Panther Lake platform) On system
>> suspend:
>> [Fri Jul 25 11:05:37 2025] Bluetooth: hci0: device entered into d3
>> state from d0 in 80 us
>>
>> On system resume:
>> [Fri Jul 25 11:06:36 2025] Bluetooth: hci0: device entered into d0
>> state from d3 in 7117 us
>>
>> Signed-off-by: Chandrashekar Devegowda
>> <chandrashekar.devegowda@xxxxxxxxx>
>> Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
>> ---
>> changes in v6:
>> - s/delta/delta_us/g
>> - s/CONFIG_PM/CONFIG_PM_SLEEP/g
>> - use pm_sleep_pr()/pm_str() to avoid #ifdefs
>> - remove the code to set persistance mode as its not relevant to
>> this patch
>>
>> changes in v5:
>> - refactor _suspend() / _resume() to set the D3HOT/D3COLD based on
>power
>> event
>> - remove SIMPLE_DEV_PM_OPS and define the required pm_ops callback
>> functions
>>
>> changes in v4:
>> - Moved document and section details from the commit message as
>comment in code.
>>
>> changes in v3:
>> - Corrected the typo's
>> - Updated the CC list as suggested.
>> - Corrected the format specifiers in the logs.
>>
>> changes in v2:
>> - Updated the commit message with test steps and logs.
>> - Added logs to include the timeout message.
>> - Fixed a potential race condition during suspend and resume.
>>
>> drivers/bluetooth/btintel_pcie.c | 90
>> ++++++++++++++++++++++++++++++++
>> 1 file changed, 90 insertions(+)
>>
>> diff --git a/drivers/bluetooth/btintel_pcie.c
>> b/drivers/bluetooth/btintel_pcie.c
>> index 6e7bbbd35279..c419521493fe 100644
>> --- a/drivers/bluetooth/btintel_pcie.c
>> +++ b/drivers/bluetooth/btintel_pcie.c
>> @@ -2573,11 +2573,101 @@ static void btintel_pcie_coredump(struct
>> device *dev) } #endif
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int btintel_pcie_suspend_late(struct device *dev, pm_message_t
>> +mesg) {
>> + struct pci_dev *pdev = to_pci_dev(dev);
>> + struct btintel_pcie_data *data;
>> + ktime_t start;
>> + u32 dxstate;
>> + s64 delta_us;
>> + int err;
>> +
>> + data = pci_get_drvdata(pdev);
>> +
>> + dxstate = (mesg.event == PM_EVENT_SUSPEND ?
>> + BTINTEL_PCIE_STATE_D3_HOT :
>> + BTINTEL_PCIE_STATE_D3_COLD);
>> +
>> + data->gp0_received = false;
>> +
>> + start = ktime_get();
>> +
>> + /* Refer: 6.4.11.7 -> Platform power management */
>> + btintel_pcie_wr_sleep_cntrl(data, dxstate);
>> + err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
>> + msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>> + if (err == 0) {
>> + bt_dev_err(data->hdev, "Timeout (%u ms) on alive interrupt for D3
>entry",
>> + BTINTEL_DEFAULT_INTR_TIMEOUT_MS);
>> + return -EBUSY;
>> + }
>> +
>> + delta_us = ktime_to_ns(ktime_get() - start) / 1000;
>> + bt_dev_info(data->hdev, "device entered into d3 state from d0 in %lld
>us",
>> + delta_us);
>> + return 0;
>> +}
>> +
>> +static int btintel_pcie_suspend(struct device *dev) {
>> + return btintel_pcie_suspend_late(dev, PMSG_SUSPEND); }
>> +
>> +static int btintel_pcie_hibernate(struct device *dev) {
>> + return btintel_pcie_suspend_late(dev, PMSG_HIBERNATE); }
>> +
>> +static int btintel_pcie_freeze(struct device *dev) {
>> + return btintel_pcie_suspend_late(dev, PMSG_FREEZE); }
>> +
>> +static int btintel_pcie_resume(struct device *dev) {
>> + struct pci_dev *pdev = to_pci_dev(dev);
>> + struct btintel_pcie_data *data;
>> + ktime_t start;
>> + int err;
>> + s64 delta_us;
>> +
>> + data = pci_get_drvdata(pdev);
>> + data->gp0_received = false;
>> +
>> + start = ktime_get();
>> +
>> + /* Refer: 6.4.11.7 -> Platform power management */
>> + btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D0);
>> + err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
>> + msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>> + if (err == 0) {
>> + bt_dev_err(data->hdev, "Timeout (%u ms) on alive interrupt for D0
>entry",
>> + BTINTEL_DEFAULT_INTR_TIMEOUT_MS);
>> + return -EBUSY;
>> + }
>> +
>> + delta_us = ktime_to_ns(ktime_get() - start) / 1000;
>> + bt_dev_info(data->hdev, "device entered into d0 state from d3 in %lld
>us",
>> + delta_us);
>> + return 0;
>> +}
>> +
>> +const struct dev_pm_ops btintel_pcie_pm_ops = {
>> + .suspend = pm_sleep_ptr(btintel_pcie_suspend),
>> + .resume = pm_sleep_ptr(btintel_pcie_resume),
>> + .freeze = pm_sleep_ptr(btintel_pcie_freeze),
>> + .thaw = pm_sleep_ptr(btintel_pcie_resume),
>> + .poweroff = pm_sleep_ptr(btintel_pcie_hibernate),
>> + .restore = pm_sleep_ptr(btintel_pcie_resume),
>> +};
>> +#endif
>> +
>> static struct pci_driver btintel_pcie_driver = {
>> .name = KBUILD_MODNAME,
>> .id_table = btintel_pcie_table,
>> .probe = btintel_pcie_probe,
>> .remove = btintel_pcie_remove,
>> + .driver.pm = pm_ptr(&btintel_pcie_pm_ops),
>
>This doesn't seem quite right, btintel_pcie_pm_ops is behind
>CONFIG_PM_SLEEP not just CONFIG_PM, so it would be undefined if just
>CONFIG_PM is set, so we might as well do:
>
Agree. I will remove the usage of CONFIG_PM_SLEEP and instead rely on pm_sleep_ptr() and pm_ptr().
>diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
>index 5b32f5a6b0b0..2f1b1be94080 100644
>--- a/drivers/bluetooth/btintel_pcie.c
>+++ b/drivers/bluetooth/btintel_pcie.c
>@@ -2654,12 +2654,12 @@ static int btintel_pcie_resume(struct device *dev)
>}
>
> const struct dev_pm_ops btintel_pcie_pm_ops = {
>- .suspend = pm_sleep_ptr(btintel_pcie_suspend),
>- .resume = pm_sleep_ptr(btintel_pcie_resume),
>- .freeze = pm_sleep_ptr(btintel_pcie_freeze),
>- .thaw = pm_sleep_ptr(btintel_pcie_resume),
>- .poweroff = pm_sleep_ptr(btintel_pcie_hibernate),
>- .restore = pm_sleep_ptr(btintel_pcie_resume),
>+ .suspend = btintel_pcie_suspend,
>+ .resume = btintel_pcie_resume,
>+ .freeze = btintel_pcie_freeze,
>+ .thaw = btintel_pcie_resume,
>+ .poweroff = btintel_pcie_hibernate,
>+ .restore = btintel_pcie_resume,
> };
> #endif
>
>@@ -2668,7 +2668,7 @@ static struct pci_driver btintel_pcie_driver = {
> .id_table = btintel_pcie_table,
> .probe = btintel_pcie_probe,
> .remove = btintel_pcie_remove,
>- .driver.pm = pm_ptr(&btintel_pcie_pm_ops),
>+ .driver.pm = pm_sleep_ptr(&btintel_pcie_pm_ops),
> #ifdef CONFIG_DEV_COREDUMP
> .driver.coredump = btintel_pcie_coredump #endif
>
>
>> #ifdef CONFIG_DEV_COREDUMP
>> .driver.coredump = btintel_pcie_coredump #endif
>> --
>> 2.43.0
>>
>>
>
>
>--
>Luiz Augusto von Dentz
Thanks,
Kiran
