| Message ID | 20181121235020.29461-5-rajatja@google.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v3,1/5] usb: split code locating ACPI companion into port and device | expand |
On Wed, Nov 21, 2018 at 3:50 PM Rajat Jain <rajatja@google.com> wrote: > > If the platform provides it, use the reset gpio to reset the BT > chip (requested by the HCI core if needed). This has been found helpful > on some of Intel bluetooth controllers where the firmware gets stuck and > the only way out is a hard reset pin provided by the platform. Hi Folks, I was wondering if you got a change to look at this patchset, and if there is any feedback. Thanks, Rajat > > Signed-off-by: Rajat Jain <rajatja@google.com> > --- > v3: Better error handling for gpiod_get_optional() > v2: Handle the EPROBE_DEFER case. > > drivers/bluetooth/btusb.c | 40 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index e8e148480c91..e7631f770fae 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> > @@ -475,6 +476,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; > > @@ -490,6 +493,26 @@ struct btusb_data { > int oob_wake_irq; /* irq for out-of-band wake-on-bt */ > }; > > + > +static void btusb_hw_reset(struct hci_dev *hdev) > +{ > + struct btusb_data *data = hci_get_drvdata(hdev); > + struct gpio_desc *reset_gpio = data->reset_gpio; > + > + /* > + * 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. > + */ > + bt_dev_dbg(hdev, "%s: Initiating HW reset via gpio", __func__); > + clear_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks); > + 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; > @@ -2917,6 +2940,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; > @@ -3030,6 +3054,16 @@ 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); > + if (IS_ERR(reset_gpio)) { > + err = PTR_ERR(reset_gpio); > + goto out_free_dev; > + } else if (reset_gpio) { > + data->reset_gpio = reset_gpio; > + hdev->hw_reset = btusb_hw_reset; > + } > + > hdev->open = btusb_open; > hdev->close = btusb_close; > hdev->flush = btusb_flush; > @@ -3085,6 +3119,7 @@ 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); > + set_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks); > > if (id->driver_info & BTUSB_INTEL) { > hdev->setup = btusb_setup_intel; > @@ -3225,6 +3260,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; > } > @@ -3268,6 +3305,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); > } > > -- > 2.19.1.1215.g8438c0b245-goog >
Hi Rajat, > If the platform provides it, use the reset gpio to reset the BT > chip (requested by the HCI core if needed). This has been found helpful > on some of 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 <rajatja@google.com> > --- > v3: Better error handling for gpiod_get_optional() > v2: Handle the EPROBE_DEFER case. > > drivers/bluetooth/btusb.c | 40 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index e8e148480c91..e7631f770fae 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> > @@ -475,6 +476,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; > > @@ -490,6 +493,26 @@ struct btusb_data { > int oob_wake_irq; /* irq for out-of-band wake-on-bt */ > }; > > + > +static void btusb_hw_reset(struct hci_dev *hdev) > +{ > + struct btusb_data *data = hci_get_drvdata(hdev); > + struct gpio_desc *reset_gpio = data->reset_gpio; > + > + /* > + * 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. > + */ > + bt_dev_dbg(hdev, "%s: Initiating HW reset via gpio", __func__); No __func__ here. That bt_dev_dbg does all of that via dynamic debug already. > + clear_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks); > + 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; > @@ -2917,6 +2940,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; > @@ -3030,6 +3054,16 @@ 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); > + if (IS_ERR(reset_gpio)) { > + err = PTR_ERR(reset_gpio); > + goto out_free_dev; > + } else if (reset_gpio) { > + data->reset_gpio = reset_gpio; > + hdev->hw_reset = btusb_hw_reset; > + } > + How do we ensure that this is the right “reset” line. And it also needs to be bound to some hardware unless we can guarantee that this is always the same. > hdev->open = btusb_open; > hdev->close = btusb_close; > hdev->flush = btusb_flush; > @@ -3085,6 +3119,7 @@ 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); > + set_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks); You are not messing with the quirks here please. Clearing quirks is crazy. Use the data->flags since this should be all btusb.c specific. > > if (id->driver_info & BTUSB_INTEL) { > hdev->setup = btusb_setup_intel; > @@ -3225,6 +3260,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; > } > @@ -3268,6 +3305,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
Hi Marcel, Thanks for your review. On Fri, Jan 18, 2019 at 3:04 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Rajat, > > > If the platform provides it, use the reset gpio to reset the BT > > chip (requested by the HCI core if needed). This has been found helpful > > on some of 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 <rajatja@google.com> > > --- > > v3: Better error handling for gpiod_get_optional() > > v2: Handle the EPROBE_DEFER case. > > > > drivers/bluetooth/btusb.c | 40 +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > > index e8e148480c91..e7631f770fae 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> > > @@ -475,6 +476,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; > > > > @@ -490,6 +493,26 @@ struct btusb_data { > > int oob_wake_irq; /* irq for out-of-band wake-on-bt */ > > }; > > > > + > > +static void btusb_hw_reset(struct hci_dev *hdev) > > +{ > > + struct btusb_data *data = hci_get_drvdata(hdev); > > + struct gpio_desc *reset_gpio = data->reset_gpio; > > + > > + /* > > + * 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. > > + */ > > + bt_dev_dbg(hdev, "%s: Initiating HW reset via gpio", __func__); > > No __func__ here. That bt_dev_dbg does all of that via dynamic debug already. Will fix. > > > + clear_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks); > > + 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; > > @@ -2917,6 +2940,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; > > @@ -3030,6 +3054,16 @@ 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); > > + if (IS_ERR(reset_gpio)) { > > + err = PTR_ERR(reset_gpio); > > + goto out_free_dev; > > + } else if (reset_gpio) { > > + data->reset_gpio = reset_gpio; > > + hdev->hw_reset = btusb_hw_reset; > > + } > > + > > How do we ensure that this is the right “reset” line. And it also needs to be bound to some hardware unless > we can guarantee that this is always the same. The BIOS / ACPI ensures that. The kernel driver just uses the ACPI provided reset line. > > > hdev->open = btusb_open; > > hdev->close = btusb_close; > > hdev->flush = btusb_flush; > > @@ -3085,6 +3119,7 @@ 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); > > + set_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks); > > You are not messing with the quirks here please. Clearing quirks is crazy. Use the data->flags since this should be all btusb.c specific. Do I understand it right that you do not want me to clear the quirks in btusb_hw_reset() and use data->flags instead? Sure, I will do that. But I'm not sure if that's all you meant, because you commented on btusb_probe() code where I'm setting the quirk (not clearing it) so that the hci core can call the hw_reset() callback on timeout. Thanks, Rajat > > > > > if (id->driver_info & BTUSB_INTEL) { > > hdev->setup = btusb_setup_intel; > > @@ -3225,6 +3260,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; > > } > > @@ -3268,6 +3305,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 >
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index e8e148480c91..e7631f770fae 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> @@ -475,6 +476,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; @@ -490,6 +493,26 @@ struct btusb_data { int oob_wake_irq; /* irq for out-of-band wake-on-bt */ }; + +static void btusb_hw_reset(struct hci_dev *hdev) +{ + struct btusb_data *data = hci_get_drvdata(hdev); + struct gpio_desc *reset_gpio = data->reset_gpio; + + /* + * 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. + */ + bt_dev_dbg(hdev, "%s: Initiating HW reset via gpio", __func__); + clear_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks); + 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; @@ -2917,6 +2940,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; @@ -3030,6 +3054,16 @@ 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); + if (IS_ERR(reset_gpio)) { + err = PTR_ERR(reset_gpio); + goto out_free_dev; + } else if (reset_gpio) { + data->reset_gpio = reset_gpio; + hdev->hw_reset = btusb_hw_reset; + } + hdev->open = btusb_open; hdev->close = btusb_close; hdev->flush = btusb_flush; @@ -3085,6 +3119,7 @@ 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); + set_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks); if (id->driver_info & BTUSB_INTEL) { hdev->setup = btusb_setup_intel; @@ -3225,6 +3260,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; } @@ -3268,6 +3305,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); }
If the platform provides it, use the reset gpio to reset the BT chip (requested by the HCI core if needed). This has been found helpful on some of 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 <rajatja@google.com> --- v3: Better error handling for gpiod_get_optional() v2: Handle the EPROBE_DEFER case. drivers/bluetooth/btusb.c | 40 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)