diff mbox series

[v1] Bluetooth: btintel_pcie: Device suspend-resume support added

Message ID 20241023114647.1011886-1-chandrashekar.devegowda@intel.com (mailing list archive)
State New
Headers show
Series [v1] Bluetooth: btintel_pcie: Device suspend-resume support added | expand

Checks

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

Commit Message

ChandraShekar Oct. 23, 2024, 11:46 a.m. UTC 
This patch contains the changes in driver to support the suspend and
resume i.e move the controller to D3 state when the platform is entering
into suspend and move the controller to D0 on resume.

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: ChandraShekar <chandrashekar.devegowda@intel.com>
---
 drivers/bluetooth/btintel_pcie.c | 52 ++++++++++++++++++++++++++++++++
 drivers/bluetooth/btintel_pcie.h |  4 +++
 2 files changed, 56 insertions(+)

Comments

Paul Menzel Oct. 23, 2024, 7:18 a.m. UTC | #1 
[Cc: +Bjorn, +linux-pci]

Dear Chandra,


Thank you for the patch.

First something minor: Should there be a space in your name?

ChandraShekar → Chandra Shekar

`git config --global user.name "…"` can configure this for your git setup.

Also for the summary/title, it’d be great if you used a statement by 
using a verb (in imperative mood):

Add device suspend-resume support

or shorter

Support suspend-resume

Am 23.10.24 um 13:46 schrieb ChandraShekar:
> This patch contains the changes in driver to support the suspend and
> resume i.e move the controller to D3 state when the platform is entering
> into suspend and move the controller to D0 on resume.

It’d be great if you elaborated. Please start by the history, since when 
Intel Bluetooth PCIe have been there, and why until now this support was 
missing.

Then please describe, what is needed, and what documentation you used to 
implement the support.

Also, please document, how you tested this, including the log messages, 
and also the time it takes to resume.

Is it also possible to use Bluetooth as a wakeup source from suspend?

> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: ChandraShekar <chandrashekar.devegowda@intel.com>
> ---
>   drivers/bluetooth/btintel_pcie.c | 52 ++++++++++++++++++++++++++++++++
>   drivers/bluetooth/btintel_pcie.h |  4 +++
>   2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index fd4a8bd056fa..f2c44b9d7328 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 |
> @@ -1653,11 +1661,55 @@ 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);
> +	btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D3_HOT);
> +	data->gp0_received = false;
> +	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");

Please include the timeout in the message.

> +		goto fail;
> +	}
> +	return 0;
> +fail:
> +	return -EBUSY;
> +}
> +
> +static int btintel_pcie_resume(struct device *dev)
> +{
> +	struct btintel_pcie_data *data;
> +	struct  pci_dev *pdev = to_pci_dev(dev);
> +	int err;
> +
> +	data = pci_get_drvdata(pdev);
> +	btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D0);
> +	data->gp0_received = false;
> +	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");

Ditto.

> +		goto fail;
> +	}
> +	return 0;
> +fail:
> +	return -EBUSY;
> +}
> +
> +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
Bjorn Helgaas Oct. 23, 2024, 7:13 p.m. UTC | #2 
[+cc Paul]

On Wed, Oct 23, 2024 at 02:46:47PM +0300, ChandraShekar wrote:
> This patch contains the changes in driver to support the suspend and
> resume i.e move the controller to D3 state when the platform is entering
> into suspend and move the controller to D0 on resume.
> 
> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: ChandraShekar <chandrashekar.devegowda@intel.com>
> ---
>  drivers/bluetooth/btintel_pcie.c | 52 ++++++++++++++++++++++++++++++++
>  drivers/bluetooth/btintel_pcie.h |  4 +++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index fd4a8bd056fa..f2c44b9d7328 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 |
> @@ -1653,11 +1661,55 @@ 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);
> +	btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D3_HOT);
> +	data->gp0_received = false;
> +	err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
> +				 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));

The generic power management code already knows how to put standard
PCI devices in D3 at suspend.  So if you have to use device-specific
code like this, I guess the implication is that this device is not
spec-compliant?  That would surprise me a little bit since Intel is
pretty good about making their devices work correctly.

Some detail about exactly how this device is non-compliant would be
helpful here and in the commit log.

It looks wrong to me that you call btintel_pcie_wr_sleep_cntrl()
(which I assume starts something that will result in an interrupt that
causes gp0_received to be set to "true") *before* you set gp0_received
to "false".

That looks like a race because the interrupt could happen before
"data->gp0_received = false", and you would return -EBUSY when you
shouldn't.  You could test this by inserting a delay before setting
"data->gp0_received = false".  Adding a delay should never break this
functionality.

It looks to me like f4c8e876ef6b ("Bluetooth: btintel_pcie: Add
handshake between driver and firmware") (currently in linux-next) has
the same race, where btintel_pcie_send_frame() starts something that
will result in an interrupt, then sets "data->gp0_received = false".

But I don't understand the workings of this hardware or the driver.

> +	if (!err) {
> +		bt_dev_err(data->hdev, "failed to receive gp0 interrupt for suspend");
> +		goto fail;
> +	}
> +	return 0;
> +fail:
> +	return -EBUSY;

Since there's no cleanup, you could just return -EBUSY directly above
and there would be no need for the goto or the "fail" label.

> +}
> +
> +static int btintel_pcie_resume(struct device *dev)
> +{
> +	struct btintel_pcie_data *data;
> +	struct  pci_dev *pdev = to_pci_dev(dev);
> +	int err;
> +
> +	data = pci_get_drvdata(pdev);
> +	btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D0);
> +	data->gp0_received = false;
> +	err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
> +				 msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));

Same potential race here.  And of course I'm still dubious about the
need for this device-specific code in the first place.

> +	if (!err) {
> +		bt_dev_err(data->hdev, "failed to receive gp0 interrupt for resume");
> +		goto fail;
> +	}
> +	return 0;
> +fail:
> +	return -EBUSY;
> +}
> +
> +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 */
> -- 
> 2.34.1
>
Luiz Augusto von Dentz Oct. 23, 2024, 7:19 p.m. UTC | #3 
Hi,

On Wed, Oct 23, 2024 at 3:19 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> [Cc: +Bjorn, +linux-pci]
>
> Dear Chandra,
>
>
> Thank you for the patch.
>
> First something minor: Should there be a space in your name?
>
> ChandraShekar → Chandra Shekar
>
> `git config --global user.name "…"` can configure this for your git setup.
>
> Also for the summary/title, it’d be great if you used a statement by
> using a verb (in imperative mood):
>
> Add device suspend-resume support
>
> or shorter
>
> Support suspend-resume
>
> Am 23.10.24 um 13:46 schrieb ChandraShekar:
> > This patch contains the changes in driver to support the suspend and
> > resume i.e move the controller to D3 state when the platform is entering
> > into suspend and move the controller to D0 on resume.
>
> It’d be great if you elaborated. Please start by the history, since when
> Intel Bluetooth PCIe have been there, and why until now this support was
> missing.
>
> Then please describe, what is needed, and what documentation you used to
> implement the support.
>
> Also, please document, how you tested this, including the log messages,
> and also the time it takes to resume.
>
> Is it also possible to use Bluetooth as a wakeup source from suspend?

This is not a system-suspend though or are you asking if the device
can wakeup itself?

> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > Signed-off-by: ChandraShekar <chandrashekar.devegowda@intel.com>
> > ---
> >   drivers/bluetooth/btintel_pcie.c | 52 ++++++++++++++++++++++++++++++++
> >   drivers/bluetooth/btintel_pcie.h |  4 +++
> >   2 files changed, 56 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> > index fd4a8bd056fa..f2c44b9d7328 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 |
> > @@ -1653,11 +1661,55 @@ 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);
> > +     btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D3_HOT);
> > +     data->gp0_received = false;
> > +     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");
>
> Please include the timeout in the message.
>
> > +             goto fail;
> > +     }
> > +     return 0;
> > +fail:
> > +     return -EBUSY;
> > +}
> > +
> > +static int btintel_pcie_resume(struct device *dev)
> > +{
> > +     struct btintel_pcie_data *data;
> > +     struct  pci_dev *pdev = to_pci_dev(dev);
> > +     int err;
> > +
> > +     data = pci_get_drvdata(pdev);
> > +     btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D0);
> > +     data->gp0_received = false;
> > +     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");
>
> Ditto.
>
> > +             goto fail;
> > +     }
> > +     return 0;
> > +fail:
> > +     return -EBUSY;
> > +}
> > +
> > +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
>
Ilpo Järvinen Oct. 24, 2024, 9:06 a.m. UTC | #4 
On Wed, 23 Oct 2024, Paul Menzel wrote:

> [Cc: +Bjorn, +linux-pci]
> 
> Dear Chandra,
> 
> 
> Thank you for the patch.
> 
> First something minor: Should there be a space in your name?
> 
> ChandraShekar → Chandra Shekar
> 
> `git config --global user.name "…"` can configure this for your git setup.
> 
> Also for the summary/title, it’d be great if you used a statement by using a
> verb (in imperative mood):
> 
> Add device suspend-resume support
> 
> or shorter
> 
> Support suspend-resume
> 
> Am 23.10.24 um 13:46 schrieb ChandraShekar:
> > This patch contains the changes in driver to support the suspend and
> > resume i.e move the controller to D3 state when the platform is entering
> > into suspend and move the controller to D0 on resume.
> 
> It’d be great if you elaborated. Please start by the history, since when Intel
> Bluetooth PCIe have been there, and why until now this support was missing.
> 
> Then please describe, what is needed, and what documentation you used to
> implement the support.
> 
> Also, please document, how you tested this, including the log messages, and
> also the time it takes to resume.
> 
> Is it also possible to use Bluetooth as a wakeup source from suspend?
> 
> > Signed-off-by: Kiran K <kiran.k@intel.com>
> > Signed-off-by: ChandraShekar <chandrashekar.devegowda@intel.com>
> > ---
> >   drivers/bluetooth/btintel_pcie.c | 52 ++++++++++++++++++++++++++++++++
> >   drivers/bluetooth/btintel_pcie.h |  4 +++
> >   2 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/bluetooth/btintel_pcie.c
> > b/drivers/bluetooth/btintel_pcie.c
> > index fd4a8bd056fa..f2c44b9d7328 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 |
> > @@ -1653,11 +1661,55 @@ 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);

Extra space.

> > +
> > +	data = pci_get_drvdata(pdev);
> > +	btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D3_HOT);
> > +	data->gp0_received = false;
> > +	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");
> 
> Please include the timeout in the message.
> 
> > +		goto fail;
> > +	}
> > +	return 0;
> > +fail:
> > +	return -EBUSY;
> > +}
> > +
> > +static int btintel_pcie_resume(struct device *dev)
> > +{
> > +	struct btintel_pcie_data *data;
> > +	struct  pci_dev *pdev = to_pci_dev(dev);

Ditto.

> > +	int err;

Why's the order inconsistent compared with suspend func local vars?

> > +
> > +	data = pci_get_drvdata(pdev);
> > +	btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D0);
> > +	data->gp0_received = false;
> > +	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");
> 
> Ditto.
> 
> > +		goto fail;
> > +	}
> > +	return 0;
> > +fail:
> > +	return -EBUSY;

Quite much common code here compared with the suspend side. Perhaps a 
helper function would be useful?

> > +}
> > +
> > +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))

Extra parenthesis.
diff mbox series

Patch

diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
index fd4a8bd056fa..f2c44b9d7328 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 |
@@ -1653,11 +1661,55 @@  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);
+	btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D3_HOT);
+	data->gp0_received = false;
+	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");
+		goto fail;
+	}
+	return 0;
+fail:
+	return -EBUSY;
+}
+
+static int btintel_pcie_resume(struct device *dev)
+{
+	struct btintel_pcie_data *data;
+	struct  pci_dev *pdev = to_pci_dev(dev);
+	int err;
+
+	data = pci_get_drvdata(pdev);
+	btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D0);
+	data->gp0_received = false;
+	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");
+		goto fail;
+	}
+	return 0;
+fail:
+	return -EBUSY;
+}
+
+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 */