Re: [PATCH v2] Bluetooth: btintel_pcie: Support suspend-resume

[Paul Menzel <pmenzel@molgen.mpg.de>]

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