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@xxxxxxx>
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@xxxxxxxxxx>
Tested-by: Hans de Goede <hansg@xxxxxxxxxx>
Regards,
Hans
> ---
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Cc: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Cc: Marcel Holtmann <marcel@xxxxxxxxxxxx>
> Cc: kernel@xxxxxxxxxxxxxxxxxx
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx
> ---
> 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;
