Hi Paul,
Thank you for your comments,
> -----Original Message-----
> From: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
> Sent: Friday, November 08, 2024 2:49 PM
> To: Devegowda, Chandrashekar <chandrashekar.devegowda@xxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Srivatsa, Ravishankar
> <ravishankar.srivatsa@xxxxxxxxx>; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@xxxxxxxxx>; K, Kiran <kiran.k@xxxxxxxxx>
> Subject: Re: [PATCH v2] Bluetooth: btintel_pcie: Support suspend-resume
>
> Dear Chandra,
>
>
> Thank you for sending the second iteration. Please also include the previous
> reviewers in the Cc: list.
>
Ack have added them back in the CC list and also will add them for the next version of patches.
>
> Am 08.11.24 um 15:39 schrieb ChandraShekar Devegowda:
>
> The space in your name is still missing.
>
I have added my second name , my first name doesn’t have a space so please consider ChandraShekar as a single name.
> > This patch contains the changes in driver for vendor specific handshake
> > during enter of D3 and D0 exit.
>
> Please document the datasheet name and revision.
>
Datasheet is internal to Intel, sorry wouldn't be able to share at the moment.
> > Command to test host initiated wake up after 60seconds
>
> Please remove the space in wakeup, and add a space in 60 seconds.
>
Ack will change in the next version of the patch.
> > sudo rtcwake -m mem-s 60
>
> Please add space before the switch -s.
>
Ack will change in the next version of the patch.
> > logs from testing:
> > Bluetooth: hci0: BT device resumed to D0 in 1016 usecs
>
> Thank you for providing the logs.
>
> > Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
> > Signed-off-by: ChandraShekar Devegowda
> <chandrashekar.devegowda@xxxxxxxxx>
> > ---
>
> It’s common to add a change-log between the different versions below the
> `---` line.
>
Ack will add the change-log in the next patch.
> > drivers/bluetooth/btintel_pcie.c | 58
> ++++++++++++++++++++++++++++++++
> > drivers/bluetooth/btintel_pcie.h | 4 +++
> > 2 files changed, 62 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btintel_pcie.c
> b/drivers/bluetooth/btintel_pcie.c
> > index 2b79952f3628..49b78d3ecdf9 100644
> > --- a/drivers/bluetooth/btintel_pcie.c
> > +++ b/drivers/bluetooth/btintel_pcie.c
> > @@ -273,6 +273,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);
> > +}
> > +
> > /* This function enables BT function by setting
> BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_INIT bit in
> > * BTINTEL_PCIE_CSR_FUNC_CTRL_REG register and wait for MSI-X with
> > * BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0.
> > @@ -297,6 +303,8 @@ static int btintel_pcie_enable_bt(struct
> btintel_pcie_data *data)
> > */
> > data->boot_stage_cache = 0x0;
> >
> > + btintel_pcie_set_persistence_mode(data);
> > +
> > /* 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 |
> > @@ -1647,11 +1655,61 @@ static void btintel_pcie_remove(struct pci_dev
> *pdev)
> > pci_set_drvdata(pdev, NULL);
> > }
> >
> > +static int btintel_pcie_suspend(struct device *dev)
> > +{
> > + struct btintel_pcie_data *data;
> > + int err;
> > + struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > + data = pci_get_drvdata(pdev);
> > + data->gp0_received = false;
> > + btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D3_HOT);
> > + err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
> > +
> msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
> > + if (!err) {
> > + bt_dev_err(data->hdev, "failed to receive gp0 interrupt for
> suspend in %lu msecs",
> > + BTINTEL_DEFAULT_INTR_TIMEOUT_MS);
> > + return -EBUSY;
> > + }
> > + return 0;
> > +}
> > +
> > +static int btintel_pcie_resume(struct device *dev)
> > +{
> > + struct btintel_pcie_data *data;
> > + struct pci_dev *pdev = to_pci_dev(dev);
> > + ktime_t calltime, delta, rettime;
> > + unsigned long long duration;
> > + int err;
> > +
> > + data = pci_get_drvdata(pdev);
> > + data->gp0_received = false;
> > + calltime = ktime_get();
> > + 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) {
> > + bt_dev_err(data->hdev, "failed to receive gp0 interrupt for
> resume in %lu msecs",
> > + BTINTEL_DEFAULT_INTR_TIMEOUT_MS);
> > + return -EBUSY;
> > + }
> > + rettime = ktime_get();
> > + delta = ktime_sub(rettime, calltime);
> > + duration = (unsigned long long)ktime_to_ns(delta) >> 10;
> > + bt_dev_info(data->hdev, "BT device resumed to D0 in %llu usecs",
> duration);
> > +
> > + return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(btintel_pcie_pm_ops, btintel_pcie_suspend,
> > + btintel_pcie_resume);
> > +
> > 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 = &btintel_pcie_pm_ops,
> > };
> > module_pci_driver(btintel_pcie_driver);
> >
> > diff --git a/drivers/bluetooth/btintel_pcie.h
> b/drivers/bluetooth/btintel_pcie.h
> > index f9aada0543c4..38d0c8ea2b6f 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)
> > @@ -48,6 +49,9 @@
> > #define BTINTEL_PCIE_CSR_MSIX_IVAR_BASE
> (BTINTEL_PCIE_CSR_MSIX_BASE + 0x0880)
> > #define BTINTEL_PCIE_CSR_MSIX_IVAR(cause)
> (BTINTEL_PCIE_CSR_MSIX_IVAR_BASE + (cause))
> >
> > +/* CSR HW BOOT CONFIG Register */
> > +#define BTINTEL_PCIE_CSR_HW_BOOT_CONFIG_KEEP_ON
> (BIT(31))
> > +
> > /* Causes for the FH register interrupts */
> > enum msix_fh_int_causes {
> > BTINTEL_PCIE_MSIX_FH_INT_CAUSES_0 = BIT(0), /*
> cause 0 */
>
>
> Kind regards,
>
> Paul
