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@xxxxxxx>
---
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; }
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
