[bluetooth-next PATCH] Bluetooth: hci_bcm: Configure sleep mode on RPM suspend/resume

[bluetooth-next PATCH] Bluetooth: hci_bcm: Configure sleep mode on RPM suspend/resume

[Marek Vasut]

The Infineon CYW43439 Bluetooth device enters suspend mode right after
receiving the Set_Sleepmode_Param sleep_mode=1 HCI command, even if the
BT_DEV_WAKE input is HIGH, i.e. device ought to be awake. This triggers
a timeout of any follow up HCI command, in case of regular boot, that is
HCI_OP_RESET command issued from hci_init1_sync() .

Rework the code such that during probe, the device is configured to not
enter sleep mode by issuing Set_Sleepmode_Param sleep_mode=0 instead of
sleep_mode=1 in bcm_setup(). Upon RPM suspend, issue Set_Sleepmode_Param
with sleep_mode=1 to allow the device to enter the sleep mode when the
BT_DEV_WAKE signal is deasserted, which is deasserted soon after in the
RPM suspend callback. Upon RPM resume, assert BT_DEV_WAKE to resume the
chip from sleep mode and then issue Set_Sleepmode_Param sleep_mode=0 to
yet again prevent the device from entering sleep mode until the next RPM
suspend.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: kernel@dh-electronics.com
Cc: linux-bluetooth@vger.kernel.org
---
 drivers/bluetooth/hci_bcm.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 89d4c2224546f..fde5e0136c392 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -389,13 +389,19 @@ static const struct bcm_set_sleep_mode default_sleep_params = {
 	.pulsed_host_wake = 1,
 };
 
-static int bcm_setup_sleep(struct hci_uart *hu)
+static int bcm_setup_sleep(struct hci_uart *hu, bool sync, int mode)
 {
 	struct bcm_data *bcm = hu->priv;
 	struct sk_buff *skb;
 	struct bcm_set_sleep_mode sleep_params = default_sleep_params;
 
 	sleep_params.host_wake_active = !bcm->dev->irq_active_low;
+	sleep_params.sleep_mode = mode;
+
+	if (!sync) {
+		return __hci_cmd_send(hu->hdev, 0xfc27, sizeof(sleep_params),
+				      &sleep_params);
+	}
 
 	skb = __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_params),
 			     &sleep_params, HCI_INIT_TIMEOUT);
@@ -412,7 +418,7 @@ static int bcm_setup_sleep(struct hci_uart *hu)
 }
 #else
 static inline int bcm_request_irq(struct bcm_data *bcm) { return 0; }
-static inline int bcm_setup_sleep(struct hci_uart *hu) { return 0; }
+static inline int bcm_setup_sleep(struct hci_uart *hu, bool sync, int mode) { return 0; }
 #endif
 
 static int bcm_set_diag(struct hci_dev *hdev, bool enable)
@@ -647,7 +653,7 @@ static int bcm_setup(struct hci_uart *hu)
 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hu->hdev->quirks);
 
 	if (!bcm_request_irq(bcm))
-		err = bcm_setup_sleep(hu);
+		err = bcm_setup_sleep(hu, true, 0);
 
 	return err;
 }
@@ -767,6 +773,16 @@ static int bcm_suspend_device(struct device *dev)
 	bt_dev_dbg(bdev, "");
 
 	if (!bdev->is_suspended && bdev->hu) {
+		err = bcm_setup_sleep(bdev->hu, false, 1);
+		/*
+		 * If the sleep mode cannot be enabled, the BT device
+		 * may consume more power, but this should not prevent
+		 * RPM suspend from completion. Warn about this, but
+		 * attempt to suspend anyway.
+		 */
+		if (err)
+			dev_err(dev, "Failed to enable sleep mode\n");
+
 		hci_uart_set_flow_control(bdev->hu, true);
 
 		/* Once this returns, driver suspends BT via GPIO */
@@ -810,6 +826,16 @@ static int bcm_resume_device(struct device *dev)
 		bdev->is_suspended = false;
 
 		hci_uart_set_flow_control(bdev->hu, false);
+
+		err = bcm_setup_sleep(bdev->hu, false, 0);
+		/*
+		 * If the sleep mode cannot be disabled, the BT device
+		 * may fail to respond to commands at times, or may be
+		 * completely unresponsive. Warn user about this, but
+		 * attempt to resume anyway in best effort manner.
+		 */
+		if (err)
+			dev_err(dev, "Failed to disable sleep mode\n");
 	}
 
 	return 0;
-- 
2.43.0

RE: [bluetooth-next] Bluetooth: hci_bcm: Configure sleep mode on RPM suspend/resume

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1775 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=866844

---Test result---

Test Summary:
CheckPatch                    PASS      0.70 seconds
GitLint                       PASS      0.33 seconds
SubjectPrefix                 PASS      0.14 seconds
BuildKernel                   PASS      28.77 seconds
CheckAllWarning               PASS      31.02 seconds
CheckSparse                   PASS      36.35 seconds
CheckSmatch                   PASS      99.34 seconds
BuildKernel32                 PASS      30.39 seconds
TestRunnerSetup               PASS      504.07 seconds
TestRunner_l2cap-tester       PASS      19.65 seconds
TestRunner_iso-tester         FAIL      42.46 seconds
TestRunner_bnep-tester        PASS      4.64 seconds
TestRunner_mgmt-tester        PASS      110.15 seconds
TestRunner_rfcomm-tester      PASS      7.21 seconds
TestRunner_sco-tester         PASS      14.80 seconds
TestRunner_ioctl-tester       PASS      7.61 seconds
TestRunner_mesh-tester        PASS      10.21 seconds
TestRunner_smp-tester         PASS      6.67 seconds
TestRunner_userchan-tester    PASS      4.88 seconds
IncrementalBuild              PASS      26.91 seconds

Details
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
Total: 122, Passed: 116 (95.1%), Failed: 2, Not Run: 4

Failed Test Cases
ISO Connect Suspend - Success                        Failed       6.185 seconds
ISO Connect2 Suspend - Success                       Failed       6.233 seconds


---
Regards,
Linux Bluetooth

Re: [bluetooth-next PATCH] Bluetooth: hci_bcm: Configure sleep mode on RPM suspend/resume

[Marek Vasut]

On 6/29/24 7:22 PM, Marek Vasut wrote:
> The Infineon CYW43439 Bluetooth device enters suspend mode right after
> receiving the Set_Sleepmode_Param sleep_mode=1 HCI command, even if the
> BT_DEV_WAKE input is HIGH, i.e. device ought to be awake. This triggers
> a timeout of any follow up HCI command, in case of regular boot, that is
> HCI_OP_RESET command issued from hci_init1_sync() .
> 
> Rework the code such that during probe, the device is configured to not
> enter sleep mode by issuing Set_Sleepmode_Param sleep_mode=0 instead of
> sleep_mode=1 in bcm_setup(). Upon RPM suspend, issue Set_Sleepmode_Param
> with sleep_mode=1 to allow the device to enter the sleep mode when the
> BT_DEV_WAKE signal is deasserted, which is deasserted soon after in the
> RPM suspend callback. Upon RPM resume, assert BT_DEV_WAKE to resume the
> chip from sleep mode and then issue Set_Sleepmode_Param sleep_mode=0 to
> yet again prevent the device from entering sleep mode until the next RPM
> suspend.
Is there any feedback on this patch ?

Re: [bluetooth-next PATCH] Bluetooth: hci_bcm: Configure sleep mode on RPM suspend/resume

[Hans de Goede]

Hi Marek,

On 29-Jun-24 7:22 PM, Marek Vasut wrote:
> The Infineon CYW43439 Bluetooth device enters suspend mode right after
> receiving the Set_Sleepmode_Param sleep_mode=1 HCI command, even if the
> BT_DEV_WAKE input is HIGH, i.e. device ought to be awake. This triggers
> a timeout of any follow up HCI command, in case of regular boot, that is
> HCI_OP_RESET command issued from hci_init1_sync() .
> 
> Rework the code such that during probe, the device is configured to not
> enter sleep mode by issuing Set_Sleepmode_Param sleep_mode=0 instead of
> sleep_mode=1 in bcm_setup(). Upon RPM suspend, issue Set_Sleepmode_Param
> with sleep_mode=1 to allow the device to enter the sleep mode when the
> BT_DEV_WAKE signal is deasserted, which is deasserted soon after in the
> RPM suspend callback. Upon RPM resume, assert BT_DEV_WAKE to resume the
> chip from sleep mode and then issue Set_Sleepmode_Param sleep_mode=0 to
> yet again prevent the device from entering sleep mode until the next RPM
> suspend.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Thank you for your patch. The patch looks good to me and I've been
running this in my personal tree / test-kernels which includes
various devices with BCM BT serdevs without issues:

Reviewed-by: Hans de Goede <hansg@kernel.org>
Tested-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans



> ---
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: kernel@dh-electronics.com
> Cc: linux-bluetooth@vger.kernel.org
> ---
>  drivers/bluetooth/hci_bcm.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 89d4c2224546f..fde5e0136c392 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -389,13 +389,19 @@ static const struct bcm_set_sleep_mode default_sleep_params = {
>  	.pulsed_host_wake = 1,
>  };
>  
> -static int bcm_setup_sleep(struct hci_uart *hu)
> +static int bcm_setup_sleep(struct hci_uart *hu, bool sync, int mode)
>  {
>  	struct bcm_data *bcm = hu->priv;
>  	struct sk_buff *skb;
>  	struct bcm_set_sleep_mode sleep_params = default_sleep_params;
>  
>  	sleep_params.host_wake_active = !bcm->dev->irq_active_low;
> +	sleep_params.sleep_mode = mode;
> +
> +	if (!sync) {
> +		return __hci_cmd_send(hu->hdev, 0xfc27, sizeof(sleep_params),
> +				      &sleep_params);
> +	}
>  
>  	skb = __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_params),
>  			     &sleep_params, HCI_INIT_TIMEOUT);
> @@ -412,7 +418,7 @@ static int bcm_setup_sleep(struct hci_uart *hu)
>  }
>  #else
>  static inline int bcm_request_irq(struct bcm_data *bcm) { return 0; }
> -static inline int bcm_setup_sleep(struct hci_uart *hu) { return 0; }
> +static inline int bcm_setup_sleep(struct hci_uart *hu, bool sync, int mode) { return 0; }
>  #endif
>  
>  static int bcm_set_diag(struct hci_dev *hdev, bool enable)
> @@ -647,7 +653,7 @@ static int bcm_setup(struct hci_uart *hu)
>  		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hu->hdev->quirks);
>  
>  	if (!bcm_request_irq(bcm))
> -		err = bcm_setup_sleep(hu);
> +		err = bcm_setup_sleep(hu, true, 0);
>  
>  	return err;
>  }
> @@ -767,6 +773,16 @@ static int bcm_suspend_device(struct device *dev)
>  	bt_dev_dbg(bdev, "");
>  
>  	if (!bdev->is_suspended && bdev->hu) {
> +		err = bcm_setup_sleep(bdev->hu, false, 1);
> +		/*
> +		 * If the sleep mode cannot be enabled, the BT device
> +		 * may consume more power, but this should not prevent
> +		 * RPM suspend from completion. Warn about this, but
> +		 * attempt to suspend anyway.
> +		 */
> +		if (err)
> +			dev_err(dev, "Failed to enable sleep mode\n");
> +
>  		hci_uart_set_flow_control(bdev->hu, true);
>  
>  		/* Once this returns, driver suspends BT via GPIO */
> @@ -810,6 +826,16 @@ static int bcm_resume_device(struct device *dev)
>  		bdev->is_suspended = false;
>  
>  		hci_uart_set_flow_control(bdev->hu, false);
> +
> +		err = bcm_setup_sleep(bdev->hu, false, 0);
> +		/*
> +		 * If the sleep mode cannot be disabled, the BT device
> +		 * may fail to respond to commands at times, or may be
> +		 * completely unresponsive. Warn user about this, but
> +		 * attempt to resume anyway in best effort manner.
> +		 */
> +		if (err)
> +			dev_err(dev, "Failed to disable sleep mode\n");
>  	}
>  
>  	return 0;

Re: [bluetooth-next PATCH] Bluetooth: hci_bcm: Configure sleep mode on RPM suspend/resume

[Paul Menzel]

Dear Marek,


Thank you for your patch. I was made aware of it by Hans’ recent review. 
Some minor nits. Probably, you need to resend the patch anyway.

Am 29.06.24 um 19:22 schrieb Marek Vasut:
> The Infineon CYW43439 Bluetooth device enters suspend mode right after
> receiving the Set_Sleepmode_Param sleep_mode=1 HCI command, even if the
> BT_DEV_WAKE input is HIGH, i.e. device ought to be awake. This triggers
> a timeout of any follow up HCI command, in case of regular boot, that is

follow-up

> HCI_OP_RESET command issued from hci_init1_sync() .
> 
> Rework the code such that during probe, the device is configured to not
> enter sleep mode by issuing Set_Sleepmode_Param sleep_mode=0 instead of
> sleep_mode=1 in bcm_setup(). Upon RPM suspend, issue Set_Sleepmode_Param
> with sleep_mode=1 to allow the device to enter the sleep mode when the
> BT_DEV_WAKE signal is deasserted, which is deasserted soon after in the
> RPM suspend callback. Upon RPM resume, assert BT_DEV_WAKE to resume the
> chip from sleep mode and then issue Set_Sleepmode_Param sleep_mode=0 to
> yet again prevent the device from entering sleep mode until the next RPM
> suspend.

How did you test this? It’d be great if you gave more details.

> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: kernel@dh-electronics.com
> Cc: linux-bluetooth@vger.kernel.org
> ---
>   drivers/bluetooth/hci_bcm.c | 32 +++++++++++++++++++++++++++++---
>   1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 89d4c2224546f..fde5e0136c392 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -389,13 +389,19 @@ static const struct bcm_set_sleep_mode default_sleep_params = {
>   	.pulsed_host_wake = 1,
>   };
>   
> -static int bcm_setup_sleep(struct hci_uart *hu)
> +static int bcm_setup_sleep(struct hci_uart *hu, bool sync, int mode)
>   {
>   	struct bcm_data *bcm = hu->priv;
>   	struct sk_buff *skb;
>   	struct bcm_set_sleep_mode sleep_params = default_sleep_params;
>   
>   	sleep_params.host_wake_active = !bcm->dev->irq_active_low;
> +	sleep_params.sleep_mode = mode;
> +
> +	if (!sync) {
> +		return __hci_cmd_send(hu->hdev, 0xfc27, sizeof(sleep_params),
> +				      &sleep_params);
> +	}
>   
>   	skb = __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_params),
>   			     &sleep_params, HCI_INIT_TIMEOUT);
> @@ -412,7 +418,7 @@ static int bcm_setup_sleep(struct hci_uart *hu)
>   }
>   #else
>   static inline int bcm_request_irq(struct bcm_data *bcm) { return 0; }
> -static inline int bcm_setup_sleep(struct hci_uart *hu) { return 0; }
> +static inline int bcm_setup_sleep(struct hci_uart *hu, bool sync, int mode) { return 0; }

Without an IDE, I always wonder, if an enum or macro should be used for 
modes, so readers do not need to look up what 0 and 1 mean.

>   #endif
>   
>   static int bcm_set_diag(struct hci_dev *hdev, bool enable)
> @@ -647,7 +653,7 @@ static int bcm_setup(struct hci_uart *hu)
>   		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hu->hdev->quirks);
>   
>   	if (!bcm_request_irq(bcm))
> -		err = bcm_setup_sleep(hu);
> +		err = bcm_setup_sleep(hu, true, 0);
>   
>   	return err;
>   }
> @@ -767,6 +773,16 @@ static int bcm_suspend_device(struct device *dev)
>   	bt_dev_dbg(bdev, "");
>   
>   	if (!bdev->is_suspended && bdev->hu) {
> +		err = bcm_setup_sleep(bdev->hu, false, 1);
> +		/*
> +		 * If the sleep mode cannot be enabled, the BT device
> +		 * may consume more power, but this should not prevent
> +		 * RPM suspend from completion. Warn about this, but
> +		 * attempt to suspend anyway.
> +		 */
> +		if (err)
> +			dev_err(dev, "Failed to enable sleep mode\n");

Please print out the returned error, and maybe give suggestion, what the 
user should do in this case.

> +
>   		hci_uart_set_flow_control(bdev->hu, true);
>   
>   		/* Once this returns, driver suspends BT via GPIO */
> @@ -810,6 +826,16 @@ static int bcm_resume_device(struct device *dev)
>   		bdev->is_suspended = false;
>   
>   		hci_uart_set_flow_control(bdev->hu, false);
> +
> +		err = bcm_setup_sleep(bdev->hu, false, 0);
> +		/*
> +		 * If the sleep mode cannot be disabled, the BT device
> +		 * may fail to respond to commands at times, or may be
> +		 * completely unresponsive. Warn user about this, but
> +		 * attempt to resume anyway in best effort manner.
> +		 */
> +		if (err)
> +			dev_err(dev, "Failed to disable sleep mode\n");

Ditto.

>   	}
>   
>   	return 0;

Overall this looks good.


Kind regards,

Paul