Re: [PATCH v5 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip
From: Marcel Holtmann <marcel@holtmann.org>
Date: 2019-01-24 09:46:36
Also in:
linux-bluetooth, linux-usb, lkml
Hi Rajat,
quoted hunk ↗ jump to hunk
If the platform provides it, use the reset gpio to reset the Intel BT chip, as part of cmd_timeout handling. This has been found helpful on Intel bluetooth controllers where the firmware gets stuck and the only way out is a hard reset pin provided by the platform. Signed-off-by: Rajat Jain <redacted> --- v5: Rename the hook to cmd_timeout, and wait for 5 timeouts before resetting the device. v4: Use data->flags instead of clearing the quirk in btusb_hw_reset() v3: Better error handling for gpiod_get_optional() v2: Handle the EPROBE_DEFER case. drivers/bluetooth/btusb.c | 54 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 4761499db9ee..8949ea30a1bc 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c@@ -29,6 +29,7 @@#include <linux/of_device.h> #include <linux/of_irq.h> #include <linux/suspend.h> +#include <linux/gpio/consumer.h> #include <asm/unaligned.h> #include <net/bluetooth/bluetooth.h>@@ -439,6 +440,7 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = {#define BTUSB_BOOTING 9 #define BTUSB_DIAG_RUNNING 10 #define BTUSB_OOB_WAKE_ENABLED 11 +#define BTUSB_HW_RESET_ACTIVE 12 struct btusb_data { struct hci_dev *hdev;@@ -476,6 +478,8 @@ struct btusb_data {struct usb_endpoint_descriptor *diag_tx_ep; struct usb_endpoint_descriptor *diag_rx_ep; + struct gpio_desc *reset_gpio; + __u8 cmdreq_type; __u8 cmdreq;@@ -489,8 +493,39 @@ struct btusb_data {int (*setup_on_usb)(struct hci_dev *hdev); int oob_wake_irq; /* irq for out-of-band wake-on-bt */ + unsigned num_cmd_timeout;
Make this cmd_timeout_count or cmd_timeout_cnt.
};
+
+static void btusb_intel_cmd_timeout(struct hci_dev *hdev,
+ struct hci_command_hdr *cmd)
+{
+ struct btusb_data *data = hci_get_drvdata(hdev);
+ struct gpio_desc *reset_gpio = data->reset_gpio;
+
+ bt_dev_err(hdev, "btusb num_cmd_timeout = %u", ++data->num_cmd_timeout);I would prefer if you don’t do the increase in the bt_dev_err(). And you can also scrap the “btusb “ prefix here since that is all present via bt_dev_err if really needed at some point. Actually I question the whole bt_dev_err here in the first place since the core will put our the error. You are just repeating it here.
+ + if (data->num_cmd_timeout < 5) + return;
So I would propose to do just this: if (++data->cmd_timeout_count < 5) return;
quoted hunk ↗ jump to hunk
+ + /* + * Toggle the hard reset line if the platform provides one. The reset + * is going to yank the device off the USB and then replug. So doing + * once is enough. The cleanup is handled correctly on the way out + * (standard USB disconnect), and the new device is detected cleanly + * and bound to the driver again like it should be. + */ + if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, &data->flags)) { + bt_dev_err(hdev, "last reset failed? Not resetting again"); + return; + } + + bt_dev_err(hdev, "Initiating HW reset via gpio"); + gpiod_set_value(reset_gpio, 1); + mdelay(100); + gpiod_set_value(reset_gpio, 0); +} + static inline void btusb_free_frags(struct btusb_data *data) { unsigned long flags;@@ -2915,6 +2950,7 @@ static int btusb_probe(struct usb_interface *intf,const struct usb_device_id *id) { struct usb_endpoint_descriptor *ep_desc; + struct gpio_desc *reset_gpio; struct btusb_data *data; struct hci_dev *hdev; unsigned ifnum_base;@@ -3028,6 +3064,15 @@ static int btusb_probe(struct usb_interface *intf,SET_HCIDEV_DEV(hdev, &intf->dev); + reset_gpio = gpiod_get_optional(&data->udev->dev, "reset", + GPIOD_OUT_LOW);
Any reason to not use the devm_gpiod_get_optional() version here?
quoted hunk ↗ jump to hunk
+ if (IS_ERR(reset_gpio)) { + err = PTR_ERR(reset_gpio); + goto out_free_dev; + } else if (reset_gpio) { + data->reset_gpio = reset_gpio; + } + hdev->open = btusb_open; hdev->close = btusb_close; hdev->flush = btusb_flush;@@ -3085,6 +3130,8 @@ static int btusb_probe(struct usb_interface *intf,set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks); set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks); set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks); + if (data->reset_gpio) + hdev->cmd_timeout = btusb_intel_cmd_timeout; }
Always assign hdev->cmd_timeout = btusb_intel_cmd_timeout and put it after btintel_set_bdaddr and before the quirks. Move the if (data->reset_gpio) check into btusb_intel_cmd_timeout() function and print a warning that no reset GPIO is present.
quoted hunk ↗ jump to hunk
if (id->driver_info & BTUSB_INTEL_NEW) {@@ -3097,6 +3144,8 @@ static int btusb_probe(struct usb_interface *intf,set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks); set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks); set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks); + if (data->reset_gpio) + hdev->cmd_timeout = btusb_intel_cmd_timeout; } if (id->driver_info & BTUSB_MARVELL)@@ -3226,6 +3275,8 @@ static int btusb_probe(struct usb_interface *intf,return 0; out_free_dev: + if (data->reset_gpio) + gpiod_put(data->reset_gpio); hci_free_dev(hdev); return err; }@@ -3269,6 +3320,9 @@ static void btusb_disconnect(struct usb_interface *intf)if (data->oob_wake_irq) device_init_wakeup(&data->udev->dev, false); + if (data->reset_gpio) + gpiod_put(data->reset_gpio); + hci_free_dev(hdev); }
Regards Marcel