Hi Paul,
Appreciate your comments.
>-----Original Message-----
>From: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
>Sent: Thursday, July 24, 2025 11:48 AM
>To: K, Kiran <kiran.k@xxxxxxxxx>; Devegowda, Chandrashekar
><chandrashekar.devegowda@xxxxxxxxx>
>Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Srivatsa, Ravishankar
><ravishankar.srivatsa@xxxxxxxxx>; Tumkur Narayan, Chethan
><chethan.tumkur.narayan@xxxxxxxxx>; bhelgaas@xxxxxxxxxx; linux-
>pci@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH v5] Bluetooth: btintel_pcie: Add support for _suspend() /
>_resume()
>
>Dear Kiran, dear Chandrashekar,
>
>
>Thank you for your patch.
>
>Am 23.07.25 um 15:57 schrieb Kiran K:
>> 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:
>> [ 516.418316] Bluetooth: hci0: device entered into d3 state from d0
>> in 81 us
>>
>> On system resume:
>> [ 542.174128] Bluetooth: hci0: device entered into d0 state from d3
>> in 357 us
>
>Just to avoid confusion, is the timestamp correct, as this is only a 26 s
>difference and your command says 60 s. (I am only aware of inaccurate
>timestamps using human-readable format (`dmesg -T`).)
The system was woken up by key press before the sleep timeout. To avoid the confusion, I will update the traces to reflect the system waking up independently.
>
>> Signed-off-by: Chandrashekar Devegowda
>> <chandrashekar.devegowda@xxxxxxxxx>
>> Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
>> ---
>> changes in v5:
>> - Address review comments
>
>This should be more detailed, if code was changed.
Ack.
>
>> 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 | 102 +++++++++++++++++++++++++++++++
>> drivers/bluetooth/btintel_pcie.h | 4 ++
>> 2 files changed, 106 insertions(+)
>>
>> diff --git a/drivers/bluetooth/btintel_pcie.c
>> b/drivers/bluetooth/btintel_pcie.c
>> index 6e7bbbd35279..a96975a55cbe 100644
>> --- a/drivers/bluetooth/btintel_pcie.c
>> +++ b/drivers/bluetooth/btintel_pcie.c
>> @@ -540,6 +540,12 @@ static int btintel_pcie_reset_bt(struct
>btintel_pcie_data *data)
>> return reg == 0 ? 0 : -ENODEV;
>> }
>>
>> +static void btintel_pcie_set_persistence_mode(struct
>> +btintel_pcie_data *data) {
>> + btintel_pcie_set_reg_bits(data,
>BTINTEL_PCIE_CSR_HW_BOOT_CONFIG,
>> +
>BTINTEL_PCIE_CSR_HW_BOOT_CONFIG_KEEP_ON);
>> +}
>> +
>> static void btintel_pcie_mac_init(struct btintel_pcie_data *data)
>> {
>> u32 reg;
>> @@ -829,6 +835,8 @@ static int btintel_pcie_enable_bt(struct
>btintel_pcie_data *data)
>> */
>> data->boot_stage_cache = 0x0;
>>
>> + btintel_pcie_set_persistence_mode(data);
>> +
>
>This hunk is not described in the commit message.
This register write is intended to retain firmware state across a warm reboot (S5) and is not relevant to this patch. Apologies for overlooking this detail.
>
>> /* Set MAC_INIT bit to start primary bootloader */
>> reg = btintel_pcie_rd_reg32(data,
>BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
>> reg &= ~(BTINTEL_PCIE_CSR_FUNC_CTRL_FUNC_INIT | @@ -2573,11
>> +2581,105 @@ static void btintel_pcie_coredump(struct device *dev)
>> }
>> #endif
>>
>> +#ifdef CONFIG_PM
>> +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;
>
>Append the unit to the name? `delta_us`?
Ack
>
>> + int err;
>> +
>> + data = pci_get_drvdata(pdev);
>> +
>> + if (mesg.event == PM_EVENT_SUSPEND)
>> + dxstate = BTINTEL_PCIE_STATE_D3_HOT;
>> + else
>> + dxstate = BTINTEL_PCIE_STATE_D3_COLD;
>
>I’d use the ternary operator.
Ack.
>
>> +
>> + 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));
>> + delta = ktime_to_ns(ktime_get() - start) / 1000;
>
>Move it below right before `bt_dev_info`?
Ack.
>
>> +
>> + if (err == 0) {
>> + bt_dev_err(data->hdev, "Timeout (%u ms) on alive interrupt
>for D3 entry",
>> + BTINTEL_DEFAULT_INTR_TIMEOUT_MS);
>> + return -EBUSY;
>> + }
>> + bt_dev_info(data->hdev, "device entered into d3 state from d0 in %lld
>us",
>> + delta);
>> + 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;
>> +
>> + 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));
>> + delta = ktime_to_ns(ktime_get() - start) / 1000;
>> +
>> + if (err == 0) {
>> + bt_dev_err(data->hdev, "Timeout (%u ms) on alive interrupt
>for D0 entry",
>> + BTINTEL_DEFAULT_INTR_TIMEOUT_MS);
>> + return -EBUSY;
>> + }
>> + bt_dev_info(data->hdev, "device entered into d0 state from d3 in %lld
>us",
>> + delta);
>> + return 0;
>> +}
>> +
>> +const struct dev_pm_ops btintel_pcie_pm_ops = {
>> + .suspend = btintel_pcie_suspend,
>> + .resume = btintel_pcie_resume,
>> + .freeze = btintel_pcie_freeze,
>> + .thaw = btintel_pcie_resume,
>> + .poweroff = btintel_pcie_hibernate,
>> + .restore = btintel_pcie_resume,
>> +};
>> +#define BTINTELPCIE_PM_OPS (&btintel_pcie_pm_ops)
>> +#else
>> +#define BTINTELPCIE_PM_OPS NULL
>> +#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 = BTINTELPCIE_PM_OPS,
>> #ifdef CONFIG_DEV_COREDUMP
>> .driver.coredump = btintel_pcie_coredump
>> #endif
>> diff --git a/drivers/bluetooth/btintel_pcie.h
>> b/drivers/bluetooth/btintel_pcie.h
>> index 0fa876c5b954..5bc69004b692 100644
>> --- a/drivers/bluetooth/btintel_pcie.h
>> +++ b/drivers/bluetooth/btintel_pcie.h
>> @@ -8,6 +8,7 @@
>>
>> /* Control and Status Register(BTINTEL_PCIE_CSR) */
>> #define BTINTEL_PCIE_CSR_BASE (0x000)
>> +#define BTINTEL_PCIE_CSR_HW_BOOT_CONFIG
> (BTINTEL_PCIE_CSR_BASE + 0x000)
>> #define BTINTEL_PCIE_CSR_FUNC_CTRL_REG
> (BTINTEL_PCIE_CSR_BASE + 0x024)
>> #define BTINTEL_PCIE_CSR_HW_REV_REG
> (BTINTEL_PCIE_CSR_BASE + 0x028)
>> #define BTINTEL_PCIE_CSR_RF_ID_REG
> (BTINTEL_PCIE_CSR_BASE + 0x09C)
>> @@ -55,6 +56,9 @@
>> #define BTINTEL_PCIE_CSR_BOOT_STAGE_ALIVE (BIT(23))
>> #define BTINTEL_PCIE_CSR_BOOT_STAGE_D3_STATE_READY (BIT(24))
>>
>> +/* CSR HW BOOT CONFIG Register */
>> +#define BTINTEL_PCIE_CSR_HW_BOOT_CONFIG_KEEP_ON
> (BIT(31))
>> +
>> /* Registers for MSI-X */
>> #define BTINTEL_PCIE_CSR_MSIX_BASE (0x2000)
>> #define BTINTEL_PCIE_CSR_MSIX_FH_INT_CAUSES
> (BTINTEL_PCIE_CSR_MSIX_BASE + 0x0800)
>
>
>Kind regards,
>
>Paul
Thanks,
Kiran
