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

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

[Paul Menzel]

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

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

[ChandraShekar Devegowda]

This patch contains the changes in driver for vendor specific handshake
during enter of D3 and D0 exit.
Command to test host initiated wake up after 60seconds
    sudo rtcwake -m mem-s 60
logs from testing:
    Bluetooth: hci0: BT device resumed to D0 in 1016 usecs

Signed-off-by: Kiran K <kiran.k@intel.com>
Signed-off-by: ChandraShekar Devegowda <chandrashekar.devegowda@intel.com>
---
 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 */
-- 
2.34.1

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

[Devegowda, Chandrashekar]

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

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

[Paul Menzel]

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

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

[Bjorn Helgaas]

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

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

[Devegowda, Chandrashekar]

Hi Paul,
    Thank you for the comments

> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Monday, November 11, 2024 12:51 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>; Bjorn
> Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org
> Subject: Re: [PATCH v2] Bluetooth: btintel_pcie: Support suspend-resume
> 
> 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.
> 

Ack, I have added the Intel specific SAS document name and the relevant sections in the commit message.

> [...]
> 
> Thank you for acknowledging the other comments.
> 
> 
> Kind regards
> 
> Paul

Regards,
Chandru

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

[Devegowda, Chandrashekar]

Hi Bjorn,
    Thanks for your comments

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Tuesday, November 12, 2024 12:11 AM
> To: Paul Menzel <pmenzel@molgen.mpg.de>
> Cc: Devegowda, Chandrashekar <chandrashekar.devegowda@intel.com>; 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
> 
> 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.
> 

Ack, I have added the Intel specific SAS document name and the sections which details the flow in the commit message.

> 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

Regards,
Chandru