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

[Hans de Goede <hdegoede@redhat.com>]

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;