Message ID | 20241108143931.2422947-1-chandrashekar.devegowda@intel.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [v2] Bluetooth: btintel_pcie: Support suspend-resume | expand |
Context | Check | Description |
---|---|---|
tedd_an/pre-ci_am | success | Success |
tedd_an/CheckPatch | success | CheckPatch PASS |
tedd_an/GitLint | success | Gitlint PASS |
tedd_an/SubjectPrefix | success | Gitlint PASS |
tedd_an/BuildKernel | success | BuildKernel PASS |
tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
tedd_an/CheckSparse | success | CheckSparse PASS |
Dear Chandra, Thank you for sending the second iteration. Please also include the previous reviewers in the Cc: list. Am 08.11.24 um 15:39 schrieb ChandraShekar Devegowda: The space in your name is still missing. > 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. > Command to test host initiated wake up after 60seconds Please remove the space in wakeup, and add a space in 60 seconds. > sudo rtcwake -m mem-s 60 Please add space before the switch -s. > 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@intel.com> > Signed-off-by: ChandraShekar Devegowda <chandrashekar.devegowda@intel.com> > --- It’s common to add a change-log between the different versions below the `---` line. > 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
Hi Paul, Thank you for your comments, > -----Original Message----- > From: Paul Menzel <pmenzel@molgen.mpg.de> > Sent: Friday, November 08, 2024 2:49 PM > To: Devegowda, Chandrashekar <chandrashekar.devegowda@intel.com> > Cc: linux-bluetooth@vger.kernel.org; Srivatsa, Ravishankar > <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan > <chethan.tumkur.narayan@intel.com>; K, Kiran <kiran.k@intel.com> > 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@intel.com> > > Signed-off-by: ChandraShekar Devegowda > <chandrashekar.devegowda@intel.com> > > --- > > 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
Dear Chandrashekar, Thank you for your space. Am 11.11.24 um 07:33 schrieb Devegowda, Chandrashekar: >> -----Original Message----- >> Sent: Friday, November 08, 2024 2:49 PM >> To: Devegowda, Chandrashekar <chandrashekar.devegowda@intel.com> […] >> 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. Thank you. In your email you now do *not* use CamelCase, which is more common in Western culture. (Of course you can write your name as you want, and I just pointed it out.) >>> 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. Although it is not public, the name would still be good to have. Intel employees can probably get access more easily, and non-Intel folks could directly approach with the name, and in the future it might be even made public. […] Thank you for acknowledging the other comments. Kind regards Paul
On Fri, Nov 08, 2024 at 10:18:54AM +0100, Paul Menzel wrote: > Am 08.11.24 um 15:39 schrieb ChandraShekar Devegowda: > > 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. Please also include a section reference for this vendor-specific handshake required for transitions between D0 and D3hot. Most devices don't require this kind of code, and its existence is a hint that extra work will be needed to maintain it. Bjorn
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 */